[4/6] drm/i915: Add NV12 as supported format for primary plane

Commit Message

Vidya Srinivas Aug. 28, 2017, 10:52 a.m. UTC

From: Chandra Konduru <[e-mail]>

This patch adds NV12 to list of supported formats for
primary plane

v2: Rebased (Chandra Konduru)

v3: Rebased (me)

v4: Review comments by Ville addressed
	Removed the skl_primary_formats_with_nv12 and
	added NV12 case in existing skl_primary_formats

v5: Rebased (me)

v6: Missed the Tested-by/Reviewed-by in the previous series
	Adding the same to commit message in this version.

v7: Review comments by Ville addressed
	Restricting the NV12 for BXT and on PIPE A and B
	Rebased (me)

v8: Rebased (me)

Tested-by: Clinton Taylor <[e-mail]>
Reviewed-by: Clinton Taylor <[e-mail]>
Signed-off-by: Chandra Konduru <[e-mail]>
Signed-off-by: Nabendu Maiti <[e-mail]>
Signed-off-by: Vidya Srinivas <[e-mail]>
---
 drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

Comments

Matt Roper Sept. 6, 2017, 4:36 p.m. UTC | #1

On Mon, Aug 28, 2017 at 04:22:20PM +0530, Vidya Srinivas wrote:
> From: Chandra Konduru <[e-mail]>
> 
> This patch adds NV12 to list of supported formats for
> primary plane
> 
> v2: Rebased (Chandra Konduru)
> 
> v3: Rebased (me)
> 
> v4: Review comments by Ville addressed
> 	Removed the skl_primary_formats_with_nv12 and
> 	added NV12 case in existing skl_primary_formats
> 
> v5: Rebased (me)
> 
> v6: Missed the Tested-by/Reviewed-by in the previous series
> 	Adding the same to commit message in this version.
> 
> v7: Review comments by Ville addressed
> 	Restricting the NV12 for BXT and on PIPE A and B
> 	Rebased (me)
> 
> v8: Rebased (me)
> 
> Tested-by: Clinton Taylor <[e-mail]>
> Reviewed-by: Clinton Taylor <[e-mail]>
> Signed-off-by: Chandra Konduru <[e-mail]>
> Signed-off-by: Nabendu Maiti <[e-mail]>
> Signed-off-by: Vidya Srinivas <[e-mail]>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4e73d88..6cf8806 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -106,6 +106,22 @@
>  	DRM_FORMAT_MOD_INVALID
>  };
>  
> +static const uint32_t nv12_primary_formats[] = {
> +	DRM_FORMAT_C8,
> +	DRM_FORMAT_RGB565,
> +	DRM_FORMAT_XRGB8888,
> +	DRM_FORMAT_XBGR8888,
> +	DRM_FORMAT_ARGB8888,
> +	DRM_FORMAT_ABGR8888,
> +	DRM_FORMAT_XRGB2101010,
> +	DRM_FORMAT_XBGR2101010,
> +	DRM_FORMAT_YUYV,
> +	DRM_FORMAT_YVYU,
> +	DRM_FORMAT_UYVY,
> +	DRM_FORMAT_VYUY,
> +	DRM_FORMAT_NV12,
> +};
> +
>  /* Cursor formats */
>  static const uint32_t intel_cursor_formats[] = {
>  	DRM_FORMAT_ARGB8888,
> @@ -13280,8 +13296,14 @@ static bool intel_cursor_plane_format_mod_supported(struct drm_plane *plane,
>  		primary->update_plane = skylake_update_primary_plane;
>  		primary->disable_plane = skylake_disable_primary_plane;
>  	} else if (INTEL_GEN(dev_priv) >= 9) {
> -		intel_primary_formats = skl_primary_formats;
> -		num_formats = ARRAY_SIZE(skl_primary_formats);
> +		if (IS_BROXTON(dev_priv) &&

I believe this needs to be

   if (IS_BXT_REVID(dev_priv, BXT_REVID_D0, BXT_REVID_FOREVER) ...

There were unavoidable flickering/underrun issues on the earlier
steppings due to memory fetch issues for the second color plane.  Those
issues were only fixed on the E0 SoC stepping (which incorporates the D0
Display/GT).

Same change for your sprite plane changes in the next patch.


Matt

> +			((pipe == PIPE_A || pipe == PIPE_B))) {
> +			intel_primary_formats = nv12_primary_formats;
> +			num_formats = ARRAY_SIZE(nv12_primary_formats);
> +		} else {
> +			intel_primary_formats = skl_primary_formats;
> +			num_formats = ARRAY_SIZE(skl_primary_formats);
> +		}
>  		if (pipe < PIPE_C)
>  			modifiers = skl_format_modifiers_ccs;
>  		else
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Rodrigo Vivi Sept. 6, 2017, 7:12 p.m. UTC | #2

On Wed, Sep 06, 2017 at 09:36:27AM -0700, Matt Roper wrote:
> On Mon, Aug 28, 2017 at 04:22:20PM +0530, Vidya Srinivas wrote:
> > From: Chandra Konduru <[e-mail]>
> > 
> > This patch adds NV12 to list of supported formats for
> > primary plane
> > 
> > v2: Rebased (Chandra Konduru)
> > 
> > v3: Rebased (me)
> > 
> > v4: Review comments by Ville addressed
> > 	Removed the skl_primary_formats_with_nv12 and
> > 	added NV12 case in existing skl_primary_formats
> > 
> > v5: Rebased (me)
> > 
> > v6: Missed the Tested-by/Reviewed-by in the previous series
> > 	Adding the same to commit message in this version.
> > 
> > v7: Review comments by Ville addressed
> > 	Restricting the NV12 for BXT and on PIPE A and B
> > 	Rebased (me)
> > 
> > v8: Rebased (me)
> > 
> > Tested-by: Clinton Taylor <[e-mail]>
> > Reviewed-by: Clinton Taylor <[e-mail]>
> > Signed-off-by: Chandra Konduru <[e-mail]>
> > Signed-off-by: Nabendu Maiti <[e-mail]>
> > Signed-off-by: Vidya Srinivas <[e-mail]>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++++++++++++++--
> >  1 file changed, 24 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 4e73d88..6cf8806 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -106,6 +106,22 @@
> >  	DRM_FORMAT_MOD_INVALID
> >  };
> >  
> > +static const uint32_t nv12_primary_formats[] = {
> > +	DRM_FORMAT_C8,
> > +	DRM_FORMAT_RGB565,
> > +	DRM_FORMAT_XRGB8888,
> > +	DRM_FORMAT_XBGR8888,
> > +	DRM_FORMAT_ARGB8888,
> > +	DRM_FORMAT_ABGR8888,
> > +	DRM_FORMAT_XRGB2101010,
> > +	DRM_FORMAT_XBGR2101010,
> > +	DRM_FORMAT_YUYV,
> > +	DRM_FORMAT_YVYU,
> > +	DRM_FORMAT_UYVY,
> > +	DRM_FORMAT_VYUY,
> > +	DRM_FORMAT_NV12,
> > +};
> > +
> >  /* Cursor formats */
> >  static const uint32_t intel_cursor_formats[] = {
> >  	DRM_FORMAT_ARGB8888,
> > @@ -13280,8 +13296,14 @@ static bool intel_cursor_plane_format_mod_supported(struct drm_plane *plane,
> >  		primary->update_plane = skylake_update_primary_plane;
> >  		primary->disable_plane = skylake_disable_primary_plane;
> >  	} else if (INTEL_GEN(dev_priv) >= 9) {
> > -		intel_primary_formats = skl_primary_formats;
> > -		num_formats = ARRAY_SIZE(skl_primary_formats);
> > +		if (IS_BROXTON(dev_priv) &&
> 
> I believe this needs to be
> 
>    if (IS_BXT_REVID(dev_priv, BXT_REVID_D0, BXT_REVID_FOREVER) ...

We usually use this stepping information for Workarounds. So usually
blocks around this are the non-expected default behaviour.
So I'd handle that from A0 to C0 and else the default behaviour or at least
![A0,C0]...

> 
> There were unavoidable flickering/underrun issues on the earlier
> steppings due to memory fetch issues for the second color plane.  Those
> issues were only fixed on the E0 SoC stepping (which incorporates the D0
> Display/GT).

Also we usuallly use this steppings checking with a documented W/a.
Is there one? Anything that would justify this?

But also, is there any team there still using anything older than D0? yet?

If we don't know anyone probably pure IS_BROXTON is the best option.

> 
> Same change for your sprite plane changes in the next patch.
> 
> 
> Matt
> 
> > +			((pipe == PIPE_A || pipe == PIPE_B))) {
> > +			intel_primary_formats = nv12_primary_formats;
> > +			num_formats = ARRAY_SIZE(nv12_primary_formats);
> > +		} else {
> > +			intel_primary_formats = skl_primary_formats;
> > +			num_formats = ARRAY_SIZE(skl_primary_formats);
> > +		}
> >  		if (pipe < PIPE_C)
> >  			modifiers = skl_format_modifiers_ccs;
> >  		else
> > -- 
> > 1.9.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > [e-mail]
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> [phone]
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Matt Roper Sept. 6, 2017, 8:17 p.m. UTC | #3

On Wed, Sep 06, 2017 at 12:12:10PM -0700, Rodrigo Vivi wrote:
> On Wed, Sep 06, 2017 at 09:36:27AM -0700, Matt Roper wrote:
> > On Mon, Aug 28, 2017 at 04:22:20PM +0530, Vidya Srinivas wrote:
> > > From: Chandra Konduru <[e-mail]>
> > > 
> > > This patch adds NV12 to list of supported formats for
> > > primary plane
> > > 
> > > v2: Rebased (Chandra Konduru)
> > > 
> > > v3: Rebased (me)
> > > 
> > > v4: Review comments by Ville addressed
> > > 	Removed the skl_primary_formats_with_nv12 and
> > > 	added NV12 case in existing skl_primary_formats
> > > 
> > > v5: Rebased (me)
> > > 
> > > v6: Missed the Tested-by/Reviewed-by in the previous series
> > > 	Adding the same to commit message in this version.
> > > 
> > > v7: Review comments by Ville addressed
> > > 	Restricting the NV12 for BXT and on PIPE A and B
> > > 	Rebased (me)
> > > 
> > > v8: Rebased (me)
> > > 
> > > Tested-by: Clinton Taylor <[e-mail]>
> > > Reviewed-by: Clinton Taylor <[e-mail]>
> > > Signed-off-by: Chandra Konduru <[e-mail]>
> > > Signed-off-by: Nabendu Maiti <[e-mail]>
> > > Signed-off-by: Vidya Srinivas <[e-mail]>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++++++++++++++--
> > >  1 file changed, 24 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 4e73d88..6cf8806 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -106,6 +106,22 @@
> > >  	DRM_FORMAT_MOD_INVALID
> > >  };
> > >  
> > > +static const uint32_t nv12_primary_formats[] = {
> > > +	DRM_FORMAT_C8,
> > > +	DRM_FORMAT_RGB565,
> > > +	DRM_FORMAT_XRGB8888,
> > > +	DRM_FORMAT_XBGR8888,
> > > +	DRM_FORMAT_ARGB8888,
> > > +	DRM_FORMAT_ABGR8888,
> > > +	DRM_FORMAT_XRGB2101010,
> > > +	DRM_FORMAT_XBGR2101010,
> > > +	DRM_FORMAT_YUYV,
> > > +	DRM_FORMAT_YVYU,
> > > +	DRM_FORMAT_UYVY,
> > > +	DRM_FORMAT_VYUY,
> > > +	DRM_FORMAT_NV12,
> > > +};
> > > +
> > >  /* Cursor formats */
> > >  static const uint32_t intel_cursor_formats[] = {
> > >  	DRM_FORMAT_ARGB8888,
> > > @@ -13280,8 +13296,14 @@ static bool intel_cursor_plane_format_mod_supported(struct drm_plane *plane,
> > >  		primary->update_plane = skylake_update_primary_plane;
> > >  		primary->disable_plane = skylake_disable_primary_plane;
> > >  	} else if (INTEL_GEN(dev_priv) >= 9) {
> > > -		intel_primary_formats = skl_primary_formats;
> > > -		num_formats = ARRAY_SIZE(skl_primary_formats);
> > > +		if (IS_BROXTON(dev_priv) &&
> > 
> > I believe this needs to be
> > 
> >    if (IS_BXT_REVID(dev_priv, BXT_REVID_D0, BXT_REVID_FOREVER) ...
> 
> We usually use this stepping information for Workarounds. So usually
> blocks around this are the non-expected default behaviour.
> So I'd handle that from A0 to C0 and else the default behaviour or at least
> ![A0,C0]...
> 
> > 
> > There were unavoidable flickering/underrun issues on the earlier
> > steppings due to memory fetch issues for the second color plane.  Those
> > issues were only fixed on the E0 SoC stepping (which incorporates the D0
> > Display/GT).
> 
> Also we usuallly use this steppings checking with a documented W/a.
> Is there one? Anything that would justify this?

Unfortunately the bspec WA database still hasn't been updated to
indicate that *any* SKU of can properly support NV12.  So the workaround
database still just gives a general "don't use NV12 at all" statement
(entry 0870 in the display WA database which is listed for "BXT:ALL").
I tried unravel what the internal communication channels are for this
kind of update a few months ago, but didn't make much headway.

> 
> But also, is there any team there still using anything older than D0? yet?

Yes, definitely.  Pre-E0 SoC's (and thus pre-D0 graphics) is what a lot
of our embedded customers have already gone to production with.


Matt


> 
> If we don't know anyone probably pure IS_BROXTON is the best option.
> 
> > 
> > Same change for your sprite plane changes in the next patch.
> > 
> > 
> > Matt
> > 
> > > +			((pipe == PIPE_A || pipe == PIPE_B))) {
> > > +			intel_primary_formats = nv12_primary_formats;
> > > +			num_formats = ARRAY_SIZE(nv12_primary_formats);
> > > +		} else {
> > > +			intel_primary_formats = skl_primary_formats;
> > > +			num_formats = ARRAY_SIZE(skl_primary_formats);
> > > +		}
> > >  		if (pipe < PIPE_C)
> > >  			modifiers = skl_format_modifiers_ccs;
> > >  		else
> > > -- 
> > > 1.9.1
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > [e-mail]
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Matt Roper
> > Graphics Software Engineer
> > IoTG Platform Enabling & Development
> > Intel Corporation
> > [phone]
> > _______________________________________________
> > Intel-gfx mailing list
> > [e-mail]
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Kristian Høgsberg Sept. 6, 2017, 9:49 p.m. UTC | #4

On Wed, Sep 6, 2017 at 1:17 PM, Matt Roper <[e-mail]> wrote:
> On Wed, Sep 06, 2017 at 12:12:10PM -0700, Rodrigo Vivi wrote:
>> On Wed, Sep 06, 2017 at 09:36:27AM -0700, Matt Roper wrote:
>> > On Mon, Aug 28, 2017 at 04:22:20PM +0530, Vidya Srinivas wrote:
>> > > From: Chandra Konduru <[e-mail]>
>> > >
>> > > This patch adds NV12 to list of supported formats for
>> > > primary plane
>> > >
>> > > v2: Rebased (Chandra Konduru)
>> > >
>> > > v3: Rebased (me)
>> > >
>> > > v4: Review comments by Ville addressed
>> > >   Removed the skl_primary_formats_with_nv12 and
>> > >   added NV12 case in existing skl_primary_formats
>> > >
>> > > v5: Rebased (me)
>> > >
>> > > v6: Missed the Tested-by/Reviewed-by in the previous series
>> > >   Adding the same to commit message in this version.
>> > >
>> > > v7: Review comments by Ville addressed
>> > >   Restricting the NV12 for BXT and on PIPE A and B
>> > >   Rebased (me)
>> > >
>> > > v8: Rebased (me)
>> > >
>> > > Tested-by: Clinton Taylor <[e-mail]>
>> > > Reviewed-by: Clinton Taylor <[e-mail]>
>> > > Signed-off-by: Chandra Konduru <[e-mail]>
>> > > Signed-off-by: Nabendu Maiti <[e-mail]>
>> > > Signed-off-by: Vidya Srinivas <[e-mail]>
>> > > ---
>> > >  drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++++++++++++++--
>> > >  1 file changed, 24 insertions(+), 2 deletions(-)
>> > >
>> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> > > index 4e73d88..6cf8806 100644
>> > > --- a/drivers/gpu/drm/i915/intel_display.c
>> > > +++ b/drivers/gpu/drm/i915/intel_display.c
>> > > @@ -106,6 +106,22 @@
>> > >   DRM_FORMAT_MOD_INVALID
>> > >  };
>> > >
>> > > +static const uint32_t nv12_primary_formats[] = {
>> > > + DRM_FORMAT_C8,
>> > > + DRM_FORMAT_RGB565,
>> > > + DRM_FORMAT_XRGB8888,
>> > > + DRM_FORMAT_XBGR8888,
>> > > + DRM_FORMAT_ARGB8888,
>> > > + DRM_FORMAT_ABGR8888,
>> > > + DRM_FORMAT_XRGB2101010,
>> > > + DRM_FORMAT_XBGR2101010,
>> > > + DRM_FORMAT_YUYV,
>> > > + DRM_FORMAT_YVYU,
>> > > + DRM_FORMAT_UYVY,
>> > > + DRM_FORMAT_VYUY,
>> > > + DRM_FORMAT_NV12,
>> > > +};
>> > > +
>> > >  /* Cursor formats */
>> > >  static const uint32_t intel_cursor_formats[] = {
>> > >   DRM_FORMAT_ARGB8888,
>> > > @@ -13280,8 +13296,14 @@ static bool intel_cursor_plane_format_mod_supported(struct drm_plane *plane,
>> > >           primary->update_plane = skylake_update_primary_plane;
>> > >           primary->disable_plane = skylake_disable_primary_plane;
>> > >   } else if (INTEL_GEN(dev_priv) >= 9) {
>> > > -         intel_primary_formats = skl_primary_formats;
>> > > -         num_formats = ARRAY_SIZE(skl_primary_formats);
>> > > +         if (IS_BROXTON(dev_priv) &&
>> >
>> > I believe this needs to be
>> >
>> >    if (IS_BXT_REVID(dev_priv, BXT_REVID_D0, BXT_REVID_FOREVER) ...
>>
>> We usually use this stepping information for Workarounds. So usually
>> blocks around this are the non-expected default behaviour.
>> So I'd handle that from A0 to C0 and else the default behaviour or at least
>> ![A0,C0]...
>>
>> >
>> > There were unavoidable flickering/underrun issues on the earlier
>> > steppings due to memory fetch issues for the second color plane.  Those
>> > issues were only fixed on the E0 SoC stepping (which incorporates the D0
>> > Display/GT).
>>
>> Also we usuallly use this steppings checking with a documented W/a.
>> Is there one? Anything that would justify this?
>
> Unfortunately the bspec WA database still hasn't been updated to
> indicate that *any* SKU of can properly support NV12.  So the workaround
> database still just gives a general "don't use NV12 at all" statement
> (entry 0870 in the display WA database which is listed for "BXT:ALL").
> I tried unravel what the internal communication channels are for this
> kind of update a few months ago, but didn't make much headway.

What about KBL support?

>>
>> But also, is there any team there still using anything older than D0? yet?
>
> Yes, definitely.  Pre-E0 SoC's (and thus pre-D0 graphics) is what a lot
> of our embedded customers have already gone to production with.
>
>
> Matt
>
>
>>
>> If we don't know anyone probably pure IS_BROXTON is the best option.
>>
>> >
>> > Same change for your sprite plane changes in the next patch.
>> >
>> >
>> > Matt
>> >
>> > > +                 ((pipe == PIPE_A || pipe == PIPE_B))) {
>> > > +                 intel_primary_formats = nv12_primary_formats;
>> > > +                 num_formats = ARRAY_SIZE(nv12_primary_formats);
>> > > +         } else {
>> > > +                 intel_primary_formats = skl_primary_formats;
>> > > +                 num_formats = ARRAY_SIZE(skl_primary_formats);
>> > > +         }
>> > >           if (pipe < PIPE_C)
>> > >                   modifiers = skl_format_modifiers_ccs;
>> > >           else
>> > > --
>> > > 1.9.1
>> > >
>> > > _______________________________________________
>> > > Intel-gfx mailing list
>> > > [e-mail]
>> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> >
>> > --
>> > Matt Roper
>> > Graphics Software Engineer
>> > IoTG Platform Enabling & Development
>> > Intel Corporation
>> > [phone]
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > [e-mail]
>> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> [phone]
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Matt Roper Sept. 6, 2017, 10:02 p.m. UTC | #5

On Wed, Sep 06, 2017 at 02:49:07PM -0700, Kristian Høgsberg wrote:
> On Wed, Sep 6, 2017 at 1:17 PM, Matt Roper <[e-mail]> wrote:
> > On Wed, Sep 06, 2017 at 12:12:10PM -0700, Rodrigo Vivi wrote:
> >> On Wed, Sep 06, 2017 at 09:36:27AM -0700, Matt Roper wrote:
> >> > On Mon, Aug 28, 2017 at 04:22:20PM +0530, Vidya Srinivas wrote:
> >> > > From: Chandra Konduru <[e-mail]>
> >> > >
> >> > > This patch adds NV12 to list of supported formats for
> >> > > primary plane
> >> > >
> >> > > v2: Rebased (Chandra Konduru)
> >> > >
> >> > > v3: Rebased (me)
> >> > >
> >> > > v4: Review comments by Ville addressed
> >> > >   Removed the skl_primary_formats_with_nv12 and
> >> > >   added NV12 case in existing skl_primary_formats
> >> > >
> >> > > v5: Rebased (me)
> >> > >
> >> > > v6: Missed the Tested-by/Reviewed-by in the previous series
> >> > >   Adding the same to commit message in this version.
> >> > >
> >> > > v7: Review comments by Ville addressed
> >> > >   Restricting the NV12 for BXT and on PIPE A and B
> >> > >   Rebased (me)
> >> > >
> >> > > v8: Rebased (me)
> >> > >
> >> > > Tested-by: Clinton Taylor <[e-mail]>
> >> > > Reviewed-by: Clinton Taylor <[e-mail]>
> >> > > Signed-off-by: Chandra Konduru <[e-mail]>
> >> > > Signed-off-by: Nabendu Maiti <[e-mail]>
> >> > > Signed-off-by: Vidya Srinivas <[e-mail]>
> >> > > ---
> >> > >  drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++++++++++++++--
> >> > >  1 file changed, 24 insertions(+), 2 deletions(-)
> >> > >
> >> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> > > index 4e73d88..6cf8806 100644
> >> > > --- a/drivers/gpu/drm/i915/intel_display.c
> >> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> >> > > @@ -106,6 +106,22 @@
> >> > >   DRM_FORMAT_MOD_INVALID
> >> > >  };
> >> > >
> >> > > +static const uint32_t nv12_primary_formats[] = {
> >> > > + DRM_FORMAT_C8,
> >> > > + DRM_FORMAT_RGB565,
> >> > > + DRM_FORMAT_XRGB8888,
> >> > > + DRM_FORMAT_XBGR8888,
> >> > > + DRM_FORMAT_ARGB8888,
> >> > > + DRM_FORMAT_ABGR8888,
> >> > > + DRM_FORMAT_XRGB2101010,
> >> > > + DRM_FORMAT_XBGR2101010,
> >> > > + DRM_FORMAT_YUYV,
> >> > > + DRM_FORMAT_YVYU,
> >> > > + DRM_FORMAT_UYVY,
> >> > > + DRM_FORMAT_VYUY,
> >> > > + DRM_FORMAT_NV12,
> >> > > +};
> >> > > +
> >> > >  /* Cursor formats */
> >> > >  static const uint32_t intel_cursor_formats[] = {
> >> > >   DRM_FORMAT_ARGB8888,
> >> > > @@ -13280,8 +13296,14 @@ static bool intel_cursor_plane_format_mod_supported(struct drm_plane *plane,
> >> > >           primary->update_plane = skylake_update_primary_plane;
> >> > >           primary->disable_plane = skylake_disable_primary_plane;
> >> > >   } else if (INTEL_GEN(dev_priv) >= 9) {
> >> > > -         intel_primary_formats = skl_primary_formats;
> >> > > -         num_formats = ARRAY_SIZE(skl_primary_formats);
> >> > > +         if (IS_BROXTON(dev_priv) &&
> >> >
> >> > I believe this needs to be
> >> >
> >> >    if (IS_BXT_REVID(dev_priv, BXT_REVID_D0, BXT_REVID_FOREVER) ...
> >>
> >> We usually use this stepping information for Workarounds. So usually
> >> blocks around this are the non-expected default behaviour.
> >> So I'd handle that from A0 to C0 and else the default behaviour or at least
> >> ![A0,C0]...
> >>
> >> >
> >> > There were unavoidable flickering/underrun issues on the earlier
> >> > steppings due to memory fetch issues for the second color plane.  Those
> >> > issues were only fixed on the E0 SoC stepping (which incorporates the D0
> >> > Display/GT).
> >>
> >> Also we usuallly use this steppings checking with a documented W/a.
> >> Is there one? Anything that would justify this?
> >
> > Unfortunately the bspec WA database still hasn't been updated to
> > indicate that *any* SKU of can properly support NV12.  So the workaround
> > database still just gives a general "don't use NV12 at all" statement
> > (entry 0870 in the display WA database which is listed for "BXT:ALL").

Woops, this is actually 0826, not 0870 (0870 is a different NV12 entry
specifically for y-tile).


> > I tried unravel what the internal communication channels are for this
> > kind of update a few months ago, but didn't make much headway.
> 
> What about KBL support?
> 

The only NV12 workaround I see for KBL is that KBL A-step and B-step
shouldn't do NV12 + ytile.


Matt


> >>
> >> But also, is there any team there still using anything older than D0? yet?
> >
> > Yes, definitely.  Pre-E0 SoC's (and thus pre-D0 graphics) is what a lot
> > of our embedded customers have already gone to production with.
> >
> >
> > Matt
> >
> >
> >>
> >> If we don't know anyone probably pure IS_BROXTON is the best option.
> >>
> >> >
> >> > Same change for your sprite plane changes in the next patch.
> >> >
> >> >
> >> > Matt
> >> >
> >> > > +                 ((pipe == PIPE_A || pipe == PIPE_B))) {
> >> > > +                 intel_primary_formats = nv12_primary_formats;
> >> > > +                 num_formats = ARRAY_SIZE(nv12_primary_formats);
> >> > > +         } else {
> >> > > +                 intel_primary_formats = skl_primary_formats;
> >> > > +                 num_formats = ARRAY_SIZE(skl_primary_formats);
> >> > > +         }
> >> > >           if (pipe < PIPE_C)
> >> > >                   modifiers = skl_format_modifiers_ccs;
> >> > >           else
> >> > > --
> >> > > 1.9.1
> >> > >
> >> > > _______________________________________________
> >> > > Intel-gfx mailing list
> >> > > [e-mail]
> >> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >> >
> >> > --
> >> > Matt Roper
> >> > Graphics Software Engineer
> >> > IoTG Platform Enabling & Development
> >> > Intel Corporation
> >> > [phone]
> >> > _______________________________________________
> >> > Intel-gfx mailing list
> >> > [e-mail]
> >> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Matt Roper
> > Graphics Software Engineer
> > IoTG Platform Enabling & Development
> > Intel Corporation
> > [phone]
> > _______________________________________________
> > Intel-gfx mailing list
> > [e-mail]
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Kristian Høgsberg Sept. 6, 2017, 10:53 p.m. UTC | #6

On Wed, Sep 6, 2017 at 3:02 PM, Matt Roper <[e-mail]> wrote:
> On Wed, Sep 06, 2017 at 02:49:07PM -0700, Kristian Høgsberg wrote:
>> On Wed, Sep 6, 2017 at 1:17 PM, Matt Roper <[e-mail]> wrote:
>> > On Wed, Sep 06, 2017 at 12:12:10PM -0700, Rodrigo Vivi wrote:
>> >> On Wed, Sep 06, 2017 at 09:36:27AM -0700, Matt Roper wrote:
>> >> > On Mon, Aug 28, 2017 at 04:22:20PM +0530, Vidya Srinivas wrote:
>> >> > > From: Chandra Konduru <[e-mail]>
>> >> > >
>> >> > > This patch adds NV12 to list of supported formats for
>> >> > > primary plane
>> >> > >
>> >> > > v2: Rebased (Chandra Konduru)
>> >> > >
>> >> > > v3: Rebased (me)
>> >> > >
>> >> > > v4: Review comments by Ville addressed
>> >> > >   Removed the skl_primary_formats_with_nv12 and
>> >> > >   added NV12 case in existing skl_primary_formats
>> >> > >
>> >> > > v5: Rebased (me)
>> >> > >
>> >> > > v6: Missed the Tested-by/Reviewed-by in the previous series
>> >> > >   Adding the same to commit message in this version.
>> >> > >
>> >> > > v7: Review comments by Ville addressed
>> >> > >   Restricting the NV12 for BXT and on PIPE A and B
>> >> > >   Rebased (me)
>> >> > >
>> >> > > v8: Rebased (me)
>> >> > >
>> >> > > Tested-by: Clinton Taylor <[e-mail]>
>> >> > > Reviewed-by: Clinton Taylor <[e-mail]>
>> >> > > Signed-off-by: Chandra Konduru <[e-mail]>
>> >> > > Signed-off-by: Nabendu Maiti <[e-mail]>
>> >> > > Signed-off-by: Vidya Srinivas <[e-mail]>
>> >> > > ---
>> >> > >  drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++++++++++++++--
>> >> > >  1 file changed, 24 insertions(+), 2 deletions(-)
>> >> > >
>> >> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> >> > > index 4e73d88..6cf8806 100644
>> >> > > --- a/drivers/gpu/drm/i915/intel_display.c
>> >> > > +++ b/drivers/gpu/drm/i915/intel_display.c
>> >> > > @@ -106,6 +106,22 @@
>> >> > >   DRM_FORMAT_MOD_INVALID
>> >> > >  };
>> >> > >
>> >> > > +static const uint32_t nv12_primary_formats[] = {
>> >> > > + DRM_FORMAT_C8,
>> >> > > + DRM_FORMAT_RGB565,
>> >> > > + DRM_FORMAT_XRGB8888,
>> >> > > + DRM_FORMAT_XBGR8888,
>> >> > > + DRM_FORMAT_ARGB8888,
>> >> > > + DRM_FORMAT_ABGR8888,
>> >> > > + DRM_FORMAT_XRGB2101010,
>> >> > > + DRM_FORMAT_XBGR2101010,
>> >> > > + DRM_FORMAT_YUYV,
>> >> > > + DRM_FORMAT_YVYU,
>> >> > > + DRM_FORMAT_UYVY,
>> >> > > + DRM_FORMAT_VYUY,
>> >> > > + DRM_FORMAT_NV12,
>> >> > > +};
>> >> > > +
>> >> > >  /* Cursor formats */
>> >> > >  static const uint32_t intel_cursor_formats[] = {
>> >> > >   DRM_FORMAT_ARGB8888,
>> >> > > @@ -13280,8 +13296,14 @@ static bool intel_cursor_plane_format_mod_supported(struct drm_plane *plane,
>> >> > >           primary->update_plane = skylake_update_primary_plane;
>> >> > >           primary->disable_plane = skylake_disable_primary_plane;
>> >> > >   } else if (INTEL_GEN(dev_priv) >= 9) {
>> >> > > -         intel_primary_formats = skl_primary_formats;
>> >> > > -         num_formats = ARRAY_SIZE(skl_primary_formats);
>> >> > > +         if (IS_BROXTON(dev_priv) &&
>> >> >
>> >> > I believe this needs to be
>> >> >
>> >> >    if (IS_BXT_REVID(dev_priv, BXT_REVID_D0, BXT_REVID_FOREVER) ...
>> >>
>> >> We usually use this stepping information for Workarounds. So usually
>> >> blocks around this are the non-expected default behaviour.
>> >> So I'd handle that from A0 to C0 and else the default behaviour or at least
>> >> ![A0,C0]...
>> >>
>> >> >
>> >> > There were unavoidable flickering/underrun issues on the earlier
>> >> > steppings due to memory fetch issues for the second color plane.  Those
>> >> > issues were only fixed on the E0 SoC stepping (which incorporates the D0
>> >> > Display/GT).
>> >>
>> >> Also we usuallly use this steppings checking with a documented W/a.
>> >> Is there one? Anything that would justify this?
>> >
>> > Unfortunately the bspec WA database still hasn't been updated to
>> > indicate that *any* SKU of can properly support NV12.  So the workaround
>> > database still just gives a general "don't use NV12 at all" statement
>> > (entry 0870 in the display WA database which is listed for "BXT:ALL").
>
> Woops, this is actually 0826, not 0870 (0870 is a different NV12 entry
> specifically for y-tile).
>
>
>> > I tried unravel what the internal communication channels are for this
>> > kind of update a few months ago, but didn't make much headway.
>>
>> What about KBL support?
>>
>
> The only NV12 workaround I see for KBL is that KBL A-step and B-step
> shouldn't do NV12 + ytile.

But does this series actually enable KBL NV12 overlays? I only see
IS_BROXTON() in the conditional - that doesn't include KBL, does it?

> Matt
>
>
>> >>
>> >> But also, is there any team there still using anything older than D0? yet?
>> >
>> > Yes, definitely.  Pre-E0 SoC's (and thus pre-D0 graphics) is what a lot
>> > of our embedded customers have already gone to production with.
>> >
>> >
>> > Matt
>> >
>> >
>> >>
>> >> If we don't know anyone probably pure IS_BROXTON is the best option.
>> >>
>> >> >
>> >> > Same change for your sprite plane changes in the next patch.
>> >> >
>> >> >
>> >> > Matt
>> >> >
>> >> > > +                 ((pipe == PIPE_A || pipe == PIPE_B))) {
>> >> > > +                 intel_primary_formats = nv12_primary_formats;
>> >> > > +                 num_formats = ARRAY_SIZE(nv12_primary_formats);
>> >> > > +         } else {
>> >> > > +                 intel_primary_formats = skl_primary_formats;
>> >> > > +                 num_formats = ARRAY_SIZE(skl_primary_formats);
>> >> > > +         }
>> >> > >           if (pipe < PIPE_C)
>> >> > >                   modifiers = skl_format_modifiers_ccs;
>> >> > >           else
>> >> > > --
>> >> > > 1.9.1
>> >> > >
>> >> > > _______________________________________________
>> >> > > Intel-gfx mailing list
>> >> > > [e-mail]
>> >> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> >> >
>> >> > --
>> >> > Matt Roper
>> >> > Graphics Software Engineer
>> >> > IoTG Platform Enabling & Development
>> >> > Intel Corporation
>> >> > [phone]
>> >> > _______________________________________________
>> >> > Intel-gfx mailing list
>> >> > [e-mail]
>> >> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> >
>> > --
>> > Matt Roper
>> > Graphics Software Engineer
>> > IoTG Platform Enabling & Development
>> > Intel Corporation
>> > [phone]
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > [e-mail]
>> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> [phone]

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4e73d88..6cf8806 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -106,6 +106,22 @@ 
 	DRM_FORMAT_MOD_INVALID
 };
 
+static const uint32_t nv12_primary_formats[] = {
+	DRM_FORMAT_C8,
+	DRM_FORMAT_RGB565,
+	DRM_FORMAT_XRGB8888,
+	DRM_FORMAT_XBGR8888,
+	DRM_FORMAT_ARGB8888,
+	DRM_FORMAT_ABGR8888,
+	DRM_FORMAT_XRGB2101010,
+	DRM_FORMAT_XBGR2101010,
+	DRM_FORMAT_YUYV,
+	DRM_FORMAT_YVYU,
+	DRM_FORMAT_UYVY,
+	DRM_FORMAT_VYUY,
+	DRM_FORMAT_NV12,
+};
+
 /* Cursor formats */
 static const uint32_t intel_cursor_formats[] = {
 	DRM_FORMAT_ARGB8888,
@@ -13280,8 +13296,14 @@  static bool intel_cursor_plane_format_mod_supported(struct drm_plane *plane,
 		primary->update_plane = skylake_update_primary_plane;
 		primary->disable_plane = skylake_disable_primary_plane;
 	} else if (INTEL_GEN(dev_priv) >= 9) {
-		intel_primary_formats = skl_primary_formats;
-		num_formats = ARRAY_SIZE(skl_primary_formats);
+		if (IS_BROXTON(dev_priv) &&
+			((pipe == PIPE_A || pipe == PIPE_B))) {
+			intel_primary_formats = nv12_primary_formats;
+			num_formats = ARRAY_SIZE(nv12_primary_formats);
+		} else {
+			intel_primary_formats = skl_primary_formats;
+			num_formats = ARRAY_SIZE(skl_primary_formats);
+		}
 		if (pipe < PIPE_C)
 			modifiers = skl_format_modifiers_ccs;
 		else