Re: [Intel-gfx] [PATCH 7/7] drm/i915/psr: Disable DRRS if enabled when enabling PSR from debugfs
On Wed, 2018-12-12 at 17:11 -0800, Dhinakaran Pandiyan wrote:
> On Wed, 2018-12-12 at 05:02 -0800, Souza, Jose wrote:
> > On Tue, 2018-12-11 at 14:02 -0800, Dhinakaran Pandiyan wrote:
> > > On Mon, 2018-11-12 at 11:17 +0100, Maarten Lankhorst wrote:
> > > > Op 09-11-18 om 21:20 schreef José Roberto de Souza:
> > > > > If panel supports DRRS and PSR and if driver is loaded
> > > > > without
> > > > > PSR
> > > > > enabled, driver will enable DRRS as expected but if PSR is
> > > > > enabled
> > > > > by
> > > > > debugfs latter it will keep PSR and DRRS enabled causing
> > > > > possible
> > > > > problems as DRRS will lower the refresh rate while PSR
> > > > > enabled.
> > > > >
> > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108341
> > > > > Cc: Maarten Lankhorst
> > > > > Cc: Dhinakaran Pandiyan
> > > > > Signed-off-by: José Roberto de Souza
> > > > > ---
> > > > > drivers/gpu/drm/i915/intel_psr.c | 5 -
> > > > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > > index 853e3f1370a0..bfc6a08b5cf4 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > > @@ -904,8 +904,11 @@ int intel_psr_set_debugfs_mode(struct
> > > > > drm_i915_private *dev_priv,
> > > > >
> > > > > intel_psr_irq_control(dev_priv, dev_priv->psr.debug);
> > > > >
> > > > > - if (dev_priv->psr.prepared && enable)
> > > > > + if (dev_priv->psr.prepared && enable) {
> > > > > + if (crtc_state)
> > > > > + intel_edp_drrs_disable(dp, crtc_state);
> > > > > intel_psr_enable_locked(dev_priv, crtc_state);
> > > > > + }
> > > > >
> > > > > mutex_unlock(_priv->psr.lock);
> > > > > return ret;
> > > >
> > > > I've considered this, but I thought it was a feature, not a
> > > > bug.
> > > > It's
> > > > a pain to track
> > > > how we handle this as intended.
> > > >
> > > > kms_frontbuffer_tracking is also controlling DRRS during the
> > > > test,
> > > > so
> > > > perhaps simply
> > > > fix the test?
> > > >
> > > > It seems the no_drrs test simply checks that if PSR is enabled,
> > > > we
> > > > don't have drrs
> > > > enabled. We probably care about the default configuration, so I
> > > > would
> > > > simply disable
> > > > the pipe, update the PSR flag, and then start running the
> > > > tests.
> > > > Else
> > > > the only thing
> > > > we test is that debugfs disables DRRS. Not that the default
> > > > modeset
> > > > path prevents
> > > > PSR and DRRS simultaneously.
> > > >
> > > > ~Maarten
> > > >
> > > > Maybe something like below?
> > > >
> > > > Perhaps move the drrs manipulation functions from
> > > > kms_frontbuffer_tracking to lib/kms_psr.c
> > > >
> > > > 8<---
> > > > diff --git a/tests/kms_psr.c b/tests/kms_psr.c
> > > > index 9767f475bf23..ffc356df06ce 100644
> > > > --- a/tests/kms_psr.c
> > > > +++ b/tests/kms_psr.c
> > > > @@ -414,9 +414,6 @@ int main(int argc, char *argv[])
> > > > kmstest_set_vt_graphics_mode();
> > > > data.devid = intel_get_drm_devid(data.drm_fd);
> > > >
> > > > - if (!data.with_psr_disabled)
> > > > - psr_enable(data.debugfs_fd);
> > > > -
> > > > igt_require_f(sink_support(),
> > > > "Sink does not support PSR\n");
> > > >
> > > > @@ -428,18 +425,25 @@ int main(int argc, char *argv[])
> > > > }
> > > >
> > > > igt_subtest("basic") {
> > > > - setup_test_plane(,
> > > > DRM_PLANE_TYPE_PRIMARY);
> > > > - igt_assert(psr_wait_entry_if_enabled());
> > > > - test_cleanup();
> > > > - }
> > > > + /* Disable display to get a default setup. */
> > > > + igt_display_commit2(,
> > > > data.display.is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
> > > > +
> > > > + if (!data.with_psr_disabled)
> > > > + psr_enable(data.debugfs_fd);
> > > >
> > > > - igt_subtest("no_drrs") {
> > > > setup_test_plane(,
> > > > DRM_PLANE_TYPE_PRIMARY);
> > > > igt_assert(psr_wait_entry_if_enabled());
> > > > igt_assert(drrs_disabled());
> > > > test_cleanup();
> > >
> > > This makes a lot more sense to me, ensuring that DRRS does not
> > > get
> > > enabled in the default code path was the goal of the no-drrs
> > > test.
> >
> > The problem with this approach is if user wants to run just one
> > test
> > the basic test will not run and DRRS will be kept on, it will not
> > fail
> > the no_drrs test but DRRS could interfere with other PSR tests.
> Disable DRRS for the other sub-tests, doesn't this work?
> echo 0 > /sys/kernel/debug/dri/0/i915_drrs_ctl
It would work but with this we are not testing or stressing any code
Re: [Intel-gfx] [PATCH 7/7] drm/i915/psr: Disable DRRS if enabled when enabling PSR from debugfs
On Wed, 2018-12-12 at 05:02 -0800, Souza, Jose wrote:
> On Tue, 2018-12-11 at 14:02 -0800, Dhinakaran Pandiyan wrote:
> > On Mon, 2018-11-12 at 11:17 +0100, Maarten Lankhorst wrote:
> > > Op 09-11-18 om 21:20 schreef José Roberto de Souza:
> > > > If panel supports DRRS and PSR and if driver is loaded without
> > > > PSR
> > > > enabled, driver will enable DRRS as expected but if PSR is
> > > > enabled
> > > > by
> > > > debugfs latter it will keep PSR and DRRS enabled causing
> > > > possible
> > > > problems as DRRS will lower the refresh rate while PSR enabled.
> > > >
> > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108341
> > > > Cc: Maarten Lankhorst
> > > > Cc: Dhinakaran Pandiyan
> > > > Signed-off-by: José Roberto de Souza
> > > > ---
> > > > drivers/gpu/drm/i915/intel_psr.c | 5 -
> > > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > index 853e3f1370a0..bfc6a08b5cf4 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -904,8 +904,11 @@ int intel_psr_set_debugfs_mode(struct
> > > > drm_i915_private *dev_priv,
> > > >
> > > > intel_psr_irq_control(dev_priv, dev_priv->psr.debug);
> > > >
> > > > - if (dev_priv->psr.prepared && enable)
> > > > + if (dev_priv->psr.prepared && enable) {
> > > > + if (crtc_state)
> > > > + intel_edp_drrs_disable(dp, crtc_state);
> > > > intel_psr_enable_locked(dev_priv, crtc_state);
> > > > + }
> > > >
> > > > mutex_unlock(_priv->psr.lock);
> > > > return ret;
> > >
> > > I've considered this, but I thought it was a feature, not a bug.
> > > It's
> > > a pain to track
> > > how we handle this as intended.
> > >
> > > kms_frontbuffer_tracking is also controlling DRRS during the
> > > test,
> > > so
> > > perhaps simply
> > > fix the test?
> > >
> > > It seems the no_drrs test simply checks that if PSR is enabled,
> > > we
> > > don't have drrs
> > > enabled. We probably care about the default configuration, so I
> > > would
> > > simply disable
> > > the pipe, update the PSR flag, and then start running the tests.
> > > Else
> > > the only thing
> > > we test is that debugfs disables DRRS. Not that the default
> > > modeset
> > > path prevents
> > > PSR and DRRS simultaneously.
> > >
> > > ~Maarten
> > >
> > > Maybe something like below?
> > >
> > > Perhaps move the drrs manipulation functions from
> > > kms_frontbuffer_tracking to lib/kms_psr.c
> > >
> > > 8<---
> > > diff --git a/tests/kms_psr.c b/tests/kms_psr.c
> > > index 9767f475bf23..ffc356df06ce 100644
> > > --- a/tests/kms_psr.c
> > > +++ b/tests/kms_psr.c
> > > @@ -414,9 +414,6 @@ int main(int argc, char *argv[])
> > > kmstest_set_vt_graphics_mode();
> > > data.devid = intel_get_drm_devid(data.drm_fd);
> > >
> > > - if (!data.with_psr_disabled)
> > > - psr_enable(data.debugfs_fd);
> > > -
> > > igt_require_f(sink_support(),
> > > "Sink does not support PSR\n");
> > >
> > > @@ -428,18 +425,25 @@ int main(int argc, char *argv[])
> > > }
> > >
> > > igt_subtest("basic") {
> > > - setup_test_plane(, DRM_PLANE_TYPE_PRIMARY);
> > > - igt_assert(psr_wait_entry_if_enabled());
> > > - test_cleanup();
> > > - }
> > > + /* Disable display to get a default setup. */
> > > + igt_display_commit2(,
> > > data.display.is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
> > > +
> > > + if (!data.with_psr_disabled)
> > > + psr_enable(data.debugfs_fd);
> > >
> > > - igt_subtest("no_drrs") {
> > > setup_test_plane(, DRM_PLANE_TYPE_PRIMARY);
> > > igt_assert(psr_wait_entry_if_enabled());
> > > igt_assert(drrs_disabled());
> > > test_cleanup();
> >
> > This makes a lot more sense to me, ensuring that DRRS does not get
> > enabled in the default code path was the goal of the no-drrs test.
>
> The problem with this approach is if user wants to run just one test
> the basic test will not run and DRRS will be kept on, it will not
> fail
> the no_drrs test but DRRS could interfere with other PSR tests.
Disable DRRS for the other sub-tests, doesn't this work?
echo 0 > /sys/kernel/debug/dri/0/i915_drrs_ctl
>
> If the call to disable display is moved to igt_fixture() it would
> work
> fine but in terms of modesets this approach have just one more
> modeset
> than forcing a modeset every time we write to PSR i915_edp_psr_debug,
> for just the cost of one modeset in my opnion is better drop the
> aditional PSR code path and just test the main one.
>
> >
> > -DK
> >
> > > }
> > >
> > > + igt_fixture {
> > > + drrs_disable();
> > > +
> > > + if (!data.with_psr_disabled)
> > > +
Re: [Intel-gfx] [PATCH 7/7] drm/i915/psr: Disable DRRS if enabled when enabling PSR from debugfs
On Tue, 2018-12-11 at 14:02 -0800, Dhinakaran Pandiyan wrote:
> On Mon, 2018-11-12 at 11:17 +0100, Maarten Lankhorst wrote:
> > Op 09-11-18 om 21:20 schreef José Roberto de Souza:
> > > If panel supports DRRS and PSR and if driver is loaded without
> > > PSR
> > > enabled, driver will enable DRRS as expected but if PSR is
> > > enabled
> > > by
> > > debugfs latter it will keep PSR and DRRS enabled causing possible
> > > problems as DRRS will lower the refresh rate while PSR enabled.
> > >
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108341
> > > Cc: Maarten Lankhorst
> > > Cc: Dhinakaran Pandiyan
> > > Signed-off-by: José Roberto de Souza
> > > ---
> > > drivers/gpu/drm/i915/intel_psr.c | 5 -
> > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index 853e3f1370a0..bfc6a08b5cf4 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -904,8 +904,11 @@ int intel_psr_set_debugfs_mode(struct
> > > drm_i915_private *dev_priv,
> > >
> > > intel_psr_irq_control(dev_priv, dev_priv->psr.debug);
> > >
> > > - if (dev_priv->psr.prepared && enable)
> > > + if (dev_priv->psr.prepared && enable) {
> > > + if (crtc_state)
> > > + intel_edp_drrs_disable(dp, crtc_state);
> > > intel_psr_enable_locked(dev_priv, crtc_state);
> > > + }
> > >
> > > mutex_unlock(_priv->psr.lock);
> > > return ret;
> >
> > I've considered this, but I thought it was a feature, not a bug.
> > It's
> > a pain to track
> > how we handle this as intended.
> >
> > kms_frontbuffer_tracking is also controlling DRRS during the test,
> > so
> > perhaps simply
> > fix the test?
> >
> > It seems the no_drrs test simply checks that if PSR is enabled, we
> > don't have drrs
> > enabled. We probably care about the default configuration, so I
> > would
> > simply disable
> > the pipe, update the PSR flag, and then start running the tests.
> > Else
> > the only thing
> > we test is that debugfs disables DRRS. Not that the default modeset
> > path prevents
> > PSR and DRRS simultaneously.
> >
> > ~Maarten
> >
> > Maybe something like below?
> >
> > Perhaps move the drrs manipulation functions from
> > kms_frontbuffer_tracking to lib/kms_psr.c
> >
> > 8<---
> > diff --git a/tests/kms_psr.c b/tests/kms_psr.c
> > index 9767f475bf23..ffc356df06ce 100644
> > --- a/tests/kms_psr.c
> > +++ b/tests/kms_psr.c
> > @@ -414,9 +414,6 @@ int main(int argc, char *argv[])
> > kmstest_set_vt_graphics_mode();
> > data.devid = intel_get_drm_devid(data.drm_fd);
> >
> > - if (!data.with_psr_disabled)
> > - psr_enable(data.debugfs_fd);
> > -
> > igt_require_f(sink_support(),
> > "Sink does not support PSR\n");
> >
> > @@ -428,18 +425,25 @@ int main(int argc, char *argv[])
> > }
> >
> > igt_subtest("basic") {
> > - setup_test_plane(, DRM_PLANE_TYPE_PRIMARY);
> > - igt_assert(psr_wait_entry_if_enabled());
> > - test_cleanup();
> > - }
> > + /* Disable display to get a default setup. */
> > + igt_display_commit2(,
> > data.display.is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
> > +
> > + if (!data.with_psr_disabled)
> > + psr_enable(data.debugfs_fd);
> >
> > - igt_subtest("no_drrs") {
> > setup_test_plane(, DRM_PLANE_TYPE_PRIMARY);
> > igt_assert(psr_wait_entry_if_enabled());
> > igt_assert(drrs_disabled());
> > test_cleanup();
> This makes a lot more sense to me, ensuring that DRRS does not get
> enabled in the default code path was the goal of the no-drrs test.
The problem with this approach is if user wants to run just one test
the basic test will not run and DRRS will be kept on, it will not fail
the no_drrs test but DRRS could interfere with other PSR tests.
If the call to disable display is moved to igt_fixture() it would work
fine but in terms of modesets this approach have just one more modeset
than forcing a modeset every time we write to PSR i915_edp_psr_debug,
for just the cost of one modeset in my opnion is better drop the
aditional PSR code path and just test the main one.
>
> -DK
>
> > }
> >
> > + igt_fixture {
> > + drrs_disable();
> > +
> > + if (!data.with_psr_disabled)
> > + psr_enable(data.debugfs_fd);
> > + }
> > +
> > for (op = PAGE_FLIP; op <= RENDER; op++) {
> > igt_subtest_f("primary_%s", op_str(op)) {
> > data.op = op;
> >
signature.asc
Description: This is a digitally signed message part
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 7/7] drm/i915/psr: Disable DRRS if enabled when enabling PSR from debugfs
On Mon, 2018-11-12 at 11:17 +0100, Maarten Lankhorst wrote:
> Op 09-11-18 om 21:20 schreef José Roberto de Souza:
> > If panel supports DRRS and PSR and if driver is loaded without PSR
> > enabled, driver will enable DRRS as expected but if PSR is enabled
> > by
> > debugfs latter it will keep PSR and DRRS enabled causing possible
> > problems as DRRS will lower the refresh rate while PSR enabled.
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108341
> > Cc: Maarten Lankhorst
> > Cc: Dhinakaran Pandiyan
> > Signed-off-by: José Roberto de Souza
> > ---
> > drivers/gpu/drm/i915/intel_psr.c | 5 -
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 853e3f1370a0..bfc6a08b5cf4 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -904,8 +904,11 @@ int intel_psr_set_debugfs_mode(struct
> > drm_i915_private *dev_priv,
> >
> > intel_psr_irq_control(dev_priv, dev_priv->psr.debug);
> >
> > - if (dev_priv->psr.prepared && enable)
> > + if (dev_priv->psr.prepared && enable) {
> > + if (crtc_state)
> > + intel_edp_drrs_disable(dp, crtc_state);
> > intel_psr_enable_locked(dev_priv, crtc_state);
> > + }
> >
> > mutex_unlock(_priv->psr.lock);
> > return ret;
>
> I've considered this, but I thought it was a feature, not a bug. It's
> a pain to track
> how we handle this as intended.
>
> kms_frontbuffer_tracking is also controlling DRRS during the test, so
> perhaps simply
> fix the test?
>
> It seems the no_drrs test simply checks that if PSR is enabled, we
> don't have drrs
> enabled. We probably care about the default configuration, so I would
> simply disable
> the pipe, update the PSR flag, and then start running the tests. Else
> the only thing
> we test is that debugfs disables DRRS. Not that the default modeset
> path prevents
> PSR and DRRS simultaneously.
>
> ~Maarten
>
> Maybe something like below?
>
> Perhaps move the drrs manipulation functions from
> kms_frontbuffer_tracking to lib/kms_psr.c
>
> 8<---
> diff --git a/tests/kms_psr.c b/tests/kms_psr.c
> index 9767f475bf23..ffc356df06ce 100644
> --- a/tests/kms_psr.c
> +++ b/tests/kms_psr.c
> @@ -414,9 +414,6 @@ int main(int argc, char *argv[])
> kmstest_set_vt_graphics_mode();
> data.devid = intel_get_drm_devid(data.drm_fd);
>
> - if (!data.with_psr_disabled)
> - psr_enable(data.debugfs_fd);
> -
> igt_require_f(sink_support(),
> "Sink does not support PSR\n");
>
> @@ -428,18 +425,25 @@ int main(int argc, char *argv[])
> }
>
> igt_subtest("basic") {
> - setup_test_plane(, DRM_PLANE_TYPE_PRIMARY);
> - igt_assert(psr_wait_entry_if_enabled());
> - test_cleanup();
> - }
> + /* Disable display to get a default setup. */
> + igt_display_commit2(,
> data.display.is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
> +
> + if (!data.with_psr_disabled)
> + psr_enable(data.debugfs_fd);
>
> - igt_subtest("no_drrs") {
> setup_test_plane(, DRM_PLANE_TYPE_PRIMARY);
> igt_assert(psr_wait_entry_if_enabled());
> igt_assert(drrs_disabled());
> test_cleanup();
This makes a lot more sense to me, ensuring that DRRS does not get
enabled in the default code path was the goal of the no-drrs test.
-DK
> }
>
> + igt_fixture {
> + drrs_disable();
> +
> + if (!data.with_psr_disabled)
> + psr_enable(data.debugfs_fd);
> + }
> +
> for (op = PAGE_FLIP; op <= RENDER; op++) {
> igt_subtest_f("primary_%s", op_str(op)) {
> data.op = op;
>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 7/7] drm/i915/psr: Disable DRRS if enabled when enabling PSR from debugfs
On Thu, 2018-11-15 at 12:57 -0800, Souza, Jose wrote:
> On Mon, 2018-11-12 at 11:17 +0100, Maarten Lankhorst wrote:
> > Op 09-11-18 om 21:20 schreef José Roberto de Souza:
> > > If panel supports DRRS and PSR and if driver is loaded without
> > > PSR
> > > enabled, driver will enable DRRS as expected but if PSR is
> > > enabled
> > > by
> > > debugfs latter it will keep PSR and DRRS enabled causing possible
> > > problems as DRRS will lower the refresh rate while PSR enabled.
> > >
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108341
> > > Cc: Maarten Lankhorst
> > > Cc: Dhinakaran Pandiyan
> > > Signed-off-by: José Roberto de Souza
> > > ---
> > > drivers/gpu/drm/i915/intel_psr.c | 5 -
> > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index 853e3f1370a0..bfc6a08b5cf4 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -904,8 +904,11 @@ int intel_psr_set_debugfs_mode(struct
> > > drm_i915_private *dev_priv,
> > >
> > > intel_psr_irq_control(dev_priv, dev_priv->psr.debug);
> > >
> > > - if (dev_priv->psr.prepared && enable)
> > > + if (dev_priv->psr.prepared && enable) {
> > > + if (crtc_state)
> > > + intel_edp_drrs_disable(dp, crtc_state);
> > > intel_psr_enable_locked(dev_priv, crtc_state);
> > > + }
> > >
> > > mutex_unlock(_priv->psr.lock);
> > > return ret;
> >
> > I've considered this, but I thought it was a feature, not a bug.
> > It's
> > a pain to track
> > how we handle this as intended.
> >
> > kms_frontbuffer_tracking is also controlling DRRS during the test,
> > so
> > perhaps simply
> > fix the test?
> >
> > It seems the no_drrs test simply checks that if PSR is enabled, we
> > don't have drrs
> > enabled. We probably care about the default configuration, so I
> > would
> > simply disable
> > the pipe, update the PSR flag, and then start running the tests.
> > Else
> > the only thing
> > we test is that debugfs disables DRRS. Not that the default modeset
> > path prevents
> > PSR and DRRS simultaneously.
>
>
> Yeah, I think we should force a modeset from debugs to test the
> default
> modeset path, fix the tests I think is a bad idea as it could leave
> some test cases unhandled.
Maarten,
Can you comment on the Jose's suggestion above? I recall the idea
behind your patches was to avoid modesets while enabling and disabling
PSR. Jose's latest version otoh forces a modeset.
-DK
>
> Also looking at DRRS it looks complete broken for page flips, it
> would
> kept set to DRRS_LOW forever.
>
>
> >
> > ~Maarten
> >
> > Maybe something like below?
> >
> > Perhaps move the drrs manipulation functions from
> > kms_frontbuffer_tracking to lib/kms_psr.c
> >
> > 8<---
> > diff --git a/tests/kms_psr.c b/tests/kms_psr.c
> > index 9767f475bf23..ffc356df06ce 100644
> > --- a/tests/kms_psr.c
> > +++ b/tests/kms_psr.c
> > @@ -414,9 +414,6 @@ int main(int argc, char *argv[])
> > kmstest_set_vt_graphics_mode();
> > data.devid = intel_get_drm_devid(data.drm_fd);
> >
> > - if (!data.with_psr_disabled)
> > - psr_enable(data.debugfs_fd);
> > -
> > igt_require_f(sink_support(),
> > "Sink does not support PSR\n");
> >
> > @@ -428,18 +425,25 @@ int main(int argc, char *argv[])
> > }
> >
> > igt_subtest("basic") {
> > - setup_test_plane(, DRM_PLANE_TYPE_PRIMARY);
> > - igt_assert(psr_wait_entry_if_enabled());
> > - test_cleanup();
> > - }
> > + /* Disable display to get a default setup. */
> > + igt_display_commit2(,
> > data.display.is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
> > +
> > + if (!data.with_psr_disabled)
> > + psr_enable(data.debugfs_fd);
> >
> > - igt_subtest("no_drrs") {
> > setup_test_plane(, DRM_PLANE_TYPE_PRIMARY);
> > igt_assert(psr_wait_entry_if_enabled());
> > igt_assert(drrs_disabled());
> > test_cleanup();
> > }
> >
> > + igt_fixture {
> > + drrs_disable();
> > +
> > + if (!data.with_psr_disabled)
> > + psr_enable(data.debugfs_fd);
> > + }
> > +
> > for (op = PAGE_FLIP; op <= RENDER; op++) {
> > igt_subtest_f("primary_%s", op_str(op)) {
> > data.op = op;
> >
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 7/7] drm/i915/psr: Disable DRRS if enabled when enabling PSR from debugfs
On Mon, 2018-11-12 at 11:17 +0100, Maarten Lankhorst wrote:
> Op 09-11-18 om 21:20 schreef José Roberto de Souza:
> > If panel supports DRRS and PSR and if driver is loaded without PSR
> > enabled, driver will enable DRRS as expected but if PSR is enabled
> > by
> > debugfs latter it will keep PSR and DRRS enabled causing possible
> > problems as DRRS will lower the refresh rate while PSR enabled.
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108341
> > Cc: Maarten Lankhorst
> > Cc: Dhinakaran Pandiyan
> > Signed-off-by: José Roberto de Souza
> > ---
> > drivers/gpu/drm/i915/intel_psr.c | 5 -
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 853e3f1370a0..bfc6a08b5cf4 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -904,8 +904,11 @@ int intel_psr_set_debugfs_mode(struct
> > drm_i915_private *dev_priv,
> >
> > intel_psr_irq_control(dev_priv, dev_priv->psr.debug);
> >
> > - if (dev_priv->psr.prepared && enable)
> > + if (dev_priv->psr.prepared && enable) {
> > + if (crtc_state)
> > + intel_edp_drrs_disable(dp, crtc_state);
> > intel_psr_enable_locked(dev_priv, crtc_state);
> > + }
> >
> > mutex_unlock(_priv->psr.lock);
> > return ret;
>
> I've considered this, but I thought it was a feature, not a bug. It's
> a pain to track
> how we handle this as intended.
>
> kms_frontbuffer_tracking is also controlling DRRS during the test, so
> perhaps simply
> fix the test?
>
> It seems the no_drrs test simply checks that if PSR is enabled, we
> don't have drrs
> enabled. We probably care about the default configuration, so I would
> simply disable
> the pipe, update the PSR flag, and then start running the tests. Else
> the only thing
> we test is that debugfs disables DRRS. Not that the default modeset
> path prevents
> PSR and DRRS simultaneously.
Yeah, I think we should force a modeset from debugs to test the default
modeset path, fix the tests I think is a bad idea as it could leave
some test cases unhandled.
Also looking at DRRS it looks complete broken for page flips, it would
kept set to DRRS_LOW forever.
>
> ~Maarten
>
> Maybe something like below?
>
> Perhaps move the drrs manipulation functions from
> kms_frontbuffer_tracking to lib/kms_psr.c
>
> 8<---
> diff --git a/tests/kms_psr.c b/tests/kms_psr.c
> index 9767f475bf23..ffc356df06ce 100644
> --- a/tests/kms_psr.c
> +++ b/tests/kms_psr.c
> @@ -414,9 +414,6 @@ int main(int argc, char *argv[])
> kmstest_set_vt_graphics_mode();
> data.devid = intel_get_drm_devid(data.drm_fd);
>
> - if (!data.with_psr_disabled)
> - psr_enable(data.debugfs_fd);
> -
> igt_require_f(sink_support(),
> "Sink does not support PSR\n");
>
> @@ -428,18 +425,25 @@ int main(int argc, char *argv[])
> }
>
> igt_subtest("basic") {
> - setup_test_plane(, DRM_PLANE_TYPE_PRIMARY);
> - igt_assert(psr_wait_entry_if_enabled());
> - test_cleanup();
> - }
> + /* Disable display to get a default setup. */
> + igt_display_commit2(,
> data.display.is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
> +
> + if (!data.with_psr_disabled)
> + psr_enable(data.debugfs_fd);
>
> - igt_subtest("no_drrs") {
> setup_test_plane(, DRM_PLANE_TYPE_PRIMARY);
> igt_assert(psr_wait_entry_if_enabled());
> igt_assert(drrs_disabled());
> test_cleanup();
> }
>
> + igt_fixture {
> + drrs_disable();
> +
> + if (!data.with_psr_disabled)
> + psr_enable(data.debugfs_fd);
> + }
> +
> for (op = PAGE_FLIP; op <= RENDER; op++) {
> igt_subtest_f("primary_%s", op_str(op)) {
> data.op = op;
>
signature.asc
Description: This is a digitally signed message part
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 7/7] drm/i915/psr: Disable DRRS if enabled when enabling PSR from debugfs
Op 09-11-18 om 21:20 schreef José Roberto de Souza:
> If panel supports DRRS and PSR and if driver is loaded without PSR
> enabled, driver will enable DRRS as expected but if PSR is enabled by
> debugfs latter it will keep PSR and DRRS enabled causing possible
> problems as DRRS will lower the refresh rate while PSR enabled.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108341
> Cc: Maarten Lankhorst
> Cc: Dhinakaran Pandiyan
> Signed-off-by: José Roberto de Souza
> ---
> drivers/gpu/drm/i915/intel_psr.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 853e3f1370a0..bfc6a08b5cf4 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -904,8 +904,11 @@ int intel_psr_set_debugfs_mode(struct drm_i915_private
> *dev_priv,
>
> intel_psr_irq_control(dev_priv, dev_priv->psr.debug);
>
> - if (dev_priv->psr.prepared && enable)
> + if (dev_priv->psr.prepared && enable) {
> + if (crtc_state)
> + intel_edp_drrs_disable(dp, crtc_state);
> intel_psr_enable_locked(dev_priv, crtc_state);
> + }
>
> mutex_unlock(_priv->psr.lock);
> return ret;
I've considered this, but I thought it was a feature, not a bug. It's a pain to
track
how we handle this as intended.
kms_frontbuffer_tracking is also controlling DRRS during the test, so perhaps
simply
fix the test?
It seems the no_drrs test simply checks that if PSR is enabled, we don't have
drrs
enabled. We probably care about the default configuration, so I would simply
disable
the pipe, update the PSR flag, and then start running the tests. Else the only
thing
we test is that debugfs disables DRRS. Not that the default modeset path
prevents
PSR and DRRS simultaneously.
~Maarten
Maybe something like below?
Perhaps move the drrs manipulation functions from kms_frontbuffer_tracking to
lib/kms_psr.c
8<---
diff --git a/tests/kms_psr.c b/tests/kms_psr.c
index 9767f475bf23..ffc356df06ce 100644
--- a/tests/kms_psr.c
+++ b/tests/kms_psr.c
@@ -414,9 +414,6 @@ int main(int argc, char *argv[])
kmstest_set_vt_graphics_mode();
data.devid = intel_get_drm_devid(data.drm_fd);
- if (!data.with_psr_disabled)
- psr_enable(data.debugfs_fd);
-
igt_require_f(sink_support(),
"Sink does not support PSR\n");
@@ -428,18 +425,25 @@ int main(int argc, char *argv[])
}
igt_subtest("basic") {
- setup_test_plane(, DRM_PLANE_TYPE_PRIMARY);
- igt_assert(psr_wait_entry_if_enabled());
- test_cleanup();
- }
+ /* Disable display to get a default setup. */
+ igt_display_commit2(, data.display.is_atomic ?
COMMIT_ATOMIC : COMMIT_LEGACY);
+
+ if (!data.with_psr_disabled)
+ psr_enable(data.debugfs_fd);
- igt_subtest("no_drrs") {
setup_test_plane(, DRM_PLANE_TYPE_PRIMARY);
igt_assert(psr_wait_entry_if_enabled());
igt_assert(drrs_disabled());
test_cleanup();
}
+ igt_fixture {
+ drrs_disable();
+
+ if (!data.with_psr_disabled)
+ psr_enable(data.debugfs_fd);
+ }
+
for (op = PAGE_FLIP; op <= RENDER; op++) {
igt_subtest_f("primary_%s", op_str(op)) {
data.op = op;
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 7/7] drm/i915/psr: Disable DRRS if enabled when enabling PSR from debugfs
If panel supports DRRS and PSR and if driver is loaded without PSR
enabled, driver will enable DRRS as expected but if PSR is enabled by
debugfs latter it will keep PSR and DRRS enabled causing possible
problems as DRRS will lower the refresh rate while PSR enabled.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108341
Cc: Maarten Lankhorst
Cc: Dhinakaran Pandiyan
Signed-off-by: José Roberto de Souza
---
drivers/gpu/drm/i915/intel_psr.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 853e3f1370a0..bfc6a08b5cf4 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -904,8 +904,11 @@ int intel_psr_set_debugfs_mode(struct drm_i915_private
*dev_priv,
intel_psr_irq_control(dev_priv, dev_priv->psr.debug);
- if (dev_priv->psr.prepared && enable)
+ if (dev_priv->psr.prepared && enable) {
+ if (crtc_state)
+ intel_edp_drrs_disable(dp, crtc_state);
intel_psr_enable_locked(dev_priv, crtc_state);
+ }
mutex_unlock(_priv->psr.lock);
return ret;
--
2.19.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
