Re: [Intel-gfx] [PATCH v2 2/3] drm/i915/frontbuffer: HW tracking for cursor moves to fix PSR lags.

2018-03-08 Thread Rodrigo Vivi
On Wed, Mar 07, 2018 at 04:21:53PM -0800, Pandiyan, Dhinakaran wrote:
> On Wed, 2018-03-07 at 15:43 -0800, Rodrigo Vivi wrote:
> > On Wed, Mar 07, 2018 at 03:23:13PM -0800, Rodrigo Vivi wrote:
> > > On Wed, Mar 07, 2018 at 11:10:35PM +, Pandiyan, Dhinakaran wrote:
> > > > On Wed, 2018-03-07 at 22:53 +, Chris Wilson wrote:
> > > > > Quoting Dhinakaran Pandiyan (2018-03-07 03:34:19)
> > > > > > DRM_IOCTL_MODE_CURSOR results in frontbuffer flush before the cursor
> > > > > > plane MMIOs are written to. But this flush should not be necessary 
> > > > > > for
> > > > > > PSR as hardware tracking triggers PSR exit when MMIOs are written. 
> > > > > > As
> > > > > > for FBC, the spec says "Flips or changes to plane size and panning" 
> > > > > > cause
> > > > > > FBC to be nuked. Use origin == ORIGIN_FLIP so that features can 
> > > > > > ignore
> > > > > > cursor updates in their frontbuffer_flush implementations.
> > > > > > 
> > > > > >  /sys/kernel/debug/dri/0/i915_fbc_status shows
> > > > > > "Compressing: yes" when I move the cursor around.
> > > > > > 
> > > > > > v3: Use ORIGIN_FLIP now that pin_to_display does not flush 
> > > > > > frontbuffer.
> > > > > > v2: Update comment in i915_gem_object_pin_to_display_plane. (Chris)
> > > > > > 
> > > > > > Cc: Paulo Zanoni 
> > > > > > Cc: Ville Syrjälä 
> > > > > > Cc: Chris Wilson 
> > > > > > Cc: Rodrigo Vivi 
> > > > > > Signed-off-by: Dhinakaran Pandiyan 
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/intel_display.c | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c 
> > > > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > > > index 91ce8a0522a3..18b08e263ee1 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > > @@ -13176,7 +13176,7 @@ intel_legacy_cursor_update(struct drm_plane 
> > > > > > *plane,
> > > > > > if (ret)
> > > > > > goto out_unlock;
> > > > > >  
> > > > > > -   intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_DIRTYFB);
> > > > > > +   intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_FLIP);
> > > > > 
> > > > > What about prepare_plane? That should reduce to ORIGIN_FLIP as well,
> > > > > aiui.
> > > > > -Chris
> > > > 
> > > > 
> > > > That was the idea but there's a problem with not knowing if PSR exit is
> > > > fully complete before we begin updating the plane registers in
> > > > pipe_update_start().
> > > > 
> > > > Let's say PSR was active and display is in DC6. A flip comes in, without
> > > > _flush(DIRTYFB) in prepare_plane_fb(), PSR exit is delayed until vblank
> > > > enabling that happens in pipe_update_start. We immediately follow that
> > > > with programming the plane MMIO's without checking if PSR fully exited.
> > > > If PSR and DC6 happen to exit while we were in the middle of programming
> > > > plane MMIO's, the resulting vblank toggle (from PSR exit) might activate
> > > > partially programmed registers. _flush(DIRTYFB) gives us an opportunity
> > > > to exit PSR fully by starting early.
> > > > 
> > > > As for legacy_cursor_update(), since there is no vblank enabling
> > > > involved, we avoid updating the MMIO's in the midst of PSR exit
> > > 
> > > I don't believe you will be ever in a case that you write to any register
> > > and get any flip or anything without exiting DC6 before that happens.
> > > 
> > > Or the CSR mechanism of DC6 will be simply wrong.
> > > 
> > > Would this be enough?
> > 
> > ok... just ignore my previous comment...
> > I believe we can move with the safest side and maybe revisit this later.
> > 
> > From what I remember of the FBC nuke needs as well I believe this is
> > the right move.
> > 
> > Although I'm asking myself now if we are not changing the meaning of the
> > ORIGINS here. Shouldn't we add a new origin and update the handling?
> > 
> > of "flip" is a good description for this call?
> > 
> 
> The action taken for this frontbuffer_flush() is the same as origin ==
> FLIP. I think it is better to add a new one when we want the features
> (psr,fbc) to distinguish and react differently. 

Maybe we could change the names from ORIGIN_something to something more
meaninful about the operation to be performed then...

Anyways I don't want to force increase the number of origins and duplicate
the code on a niptic and I don't have better suggestions for names...

So I guess we move fwd with this for now...


Reviewed-by: Rodrigo Vivi 




> 
> 
> > > 
> > > > 
> > > > 
> > > > > ___
> > > > > Intel-gfx mailing list
> > > > > Intel-gfx@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > ___
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 

Re: [Intel-gfx] [PATCH v2 2/3] drm/i915/frontbuffer: HW tracking for cursor moves to fix PSR lags.

2018-03-08 Thread Pandiyan, Dhinakaran



On Wed, 2018-03-07 at 15:34 -0800, Dhinakaran Pandiyan wrote:
> On Wed, 2018-03-07 at 22:53 +, Chris Wilson wrote:
> > Quoting Dhinakaran Pandiyan (2018-03-07 03:34:19)
> > > DRM_IOCTL_MODE_CURSOR results in frontbuffer flush before the cursor
> > > plane MMIOs are written to. But this flush should not be necessary for
> > > PSR as hardware tracking triggers PSR exit when MMIOs are written. As
> > > for FBC, the spec says "Flips or changes to plane size and panning" cause
> > > FBC to be nuked. Use origin == ORIGIN_FLIP so that features can ignore
> > > cursor updates in their frontbuffer_flush implementations.
> > > 
> > >  /sys/kernel/debug/dri/0/i915_fbc_status shows
> > > "Compressing: yes" when I move the cursor around.
> > > 
> > > v3: Use ORIGIN_FLIP now that pin_to_display does not flush frontbuffer.
> > > v2: Update comment in i915_gem_object_pin_to_display_plane. (Chris)
> > > 
> > > Cc: Paulo Zanoni 
> > > Cc: Ville Syrjälä 
> > > Cc: Chris Wilson 
> > > Cc: Rodrigo Vivi 
> > > Signed-off-by: Dhinakaran Pandiyan 
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c 
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index 91ce8a0522a3..18b08e263ee1 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -13176,7 +13176,7 @@ intel_legacy_cursor_update(struct drm_plane 
> > > *plane,
> > > if (ret)
> > > goto out_unlock;
> > >  
> > > -   intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_DIRTYFB);
> > > +   intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_FLIP);
> > 
> > What about prepare_plane? That should reduce to ORIGIN_FLIP as well,
> > aiui.
> > -Chris
> 
> 
> That was the idea but there's a problem with not knowing if PSR exit is
> fully complete before we begin updating the plane registers in
> pipe_update_start().
> 
> Let's say PSR was active and display is in DC6. A flip comes in, without
> _flush(DIRTYFB) in prepare_plane_fb(), PSR exit is delayed until vblank
> enabling that happens in pipe_update_start.

Correction, _flush(DIRTYFB) in prepare_plane_fb() has no effect because
the gem object has no frontbuffer bits set. We should just remove that
misleading call.

>  We immediately follow that
> with programming the plane MMIO's without checking if PSR fully exited.
> If PSR and DC6 happen to exit while we were in the middle of programming
> plane MMIO's, the resulting vblank toggle (from PSR exit) might activate
> partially programmed registers. _flush(DIRTYFB) gives us an opportunity
> to exit PSR fully by starting early.
> 
> As for legacy_cursor_update(), since there is no vblank enabling
> involved, we avoid updating the MMIO's in the midst of PSR exit
> 
> 
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH v2 2/3] drm/i915/frontbuffer: HW tracking for cursor moves to fix PSR lags.

