> Subject: Re: [PATCH v2 1/7] drm: writeback: Refactor drm_writeback_connector
> structure
>
> On Wed, Nov 05, 2025 at 02:39:15AM +0200, Dmitry Baryshkov wrote:
> > On Tue, 4 Nov 2025 at 16:05, Liviu Dudau <[email protected]> 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 <[email protected]>
> > > > > > ---
> > > > > > 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 <[email protected]>
> > > > >
Hi Liviu,
I am planning to refresh this series.
Can I take this Rb for the whole series or just this patch.
Regards,
Suraj Kandpal
> > > > >
> > > > > [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.
>
> Best regards,
> Liviu
>
> >
> > --
> > With best wishes
> > Dmitry
>
> --
> ====================
> | I would like to |
> | fix the world, |
> | but they're not |
> | giving me the |
> \ source code! /
> ---------------
> ¯\_(ツ)_/¯
