[Intel-gfx] [PATCH 09/15] drm/i915: Add NV12 support to intel_framebuffer_init

[Konduru, Chandra]

> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
> Sent: Thursday, October 01, 2015 4:41 AM
> To: Konduru, Chandra
> Cc: intel-gfx at lists.freedesktop.org; Vetter, Daniel; Syrjala, Ville
> Subject: Re: [Intel-gfx] [PATCH 09/15] drm/i915: Add NV12 support to
> intel_framebuffer_init
> 
> 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.

What is the issue with current offsets the patch is using?
Also I think linear offset doesn't work for hw (it is actually confusing to say
linear offset for a tiling format to start with). Requires a conversion to tiling format
which requires i915 addfb logic to know tiling limitations/restrictions from media 
and/or other engines. 
Userlands know where the UV start is for every tiling format if they are using the
tiling format and giving the UV offset to i915. If someone didn't set offset right, 
above check is helping to point that out.