On Thu, Nov 10, 2016 at 09:58:31PM +0100, Daniel Vetter wrote:
> On Wed, Nov 09, 2016 at 08:42:08PM -0800, Manasi Navare wrote:
> > @@ -5692,6 +5751,39 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
> > return false;
> > }
> >
> > +static void intel_dp_modeset_retry_work_fn(struct work_struct *work)
> > +{
> > + struct intel_connector *intel_connector;
> > + struct drm_connector *connector;
> > + struct drm_display_mode *mode;
> > + bool verbose_prune = true;
> > +
> > + intel_connector = container_of(work, typeof(*intel_connector),
> > + modeset_retry_work);
> > + connector = &intel_connector->base;
> > +
> > + /* Grab the locks before changing connector property*/
> > + mutex_lock(&connector->dev->mode_config.mutex);
> > + DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id,
> > + connector->name);
> > + list_for_each_entry(mode, &connector->modes, head) {
> > + mode->status = intel_dp_mode_valid(connector,
> > + mode);
> > + }
> > + drm_mode_prune_invalid(connector->dev, &connector->modes,
> > + verbose_prune);
> > +
> > + /* Set connector link status to BAD and send a Uevent to notify
> > + * userspace to do a modeset.
> > + */
> > + intel_dp_set_link_status_property(connector,
> > + DRM_MODE_LINK_STATUS_BAD);
> > + mutex_unlock(&connector->dev->mode_config.mutex);
> > +
> > + /* Send Hotplug uevent so userspace can reprobe */
> > + drm_kms_helper_hotplug_event(connector->dev);
>
> One downside of keeping all the signalling logic here in i915 is that we
> don't have a good spot to put the kerneldoc for this new link status
> property within drm_connector.c for others to easily spot. My suggestion
> would be to extract function with the following rough pseudo-code:
>
> drm_connector_set_link_status(connector, status)
> {
> mutex_lock(mode_config.mutex);
>
> /* what intel_dp_set_link_status_property() does */
>
> mutex_unlock(mode_config.mutex);
> drm_kms_helper_hotplug_event()
> }
>
> Within intel_dp_modeset_retry_work_fn we'd then first need to drop the
> lock, and then call this function. The lock drop&reacquire is a bit ugly,
> but:
The mode re-validation should be done in the core as well. Not sure if
we could just stuff it all into one place, and then there would be no
need for any lock dropping.
> - it doesn't matter from a performance pov, this is a slow, asynchronous
> work.
> - it doesn't matter for correctnes, the only thing we need is to drop the
> lock before calling drm_kms_helper_hotplug_event, and that we update the
> link status and the mode list before that too.
> - long term I expect that properties will get separate locks to protect
> their values, and restrict mode_config.mutex to just mode probing. Which
> means drm_connnector_set_link_status will use a different lock anyway.
> - it nicely encapsulates stuff imo.
>
> Kerneldoc for drm_connector_set_link_status should mention that this needs
> to be run from some async work item, and ofc have the full explanation
> (maybe even quote some pseudo-code, see e.g. drm_modeset_lock.c comments)
> of how this should be used.
>
> Since this is late-stage polish definitely wait for more feedback and for
> the design to fully settle first.
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
