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

[Lucas De Marchi]

On Fri, Sep 06, 2024 at 11:51:50AM GMT, Jani Nikula wrote:
>
>Cc: Rodrigo and Lucas, note for you at the end.
>
>On Fri, 06 Sep 2024, Nemesa Garg <nemesa.garg at 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 at 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.

Do you expect any WA like this to be enabled for both xe and i915? There
are no platform with i915 and display_ver == 20.  I think for past
platforms, just following the display version and platform check is fine.
For newer ones, where there's only support in xe, we use the xe way.

Lucas De Marchi

>
>BR,
>Jani.
>
>
>
>> +}
>
>
>
>
>
>-- 
>Jani Nikula, Intel