[Intel-gfx] [PATCH v18 18/18] drm/i915: Keep plane size mult of 4 for NV12

[Maarten Lankhorst]

Op 29-03-18 om 11:19 schreef Srinivas, Vidya:
>
>> -----Original Message-----
>> From: Maarten Lankhorst [mailto:maarten.lankhorst at linux.intel.com]
>> Sent: Thursday, March 29, 2018 2:19 PM
>> To: Srinivas, Vidya <vidya.srinivas at intel.com>; intel-
>> gfx at lists.freedesktop.org
>> Subject: Re: [Intel-gfx] [PATCH v18 18/18] drm/i915: Keep plane size mult of
>> 4 for NV12
>>
>> Op 29-03-18 om 10:06 schreef Vidya Srinivas:
>>> As per display WA 1106, to avoid corruption issues
>>> NV12 plane height needs to be multiplier of 4 Hence we modify the fb
>>> src and destination height and width to be multiples of 4. Without
>>> this, pipe fifo underruns were seen on APL and KBL.
>>>
>>> Credits-to: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>>> Signed-off-by: Vidya Srinivas <vidya.srinivas at intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/intel_drv.h    | 2 ++
>>>  drivers/gpu/drm/i915/intel_sprite.c | 8 ++++++++
>>>  2 files changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>>> b/drivers/gpu/drm/i915/intel_drv.h
>>> index 9c58da0..a1f718d 100644
>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> @@ -159,6 +159,8 @@
>>>  #define INTEL_I2C_BUS_DVO 1
>>>  #define INTEL_I2C_BUS_SDVO 2
>>>
>>> +#define MULT4(x) ((x + 3) & ~0x03)
>>> +
>>>  /* these are outputs from the chip - integrated only
>>>     external chips are via DVO or SDVO output */  enum
>>> intel_output_type { diff --git a/drivers/gpu/drm/i915/intel_sprite.c
>>> b/drivers/gpu/drm/i915/intel_sprite.c
>>> index 538d938..9f466c6 100644
>>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>>> @@ -261,6 +261,14 @@ skl_update_plane(struct intel_plane *plane,
>>>  	crtc_w--;
>>>  	crtc_h--;
>>>
>>> +	if (fb->format->format == DRM_FORMAT_NV12) {
>>> +		src_w = MULT4(src_w);
>>> +		src_h = MULT4(src_h);
>>> +		crtc_w = MULT4(crtc_w);
>>> +		crtc_h = MULT4(crtc_h);
>>> +		DRM_ERROR("%d %d %d %d\n", src_w, src_h, crtc_w,
>> crtc_h);
>>> +	}
>>> +
>>>  	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>>>
>>>  	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
>> Nearly there!
>>
>> Do we have limitations for width too? But I think we shouldn't ever adjust
>> src for any format.
>> This means that we should probably get rid of the drm_rect_adjust_size call
>> in intel_check_sprite_plane.
>>
>> If any limitations of NV12 are hit, we should reject with -EINVAL instead so
>> userspace can decide what to do.
>> The best place to put those checks is probably in skl_update_scaler, where
>> they will be checked by the primary plane too.
>>
>> This will mean the tests fail, but that can be fixed by selecting 16 as
>> width/height for NV12 in IGT. If you change it to 16 you can put my r-b on it.
>>
>> Also I think we should put the same limitations for width and height being a
>> multiple in intel_framebuffer_init.
>>
>> And on a final note for patch ordering, put the workaround and gen10 patch
>> before enabling nv12 support.
> Thank you. Okay, I will make these changes and check once. The limitation in
> Framebuffer init is already present where it expects width and height >= 16
> As per bspec no minimum for width has been mentioned. And regarding the
> Add check for primary plane (same like sprite), can we add that as a separate patch
> Because if we add it with NV12 series, it would be like adding the changes and 
> Returning before executing them.
I don't think we'll lose much if we fail to create a fb that's not a multiple of 4 in
height and width. Since the NV12 format is defined in terms of 4x4 pixel blocks,
I don't think it would be a loss to fail to create it, if we can't even display it.
> Right now range check only exists for NV12 in skl_update_scaler. My worry was:
> If the width and height are not multiplier of 4 do we return from skl_update_scaler?
We should always refuse to show when the src height is not a multiple of 4, and return -EINVAL.
> What if some other user level program wants to set src width and height 23x23 etc?
Then userspace will see that it will fail with -EINVAL, if it's done by a compositor with a TEST_ONLY commit,
it will see the src cannot be set and either choose a different size or fallback to software rendering before
displaying the output.

This is still better than silently succeeding, but showing something different.
> I will check if we remove the src aligning from skl_update_plane and just keep the
> Destination as multiplier of 4 in skl_update_plane.
I think it's more likely the src that needs to be a multiple of 4. I don't think there's
much of a failure in destination.
> Regarding the reordering, I will make the change and float the series. Thank you
> So much for all the support and pointers.
>
> If no fifo underruns are seen with just keeping the dest width and height mult of 4,
> We anyways don’t do the drm_rect_adjust_size, then we can avoid putting any
> Limitation (other than range check) in skl_update_scaler correct?
Perhaps, but wouldn't hurt to be paranoid.