Re: [PATCH 08/12] drm/i915: Add NV12 support to intel_framebuffer_init

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, May 19, 2015 at 04:31:54PM +0000, Konduru, Chandra wrote:
> 
> 
> > -----Original Message-----
> > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> > Sent: Tuesday, May 19, 2015 1:24 AM
> > To: Konduru, Chandra
> > Cc: intel-gfx@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@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?

For a start you don't reject multiplane stuff where this isn't the case.
And if we indeed have the hw requirement that the gtt address for y must
be before the gtt address for uv (sounds strange to me, definitely need a
bspec reference for that) then we need to check that throughroughly:
Currently you could place Y after UV even in a single BO.

Also we need igts to make sure we catch userspace throwing invalid stuff
at us.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx