[Intel-gfx] [PATCH 14/15] drm/i915: Add NV12 as supported format for sprite plane

[Srinivas, Vidya]

> -----Original Message-----
> From: Maarten Lankhorst [mailto:maarten.lankhorst at linux.intel.com]
> Sent: Tuesday, January 9, 2018 3:32 PM
> To: Srinivas, Vidya <vidya.srinivas at intel.com>; intel-
> gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 14/15] drm/i915: Add NV12 as supported
> format for sprite plane
> 
> Op 09-01-18 om 03:19 schreef Srinivas, Vidya:
> >
> >> -----Original Message-----
> >> From: Maarten Lankhorst [mailto:maarten.lankhorst at linux.intel.com]
> >> Sent: Monday, January 8, 2018 6:04 PM
> >> To: Srinivas, Vidya <vidya.srinivas at intel.com>; intel-
> >> gfx at lists.freedesktop.org
> >> Subject: Re: [Intel-gfx] [PATCH 14/15] drm/i915: Add NV12 as
> >> supported format for sprite plane
> >>
> >> Op 07-01-18 om 10:59 schreef Vidya Srinivas:
> >>> From: Chandra Konduru <chandra.konduru at intel.com>
> >>>
> >>> This patch adds NV12 to list of supported formats for sprite plane.
> >>>
> >>> v2: Rebased (me)
> >>>
> >>> v3: Review comments by Ville addressed
> >>> - Removed skl_plane_formats_with_nv12 and added
> >>> NV12 case in existing skl_plane_formats
> >>> - Added the 10bpc RGB formats
> >>>
> >>> v4: Addressed review comments from Clinton A Taylor "Why are we
> >> adding
> >>> 10 bit RGB formats with the NV12 series patches?
> >>> Trying to set XR30 or AB30 results in error returned even though the
> >>> modes are advertised for the planes"
> >>> - Removed 10bit RGB formats added previously with NV12 series
> >>>
> >>> v5: Missed the Tested-by/Reviewed-by in the previous series Adding
> >>> the same to commit message in this version.
> >>> Addressed review comments from Clinton A Taylor "Why are we adding
> >>> 10 bit RGB formats with the NV12 series patches?
> >>> Trying to set XR30 or AB30 results in error returned even though the
> >>> modes are advertised for the planes"
> >>> - Previous version has 10bit RGB format removed from VLV formats by
> >>> mistake. Fixing that in this version.
> >>> Removed 10bit RGB formats added previously with NV12 series for SKL.
> >>>
> >>> v6: Addressed review comments by Ville Restricting the NV12 to BXT
> >>> and PIPE A and B
> >>>
> >>> v7: Rebased (me)
> >>>
> >>> v8: Rebased (me)
> >>> Restricting NV12 changes to BXT and KBL Restricting NV12 changes for
> >>> plane 0 (overlay)
> >>>
> >>> v9: Rebased (me)
> >>>
> >>> Tested-by: Clinton Taylor <clinton.a.taylor at intel.com>
> >>> Reviewed-by: Clinton Taylor <clinton.a.taylor at intel.com>
> >>> Signed-off-by: Chandra Konduru <chandra.konduru at intel.com>
> >>> Signed-off-by: Nabendu Maiti <nabendu.bikash.maiti at intel.com>
> >>> Signed-off-by: Vidya Srinivas <vidya.srinivas at intel.com>
> >>> ---
> >>>  drivers/gpu/drm/i915/intel_sprite.c | 24 +++++++++++++++++++++---
> >>>  1 file changed, 21 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> >>> b/drivers/gpu/drm/i915/intel_sprite.c
> >>> index 09732ae..1d35a18 100644
> >>> --- a/drivers/gpu/drm/i915/intel_sprite.c
> >>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> >>> @@ -1279,6 +1279,19 @@ static const struct drm_plane_funcs
> >> intel_sprite_plane_funcs = {
> >>>          .format_mod_supported =
> >>> intel_sprite_plane_format_mod_supported,
> >>>  };
> >>>
> >>> +static uint32_t nv12_plane_formats[] = {
> >>> +	DRM_FORMAT_RGB565,
> >>> +	DRM_FORMAT_ABGR8888,
> >>> +	DRM_FORMAT_ARGB8888,
> >>> +	DRM_FORMAT_XBGR8888,
> >>> +	DRM_FORMAT_XRGB8888,
> >>> +	DRM_FORMAT_YUYV,
> >>> +	DRM_FORMAT_YVYU,
> >>> +	DRM_FORMAT_UYVY,
> >>> +	DRM_FORMAT_VYUY,
> >>> +	DRM_FORMAT_NV12,
> >>> +};
> >>> +
> >>>  struct intel_plane *
> >>>  intel_sprite_plane_create(struct drm_i915_private *dev_priv,
> >>>  			  enum pipe pipe, int plane)
> >>> @@ -1323,9 +1336,14 @@ intel_sprite_plane_create(struct
> >> drm_i915_private *dev_priv,
> >>>  		intel_plane->update_plane = skl_update_plane;
> >>>  		intel_plane->disable_plane = skl_disable_plane;
> >>>  		intel_plane->get_hw_state = skl_plane_get_hw_state;
> >>> -
> >>> -		plane_formats = skl_plane_formats;
> >>> -		num_plane_formats = ARRAY_SIZE(skl_plane_formats);
> >>> +		if ((IS_BROXTON(dev_priv) || IS_KABYLAKE(dev_priv)) &&
> >>> +			(pipe == PIPE_A || pipe == PIPE_B) && plane == 0) {
> >>> +			plane_formats = nv12_plane_formats;
> >>> +			num_plane_formats =
> >> ARRAY_SIZE(nv12_plane_formats);
> >>> +		} else {
> >>> +			plane_formats = skl_plane_formats;
> >>> +			num_plane_formats =
> >> ARRAY_SIZE(skl_plane_formats);
> >>> +		}
> >> I understand we don't have enough scalers, but is there something
> >> wrong with allowing nv12 on any plane?
> >>
> > On BXT (Gen9), NV12 is supported only on primary and sprite 0.
> Ah ok, nm that comment then. :)
> 
> Same comment applies as for the other patch that adds it to the primary
> plane, would be better to have a single array and tinker with
> num_plane_formats, should gen10 also have NV12 support here btw?
> 
> I can't r-b the series right now, there are no tests. Judging from the
> comments this is what the kms_plane_scaling is about, so I started looking
> at it.
> 
Thank you. We need to add NV12 support for Gen10 also but for now
we are trying to get the base framework in. We have tested it on
APL board for now.Just for reference,
in case you need you can see this igt which was floated to cover
nv12 testing.
https://patchwork.kernel.org/patch/9995943/ 
https://patchwork.kernel.org/patch/9995945/ 

Regards
Vidya