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;elsestatus = 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