2018-03-07 Thread Pandiyan, Dhinakaran
On Wed, 2018-03-07 at 15:43 -0800, Rodrigo Vivi wrote:
> On Wed, Mar 07, 2018 at 03:23:13PM -0800, Rodrigo Vivi wrote:
> > On Wed, Mar 07, 2018 at 11:10:35PM +, Pandiyan, Dhinakaran wrote:
> > > On Wed, 2018-03-07 at 22:53 +, Chris Wilson wrote:
> > > > Quoting Dhinakaran Pandiyan (2018-03-07 03:34:19)
> > > > > DRM_IOCTL_MODE_CURSOR results in frontbuffer flush before the cursor
> > > > > plane MMIOs are written to. But this flush should not be necessary for
> > > > > PSR as hardware tracking triggers PSR exit when MMIOs are written. As
> > > > > for FBC, the spec says "Flips or changes to plane size and panning" 
> > > > > cause
> > > > > FBC to be nuked. Use origin == ORIGIN_FLIP so that features can ignore
> > > > > cursor updates in their frontbuffer_flush implementations.
> > > > > 
> > > > >  /sys/kernel/debug/dri/0/i915_fbc_status shows
> > > > > "Compressing: yes" when I move the cursor around.
> > > > > 
> > > > > v3: Use ORIGIN_FLIP now that pin_to_display does not flush 
> > > > > frontbuffer.
> > > > > v2: Update comment in i915_gem_object_pin_to_display_plane. (Chris)
> > > > > 
> > > > > Cc: Paulo Zanoni 
> > > > > Cc: Ville Syrjälä 
> > > > > Cc: Chris Wilson 
> > > > > Cc: Rodrigo Vivi 
> > > > > Signed-off-by: Dhinakaran Pandiyan 
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_display.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c 
> > > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > > index 91ce8a0522a3..18b08e263ee1 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > @@ -13176,7 +13176,7 @@ intel_legacy_cursor_update(struct drm_plane 
> > > > > *plane,
> > > > > if (ret)
> > > > > goto out_unlock;
> > > > >  
> > > > > -   intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_DIRTYFB);
> > > > > +   intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_FLIP);
> > > > 
> > > > What about prepare_plane? That should reduce to ORIGIN_FLIP as well,
> > > > aiui.
> > > > -Chris
> > > 
> > > 
> > > That was the idea but there's a problem with not knowing if PSR exit is
> > > fully complete before we begin updating the plane registers in
> > > pipe_update_start().
> > > 
> > > Let's say PSR was active and display is in DC6. A flip comes in, without
> > > _flush(DIRTYFB) in prepare_plane_fb(), PSR exit is delayed until vblank
> > > enabling that happens in pipe_update_start. We immediately follow that
> > > with programming the plane MMIO's without checking if PSR fully exited.
> > > If PSR and DC6 happen to exit while we were in the middle of programming
> > > plane MMIO's, the resulting vblank toggle (from PSR exit) might activate
> > > partially programmed registers. _flush(DIRTYFB) gives us an opportunity
> > > to exit PSR fully by starting early.
> > > 
> > > As for legacy_cursor_update(), since there is no vblank enabling
> > > involved, we avoid updating the MMIO's in the midst of PSR exit
> > 
> > I don't believe you will be ever in a case that you write to any register
> > and get any flip or anything without exiting DC6 before that happens.
> > 
> > Or the CSR mechanism of DC6 will be simply wrong.
> > 
> > Would this be enough?
> 
> ok... just ignore my previous comment...
> I believe we can move with the safest side and maybe revisit this later.
> 
> From what I remember of the FBC nuke needs as well I believe this is
> the right move.
> 
> Although I'm asking myself now if we are not changing the meaning of the
> ORIGINS here. Shouldn't we add a new origin and update the handling?
> 
> of "flip" is a good description for this call?
> 

The action taken for this frontbuffer_flush() is the same as origin ==
FLIP. I think it is better to add a new one when we want the features
(psr,fbc) to distinguish and react differently. 


> > 
> > > 
> > > 
> > > > ___
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > ___
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH v2 2/3] drm/i915/frontbuffer: HW tracking for cursor moves to fix PSR lags.

