| Message ID | 1516484713-5837-16-git-send-email-vidya.srinivas@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Op 20-01-18 om 22:45 schreef Vidya Srinivas: > From: Chandra Konduru <chandra.konduru@intel.com> > > This patch adds NV12 as supported format > to intel_framebuffer_init and performs various checks. > > v2: > -Fix an issue in checks added (Chandra Konduru) > > v3: rebased (me) > > v4: Review comments by Ville addressed > Added platform check for NV12 in intel_framebuffer_init > Removed offset checks for NV12 case > > v5: Addressed review comments by Clinton A Taylor > This NV12 support only correctly works on SKL. > Plane color space conversion is different on GLK and later platforms > causing the colors to display incorrectly. > Ville's plane color space property patch series > in review will fix this issue. > - Restricted the NV12 case in intel_framebuffer_init to > SKL and BXT only. > > v6: Rebased (me) > > v7: Addressed review comments by Ville > Restricting the NV12 to BXT for now. > > v8: Rebased (me) > Restricting the NV12 changes to BXT and KBL for now. > > v9: Rebased (me) > > Tested-by: Clinton Taylor <clinton.a.taylor@intel.com> > Reviewed-by: Clinton Taylor <clinton.a.taylor@intel.com> > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com> > Signed-off-by: Nabendu Maiti <nabendu.bikash.maiti@intel.com> > Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 88bc750..db42448 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -14005,6 +14005,14 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb, > goto err; > } > break; > + case DRM_FORMAT_NV12: > + if (!IS_BROXTON(dev_priv) && !IS_KABYLAKE(dev_priv)) { > + DRM_DEBUG_KMS("unsupported pixel format: %s\n", > + drm_get_format_name(mode_cmd->pixel_format, > + &format_name)); > + goto err; > + } > + break; > default: > DRM_DEBUG_KMS("unsupported pixel format: %s\n", > drm_get_format_name(mode_cmd->pixel_format, &format_name)); Hey, When implementing this for IGT I've noticed a small gap in documentation, which will definitely need clarification somewhere. intel_framebuffer_init is probably good enough since it's specific to how intel works internally.. The Yf tiling mode depends on BPP, is it correct to say that plane 0 has 8 bpp and plane 1 16 bpp for tiling calculations? What bpp is used? Are macroblock sizes always the same for Y and UV? ~Maarten
Op 20-01-18 om 22:45 schreef Vidya Srinivas: > From: Chandra Konduru <chandra.konduru@intel.com> > > This patch adds NV12 as supported format > to intel_framebuffer_init and performs various checks. > > v2: > -Fix an issue in checks added (Chandra Konduru) > > v3: rebased (me) > > v4: Review comments by Ville addressed > Added platform check for NV12 in intel_framebuffer_init > Removed offset checks for NV12 case > > v5: Addressed review comments by Clinton A Taylor > This NV12 support only correctly works on SKL. > Plane color space conversion is different on GLK and later platforms > causing the colors to display incorrectly. > Ville's plane color space property patch series > in review will fix this issue. > - Restricted the NV12 case in intel_framebuffer_init to > SKL and BXT only. > > v6: Rebased (me) > > v7: Addressed review comments by Ville > Restricting the NV12 to BXT for now. > > v8: Rebased (me) > Restricting the NV12 changes to BXT and KBL for now. > > v9: Rebased (me) Hey, Has NV12 been tested at all on skylake with rotation? y_min_scanlines = 16 for 90°/270° rotation with cpp=1 skl_needs_memory_bw_wa() doubles the scanlines required, which will trigger the following error quite easily when patching kms_rotation_crc to use NV12: [ 67.049190] [drm:skl_compute_wm_levels [i915]] Requested display configuration exceeds system watermark limitations [ 67.049212] [drm:skl_compute_wm_levels [i915]] [PLANE:28:plane 1A] blocks required = 161/572, lines required = 32/31 If we violate the spec for the workaround, by using 31 lines instead of 32 for 90/270 rotation, the tests work but this needs to be solved first before we can move forward. ~Maarten
> -----Original Message----- > From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com] > Sent: Monday, January 29, 2018 10:47 PM > To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel- > gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH 15/15] drm/i915: Add NV12 support to > intel_framebuffer_init > > Op 20-01-18 om 22:45 schreef Vidya Srinivas: > > From: Chandra Konduru <chandra.konduru@intel.com> > > > > This patch adds NV12 as supported format to intel_framebuffer_init and > > performs various checks. > > > > v2: > > -Fix an issue in checks added (Chandra Konduru) > > > > v3: rebased (me) > > > > v4: Review comments by Ville addressed Added platform check for NV12 > > in intel_framebuffer_init Removed offset checks for NV12 case > > > > v5: Addressed review comments by Clinton A Taylor This NV12 support > > only correctly works on SKL. > > Plane color space conversion is different on GLK and later platforms > > causing the colors to display incorrectly. > > Ville's plane color space property patch series in review will fix > > this issue. > > - Restricted the NV12 case in intel_framebuffer_init to SKL and BXT > > only. > > > > v6: Rebased (me) > > > > v7: Addressed review comments by Ville Restricting the NV12 to BXT for > > now. > > > > v8: Rebased (me) > > Restricting the NV12 changes to BXT and KBL for now. > > > > v9: Rebased (me) > Hey, > > Has NV12 been tested at all on skylake with rotation? > > y_min_scanlines = 16 for 90°/270° rotation with cpp=1 > skl_needs_memory_bw_wa() doubles the scanlines required, which will > trigger the following error quite easily when patching kms_rotation_crc to > use NV12: > > [ 67.049190] [drm:skl_compute_wm_levels [i915]] Requested display > configuration exceeds system watermark limitations [ 67.049212] > [drm:skl_compute_wm_levels [i915]] [PLANE:28:plane 1A] blocks required = > 161/572, lines required = 32/31 > > If we violate the spec for the workaround, by using 31 lines instead of 32 for > 90/270 rotation, the tests work but this needs to be solved first before we > can move forward. > We came across many such scenarios during NV12 testing on BXT. During downscaling, the "Max supported pixel clock with scaling exceeded" is seen if the user layer tries to downscale much. And rotation, as you mentioned we have seen such messages. We discussed the same during the meeting long back and mentioned that if we remove the limitations, it works. But as per discussion, if limitations are removed, it may work, but there may be certain conditions where it may not work. Hence we concluded that some such cases are expected not to work. However, I will discuss once again with Mahesh Kumar (CC) who works on WM implementation to re-confirm on the same. > ~Maarten
> -----Original Message----- > From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com] > Sent: Monday, January 29, 2018 5:12 PM > To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel- > gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH 15/15] drm/i915: Add NV12 support to > intel_framebuffer_init > > Op 20-01-18 om 22:45 schreef Vidya Srinivas: > > From: Chandra Konduru <chandra.konduru@intel.com> > > > > This patch adds NV12 as supported format to intel_framebuffer_init and > > performs various checks. > > > > v2: > > -Fix an issue in checks added (Chandra Konduru) > > > > v3: rebased (me) > > > > v4: Review comments by Ville addressed Added platform check for NV12 > > in intel_framebuffer_init Removed offset checks for NV12 case > > > > v5: Addressed review comments by Clinton A Taylor This NV12 support > > only correctly works on SKL. > > Plane color space conversion is different on GLK and later platforms > > causing the colors to display incorrectly. > > Ville's plane color space property patch series in review will fix > > this issue. > > - Restricted the NV12 case in intel_framebuffer_init to SKL and BXT > > only. > > > > v6: Rebased (me) > > > > v7: Addressed review comments by Ville Restricting the NV12 to BXT for > > now. > > > > v8: Rebased (me) > > Restricting the NV12 changes to BXT and KBL for now. > > > > v9: Rebased (me) > > > > Tested-by: Clinton Taylor <clinton.a.taylor@intel.com> > > Reviewed-by: Clinton Taylor <clinton.a.taylor@intel.com> > > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com> > > Signed-off-by: Nabendu Maiti <nabendu.bikash.maiti@intel.com> > > Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index 88bc750..db42448 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -14005,6 +14005,14 @@ static int intel_framebuffer_init(struct > intel_framebuffer *intel_fb, > > goto err; > > } > > break; > > + case DRM_FORMAT_NV12: > > + if (!IS_BROXTON(dev_priv) && !IS_KABYLAKE(dev_priv)) { > > + DRM_DEBUG_KMS("unsupported pixel format: > %s\n", > > + drm_get_format_name(mode_cmd->pixel_format, > > + &format_name)); > > + goto err; > > + } > > + break; > > default: > > DRM_DEBUG_KMS("unsupported pixel format: %s\n", > > drm_get_format_name(mode_cmd- > >pixel_format, &format_name)); > > Hey, > > When implementing this for IGT I've noticed a small gap in documentation, > which will definitely need clarification somewhere. intel_framebuffer_init is > probably good enough since it's specific to how intel works internally.. > The Yf tiling mode depends on BPP, is it correct to say that plane 0 has 8 bpp > and plane 1 16 bpp for tiling calculations? > > What bpp is used? > > Are macroblock sizes always the same for Y and UV? I think, under display buffer programming it is mentioned that: For NV12 Bpp is 1 for Y and 2 for UV. This is for all SKL+, it says. > > ~Maarten
Hi, On 1/30/2018 9:35 AM, Srinivas, Vidya wrote: > >> -----Original Message----- >> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com] >> Sent: Monday, January 29, 2018 10:47 PM >> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel- >> gfx@lists.freedesktop.org >> Subject: Re: [Intel-gfx] [PATCH 15/15] drm/i915: Add NV12 support to >> intel_framebuffer_init >> >> Op 20-01-18 om 22:45 schreef Vidya Srinivas: >>> From: Chandra Konduru <chandra.konduru@intel.com> >>> >>> This patch adds NV12 as supported format to intel_framebuffer_init and >>> performs various checks. >>> >>> v2: >>> -Fix an issue in checks added (Chandra Konduru) >>> >>> v3: rebased (me) >>> >>> v4: Review comments by Ville addressed Added platform check for NV12 >>> in intel_framebuffer_init Removed offset checks for NV12 case >>> >>> v5: Addressed review comments by Clinton A Taylor This NV12 support >>> only correctly works on SKL. >>> Plane color space conversion is different on GLK and later platforms >>> causing the colors to display incorrectly. >>> Ville's plane color space property patch series in review will fix >>> this issue. >>> - Restricted the NV12 case in intel_framebuffer_init to SKL and BXT >>> only. >>> >>> v6: Rebased (me) >>> >>> v7: Addressed review comments by Ville Restricting the NV12 to BXT for >>> now. >>> >>> v8: Rebased (me) >>> Restricting the NV12 changes to BXT and KBL for now. >>> >>> v9: Rebased (me) >> Hey, >> >> Has NV12 been tested at all on skylake with rotation? >> >> y_min_scanlines = 16 for 90°/270° rotation with cpp=1 >> skl_needs_memory_bw_wa() doubles the scanlines required, which will >> trigger the following error quite easily when patching kms_rotation_crc to >> use NV12: >> >> [ 67.049190] [drm:skl_compute_wm_levels [i915]] Requested display >> configuration exceeds system watermark limitations [ 67.049212] >> [drm:skl_compute_wm_levels [i915]] [PLANE:28:plane 1A] blocks required = >> 161/572, lines required = 32/31 >> >> If we violate the spec for the workaround, by using 31 lines instead of 32 for >> 90/270 rotation, the tests work but this needs to be solved first before we >> can move forward. We are hitting this issue due to memory_bw_wa (this is applicable for GEN-9 only) & we are applying WA irrespective of it's required or not. I had plan to apply this WA based on our discussion, but it never materialized :( https://www.spinics.net/lists/intel-gfx/msg124872.html > We came across many such scenarios during NV12 testing on BXT. During downscaling, > the "Max supported pixel clock with scaling exceeded" is seen if the user layer tries to downscale > much. And rotation, as you mentioned we have seen such messages. We discussed the same during the meeting > long back and mentioned that if we remove the limitations, it works. But as per discussion, if limitations are removed, it may work, > but there may be certain conditions where it may not work. Hence we concluded that some such cases are expected not to work. Either we can conclude that as a limitation (as expected behavior) OR we can enable WA only when it's required (as discussed, assuming all the planes are enabled with max supported scaling to avoid taking mutex in each flip). what you say? -Mahesh > However, I will discuss once again with Mahesh Kumar (CC) who works on WM implementation to re-confirm on the same. > >> ~Maarten
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 88bc750..db42448 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -14005,6 +14005,14 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb, goto err; } break; + case DRM_FORMAT_NV12: + if (!IS_BROXTON(dev_priv) && !IS_KABYLAKE(dev_priv)) { + DRM_DEBUG_KMS("unsupported pixel format: %s\n", + drm_get_format_name(mode_cmd->pixel_format, + &format_name)); + goto err; + } + break; default: DRM_DEBUG_KMS("unsupported pixel format: %s\n", drm_get_format_name(mode_cmd->pixel_format, &format_name));