> > > > > > > > > > + /* 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? > > -- > Ville Syrjälä > Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx