[Intel-gfx] [PATCH 1/5] drm/i915/display: Handle fused off display correctly

[Jani Nikula]

On Fri, 18 Oct 2019, José Roberto de Souza <jose.souza at intel.com> wrote:
> If all pipes are fused off it means that display is disabled, similar
> like we handle for GEN 7 and 8 right above but for GEN9+ spec says
> that hardware will override the pipe output to a solid color, so
> some display is there and maybe we would need to shutdown display
> to save power, so setting disable_display = true, to keep consistent
> to HAS_DISPLAY() and INTEL_DISPLAY_ENABLED().
>
> In addition to have all pipes fused off, GEN/display 9 have the
> bit 30 "Internal Display Disable", not sure if all pipes will be set
> as unfused when this bit is set so handling both.
>
> Cc: Jani Nikula <jani.nikula at intel.com>
> Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Cc: Martin Peres <martin.peres at linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h          | 21 +++++++++++----------
>  drivers/gpu/drm/i915/intel_device_info.c | 14 ++++++++++----
>  2 files changed, 21 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 855db888516c..6e3ae6e9cbb8 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7651,16 +7651,17 @@ enum {
>  #define   MASK_WAKEMEM			(1 << 13)
>  #define   CNL_DDI_CLOCK_REG_ACCESS_ON	(1 << 7)
>  
> -#define SKL_DFSM			_MMIO(0x51000)
> -#define SKL_DFSM_CDCLK_LIMIT_MASK	(3 << 23)
> -#define SKL_DFSM_CDCLK_LIMIT_675	(0 << 23)
> -#define SKL_DFSM_CDCLK_LIMIT_540	(1 << 23)
> -#define SKL_DFSM_CDCLK_LIMIT_450	(2 << 23)
> -#define SKL_DFSM_CDCLK_LIMIT_337_5	(3 << 23)
> -#define SKL_DFSM_PIPE_A_DISABLE		(1 << 30)
> -#define SKL_DFSM_PIPE_B_DISABLE		(1 << 21)
> -#define SKL_DFSM_PIPE_C_DISABLE		(1 << 28)
> -#define TGL_DFSM_PIPE_D_DISABLE		(1 << 22)
> +#define SKL_DFSM				_MMIO(0x51000)
> +#define SKL_DFSM_INTERNAL_DISPLAY_DISABLE	(1 << 30)
> +#define SKL_DFSM_CDCLK_LIMIT_MASK		(3 << 23)
> +#define SKL_DFSM_CDCLK_LIMIT_675		(0 << 23)
> +#define SKL_DFSM_CDCLK_LIMIT_540		(1 << 23)
> +#define SKL_DFSM_CDCLK_LIMIT_450		(2 << 23)
> +#define SKL_DFSM_CDCLK_LIMIT_337_5		(3 << 23)
> +#define SKL_DFSM_PIPE_A_DISABLE			(1 << 30)
> +#define SKL_DFSM_PIPE_B_DISABLE			(1 << 21)
> +#define SKL_DFSM_PIPE_C_DISABLE			(1 << 28)
> +#define TGL_DFSM_PIPE_D_DISABLE			(1 << 22)
>  
>  #define SKL_DSSM				_MMIO(0x51004)
>  #define CNL_DSSM_CDCLK_PLL_REFCLK_24MHz		(1 << 31)
> diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
> index 85e480bdc673..8d6492afdd6a 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.c
> +++ b/drivers/gpu/drm/i915/intel_device_info.c
> @@ -972,15 +972,21 @@ void intel_device_info_runtime_init(struct drm_i915_private *dev_priv)
>  			enabled_mask &= ~BIT(PIPE_D);
>  
>  		/*
> -		 * At least one pipe should be enabled and if there are
> -		 * disabled pipes, they should be the last ones, with no holes
> -		 * in the mask.
> +		 * If there are disabled pipes, they should be the last ones,
> +		 * with no holes in the mask.
>  		 */
> -		if (enabled_mask == 0 || !is_power_of_2(enabled_mask + 1))
> +		if (enabled_mask && !is_power_of_2(enabled_mask + 1))
>  			DRM_ERROR("invalid pipe fuse configuration: enabled_mask=0x%x\n",
>  				  enabled_mask);
>  		else
>  			info->pipe_mask = enabled_mask;
> +
> +		if ((INTEL_GEN(dev_priv) == 9 && !IS_GEMINILAKE(dev_priv)) &&
> +		    (dfsm & SKL_DFSM_INTERNAL_DISPLAY_DISABLE))
> +			i915_modparams.disable_display = true;
> +
> +		if (!enabled_mask)
> +			i915_modparams.disable_display = true;

Hrmh, I really hate the approach of setting module parameters from the
kernel. Including the "sanitizing" them. IMO they should only ever be
set or modified by userspace.

BR,
Jani.

>  	}
>  
>  	/* Initialize slice/subslice/EU info */

-- 
Jani Nikula, Intel Open Source Graphics Center