[v2] drm/i915: correct the pitch check for NV12 framebuffer

Commit Message

Hwang, Dongseong Dec. 12, 2018, 12:39 a.m. UTC

framebuffer for NV12 requires the pitch to the multiplier of 4, instead
of the width. This patch corrects it.

For instance, a 480p video, whose width and pitch are 854 and 896
respectively, is excluded for NV12 plane so far.

Signed-off-by: Dongseong Hwang <[e-mail]>
Cc: Chandra Konduru <[e-mail]>
Cc: Vidya Srinivas <[e-mail]>
Cc: Ville Syrjälä <[e-mail]>
Cc: Juha-Pekka Heikkila <[e-mail]>
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Ville Syrjälä Dec. 12, 2018, 8:24 p.m. UTC | #1

On Tue, Dec 11, 2018 at 04:39:05PM -0800, Dongseong Hwang wrote:
> framebuffer for NV12 requires the pitch to the multiplier of 4, instead
> of the width. This patch corrects it.
> 
> For instance, a 480p video, whose width and pitch are 854 and 896
> respectively, is excluded for NV12 plane so far.
> 
> Signed-off-by: Dongseong Hwang <[e-mail]>
> Cc: Chandra Konduru <[e-mail]>
> Cc: Vidya Srinivas <[e-mail]>
> Cc: Ville Syrjälä <[e-mail]>
> Cc: Juha-Pekka Heikkila <[e-mail]>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 13e5650..8a3de12 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14600,7 +14600,7 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
>  	if (fb->format->format == DRM_FORMAT_NV12 &&
>  	    (fb->width < SKL_MIN_YUV_420_SRC_W ||
>  	     fb->height < SKL_MIN_YUV_420_SRC_H ||
> -	     (fb->width % 4) != 0 || (fb->height % 4) != 0)) {
> +	     (fb->pitches[0] % 4) != 0 || (fb->height % 4) != 0)) {

The stride can never be misaligned like that. It'll be at least tile
aligned, or 64 byte aligned with linear buffers.

Anyways this entire piece of code doesn't make too much sense. The fb
size doesn't really matter for us, only the src viewport size matters.
That one we limit to a minimum of 2x2 pixels w/o scaling, and 16x16
pixems w/ scaling. So looks like this code can be just ripped out.

>  		DRM_DEBUG_KMS("src dimensions not correct for NV12\n");
>  		goto err;
>  	}
> -- 
> 2.7.4

Daniel Vetter Dec. 12, 2018, 8:33 p.m. UTC | #2

On Wed, Dec 12, 2018 at 9:24 PM Ville Syrjälä
<[e-mail]> wrote:
>
> On Tue, Dec 11, 2018 at 04:39:05PM -0800, Dongseong Hwang wrote:
> > framebuffer for NV12 requires the pitch to the multiplier of 4, instead
> > of the width. This patch corrects it.
> >
> > For instance, a 480p video, whose width and pitch are 854 and 896
> > respectively, is excluded for NV12 plane so far.
> >
> > Signed-off-by: Dongseong Hwang <[e-mail]>
> > Cc: Chandra Konduru <[e-mail]>
> > Cc: Vidya Srinivas <[e-mail]>
> > Cc: Ville Syrjälä <[e-mail]>
> > Cc: Juha-Pekka Heikkila <[e-mail]>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 13e5650..8a3de12 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -14600,7 +14600,7 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
> >       if (fb->format->format == DRM_FORMAT_NV12 &&
> >           (fb->width < SKL_MIN_YUV_420_SRC_W ||
> >            fb->height < SKL_MIN_YUV_420_SRC_H ||
> > -          (fb->width % 4) != 0 || (fb->height % 4) != 0)) {
> > +          (fb->pitches[0] % 4) != 0 || (fb->height % 4) != 0)) {
>
> The stride can never be misaligned like that. It'll be at least tile
> aligned, or 64 byte aligned with linear buffers.
>
> Anyways this entire piece of code doesn't make too much sense. The fb
> size doesn't really matter for us, only the src viewport size matters.
> That one we limit to a minimum of 2x2 pixels w/o scaling, and 16x16
> pixems w/ scaling. So looks like this code can be just ripped out.

Do we have igt testcases for these cornercases in igt? Obviously would
need to be intel specific subtests ...
-Daniel

>
> >               DRM_DEBUG_KMS("src dimensions not correct for NV12\n");
> >               goto err;
> >       }
> > --
> > 2.7.4
>
> --
> Ville Syrjälä
> Intel
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Ville Syrjälä Dec. 12, 2018, 9:15 p.m. UTC | #3

On Wed, Dec 12, 2018 at 09:33:49PM +0100, Daniel Vetter wrote:
> On Wed, Dec 12, 2018 at 9:24 PM Ville Syrjälä
> <[e-mail]> wrote:
> >
> > On Tue, Dec 11, 2018 at 04:39:05PM -0800, Dongseong Hwang wrote:
> > > framebuffer for NV12 requires the pitch to the multiplier of 4, instead
> > > of the width. This patch corrects it.
> > >
> > > For instance, a 480p video, whose width and pitch are 854 and 896
> > > respectively, is excluded for NV12 plane so far.
> > >
> > > Signed-off-by: Dongseong Hwang <[e-mail]>
> > > Cc: Chandra Konduru <[e-mail]>
> > > Cc: Vidya Srinivas <[e-mail]>
> > > Cc: Ville Syrjälä <[e-mail]>
> > > Cc: Juha-Pekka Heikkila <[e-mail]>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 13e5650..8a3de12 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -14600,7 +14600,7 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
> > >       if (fb->format->format == DRM_FORMAT_NV12 &&
> > >           (fb->width < SKL_MIN_YUV_420_SRC_W ||
> > >            fb->height < SKL_MIN_YUV_420_SRC_H ||
> > > -          (fb->width % 4) != 0 || (fb->height % 4) != 0)) {
> > > +          (fb->pitches[0] % 4) != 0 || (fb->height % 4) != 0)) {
> >
> > The stride can never be misaligned like that. It'll be at least tile
> > aligned, or 64 byte aligned with linear buffers.
> >
> > Anyways this entire piece of code doesn't make too much sense. The fb
> > size doesn't really matter for us, only the src viewport size matters.
> > That one we limit to a minimum of 2x2 pixels w/o scaling, and 16x16
> > pixems w/ scaling. So looks like this code can be just ripped out.
> 
> Do we have igt testcases for these cornercases in igt? Obviously would
> need to be intel specific subtests ...

Can't spot anything quite that specific. Someone would need to write
one I suppose. Also Imre has a test somewhere on the list for testing
the plane clipping underrun fails which tests small src viewport sizes,
and JP has been working on a rotation vs. clipping test that is also
somewhat related. Not sure if we could combine any of these somehow
to avoid having too many similar tests.

Singaiah Chintalapudi Dec. 12, 2018, 9:18 p.m. UTC | #4

Hello,

I am not sure why I am receiving these emails. Please remove me.

Thanks.

On 12/12/18, 3:15 PM, "dri-devel on behalf of Ville Syrjälä" <[e-mail] on behalf of [e-mail]> wrote:

    On Wed, Dec 12, 2018 at 09:33:49PM +0100, Daniel Vetter wrote:
    > On Wed, Dec 12, 2018 at 9:24 PM Ville Syrjälä
    > <[e-mail]> wrote:
    > >
    > > On Tue, Dec 11, 2018 at 04:39:05PM -0800, Dongseong Hwang wrote:
    > > > framebuffer for NV12 requires the pitch to the multiplier of 4, instead
    > > > of the width. This patch corrects it.
    > > >
    > > > For instance, a 480p video, whose width and pitch are 854 and 896
    > > > respectively, is excluded for NV12 plane so far.
    > > >
    > > > Signed-off-by: Dongseong Hwang <[e-mail]>
    > > > Cc: Chandra Konduru <[e-mail]>
    > > > Cc: Vidya Srinivas <[e-mail]>
    > > > Cc: Ville Syrjälä <[e-mail]>
    > > > Cc: Juha-Pekka Heikkila <[e-mail]>
    > > > ---
    > > >  drivers/gpu/drm/i915/intel_display.c | 2 +-
    > > >  1 file changed, 1 insertion(+), 1 deletion(-)
    > > >
    > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
    > > > index 13e5650..8a3de12 100644
    > > > --- a/drivers/gpu/drm/i915/intel_display.c
    > > > +++ b/drivers/gpu/drm/i915/intel_display.c
    > > > @@ -14600,7 +14600,7 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
    > > >       if (fb->format->format == DRM_FORMAT_NV12 &&
    > > >           (fb->width < SKL_MIN_YUV_420_SRC_W ||
    > > >            fb->height < SKL_MIN_YUV_420_SRC_H ||
    > > > -          (fb->width % 4) != 0 || (fb->height % 4) != 0)) {
    > > > +          (fb->pitches[0] % 4) != 0 || (fb->height % 4) != 0)) {
    > >
    > > The stride can never be misaligned like that. It'll be at least tile
    > > aligned, or 64 byte aligned with linear buffers.
    > >
    > > Anyways this entire piece of code doesn't make too much sense. The fb
    > > size doesn't really matter for us, only the src viewport size matters.
    > > That one we limit to a minimum of 2x2 pixels w/o scaling, and 16x16
    > > pixems w/ scaling. So looks like this code can be just ripped out.
    > 
    > Do we have igt testcases for these cornercases in igt? Obviously would
    > need to be intel specific subtests ...
    
    Can't spot anything quite that specific. Someone would need to write
    one I suppose. Also Imre has a test somewhere on the list for testing
    the plane clipping underrun fails which tests small src viewport sizes,
    and JP has been working on a rotation vs. clipping test that is also
    somewhat related. Not sure if we could combine any of these somehow
    to avoid having too many similar tests.
    
    -- 
    Ville Syrjälä
    Intel
    _______________________________________________
    dri-devel mailing list
    [e-mail]
    https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_dri-2Ddevel&d=DwIGaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=AqbVQX0MMoes-vp18k-9nce8K2Yg4CXmWwlCoNEEl4k&m=kffYCLL5-FFzZkD5uXpdCQgXMa3WtXfkAByKQlZ0ovM&s=gEd_OrHeidWz5ViCEWQMKjVQOHqxVUhrfz0RsNrOZOc&e=

Hwang, Dongseong Dec. 13, 2018, 12:51 a.m. UTC | #5

On Wed, Dec 12, 2018 at 1:15 PM Ville Syrjälä
<[e-mail]> wrote:
>
> On Wed, Dec 12, 2018 at 09:33:49PM +0100, Daniel Vetter wrote:
> > On Wed, Dec 12, 2018 at 9:24 PM Ville Syrjälä
> > <[e-mail]> wrote:
> > >
> > > On Tue, Dec 11, 2018 at 04:39:05PM -0800, Dongseong Hwang wrote:
> > > > framebuffer for NV12 requires the pitch to the multiplier of 4, instead
> > > > of the width. This patch corrects it.
> > > >
> > > > For instance, a 480p video, whose width and pitch are 854 and 896
> > > > respectively, is excluded for NV12 plane so far.
> > > >
> > > > Signed-off-by: Dongseong Hwang <[e-mail]>
> > > > Cc: Chandra Konduru <[e-mail]>
> > > > Cc: Vidya Srinivas <[e-mail]>
> > > > Cc: Ville Syrjälä <[e-mail]>
> > > > Cc: Juha-Pekka Heikkila <[e-mail]>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_display.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > index 13e5650..8a3de12 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -14600,7 +14600,7 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
> > > >       if (fb->format->format == DRM_FORMAT_NV12 &&
> > > >           (fb->width < SKL_MIN_YUV_420_SRC_W ||
> > > >            fb->height < SKL_MIN_YUV_420_SRC_H ||
> > > > -          (fb->width % 4) != 0 || (fb->height % 4) != 0)) {
> > > > +          (fb->pitches[0] % 4) != 0 || (fb->height % 4) != 0)) {
> > >
> > > The stride can never be misaligned like that. It'll be at least tile
> > > aligned, or 64 byte aligned with linear buffers.
> > >
> > > Anyways this entire piece of code doesn't make too much sense. The fb
> > > size doesn't really matter for us, only the src viewport size matters.
> > > That one we limit to a minimum of 2x2 pixels w/o scaling, and 16x16
> > > pixems w/ scaling. So looks like this code can be just ripped out.
> >
> > Do we have igt testcases for these cornercases in igt? Obviously would
> > need to be intel specific subtests ...
>
> Can't spot anything quite that specific. Someone would need to write
> one I suppose. Also Imre has a test somewhere on the list for testing
> the plane clipping underrun fails which tests small src viewport sizes,
> and JP has been working on a rotation vs. clipping test that is also
> somewhat related. Not sure if we could combine any of these somehow
> to avoid having too many similar tests.

Ap pointed out there is i915 workaround

1106 Display NV12, Rotation, Horizontal flip
Display corruption/color shift observed when using NV12 with 270
rotation or 90 rotation + horizontal flip.
WA: NV12 with 270 rotation or 90 rotation + horizontal flip requires
the programmed plane height to be a multiple of 4.
GEN9:ALL
GLK:ALL
GLV:ALL
CNL:*:A
CNL:*:B

I think this condition was introduced to deal with the workaround, and
I think the stride restriction is enough for the above workaround.
Ville, if I add the igt covering this change, is it good to land?

Best regards,
Dongseong


>
> --
> Ville Syrjälä
> Intel

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 13e5650..8a3de12 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14600,7 +14600,7 @@  static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
 	if (fb->format->format == DRM_FORMAT_NV12 &&
 	    (fb->width < SKL_MIN_YUV_420_SRC_W ||
 	     fb->height < SKL_MIN_YUV_420_SRC_H ||
-	     (fb->width % 4) != 0 || (fb->height % 4) != 0)) {
+	     (fb->pitches[0] % 4) != 0 || (fb->height % 4) != 0)) {
 		DRM_DEBUG_KMS("src dimensions not correct for NV12\n");
 		goto err;
 	}