* Re: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for NV12
[not found] ` <F653A0A18852B74D88578FA2EB7094EAB6850B06@BGSMSX108.gar.corp.intel.com>
@ 2018-04-17 6:03 ` Saarinen, Jani
2018-04-17 7:25 ` Maarten Lankhorst
[not found] ` <F653A0A18852B74D88578FA2EB7094EAB6850CA9@BGSMSX108.gar.corp.intel.com>
1 sibling, 1 reply; 20+ messages in thread
From: Saarinen, Jani @ 2018-04-17 6:03 UTC (permalink / raw)
To: Srinivas, Vidya, Maarten Lankhorst, intel-gfx-trybot, intel-gfx
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@linux.intel.com>; intel-gfx-
> trybot@lists.freedesktop.org
> Cc: Kamath, Sunil <sunil.kamath@intel.com>; Saarinen, Jani
> <jani.saarinen@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@linux.intel.com]
> > Sent: Monday, April 16, 2018 8:00 PM
> > To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-gfx-
> > trybot@lists.freedesktop.org
> > Cc: Kamath, Sunil <sunil.kamath@intel.com>; Saarinen, Jani
> > <jani.saarinen@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@linux.intel.com>
> > > Signed-off-by: Vidya Srinivas <vidya.srinivas@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.
>
> >
> > skl_check_plane_surface could do all the fixups and is allowed to
> > return an error code, so we could put all SKL specific NV12 handling in
> there.
> > It doesn't matter that we calculate potentially illegal values, if we
> > never program them and return -EINVAL there. If it turns out we've
> > been too paranoid we could relax the condition.
> >
> > This would mean unified handling for NV12 for primary and sprite. :)
> >
> > > if (INTEL_GEN(dev_priv) >= 9) {
> > > ret = skl_check_plane_surface(crtc_state, state);
> > > if (ret)
> > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> > > b/drivers/gpu/drm/i915/intel_sprite.c
> > > index 8b7947d..c1dd85e 100644
> > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > @@ -1035,11 +1035,20 @@ intel_check_sprite_plane(struct intel_plane
> > *plane,
> > > return vscale;
> > > }
> > >
> > > + if (fb->format->format == DRM_FORMAT_NV12) {
> > > + if (src->x2 >> 16 == 16)
> > > + src->x2 = 16 << 16;
> > > + if (src->y2 >> 16 == 16)
> > > + src->y2 = 16 << 16;
> > This part doesn't look required, the magic below for the nv12 format
> > is similar.. Just conditionally call adjust_size for format != NV12.
> > :)
> > > + goto nv12_min_no_clip;
> > > + }
> > > +
> > > /* Make the source viewport size an exact multiple of the
> > scaling factors. */
> > > drm_rect_adjust_size(src,
> > > drm_rect_width(dst) * hscale -
> > drm_rect_width(src),
> > > drm_rect_height(dst) * vscale -
> > drm_rect_height(src));
> > >
> > > +nv12_min_no_clip:
> > > drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16,
> > > state->base.rotation);
> > >
> > > @@ -1105,6 +1114,12 @@ intel_check_sprite_plane(struct intel_plane
> > *plane,
> > > src->x2 = (src_x + src_w) << 16;
> > > src->y1 = src_y << 16;
> > > src->y2 = (src_y + src_h) << 16;
> > > + if (fb->format->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;
> > > + }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for NV12
[not found] ` <F653A0A18852B74D88578FA2EB7094EAB6850CA9@BGSMSX108.gar.corp.intel.com>
@ 2018-04-17 6:06 ` Saarinen, Jani
0 siblings, 0 replies; 20+ messages in thread
From: Saarinen, Jani @ 2018-04-17 6:06 UTC (permalink / raw)
To: Srinivas, Vidya, 'Maarten Lankhorst',
'intel-gfx-trybot@lists.freedesktop.org',
intel-gfx
+wider-list.
> -----Original Message-----
> From: Srinivas, Vidya
> Sent: tiistai 17. huhtikuuta 2018 7.59
> To: 'Maarten Lankhorst' <maarten.lankhorst@linux.intel.com>; 'intel-gfx-
> trybot@lists.freedesktop.org' <intel-gfx-trybot@lists.freedesktop.org>
> Cc: Kamath, Sunil <sunil.kamath@intel.com>; Saarinen, Jani
> <jani.saarinen@intel.com>
> Subject: RE: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for NV12
>
>
>
>
>
> > -----Original Message-----
>
> > From: Srinivas, Vidya
>
> > Sent: Tuesday, April 17, 2018 8:17 AM
>
> > To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>; intel-gfx-
>
> > trybot@lists.freedesktop.org
>
> > Cc: Kamath, Sunil <sunil.kamath@intel.com>; Saarinen, Jani
>
> > <jani.saarinen@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@linux.intel.com
> <mailto:maarten.lankhorst@linux.intel.com> ]
>
> > > Sent: Monday, April 16, 2018 8:00 PM
>
> > > To: Srinivas, Vidya <vidya.srinivas@intel.com
> <mailto:vidya.srinivas@intel.com> >; intel-gfx-
>
> > > trybot@lists.freedesktop.org <mailto:trybot@lists.freedesktop.org>
>
> > > Cc: Kamath, Sunil <sunil.kamath@intel.com
> <mailto:sunil.kamath@intel.com> >; Saarinen, Jani
>
> > > <jani.saarinen@intel.com <mailto:jani.saarinen@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@linux.intel.com
> <mailto:maarten.lankhorst@linux.intel.com> >
>
> > > > Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com
> <mailto:vidya.srinivas@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.
>
>
>
> Example:
>
> During kms_plane_scaling-with-rotation execution - print cooridinates here:
> src->x1, src->x2, src->y1, src->y2
>
>
>
> [ 43.542044] [drm:intel_check_sprite_plane] *ERROR* Before adjust: 1040:
> 0 16 0 16
>
> [ 43.542054] [drm:intel_check_sprite_plane] *ERROR* After adjust: 1054: 0
> 15 0 15
>
> [ 43.542058] [drm:intel_check_sprite_plane] *ERROR* Before check plane
> surface: 1135: 0 15 0 15
>
> [ 43.570827] [drm:intel_cpu_fifo_underrun_irq_handler] *ERROR* CPU
> pipe A FIFO underrun
>
>
>
> 16x16 becomes 15x15 after rect adjustment in check_sprite_plane
>
>
>
> During kms_plane_scaling-with-clipping-clamping – print coordinates here
> src->x1, src->x2, src->y1, src->y2
>
>
>
> [ 125.432029] [drm:intel_check_primary_plane] *ERROR* Before check
> plane state: 12998: 0 300 0 300
>
> [ 125.432041] [drm:intel_check_primary_plane] *ERROR* After check plane
> state: 13004: 0 257 0 159
>
> [ 128.057081] [drm:intel_cpu_fifo_underrun_irq_handler] *ERROR* CPU
> pipe A FIFO underrun
>
>
>
> 300x300 after primary plane clipping becomes 257 in check_primary_plane
>
>
>
> All these generate fifo underruns. In the first example here, 16x16, before
> adjustment we check if it is multiplier
>
> of 4, and since it is, we allow. But after rect_adjust it becomes 15x15
>
> In example 2, clipping clamping, 300x300 is the value before clipping in
> primary plane, since it is a multiple of 4, we allow
>
> But after clipping it becomes 257x159.
>
>
>
>
>
> >
>
> > >
>
> > > skl_check_plane_surface could do all the fixups and is allowed to
>
> > > return an error code, so we could put all SKL specific NV12 handling in
>
> > there.
>
> > > It doesn't matter that we calculate potentially illegal values, if we
>
> > > never program them and return -EINVAL there. If it turns out we've
>
> > > been too paranoid we could relax the condition.
>
> > >
>
> > > This would mean unified handling for NV12 for primary and sprite. :)
>
> > >
>
> > > > if (INTEL_GEN(dev_priv) >= 9) {
>
> > > > ret = skl_check_plane_surface(crtc_state, state);
>
> > > > if (ret)
>
> > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
>
> > > > b/drivers/gpu/drm/i915/intel_sprite.c
>
> > > > index 8b7947d..c1dd85e 100644
>
> > > > --- a/drivers/gpu/drm/i915/intel_sprite.c
>
> > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
>
> > > > @@ -1035,11 +1035,20 @@ intel_check_sprite_plane(struct intel_plane
>
> > > *plane,
>
> > > > return vscale;
>
> > > > }
>
> > > >
>
> > > > + if (fb->format->format == DRM_FORMAT_NV12) {
>
> > > > + if (src->x2 >> 16 == 16)
>
> > > > + src->x2 = 16 << 16;
>
> > > > + if (src->y2 >> 16 == 16)
>
> > > > + src->y2 = 16 << 16;
>
> > > This part doesn't look required, the magic below for the nv12 format
>
> > > is similar.. Just conditionally call adjust_size for format != NV12.
>
>
>
> This part is required only if we retain 16x16 buffer, Because after adjust_size
> it becomes
>
> 15x15 as I have mentioned above. Then our minimum criteria for NV12 fails
> and our igt buffer
>
> As of now is 16x16. To get a IGT pass I had added this work around.
>
>
>
> > > :)
>
> > > > + goto nv12_min_no_clip;
>
> > > > + }
>
> > > > +
>
> > > > /* Make the source viewport size an exact multiple of the
>
> > > scaling factors. */
>
> > > > drm_rect_adjust_size(src,
>
> > > > drm_rect_width(dst) * hscale -
>
> > > drm_rect_width(src),
>
> > > > drm_rect_height(dst) * vscale -
>
> > > drm_rect_height(src));
>
> > > >
>
> > > > +nv12_min_no_clip:
>
> > > > drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16,
>
> > > > state->base.rotation);
>
> > > >
>
> > > > @@ -1105,6 +1114,12 @@ intel_check_sprite_plane(struct intel_plane
>
> > > *plane,
>
> > > > src->x2 = (src_x + src_w) << 16;
>
> > > > src->y1 = src_y << 16;
>
> > > > src->y2 = (src_y + src_h) << 16;
>
> > > > + if (fb->format->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;
>
> > > > + }
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for NV12
2018-04-17 6:03 ` [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for NV12 Saarinen, Jani
@ 2018-04-17 7:25 ` Maarten Lankhorst
2018-04-17 7:38 ` Srinivas, Vidya
0 siblings, 1 reply; 20+ messages in thread
From: Maarten Lankhorst @ 2018-04-17 7:25 UTC (permalink / raw)
To: Saarinen, Jani, Srinivas, Vidya, intel-gfx-trybot, intel-gfx
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@linux.intel.com>; intel-gfx-
>> trybot@lists.freedesktop.org
>> Cc: Kamath, Sunil <sunil.kamath@intel.com>; Saarinen, Jani
>> <jani.saarinen@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@linux.intel.com]
>>> Sent: Monday, April 16, 2018 8:00 PM
>>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-gfx-
>>> trybot@lists.freedesktop.org
>>> Cc: Kamath, Sunil <sunil.kamath@intel.com>; Saarinen, Jani
>>> <jani.saarinen@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@linux.intel.com>
>>>> Signed-off-by: Vidya Srinivas <vidya.srinivas@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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for NV12
2018-04-17 7:25 ` Maarten Lankhorst
@ 2018-04-17 7:38 ` Srinivas, Vidya
2018-04-17 7:50 ` Maarten Lankhorst
0 siblings, 1 reply; 20+ messages in thread
From: Srinivas, Vidya @ 2018-04-17 7:38 UTC (permalink / raw)
To: Maarten Lankhorst, Saarinen, Jani, intel-gfx-trybot, intel-gfx
> -----Original Message-----
> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com]
> Sent: Tuesday, April 17, 2018 12:55 PM
> To: Saarinen, Jani <jani.saarinen@intel.com>; Srinivas, Vidya
> <vidya.srinivas@intel.com>; intel-gfx-trybot@lists.freedesktop.org; intel-
> gfx@lists.freedesktop.org
> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com>; intel-gfx-
> >> trybot@lists.freedesktop.org
> >> Cc: Kamath, Sunil <sunil.kamath@intel.com>; Saarinen, Jani
> >> <jani.saarinen@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@linux.intel.com]
> >>> Sent: Monday, April 16, 2018 8:00 PM
> >>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-gfx-
> >>> trybot@lists.freedesktop.org
> >>> Cc: Kamath, Sunil <sunil.kamath@intel.com>; Saarinen, Jani
> >>> <jani.saarinen@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@linux.intel.com>
> >>>> Signed-off-by: Vidya Srinivas <vidya.srinivas@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).
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for NV12
2018-04-17 7:38 ` Srinivas, Vidya
@ 2018-04-17 7:50 ` Maarten Lankhorst
2018-04-17 8:02 ` Srinivas, Vidya
2018-04-17 8:09 ` Srinivas, Vidya
0 siblings, 2 replies; 20+ messages in thread
From: Maarten Lankhorst @ 2018-04-17 7:50 UTC (permalink / raw)
To: Srinivas, Vidya, Saarinen, Jani, intel-gfx
Op 17-04-18 om 09:38 schreef Srinivas, Vidya:
>
>> -----Original Message-----
>> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com]
>> Sent: Tuesday, April 17, 2018 12:55 PM
>> To: Saarinen, Jani <jani.saarinen@intel.com>; Srinivas, Vidya
>> <vidya.srinivas@intel.com>; intel-gfx-trybot@lists.freedesktop.org; intel-
>> gfx@lists.freedesktop.org
>> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com>; intel-gfx-
>>>> trybot@lists.freedesktop.org
>>>> Cc: Kamath, Sunil <sunil.kamath@intel.com>; Saarinen, Jani
>>>> <jani.saarinen@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@linux.intel.com]
>>>>> Sent: Monday, April 16, 2018 8:00 PM
>>>>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-gfx-
>>>>> trybot@lists.freedesktop.org
>>>>> Cc: Kamath, Sunil <sunil.kamath@intel.com>; Saarinen, Jani
>>>>> <jani.saarinen@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@linux.intel.com>
>>>>>> Signed-off-by: Vidya Srinivas <vidya.srinivas@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.
~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for NV12
2018-04-17 7:50 ` Maarten Lankhorst
@ 2018-04-17 8:02 ` Srinivas, Vidya
2018-04-17 8:09 ` Srinivas, Vidya
1 sibling, 0 replies; 20+ messages in thread
From: Srinivas, Vidya @ 2018-04-17 8:02 UTC (permalink / raw)
To: Maarten Lankhorst, Saarinen, Jani, intel-gfx
> -----Original Message-----
> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com]
> Sent: Tuesday, April 17, 2018 1:21 PM
> To: Srinivas, Vidya <vidya.srinivas@intel.com>; Saarinen, Jani
> <jani.saarinen@intel.com>; intel-gfx@lists.freedesktop.org
> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com]
> >> Sent: Tuesday, April 17, 2018 12:55 PM
> >> To: Saarinen, Jani <jani.saarinen@intel.com>; Srinivas, Vidya
> >> <vidya.srinivas@intel.com>; intel-gfx-trybot@lists.freedesktop.org;
> >> intel- gfx@lists.freedesktop.org
> >> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com>;
> >>>> intel-gfx- trybot@lists.freedesktop.org
> >>>> Cc: Kamath, Sunil <sunil.kamath@intel.com>; Saarinen, Jani
> >>>> <jani.saarinen@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@linux.intel.com]
> >>>>> Sent: Monday, April 16, 2018 8:00 PM
> >>>>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-gfx-
> >>>>> trybot@lists.freedesktop.org
> >>>>> Cc: Kamath, Sunil <sunil.kamath@intel.com>; Saarinen, Jani
> >>>>> <jani.saarinen@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@linux.intel.com>
> >>>>>> Signed-off-by: Vidya Srinivas <vidya.srinivas@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?
Regards
Vidya
>
> ~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for NV12
2018-04-17 7:50 ` Maarten Lankhorst
2018-04-17 8:02 ` Srinivas, Vidya
@ 2018-04-17 8:09 ` Srinivas, Vidya
2018-04-17 9:34 ` Maarten Lankhorst
1 sibling, 1 reply; 20+ messages in thread
From: Srinivas, Vidya @ 2018-04-17 8:09 UTC (permalink / raw)
To: Maarten Lankhorst, Saarinen, Jani, intel-gfx
> -----Original Message-----
> From: Srinivas, Vidya
> Sent: Tuesday, April 17, 2018 1:32 PM
> To: 'Maarten Lankhorst' <maarten.lankhorst@linux.intel.com>; Saarinen,
> Jani <jani.saarinen@intel.com>; intel-gfx@lists.freedesktop.org
> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com]
> > Sent: Tuesday, April 17, 2018 1:21 PM
> > To: Srinivas, Vidya <vidya.srinivas@intel.com>; Saarinen, Jani
> > <jani.saarinen@intel.com>; intel-gfx@lists.freedesktop.org
> > Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com]
> > >> Sent: Tuesday, April 17, 2018 12:55 PM
> > >> To: Saarinen, Jani <jani.saarinen@intel.com>; Srinivas, Vidya
> > >> <vidya.srinivas@intel.com>; intel-gfx-trybot@lists.freedesktop.org;
> > >> intel- gfx@lists.freedesktop.org
> > >> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com>;
> > >>>> intel-gfx- trybot@lists.freedesktop.org
> > >>>> Cc: Kamath, Sunil <sunil.kamath@intel.com>; Saarinen, Jani
> > >>>> <jani.saarinen@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@linux.intel.com]
> > >>>>> Sent: Monday, April 16, 2018 8:00 PM
> > >>>>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-gfx-
> > >>>>> trybot@lists.freedesktop.org
> > >>>>> Cc: Kamath, Sunil <sunil.kamath@intel.com>; Saarinen, Jani
> > >>>>> <jani.saarinen@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@linux.intel.com>
> > >>>>>> Signed-off-by: Vidya Srinivas <vidya.srinivas@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.
> Regards
> Vidya
> >
> > ~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for NV12
2018-04-17 8:09 ` Srinivas, Vidya
@ 2018-04-17 9:34 ` Maarten Lankhorst
2018-04-17 9:42 ` Srinivas, Vidya
0 siblings, 1 reply; 20+ messages in thread
From: Maarten Lankhorst @ 2018-04-17 9:34 UTC (permalink / raw)
To: Srinivas, Vidya, Saarinen, Jani, intel-gfx
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@linux.intel.com>; Saarinen,
>> Jani <jani.saarinen@intel.com>; intel-gfx@lists.freedesktop.org
>> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com]
>>> Sent: Tuesday, April 17, 2018 1:21 PM
>>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; Saarinen, Jani
>>> <jani.saarinen@intel.com>; intel-gfx@lists.freedesktop.org
>>> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com]
>>>>> Sent: Tuesday, April 17, 2018 12:55 PM
>>>>> To: Saarinen, Jani <jani.saarinen@intel.com>; Srinivas, Vidya
>>>>> <vidya.srinivas@intel.com>; intel-gfx-trybot@lists.freedesktop.org;
>>>>> intel- gfx@lists.freedesktop.org
>>>>> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com>;
>>>>>>> intel-gfx- trybot@lists.freedesktop.org
>>>>>>> Cc: Kamath, Sunil <sunil.kamath@intel.com>; Saarinen, Jani
>>>>>>> <jani.saarinen@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@linux.intel.com]
>>>>>>>> Sent: Monday, April 16, 2018 8:00 PM
>>>>>>>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-gfx-
>>>>>>>> trybot@lists.freedesktop.org
>>>>>>>> Cc: Kamath, Sunil <sunil.kamath@intel.com>; Saarinen, Jani
>>>>>>>> <jani.saarinen@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@linux.intel.com>
>>>>>>>>> Signed-off-by: Vidya Srinivas <vidya.srinivas@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;
+}
+
static int skl_check_nv12_aux_surface(struct intel_plane_state *plane_state)
{
const struct drm_framebuffer *fb = plane_state->base.fb;
@@ -3162,6 +3194,12 @@ int skl_check_plane_surface(const struct intel_crtc_state *crtc_state,
if (!plane_state->base.visible)
return 0;
+ if (fb->format->format == DRM_FORMAT_NV12) {
+ ret = skl_check_nv12_surface(crtc_state, plane_state);
+ if (ret)
+ return ret;
+ }
+
/* Rotate src coordinates to match rotated GTT view */
if (drm_rotation_90_or_270(rotation))
drm_rect_rotate(&plane_state->base.src,
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for NV12
2018-04-17 9:34 ` Maarten Lankhorst
@ 2018-04-17 9:42 ` Srinivas, Vidya
2018-04-17 10:01 ` Maarten Lankhorst
0 siblings, 1 reply; 20+ messages in thread
From: Srinivas, Vidya @ 2018-04-17 9:42 UTC (permalink / raw)
To: Maarten Lankhorst, Saarinen, Jani, intel-gfx
> -----Original Message-----
> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com]
> Sent: Tuesday, April 17, 2018 3:04 PM
> To: Srinivas, Vidya <vidya.srinivas@intel.com>; Saarinen, Jani
> <jani.saarinen@intel.com>; intel-gfx@lists.freedesktop.org
> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com>;
> >> Saarinen, Jani <jani.saarinen@intel.com>;
> >> intel-gfx@lists.freedesktop.org
> >> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com]
> >>> Sent: Tuesday, April 17, 2018 1:21 PM
> >>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; Saarinen, Jani
> >>> <jani.saarinen@intel.com>; intel-gfx@lists.freedesktop.org
> >>> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com]
> >>>>> Sent: Tuesday, April 17, 2018 12:55 PM
> >>>>> To: Saarinen, Jani <jani.saarinen@intel.com>; Srinivas, Vidya
> >>>>> <vidya.srinivas@intel.com>;
> >>>>> intel-gfx-trybot@lists.freedesktop.org;
> >>>>> intel- gfx@lists.freedesktop.org
> >>>>> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com>;
> >>>>>>> intel-gfx- trybot@lists.freedesktop.org
> >>>>>>> Cc: Kamath, Sunil <sunil.kamath@intel.com>; Saarinen, Jani
> >>>>>>> <jani.saarinen@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@linux.intel.com]
> >>>>>>>> Sent: Monday, April 16, 2018 8:00 PM
> >>>>>>>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-gfx-
> >>>>>>>> trybot@lists.freedesktop.org
> >>>>>>>> Cc: Kamath, Sunil <sunil.kamath@intel.com>; Saarinen, Jani
> >>>>>>>> <jani.saarinen@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@linux.intel.com>
> >>>>>>>>> Signed-off-by: Vidya Srinivas <vidya.srinivas@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?
> static int skl_check_nv12_aux_surface(struct intel_plane_state
> *plane_state) {
> const struct drm_framebuffer *fb = plane_state->base.fb; @@ -
> 3162,6 +3194,12 @@ int skl_check_plane_surface(const struct
> intel_crtc_state *crtc_state,
> if (!plane_state->base.visible)
> return 0;
>
> + if (fb->format->format == DRM_FORMAT_NV12) {
> + ret = skl_check_nv12_surface(crtc_state, plane_state);
> + if (ret)
> + return ret;
> + }
> +
> /* Rotate src coordinates to match rotated GTT view */
> if (drm_rotation_90_or_270(rotation))
> drm_rect_rotate(&plane_state->base.src,
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for NV12
2018-04-17 9:42 ` Srinivas, Vidya
@ 2018-04-17 10:01 ` Maarten Lankhorst
2018-04-17 10:18 ` Srinivas, Vidya
` (2 more replies)
0 siblings, 3 replies; 20+ messages in thread
From: Maarten Lankhorst @ 2018-04-17 10:01 UTC (permalink / raw)
To: Srinivas, Vidya, Saarinen, Jani, intel-gfx
Op 17-04-18 om 11:42 schreef Srinivas, Vidya:
>
>> -----Original Message-----
>> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com]
>> Sent: Tuesday, April 17, 2018 3:04 PM
>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; Saarinen, Jani
>> <jani.saarinen@intel.com>; intel-gfx@lists.freedesktop.org
>> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com>;
>>>> Saarinen, Jani <jani.saarinen@intel.com>;
>>>> intel-gfx@lists.freedesktop.org
>>>> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com]
>>>>> Sent: Tuesday, April 17, 2018 1:21 PM
>>>>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; Saarinen, Jani
>>>>> <jani.saarinen@intel.com>; intel-gfx@lists.freedesktop.org
>>>>> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com]
>>>>>>> Sent: Tuesday, April 17, 2018 12:55 PM
>>>>>>> To: Saarinen, Jani <jani.saarinen@intel.com>; Srinivas, Vidya
>>>>>>> <vidya.srinivas@intel.com>;
>>>>>>> intel-gfx-trybot@lists.freedesktop.org;
>>>>>>> intel- gfx@lists.freedesktop.org
>>>>>>> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com>;
>>>>>>>>> intel-gfx- trybot@lists.freedesktop.org
>>>>>>>>> Cc: Kamath, Sunil <sunil.kamath@intel.com>; Saarinen, Jani
>>>>>>>>> <jani.saarinen@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@linux.intel.com]
>>>>>>>>>> Sent: Monday, April 16, 2018 8:00 PM
>>>>>>>>>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-gfx-
>>>>>>>>>> trybot@lists.freedesktop.org
>>>>>>>>>> Cc: Kamath, Sunil <sunil.kamath@intel.com>; Saarinen, Jani
>>>>>>>>>> <jani.saarinen@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@linux.intel.com>
>>>>>>>>>>> Signed-off-by: Vidya Srinivas <vidya.srinivas@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for NV12
2018-04-17 10:01 ` Maarten Lankhorst
@ 2018-04-17 10:18 ` Srinivas, Vidya
2018-04-17 10:48 ` Srinivas, Vidya
2018-04-18 4:16 ` Srinivas, Vidya
2 siblings, 0 replies; 20+ messages in thread
From: Srinivas, Vidya @ 2018-04-17 10:18 UTC (permalink / raw)
To: Maarten Lankhorst, Saarinen, Jani, intel-gfx
> -----Original Message-----
> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com]
> Sent: Tuesday, April 17, 2018 3:32 PM
> To: Srinivas, Vidya <vidya.srinivas@intel.com>; Saarinen, Jani
> <jani.saarinen@intel.com>; intel-gfx@lists.freedesktop.org
> Cc: Kamath, Sunil <sunil.kamath@intel.com>
> Subject: Re: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for
> NV12
>
> Op 17-04-18 om 11:42 schreef Srinivas, Vidya:
> >
> >> -----Original Message-----
> >> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com]
> >> Sent: Tuesday, April 17, 2018 3:04 PM
> >> To: Srinivas, Vidya <vidya.srinivas@intel.com>; Saarinen, Jani
> >> <jani.saarinen@intel.com>; intel-gfx@lists.freedesktop.org
> >> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com>;
> >>>> Saarinen, Jani <jani.saarinen@intel.com>;
> >>>> intel-gfx@lists.freedesktop.org
> >>>> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com]
> >>>>> Sent: Tuesday, April 17, 2018 1:21 PM
> >>>>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; Saarinen, Jani
> >>>>> <jani.saarinen@intel.com>; intel-gfx@lists.freedesktop.org
> >>>>> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com]
> >>>>>>> Sent: Tuesday, April 17, 2018 12:55 PM
> >>>>>>> To: Saarinen, Jani <jani.saarinen@intel.com>; Srinivas, Vidya
> >>>>>>> <vidya.srinivas@intel.com>;
> >>>>>>> intel-gfx-trybot@lists.freedesktop.org;
> >>>>>>> intel- gfx@lists.freedesktop.org
> >>>>>>> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com>;
> >>>>>>>>> intel-gfx- trybot@lists.freedesktop.org
> >>>>>>>>> Cc: Kamath, Sunil <sunil.kamath@intel.com>; Saarinen, Jani
> >>>>>>>>> <jani.saarinen@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@linux.intel.com]
> >>>>>>>>>> Sent: Monday, April 16, 2018 8:00 PM
> >>>>>>>>>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-gfx-
> >>>>>>>>>> trybot@lists.freedesktop.org
> >>>>>>>>>> Cc: Kamath, Sunil <sunil.kamath@intel.com>; Saarinen, Jani
> >>>>>>>>>> <jani.saarinen@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@linux.intel.com>
> >>>>>>>>>>> Signed-off-by: Vidya Srinivas <vidya.srinivas@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. :)
Oh sorry. I will put this patch. Thanks so much.
>
> ~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for NV12
2018-04-17 10:01 ` Maarten Lankhorst
2018-04-17 10:18 ` Srinivas, Vidya
@ 2018-04-17 10:48 ` Srinivas, Vidya
2018-04-17 10:52 ` Saarinen, Jani
2018-04-18 4:16 ` Srinivas, Vidya
2 siblings, 1 reply; 20+ messages in thread
From: Srinivas, Vidya @ 2018-04-17 10:48 UTC (permalink / raw)
To: Maarten Lankhorst, Saarinen, Jani, intel-gfx
> -----Original Message-----
> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com]
> Sent: Tuesday, April 17, 2018 3:32 PM
> To: Srinivas, Vidya <vidya.srinivas@intel.com>; Saarinen, Jani
> <jani.saarinen@intel.com>; intel-gfx@lists.freedesktop.org
> Cc: Kamath, Sunil <sunil.kamath@intel.com>
> Subject: Re: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for
> NV12
>
> Op 17-04-18 om 11:42 schreef Srinivas, Vidya:
> >
> >> -----Original Message-----
> >> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com]
> >> Sent: Tuesday, April 17, 2018 3:04 PM
> >> To: Srinivas, Vidya <vidya.srinivas@intel.com>; Saarinen, Jani
> >> <jani.saarinen@intel.com>; intel-gfx@lists.freedesktop.org
> >> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com>;
> >>>> Saarinen, Jani <jani.saarinen@intel.com>;
> >>>> intel-gfx@lists.freedesktop.org
> >>>> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com]
> >>>>> Sent: Tuesday, April 17, 2018 1:21 PM
> >>>>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; Saarinen, Jani
> >>>>> <jani.saarinen@intel.com>; intel-gfx@lists.freedesktop.org
> >>>>> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com]
> >>>>>>> Sent: Tuesday, April 17, 2018 12:55 PM
> >>>>>>> To: Saarinen, Jani <jani.saarinen@intel.com>; Srinivas, Vidya
> >>>>>>> <vidya.srinivas@intel.com>;
> >>>>>>> intel-gfx-trybot@lists.freedesktop.org;
> >>>>>>> intel- gfx@lists.freedesktop.org
> >>>>>>> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com>;
> >>>>>>>>> intel-gfx- trybot@lists.freedesktop.org
> >>>>>>>>> Cc: Kamath, Sunil <sunil.kamath@intel.com>; Saarinen, Jani
> >>>>>>>>> <jani.saarinen@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@linux.intel.com]
> >>>>>>>>>> Sent: Monday, April 16, 2018 8:00 PM
> >>>>>>>>>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-gfx-
> >>>>>>>>>> trybot@lists.freedesktop.org
> >>>>>>>>>> Cc: Kamath, Sunil <sunil.kamath@intel.com>; Saarinen, Jani
> >>>>>>>>>> <jani.saarinen@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@linux.intel.com>
> >>>>>>>>>>> Signed-off-by: Vidya Srinivas <vidya.srinivas@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.
Thank you so much. Have submitted the changes to trybot
https://patchwork.freedesktop.org/series/41532/
PS: fb creation to not allow CCS - not done in this series
Regards
Vidya
> >>
> >> ~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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for NV12
2018-04-17 10:48 ` Srinivas, Vidya
@ 2018-04-17 10:52 ` Saarinen, Jani
2018-04-18 3:55 ` Srinivas, Vidya
0 siblings, 1 reply; 20+ messages in thread
From: Saarinen, Jani @ 2018-04-17 10:52 UTC (permalink / raw)
To: Srinivas, Vidya, Maarten Lankhorst, intel-gfx
HI,
> -----Original Message-----
> From: Srinivas, Vidya
> Sent: tiistai 17. huhtikuuta 2018 13.49
> To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>; Saarinen, Jani
> <jani.saarinen@intel.com>; intel-gfx@lists.freedesktop.org
> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com]
> > Sent: Tuesday, April 17, 2018 3:32 PM
> > To: Srinivas, Vidya <vidya.srinivas@intel.com>; Saarinen, Jani
> > <jani.saarinen@intel.com>; intel-gfx@lists.freedesktop.org
> > Cc: Kamath, Sunil <sunil.kamath@intel.com>
> > Subject: Re: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for
> > NV12
> >
> > Op 17-04-18 om 11:42 schreef Srinivas, Vidya:
> > >
> > >> -----Original Message-----
> > >> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com]
> > >> Sent: Tuesday, April 17, 2018 3:04 PM
> > >> To: Srinivas, Vidya <vidya.srinivas@intel.com>; Saarinen, Jani
> > >> <jani.saarinen@intel.com>; intel-gfx@lists.freedesktop.org
> > >> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com>;
> > >>>> Saarinen, Jani <jani.saarinen@intel.com>;
> > >>>> intel-gfx@lists.freedesktop.org
> > >>>> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com]
> > >>>>> Sent: Tuesday, April 17, 2018 1:21 PM
> > >>>>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; Saarinen, Jani
> > >>>>> <jani.saarinen@intel.com>; intel-gfx@lists.freedesktop.org
> > >>>>> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com]
> > >>>>>>> Sent: Tuesday, April 17, 2018 12:55 PM
> > >>>>>>> To: Saarinen, Jani <jani.saarinen@intel.com>; Srinivas, Vidya
> > >>>>>>> <vidya.srinivas@intel.com>;
> > >>>>>>> intel-gfx-trybot@lists.freedesktop.org;
> > >>>>>>> intel- gfx@lists.freedesktop.org
> > >>>>>>> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com>;
> > >>>>>>>>> intel-gfx- trybot@lists.freedesktop.org
> > >>>>>>>>> Cc: Kamath, Sunil <sunil.kamath@intel.com>; Saarinen, Jani
> > >>>>>>>>> <jani.saarinen@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@linux.intel.com]
> > >>>>>>>>>> Sent: Monday, April 16, 2018 8:00 PM
> > >>>>>>>>>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-gfx-
> > >>>>>>>>>> trybot@lists.freedesktop.org
> > >>>>>>>>>> Cc: Kamath, Sunil <sunil.kamath@intel.com>; Saarinen, Jani
> > >>>>>>>>>> <jani.saarinen@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@linux.intel.com>
> > >>>>>>>>>>> Signed-off-by: Vidya Srinivas <vidya.srinivas@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.
>
> Thank you so much. Have submitted the changes to trybot
> https://patchwork.freedesktop.org/series/41532/
> PS: fb creation to not allow CCS - not done in this series
Why not directly to intel-gfx?
>
> Regards
> Vidya
>
> > >>
> > >> ~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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for NV12
2018-04-17 10:52 ` Saarinen, Jani
@ 2018-04-18 3:55 ` Srinivas, Vidya
2018-04-18 5:36 ` Saarinen, Jani
0 siblings, 1 reply; 20+ messages in thread
From: Srinivas, Vidya @ 2018-04-18 3:55 UTC (permalink / raw)
To: Saarinen, Jani, Maarten Lankhorst, intel-gfx
> -----Original Message-----
> From: Saarinen, Jani
> Sent: Tuesday, April 17, 2018 4:23 PM
> To: Srinivas, Vidya <vidya.srinivas@intel.com>; Maarten Lankhorst
> <maarten.lankhorst@linux.intel.com>; intel-gfx@lists.freedesktop.org
> Cc: Kamath, Sunil <sunil.kamath@intel.com>
> Subject: RE: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for
> NV12
>
> HI,
> > -----Original Message-----
> > From: Srinivas, Vidya
> > Sent: tiistai 17. huhtikuuta 2018 13.49
> > To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>; Saarinen,
> > Jani <jani.saarinen@intel.com>; intel-gfx@lists.freedesktop.org
> > Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com]
> > > Sent: Tuesday, April 17, 2018 3:32 PM
> > > To: Srinivas, Vidya <vidya.srinivas@intel.com>; Saarinen, Jani
> > > <jani.saarinen@intel.com>; intel-gfx@lists.freedesktop.org
> > > Cc: Kamath, Sunil <sunil.kamath@intel.com>
> > > Subject: Re: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments
> > > for
> > > NV12
> > >
> > > Op 17-04-18 om 11:42 schreef Srinivas, Vidya:
> > > >
> > > >> -----Original Message-----
> > > >> From: Maarten Lankhorst
> > > >> [mailto:maarten.lankhorst@linux.intel.com]
> > > >> Sent: Tuesday, April 17, 2018 3:04 PM
> > > >> To: Srinivas, Vidya <vidya.srinivas@intel.com>; Saarinen, Jani
> > > >> <jani.saarinen@intel.com>; intel-gfx@lists.freedesktop.org
> > > >> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com>;
> > > >>>> Saarinen, Jani <jani.saarinen@intel.com>;
> > > >>>> intel-gfx@lists.freedesktop.org
> > > >>>> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com]
> > > >>>>> Sent: Tuesday, April 17, 2018 1:21 PM
> > > >>>>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; Saarinen, Jani
> > > >>>>> <jani.saarinen@intel.com>; intel-gfx@lists.freedesktop.org
> > > >>>>> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com]
> > > >>>>>>> Sent: Tuesday, April 17, 2018 12:55 PM
> > > >>>>>>> To: Saarinen, Jani <jani.saarinen@intel.com>; Srinivas,
> > > >>>>>>> Vidya <vidya.srinivas@intel.com>;
> > > >>>>>>> intel-gfx-trybot@lists.freedesktop.org;
> > > >>>>>>> intel- gfx@lists.freedesktop.org
> > > >>>>>>> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com>;
> > > >>>>>>>>> intel-gfx- trybot@lists.freedesktop.org
> > > >>>>>>>>> Cc: Kamath, Sunil <sunil.kamath@intel.com>; Saarinen, Jani
> > > >>>>>>>>> <jani.saarinen@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@linux.intel.com]
> > > >>>>>>>>>> Sent: Monday, April 16, 2018 8:00 PM
> > > >>>>>>>>>> To: Srinivas, Vidya <vidya.srinivas@intel.com>;
> > > >>>>>>>>>> intel-gfx- trybot@lists.freedesktop.org
> > > >>>>>>>>>> Cc: Kamath, Sunil <sunil.kamath@intel.com>; Saarinen,
> > > >>>>>>>>>> Jani <jani.saarinen@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@linux.intel.com>
> > > >>>>>>>>>>> Signed-off-by: Vidya Srinivas <vidya.srinivas@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.
> >
> > Thank you so much. Have submitted the changes to trybot
> > https://patchwork.freedesktop.org/series/41532/
> > PS: fb creation to not allow CCS - not done in this series
> Why not directly to intel-gfx?
Sure, I thought I had to send to trybot first. Will send it today.
Thanks
Vidya
>
>
> >
> > Regards
> > Vidya
> >
> > > >>
> > > >> ~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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for NV12
2018-04-17 10:01 ` Maarten Lankhorst
2018-04-17 10:18 ` Srinivas, Vidya
2018-04-17 10:48 ` Srinivas, Vidya
@ 2018-04-18 4:16 ` Srinivas, Vidya
2 siblings, 0 replies; 20+ messages in thread
From: Srinivas, Vidya @ 2018-04-18 4:16 UTC (permalink / raw)
To: Maarten Lankhorst, Saarinen, Jani, intel-gfx
> -----Original Message-----
> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com]
> Sent: Tuesday, April 17, 2018 3:32 PM
> To: Srinivas, Vidya <vidya.srinivas@intel.com>; Saarinen, Jani
> <jani.saarinen@intel.com>; intel-gfx@lists.freedesktop.org
> Cc: Kamath, Sunil <sunil.kamath@intel.com>
> Subject: Re: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for
> NV12
>
> Op 17-04-18 om 11:42 schreef Srinivas, Vidya:
> >
> >> -----Original Message-----
> >> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com]
> >> Sent: Tuesday, April 17, 2018 3:04 PM
> >> To: Srinivas, Vidya <vidya.srinivas@intel.com>; Saarinen, Jani
> >> <jani.saarinen@intel.com>; intel-gfx@lists.freedesktop.org
> >> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com>;
> >>>> Saarinen, Jani <jani.saarinen@intel.com>;
> >>>> intel-gfx@lists.freedesktop.org
> >>>> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com]
> >>>>> Sent: Tuesday, April 17, 2018 1:21 PM
> >>>>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; Saarinen, Jani
> >>>>> <jani.saarinen@intel.com>; intel-gfx@lists.freedesktop.org
> >>>>> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com]
> >>>>>>> Sent: Tuesday, April 17, 2018 12:55 PM
> >>>>>>> To: Saarinen, Jani <jani.saarinen@intel.com>; Srinivas, Vidya
> >>>>>>> <vidya.srinivas@intel.com>;
> >>>>>>> intel-gfx-trybot@lists.freedesktop.org;
> >>>>>>> intel- gfx@lists.freedesktop.org
> >>>>>>> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com>;
> >>>>>>>>> intel-gfx- trybot@lists.freedesktop.org
> >>>>>>>>> Cc: Kamath, Sunil <sunil.kamath@intel.com>; Saarinen, Jani
> >>>>>>>>> <jani.saarinen@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@linux.intel.com]
> >>>>>>>>>> Sent: Monday, April 16, 2018 8:00 PM
> >>>>>>>>>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-gfx-
> >>>>>>>>>> trybot@lists.freedesktop.org
> >>>>>>>>>> Cc: Kamath, Sunil <sunil.kamath@intel.com>; Saarinen, Jani
> >>>>>>>>>> <jani.saarinen@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@linux.intel.com>
> >>>>>>>>>>> Signed-off-by: Vidya Srinivas <vidya.srinivas@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.
Thank you for all the help. Have floated the series with your RB.
Have also included the change you mentioned in framebuffer_init
Not to allow CCS formats. However, when I checked code in framebuffer_init
If the modifier is CCS, only RGB8888 related are allowed.
Please have a check. If you have any suggestion, I will change it.
https://patchwork.freedesktop.org/series/41674/ Rev2
Regards
Vidya
> >>
> >> ~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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for NV12
2018-04-18 3:55 ` Srinivas, Vidya
@ 2018-04-18 5:36 ` Saarinen, Jani
2018-04-18 5:55 ` Srinivas, Vidya
0 siblings, 1 reply; 20+ messages in thread
From: Saarinen, Jani @ 2018-04-18 5:36 UTC (permalink / raw)
To: Srinivas, Vidya, Maarten Lankhorst, intel-gfx
Hi,
> -----Original Message-----
> From: Srinivas, Vidya
> Sent: keskiviikko 18. huhtikuuta 2018 6.56
> To: Saarinen, Jani <jani.saarinen@intel.com>; Maarten Lankhorst
> <maarten.lankhorst@linux.intel.com>; intel-gfx@lists.freedesktop.org
> Cc: Kamath, Sunil <sunil.kamath@intel.com>
> Subject: RE: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for NV12
>
>
>
> > -----Original Message-----
> > From: Saarinen, Jani
> > Sent: Tuesday, April 17, 2018 4:23 PM
> > To: Srinivas, Vidya <vidya.srinivas@intel.com>; Maarten Lankhorst
> > <maarten.lankhorst@linux.intel.com>; intel-gfx@lists.freedesktop.org
> > Cc: Kamath, Sunil <sunil.kamath@intel.com>
> > Subject: RE: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for
> > NV12
> >
> > HI,
> > > -----Original Message-----
> > > From: Srinivas, Vidya
> > > Sent: tiistai 17. huhtikuuta 2018 13.49
> > > To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>; Saarinen,
> > > Jani <jani.saarinen@intel.com>; intel-gfx@lists.freedesktop.org
> > > Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com]
> > > > Sent: Tuesday, April 17, 2018 3:32 PM
> > > > To: Srinivas, Vidya <vidya.srinivas@intel.com>; Saarinen, Jani
> > > > <jani.saarinen@intel.com>; intel-gfx@lists.freedesktop.org
> > > > Cc: Kamath, Sunil <sunil.kamath@intel.com>
> > > > Subject: Re: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments
> > > > for
> > > > NV12
> > > >
> > > > Op 17-04-18 om 11:42 schreef Srinivas, Vidya:
> > > > >
> > > > >> -----Original Message-----
> > > > >> From: Maarten Lankhorst
> > > > >> [mailto:maarten.lankhorst@linux.intel.com]
> > > > >> Sent: Tuesday, April 17, 2018 3:04 PM
> > > > >> To: Srinivas, Vidya <vidya.srinivas@intel.com>; Saarinen, Jani
> > > > >> <jani.saarinen@intel.com>; intel-gfx@lists.freedesktop.org
> > > > >> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com>;
> > > > >>>> Saarinen, Jani <jani.saarinen@intel.com>;
> > > > >>>> intel-gfx@lists.freedesktop.org
> > > > >>>> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com]
> > > > >>>>> Sent: Tuesday, April 17, 2018 1:21 PM
> > > > >>>>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; Saarinen,
> > > > >>>>> Jani <jani.saarinen@intel.com>;
> > > > >>>>> intel-gfx@lists.freedesktop.org
> > > > >>>>> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com]
> > > > >>>>>>> Sent: Tuesday, April 17, 2018 12:55 PM
> > > > >>>>>>> To: Saarinen, Jani <jani.saarinen@intel.com>; Srinivas,
> > > > >>>>>>> Vidya <vidya.srinivas@intel.com>;
> > > > >>>>>>> intel-gfx-trybot@lists.freedesktop.org;
> > > > >>>>>>> intel- gfx@lists.freedesktop.org
> > > > >>>>>>> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com>;
> > > > >>>>>>>>> intel-gfx- trybot@lists.freedesktop.org
> > > > >>>>>>>>> Cc: Kamath, Sunil <sunil.kamath@intel.com>; Saarinen,
> > > > >>>>>>>>> Jani <jani.saarinen@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@linux.intel.com]
> > > > >>>>>>>>>> Sent: Monday, April 16, 2018 8:00 PM
> > > > >>>>>>>>>> To: Srinivas, Vidya <vidya.srinivas@intel.com>;
> > > > >>>>>>>>>> intel-gfx- trybot@lists.freedesktop.org
> > > > >>>>>>>>>> Cc: Kamath, Sunil <sunil.kamath@intel.com>; Saarinen,
> > > > >>>>>>>>>> Jani <jani.saarinen@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@linux.intel.com>
> > > > >>>>>>>>>>> Signed-off-by: Vidya Srinivas
> > > > >>>>>>>>>>> <vidya.srinivas@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.
> > >
> > > Thank you so much. Have submitted the changes to trybot
> > > https://patchwork.freedesktop.org/series/41532/
> > > PS: fb creation to not allow CCS - not done in this series
> > Why not directly to intel-gfx?
> Sure, I thought I had to send to trybot first. Will send it today.
I see trybot for patches as test for RFC to see if solution is even near satisfactory in the beginning but for patches that has sent several times no need to send to trybot.
>
> Thanks
> Vidya
> >
> >
> > >
> > > Regards
> > > Vidya
> > >
> > > > >>
> > > > >> ~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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for NV12
2018-04-18 5:36 ` Saarinen, Jani
@ 2018-04-18 5:55 ` Srinivas, Vidya
0 siblings, 0 replies; 20+ messages in thread
From: Srinivas, Vidya @ 2018-04-18 5:55 UTC (permalink / raw)
To: Saarinen, Jani, Maarten Lankhorst, intel-gfx
> -----Original Message-----
> From: Saarinen, Jani
> Sent: Wednesday, April 18, 2018 11:07 AM
> To: Srinivas, Vidya <vidya.srinivas@intel.com>; Maarten Lankhorst
> <maarten.lankhorst@linux.intel.com>; intel-gfx@lists.freedesktop.org
> Cc: Kamath, Sunil <sunil.kamath@intel.com>
> Subject: RE: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for
> NV12
>
> Hi,
>
> > -----Original Message-----
> > From: Srinivas, Vidya
> > Sent: keskiviikko 18. huhtikuuta 2018 6.56
> > To: Saarinen, Jani <jani.saarinen@intel.com>; Maarten Lankhorst
> > <maarten.lankhorst@linux.intel.com>; intel-gfx@lists.freedesktop.org
> > Cc: Kamath, Sunil <sunil.kamath@intel.com>
> > Subject: RE: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for
> > NV12
> >
> >
> >
> > > -----Original Message-----
> > > From: Saarinen, Jani
> > > Sent: Tuesday, April 17, 2018 4:23 PM
> > > To: Srinivas, Vidya <vidya.srinivas@intel.com>; Maarten Lankhorst
> > > <maarten.lankhorst@linux.intel.com>; intel-gfx@lists.freedesktop.org
> > > Cc: Kamath, Sunil <sunil.kamath@intel.com>
> > > Subject: RE: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments
> > > for
> > > NV12
> > >
> > > HI,
> > > > -----Original Message-----
> > > > From: Srinivas, Vidya
> > > > Sent: tiistai 17. huhtikuuta 2018 13.49
> > > > To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>;
> > > > Saarinen, Jani <jani.saarinen@intel.com>;
> > > > intel-gfx@lists.freedesktop.org
> > > > Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com]
> > > > > Sent: Tuesday, April 17, 2018 3:32 PM
> > > > > To: Srinivas, Vidya <vidya.srinivas@intel.com>; Saarinen, Jani
> > > > > <jani.saarinen@intel.com>; intel-gfx@lists.freedesktop.org
> > > > > Cc: Kamath, Sunil <sunil.kamath@intel.com>
> > > > > Subject: Re: [PATCH v2 6/6] drm/i915: Do not do fb src
> > > > > adjustments for
> > > > > NV12
> > > > >
> > > > > Op 17-04-18 om 11:42 schreef Srinivas, Vidya:
> > > > > >
> > > > > >> -----Original Message-----
> > > > > >> From: Maarten Lankhorst
> > > > > >> [mailto:maarten.lankhorst@linux.intel.com]
> > > > > >> Sent: Tuesday, April 17, 2018 3:04 PM
> > > > > >> To: Srinivas, Vidya <vidya.srinivas@intel.com>; Saarinen,
> > > > > >> Jani <jani.saarinen@intel.com>;
> > > > > >> intel-gfx@lists.freedesktop.org
> > > > > >> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com>;
> > > > > >>>> Saarinen, Jani <jani.saarinen@intel.com>;
> > > > > >>>> intel-gfx@lists.freedesktop.org
> > > > > >>>> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com]
> > > > > >>>>> Sent: Tuesday, April 17, 2018 1:21 PM
> > > > > >>>>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; Saarinen,
> > > > > >>>>> Jani <jani.saarinen@intel.com>;
> > > > > >>>>> intel-gfx@lists.freedesktop.org
> > > > > >>>>> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com]
> > > > > >>>>>>> Sent: Tuesday, April 17, 2018 12:55 PM
> > > > > >>>>>>> To: Saarinen, Jani <jani.saarinen@intel.com>; Srinivas,
> > > > > >>>>>>> Vidya <vidya.srinivas@intel.com>;
> > > > > >>>>>>> intel-gfx-trybot@lists.freedesktop.org;
> > > > > >>>>>>> intel- gfx@lists.freedesktop.org
> > > > > >>>>>>> Cc: Kamath, Sunil <sunil.kamath@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@linux.intel.com>;
> > > > > >>>>>>>>> intel-gfx- trybot@lists.freedesktop.org
> > > > > >>>>>>>>> Cc: Kamath, Sunil <sunil.kamath@intel.com>; Saarinen,
> > > > > >>>>>>>>> Jani <jani.saarinen@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@linux.intel.com]
> > > > > >>>>>>>>>> Sent: Monday, April 16, 2018 8:00 PM
> > > > > >>>>>>>>>> To: Srinivas, Vidya <vidya.srinivas@intel.com>;
> > > > > >>>>>>>>>> intel-gfx- trybot@lists.freedesktop.org
> > > > > >>>>>>>>>> Cc: Kamath, Sunil <sunil.kamath@intel.com>; Saarinen,
> > > > > >>>>>>>>>> Jani <jani.saarinen@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@linux.intel.com>
> > > > > >>>>>>>>>>> Signed-off-by: Vidya Srinivas
> > > > > >>>>>>>>>>> <vidya.srinivas@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.
> > > >
> > > > Thank you so much. Have submitted the changes to trybot
> > > > https://patchwork.freedesktop.org/series/41532/
> > > > PS: fb creation to not allow CCS - not done in this series
> > > Why not directly to intel-gfx?
> > Sure, I thought I had to send to trybot first. Will send it today.
> I see trybot for patches as test for RFC to see if solution is even near
> satisfactory in the beginning but for patches that has sent several times no
> need to send to trybot.
Oh okay. Sure, thank you so much.
Regards
Vidya
>
>
>
> >
> > Thanks
> > Vidya
> > >
> > >
> > > >
> > > > Regards
> > > > Vidya
> > > >
> > > > > >>
> > > > > >> ~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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for NV12
2018-04-17 9:18 ` Mika Kahola
@ 2018-04-17 9:20 ` Srinivas, Vidya
0 siblings, 0 replies; 20+ messages in thread
From: Srinivas, Vidya @ 2018-04-17 9:20 UTC (permalink / raw)
To: Kahola, Mika, intel-gfx
> -----Original Message-----
> From: Kahola, Mika
> Sent: Tuesday, April 17, 2018 2:49 PM
> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-
> gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH v2 6/6] drm/i915: Do not do fb src
> adjustments for NV12
>
> On Fri, 2018-04-13 at 17:44 +0530, Vidya Srinivas wrote:
> > 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@linux.intel.com>
> > Signed-off-by: Vidya Srinivas <vidya.srinivas@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;
> This seems unnecessary. We could use the line below to get the src's.
Sure thank you, am moving all the handling code to skl_check_nv12_aux_surface as suggested
By Maarten. So this wont be necessary.
Regards
Vidya
>
> > +
> > + *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;
> > + }
> > +
> > if (INTEL_GEN(dev_priv) >= 9) {
> > ret = skl_check_plane_surface(crtc_state, state);
> > if (ret)
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> > b/drivers/gpu/drm/i915/intel_sprite.c
> > index 8b7947d..c1dd85e 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -1035,11 +1035,20 @@ intel_check_sprite_plane(struct intel_plane
> > *plane,
> > return vscale;
> > }
> >
> > + if (fb->format->format == DRM_FORMAT_NV12) {
> > + if (src->x2 >> 16 == 16)
> > + src->x2 = 16 << 16;
> > + if (src->y2 >> 16 == 16)
> > + src->y2 = 16 << 16;
> > + goto nv12_min_no_clip;
> > + }
> > +
> > /* Make the source viewport size an exact multiple of the
> scaling
> > factors. */
> > drm_rect_adjust_size(src,
> > drm_rect_width(dst) * hscale -
> drm_rect_width(src),
> > drm_rect_height(dst) * vscale -
> drm_rect_height(src));
> >
> > +nv12_min_no_clip:
> > drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16,
> > state->base.rotation);
> >
> > @@ -1105,6 +1114,12 @@ intel_check_sprite_plane(struct intel_plane
> > *plane,
> > src->x2 = (src_x + src_w) << 16;
> > src->y1 = src_y << 16;
> > src->y2 = (src_y + src_h) << 16;
> > + if (fb->format->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;
> > + }
> > }
> >
> > dst->x1 = crtc_x;
> --
> Mika Kahola - Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for NV12
2018-04-13 12:14 ` [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for NV12 Vidya Srinivas
@ 2018-04-17 9:18 ` Mika Kahola
2018-04-17 9:20 ` Srinivas, Vidya
0 siblings, 1 reply; 20+ messages in thread
From: Mika Kahola @ 2018-04-17 9:18 UTC (permalink / raw)
To: Vidya Srinivas, intel-gfx
On Fri, 2018-04-13 at 17:44 +0530, Vidya Srinivas wrote:
> 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@linux.intel.com>
> Signed-off-by: Vidya Srinivas <vidya.srinivas@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;
This seems unnecessary. We could use the line below to get the src's.
> +
> + *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;
> + }
> +
> if (INTEL_GEN(dev_priv) >= 9) {
> ret = skl_check_plane_surface(crtc_state, state);
> if (ret)
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> b/drivers/gpu/drm/i915/intel_sprite.c
> index 8b7947d..c1dd85e 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1035,11 +1035,20 @@ intel_check_sprite_plane(struct intel_plane
> *plane,
> return vscale;
> }
>
> + if (fb->format->format == DRM_FORMAT_NV12) {
> + if (src->x2 >> 16 == 16)
> + src->x2 = 16 << 16;
> + if (src->y2 >> 16 == 16)
> + src->y2 = 16 << 16;
> + goto nv12_min_no_clip;
> + }
> +
> /* Make the source viewport size an exact multiple
> of the scaling factors. */
> drm_rect_adjust_size(src,
> drm_rect_width(dst) * hscale -
> drm_rect_width(src),
> drm_rect_height(dst) * vscale -
> drm_rect_height(src));
>
> +nv12_min_no_clip:
> drm_rect_rotate_inv(src, fb->width << 16, fb->height
> << 16,
> state->base.rotation);
>
> @@ -1105,6 +1114,12 @@ intel_check_sprite_plane(struct intel_plane
> *plane,
> src->x2 = (src_x + src_w) << 16;
> src->y1 = src_y << 16;
> src->y2 = (src_y + src_h) << 16;
> + if (fb->format->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;
> + }
> }
>
> dst->x1 = crtc_x;
--
Mika Kahola - Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for NV12
2018-04-13 12:13 [PATCH v2 0/6] Enable NV12 support Vidya Srinivas
@ 2018-04-13 12:14 ` Vidya Srinivas
2018-04-17 9:18 ` Mika Kahola
0 siblings, 1 reply; 20+ messages in thread
From: Vidya Srinivas @ 2018-04-13 12:14 UTC (permalink / raw)
To: intel-gfx
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@linux.intel.com>
Signed-off-by: Vidya Srinivas <vidya.srinivas@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;
+ }
+
if (INTEL_GEN(dev_priv) >= 9) {
ret = skl_check_plane_surface(crtc_state, state);
if (ret)
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 8b7947d..c1dd85e 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1035,11 +1035,20 @@ intel_check_sprite_plane(struct intel_plane *plane,
return vscale;
}
+ if (fb->format->format == DRM_FORMAT_NV12) {
+ if (src->x2 >> 16 == 16)
+ src->x2 = 16 << 16;
+ if (src->y2 >> 16 == 16)
+ src->y2 = 16 << 16;
+ goto nv12_min_no_clip;
+ }
+
/* Make the source viewport size an exact multiple of the scaling factors. */
drm_rect_adjust_size(src,
drm_rect_width(dst) * hscale - drm_rect_width(src),
drm_rect_height(dst) * vscale - drm_rect_height(src));
+nv12_min_no_clip:
drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16,
state->base.rotation);
@@ -1105,6 +1114,12 @@ intel_check_sprite_plane(struct intel_plane *plane,
src->x2 = (src_x + src_w) << 16;
src->y1 = src_y << 16;
src->y2 = (src_y + src_h) << 16;
+ if (fb->format->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;
+ }
}
dst->x1 = crtc_x;
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 20+ messages in thread
end of thread, other threads:[~2018-04-18 5:55 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <1523527177-14285-6-git-send-email-vidya.srinivas@intel.com>
[not found] ` <1523596958-26701-1-git-send-email-vidya.srinivas@intel.com>
[not found] ` <d729da8e-d5d5-9da9-221e-5da0f590ce0e@linux.intel.com>
[not found] ` <F653A0A18852B74D88578FA2EB7094EAB6850B06@BGSMSX108.gar.corp.intel.com>
2018-04-17 6:03 ` [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for NV12 Saarinen, Jani
2018-04-17 7:25 ` Maarten Lankhorst
2018-04-17 7:38 ` Srinivas, Vidya
2018-04-17 7:50 ` Maarten Lankhorst
2018-04-17 8:02 ` Srinivas, Vidya
2018-04-17 8:09 ` Srinivas, Vidya
2018-04-17 9:34 ` Maarten Lankhorst
2018-04-17 9:42 ` Srinivas, Vidya
2018-04-17 10:01 ` Maarten Lankhorst
2018-04-17 10:18 ` Srinivas, Vidya
2018-04-17 10:48 ` Srinivas, Vidya
2018-04-17 10:52 ` Saarinen, Jani
2018-04-18 3:55 ` Srinivas, Vidya
2018-04-18 5:36 ` Saarinen, Jani
2018-04-18 5:55 ` Srinivas, Vidya
2018-04-18 4:16 ` Srinivas, Vidya
[not found] ` <F653A0A18852B74D88578FA2EB7094EAB6850CA9@BGSMSX108.gar.corp.intel.com>
2018-04-17 6:06 ` Saarinen, Jani
2018-04-13 12:13 [PATCH v2 0/6] Enable NV12 support Vidya Srinivas
2018-04-13 12:14 ` [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for NV12 Vidya Srinivas
2018-04-17 9:18 ` Mika Kahola
2018-04-17 9:20 ` Srinivas, Vidya
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.