Message-ID: <CAO9ioeUwt2aR5y9FkMPOKz-4dPO8pUvxeEYUqGrGVZWaUHYw-g@mail.gmail.com>
Date: Thu, 6 Nov 2025 14:17:02 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
To: Liviu Dudau <liviu.dudau@....com>
Cc: "Kandpal, Suraj" <suraj.kandpal@...el.com>,
        "linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>,
        "freedreno@...ts.freedesktop.org" <freedreno@...ts.freedesktop.org>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "intel-xe@...ts.freedesktop.org" <intel-xe@...ts.freedesktop.org>,
        "intel-gfx@...ts.freedesktop.org" <intel-gfx@...ts.freedesktop.org>,
        "kernel-list@...pberrypi.com" <kernel-list@...pberrypi.com>,
        "amd-gfx@...ts.freedesktop.org" <amd-gfx@...ts.freedesktop.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-renesas-soc@...r.kernel.org" <linux-renesas-soc@...r.kernel.org>,
        "Nautiyal, Ankit K" <ankit.k.nautiyal@...el.com>,
        "Murthy, Arun R" <arun.r.murthy@...el.com>,
        "Shankar, Uma" <uma.shankar@...el.com>,
        "Nikula, Jani" <jani.nikula@...el.com>,
        "harry.wentland@....com" <harry.wentland@....com>,
        "siqueira@...lia.com" <siqueira@...lia.com>,
        "alexander.deucher@....com" <alexander.deucher@....com>,
        "christian.koenig@....com" <christian.koenig@....com>,
        "airlied@...il.com" <airlied@...il.com>,
        "simona@...ll.ch" <simona@...ll.ch>,
        "maarten.lankhorst@...ux.intel.com" <maarten.lankhorst@...ux.intel.com>,
        "mripard@...nel.org" <mripard@...nel.org>,
        "robin.clark@....qualcomm.com" <robin.clark@....qualcomm.com>,
        "abhinav.kumar@...ux.dev" <abhinav.kumar@...ux.dev>,
        "tzimmermann@...e.de" <tzimmermann@...e.de>,
        "jessica.zhang@....qualcomm.com" <jessica.zhang@....qualcomm.com>,
        "sean@...rly.run" <sean@...rly.run>,
        "marijn.suijten@...ainline.org" <marijn.suijten@...ainline.org>,
        "laurent.pinchart+renesas@...asonboard.com" <laurent.pinchart+renesas@...asonboard.com>,
        "mcanal@...lia.com" <mcanal@...lia.com>,
        "dave.stevenson@...pberrypi.com" <dave.stevenson@...pberrypi.com>,
        "tomi.valkeinen+renesas@...asonboard.com" <tomi.valkeinen+renesas@...asonboard.com>,
        "kieran.bingham+renesas@...asonboard.com" <kieran.bingham+renesas@...asonboard.com>,
        "louis.chauvet@...tlin.com" <louis.chauvet@...tlin.com>
Subject: Re: [PATCH v2 1/7] drm: writeback: Refactor drm_writeback_connector structure

