[v5,1/2] drm: Add detection of changing of edid on between suspend and resume

Submitted by Mun, Gwan-gyeong on April 11, 2019, 2:36 p.m.

Details

Message ID 20190411143632.26838-2-gwan-gyeong.mun@intel.com
State New
Headers show
Series "drm: Add detection of changing of edid on between suspend and resume" ( rev: 2 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Mun, Gwan-gyeong April 11, 2019, 2:36 p.m.
The hotplug detection routine of drm_helper_hpd_irq_event() can detect
changing of status of connector, but it can not detect changing of edid.

Following scenario requires detection of changing of edid.

 1) plug display device to a connector
 2) system suspend
 3) unplug 1)'s display device and plug the other display device to a
    connector
 4) system resume

It adds edid check routine when a connector status still remains as
"connector_status_connected".

v2: Add NULL check before comparing of EDIDs.
v3: Make it as part of existing drm_helper_hpd_irq_event() (Stan, Mika)
v4: Rebased
v5: Use a cached edid property blob data of connector instead of adding
    a new detected_edid variable. (Maarten)
    Add an using of reference count for getting a cached edid property
    blob data. (Maarten)

Testcase: igt/kms_chamelium/hdmi-edid-change-during-hibernate
Testcase: igt/kms_chamelium/hdmi-edid-change-during-suspend
Testcase: igt/kms_chamelium/dp-edid-change-during-hibernate
Testcase: igt/kms_chamelium/dp-edid-change-during-suspend

Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
---
 drivers/gpu/drm/drm_probe_helper.c | 34 +++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 6fd08e04b323..27ad7f3dabb7 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -742,7 +742,16 @@  EXPORT_SYMBOL(drm_kms_helper_poll_fini);
  * panels.
  *
  * This helper function is useful for drivers which can't or don't track hotplug
- * interrupts for each connector.
+ * interrupts for each connector. And it also supports a detection of changing
+ * of edid on between suspend and resume when a connector status still remains
+ * as "connector_status_connected".
+ *
+ * Following scenario requires detection of changing of edid.
+ *  1) plug display device to a connector
+ *  2) system suspend
+ *  3) unplug 1)'s display device and plug the other display device to a
+ *     connector
+ *  4) system resume
  *
  * Drivers which support hotplug interrupts for each connector individually and
  * which have a more fine-grained detect logic should bypass this code and
@@ -760,6 +769,7 @@  bool drm_helper_hpd_irq_event(struct drm_device *dev)
 	struct drm_connector *connector;
 	struct drm_connector_list_iter conn_iter;
 	enum drm_connector_status old_status;
+	struct drm_property_blob *old_edid_blob_ptr;
 	bool changed = false;
 
 	if (!dev->mode_config.poll_enabled)
@@ -774,6 +784,11 @@  bool drm_helper_hpd_irq_event(struct drm_device *dev)
 
 		old_status = connector->status;
 
+		if (connector->edid_blob_ptr)
+			old_edid_blob_ptr = drm_property_blob_get(connector->edid_blob_ptr);
+		else
+			old_edid_blob_ptr = NULL;
+
 		connector->status = drm_helper_probe_detect(connector, NULL, false);
 		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s to %s\n",
 			      connector->base.id,
@@ -782,6 +797,23 @@  bool drm_helper_hpd_irq_event(struct drm_device *dev)
 			      drm_get_connector_status_name(connector->status));
 		if (old_status != connector->status)
 			changed = true;
+
+		/* Check changing of edid when a connector status still remains
+		 * as "connector_status_connected".
+		 */
+		if (old_edid_blob_ptr && connector->edid_blob_ptr &&
+		    old_status == connector->status &&
+		    old_status == connector_status_connected) {
+			if (memcmp(old_edid_blob_ptr->data,
+				connector->edid_blob_ptr->data,
+				old_edid_blob_ptr->length)) {
+				changed = true;
+				DRM_DEBUG_KMS("[CONNECTOR:%d:%s] edid updated\n",
+					      connector->base.id,
+					      connector->name);
+			}
+		}
+		drm_property_blob_put(old_edid_blob_ptr);
 	}
 	drm_connector_list_iter_end(&conn_iter);
 	mutex_unlock(&dev->mode_config.mutex);

Comments

On Thu, 2019-04-11 at 17:36 +0300, Gwan-gyeong Mun wrote:
> The hotplug detection routine of drm_helper_hpd_irq_event() can

> detect

> changing of status of connector, but it can not detect changing of

> edid.

> 

> Following scenario requires detection of changing of edid.

> 

>  1) plug display device to a connector

>  2) system suspend

>  3) unplug 1)'s display device and plug the other display device to a

>     connector

>  4) system resume

> 

> It adds edid check routine when a connector status still remains as

> "connector_status_connected".

> 

> v2: Add NULL check before comparing of EDIDs.

> v3: Make it as part of existing drm_helper_hpd_irq_event() (Stan,

> Mika)

> v4: Rebased

> v5: Use a cached edid property blob data of connector instead of

> adding

>     a new detected_edid variable. (Maarten)

>     Add an using of reference count for getting a cached edid

> property

>     blob data. (Maarten)

> 

> Testcase: igt/kms_chamelium/hdmi-edid-change-during-hibernate

> Testcase: igt/kms_chamelium/hdmi-edid-change-during-suspend

> Testcase: igt/kms_chamelium/dp-edid-change-during-hibernate

> Testcase: igt/kms_chamelium/dp-edid-change-during-suspend

> 

> Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>

> ---

>  drivers/gpu/drm/drm_probe_helper.c | 34

> +++++++++++++++++++++++++++++-

>  1 file changed, 33 insertions(+), 1 deletion(-)

> 

> diff --git a/drivers/gpu/drm/drm_probe_helper.c

> b/drivers/gpu/drm/drm_probe_helper.c

> index 6fd08e04b323..27ad7f3dabb7 100644

> --- a/drivers/gpu/drm/drm_probe_helper.c

> +++ b/drivers/gpu/drm/drm_probe_helper.c

> @@ -742,7 +742,16 @@ EXPORT_SYMBOL(drm_kms_helper_poll_fini);

>   * panels.

>   *

>   * This helper function is useful for drivers which can't or don't

> track hotplug

> - * interrupts for each connector.

> + * interrupts for each connector. And it also supports a detection

> of changing

> + * of edid on between suspend and resume when a connector status

> still remains

> + * as "connector_status_connected".

> + *

> + * Following scenario requires detection of changing of edid.

> + *  1) plug display device to a connector

> + *  2) system suspend

> + *  3) unplug 1)'s display device and plug the other display device

> to a

> + *     connector

> + *  4) system resume

>   *

>   * Drivers which support hotplug interrupts for each connector

> individually and

>   * which have a more fine-grained detect logic should bypass this

> code and

> @@ -760,6 +769,7 @@ bool drm_helper_hpd_irq_event(struct drm_device

> *dev)

>  	struct drm_connector *connector;

>  	struct drm_connector_list_iter conn_iter;

>  	enum drm_connector_status old_status;

> +	struct drm_property_blob *old_edid_blob_ptr;

>  	bool changed = false;

>  

>  	if (!dev->mode_config.poll_enabled)

> @@ -774,6 +784,11 @@ bool drm_helper_hpd_irq_event(struct drm_device

> *dev)

>  

>  		old_status = connector->status;

>  

> +		if (connector->edid_blob_ptr)

> +			old_edid_blob_ptr =

> drm_property_blob_get(connector->edid_blob_ptr);

> +		else

> +			old_edid_blob_ptr = NULL;

> +

>  		connector->status = drm_helper_probe_detect(connector,

> NULL, false);

>  		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s

> to %s\n",

>  			      connector->base.id,

> @@ -782,6 +797,23 @@ bool drm_helper_hpd_irq_event(struct drm_device

> *dev)

>  			      drm_get_connector_status_name(connector-

> >status));

>  		if (old_status != connector->status)

>  			changed = true;

> +

> +		/* Check changing of edid when a connector status still

> remains

> +		 * as "connector_status_connected".

> +		 */

> +		if (old_edid_blob_ptr && connector->edid_blob_ptr &&


I guess you don't need to check both old_edid_blob_ptr && connector-
>edid_blob_ptr here. Because if old_edid_blob_ptr is not NULL - it

means that connector->edid_blob_ptr was not NULL for sure. See the 
condition you have added above.
I mean this one:

> +		if (connector->edid_blob_ptr)

> +			old_edid_blob_ptr =

> drm_property_blob_get(connector->edid_blob_ptr);

> +		else

> +			old_edid_blob_ptr = NULL;


So I would check only old_edid_blob_ptr for not being NULL here.



> +		    old_status == connector->status &&

> +		    old_status == connector_status_connected) {

> +			if (memcmp(old_edid_blob_ptr->data,

> +				connector->edid_blob_ptr->data,

> +				old_edid_blob_ptr->length)) {

> +				changed = true;

> +				DRM_DEBUG_KMS("[CONNECTOR:%d:%s] edid

> updated\n",

> +					      connector->base.id,

> +					      connector->name);

> +			}

> +		}

> +		drm_property_blob_put(old_edid_blob_ptr);

>  	}

>  	drm_connector_list_iter_end(&conn_iter);

>  	mutex_unlock(&dev->mode_config.mutex);
On Thu, 2019-04-11 at 17:00 +0100, Lisovskiy, Stanislav wrote:
> On Thu, 2019-04-11 at 17:36 +0300, Gwan-gyeong Mun wrote:

> > The hotplug detection routine of drm_helper_hpd_irq_event() can

> > detect

> > changing of status of connector, but it can not detect changing of

> > edid.

> > 

> > Following scenario requires detection of changing of edid.

> > 

> >  1) plug display device to a connector

> >  2) system suspend

> >  3) unplug 1)'s display device and plug the other display device to

> > a

> >     connector

> >  4) system resume

> > 

> > It adds edid check routine when a connector status still remains as

> > "connector_status_connected".

> > 

> > v2: Add NULL check before comparing of EDIDs.

> > v3: Make it as part of existing drm_helper_hpd_irq_event() (Stan,

> > Mika)

> > v4: Rebased

> > v5: Use a cached edid property blob data of connector instead of

> > adding

> >     a new detected_edid variable. (Maarten)

> >     Add an using of reference count for getting a cached edid

> > property

> >     blob data. (Maarten)

> > 

> > Testcase: igt/kms_chamelium/hdmi-edid-change-during-hibernate

> > Testcase: igt/kms_chamelium/hdmi-edid-change-during-suspend

> > Testcase: igt/kms_chamelium/dp-edid-change-during-hibernate

> > Testcase: igt/kms_chamelium/dp-edid-change-during-suspend

> > 

> > Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>

> > ---

> >  drivers/gpu/drm/drm_probe_helper.c | 34

> > +++++++++++++++++++++++++++++-

> >  1 file changed, 33 insertions(+), 1 deletion(-)

> > 

> > diff --git a/drivers/gpu/drm/drm_probe_helper.c

> > b/drivers/gpu/drm/drm_probe_helper.c

> > index 6fd08e04b323..27ad7f3dabb7 100644

> > --- a/drivers/gpu/drm/drm_probe_helper.c

> > +++ b/drivers/gpu/drm/drm_probe_helper.c

> > @@ -742,7 +742,16 @@ EXPORT_SYMBOL(drm_kms_helper_poll_fini);

> >   * panels.

> >   *

> >   * This helper function is useful for drivers which can't or don't

> > track hotplug

> > - * interrupts for each connector.

> > + * interrupts for each connector. And it also supports a detection

> > of changing

> > + * of edid on between suspend and resume when a connector status

> > still remains

> > + * as "connector_status_connected".

> > + *

> > + * Following scenario requires detection of changing of edid.

> > + *  1) plug display device to a connector

> > + *  2) system suspend

> > + *  3) unplug 1)'s display device and plug the other display

> > device

> > to a

> > + *     connector

> > + *  4) system resume

> >   *

> >   * Drivers which support hotplug interrupts for each connector

> > individually and

> >   * which have a more fine-grained detect logic should bypass this

> > code and

> > @@ -760,6 +769,7 @@ bool drm_helper_hpd_irq_event(struct drm_device

> > *dev)

> >  	struct drm_connector *connector;

> >  	struct drm_connector_list_iter conn_iter;

> >  	enum drm_connector_status old_status;

> > +	struct drm_property_blob *old_edid_blob_ptr;

> >  	bool changed = false;

> >  

> >  	if (!dev->mode_config.poll_enabled)

> > @@ -774,6 +784,11 @@ bool drm_helper_hpd_irq_event(struct

> > drm_device

> > *dev)

> >  

> >  		old_status = connector->status;

> >  

> > +		if (connector->edid_blob_ptr)

> > +			old_edid_blob_ptr =

> > drm_property_blob_get(connector->edid_blob_ptr);

> > +		else

> > +			old_edid_blob_ptr = NULL;

> > +

> >  		connector->status = drm_helper_probe_detect(connector,

> > NULL, false);

> >  		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s

> > to %s\n",

> >  			      connector->base.id,

> > @@ -782,6 +797,23 @@ bool drm_helper_hpd_irq_event(struct

> > drm_device

> > *dev)

> >  			      drm_get_connector_status_name(connector-

> > > status));

> >  		if (old_status != connector->status)

> >  			changed = true;

> > +

> > +		/* Check changing of edid when a connector status still

> > remains

> > +		 * as "connector_status_connected".

> > +		 */

> > +		if (old_edid_blob_ptr && connector->edid_blob_ptr &&

> 

> I guess you don't need to check both old_edid_blob_ptr && connector-

> > edid_blob_ptr here. Because if old_edid_blob_ptr is not NULL - it

> means that connector->edid_blob_ptr was not NULL for sure. See the 

> condition you have added above.

> I mean this one:

> 

> > +		if (connector->edid_blob_ptr)

> > +			old_edid_blob_ptr =

> > drm_property_blob_get(connector->edid_blob_ptr);

> > +		else

> > +			old_edid_blob_ptr = NULL;

> 

> So I would check only old_edid_blob_ptr for not being NULL here.

> 

> 

Hi Stan,

Thank you for checking it.

First, drivers could set edid_blob_ptr to NULL with
drm_connector_update_edid_property().
And drm_helper_probe_detect() could lead updating of an internal edid
of driver and if driver can not get EDID from I2C, it could call
drm_connector_update_edid_property() with NULL.

therefore I think that we should check old and new edid_blob_ptr are
NULL or not.
> 

> > +		    old_status == connector->status &&

> > +		    old_status == connector_status_connected) {

> > +			if (memcmp(old_edid_blob_ptr->data,

> > +				connector->edid_blob_ptr->data,

> > +				old_edid_blob_ptr->length)) {

> > +				changed = true;

> > +				DRM_DEBUG_KMS("[CONNECTOR:%d:%s] edid

> > updated\n",

> > +					      connector->base.id,

> > +					      connector->name);

> > +			}

> > +		}

> > +		drm_property_blob_put(old_edid_blob_ptr);

> >  	}

> >  	drm_connector_list_iter_end(&conn_iter);

> >  	mutex_unlock(&dev->mode_config.mutex);