Re: [PATCH v1 6/6] drm/i915: Enable Display WA 0528

Re: [PATCH v1 6/6] drm/i915: Enable Display WA 0528

[Maarten Lankhorst]

Op 11-04-18 om 11:09 schreef Vidya Srinivas:
> Possible hang with NV12 plane surface formats.
> WA: When the plane source pixel format is NV12,
> the CHICKEN_PIPESL_* register bit 22 must be set to 1
> and the render decompression must not be enabled
> on any of the planes in that pipe.
The last part is still missing from this patch, you still need to reject CCS fb's?

And this should probably be patch 2.
> Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e4cf7a6..71eb49e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -511,12 +511,28 @@ skl_wa_clkgate(struct drm_i915_private *dev_priv, int pipe, bool enable)
>  		return;
>  
>  	if (enable)
> +		I915_WRITE(CHICKEN_PIPESL_1(pipe), HSW_FBCQ_DIS);
> +	else
> +		I915_WRITE(CHICKEN_PIPESL_1(pipe), ~HSW_FBCQ_DIS);
You're writing all bits except HSW_FBCQ_DIS here to the chicken register.. I don't think you want that. :)
> +
> +	POSTING_READ(CHICKEN_PIPESL_1(pipe));
> +}
> +
> +static void
> +skl_wa_clkgate(struct drm_i915_private *dev_priv, int pipe, bool enable)
> +{
> +	if (IS_SKYLAKE(dev_priv))
> +		return;
> +
> +	if (enable)
>  		I915_WRITE(CLKGATE_DIS_PSL(pipe),
>  			   DUPS1_GATING_DIS | DUPS2_GATING_DIS);
>  	else
>  		I915_WRITE(CLKGATE_DIS_PSL(pipe),
>  			   I915_READ(CLKGATE_DIS_PSL(pipe)) &
>  			   ~(DUPS1_GATING_DIS | DUPS2_GATING_DIS));
> +
> +	POSTING_READ(CLKGATE_DIS_PSL(pipe));
>  }
>  
>  static bool
> @@ -5202,8 +5218,10 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
>  
>  	/* Display WA 827 */
>  	if (needs_nv12_wa(dev_priv, old_crtc_state) &&
> -	    !needs_nv12_wa(dev_priv, pipe_config))
> +	    !needs_nv12_wa(dev_priv, pipe_config)) {
>  		skl_wa_clkgate(dev_priv, crtc->pipe, false);
> +		skl_wa_528(dev_priv, crtc->pipe, false);
> +	}
>  }
>  
>  static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
> @@ -5240,8 +5258,10 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
>  
>  	/* Display WA 827 */
>  	if (!needs_nv12_wa(dev_priv, old_crtc_state) &&
> -	    needs_nv12_wa(dev_priv, pipe_config))
> +	    needs_nv12_wa(dev_priv, pipe_config)) {
>  		skl_wa_clkgate(dev_priv, crtc->pipe, true);
> +		skl_wa_528(dev_priv, crtc->pipe, true);
> +	}
>  
>  	/*
>  	 * Vblank time updates from the shadow to live plane control register


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v1 6/6] drm/i915: Enable Display WA 0528

[Srinivas, Vidya]

> -----Original Message-----
> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com]
> Sent: Wednesday, April 11, 2018 3:09 PM
> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-
> gfx@lists.freedesktop.org
> Cc: Kamath, Sunil <sunil.kamath@intel.com>; Saarinen, Jani
> <jani.saarinen@intel.com>
> Subject: Re: [PATCH v1 6/6] drm/i915: Enable Display WA 0528
> 
> Op 11-04-18 om 11:09 schreef Vidya Srinivas:
> > Possible hang with NV12 plane surface formats.
> > WA: When the plane source pixel format is NV12, the CHICKEN_PIPESL_*
> > register bit 22 must be set to 1 and the render decompression must not
> > be enabled on any of the planes in that pipe.
> The last part is still missing from this patch, you still need to reject CCS fb's?


Thank you. Yeah, RC should not be enabled on the plane which has NV12 enabled. If we want
RC, then I need to set it to 22. Right now on APL I see RC was disabled at that time.
So I did not include this portion yet.

> 
> And this should probably be patch 2.
> > Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 24 ++++++++++++++++++++++--
> >  1 file changed, 22 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index e4cf7a6..71eb49e 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -511,12 +511,28 @@ skl_wa_clkgate(struct drm_i915_private
> *dev_priv, int pipe, bool enable)
> >  		return;
> >
> >  	if (enable)
> > +		I915_WRITE(CHICKEN_PIPESL_1(pipe), HSW_FBCQ_DIS);
> > +	else
> > +		I915_WRITE(CHICKEN_PIPESL_1(pipe), ~HSW_FBCQ_DIS);
> You're writing all bits except HSW_FBCQ_DIS here to the chicken register.. I
> don't think you want that. :)
Oops sorry, the name of the function was supposed to be skl_wa_528.
Thank you.

> > +
> > +	POSTING_READ(CHICKEN_PIPESL_1(pipe));
> > +}
> > +
> > +static void
> > +skl_wa_clkgate(struct drm_i915_private *dev_priv, int pipe, bool
> > +enable) {
> > +	if (IS_SKYLAKE(dev_priv))
> > +		return;
> > +
> > +	if (enable)
> >  		I915_WRITE(CLKGATE_DIS_PSL(pipe),
> >  			   DUPS1_GATING_DIS | DUPS2_GATING_DIS);
> >  	else
> >  		I915_WRITE(CLKGATE_DIS_PSL(pipe),
> >  			   I915_READ(CLKGATE_DIS_PSL(pipe)) &
> >  			   ~(DUPS1_GATING_DIS | DUPS2_GATING_DIS));
> > +
> > +	POSTING_READ(CLKGATE_DIS_PSL(pipe));
> >  }
> >
> >  static bool
> > @@ -5202,8 +5218,10 @@ static void intel_post_plane_update(struct
> > intel_crtc_state *old_crtc_state)
> >
> >  	/* Display WA 827 */
> >  	if (needs_nv12_wa(dev_priv, old_crtc_state) &&
> > -	    !needs_nv12_wa(dev_priv, pipe_config))
> > +	    !needs_nv12_wa(dev_priv, pipe_config)) {
> >  		skl_wa_clkgate(dev_priv, crtc->pipe, false);
> > +		skl_wa_528(dev_priv, crtc->pipe, false);
> > +	}
> >  }
> >
> >  static void intel_pre_plane_update(struct intel_crtc_state
> > *old_crtc_state, @@ -5240,8 +5258,10 @@ static void
> > intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
> >
> >  	/* Display WA 827 */
> >  	if (!needs_nv12_wa(dev_priv, old_crtc_state) &&
> > -	    needs_nv12_wa(dev_priv, pipe_config))
> > +	    needs_nv12_wa(dev_priv, pipe_config)) {
> >  		skl_wa_clkgate(dev_priv, crtc->pipe, true);
> > +		skl_wa_528(dev_priv, crtc->pipe, true);
> > +	}
> >
> >  	/*
> >  	 * Vblank time updates from the shadow to live plane control
> > register
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v1 5/6] drm/i915: Do not do fb src adjustments for NV12

[Maarten Lankhorst]

Op 12-04-18 om 12:07 schreef Srinivas, Vidya:
>
>> -----Original Message-----
>> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com]
>> Sent: Wednesday, April 11, 2018 4:08 PM
>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-gfx-
>> trybot@lists.freedesktop.org
>> Cc: Kamath, Sunil <sunil.kamath@intel.com>; Saarinen, Jani
>> <jani.saarinen@intel.com>
>> Subject: Re: [PATCH v1 5/6] drm/i915: Do not do fb src adjustments for
>> NV12
>>
>> Op 11-04-18 om 11:09 schreef Vidya Srinivas:
>>> We skip src trunction/adjustments for
>>> NV12 case and handle the sizes directly.
>>> Without this, pipe fifo underruns are seen on APL/KBL.
>>>
>>> Credits-to: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/intel_display.c | 88
>>> ++++++++++++++++++++++++++++++++++--
>>>  drivers/gpu/drm/i915/intel_sprite.c  | 10 +++-
>>>  2 files changed, 92 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>>> b/drivers/gpu/drm/i915/intel_display.c
>>> index ebb3f8e..e4cf7a6 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -12951,6 +12951,86 @@ skl_max_scale(struct intel_crtc *intel_crtc,
>>> }
>>>
>>>  static int
>>> +intel_primary_plane_state(struct drm_plane_state *plane_state,
>>> +			  const struct drm_crtc_state *crtc_state,
>>> +			  int min_scale, int max_scale,
>>> +			  bool can_position, bool can_update_disabled) {
>>> +	struct drm_framebuffer *fb = plane_state->fb;
>>> +	struct drm_rect *src = &plane_state->src;
>>> +	struct drm_rect *dst = &plane_state->dst;
>>> +	unsigned int rotation = plane_state->rotation;
>>> +	struct drm_rect clip = {};
>>> +	int hscale, vscale;
>>> +
>>> +	WARN_ON(plane_state->crtc && plane_state->crtc != crtc_state-
>>> crtc);
>>> +
>>> +	*src = drm_plane_state_src(plane_state);
>>> +	*dst = drm_plane_state_dest(plane_state);
>>> +
>>> +	if (!fb) {
>>> +		plane_state->visible = false;
>>> +		return 0;
>>> +	}
>>> +
>>> +	/* crtc should only be NULL when disabling (i.e., !fb) */
>>> +	if (WARN_ON(!plane_state->crtc)) {
>>> +		plane_state->visible = false;
>>> +		return 0;
>>> +	}
>>> +
>>> +	if (!crtc_state->enable && !can_update_disabled) {
>>> +		DRM_DEBUG_KMS("Cannot update plane of a disabled
>> CRTC.\n");
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	drm_rect_rotate(src, fb->width << 16, fb->height << 16, rotation);
>>> +
>>> +	/* Check scaling */
>>> +	hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale);
>>> +	vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale);
>>> +	if (hscale < 0 || vscale < 0) {
>>> +		DRM_DEBUG_KMS("Invalid scaling of plane\n");
>>> +		drm_rect_debug_print("src: ", &plane_state->src, true);
>>> +		drm_rect_debug_print("dst: ", &plane_state->dst, false);
>>> +		return -ERANGE;
>>> +	}
>>> +
>>> +	if (crtc_state->enable)
>>> +		drm_mode_get_hv_timing(&crtc_state->mode, &clip.x2,
>> &clip.y2);
>>> +
>>> +	if (fb->format->format == DRM_FORMAT_NV12) {
>>> +		plane_state->visible = true;
>>> +		goto skip_clip;
>>> +	}
>>> +
>>> +	plane_state->visible =
>>> +		drm_rect_clip_scaled(src, dst, &clip, hscale, vscale);
>> The real problem is that it needs to be a multiple of 4. I think the clipping
>> here is harmless, we should just adjust intel_check_sprite_plane for >= SKL.
>> This will make it so we don't have to duplicate its checks.
>>
>> For NV12 we still want to call clip_rect_scaled, but then adjust all
>> coordinates by dividing by 4 on before the check, and multiplying with 4
>> after?
>>
> Thank you. Have made the changes in https://patchwork.freedesktop.org/patch/216682/
> With this, I did not see any underruns. For now, have a WA only when we pass 16x16
> Because, with that it further clips down and gets rejected in skl_update_scaler as it is
> Less than 16. If we increase our buffer in igt, then this issue wont be there.
> Please have a check. Initially, I tried the /4 before the adjustments and *4 later, that wouldn’t work
> We need to have the 16.16 values multiplier of 4. So, just put it towards the end of both plane checks.	
Well, this is annoying.

It seems we never rejected subpixel precision before, and resorted to clipping instead of rejecting.
Could we be more strict about this without breaking existing userspace?

~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v1 5/6] drm/i915: Do not do fb src adjustments for NV12

[Srinivas, Vidya]

> -----Original Message-----
> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com]
> Sent: Thursday, April 12, 2018 4:41 PM
> To: Srinivas, Vidya <vidya.srinivas@intel.com>; Intel Graphics Development
> <intel-gfx@lists.freedesktop.org>; Ville Syrjälä
> <ville.syrjala@linux.intel.com>
> Cc: Kamath, Sunil <sunil.kamath@intel.com>; Saarinen, Jani
> <jani.saarinen@intel.com>
> Subject: Re: [PATCH v1 5/6] drm/i915: Do not do fb src adjustments for
> NV12
> 
> Op 12-04-18 om 12:07 schreef Srinivas, Vidya:
> >
> >> -----Original Message-----
> >> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com]
> >> Sent: Wednesday, April 11, 2018 4:08 PM
> >> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-gfx-
> >> trybot@lists.freedesktop.org
> >> Cc: Kamath, Sunil <sunil.kamath@intel.com>; Saarinen, Jani
> >> <jani.saarinen@intel.com>
> >> Subject: Re: [PATCH v1 5/6] drm/i915: Do not do fb src adjustments
> >> for
> >> NV12
> >>
> >> Op 11-04-18 om 11:09 schreef Vidya Srinivas:
> >>> We skip src trunction/adjustments for
> >>> NV12 case and handle the sizes directly.
> >>> Without this, pipe fifo underruns are seen on APL/KBL.
> >>>
> >>> Credits-to: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>> Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
> >>> ---
> >>>  drivers/gpu/drm/i915/intel_display.c | 88
> >>> ++++++++++++++++++++++++++++++++++--
> >>>  drivers/gpu/drm/i915/intel_sprite.c  | 10 +++-
> >>>  2 files changed, 92 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/intel_display.c
> >>> b/drivers/gpu/drm/i915/intel_display.c
> >>> index ebb3f8e..e4cf7a6 100644
> >>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>> @@ -12951,6 +12951,86 @@ skl_max_scale(struct intel_crtc
> >>> *intel_crtc, }
> >>>
> >>>  static int
> >>> +intel_primary_plane_state(struct drm_plane_state *plane_state,
> >>> +			  const struct drm_crtc_state *crtc_state,
> >>> +			  int min_scale, int max_scale,
> >>> +			  bool can_position, bool can_update_disabled) {
> >>> +	struct drm_framebuffer *fb = plane_state->fb;
> >>> +	struct drm_rect *src = &plane_state->src;
> >>> +	struct drm_rect *dst = &plane_state->dst;
> >>> +	unsigned int rotation = plane_state->rotation;
> >>> +	struct drm_rect clip = {};
> >>> +	int hscale, vscale;
> >>> +
> >>> +	WARN_ON(plane_state->crtc && plane_state->crtc != crtc_state-
> >>> crtc);
> >>> +
> >>> +	*src = drm_plane_state_src(plane_state);
> >>> +	*dst = drm_plane_state_dest(plane_state);
> >>> +
> >>> +	if (!fb) {
> >>> +		plane_state->visible = false;
> >>> +		return 0;
> >>> +	}
> >>> +
> >>> +	/* crtc should only be NULL when disabling (i.e., !fb) */
> >>> +	if (WARN_ON(!plane_state->crtc)) {
> >>> +		plane_state->visible = false;
> >>> +		return 0;
> >>> +	}
> >>> +
> >>> +	if (!crtc_state->enable && !can_update_disabled) {
> >>> +		DRM_DEBUG_KMS("Cannot update plane of a disabled
> >> CRTC.\n");
> >>> +		return -EINVAL;
> >>> +	}
> >>> +
> >>> +	drm_rect_rotate(src, fb->width << 16, fb->height << 16, rotation);
> >>> +
> >>> +	/* Check scaling */
> >>> +	hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale);
> >>> +	vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale);
> >>> +	if (hscale < 0 || vscale < 0) {
> >>> +		DRM_DEBUG_KMS("Invalid scaling of plane\n");
> >>> +		drm_rect_debug_print("src: ", &plane_state->src, true);
> >>> +		drm_rect_debug_print("dst: ", &plane_state->dst, false);
> >>> +		return -ERANGE;
> >>> +	}
> >>> +
> >>> +	if (crtc_state->enable)
> >>> +		drm_mode_get_hv_timing(&crtc_state->mode, &clip.x2,
> >> &clip.y2);
> >>> +
> >>> +	if (fb->format->format == DRM_FORMAT_NV12) {
> >>> +		plane_state->visible = true;
> >>> +		goto skip_clip;
> >>> +	}
> >>> +
> >>> +	plane_state->visible =
> >>> +		drm_rect_clip_scaled(src, dst, &clip, hscale, vscale);
> >> The real problem is that it needs to be a multiple of 4. I think the
> >> clipping here is harmless, we should just adjust intel_check_sprite_plane
> for >= SKL.
> >> This will make it so we don't have to duplicate its checks.
> >>
> >> For NV12 we still want to call clip_rect_scaled, but then adjust all
> >> coordinates by dividing by 4 on before the check, and multiplying
> >> with 4 after?
> >>
> > Thank you. Have made the changes in
> > https://patchwork.freedesktop.org/patch/216682/
> > With this, I did not see any underruns. For now, have a WA only when
> > we pass 16x16 Because, with that it further clips down and gets
> > rejected in skl_update_scaler as it is Less than 16. If we increase our
> buffer in igt, then this issue wont be there.
> > Please have a check. Initially, I tried the /4 before the adjustments and *4
> later, that wouldn’t work
> > We need to have the 16.16 values multiplier of 4. So, just put it towards
> the end of both plane checks.
> Well, this is annoying.
> 
> It seems we never rejected subpixel precision before, and resorted to
> clipping instead of rejecting.
> Could we be more strict about this without breaking existing userspace?
> 

Hi,
I am sorry. Not sure if I understood this clearly. The new data (fb info) association  is done in
check_primary_plane and check_sprite_plane just before the clipping/adjust. So, could not understand where
before in the code, we could reject.

With your suggestion of /4 and *4 implemented in https://patchwork.freedesktop.org/patch/216867/
All tests passed on both pipes on my APL without any underruns. Even IGT BAT resulted in PASS.
Can we not use this approach?

> ~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx