Re: [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

From: Abhinav Kumar <quic_abhinavk@quicinc.com>
To: "Kandpal, Suraj" <suraj.kandpal@intel.com>,
	"Nikula, Jani" <jani.nikula@intel.com>,
	Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Cc: "carsten.haitzler@arm.com" <carsten.haitzler@arm.com>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
	"Murthy,  Arun R" <arun.r.murthy@intel.com>
Subject: Re: [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes
Date: Fri, 25 Feb 2022 15:26:46 -0800	[thread overview]
Message-ID: <d0677cbd-64f1-eb13-7aea-3de599134d09@quicinc.com> (raw)
In-Reply-To: <MWHPR11MB17412030E65D4E4821549E24E33C9@MWHPR11MB1741.namprd11.prod.outlook.com>

Hi Suraj

On 2/22/2022 10:17 PM, Kandpal, Suraj wrote:
> Hey,
> 
>> The connector/encoder funcs you do have to pass to
>> drm_writeback_connector_init() can't use any of the shared driver
>> infrastructure that assume a certain inheritance.
>>
>> See also my reply to Laurent [1].
>>
>>> It well might be that we all misunderstand your design. Do you have a
>>> complete intel drm_writeback implementation based on this patchset? It
>>> would be easier to judge if the approach is correct seeing your
>>> target.
>>
>> That would be up to Suraj Kandpal.
> I have floated the intel drm_writeback implementation.
> Please refer to [1] to get a better understanding of how we are implementing
> writeback functionality.
> 
> [1] https://patchwork.freedesktop.org/series/100617/
> 
> Thanks,
> Suraj Kandpal

Based on the discussion in this thread [1] , it seems like having 
drm_encoder as a pointer seems to have merits for both of us and also in 
agreement with the folks on this thread and has a better chance of an ack.

However drm_connector is not.

I had a brief look at your implementation. Any reason you need to go 
through intel_connector here and not drm_writeback_connector directly?

The reason I ask is that I see you are not using prepare_writeback_job 
hook. If you use that, you can use the drm_writeback_connector passed on 
from there instead of looping through your list like you are doing in 
intel_find_writeback_connector.

Also, none of the other entries of struct intel_connector are being used 
for the writeback implementation. So does the drm_writeback_connector in 
your implementation need to be an intel_connector when its anyway not 
using other fields? Why cant it be just stored as a 
drm_writeback_connector itself in your struct intel_wd.

@@ -539,6 +540,8 @@  struct intel_connector {
  	struct work_struct modeset_retry_work;

  	struct intel_hdcp hdcp;
+
+	struct drm_writeback_connector wb_conn;
  };

[1] 
https://patchwork.kernel.org/project/dri-devel/patch/20220202085429.22261-6-suraj.kandpal@intel.com/#24747889

If you are in agreement with this, do you think you can re-spin the 
series only with the drm_encoder as a pointer without the drm_connector 
part.
