On Mon, Aug 04, 2025 at 03:43:12PM +0100, Liviu Dudau wrote: > Hi, > > On Fri, Aug 01, 2025 at 04:51:08PM +0300, Dmitry Baryshkov wrote: > > Drivers using drm_writeback_connector_init() / _with_encoder() don't > > perform cleanup in a manner similar to drmm_writeback_connector_init() > > (see drm_writeback_connector_cleanup()). Migrate all existing drivers > > to use drmm_writeback_connector_init(), drop > > drm_writeback_connector_init() and drm_writeback_connector::encoder > > (it's unused afterwards). > > > > This series leaves former drm_writeback_connector_init_with_encoder() > > (renamed to drm_writeback_connector_init as a non-managed counterpart > > for drmm_writeback_connector_init()). It is supposed to be used by > > drivers which can not use drmm functions (like Intel). However I think > > it would be better to drop it completely. > > The intent of _init_with_encoder() was to be a special case for drivers > that use their own specific encoder and the rest use the generic function > that creates the virtual encoder inside the call. The API for > _init_with_encoder() was actually introduced 4 years after the original > patch, so that should give a hint. > > drmm_writeback_connector_init() is more like _init_with_encoder() and > I don't remember reviewing it, so I'm not sure why that was considered > to be the better behaviour for the managed version. Now you're moving > all the drivers to the managed version and you have to duplicate code > in each driver to create the ENCODER_VIRTUAL encoder.
This follows e.g. the process of deprecating drm_simple_* / drm_simple_encoder. The drivers are expected to open code empty encoder handling on their own. > I'm not against the changes being made in the series, I just want to > see a better justification on why _init_with_encoder() behaviour is > better than the previous default that you're removing. This was triggered by the discussion of Intel writeback patchset, see the threads for first three patches of [1]. We have an optional non-pointer field inside drm_writeback_connector, which can be left uninitialized (or zero-filled). I have checked and the drivers are not actually using the embedded connector for anything after linking it to the drm_connector. So, by removing the encoder from the drm_writeback_connector structure we are tying a loose end. [1] https://lore.kernel.org/dri-devel/20250725050409.2687242-1-suraj.kand...@intel.com/ -- With best wishes Dmitry
