| Message ID | 20180307033420.3086-2-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <paulo.r.zanoni@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > 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
On Wed, 2018-03-07 at 22:53 +0000, 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 <paulo.r.zanoni@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > --- > > 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
On Wed, Mar 07, 2018 at 11:10:35PM +0000, Pandiyan, Dhinakaran wrote: > On Wed, 2018-03-07 at 22:53 +0000, 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 <paulo.r.zanoni@intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > --- > > > 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
On Wed, Mar 07, 2018 at 03:23:13PM -0800, Rodrigo Vivi wrote: > On Wed, Mar 07, 2018 at 11:10:35PM +0000, Pandiyan, Dhinakaran wrote: > > On Wed, 2018-03-07 at 22:53 +0000, 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 <paulo.r.zanoni@intel.com> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > --- > > > > 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
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 +0000, Pandiyan, Dhinakaran wrote: > > > On Wed, 2018-03-07 at 22:53 +0000, 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 <paulo.r.zanoni@intel.com> > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > > --- > > > > > 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
On Wed, 2018-03-07 at 15:34 -0800, Dhinakaran Pandiyan wrote: > On Wed, 2018-03-07 at 22:53 +0000, 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 <paulo.r.zanoni@intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > --- > > > 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
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 +0000, Pandiyan, Dhinakaran wrote: > > > > On Wed, 2018-03-07 at 22:53 +0000, 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 <paulo.r.zanoni@intel.com> > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > > > --- > > > > > > 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 <rodrigo.vivi@intel.com> > > > > > > > > > > > > > > > > > > _______________________________________________ > > > > > 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
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),
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 <paulo.r.zanoni@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)