Re: [PATCH v8 07/10] drm: writeback: don't destroy the shared writeback properties

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

On 11/10/26 16:42, Dmitry Baryshkov wrote:
> The writeback properties are created once per device and attached to
> every writeback connector. Nevertheless, a failing
> __drm_writeback_connector_init() destroys all three of them. If an
> earlier writeback connector has already been registered (komeda and
> rcar-du create one per CRTC), it is left holding stale property
> pointers. The drmm cleanup action likewise destroys them as soon as the
> first connector goes away.
>
> Leave the properties alone. They are on the device's property list, so
> drm_mode_config_cleanup() destroys them together with all other
> properties.
This does appear to be the actual behaviour. More importantly, it is 
also the intended behaviour according to various comments sprinkled 
around the property creation functions.

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

> Fixes: 2f3f4a73631b ("drm: writeback: Add missing cleanup in case of initialization failure")
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> ---
>   drivers/gpu/drm/drm_writeback.c | 34 ++++++----------------------------
>   1 file changed, 6 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> index 89c36f1414ce..2f6a7b50336c 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -148,22 +148,6 @@ static int create_writeback_properties(struct drm_device *dev)
>   	return 0;
>   }
>   
> -static void delete_writeback_properties(struct drm_device *dev)
> -{
> -	if (dev->mode_config.writeback_pixel_formats_property) {
> -		drm_property_destroy(dev, dev->mode_config.writeback_pixel_formats_property);
> -		dev->mode_config.writeback_pixel_formats_property = NULL;
> -	}
> -	if (dev->mode_config.writeback_out_fence_ptr_property) {
> -		drm_property_destroy(dev, dev->mode_config.writeback_out_fence_ptr_property);
> -		dev->mode_config.writeback_out_fence_ptr_property = NULL;
> -	}
> -	if (dev->mode_config.writeback_fb_id_property) {
> -		drm_property_destroy(dev, dev->mode_config.writeback_fb_id_property);
> -		dev->mode_config.writeback_fb_id_property = NULL;
> -	}
> -}
> -
>   /**
>    * __drm_writeback_connector_init - Initialize a writeback connector
>    *
> @@ -198,20 +182,18 @@ static int __drm_writeback_connector_init(struct drm_device *dev,
>   	int ret = create_writeback_properties(dev);
>   
>   	if (ret != 0)
> -		goto failed_properties;
> +		return ret;
>   
>   	connector->interlace_allowed = 0;
>   
>   	ret = drm_connector_attach_encoder(connector, enc);
>   	if (ret)
> -		goto failed_properties;
> +		return ret;
>   
>   	blob = drm_property_create_blob(dev, n_formats * sizeof(*formats),
>   					formats);
> -	if (IS_ERR(blob)) {
> -		ret = PTR_ERR(blob);
> -		goto failed_properties;
> -	}
> +	if (IS_ERR(blob))
> +		return PTR_ERR(blob);
>   
>   	INIT_LIST_HEAD(&wb_connector->job_queue);
>   	spin_lock_init(&wb_connector->job_lock);
> @@ -234,9 +216,6 @@ static int __drm_writeback_connector_init(struct drm_device *dev,
>   	wb_connector->pixel_formats_blob_ptr = blob;
>   
>   	return 0;
> -failed_properties:
> -	delete_writeback_properties(dev);
> -	return ret;
>   }
>   
>   /**
> @@ -295,8 +274,8 @@ EXPORT_SYMBOL(drm_writeback_connector_init);
>    * @dev: DRM device
>    * @data: Pointer to the writeback connector to clean up
>    *
> - * This will decrement the reference counter of blobs and destroy properties. It
> - * will also clean the remaining jobs in this writeback connector. Caution: This helper will not
> + * 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.
>    */
>   static void drm_writeback_connector_cleanup(struct drm_device *dev,
> @@ -306,7 +285,6 @@ static void drm_writeback_connector_cleanup(struct drm_device *dev,
>   	struct drm_writeback_job *pos, *n;
>   	struct drm_writeback_connector *wb_connector = data;
>   
> -	delete_writeback_properties(dev);
>   	drm_property_blob_put(wb_connector->pixel_formats_blob_ptr);
>   
>   	spin_lock_irqsave(&wb_connector->job_lock, flags);
>