[09/11] drm: Amend connector/encoder list locking rules
diff mbox

Message ID 1435092362-31062-10-git-send-email-daniel.vetter@ffwll.ch
State New
Headers show

Commit Message

Daniel Vetter June 23, 2015, 8:46 p.m. UTC
Now that dp mst hotplug takes all locks we can amend the locking rules
for the iterators. This is needed before we can roll these out in the
atomic code to avoid getting burried in WARNINGs.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 include/drm/drm_crtc.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Chris Wilson June 24, 2015, 7:57 a.m. UTC | #1
On Tue, Jun 23, 2015 at 10:46:00PM +0200, Daniel Vetter wrote:
> Now that dp mst hotplug takes all locks we can amend the locking rules
> for the iterators. This is needed before we can roll these out in the
> atomic code to avoid getting burried in WARNINGs.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  include/drm/drm_crtc.h | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 3646c47b43de..c1c4f16f01ca 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1588,14 +1588,16 @@ static inline struct drm_property *drm_property_find(struct drm_device *dev,
>  	list_for_each_entry(crtc, &(dev)->mode_config.crtc_list, head)
>  
>  #define drm_for_each_connector(connector, dev) \
> -	for (WARN_ON(!mutex_is_locked(&(dev)->mode_config.mutex)),		\
> +	for (WARN_ON(!mutex_is_locked(&(dev)->mode_config.mutex) &&		\
> +		     !drm_modeset_is_locked(&(dev)->mode_config.connection_mutex)), \
>  	     connector = list_first_entry(&(dev)->mode_config.connector_list,	\
>  					  struct drm_connector, head);		\
>  	     &connector->head != (&(dev)->mode_config.connector_list);		\
>  	     connector = list_next_entry(connector, head))
>  
>  #define drm_for_each_encoder(encoder, dev) \
> -	for (WARN_ON(!mutex_is_locked(&(dev)->mode_config.mutex)),		\
> +	for (WARN_ON(!mutex_is_locked(&(dev)->mode_config.mutex) &&		\
> +		     !drm_modeset_is_locked(&(dev)->mode_config.connection_mutex)), \
>  	     encoder = list_first_entry(&(dev)->mode_config.encoder_list,	\
>  					  struct drm_encoder, head);		\
>  	     &encoder->head != (&(dev)->mode_config.encoder_list);		\

Maybe something like:

	// not sure how specifc each lock will end up being
	static inline void assert_drm_mode_list_locked(dev)
	{
		/* lockdep for accuracy and verbose debug reportts,
		 * WARN_ON for warnings everywhere.
		 */
		lockdep_assert_held(&dev->mode_config.mutex);
		WARN_ON(!(mutex_is_locked(&dev->mode_config.mutex) &&
			  drm_modeset_is_locked(&dev->mode_config.connection_mutex)));

		// Daniel, it is not clear from context whether you mean
		// !a && !b, or !(a && b)
	}

	static inline struct drm_connector *drm_first_connector(dev)
	{
		assert_drm_mode_list_locked(dev);
		return list_first_entry(&dev->mode_config.connector_list,
					struct drm_connector, head);
	}

	#define drm_for_each_connector(connector, dev) \
		for (connector = drm_first_connector(dev); \
		     &connector->head != (&(dev)->mode_config.connector_list);		\
		     connector = list_next_entry(connector, head))

	static inline struct drm_encoder *drm_first_encoder(dev)
	{
		assert_drm_mode_list_locked(dev);
		return list_first_entry(&dev->mode_config.encoder_list,
					struct drm_encoder, head);
	}

	#define drm_for_each_encoder(encoder, dev) \
		for (encoder = drm_first_encoder(dev); \
		     &encoder->head != (&(dev)->mode_config.encoder_list);		\
		     encoder = list_next_entry(encoder, head))

Usual arguments of reusing code for clarity.
-Chris
Daniel Vetter June 24, 2015, 8:36 a.m. UTC | #2
On Wed, Jun 24, 2015 at 08:57:23AM +0100, Chris Wilson wrote:
> On Tue, Jun 23, 2015 at 10:46:00PM +0200, Daniel Vetter wrote:
> > Now that dp mst hotplug takes all locks we can amend the locking rules
> > for the iterators. This is needed before we can roll these out in the
> > atomic code to avoid getting burried in WARNINGs.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  include/drm/drm_crtc.h | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index 3646c47b43de..c1c4f16f01ca 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -1588,14 +1588,16 @@ static inline struct drm_property *drm_property_find(struct drm_device *dev,
> >  	list_for_each_entry(crtc, &(dev)->mode_config.crtc_list, head)
> >  
> >  #define drm_for_each_connector(connector, dev) \
> > -	for (WARN_ON(!mutex_is_locked(&(dev)->mode_config.mutex)),		\
> > +	for (WARN_ON(!mutex_is_locked(&(dev)->mode_config.mutex) &&		\
> > +		     !drm_modeset_is_locked(&(dev)->mode_config.connection_mutex)), \
> >  	     connector = list_first_entry(&(dev)->mode_config.connector_list,	\
> >  					  struct drm_connector, head);		\
> >  	     &connector->head != (&(dev)->mode_config.connector_list);		\
> >  	     connector = list_next_entry(connector, head))
> >  
> >  #define drm_for_each_encoder(encoder, dev) \
> > -	for (WARN_ON(!mutex_is_locked(&(dev)->mode_config.mutex)),		\
> > +	for (WARN_ON(!mutex_is_locked(&(dev)->mode_config.mutex) &&		\
> > +		     !drm_modeset_is_locked(&(dev)->mode_config.connection_mutex)), \
> >  	     encoder = list_first_entry(&(dev)->mode_config.encoder_list,	\
> >  					  struct drm_encoder, head);		\
> >  	     &encoder->head != (&(dev)->mode_config.encoder_list);		\
> 
> Maybe something like:
> 
> 	// not sure how specifc each lock will end up being
> 	static inline void assert_drm_mode_list_locked(dev)
> 	{
> 		/* lockdep for accuracy and verbose debug reportts,
> 		 * WARN_ON for warnings everywhere.
> 		 */
> 		lockdep_assert_held(&dev->mode_config.mutex);
> 		WARN_ON(!(mutex_is_locked(&dev->mode_config.mutex) &&
> 			  drm_modeset_is_locked(&dev->mode_config.connection_mutex)));
> 
> 		// Daniel, it is not clear from context whether you mean
> 		// !a && !b, or !(a && b)

Write access holds both locks, which means for read access it's ok to just
hold one. But since that's caused confusions I fully agree that extracting
this into a static inline and commenting properly is a good idea.
-Daniel

> 	}
> 
> 	static inline struct drm_connector *drm_first_connector(dev)
> 	{
> 		assert_drm_mode_list_locked(dev);
> 		return list_first_entry(&dev->mode_config.connector_list,
> 					struct drm_connector, head);
> 	}
> 
> 	#define drm_for_each_connector(connector, dev) \
> 		for (connector = drm_first_connector(dev); \
> 		     &connector->head != (&(dev)->mode_config.connector_list);		\
> 		     connector = list_next_entry(connector, head))
> 
> 	static inline struct drm_encoder *drm_first_encoder(dev)
> 	{
> 		assert_drm_mode_list_locked(dev);
> 		return list_first_entry(&dev->mode_config.encoder_list,
> 					struct drm_encoder, head);
> 	}
> 
> 	#define drm_for_each_encoder(encoder, dev) \
> 		for (encoder = drm_first_encoder(dev); \
> 		     &encoder->head != (&(dev)->mode_config.encoder_list);		\
> 		     encoder = list_next_entry(encoder, head))
> 
> Usual arguments of reusing code for clarity.
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patch
diff mbox

diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 3646c47b43de..c1c4f16f01ca 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1588,14 +1588,16 @@  static inline struct drm_property *drm_property_find(struct drm_device *dev,
 	list_for_each_entry(crtc, &(dev)->mode_config.crtc_list, head)
 
 #define drm_for_each_connector(connector, dev) \
-	for (WARN_ON(!mutex_is_locked(&(dev)->mode_config.mutex)),		\
+	for (WARN_ON(!mutex_is_locked(&(dev)->mode_config.mutex) &&		\
+		     !drm_modeset_is_locked(&(dev)->mode_config.connection_mutex)), \
 	     connector = list_first_entry(&(dev)->mode_config.connector_list,	\
 					  struct drm_connector, head);		\
 	     &connector->head != (&(dev)->mode_config.connector_list);		\
 	     connector = list_next_entry(connector, head))
 
 #define drm_for_each_encoder(encoder, dev) \
-	for (WARN_ON(!mutex_is_locked(&(dev)->mode_config.mutex)),		\
+	for (WARN_ON(!mutex_is_locked(&(dev)->mode_config.mutex) &&		\
+		     !drm_modeset_is_locked(&(dev)->mode_config.connection_mutex)), \
 	     encoder = list_first_entry(&(dev)->mode_config.encoder_list,	\
 					  struct drm_encoder, head);		\
 	     &encoder->head != (&(dev)->mode_config.encoder_list);		\