Message ID | 1544575145-10346-1-git-send-email-dongseong.hwang@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v2] drm/i915: correct the pitch check for NV12 framebuffer | expand |
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 <dongseong.hwang@intel.com> > Cc: Chandra Konduru <chandra.konduru@intel.com> > Cc: Vidya Srinivas <vidya.srinivas@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> > --- > 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
On Wed, Dec 12, 2018 at 9:24 PM Ville Syrjälä <ville.syrjala@linux.intel.com> 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 <dongseong.hwang@intel.com> > > Cc: Chandra Konduru <chandra.konduru@intel.com> > > Cc: Vidya Srinivas <vidya.srinivas@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> > > --- > > 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 > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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ä > <ville.syrjala@linux.intel.com> 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 <dongseong.hwang@intel.com> > > > Cc: Chandra Konduru <chandra.konduru@intel.com> > > > Cc: Vidya Srinivas <vidya.srinivas@intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> > > > --- > > > 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.
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ä" <dri-devel-bounces@lists.freedesktop.org on behalf of ville.syrjala@linux.intel.com> 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ä > <ville.syrjala@linux.intel.com> 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 <dongseong.hwang@intel.com> > > > Cc: Chandra Konduru <chandra.konduru@intel.com> > > > Cc: Vidya Srinivas <vidya.srinivas@intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> > > > --- > > > 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 dri-devel@lists.freedesktop.org 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=
On Wed, Dec 12, 2018 at 1:15 PM Ville Syrjälä <ville.syrjala@linux.intel.com> 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ä > > <ville.syrjala@linux.intel.com> 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 <dongseong.hwang@intel.com> > > > > Cc: Chandra Konduru <chandra.konduru@intel.com> > > > > Cc: Vidya Srinivas <vidya.srinivas@intel.com> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> > > > > --- > > > > 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
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; }
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 <dongseong.hwang@intel.com> Cc: Chandra Konduru <chandra.konduru@intel.com> Cc: Vidya Srinivas <vidya.srinivas@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> --- drivers/gpu/drm/i915/intel_display.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)