| Message ID | 20170515090312.32051-3-peter.ujfalusi@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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;
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
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;
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(+)