| Message ID | 20180216043322.22874-1-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Dhinakaran Pandiyan (2018-02-16 04:33:18) > i915_gem_obj_pin_to_display() calls frontbuffer_flush with origin set to > DIRTYFB. The callers however are at a vantage point to decide if hardware > frontbuffer tracking can do the flush for us. For example, legacy cursor > updates, like flips, write to MMIO registers, which then triggers PSR flush > by the hardware. Moving frontbuffer_flush out will enable us to skip a > software initiated flush by setting origin to FLIP. Thanks to Chris for the > idea. > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 9 ++++----- > drivers/gpu/drm/i915/intel_display.c | 13 ++++++++++--- > drivers/gpu/drm/i915/intel_fbdev.c | 6 +++--- > drivers/gpu/drm/i915/intel_overlay.c | 1 + > 4 files changed, 18 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index fc68b35854df..22b598e1e0b9 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -4071,9 +4071,10 @@ int i915_gem_set_caching_ioctl(struct drm_device *dev, void *data, > } > > /* > - * Prepare buffer for display plane (scanout, cursors, etc). > - * Can be called from an uninterruptible phase (modesetting) and allows > - * any flushes to be pipelined (for pageflips). > + * Prepare buffer for display plane (scanout, cursors, etc). Can be called from > + * an uninterruptible phase (modesetting) and allows any flushes to be pipelined > + * (for pageflips). We only flush the caches while preparing the buffer for > + * display, the callers are responsible for frontbuffer flush. > */ > struct i915_vma * > i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj, > @@ -4139,9 +4140,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj, > > vma->display_alignment = max_t(u64, vma->display_alignment, alignment); > > - /* Treat this as an end-of-frame, like intel_user_framebuffer_dirty() */ > __i915_gem_object_flush_for_display(obj); > - intel_fb_obj_flush(obj, ORIGIN_DIRTYFB); > > /* It should now be out of any other write domains, and we can update > * the domain values for our changes. > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 286a9591d179..2b70714ead0f 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2806,10 +2806,15 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc, > return; > > valid_fb: > + obj = intel_fb_obj(fb); > mutex_lock(&dev->struct_mutex); > intel_state->vma = > intel_pin_and_fence_fb_obj(fb, primary->state->rotation); > + > + if (!IS_ERR(intel_state->vma)) > + intel_fb_obj_flush(obj, ORIGIN_DIRTYFB); The fb_obj_flush doesn't need to be inside the lock, so you can do it after the normal error testing (or just do it since the flush doesn't depend on having the vma bound). > mutex_unlock(&dev->struct_mutex); > + > if (IS_ERR(intel_state->vma)) { > DRM_ERROR("failed to pin boot fb on pipe %d: %li\n", > intel_crtc->pipe, PTR_ERR(intel_state->vma)); > @@ -12713,10 +12717,12 @@ intel_prepare_plane_fb(struct drm_plane *plane, > struct i915_vma *vma; > > vma = intel_pin_and_fence_fb_obj(fb, new_state->rotation); > - if (!IS_ERR(vma)) > + if (!IS_ERR(vma)) { > to_intel_plane_state(new_state)->vma = vma; > - else > + intel_fb_obj_flush(obj, ORIGIN_DIRTYFB); > + } else { > ret = PTR_ERR(vma); Would if (IS_ERR(vma)) { ret = PTR_ERR(vma); break; } intel_fb_obj_flush(obj, ORIGIN_DIRTYFB); to_intel_plane_state(new_state)->vma = vma; look neater here? Lgtm, -Chris
On Fri, 2018-02-16 at 08:52 +0000, Chris Wilson wrote: > Quoting Dhinakaran Pandiyan (2018-02-16 04:33:18) > > i915_gem_obj_pin_to_display() calls frontbuffer_flush with origin set to > > DIRTYFB. The callers however are at a vantage point to decide if hardware > > frontbuffer tracking can do the flush for us. For example, legacy cursor > > updates, like flips, write to MMIO registers, which then triggers PSR flush > > by the hardware. Moving frontbuffer_flush out will enable us to skip a > > software initiated flush by setting origin to FLIP. Thanks to Chris for the > > idea. > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > --- > > drivers/gpu/drm/i915/i915_gem.c | 9 ++++----- > > drivers/gpu/drm/i915/intel_display.c | 13 ++++++++++--- > > drivers/gpu/drm/i915/intel_fbdev.c | 6 +++--- > > drivers/gpu/drm/i915/intel_overlay.c | 1 + > > 4 files changed, 18 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > index fc68b35854df..22b598e1e0b9 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -4071,9 +4071,10 @@ int i915_gem_set_caching_ioctl(struct drm_device *dev, void *data, > > } > > > > /* > > - * Prepare buffer for display plane (scanout, cursors, etc). > > - * Can be called from an uninterruptible phase (modesetting) and allows > > - * any flushes to be pipelined (for pageflips). > > + * Prepare buffer for display plane (scanout, cursors, etc). Can be called from > > + * an uninterruptible phase (modesetting) and allows any flushes to be pipelined > > + * (for pageflips). We only flush the caches while preparing the buffer for > > + * display, the callers are responsible for frontbuffer flush. > > */ > > struct i915_vma * > > i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj, > > @@ -4139,9 +4140,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj, > > > > vma->display_alignment = max_t(u64, vma->display_alignment, alignment); > > > > - /* Treat this as an end-of-frame, like intel_user_framebuffer_dirty() */ > > __i915_gem_object_flush_for_display(obj); > > - intel_fb_obj_flush(obj, ORIGIN_DIRTYFB); > > > > /* It should now be out of any other write domains, and we can update > > * the domain values for our changes. > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 286a9591d179..2b70714ead0f 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -2806,10 +2806,15 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc, > > return; > > > > valid_fb: > > + obj = intel_fb_obj(fb); > > mutex_lock(&dev->struct_mutex); > > intel_state->vma = > > intel_pin_and_fence_fb_obj(fb, primary->state->rotation); > > + > > + if (!IS_ERR(intel_state->vma)) > > + intel_fb_obj_flush(obj, ORIGIN_DIRTYFB); > > The fb_obj_flush doesn't need to be inside the lock, so you can do it > after the normal error testing (or just do it since the flush doesn't > depend on having the vma bound). Okay, that answers the question I meant to ask. Decided to err on the side of caution. > > > mutex_unlock(&dev->struct_mutex); > > + > > if (IS_ERR(intel_state->vma)) { > > DRM_ERROR("failed to pin boot fb on pipe %d: %li\n", > > intel_crtc->pipe, PTR_ERR(intel_state->vma)); > > @@ -12713,10 +12717,12 @@ intel_prepare_plane_fb(struct drm_plane *plane, > > struct i915_vma *vma; > > > > vma = intel_pin_and_fence_fb_obj(fb, new_state->rotation); > > - if (!IS_ERR(vma)) > > + if (!IS_ERR(vma)) { > > to_intel_plane_state(new_state)->vma = vma; > > - else > > + intel_fb_obj_flush(obj, ORIGIN_DIRTYFB); > > + } else { > > ret = PTR_ERR(vma); > > Would > if (IS_ERR(vma)) { > ret = PTR_ERR(vma); > break; There's no loop around this, but this change goes away in [Patch 4/5]. > } > > intel_fb_obj_flush(obj, ORIGIN_DIRTYFB); > to_intel_plane_state(new_state)->vma = vma; > > look neater here? > > Lgtm, > -Chris > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index fc68b35854df..22b598e1e0b9 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4071,9 +4071,10 @@ int i915_gem_set_caching_ioctl(struct drm_device *dev, void *data, } /* - * Prepare buffer for display plane (scanout, cursors, etc). - * Can be called from an uninterruptible phase (modesetting) and allows - * any flushes to be pipelined (for pageflips). + * Prepare buffer for display plane (scanout, cursors, etc). Can be called from + * an uninterruptible phase (modesetting) and allows any flushes to be pipelined + * (for pageflips). We only flush the caches while preparing the buffer for + * display, the callers are responsible for frontbuffer flush. */ struct i915_vma * i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj, @@ -4139,9 +4140,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj, vma->display_alignment = max_t(u64, vma->display_alignment, alignment); - /* Treat this as an end-of-frame, like intel_user_framebuffer_dirty() */ __i915_gem_object_flush_for_display(obj); - intel_fb_obj_flush(obj, ORIGIN_DIRTYFB); /* It should now be out of any other write domains, and we can update * the domain values for our changes. diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 286a9591d179..2b70714ead0f 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2806,10 +2806,15 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc, return; valid_fb: + obj = intel_fb_obj(fb); mutex_lock(&dev->struct_mutex); intel_state->vma = intel_pin_and_fence_fb_obj(fb, primary->state->rotation); + + if (!IS_ERR(intel_state->vma)) + intel_fb_obj_flush(obj, ORIGIN_DIRTYFB); mutex_unlock(&dev->struct_mutex); + if (IS_ERR(intel_state->vma)) { DRM_ERROR("failed to pin boot fb on pipe %d: %li\n", intel_crtc->pipe, PTR_ERR(intel_state->vma)); @@ -2832,7 +2837,6 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc, intel_state->base.src = drm_plane_state_src(plane_state); intel_state->base.dst = drm_plane_state_dest(plane_state); - obj = intel_fb_obj(fb); if (i915_gem_object_is_tiled(obj)) dev_priv->preserve_bios_swizzle = true; @@ -12713,10 +12717,12 @@ intel_prepare_plane_fb(struct drm_plane *plane, struct i915_vma *vma; vma = intel_pin_and_fence_fb_obj(fb, new_state->rotation); - if (!IS_ERR(vma)) + if (!IS_ERR(vma)) { to_intel_plane_state(new_state)->vma = vma; - else + intel_fb_obj_flush(obj, ORIGIN_DIRTYFB); + } else { ret = PTR_ERR(vma); + } } i915_gem_object_wait_priority(obj, 0, I915_PRIORITY_DISPLAY); @@ -13136,6 +13142,7 @@ intel_legacy_cursor_update(struct drm_plane *plane, goto out_unlock; } + intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_DIRTYFB); to_intel_plane_state(new_plane_state)->vma = vma; } diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c index da48af11eb6b..6104d0eadbbc 100644 --- a/drivers/gpu/drm/i915/intel_fbdev.c +++ b/drivers/gpu/drm/i915/intel_fbdev.c @@ -204,6 +204,7 @@ static int intelfb_create(struct drm_fb_helper *helper, sizes->fb_height = intel_fb->base.height; } + fb = &ifbdev->fb->base; mutex_lock(&dev->struct_mutex); intel_runtime_pm_get(dev_priv); @@ -211,11 +212,12 @@ static int intelfb_create(struct drm_fb_helper *helper, * This also validates that any existing fb inherited from the * BIOS is suitable for own access. */ - vma = intel_pin_and_fence_fb_obj(&ifbdev->fb->base, DRM_MODE_ROTATE_0); + vma = intel_pin_and_fence_fb_obj(fb, DRM_MODE_ROTATE_0); if (IS_ERR(vma)) { ret = PTR_ERR(vma); goto out_unlock; } + intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_DIRTYFB); info = drm_fb_helper_alloc_fbi(helper); if (IS_ERR(info)) { @@ -226,8 +228,6 @@ static int intelfb_create(struct drm_fb_helper *helper, info->par = helper; - fb = &ifbdev->fb->base; - ifbdev->helper.fb = fb; strcpy(info->fix.id, "inteldrmfb"); diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c index 41e9465d44a8..16bfb68f8e42 100644 --- a/drivers/gpu/drm/i915/intel_overlay.c +++ b/drivers/gpu/drm/i915/intel_overlay.c @@ -806,6 +806,7 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay, ret = PTR_ERR(vma); goto out_pin_section; } + intel_fb_obj_flush(new_bo, ORIGIN_DIRTYFB); ret = i915_vma_put_fence(vma); if (ret)
i915_gem_obj_pin_to_display() calls frontbuffer_flush with origin set to DIRTYFB. The callers however are at a vantage point to decide if hardware frontbuffer tracking can do the flush for us. For example, legacy cursor updates, like flips, write to MMIO registers, which then triggers PSR flush by the hardware. Moving frontbuffer_flush out will enable us to skip a software initiated flush by setting origin to FLIP. Thanks to Chris for the idea. Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> --- drivers/gpu/drm/i915/i915_gem.c | 9 ++++----- drivers/gpu/drm/i915/intel_display.c | 13 ++++++++++--- drivers/gpu/drm/i915/intel_fbdev.c | 6 +++--- drivers/gpu/drm/i915/intel_overlay.c | 1 + 4 files changed, 18 insertions(+), 11 deletions(-)