| Message ID | 20220303233222.4698-1-manasi.d.navare@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v7] drm/i915/display/vrr: Reset VRR capable property on a long hpd | expand |
Hi Ville, Here the VRR set/reset moved to set/unset edid like you suggested. Anything else needed here? Manasi On Thu, Mar 03, 2022 at 03:32:22PM -0800, Manasi Navare wrote: > With some VRR panels, user can turn VRR ON/OFF on the fly from the panel settings. > When VRR is turned OFF ,sends a long HPD to the driver clearing the Ignore MSA bit > in the DPCD. Currently the driver parses that onevery HPD but fails to reset > the corresponding VRR Capable Connector property. > Hence the userspace still sees this as VRR Capable panel which is incorrect. > > Fix this by explicitly resetting the connector property. > > v2: Reset vrr capable if status == connector_disconnected > v3: Use i915 and use bool vrr_capable (Jani Nikula) > v4: Move vrr_capable to after update modes call (Jani N) > Remove the redundant comment (Jan N) > v5: Fixes the regression on older platforms by reseting the VRR > only if HAS_VRR > v6: Remove the checks from driver, add in drm core before > setting VRR prop (Ville) > v7: Move VRR set/reset to set/unset_edid (Ville) > > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Fixes: 390a1f8beb87 ("Revert "drm/i915/display/vrr: Reset VRR capable property on a long hpd") > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > --- > drivers/gpu/drm/i915/display/intel_dp.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > index d6ef33096bb6..1d0f8fc39005 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -4385,13 +4385,20 @@ intel_dp_update_420(struct intel_dp *intel_dp) > static void > intel_dp_set_edid(struct intel_dp *intel_dp) > { > + struct drm_i915_private *i915 = dp_to_i915(intel_dp); > struct intel_connector *connector = intel_dp->attached_connector; > struct edid *edid; > + bool vrr_capable; > > intel_dp_unset_edid(intel_dp); > edid = intel_dp_get_edid(intel_dp); > connector->detect_edid = edid; > > + vrr_capable = intel_vrr_is_capable(&connector->base); > + drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] VRR capable: %s\n", > + connector->base.base.id, connector->base.name, str_yes_no(vrr_capable)); > + drm_connector_set_vrr_capable_property(&connector->base, vrr_capable); > + > intel_dp_update_dfp(intel_dp, edid); > intel_dp_update_420(intel_dp); > > @@ -4424,6 +4431,9 @@ intel_dp_unset_edid(struct intel_dp *intel_dp) > > intel_dp->dfp.ycbcr_444_to_420 = false; > connector->base.ycbcr_420_allowed = false; > + > + drm_connector_set_vrr_capable_property(&connector->base, > + false); > } > > static int > @@ -4574,14 +4584,9 @@ static int intel_dp_get_modes(struct drm_connector *connector) > int num_modes = 0; > > edid = intel_connector->detect_edid; > - if (edid) { > + if (edid) > num_modes = intel_connector_update_modes(connector, edid); > > - 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 */ > if (intel_dp_is_edp(intel_attached_dp(intel_connector)) && > intel_connector->panel.fixed_mode) { > -- > 2.19.1 >
On Thu, Mar 03, 2022 at 03:32:22PM -0800, Manasi Navare wrote: > With some VRR panels, user can turn VRR ON/OFF on the fly from the panel settings. > When VRR is turned OFF ,sends a long HPD to the driver clearing the Ignore MSA bit > in the DPCD. Currently the driver parses that onevery HPD but fails to reset > the corresponding VRR Capable Connector property. > Hence the userspace still sees this as VRR Capable panel which is incorrect. > > Fix this by explicitly resetting the connector property. > > v2: Reset vrr capable if status == connector_disconnected > v3: Use i915 and use bool vrr_capable (Jani Nikula) > v4: Move vrr_capable to after update modes call (Jani N) > Remove the redundant comment (Jan N) > v5: Fixes the regression on older platforms by reseting the VRR > only if HAS_VRR > v6: Remove the checks from driver, add in drm core before > setting VRR prop (Ville) > v7: Move VRR set/reset to set/unset_edid (Ville) > > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Fixes: 390a1f8beb87 ("Revert "drm/i915/display/vrr: Reset VRR capable property on a long hpd") > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> Looks reasoanble enough to me. Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> But be careful when pushing this since I don't think the drm_connector_set_vrr_capable_property() change has percolated into drm-intel-next yet. > --- > drivers/gpu/drm/i915/display/intel_dp.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > index d6ef33096bb6..1d0f8fc39005 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -4385,13 +4385,20 @@ intel_dp_update_420(struct intel_dp *intel_dp) > static void > intel_dp_set_edid(struct intel_dp *intel_dp) > { > + struct drm_i915_private *i915 = dp_to_i915(intel_dp); > struct intel_connector *connector = intel_dp->attached_connector; > struct edid *edid; > + bool vrr_capable; > > intel_dp_unset_edid(intel_dp); > edid = intel_dp_get_edid(intel_dp); > connector->detect_edid = edid; > > + vrr_capable = intel_vrr_is_capable(&connector->base); > + drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] VRR capable: %s\n", > + connector->base.base.id, connector->base.name, str_yes_no(vrr_capable)); > + drm_connector_set_vrr_capable_property(&connector->base, vrr_capable); > + > intel_dp_update_dfp(intel_dp, edid); > intel_dp_update_420(intel_dp); > > @@ -4424,6 +4431,9 @@ intel_dp_unset_edid(struct intel_dp *intel_dp) > > intel_dp->dfp.ycbcr_444_to_420 = false; > connector->base.ycbcr_420_allowed = false; > + > + drm_connector_set_vrr_capable_property(&connector->base, > + false); > } > > static int > @@ -4574,14 +4584,9 @@ static int intel_dp_get_modes(struct drm_connector *connector) > int num_modes = 0; > > edid = intel_connector->detect_edid; > - if (edid) { > + if (edid) > num_modes = intel_connector_update_modes(connector, edid); > > - 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 */ > if (intel_dp_is_edp(intel_attached_dp(intel_connector)) && > intel_connector->panel.fixed_mode) { > -- > 2.19.1
On Thu, Mar 10, 2022 at 01:43:57AM +0200, Ville Syrjälä wrote: > On Thu, Mar 03, 2022 at 03:32:22PM -0800, Manasi Navare wrote: > > With some VRR panels, user can turn VRR ON/OFF on the fly from the panel settings. > > When VRR is turned OFF ,sends a long HPD to the driver clearing the Ignore MSA bit > > in the DPCD. Currently the driver parses that onevery HPD but fails to reset > > the corresponding VRR Capable Connector property. > > Hence the userspace still sees this as VRR Capable panel which is incorrect. > > > > Fix this by explicitly resetting the connector property. > > > > v2: Reset vrr capable if status == connector_disconnected > > v3: Use i915 and use bool vrr_capable (Jani Nikula) > > v4: Move vrr_capable to after update modes call (Jani N) > > Remove the redundant comment (Jan N) > > v5: Fixes the regression on older platforms by reseting the VRR > > only if HAS_VRR > > v6: Remove the checks from driver, add in drm core before > > setting VRR prop (Ville) > > v7: Move VRR set/reset to set/unset_edid (Ville) > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Fixes: 390a1f8beb87 ("Revert "drm/i915/display/vrr: Reset VRR capable property on a long hpd") > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > Looks reasoanble enough to me. > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > But be careful when pushing this since I don't think > the drm_connector_set_vrr_capable_property() change has > percolated into drm-intel-next yet. Okay I can wait then to push until that gets percolated from drm-misc Manasi > > > --- > > drivers/gpu/drm/i915/display/intel_dp.c | 17 +++++++++++------ > > 1 file changed, 11 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > > index d6ef33096bb6..1d0f8fc39005 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > @@ -4385,13 +4385,20 @@ intel_dp_update_420(struct intel_dp *intel_dp) > > static void > > intel_dp_set_edid(struct intel_dp *intel_dp) > > { > > + struct drm_i915_private *i915 = dp_to_i915(intel_dp); > > struct intel_connector *connector = intel_dp->attached_connector; > > struct edid *edid; > > + bool vrr_capable; > > > > intel_dp_unset_edid(intel_dp); > > edid = intel_dp_get_edid(intel_dp); > > connector->detect_edid = edid; > > > > + vrr_capable = intel_vrr_is_capable(&connector->base); > > + drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] VRR capable: %s\n", > > + connector->base.base.id, connector->base.name, str_yes_no(vrr_capable)); > > + drm_connector_set_vrr_capable_property(&connector->base, vrr_capable); > > + > > intel_dp_update_dfp(intel_dp, edid); > > intel_dp_update_420(intel_dp); > > > > @@ -4424,6 +4431,9 @@ intel_dp_unset_edid(struct intel_dp *intel_dp) > > > > intel_dp->dfp.ycbcr_444_to_420 = false; > > connector->base.ycbcr_420_allowed = false; > > + > > + drm_connector_set_vrr_capable_property(&connector->base, > > + false); > > } > > > > static int > > @@ -4574,14 +4584,9 @@ static int intel_dp_get_modes(struct drm_connector *connector) > > int num_modes = 0; > > > > edid = intel_connector->detect_edid; > > - if (edid) { > > + if (edid) > > num_modes = intel_connector_update_modes(connector, edid); > > > > - 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 */ > > if (intel_dp_is_edp(intel_attached_dp(intel_connector)) && > > intel_connector->panel.fixed_mode) { > > -- > > 2.19.1 > > -- > Ville Syrjälä > Intel
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index d6ef33096bb6..1d0f8fc39005 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -4385,13 +4385,20 @@ intel_dp_update_420(struct intel_dp *intel_dp) static void intel_dp_set_edid(struct intel_dp *intel_dp) { + struct drm_i915_private *i915 = dp_to_i915(intel_dp); struct intel_connector *connector = intel_dp->attached_connector; struct edid *edid; + bool vrr_capable; intel_dp_unset_edid(intel_dp); edid = intel_dp_get_edid(intel_dp); connector->detect_edid = edid; + vrr_capable = intel_vrr_is_capable(&connector->base); + drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s] VRR capable: %s\n", + connector->base.base.id, connector->base.name, str_yes_no(vrr_capable)); + drm_connector_set_vrr_capable_property(&connector->base, vrr_capable); + intel_dp_update_dfp(intel_dp, edid); intel_dp_update_420(intel_dp); @@ -4424,6 +4431,9 @@ intel_dp_unset_edid(struct intel_dp *intel_dp) intel_dp->dfp.ycbcr_444_to_420 = false; connector->base.ycbcr_420_allowed = false; + + drm_connector_set_vrr_capable_property(&connector->base, + false); } static int @@ -4574,14 +4584,9 @@ static int intel_dp_get_modes(struct drm_connector *connector) int num_modes = 0; edid = intel_connector->detect_edid; - if (edid) { + if (edid) num_modes = intel_connector_update_modes(connector, edid); - 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 */ if (intel_dp_is_edp(intel_attached_dp(intel_connector)) && intel_connector->panel.fixed_mode) {
With some VRR panels, user can turn VRR ON/OFF on the fly from the panel settings. When VRR is turned OFF ,sends a long HPD to the driver clearing the Ignore MSA bit in the DPCD. Currently the driver parses that onevery HPD but fails to reset the corresponding VRR Capable Connector property. Hence the userspace still sees this as VRR Capable panel which is incorrect. Fix this by explicitly resetting the connector property. v2: Reset vrr capable if status == connector_disconnected v3: Use i915 and use bool vrr_capable (Jani Nikula) v4: Move vrr_capable to after update modes call (Jani N) Remove the redundant comment (Jan N) v5: Fixes the regression on older platforms by reseting the VRR only if HAS_VRR v6: Remove the checks from driver, add in drm core before setting VRR prop (Ville) v7: Move VRR set/reset to set/unset_edid (Ville) Cc: Jani Nikula <jani.nikula@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Fixes: 390a1f8beb87 ("Revert "drm/i915/display/vrr: Reset VRR capable property on a long hpd") Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> --- drivers/gpu/drm/i915/display/intel_dp.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)