[Intel-gfx] [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for NV12

Tue Apr 17 10:01:32 UTC 2018

Op 17-04-18 om 11:42 schreef Srinivas, Vidya:
>
>> -----Original Message-----
>> From: Maarten Lankhorst [mailto:maarten.lankhorst at linux.intel.com]
>> Sent: Tuesday, April 17, 2018 3:04 PM
>> To: Srinivas, Vidya <vidya.srinivas at intel.com>; Saarinen, Jani
>> <jani.saarinen at intel.com>; intel-gfx at lists.freedesktop.org
>> Cc: Kamath, Sunil <sunil.kamath at intel.com>
>> Subject: Re: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for
>> NV12
>>
>> Op 17-04-18 om 10:09 schreef Srinivas, Vidya:
>>>> -----Original Message-----
>>>> From: Srinivas, Vidya
>>>> Sent: Tuesday, April 17, 2018 1:32 PM
>>>> To: 'Maarten Lankhorst' <maarten.lankhorst at linux.intel.com>;
>>>> Saarinen, Jani <jani.saarinen at intel.com>;
>>>> intel-gfx at lists.freedesktop.org
>>>> Cc: Kamath, Sunil <sunil.kamath at intel.com>
>>>> Subject: RE: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments
>>>> for
>>>> NV12
>>>>
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Maarten Lankhorst [mailto:maarten.lankhorst at linux.intel.com]
>>>>> Sent: Tuesday, April 17, 2018 1:21 PM
>>>>> To: Srinivas, Vidya <vidya.srinivas at intel.com>; Saarinen, Jani
>>>>> <jani.saarinen at intel.com>; intel-gfx at lists.freedesktop.org
>>>>> Cc: Kamath, Sunil <sunil.kamath at intel.com>
>>>>> Subject: Re: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments
>>>>> for
>>>>> NV12
>>>>>
>>>>> Op 17-04-18 om 09:38 schreef Srinivas, Vidya:
>>>>>>> -----Original Message-----
>>>>>>> From: Maarten Lankhorst [mailto:maarten.lankhorst at linux.intel.com]
>>>>>>> Sent: Tuesday, April 17, 2018 12:55 PM
>>>>>>> To: Saarinen, Jani <jani.saarinen at intel.com>; Srinivas, Vidya
>>>>>>> <vidya.srinivas at intel.com>;
>>>>>>> intel-gfx-trybot at lists.freedesktop.org;
>>>>>>> intel- gfx at lists.freedesktop.org
>>>>>>> Cc: Kamath, Sunil <sunil.kamath at intel.com>
>>>>>>> Subject: Re: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments
>>>>>>> for
>>>>>>> NV12
>>>>>>>
>>>>>>> Op 17-04-18 om 08:03 schreef Saarinen, Jani:
>>>>>>>> Lets move this to intel-gfx to get more eyes on it.
>>>>>>>>
>>>>>>>>> -----Original Message-----
>>>>>>>>> From: Srinivas, Vidya
>>>>>>>>> Sent: tiistai 17. huhtikuuta 2018 5.47
>>>>>>>>> To: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>;
>>>>>>>>> intel-gfx- trybot at lists.freedesktop.org
>>>>>>>>> Cc: Kamath, Sunil <sunil.kamath at intel.com>; Saarinen, Jani
>>>>>>>>> <jani.saarinen at intel.com>
>>>>>>>>> Subject: RE: [PATCH v2 6/6] drm/i915: Do not do fb src
>>>>>>>>> adjustments for NV12
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> -----Original Message-----
>>>>>>>>>> From: Maarten Lankhorst
>>>>>>>>>> [mailto:maarten.lankhorst at linux.intel.com]
>>>>>>>>>> Sent: Monday, April 16, 2018 8:00 PM
>>>>>>>>>> To: Srinivas, Vidya <vidya.srinivas at intel.com>; intel-gfx-
>>>>>>>>>> trybot at lists.freedesktop.org
>>>>>>>>>> Cc: Kamath, Sunil <sunil.kamath at intel.com>; Saarinen, Jani
>>>>>>>>>> <jani.saarinen at intel.com>
>>>>>>>>>> Subject: Re: [PATCH v2 6/6] drm/i915: Do not do fb src
>>>>>>>>>> adjustments for
>>>>>>>>>> NV12
>>>>>>>>>>
>>>>>>>>>> Hmm, more thought about src adjustments...
>>>>>>>>>>
>>>>>>>>>> Op 13-04-18 om 07:22 schreef Vidya Srinivas:
>>>>>>>>>>> We skip src trunction/adjustments for
>>>>>>>>>>> NV12 case and handle the sizes directly.
>>>>>>>>>>> Without this, pipe fifo underruns are seen on APL/KBL.
>>>>>>>>>>>
>>>>>>>>>>> v2: For NV12, making the src coordinates multiplier of 4
>>>>>>>>>>>
>>>>>>>>>>> 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_display.c | 11 +++++++++++
>>>>>>>>>>> drivers/gpu/drm/i915/intel_sprite.c  | 15 +++++++++++++++
>>>>>>>>>>>  2 files changed, 26 insertions(+)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>>>>>>>>>>> b/drivers/gpu/drm/i915/intel_display.c
>>>>>>>>>>> index bc83f10..f64708f 100644
>>>>>>>>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>>>>>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>>>>>>>>> @@ -12978,6 +12978,10 @@ intel_check_primary_plane(struct
>>>>>>>>>> intel_plane *plane,
>>>>>>>>>>>  	bool can_position = false;
>>>>>>>>>>>  	int ret;
>>>>>>>>>>>  	uint32_t pixel_format = 0;
>>>>>>>>>>> +	struct drm_plane_state *plane_state = &state->base;
>>>>>>>>>>> +	struct drm_rect *src = &plane_state->src;
>>>>>>>>>>> +
>>>>>>>>>>> +	*src = drm_plane_state_src(plane_state);
>>>>>>>>>>>
>>>>>>>>>>>  	if (INTEL_GEN(dev_priv) >= 9) {
>>>>>>>>>>>  		/* use scaler when colorkey is not required */ @@ -
>>>>> 13001,6
>>>>>>>>>>> +13005,13 @@ intel_check_primary_plane(struct intel_plane
>>>>> *plane,
>>>>>>>>>>>  	if (!state->base.fb)
>>>>>>>>>>>  		return 0;
>>>>>>>>>>>
>>>>>>>>>>> +	if (pixel_format == DRM_FORMAT_NV12) {
>>>>>>>>>>> +		src->x1 = (((src->x1 >> 16)/4)*4) << 16;
>>>>>>>>>>> +		src->x2 = (((src->x2 >> 16)/4)*4) << 16;
>>>>>>>>>>> +		src->y1 = (((src->y1 >> 16)/4)*4) << 16;
>>>>>>>>>>> +		src->y2 = (((src->y2 >> 16)/4)*4) << 16;
>>>>>>>>>>> +	}
>>>>>>>>>> Lets reject non multiple of 4 coordinates for plane_state's
>>>>>>>>>> src_x and src_y, and before adjustment also reject non multiple
>>>>>>>>>> of 4 src_w/src_h.fter that we can also reject clipping to the
>>>>>>>>>> right/bottom edge of the screen, if the pipe_src_w/h is not a
>>>>>>>>>> multiple of 4. That way an application trying to go beyond the
>>>> screen.
>>>>>>>>> kms_plane_scaling and some other tests during execution generate
>>>>>>>>> lots of non mult of 4 buffer width/height. All these tests will
>>>>>>>>> show fail in the IGT BAT.
>>>>>>>>> In some cases, even thought the width and height will be
>>>>>>>>> multiple of
>>>>>>>>> 4 before the Adjustment (say our 16x16 buffer), this after
>>>>>>>>> adjustment becomes a 15 in intel_check_sprite_plane.
>>>>>>>>> This again would cause underrun. If we reject non multiple of 4s
>>>>>>>>> in our skl_update_scaler also, then even simple tests with 16x16
>>>>>>>>> like rotation will fail due to it getting adjusted.
>>>>>>> Correct. This is why we fix up to a multiple of 4 after
>>>>>>> adjustments, and reject user passed coordinates before adjustment..
>>>>>>> plane_state->src_x/y/w/h is the property set by the user, while
>>>>>>> the plane_state->src rect is set and fixed up in the kernel.
>>>>>>>
>>>>>>> Rejecting clipping with positive coordinates if pipe_src w/h is
>>>>>>> not a multiple of 4 will fix a border case that shouldn't affect
>>>>>>> most modes, where the plane wouldn't extend to the crtc borders,
>>>>>>> which is
>>>>> what userspace wants there.
>>>>>> Thank you. Oh okay. Sorry. I thought we just reject and not do the
>>>>>> fixing
>>>>> (mult of 4 later).
>>>>>> Sure, I will change the patch to also reject the coordinates before
>>>>>> adjustment (if its not Mult of 4).
>>>>>>
>>>>>>
>>>>> This can be done in skl_check_plane_surface,
>>>>> plane_state->src_{x,y,w,h} contains the user requested values.
>>>> Apologies for asking again. The adjustments are done in check_plane
>>>> (both primary and sprite) And towards the end of check_plane we call
>>>> the skl_check_plane_surface.
>>>> I got your point that we will reject the user requested values if
>>>> they are not multiple of 4.
>>>> But before we reach skl_check_plane_surface, they would’ve been
>>>> adjusted/clipped etc They wont remain user requested as is when they
>>>> enter skl_check_plane_surface.
>>>> Please correct me if I am wrong or I am missing in understanding
>>>> something here :(
>>>>
>>>> I can add the add conversion to multiple of 4 in
>>>> skl_check_plane_surface but rejection should be done before the
>>>> clipping/adjustment Or else, I move the entire clipping/adjustment code
>> to skl_check_plane_surface?
>>> Sorry, I got it. You said, we don’t do adjustments in case of NV12 in
>>> sprite and move The handling to skl_check_plane_surface. I will do the
>> change and submit. Sorry.
>>> Thank you.
>>>
>> Something like below? Untested..
>> We should probably also restrict NV12 fb creation to not allow CCS formats.
>>
>> ~Maarten
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 4b3735720fee..c5da30c7ad04 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -3090,6 +3090,38 @@ static int skl_check_main_surface(const struct
>> intel_crtc_state *crtc_state,
>>  	return 0;
>>  }
>>
>> +static int skl_check_nv12_surface(const struct intel_crtc_state *crtc_state,
>> +				  struct intel_plane_state *plane_state) {
>> +	int crtc_x2 = plane_state->base.crtc_x + plane_state->base.crtc_w;
>> +	int crtc_y2 = plane_state->base.crtc_y + plane_state->base.crtc_h;
>> +
>> +	/* Are the unadjusted coordinates passed in valid? */
>> +	if ((plane_state->base.src_x >> 16) % 4 ||
>> +	    (plane_state->base.src_y >> 16) % 4 ||
>> +	    (plane_state->base.src_w >> 16) % 4 ||
>> +	    (plane_state->base.src_h >> 16) % 4) {
>> +		DRM_DEBUG_KMS("Source coordinates must be a multiple
>> of 4 for NV12\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Clipping would cause a 1-3 pixel gap at the edge of the screen? */
>> +	if ((crtc_x2 > crtc_state->pipe_src_w && crtc_state->pipe_src_w %
>> 4) ||
>> +	    (crtc_y2 > crtc_state->pipe_src_h && crtc_state->pipe_src_h % 4))
>> {
>> +		DRM_DEBUG_KMS("It's not possible to clip %u,%u to
>> %u,%u\n",
>> +			      crtc_x2, crtc_y2,
>> +			      crtc_state->pipe_src_w, crtc_state->pipe_src_h);
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Make adjusted src coordinates a multiple of 4. */
>> +	plane_state->base.src.x1 = DIV_ROUND_CLOSEST(plane_state-
>>> base.src.x1, 1 << 18) << 18;
>> +	plane_state->base.src.y1 = DIV_ROUND_CLOSEST(plane_state-
>>> base.src.y1, 1 << 18) << 18;
>> +	plane_state->base.src.x2 = DIV_ROUND_CLOSEST(plane_state-
>>> base.src.x2, 1 << 18) << 18;
>> +	plane_state->base.src.y2 = DIV_ROUND_CLOSEST(plane_state-
>>> base.src.y2, 1 << 18) << 18;
>> +	return 0;
>> +}
>> +
> Thank you so much :) Only one problem. The primary plane clipping
> happens inside the drm layer in drm_atomic_helper_check_plane_state
> So, clipping-clamping test passes coordinates 0, 257, 0, 159 to check_plane.
> And there before adjusting we reject. So for primary alone, can I retain 
> the make multiple of 4 code in primary itself?

The clipping affects plane_state->src.{x1, y1, x2, y2}, not plane_state->src_{x,y,w,h}.
They're different. The last set of coordinates will not be rotated or anything, the
first set of coordinates are what we end up programming into the hardware. The patch
I put above should be correctly rounding to a multiple of 4. :)

~Maarten