On Wed, 2019-10-23 at 16:23 +0300, Jani Nikula wrote:
> On Wed, 23 Oct 2019, Ramalingam C <ramalingam.c@xxxxxxxxx> 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@xxxxxxxxx>
> > > Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > Cc: Martin Peres <martin.peres@xxxxxxxxxxxxxxx>
> > > Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > > ---
> > > 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.
>
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
