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@xxxxxxxxx>
> > 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@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
