| Message ID | 1435092362-31062-10-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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); \
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(-)