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