Thanks
Reviewed-by: Vinod Govindapillai <vinod.govindapillai@xxxxxxxxx>
BR
Vinod
On Wed, 2023-01-25 at 20:52 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> At least on some platforms (tested on ctg) the way
> vgacon does screen blanking seems to flag constant
> FIFO underruns, which means we have to be prepared
> for them while the driver is loading. Currently
> there is a time window between drm_crtc_init() and
> intel_sanitize_fifo_underrun_reporting() during
> which FIFO underrun reporting is in fact marked as
> enabled. Thus we may end up mistakenly detecting
> these bogus underruns during driver init.
>
> Close the race by marking FIFO underrun reporting
> as disabled prior to even registering the crtc.
> intel_sanitize_fifo_underrun_reporting()/etc. will
> re-enable it later if needed.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/display/intel_crtc.c | 3 +++
> .../drm/i915/display/intel_fifo_underrun.c | 20 ++++++++++++++++
> .../drm/i915/display/intel_fifo_underrun.h | 3 +++
> .../drm/i915/display/intel_modeset_setup.c | 24 +++++--------------
> 4 files changed, 32 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c
> index 82be0fbe9934..b79a8834559f 100644
> --- a/drivers/gpu/drm/i915/display/intel_crtc.c
> +++ b/drivers/gpu/drm/i915/display/intel_crtc.c
> @@ -25,6 +25,7 @@
> #include "intel_display_types.h"
> #include "intel_drrs.h"
> #include "intel_dsi.h"
> +#include "intel_fifo_underrun.h"
> #include "intel_pipe_crc.h"
> #include "intel_psr.h"
> #include "intel_sprite.h"
> @@ -314,6 +315,8 @@ int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe)
> }
> crtc->plane_ids_mask |= BIT(primary->id);
>
> + intel_init_fifo_underrun_reporting(dev_priv, crtc, false);
> +
> for_each_sprite(dev_priv, pipe, sprite) {
> struct intel_plane *plane;
>
> diff --git a/drivers/gpu/drm/i915/display/intel_fifo_underrun.c
> b/drivers/gpu/drm/i915/display/intel_fifo_underrun.c
> index d636d21fa9ce..b708a62e509a 100644
> --- a/drivers/gpu/drm/i915/display/intel_fifo_underrun.c
> +++ b/drivers/gpu/drm/i915/display/intel_fifo_underrun.c
> @@ -31,6 +31,7 @@
> #include "intel_display_types.h"
> #include "intel_fbc.h"
> #include "intel_fifo_underrun.h"
> +#include "intel_pch_display.h"
>
> /**
> * DOC: fifo underrun handling
> @@ -509,3 +510,22 @@ void intel_check_pch_fifo_underruns(struct drm_i915_private *dev_priv)
>
> spin_unlock_irq(&dev_priv->irq_lock);
> }
> +
> +void intel_init_fifo_underrun_reporting(struct drm_i915_private *i915,
> + struct intel_crtc *crtc,
> + bool enable)
> +{
> + crtc->cpu_fifo_underrun_disabled = !enable;
> +
> + /*
> + * We track the PCH trancoder underrun reporting state
> + * within the crtc. With crtc for pipe A housing the underrun
> + * reporting state for PCH transcoder A, crtc for pipe B housing
> + * it for PCH transcoder B, etc. LPT-H has only PCH transcoder A,
> + * and marking underrun reporting as disabled for the non-existing
> + * PCH transcoders B and C would prevent enabling the south
> + * error interrupt (see cpt_can_enable_serr_int()).
> + */
> + if (intel_has_pch_trancoder(i915, crtc->pipe))
> + crtc->pch_fifo_underrun_disabled = !enable;
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_fifo_underrun.h
> b/drivers/gpu/drm/i915/display/intel_fifo_underrun.h
> index 2e47d7d3c101..b00d8abebcf9 100644
> --- a/drivers/gpu/drm/i915/display/intel_fifo_underrun.h
> +++ b/drivers/gpu/drm/i915/display/intel_fifo_underrun.h
> @@ -9,8 +9,11 @@
> #include <linux/types.h>
>
> struct drm_i915_private;
> +struct intel_crtc;
> enum pipe;
>
> +void intel_init_fifo_underrun_reporting(struct drm_i915_private *i915,
> + struct intel_crtc *crtc, bool enable);
> bool intel_set_cpu_fifo_underrun_reporting(struct drm_i915_private *dev_priv,
> enum pipe pipe, bool enable);
> bool intel_set_pch_fifo_underrun_reporting(struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> index 52cdbd4fc2fa..be095327a9ba 100644
> --- a/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> +++ b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> @@ -21,6 +21,7 @@
> #include "intel_display.h"
> #include "intel_display_power.h"
> #include "intel_display_types.h"
> +#include "intel_fifo_underrun.h"
> #include "intel_modeset_setup.h"
> #include "intel_pch_display.h"
> #include "intel_pm.h"
> @@ -234,12 +235,9 @@ static void intel_sanitize_fifo_underrun_reporting(const struct
> intel_crtc_state
> struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> struct drm_i915_private *i915 = to_i915(crtc->base.dev);
>
> - if (!crtc_state->hw.active && !HAS_GMCH(i915))
> - return;
> -
> /*
> - * We start out with underrun reporting disabled to avoid races.
> - * For correct bookkeeping mark this on active crtcs.
> + * We start out with underrun reporting disabled on active
> + * pipes to avoid races.
> *
> * Also on gmch platforms we dont have any hardware bits to
> * disable the underrun reporting. Which means we need to start
> @@ -250,19 +248,9 @@ static void intel_sanitize_fifo_underrun_reporting(const struct
> intel_crtc_state
> * No protection against concurrent access is required - at
> * worst a fifo underrun happens which also sets this to false.
> */
> - crtc->cpu_fifo_underrun_disabled = true;
> -
> - /*
> - * We track the PCH trancoder underrun reporting state
> - * within the crtc. With crtc for pipe A housing the underrun
> - * reporting state for PCH transcoder A, crtc for pipe B housing
> - * it for PCH transcoder B, etc. LPT-H has only PCH transcoder A,
> - * and marking underrun reporting as disabled for the non-existing
> - * PCH transcoders B and C would prevent enabling the south
> - * error interrupt (see cpt_can_enable_serr_int()).
> - */
> - if (intel_has_pch_trancoder(i915, crtc->pipe))
> - crtc->pch_fifo_underrun_disabled = true;
> + intel_init_fifo_underrun_reporting(i915, crtc,
> + !crtc_state->hw.active &&
> + !HAS_GMCH(i915));
> }
>
> static void intel_sanitize_crtc(struct intel_crtc *crtc,
