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

[Ville Syrjälä]

On Thu, Oct 01, 2015 at 02:37:27PM +0300, Ville Syrjälä wrote:
> On Wed, Sep 30, 2015 at 10:58:07PM +, 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(>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

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

[Ville Syrjälä]

On Wed, Sep 30, 2015 at 10:58:07PM +, 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(>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

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

[Konduru, Chandra]

> -Original Message-
> From: Ville Syrjälä [mailto:ville.syrj...@linux.intel.com]
> Sent: Thursday, October 01, 2015 4:41 AM
> To: Konduru, Chandra
> Cc: intel-gfx@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 +, 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(>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 t

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

[Konduru, Chandra]

> > @@ -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(>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;
+   }
}
} 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?
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

On Fri, Sep 11, 2015 at 09:44:18AM -0700, Chandra Konduru wrote:
> This patch adds NV12 as supported format to
> intel_framebuffer_init and performs various checks.
> 
> v2:
> -Fix an issue in checks added (me)
> 
> v3:
> -cosmetic update, split checks into two (Ville)
> 
> v4:
> -Add stride alignment and modifier checks for UV subplane (Ville)
> 
> v5:
> -Make modifier check general (Ville)
> -Check tile-y uv start alignment from begining of page (Ville)
> 
> Signed-off-by: Chandra Konduru 
> Testcase: igt/kms_nv12
> ---
>  drivers/gpu/drm/i915/intel_display.c |   66 
> +++---
>  drivers/gpu/drm/i915/intel_drv.h |2 +-
>  drivers/gpu/drm/i915/intel_sprite.c  |2 +-
>  3 files changed, 55 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index 2a5170e..af28ca9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2914,9 +2914,9 @@ static void ironlake_update_primary_plane(struct 
> drm_crtc *crtc,
>  }
>  
>  u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier,
> -   uint32_t pixel_format)
> +   uint32_t pixel_format, int plane)
>  {
> - u32 bits_per_pixel = drm_format_plane_cpp(pixel_format, 0) * 8;
> + u32 bits_per_pixel = drm_format_plane_cpp(pixel_format, plane) * 8;
>  
>   /*
>* The stride is either expressed as a multiple of 64 bytes
> @@ -3125,7 +3125,7 @@ static void skylake_update_primary_plane(struct 
> drm_crtc *crtc,
>  
>   obj = intel_fb_obj(fb);
>   stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
> -fb->pixel_format);
> +fb->pixel_format, 0);
>   surf_addr = intel_plane_obj_offset(to_intel_plane(plane), obj, 0);
>  
>   /*
> @@ -9104,7 +9104,7 @@ skylake_get_initial_plane_config(struct intel_crtc 
> *crtc,
>  
>   val = I915_READ(PLANE_STRIDE(pipe, 0));
>   stride_mult = intel_fb_stride_alignment(dev, fb->modifier[0],
> - fb->pixel_format);
> + fb->pixel_format, 0);
>   fb->pitches[0] = (val & 0x3ff) * stride_mult;
>  
>   aligned_height = intel_fb_align_height(dev, fb->height,
> @@ -11175,7 +11175,7 @@ static void skl_do_mmio_flip(struct intel_crtc 
> *intel_crtc)
>*/
>   stride = fb->pitches[0] /
>intel_fb_stride_alignment(dev, fb->modifier[0],
> -fb->pixel_format);
> +fb->pixel_format, 0);
>  
>   /*
>* Both PLANE_CTL and PLANE_STRIDE are not updated on vblank but on
> @@ -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(>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.

>   else if (obj->tiling_mode == I915_TILING_Y) {
>   DRM_DEBUG("No Y tiling for legacy addfb\n");
>   return -EINVAL;
> @@ -14280,12 +14282,15 @@ static int intel_framebuffer_init(struct drm_device 
> *dev,
>   return -EINVAL;
>   }
>  
> - stride_alignment = intel_fb_stride_alignment(dev, mode_cmd->modifier[0],
> -  mode_cmd->pixel_format);
> - if (mode_cmd->pitches[0] & (stride_alignment - 1)) {
> - DRM_DEBUG("pitch (%d) must be at least %u byte aligned\n",
> -   mode_cmd->pitches[0], stride_alignment);
> - return -EINVAL;
> + /* check stride alignment for sub-planes */
> + for (i = 0; i < drm_format_num_planes(mode_cmd->pixel_format); i++) {
> + stride_alignment = intel_fb_stride_alignment(dev, 
> mode_cmd->modifier[i],
> +  mode_cmd->pixel_format, i);
> + if (mode_cmd->pitches[i] & (stride_alignment - 1)) {
> + DRM_DEBUG("subplane %d pitch (%d) must be at least %u 
> bytes "
> + "aligned\n", i, mode_cmd->pitches[i], 
> stride_alignment);
> + return -EINVAL;
> + }
>   }
>  
>   

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

