| Message ID | 1422639418-28845-5-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jan 30, 2015 at 05:36:56PM +0000, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > 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@intel.com> > --- > 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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 02/02/2015 09:49 AM, Daniel Vetter wrote: > On Fri, Jan 30, 2015 at 05:36:56PM +0000, Tvrtko Ursulin wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> 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@intel.com> >> --- >> 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. What do you mean by "flip over"? To make places which need to get fb tiling format use fb->modifier[0] directly rather than have them mapped to tiling enums at fb init? >> @@ -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. Ok I'll pull it out in a separate patch. >> 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. If I understood correctly what you meant in the first quote then yes. Regards, Tvrtko
On Mon, Feb 02, 2015 at 10:29:19AM +0000, Tvrtko Ursulin wrote: > > On 02/02/2015 09:49 AM, Daniel Vetter wrote: > >On Fri, Jan 30, 2015 at 05:36:56PM +0000, Tvrtko Ursulin wrote: > >>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >> > >>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@intel.com> > >>--- > >> 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. > > What do you mean by "flip over"? > > To make places which need to get fb tiling format use fb->modifier[0] > directly rather than have them mapped to tiling enums at fb init? Yes. That way we can drop intel_fb->tiling_mode and have one less somewhat redundant thing to keep in sync with everything else. Hoping that everyone just uses the same ime just doesn't work ;-) > >>@@ -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. > > Ok I'll pull it out in a separate patch. > > >> 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. > > If I understood correctly what you meant in the first quote then yes. You've already done it, it's the next patch - I just read patch series sequentially ;-) Patch needs a bit of adjustment ofc due to the removal of intel_fb->tiling_mode, and needs to be before this one here so that fb->modifier[0] is guaranteed to be valid also for legacy userspace. -Daniel
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: 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; 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; 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;