[PATCH 1/3] drm/i915: Make fb user dirty operation to invalidate frontbuffer

[PATCH 1/3] drm/i915: Make fb user dirty operation to invalidate frontbuffer

[Rodrigo Vivi]

Let's do a frontbuffer invalidation 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.

Plymoth will do a modeset over ioctl that flushes frontbuffer
tracking and PSR gets back to work while it cannot track the
screen updates and exit properly. However plymouth also uses
a dirtyfb ioctl whenever updating the screen. So let's use it
to invalidate PSR back again.

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.

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 724b0e3..b55b1b6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14393,9 +14393,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_invalidate(obj, 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
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/3] drm/i915: PSR: Remove Low Power HW tracking mask.

[Rodrigo Vivi]

By Spec we should only mask memup and hotplug detection
for hardware tracking cases. However we always masked
LPSP because with power well always enabled on audio
PSR was never being activated and residency was always
zeroed.

Apparently audio driver is tying power well management
and runtime PM for some reason. But with audio runtime
PM working or with audio completely out of picture
we should remove this mask, otherwise we have a high
risk of miss screen updates as faced by Matthew.

WARNING: With this patch if snd_intel_hda driver is
runing and not releasing power well properly PSR will
constant Exit and Performance Counter will be 0.

But the best thing of this patch is that with one more
HW tracking working the risks of missed blank screen
are minimized at most.

This affects just core platforms where PSR exit are also
helped by HW tracking: Haswell, Broadwell and Skylake
for now.

v2: Fix commit message explanation. It has nothing to do
with runtime PM on i915 as previously advertised.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Matthew Garrett <mjg59@srcf.ucam.org via codon.org.uk>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index d79ba58..cab5153 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -400,7 +400,7 @@ void intel_psr_enable(struct intel_dp *intel_dp)
 
 		/* Avoid continuous PSR exit by masking memup and hpd */
 		I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP |
-			   EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP);
+			   EDP_PSR_DEBUG_MASK_HPD);
 
 		/* Enable PSR on the panel */
 		hsw_psr_enable_sink(intel_dp);
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: PSR: Remove Low Power HW tracking mask.

[Paulo Zanoni]

2015-06-30 20:42 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> By Spec we should only mask memup and hotplug detection
> for hardware tracking cases. However we always masked
> LPSP because with power well always enabled on audio
> PSR was never being activated and residency was always
> zeroed.
>
> Apparently audio driver is tying power well management
> and runtime PM for some reason. But with audio runtime
> PM working or with audio completely out of picture
> we should remove this mask, otherwise we have a high
> risk of miss screen updates as faced by Matthew.
>
> WARNING: With this patch if snd_intel_hda driver is
> runing and not releasing power well properly PSR will
> constant Exit and Performance Counter will be 0.
>
> But the best thing of this patch is that with one more
> HW tracking working the risks of missed blank screen
> are minimized at most.
>
> This affects just core platforms where PSR exit are also
> helped by HW tracking: Haswell, Broadwell and Skylake
> for now.
>
> v2: Fix commit message explanation. It has nothing to do
> with runtime PM on i915 as previously advertised.

But then, as far as I understood, if you remove the mask, our current
IGT patches will start failing with "PSR disabled" errors, right? So
can you please also provide IGT fixes?

If the only thing we really need is Audio RPM instead of full graphics
RPM, then I suggest you extract igt_setup_audio_runtime_pm() from the
current igt_setup_runtime_pm(), and then call it on the tests that
need PSR.

>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Matthew Garrett <mjg59@srcf.ucam.org via codon.org.uk>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index d79ba58..cab5153 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -400,7 +400,7 @@ void intel_psr_enable(struct intel_dp *intel_dp)
>
>                 /* Avoid continuous PSR exit by masking memup and hpd */
>                 I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP |
> -                          EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP);
> +                          EDP_PSR_DEBUG_MASK_HPD);
>
>                 /* Enable PSR on the panel */
>                 hsw_psr_enable_sink(intel_dp);
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/3] drm/i915: Enable PSR by default.

[Rodrigo Vivi]

With a reliable frontbuffer tracking and all instability corner cases solved
let's re-enabled PSR by default on all supported platforms.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_params.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 7983fe4..2eb7c65 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -36,7 +36,7 @@ struct i915_params i915 __read_mostly = {
 	.enable_execlists = -1,
 	.enable_hangcheck = true,
 	.enable_ppgtt = -1,
-	.enable_psr = 0,
+	.enable_psr = 1,
 	.preliminary_hw_support = IS_ENABLED(CONFIG_DRM_I915_PRELIMINARY_HW_SUPPORT),
 	.disable_power_well = 1,
 	.enable_ips = 1,
@@ -118,7 +118,7 @@ MODULE_PARM_DESC(enable_execlists,
 	"(-1=auto [default], 0=disabled, 1=enabled)");
 
 module_param_named(enable_psr, i915.enable_psr, int, 0600);
-MODULE_PARM_DESC(enable_psr, "Enable PSR (default: false)");
+MODULE_PARM_DESC(enable_psr, "Enable PSR (default: true)");
 
 module_param_named(preliminary_hw_support, i915.preliminary_hw_support, int, 0600);
 MODULE_PARM_DESC(preliminary_hw_support,
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Enable PSR by default.

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6691
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                                  302/302              302/302
SNB                                  312/316              312/316
IVB                                  343/343              343/343
BYT                 -2              287/287              285/287
HSW                                  380/380              380/380
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*BYT  igt@gem_partial_pwrite_pread@reads      PASS(1)      FAIL(1)
*BYT  igt@gem_tiled_partial_pwrite_pread@reads      PASS(1)      FAIL(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Make fb user dirty operation to invalidate frontbuffer

[Chris Wilson]

On Tue, Jun 30, 2015 at 04:42:00PM -0700, Rodrigo Vivi wrote:
> Let's do a frontbuffer invalidation 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.
> 
> Plymoth will do a modeset over ioctl that flushes frontbuffer
> tracking and PSR gets back to work while it cannot track the
> screen updates and exit properly. However plymouth also uses
> a dirtyfb ioctl whenever updating the screen. So let's use it
> to invalidate PSR back again.
> 
> 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.
> 
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Will it ever grow the ability to handle clip rects? I can detect the
presence of the syscall and call it appropriately, but I don't want to
have to start tracking frontbuffer damage unless there's a significant
advantage in doing so (to offset the cost of the tracking).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Make fb user dirty operation to invalidate frontbuffer

[Daniel Vetter]

On Wed, Jul 01, 2015 at 09:04:08AM +0100, Chris Wilson wrote:
> On Tue, Jun 30, 2015 at 04:42:00PM -0700, Rodrigo Vivi wrote:
> > Let's do a frontbuffer invalidation 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.
> > 
> > Plymoth will do a modeset over ioctl that flushes frontbuffer
> > tracking and PSR gets back to work while it cannot track the
> > screen updates and exit properly. However plymouth also uses
> > a dirtyfb ioctl whenever updating the screen. So let's use it
> > to invalidate PSR back again.
> > 
> > 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.
> > 
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> Will it ever grow the ability to handle clip rects? I can detect the
> presence of the syscall and call it appropriately, but I don't want to
> have to start tracking frontbuffer damage unless there's a significant
> advantage in doing so (to offset the cost of the tracking).

For now this is just for generic userspace using the dumb mmap ioctls,
which does already dirty everything. For gem/i915 userspace the existing
frontbuffer tracking rules will still apply.

I think the damage rect will only be useful for psr2 single-shot uploads,
but we need to do some serious hacks using debug rects to pull this off -
the hw wants to be way to helpful and gets in the way. And I think for
that usecase it only makes sense to supply the overall per-crtc damage
rect in an atomic flip. Tracking damage for frontbuffer rendering probably
won't be worth it anytime soon at all.

In short: Nothing to worry about yet for a long time ;-)
-Daniel
-- 
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

Re: [PATCH 1/3] drm/i915: Make fb user dirty operation to invalidate frontbuffer

[Chris Wilson]

On Wed, Jul 01, 2015 at 03:19:31PM +0200, Daniel Vetter wrote:
> On Wed, Jul 01, 2015 at 09:04:08AM +0100, Chris Wilson wrote:
> > On Tue, Jun 30, 2015 at 04:42:00PM -0700, Rodrigo Vivi wrote:
> > > Let's do a frontbuffer invalidation 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.
> > > 
> > > Plymoth will do a modeset over ioctl that flushes frontbuffer
> > > tracking and PSR gets back to work while it cannot track the
> > > screen updates and exit properly. However plymouth also uses
> > > a dirtyfb ioctl whenever updating the screen. So let's use it
> > > to invalidate PSR back again.
> > > 
> > > 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.
> > > 
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > 
> > Will it ever grow the ability to handle clip rects? I can detect the
> > presence of the syscall and call it appropriately, but I don't want to
> > have to start tracking frontbuffer damage unless there's a significant
> > advantage in doing so (to offset the cost of the tracking).
> 
> For now this is just for generic userspace using the dumb mmap ioctls,
> which does already dirty everything. For gem/i915 userspace the existing
> frontbuffer tracking rules will still apply.

But they are inadequate for the map/set-domain scanout once and write
through the GTT for umpteen seconds, which can happen quite frequenctly.

In that situation, we behave exactly like fbdev/dumb fb.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Make fb user dirty operation to invalidate frontbuffer

[Daniel Vetter]

On Wed, Jul 01, 2015 at 02:21:40PM +0100, Chris Wilson wrote:
> On Wed, Jul 01, 2015 at 03:19:31PM +0200, Daniel Vetter wrote:
> > On Wed, Jul 01, 2015 at 09:04:08AM +0100, Chris Wilson wrote:
> > > On Tue, Jun 30, 2015 at 04:42:00PM -0700, Rodrigo Vivi wrote:
> > > > Let's do a frontbuffer invalidation 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.
> > > > 
> > > > Plymoth will do a modeset over ioctl that flushes frontbuffer
> > > > tracking and PSR gets back to work while it cannot track the
> > > > screen updates and exit properly. However plymouth also uses
> > > > a dirtyfb ioctl whenever updating the screen. So let's use it
> > > > to invalidate PSR back again.
> > > > 
> > > > 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.
> > > > 
> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > 
> > > Will it ever grow the ability to handle clip rects? I can detect the
> > > presence of the syscall and call it appropriately, but I don't want to
> > > have to start tracking frontbuffer damage unless there's a significant
> > > advantage in doing so (to offset the cost of the tracking).
> > 
> > For now this is just for generic userspace using the dumb mmap ioctls,
> > which does already dirty everything. For gem/i915 userspace the existing
> > frontbuffer tracking rules will still apply.
> 
> But they are inadequate for the map/set-domain scanout once and write
> through the GTT for umpteen seconds, which can happen quite frequenctly.
> 
> In that situation, we behave exactly like fbdev/dumb fb.

Yeah you can use it to flush gtt of course too. And there I'd just
defensively flush the entire fb until we've grown more clueful in the
kernel. But for forntbuffer flushing I don't expect that to ever happen
for i915. It makes more sense ofc for udl/qxl and others where uploads are
really expensive.
-Daniel
-- 
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

Re: [PATCH 1/3] drm/i915: Make fb user dirty operation to invalidate frontbuffer

[Chris Wilson]

On Wed, Jul 01, 2015 at 04:09:19PM +0200, Daniel Vetter wrote:
> On Wed, Jul 01, 2015 at 02:21:40PM +0100, Chris Wilson wrote:
> > On Wed, Jul 01, 2015 at 03:19:31PM +0200, Daniel Vetter wrote:
> > > On Wed, Jul 01, 2015 at 09:04:08AM +0100, Chris Wilson wrote:
> > > > On Tue, Jun 30, 2015 at 04:42:00PM -0700, Rodrigo Vivi wrote:
> > > > > Let's do a frontbuffer invalidation 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.
> > > > > 
> > > > > Plymoth will do a modeset over ioctl that flushes frontbuffer
> > > > > tracking and PSR gets back to work while it cannot track the
> > > > > screen updates and exit properly. However plymouth also uses
> > > > > a dirtyfb ioctl whenever updating the screen. So let's use it
> > > > > to invalidate PSR back again.
> > > > > 
> > > > > 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.
> > > > > 
> > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > 
> > > > Will it ever grow the ability to handle clip rects? I can detect the
> > > > presence of the syscall and call it appropriately, but I don't want to
> > > > have to start tracking frontbuffer damage unless there's a significant
> > > > advantage in doing so (to offset the cost of the tracking).
> > > 
> > > For now this is just for generic userspace using the dumb mmap ioctls,
> > > which does already dirty everything. For gem/i915 userspace the existing
> > > frontbuffer tracking rules will still apply.
> > 
> > But they are inadequate for the map/set-domain scanout once and write
> > through the GTT for umpteen seconds, which can happen quite frequenctly.
> > 
> > In that situation, we behave exactly like fbdev/dumb fb.
> 
> Yeah you can use it to flush gtt of course too. And there I'd just
> defensively flush the entire fb until we've grown more clueful in the
> kernel. But for forntbuffer flushing I don't expect that to ever happen
> for i915. It makes more sense ofc for udl/qxl and others where uploads are
> really expensive.

Ha, I thought that MIPI was going off in this direction precisely
because high pixel count displays only transferring the dirty regions is
a big power saving. Likewise I expect at some point, there will be a
chipset mode to only portions of the framebuffer out of the chip local
cache across the bus.

I want to make sure we are not going to shoot ourselves in the foot and
can forward-proof the design so that we can easily detect if we want to
use cliprects.

An easy way would be to return the number of rectangles pushed by
dirtyfb i.e. if dirtyfb(fb, .num_rects=1)== 0 I know that it just pushes
the whole framebuffer everytime and need not track damage.

The ABI would be negative error return on failure, 0 or postive value on
success, where the positive value is the number of rects pushed (which
should be identical to the user request on success). Dumb users then
don't need to care as they can either always request full fb flushes or
always push rects.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Make fb user dirty operation to invalidate frontbuffer

[Paulo Zanoni]

2015-06-30 20:42 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> Let's do a frontbuffer invalidation 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.
>
> Plymoth will do a modeset over ioctl that flushes frontbuffer
> tracking and PSR gets back to work while it cannot track the
> screen updates and exit properly. However plymouth also uses
> a dirtyfb ioctl whenever updating the screen. So let's use it
> to invalidate PSR back again.
>
> 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.
>
> 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 724b0e3..b55b1b6 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14393,9 +14393,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)

You don't need the white space on the lines above, just the tabs.

> +{
> +       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_invalidate(obj, ORIGIN_GTT);

As far as I understood from my brief investigation, the dirty IOCTL
just says "hey, I'm done writing on the memory, you can flush things
now", and if the user writes on the buffer again later, it will need
to call the dirty IOCTL again. So shouldn't we call
intel_fb_obj_flush(obj, false) here? It seems this was also pointed by
Daniel on the first review. It would be better because it would allow
us to actually keep PSR/FBC enabled.

Also, don't forget that switching to intel_fb_obj_flush() will allow
you to kill the struct_mutex lock.

BTW, I'm also looking forward to see the IGT test suggested by Daniel :)

> +       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
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Make fb user dirty operation to invalidate frontbuffer

[Vivi, Rodrigo]

On Thu, 2015-07-02 at 13:03 -0300, Paulo Zanoni wrote:
> 2015-06-30 20:42 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> > Let's do a frontbuffer invalidation 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.
> >
> > Plymoth will do a modeset over ioctl that flushes frontbuffer
> > tracking and PSR gets back to work while it cannot track the
> > screen updates and exit properly. However plymouth also uses
> > a dirtyfb ioctl whenever updating the screen. So let's use it
> > to invalidate PSR back again.
> >
> > 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.
> >
> > 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 724b0e3..b55b1b6 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -14393,9 +14393,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)
> 
> You don't need the white space on the lines above, just the tabs.
> 
> > +{
> > +       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_invalidate(obj, ORIGIN_GTT);
> 
> As far as I understood from my brief investigation, the dirty IOCTL
> just says "hey, I'm done writing on the memory, you can flush things
> now", and if the user writes on the buffer again later, it will need
> to call the dirty IOCTL again. So shouldn't we call
> intel_fb_obj_flush(obj, false) here? It seems this was also pointed by
> Daniel on the first review. It would be better because it would allow
> us to actually keep PSR/FBC enabled.

The flush caused by the dumb modeset ioctl is exactly what I want to
revert here.

Well, I just tested to double check here and flush makes me to miss
screen updates. (triple checked with retired = true as well just in
case)

fbdev comes to place and invalidated frontbuffer, then plymouth does a
flush that makes psr start working when it should continue disabled.
Continue flushing it doesn't solve the problem, just ratify it.

But beside the issue that it is solving I don't believe we want is a
flush anyway. There is something writing directly to frontbuffer with no
invalidation. The dirty call is supposed to be a damage call that
actually tells something on the screen got written and needs to be
updated if it hasn't still. In our cause this is exactly the frontbuffer
invalidate, not the flush.

The flush place would be after it really stopped doing something, but
since I don't trust it I prefer to let it invalidated until next flip.

> 
> Also, don't forget that switching to intel_fb_obj_flush() will allow
> you to kill the struct_mutex lock.
> 
> BTW, I'm also looking forward to see the IGT test suggested by Daniel :)
> 
> > +       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
> > --
> > 2.1.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Make fb user dirty operation to invalidate frontbuffer

[Daniel Vetter]

On Thu, Jul 02, 2015 at 04:41:32PM +0000, Vivi, Rodrigo wrote:
> On Thu, 2015-07-02 at 13:03 -0300, Paulo Zanoni wrote:
> > 2015-06-30 20:42 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> > > Let's do a frontbuffer invalidation 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.
> > >
> > > Plymoth will do a modeset over ioctl that flushes frontbuffer
> > > tracking and PSR gets back to work while it cannot track the
> > > screen updates and exit properly. However plymouth also uses
> > > a dirtyfb ioctl whenever updating the screen. So let's use it
> > > to invalidate PSR back again.
> > >
> > > 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.
> > >
> > > 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 724b0e3..b55b1b6 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -14393,9 +14393,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)
> > 
> > You don't need the white space on the lines above, just the tabs.
> > 
> > > +{
> > > +       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_invalidate(obj, ORIGIN_GTT);
> > 
> > As far as I understood from my brief investigation, the dirty IOCTL
> > just says "hey, I'm done writing on the memory, you can flush things
> > now", and if the user writes on the buffer again later, it will need
> > to call the dirty IOCTL again. So shouldn't we call
> > intel_fb_obj_flush(obj, false) here? It seems this was also pointed by
> > Daniel on the first review. It would be better because it would allow
> > us to actually keep PSR/FBC enabled.
> 
> The flush caused by the dumb modeset ioctl is exactly what I want to
> revert here.
> 
> Well, I just tested to double check here and flush makes me to miss
> screen updates. (triple checked with retired = true as well just in
> case)
> 
> fbdev comes to place and invalidated frontbuffer, then plymouth does a
> flush that makes psr start working when it should continue disabled.
> Continue flushing it doesn't solve the problem, just ratify it.
> 
> But beside the issue that it is solving I don't believe we want is a
> flush anyway. There is something writing directly to frontbuffer with no
> invalidation. The dirty call is supposed to be a damage call that
> actually tells something on the screen got written and needs to be
> updated if it hasn't still. In our cause this is exactly the frontbuffer
> invalidate, not the flush.
> 
> The flush place would be after it really stopped doing something, but
> since I don't trust it I prefer to let it invalidated until next flip.

See my review on the first round of this patch: dirtyfb _is_ a flush. If
it's not enough then there's some other problems still around.
-Daniel
-- 
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

Re: [PATCH 1/3] drm/i915: Make fb user dirty operation to invalidate frontbuffer

[Vivi, Rodrigo]

On Fri, 2015-07-03 at 09:10 +0200, Daniel Vetter wrote:
> On Thu, Jul 02, 2015 at 04:41:32PM +0000, Vivi, Rodrigo wrote:
> > On Thu, 2015-07-02 at 13:03 -0300, Paulo Zanoni wrote:
> > > 2015-06-30 20:42 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> > > > Let's do a frontbuffer invalidation 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.
> > > >
> > > > Plymoth will do a modeset over ioctl that flushes frontbuffer
> > > > tracking and PSR gets back to work while it cannot track the
> > > > screen updates and exit properly. However plymouth also uses
> > > > a dirtyfb ioctl whenever updating the screen. So let's use it
> > > > to invalidate PSR back again.
> > > >
> > > > 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.
> > > >
> > > > 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 724b0e3..b55b1b6 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -14393,9 +14393,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)
> > > 
> > > You don't need the white space on the lines above, just the tabs.
> > > 
> > > > +{
> > > > +       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_invalidate(obj, ORIGIN_GTT);
> > > 
> > > As far as I understood from my brief investigation, the dirty IOCTL
> > > just says "hey, I'm done writing on the memory, you can flush things
> > > now", and if the user writes on the buffer again later, it will need
> > > to call the dirty IOCTL again. So shouldn't we call
> > > intel_fb_obj_flush(obj, false) here? It seems this was also pointed by
> > > Daniel on the first review. It would be better because it would allow
> > > us to actually keep PSR/FBC enabled.
> > 
> > The flush caused by the dumb modeset ioctl is exactly what I want to
> > revert here.
> > 
> > Well, I just tested to double check here and flush makes me to miss
> > screen updates. (triple checked with retired = true as well just in
> > case)
> > 
> > fbdev comes to place and invalidated frontbuffer, then plymouth does a
> > flush that makes psr start working when it should continue disabled.
> > Continue flushing it doesn't solve the problem, just ratify it.
> > 
> > But beside the issue that it is solving I don't believe we want is a
> > flush anyway. There is something writing directly to frontbuffer with no
> > invalidation. The dirty call is supposed to be a damage call that
> > actually tells something on the screen got written and needs to be
> > updated if it hasn't still. In our cause this is exactly the frontbuffer
> > invalidate, not the flush.
> > 
> > The flush place would be after it really stopped doing something, but
> > since I don't trust it I prefer to let it invalidated until next flip.
> 
> See my review on the first round of this patch: dirtyfb _is_ a flush. If
> it's not enough then there's some other problems still around.

Well, the flush itself in the way it is defined is to flush frontbuffer
bits and put power saving features back to work. PSR flush for instance
doesn't exit on every flush. Only in the cases that we know HW tracking
doesn't work.

One possibility would be change PSR to respect that all flush always is
invalidate (exit) + flush fb_bits. But I'm against this since PSR on
core platforms doesn't have SW trackking ext and it wasn't implemented
to be fully enabled/disabled every time.

Another idea on this line is to make flushs know origins or at least a
boolean to separate cases where flush must be handled as flush bits +
continue working or invalidate + flush bits + start working again

Please let me know what you think.

But putting flushs on this dirty callback is currently useless because
flush just tells psr to continue working without getting screen updates.

Well, this is the most important discussion for now, but also even we
change flush interface or only in PSR I'm not sure this will work.

This dirty case happens in the middle of fbdev and when it gives control
back to fbcon psr would be working without no one else invalidate or
flushing it and we would miss screen updates anyway.
But in this case I understand this is a hack and if we put the
invalidate there I should also put a FIXME XXX explaining that...


> -Daniel

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Make fb user dirty operation to invalidate frontbuffer

[Paulo Zanoni]

2015-07-06 13:43 GMT-03:00 Vivi, Rodrigo <rodrigo.vivi@intel.com>:
> On Fri, 2015-07-03 at 09:10 +0200, Daniel Vetter wrote:
>> On Thu, Jul 02, 2015 at 04:41:32PM +0000, Vivi, Rodrigo wrote:
>> > On Thu, 2015-07-02 at 13:03 -0300, Paulo Zanoni wrote:
>> > > 2015-06-30 20:42 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
>> > > > Let's do a frontbuffer invalidation 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.
>> > > >
>> > > > Plymoth will do a modeset over ioctl that flushes frontbuffer
>> > > > tracking and PSR gets back to work while it cannot track the
>> > > > screen updates and exit properly. However plymouth also uses
>> > > > a dirtyfb ioctl whenever updating the screen. So let's use it
>> > > > to invalidate PSR back again.
>> > > >
>> > > > 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.
>> > > >
>> > > > 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 724b0e3..b55b1b6 100644
>> > > > --- a/drivers/gpu/drm/i915/intel_display.c
>> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
>> > > > @@ -14393,9 +14393,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)
>> > >
>> > > You don't need the white space on the lines above, just the tabs.
>> > >
>> > > > +{
>> > > > +       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_invalidate(obj, ORIGIN_GTT);
>> > >
>> > > As far as I understood from my brief investigation, the dirty IOCTL
>> > > just says "hey, I'm done writing on the memory, you can flush things
>> > > now", and if the user writes on the buffer again later, it will need
>> > > to call the dirty IOCTL again. So shouldn't we call
>> > > intel_fb_obj_flush(obj, false) here? It seems this was also pointed by
>> > > Daniel on the first review. It would be better because it would allow
>> > > us to actually keep PSR/FBC enabled.
>> >
>> > The flush caused by the dumb modeset ioctl is exactly what I want to
>> > revert here.
>> >
>> > Well, I just tested to double check here and flush makes me to miss
>> > screen updates. (triple checked with retired = true as well just in
>> > case)
>> >
>> > fbdev comes to place and invalidated frontbuffer, then plymouth does a
>> > flush that makes psr start working when it should continue disabled.
>> > Continue flushing it doesn't solve the problem, just ratify it.
>> >
>> > But beside the issue that it is solving I don't believe we want is a
>> > flush anyway. There is something writing directly to frontbuffer with no
>> > invalidation. The dirty call is supposed to be a damage call that
>> > actually tells something on the screen got written and needs to be
>> > updated if it hasn't still. In our cause this is exactly the frontbuffer
>> > invalidate, not the flush.
>> >
>> > The flush place would be after it really stopped doing something, but
>> > since I don't trust it I prefer to let it invalidated until next flip.
>>
>> See my review on the first round of this patch: dirtyfb _is_ a flush. If
>> it's not enough then there's some other problems still around.
>
> Well, the flush itself in the way it is defined is to flush frontbuffer
> bits and put power saving features back to work. PSR flush for instance
> doesn't exit on every flush. Only in the cases that we know HW tracking
> doesn't work.
>
> One possibility would be change PSR to respect that all flush always is
> invalidate (exit) + flush fb_bits. But I'm against this since PSR on
> core platforms doesn't have SW trackking ext and it wasn't implemented
> to be fully enabled/disabled every time.
>
> Another idea on this line is to make flushs know origins or at least a
> boolean to separate cases where flush must be handled as flush bits +
> continue working or invalidate + flush bits + start working again
>
> Please let me know what you think.
>
> But putting flushs on this dirty callback is currently useless because
> flush just tells psr to continue working without getting screen updates.
>
> Well, this is the most important discussion for now, but also even we
> change flush interface or only in PSR I'm not sure this will work.
>
> This dirty case happens in the middle of fbdev and when it gives control
> back to fbcon psr would be working without no one else invalidate or
> flushing it and we would miss screen updates anyway.
> But in this case I understand this is a hack and if we put the
> invalidate there I should also put a FIXME XXX explaining that...

Last week I discussed some of this with Rodrigo on IRC and we have
different interpretations on what the frontbuffer tracking calls mean.
Daniel, can you please clarify a few things?

One of the things I've discussed with Rodrigo is that I consider
invalidate+flush to be equivalent to just a flush call, while Rodrigo
sees both cases as different. Daniel, can you please clarify the
intentions of the frontbuffer tracking infrastructure here?

One contradiction I see with the current code is that PSR does nothing
on flush() for BDW because it believes the HW tracking, but it does
psr_exit() on invalidate for every single case. If it relies on the HW
tracking, shouldn't it also do nothing on invalidate()? If it doesn't
rely on HW tracking, shouldn't it do something on flush()? If it
relies on just a few specific pieces of the HW tracking, shouldn't it
check enum fb_op_origin? The conclusion here is that the PSR code is
completely relying on having invalidate() calls before the flush() for
cases where it needs help from software.

Also, we've been discussing a boot splash screen, and, according to
what you said, it's supposed to be using the dumb interfaces, right?
So I see that the dumb mmap is just a gem_mmap() to our driver. But
PSR is supposed to get disabled after GTT mmaps. Why is it not
disabled in this case? That is because we don't get the invalidate
call after the mmap, right? If we require an invalidate on the dirty
ioctl, it means that PSR is enabled even though it is GTT mmapped...
That's supposed to be wrong, right? Shouldn't the mmap IOCTL force an
invalidate?

>
>
>> -Daniel
>



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Make fb user dirty operation to invalidate frontbuffer

[Daniel Vetter]

On Mon, Jul 06, 2015 at 02:11:55PM -0300, Paulo Zanoni wrote:
> 2015-07-06 13:43 GMT-03:00 Vivi, Rodrigo <rodrigo.vivi@intel.com>:
> > On Fri, 2015-07-03 at 09:10 +0200, Daniel Vetter wrote:
> >> On Thu, Jul 02, 2015 at 04:41:32PM +0000, Vivi, Rodrigo wrote:
> >> > On Thu, 2015-07-02 at 13:03 -0300, Paulo Zanoni wrote:
> >> > > 2015-06-30 20:42 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> >> > > > Let's do a frontbuffer invalidation 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.
> >> > > >
> >> > > > Plymoth will do a modeset over ioctl that flushes frontbuffer
> >> > > > tracking and PSR gets back to work while it cannot track the
> >> > > > screen updates and exit properly. However plymouth also uses
> >> > > > a dirtyfb ioctl whenever updating the screen. So let's use it
> >> > > > to invalidate PSR back again.
> >> > > >
> >> > > > 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.
> >> > > >
> >> > > > 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 724b0e3..b55b1b6 100644
> >> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> >> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> >> > > > @@ -14393,9 +14393,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)
> >> > >
> >> > > You don't need the white space on the lines above, just the tabs.
> >> > >
> >> > > > +{
> >> > > > +       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_invalidate(obj, ORIGIN_GTT);
> >> > >
> >> > > As far as I understood from my brief investigation, the dirty IOCTL
> >> > > just says "hey, I'm done writing on the memory, you can flush things
> >> > > now", and if the user writes on the buffer again later, it will need
> >> > > to call the dirty IOCTL again. So shouldn't we call
> >> > > intel_fb_obj_flush(obj, false) here? It seems this was also pointed by
> >> > > Daniel on the first review. It would be better because it would allow
> >> > > us to actually keep PSR/FBC enabled.
> >> >
> >> > The flush caused by the dumb modeset ioctl is exactly what I want to
> >> > revert here.
> >> >
> >> > Well, I just tested to double check here and flush makes me to miss
> >> > screen updates. (triple checked with retired = true as well just in
> >> > case)
> >> >
> >> > fbdev comes to place and invalidated frontbuffer, then plymouth does a
> >> > flush that makes psr start working when it should continue disabled.
> >> > Continue flushing it doesn't solve the problem, just ratify it.
> >> >
> >> > But beside the issue that it is solving I don't believe we want is a
> >> > flush anyway. There is something writing directly to frontbuffer with no
> >> > invalidation. The dirty call is supposed to be a damage call that
> >> > actually tells something on the screen got written and needs to be
> >> > updated if it hasn't still. In our cause this is exactly the frontbuffer
> >> > invalidate, not the flush.
> >> >
> >> > The flush place would be after it really stopped doing something, but
> >> > since I don't trust it I prefer to let it invalidated until next flip.
> >>
> >> See my review on the first round of this patch: dirtyfb _is_ a flush. If
> >> it's not enough then there's some other problems still around.
> >
> > Well, the flush itself in the way it is defined is to flush frontbuffer
> > bits and put power saving features back to work. PSR flush for instance
> > doesn't exit on every flush. Only in the cases that we know HW tracking
> > doesn't work.
> >
> > One possibility would be change PSR to respect that all flush always is
> > invalidate (exit) + flush fb_bits. But I'm against this since PSR on
> > core platforms doesn't have SW trackking ext and it wasn't implemented
> > to be fully enabled/disabled every time.
> >
> > Another idea on this line is to make flushs know origins or at least a
> > boolean to separate cases where flush must be handled as flush bits +
> > continue working or invalidate + flush bits + start working again
> >
> > Please let me know what you think.
> >
> > But putting flushs on this dirty callback is currently useless because
> > flush just tells psr to continue working without getting screen updates.
> >
> > Well, this is the most important discussion for now, but also even we
> > change flush interface or only in PSR I'm not sure this will work.
> >
> > This dirty case happens in the middle of fbdev and when it gives control
> > back to fbcon psr would be working without no one else invalidate or
> > flushing it and we would miss screen updates anyway.
> > But in this case I understand this is a hack and if we put the
> > invalidate there I should also put a FIXME XXX explaining that...
> 
> Last week I discussed some of this with Rodrigo on IRC and we have
> different interpretations on what the frontbuffer tracking calls mean.
> Daniel, can you please clarify a few things?
> 
> One of the things I've discussed with Rodrigo is that I consider
> invalidate+flush to be equivalent to just a flush call, while Rodrigo
> sees both cases as different. Daniel, can you please clarify the
> intentions of the frontbuffer tracking infrastructure here?

Yeah essentially flush without an invalidate is just a instantaneous
invalidate+flush. invalidate just means "someone started to frontbuffer
render and I have no idea at all when that will stop". Flush means
"someone has done some frontbuffer rendering, make sure those updates land
on the screen". So yeah Paulo's idea is correct.

Problem is now that when we get a flip we also call flush, and for psr on
big core that's probably not what we want since it can handle flips
correctly, but not real frontbuffer flushes.

> One contradiction I see with the current code is that PSR does nothing
> on flush() for BDW because it believes the HW tracking, but it does
> psr_exit() on invalidate for every single case. If it relies on the HW
> tracking, shouldn't it also do nothing on invalidate()? If it doesn't
> rely on HW tracking, shouldn't it do something on flush()? If it
> relies on just a few specific pieces of the HW tracking, shouldn't it
> check enum fb_op_origin? The conclusion here is that the PSR code is
> completely relying on having invalidate() calls before the flush() for
> cases where it needs help from software.

Calling invalidate before flush isn't imo the right fix since usually
(except when you have working hw tracking like fbc for gtt mmaps) you need
to stop psr/fbc/whatever on invalidate. Whereas on flush you just need to
make sure screen contents get to the screen.

I think the best option would be to wire up the fb_op_origin for flushes
too, and then filter out ORIGIN_FLIP for psr (except for sprites on hsw)
on big core.
 
> Also, we've been discussing a boot splash screen, and, according to
> what you said, it's supposed to be using the dumb interfaces, right?
> So I see that the dumb mmap is just a gem_mmap() to our driver. But
> PSR is supposed to get disabled after GTT mmaps. Why is it not
> disabled in this case? That is because we don't get the invalidate
> call after the mmap, right? If we require an invalidate on the dirty
> ioctl, it means that PSR is enabled even though it is GTT mmapped...
> That's supposed to be wrong, right? Shouldn't the mmap IOCTL force an
> invalidate?

Yeah dumb mmap users are supposed to call dirtyfb after each draw call.

i915 userspace could start doing the same to mark the gtt mmap invalidate
as done since we don't have any point where we stop it right now with an
i915 ioctl. And to be able to work together with set_domain(GTT) it really
needs to be a flush in ->dirty()
-Daniel
-- 
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

Re: [PATCH 1/3] drm/i915: Make fb user dirty operation to invalidate frontbuffer

[Vivi, Rodrigo]

On Mon, 2015-07-06 at 19:56 +0200, Daniel Vetter wrote:
> On Mon, Jul 06, 2015 at 02:11:55PM -0300, Paulo Zanoni wrote:
> > 2015-07-06 13:43 GMT-03:00 Vivi, Rodrigo <rodrigo.vivi@intel.com>:
> > > On Fri, 2015-07-03 at 09:10 +0200, Daniel Vetter wrote:
> > >> On Thu, Jul 02, 2015 at 04:41:32PM +0000, Vivi, Rodrigo wrote:
> > >> > On Thu, 2015-07-02 at 13:03 -0300, Paulo Zanoni wrote:
> > >> > > 2015-06-30 20:42 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> > >> > > > Let's do a frontbuffer invalidation 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.
> > >> > > >
> > >> > > > Plymoth will do a modeset over ioctl that flushes frontbuffer
> > >> > > > tracking and PSR gets back to work while it cannot track the
> > >> > > > screen updates and exit properly. However plymouth also uses
> > >> > > > a dirtyfb ioctl whenever updating the screen. So let's use it
> > >> > > > to invalidate PSR back again.
> > >> > > >
> > >> > > > 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.
> > >> > > >
> > >> > > > 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 724b0e3..b55b1b6 100644
> > >> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > >> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > >> > > > @@ -14393,9 +14393,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)
> > >> > >
> > >> > > You don't need the white space on the lines above, just the tabs.
> > >> > >
> > >> > > > +{
> > >> > > > +       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_invalidate(obj, ORIGIN_GTT);
> > >> > >
> > >> > > As far as I understood from my brief investigation, the dirty IOCTL
> > >> > > just says "hey, I'm done writing on the memory, you can flush things
> > >> > > now", and if the user writes on the buffer again later, it will need
> > >> > > to call the dirty IOCTL again. So shouldn't we call
> > >> > > intel_fb_obj_flush(obj, false) here? It seems this was also pointed by
> > >> > > Daniel on the first review. It would be better because it would allow
> > >> > > us to actually keep PSR/FBC enabled.
> > >> >
> > >> > The flush caused by the dumb modeset ioctl is exactly what I want to
> > >> > revert here.
> > >> >
> > >> > Well, I just tested to double check here and flush makes me to miss
> > >> > screen updates. (triple checked with retired = true as well just in
> > >> > case)
> > >> >
> > >> > fbdev comes to place and invalidated frontbuffer, then plymouth does a
> > >> > flush that makes psr start working when it should continue disabled.
> > >> > Continue flushing it doesn't solve the problem, just ratify it.
> > >> >
> > >> > But beside the issue that it is solving I don't believe we want is a
> > >> > flush anyway. There is something writing directly to frontbuffer with no
> > >> > invalidation. The dirty call is supposed to be a damage call that
> > >> > actually tells something on the screen got written and needs to be
> > >> > updated if it hasn't still. In our cause this is exactly the frontbuffer
> > >> > invalidate, not the flush.
> > >> >
> > >> > The flush place would be after it really stopped doing something, but
> > >> > since I don't trust it I prefer to let it invalidated until next flip.
> > >>
> > >> See my review on the first round of this patch: dirtyfb _is_ a flush. If
> > >> it's not enough then there's some other problems still around.
> > >
> > > Well, the flush itself in the way it is defined is to flush frontbuffer
> > > bits and put power saving features back to work. PSR flush for instance
> > > doesn't exit on every flush. Only in the cases that we know HW tracking
> > > doesn't work.
> > >
> > > One possibility would be change PSR to respect that all flush always is
> > > invalidate (exit) + flush fb_bits. But I'm against this since PSR on
> > > core platforms doesn't have SW trackking ext and it wasn't implemented
> > > to be fully enabled/disabled every time.
> > >
> > > Another idea on this line is to make flushs know origins or at least a
> > > boolean to separate cases where flush must be handled as flush bits +
> > > continue working or invalidate + flush bits + start working again
> > >
> > > Please let me know what you think.
> > >
> > > But putting flushs on this dirty callback is currently useless because
> > > flush just tells psr to continue working without getting screen updates.
> > >
> > > Well, this is the most important discussion for now, but also even we
> > > change flush interface or only in PSR I'm not sure this will work.
> > >
> > > This dirty case happens in the middle of fbdev and when it gives control
> > > back to fbcon psr would be working without no one else invalidate or
> > > flushing it and we would miss screen updates anyway.
> > > But in this case I understand this is a hack and if we put the
> > > invalidate there I should also put a FIXME XXX explaining that...
> > 
> > Last week I discussed some of this with Rodrigo on IRC and we have
> > different interpretations on what the frontbuffer tracking calls mean.
> > Daniel, can you please clarify a few things?
> > 
> > One of the things I've discussed with Rodrigo is that I consider
> > invalidate+flush to be equivalent to just a flush call, while Rodrigo
> > sees both cases as different. Daniel, can you please clarify the
> > intentions of the frontbuffer tracking infrastructure here?
> 
> Yeah essentially flush without an invalidate is just a instantaneous
> invalidate+flush. invalidate just means "someone started to frontbuffer
> render and I have no idea at all when that will stop". Flush means
> "someone has done some frontbuffer rendering, make sure those updates land
> on the screen". So yeah Paulo's idea is correct.

Thanks for the clarification...

> 
> Problem is now that when we get a flip we also call flush, and for psr on
> big core that's probably not what we want since it can handle flips
> correctly, but not real frontbuffer flushes.
> 
> > One contradiction I see with the current code is that PSR does nothing
> > on flush() for BDW because it believes the HW tracking, but it does
> > psr_exit() on invalidate for every single case. If it relies on the HW
> > tracking, shouldn't it also do nothing on invalidate()? If it doesn't
> > rely on HW tracking, shouldn't it do something on flush()? If it
> > relies on just a few specific pieces of the HW tracking, shouldn't it
> > check enum fb_op_origin? The conclusion here is that the PSR code is
> > completely relying on having invalidate() calls before the flush() for
> > cases where it needs help from software.
> 
> Calling invalidate before flush isn't imo the right fix since usually
> (except when you have working hw tracking like fbc for gtt mmaps) you need
> to stop psr/fbc/whatever on invalidate. Whereas on flush you just need to
> make sure screen contents get to the screen.
> 
> I think the best option would be to wire up the fb_op_origin for flushes
> too, and then filter out ORIGIN_FLIP for psr (except for sprites on hsw)
> on big core.

Ok, this approach here works here when I have splash screen taking place
and X coming right after it. However we still have one issue that is
when it goes back to console.

fbdev on set_par invalidated front buffer, but then splash screen came
and flushed it, psr started working, then control gets back to fbdev and
nothing invalidates more so from this point on we miss many (if not all)
screen updates.

So we are going to a endless path where we could have many cases where
fbdev will be flushed with no reliable way to get invalidated again.

So, right now I just see 3 paths:
1. invalidate instead of flush on fb dirty callback just to make sure
everything gets invalidated when using dumb ioctls
2. change fbdev and all its clients to make a proper use of a dirty.
3. change fbdev, fbcon to propagate console switch (KD_TEXT info) where
we would have a way to know exactly who is in control
4. Introdoce IOCTLs for PSR and enable/disable it on DDX when we know
DDX has control.

I really don't like the idea to change fbdev scructures and its
clients...

Please let me know if you have other/better ideas and also what path you
prefer...


Thanks,
Rodrigo.
>  
> > Also, we've been discussing a boot splash screen, and, according to
> > what you said, it's supposed to be using the dumb interfaces, right?
> > So I see that the dumb mmap is just a gem_mmap() to our driver. But
> > PSR is supposed to get disabled after GTT mmaps. Why is it not
> > disabled in this case? That is because we don't get the invalidate
> > call after the mmap, right? If we require an invalidate on the dirty
> > ioctl, it means that PSR is enabled even though it is GTT mmapped...
> > That's supposed to be wrong, right? Shouldn't the mmap IOCTL force an
> > invalidate?
> 
> Yeah dumb mmap users are supposed to call dirtyfb after each draw call.
> 
> i915 userspace could start doing the same to mark the gtt mmap invalidate
> as done since we don't have any point where we stop it right now with an
> i915 ioctl. And to be able to work together with set_domain(GTT) it really
> needs to be a flush in ->dirty()
> -Daniel

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Make fb user dirty operation to invalidate frontbuffer

[Daniel Vetter]

On Mon, Jul 06, 2015 at 11:19:03PM +0000, Vivi, Rodrigo wrote:
> On Mon, 2015-07-06 at 19:56 +0200, Daniel Vetter wrote:
> > On Mon, Jul 06, 2015 at 02:11:55PM -0300, Paulo Zanoni wrote:
> > > 2015-07-06 13:43 GMT-03:00 Vivi, Rodrigo <rodrigo.vivi@intel.com>:
> > > > On Fri, 2015-07-03 at 09:10 +0200, Daniel Vetter wrote:
> > > >> On Thu, Jul 02, 2015 at 04:41:32PM +0000, Vivi, Rodrigo wrote:
> > > >> > On Thu, 2015-07-02 at 13:03 -0300, Paulo Zanoni wrote:
> > > >> > > 2015-06-30 20:42 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> > > >> > > > Let's do a frontbuffer invalidation 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.
> > > >> > > >
> > > >> > > > Plymoth will do a modeset over ioctl that flushes frontbuffer
> > > >> > > > tracking and PSR gets back to work while it cannot track the
> > > >> > > > screen updates and exit properly. However plymouth also uses
> > > >> > > > a dirtyfb ioctl whenever updating the screen. So let's use it
> > > >> > > > to invalidate PSR back again.
> > > >> > > >
> > > >> > > > 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.
> > > >> > > >
> > > >> > > > 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 724b0e3..b55b1b6 100644
> > > >> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > >> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > >> > > > @@ -14393,9 +14393,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)
> > > >> > >
> > > >> > > You don't need the white space on the lines above, just the tabs.
> > > >> > >
> > > >> > > > +{
> > > >> > > > +       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_invalidate(obj, ORIGIN_GTT);
> > > >> > >
> > > >> > > As far as I understood from my brief investigation, the dirty IOCTL
> > > >> > > just says "hey, I'm done writing on the memory, you can flush things
> > > >> > > now", and if the user writes on the buffer again later, it will need
> > > >> > > to call the dirty IOCTL again. So shouldn't we call
> > > >> > > intel_fb_obj_flush(obj, false) here? It seems this was also pointed by
> > > >> > > Daniel on the first review. It would be better because it would allow
> > > >> > > us to actually keep PSR/FBC enabled.
> > > >> >
> > > >> > The flush caused by the dumb modeset ioctl is exactly what I want to
> > > >> > revert here.
> > > >> >
> > > >> > Well, I just tested to double check here and flush makes me to miss
> > > >> > screen updates. (triple checked with retired = true as well just in
> > > >> > case)
> > > >> >
> > > >> > fbdev comes to place and invalidated frontbuffer, then plymouth does a
> > > >> > flush that makes psr start working when it should continue disabled.
> > > >> > Continue flushing it doesn't solve the problem, just ratify it.
> > > >> >
> > > >> > But beside the issue that it is solving I don't believe we want is a
> > > >> > flush anyway. There is something writing directly to frontbuffer with no
> > > >> > invalidation. The dirty call is supposed to be a damage call that
> > > >> > actually tells something on the screen got written and needs to be
> > > >> > updated if it hasn't still. In our cause this is exactly the frontbuffer
> > > >> > invalidate, not the flush.
> > > >> >
> > > >> > The flush place would be after it really stopped doing something, but
> > > >> > since I don't trust it I prefer to let it invalidated until next flip.
> > > >>
> > > >> See my review on the first round of this patch: dirtyfb _is_ a flush. If
> > > >> it's not enough then there's some other problems still around.
> > > >
> > > > Well, the flush itself in the way it is defined is to flush frontbuffer
> > > > bits and put power saving features back to work. PSR flush for instance
> > > > doesn't exit on every flush. Only in the cases that we know HW tracking
> > > > doesn't work.
> > > >
> > > > One possibility would be change PSR to respect that all flush always is
> > > > invalidate (exit) + flush fb_bits. But I'm against this since PSR on
> > > > core platforms doesn't have SW trackking ext and it wasn't implemented
> > > > to be fully enabled/disabled every time.
> > > >
> > > > Another idea on this line is to make flushs know origins or at least a
> > > > boolean to separate cases where flush must be handled as flush bits +
> > > > continue working or invalidate + flush bits + start working again
> > > >
> > > > Please let me know what you think.
> > > >
> > > > But putting flushs on this dirty callback is currently useless because
> > > > flush just tells psr to continue working without getting screen updates.
> > > >
> > > > Well, this is the most important discussion for now, but also even we
> > > > change flush interface or only in PSR I'm not sure this will work.
> > > >
> > > > This dirty case happens in the middle of fbdev and when it gives control
> > > > back to fbcon psr would be working without no one else invalidate or
> > > > flushing it and we would miss screen updates anyway.
> > > > But in this case I understand this is a hack and if we put the
> > > > invalidate there I should also put a FIXME XXX explaining that...
> > > 
> > > Last week I discussed some of this with Rodrigo on IRC and we have
> > > different interpretations on what the frontbuffer tracking calls mean.
> > > Daniel, can you please clarify a few things?
> > > 
> > > One of the things I've discussed with Rodrigo is that I consider
> > > invalidate+flush to be equivalent to just a flush call, while Rodrigo
> > > sees both cases as different. Daniel, can you please clarify the
> > > intentions of the frontbuffer tracking infrastructure here?
> > 
> > Yeah essentially flush without an invalidate is just a instantaneous
> > invalidate+flush. invalidate just means "someone started to frontbuffer
> > render and I have no idea at all when that will stop". Flush means
> > "someone has done some frontbuffer rendering, make sure those updates land
> > on the screen". So yeah Paulo's idea is correct.
> 
> Thanks for the clarification...
> 
> > 
> > Problem is now that when we get a flip we also call flush, and for psr on
> > big core that's probably not what we want since it can handle flips
> > correctly, but not real frontbuffer flushes.
> > 
> > > One contradiction I see with the current code is that PSR does nothing
> > > on flush() for BDW because it believes the HW tracking, but it does
> > > psr_exit() on invalidate for every single case. If it relies on the HW
> > > tracking, shouldn't it also do nothing on invalidate()? If it doesn't
> > > rely on HW tracking, shouldn't it do something on flush()? If it
> > > relies on just a few specific pieces of the HW tracking, shouldn't it
> > > check enum fb_op_origin? The conclusion here is that the PSR code is
> > > completely relying on having invalidate() calls before the flush() for
> > > cases where it needs help from software.
> > 
> > Calling invalidate before flush isn't imo the right fix since usually
> > (except when you have working hw tracking like fbc for gtt mmaps) you need
> > to stop psr/fbc/whatever on invalidate. Whereas on flush you just need to
> > make sure screen contents get to the screen.
> > 
> > I think the best option would be to wire up the fb_op_origin for flushes
> > too, and then filter out ORIGIN_FLIP for psr (except for sprites on hsw)
> > on big core.
> 
> Ok, this approach here works here when I have splash screen taking place
> and X coming right after it. However we still have one issue that is
> when it goes back to console.
> 
> fbdev on set_par invalidated front buffer, but then splash screen came
> and flushed it, psr started working, then control gets back to fbdev and
> nothing invalidates more so from this point on we miss many (if not all)
> screen updates.
> 
> So we are going to a endless path where we could have many cases where
> fbdev will be flushed with no reliable way to get invalidated again.
> 
> So, right now I just see 3 paths:
> 1. invalidate instead of flush on fb dirty callback just to make sure
> everything gets invalidated when using dumb ioctls
> 2. change fbdev and all its clients to make a proper use of a dirty.
> 3. change fbdev, fbcon to propagate console switch (KD_TEXT info) where
> we would have a way to know exactly who is in control
> 4. Introdoce IOCTLs for PSR and enable/disable it on DDX when we know
> DDX has control.
> 
> I really don't like the idea to change fbdev scructures and its
> clients...
> 
> Please let me know if you have other/better ideas and also what path you
> prefer...

As I mentioned we're not the only ones with exactly this problem, udl &
qxl have the same need for calling dirtyfb all over the place from fbdev
hooks. qxl seems to have the more complete solution using a workqueue.

What I'm still wondering about though is why we don't get an fbdev
invalidate when switching back to the console. boot splash should be doing
it's drawing on its own buffer, fbcon would switch back (which means a
set_par call) and that should invalidate. I have no idea why we don't get
this call in your setup ...
-Daniel
-- 
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

Re: [PATCH 1/3] drm/i915: Make fb user dirty operation to invalidate frontbuffer

[Vivi, Rodrigo]

On Tue, 2015-07-07 at 13:24 +0200, Daniel Vetter wrote:
> On Mon, Jul 06, 2015 at 11:19:03PM +0000, Vivi, Rodrigo wrote:
> > On Mon, 2015-07-06 at 19:56 +0200, Daniel Vetter wrote:
> > > On Mon, Jul 06, 2015 at 02:11:55PM -0300, Paulo Zanoni wrote:
> > > > 2015-07-06 13:43 GMT-03:00 Vivi, Rodrigo <rodrigo.vivi@intel.com>:
> > > > > On Fri, 2015-07-03 at 09:10 +0200, Daniel Vetter wrote:
> > > > >> On Thu, Jul 02, 2015 at 04:41:32PM +0000, Vivi, Rodrigo wrote:
> > > > >> > On Thu, 2015-07-02 at 13:03 -0300, Paulo Zanoni wrote:
> > > > >> > > 2015-06-30 20:42 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> > > > >> > > > Let's do a frontbuffer invalidation 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.
> > > > >> > > >
> > > > >> > > > Plymoth will do a modeset over ioctl that flushes frontbuffer
> > > > >> > > > tracking and PSR gets back to work while it cannot track the
> > > > >> > > > screen updates and exit properly. However plymouth also uses
> > > > >> > > > a dirtyfb ioctl whenever updating the screen. So let's use it
> > > > >> > > > to invalidate PSR back again.
> > > > >> > > >
> > > > >> > > > 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.
> > > > >> > > >
> > > > >> > > > 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 724b0e3..b55b1b6 100644
> > > > >> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > >> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > >> > > > @@ -14393,9 +14393,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)
> > > > >> > >
> > > > >> > > You don't need the white space on the lines above, just the tabs.
> > > > >> > >
> > > > >> > > > +{
> > > > >> > > > +       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_invalidate(obj, ORIGIN_GTT);
> > > > >> > >
> > > > >> > > As far as I understood from my brief investigation, the dirty IOCTL
> > > > >> > > just says "hey, I'm done writing on the memory, you can flush things
> > > > >> > > now", and if the user writes on the buffer again later, it will need
> > > > >> > > to call the dirty IOCTL again. So shouldn't we call
> > > > >> > > intel_fb_obj_flush(obj, false) here? It seems this was also pointed by
> > > > >> > > Daniel on the first review. It would be better because it would allow
> > > > >> > > us to actually keep PSR/FBC enabled.
> > > > >> >
> > > > >> > The flush caused by the dumb modeset ioctl is exactly what I want to
> > > > >> > revert here.
> > > > >> >
> > > > >> > Well, I just tested to double check here and flush makes me to miss
> > > > >> > screen updates. (triple checked with retired = true as well just in
> > > > >> > case)
> > > > >> >
> > > > >> > fbdev comes to place and invalidated frontbuffer, then plymouth does a
> > > > >> > flush that makes psr start working when it should continue disabled.
> > > > >> > Continue flushing it doesn't solve the problem, just ratify it.
> > > > >> >
> > > > >> > But beside the issue that it is solving I don't believe we want is a
> > > > >> > flush anyway. There is something writing directly to frontbuffer with no
> > > > >> > invalidation. The dirty call is supposed to be a damage call that
> > > > >> > actually tells something on the screen got written and needs to be
> > > > >> > updated if it hasn't still. In our cause this is exactly the frontbuffer
> > > > >> > invalidate, not the flush.
> > > > >> >
> > > > >> > The flush place would be after it really stopped doing something, but
> > > > >> > since I don't trust it I prefer to let it invalidated until next flip.
> > > > >>
> > > > >> See my review on the first round of this patch: dirtyfb _is_ a flush. If
> > > > >> it's not enough then there's some other problems still around.
> > > > >
> > > > > Well, the flush itself in the way it is defined is to flush frontbuffer
> > > > > bits and put power saving features back to work. PSR flush for instance
> > > > > doesn't exit on every flush. Only in the cases that we know HW tracking
> > > > > doesn't work.
> > > > >
> > > > > One possibility would be change PSR to respect that all flush always is
> > > > > invalidate (exit) + flush fb_bits. But I'm against this since PSR on
> > > > > core platforms doesn't have SW trackking ext and it wasn't implemented
> > > > > to be fully enabled/disabled every time.
> > > > >
> > > > > Another idea on this line is to make flushs know origins or at least a
> > > > > boolean to separate cases where flush must be handled as flush bits +
> > > > > continue working or invalidate + flush bits + start working again
> > > > >
> > > > > Please let me know what you think.
> > > > >
> > > > > But putting flushs on this dirty callback is currently useless because
> > > > > flush just tells psr to continue working without getting screen updates.
> > > > >
> > > > > Well, this is the most important discussion for now, but also even we
> > > > > change flush interface or only in PSR I'm not sure this will work.
> > > > >
> > > > > This dirty case happens in the middle of fbdev and when it gives control
> > > > > back to fbcon psr would be working without no one else invalidate or
> > > > > flushing it and we would miss screen updates anyway.
> > > > > But in this case I understand this is a hack and if we put the
> > > > > invalidate there I should also put a FIXME XXX explaining that...
> > > > 
> > > > Last week I discussed some of this with Rodrigo on IRC and we have
> > > > different interpretations on what the frontbuffer tracking calls mean.
> > > > Daniel, can you please clarify a few things?
> > > > 
> > > > One of the things I've discussed with Rodrigo is that I consider
> > > > invalidate+flush to be equivalent to just a flush call, while Rodrigo
> > > > sees both cases as different. Daniel, can you please clarify the
> > > > intentions of the frontbuffer tracking infrastructure here?
> > > 
> > > Yeah essentially flush without an invalidate is just a instantaneous
> > > invalidate+flush. invalidate just means "someone started to frontbuffer
> > > render and I have no idea at all when that will stop". Flush means
> > > "someone has done some frontbuffer rendering, make sure those updates land
> > > on the screen". So yeah Paulo's idea is correct.
> > 
> > Thanks for the clarification...
> > 
> > > 
> > > Problem is now that when we get a flip we also call flush, and for psr on
> > > big core that's probably not what we want since it can handle flips
> > > correctly, but not real frontbuffer flushes.
> > > 
> > > > One contradiction I see with the current code is that PSR does nothing
> > > > on flush() for BDW because it believes the HW tracking, but it does
> > > > psr_exit() on invalidate for every single case. If it relies on the HW
> > > > tracking, shouldn't it also do nothing on invalidate()? If it doesn't
> > > > rely on HW tracking, shouldn't it do something on flush()? If it
> > > > relies on just a few specific pieces of the HW tracking, shouldn't it
> > > > check enum fb_op_origin? The conclusion here is that the PSR code is
> > > > completely relying on having invalidate() calls before the flush() for
> > > > cases where it needs help from software.
> > > 
> > > Calling invalidate before flush isn't imo the right fix since usually
> > > (except when you have working hw tracking like fbc for gtt mmaps) you need
> > > to stop psr/fbc/whatever on invalidate. Whereas on flush you just need to
> > > make sure screen contents get to the screen.
> > > 
> > > I think the best option would be to wire up the fb_op_origin for flushes
> > > too, and then filter out ORIGIN_FLIP for psr (except for sprites on hsw)
> > > on big core.
> > 
> > Ok, this approach here works here when I have splash screen taking place
> > and X coming right after it. However we still have one issue that is
> > when it goes back to console.
> > 
> > fbdev on set_par invalidated front buffer, but then splash screen came
> > and flushed it, psr started working, then control gets back to fbdev and
> > nothing invalidates more so from this point on we miss many (if not all)
> > screen updates.
> > 
> > So we are going to a endless path where we could have many cases where
> > fbdev will be flushed with no reliable way to get invalidated again.
> > 
> > So, right now I just see 3 paths:
> > 1. invalidate instead of flush on fb dirty callback just to make sure
> > everything gets invalidated when using dumb ioctls
> > 2. change fbdev and all its clients to make a proper use of a dirty.
> > 3. change fbdev, fbcon to propagate console switch (KD_TEXT info) where
> > we would have a way to know exactly who is in control
> > 4. Introdoce IOCTLs for PSR and enable/disable it on DDX when we know
> > DDX has control.
> > 
> > I really don't like the idea to change fbdev scructures and its
> > clients...
> > 
> > Please let me know if you have other/better ideas and also what path you
> > prefer...
> 
> As I mentioned we're not the only ones with exactly this problem, udl &
> qxl have the same need for calling dirtyfb all over the place from fbdev
> hooks. qxl seems to have the more complete solution using a workqueue.

Yes, I agree. I just want to block PSR on this fbdev rework that is not
simple, involve many maintainers and it is not the main PSR use case.

> 
> What I'm still wondering about though is why we don't get an fbdev
> invalidate when switching back to the console. boot splash should be doing
> it's drawing on its own buffer, fbcon would switch back (which means a
> set_par call) and that should invalidate. I have no idea why we don't get
> this call in your setup ...

You were right.... I wasn't relying on set_par but it is actually called
on fbcon switch back, but the invalidate was happening only on the first
time because write_domain was already GTT. So with the following patch
we can solve the case where I press "Esc" key on the middle of splash
screen:
http://cgit.freedesktop.org/~vivijim/drm-intel/commit/?h=psr&id=ce7226054085080c10e53b82743a76e0593b785a


But I was facing also another issue with crypto pass and without any
splash and trace lead me to fbdev_restore_mode so another hack:
http://cgit.freedesktop.org/~vivijim/drm-intel/commit/?h=psr&id=9a7c3f7853a327a82551e335b6056b5e5362004a


All (an more) in
http://cgit.freedesktop.org/~vivijim/drm-intel/log/?h=psr
for now... while I'm not sure a flicker on fedora reported by community
is not reliably solved:

https://bugzilla.redhat.com/show_bug.cgi?id=1236303

Thanks,
Rodrigo.





> -Daniel

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Make fb user dirty operation to invalidate frontbuffer

[Daniel Vetter]

On Tue, Jul 07, 2015 at 08:23:46PM +0000, Vivi, Rodrigo wrote:
> On Tue, 2015-07-07 at 13:24 +0200, Daniel Vetter wrote:
> > On Mon, Jul 06, 2015 at 11:19:03PM +0000, Vivi, Rodrigo wrote:
> > > On Mon, 2015-07-06 at 19:56 +0200, Daniel Vetter wrote:
> > > > On Mon, Jul 06, 2015 at 02:11:55PM -0300, Paulo Zanoni wrote:
> > > > > 2015-07-06 13:43 GMT-03:00 Vivi, Rodrigo <rodrigo.vivi@intel.com>:
> > > > > > On Fri, 2015-07-03 at 09:10 +0200, Daniel Vetter wrote:
> > > > > >> On Thu, Jul 02, 2015 at 04:41:32PM +0000, Vivi, Rodrigo wrote:
> > > > > >> > On Thu, 2015-07-02 at 13:03 -0300, Paulo Zanoni wrote:
> > > > > >> > > 2015-06-30 20:42 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> > > > > >> > > > Let's do a frontbuffer invalidation 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.
> > > > > >> > > >
> > > > > >> > > > Plymoth will do a modeset over ioctl that flushes frontbuffer
> > > > > >> > > > tracking and PSR gets back to work while it cannot track the
> > > > > >> > > > screen updates and exit properly. However plymouth also uses
> > > > > >> > > > a dirtyfb ioctl whenever updating the screen. So let's use it
> > > > > >> > > > to invalidate PSR back again.
> > > > > >> > > >
> > > > > >> > > > 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.
> > > > > >> > > >
> > > > > >> > > > 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 724b0e3..b55b1b6 100644
> > > > > >> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > >> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > >> > > > @@ -14393,9 +14393,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)
> > > > > >> > >
> > > > > >> > > You don't need the white space on the lines above, just the tabs.
> > > > > >> > >
> > > > > >> > > > +{
> > > > > >> > > > +       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_invalidate(obj, ORIGIN_GTT);
> > > > > >> > >
> > > > > >> > > As far as I understood from my brief investigation, the dirty IOCTL
> > > > > >> > > just says "hey, I'm done writing on the memory, you can flush things
> > > > > >> > > now", and if the user writes on the buffer again later, it will need
> > > > > >> > > to call the dirty IOCTL again. So shouldn't we call
> > > > > >> > > intel_fb_obj_flush(obj, false) here? It seems this was also pointed by
> > > > > >> > > Daniel on the first review. It would be better because it would allow
> > > > > >> > > us to actually keep PSR/FBC enabled.
> > > > > >> >
> > > > > >> > The flush caused by the dumb modeset ioctl is exactly what I want to
> > > > > >> > revert here.
> > > > > >> >
> > > > > >> > Well, I just tested to double check here and flush makes me to miss
> > > > > >> > screen updates. (triple checked with retired = true as well just in
> > > > > >> > case)
> > > > > >> >
> > > > > >> > fbdev comes to place and invalidated frontbuffer, then plymouth does a
> > > > > >> > flush that makes psr start working when it should continue disabled.
> > > > > >> > Continue flushing it doesn't solve the problem, just ratify it.
> > > > > >> >
> > > > > >> > But beside the issue that it is solving I don't believe we want is a
> > > > > >> > flush anyway. There is something writing directly to frontbuffer with no
> > > > > >> > invalidation. The dirty call is supposed to be a damage call that
> > > > > >> > actually tells something on the screen got written and needs to be
> > > > > >> > updated if it hasn't still. In our cause this is exactly the frontbuffer
> > > > > >> > invalidate, not the flush.
> > > > > >> >
> > > > > >> > The flush place would be after it really stopped doing something, but
> > > > > >> > since I don't trust it I prefer to let it invalidated until next flip.
> > > > > >>
> > > > > >> See my review on the first round of this patch: dirtyfb _is_ a flush. If
> > > > > >> it's not enough then there's some other problems still around.
> > > > > >
> > > > > > Well, the flush itself in the way it is defined is to flush frontbuffer
> > > > > > bits and put power saving features back to work. PSR flush for instance
> > > > > > doesn't exit on every flush. Only in the cases that we know HW tracking
> > > > > > doesn't work.
> > > > > >
> > > > > > One possibility would be change PSR to respect that all flush always is
> > > > > > invalidate (exit) + flush fb_bits. But I'm against this since PSR on
> > > > > > core platforms doesn't have SW trackking ext and it wasn't implemented
> > > > > > to be fully enabled/disabled every time.
> > > > > >
> > > > > > Another idea on this line is to make flushs know origins or at least a
> > > > > > boolean to separate cases where flush must be handled as flush bits +
> > > > > > continue working or invalidate + flush bits + start working again
> > > > > >
> > > > > > Please let me know what you think.
> > > > > >
> > > > > > But putting flushs on this dirty callback is currently useless because
> > > > > > flush just tells psr to continue working without getting screen updates.
> > > > > >
> > > > > > Well, this is the most important discussion for now, but also even we
> > > > > > change flush interface or only in PSR I'm not sure this will work.
> > > > > >
> > > > > > This dirty case happens in the middle of fbdev and when it gives control
> > > > > > back to fbcon psr would be working without no one else invalidate or
> > > > > > flushing it and we would miss screen updates anyway.
> > > > > > But in this case I understand this is a hack and if we put the
> > > > > > invalidate there I should also put a FIXME XXX explaining that...
> > > > > 
> > > > > Last week I discussed some of this with Rodrigo on IRC and we have
> > > > > different interpretations on what the frontbuffer tracking calls mean.
> > > > > Daniel, can you please clarify a few things?
> > > > > 
> > > > > One of the things I've discussed with Rodrigo is that I consider
> > > > > invalidate+flush to be equivalent to just a flush call, while Rodrigo
> > > > > sees both cases as different. Daniel, can you please clarify the
> > > > > intentions of the frontbuffer tracking infrastructure here?
> > > > 
> > > > Yeah essentially flush without an invalidate is just a instantaneous
> > > > invalidate+flush. invalidate just means "someone started to frontbuffer
> > > > render and I have no idea at all when that will stop". Flush means
> > > > "someone has done some frontbuffer rendering, make sure those updates land
> > > > on the screen". So yeah Paulo's idea is correct.
> > > 
> > > Thanks for the clarification...
> > > 
> > > > 
> > > > Problem is now that when we get a flip we also call flush, and for psr on
> > > > big core that's probably not what we want since it can handle flips
> > > > correctly, but not real frontbuffer flushes.
> > > > 
> > > > > One contradiction I see with the current code is that PSR does nothing
> > > > > on flush() for BDW because it believes the HW tracking, but it does
> > > > > psr_exit() on invalidate for every single case. If it relies on the HW
> > > > > tracking, shouldn't it also do nothing on invalidate()? If it doesn't
> > > > > rely on HW tracking, shouldn't it do something on flush()? If it
> > > > > relies on just a few specific pieces of the HW tracking, shouldn't it
> > > > > check enum fb_op_origin? The conclusion here is that the PSR code is
> > > > > completely relying on having invalidate() calls before the flush() for
> > > > > cases where it needs help from software.
> > > > 
> > > > Calling invalidate before flush isn't imo the right fix since usually
> > > > (except when you have working hw tracking like fbc for gtt mmaps) you need
> > > > to stop psr/fbc/whatever on invalidate. Whereas on flush you just need to
> > > > make sure screen contents get to the screen.
> > > > 
> > > > I think the best option would be to wire up the fb_op_origin for flushes
> > > > too, and then filter out ORIGIN_FLIP for psr (except for sprites on hsw)
> > > > on big core.
> > > 
> > > Ok, this approach here works here when I have splash screen taking place
> > > and X coming right after it. However we still have one issue that is
> > > when it goes back to console.
> > > 
> > > fbdev on set_par invalidated front buffer, but then splash screen came
> > > and flushed it, psr started working, then control gets back to fbdev and
> > > nothing invalidates more so from this point on we miss many (if not all)
> > > screen updates.
> > > 
> > > So we are going to a endless path where we could have many cases where
> > > fbdev will be flushed with no reliable way to get invalidated again.
> > > 
> > > So, right now I just see 3 paths:
> > > 1. invalidate instead of flush on fb dirty callback just to make sure
> > > everything gets invalidated when using dumb ioctls
> > > 2. change fbdev and all its clients to make a proper use of a dirty.
> > > 3. change fbdev, fbcon to propagate console switch (KD_TEXT info) where
> > > we would have a way to know exactly who is in control
> > > 4. Introdoce IOCTLs for PSR and enable/disable it on DDX when we know
> > > DDX has control.
> > > 
> > > I really don't like the idea to change fbdev scructures and its
> > > clients...
> > > 
> > > Please let me know if you have other/better ideas and also what path you
> > > prefer...
> > 
> > As I mentioned we're not the only ones with exactly this problem, udl &
> > qxl have the same need for calling dirtyfb all over the place from fbdev
> > hooks. qxl seems to have the more complete solution using a workqueue.
> 
> Yes, I agree. I just want to block PSR on this fbdev rework that is not
> simple, involve many maintainers and it is not the main PSR use case.
> 
> > 
> > What I'm still wondering about though is why we don't get an fbdev
> > invalidate when switching back to the console. boot splash should be doing
> > it's drawing on its own buffer, fbcon would switch back (which means a
> > set_par call) and that should invalidate. I have no idea why we don't get
> > this call in your setup ...
> 
> You were right.... I wasn't relying on set_par but it is actually called
> on fbcon switch back, but the invalidate was happening only on the first
> time because write_domain was already GTT. So with the following patch
> we can solve the case where I press "Esc" key on the middle of splash
> screen:
> http://cgit.freedesktop.org/~vivijim/drm-intel/commit/?h=psr&id=ce7226054085080c10e53b82743a76e0593b785a

