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]> > > > > > > > > > [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. -- With best wishes Dmitry
