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

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

On Fri, 06 Sep 2024, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> 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.

Oh, to that end I prefer the style from earlier version [1]. I also
noticed that you've sent several versions of this (two today, why?) but
there's no versions or changelog or anything in the patches.

BR,
Jani.


[1] https://lore.kernel.org/r/20240724130607.944085-1-nemesa.garg@intel.com


>
> 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