| Message ID | 20190411143632.26838-2-gwan-gyeong.mun@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Add detection of changing of edid on between suspend and resume | expand |
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);
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);
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(-)