Oops I've accidentally broken this even worse with 

commit 031b698a77a70a6c394568034437b5486a44e868
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Fri Jun 26 19:35:16 2015 +0200

    drm/i915: Unconditionally do fb tracking invalidate in set_domain

This commit fixes this exact bug (already in gtt write domain), but
unfortunately only for the set_domain ioctl, not for fbcon.

We really just need an fb invalidate in there, no need for the if.

> But I was facing also another issue with crypto pass and without any
> splash and trace lead me to fbdev_restore_mode so another hack:
> http://cgit.freedesktop.org/~vivijim/drm-intel/commit/?h=psr&id=9a7c3f7853a327a82551e335b6056b5e5362004a

Excellent catch, we definitely need that one too.
-Daniel

> All (an more) in
> http://cgit.freedesktop.org/~vivijim/drm-intel/log/?h=psr
> for now... while I'm not sure a flicker on fedora reported by community
> is not reliably solved:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1236303
> 
> Thanks,
> Rodrigo.
> 
> 
> 
> 
> 
> > -Daniel
> 

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

[PATCH 1/3] drm/i915: PSR also doesn't have link_entry_time on SKL.

[Rodrigo Vivi]

This bit is also reserved on Skylake. Actually the only
platform that supports this is Haswell, so let's fix
this logic and apply this link entry time only for the
platform that supports it, i.e. Haswell.

