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

From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
To: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: "Kandpal, Suraj" <suraj.kandpal@intel.com>,
	"carsten.haitzler@arm.com" <carsten.haitzler@arm.com>,
	"Nikula, Jani" <jani.nikula@intel.com>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	Abhinav Kumar <quic_abhinavk@quicinc.com>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	"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: Mon, 28 Feb 2022 11:07:41 +0300	[thread overview]
Message-ID: <CAA8EJpqkW2FO_G-NTodn9jrAoDtjB+GfvLoZV97Ci9WNsJhAKA@mail.gmail.com> (raw)
In-Reply-To: <YhyBEnI/P5KezATw@pendragon.ideasonboard.com>

On Mon, 28 Feb 2022 at 11:00, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Suraj,
>
> On Sat, Feb 26, 2022 at 05:10:06AM +0000, Kandpal, Suraj wrote:
> > 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.
>
> This is a direct consequence of the decision to use connectors for
> writeback in the userspace API. This disrupts any code that assumes that
> a connector is a connector. The problem isn't limited to kernelspace,
> userspace has the same exact problem, which resulted in a hack to avoid
> breaking everything. Userspace software that needs to deal with
> writeback needs to set the DRM_CLIENT_CAP_WRITEBACK_CONNECTORS
> capability to get the writeback connectors exposed by the kernel, but
> more than that, a large refactoring is then often needed to chase all
> code paths that assume a connector is a connector.
>
> I'm afraid the same applies to the kernel. Drivers that don't use
> writeback are largely unaffected. Drievrs that want to use writeback
> need to be refactored to properly handle the fact that writeback
> connectors are special. i915 will need to go that way.

Laurent, you have frown upon the idea of separating the connector from
the drm_writeback_connector struct. What about the encoder?
The msm code in question can be found at the patchwork:
https://patchwork.freedesktop.org/series/99724/. This series depends
on Intel's patch, but should give you the overall feeling of the code
being shared between to-the-display and writeback pipelines.

-- 
With best wishes
Dmitry