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

[Konduru, Chandra]

> > > > > > 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?
> > >
> > > 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.
> >
> > Hi Daniel,
> > NV12 programming is documented in bspec under display planes "Plane
> > Planar YUV programming". There it talks about aux_dist which is the
> > distance between y and uv planes expecting uv to be after y.
> 
> Bspec talks about wrap-around, which at least indicates that this might be
> possible and the hw doesn't have a real restriction here. Art, can you please
> double-check whether we could wrape-around PLANE_AUX_DIST with a 2s
> complement to avoid placement restrictions on the aux buffers?
> 
> Bspec doesn't say anything about this being required to be positive ...
> -Daniel

Yeah, it isn't explicit, so to be sure a while ago checked with hw team about
uv plane positioning relative to y-plane. It was confirmed that aux_dist to be 
positive. Certainly Art can double confirm for you.

> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch