Re: [PATCH v2 4/9] drm/fb-helper: Push down modeset lock into FB helpers

From: Inki Dae <inki.dae@samsung.com>
To: Thierry Reding <thierry.reding@gmail.com>,
	Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: "Chen Feng" <puck.chen@hisilicon.com>,
	intel-gfx@lists.freedesktop.org,
	"Seung-Woo Kim" <sw0312.kim@samsung.com>,
	dri-devel@lists.freedesktop.org,
	"Xinliang Liu" <z.liuxinliang@hisilicon.com>,
	"Kyungmin Park" <kyungmin.park@samsung.com>,
	"Alex Deucher" <alexander.deucher@amd.com>,
	"Christian König" <christian.koenig@amd.com>
Subject: Re: [PATCH v2 4/9] drm/fb-helper: Push down modeset lock into FB helpers
Date: Fri, 10 Jun 2016 08:56:50 +0900	[thread overview]
Message-ID: <575A0242.5090406@samsung.com> (raw)
In-Reply-To: <20160607152625.9511-5-thierry.reding@gmail.com>

Hi Thierry,

2016년 06월 08일 00:26에 Thierry Reding 이(가) 쓴 글:
> From: Thierry Reding <treding@nvidia.com>
> 
> Move the modeset locking from drivers into FB helpers.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/gpu/drm/drm_fb_helper.c        | 59 +++++++++++++++++++++++++---------
>  drivers/gpu/drm/i915/intel_dp_mst.c    |  4 ---
>  drivers/gpu/drm/radeon/radeon_dp_mst.c |  7 ----
>  3 files changed, 44 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 9afd4208596f..252c7557bdb5 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -95,8 +95,8 @@ static LIST_HEAD(kernel_fb_helper_list);
>   * mmap page writes.
>   */
>  
> -int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
> -				    struct drm_connector *connector)
> +static int __drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
> +					     struct drm_connector *connector)
>  {
>  	struct drm_fb_helper_connector **temp;
>  	struct drm_fb_helper_connector *conn;
> @@ -129,6 +129,20 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
>  	fb_helper->connector_info[fb_helper->connector_count++] = conn;
>  	return 0;
>  }
> +
> +int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
> +				    struct drm_connector *connector)
> +{
> +	int err;
> +
> +	mutex_lock(&fb_helper->dev->mode_config.mutex);
> +
> +	err = __drm_fb_helper_add_one_connector(fb_helper, connector);

I think you don't need to make __drm_fb_helper_add_on_connector function but just you can implement the codes here instead. And the use of prefix '__' thing would not good.
And my concern about this is that other drivers may need to call this function after taking a same mutex lock. Can you assure anyone not to call this function with mutex lock?

If there is such case then,

some_function()
{
	mutex_lock();
	...
	mutex_unlock();
	drm_fb_helper_add_one_connector();
	...
}

So in this case, other users should consider to make sure to unlock before calling this function. That would be now really what you want.

Thanks,
Inki Dae

> +
> +	mutex_unlock(&fb_helper->dev->mode_config.mutex);
> +
> +	return err;
> +}
>  EXPORT_SYMBOL(drm_fb_helper_add_one_connector);
>  
>  /**
> @@ -155,28 +169,28 @@ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
>  		return 0;
>  
>  	mutex_lock(&dev->mode_config.mutex);
> +
>  	drm_for_each_connector(connector, dev) {
> -		ret = drm_fb_helper_add_one_connector(fb_helper, connector);
> +		ret = __drm_fb_helper_add_one_connector(fb_helper, connector);
> +		if (ret) {
> +			for (i = 0; i < fb_helper->connector_count; i++) {
> +				kfree(fb_helper->connector_info[i]);
> +				fb_helper->connector_info[i] = NULL;
> +			}

I think all resources should be released by someone who allocated the resources in case of failure. I mean that if some routine of __drm_fb_helper_add_on_connector function failed than the function make sure to release the resources allocated. I know this is really not what you did but original code did. So how about cleanning up this thing? 

Thanks,
Inki Dae

>  
> -		if (ret)
> -			goto fail;
> -	}
> -	mutex_unlock(&dev->mode_config.mutex);
> -	return 0;
> -fail:
> -	for (i = 0; i < fb_helper->connector_count; i++) {
> -		kfree(fb_helper->connector_info[i]);
> -		fb_helper->connector_info[i] = NULL;
> +			fb_helper->connector_count = 0;
> +			break;
> +		}
>  	}
> -	fb_helper->connector_count = 0;
> +
>  	mutex_unlock(&dev->mode_config.mutex);
>  
>  	return ret;
>  }
>  EXPORT_SYMBOL(drm_fb_helper_single_add_all_connectors);
>  
> -int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
> -				       struct drm_connector *connector)
> +static int __drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
> +						struct drm_connector *connector)
>  {
>  	struct drm_fb_helper_connector *fb_helper_connector;
>  	int i, j;
> @@ -193,6 +207,7 @@ int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
>  
>  	if (i == fb_helper->connector_count)
>  		return -EINVAL;
> +
>  	fb_helper_connector = fb_helper->connector_info[i];
>  	drm_connector_unreference(fb_helper_connector->connector);
>  
> @@ -204,6 +219,20 @@ int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
>  
>  	return 0;
>  }
> +
> +int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
> +				       struct drm_connector *connector)
> +{
> +	int err;
> +
> +	mutex_lock(&fb_helper->dev->mode_config.mutex);
> +
> +	err = __drm_fb_helper_remove_one_connector(fb_helper, connector);
> +
> +	mutex_unlock(&fb_helper->dev->mode_config.mutex);
> +
> +	return err;
> +}
>  EXPORT_SYMBOL(drm_fb_helper_remove_one_connector);
>  
>  static void drm_fb_helper_save_lut_atomic(struct drm_crtc *crtc, struct drm_fb_helper *helper)
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 7a34090cef34..47c3980cb3b9 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -477,9 +477,7 @@ static void intel_dp_register_mst_connector(struct drm_connector *connector)
>  {
>  	struct intel_connector *intel_connector = to_intel_connector(connector);
>  	struct drm_device *dev = connector->dev;
> -	drm_modeset_lock_all(dev);
>  	intel_connector_add_to_fbdev(intel_connector);
> -	drm_modeset_unlock_all(dev);
>  	drm_connector_register(&intel_connector->base);
>  }
>  
> @@ -492,10 +490,8 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
>  	intel_connector->unregister(intel_connector);
>  
>  	/* need to nuke the connector */
> -	drm_modeset_lock_all(dev);
>  	intel_connector_remove_from_fbdev(intel_connector);
>  	intel_connector->mst_port = NULL;
> -	drm_modeset_unlock_all(dev);
>  
>  	drm_connector_unreference(&intel_connector->base);
>  	DRM_DEBUG_KMS("\n");
> diff --git a/drivers/gpu/drm/radeon/radeon_dp_mst.c b/drivers/gpu/drm/radeon/radeon_dp_mst.c
> index de504ea29c06..a4ccaa5af104 100644
> --- a/drivers/gpu/drm/radeon/radeon_dp_mst.c
> +++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c
> @@ -299,9 +299,7 @@ static void radeon_dp_register_mst_connector(struct drm_connector *connector)
>  	struct drm_device *dev = connector->dev;
>  	struct radeon_device *rdev = dev->dev_private;
>  
> -	drm_modeset_lock_all(dev);
>  	radeon_fb_add_connector(rdev, connector);
> -	drm_modeset_unlock_all(dev);
>  
>  	drm_connector_register(connector);
>  }
> @@ -314,13 +312,8 @@ static void radeon_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
>  	struct radeon_device *rdev = dev->dev_private;
>  
>  	drm_connector_unregister(connector);
> -	/* need to nuke the connector */
> -	drm_modeset_lock_all(dev);
> -	/* dpms off */
>  	radeon_fb_remove_connector(rdev, connector);
> -
>  	drm_connector_cleanup(connector);
> -	drm_modeset_unlock_all(dev);
>  
>  	kfree(connector);
>  	DRM_DEBUG_KMS("\n");
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel