[Intel-gfx] [PATCH v8 4/6] drm/i915: Add NV12 support to intel_framebuffer_init

[John Harrison]

On 5/11/2018 2:33 PM, Vidya Srinivas wrote:
> From: Chandra Konduru <chandra.konduru at 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)
>
> v10: NV12 supported by all GEN >= 9.
> Making this change in intel_framebuffer_init. This is
> part of addressing Maarten's review comments.
> Comment under v8 no longer applicable
>
> v11: Addressed review comments from Shashank Sharma
>
> v12: Adding Reviewed By from Shashank Sharma
>
> v13: Addressed review comments from Juha-Pekka Heikkila
> "NV12 not to be supported by SKL"
>
> v14: Addressed review comments from Maarten.
> Add checks for fb width height for NV12 and fail the fb
> creation if check fails. Added reviewed by from
> Juha-Pekka Heikkila
>
> v15: Rebased the series
>
> v16: Setting the minimum value during fb creating to 16
> as per Bspec for NV12. Earlier minimum was expected
> to be > 16. Now changed it to >=16.
>
> v17: Adding restriction to framebuffer_init - the fb
> width and height should be a multiplier of 4
>
> v18: Added RB from Maarten. Included Maarten's review comments
> Dont allow CCS formats for fb creation of NV12
>
> v19: Review comments from Maarten addressed -
> Removing BROXTON support for NV12 due to WA826
>
> Credits-to: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Tested-by: Clinton Taylor <clinton.a.taylor at intel.com>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Reviewed-by: Shashank Sharma <shashank.sharma at intel.com>
> Reviewed-by: Clinton Taylor <clinton.a.taylor at intel.com>
> Reviewed-by: Juha-Pekka Heikkila <juhapekka.heikkila at gmail.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_display.c | 22 ++++++++++++++++++++++
>   1 file changed, 22 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 6c42910..e746948 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14291,6 +14291,20 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
>   			goto err;
>   		}
>   		break;
> +	case DRM_FORMAT_NV12:
> +		if (mode_cmd->modifier[0] == I915_FORMAT_MOD_Y_TILED_CCS ||
> +		    mode_cmd->modifier[0] == I915_FORMAT_MOD_Yf_TILED_CCS) {
> +			DRM_DEBUG_KMS("RC not to be enabled with NV12\n");
> +			goto err;
> +		}
> +		if (INTEL_GEN(dev_priv) < 9 || IS_SKYLAKE(dev_priv) ||
> +		    IS_BROXTON(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));
> @@ -14303,6 +14317,14 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
>   
>   	drm_helper_mode_fill_fb_struct(&dev_priv->drm, fb, mode_cmd);
>   
> +	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)) {
> +		DRM_DEBUG_KMS("src dimensions not correct for NV12\n");
> +		return -EINVAL;

Shouldn't this be 'goto err'? All other early exits before and after 
this do the goto to get the reference count clean up.


> +	}
> +
>   	for (i = 0; i < fb->format->num_planes; i++) {
>   		u32 stride_alignment;
>