Re: [PATCH 5/9] drm/i915: also do frontbuffer tracking on pwrites

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Feb 09, 2015 at 02:46:31PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> We need this for FBC, and possibly for PSR too.
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 3d198f8..15910fa 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1111,6 +1111,10 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
>  			ret = i915_gem_phys_pwrite(obj, args, file);
>  		else
>  			ret = i915_gem_shmem_pwrite(dev, obj, args, file);
> +
> +		intel_fb_obj_flush(obj, false, ORIGIN_CPU);
> +	} else {
> +		intel_fb_obj_flush(obj, false, ORIGIN_GTT);

A flush alone does nothing. Well should, but you're kinda not quite using
it correctly in the next patch to convert fbc over to frontbuffer
tracking.

I guess the docs aren't perfect, so let me try again. There are two kinds
of events the frontbuffer tracking code supplies to tell its consumers
that screen changes are happening:
- invalidate/flush: Invalidate denotes the start of the frontbuffer
  rendering, from that point on psr/fbc/drrs must update the screen with
  the usual refresh rate and not cache anything anywhere. When the flush
  happens (which could easily be after a _very_ long time, e.g. fbcon)
  then only can caching recomence. Caching = enable fbc, allow psr or
  reduce refresh rate.
- flip events: That's an instantenous event (well at least for consumer,
  internally we need to track it as prepare/complete for async flips), and
  mostly just interesting when the hw doesn't notice flips (some psr modes
  and drrs).

So if you want to add frontbuffer tracking to pwrite then we need both an
invalidate (before the actual pwrite) and a flush (after the pwrite, like
you've added here).

The other issue is that there's a bug with the origin assignemnt:
phys_pwrite also goes through the gtt. I think it would be best if we push
the fb_obj_invalidate/flush into the relevant pwrite functions. That
should make it easier to review, since the invalidate/flush will be next
to the write op.
-Daniel

>  	}
>  
>  out:
> -- 
> 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