2018-03-07 Thread Rodrigo Vivi
On Wed, Mar 07, 2018 at 03:23:13PM -0800, Rodrigo Vivi wrote:
> On Wed, Mar 07, 2018 at 11:10:35PM +, Pandiyan, Dhinakaran wrote:
> > On Wed, 2018-03-07 at 22:53 +, Chris Wilson wrote:
> > > Quoting Dhinakaran Pandiyan (2018-03-07 03:34:19)
> > > > DRM_IOCTL_MODE_CURSOR results in frontbuffer flush before the cursor
> > > > plane MMIOs are written to. But this flush should not be necessary for
> > > > PSR as hardware tracking triggers PSR exit when MMIOs are written. As
> > > > for FBC, the spec says "Flips or changes to plane size and panning" 
> > > > cause
> > > > FBC to be nuked. Use origin == ORIGIN_FLIP so that features can ignore
> > > > cursor updates in their frontbuffer_flush implementations.
> > > > 
> > > >  /sys/kernel/debug/dri/0/i915_fbc_status shows
> > > > "Compressing: yes" when I move the cursor around.
> > > > 
> > > > v3: Use ORIGIN_FLIP now that pin_to_display does not flush frontbuffer.
> > > > v2: Update comment in i915_gem_object_pin_to_display_plane. (Chris)
> > > > 
> > > > Cc: Paulo Zanoni 
> > > > Cc: Ville Syrjälä 
> > > > Cc: Chris Wilson 
> > > > Cc: Rodrigo Vivi 
> > > > Signed-off-by: Dhinakaran Pandiyan 
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_display.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c 
> > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > index 91ce8a0522a3..18b08e263ee1 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -13176,7 +13176,7 @@ intel_legacy_cursor_update(struct drm_plane 
> > > > *plane,
> > > > if (ret)
> > > > goto out_unlock;
> > > >  
> > > > -   intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_DIRTYFB);
> > > > +   intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_FLIP);
> > > 
> > > What about prepare_plane? That should reduce to ORIGIN_FLIP as well,
> > > aiui.
> > > -Chris
> > 
> > 
> > That was the idea but there's a problem with not knowing if PSR exit is
> > fully complete before we begin updating the plane registers in
> > pipe_update_start().
> > 
> > Let's say PSR was active and display is in DC6. A flip comes in, without
> > _flush(DIRTYFB) in prepare_plane_fb(), PSR exit is delayed until vblank
> > enabling that happens in pipe_update_start. We immediately follow that
> > with programming the plane MMIO's without checking if PSR fully exited.
> > If PSR and DC6 happen to exit while we were in the middle of programming
> > plane MMIO's, the resulting vblank toggle (from PSR exit) might activate
> > partially programmed registers. _flush(DIRTYFB) gives us an opportunity
> > to exit PSR fully by starting early.
> > 
> > As for legacy_cursor_update(), since there is no vblank enabling
> > involved, we avoid updating the MMIO's in the midst of PSR exit
> 
> I don't believe you will be ever in a case that you write to any register
> and get any flip or anything without exiting DC6 before that happens.
> 
> Or the CSR mechanism of DC6 will be simply wrong.
> 
> Would this be enough?

ok... just ignore my previous comment...
I believe we can move with the safest side and maybe revisit this later.

From what I remember of the FBC nuke needs as well I believe this is
the right move.

Although I'm asking myself now if we are not changing the meaning of the
ORIGINS here. Shouldn't we add a new origin and update the handling?

of "flip" is a good description for this call?

> 
> > 
> > 
> > > ___
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH v2 2/3] drm/i915/frontbuffer: HW tracking for cursor moves to fix PSR lags.

2018-03-07 Thread Rodrigo Vivi
On Wed, Mar 07, 2018 at 11:10:35PM +, Pandiyan, Dhinakaran wrote:
> On Wed, 2018-03-07 at 22:53 +, Chris Wilson wrote:
> > Quoting Dhinakaran Pandiyan (2018-03-07 03:34:19)
> > > DRM_IOCTL_MODE_CURSOR results in frontbuffer flush before the cursor
> > > plane MMIOs are written to. But this flush should not be necessary for
> > > PSR as hardware tracking triggers PSR exit when MMIOs are written. As
> > > for FBC, the spec says "Flips or changes to plane size and panning" cause
> > > FBC to be nuked. Use origin == ORIGIN_FLIP so that features can ignore
> > > cursor updates in their frontbuffer_flush implementations.
> > > 
> > >  /sys/kernel/debug/dri/0/i915_fbc_status shows
> > > "Compressing: yes" when I move the cursor around.
> > > 
> > > v3: Use ORIGIN_FLIP now that pin_to_display does not flush frontbuffer.
> > > v2: Update comment in i915_gem_object_pin_to_display_plane. (Chris)
> > > 
> > > Cc: Paulo Zanoni 
> > > Cc: Ville Syrjälä 
> > > Cc: Chris Wilson 
> > > Cc: Rodrigo Vivi 
> > > Signed-off-by: Dhinakaran Pandiyan 
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c 
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index 91ce8a0522a3..18b08e263ee1 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -13176,7 +13176,7 @@ intel_legacy_cursor_update(struct drm_plane 
> > > *plane,
> > > if (ret)
> > > goto out_unlock;
> > >  
> > > -   intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_DIRTYFB);
> > > +   intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_FLIP);
> > 
> > What about prepare_plane? That should reduce to ORIGIN_FLIP as well,
> > aiui.
> > -Chris
> 
> 
> That was the idea but there's a problem with not knowing if PSR exit is
> fully complete before we begin updating the plane registers in
> pipe_update_start().
> 
> Let's say PSR was active and display is in DC6. A flip comes in, without
> _flush(DIRTYFB) in prepare_plane_fb(), PSR exit is delayed until vblank
> enabling that happens in pipe_update_start. We immediately follow that
> with programming the plane MMIO's without checking if PSR fully exited.
> If PSR and DC6 happen to exit while we were in the middle of programming
> plane MMIO's, the resulting vblank toggle (from PSR exit) might activate
> partially programmed registers. _flush(DIRTYFB) gives us an opportunity
> to exit PSR fully by starting early.
> 
> As for legacy_cursor_update(), since there is no vblank enabling
> involved, we avoid updating the MMIO's in the midst of PSR exit

I don't believe you will be ever in a case that you write to any register
and get any flip or anything without exiting DC6 before that happens.

Or the CSR mechanism of DC6 will be simply wrong.

Would this be enough?

> 
> 
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH v2 2/3] drm/i915/frontbuffer: HW tracking for cursor moves to fix PSR lags.

2018-03-07 Thread Pandiyan, Dhinakaran
On Wed, 2018-03-07 at 22:53 +, Chris Wilson wrote:
> Quoting Dhinakaran Pandiyan (2018-03-07 03:34:19)
> > DRM_IOCTL_MODE_CURSOR results in frontbuffer flush before the cursor
> > plane MMIOs are written to. But this flush should not be necessary for
> > PSR as hardware tracking triggers PSR exit when MMIOs are written. As
> > for FBC, the spec says "Flips or changes to plane size and panning" cause
> > FBC to be nuked. Use origin == ORIGIN_FLIP so that features can ignore
> > cursor updates in their frontbuffer_flush implementations.
> > 
> >  /sys/kernel/debug/dri/0/i915_fbc_status shows
> > "Compressing: yes" when I move the cursor around.
> > 
> > v3: Use ORIGIN_FLIP now that pin_to_display does not flush frontbuffer.
> > v2: Update comment in i915_gem_object_pin_to_display_plane. (Chris)
> > 
> > Cc: Paulo Zanoni 
> > Cc: Ville Syrjälä 
> > Cc: Chris Wilson 
> > Cc: Rodrigo Vivi 
> > Signed-off-by: Dhinakaran Pandiyan 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c 
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 91ce8a0522a3..18b08e263ee1 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -13176,7 +13176,7 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> > if (ret)
> > goto out_unlock;
> >  
> > -   intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_DIRTYFB);
> > +   intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_FLIP);
> 
> What about prepare_plane? That should reduce to ORIGIN_FLIP as well,
> aiui.
> -Chris


