| Message ID | 20191019004124.371929-1-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] drm/i915/display: Handle fused off display correctly | expand |
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 */
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
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.
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
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. > > >
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 */
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(-)