| Message ID | 20200212152525.23108-1-stanislav.lisovskiy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] drm/i915: Ensure no conflicts with BIOS when updating Dbuf | expand |
On Wed, 12 Feb 2020, Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> wrote: > TGL BIOS seems to enable both DBuf slices ocasionally, depending > how many displays are connected, while i915 according to BSpec > was powering on S1 DBuf slice, until a modeset was done. > > This was causing a brief flash during the boot as we were > disabling slice, previously used by BIOS with that. > > To prevent this, now we are ensuring tht we are enabling > _at least_ one slice, but if there are more, let's not > power them off. > > Fixes: ff2cd8635e41 ("drm/i915: Correctly map DBUF slices to pipes") > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > --- > drivers/gpu/drm/i915/display/intel_display_power.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c > index 53056def5414..b9a9cbad8a03 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_power.c > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c > @@ -4470,11 +4470,13 @@ void icl_dbuf_slices_update(struct drm_i915_private *dev_priv, > > static void icl_dbuf_enable(struct drm_i915_private *dev_priv) > { > + skl_ddb_get_hw_state(dev_priv); > /* > - * Just power up 1 slice, we will > + * Just power up at least 1 slice, we will > * figure out later which slices we have and what we need. > */ > - icl_dbuf_slices_update(dev_priv, BIT(DBUF_S1)); > + icl_dbuf_slices_update(dev_priv, dev_priv->enabled_dbuf_slices_mask | > + BIT(DBUF_S1)); I don't know anything about this, but it seems obvious to me the enabling code should not do hardware readout; they should be separated from a much higher level. BR, Jani. > } > > static void icl_dbuf_disable(struct drm_i915_private *dev_priv)
On Wed, Feb 12, 2020 at 05:36:40PM +0200, Jani Nikula wrote: > On Wed, 12 Feb 2020, Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> wrote: > > TGL BIOS seems to enable both DBuf slices ocasionally, depending > > how many displays are connected, while i915 according to BSpec > > was powering on S1 DBuf slice, until a modeset was done. > > > > This was causing a brief flash during the boot as we were > > disabling slice, previously used by BIOS with that. > > > > To prevent this, now we are ensuring tht we are enabling > > _at least_ one slice, but if there are more, let's not > > power them off. > > > > Fixes: ff2cd8635e41 ("drm/i915: Correctly map DBUF slices to pipes") > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_display_power.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c > > index 53056def5414..b9a9cbad8a03 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_power.c > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c > > @@ -4470,11 +4470,13 @@ void icl_dbuf_slices_update(struct drm_i915_private *dev_priv, > > > > static void icl_dbuf_enable(struct drm_i915_private *dev_priv) > > { > > + skl_ddb_get_hw_state(dev_priv); > > /* > > - * Just power up 1 slice, we will > > + * Just power up at least 1 slice, we will > > * figure out later which slices we have and what we need. > > */ > > - icl_dbuf_slices_update(dev_priv, BIT(DBUF_S1)); > > + icl_dbuf_slices_update(dev_priv, dev_priv->enabled_dbuf_slices_mask | > > + BIT(DBUF_S1)); > > I don't know anything about this, but it seems obvious to me the > enabling code should not do hardware readout; they should be separated > from a much higher level. This is part of display core init, which is more or less half readout vs. half initialization anyway. So can't think of a better place for it really. What I don't like is that it's now only in the icl+ function but not in the pre-icl stuff. In fact I don't see why we even have this pre-icl vs. icl split anymore. The same code should work for both. So as a followup I'd like to see a patch to nuke the redundant code.
On Wed, 2020-02-12 at 17:36 +0200, Jani Nikula wrote: > On Wed, 12 Feb 2020, Stanislav Lisovskiy < > stanislav.lisovskiy@intel.com> wrote: > > TGL BIOS seems to enable both DBuf slices ocasionally, depending > > how many displays are connected, while i915 according to BSpec > > was powering on S1 DBuf slice, until a modeset was done. > > > > This was causing a brief flash during the boot as we were > > disabling slice, previously used by BIOS with that. > > > > To prevent this, now we are ensuring tht we are enabling > > _at least_ one slice, but if there are more, let's not > > power them off. > > > > Fixes: ff2cd8635e41 ("drm/i915: Correctly map DBUF slices to > > pipes") > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_display_power.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c > > b/drivers/gpu/drm/i915/display/intel_display_power.c > > index 53056def5414..b9a9cbad8a03 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_power.c > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c > > @@ -4470,11 +4470,13 @@ void icl_dbuf_slices_update(struct > > drm_i915_private *dev_priv, > > > > static void icl_dbuf_enable(struct drm_i915_private *dev_priv) > > { > > + skl_ddb_get_hw_state(dev_priv); > > /* > > - * Just power up 1 slice, we will > > + * Just power up at least 1 slice, we will > > * figure out later which slices we have and what we need. > > */ > > - icl_dbuf_slices_update(dev_priv, BIT(DBUF_S1)); > > + icl_dbuf_slices_update(dev_priv, dev_priv- > > >enabled_dbuf_slices_mask | > > + BIT(DBUF_S1)); > > I don't know anything about this, but it seems obvious to me the > enabling code should not do hardware readout; they should be > separated > from a much higher level. Current consensus with Ville is that there is no better place to stick it(could be moved to icl_core_init which calls that however not sure this is any better). That whole readout is simply done just because at this early stage we don't yet do a modeset neither have any info, which slices should be powered on, however if we enable only single slice as BSpec says, we might briefly introduce some screen glithes as BIOS could have used two slices already. Stan > > BR, > Jani. > > > > } > > > > static void icl_dbuf_disable(struct drm_i915_private *dev_priv) > >
diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c index 53056def5414..b9a9cbad8a03 100644 --- a/drivers/gpu/drm/i915/display/intel_display_power.c +++ b/drivers/gpu/drm/i915/display/intel_display_power.c @@ -4470,11 +4470,13 @@ void icl_dbuf_slices_update(struct drm_i915_private *dev_priv, static void icl_dbuf_enable(struct drm_i915_private *dev_priv) { + skl_ddb_get_hw_state(dev_priv); /* - * Just power up 1 slice, we will + * Just power up at least 1 slice, we will * figure out later which slices we have and what we need. */ - icl_dbuf_slices_update(dev_priv, BIT(DBUF_S1)); + icl_dbuf_slices_update(dev_priv, dev_priv->enabled_dbuf_slices_mask | + BIT(DBUF_S1)); } static void icl_dbuf_disable(struct drm_i915_private *dev_priv)
TGL BIOS seems to enable both DBuf slices ocasionally, depending how many displays are connected, while i915 according to BSpec was powering on S1 DBuf slice, until a modeset was done. This was causing a brief flash during the boot as we were disabling slice, previously used by BIOS with that. To prevent this, now we are ensuring tht we are enabling _at least_ one slice, but if there are more, let's not power them off. Fixes: ff2cd8635e41 ("drm/i915: Correctly map DBUF slices to pipes") Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> --- drivers/gpu/drm/i915/display/intel_display_power.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)