[Intel-gfx] [PATCH] drm/i915: Handle YUV subpixel support better

[Ville Syrjälä]

On Tue, Mar 19, 2019 at 01:15:55PM +0200, Ville Syrjälä wrote:
> On Tue, Mar 19, 2019 at 08:28:58AM +0100, Maarten Lankhorst wrote:
> > Op 18-03-2019 om 19:15 schreef Ville Syrjälä:
> > > On Mon, Mar 18, 2019 at 04:13:57PM +0100, Maarten Lankhorst wrote:
> > >> Op 18-03-2019 om 15:18 schreef Ville Syrjälä:
> > >>> On Mon, Mar 18, 2019 at 03:07:18PM +0100, Maarten Lankhorst wrote:
> > >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > >>>> ---
> > >>>>  drivers/gpu/drm/i915/intel_sprite.c | 29 +++++++++++++++++++----------
> > >>>>  1 file changed, 19 insertions(+), 10 deletions(-)
> > >>>>
> > >>>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > >>>> index 268fb34ff0e2..862fc172042f 100644
> > >>>> --- a/drivers/gpu/drm/i915/intel_sprite.c
> > >>>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > >>>> @@ -269,7 +269,8 @@ int intel_plane_check_src_coordinates(struct intel_plane_state *plane_state)
> > >>>>  {
> > >>>>  	const struct drm_framebuffer *fb = plane_state->base.fb;
> > >>>>  	struct drm_rect *src = &plane_state->base.src;
> > >>>> -	u32 src_x, src_y, src_w, src_h;
> > >>>> +	u32 src_x, src_y, src_w, src_h, hsub, vsub;
> > >>>> +	bool rotated = drm_rotation_90_or_270(plane_state->base.rotation);
> > >>>>  
> > >>>>  	/*
> > >>>>  	 * Hardware doesn't handle subpixel coordinates.
> > >>>> @@ -287,18 +288,26 @@ int intel_plane_check_src_coordinates(struct intel_plane_state *plane_state)
> > >>>>  	src->y1 = src_y << 16;
> > >>>>  	src->y2 = (src_y + src_h) << 16;
> > >>>>  
> > >>>> -	if (fb->format->is_yuv &&
> > >>>> -	    (src_x & 1 || src_w & 1)) {
> > >>>> -		DRM_DEBUG_KMS("src x/w (%u, %u) must be a multiple of 2 for YUV planes\n",
> > >>>> -			      src_x, src_w);
> > >>>> +	if (!fb->format->is_yuv)
> > >>>> +		return 0;
> > >>>> +
> > >>>> +	/* YUV specific checks */
> > >>>> +	if (!rotated) {
> > >>>> +		hsub = fb->format->hsub;
> > >>>> +		vsub = fb->format->vsub;
> > >>>> +	} else {
> > >>>> +		hsub = vsub = max(fb->format->hsub, fb->format->vsub);
> > >>> Why this? From the looks of things there should be no need to deal with
> > >>> rotation in this function at all.
> > >> I wrote a dumb test that fails if I rotate YUYV.
> > >>
> > >> https://patchwork.freedesktop.org/patch/286170/
> > >>
> > >> Corrupted image:
> > >>
> > >> (kms_yuv:1155) igt_kms-DEBUG: display: A.0: plane_set_rotation(90°)
> > >> (kms_yuv:1155) igt_kms-DEBUG: display: A.0: src_set_position(18,33)
> > >> (kms_yuv:1155) igt_kms-DEBUG: display: A.0: src_set_size(44x65)
> > >> (kms_yuv:1155) igt_kms-DEBUG: display: A.0: plane_set_position(64,64)
> > >> (kms_yuv:1155) igt_kms-DEBUG: display: A.0: plane_set_size (256x256)
> > >>
> > >> I had a 80x128 fb, only showing the center part which should be white, with a black border around it to cause CRC errors if we mess up clipping.
> > >>
> > >> The scaling works fine, but the clipping does not in this case. I am getting a corrupted plane on screen which is mostly white, but with black dots in each tile.
> > >>
> > >> Scaling just magnifies this corruption. :)
> > > Hmm. I just poked my KBL a bit and it is also showing curious
> > > behaviour. Even with 90/270 rotation it is in fact the TILEOFF
> > > X coordinate that needs to be even (actually the hw just appears
> > > to ignore the lsb). I can make the Y coordinate odd, and the image
> > > still looks correct to my eyes. So feels like someone forgot to
> > > to remove a (x&~1) from the hw when they added the 90/270 rotation,
> > > and yet they went to the trouble of making odd Y coordinates work
> > > correctly. Quite stange.
> > >
> > > Width/height being odd seems to handled just fine by the hw.
> > >
> > Hmm does that mean we should keep the original checks in place while checking format->h/vsub, and on top reject the unrotated Y coordinate being a multiple of hsub when rotating?
> 
> Not quite sure. Based on what I see we could actually just swap the
> coordinates (or do the check after the coordinates are already rotated)
> and it should still work. But I didn't check if that would still work
> when the scaler is involved.

Hmm. The spec disagrees with this observed behaviour of PLANE_OFFSET.
It claims our current code should be fine.

PLANE_SIZE also has this slightly confusing table for GLK+:
PixelFormat		Rotate		Width	Height
YUV 420 Planar - NV12	All		Even	Even
YUV 420 Planar - P01x	All		Even	Even
YUV 422 		All 		Even	Even
RGB565			90, 270		Even	Even

which pretty much matches the w/h part of your patch.

-- 
Ville Syrjälä
Intel