Re: [Intel-gfx] [PATCH] drm/i915/vlv: Enable polling when we shut off all power domains

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Apr 21, 2016 at 11:44:48AM -0400, Lyude wrote:
> Unfortunately HPD isn't functional once we shut off all of the power
> domains. Unfortunately we can end up shutting off all of the power
> domains in any situation where we don't have any monitors connected,
> essentially breaking hpd for the user unless they reboot with one of
> their monitors connected.
> 
> In addition, enabling polling has to be done in it's own seperate
> worker. The reason for this is that vlv_display_power_well_init/deinit()
> will get called during the DRM polling process and try to grab the locks
> required for turning on polling and cause a deadlock.
> 
> This breaks runtime PM due to the constant wakeups, so this is more of a
> temporary workaround then a solution.
> 
> Signed-off-by: Lyude <cpaul@redhat.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/i915/intel_display.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 551541b303..f644814 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14531,7 +14531,22 @@ static void intel_setup_outputs(struct drm_device *dev)
>  		    intel_dp_is_edp(dev, PORT_C))
>  			intel_dp_init(dev, VLV_DP_C, PORT_C);
>  
> -		if (IS_CHERRYVIEW(dev)) {
> +		if (IS_VALLEYVIEW(dev)) {
> +			struct intel_connector *connector;
> +
> +			/*
> +			 * On vlv, turning off all of the power domains results
> +			 * in a loss of hpd, enable polling on all of the
> +			 * connectors so that drm polls them when this happens
> +			 */
> +			drm_modeset_lock(&dev->mode_config.connection_mutex,
> +					 NULL);
> +			for_each_intel_connector(dev, connector) {
> +				connector->polled = DRM_CONNECTOR_POLL_CONNECT |
> +					DRM_CONNECTOR_POLL_DISCONNECT;
> +			}
> +			drm_modeset_unlock(&dev->mode_config.connection_mutex);
> +		} else if (IS_CHERRYVIEW(dev)) {

Ok, so this is just review on this patch. Imo what we need to avoid
polling all the time is a intel_hpd_polling() function or similar, which
sets drm_connector->polled to enable polling. We need to insert this in
the right places, i.e. in intel_runtime_suspend for !VLV && !CHV and in
vlv_display_power_well_disable for VLV && CHV. In short exactly symmetric
to how we call intel_hpd_init() on the enable/resume side.

intel_hpd_init() already disables polling again on its own (as long as we
only touch drm_connector->polled and not intel_connector->polled), which
means polling will only be enabled for exactly as much time as we need.

The other bit we need is to re-enabled the poll worker from that new
intel_hpd_polling() functions. We can't just call
drm_kms_helper_poll_enable because that requires a mutex. Instead the
simplest trick is to just schedule the poll work directly:

schedule_delayed_work(&dev->mode_config.output_poll_work, DRM_OUTPUT_POLL_PERIOD);

The simplest solution tbh would be to simplify drm_kms_helper_poll_enable
to drop the locking and just do this directly. The worker will
self-disable if it's not needed.

This would at least fix the "always polls" problem your patch has. We'd
still have the problem with irq storms happening due to the power well
switching all the time. But sounds like Ville has at least a partial
solution for that.
-Daniel

>  			/* eDP not supported on port D, so don't check VBT */
>  			if (I915_READ(CHV_HDMID) & SDVO_DETECTED)
>  				intel_hdmi_init(dev, CHV_HDMID, PORT_D);
> -- 
> 2.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel