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

[Srinivas, Vidya]

> -Original Message-
> From: Maarten Lankhorst [mailto:maarten.lankho...@linux.intel.com]
> Sent: Thursday, April 12, 2018 4:41 PM
> To: Srinivas, Vidya ; Intel Graphics Development
> ; Ville Syrjälä
> 
> Cc: Kamath, Sunil ; Saarinen, Jani
> 
> Subject: Re: [PATCH v1 5/6] drm/i915: Do not do fb src adjustments for
> NV12
> 
> Op 12-04-18 om 12:07 schreef Srinivas, Vidya:
> >
> >> -Original Message-
> >> From: Maarten Lankhorst [mailto:maarten.lankho...@linux.intel.com]
> >> Sent: Wednesday, April 11, 2018 4:08 PM
> >> To: Srinivas, Vidya ; intel-gfx-
> >> try...@lists.freedesktop.org
> >> Cc: Kamath, Sunil ; Saarinen, Jani
> >> 
> >> Subject: Re: [PATCH v1 5/6] drm/i915: Do not do fb src adjustments
> >> for
> >> NV12
> >>
> >> Op 11-04-18 om 11:09 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.
> >>>
> >>> Credits-to: Maarten Lankhorst 
> >>> Signed-off-by: Vidya Srinivas 
> >>> ---
> >>>  drivers/gpu/drm/i915/intel_display.c | 88
> >>> ++--
> >>>  drivers/gpu/drm/i915/intel_sprite.c  | 10 +++-
> >>>  2 files changed, 92 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/intel_display.c
> >>> b/drivers/gpu/drm/i915/intel_display.c
> >>> index ebb3f8e..e4cf7a6 100644
> >>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>> @@ -12951,6 +12951,86 @@ skl_max_scale(struct intel_crtc
> >>> *intel_crtc, }
> >>>
> >>>  static int
> >>> +intel_primary_plane_state(struct drm_plane_state *plane_state,
> >>> +   const struct drm_crtc_state *crtc_state,
> >>> +   int min_scale, int max_scale,
> >>> +   bool can_position, bool can_update_disabled) {
> >>> + struct drm_framebuffer *fb = plane_state->fb;
> >>> + struct drm_rect *src = _state->src;
> >>> + struct drm_rect *dst = _state->dst;
> >>> + unsigned int rotation = plane_state->rotation;
> >>> + struct drm_rect clip = {};
> >>> + int hscale, vscale;
> >>> +
> >>> + WARN_ON(plane_state->crtc && plane_state->crtc != crtc_state-
> >>> crtc);
> >>> +
> >>> + *src = drm_plane_state_src(plane_state);
> >>> + *dst = drm_plane_state_dest(plane_state);
> >>> +
> >>> + if (!fb) {
> >>> + plane_state->visible = false;
> >>> + return 0;
> >>> + }
> >>> +
> >>> + /* crtc should only be NULL when disabling (i.e., !fb) */
> >>> + if (WARN_ON(!plane_state->crtc)) {
> >>> + plane_state->visible = false;
> >>> + return 0;
> >>> + }
> >>> +
> >>> + if (!crtc_state->enable && !can_update_disabled) {
> >>> + DRM_DEBUG_KMS("Cannot update plane of a disabled
> >> CRTC.\n");
> >>> + return -EINVAL;
> >>> + }
> >>> +
> >>> + drm_rect_rotate(src, fb->width << 16, fb->height << 16, rotation);
> >>> +
> >>> + /* Check scaling */
> >>> + hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale);
> >>> + vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale);
> >>> + if (hscale < 0 || vscale < 0) {
> >>> + DRM_DEBUG_KMS("Invalid scaling of plane\n");
> >>> + drm_rect_debug_print("src: ", _state->src, true);
> >>> + drm_rect_debug_print("dst: ", _state->dst, false);
> >>> + return -ERANGE;
> >>> + }
> >>> +
> >>> + if (crtc_state->enable)
> >>> + drm_mode_get_hv_timing(_state->mode, ,
> >> );
> >>> +
> >>> + if (fb->format->format == DRM_FORMAT_NV12) {
> >>> + plane_state->visible = true;
> >>> + goto skip_clip;
> >>> + }
> >>> +
> >>> + plane_state->visible =
> >>> + drm_rect_clip_scaled(src, dst, , hscale, vscale);
> >> The real problem is that it needs to be a multiple of 4. I think the
> >> clipping here is harmless, we should just adjust intel_check_sprite_plane
> for >= SKL.
> >> This will make it so we don't have to duplicate its checks.
> >>
> >> For NV12 we still want to call clip_rect_scaled, but then adjust all
> >> coordinates by dividing by 4 on before the check, and multiplying
> >> with 4 after?
> >>
> > Thank you. Have made the changes in
> > https://patchwork.freedesktop.org/patch/216682/
> > With this, I did not see any underruns. For now, have a WA only when
> > we pass 16x16 Because, with that it further clips down and gets
> > rejected in skl_update_scaler as it is Less than 16. If we increase our
> buffer in igt, then this issue wont be there.
> > Please have a check. Initially, I tried the /4 before the adjustments and *4
> later, that wouldn’t work
> > We need to have the 16.16 values multiplier of 4. So, just put it towards
> the end of both plane checks.
> Well, this is annoying.
> 
> 

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

