Re: [PATCH] drm/i915/display: Workaround for odd panning for planar yuv

[Jani Nikula <jani.nikula@linux.intel.com>]

Cc: Rodrigo and Lucas, note for you at the end.

On Fri, 06 Sep 2024, Nemesa Garg <nemesa.garg@intel.com> wrote:
> Disable the support for odd x pan for NV12 format as underrun
> issue is seen.
>
> WA: 16024459452
>
> Signed-off-by: Nemesa Garg <nemesa.garg@intel.com>
> ---
>  .../gpu/drm/i915/display/intel_atomic_plane.c    | 16 ++++++++++++++--
>  drivers/gpu/drm/i915/display/intel_display_wa.h  |  2 ++
>  drivers/gpu/drm/xe/display/xe_display_wa.c       |  5 +++++
>  3 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> index e979786aa5cf..9b17321f3477 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> @@ -47,11 +47,13 @@
>  #include "intel_display_rps.h"
>  #include "intel_display_trace.h"
>  #include "intel_display_types.h"
> +#include "intel_display_wa.h"
>  #include "intel_fb.h"
>  #include "intel_fb_pin.h"
>  #include "skl_scaler.h"
>  #include "skl_watermark.h"
>  
> +

Superfluous newline.

>  static void intel_plane_state_reset(struct intel_plane_state *plane_state,
>  				    struct intel_plane *plane)
>  {
> @@ -1029,8 +1031,18 @@ int intel_plane_check_src_coordinates(struct intel_plane_state *plane_state)
>  		 * This allows NV12 and P0xx formats to have odd size and/or odd
>  		 * source coordinates on DISPLAY_VER(i915) >= 20
>  		 */
> -		hsub = 1;
> -		vsub = 1;
> +
> +		/*
> +		 * Wa_16023981245 for display version 20.
> +		 * Do not support odd x-panning for NV12.
> +		 */
> +		if (intel_display_needs_wa_16023981245(i915) &&
> +		    fb->format->format == DRM_FORMAT_NV12) {
> +			vsub = 1;
> +		} else {
> +			hsub = 1;
> +			vsub = 1;
> +		}

Nitpick, the whole thing could be simplified to only touch hsub since
the w/a is about x-panning and vsub is the same in both branches.

>  	} else {
>  		hsub = fb->format->hsub;
>  		vsub = fb->format->vsub;
> diff --git a/drivers/gpu/drm/i915/display/intel_display_wa.h b/drivers/gpu/drm/i915/display/intel_display_wa.h
> index be644ab6ae00..9be35a751503 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_wa.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_wa.h
> @@ -14,8 +14,10 @@ void intel_display_wa_apply(struct drm_i915_private *i915);
>  
>  #ifdef I915
>  static inline bool intel_display_needs_wa_16023588340(struct drm_i915_private *i915) { return false; }
> +static inline bool intel_display_needs_wa_16023981245(struct drm_i915_private *i915) { return false; }
>  #else
>  bool intel_display_needs_wa_16023588340(struct drm_i915_private *i915);
> +bool intel_display_needs_wa_16023981245(struct drm_i915_private *i915);
>  #endif
>  
>  #endif
> diff --git a/drivers/gpu/drm/xe/display/xe_display_wa.c b/drivers/gpu/drm/xe/display/xe_display_wa.c
> index 68e3d1959ad6..fde4e09589a3 100644
> --- a/drivers/gpu/drm/xe/display/xe_display_wa.c
> +++ b/drivers/gpu/drm/xe/display/xe_display_wa.c
> @@ -14,3 +14,8 @@ bool intel_display_needs_wa_16023588340(struct drm_i915_private *i915)
>  {
>  	return XE_WA(xe_root_mmio_gt(i915), 16023588340);
>  }
> +
> +bool intel_display_needs_wa_16023981245(struct drm_i915_private *i915)
> +{
> +	return XE_WA(xe_root_mmio_gt(i915), 22019338487_display);

16023981245 vs 22019338487 and not explained in the commit message?!?

Rodrigo, Lucas, I think we're going to need to handle display
workarounds separately in i915 display. I'm fine with merging this now,
it's not a big deal, but this interface is not future compatible.

The first step could be simply converting these two to the old style
workarounds in i915 display, i.e. just checking for display version or
platform directly, and later adding wa infrastructure similar to what xe
has, but for display only.

BR,
Jani.



> +}





-- 
Jani Nikula, Intel