| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <DM3PPF208195D8D5DDD56AA88E006E66AD9E3C4A@DM3PPF208195D8D.namprd11.prod.outlook.com>
Date: Tue, 4 Nov 2025 05:11:25 +0000
From: "Kandpal, Suraj" <suraj.kandpal@...el.com>
To: Liviu Dudau <liviu.dudau@....com>
CC: "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>,
"dmitry.baryshkov@....qualcomm.com" <dmitry.baryshkov@....qualcomm.com>,
"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
> 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.
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! /
> ---------------
> ¯\_(ツ)_/¯
Powered by blists - more mailing lists