From: Daniel Vetter <daniel@ffwll.ch>
To: Paulo Zanoni <przanoni@gmail.com>
Cc: intel-gfx@lists.freedesktop.org, Paulo Zanoni <paulo.r.zanoni@intel.com>
Subject: Re: [PATCH 4/9] drm/i915: pass which operation triggered the frontbuffer tracking
Date: Mon, 9 Feb 2015 19:29:28 +0100 [thread overview]
Message-ID: <20150209182928.GG24485@phenom.ffwll.local> (raw)
In-Reply-To: <1423500395-1787-5-git-send-email-przanoni@gmail.com>
On Mon, Feb 09, 2015 at 02:46:30PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> We want to port FBC to the frontbuffer tracking infrastructure, but
> for that we need to know what caused the object invalidation/flush so
> we can react accordingly: CPU mmaps need manual, GTT mmaps and
> flips don't need handling and ring rendering needs nukes.
>
> v2: - s/ORIGIN_RENDER/ORIGIN_CS/ (Daniel, Rodrigo)
> - Fix copy/pasted wrong documentation
> - Rebase
> v3: - Rebase
>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> (v2)
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Sorry for being really late with my review here. I fully agree that origin
for invalidate makes a lot of sense, but for flush it kinda doesn't.
There's no origin for a flush, it just means that any previous rendering
(from _any_ origin) has now completed.
I've looked ahead a bit and I'll comment on the patch that uses the flush
origin what should imo be used instead.
Merged the first 3 patches from this series meanwhile, thanks.
-Daniel
> ---
> drivers/gpu/drm/i915/i915_drv.h | 7 +++++++
> drivers/gpu/drm/i915/i915_gem.c | 10 +++++-----
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
> drivers/gpu/drm/i915/intel_drv.h | 11 +++++++----
> drivers/gpu/drm/i915/intel_frontbuffer.c | 15 ++++++++++-----
> 5 files changed, 30 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c0b8644..d578211 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -771,6 +771,13 @@ struct intel_context {
> struct list_head link;
> };
>
> +enum fb_op_origin {
> + ORIGIN_GTT,
> + ORIGIN_CPU,
> + ORIGIN_CS,
> + ORIGIN_FLIP,
> +};
> +
> struct i915_fbc {
> unsigned long uncompressed_size;
> unsigned threshold;
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index c26d36c..3d198f8 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2310,7 +2310,7 @@ i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
> list_move_tail(&vma->mm_list, &vma->vm->inactive_list);
> }
>
> - intel_fb_obj_flush(obj, true);
> + intel_fb_obj_flush(obj, true, ORIGIN_CS);
>
> list_del_init(&obj->ring_list);
>
> @@ -3677,7 +3677,7 @@ i915_gem_object_flush_gtt_write_domain(struct drm_i915_gem_object *obj)
> old_write_domain = obj->base.write_domain;
> obj->base.write_domain = 0;
>
> - intel_fb_obj_flush(obj, false);
> + intel_fb_obj_flush(obj, false, ORIGIN_GTT);
>
> trace_i915_gem_object_change_domain(obj,
> obj->base.read_domains,
> @@ -3699,7 +3699,7 @@ i915_gem_object_flush_cpu_write_domain(struct drm_i915_gem_object *obj)
> old_write_domain = obj->base.write_domain;
> obj->base.write_domain = 0;
>
> - intel_fb_obj_flush(obj, false);
> + intel_fb_obj_flush(obj, false, ORIGIN_CPU);
>
> trace_i915_gem_object_change_domain(obj,
> obj->base.read_domains,
> @@ -3764,7 +3764,7 @@ i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj, bool write)
> }
>
> if (write)
> - intel_fb_obj_invalidate(obj, NULL);
> + intel_fb_obj_invalidate(obj, NULL, ORIGIN_GTT);
>
> trace_i915_gem_object_change_domain(obj,
> old_read_domains,
> @@ -4079,7 +4079,7 @@ i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write)
> }
>
> if (write)
> - intel_fb_obj_invalidate(obj, NULL);
> + intel_fb_obj_invalidate(obj, NULL, ORIGIN_CPU);
>
> trace_i915_gem_object_change_domain(obj,
> old_read_domains,
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index b773368..f4342f0 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -971,7 +971,7 @@ i915_gem_execbuffer_move_to_active(struct list_head *vmas,
> obj->dirty = 1;
> i915_gem_request_assign(&obj->last_write_req, req);
>
> - intel_fb_obj_invalidate(obj, ring);
> + intel_fb_obj_invalidate(obj, ring, ORIGIN_CS);
>
> /* update for the implicit flush after a batch */
> obj->base.write_domain &= ~I915_GEM_GPU_DOMAINS;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 76b3c20..a73d091 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -853,13 +853,15 @@ void intel_ddi_set_vc_payload_alloc(struct drm_crtc *crtc, bool state);
>
> /* intel_frontbuffer.c */
> void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
> - struct intel_engine_cs *ring);
> + struct intel_engine_cs *ring,
> + enum fb_op_origin origin);
> void intel_frontbuffer_flip_prepare(struct drm_device *dev,
> unsigned frontbuffer_bits);
> void intel_frontbuffer_flip_complete(struct drm_device *dev,
> unsigned frontbuffer_bits);
> void intel_frontbuffer_flush(struct drm_device *dev,
> - unsigned frontbuffer_bits);
> + unsigned frontbuffer_bits,
> + enum fb_op_origin origin);
> /**
> * intel_frontbuffer_flip - synchronous frontbuffer flip
> * @dev: DRM device
> @@ -875,12 +877,13 @@ static inline
> void intel_frontbuffer_flip(struct drm_device *dev,
> unsigned frontbuffer_bits)
> {
> - intel_frontbuffer_flush(dev, frontbuffer_bits);
> + intel_frontbuffer_flush(dev, frontbuffer_bits, ORIGIN_FLIP);
> }
>
> int intel_fb_align_height(struct drm_device *dev, int height,
> unsigned int tiling);
> -void intel_fb_obj_flush(struct drm_i915_gem_object *obj, bool retire);
> +void intel_fb_obj_flush(struct drm_i915_gem_object *obj, bool retire,
> + enum fb_op_origin origin);
>
>
> /* intel_audio.c */
> diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
> index 73cb6e0..0a773981 100644
> --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> @@ -127,6 +127,7 @@ static void intel_mark_fb_busy(struct drm_device *dev,
> * intel_fb_obj_invalidate - invalidate frontbuffer object
> * @obj: GEM object to invalidate
> * @ring: set for asynchronous rendering
> + * @origin: which operation caused the invalidation
> *
> * This function gets called every time rendering on the given object starts and
> * frontbuffer caching (fbc, low refresh rate for DRRS, panel self refresh) must
> @@ -135,7 +136,8 @@ static void intel_mark_fb_busy(struct drm_device *dev,
> * scheduled.
> */
> void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
> - struct intel_engine_cs *ring)
> + struct intel_engine_cs *ring,
> + enum fb_op_origin origin)
> {
> struct drm_device *dev = obj->base.dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -164,6 +166,7 @@ void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
> * intel_frontbuffer_flush - flush frontbuffer
> * @dev: DRM device
> * @frontbuffer_bits: frontbuffer plane tracking bits
> + * @origin: which operation caused the flush
> *
> * This function gets called every time rendering on the given planes has
> * completed and frontbuffer caching can be started again. Flushes will get
> @@ -172,7 +175,8 @@ void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
> * Can be called without any locks held.
> */
> void intel_frontbuffer_flush(struct drm_device *dev,
> - unsigned frontbuffer_bits)
> + unsigned frontbuffer_bits,
> + enum fb_op_origin origin)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
>
> @@ -201,13 +205,14 @@ void intel_frontbuffer_flush(struct drm_device *dev,
> * intel_fb_obj_flush - flush frontbuffer object
> * @obj: GEM object to flush
> * @retire: set when retiring asynchronous rendering
> + * @origin: which operation caused the flush
> *
> * This function gets called every time rendering on the given object has
> * completed and frontbuffer caching can be started again. If @retire is true
> * then any delayed flushes will be unblocked.
> */
> void intel_fb_obj_flush(struct drm_i915_gem_object *obj,
> - bool retire)
> + bool retire, enum fb_op_origin origin)
> {
> struct drm_device *dev = obj->base.dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -229,7 +234,7 @@ void intel_fb_obj_flush(struct drm_i915_gem_object *obj,
> mutex_unlock(&dev_priv->fb_tracking.lock);
> }
>
> - intel_frontbuffer_flush(dev, frontbuffer_bits);
> + intel_frontbuffer_flush(dev, frontbuffer_bits, origin);
> }
>
> /**
> @@ -277,5 +282,5 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
> dev_priv->fb_tracking.flip_bits &= ~frontbuffer_bits;
> mutex_unlock(&dev_priv->fb_tracking.lock);
>
> - intel_frontbuffer_flush(dev, frontbuffer_bits);
> + intel_frontbuffer_flush(dev, frontbuffer_bits, ORIGIN_FLIP);
> }
> --
> 2.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> 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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2015-02-09 18:28 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-02-09 16:46 [RESEND 0/9] Reviewed FBC patches Paulo Zanoni
2015-02-09 16:46 ` [PATCH 1/9] drm/i915: don't try to find crtcs for FBC if it's disabled Paulo Zanoni
2015-02-09 16:46 ` [PATCH 2/9] drm/i915: don't keep reassigning FBC_UNSUPPORTED Paulo Zanoni
2015-02-09 16:46 ` [PATCH 3/9] drm/i915: change dev_priv->fbc.plane to dev_priv->fbc.crtc Paulo Zanoni
2015-02-09 16:46 ` [PATCH 4/9] drm/i915: pass which operation triggered the frontbuffer tracking Paulo Zanoni
2015-02-09 18:29 ` Daniel Vetter [this message]
2015-02-09 16:46 ` [PATCH 5/9] drm/i915: also do frontbuffer tracking on pwrites Paulo Zanoni
2015-02-09 18:41 ` Daniel Vetter
2015-02-11 8:30 ` Daniel Vetter
2015-02-12 14:12 ` Jani Nikula
2015-02-12 17:58 ` Paulo Zanoni
2015-02-09 16:46 ` [PATCH 6/9] drm/i915: add frontbuffer tracking to FBC Paulo Zanoni
2015-02-09 19:05 ` Daniel Vetter
2015-02-10 12:19 ` Paulo Zanoni
2015-02-11 8:25 ` Daniel Vetter
2015-02-09 16:46 ` [PATCH 7/9] drm/i915: extract intel_fbc_find_crtc() Paulo Zanoni
2015-02-09 16:46 ` [PATCH 8/9] drm/i915: HSW+ FBC is tied to pipe A Paulo Zanoni
2015-02-09 16:46 ` [PATCH 9/9] drm/i915: gen5+ can have FBC with multiple pipes Paulo Zanoni
2015-02-10 11:20 ` shuang.he
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150209182928.GG24485@phenom.ffwll.local \
--to=daniel@ffwll.ch \
--cc=intel-gfx@lists.freedesktop.org \
--cc=paulo.r.zanoni@intel.com \
--cc=przanoni@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.