diff mbox series

Revert "drm/i915/display/vrr: Reset VRR capable property on a long hpd"

Message ID 20220223081810.19917-1-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series Revert "drm/i915/display/vrr: Reset VRR capable property on a long hpd" | expand

Commit Message

Ville Syrjälä Feb. 23, 2022, 8:18 a.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

This reverts commit 9bc34b4d0f3cb368241684cc5e0445d435dded44.

Just oopses on most machines.

Cc: Manasi Navare <manasi.d.navare@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

Comments

Jani Nikula Feb. 23, 2022, 8:29 a.m. UTC | #1 
On Wed, 23 Feb 2022, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> This reverts commit 9bc34b4d0f3cb368241684cc5e0445d435dded44.
>
> Just oopses on most machines.
>
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Auch, "ERROR: It appears as if the changes made in Patchwork_22288
prevented too many machines from booting." [1].

Acked-by: Jani Nikula <jani.nikula@intel.com>


[1] https://lore.kernel.org/r/164506400291.18053.4904919379639827273@emeril.freedesktop.org


> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 17 ++++-------------
>  1 file changed, 4 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 929e9b6febf1..1046e7fe310a 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -4455,12 +4455,6 @@ intel_dp_detect(struct drm_connector *connector,
>  		memset(&intel_dp->compliance, 0, sizeof(intel_dp->compliance));
>  		memset(intel_dp->dsc_dpcd, 0, sizeof(intel_dp->dsc_dpcd));
>  
> -		/* Reset VRR Capable property */
> -		drm_dbg_kms(&dev_priv->drm, "[CONNECTOR:%d:%s] VRR capable: FALSE\n",
> -			    connector->base.id, connector->name);
> -		drm_connector_set_vrr_capable_property(connector,
> -						       false);
> -
>  		if (intel_dp->is_mst) {
>  			drm_dbg_kms(&dev_priv->drm,
>  				    "MST device may have disappeared %d vs %d\n",
> @@ -4575,18 +4569,15 @@ static int intel_dp_get_modes(struct drm_connector *connector)
>  {
>  	struct intel_connector *intel_connector = to_intel_connector(connector);
>  	struct edid *edid;
> -	struct drm_i915_private *i915 = to_i915(connector->dev);
>  	int num_modes = 0;
>  
>  	edid = intel_connector->detect_edid;
>  	if (edid) {
> -		bool vrr_capable;
> -
>  		num_modes = intel_connector_update_modes(connector, edid);
> -		vrr_capable = intel_vrr_is_capable(connector);
> -		drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] VRR capable: %s\n",
> -			    connector->base.id, connector->name, yesno(vrr_capable));
> -		drm_connector_set_vrr_capable_property(connector, vrr_capable);
> +
> +		if (intel_vrr_is_capable(connector))
> +			drm_connector_set_vrr_capable_property(connector,
> +							       true);
>  	}
>  
>  	/* Also add fixed mode, which may or may not be present in EDID */
Navare, Manasi Feb. 23, 2022, 7:26 p.m. UTC | #2 
On Wed, Feb 23, 2022 at 10:29:16AM +0200, Jani Nikula wrote:
> On Wed, 23 Feb 2022, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > This reverts commit 9bc34b4d0f3cb368241684cc5e0445d435dded44.
> >
> > Just oopses on most machines.
> >
> > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Auch, "ERROR: It appears as if the changes made in Patchwork_22288
> prevented too many machines from booting." [1].

Why would it prevent machines from booting where this only sets the property to false and we tested this
on VRR panels and it works as expected and infact is needed to turn VRR off if disabled from
the panel and also on the connector disconnect.

Our customers already want this.

We should look at whther this prevented the machines from booting or this just revealed some other issue.

> 
> Acked-by: Jani Nikula <jani.nikula@intel.com>
> 
> 
> [1] https://lore.kernel.org/r/164506400291.18053.4904919379639827273@emeril.freedesktop.org
> 
> 
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp.c | 17 ++++-------------
> >  1 file changed, 4 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 929e9b6febf1..1046e7fe310a 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -4455,12 +4455,6 @@ intel_dp_detect(struct drm_connector *connector,
> >  		memset(&intel_dp->compliance, 0, sizeof(intel_dp->compliance));
> >  		memset(intel_dp->dsc_dpcd, 0, sizeof(intel_dp->dsc_dpcd));
> >  
> > -		/* Reset VRR Capable property */
> > -		drm_dbg_kms(&dev_priv->drm, "[CONNECTOR:%d:%s] VRR capable: FALSE\n",
> > -			    connector->base.id, connector->name);
> > -		drm_connector_set_vrr_capable_property(connector,
> > -						       false);

Do we perhaps need a condition here to reset this only if it was set previously or if it is a VRR panel?

Manasi

> > -
> >  		if (intel_dp->is_mst) {
> >  			drm_dbg_kms(&dev_priv->drm,
> >  				    "MST device may have disappeared %d vs %d\n",
> > @@ -4575,18 +4569,15 @@ static int intel_dp_get_modes(struct drm_connector *connector)
> >  {
> >  	struct intel_connector *intel_connector = to_intel_connector(connector);
> >  	struct edid *edid;
> > -	struct drm_i915_private *i915 = to_i915(connector->dev);
> >  	int num_modes = 0;
> >  
> >  	edid = intel_connector->detect_edid;
> >  	if (edid) {
> > -		bool vrr_capable;
> > -
> >  		num_modes = intel_connector_update_modes(connector, edid);
> > -		vrr_capable = intel_vrr_is_capable(connector);
> > -		drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] VRR capable: %s\n",
> > -			    connector->base.id, connector->name, yesno(vrr_capable));
> > -		drm_connector_set_vrr_capable_property(connector, vrr_capable);
> > +
> > +		if (intel_vrr_is_capable(connector))
> > +			drm_connector_set_vrr_capable_property(connector,
> > +							       true);
> >  	}
> >  
> >  	/* Also add fixed mode, which may or may not be present in EDID */
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 929e9b6febf1..1046e7fe310a 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -4455,12 +4455,6 @@  intel_dp_detect(struct drm_connector *connector,
 		memset(&intel_dp->compliance, 0, sizeof(intel_dp->compliance));
 		memset(intel_dp->dsc_dpcd, 0, sizeof(intel_dp->dsc_dpcd));
 
-		/* Reset VRR Capable property */
-		drm_dbg_kms(&dev_priv->drm, "[CONNECTOR:%d:%s] VRR capable: FALSE\n",
-			    connector->base.id, connector->name);
-		drm_connector_set_vrr_capable_property(connector,
-						       false);
-
 		if (intel_dp->is_mst) {
 			drm_dbg_kms(&dev_priv->drm,
 				    "MST device may have disappeared %d vs %d\n",
@@ -4575,18 +4569,15 @@  static int intel_dp_get_modes(struct drm_connector *connector)
 {
 	struct intel_connector *intel_connector = to_intel_connector(connector);
 	struct edid *edid;
-	struct drm_i915_private *i915 = to_i915(connector->dev);
 	int num_modes = 0;
 
 	edid = intel_connector->detect_edid;
 	if (edid) {
-		bool vrr_capable;
-
 		num_modes = intel_connector_update_modes(connector, edid);
-		vrr_capable = intel_vrr_is_capable(connector);
-		drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] VRR capable: %s\n",
-			    connector->base.id, connector->name, yesno(vrr_capable));
-		drm_connector_set_vrr_capable_property(connector, vrr_capable);
+
+		if (intel_vrr_is_capable(connector))
+			drm_connector_set_vrr_capable_property(connector,
+							       true);
 	}
 
 	/* Also add fixed mode, which may or may not be present in EDID */