On Thu, 6 Nov 2025 at 13:04, Liviu Dudau <liviu.dudau@....com> wrote:
>
> On Wed, Nov 05, 2025 at 02:39:15AM +0200, Dmitry Baryshkov wrote:
> > On Tue, 4 Nov 2025 at 16:05, Liviu Dudau <liviu.dudau@....com> wrote:
> > >
> > > On Tue, Nov 04, 2025 at 05:11:25AM +0000, Kandpal, Suraj wrote:
> > > > > Subject: Re: [PATCH v2 1/7] drm: writeback: Refactor
> > > > > drm_writeback_connector structure
> > > > >
> > > > > On Tue, Oct 07, 2025 at 11:15:23AM +0530, Suraj Kandpal wrote:
> > > > > > Some drivers cannot work with the current design where the connector
> > > > > > is embedded within the drm_writeback_connector such as Intel and some
> > > > > > drivers that can get it working end up adding a lot of checks all
> > > > > > around the code to check if it's a writeback conenctor or not, this is
> > > > > > due to the limitation of inheritance in C.
> > > > > > To solve this move the drm_writeback_connector within the
> > > > > > drm_connector and remove the drm_connector base which was in
> > > > > > drm_writeback_connector. Make this drm_writeback_connector a union
> > > > > > with hdmi connector to save memory and since a connector can never be
> > > > > > both writeback and hdmi it should serve us well.
> > > > > > Do all other required modifications that come with these changes along
> > > > > > with addition of new function which returns the drm_connector when
> > > > > > drm_writeback_connector is present.
> > > > > > Modify drivers using the drm_writeback_connector to allow them to use
> > > > > > this connector without breaking them.
> > > > > > The drivers modified here are amd, komeda, mali, vc4, vkms, rcar_du,
> > > > > > msm
> > > > > >
> > > > > > Signed-off-by: Suraj Kandpal <suraj.kandpal@...el.com>
> > > > > > ---
> > > > > > V1 -> V2: Use &connector->writeback, make commit message imperative
> > > > > > (Dmitry)
> > > > > > ---
> > > > > >  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  6 +-
> > > > > > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  2 +-
> > > > > > .../drm/amd/display/amdgpu_dm/amdgpu_dm_wb.c  |  8 +--
> > > > > > .../gpu/drm/arm/display/komeda/komeda_crtc.c  |  6 +-
> > > > > >  .../gpu/drm/arm/display/komeda/komeda_kms.h   |  6 +-
> > > > > >  .../arm/display/komeda/komeda_wb_connector.c  |  8 +--
> > > > > >  drivers/gpu/drm/arm/malidp_crtc.c             |  2 +-
> > > > > >  drivers/gpu/drm/arm/malidp_drv.h              |  2 +-
> > > > > >  drivers/gpu/drm/arm/malidp_hw.c               |  6 +-
> > > > > >  drivers/gpu/drm/arm/malidp_mw.c               |  8 +--
> > > > > >  drivers/gpu/drm/drm_atomic_uapi.c             |  2 +-
> > > > > >  drivers/gpu/drm/drm_writeback.c               | 35 ++++++----
> > > > >
> > > > > For the komeda and malidp drivers, as well as for the drm_writeback.c
> > > > > changes:
> > > > >
> > > > > Reviewed-by: Liviu Dudau <liviu.dudau@....com>
> > > > >
> > > > >
> > > > > [snip]
> > > > >
> > > > >
> > > > > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > > > > index 8f34f4b8183d..1b090e6bddc1 100644
> > > > > > --- a/include/drm/drm_connector.h
> > > > > > +++ b/include/drm/drm_connector.h
> > > > > > @@ -1882,6 +1882,61 @@ struct drm_connector_cec {
> > > > > >   void *data;
> > > > > >  };
> > > > > >
> > > > > > +/**
> > > > > > + * struct drm_writeback_connector - DRM writeback connector  */
> > > > > > +struct drm_writeback_connector {
> > > > > > + /**
> > > > > > +  * @pixel_formats_blob_ptr:
> > > > > > +  *
> > > > > > +  * DRM blob property data for the pixel formats list on writeback
> > > > > > +  * connectors
> > > > > > +  * See also drm_writeback_connector_init()
> > > > > > +  */
> > > > > > + struct drm_property_blob *pixel_formats_blob_ptr;
> > > > > > +
> > > > > > + /** @job_lock: Protects job_queue */
> > > > > > + spinlock_t job_lock;
> > > > > > +
> > > > > > + /**
> > > > > > +  * @job_queue:
> > > > > > +  *
> > > > > > +  * Holds a list of a connector's writeback jobs; the last item is the
> > > > > > +  * most recent. The first item may be either waiting for the hardware
> > > > > > +  * to begin writing, or currently being written.
> > > > > > +  *
> > > > > > +  * See also: drm_writeback_queue_job() and
> > > > > > +  * drm_writeback_signal_completion()
> > > > > > +  */
> > > > > > + struct list_head job_queue;
> > > > > > +
> > > > > > + /**
> > > > > > +  * @fence_context:
> > > > > > +  *
> > > > > > +  * timeline context used for fence operations.
> > > > > > +  */
> > > > > > + unsigned int fence_context;
> > > > > > + /**
> > > > > > +  * @fence_lock:
> > > > > > +  *
> > > > > > +  * spinlock to protect the fences in the fence_context.
> > > > > > +  */
> > > > > > + spinlock_t fence_lock;
> > > > > > + /**
> > > > > > +  * @fence_seqno:
> > > > > > +  *
> > > > > > +  * Seqno variable used as monotonic counter for the fences
> > > > > > +  * created on the connector's timeline.
> > > > > > +  */
> > > > > > + unsigned long fence_seqno;
> > > > > > + /**
> > > > > > +  * @timeline_name:
> > > > > > +  *
> > > > > > +  * The name of the connector's fence timeline.
> > > > > > +  */
> > > > > > + char timeline_name[32];
> > > > > > +};
> > > > > > +
> > > > > >  /**
> > > > > >   * struct drm_connector - central DRM connector control structure
> > > > > >   *
> > > > > > @@ -2291,10 +2346,16 @@ struct drm_connector {
> > > > > >    */
> > > > > >   struct llist_node free_node;
> > > > > >
> > > > > > - /**
> > > > > > -  * @hdmi: HDMI-related variable and properties.
> > > > > > -  */
> > > > > > - struct drm_connector_hdmi hdmi;
> > > > > > + union {
> > > > >
> > > > > This is a surprising choice. Before this patch one had to have a separate
> > > > > writeback connector besides the HDMI connector. Going forward it looks like
> > > > > you still need two connectors, one that uses the writeback member and one
> > > > > that uses the hdmi one. Is that intended?
> > > > >
> > > > > I was expecting that you're going to declare the writeback member next to the
> > > > > hdmi, without overlap. If you do that, then you also don't need to move the
> > > > > struct drm_writeback declaration from the header file and it should be enough
> > > > > to include the drm_writeback.h file.
> > > >
> > > > Hi,
> > > > Thanks for the review
> > > > The reason for this came from the discussion on previous patches and was suggested by Dmitry.
> > > > The idea is that a connector can never be both an HDMI and writeback connector at the same time
> > > > Hence we save space if we pack them together.
> > >
> > > Hmm, but you can still have all the CEC and HDMI codecs data in that connector,
> > > which feels strange.  Also, what's the issue with having a connector that has
> > > both a valid HDMI state and an associated writeback at the same time (i.e.
> > > don't use the union)? Writing back the memory the output that goes to HDMI is
> > > valid, right?
> >
> > Writing back to memory requires a separate connector (with separate
> > setup). The CRTC should also support outputting data to both HDMI
> > _and_ Writeback connectors at the same time (aka cloning). Not all
> > configurations are possible, writeback requires additional bandwidth,
> > etc., etc.
> >
> > >
> > > Maybe that is not something that you considered, but with this patch (without union)
> > > we can drop the need to have a separate connector just for writeback. We're breaking
> > > user space compatibility, true, but it feels like a good change to be able to
> > > attach a writeback to any connector and get its output. The drivers that don't support
> > > that can reject the commit that attaches the writeback to the existing connector.
> >
> > Well... No. It's not how it is being handled in the (existing)
> > hardware. Nor does it make it easier to handle resources for the
> > writeback.
>
> Which (existing) hardware? Komeda can do it mainly because it doesn't have an HDMI connector,
> but an output that can be cloned to writeback while it is being sent out on a bus to an encoder.
> You have to remember that writeback is a connector because we didn't have a better concept for
> it. It doesn't have to be a separate connector from an HDMI or eDP or DP.

drm/msm. It requires a separate setup for the Writeback, which is
described as an encoder. As such, we need a separate connector for the
writeback.

-- 
With best wishes
Dmitry

Powered by blists - more mailing lists