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