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

From: "Kandpal, Suraj" <suraj.kandpal@intel.com>
To: Abhinav Kumar <quic_abhinavk@quicinc.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: Sat, 26 Feb 2022 05:10:06 +0000	[thread overview]
Message-ID: <MWHPR11MB1741E1C4A6247CCCBECC6793E33F9@MWHPR11MB1741.namprd11.prod.outlook.com> (raw)
In-Reply-To: <d0677cbd-64f1-eb13-7aea-3de599134d09@quicinc.com>

Hi Abhinav,

> 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.

The reason we can't do that is i915 driver always assumes that all connectors
present in device list is an intel connector and since drm_writeback_connector
even though a faux connector in it's initialization calls drm_connector_init()
and gets added to the drm device list and hence the i915 driver also expects 
a corresponding intel connector to go with it. In case I do try to make writeback
connector standalone a lot of checks, a lot will have to be added all around the 
driver as there could be a chance that one of the drm_connector in the drm device
list may not be an intel_connector.
Yes right now not all fields of intel_connector are being used but we will be working
on filling them as we add more functionality to writeback for example introducing
content protection. 
So even if I do float the patch series with just drm_encoder as pointer it won't help
us with our implementation if drm_connector isn't a pointer too.

Regards,
Suraj Kandpal