Re: [PATCH 09/11] drm: Amend connector/encoder list locking rules

[Daniel Vetter <daniel@ffwll.ch>]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx