| Message ID | 1436311737-18270-3-git-send-email-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > > 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. > > 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@chris-wilson.co.uk> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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. -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 >
2015-07-08 6:47 GMT-03:00 Daniel Vetter <daniel@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@chris-wilson.co.uk> >> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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@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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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); + + 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
Let's do a frontbuffer flush on dirty fb. To be used for DIRTYFB drm ioctl. 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. 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@chris-wilson.co.uk> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)