Hi Thierry,
2016ë
06ì 08ì¼ 00:26ì Thierry Reding ì´(ê°) ì´ ê¸:
> From: Thierry Reding <treding at nvidia.com>
>
> Move the modeset locking from drivers into FB helpers.
>
> Signed-off-by: Thierry Reding <treding at 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");
>
