[Intel-gfx] [PATCH 5/5] drm/i915: Implement Link Rate fallback on Link training failure

[Jani Nikula]

On Thu, 10 Nov 2016, Daniel Vetter <daniel at ffwll.ch> 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:

Thanks for this. I wanted Manasi to keep the work struct and function
within i915, but I fell short of coming up with this division.

BR,
Jani.



>
> 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:
> - 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

-- 
Jani Nikula, Intel Open Source Technology Center