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

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

On Thu, Oct 01, 2015 at 02:37:27PM +0300, Ville Syrjälä wrote:
> 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.

Oh and I just figured out that linear offset would probably be better
because that also isolates us from having to think about the internal
tile layout, as in which way the bytes/owords/whatver are walked within
the tile.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx