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

From: Jani Nikula <jani.nikula@intel.com>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Kandpal Suraj <suraj.kandpal@intel.com>,
	carsten.haitzler@arm.com, intel-gfx@lists.freedesktop.org,
	quic_abhinavk@quicinc.com, dri-devel@lists.freedesktop.org,
	dmitry.baryshkov@linaro.org, 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: Wed, 02 Feb 2022 17:38:00 +0200	[thread overview]
Message-ID: <87v8xxs2hz.fsf@intel.com> (raw)
In-Reply-To: <YfqGbqQQz5vrDaLI@pendragon.ideasonboard.com>

On Wed, 02 Feb 2022, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> Hi Jani,
>
> On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote:
>> On Wed, 02 Feb 2022, Laurent Pinchart wrote:
>> > On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote:
>> >> Changing rcar_du driver to accomadate the change of
>> >> drm_writeback_connector.base and drm_writeback_connector.encoder
>> >> to a pointer the reason for which is explained in the
>> >> Patch(drm: add writeback pointers to drm_connector).
>> >> 
>> >> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com>
>> >> ---
>> >>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h      | 2 ++
>> >>  drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++---
>> >>  2 files changed, 7 insertions(+), 3 deletions(-)
>> >> 
>> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>> >> index 66e8839db708..68f387a04502 100644
>> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>> >> @@ -72,6 +72,8 @@ struct rcar_du_crtc {
>> >>  	const char *const *sources;
>> >>  	unsigned int sources_count;
>> >>  
>> >> +	struct drm_connector connector;
>> >> +	struct drm_encoder encoder;
>> >
>> > Those fields are, at best, poorly named. Furthermore, there's no need in
>> > this driver or in other drivers using drm_writeback_connector to create
>> > an encoder or connector manually. Let's not polute all drivers because
>> > i915 doesn't have its abstractions right.
>> 
>> i915 uses the quite common model for struct inheritance:
>> 
>> 	struct intel_connector {
>> 		struct drm_connector base;
>> 		/* ... */
>> 	}
>> 
>> Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau,
>> radeon, tilcdc, and vboxvideo.
>> 
>> We could argue about the relative merits of that abstraction, but I
>> think the bottom line is that it's popular and the drivers using it are
>> not going to be persuaded to move away from it.
>
> Nobody said inheritance is bad.
>
>> It's no coincidence that the drivers who've implemented writeback so far
>> (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction,
>> because the drm_writeback_connector midlayer does, forcing the issue.
>
> Are you sure it's not a coincidence ? :-)
>
> The encoder and especially connector created by drm_writeback_connector
> are there only because KMS requires a drm_encoder and a drm_connector to
> be exposed to userspace (and I could argue that using a connector for
> writeback is a hack, but that won't change). The connector is "virtual",
> I still fail to see why i915 or any other driver would need to wrap it
> into something else. The whole point of the drm_writeback_connector
> abstraction is that drivers do not have to manage the writeback
> drm_connector manually, they shouldn't touch it at all.

The thing is, drm_writeback_connector_init() calling
drm_connector_init() on the drm_connector embedded in
drm_writeback_connector leads to that connector being added to the
drm_device's list of connectors. Ditto for the encoder.

All the driver code that handles drm_connectors would need to take into
account they might not be embedded in intel_connector. Throughout the
driver. Ditto for the encoders.

The point is, you can't initialize a connector or an encoder for a
drm_device in isolation of the rest of the driver, even if it were
supposed to be hidden away.

BR,
Jani.

>
>> So I think drm_writeback_connector should *not* use the inheritance
>> abstraction because it's a midlayer that should leave that option to the
>> drivers. I think drm_writeback_connector needs to be changed to
>> accommodate that, and, unfortunately, it means current writeback users
>> need to be changed as well.
>> 
>> I am not sure, however, if the series at hand is the right
>> approach. Perhaps writeback can be modified to allocate the stuff for
>> you if you prefer it that way, as long as the drm_connector is not
>> embedded in struct drm_writeback_connector.
>> 
>> > Nack.
>> >
>> >>  	struct drm_writeback_connector writeback;
>> >>  };
>> >>  
>> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
>> >> index c79d1259e49b..5b1e83380c47 100644
>> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
>> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
>> >> @@ -200,8 +200,10 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu,
>> >>  {
>> >>  	struct drm_writeback_connector *wb_conn = &rcrtc->writeback;
>> >>  
>> >> -	wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
>> >> -	drm_connector_helper_add(&wb_conn->base,
>> >> +	wb_conn->base = &rcrtc->connector;
>> >> +	wb_conn->encoder = &rcrtc->encoder;
>> >> +	wb_conn->encoder->possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
>> >> +	drm_connector_helper_add(wb_conn->base,
>> >>  				 &rcar_du_wb_conn_helper_funcs);
>> >>  
>> >>  	return drm_writeback_connector_init(&rcdu->ddev, wb_conn,
>> >> @@ -220,7 +222,7 @@ void rcar_du_writeback_setup(struct rcar_du_crtc *rcrtc,
>> >>  	struct drm_framebuffer *fb;
>> >>  	unsigned int i;
>> >>  
>> >> -	state = rcrtc->writeback.base.state;
>> >> +	state = rcrtc->writeback.base->state;
>> >>  	if (!state || !state->writeback_job)
>> >>  		return;
>> >>  

-- 
Jani Nikula, Intel Open Source Graphics Center