That was the idea but there's a problem with not knowing if PSR exit is
fully complete before we begin updating the plane registers in
pipe_update_start().

Let's say PSR was active and display is in DC6. A flip comes in, without
_flush(DIRTYFB) in prepare_plane_fb(), PSR exit is delayed until vblank
enabling that happens in pipe_update_start. We immediately follow that
with programming the plane MMIO's without checking if PSR fully exited.
If PSR and DC6 happen to exit while we were in the middle of programming
plane MMIO's, the resulting vblank toggle (from PSR exit) might activate
partially programmed registers. _flush(DIRTYFB) gives us an opportunity
to exit PSR fully by starting early.

As for legacy_cursor_update(), since there is no vblank enabling
involved, we avoid updating the MMIO's in the midst of PSR exit


> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH v2 2/3] drm/i915/frontbuffer: HW tracking for cursor moves to fix PSR lags.

2018-03-07 Thread Chris Wilson
Quoting Dhinakaran Pandiyan (2018-03-07 03:34:19)
> DRM_IOCTL_MODE_CURSOR results in frontbuffer flush before the cursor
> plane MMIOs are written to. But this flush should not be necessary for
> PSR as hardware tracking triggers PSR exit when MMIOs are written. As
> for FBC, the spec says "Flips or changes to plane size and panning" cause
> FBC to be nuked. Use origin == ORIGIN_FLIP so that features can ignore
> cursor updates in their frontbuffer_flush implementations.
> 
>  /sys/kernel/debug/dri/0/i915_fbc_status shows
> "Compressing: yes" when I move the cursor around.
> 
> v3: Use ORIGIN_FLIP now that pin_to_display does not flush frontbuffer.
> v2: Update comment in i915_gem_object_pin_to_display_plane. (Chris)
> 
> Cc: Paulo Zanoni 
> Cc: Ville Syrjälä 
> Cc: Chris Wilson 
> Cc: Rodrigo Vivi 
> Signed-off-by: Dhinakaran Pandiyan 
> ---
>  drivers/gpu/drm/i915/intel_display.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index 91ce8a0522a3..18b08e263ee1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13176,7 +13176,7 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> if (ret)
> goto out_unlock;
>  
> -   intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_DIRTYFB);
> +   intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_FLIP);

What about prepare_plane? That should reduce to ORIGIN_FLIP as well,
aiui.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


[Intel-gfx] [PATCH v2 2/3] drm/i915/frontbuffer: HW tracking for cursor moves to fix PSR lags.

2018-03-06 Thread Dhinakaran Pandiyan
DRM_IOCTL_MODE_CURSOR results in frontbuffer flush before the cursor
plane MMIOs are written to. But this flush should not be necessary for
PSR as hardware tracking triggers PSR exit when MMIOs are written. As
for FBC, the spec says "Flips or changes to plane size and panning" cause
FBC to be nuked. Use origin == ORIGIN_FLIP so that features can ignore
cursor updates in their frontbuffer_flush implementations.

 /sys/kernel/debug/dri/0/i915_fbc_status shows
"Compressing: yes" when I move the cursor around.

v3: Use ORIGIN_FLIP now that pin_to_display does not flush frontbuffer.
v2: Update comment in i915_gem_object_pin_to_display_plane. (Chris)

Cc: Paulo Zanoni 
Cc: Ville Syrjälä 
Cc: Chris Wilson 
Cc: Rodrigo Vivi 
Signed-off-by: Dhinakaran Pandiyan 
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 91ce8a0522a3..18b08e263ee1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13176,7 +13176,7 @@ intel_legacy_cursor_update(struct drm_plane *plane,
if (ret)
goto out_unlock;
 
-   intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_DIRTYFB);
+   intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_FLIP);
 
old_fb = old_plane_state->fb;
i915_gem_track_fb(intel_fb_obj(old_fb), intel_fb_obj(fb),
-- 
2.14.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx