Re: [PATCH 11/16] drm/i915: Update format_is_yuv() to include NV12

["Sharma, Shashank" <shashank.sharma@intel.com>]

Regards

Shashank


On 2/6/2018 6:28 PM, Vidya Srinivas wrote:
> From: Chandra Konduru <chandra.konduru@intel.com>
>
> This patch adds NV12 to format_is_yuv() function
> for sprite planes.
>
> v2:
> -Use intel_ prefix for format_is_yuv (Ville)
>
> v3: Rebased (me)
>
> v4: Rebased and addressed review comments from Clinton A Taylor.
> "static function in intel_sprite.c is not available
> to the primary plane functions".
> Changed commit message - function modified for
> sprite planes.
>
> v5: Missed the Tested-by/Reviewed-by in the previous series
> Adding the same to commit message in this version.
>
> v6: Rebased (me)
>
> v7: Rebased (me)
>
> v8: Rebased (me)
>
> v9: Rebased (me)
>
> v10: Changed intel_format_is_yuv function from
> static to non-static. We need to use it later from
> other files for check.
>
> 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_drv.h    | 1 +
>   drivers/gpu/drm/i915/intel_sprite.c | 8 ++++----
>   2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 65dac21..d4c027a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -2032,6 +2032,7 @@ void skl_disable_plane(struct intel_plane *plane, struct intel_crtc *crtc);
>   bool skl_plane_get_hw_state(struct intel_plane *plane);
>   bool skl_plane_has_ccs(struct drm_i915_private *dev_priv,
>   		       enum pipe pipe, enum plane_id plane_id);
> +bool intel_format_is_yuv(uint32_t format);
>   
>   /* intel_tv.c */
>   void intel_tv_init(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 3be22c0..9e31be2 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -41,14 +41,14 @@
>   #include <drm/i915_drm.h>
>   #include "i915_drv.h"
>   
> -static bool
> -format_is_yuv(uint32_t format)
> +bool intel_format_is_yuv(uint32_t format)
>   {
>   	switch (format) {
>   	case DRM_FORMAT_YUYV:
>   	case DRM_FORMAT_UYVY:
>   	case DRM_FORMAT_VYUY:
>   	case DRM_FORMAT_YVYU:
> +	case DRM_FORMAT_NV12:
>   		return true;
>   	default:
>   		return false;
> @@ -352,7 +352,7 @@ chv_update_csc(struct intel_plane *plane, uint32_t format)
>   	enum plane_id plane_id = plane->id;
>   
>   	/* Seems RGB data bypasses the CSC always */
> -	if (!format_is_yuv(format))
> +	if (!intel_format_is_yuv(format))
This assumption that CSC does only limited_range to full_range 
conversion is only correct until we enable plane level gamut mapping / 
generic color conversion. It would be great if we keep it like:
chv_update_csc() {
     if (intel_format_is_yuv()) {
         /* limited_range -> full_range code here */
     }
     /* So that generic CSC/GM code can be added here */
}

But this should not really a blocker for this patch,
>   		return;
>   
>   	/*
> @@ -979,7 +979,7 @@ intel_check_sprite_plane(struct intel_plane *plane,
>   		src_y = src->y1 >> 16;
>   		src_h = drm_rect_height(src) >> 16;
>   
> -		if (format_is_yuv(fb->format->format)) {
> +		if (intel_format_is_yuv(fb->format->format)) {
>   			src_x &= ~1;
>   			src_w &= ~1;
>   
so with or without above comment addressed:
Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx