[PATCH 0/3] drm/omap: Support for hotplug detection

[PATCH 0/3] drm/omap: Support for hotplug detection

[Peter Ujfalusi]

Hi,

this series will add support for HPD in omapdrm. Instead of polling for HPD
changes we can use interrupts to be notified of HPD change, thus we can react to
events faster.

The series is based on top of 4.12-rc1

Regards,
Peter
---
Peter Ujfalusi (3):
  drm/omap: Support for HDMI hot plug detection
  drm/omap: displays: connector-hdmi: Support for hot plug detection
  drm/omap: displays: encoder-tpd12s015: Support for hot plug detection

 drivers/gpu/drm/omapdrm/displays/connector-hdmi.c  | 104 +++++++++++++++++++++
 .../gpu/drm/omapdrm/displays/encoder-tpd12s015.c   |  81 ++++++++++++++++
 drivers/gpu/drm/omapdrm/dss/omapdss.h              |  17 ++++
 drivers/gpu/drm/omapdrm/omap_connector.c           |  32 ++++++-
 drivers/gpu/drm/omapdrm/omap_drv.c                 |  29 ++++++
 5 files changed, 262 insertions(+), 1 deletion(-)

-- 
2.12.2

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

[Peter Ujfalusi]

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);
+	}
+}
+
 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);
+	}
+
+	if (hpd_supported)
+		connector->polled = DRM_CONNECTOR_POLL_HPD;
+	else if (dssdev->driver->detect)
 		connector->polled = DRM_CONNECTOR_POLL_CONNECT |
 				    DRM_CONNECTOR_POLL_DISCONNECT;
 	else
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index e1f47f0b3ccf..6e54e4d446a9 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -394,6 +394,32 @@ static int omap_modeset_init(struct drm_device *dev)
 }
 
 /*
+ * Enable the HPD in external components if supported
+ */
+static void omap_modeset_enable_external_hpd(void)
+{
+	struct omap_dss_device *dssdev = NULL;
+
+	for_each_dss_dev(dssdev) {
+		if (dssdev->driver->enable_hpd)
+			dssdev->driver->enable_hpd(dssdev);
+	}
+}
+
+/*
+ * Disable the HPD in external components if supported
+ */
+static void omap_modeset_disable_external_hpd(void)
+{
+	struct omap_dss_device *dssdev = NULL;
+
+	for_each_dss_dev(dssdev) {
+		if (dssdev->driver->disable_hpd)
+			dssdev->driver->disable_hpd(dssdev);
+	}
+}
+
+/*
  * drm ioctl funcs
  */
 
@@ -710,6 +736,7 @@ static int pdev_probe(struct platform_device *pdev)
 	priv->fbdev = omap_fbdev_init(ddev);
 
 	drm_kms_helper_poll_init(ddev);
+	omap_modeset_enable_external_hpd();
 
 	/*
 	 * Register the DRM device with the core and the connectors with
@@ -722,6 +749,7 @@ static int pdev_probe(struct platform_device *pdev)
 	return 0;
 
 err_cleanup_helpers:
+	omap_modeset_disable_external_hpd();
 	drm_kms_helper_poll_fini(ddev);
 	if (priv->fbdev)
 		omap_fbdev_free(ddev);
@@ -750,6 +778,7 @@ static int pdev_remove(struct platform_device *pdev)
 
 	drm_dev_unregister(ddev);
 
+	omap_modeset_disable_external_hpd();
 	drm_kms_helper_poll_fini(ddev);
 
 	if (priv->fbdev)
-- 
2.12.2

[PATCH 2/3] drm/omap: displays: connector-hdmi: Support for hot plug detection

[Peter Ujfalusi]

If the hpd_gpio is valid, use interrupt handler to react to HPD changes.
In case the hpd_gpio is not valid, try to enable hpd detection on the
encoder if it supports it.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/gpu/drm/omapdrm/displays/connector-hdmi.c | 104 ++++++++++++++++++++++
 1 file changed, 104 insertions(+)

diff --git a/drivers/gpu/drm/omapdrm/displays/connector-hdmi.c b/drivers/gpu/drm/omapdrm/displays/connector-hdmi.c
index 1ef130641bae..3a90f89ada77 100644
--- a/drivers/gpu/drm/omapdrm/displays/connector-hdmi.c
+++ b/drivers/gpu/drm/omapdrm/displays/connector-hdmi.c
@@ -15,6 +15,7 @@
 #include <linux/platform_device.h>
 #include <linux/of.h>
 #include <linux/of_gpio.h>
+#include <linux/spinlock.h>
 
 #include <drm/drm_edid.h>
 #include <video/omap-panel-data.h>
@@ -38,6 +39,10 @@ static const struct videomode hdmic_default_vm = {
 struct panel_drv_data {
 	struct omap_dss_device dssdev;
 	struct omap_dss_device *in;
+	void (*hpd_cb)(void *cb_data, enum drm_connector_status status);
+	void *hpd_cb_data;
+	bool hpd_enabled;
+	struct mutex hpd_lock;
 
 	struct device *dev;
 
@@ -168,6 +173,70 @@ static bool hdmic_detect(struct omap_dss_device *dssdev)
 		return in->ops.hdmi->detect(in);
 }
 
+static int hdmic_register_hpd_cb(struct omap_dss_device *dssdev,
+				 void (*cb)(void *cb_data,
+					    enum drm_connector_status status),
+				 void *cb_data)
+{
+	struct panel_drv_data *ddata = to_panel_data(dssdev);
+	struct omap_dss_device *in = ddata->in;
+
+	if (gpio_is_valid(ddata->hpd_gpio)) {
+		mutex_lock(&ddata->hpd_lock);
+		ddata->hpd_cb = cb;
+		ddata->hpd_cb_data = cb_data;
+		mutex_unlock(&ddata->hpd_lock);
+		return 0;
+	} else if (in->ops.hdmi->register_hpd_cb) {
+		return in->ops.hdmi->register_hpd_cb(in, cb, cb_data);
+	}
+
+	return -ENOTSUPP;
+}
+
+static void hdmic_unregister_hpd_cb(struct omap_dss_device *dssdev)
+{
+	struct panel_drv_data *ddata = to_panel_data(dssdev);
+	struct omap_dss_device *in = ddata->in;
+
+	if (gpio_is_valid(ddata->hpd_gpio)) {
+		mutex_lock(&ddata->hpd_lock);
+		ddata->hpd_cb = NULL;
+		ddata->hpd_cb_data = NULL;
+		mutex_unlock(&ddata->hpd_lock);
+	} else if (in->ops.hdmi->unregister_hpd_cb) {
+		in->ops.hdmi->unregister_hpd_cb(in);
+	}
+}
+
+static void hdmic_enable_hpd(struct omap_dss_device *dssdev)
+{
+	struct panel_drv_data *ddata = to_panel_data(dssdev);
+	struct omap_dss_device *in = ddata->in;
+
+	if (gpio_is_valid(ddata->hpd_gpio)) {
+		mutex_lock(&ddata->hpd_lock);
+		ddata->hpd_enabled = true;
+		mutex_unlock(&ddata->hpd_lock);
+	} else if (in->ops.hdmi->enable_hpd) {
+		in->ops.hdmi->enable_hpd(in);
+	}
+}
+
+static void hdmic_disable_hpd(struct omap_dss_device *dssdev)
+{
+	struct panel_drv_data *ddata = to_panel_data(dssdev);
+	struct omap_dss_device *in = ddata->in;
+
+	if (gpio_is_valid(ddata->hpd_gpio)) {
+		mutex_lock(&ddata->hpd_lock);
+		ddata->hpd_enabled = false;
+		mutex_unlock(&ddata->hpd_lock);
+	} else if (in->ops.hdmi->disable_hpd) {
+		in->ops.hdmi->disable_hpd(in);
+	}
+}
+
 static int hdmic_set_hdmi_mode(struct omap_dss_device *dssdev, bool hdmi_mode)
 {
 	struct panel_drv_data *ddata = to_panel_data(dssdev);
@@ -200,10 +269,34 @@ static struct omap_dss_driver hdmic_driver = {
 
 	.read_edid		= hdmic_read_edid,
 	.detect			= hdmic_detect,
+	.register_hpd_cb	= hdmic_register_hpd_cb,
+	.unregister_hpd_cb	= hdmic_unregister_hpd_cb,
+	.enable_hpd		= hdmic_enable_hpd,
+	.disable_hpd		= hdmic_disable_hpd,
 	.set_hdmi_mode		= hdmic_set_hdmi_mode,
 	.set_hdmi_infoframe	= hdmic_set_infoframe,
 };
 
+static irqreturn_t hdmic_hpd_isr(int irq, void *data)
+{
+	struct panel_drv_data *ddata = data;
+
+	mutex_lock(&ddata->hpd_lock);
+	if (ddata->hpd_enabled && ddata->hpd_cb) {
+		enum drm_connector_status status;
+
+		if (hdmic_detect(&ddata->dssdev))
+			status = connector_status_connected;
+		else
+			status = connector_status_disconnected;
+
+		ddata->hpd_cb(ddata->hpd_cb_data, status);
+	}
+	mutex_unlock(&ddata->hpd_lock);
+
+	return IRQ_HANDLED;
+}
+
 static int hdmic_probe_of(struct platform_device *pdev)
 {
 	struct panel_drv_data *ddata = platform_get_drvdata(pdev);
@@ -249,11 +342,22 @@ static int hdmic_probe(struct platform_device *pdev)
 	if (r)
 		return r;
 
+	mutex_init(&ddata->hpd_lock);
+
 	if (gpio_is_valid(ddata->hpd_gpio)) {
 		r = devm_gpio_request_one(&pdev->dev, ddata->hpd_gpio,
 				GPIOF_DIR_IN, "hdmi_hpd");
 		if (r)
 			goto err_reg;
+
+		r = devm_request_threaded_irq(&pdev->dev,
+				gpio_to_irq(ddata->hpd_gpio),
+				NULL, hdmic_hpd_isr,
+				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING |
+				IRQF_ONESHOT,
+				"hdmic hpd", ddata);
+		if (r)
+			goto err_reg;
 	}
 
 	ddata->vm = hdmic_default_vm;
-- 
2.12.2

[PATCH 3/3] drm/omap: displays: encoder-tpd12s015: Support for hot plug detection

[Peter Ujfalusi]

Use interrupt handler for hpd GPIO to react to HPD changes.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 .../gpu/drm/omapdrm/displays/encoder-tpd12s015.c   | 81 ++++++++++++++++++++++
 1 file changed, 81 insertions(+)

diff --git a/drivers/gpu/drm/omapdrm/displays/encoder-tpd12s015.c b/drivers/gpu/drm/omapdrm/displays/encoder-tpd12s015.c
index 58276a48112e..529b8509dd99 100644
--- a/drivers/gpu/drm/omapdrm/displays/encoder-tpd12s015.c
+++ b/drivers/gpu/drm/omapdrm/displays/encoder-tpd12s015.c
@@ -15,12 +15,17 @@
 #include <linux/slab.h>
 #include <linux/platform_device.h>
 #include <linux/gpio/consumer.h>
+#include <linux/spinlock.h>
 
 #include "../dss/omapdss.h"
 
 struct panel_drv_data {
 	struct omap_dss_device dssdev;
 	struct omap_dss_device *in;
+	void (*hpd_cb)(void *cb_data, enum drm_connector_status status);
+	void *hpd_cb_data;
+	bool hpd_enabled;
+	struct mutex hpd_lock;
 
 	struct gpio_desc *ct_cp_hpd_gpio;
 	struct gpio_desc *ls_oe_gpio;
@@ -162,6 +167,49 @@ static bool tpd_detect(struct omap_dss_device *dssdev)
 	return gpiod_get_value_cansleep(ddata->hpd_gpio);
 }
 
+static int tpd_register_hpd_cb(struct omap_dss_device *dssdev,
+			       void (*cb)(void *cb_data,
+					  enum drm_connector_status status),
+			       void *cb_data)
+{
+	struct panel_drv_data *ddata = to_panel_data(dssdev);
+
+	mutex_lock(&ddata->hpd_lock);
+	ddata->hpd_cb = cb;
+	ddata->hpd_cb_data = cb_data;
+	mutex_unlock(&ddata->hpd_lock);
+
+	return 0;
+}
+
+static void tpd_unregister_hpd_cb(struct omap_dss_device *dssdev)
+{
+	struct panel_drv_data *ddata = to_panel_data(dssdev);
+
+	mutex_lock(&ddata->hpd_lock);
+	ddata->hpd_cb = NULL;
+	ddata->hpd_cb_data = NULL;
+	mutex_unlock(&ddata->hpd_lock);
+}
+
+static void tpd_enable_hpd(struct omap_dss_device *dssdev)
+{
+	struct panel_drv_data *ddata = to_panel_data(dssdev);
+
+	mutex_lock(&ddata->hpd_lock);
+	ddata->hpd_enabled = true;
+	mutex_unlock(&ddata->hpd_lock);
+}
+
+static void tpd_disable_hpd(struct omap_dss_device *dssdev)
+{
+	struct panel_drv_data *ddata = to_panel_data(dssdev);
+
+	mutex_lock(&ddata->hpd_lock);
+	ddata->hpd_enabled = false;
+	mutex_unlock(&ddata->hpd_lock);
+}
+
 static int tpd_set_infoframe(struct omap_dss_device *dssdev,
 		const struct hdmi_avi_infoframe *avi)
 {
@@ -193,10 +241,34 @@ static const struct omapdss_hdmi_ops tpd_hdmi_ops = {
 
 	.read_edid		= tpd_read_edid,
 	.detect			= tpd_detect,
+	.register_hpd_cb	= tpd_register_hpd_cb,
+	.unregister_hpd_cb	= tpd_unregister_hpd_cb,
+	.enable_hpd		= tpd_enable_hpd,
+	.disable_hpd		= tpd_disable_hpd,
 	.set_infoframe		= tpd_set_infoframe,
 	.set_hdmi_mode		= tpd_set_hdmi_mode,
 };
 
+static irqreturn_t tpd_hpd_isr(int irq, void *data)
+{
+	struct panel_drv_data *ddata = data;
+
+	mutex_lock(&ddata->hpd_lock);
+	if (ddata->hpd_enabled && ddata->hpd_cb) {
+		enum drm_connector_status status;
+
+		if (tpd_detect(&ddata->dssdev))
+			status = connector_status_connected;
+		else
+			status = connector_status_disconnected;
+
+		ddata->hpd_cb(ddata->hpd_cb_data, status);
+	}
+	mutex_unlock(&ddata->hpd_lock);
+
+	return IRQ_HANDLED;
+}
+
 static int tpd_probe_of(struct platform_device *pdev)
 {
 	struct panel_drv_data *ddata = platform_get_drvdata(pdev);
@@ -261,6 +333,15 @@ static int tpd_probe(struct platform_device *pdev)
 
 	ddata->hpd_gpio = gpio;
 
+	mutex_init(&ddata->hpd_lock);
+
+	r = devm_request_threaded_irq(&pdev->dev, gpiod_to_irq(ddata->hpd_gpio),
+		NULL, tpd_hpd_isr,
+		IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+		"tpd12s015 hpd", ddata);
+	if (r)
+		goto err_gpio;
+
 	dssdev = &ddata->dssdev;
 	dssdev->ops.hdmi = &tpd_hdmi_ops;
 	dssdev->dev = &pdev->dev;
-- 
2.12.2

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

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 4952 bytes --]

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?

> +
>  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?

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] drm/omap: displays: connector-hdmi: Support for hot plug detection

[Laurent Pinchart]

Hi Peter,

Thank you for the patch.

On Monday 15 May 2017 12:03:11 Peter Ujfalusi wrote:
> If the hpd_gpio is valid, use interrupt handler to react to HPD changes.
> In case the hpd_gpio is not valid, try to enable hpd detection on the
> encoder if it supports it.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/displays/connector-hdmi.c | 104 +++++++++++++++++++
>  1 file changed, 104 insertions(+)
> 
> diff --git a/drivers/gpu/drm/omapdrm/displays/connector-hdmi.c
> b/drivers/gpu/drm/omapdrm/displays/connector-hdmi.c index
> 1ef130641bae..3a90f89ada77 100644
> --- a/drivers/gpu/drm/omapdrm/displays/connector-hdmi.c
> +++ b/drivers/gpu/drm/omapdrm/displays/connector-hdmi.c
> @@ -15,6 +15,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/of.h>
>  #include <linux/of_gpio.h>
> +#include <linux/spinlock.h>

Did you mean linux/mutex.h ?

> 
>  #include <drm/drm_edid.h>
>  #include <video/omap-panel-data.h>
> @@ -38,6 +39,10 @@ static const struct videomode hdmic_default_vm = {
>  struct panel_drv_data {
>  	struct omap_dss_device dssdev;
>  	struct omap_dss_device *in;
> +	void (*hpd_cb)(void *cb_data, enum drm_connector_status status);
> +	void *hpd_cb_data;
> +	bool hpd_enabled;
> +	struct mutex hpd_lock;
> 
>  	struct device *dev;
> 
> @@ -168,6 +173,70 @@ static bool hdmic_detect(struct omap_dss_device
> *dssdev) return in->ops.hdmi->detect(in);
>  }
> 
> +static int hdmic_register_hpd_cb(struct omap_dss_device *dssdev,
> +				 void (*cb)(void *cb_data,
> +					    enum drm_connector_status status),
> +				 void *cb_data)
> +{
> +	struct panel_drv_data *ddata = to_panel_data(dssdev);
> +	struct omap_dss_device *in = ddata->in;
> +
> +	if (gpio_is_valid(ddata->hpd_gpio)) {
> +		mutex_lock(&ddata->hpd_lock);
> +		ddata->hpd_cb = cb;
> +		ddata->hpd_cb_data = cb_data;
> +		mutex_unlock(&ddata->hpd_lock);
> +		return 0;
> +	} else if (in->ops.hdmi->register_hpd_cb) {
> +		return in->ops.hdmi->register_hpd_cb(in, cb, cb_data);
> +	}
> +
> +	return -ENOTSUPP;
> +}
> +
> +static void hdmic_unregister_hpd_cb(struct omap_dss_device *dssdev)
> +{
> +	struct panel_drv_data *ddata = to_panel_data(dssdev);
> +	struct omap_dss_device *in = ddata->in;
> +
> +	if (gpio_is_valid(ddata->hpd_gpio)) {
> +		mutex_lock(&ddata->hpd_lock);
> +		ddata->hpd_cb = NULL;
> +		ddata->hpd_cb_data = NULL;
> +		mutex_unlock(&ddata->hpd_lock);
> +	} else if (in->ops.hdmi->unregister_hpd_cb) {
> +		in->ops.hdmi->unregister_hpd_cb(in);
> +	}
> +}
> +
> +static void hdmic_enable_hpd(struct omap_dss_device *dssdev)
> +{
> +	struct panel_drv_data *ddata = to_panel_data(dssdev);
> +	struct omap_dss_device *in = ddata->in;
> +
> +	if (gpio_is_valid(ddata->hpd_gpio)) {
> +		mutex_lock(&ddata->hpd_lock);
> +		ddata->hpd_enabled = true;
> +		mutex_unlock(&ddata->hpd_lock);
> +	} else if (in->ops.hdmi->enable_hpd) {
> +		in->ops.hdmi->enable_hpd(in);
> +	}
> +}
> +
> +static void hdmic_disable_hpd(struct omap_dss_device *dssdev)
> +{
> +	struct panel_drv_data *ddata = to_panel_data(dssdev);
> +	struct omap_dss_device *in = ddata->in;
> +
> +	if (gpio_is_valid(ddata->hpd_gpio)) {
> +		mutex_lock(&ddata->hpd_lock);
> +		ddata->hpd_enabled = false;
> +		mutex_unlock(&ddata->hpd_lock);
> +	} else if (in->ops.hdmi->disable_hpd) {
> +		in->ops.hdmi->disable_hpd(in);
> +	}
> +}
> +
>  static int hdmic_set_hdmi_mode(struct omap_dss_device *dssdev, bool
> hdmi_mode) {
>  	struct panel_drv_data *ddata = to_panel_data(dssdev);
> @@ -200,10 +269,34 @@ static struct omap_dss_driver hdmic_driver = {
> 
>  	.read_edid		= hdmic_read_edid,
>  	.detect			= hdmic_detect,
> +	.register_hpd_cb	= hdmic_register_hpd_cb,
> +	.unregister_hpd_cb	= hdmic_unregister_hpd_cb,
> +	.enable_hpd		= hdmic_enable_hpd,
> +	.disable_hpd		= hdmic_disable_hpd,
>  	.set_hdmi_mode		= hdmic_set_hdmi_mode,
>  	.set_hdmi_infoframe	= hdmic_set_infoframe,
>  };
> 
> +static irqreturn_t hdmic_hpd_isr(int irq, void *data)
> +{
> +	struct panel_drv_data *ddata = data;
> +
> +	mutex_lock(&ddata->hpd_lock);
> +	if (ddata->hpd_enabled && ddata->hpd_cb) {
> +		enum drm_connector_status status;
> +
> +		if (hdmic_detect(&ddata->dssdev))
> +			status = connector_status_connected;
> +		else
> +			status = connector_status_disconnected;
> +
> +		ddata->hpd_cb(ddata->hpd_cb_data, status);
> +	}
> +	mutex_unlock(&ddata->hpd_lock);

Shouldn't ddata->hpd_cb() be called without the mutex held ? I don't think 
core code should rely on callers to handle locking in this case.


> +	return IRQ_HANDLED;
> +}
> +
>  static int hdmic_probe_of(struct platform_device *pdev)
>  {
>  	struct panel_drv_data *ddata = platform_get_drvdata(pdev);
> @@ -249,11 +342,22 @@ static int hdmic_probe(struct platform_device *pdev)
>  	if (r)
>  		return r;
> 
> +	mutex_init(&ddata->hpd_lock);
> +
>  	if (gpio_is_valid(ddata->hpd_gpio)) {
>  		r = devm_gpio_request_one(&pdev->dev, ddata->hpd_gpio,
>  				GPIOF_DIR_IN, "hdmi_hpd");
>  		if (r)
>  			goto err_reg;
> +
> +		r = devm_request_threaded_irq(&pdev->dev,
> +				gpio_to_irq(ddata->hpd_gpio),

What is hpd_gpio is valid but has no IRQ support ?

> +				NULL, hdmic_hpd_isr,
> +				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING |
> +				IRQF_ONESHOT,
> +				"hdmic hpd", ddata);
> +		if (r)
> +			goto err_reg;
>  	}
> 
>  	ddata->vm = hdmic_default_vm;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/3] drm/omap: displays: encoder-tpd12s015: Support for hot plug detection

[Laurent Pinchart]

Hi Peter,

Thank you for the patch.

On Monday 15 May 2017 12:03:12 Peter Ujfalusi wrote:
> Use interrupt handler for hpd GPIO to react to HPD changes.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
>  .../gpu/drm/omapdrm/displays/encoder-tpd12s015.c   | 81 +++++++++++++++++++
>  1 file changed, 81 insertions(+)
> 
> diff --git a/drivers/gpu/drm/omapdrm/displays/encoder-tpd12s015.c
> b/drivers/gpu/drm/omapdrm/displays/encoder-tpd12s015.c index
> 58276a48112e..529b8509dd99 100644
> --- a/drivers/gpu/drm/omapdrm/displays/encoder-tpd12s015.c
> +++ b/drivers/gpu/drm/omapdrm/displays/encoder-tpd12s015.c
> @@ -15,12 +15,17 @@
>  #include <linux/slab.h>
>  #include <linux/platform_device.h>
>  #include <linux/gpio/consumer.h>
> +#include <linux/spinlock.h>

Did you mean linux/mutex.h ?

> 
>  #include "../dss/omapdss.h"
> 
>  struct panel_drv_data {
>  	struct omap_dss_device dssdev;
>  	struct omap_dss_device *in;
> +	void (*hpd_cb)(void *cb_data, enum drm_connector_status status);
> +	void *hpd_cb_data;
> +	bool hpd_enabled;
> +	struct mutex hpd_lock;
> 
>  	struct gpio_desc *ct_cp_hpd_gpio;
>  	struct gpio_desc *ls_oe_gpio;
> @@ -162,6 +167,49 @@ static bool tpd_detect(struct omap_dss_device *dssdev)
>  	return gpiod_get_value_cansleep(ddata->hpd_gpio);
>  }
> 
> +static int tpd_register_hpd_cb(struct omap_dss_device *dssdev,
> +			       void (*cb)(void *cb_data,
> +					  enum drm_connector_status status),
> +			       void *cb_data)
> +{
> +	struct panel_drv_data *ddata = to_panel_data(dssdev);
> +
> +	mutex_lock(&ddata->hpd_lock);
> +	ddata->hpd_cb = cb;
> +	ddata->hpd_cb_data = cb_data;
> +	mutex_unlock(&ddata->hpd_lock);
> +
> +	return 0;
> +}
> +
> +static void tpd_unregister_hpd_cb(struct omap_dss_device *dssdev)
> +{
> +	struct panel_drv_data *ddata = to_panel_data(dssdev);
> +
> +	mutex_lock(&ddata->hpd_lock);
> +	ddata->hpd_cb = NULL;
> +	ddata->hpd_cb_data = NULL;
> +	mutex_unlock(&ddata->hpd_lock);
> +}
> +
> +static void tpd_enable_hpd(struct omap_dss_device *dssdev)
> +{
> +	struct panel_drv_data *ddata = to_panel_data(dssdev);
> +
> +	mutex_lock(&ddata->hpd_lock);
> +	ddata->hpd_enabled = true;
> +	mutex_unlock(&ddata->hpd_lock);
> +}
> +
> +static void tpd_disable_hpd(struct omap_dss_device *dssdev)
> +{
> +	struct panel_drv_data *ddata = to_panel_data(dssdev);
> +
> +	mutex_lock(&ddata->hpd_lock);
> +	ddata->hpd_enabled = false;
> +	mutex_unlock(&ddata->hpd_lock);
> +}
> +
>  static int tpd_set_infoframe(struct omap_dss_device *dssdev,
>  		const struct hdmi_avi_infoframe *avi)
>  {
> @@ -193,10 +241,34 @@ static const struct omapdss_hdmi_ops tpd_hdmi_ops = {
> 
>  	.read_edid		= tpd_read_edid,
>  	.detect			= tpd_detect,
> +	.register_hpd_cb	= tpd_register_hpd_cb,
> +	.unregister_hpd_cb	= tpd_unregister_hpd_cb,
> +	.enable_hpd		= tpd_enable_hpd,
> +	.disable_hpd		= tpd_disable_hpd,
>  	.set_infoframe		= tpd_set_infoframe,
>  	.set_hdmi_mode		= tpd_set_hdmi_mode,
>  };
> 
> +static irqreturn_t tpd_hpd_isr(int irq, void *data)
> +{
> +	struct panel_drv_data *ddata = data;
> +
> +	mutex_lock(&ddata->hpd_lock);
> +	if (ddata->hpd_enabled && ddata->hpd_cb) {
> +		enum drm_connector_status status;
> +
> +		if (tpd_detect(&ddata->dssdev))
> +			status = connector_status_connected;
> +		else
> +			status = connector_status_disconnected;
> +
> +		ddata->hpd_cb(ddata->hpd_cb_data, status);
> +	}
> +	mutex_unlock(&ddata->hpd_lock);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int tpd_probe_of(struct platform_device *pdev)
>  {
>  	struct panel_drv_data *ddata = platform_get_drvdata(pdev);
> @@ -261,6 +333,15 @@ static int tpd_probe(struct platform_device *pdev)
> 
>  	ddata->hpd_gpio = gpio;
> 
> +	mutex_init(&ddata->hpd_lock);
> +
> +	r = devm_request_threaded_irq(&pdev->dev, gpiod_to_irq(ddata-
>hpd_gpio),

As the connector code can handle GPIO HPD thanks to patch 2/3, why does it 
have to be duplicated here ? I agree that encoders should support reporting of 
hotplug events when the HPD signal is connected to an encoder, but if it's 
connected to a GPIO, it seems to me that it should be the sole responsibility 
of the connector code to handle it.

> +		NULL, tpd_hpd_isr,
> +		IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +		"tpd12s015 hpd", ddata);
> +	if (r)
> +		goto err_gpio;
> +
>  	dssdev = &ddata->dssdev;
>  	dssdev->ops.hdmi = &tpd_hdmi_ops;
>  	dssdev->dev = &pdev->dev;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/3] drm/omap: displays: encoder-tpd12s015: Support for hot plug detection

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 525 bytes --]

On 23/05/17 12:48, Laurent Pinchart wrote:

> As the connector code can handle GPIO HPD thanks to patch 2/3, why does it 
> have to be duplicated here ? I agree that encoders should support reporting of 
> hotplug events when the HPD signal is connected to an encoder, but if it's 
> connected to a GPIO, it seems to me that it should be the sole responsibility 
> of the connector code to handle it.

The HPD line goes from the connector to TPD12S015. From TPD12S015
another line goes to the SoCs GPIO.

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/3] drm/omap: displays: encoder-tpd12s015: Support for hot plug detection

[Laurent Pinchart]

Hi Tomi,

On Tuesday 23 May 2017 13:25:34 Tomi Valkeinen wrote:
> On 23/05/17 12:48, Laurent Pinchart wrote:
> > As the connector code can handle GPIO HPD thanks to patch 2/3, why does it
> > have to be duplicated here ? I agree that encoders should support
> > reporting of hotplug events when the HPD signal is connected to an
> > encoder, but if it's connected to a GPIO, it seems to me that it should
> > be the sole responsibility of the connector code to handle it.
> 
> The HPD line goes from the connector to TPD12S015. From TPD12S015
> another line goes to the SoCs GPIO.

Isn't it the same signal, just with glitches filtered ? The TPD12S015 is an 
ESD clamp, level shifter and DC-DC converter, if it wasn't for the two control 
inputs CT_CP_HPD and LS_OE, we could probably do without a driver. I wouldn't 
add HPD support to this driver, as the chip really can't detect HPD.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/3] drm/omap: displays: encoder-tpd12s015: Support for hot plug detection

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 1833 bytes --]

On 23/05/17 13:42, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Tuesday 23 May 2017 13:25:34 Tomi Valkeinen wrote:
>> On 23/05/17 12:48, Laurent Pinchart wrote:
>>> As the connector code can handle GPIO HPD thanks to patch 2/3, why does it
>>> have to be duplicated here ? I agree that encoders should support
>>> reporting of hotplug events when the HPD signal is connected to an
>>> encoder, but if it's connected to a GPIO, it seems to me that it should
>>> be the sole responsibility of the connector code to handle it.
>>
>> The HPD line goes from the connector to TPD12S015. From TPD12S015
>> another line goes to the SoCs GPIO.
> 
> Isn't it the same signal, just with glitches filtered ? The TPD12S015 is an 
> ESD clamp, level shifter and DC-DC converter, if it wasn't for the two control 
> inputs CT_CP_HPD and LS_OE, we could probably do without a driver. I wouldn't 
> add HPD support to this driver, as the chip really can't detect HPD.

Yes, it is the same signal, level shifted and filtered, if I'm not mistaken.

The HPD gpio is defined in TPD's DT data. And can we know what is the
state of the HPD signal when TPD hasn't configured CT_CP_HPD and LS_OE.
Will there be a glitch when CT_CP_HPD is enabled? What if connector-hdmi
is already listening to HPD signal at that point?

My guess is that having gpio handling just in the connector would work,
especially as we set the CT_CP_HPD very early when TPD is being set up.
With this series we could probably do a bit more optimized management
for CT_CP_HPD, but probably we could still make sure the sequence is
such that the CT_CP_HPD is enabled before connector-hdmi registers the irq.

But that doesn't sound quite correct to me... I do think it's TPD
driver's responsibility, and it removes all the "make sure that"'s.

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/3] drm/omap: displays: connector-hdmi: Support for hot plug detection

[Peter Ujfalusi]

On 2017-05-23 12:45, Laurent Pinchart wrote:
> Hi Peter,
> 
> Thank you for the patch.
> 
> On Monday 15 May 2017 12:03:11 Peter Ujfalusi wrote:
>> If the hpd_gpio is valid, use interrupt handler to react to HPD changes.
>> In case the hpd_gpio is not valid, try to enable hpd detection on the
>> encoder if it supports it.
>>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>> ---
>>  drivers/gpu/drm/omapdrm/displays/connector-hdmi.c | 104 +++++++++++++++++++
>>  1 file changed, 104 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/displays/connector-hdmi.c
>> b/drivers/gpu/drm/omapdrm/displays/connector-hdmi.c index
>> 1ef130641bae..3a90f89ada77 100644
>> --- a/drivers/gpu/drm/omapdrm/displays/connector-hdmi.c
>> +++ b/drivers/gpu/drm/omapdrm/displays/connector-hdmi.c
>> @@ -15,6 +15,7 @@
>>  #include <linux/platform_device.h>
>>  #include <linux/of.h>
>>  #include <linux/of_gpio.h>
>> +#include <linux/spinlock.h>
> 
> Did you mean linux/mutex.h ?

Oops, yes I mean linux/mutex.h.

>> +static irqreturn_t hdmic_hpd_isr(int irq, void *data)
>> +{
>> +	struct panel_drv_data *ddata = data;
>> +
>> +	mutex_lock(&ddata->hpd_lock);
>> +	if (ddata->hpd_enabled && ddata->hpd_cb) {
>> +		enum drm_connector_status status;
>> +
>> +		if (hdmic_detect(&ddata->dssdev))
>> +			status = connector_status_connected;
>> +		else
>> +			status = connector_status_disconnected;
>> +
>> +		ddata->hpd_cb(ddata->hpd_cb_data, status);
>> +	}
>> +	mutex_unlock(&ddata->hpd_lock);
> 
> Shouldn't ddata->hpd_cb() be called without the mutex held ? I don't think 
> core code should rely on callers to handle locking in this case.

the mutex is for protecting the internal data of the driver (hpd_cb
mainly). W/o keeping the mutex there might be a chance that we are going
to get an unregister call (unlikely, but in theory it can happen) which
would NULL out the hpd_cb, if we save the hpd_cb and data while holding
the mux, it is possible that the  omap DRM driver was removed already
causing other type of crash.

> 
> 
>> +	return IRQ_HANDLED;
>> +}
>> +
>>  static int hdmic_probe_of(struct platform_device *pdev)
>>  {
>>  	struct panel_drv_data *ddata = platform_get_drvdata(pdev);
>> @@ -249,11 +342,22 @@ static int hdmic_probe(struct platform_device *pdev)
>>  	if (r)
>>  		return r;
>>
>> +	mutex_init(&ddata->hpd_lock);
>> +
>>  	if (gpio_is_valid(ddata->hpd_gpio)) {
>>  		r = devm_gpio_request_one(&pdev->dev, ddata->hpd_gpio,
>>  				GPIOF_DIR_IN, "hdmi_hpd");
>>  		if (r)
>>  			goto err_reg;
>> +
>> +		r = devm_request_threaded_irq(&pdev->dev,
>> +				gpio_to_irq(ddata->hpd_gpio),
> 
> What is hpd_gpio is valid but has no IRQ support ?
> 
>> +				NULL, hdmic_hpd_isr,
>> +				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING |
>> +				IRQF_ONESHOT,
>> +				"hdmic hpd", ddata);
>> +		if (r)
>> +			goto err_reg;
>>  	}
>>
>>  	ddata->vm = hdmic_default_vm;
> 

- Péter