On Fri, Jan 30, 2015 at 05:36:56PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
>
> To prepare for framebuffer modifiers, move tiling definition from the
> object into the framebuffer. Move in a way that framebuffer tiling is
> now used for display while object tiling remains for fencing.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_display.c | 46 +++++++++++++++++++++---------------
> drivers/gpu/drm/i915/intel_drv.h | 2 ++
> drivers/gpu/drm/i915/intel_pm.c | 7 +++---
> drivers/gpu/drm/i915/intel_sprite.c | 26 ++++++++++----------
> 4 files changed, 46 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4425e86..e22afbe 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2211,7 +2211,7 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>
> WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>
> - switch (obj->tiling_mode) {
> + switch (to_intel_framebuffer(fb)->tiling_mode) {
> case I915_TILING_NONE:
Imo we should just look at fb->modifier[0] and flip over all the enums. A
bit more invasive, but we also don't need to change all the platform code
at once - set_tiling already guarantees that no one can modify the bo
tiling mode when there's an fb object using it. Which means we can change
the code over at leasure.
> if (INTEL_INFO(dev)->gen >= 9)
> alignment = 256 * 1024;
> @@ -2474,6 +2474,7 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
> u32 dspcntr;
> u32 reg = DSPCNTR(plane);
> int pixel_size;
> + unsigned int tiling_mode = to_intel_framebuffer(fb)->tiling_mode;
>
> if (!intel_crtc->primary_enabled) {
> I915_WRITE(reg, 0);
> @@ -2545,8 +2546,7 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
> BUG();
> }
>
> - if (INTEL_INFO(dev)->gen >= 4 &&
> - obj->tiling_mode != I915_TILING_NONE)
> + if (INTEL_INFO(dev)->gen >= 4 && tiling_mode != I915_TILING_NONE)
> dspcntr |= DISPPLANE_TILED;
>
> if (IS_G4X(dev))
> @@ -2556,7 +2556,8 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
>
> if (INTEL_INFO(dev)->gen >= 4) {
> intel_crtc->dspaddr_offset =
> - intel_gen4_compute_page_offset(&x, &y, obj->tiling_mode,
> + intel_gen4_compute_page_offset(&x, &y,
> + tiling_mode,
> pixel_size,
> fb->pitches[0]);
> linear_offset -= intel_crtc->dspaddr_offset;
> @@ -2606,6 +2607,7 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
> u32 dspcntr;
> u32 reg = DSPCNTR(plane);
> int pixel_size;
> + unsigned int tiling_mode = to_intel_framebuffer(fb)->tiling_mode;
>
> if (!intel_crtc->primary_enabled) {
> I915_WRITE(reg, 0);
> @@ -2654,7 +2656,7 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
> BUG();
> }
>
> - if (obj->tiling_mode != I915_TILING_NONE)
> + if (tiling_mode != I915_TILING_NONE)
> dspcntr |= DISPPLANE_TILED;
>
> if (!IS_HASWELL(dev) && !IS_BROADWELL(dev))
> @@ -2662,7 +2664,8 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
>
> linear_offset = y * fb->pitches[0] + x * pixel_size;
> intel_crtc->dspaddr_offset =
> - intel_gen4_compute_page_offset(&x, &y, obj->tiling_mode,
> + intel_gen4_compute_page_offset(&x, &y,
> + tiling_mode,
> pixel_size,
> fb->pitches[0]);
> linear_offset -= intel_crtc->dspaddr_offset;
> @@ -2750,7 +2753,7 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
> * The stride is either expressed as a multiple of 64 bytes chunks for
> * linear buffers or in number of tiles for tiled buffers.
> */
> - switch (obj->tiling_mode) {
> + switch (to_intel_framebuffer(fb)->tiling_mode) {
> case I915_TILING_NONE:
> stride = fb->pitches[0] >> 6;
> break;
> @@ -9291,7 +9294,7 @@ static int intel_gen4_queue_flip(struct drm_device *dev,
> MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
> intel_ring_emit(ring, fb->pitches[0]);
> intel_ring_emit(ring, intel_crtc->unpin_work->gtt_offset |
> - obj->tiling_mode);
> + to_intel_framebuffer(fb)->tiling_mode);
>
> /* XXX Enabling the panel-fitter across page-flip is so far
> * untested on non-native modes, so ignore it for now.
> @@ -9324,7 +9327,8 @@ static int intel_gen6_queue_flip(struct drm_device *dev,
>
> intel_ring_emit(ring, MI_DISPLAY_FLIP |
> MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
> - intel_ring_emit(ring, fb->pitches[0] | obj->tiling_mode);
> + intel_ring_emit(ring,
> + fb->pitches[0] | to_intel_framebuffer(fb)->tiling_mode);
> intel_ring_emit(ring, intel_crtc->unpin_work->gtt_offset);
>
> /* Contrary to the suggestions in the documentation,
> @@ -9428,7 +9432,8 @@ static int intel_gen7_queue_flip(struct drm_device *dev,
> }
>
> intel_ring_emit(ring, MI_DISPLAY_FLIP_I915 | plane_bit);
> - intel_ring_emit(ring, (fb->pitches[0] | obj->tiling_mode));
> + intel_ring_emit(ring,
> + fb->pitches[0] | to_intel_framebuffer(fb)->tiling_mode);
> intel_ring_emit(ring, intel_crtc->unpin_work->gtt_offset);
> intel_ring_emit(ring, (MI_NOOP));
>
> @@ -9470,13 +9475,12 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc)
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct drm_framebuffer *fb = intel_crtc->base.primary->fb;
> struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> - struct drm_i915_gem_object *obj = intel_fb->obj;
> const enum pipe pipe = intel_crtc->pipe;
> u32 ctl, stride;
>
> ctl = I915_READ(PLANE_CTL(pipe, 0));
> ctl &= ~PLANE_CTL_TILED_MASK;
> - if (obj->tiling_mode == I915_TILING_X)
> + if (intel_fb->tiling_mode == I915_TILING_X)
> ctl |= PLANE_CTL_TILED_X;
>
> /*
> @@ -9484,7 +9488,7 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc)
> * linear buffers or in number of tiles for tiled buffers.
> */
> stride = fb->pitches[0] >> 6;
> - if (obj->tiling_mode == I915_TILING_X)
> + if (intel_fb->tiling_mode == I915_TILING_X)
> stride = fb->pitches[0] >> 9; /* X tiles are 512 bytes wide */
>
> /*
> @@ -9504,14 +9508,13 @@ static void ilk_do_mmio_flip(struct intel_crtc *intel_crtc)
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_framebuffer *intel_fb =
> to_intel_framebuffer(intel_crtc->base.primary->fb);
> - struct drm_i915_gem_object *obj = intel_fb->obj;
> u32 dspcntr;
> u32 reg;
>
> reg = DSPCNTR(intel_crtc->plane);
> dspcntr = I915_READ(reg);
>
> - if (obj->tiling_mode != I915_TILING_NONE)
> + if (intel_fb->tiling_mode != I915_TILING_NONE)
> dspcntr |= DISPPLANE_TILED;
> else
> dspcntr &= ~DISPPLANE_TILED;
> @@ -9595,6 +9598,7 @@ static int intel_gen9_queue_flip(struct drm_device *dev,
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> uint32_t plane = 0, stride;
> int ret;
> + unsigned int tiling_mode = to_intel_framebuffer(fb)->tiling_mode;
>
> switch(intel_crtc->pipe) {
> case PIPE_A:
> @@ -9611,7 +9615,7 @@ static int intel_gen9_queue_flip(struct drm_device *dev,
> return -ENODEV;
> }
>
> - switch (obj->tiling_mode) {
> + switch (tiling_mode) {
> case I915_TILING_NONE:
> stride = fb->pitches[0] >> 6;
> break;
> @@ -9639,7 +9643,7 @@ static int intel_gen9_queue_flip(struct drm_device *dev,
> intel_ring_emit(ring, 0);
>
> intel_ring_emit(ring, MI_DISPLAY_FLIP_I915 | plane);
> - intel_ring_emit(ring, stride << 6 | obj->tiling_mode);
> + intel_ring_emit(ring, stride << 6 | tiling_mode);
> intel_ring_emit(ring, intel_crtc->unpin_work->gtt_offset);
>
> intel_mark_page_flip_active(intel_crtc);
> @@ -9764,6 +9768,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
> work->event = event;
> work->crtc = crtc;
> work->old_fb_obj = intel_fb_obj(old_fb);
> + work->old_tiling_mode = to_intel_framebuffer(old_fb)->tiling_mode;
Hm, that's actually an interesting bugfix - currently userspace could be
sneaky and destroy the old fb immediately after the flip completes and the
change the tiling of the underlying object before the unpin work had a
chance to run (needs some fudgin with rt prios to starve workers to make
this work though).
Imo the right fix is to hold a reference onto the fb and not the
underlying gem object. With that tiling is guaranteed not to change.
> INIT_WORK(&work->work, intel_unpin_work_fn);
>
> ret = drm_crtc_vblank_get(crtc);
> @@ -9814,7 +9819,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>
> if (IS_VALLEYVIEW(dev)) {
> ring = &dev_priv->ring[BCS];
> - if (obj->tiling_mode != work->old_fb_obj->tiling_mode)
> + if (to_intel_framebuffer(fb)->tiling_mode !=
> + work->old_tiling_mode)
> /* vlv: DISPLAY_FLIP fails to change tiling */
> ring = NULL;
> } else if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev)) {
> @@ -12190,7 +12196,8 @@ intel_check_cursor_plane(struct drm_plane *plane,
>
> /* we only need to pin inside GTT if cursor is non-phy */
> mutex_lock(&dev->struct_mutex);
> - if (!INTEL_INFO(dev)->cursor_needs_physical && obj->tiling_mode) {
> + if (!INTEL_INFO(dev)->cursor_needs_physical &&
> + to_intel_framebuffer(fb)->tiling_mode) {
> DRM_DEBUG_KMS("cursor cannot be tiled\n");
> ret = -EINVAL;
> }
> @@ -12772,6 +12779,7 @@ static int intel_framebuffer_init(struct drm_device *dev,
> drm_helper_mode_fill_fb_struct(&intel_fb->base, mode_cmd);
> intel_fb->obj = obj;
> intel_fb->obj->framebuffer_references++;
> + intel_fb->tiling_mode = obj->tiling_mode;
One side-effect of using fb->modifier[0] is that if the modifier flag is
_not_ set, we need to reconstruct this field from obj->tiling_mode here.
Otoh if it is set this code here should check that fb->modifier and
obj->tiling_mode are consistent.
Perhaps best to split this change out as a prep patch, like you've done
with the code for the initial framebuffer.
>
> ret = drm_framebuffer_init(dev, &intel_fb->base, &intel_fb_funcs);
> if (ret) {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index eef79cc..b8d8a1d 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -120,6 +120,7 @@ enum intel_output_type {
> struct intel_framebuffer {
> struct drm_framebuffer base;
> struct drm_i915_gem_object *obj;
> + unsigned int tiling_mode;
> };
>
> struct intel_fbdev {
> @@ -723,6 +724,7 @@ struct intel_unpin_work {
> int flip_queued_vblank;
> int flip_ready_vblank;
> bool enable_stall_check;
> + unsigned int old_tiling_mode;
> };
>
> struct intel_set_config {
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 6ece663..0ca4088 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -1182,12 +1182,11 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
> DRM_DEBUG_KMS("FIFO watermarks - A: %d, B: %d\n", planea_wm, planeb_wm);
>
> if (IS_I915GM(dev) && enabled) {
> - struct drm_i915_gem_object *obj;
> -
> - obj = intel_fb_obj(enabled->primary->fb);
> + struct intel_framebuffer *intel_fb;
>
> /* self-refresh seems busted with untiled */
> - if (obj->tiling_mode == I915_TILING_NONE)
> + intel_fb = to_intel_framebuffer(enabled->primary->fb);
> + if (intel_fb->tiling_mode == I915_TILING_NONE)
> enabled = NULL;
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 0a52c44..0659802 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -245,7 +245,7 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
> BUG();
> }
>
> - switch (obj->tiling_mode) {
> + switch (to_intel_framebuffer(fb)->tiling_mode) {
> case I915_TILING_NONE:
> stride = fb->pitches[0] >> 6;
> break;
> @@ -413,6 +413,7 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
> u32 sprctl;
> unsigned long sprsurf_offset, linear_offset;
> int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> + unsigned int tiling_mode = to_intel_framebuffer(fb)->tiling_mode;
>
> sprctl = I915_READ(SPCNTR(pipe, plane));
>
> @@ -471,7 +472,7 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
> */
> sprctl |= SP_GAMMA_ENABLE;
>
> - if (obj->tiling_mode != I915_TILING_NONE)
> + if (tiling_mode != I915_TILING_NONE)
> sprctl |= SP_TILED;
>
> sprctl |= SP_ENABLE;
> @@ -488,7 +489,7 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>
> linear_offset = y * fb->pitches[0] + x * pixel_size;
> sprsurf_offset = intel_gen4_compute_page_offset(&x, &y,
> - obj->tiling_mode,
> + tiling_mode,
> pixel_size,
> fb->pitches[0]);
> linear_offset -= sprsurf_offset;
> @@ -509,7 +510,7 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
> I915_WRITE(SPSTRIDE(pipe, plane), fb->pitches[0]);
> I915_WRITE(SPPOS(pipe, plane), (crtc_y << 16) | crtc_x);
>
> - if (obj->tiling_mode != I915_TILING_NONE)
> + if (tiling_mode != I915_TILING_NONE)
> I915_WRITE(SPTILEOFF(pipe, plane), (y << 16) | x);
> else
> I915_WRITE(SPLINOFF(pipe, plane), linear_offset);
> @@ -613,6 +614,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> u32 sprctl, sprscale = 0;
> unsigned long sprsurf_offset, linear_offset;
> int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> + unsigned int tiling_mode = to_intel_framebuffer(fb)->tiling_mode;
>
> sprctl = I915_READ(SPRCTL(pipe));
>
> @@ -652,7 +654,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> */
> sprctl |= SPRITE_GAMMA_ENABLE;
>
> - if (obj->tiling_mode != I915_TILING_NONE)
> + if (tiling_mode != I915_TILING_NONE)
> sprctl |= SPRITE_TILED;
>
> if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> @@ -680,7 +682,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>
> linear_offset = y * fb->pitches[0] + x * pixel_size;
> sprsurf_offset =
> - intel_gen4_compute_page_offset(&x, &y, obj->tiling_mode,
> + intel_gen4_compute_page_offset(&x, &y, tiling_mode,
> pixel_size, fb->pitches[0]);
> linear_offset -= sprsurf_offset;
>
> @@ -705,7 +707,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> * register */
> if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> I915_WRITE(SPROFFSET(pipe), (y << 16) | x);
> - else if (obj->tiling_mode != I915_TILING_NONE)
> + else if (tiling_mode != I915_TILING_NONE)
> I915_WRITE(SPRTILEOFF(pipe), (y << 16) | x);
> else
> I915_WRITE(SPRLINOFF(pipe), linear_offset);
> @@ -818,6 +820,7 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> unsigned long dvssurf_offset, linear_offset;
> u32 dvscntr, dvsscale;
> int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> + unsigned int tiling_mode = to_intel_framebuffer(fb)->tiling_mode;
>
> dvscntr = I915_READ(DVSCNTR(pipe));
>
> @@ -857,7 +860,7 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> */
> dvscntr |= DVS_GAMMA_ENABLE;
>
> - if (obj->tiling_mode != I915_TILING_NONE)
> + if (tiling_mode != I915_TILING_NONE)
> dvscntr |= DVS_TILED;
>
> if (IS_GEN6(dev))
> @@ -880,7 +883,7 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>
> linear_offset = y * fb->pitches[0] + x * pixel_size;
> dvssurf_offset =
> - intel_gen4_compute_page_offset(&x, &y, obj->tiling_mode,
> + intel_gen4_compute_page_offset(&x, &y, tiling_mode,
> pixel_size, fb->pitches[0]);
> linear_offset -= dvssurf_offset;
>
> @@ -897,7 +900,7 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> I915_WRITE(DVSSTRIDE(pipe), fb->pitches[0]);
> I915_WRITE(DVSPOS(pipe), (crtc_y << 16) | crtc_x);
>
> - if (obj->tiling_mode != I915_TILING_NONE)
> + if (tiling_mode != I915_TILING_NONE)
> I915_WRITE(DVSTILEOFF(pipe), (y << 16) | x);
> else
> I915_WRITE(DVSLINOFF(pipe), linear_offset);
> @@ -1076,7 +1079,6 @@ intel_check_sprite_plane(struct drm_plane *plane,
> struct intel_crtc *intel_crtc = to_intel_crtc(state->base.crtc);
> struct intel_plane *intel_plane = to_intel_plane(plane);
> struct drm_framebuffer *fb = state->base.fb;
> - struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> int crtc_x, crtc_y;
> unsigned int crtc_w, crtc_h;
> uint32_t src_x, src_y, src_w, src_h;
> @@ -1107,7 +1109,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
> }
>
> /* Sprite planes can be linear or x-tiled surfaces */
> - switch (obj->tiling_mode) {
> + switch (to_intel_framebuffer(fb)->tiling_mode) {
> case I915_TILING_NONE:
> case I915_TILING_X:
> break;
> --
> 2.2.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
