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

[Matt Roper <matthew.d.roper@intel.com>]

On Fri, Oct 18, 2024 at 11:14:40AM +0530, Chauhan, Shekhar wrote:
> 
> On 10/18/2024 10:34, Garg, Nemesa wrote:
> > 
> > > -----Original Message-----
> > > From: Pottumuttu, Sai Teja <sai.teja.pottumuttu@intel.com>
> > > Sent: Thursday, October 17, 2024 10:24 PM
> > > To: Kandpal, Suraj <suraj.kandpal@intel.com>; Garg, Nemesa
> > > <nemesa.garg@intel.com>; intel-xe@lists.freedesktop.org
> > > Cc: Roper, Matthew D <matthew.d.roper@intel.com>
> > > Subject: Re: [PATCH] drm/i915/display: Workaround for odd panning for planar
> > > yuv
> > > 
> > > 
> > > On 17-10-2024 13:35, Kandpal, Suraj wrote:
> > > > > -----Original Message-----
> > > > > From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of
> > > > > Nemesa Garg
> > > > > Sent: Wednesday, October 16, 2024 12:55 PM
> > > > > To: intel-xe@lists.freedesktop.org
> > > > > Cc: Garg, Nemesa <nemesa.garg@intel.com>
> > > > > Subject: [PATCH] drm/i915/display: Workaround for odd panning for
> > > > > planar yuv
> > > > > 
> > > > > Disable the support for odd x pan for even xsize for NV12 format as
> > > > > underrun issue is seen.
> > > - Probably with the discussion and the latest update to the patch we should also
> > > update the commit message to aptly reflect what are we doing here. (We can
> > > remove even xsize part or maybe give more explanation on why even x size is not
> > > being checked in the patch)
> > > 
> > > - We should ideally be using the workaround lineage number in the commit
> > > message.
> > > 
> > Sure.
> > > - Also, seems like NV12 is not the only planar format, should we check for a planar
> > > format in general instead of NV12 specifically?
> > > Cc: Matt Roper
> > > 
> > In WA its mentioned that the issue is seen for YUV420 format so that's why I'm checking for NV12 format.
> Could you please explain me:
> 
> 1. Was this issue reproducible for Planar formats or Semi Planar formats (if
> they are different)?
> 
> 2. Did we check it on only YUV_420 or all the planar formats (if the issue
> is only on Planar Formats). If it's only for YUV_420, then can we have
> commit title as "Workaround for odd panning of YUV_420..." or something like
> that. If not, then can we test it for the other formats as well?
> 
> 3. Are YUV_420 and NV_12 formats the same, and is NV_12 a planar format or a
> semi_planar format? (drm/drm_fourcc.h makes me think otherwise, correct me
> if I'm wrong??)

YUV in general can be stored in either "packed" form or "planar."  In
packed form, all the information for a single pixel is together in
memory, then all the information for the next pixel, and so on.  I.e.,
the same way you're familiar with from RGB formats, just encoded as
luminance and chrominance in the case of YUV instead of red/green/blue.

In a planar format, the components of each pixel get separated out into
different regions of the framebuffer memory.  These are called "planes"
but that's a heavily overloaded term unfortunately; these aren't the
same thing as primary/overlay/cursor planes in the display controller.
For example, NV12 is a planar YUV format that has two planes, so one
region of the framebuffer memory contains the luma values (Y) for all
the pixels, and another region of memory contains the two chroma values
(U and V) for all pixels.  In general there are also planar YUV formats
that use three planes instead of just two (so each chroma value, U and
V, gets collected into its own plane).  I don't believe any of our Intel
platforms support any of the three-plane formats today though, so our
driver is mainly concerned with the two-plane (sometimes called
"semiplanar") formats.

In the Intel world, NV12 is probably the most common planar YUV format
you'll run across, but we do support others, specifically the P01x
formats which are also two-plane formats with the same 4:2:0
subsampling, just with more bits used to encode each component.  And
since the workaround description here says "YUV Planar format" it should
presumably apply to them as well.  You probably want to use
intel_format_info_is_yuv_semiplanar() rather than testing against NV12
specifically when determining if/when the workaround should be applied.


Matt

> 
> -shekhar
> 
> CC: Matt Roper, Sai Teja
> 
> > 
> > Thanks,
> > Nemesa
> > 
> > > Thanks
> > > Sai Teja
> > > 
> > > > > WA: 16024459452
> > > > This should be right above your signed-off-by The rest LGTM,
> > > > Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>
> > > > 
> > > > > 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]
> > > > > 
> > > > > v5: Replace IS_LUNARLAKE with display version.
> > > > >       Resolve nitpicks[Jani]
> > > > > 
> > > > > v6: Replace -EINVAL with hsub [Suraj]
> > > > >       Remove src_w check as not required
> > > > > 
> > > > > Signed-off-by: Nemesa Garg <nemesa.garg@intel.com>
> > > > > ---
> > > > >    drivers/gpu/drm/i915/display/intel_atomic_plane.c | 5 +++++
> > > > >    1 file changed, 5 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > > > > b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > > > > index e979786aa5cf..2d7ca6e62926 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > > > > @@ -1031,6 +1031,11 @@ int intel_plane_check_src_coordinates(struct
> > > > > intel_plane_state *plane_state)
> > > > >    		 */
> > > > >    		hsub = 1;
> > > > >    		vsub = 1;
> > > > > +
> > > > > +		/* Wa_16023981245 */
> > > > > +		if (DISPLAY_VER(i915) == 20 && fb->format->format ==
> > > > > DRM_FORMAT_NV12 &&
> > > > > +		    src_x % 2 != 0)
> > > > > +			hsub = 2;
> > > > >    	} else {
> > > > >    		hsub = fb->format->hsub;
> > > > >    		vsub = fb->format->vsub;
> > > > > --
> > > > > 2.25.1
> 
> -- 
> -shekhar
> 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation