Re: [PATCH 09/15] drm/i915: Add NV12 support to intel_framebuffer_init

Ville Syrjälä
2015-10-01T11:37:32Z

On Wed, Sep 30, 2015 at 10:58:07PM +0000, Konduru, Chandra wrote:
> > > @@ -14241,6 +14241,7 @@ static int intel_framebuffer_init(struct
> > drm_device *dev,
> > > {
> > > unsigned int aligned_height;
> > > int ret;
> > > + int i;
> > > u32 pitch_limit, stride_alignment;
> > >
> > > WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> > > @@ -14255,7 +14256,8 @@ static int intel_framebuffer_init(struct
> > drm_device *dev,
> > > }
> > > } else {
> > > if (obj->tiling_mode == I915_TILING_X)
> > > - mode_cmd->modifier[0] =
> > I915_FORMAT_MOD_X_TILED;
> > > + for (i = 0; i < drm_format_num_planes(mode_cmd-
> > >pixel_format); i++)
> > > + mode_cmd->modifier[i] =
> > I915_FORMAT_MOD_X_TILED;
> >
> > The other branch needs updating too so that it will reject the operation
> > if the modifier disagrees with the obj tiling mode.
>
> Is below something you meant?
>
> @@ -14223,10 +14223,12 @@ static int intel_framebuffer_init(struct drm_device *d
> if (mode_cmd->flags & DRM_MODE_FB_MODIFIERS) {
> /* Enforce that fb modifier and tiling mode match, but only for
> * X-tiled. This is needed for FBC. */
> - if (!!(obj->tiling_mode == I915_TILING_X) !=
> - !!(mode_cmd->modifier[0] == I915_FORMAT_MOD_X_TILED)) {
> - DRM_DEBUG("tiling_mode doesn't match fb modifier\n");
> - return -EINVAL;
> + for (i = 0; i < drm_format_num_planes(mode_cmd->pixel_format); i
> + if (!!(obj->tiling_mode == I915_TILING_X) !=
> + !!(mode_cmd->modifier[i] == I915_FORMAT_MOD_X_TILED)) {
> + DRM_DEBUG("tiling_mode doesn't match fb modifier
> + return -EINVAL;
> + }
> }

Yep.

> } else {
>
> > > + if (mode_cmd->modifier[1] == I915_FORMAT_MOD_Yf_TILED
> > &&
> > > + (mode_cmd->offsets[1] & 0xFFF)) {
> >
> > I've been trying to solicit ideas on how we should define the offsets[];
> > raw byte offset, or linear offset. I didn't get many opinions yet. So we
> > need to figure it out and document it somewhere before we expose it to
> > the world. In the meantime we could just reject non tile row aligned
> > offsets regardless of the tiling mode.
>
> Above check is simply making sure tile Yf, uv offset starts on a new page.
> Is there any issue with above check?

It won't necessarily be a page boundary if we interpret offsets[] as a linear
offset.

Eg. let's assume 4x4 tile size, stride=8, and offset=16. If interpret
the offset as a linear offset we would land at 'x', but interpreted as
a raw byte offset (not sure that's a good name, maybe untiled offset?)
we'd land at 'y'.

-----------
| |y |
| | |
|x | |
| | |
-----------
|z | |
| | |
| | |
| | |
-----------

If we had offset=32, then of course we would land at 'z' for both cases,
which is why I suggested that if we haven't made up our mind about what
offsets[] is, we could require it to be tile row aligned so that there
would be no difference between the two interpretations.

--
Ville Syrjälä
Intel OTC