Re: [PATCH 0/8] drm: writeback: clean up writeback connector initialization

[Liviu Dudau <liviu.dudau@arm.com>]

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.

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.

Best regards,
Liviu


> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> ---
> Dmitry Baryshkov (8):
>       drm/amd/display: use drmm_writeback_connector_init()
>       drm/komeda: use drmm_writeback_connector_init()
>       drm/mali: use drmm_writeback_connector_init()
>       drm/msm/dpu: use drmm_writeback_connector_init()
>       drm/msm/dpu: use drmm_writeback_connector_init()
>       drm/vc4: use drmm_writeback_connector_init()
>       drm: writeback: drop excess connector initialization functions
>       drm: writeback: rename drm_writeback_connector_init_with_encoder()
> 
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c  |  2 +-
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_wb.c   | 18 ++++--
>  .../drm/arm/display/komeda/komeda_wb_connector.c   | 30 ++++++----
>  drivers/gpu/drm/arm/malidp_mw.c                    | 25 ++++----
>  drivers/gpu/drm/drm_writeback.c                    | 69 +++-------------------
>  drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c      | 10 +---
>  .../gpu/drm/renesas/rcar-du/rcar_du_writeback.c    | 23 +++++---
>  drivers/gpu/drm/vc4/vc4_txp.c                      |  9 ++-
>  include/drm/drm_writeback.h                        | 22 +------
>  9 files changed, 77 insertions(+), 131 deletions(-)
> ---
> base-commit: 94f208ff622b09309358abaf26d7acca0c318fae
> change-id: 20250801-wb-drop-encoder-97a0c75bd5d7
> 
> Best regards,
> -- 
> With best wishes
> Dmitry
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