Re: [PATCH 74/89] drm/i915/skl: Implement queue_flip

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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)
>
> Signed-off-by: Damien Lespiau <damien.lespiau@xxxxxxxxx>
> ---
>  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.


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


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


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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux