Re: [PATCH 10/15] drm/i915: Add NV12 support to intel_framebuffer_init

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Aug 19, 2015 at 06:02:31PM -0700, Chandra Konduru wrote:
> This patch adds NV12 as supported format to
> intel_framebuffer_init and performs various checks.
> 
> v2:
> -Fix an issue in checks added (me)
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> Testcase: igt/kms_nv12
> ---
>  drivers/gpu/drm/i915/intel_display.c |   28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e4a6a91..4df4d77 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14343,6 +14343,34 @@ static int intel_framebuffer_init(struct drm_device *dev,
>  			return -EINVAL;
>  		}
>  		break;
> +	case DRM_FORMAT_NV12:
> +		if (INTEL_INFO(dev)->gen < 9) {
> +			DRM_DEBUG("unsupported pixel format: %s\n",
> +				  drm_get_format_name(mode_cmd->pixel_format));
> +			return -EINVAL;
> +		}
> +		if (!mode_cmd->offsets[1]) {
> +			DRM_DEBUG("uv start offset not set\n");
> +			return -EINVAL;
> +		}

I'm not sure this check makes much sense. We should perhaps either reject
all cases where the planes overlap, or allow it all.

Some years ago I was thinking of adding overlap check to drm core, but
then I figured maybe someone wants to interleave the planes in memory,
so the overlap checks would then have to check each line for overlap
so it gets a bit nasty. So I'm not sure there's any point in disallowing
overlapping planes.

> +		if (mode_cmd->pitches[0] != mode_cmd->pitches[1] ||
> +			mode_cmd->handles[0] != mode_cmd->handles[1]) {
> +			DRM_DEBUG("y and uv subplanes have different parameters\n");
> +			return -EINVAL;
> +		}

Maybe we'd want a separate debug message for these two. I think the
shared stride limitation would be fairly common in other hardware, but
the fact that we require the bo to be the same is unfortunately quite
a special "feature" of our latest hardware. So having a clear debug
message for it might help people figure out why their seemingly valid
code doesn't work on i915.

> +		if (mode_cmd->modifier[1] == I915_FORMAT_MOD_Yf_TILED &&
> +			(mode_cmd->offsets[1] & 0xFFF)) {

Bunch of indentation problems in this patch as well.

> +			DRM_DEBUG("tile-Yf uv offset 0x%x isn't starting on new tile-row\n",
> +				mode_cmd->offsets[1]);
> +			return -EINVAL;
> +		}
> +		if (mode_cmd->modifier[1] == I915_FORMAT_MOD_Y_TILED &&
> +			((mode_cmd->offsets[1] / mode_cmd->pitches[1]) % 4)) {
> +			DRM_DEBUG("tile-Y uv offset 0x%x isn't 4-line aligned\n",
> +				mode_cmd->offsets[1]);
> +			return -EINVAL;
> +		}
> +		break;
>  	default:
>  		DRM_DEBUG("unsupported pixel format: %s\n",
>  			  drm_get_format_name(mode_cmd->pixel_format));
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx