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

[Ville Syrjälä]

On Wed, Sep 09, 2015 at 09:09:27PM +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?
> > 
> > Yeah. You could in fact just have it loop over all the planes
> > and call it for each. Something like this perhaps:
> > for (i = 0; i < drm_format_num_planes(formt); i++) {
> > 	intel_fb_stride_alignment(modifier[i], format, i);
> > 	...
> > }
> I planned the same, looping for all subplanes.
> 
> > 
> > Or you could pass the cpp/bits_per_pixel instead of the plane index,
> > since that's the only thing for which you need the plane index within
> > the function.
> > 
> > I also started to wonder whether we should repeat most of the other
> > checks in intel_framebuffer_init() for each plane. But it's probably
> > just easier to check that handle, pitch and modifier matches for
> > both planes in the NV12 case. I think you actually missed the
> > modifier check. You just checked that modifier[1] is Yf, but that
> > leaves modifier[0] unchecked. I failed to notice it as well during
> > my review of the relevant patch.
> 
> Added modifier check.
> Made these changes to " drm/i915: Add NV12 support to intel_framebuffer_init"
> patch. Also sending out updated WA patch to move to init clockgating.
> Rest of your feedback is already addressed. 
> Before sending out these 2 patches, any other comments?

Looking at what was said, I think we covered most open items
reasonably well, so fire away. I'll start going through the
updated patches tomorrow.

-- 
Ville Syrjälä
Intel OTC