diff mbox series

drm/i915: Force initial atomic check in all eDP panels

Message ID 20201028210712.66475-1-jose.souza@intel.com
State New, archived
Headers show
Series drm/i915: Force initial atomic check in all eDP panels | expand

Commit Message

Souza, Jose Oct. 28, 2020, 9:07 p.m. UTC
After commit 00e5deb5c4f5 ("drm/i915: Fix encoder lookup during PSR
atomic check") dig_port was not being used but while fixing it I
realized that would be better to mark all CRTCs that has a eDP
connector as needing to have their state computed.
The principal reason is that in future we will support PSR in
multiple panels.
And this is only forcing the state compute if no register change is
need our atomic handling will just ignore this CRTC + connector
during the atomic commit phase.

Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/display/intel_psr.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

Comments

Imre Deak Oct. 29, 2020, 4:12 p.m. UTC | #1
On Wed, Oct 28, 2020 at 02:07:12PM -0700, José Roberto de Souza wrote:
> After commit 00e5deb5c4f5 ("drm/i915: Fix encoder lookup during PSR
> atomic check") dig_port was not being used but while fixing it I
> realized that would be better to mark all CRTCs that has a eDP
> connector as needing to have their state computed.
> The principal reason is that in future we will support PSR in
> multiple panels.
> And this is only forcing the state compute if no register change is
> need our atomic handling will just ignore this CRTC + connector
> during the atomic commit phase.
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_psr.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> index 1576c3722d0b..b5441f0b5b58 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -1875,17 +1875,11 @@ void intel_psr_atomic_check(struct drm_connector *connector,
>  			    struct drm_connector_state *new_state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(connector->dev);
> -	struct intel_connector *intel_connector;

Arg, compiler didn't warn for this.

> -	struct intel_digital_port *dig_port;
>  	struct drm_crtc_state *crtc_state;
>  
>  	if (!CAN_PSR(dev_priv) || !new_state->crtc ||
> -	    !dev_priv->psr.force_mode_changed)
> -		return;
> -
> -	intel_connector = to_intel_connector(connector);
> -	dig_port = enc_to_dig_port(to_intel_encoder(new_state->best_encoder));
> -	if (dev_priv->psr.dp != &dig_port->dp)
> +	    !dev_priv->psr.force_mode_changed ||
> +	    connector->connector_type != DRM_MODE_CONNECTOR_eDP)
>  		return;

Can't you simplify even more by an is_edp && CAN_PSR check in
intel_dp_initial_fastset_check() instead of the psr.force_mode_changed
logic?

>  
>  	crtc_state = drm_atomic_get_new_crtc_state(new_state->state,
> -- 
> 2.29.1
>
Souza, Jose Oct. 30, 2020, 4:26 p.m. UTC | #2
On Thu, 2020-10-29 at 18:12 +0200, Imre Deak wrote:
> On Wed, Oct 28, 2020 at 02:07:12PM -0700, José Roberto de Souza wrote:
> > After commit 00e5deb5c4f5 ("drm/i915: Fix encoder lookup during PSR
> > atomic check") dig_port was not being used but while fixing it I
> > realized that would be better to mark all CRTCs that has a eDP
> > connector as needing to have their state computed.
> > The principal reason is that in future we will support PSR in
> > multiple panels.
> > And this is only forcing the state compute if no register change is
> > need our atomic handling will just ignore this CRTC + connector
> > during the atomic commit phase.
> > 
> > Cc: Imre Deak <imre.deak@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_psr.c | 10 ++--------
> >  1 file changed, 2 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> > index 1576c3722d0b..b5441f0b5b58 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -1875,17 +1875,11 @@ void intel_psr_atomic_check(struct drm_connector *connector,
> >  			    struct drm_connector_state *new_state)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(connector->dev);
> > -	struct intel_connector *intel_connector;
> 
> Arg, compiler didn't warn for this.
> 
> > -	struct intel_digital_port *dig_port;
> >  	struct drm_crtc_state *crtc_state;
> >  
> > 
> > 
> > 
> >  	if (!CAN_PSR(dev_priv) || !new_state->crtc ||
> > -	    !dev_priv->psr.force_mode_changed)
> > -		return;
> > -
> > -	intel_connector = to_intel_connector(connector);
> > -	dig_port = enc_to_dig_port(to_intel_encoder(new_state->best_encoder));
> > -	if (dev_priv->psr.dp != &dig_port->dp)
> > +	    !dev_priv->psr.force_mode_changed ||
> > +	    connector->connector_type != DRM_MODE_CONNECTOR_eDP)
> >  		return;
> 
> Can't you simplify even more by an is_edp && CAN_PSR check in
> intel_dp_initial_fastset_check() instead of the psr.force_mode_changed
> logic?

In the past we ha problems forcing a modeset from the initial commit but with this new callback I guess it was fixed.
I sent a test to try-bot, if that work we simply and remove some more code around it.

thanks for the headsup.

> 
> >  
> > 
> > 
> > 
> >  	crtc_state = drm_atomic_get_new_crtc_state(new_state->state,
> > -- 
> > 2.29.1
> >
Imre Deak Oct. 30, 2020, 4:48 p.m. UTC | #3
On Fri, Oct 30, 2020 at 06:26:06PM +0200, Souza, Jose wrote:
> On Thu, 2020-10-29 at 18:12 +0200, Imre Deak wrote:
> > On Wed, Oct 28, 2020 at 02:07:12PM -0700, José Roberto de Souza wrote:
> > > After commit 00e5deb5c4f5 ("drm/i915: Fix encoder lookup during PSR
> > > atomic check") dig_port was not being used but while fixing it I
> > > realized that would be better to mark all CRTCs that has a eDP
> > > connector as needing to have their state computed.
> > > The principal reason is that in future we will support PSR in
> > > multiple panels.
> > > And this is only forcing the state compute if no register change is
> > > need our atomic handling will just ignore this CRTC + connector
> > > during the atomic commit phase.
> > > 
> > > Cc: Imre Deak <imre.deak@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_psr.c | 10 ++--------
> > >  1 file changed, 2 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> > > index 1576c3722d0b..b5441f0b5b58 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > @@ -1875,17 +1875,11 @@ void intel_psr_atomic_check(struct drm_connector *connector,
> > >  			    struct drm_connector_state *new_state)
> > >  {
> > >  	struct drm_i915_private *dev_priv = to_i915(connector->dev);
> > > -	struct intel_connector *intel_connector;
> > 
> > Arg, compiler didn't warn for this.
> > 
> > > -	struct intel_digital_port *dig_port;
> > >  	struct drm_crtc_state *crtc_state;
> > >  
> > > 
> > > 
> > > 
> > >  	if (!CAN_PSR(dev_priv) || !new_state->crtc ||
> > > -	    !dev_priv->psr.force_mode_changed)
> > > -		return;
> > > -
> > > -	intel_connector = to_intel_connector(connector);
> > > -	dig_port = enc_to_dig_port(to_intel_encoder(new_state->best_encoder));
> > > -	if (dev_priv->psr.dp != &dig_port->dp)
> > > +	    !dev_priv->psr.force_mode_changed ||
> > > +	    connector->connector_type != DRM_MODE_CONNECTOR_eDP)
> > >  		return;
> > 
> > Can't you simplify even more by an is_edp && CAN_PSR check in
> > intel_dp_initial_fastset_check() instead of the psr.force_mode_changed
> > logic?
> 
> In the past we ha problems forcing a modeset from the initial commit
> but with this new callback I guess it was fixed. 

Ok. Note that now a forced initial modeset can anyway happen due to
other reasons (DSC, wrong DP link rate). Also imo the only way to
guarantee enabling PSR (the purpose of psr.force_mode_changed afaiu) is
to do it in the initial modeset, (no subsequent modeset is guaranteed).

> I sent a test to try-bot, if that work we simply and remove some more
> code around it.

Ok.

> thanks for the headsup.

The fastset hook was just recently added, so thought it's better to have
all similar modeset forcing at one place. Btw, if
intel_pipe_config_compare() wouldn't show a mismatch for the PSR state
(I suppose it doesn't) then intel_crtc_check_fastset() will reset
mode_changed, so probably better to use connectors_changed.

--Imre
Imre Deak Oct. 30, 2020, 4:57 p.m. UTC | #4
On Fri, Oct 30, 2020 at 06:48:12PM +0200, Imre Deak wrote:
> [...]
>
> Btw, if intel_pipe_config_compare() wouldn't show a mismatch for the
> PSR state (I suppose it doesn't) then intel_crtc_check_fastset() will
> reset mode_changed, so probably better to use connectors_changed.

Ah, nvm, PSR will get enabled even during a fastset, so you don't want
to force a full modeset -> mode_changed is ok.

> 
> --Imre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index 1576c3722d0b..b5441f0b5b58 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -1875,17 +1875,11 @@  void intel_psr_atomic_check(struct drm_connector *connector,
 			    struct drm_connector_state *new_state)
 {
 	struct drm_i915_private *dev_priv = to_i915(connector->dev);
-	struct intel_connector *intel_connector;
-	struct intel_digital_port *dig_port;
 	struct drm_crtc_state *crtc_state;
 
 	if (!CAN_PSR(dev_priv) || !new_state->crtc ||
-	    !dev_priv->psr.force_mode_changed)
-		return;
-
-	intel_connector = to_intel_connector(connector);
-	dig_port = enc_to_dig_port(to_intel_encoder(new_state->best_encoder));
-	if (dev_priv->psr.dp != &dig_port->dp)
+	    !dev_priv->psr.force_mode_changed ||
+	    connector->connector_type != DRM_MODE_CONNECTOR_eDP)
 		return;
 
 	crtc_state = drm_atomic_get_new_crtc_state(new_state->state,