[Konduru, Chandra]

> > > > +   if (mode_cmd->modifier[1] == I915_FORMAT_MOD_Y_TILED
> > &&
> > > > +   ((mode_cmd->offsets[1] / mode_cmd->pitches[1]) %
> > 4)) {
> > > > +   DRM_DEBUG("tile-Y uv offset 0x%x isn't 4-line
> > aligned\n",
> > > > +   mode_cmd->offsets[1]);
> > > > +   return -EINVAL;
> > > > +   }
> > >
> > > I was going to say I can't find anything in the spec to support this,
> > > but after some more reading I got it "The display hardware requires
> > > that the UV surface start satisfies four line alignment from the
> > > begining of the page." So the check should be something like
> > > ((offsets[1] & 0xfff) / pitches[1] % 4.
> > >
> Ignore my previous response for this. Yes, above check should
> check for 12-lsbs. Will update and respun shortly.
Submitted updated patch. 
With this change, resolved all your feedback.
Is there any more feedback?
If not, can you issue R-B tag for the patches?
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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.
> 
> Yeah, continuation lines should be aligned with the opening ( whether
> that's an if condition, function parameter list or something else.
> checkpatch --strict will spot these for you.
> 
> I don't mind if it's just the occasional one, but if you get it
> consistently wrong it does make the code a bit harder to follow (simply
> because it's unexpected).

Point noted.


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Thu, Sep 10, 2015 at 08:45:48PM +, Konduru, Chandra wrote:
> > > > > + 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.

Yeah, continuation lines should be aligned with the opening ( whether
that's an if condition, function parameter list or something else.
checkpatch --strict will spot these for you.

I don't mind if it's just the occasional one, but if you get it
consistently wrong it does make the code a bit harder to follow (simply
because it's unexpected).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

On Wed, Sep 09, 2015 at 03:59:03PM -0700, Chandra Konduru wrote:
> This patch adds NV12 as supported format to
> intel_framebuffer_init and performs various checks.
> 
> v2:
> -Fix an issue in checks added (me)
> 
> v3:
> -cosmetic update, split checks into two (Ville)
> 
> v4:
> -Add stride alignment and modifier checks for UV subplane (Ville)
> 
> Signed-off-by: Chandra Konduru 
> Testcase: igt/kms_nv12
> ---
>  drivers/gpu/drm/i915/intel_display.c |   67 
> --
>  drivers/gpu/drm/i915/intel_drv.h |2 +-
>  drivers/gpu/drm/i915/intel_sprite.c  |2 +-
>  3 files changed, 57 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index 84dad95..6124339 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2906,9 +2906,9 @@ static void ironlake_update_primary_plane(struct 
> drm_crtc *crtc,
>  }
>  
>  u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier,
> -   uint32_t pixel_format)
> +   uint32_t pixel_format, int plane)
>  {
> - u32 bits_per_pixel = drm_format_plane_cpp(pixel_format, 0) * 8;
> + u32 bits_per_pixel = drm_format_plane_cpp(pixel_format, plane) * 8;
>  
>   /*
>* The stride is either expressed as a multiple of 64 bytes
> @@ -3117,7 +3117,7 @@ static void skylake_update_primary_plane(struct 
> drm_crtc *crtc,
>  
>   obj = intel_fb_obj(fb);
>   stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
> -fb->pixel_format);
> +fb->pixel_format, 0);
>   surf_addr = intel_plane_obj_offset(to_intel_plane(plane), obj, 0);
>  
>   /*
> @@ -9101,7 +9101,7 @@ skylake_get_initial_plane_config(struct intel_crtc 
> *crtc,
>  
>   val = I915_READ(PLANE_STRIDE(pipe, 0));
>   stride_mult = intel_fb_stride_alignment(dev, fb->modifier[0],
> - fb->pixel_format);
> + fb->pixel_format, 0);
>   fb->pitches[0] = (val & 0x3ff) * stride_mult;
>  
>   aligned_height = intel_fb_align_height(dev, fb->height,
> @@ -11172,7 +11172,7 @@ static void skl_do_mmio_flip(struct intel_crtc 
> *intel_crtc)
>*/
>   stride = fb->pitches[0] /
>intel_fb_stride_alignment(dev, fb->modifier[0],
> -fb->pixel_format);
> +fb->pixel_format, 0);
>  
>   /*
>* Both PLANE_CTL and PLANE_STRIDE are not updated on vblank but on
> @@ -14238,6 +14238,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(>struct_mutex));
> @@ -14277,12 +14278,15 @@ static int intel_framebuffer_init(struct drm_device 
> *dev,
>   return -EINVAL;
>   }
>  
> - stride_alignment = intel_fb_stride_alignment(dev, mode_cmd->modifier[0],
> -  mode_cmd->pixel_format);
> - if (mode_cmd->pitches[0] & (stride_alignment - 1)) {
> - DRM_DEBUG("pitch (%d) must be at least %u byte aligned\n",
> -   mode_cmd->pitches[0], stride_alignment);
> - return -EINVAL;
> + /* check stride alignment for sub-planes */
> + for (i = 0; i < drm_format_num_planes(mode_cmd->pixel_format); i++) {
> + stride_alignment = intel_fb_stride_alignment(dev, 
> mode_cmd->modifier[i],
> +  mode_cmd->pixel_format, i);
> + if (mode_cmd->pitches[i] & (stride_alignment - 1)) {
> + DRM_DEBUG("subplane %d pitch (%d) must be at least %u 
> bytes "
> + "aligned\n", i, mode_cmd->pitches[i], 
> stride_alignment);
> + return -EINVAL;
> + }
>   }
>  
>   pitch_limit = intel_fb_pitch_limit(dev, mode_cmd->modifier[0],
> @@ -14349,9 +14353,48 @@ static int intel_framebuffer_init(struct drm_device 
> *dev,
>   return -EINVAL;
>   }
>   break;
> + case DRM_FORMAT_NV12:
> + if (INTEL_INFO(dev)->gen < 9) {
> + DRM_DEBUG("unsupported pixel format: %s\n",
> + drm_get_format_name(mode_cmd->pixel_format));
> + return -EINVAL;
> + }
> + 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?

> + mode_cmd->modifier[1] = I915_FORMAT_MOD_X_TILED;
> + }

Hmm. This obj 

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

[Ville Syrjälä]

On Thu, Sep 10, 2015 at 07:14:58PM +, Konduru, Chandra wrote:
> > > + 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)


> 
> > 
> > > + 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.

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

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

[Ville Syrjälä]

On Thu, Sep 10, 2015 at 09:34:59PM +0300, Ville Syrjälä wrote:
> On Wed, Sep 09, 2015 at 03:59:03PM -0700, Chandra Konduru wrote:
> > This patch adds NV12 as supported format to
> > intel_framebuffer_init and performs various checks.
> > 
> > v2:
> > -Fix an issue in checks added (me)
> > 
> > v3:
> > -cosmetic update, split checks into two (Ville)
> > 
> > v4:
> > -Add stride alignment and modifier checks for UV subplane (Ville)
> > 
> > Signed-off-by: Chandra Konduru 
> > Testcase: igt/kms_nv12
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |   67 
> > --
> >  drivers/gpu/drm/i915/intel_drv.h |2 +-
> >  drivers/gpu/drm/i915/intel_sprite.c  |2 +-
> >  3 files changed, 57 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c 
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 84dad95..6124339 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2906,9 +2906,9 @@ static void ironlake_update_primary_plane(struct 
> > drm_crtc *crtc,
> >  }
> >  
> >  u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier,
> > - uint32_t pixel_format)
> > + uint32_t pixel_format, int plane)
> >  {
> > -   u32 bits_per_pixel = drm_format_plane_cpp(pixel_format, 0) * 8;
> > +   u32 bits_per_pixel = drm_format_plane_cpp(pixel_format, plane) * 8;
> >  
> > /*
> >  * The stride is either expressed as a multiple of 64 bytes
> > @@ -3117,7 +3117,7 @@ static void skylake_update_primary_plane(struct 
> > drm_crtc *crtc,
> >  
> > obj = intel_fb_obj(fb);
> > stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
> > -  fb->pixel_format);
> > +  fb->pixel_format, 0);
> > surf_addr = intel_plane_obj_offset(to_intel_plane(plane), obj, 0);
> >  
> > /*
> > @@ -9101,7 +9101,7 @@ skylake_get_initial_plane_config(struct intel_crtc 
> > *crtc,
> >  
> > val = I915_READ(PLANE_STRIDE(pipe, 0));
> > stride_mult = intel_fb_stride_alignment(dev, fb->modifier[0],
> > -   fb->pixel_format);
> > +   fb->pixel_format, 0);
> > fb->pitches[0] = (val & 0x3ff) * stride_mult;
> >  
> > aligned_height = intel_fb_align_height(dev, fb->height,
> > @@ -11172,7 +11172,7 @@ static void skl_do_mmio_flip(struct intel_crtc 
> > *intel_crtc)
> >  */
> > stride = fb->pitches[0] /
> >  intel_fb_stride_alignment(dev, fb->modifier[0],
> > -  fb->pixel_format);
> > +  fb->pixel_format, 0);
> >  
> > /*
> >  * Both PLANE_CTL and PLANE_STRIDE are not updated on vblank but on
> > @@ -14238,6 +14238,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(>struct_mutex));
> > @@ -14277,12 +14278,15 @@ static int intel_framebuffer_init(struct 
> > drm_device *dev,
> > return -EINVAL;
> > }
> >  
> > -   stride_alignment = intel_fb_stride_alignment(dev, mode_cmd->modifier[0],
> > -mode_cmd->pixel_format);
> > -   if (mode_cmd->pitches[0] & (stride_alignment - 1)) {
> > -   DRM_DEBUG("pitch (%d) must be at least %u byte aligned\n",
> > - mode_cmd->pitches[0], stride_alignment);
> > -   return -EINVAL;
> > +   /* check stride alignment for sub-planes */
> > +   for (i = 0; i < drm_format_num_planes(mode_cmd->pixel_format); i++) {
> > +   stride_alignment = intel_fb_stride_alignment(dev, 
> > mode_cmd->modifier[i],
> > +mode_cmd->pixel_format, i);
> > +   if (mode_cmd->pitches[i] & (stride_alignment - 1)) {
> > +   DRM_DEBUG("subplane %d pitch (%d) must be at least %u 
> > bytes "
> > +   "aligned\n", i, mode_cmd->pitches[i], 
> > stride_alignment);
> > +   return -EINVAL;
> > +   }
> > }
> >  
> > pitch_limit = intel_fb_pitch_limit(dev, mode_cmd->modifier[0],
> > @@ -14349,9 +14353,48 @@ static int intel_framebuffer_init(struct 
> > drm_device *dev,
> > return -EINVAL;
> > }
> > break;
> > +   case DRM_FORMAT_NV12:
> > +   if (INTEL_INFO(dev)->gen < 9) {
> > +   DRM_DEBUG("unsupported pixel format: %s\n",
> > +   drm_get_format_name(mode_cmd->pixel_format));
> > +   return -EINVAL;
> > +   }
> > +   if (obj->tiling_mode == I915_TILING_X &&
> > +   !(mode_cmd->flags & DRM_MODE_FB_MODIFIERS)) {
> 
> Your editor 

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

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

[Konduru, Chandra]

> > > + if (mode_cmd->modifier[1] == I915_FORMAT_MOD_Y_TILED
> &&
> > > + ((mode_cmd->offsets[1] / mode_cmd->pitches[1]) %
> 4)) {
> > > + DRM_DEBUG("tile-Y uv offset 0x%x isn't 4-line
> aligned\n",
> > > + mode_cmd->offsets[1]);
> > > + return -EINVAL;
> > > + }
> >
> > I was going to say I can't find anything in the spec to support this,
> > but after some more reading I got it "The display hardware requires
> > that the UV surface start satisfies four line alignment from the
> > begining of the page." So the check should be something like
> > ((offsets[1] & 0xfff) / pitches[1] % 4.

Yes, that is the bspec documentation for this check.
It is requiring the uv-offset to be 4-line aligned i.e., checking
uv-offset that is given to driver. If you take away 12-lsbs
then it is already at the page start. Why you want to take
away 12-LSBs?

> >
> > However we should anyway be able to adjust the X/Y offsets to account
> > for misalignment of offsets[1]. I think the only thing we should need
> > to check is that offsets[1] is macropixel aligned. The patch lacks such
> > a check in fact.

UV-start is with respect to buffer and not relate to any incoming flip
clipping the uv where macro-pixel adjustments are done.
Sorry, I didn't get why you think X/Y offset maco-pixel adjustments 
have anything to do with uv-offset checking here in this if block?
 
> >
> > Feels like it would be time to expand intel_gen4_compute_page_offset()
> > to handle the new tiling formats and start using it for SKL+. Though
> > rotation may need some additional thought. Also maybe it's time to
> > dig up my old offsets[0] handling patch and refresh it a bit and try
> > to get it merged again.
> 
> Just to clarify a bit. I think we can initially go with the checks in
> place, and we can work on refactoring the page offset stuff afterwards
> Actually I already started to sketch something together here :) so I
> might have a few patches to post sooner rather than later.


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx