[Intel-gfx] [PATCH 11/15] drm/i915: Add NV12 to primary plane programming.

[Konduru, Chandra]

> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
> Sent: Wednesday, September 09, 2015 11:06 AM
> To: Konduru, Chandra
> Cc: Syrjala, Ville; intel-gfx at lists.freedesktop.org; Vetter, Daniel
> Subject: Re: [Intel-gfx] [PATCH 11/15] drm/i915: Add NV12 to primary plane
> programming.
> 
> On Wed, Sep 09, 2015 at 05:12:06PM +0000, Konduru, Chandra wrote:
> > > > > > > > +			/* For tile-Yf, uv-subplane tile width is 2x of Y-
> > > subplane
> > > > > > > */
> > > > > > > > +			aux_stride = fb->modifier[0] ==
> > > > > > > I915_FORMAT_MOD_Yf_TILED ?
> > > > > > > > +				stride / 2 : stride;
> > > > > > >
> > > > > > > The 2x part was rather well hidden in the spec. How do we deal with
> > > > > > > cases when the Y stride is an odd number of tiles?
> > > > > >
> > > > > > It should be a round up division to take care of that scenario.
> > > > >
> > > > > That would stil lresult in a corrupted picture I think. So I was
> > > > > thinking that we should just refuse to create NCV12 framebuffers with a
> > > > > poorly aligned stride.
> > > > >
> > > > I added a check in intel_framebuffer_init() which should catch them:
> > > >         if (mode_cmd->pitches[0] != mode_cmd->pitches[1]) {
> > > >             DRM_DEBUG("y and uv subplanes have different pitches\n");
> > > >             return -EINVAL;
> > > >         }
> > >
> > > That won't catch the case I'm worried about. We would also need to make
> > > sure pitches[1] is aligned to the UV tile width.
> >
> > If caller is following tile/row/pitch alignments properly for sub-planes of
> > NV12 Yf buffer, above check will catch.
> > But are you referring a case where userland isn't following tile/row size
> > alignments properly? In that case, above may not catch. But
> > isn't that is the case even with other FB formats if user land not
> > following tile/row/pitch alignments?
> 
> We reject any attempt to create a framebuffer with a poorly aligned
> stride.
> 
> Hmm. Actually I suppose we should just handle it in
> intel_fb_stride_alignment(). Eg.:
> 
> case Yf:
> 	if (cpp != 1 || pixel_format == DRM_FORMAT_NV12)
> 		return 128;
> 	else
> 		return 64;

This check is already there in intel_fb_stride_alignment()
which is called from intel_framebuffer_init(). But it always
does for 1st sub-plane which means does for Y.
It needs an update to pass a sub-plane (for UV) parameter, 
and made another call for UV-plane alignment check.
Will this be ok?

> 
> --
> Ville Syrjälä
> Intel OTC