Re: [PATCH 1/3] drm/omap: Support for HDMI hot plug detection

Hi Tomi,

On Monday 22 May 2017 14:59:09 Tomi Valkeinen wrote:

On 15/05/17 12:03, Peter Ujfalusi wrote:

...

The HPD signal can be used for detecting HDMI cable plug and unplug event without the need for polling the status of the line. This will speed up detecting such event because we do not need to wait for the next poll event to notice the state change.

Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com

drivers/gpu/drm/omapdrm/dss/omapdss.h | 17 +++++++++++++++++ drivers/gpu/drm/omapdrm/omap_connector.c | 32 ++++++++++++++++++++++++++- drivers/gpu/drm/omapdrm/omap_drv.c | 29 ++++++++++++++++++++++++++ 3 files changed, 77 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h index b19dae1fd6c5..1f01669eb610 100644 --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h @@ -25,6 +25,7 @@

#include <video/videomode.h> #include <linux/platform_data/omapdss.h> #include <uapi/drm/drm_mode.h>

+#include <drm/drm_crtc.h>

#define DISPC_IRQ_FRAMEDONE (1 << 0) #define DISPC_IRQ_VSYNC (1 << 1)

@@ -555,6 +556,14 @@ struct omapdss_hdmi_ops {

int (*read_edid)(struct omap_dss_device *dssdev, u8 *buf, int len); bool (*detect)(struct omap_dss_device *dssdev);

• int (*register_hpd_cb)(struct omap_dss_device *dssdev,

• 	       void (*cb)(void *cb_data,
  

• 			  enum drm_connector_status status),
  

• 	       void *cb_data);
  

• void (*unregister_hpd_cb)(struct omap_dss_device *dssdev);

• void (*enable_hpd)(struct omap_dss_device *dssdev);

• void (*disable_hpd)(struct omap_dss_device *dssdev);

• int (*set_hdmi_mode)(struct omap_dss_device *dssdev, bool hdmi_mode); int (*set_infoframe)(struct omap_dss_device *dssdev,

  const struct hdmi_avi_infoframe *avi);

@@ -761,6 +770,14 @@ struct omap_dss_driver {

int (*read_edid)(struct omap_dss_device *dssdev, u8 *buf, int len); bool (*detect)(struct omap_dss_device *dssdev);

• int (*register_hpd_cb)(struct omap_dss_device *dssdev,

• 	       void (*cb)(void *cb_data,
  

• 			  enum drm_connector_status status),
  

• 	       void *cb_data);
  

• void (*unregister_hpd_cb)(struct omap_dss_device *dssdev);

• void (*enable_hpd)(struct omap_dss_device *dssdev);

• void (*disable_hpd)(struct omap_dss_device *dssdev);

• int (*set_hdmi_mode)(struct omap_dss_device *dssdev, bool hdmi_mode); int (*set_hdmi_infoframe)(struct omap_dss_device *dssdev,

  const struct hdmi_avi_infoframe *avi);

diff --git a/drivers/gpu/drm/omapdrm/omap_connector.c b/drivers/gpu/drm/omapdrm/omap_connector.c index c24b6b783e9a..5219e340ed9d 100644 --- a/drivers/gpu/drm/omapdrm/omap_connector.c +++ b/drivers/gpu/drm/omapdrm/omap_connector.c @@ -35,6 +35,18 @@ struct omap_connector {

bool hdmi_mode;

};

+static void omap_connector_hpd_cb(void *cb_data,

• 		  enum drm_connector_status status)
  

+{

• struct omap_connector *omap_connector = cb_data;
• struct drm_connector *connector = &omap_connector->base;
• if (connector->status != status) {

• connector->status = status;
  

• drm_kms_helper_hotplug_event(omap_connector->base.dev);
  

• }

+}

I'm not sure if this is racy or not... drm_kms_helper_hotplug_event() should be called without locks held, but is it safe to touch connector->status without any locks?

We should at least protect against internal race conditions if the hpd callback can be called concurrently (I haven't checked the callers yet). I think it would be safer to use the mode_config mutex the same way drm_helper_hpd_irq_event() does. Something like the following should do.

struct omap_connector *omap_connector = cb_data; struct drm_connector *connector = &omap_connector->base; struct drm_device *dev = connector->dev; enum drm_connector_status old_status;

mutex_lock(&dev->mode_config.mutex); old_status = connector->status; connector->status = status; mutex_unlock(&dev->mode_config.mutex);

if (old_status != status) drm_kms_helper_hotplug_event(dev);

...

• if (connector->status != status) {

• connector->status = status;
  

• drm_kms_helper_hotplug_event(omap_connector->base.dev);
  

• }

bool omap_connector_get_hdmi_mode(struct drm_connector *connector) { struct omap_connector *omap_connector = to_omap_connector(connector); @@ -75,6 +87,10 @@ static void omap_connector_destroy(struct drm_connector *connector) struct omap_dss_device *dssdev = omap_connector->dssdev;

DBG("%s", omap_connector->dssdev->name);

• if (connector->polled == DRM_CONNECTOR_POLL_HPD &&

•    dssdev->driver->unregister_hpd_cb) {
  

• dssdev->driver->unregister_hpd_cb(dssdev);
  

• } drm_connector_unregister(connector); drm_connector_cleanup(connector); kfree(omap_connector);

@@ -216,6 +232,7 @@ struct drm_connector *omap_connector_init(struct drm_device *dev, { struct drm_connector *connector = NULL; struct omap_connector *omap_connector;

• bool hpd_supported = false;

  DBG("%s", dssdev->name);

@@ -233,7 +250,20 @@ struct drm_connector *omap_connector_init(struct drm_device *dev, connector_type);

drm_connector_helper_add(connector, &omap_connector_helper_funcs);

• if (dssdev->driver->detect)

• if (dssdev->driver->register_hpd_cb) {

• int ret = dssdev->driver->register_hpd_cb(dssdev,
  

omap_connector_hpd_cb,

...

• 					  omap_connector);
  

• if (!ret)
  

• 	hpd_supported = true;
  

• else if (ret != -ENOTSUPP)
  

• 	DBG("%s: Failed to register HPD callback (%d).",
  

• 	    dssdev->name, ret);
  

This should be an error print, shouldn't it?

Can it actually happen ? If it can't, I wouldn't bother with a message at all.