This series updates remaining drivers in DRM to use new CEC notifier API. Those complement the "cec: improve notifier support, add connector info" patch series and also replace 2 following patches from there: - [PATCHv8 09/13] dw-hdmi: use cec_notifier_conn_(un)register - [PATCHv9 12/13] tda998x: use cec_notifier_conn_(un)register None of those changes were not tested on a real hardware. Changes since v1: Those patches delay creation of notifiers until respective connectors are constructed. It seems that those patches, for a couple of drivers, by adding the delay, introduce a race between notifiers' creation and the IRQs handling threads - at least I don't see anything obvious in there that would explicitly forbid such races to occur. v2 adds a write barrier to make sure IRQ threads see the notifier once it is created (replacing the WRITE_ONCE I put in v1). The best thing to do here, I believe, would be not to have any synchronization and make sure that an IRQ only gets enabled after the notifier is created. Dariusz Marcinkiewicz (5): drm: tda998x: use cec_notifier_conn_(un)register drm: sti: use cec_notifier_conn_(un)register drm: tegra: use cec_notifier_conn_(un)register drm: dw-hdmi: use cec_notifier_conn_(un)register drm: exynos: exynos_hdmi: use cec_notifier_conn_(un)register drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 37 +++++++++++++++-------- drivers/gpu/drm/exynos/exynos_hdmi.c | 33 ++++++++++++-------- drivers/gpu/drm/i2c/tda998x_drv.c | 32 ++++++++++++++------ drivers/gpu/drm/sti/sti_hdmi.c | 20 +++++++----- drivers/gpu/drm/tegra/output.c | 18 ++++++----- 5 files changed, 91 insertions(+), 49 deletions(-) -- 2.22.0.410.gd8fdbe21b5-goog
Use the new cec_notifier_conn_(un)register() functions to (un)register the notifier for the HDMI connector, and fill in the cec_connector_info. Changes since v1: Add memory barrier to make sure that the notifier becomes visible to the irq thread once it is fully constructed. Signed-off-by: Dariusz Marcinkiewicz <darekm@google.com> --- drivers/gpu/drm/i2c/tda998x_drv.c | 32 ++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 7f34601bb5155..7844f4113a839 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -794,9 +794,14 @@ static irqreturn_t tda998x_irq_thread(int irq, void *data) if (lvl & CEC_RXSHPDLEV_HPD) { tda998x_edid_delay_start(priv); } else { + struct cec_notifier *notify; + schedule_work(&priv->detect_work); - cec_notifier_set_phys_addr(priv->cec_notify, - CEC_PHYS_ADDR_INVALID); + + notify = READ_ONCE(priv->cec_notify); + if (notify) + cec_notifier_set_phys_addr(notify, + CEC_PHYS_ADDR_INVALID); } handled = true; @@ -1253,6 +1258,8 @@ static int tda998x_connector_init(struct tda998x_priv *priv, struct drm_device *drm) { struct drm_connector *connector = &priv->connector; + struct cec_connector_info conn_info; + struct cec_notifier *notifier; int ret; connector->interlace_allowed = 1; @@ -1269,6 +1276,19 @@ static int tda998x_connector_init(struct tda998x_priv *priv, if (ret) return ret; + cec_fill_conn_info_from_drm(&conn_info, connector); + + notifier = cec_notifier_conn_register(priv->cec_glue.parent, + NULL, &conn_info); + if (!notifier) + return -ENOMEM; + /* + * Make sure that tda998x_irq_thread does see the notifier + * when it fully constructed. + */ + smp_wmb(); + priv->cec_notify = notifier; + drm_connector_attach_encoder(&priv->connector, priv->bridge.encoder); @@ -1651,7 +1671,7 @@ static void tda998x_destroy(struct device *dev) i2c_unregister_device(priv->cec); if (priv->cec_notify) - cec_notifier_put(priv->cec_notify); + cec_notifier_conn_unregister(priv->cec_notify); } static int tda998x_create(struct device *dev) @@ -1776,12 +1796,6 @@ static int tda998x_create(struct device *dev) cec_write(priv, REG_CEC_RXSHPDINTENA, CEC_RXSHPDLEV_HPD); } - priv->cec_notify = cec_notifier_get(dev); - if (!priv->cec_notify) { - ret = -ENOMEM; - goto fail; - } - priv->cec_glue.parent = dev; priv->cec_glue.data = priv; priv->cec_glue.init = tda998x_cec_hook_init; -- 2.22.0.410.gd8fdbe21b5-goog
Use the new cec_notifier_conn_(un)register() functions to (un)register the notifier for the HDMI connector, and fill in the cec_connector_info. Signed-off-by: Dariusz Marcinkiewicz <darekm@google.com> --- drivers/gpu/drm/sti/sti_hdmi.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c index 6000df6249807..5519b0c397c72 100644 --- a/drivers/gpu/drm/sti/sti_hdmi.c +++ b/drivers/gpu/drm/sti/sti_hdmi.c @@ -1250,6 +1250,7 @@ static int sti_hdmi_bind(struct device *dev, struct device *master, void *data) struct drm_device *drm_dev = data; struct drm_encoder *encoder; struct sti_hdmi_connector *connector; + struct cec_connector_info conn_info; struct drm_connector *drm_connector; struct drm_bridge *bridge; int err; @@ -1310,6 +1311,14 @@ static int sti_hdmi_bind(struct device *dev, struct device *master, void *data) goto err_sysfs; } + cec_fill_conn_info_from_drm(&conn_info, drm_connector); + hdmi->notifier = cec_notifier_conn_register(&hdmi->dev, NULL, + &conn_info); + if (!hdmi->notifier) { + hdmi->drm_connector = NULL; + return -ENOMEM; + } + /* Enable default interrupts */ hdmi_write(hdmi, HDMI_DEFAULT_INT, HDMI_INT_EN); @@ -1323,6 +1332,10 @@ static int sti_hdmi_bind(struct device *dev, struct device *master, void *data) static void sti_hdmi_unbind(struct device *dev, struct device *master, void *data) { + struct sti_hdmi *hdmi = dev_get_drvdata(dev); + + cec_notifier_set_phys_addr(hdmi->notifier, CEC_PHYS_ADDR_INVALID); + cec_notifier_conn_unregister(hdmi->notifier); } static const struct component_ops sti_hdmi_ops = { @@ -1428,10 +1441,6 @@ static int sti_hdmi_probe(struct platform_device *pdev) goto release_adapter; } - hdmi->notifier = cec_notifier_get(&pdev->dev); - if (!hdmi->notifier) - goto release_adapter; - hdmi->reset = devm_reset_control_get(dev, "hdmi"); /* Take hdmi out of reset */ if (!IS_ERR(hdmi->reset)) @@ -1451,14 +1460,11 @@ static int sti_hdmi_remove(struct platform_device *pdev) { struct sti_hdmi *hdmi = dev_get_drvdata(&pdev->dev); - cec_notifier_set_phys_addr(hdmi->notifier, CEC_PHYS_ADDR_INVALID); - i2c_put_adapter(hdmi->ddc_adapt); if (hdmi->audio_pdev) platform_device_unregister(hdmi->audio_pdev); component_del(&pdev->dev, &sti_hdmi_ops); - cec_notifier_put(hdmi->notifier); return 0; } -- 2.22.0.410.gd8fdbe21b5-goog
Use the new cec_notifier_conn_(un)register() functions to (un)register the notifier for the HDMI connector, and fill in the cec_connector_info. Signed-off-by: Dariusz Marcinkiewicz <darekm@google.com> --- drivers/gpu/drm/tegra/output.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c index 9c2b9dad55c30..ea92e72280868 100644 --- a/drivers/gpu/drm/tegra/output.c +++ b/drivers/gpu/drm/tegra/output.c @@ -80,6 +80,9 @@ tegra_output_connector_detect(struct drm_connector *connector, bool force) void tegra_output_connector_destroy(struct drm_connector *connector) { + struct tegra_output *output = connector_to_output(connector); + + cec_notifier_conn_unregister(output->cec); drm_connector_unregister(connector); drm_connector_cleanup(connector); } @@ -174,18 +177,11 @@ int tegra_output_probe(struct tegra_output *output) disable_irq(output->hpd_irq); } - output->cec = cec_notifier_get(output->dev); - if (!output->cec) - return -ENOMEM; - return 0; } void tegra_output_remove(struct tegra_output *output) { - if (output->cec) - cec_notifier_put(output->cec); - if (gpio_is_valid(output->hpd_gpio)) { free_irq(output->hpd_irq, output); gpio_free(output->hpd_gpio); @@ -197,6 +193,7 @@ void tegra_output_remove(struct tegra_output *output) int tegra_output_init(struct drm_device *drm, struct tegra_output *output) { + struct cec_connector_info conn_info; int err; if (output->panel) { @@ -212,6 +209,13 @@ int tegra_output_init(struct drm_device *drm, struct tegra_output *output) if (gpio_is_valid(output->hpd_gpio)) enable_irq(output->hpd_irq); + cec_fill_conn_info_from_drm(&conn_info, &output->connector); + + output->cec = cec_notifier_conn_register(output->dev, NULL, &conn_info); + if (!output->cec) + return -ENOMEM; + + return 0; } -- 2.22.0.410.gd8fdbe21b5-goog
Use the new cec_notifier_conn_(un)register() functions to (un)register the notifier for the HDMI connector, and fill in the cec_connector_info. Changes since v1: Add memory barrier to make sure that the notifier becomes visible to the irq thread once it is fully constructed. Signed-off-by: Dariusz Marcinkiewicz <darekm@google.com> --- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 37 +++++++++++++++-------- 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index ab7968c8f6a29..c0f4eb3c12b18 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -2118,6 +2118,8 @@ static int dw_hdmi_bridge_attach(struct drm_bridge *bridge) struct dw_hdmi *hdmi = bridge->driver_private; struct drm_encoder *encoder = bridge->encoder; struct drm_connector *connector = &hdmi->connector; + struct cec_connector_info conn_info; + struct cec_notifier *notifier; connector->interlace_allowed = 1; connector->polled = DRM_CONNECTOR_POLL_HPD; @@ -2129,6 +2131,18 @@ static int dw_hdmi_bridge_attach(struct drm_bridge *bridge) drm_connector_attach_encoder(connector, encoder); + cec_fill_conn_info_from_drm(&conn_info, connector); + + notifier = cec_notifier_conn_register(hdmi->dev, NULL, &conn_info); + if (!notifier) + return -ENOMEM; + /* + * Make sure that dw_hdmi_irq thread does see the notifier + * when it fully constructed. + */ + smp_wmb(); + hdmi->cec_notifier = notifier; + return 0; } @@ -2295,9 +2309,15 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id) phy_stat & HDMI_PHY_HPD, phy_stat & HDMI_PHY_RX_SENSE); - if ((phy_stat & (HDMI_PHY_RX_SENSE | HDMI_PHY_HPD)) == 0) - cec_notifier_set_phys_addr(hdmi->cec_notifier, - CEC_PHYS_ADDR_INVALID); + if ((phy_stat & (HDMI_PHY_RX_SENSE | HDMI_PHY_HPD)) == 0) { + struct cec_notifier *notifer; + + notifer = READ_ONCE(hdmi->cec_notifier); + if (notifer) + cec_notifier_set_phys_addr( + notifer, + CEC_PHYS_ADDR_INVALID); + } } if (intr_stat & HDMI_IH_PHY_STAT0_HPD) { @@ -2600,12 +2620,6 @@ __dw_hdmi_probe(struct platform_device *pdev, if (ret) goto err_iahb; - hdmi->cec_notifier = cec_notifier_get(dev); - if (!hdmi->cec_notifier) { - ret = -ENOMEM; - goto err_iahb; - } - /* * To prevent overflows in HDMI_IH_FC_STAT2, set the clk regenerator * N and cts values before enabling phy @@ -2693,9 +2707,6 @@ __dw_hdmi_probe(struct platform_device *pdev, hdmi->ddc = NULL; } - if (hdmi->cec_notifier) - cec_notifier_put(hdmi->cec_notifier); - clk_disable_unprepare(hdmi->iahb_clk); if (hdmi->cec_clk) clk_disable_unprepare(hdmi->cec_clk); @@ -2718,7 +2729,7 @@ static void __dw_hdmi_remove(struct dw_hdmi *hdmi) hdmi_writeb(hdmi, ~0, HDMI_IH_MUTE_PHY_STAT0); if (hdmi->cec_notifier) - cec_notifier_put(hdmi->cec_notifier); + cec_notifier_conn_unregister(hdmi->cec_notifier); clk_disable_unprepare(hdmi->iahb_clk); clk_disable_unprepare(hdmi->isfr_clk); -- 2.22.0.410.gd8fdbe21b5-goog
Use the new cec_notifier_conn_(un)register() functions to (un)register the notifier for the HDMI connector, and fill in the cec_connector_info. Signed-off-by: Dariusz Marcinkiewicz <darekm@google.com> --- drivers/gpu/drm/exynos/exynos_hdmi.c | 33 +++++++++++++++++----------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c index 19c252f659dd0..dec4149435de1 100644 --- a/drivers/gpu/drm/exynos/exynos_hdmi.c +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c @@ -858,6 +858,11 @@ static enum drm_connector_status hdmi_detect(struct drm_connector *connector, static void hdmi_connector_destroy(struct drm_connector *connector) { + struct hdmi_context *hdata = connector_to_hdmi(connector); + + cec_notifier_set_phys_addr(hdata->notifier, CEC_PHYS_ADDR_INVALID); + cec_notifier_conn_unregister(hdata->notifier); + drm_connector_unregister(connector); drm_connector_cleanup(connector); } @@ -941,6 +946,7 @@ static int hdmi_create_connector(struct drm_encoder *encoder) { struct hdmi_context *hdata = encoder_to_hdmi(encoder); struct drm_connector *connector = &hdata->connector; + struct cec_connector_info conn_info; int ret; connector->interlace_allowed = true; @@ -963,6 +969,15 @@ static int hdmi_create_connector(struct drm_encoder *encoder) DRM_DEV_ERROR(hdata->dev, "Failed to attach bridge\n"); } + cec_fill_conn_info_from_drm(&conn_info, connector); + + hdata->notifier = cec_notifier_conn_register(hdata->dev, NULL, + &conn_info); + if (hdata->notifier == NULL) { + ret = -ENOMEM; + DRM_DEV_ERROR(hdata->dev, "Failed to allocate CEC notifier\n"); + } + return ret; } @@ -1534,8 +1549,9 @@ static void hdmi_disable(struct drm_encoder *encoder) */ mutex_unlock(&hdata->mutex); cancel_delayed_work(&hdata->hotplug_work); - cec_notifier_set_phys_addr(hdata->notifier, - CEC_PHYS_ADDR_INVALID); + if (hdata->notifier) + cec_notifier_set_phys_addr(hdata->notifier, + CEC_PHYS_ADDR_INVALID); return; } @@ -2012,12 +2028,6 @@ static int hdmi_probe(struct platform_device *pdev) } } - hdata->notifier = cec_notifier_get(&pdev->dev); - if (hdata->notifier == NULL) { - ret = -ENOMEM; - goto err_hdmiphy; - } - pm_runtime_enable(dev); audio_infoframe = &hdata->audio.infoframe; @@ -2029,7 +2039,7 @@ static int hdmi_probe(struct platform_device *pdev) ret = hdmi_register_audio_device(hdata); if (ret) - goto err_notifier_put; + goto err_runtime_disable; ret = component_add(&pdev->dev, &hdmi_component_ops); if (ret) @@ -2040,8 +2050,7 @@ static int hdmi_probe(struct platform_device *pdev) err_unregister_audio: platform_device_unregister(hdata->audio.pdev); -err_notifier_put: - cec_notifier_put(hdata->notifier); +err_runtime_disable: pm_runtime_disable(dev); err_hdmiphy: @@ -2060,12 +2069,10 @@ static int hdmi_remove(struct platform_device *pdev) struct hdmi_context *hdata = platform_get_drvdata(pdev); cancel_delayed_work_sync(&hdata->hotplug_work); - cec_notifier_set_phys_addr(hdata->notifier, CEC_PHYS_ADDR_INVALID); component_del(&pdev->dev, &hdmi_component_ops); platform_device_unregister(hdata->audio.pdev); - cec_notifier_put(hdata->notifier); pm_runtime_disable(&pdev->dev); if (!IS_ERR(hdata->reg_hdmi_en)) -- 2.22.0.410.gd8fdbe21b5-goog
On 7/1/19 4:59 PM, Dariusz Marcinkiewicz wrote: > Use the new cec_notifier_conn_(un)register() functions to > (un)register the notifier for the HDMI connector, and fill > in the cec_connector_info. > > Changes since v1: > Add memory barrier to make sure that the notifier > becomes visible to the irq thread once it is > fully constructed. > > Signed-off-by: Dariusz Marcinkiewicz <darekm@google.com> > --- > drivers/gpu/drm/i2c/tda998x_drv.c | 32 ++++++++++++++++++++++--------- > 1 file changed, 23 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c > index 7f34601bb5155..7844f4113a839 100644 > --- a/drivers/gpu/drm/i2c/tda998x_drv.c > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c > @@ -794,9 +794,14 @@ static irqreturn_t tda998x_irq_thread(int irq, void *data) > if (lvl & CEC_RXSHPDLEV_HPD) { > tda998x_edid_delay_start(priv); > } else { > + struct cec_notifier *notify; > + > schedule_work(&priv->detect_work); > - cec_notifier_set_phys_addr(priv->cec_notify, > - CEC_PHYS_ADDR_INVALID); > + > + notify = READ_ONCE(priv->cec_notify); > + if (notify) > + cec_notifier_set_phys_addr(notify, > + CEC_PHYS_ADDR_INVALID); Can you replace this with cec_notifier_phys_addr_invalidate()? > } > > handled = true; > @@ -1253,6 +1258,8 @@ static int tda998x_connector_init(struct tda998x_priv *priv, > struct drm_device *drm) > { > struct drm_connector *connector = &priv->connector; > + struct cec_connector_info conn_info; > + struct cec_notifier *notifier; > int ret; > > connector->interlace_allowed = 1; > @@ -1269,6 +1276,19 @@ static int tda998x_connector_init(struct tda998x_priv *priv, > if (ret) > return ret; > > + cec_fill_conn_info_from_drm(&conn_info, connector); > + > + notifier = cec_notifier_conn_register(priv->cec_glue.parent, > + NULL, &conn_info); > + if (!notifier) > + return -ENOMEM; > + /* > + * Make sure that tda998x_irq_thread does see the notifier > + * when it fully constructed. > + */ > + smp_wmb(); > + priv->cec_notify = notifier; > + > drm_connector_attach_encoder(&priv->connector, > priv->bridge.encoder); > > @@ -1651,7 +1671,7 @@ static void tda998x_destroy(struct device *dev) > i2c_unregister_device(priv->cec); > > if (priv->cec_notify) > - cec_notifier_put(priv->cec_notify); > + cec_notifier_conn_unregister(priv->cec_notify); You can drop the 'if': cec_notifier_conn_unregister() does nothing if the notifier is NULL. > } > > static int tda998x_create(struct device *dev) > @@ -1776,12 +1796,6 @@ static int tda998x_create(struct device *dev) > cec_write(priv, REG_CEC_RXSHPDINTENA, CEC_RXSHPDLEV_HPD); > } > > - priv->cec_notify = cec_notifier_get(dev); > - if (!priv->cec_notify) { > - ret = -ENOMEM; > - goto fail; > - } > - > priv->cec_glue.parent = dev; > priv->cec_glue.data = priv; > priv->cec_glue.init = tda998x_cec_hook_init; > Regards, Hans
On 7/1/19 4:59 PM, Dariusz Marcinkiewicz wrote: > Use the new cec_notifier_conn_(un)register() functions to > (un)register the notifier for the HDMI connector, and fill > in the cec_connector_info. > > Signed-off-by: Dariusz Marcinkiewicz <darekm@google.com> > --- > drivers/gpu/drm/sti/sti_hdmi.c | 20 +++++++++++++------- > 1 file changed, 13 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c > index 6000df6249807..5519b0c397c72 100644 > --- a/drivers/gpu/drm/sti/sti_hdmi.c > +++ b/drivers/gpu/drm/sti/sti_hdmi.c > @@ -1250,6 +1250,7 @@ static int sti_hdmi_bind(struct device *dev, struct device *master, void *data) > struct drm_device *drm_dev = data; > struct drm_encoder *encoder; > struct sti_hdmi_connector *connector; > + struct cec_connector_info conn_info; > struct drm_connector *drm_connector; > struct drm_bridge *bridge; > int err; > @@ -1310,6 +1311,14 @@ static int sti_hdmi_bind(struct device *dev, struct device *master, void *data) > goto err_sysfs; > } > > + cec_fill_conn_info_from_drm(&conn_info, drm_connector); > + hdmi->notifier = cec_notifier_conn_register(&hdmi->dev, NULL, > + &conn_info); > + if (!hdmi->notifier) { > + hdmi->drm_connector = NULL; > + return -ENOMEM; > + } > + > /* Enable default interrupts */ > hdmi_write(hdmi, HDMI_DEFAULT_INT, HDMI_INT_EN); > > @@ -1323,6 +1332,10 @@ static int sti_hdmi_bind(struct device *dev, struct device *master, void *data) > static void sti_hdmi_unbind(struct device *dev, > struct device *master, void *data) > { > + struct sti_hdmi *hdmi = dev_get_drvdata(dev); > + > + cec_notifier_set_phys_addr(hdmi->notifier, CEC_PHYS_ADDR_INVALID); No need to do this, cec_notifier_conn_unregister() takes care of this. > + cec_notifier_conn_unregister(hdmi->notifier); > } > > static const struct component_ops sti_hdmi_ops = { > @@ -1428,10 +1441,6 @@ static int sti_hdmi_probe(struct platform_device *pdev) > goto release_adapter; > } > > - hdmi->notifier = cec_notifier_get(&pdev->dev); > - if (!hdmi->notifier) > - goto release_adapter; > - > hdmi->reset = devm_reset_control_get(dev, "hdmi"); > /* Take hdmi out of reset */ > if (!IS_ERR(hdmi->reset)) > @@ -1451,14 +1460,11 @@ static int sti_hdmi_remove(struct platform_device *pdev) > { > struct sti_hdmi *hdmi = dev_get_drvdata(&pdev->dev); > > - cec_notifier_set_phys_addr(hdmi->notifier, CEC_PHYS_ADDR_INVALID); > - > i2c_put_adapter(hdmi->ddc_adapt); > if (hdmi->audio_pdev) > platform_device_unregister(hdmi->audio_pdev); > component_del(&pdev->dev, &sti_hdmi_ops); > > - cec_notifier_put(hdmi->notifier); > return 0; > } > > Regards, Hans
On 7/1/19 4:59 PM, Dariusz Marcinkiewicz wrote:
> Use the new cec_notifier_conn_(un)register() functions to
> (un)register the notifier for the HDMI connector, and fill in
> the cec_connector_info.
>
> Signed-off-by: Dariusz Marcinkiewicz <darekm@google.com>
> ---
> drivers/gpu/drm/tegra/output.c | 18 +++++++++++-------
> 1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c
> index 9c2b9dad55c30..ea92e72280868 100644
> --- a/drivers/gpu/drm/tegra/output.c
> +++ b/drivers/gpu/drm/tegra/output.c
> @@ -80,6 +80,9 @@ tegra_output_connector_detect(struct drm_connector *connector, bool force)
>
> void tegra_output_connector_destroy(struct drm_connector *connector)
> {
> + struct tegra_output *output = connector_to_output(connector);
> +
> + cec_notifier_conn_unregister(output->cec);
> drm_connector_unregister(connector);
> drm_connector_cleanup(connector);
> }
> @@ -174,18 +177,11 @@ int tegra_output_probe(struct tegra_output *output)
> disable_irq(output->hpd_irq);
> }
>
> - output->cec = cec_notifier_get(output->dev);
> - if (!output->cec)
> - return -ENOMEM;
> -
> return 0;
> }
>
> void tegra_output_remove(struct tegra_output *output)
> {
> - if (output->cec)
> - cec_notifier_put(output->cec);
> -
> if (gpio_is_valid(output->hpd_gpio)) {
> free_irq(output->hpd_irq, output);
> gpio_free(output->hpd_gpio);
> @@ -197,6 +193,7 @@ void tegra_output_remove(struct tegra_output *output)
>
> int tegra_output_init(struct drm_device *drm, struct tegra_output *output)
> {
> + struct cec_connector_info conn_info;
> int err;
>
> if (output->panel) {
> @@ -212,6 +209,13 @@ int tegra_output_init(struct drm_device *drm, struct tegra_output *output)
> if (gpio_is_valid(output->hpd_gpio))
> enable_irq(output->hpd_irq);
>
> + cec_fill_conn_info_from_drm(&conn_info, &output->connector);
> +
> + output->cec = cec_notifier_conn_register(output->dev, NULL, &conn_info);
> + if (!output->cec)
> + return -ENOMEM;
> +
> +
> return 0;
> }
>
>
Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Regards,
Hans
On 7/1/19 4:59 PM, Dariusz Marcinkiewicz wrote: > Use the new cec_notifier_conn_(un)register() functions to > (un)register the notifier for the HDMI connector, and fill in > the cec_connector_info. > > Changes since v1: > Add memory barrier to make sure that the notifier > becomes visible to the irq thread once it is fully > constructed. > > Signed-off-by: Dariusz Marcinkiewicz <darekm@google.com> > --- > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 37 +++++++++++++++-------- > 1 file changed, 24 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > index ab7968c8f6a29..c0f4eb3c12b18 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > @@ -2118,6 +2118,8 @@ static int dw_hdmi_bridge_attach(struct drm_bridge *bridge) > struct dw_hdmi *hdmi = bridge->driver_private; > struct drm_encoder *encoder = bridge->encoder; > struct drm_connector *connector = &hdmi->connector; > + struct cec_connector_info conn_info; > + struct cec_notifier *notifier; > > connector->interlace_allowed = 1; > connector->polled = DRM_CONNECTOR_POLL_HPD; > @@ -2129,6 +2131,18 @@ static int dw_hdmi_bridge_attach(struct drm_bridge *bridge) > > drm_connector_attach_encoder(connector, encoder); > > + cec_fill_conn_info_from_drm(&conn_info, connector); > + > + notifier = cec_notifier_conn_register(hdmi->dev, NULL, &conn_info); > + if (!notifier) > + return -ENOMEM; > + /* > + * Make sure that dw_hdmi_irq thread does see the notifier > + * when it fully constructed. > + */ > + smp_wmb(); > + hdmi->cec_notifier = notifier; > + > return 0; > } > > @@ -2295,9 +2309,15 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id) > phy_stat & HDMI_PHY_HPD, > phy_stat & HDMI_PHY_RX_SENSE); > > - if ((phy_stat & (HDMI_PHY_RX_SENSE | HDMI_PHY_HPD)) == 0) > - cec_notifier_set_phys_addr(hdmi->cec_notifier, > - CEC_PHYS_ADDR_INVALID); > + if ((phy_stat & (HDMI_PHY_RX_SENSE | HDMI_PHY_HPD)) == 0) { > + struct cec_notifier *notifer; > + > + notifer = READ_ONCE(hdmi->cec_notifier); > + if (notifer) > + cec_notifier_set_phys_addr( > + notifer, > + CEC_PHYS_ADDR_INVALID); Please replace with cec_notifier_phys_addr_invalidate(). > + } > } > > if (intr_stat & HDMI_IH_PHY_STAT0_HPD) { > @@ -2600,12 +2620,6 @@ __dw_hdmi_probe(struct platform_device *pdev, > if (ret) > goto err_iahb; > > - hdmi->cec_notifier = cec_notifier_get(dev); > - if (!hdmi->cec_notifier) { > - ret = -ENOMEM; > - goto err_iahb; > - } > - > /* > * To prevent overflows in HDMI_IH_FC_STAT2, set the clk regenerator > * N and cts values before enabling phy > @@ -2693,9 +2707,6 @@ __dw_hdmi_probe(struct platform_device *pdev, > hdmi->ddc = NULL; > } > > - if (hdmi->cec_notifier) > - cec_notifier_put(hdmi->cec_notifier); > - > clk_disable_unprepare(hdmi->iahb_clk); > if (hdmi->cec_clk) > clk_disable_unprepare(hdmi->cec_clk); > @@ -2718,7 +2729,7 @@ static void __dw_hdmi_remove(struct dw_hdmi *hdmi) > hdmi_writeb(hdmi, ~0, HDMI_IH_MUTE_PHY_STAT0); > > if (hdmi->cec_notifier) > - cec_notifier_put(hdmi->cec_notifier); > + cec_notifier_conn_unregister(hdmi->cec_notifier); No need for the 'if', cec_notifier_conn_unregister() already checks for a NULL notifier. Regards, Hans > > clk_disable_unprepare(hdmi->iahb_clk); > clk_disable_unprepare(hdmi->isfr_clk); >
On 7/1/19 4:59 PM, Dariusz Marcinkiewicz wrote: > Use the new cec_notifier_conn_(un)register() functions to > (un)register the notifier for the HDMI connector, and fill in > the cec_connector_info. > > Signed-off-by: Dariusz Marcinkiewicz <darekm@google.com> > --- > drivers/gpu/drm/exynos/exynos_hdmi.c | 33 +++++++++++++++++----------- > 1 file changed, 20 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c > index 19c252f659dd0..dec4149435de1 100644 > --- a/drivers/gpu/drm/exynos/exynos_hdmi.c > +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c > @@ -858,6 +858,11 @@ static enum drm_connector_status hdmi_detect(struct drm_connector *connector, > > static void hdmi_connector_destroy(struct drm_connector *connector) > { > + struct hdmi_context *hdata = connector_to_hdmi(connector); > + > + cec_notifier_set_phys_addr(hdata->notifier, CEC_PHYS_ADDR_INVALID); You can drop this, cec_notifier_conn_unregister() takes care of this for you. > + cec_notifier_conn_unregister(hdata->notifier); > + > drm_connector_unregister(connector); > drm_connector_cleanup(connector); > } > @@ -941,6 +946,7 @@ static int hdmi_create_connector(struct drm_encoder *encoder) > { > struct hdmi_context *hdata = encoder_to_hdmi(encoder); > struct drm_connector *connector = &hdata->connector; > + struct cec_connector_info conn_info; > int ret; > > connector->interlace_allowed = true; > @@ -963,6 +969,15 @@ static int hdmi_create_connector(struct drm_encoder *encoder) > DRM_DEV_ERROR(hdata->dev, "Failed to attach bridge\n"); > } > > + cec_fill_conn_info_from_drm(&conn_info, connector); > + > + hdata->notifier = cec_notifier_conn_register(hdata->dev, NULL, > + &conn_info); > + if (hdata->notifier == NULL) { > + ret = -ENOMEM; > + DRM_DEV_ERROR(hdata->dev, "Failed to allocate CEC notifier\n"); > + } > + > return ret; > } > > @@ -1534,8 +1549,9 @@ static void hdmi_disable(struct drm_encoder *encoder) > */ > mutex_unlock(&hdata->mutex); > cancel_delayed_work(&hdata->hotplug_work); > - cec_notifier_set_phys_addr(hdata->notifier, > - CEC_PHYS_ADDR_INVALID); > + if (hdata->notifier) > + cec_notifier_set_phys_addr(hdata->notifier, > + CEC_PHYS_ADDR_INVALID); Use cec_notifier_phys_addr_invalidate(). > return; > } > > @@ -2012,12 +2028,6 @@ static int hdmi_probe(struct platform_device *pdev) > } > } > > - hdata->notifier = cec_notifier_get(&pdev->dev); > - if (hdata->notifier == NULL) { > - ret = -ENOMEM; > - goto err_hdmiphy; > - } > - > pm_runtime_enable(dev); > > audio_infoframe = &hdata->audio.infoframe; > @@ -2029,7 +2039,7 @@ static int hdmi_probe(struct platform_device *pdev) > > ret = hdmi_register_audio_device(hdata); > if (ret) > - goto err_notifier_put; > + goto err_runtime_disable; > > ret = component_add(&pdev->dev, &hdmi_component_ops); > if (ret) > @@ -2040,8 +2050,7 @@ static int hdmi_probe(struct platform_device *pdev) > err_unregister_audio: > platform_device_unregister(hdata->audio.pdev); > > -err_notifier_put: > - cec_notifier_put(hdata->notifier); > +err_runtime_disable: > pm_runtime_disable(dev); > > err_hdmiphy: > @@ -2060,12 +2069,10 @@ static int hdmi_remove(struct platform_device *pdev) > struct hdmi_context *hdata = platform_get_drvdata(pdev); > > cancel_delayed_work_sync(&hdata->hotplug_work); > - cec_notifier_set_phys_addr(hdata->notifier, CEC_PHYS_ADDR_INVALID); > > component_del(&pdev->dev, &hdmi_component_ops); > platform_device_unregister(hdata->audio.pdev); > > - cec_notifier_put(hdata->notifier); > pm_runtime_disable(&pdev->dev); > > if (!IS_ERR(hdata->reg_hdmi_en)) > Regards, Hans
Hi Dariusz, On 7/1/19 4:59 PM, Dariusz Marcinkiewicz wrote: > This series updates remaining drivers in DRM to use new CEC notifier API. > > Those complement the "cec: improve notifier support, add > connector info" patch series and also replace 2 following patches from > there: > - [PATCHv8 09/13] dw-hdmi: use cec_notifier_conn_(un)register > - [PATCHv9 12/13] tda998x: use cec_notifier_conn_(un)register > > None of those changes were not tested on a real hardware. I assume you meant 'None of those changes were tested'? I just reviewed this series. Apologies for the delay, I was out for a week and then had to take care of other things. When you post v3, can you include all older drm patches (i.e. tda9950, dw-hdmi-cec, and the drm-dp-cec changes)? That way there is only one patch series that I have to review. Regards, Hans > > Changes since v1: > Those patches delay creation of notifiers until respective > connectors are constructed. It seems that those patches, for a > couple of drivers, by adding the delay, introduce a race between > notifiers' creation and the IRQs handling threads - at least I > don't see anything obvious in there that would explicitly forbid > such races to occur. v2 adds a write barrier to make sure IRQ > threads see the notifier once it is created (replacing the > WRITE_ONCE I put in v1). The best thing to do here, I believe, > would be not to have any synchronization and make sure that an IRQ > only gets enabled after the notifier is created. > > > Dariusz Marcinkiewicz (5): > drm: tda998x: use cec_notifier_conn_(un)register > drm: sti: use cec_notifier_conn_(un)register > drm: tegra: use cec_notifier_conn_(un)register > drm: dw-hdmi: use cec_notifier_conn_(un)register > drm: exynos: exynos_hdmi: use cec_notifier_conn_(un)register > > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 37 +++++++++++++++-------- > drivers/gpu/drm/exynos/exynos_hdmi.c | 33 ++++++++++++-------- > drivers/gpu/drm/i2c/tda998x_drv.c | 32 ++++++++++++++------ > drivers/gpu/drm/sti/sti_hdmi.c | 20 +++++++----- > drivers/gpu/drm/tegra/output.c | 18 ++++++----- > 5 files changed, 91 insertions(+), 49 deletions(-) >