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