[Maarten Lankhorst]

Op 12-04-18 om 12:07 schreef Srinivas, Vidya:
>
>> -Original Message-
>> From: Maarten Lankhorst [mailto:maarten.lankho...@linux.intel.com]
>> Sent: Wednesday, April 11, 2018 4:08 PM
>> To: Srinivas, Vidya ; intel-gfx-
>> try...@lists.freedesktop.org
>> Cc: Kamath, Sunil ; Saarinen, Jani
>> 
>> Subject: Re: [PATCH v1 5/6] drm/i915: Do not do fb src adjustments for
>> NV12
>>
>> Op 11-04-18 om 11:09 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.
>>>
>>> Credits-to: Maarten Lankhorst 
>>> Signed-off-by: Vidya Srinivas 
>>> ---
>>>  drivers/gpu/drm/i915/intel_display.c | 88
>>> ++--
>>>  drivers/gpu/drm/i915/intel_sprite.c  | 10 +++-
>>>  2 files changed, 92 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>>> b/drivers/gpu/drm/i915/intel_display.c
>>> index ebb3f8e..e4cf7a6 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -12951,6 +12951,86 @@ skl_max_scale(struct intel_crtc *intel_crtc,
>>> }
>>>
>>>  static int
>>> +intel_primary_plane_state(struct drm_plane_state *plane_state,
>>> + const struct drm_crtc_state *crtc_state,
>>> + int min_scale, int max_scale,
>>> + bool can_position, bool can_update_disabled) {
>>> +   struct drm_framebuffer *fb = plane_state->fb;
>>> +   struct drm_rect *src = _state->src;
>>> +   struct drm_rect *dst = _state->dst;
>>> +   unsigned int rotation = plane_state->rotation;
>>> +   struct drm_rect clip = {};
>>> +   int hscale, vscale;
>>> +
>>> +   WARN_ON(plane_state->crtc && plane_state->crtc != crtc_state-
>>> crtc);
>>> +
>>> +   *src = drm_plane_state_src(plane_state);
>>> +   *dst = drm_plane_state_dest(plane_state);
>>> +
>>> +   if (!fb) {
>>> +   plane_state->visible = false;
>>> +   return 0;
>>> +   }
>>> +
>>> +   /* crtc should only be NULL when disabling (i.e., !fb) */
>>> +   if (WARN_ON(!plane_state->crtc)) {
>>> +   plane_state->visible = false;
>>> +   return 0;
>>> +   }
>>> +
>>> +   if (!crtc_state->enable && !can_update_disabled) {
>>> +   DRM_DEBUG_KMS("Cannot update plane of a disabled
>> CRTC.\n");
>>> +   return -EINVAL;
>>> +   }
>>> +
>>> +   drm_rect_rotate(src, fb->width << 16, fb->height << 16, rotation);
>>> +
>>> +   /* Check scaling */
>>> +   hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale);
>>> +   vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale);
>>> +   if (hscale < 0 || vscale < 0) {
>>> +   DRM_DEBUG_KMS("Invalid scaling of plane\n");
>>> +   drm_rect_debug_print("src: ", _state->src, true);
>>> +   drm_rect_debug_print("dst: ", _state->dst, false);
>>> +   return -ERANGE;
>>> +   }
>>> +
>>> +   if (crtc_state->enable)
>>> +   drm_mode_get_hv_timing(_state->mode, ,
>> );
>>> +
>>> +   if (fb->format->format == DRM_FORMAT_NV12) {
>>> +   plane_state->visible = true;
>>> +   goto skip_clip;
>>> +   }
>>> +
>>> +   plane_state->visible =
>>> +   drm_rect_clip_scaled(src, dst, , hscale, vscale);
>> The real problem is that it needs to be a multiple of 4. I think the clipping
>> here is harmless, we should just adjust intel_check_sprite_plane for >= SKL.
>> This will make it so we don't have to duplicate its checks.
>>
>> For NV12 we still want to call clip_rect_scaled, but then adjust all
>> coordinates by dividing by 4 on before the check, and multiplying with 4
>> after?
>>
> Thank you. Have made the changes in 
> https://patchwork.freedesktop.org/patch/216682/
> With this, I did not see any underruns. For now, have a WA only when we pass 
> 16x16
> Because, with that it further clips down and gets rejected in 
> skl_update_scaler as it is
> Less than 16. If we increase our buffer in igt, then this issue wont be there.
> Please have a check. Initially, I tried the /4 before the adjustments and *4 
> later, that wouldn’t work
> We need to have the 16.16 values multiplier of 4. So, just put it towards the 
> end of both plane checks.   
Well, this is annoying.

It seems we never rejected subpixel precision before, and resorted to clipping 
instead of rejecting.
Could we be more strict about this without breaking existing userspace?

~Maarten
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx