On Tue, Sep 23, 2014 at 05:06:35PM -0300, Paulo Zanoni wrote:
> 2014-09-04 8:27 GMT-03:00 Damien Lespiau <damien.lespiau@xxxxxxxxx>:
> > A few bits have changed in MI_DISPLAY_FLIP to accomodate the new planes.
> > DE_RRMR seems to have kept its plane flip bits backward compatible.
> >
> > v2: Rebase on top of nightly
> > v2: Rebase on top of nightly (minor conflict in i915_reg.h)
v3
> > Signed-off-by: Damien Lespiau <damien.lespiau@xxxxxxxxx>
Some answers below, others in v4 that should follow shortly.
> > ---
> > drivers/gpu/drm/i915/i915_reg.h | 10 +++++
> > drivers/gpu/drm/i915/intel_display.c | 78 ++++++++++++++++++++++++++++++++++++
> > 2 files changed, 88 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 84a0de6..176e84e 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -240,6 +240,16 @@
> > #define MI_DISPLAY_FLIP_IVB_SPRITE_B (3 << 19)
> > #define MI_DISPLAY_FLIP_IVB_PLANE_C (4 << 19)
> > #define MI_DISPLAY_FLIP_IVB_SPRITE_C (5 << 19)
> > +/* SKL ones */
> > +#define MI_DISPLAY_FLIP_SKL_PLANE_1_A (0 << 8)
> > +#define MI_DISPLAY_FLIP_SKL_PLANE_1_B (1 << 8)
> > +#define MI_DISPLAY_FLIP_SKL_PLANE_1_C (2 << 8)
> > +#define MI_DISPLAY_FLIP_SKL_PLANE_2_A (4 << 8)
> > +#define MI_DISPLAY_FLIP_SKL_PLANE_2_B (5 << 8)
> > +#define MI_DISPLAY_FLIP_SKL_PLANE_2_C (6 << 8)
> > +#define MI_DISPLAY_FLIP_SKL_PLANE_3_A (7 << 8)
> > +#define MI_DISPLAY_FLIP_SKL_PLANE_3_B (8 << 8)
> > +#define MI_DISPLAY_FLIP_SKL_PLANE_3_C (9 << 8)
>
> BSpec seems to mention these bits on CHV too... Maybe we want the new
> function to run on CHV + GEN9? Ping Ville.
Funnily the bits at 21:19 are still documented on CHV. So maybe the
current code works? Considering we only use queue_flip() for the primary
planes at the moment ad we want to move to MMIO based flips in the near
future, probably doesn't matter too much here.
> > #define MI_SEMAPHORE_MBOX MI_INSTR(0x16, 1) /* gen6, gen7 */
> > #define MI_SEMAPHORE_GLOBAL_GTT (1<<22)
> > #define MI_SEMAPHORE_UPDATE (1<<21)
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index abd4201..393bd19 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -9913,6 +9913,81 @@ static int intel_queue_mmio_flip(struct drm_device *dev,
> > return 0;
> > }
> >
> > +static int intel_gen9_queue_flip(struct drm_device *dev,
> > + struct drm_crtc *crtc,
> > + struct drm_framebuffer *fb,
> > + struct drm_i915_gem_object *obj,
> > + struct intel_engine_cs *ring,
> > + uint32_t flags)
> > +{
> > + struct drm_i915_private *dev_priv = dev->dev_private;
> > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > + uint32_t plane = 0, stride;
> > + int ret;
> > +
> > + ring = obj->ring;
> > + if (ring == NULL || ring->id != BCS)
> > + ring = &dev_priv->ring[RCS];
>
> Why do we need these lines above? The other Gens don't have it. And it
> looks like ring shouldn't really be NULL, otherwise the other gens are
> going to crash.
>
>
> > +
> > + ret = intel_pin_and_fence_fb_obj(dev, obj, ring);
> > + if (ret)
> > + goto err;
>
> Our only caller (intel_crtc_page_flip) seems to do this for us
> already. Also, the other gens don't do this at their queue_flip
> implementations.
If you're looking for reasons, it's because the code elsewhere used to
look like that and rebasing still worked, so this patch implements
queue_flip() like it used to be implemented at the time of its writing.
Should be fixed now.
> > +
> > + switch(intel_crtc->pipe) {
> > + case PIPE_A:
> > + plane = MI_DISPLAY_FLIP_SKL_PLANE_1_A;
> > + break;
> > + case PIPE_B:
> > + plane = MI_DISPLAY_FLIP_SKL_PLANE_1_B;
> > + break;
> > + case PIPE_C:
> > + plane = MI_DISPLAY_FLIP_SKL_PLANE_1_C;
> > + break;
> > + default:
> > + BUG();
>
> The gen7 version does WARN_ONCE() and returns -ENODEV instead of
> BUG(). Seems more reasonable to just not update the screen instead of
> killing the whole machine.
>
>
> > + }
> > +
> > + switch (obj->tiling_mode) {
> > + case I915_TILING_NONE:
> > + stride = fb->pitches[0] >> 6;
> > + break;
> > + case I915_TILING_X:
> > + stride = fb->pitches[0] >> 9;
> > + break;
> > + default:
> > + BUG();
>
> Also replace this for a WARN and return an error code?
>
> > + }
> > +
> > + ret = intel_ring_begin(ring, 10);
> > + if (ret)
> > + goto err_unpin;
> > +
> > + intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> > + intel_ring_emit(ring, DERRMR);
> > + intel_ring_emit(ring, ~(DERRMR_PIPEA_PRI_FLIP_DONE |
> > + DERRMR_PIPEB_PRI_FLIP_DONE |
> > + DERRMR_PIPEC_PRI_FLIP_DONE));
> > + intel_ring_emit(ring, MI_STORE_REGISTER_MEM_GEN8(1) |
> > + MI_SRM_LRM_GLOBAL_GTT);
> > + intel_ring_emit(ring, DERRMR);
> > + intel_ring_emit(ring, ring->scratch.gtt_offset + 256);
> > + intel_ring_emit(ring, 0);
>
> Do we still need the DERRMR thing? BSpec doesn't seem to mention it
> anymore on the SKL page. And we only do it for RCS on Gens 7/8.
The answer we had from the HW guys (Art) for that is that the need to
unmask DE_RRMR depends on the gen. Turns out, on gen9, we need to unmask
those bits for the CS to receive the message as some internal state
depends on receiving it. Without it, the flips weren't working.
> > +
> > + intel_ring_emit(ring, MI_DISPLAY_FLIP_I915 | plane);
> > + intel_ring_emit(ring, stride << 6 | obj->tiling_mode);
> > + intel_ring_emit(ring, i915_gem_obj_ggtt_offset(obj));
>
> Gen 7 uses intel_crtc->unpin_work->gtt_offset, which seems to be a
> little faster than calling i915_gem_obj_ggtt_offset() again. The
> work->gtt_offset was just set by the function that calls queue_flip(),
> so it should be correct.
>
>
> > +
> > + intel_mark_page_flip_active(intel_crtc);
> > + __intel_ring_advance(ring);
> > +
> > + return 0;
> > +
> > +err_unpin:
> > + intel_unpin_fb_obj(obj);
> > +err:
> > + return ret;
> > +}
> > +
> > static int intel_default_queue_flip(struct drm_device *dev,
> > struct drm_crtc *crtc,
> > struct drm_framebuffer *fb,
> > @@ -12740,6 +12815,9 @@ static void intel_init_display(struct drm_device *dev)
> > case 8: /* FIXME(BDW): Check that the gen8 RCS flip works. */
> > dev_priv->display.queue_flip = intel_gen7_queue_flip;
> > break;
> > + case 9:
> > + dev_priv->display.queue_flip = intel_gen9_queue_flip;
> > + break;
> > }
> >
> > intel_panel_init_backlight_funcs(dev);
> > --
> > 1.8.3.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> --
> Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
