[Intel-gfx] [PATCH 08/12] drm/i915: Add NV12 support to intel_framebuffer_init

[Konduru, Chandra]

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Tuesday, May 19, 2015 1:24 AM
> To: Konduru, Chandra
> Cc: intel-gfx at lists.freedesktop.org; Vetter, Daniel; Syrjala, Ville
> Subject: Re: [Intel-gfx] [PATCH 08/12] drm/i915: Add NV12 support to
> intel_framebuffer_init
> 
> On Sun, May 17, 2015 at 10:11:01PM -0700, Chandra Konduru wrote:
> > This patch adds NV12 as supported format to
> > intel_framebuffer_init and performs various checks.
> >
> > Signed-off-by: Chandra Konduru <chandra.konduru at intel.com>
> > Testcase: igt/kms_nv12
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |   27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> > index 42924a6..41cd26f 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -14043,6 +14043,33 @@ 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;
> > +		}
> 
> Nope. It's perfectly ok to have NV12 with a 0 offset for the uv plane, if
> it's e.g. in a separate buffer object. Which is the part this series seems
> to be completely missing - there's no code at all to look up (and store in
> intel_framebuffer the 2nd i915_bo pointer) the 2nd buffer handle afaics.
> 
> You should also change your igts to use 2 separate buffers, just for test
> coverage.
> -Daniel

Hi Daniel,
Agree, in general that is very well ok. But as skl hw requires uv to be after 
y in gtt. This can be guaranteed by having a single bo and y and uv offsets 
into it. Above sanity checks in i915 specific fb init call are for that reason.
There are definitely ways to guarantee uv to be after y even with two 
separate bos (by uv remapping), but I see that is unnecessary 
complication and not sure value by allowing that. Or am I missing 
something here?

> 
> > +		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;
> > +		}
> > +		if (mode_cmd->modifier[1] == I915_FORMAT_MOD_Yf_TILED
> &&
> > +			(mode_cmd->offsets[1] & 0xFFF)) {
> > +			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]);
> > +		}
> > +		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 at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch