Re: [PATCH v8 09/10] drm: writeback: clean up writeback connectors in drm_connector_cleanup()

[John Harrison <John.Harrison@Igalia.com>]

On 11/10/26 16:42, Dmitry Baryshkov wrote:
> Only drmm_writeback_connector_init() registers
> drm_writeback_connector_cleanup(). Connectors set up with
> drm_writeback_connector_init() (amdgpu, komeda, malidp, rcar-du, vc4)
> never drop the formats blob or free the jobs still on the queue, leaking
> them together with their framebuffer and fence references.
>
> Call drm_writeback_connector_cleanup() from drm_connector_cleanup() so it
> runs for both kinds of initialization, the same way the HDMI audio and
> SCDC state is torn down there. It downcasts the connector to its
> drm_writeback_connector wrapper, which is only valid when the connector
> really is embedded in one. A plain drm_connector (or a different wrapper
> struct) may be initialized with DRM_MODE_CONNECTOR_WRITEBACK without being
> a writeback connector, as several KUnit tests do, so keying the cleanup
> off the connector type would read out of bounds. Track genuine writeback
> initialization with a drm_connector::wb_initialized flag set by
> __drm_writeback_connector_init() and clean up based on it instead. The
> drmm action now just wraps the helper; clearing the flag makes the second
> invocation a no-op and also covers drm_connector_cleanup() being called
> after a failed __drm_writeback_connector_init().
>
> Fixes: 935774cd71fe ("drm: Add writeback connector type")
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
I have verified that this version fixes the selftest failures I was hitting.

Reviewed-by: John Harrison <John.Harrison@Igalia.com>

> ---
>   drivers/gpu/drm/drm_connector.c     |  3 +++
>   drivers/gpu/drm/drm_crtc_internal.h |  3 +++
>   drivers/gpu/drm/drm_writeback.c     | 30 ++++++++++++++++++++++++------
>   include/drm/drm_connector.h         | 12 ++++++++++++
>   4 files changed, 42 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index d66188521881..da0b16f25ac4 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -921,6 +921,9 @@ void drm_connector_cleanup(struct drm_connector *connector)
>   	if (connector->hdmi.scdc_work_initialized)
>   		cancel_delayed_work_sync(&connector->hdmi.scdc_work);
>   
> +	if (connector->wb_initialized)
> +		drm_writeback_connector_cleanup(connector);
> +
>   	platform_device_unregister(connector->hdmi_audio.codec_pdev);
>   
>   	if (connector->privacy_screen) {
> diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
> index 3492787c3697..f60216adcd31 100644
> --- a/drivers/gpu/drm/drm_crtc_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_internal.h
> @@ -329,4 +329,7 @@ static inline void drm_panic_unregister(struct drm_device *dev) {}
>   static inline void drm_panic_init(void) {}
>   #endif
>   
> +/* drm_writeback.c */
> +void drm_writeback_connector_cleanup(struct drm_connector *connector);
> +
>   #endif /* __DRM_CRTC_INTERNAL_H__ */
> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> index 6487f4a104f2..6ea3e55a1962 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -21,6 +21,8 @@
>   #include <drm/drm_property.h>
>   #include <drm/drm_writeback.h>
>   
> +#include "drm_crtc_internal.h"
> +
>   /**
>    * DOC: overview
>    *
> @@ -215,6 +217,8 @@ static int __drm_writeback_connector_init(struct drm_device *dev,
>   				   blob->base.id);
>   	wb_connector->pixel_formats_blob_ptr = blob;
>   
> +	connector->wb_initialized = true;
> +
>   	return 0;
>   }
>   
> @@ -271,22 +275,28 @@ EXPORT_SYMBOL(drm_writeback_connector_init);
>   
>   /**
>    * drm_writeback_connector_cleanup - Cleanup the writeback connector
> - * @dev: DRM device
> - * @data: Pointer to the writeback connector to clean up
> + * @connector: Writeback connector to clean up
>    *
>    * This will decrement the reference counter of blobs. It will also clean the
>    * remaining jobs in this writeback connector. Caution: This helper will not
>    * clean up the attached encoder and the drm_connector.
> + *
> + * Called from drm_connector_cleanup() for all writeback connectors.
>    */
> -static void drm_writeback_connector_cleanup(struct drm_device *dev,
> -					    void *data)
> +void drm_writeback_connector_cleanup(struct drm_connector *connector)
>   {
> -	struct drm_writeback_connector *wb_connector = data;
> +	struct drm_writeback_connector *wb_connector =
> +		drm_connector_to_writeback(connector);
>   	struct drm_writeback_job *pos, *n;
>   	unsigned long flags;
>   	LIST_HEAD(jobs);
>   
> +	if (!connector->wb_initialized)
> +		return;
> +	connector->wb_initialized = false;
> +
>   	drm_property_blob_put(wb_connector->pixel_formats_blob_ptr);
> +	wb_connector->pixel_formats_blob_ptr = NULL;
>   
>   	spin_lock_irqsave(&wb_connector->job_lock, flags);
>   	list_splice_init(&wb_connector->job_queue, &jobs);
> @@ -298,6 +308,14 @@ static void drm_writeback_connector_cleanup(struct drm_device *dev,
>   	}
>   }
>   
> +static void drm_writeback_connector_cleanup_action(struct drm_device *dev,
> +						   void *data)
> +{
> +	struct drm_writeback_connector *wb_connector = data;
> +
> +	drm_writeback_connector_cleanup(&wb_connector->base);
> +}
> +
>   /**
>    * drmm_writeback_connector_init - Initialize a writeback connector
>    *
> @@ -336,7 +354,7 @@ int drmm_writeback_connector_init(struct drm_device *dev,
>   	if (ret)
>   		return ret;
>   
> -	ret = drmm_add_action_or_reset(dev, drm_writeback_connector_cleanup,
> +	ret = drmm_add_action_or_reset(dev, drm_writeback_connector_cleanup_action,
>   				       wb_connector);
>   	if (ret)
>   		return ret;
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index f656ed8324d3..07218ab26c9e 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -2688,6 +2688,18 @@ struct drm_connector {
>   	 */
>   	struct llist_node free_node;
>   
> +	/**
> +	 * @wb_initialized:
> +	 *
> +	 * Set when the connector is embedded in a &struct
> +	 * drm_writeback_connector that has been initialized through
> +	 * __drm_writeback_connector_init(). A plain &struct drm_connector may
> +	 * be created with DRM_MODE_CONNECTOR_WRITEBACK without being a
> +	 * writeback connector, so drm_connector_cleanup() keys the writeback
> +	 * teardown off this flag rather than the connector type.
> +	 */
> +	bool wb_initialized;
> +
>   	/**
>   	 * @hdmi: HDMI-related variable and properties.
>   	 */
>