diff mbox

drm: Fix locking cargo-cult in encoder/plane init/cleanup

Message ID 20161129094538.9650-1-daniel.vetter@ffwll.ch (mailing list archive)
State New, archived
Headers show

Commit Message

Daniel Vetter Nov. 29, 2016, 9:45 a.m. UTC 
Encoders&planes can't be hotplugged, we dont need looking for this
since it's all single-threaded driver setup/teardown code. CRTCs
already don't grab locks.

While at it I noticed that plane's are missing the
drm_modeset_lock_fini() call, so add it.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_encoder.c | 9 +--------
 drivers/gpu/drm/drm_plane.c   | 4 ++--
 2 files changed, 3 insertions(+), 10 deletions(-)

Comments

Frank Binns Nov. 29, 2016, 10:39 a.m. UTC | #1 
On 29/11/16 09:45, Daniel Vetter wrote:
> Encoders&planes can't be hotplugged, we dont need looking for this

s/looking/locking/

With that changed:
Reviewed-by: Frank Binns <frank.binns@imgtec.com>

> since it's all single-threaded driver setup/teardown code. CRTCs
> already don't grab locks.
>
> While at it I noticed that plane's are missing the
> drm_modeset_lock_fini() call, so add it.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>   drivers/gpu/drm/drm_encoder.c | 9 +--------
>   drivers/gpu/drm/drm_plane.c   | 4 ++--
>   2 files changed, 3 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> index 5c067719164d..992879f15f23 100644
> --- a/drivers/gpu/drm/drm_encoder.c
> +++ b/drivers/gpu/drm/drm_encoder.c
> @@ -110,11 +110,9 @@ int drm_encoder_init(struct drm_device *dev,
>   {
>   	int ret;
>   
> -	drm_modeset_lock_all(dev);
> -
>   	ret = drm_mode_object_get(dev, &encoder->base, DRM_MODE_OBJECT_ENCODER);
>   	if (ret)
> -		goto out_unlock;
> +		return ret;
>   
>   	encoder->dev = dev;
>   	encoder->encoder_type = encoder_type;
> @@ -142,9 +140,6 @@ int drm_encoder_init(struct drm_device *dev,
>   	if (ret)
>   		drm_mode_object_unregister(dev, &encoder->base);
>   
> -out_unlock:
> -	drm_modeset_unlock_all(dev);
> -
>   	return ret;
>   }
>   EXPORT_SYMBOL(drm_encoder_init);
> @@ -164,12 +159,10 @@ void drm_encoder_cleanup(struct drm_encoder *encoder)
>   	 * the indices on the drm_encoder after us in the encoder_list.
>   	 */
>   
> -	drm_modeset_lock_all(dev);
>   	drm_mode_object_unregister(dev, &encoder->base);
>   	kfree(encoder->name);
>   	list_del(&encoder->head);
>   	dev->mode_config.num_encoder--;
> -	drm_modeset_unlock_all(dev);
>   
>   	memset(encoder, 0, sizeof(*encoder));
>   }
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 419ac313c36f..9147aab182c4 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -221,7 +221,8 @@ void drm_plane_cleanup(struct drm_plane *plane)
>   {
>   	struct drm_device *dev = plane->dev;
>   
> -	drm_modeset_lock_all(dev);
> +	drm_modeset_lock_fini(&plane->mutex);
> +
>   	kfree(plane->format_types);
>   	drm_mode_object_unregister(dev, &plane->base);
>   
> @@ -236,7 +237,6 @@ void drm_plane_cleanup(struct drm_plane *plane)
>   	dev->mode_config.num_total_plane--;
>   	if (plane->type == DRM_PLANE_TYPE_OVERLAY)
>   		dev->mode_config.num_overlay_plane--;
> -	drm_modeset_unlock_all(dev);
>   
>   	WARN_ON(plane->state && !plane->funcs->atomic_destroy_state);
>   	if (plane->state && plane->funcs->atomic_destroy_state)
Daniel Vetter Nov. 29, 2016, 2:14 p.m. UTC | #2 
On Tue, Nov 29, 2016 at 10:39:03AM +0000, Frank Binns wrote:
> On 29/11/16 09:45, Daniel Vetter wrote:
> > Encoders&planes can't be hotplugged, we dont need looking for this
> 
> s/looking/locking/
> 
> With that changed:
> Reviewed-by: Frank Binns <frank.binns@imgtec.com>

Fixed&applied, thanks for your review.
-Daniel

> 
> > since it's all single-threaded driver setup/teardown code. CRTCs
> > already don't grab locks.
> > 
> > While at it I noticed that plane's are missing the
> > drm_modeset_lock_fini() call, so add it.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >   drivers/gpu/drm/drm_encoder.c | 9 +--------
> >   drivers/gpu/drm/drm_plane.c   | 4 ++--
> >   2 files changed, 3 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> > index 5c067719164d..992879f15f23 100644
> > --- a/drivers/gpu/drm/drm_encoder.c
> > +++ b/drivers/gpu/drm/drm_encoder.c
> > @@ -110,11 +110,9 @@ int drm_encoder_init(struct drm_device *dev,
> >   {
> >   	int ret;
> > -	drm_modeset_lock_all(dev);
> > -
> >   	ret = drm_mode_object_get(dev, &encoder->base, DRM_MODE_OBJECT_ENCODER);
> >   	if (ret)
> > -		goto out_unlock;
> > +		return ret;
> >   	encoder->dev = dev;
> >   	encoder->encoder_type = encoder_type;
> > @@ -142,9 +140,6 @@ int drm_encoder_init(struct drm_device *dev,
> >   	if (ret)
> >   		drm_mode_object_unregister(dev, &encoder->base);
> > -out_unlock:
> > -	drm_modeset_unlock_all(dev);
> > -
> >   	return ret;
> >   }
> >   EXPORT_SYMBOL(drm_encoder_init);
> > @@ -164,12 +159,10 @@ void drm_encoder_cleanup(struct drm_encoder *encoder)
> >   	 * the indices on the drm_encoder after us in the encoder_list.
> >   	 */
> > -	drm_modeset_lock_all(dev);
> >   	drm_mode_object_unregister(dev, &encoder->base);
> >   	kfree(encoder->name);
> >   	list_del(&encoder->head);
> >   	dev->mode_config.num_encoder--;
> > -	drm_modeset_unlock_all(dev);
> >   	memset(encoder, 0, sizeof(*encoder));
> >   }
> > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > index 419ac313c36f..9147aab182c4 100644
> > --- a/drivers/gpu/drm/drm_plane.c
> > +++ b/drivers/gpu/drm/drm_plane.c
> > @@ -221,7 +221,8 @@ void drm_plane_cleanup(struct drm_plane *plane)
> >   {
> >   	struct drm_device *dev = plane->dev;
> > -	drm_modeset_lock_all(dev);
> > +	drm_modeset_lock_fini(&plane->mutex);
> > +
> >   	kfree(plane->format_types);
> >   	drm_mode_object_unregister(dev, &plane->base);
> > @@ -236,7 +237,6 @@ void drm_plane_cleanup(struct drm_plane *plane)
> >   	dev->mode_config.num_total_plane--;
> >   	if (plane->type == DRM_PLANE_TYPE_OVERLAY)
> >   		dev->mode_config.num_overlay_plane--;
> > -	drm_modeset_unlock_all(dev);
> >   	WARN_ON(plane->state && !plane->funcs->atomic_destroy_state);
> >   	if (plane->state && plane->funcs->atomic_destroy_state)
>
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
index 5c067719164d..992879f15f23 100644
--- a/drivers/gpu/drm/drm_encoder.c
+++ b/drivers/gpu/drm/drm_encoder.c
@@ -110,11 +110,9 @@  int drm_encoder_init(struct drm_device *dev,
 {
 	int ret;
 
-	drm_modeset_lock_all(dev);
-
 	ret = drm_mode_object_get(dev, &encoder->base, DRM_MODE_OBJECT_ENCODER);
 	if (ret)
-		goto out_unlock;
+		return ret;
 
 	encoder->dev = dev;
 	encoder->encoder_type = encoder_type;
@@ -142,9 +140,6 @@  int drm_encoder_init(struct drm_device *dev,
 	if (ret)
 		drm_mode_object_unregister(dev, &encoder->base);
 
-out_unlock:
-	drm_modeset_unlock_all(dev);
-
 	return ret;
 }
 EXPORT_SYMBOL(drm_encoder_init);
@@ -164,12 +159,10 @@  void drm_encoder_cleanup(struct drm_encoder *encoder)
 	 * the indices on the drm_encoder after us in the encoder_list.
 	 */
 
-	drm_modeset_lock_all(dev);
 	drm_mode_object_unregister(dev, &encoder->base);
 	kfree(encoder->name);
 	list_del(&encoder->head);
 	dev->mode_config.num_encoder--;
-	drm_modeset_unlock_all(dev);
 
 	memset(encoder, 0, sizeof(*encoder));
 }
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 419ac313c36f..9147aab182c4 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -221,7 +221,8 @@  void drm_plane_cleanup(struct drm_plane *plane)
 {
 	struct drm_device *dev = plane->dev;
 
-	drm_modeset_lock_all(dev);
+	drm_modeset_lock_fini(&plane->mutex);
+
 	kfree(plane->format_types);
 	drm_mode_object_unregister(dev, &plane->base);
 
@@ -236,7 +237,6 @@  void drm_plane_cleanup(struct drm_plane *plane)
 	dev->mode_config.num_total_plane--;
 	if (plane->type == DRM_PLANE_TYPE_OVERLAY)
 		dev->mode_config.num_overlay_plane--;
-	drm_modeset_unlock_all(dev);
 
 	WARN_ON(plane->state && !plane->funcs->atomic_destroy_state);
 	if (plane->state && plane->funcs->atomic_destroy_state)