| Message ID | 1441839543-20005-1-git-send-email-chandra.konduru@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <chandra.konduru@intel.com> > 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(&dev->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 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. > + 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? > + if (mode_cmd->pitches[0] != mode_cmd->pitches[1]) { > + DRM_DEBUG("y and uv subplanes have different pitches\n"); > + return -EINVAL; > + } > + if (mode_cmd->handles[0] != mode_cmd->handles[1]) { > + DRM_DEBUG("y and uv subplanes have different handles\n"); > + return -EINVAL; > + } > + if (mode_cmd->modifier[0] != mode_cmd->modifier[1]) { > + DRM_DEBUG("y and uv subplanes have different modifiers\n"); > + return -EINVAL; > + } > + if (mode_cmd->modifier[1] == I915_FORMAT_MOD_Yf_TILED && > + (mode_cmd->offsets[1] & 0xFFF)) { > + DRM_DEBUG("tile-Yf uv offset 0x%x isn't starting on new tile-row\n", > + mode_cmd->offsets[1]); > + return -EINVAL; > + } > + 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. 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. 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. > + break; > default: > DRM_DEBUG("unsupported pixel format: %s\n", > - drm_get_format_name(mode_cmd->pixel_format)); > + drm_get_format_name(mode_cmd->pixel_format)); > return -EINVAL; > } > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index d50b8cb..62d2a11 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -980,7 +980,7 @@ unsigned int intel_fb_align_height(struct drm_device *dev, > void intel_fb_obj_flush(struct drm_i915_gem_object *obj, bool retire, > enum fb_op_origin origin); > u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier, > - uint32_t pixel_format); > + uint32_t pixel_format, int plane); > > /* intel_audio.c */ > void intel_init_audio(struct drm_device *dev); > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > index 797594e..49feae0 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -203,7 +203,7 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc, > src_w != crtc_w || src_h != crtc_h); > > stride_div = intel_fb_stride_alignment(dev, fb->modifier[0], > - fb->pixel_format); > + fb->pixel_format, 0); > > scaler_id = to_intel_plane_state(drm_plane->state)->scaler_id; > > -- > 1.7.9.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > + 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. > > > + 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? > > > + 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.
On Thu, Sep 10, 2015 at 07:14:58PM +0000, 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.
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 <chandra.konduru@intel.com> > > 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(&dev->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 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. > > > + 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? > > > + if (mode_cmd->pitches[0] != mode_cmd->pitches[1]) { > > + DRM_DEBUG("y and uv subplanes have different pitches\n"); > > + return -EINVAL; > > + } > > + if (mode_cmd->handles[0] != mode_cmd->handles[1]) { > > + DRM_DEBUG("y and uv subplanes have different handles\n"); > > + return -EINVAL; > > + } > > + if (mode_cmd->modifier[0] != mode_cmd->modifier[1]) { > > + DRM_DEBUG("y and uv subplanes have different modifiers\n"); > > + return -EINVAL; > > + } > > + if (mode_cmd->modifier[1] == I915_FORMAT_MOD_Yf_TILED && > > + (mode_cmd->offsets[1] & 0xFFF)) { > > + DRM_DEBUG("tile-Yf uv offset 0x%x isn't starting on new tile-row\n", > > + mode_cmd->offsets[1]); > > + return -EINVAL; > > + } > > + 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. > > 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. > > 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. > > > + break; > > default: > > DRM_DEBUG("unsupported pixel format: %s\n", > > - drm_get_format_name(mode_cmd->pixel_format)); > > + drm_get_format_name(mode_cmd->pixel_format)); > > return -EINVAL; > > } > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > index d50b8cb..62d2a11 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -980,7 +980,7 @@ unsigned int intel_fb_align_height(struct drm_device *dev, > > void intel_fb_obj_flush(struct drm_i915_gem_object *obj, bool retire, > > enum fb_op_origin origin); > > u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier, > > - uint32_t pixel_format); > > + uint32_t pixel_format, int plane); > > > > /* intel_audio.c */ > > void intel_init_audio(struct drm_device *dev); > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > > index 797594e..49feae0 100644 > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > @@ -203,7 +203,7 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc, > > src_w != crtc_w || src_h != crtc_h); > > > > stride_div = intel_fb_stride_alignment(dev, fb->modifier[0], > > - fb->pixel_format); > > + fb->pixel_format, 0); > > > > scaler_id = to_intel_plane_state(drm_plane->state)->scaler_id; > > > > -- > > 1.7.9.5 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > + 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
> > > + 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.
On Thu, Sep 10, 2015 at 08:45:48PM +0000, 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
> > > > > > + 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.
> > > > + 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?
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(&dev->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)) { + mode_cmd->modifier[1] = I915_FORMAT_MOD_X_TILED; + } + if (!mode_cmd->offsets[1]) { + DRM_DEBUG("uv start offset not set\n"); + return -EINVAL; + } + if (mode_cmd->pitches[0] != mode_cmd->pitches[1]) { + DRM_DEBUG("y and uv subplanes have different pitches\n"); + return -EINVAL; + } + if (mode_cmd->handles[0] != mode_cmd->handles[1]) { + DRM_DEBUG("y and uv subplanes have different handles\n"); + return -EINVAL; + } + if (mode_cmd->modifier[0] != mode_cmd->modifier[1]) { + DRM_DEBUG("y and uv subplanes have different modifiers\n"); + return -EINVAL; + } + if (mode_cmd->modifier[1] == I915_FORMAT_MOD_Yf_TILED && + (mode_cmd->offsets[1] & 0xFFF)) { + DRM_DEBUG("tile-Yf uv offset 0x%x isn't starting on new tile-row\n", + mode_cmd->offsets[1]); + return -EINVAL; + } + 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; + } + break; default: DRM_DEBUG("unsupported pixel format: %s\n", - drm_get_format_name(mode_cmd->pixel_format)); + drm_get_format_name(mode_cmd->pixel_format)); return -EINVAL; } diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index d50b8cb..62d2a11 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -980,7 +980,7 @@ unsigned int intel_fb_align_height(struct drm_device *dev, void intel_fb_obj_flush(struct drm_i915_gem_object *obj, bool retire, enum fb_op_origin origin); u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier, - uint32_t pixel_format); + uint32_t pixel_format, int plane); /* intel_audio.c */ void intel_init_audio(struct drm_device *dev); diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 797594e..49feae0 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -203,7 +203,7 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc, src_w != crtc_w || src_h != crtc_h); stride_div = intel_fb_stride_alignment(dev, fb->modifier[0], - fb->pixel_format); + fb->pixel_format, 0); scaler_id = to_intel_plane_state(drm_plane->state)->scaler_id;
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 <chandra.konduru@intel.com> 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(-)