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

[Konduru, Chandra]

> > > > +		if (obj->tiling_mode == I915_TILING_X &&
> > > > +			!(mode_cmd->flags & DRM_MODE_FB_MODIFIERS)) {
> > >
> > > Your editor still seems to mess up the indentation in these cases. Can
> > > you try to fix that?
> >
> > If condition isn't fitting in a single line, so continued in next line like any other if
> condition.
> > Not sure what the exact indentation issue you are observing.
> 
> Stuff should line up nicely after the opening (
> 
> if (foo &&
>     bar &&
>     zonk)
> 
> and not
> 
> if (foo &&
> 	bar &&
> 	zonk)

I am continuing tab to indent the line below "if" to move past "if".
In the code, I see "space" or "tab" to move past "if" in line below.
One or the other, I used tab. But if that is causing issue, it is
Fine to use "spaces".

Anyway, this code is going away, as it is being merged into
beginning of the function.

> 
> 
> >
> > >
> > > > +			mode_cmd->modifier[1] =
> I915_FORMAT_MOD_X_TILED;
> > > > +		}
> > >
> > > Hmm. This obj tiling -> modifier conversion should be a fairly generic
> > > thing to do, so I suggest doing it for all planes in one place. Maybe
> > > add a new function intel_fb_obj_tiling_to_modifier() or something like
> > > that and loop over all the planes there.
> >
> > Then will merge this assignment into beginning of the function
> > which does for modifier[0]. Will make a loop there.
> > Is that ok?
> 
> Sure.
> 
> >
> > >
> > > > +		if (!mode_cmd->offsets[1]) {
> > > > +			DRM_DEBUG("uv start offset not set\n");
> > > > +			return -EINVAL;
> > > > +		}
> > >
> > > Still not really happy with this check since it's either too limiting, or
> > > not restrictive enough. Depending on how you look at it. Do you know
> > > if the hardware gets upset if you tell it use overlapping Y and UV
> > > surfaces?
> >
> > May be not.
> > But setting NV12 UV offset from userland can be a miss. So added
> > this check to help userland knowing the issue.
> 
> It only catches the 0 case though. I think if userspace would forget to
> set offsets[1] it would most likely also forget handles[1], so I think
> we should be fairly well covered against the most obvious userspace
> fumbles.
Yes, both handles and offsets are covered. So keeping this if block.

> 
> --
> Ville Syrjälä
> Intel OTC