[1/5] drm/i915/display: Handle fused off display correctly
diff mbox series

Message ID 20191019004124.371929-1-jose.souza@intel.com
State New
Headers show
Series
  • [1/5] drm/i915/display: Handle fused off display correctly
Related show

Commit Message

José Roberto de Souza Oct. 19, 2019, 12:41 a.m. UTC
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@intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Martin Peres <martin.peres@linux.intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@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(-)

Comments

Jani Nikula Oct. 23, 2019, 12:15 p.m. UTC | #1
On Fri, 18 Oct 2019, José Roberto de Souza <jose.souza@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@intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Martin Peres <martin.peres@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@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 */
Ramalingam C Oct. 23, 2019, 1:18 p.m. UTC | #2
On 2019-10-18 at 17:41:20 -0700, José Roberto de Souza 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@intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Martin Peres <martin.peres@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@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;
Do we really need to set the disable_display here? on Gen 7 and 8 when
it is fused off, we were setting pipe_mask to 0. why that wont work
here?

INTEL_NUM_PIPES and HAS_DISPLAY both are based on pipe_mask only.

-Ram
>  	}
>  
>  	/* Initialize slice/subslice/EU info */
> -- 
> 2.23.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Jani Nikula Oct. 23, 2019, 1:23 p.m. UTC | #3
On Wed, 23 Oct 2019, Ramalingam C <ramalingam.c@intel.com> wrote:
> On 2019-10-18 at 17:41:20 -0700, José Roberto de Souza 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@intel.com>
>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Martin Peres <martin.peres@linux.intel.com>
>> Signed-off-by: José Roberto de Souza <jose.souza@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;
> Do we really need to set the disable_display here? on Gen 7 and 8 when
> it is fused off, we were setting pipe_mask to 0. why that wont work
> here?
>
> INTEL_NUM_PIPES and HAS_DISPLAY both are based on pipe_mask only.

Indeed that's one of the problematic features of the patch; the
->pipe_mask won't reflect reality. But then INTEL_DISPLAY_ENABLED()
assumes you *do* have display...

BR,
Jani.
Ramalingam C Oct. 23, 2019, 1:43 p.m. UTC | #4
On 2019-10-18 at 17:41:20 -0700, José Roberto de Souza 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@intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Martin Peres <martin.peres@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@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)
As we are touching all of these anyway, we can add two char space
before the bit definitions as usual.

-Ram
> +#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;
>  	}
>  
>  	/* Initialize slice/subslice/EU info */
> -- 
> 2.23.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
José Roberto de Souza Oct. 23, 2019, 7:13 p.m. UTC | #5
On Wed, 2019-10-23 at 16:23 +0300, Jani Nikula wrote:
> On Wed, 23 Oct 2019, Ramalingam C <ramalingam.c@intel.com> wrote:
> > On 2019-10-18 at 17:41:20 -0700, José Roberto de Souza 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@intel.com>
> > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Martin Peres <martin.peres@linux.intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@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;
> > Do we really need to set the disable_display here? on Gen 7 and 8
> > when
> > it is fused off, we were setting pipe_mask to 0. why that wont work
> > here?
> > 
> > INTEL_NUM_PIPES and HAS_DISPLAY both are based on pipe_mask only.

Like said in the commit description, GEN9+ even when fused off the pipe
still outputs some solid color so in future we would need to shut it
down using INTEL_DISPLAY_ENABLED().

> 
> Indeed that's one of the problematic features of the patch; the
> ->pipe_mask won't reflect reality. But then INTEL_DISPLAY_ENABLED()
> assumes you *do* have display...

In the other comment that you left, you said that you don't like the
idea of change module parameters, so the other option would be add a
has_display to intel_device_info, what do you think?

> 
> BR,
> Jani.
> 
> 
>

Patch
diff mbox series

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;
 	}
 
 	/* Initialize slice/subslice/EU info */