On Thu, 6 Nov 2025 at 13:04, Liviu Dudau <[email protected]> wrote: > > 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]> > > > > > > > > > > > > > > > [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