This also changes the style to let more clear platform
differences outside the reg write. We would probably catch
this case sooner if separated, or not...

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 14cc2cf..9ccff30 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -276,10 +276,11 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp)
 	 */
 	uint32_t idle_frames = max(6, dev_priv->vbt.psr.idle_frames);
 	uint32_t val = 0x0;
-	const uint32_t link_entry_time = EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
+
+	if (IS_HASWELL(dev))
+		val |= EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
 
 	I915_WRITE(EDP_PSR_CTL, val |
-		   (IS_BROADWELL(dev) ? 0 : link_entry_time) |
 		   max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT |
 		   idle_frames << EDP_PSR_IDLE_FRAME_SHIFT |
 		   EDP_PSR_ENABLE);
-- 
2.4.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/3] drm/i915: Enable PSR by default.

[Rodrigo Vivi]

With a reliable frontbuffer tracking and all instability corner cases
solved let's re-enabled PSR by default on all supported platforms.

v2: Rebased on top of psr paramenter change.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_params.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index def8802..7dafda8 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -37,7 +37,7 @@ struct i915_params i915 __read_mostly = {
 	.enable_execlists = -1,
 	.enable_hangcheck = true,
 	.enable_ppgtt = -1,
-	.enable_psr = 0,
+	.enable_psr = 1,
 	.preliminary_hw_support = IS_ENABLED(CONFIG_DRM_I915_PRELIMINARY_HW_SUPPORT),
 	.disable_power_well = -1,
 	.enable_ips = 1,
@@ -127,7 +127,7 @@ MODULE_PARM_DESC(enable_execlists,
 
 module_param_named_unsafe(enable_psr, i915.enable_psr, int, 0600);
 MODULE_PARM_DESC(enable_psr, "Enable PSR "
-		 "(-1=force link standby for debug, 0=disabled [default], 1=enabled)"
+		 "(-1=force link standby for debug, 0=disabled, 1=enabled [default])"
 		 "In case you needed to force debug mode, please "
 		 "report PCI device ID, subsystem vendor and subsystem device ID "
 		 "to intel-gfx@lists.freedesktop.org, if your machine needs it. "
-- 
2.4.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx