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

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

On Wed, 25 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
>
> v2: Replace HSD with WA in commit message [Suraj]
>     Modified the condition for handling odd panning
>
> v3: Simplified the condition for checking hsub
>     Using older framework for wa as rev1[Jani]
>
> v4: Modify the condition for hsub [Sai Teja]
>     Initialize hsub in else path [Dan]
>
> Signed-off-by: Nemesa Garg <nemesa.garg@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_atomic_plane.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> index e979786aa5cf..dfe795b8e917 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> @@ -1029,8 +1029,16 @@ 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
>  		 */

Okay, sorry, nitpicks galore. :/

> -		hsub = 1;
>  		vsub = 1;

If we are going to return with an error, why break the hsub/vsub
assignments apart? Maybe add the check as the first thing, and keep
these together?

> +		/*
> +		 * Wa_16023981245 for display version 20.
> +		 * Do not support odd x-panning for even xsize for NV12.
> +		 */
> +		if (IS_LUNARLAKE(i915) && fb->format->format == DRM_FORMAT_NV12

The comment and the code are contradictory. We're already in a display
version >= 20 block. Why do we have the separate IS_LUNARLAKE() check?
Should it be a version check for == 20, or nothing at all? Is the code
or the comment wrong?

> +		    && src_w % 2 == 0)

I kernel code the && usually belongs at the end of the previous line.

BR,
Jani.

> +			return -EINVAL;
> +
> +		hsub = 1;
>  	} else {
>  		hsub = fb->format->hsub;
>  		vsub = fb->format->vsub;

-- 
Jani Nikula, Intel