Re: [Intel-gfx] [PATCH v4] drm/i915/psr: Force PSR probe only after full initialization
On Fri, 2020-02-21 at 12:11 -0800, Souza, Jose wrote:
> On Fri, 2020-02-21 at 20:04 +, Mun, Gwan-gyeong wrote:
> > On Thu, 2020-02-20 at 15:15 -0800, José Roberto de Souza wrote:
> > > Commit 60c6a14b489b ("drm/i915/display: Force the state compute
> > > phase
> > > once to enable PSR") was forcing the state compute too earlier
> > > causing errors because not everything was initialized, so here
> > > moving to the end of i915_driver_modeset_probe() when the display
> > > is
> > > all initialized.
> > >
> > > Also fixing the place where it disarm the force probe as during
> > > the
> > > atomic check phase errors could happen like the ones due locking
> > > and
> > > it would cause PSR to never be enabled if that happens.
> > > Leaving the disarm to the atomic commit phase, intel_psr_enable()
> > > or
> > > intel_psr_update() will be called even if the current state do
> > > not
> > > allow PSR to be enabled.
> > >
> > > v2: Check if intel_dp is null in
> > > intel_psr_force_mode_changed_set()
> > > v3: Check intel_dp before get dev_priv
> > > v4:
> > > - renamed intel_psr_force_mode_changed_set() to
> > > intel_psr_set_force_mode_changed()
> > > - removed the set parameter from
> > > intel_psr_set_force_mode_changed()
> > > - not calling intel_psr_set_force_mode_changed() from
> > > intel_psr_enable/update(), directly setting it after the same
> > > checks
> > > that intel_psr_set_force_mode_changed() does
> > > - moved intel_psr_set_force_mode_changed() arm call to
> > > i915_driver_modeset_probe() as it is a better for a PSR call, all
> > > the
> > > functions calls happening between the old and the new function
> > > call
> > > will cause issue
> > >
> > > Fixes: 60c6a14b489b ("drm/i915/display: Force the state compute
> > > phase
> > > once to enable PSR")
> > > Closes: https://gitlab.freedesktop.org/drm/intel/issues/1151
> > > Tested-by: Ross Zwisler
> > > Reported-by: Ross Zwisler
> > > Cc: Gwan-gyeong Mun
> > > Cc: Jani Nikula
> > > Cc: Anshuman Gupta
> > > Signed-off-by: José Roberto de Souza
> > > ---
> > > drivers/gpu/drm/i915/display/intel_psr.c | 25
> > >
> > >
> > > drivers/gpu/drm/i915/display/intel_psr.h | 1 +
> > > drivers/gpu/drm/i915/i915_drv.c | 3 +++
> > > drivers/gpu/drm/i915/i915_drv.h | 2 +-
> > > 4 files changed, 26 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > index b4942b6445ae..7e754201f54d 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > @@ -936,10 +936,12 @@ void intel_psr_enable(struct intel_dp
> > > *intel_dp,
> > > {
> > > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > >
> > > - if (!crtc_state->has_psr)
> > Enabling of crtc_state->has_psr is handled on
> > intel_psr_compute_config().
> > And the intel_psr_compute_config() checks "CAN_PSR(dev_priv) and
> > intel_dp != dev_priv->psr.dp".
> > therefore if we have this line "if (!crtc_state->has_psr)", we
> > don't
> > need to add "if (!CAN_PSR(dev_priv) || dev_priv->psr.dp !=
> > intel_dp)"
>
> crtc_state->has_psr will only be set to true if the state allows, we
> don't want to keep computing pipe configuration for every following
> page flip if PSR can not be enable(like PSR setup time do not fit in
> vblank).
>
Yes, you are right. we also have to handle the case which can not
support PSR. (I missed those cases.)
> That is why it is unseting force_mode_changed even if crtc_state-
> > has_psr is false.
> > Except that, looks good to me.
> >
> > Reviewed-by: Gwan-gyeong Mun
> >
> > > + if (!CAN_PSR(dev_priv) || dev_priv->psr.dp != intel_dp)
> > > return;
> > >
> > > - if (drm_WARN_ON(_priv->drm, !CAN_PSR(dev_priv)))
> > > + dev_priv->psr.force_mode_changed = false;
> > > +
> > > + if (!crtc_state->has_psr)
> > > return;
> > >
> > > drm_WARN_ON(_priv->drm, dev_priv->drrs.dp);
> > > @@ -1099,6 +1101,8 @@ void intel_psr_update(struct intel_dp
> > > *intel_dp,
> > > if (!CAN_PSR(dev_priv) || READ_ONCE(psr->dp) != intel_dp)
> > > return;
> > >
> > > + dev_priv->psr.force_mode_changed = false;
> > > +
> > > mutex_lock(_priv->psr.lock);
> > >
> > > enable = crtc_state->has_psr && psr_global_enabled(dev_priv);
> > > @@ -1629,7 +1633,7 @@ void intel_psr_atomic_check(struct
> > > drm_connector *connector,
> > > struct drm_crtc_state *crtc_state;
> > >
> > > if (!CAN_PSR(dev_priv) || !new_state->crtc ||
> > > - dev_priv->psr.initially_probed)
> > > + !dev_priv->psr.force_mode_changed)
> > > return;
> > >
> > > intel_connector = to_intel_connector(connector);
> > > @@ -1640,5 +1644,18 @@ void intel_psr_atomic_check(struct
> > > drm_connector *connector,
> > > crtc_state = drm_atomic_get_new_crtc_state(new_state->state,
> > > new_state->crtc);
> > >
Re: [Intel-gfx] [PATCH v4] drm/i915/psr: Force PSR probe only after full initialization
On Fri, 2020-02-21 at 20:04 +, Mun, Gwan-gyeong wrote:
> On Thu, 2020-02-20 at 15:15 -0800, José Roberto de Souza wrote:
> > Commit 60c6a14b489b ("drm/i915/display: Force the state compute
> > phase
> > once to enable PSR") was forcing the state compute too earlier
> > causing errors because not everything was initialized, so here
> > moving to the end of i915_driver_modeset_probe() when the display
> > is
> > all initialized.
> >
> > Also fixing the place where it disarm the force probe as during the
> > atomic check phase errors could happen like the ones due locking
> > and
> > it would cause PSR to never be enabled if that happens.
> > Leaving the disarm to the atomic commit phase, intel_psr_enable()
> > or
> > intel_psr_update() will be called even if the current state do not
> > allow PSR to be enabled.
> >
> > v2: Check if intel_dp is null in intel_psr_force_mode_changed_set()
> > v3: Check intel_dp before get dev_priv
> > v4:
> > - renamed intel_psr_force_mode_changed_set() to
> > intel_psr_set_force_mode_changed()
> > - removed the set parameter from intel_psr_set_force_mode_changed()
> > - not calling intel_psr_set_force_mode_changed() from
> > intel_psr_enable/update(), directly setting it after the same
> > checks
> > that intel_psr_set_force_mode_changed() does
> > - moved intel_psr_set_force_mode_changed() arm call to
> > i915_driver_modeset_probe() as it is a better for a PSR call, all
> > the
> > functions calls happening between the old and the new function call
> > will cause issue
> >
> > Fixes: 60c6a14b489b ("drm/i915/display: Force the state compute
> > phase
> > once to enable PSR")
> > Closes: https://gitlab.freedesktop.org/drm/intel/issues/1151
> > Tested-by: Ross Zwisler
> > Reported-by: Ross Zwisler
> > Cc: Gwan-gyeong Mun
> > Cc: Jani Nikula
> > Cc: Anshuman Gupta
> > Signed-off-by: José Roberto de Souza
> > ---
> > drivers/gpu/drm/i915/display/intel_psr.c | 25
> >
> > drivers/gpu/drm/i915/display/intel_psr.h | 1 +
> > drivers/gpu/drm/i915/i915_drv.c | 3 +++
> > drivers/gpu/drm/i915/i915_drv.h | 2 +-
> > 4 files changed, 26 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index b4942b6445ae..7e754201f54d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -936,10 +936,12 @@ void intel_psr_enable(struct intel_dp
> > *intel_dp,
> > {
> > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> >
> > - if (!crtc_state->has_psr)
> Enabling of crtc_state->has_psr is handled on
> intel_psr_compute_config().
> And the intel_psr_compute_config() checks "CAN_PSR(dev_priv) and
> intel_dp != dev_priv->psr.dp".
> therefore if we have this line "if (!crtc_state->has_psr)", we don't
> need to add "if (!CAN_PSR(dev_priv) || dev_priv->psr.dp != intel_dp)"
crtc_state->has_psr will only be set to true if the state allows, we
don't want to keep computing pipe configuration for every following
page flip if PSR can not be enable(like PSR setup time do not fit in
vblank).
That is why it is unseting force_mode_changed even if crtc_state-
>has_psr is false.
>
> Except that, looks good to me.
>
> Reviewed-by: Gwan-gyeong Mun
>
> > + if (!CAN_PSR(dev_priv) || dev_priv->psr.dp != intel_dp)
> > return;
> >
> > - if (drm_WARN_ON(_priv->drm, !CAN_PSR(dev_priv)))
> > + dev_priv->psr.force_mode_changed = false;
> > +
> > + if (!crtc_state->has_psr)
> > return;
> >
> > drm_WARN_ON(_priv->drm, dev_priv->drrs.dp);
> > @@ -1099,6 +1101,8 @@ void intel_psr_update(struct intel_dp
> > *intel_dp,
> > if (!CAN_PSR(dev_priv) || READ_ONCE(psr->dp) != intel_dp)
> > return;
> >
> > + dev_priv->psr.force_mode_changed = false;
> > +
> > mutex_lock(_priv->psr.lock);
> >
> > enable = crtc_state->has_psr && psr_global_enabled(dev_priv);
> > @@ -1629,7 +1633,7 @@ void intel_psr_atomic_check(struct
> > drm_connector *connector,
> > struct drm_crtc_state *crtc_state;
> >
> > if (!CAN_PSR(dev_priv) || !new_state->crtc ||
> > - dev_priv->psr.initially_probed)
> > + !dev_priv->psr.force_mode_changed)
> > return;
> >
> > intel_connector = to_intel_connector(connector);
> > @@ -1640,5 +1644,18 @@ void intel_psr_atomic_check(struct
> > drm_connector *connector,
> > crtc_state = drm_atomic_get_new_crtc_state(new_state->state,
> >new_state->crtc);
> > crtc_state->mode_changed = true;
> > - dev_priv->psr.initially_probed = true;
> > +}
> > +
> > +void intel_psr_set_force_mode_changed(struct intel_dp *intel_dp)
> > +{
> > + struct drm_i915_private *dev_priv;
> > +
> > + if (!intel_dp)
> > + return;
> > +
> > + dev_priv = dp_to_i915(intel_dp);
> > + if (!CAN_PSR(dev_priv) || intel_dp != dev_priv->psr.dp)
> > +
Re: [Intel-gfx] [PATCH v4] drm/i915/psr: Force PSR probe only after full initialization
On Thu, 2020-02-20 at 15:15 -0800, José Roberto de Souza wrote:
> Commit 60c6a14b489b ("drm/i915/display: Force the state compute phase
> once to enable PSR") was forcing the state compute too earlier
> causing errors because not everything was initialized, so here
> moving to the end of i915_driver_modeset_probe() when the display is
> all initialized.
>
> Also fixing the place where it disarm the force probe as during the
> atomic check phase errors could happen like the ones due locking and
> it would cause PSR to never be enabled if that happens.
> Leaving the disarm to the atomic commit phase, intel_psr_enable() or
> intel_psr_update() will be called even if the current state do not
> allow PSR to be enabled.
>
> v2: Check if intel_dp is null in intel_psr_force_mode_changed_set()
> v3: Check intel_dp before get dev_priv
> v4:
> - renamed intel_psr_force_mode_changed_set() to
> intel_psr_set_force_mode_changed()
> - removed the set parameter from intel_psr_set_force_mode_changed()
> - not calling intel_psr_set_force_mode_changed() from
> intel_psr_enable/update(), directly setting it after the same checks
> that intel_psr_set_force_mode_changed() does
> - moved intel_psr_set_force_mode_changed() arm call to
> i915_driver_modeset_probe() as it is a better for a PSR call, all the
> functions calls happening between the old and the new function call
> will cause issue
>
> Fixes: 60c6a14b489b ("drm/i915/display: Force the state compute phase
> once to enable PSR")
> Closes: https://gitlab.freedesktop.org/drm/intel/issues/1151
> Tested-by: Ross Zwisler
> Reported-by: Ross Zwisler
> Cc: Gwan-gyeong Mun
> Cc: Jani Nikula
> Cc: Anshuman Gupta
> Signed-off-by: José Roberto de Souza
> ---
> drivers/gpu/drm/i915/display/intel_psr.c | 25
>
> drivers/gpu/drm/i915/display/intel_psr.h | 1 +
> drivers/gpu/drm/i915/i915_drv.c | 3 +++
> drivers/gpu/drm/i915/i915_drv.h | 2 +-
> 4 files changed, 26 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> b/drivers/gpu/drm/i915/display/intel_psr.c
> index b4942b6445ae..7e754201f54d 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -936,10 +936,12 @@ void intel_psr_enable(struct intel_dp
> *intel_dp,
> {
> struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
>
> - if (!crtc_state->has_psr)
Enabling of crtc_state->has_psr is handled on
intel_psr_compute_config().
And the intel_psr_compute_config() checks "CAN_PSR(dev_priv) and
intel_dp != dev_priv->psr.dp".
therefore if we have this line "if (!crtc_state->has_psr)", we don't
need to add "if (!CAN_PSR(dev_priv) || dev_priv->psr.dp != intel_dp)"
Except that, looks good to me.
Reviewed-by: Gwan-gyeong Mun
> + if (!CAN_PSR(dev_priv) || dev_priv->psr.dp != intel_dp)
> return;
>
> - if (drm_WARN_ON(_priv->drm, !CAN_PSR(dev_priv)))
> + dev_priv->psr.force_mode_changed = false;
> +
> + if (!crtc_state->has_psr)
> return;
>
> drm_WARN_ON(_priv->drm, dev_priv->drrs.dp);
> @@ -1099,6 +1101,8 @@ void intel_psr_update(struct intel_dp
> *intel_dp,
> if (!CAN_PSR(dev_priv) || READ_ONCE(psr->dp) != intel_dp)
> return;
>
> + dev_priv->psr.force_mode_changed = false;
> +
> mutex_lock(_priv->psr.lock);
>
> enable = crtc_state->has_psr && psr_global_enabled(dev_priv);
> @@ -1629,7 +1633,7 @@ void intel_psr_atomic_check(struct
> drm_connector *connector,
> struct drm_crtc_state *crtc_state;
>
> if (!CAN_PSR(dev_priv) || !new_state->crtc ||
> - dev_priv->psr.initially_probed)
> + !dev_priv->psr.force_mode_changed)
> return;
>
> intel_connector = to_intel_connector(connector);
> @@ -1640,5 +1644,18 @@ void intel_psr_atomic_check(struct
> drm_connector *connector,
> crtc_state = drm_atomic_get_new_crtc_state(new_state->state,
> new_state->crtc);
> crtc_state->mode_changed = true;
> - dev_priv->psr.initially_probed = true;
> +}
> +
> +void intel_psr_set_force_mode_changed(struct intel_dp *intel_dp)
> +{
> + struct drm_i915_private *dev_priv;
> +
> + if (!intel_dp)
> + return;
> +
> + dev_priv = dp_to_i915(intel_dp);
> + if (!CAN_PSR(dev_priv) || intel_dp != dev_priv->psr.dp)
> + return;
> +
> + dev_priv->psr.force_mode_changed = true;
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.h
> b/drivers/gpu/drm/i915/display/intel_psr.h
> index c58a1d438808..274fc6bb6221 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.h
> +++ b/drivers/gpu/drm/i915/display/intel_psr.h
> @@ -40,5 +40,6 @@ bool intel_psr_enabled(struct intel_dp *intel_dp);
> void intel_psr_atomic_check(struct drm_connector *connector,
> struct drm_connector_state *old_state,
> struct
[Intel-gfx] [PATCH v4] drm/i915/psr: Force PSR probe only after full initialization
Commit 60c6a14b489b ("drm/i915/display: Force the state compute phase
once to enable PSR") was forcing the state compute too earlier
causing errors because not everything was initialized, so here
moving to the end of i915_driver_modeset_probe() when the display is
all initialized.
Also fixing the place where it disarm the force probe as during the
atomic check phase errors could happen like the ones due locking and
it would cause PSR to never be enabled if that happens.
Leaving the disarm to the atomic commit phase, intel_psr_enable() or
intel_psr_update() will be called even if the current state do not
allow PSR to be enabled.
v2: Check if intel_dp is null in intel_psr_force_mode_changed_set()
v3: Check intel_dp before get dev_priv
v4:
- renamed intel_psr_force_mode_changed_set() to
intel_psr_set_force_mode_changed()
- removed the set parameter from intel_psr_set_force_mode_changed()
- not calling intel_psr_set_force_mode_changed() from
intel_psr_enable/update(), directly setting it after the same checks
that intel_psr_set_force_mode_changed() does
- moved intel_psr_set_force_mode_changed() arm call to
i915_driver_modeset_probe() as it is a better for a PSR call, all the
functions calls happening between the old and the new function call
will cause issue
Fixes: 60c6a14b489b ("drm/i915/display: Force the state compute phase once to
enable PSR")
Closes: https://gitlab.freedesktop.org/drm/intel/issues/1151
Tested-by: Ross Zwisler
Reported-by: Ross Zwisler
Cc: Gwan-gyeong Mun
Cc: Jani Nikula
Cc: Anshuman Gupta
Signed-off-by: José Roberto de Souza
---
drivers/gpu/drm/i915/display/intel_psr.c | 25
drivers/gpu/drm/i915/display/intel_psr.h | 1 +
drivers/gpu/drm/i915/i915_drv.c | 3 +++
drivers/gpu/drm/i915/i915_drv.h | 2 +-
4 files changed, 26 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
b/drivers/gpu/drm/i915/display/intel_psr.c
index b4942b6445ae..7e754201f54d 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -936,10 +936,12 @@ void intel_psr_enable(struct intel_dp *intel_dp,
{
struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
- if (!crtc_state->has_psr)
+ if (!CAN_PSR(dev_priv) || dev_priv->psr.dp != intel_dp)
return;
- if (drm_WARN_ON(_priv->drm, !CAN_PSR(dev_priv)))
+ dev_priv->psr.force_mode_changed = false;
+
+ if (!crtc_state->has_psr)
return;
drm_WARN_ON(_priv->drm, dev_priv->drrs.dp);
@@ -1099,6 +1101,8 @@ void intel_psr_update(struct intel_dp *intel_dp,
if (!CAN_PSR(dev_priv) || READ_ONCE(psr->dp) != intel_dp)
return;
+ dev_priv->psr.force_mode_changed = false;
+
mutex_lock(_priv->psr.lock);
enable = crtc_state->has_psr && psr_global_enabled(dev_priv);
@@ -1629,7 +1633,7 @@ void intel_psr_atomic_check(struct drm_connector
*connector,
struct drm_crtc_state *crtc_state;
if (!CAN_PSR(dev_priv) || !new_state->crtc ||
- dev_priv->psr.initially_probed)
+ !dev_priv->psr.force_mode_changed)
return;
intel_connector = to_intel_connector(connector);
@@ -1640,5 +1644,18 @@ void intel_psr_atomic_check(struct drm_connector
*connector,
crtc_state = drm_atomic_get_new_crtc_state(new_state->state,
new_state->crtc);
crtc_state->mode_changed = true;
- dev_priv->psr.initially_probed = true;
+}
+
+void intel_psr_set_force_mode_changed(struct intel_dp *intel_dp)
+{
+ struct drm_i915_private *dev_priv;
+
+ if (!intel_dp)
+ return;
+
+ dev_priv = dp_to_i915(intel_dp);
+ if (!CAN_PSR(dev_priv) || intel_dp != dev_priv->psr.dp)
+ return;
+
+ dev_priv->psr.force_mode_changed = true;
}
diff --git a/drivers/gpu/drm/i915/display/intel_psr.h
b/drivers/gpu/drm/i915/display/intel_psr.h
index c58a1d438808..274fc6bb6221 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.h
+++ b/drivers/gpu/drm/i915/display/intel_psr.h
@@ -40,5 +40,6 @@ bool intel_psr_enabled(struct intel_dp *intel_dp);
void intel_psr_atomic_check(struct drm_connector *connector,
struct drm_connector_state *old_state,
struct drm_connector_state *new_state);
+void intel_psr_set_force_mode_changed(struct intel_dp *intel_dp);
#endif /* __INTEL_PSR_H__ */
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 759d333448e1..066934327345 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -58,6 +58,7 @@
#include "display/intel_hotplug.h"
#include "display/intel_overlay.h"
#include "display/intel_pipe_crc.h"
+#include "display/intel_psr.h"
#include "display/intel_sprite.h"
#include "display/intel_vga.h"
@@ -264,6 +265,8 @@ static int
