Re: [PATCH 5/5] drm/i915: Implement Link Rate fallback on Link training failure

[Manasi Navare <manasi.d.navare@intel.com>]

On Fri, Nov 11, 2016 at 04:08:26PM +0200, Ville Syrjälä wrote:
> 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.
>

I can move the moderevalidation to the core as well, but then the function name
has to be something else than just drm_set_link_status_property(),cant think
of a name that combines mode revalidation and setting link sttaus property,
any suggestions?

Manasi 
> > - 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@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx