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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx