> 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]>
>
>
> [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.
Regards,
Suraj Kandpal
>
> Best regards,
> Liviu
>
> > + /**
> > + * @hdmi: HDMI-related variable and properties.
> > + */
> > + struct drm_connector_hdmi hdmi;
> > + /**
> > + * @writeback: Writeback related valriables.
> > + */
> > + struct drm_writeback_connector writeback;
> > + };
> >
> > /**
> > * @hdmi_audio: HDMI codec properties and non-DRM state.
> > diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
> > index 958466a05e60..702141099520 100644
> > --- a/include/drm/drm_writeback.h
> > +++ b/include/drm/drm_writeback.h
> > @@ -15,66 +15,6 @@
> > #include <drm/drm_encoder.h>
> > #include <linux/workqueue.h>
> >
> > -/**
> > - * struct drm_writeback_connector - DRM writeback connector
> > - */
> > -struct drm_writeback_connector {
> > - /**
> > - * @base: base drm_connector object
> > - */
> > - struct drm_connector base;
> > -
> > - /**
> > - * @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_writeback_job - DRM writeback job
> > */
> > @@ -131,10 +71,10 @@ struct drm_writeback_job {
> > void *priv;
> > };
> >
> > -static inline struct drm_writeback_connector *
> > -drm_connector_to_writeback(struct drm_connector *connector)
> > +static inline struct drm_connector *
> > +drm_writeback_to_connector(struct drm_writeback_connector
> > +*wb_connector)
> > {
> > - return container_of(connector, struct drm_writeback_connector,
> base);
> > + return container_of(wb_connector, struct drm_connector, writeback);
> > }
> >
> > int drm_writeback_connector_init(struct drm_device *dev,
> > --
> > 2.34.1
> >
>
> --
> ====================
> | I would like to |
> | fix the world, |
> | but they're not |
> | giving me the |
> \ source code! /
> ---------------
> ¯\_(ツ)_/¯
