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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 11 Feb 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Feb 09, 2015 at 07:41:17PM +0100, Daniel Vetter wrote:
>> 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.
>
> btw what's the use-case here? We don't upload stuff to X-tiled buffers
> with pwrite, so this isn't really relevant for fbc I think.
>
> It is a real gap for psr though, since cursor updates are done with
> pwrite. But that probably gets papered over by X also updating the
> position when the image changes, which means we'll get another (hw)
> flip combo. But that's just X, and we indeed don't have a pwrite cursor
> case yet in the psr testcase.

FYI, you asked to test an earlier version of this patch in
https://bugs.freedesktop.org/show_bug.cgi?id=87143

BR,
Jani


> -Daniel
> -- 
> 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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx