[v8,4/6] drm/i915: Add NV12 support to intel_framebuffer_init
diff mbox

Message ID 1526074397-10457-5-git-send-email-vidya.srinivas@intel.com
State New
Headers show

Commit Message

vsrini4 May 11, 2018, 9:33 p.m. UTC
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)

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@linux.intel.com>
Tested-by: Clinton Taylor <clinton.a.taylor@intel.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>
Reviewed-by: Clinton Taylor <clinton.a.taylor@intel.com>
Reviewed-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.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 | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

Comments

John Harrison June 7, 2018, 11:51 p.m. UTC | #1
On 5/11/2018 2:33 PM, Vidya Srinivas wrote:
> 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)
>
> 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@linux.intel.com>
> Tested-by: Clinton Taylor <clinton.a.taylor@intel.com>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>
> Reviewed-by: Clinton Taylor <clinton.a.taylor@intel.com>
> Reviewed-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.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 | 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;
>

Patch
diff mbox

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;
+	}
+
 	for (i = 0; i < fb->format->num_planes; i++) {
 		u32 stride_alignment;