[Intel-gfx] [PATCH 3/7] drm/i915: PSR: dirty fb operation flushsing frontbuffer

[Paulo Zanoni]

2015-07-08 6:47 GMT-03:00 Daniel Vetter <daniel at ffwll.ch>:
> On Tue, Jul 07, 2015 at 04:28:53PM -0700, Rodrigo Vivi wrote:
>> Let's do a frontbuffer flush on dirty fb.
>> To be used for DIRTYFB drm ioctl.

Just a notice: this commit will also be useful for FBC, so I hope that
marking the commit title as "PSR" won't confuse backporters.

>>
>> This patch solves the biggest PSR known issue, that is
>> missed screen updates during boot, mainly when there is a splash
>> screen involved like Plymouth.

Just a clarification: previously you claimed the flush would not solve
the problem, but you didn't have patch 2. Now with patch 2 + this one,
are we solving the biggest PSR problem?

>>
>> Previously PSR was being invalidated by fbdev and Plymounth
>> was taking control with PSR yet invalidated and could get screen
>> updates normally. However with some atomic modeset changes
>> Pymouth modeset over ioctl was now causing frontbuffer flushes
>> making PSR gets back to work while it cannot track the
>> screen updates and exit properly.
>>
>> By adding this flush on dirtyfb we properly track frontbuffer
>> writes and properly exit PSR.
>>
>> Actually all mmap_wc users should call this dirty callback
>> in order to have a proper frontbuffer tracking.
>>
>> In the future it can be extended to return 0 if the whole
>> screen has being flushed or the number of rects flushed
>> as Chris suggested.
>>
>> v2: Remove ORIGIN_FB_DIRTY and use ORIGIN_GTT instead since dirty
>>     callback is just called after few screen updates and not on
>>     everyone as pointed by Daniel.
>>
>> v3: Use flush instead of invalidate since flush means
>>     invalidate + flush and dirty means drawn had finished and
>>     it can be flushed.
>>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
>> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 3c2425f..9a60d15 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -14383,9 +14383,27 @@ static int intel_user_framebuffer_create_handle(struct drm_framebuffer *fb,
>>       return drm_gem_handle_create(file, &obj->base, handle);
>>  }
>>
>> +static int intel_user_framebuffer_dirty(struct drm_framebuffer *fb,
>> +                                            struct drm_file *file,
>> +                                            unsigned flags, unsigned color,
>> +                                            struct drm_clip_rect *clips,
>> +                                            unsigned num_clips)
>> +{
>> +     struct drm_device *dev = fb->dev;
>> +     struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
>> +     struct drm_i915_gem_object *obj = intel_fb->obj;
>> +
>> +     mutex_lock(&dev->struct_mutex);
>> +     intel_fb_obj_flush(obj, false, ORIGIN_GTT);
>> +     mutex_unlock(&dev->struct_mutex);
>
> There's some good discussion going on in some internal thread about what
> to do with wc mmaps. The idea is to use the dirty ioctl to flush them, but
> at least for fbc we already filter ORIGIN_GTT out, which means fbc won't
> see the dirtyfb flushes for cpu wc mmaps. Should we have a ORIGIN_DIRTYFB,
> which essentially just means "any kind of cpu frontbuffer rendering"?
>
> Otoh we can make this change when Paulo implements the wc-mmap for
> frontbuffers stuff.

Yeah, I wanted to discuss that. But I'll do my own local experiments
first, and I can always add a patch on top of this one, so, with or
without changes:

Reviewed-by: Paulo Zanoni <paulo.r.zanoni at intel.com>

> -Daniel
>
>> +
>> +     return 0;
>> +}
>> +
>>  static const struct drm_framebuffer_funcs intel_fb_funcs = {
>>       .destroy = intel_user_framebuffer_destroy,
>>       .create_handle = intel_user_framebuffer_create_handle,
>> +     .dirty = intel_user_framebuffer_dirty,
>>  };
>>
>>  static
>> --
>> 2.1.0
>>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni