drm/i915: Force state->modeset=true when distrust_bios_wm==true
diff mbox series

Message ID 20200212150102.7600-1-ville.syrjala@linux.intel.com
State New
Headers show
Series
  • drm/i915: Force state->modeset=true when distrust_bios_wm==true
Related show

Commit Message

Ville Syrjälä Feb. 12, 2020, 3:01 p.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently when we load the driver we set distrust_bios_wm=true, which
will cause active_pipe_changes to get flagged even when we're not
toggling any pipes on/off. The reason being that we want to fully
redistribute the dbuf among the active pipes and ignore whatever
state the firmware left behind.

Unfortunately when the code flags active_pipe_changes it doesn't
set state->modeset to true, which means the hardware dbuf state
won't actually get updated. Hence the hardware and software
states go out of sync, which can result in planes trying to use a
disabled dbuf slice. Suprisingly that only seems to corrupt the
display rather than making the whole display engine keel over.

Let's fix this for now by flagging state->modeset whenever
distrust_bios_wm is set.

Eventually we'll likely want to rip out all of this mess and
introduce proper statye tracking for dbuf. But that requires
more work. Toss in a FIXME to that effect.

Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Fixes: ff2cd8635e41 ("drm/i915: Correctly map DBUF slices to pipes")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Stanislav Lisovskiy Feb. 12, 2020, 3:20 p.m. UTC | #1
On Wed, 2020-02-12 at 17:01 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Currently when we load the driver we set distrust_bios_wm=true, which
> will cause active_pipe_changes to get flagged even when we're not
> toggling any pipes on/off. The reason being that we want to fully
> redistribute the dbuf among the active pipes and ignore whatever
> state the firmware left behind.
> 
> Unfortunately when the code flags active_pipe_changes it doesn't
> set state->modeset to true, which means the hardware dbuf state
> won't actually get updated. Hence the hardware and software
> states go out of sync, which can result in planes trying to use a
> disabled dbuf slice. Suprisingly that only seems to corrupt the
> display rather than making the whole display engine keel over.
> 
> Let's fix this for now by flagging state->modeset whenever
> distrust_bios_wm is set.
> 
> Eventually we'll likely want to rip out all of this mess and
> introduce proper statye tracking for dbuf. But that requires
> more work. Toss in a FIXME to that effect.
> 
> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Fixes: ff2cd8635e41 ("drm/i915: Correctly map DBUF slices to pipes")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 61ba1f2256a0..9e4f99ae81fb 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -15027,6 +15027,20 @@ static int intel_atomic_check(struct
> drm_device *dev,
>  	if (new_cdclk_state && new_cdclk_state-
> >force_min_cdclk_changed)
>  		any_ms = true;
>  
> +	/*
> +	 * distrust_bios_wm will force a full dbuf recomputation
> +	 * but the hardware state will only get updated accordingly
> +	 * if state->modeset==true. Hence distrust_bios_wm==true &&
> +	 * state->modeset==false is an invalid combination which
> +	 * would cause the hardware and software dbuf state to get
> +	 * out of sync. We must prevent that.
> +	 *
> +	 * FIXME clean up this mess and introduce better
> +	 * state tracking for dbuf.

I disagree here in that sense, that this is actually not a DBUF
issue :) 
But merely state->modeset and active_pipe_changes going out of sync.
However refactoring here definitely will help to unite those
to somekind of unified new state. At least having this
active_pipe_changes seems to be redundant.

> +	 */
> +	if (dev_priv->wm.distrust_bios_wm)
> +		any_ms = true;
> +

Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>

>  	if (any_ms) {
>  		ret = intel_modeset_checks(state);
>  		if (ret)
Jani Nikula Feb. 13, 2020, 12:53 p.m. UTC | #2
On Wed, 12 Feb 2020, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Currently when we load the driver we set distrust_bios_wm=true, which
> will cause active_pipe_changes to get flagged even when we're not
> toggling any pipes on/off. The reason being that we want to fully
> redistribute the dbuf among the active pipes and ignore whatever
> state the firmware left behind.
>
> Unfortunately when the code flags active_pipe_changes it doesn't
> set state->modeset to true, which means the hardware dbuf state
> won't actually get updated. Hence the hardware and software
> states go out of sync, which can result in planes trying to use a
> disabled dbuf slice. Suprisingly that only seems to corrupt the
> display rather than making the whole display engine keel over.
>
> Let's fix this for now by flagging state->modeset whenever
> distrust_bios_wm is set.
>
> Eventually we'll likely want to rip out all of this mess and
> introduce proper statye tracking for dbuf. But that requires
> more work. Toss in a FIXME to that effect.

I have a hard time following all the implications of this change. Would
this under some circumstances lead to a case where we use the state read
at probe, and do a full modeset on that state?

DSC, lacking full state readout, fails badly with this change [1]. We'll
do DSC enable using a mostly zeroed out DSC config in state. Leading to
division by zero.

BR,
Jani.


[1] https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16544/fi-tgl-dsi/boot0.txt

>
> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Fixes: ff2cd8635e41 ("drm/i915: Correctly map DBUF slices to pipes")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 61ba1f2256a0..9e4f99ae81fb 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -15027,6 +15027,20 @@ static int intel_atomic_check(struct drm_device *dev,
>  	if (new_cdclk_state && new_cdclk_state->force_min_cdclk_changed)
>  		any_ms = true;
>  
> +	/*
> +	 * distrust_bios_wm will force a full dbuf recomputation
> +	 * but the hardware state will only get updated accordingly
> +	 * if state->modeset==true. Hence distrust_bios_wm==true &&
> +	 * state->modeset==false is an invalid combination which
> +	 * would cause the hardware and software dbuf state to get
> +	 * out of sync. We must prevent that.
> +	 *
> +	 * FIXME clean up this mess and introduce better
> +	 * state tracking for dbuf.
> +	 */
> +	if (dev_priv->wm.distrust_bios_wm)
> +		any_ms = true;
> +
>  	if (any_ms) {
>  		ret = intel_modeset_checks(state);
>  		if (ret)

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 61ba1f2256a0..9e4f99ae81fb 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -15027,6 +15027,20 @@  static int intel_atomic_check(struct drm_device *dev,
 	if (new_cdclk_state && new_cdclk_state->force_min_cdclk_changed)
 		any_ms = true;
 
+	/*
+	 * distrust_bios_wm will force a full dbuf recomputation
+	 * but the hardware state will only get updated accordingly
+	 * if state->modeset==true. Hence distrust_bios_wm==true &&
+	 * state->modeset==false is an invalid combination which
+	 * would cause the hardware and software dbuf state to get
+	 * out of sync. We must prevent that.
+	 *
+	 * FIXME clean up this mess and introduce better
+	 * state tracking for dbuf.
+	 */
+	if (dev_priv->wm.distrust_bios_wm)
+		any_ms = true;
+
 	if (any_ms) {
 		ret = intel_modeset_checks(state);
 		if (ret)