This series updates DRM drivers to use new CEC notifier API.
Only first two patches were tested on the actual hardware.
Changes since v5: Fixed a warning about a missing comment for a new member of drm_dp_aux_cec struct. Sending to a wider audience, including maintainers of respective drivers. Changes since v4: Addressing review comments. Changes since v3: Updated adapter flags in dw-hdmi-cec. Changes since v2: Include all DRM patches from "cec: improve notifier support, add connector info connector info" series. 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 (8): drm/i915/intel_hdmi: use cec_notifier_conn_(un)register dw-hdmi-cec: use cec_notifier_cec_adap_(un)register tda9950: use cec_notifier_cec_adap_(un)register 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-cec.c | 13 ++++--- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 36 +++++++++++-------- drivers/gpu/drm/exynos/exynos_hdmi.c | 31 +++++++++------- drivers/gpu/drm/i2c/tda9950.c | 12 +++---- drivers/gpu/drm/i2c/tda998x_drv.c | 33 +++++++++++------ drivers/gpu/drm/i915/display/intel_hdmi.c | 13 ++++--- drivers/gpu/drm/sti/sti_hdmi.c | 19 ++++++---- drivers/gpu/drm/tegra/output.c | 28 +++++++++++---- 8 files changed, 117 insertions(+), 68 deletions(-)
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 Signed-off-by: Hans Verkuil hverkuil-cisco@xs4all.nl Tested-by: Hans Verkuil hverkuil-cisco@xs4all.nl --- drivers/gpu/drm/i915/display/intel_hdmi.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c index b1ca8e5bdb56d..9fcf2c58c29c5 100644 --- a/drivers/gpu/drm/i915/display/intel_hdmi.c +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c @@ -2752,8 +2752,9 @@ intel_hdmi_connector_register(struct drm_connector *connector)
static void intel_hdmi_destroy(struct drm_connector *connector) { - if (intel_attached_hdmi(connector)->cec_notifier) - cec_notifier_put(intel_attached_hdmi(connector)->cec_notifier); + struct cec_notifier *n = intel_attached_hdmi(connector)->cec_notifier; + + cec_notifier_conn_unregister(n);
intel_connector_destroy(connector); } @@ -3068,6 +3069,7 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port, struct drm_device *dev = intel_encoder->base.dev; struct drm_i915_private *dev_priv = to_i915(dev); enum port port = intel_encoder->port; + struct cec_connector_info conn_info;
DRM_DEBUG_KMS("Adding HDMI connector on port %c\n", port_name(port)); @@ -3120,8 +3122,11 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port, I915_WRITE(PEG_BAND_GAP_DATA, (temp & ~0xf) | 0xd); }
- intel_hdmi->cec_notifier = cec_notifier_get_conn(dev->dev, - port_identifier(port)); + cec_fill_conn_info_from_drm(&conn_info, connector); + + intel_hdmi->cec_notifier = + cec_notifier_conn_register(dev->dev, port_identifier(port), + &conn_info); if (!intel_hdmi->cec_notifier) DRM_DEBUG_KMS("CEC notifier get failed\n"); }
Use the new cec_notifier_cec_adap_(un)register() functions to (un)register the notifier for the CEC adapter.
Also adds CEC_CAP_CONNECTOR_INFO capability to the adapter.
Changes since v3: - add CEC_CAP_CONNECTOR_INFO to cec_allocate_adapter, - replace CEC_CAP_LOG_ADDRS | CEC_CAP_TRANSMIT | CEC_CAP_RC | CEC_CAP_PASSTHROUGH with CEC_CAP_DEFAULTS.
Signed-off-by: Dariusz Marcinkiewicz darekm@google.com Signed-off-by: Hans Verkuil hverkuil-cisco@xs4all.nl Tested-by: Hans Verkuil hverkuil-cisco@xs4all.nl --- drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c index 0f949978d3fcd..ac1e001d08829 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c @@ -256,8 +256,8 @@ static int dw_hdmi_cec_probe(struct platform_device *pdev) dw_hdmi_write(cec, 0, HDMI_CEC_POLARITY);
cec->adap = cec_allocate_adapter(&dw_hdmi_cec_ops, cec, "dw_hdmi", - CEC_CAP_LOG_ADDRS | CEC_CAP_TRANSMIT | - CEC_CAP_RC | CEC_CAP_PASSTHROUGH, + CEC_CAP_DEFAULTS | + CEC_CAP_CONNECTOR_INFO, CEC_MAX_LOG_ADDRS); if (IS_ERR(cec->adap)) return PTR_ERR(cec->adap); @@ -278,13 +278,14 @@ static int dw_hdmi_cec_probe(struct platform_device *pdev) if (ret < 0) return ret;
- cec->notify = cec_notifier_get(pdev->dev.parent); + cec->notify = cec_notifier_cec_adap_register(pdev->dev.parent, + NULL, cec->adap); if (!cec->notify) return -ENOMEM;
ret = cec_register_adapter(cec->adap, pdev->dev.parent); if (ret < 0) { - cec_notifier_put(cec->notify); + cec_notifier_cec_adap_unregister(cec->notify); return ret; }
@@ -294,8 +295,6 @@ static int dw_hdmi_cec_probe(struct platform_device *pdev) */ devm_remove_action(&pdev->dev, dw_hdmi_cec_del, cec);
- cec_register_cec_notifier(cec->adap, cec->notify); - return 0; }
@@ -303,8 +302,8 @@ static int dw_hdmi_cec_remove(struct platform_device *pdev) { struct dw_hdmi_cec *cec = platform_get_drvdata(pdev);
+ cec_notifier_cec_adap_unregister(cec->notify); cec_unregister_adapter(cec->adap); - cec_notifier_put(cec->notify);
return 0; }
Use the new cec_notifier_cec_adap_(un)register() functions to (un)register the notifier for the CEC adapter.
Signed-off-by: Dariusz Marcinkiewicz darekm@google.com Signed-off-by: Hans Verkuil hverkuil-cisco@xs4all.nl Tested-by: Hans Verkuil hverkuil-cisco@xs4all.nl --- drivers/gpu/drm/i2c/tda9950.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda9950.c b/drivers/gpu/drm/i2c/tda9950.c index 8039fc0d83db4..a5a75bdeb7a5f 100644 --- a/drivers/gpu/drm/i2c/tda9950.c +++ b/drivers/gpu/drm/i2c/tda9950.c @@ -420,7 +420,8 @@ static int tda9950_probe(struct i2c_client *client, priv->hdmi = glue->parent;
priv->adap = cec_allocate_adapter(&tda9950_cec_ops, priv, "tda9950", - CEC_CAP_DEFAULTS, + CEC_CAP_DEFAULTS | + CEC_CAP_CONNECTOR_INFO, CEC_MAX_LOG_ADDRS); if (IS_ERR(priv->adap)) return PTR_ERR(priv->adap); @@ -457,13 +458,14 @@ static int tda9950_probe(struct i2c_client *client, if (ret < 0) return ret;
- priv->notify = cec_notifier_get(priv->hdmi); + priv->notify = cec_notifier_cec_adap_register(priv->hdmi, NULL, + priv->adap); if (!priv->notify) return -ENOMEM;
ret = cec_register_adapter(priv->adap, priv->hdmi); if (ret < 0) { - cec_notifier_put(priv->notify); + cec_notifier_cec_adap_unregister(priv->notify); return ret; }
@@ -473,8 +475,6 @@ static int tda9950_probe(struct i2c_client *client, */ devm_remove_action(dev, tda9950_cec_del, priv);
- cec_register_cec_notifier(priv->adap, priv->notify); - return 0; }
@@ -482,8 +482,8 @@ static int tda9950_remove(struct i2c_client *client) { struct tda9950_priv *priv = i2c_get_clientdata(client);
+ cec_notifier_cec_adap_unregister(priv->notify); cec_unregister_adapter(priv->adap); - cec_notifier_put(priv->notify);
return 0; }
On Tue, Aug 13, 2019 at 01:02:35PM +0200, Dariusz Marcinkiewicz wrote:
It looks weird to have an unexpectedly different ordering of unregistration from the registration path - normally, unregistration is the reverse order of initialisation.
In the initialisation path, it seems that we register the notifier and _then_ the adapter. Here, we unregister the notifier and then the adapter rather than what would normally be expected. Why is this? I suspect there will be drivers created that do this the "normal" way round, so if this is a requirement, it needs to be made plainly obvious.
On 8/13/19 1:32 PM, Russell King - ARM Linux admin wrote:
It's not a requirement, it just feels better to do it in this order since cec_unregister_adapter will in general also delete the adapter (unless an application keeps the cec device open).
So the order is actually: allocate_adapter, then register notifier and: unregister notifier, then unregister (and typically delete) adapter
Regards,
Hans
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 v2: - cec_notifier_phys_addr_invalidate where appropriate, - don't check for NULL notifier before calling cec_notifier_conn_unregister. 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 Tested-by: Hans Verkuil hverkuil-cisco@xs4all.nl --- drivers/gpu/drm/i2c/tda998x_drv.c | 33 +++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 61e042918a7fc..19a63ee1b3f53 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -804,9 +804,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_phys_addr_invalidate( + notify); }
handled = true; @@ -1331,6 +1336,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; @@ -1347,6 +1354,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);
@@ -1790,8 +1810,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) @@ -1916,12 +1935,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;
On Tue, Aug 13, 2019 at 01:02:36PM +0200, Dariusz Marcinkiewicz wrote:
To nitpick, this comment and the following code do not go together.
I think what you actually mean is:
* Make sure that tda998x_irq_thread sees the notifier * only after it is fully constructed.
This also doesn't make sense: tda998x_destroy() is the opposite of tda998x_create(). However, tda998x_connector_destroy() is the opposite of tda998x_connector_create().
By moving the CEC creation code into tda998x_connector_create(), you are creating the possibility for the following sequence to mess up CEC and leak:
tda998x_create() tda998x_connector_create() tda998x_connector_destroy() tda998x_connector_create() tda998x_connector_destroy() tda998x_destroy()
Anything you create in tda998x_connector_create() must be cleaned up by tda998x_connector_destroy().
Hello.
On Tue, Aug 13, 2019 at 1:20 PM Russell King - ARM Linux admin linux@armlinux.org.uk wrote:
Thank you.
I've just sent out another revision of the patch, where registration and deregistration is symmetric. Please take a look.
Best regards.
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 v2: Don't invalidate physical address before unregistering the notifier.
Signed-off-by: Dariusz Marcinkiewicz darekm@google.com --- drivers/gpu/drm/sti/sti_hdmi.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c index 9862c322f0c4a..bd15902b825ad 100644 --- a/drivers/gpu/drm/sti/sti_hdmi.c +++ b/drivers/gpu/drm/sti/sti_hdmi.c @@ -1256,6 +1256,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; @@ -1318,6 +1319,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);
@@ -1331,6 +1340,9 @@ 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_conn_unregister(hdmi->notifier); }
static const struct component_ops sti_hdmi_ops = { @@ -1436,10 +1448,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)) @@ -1459,14 +1467,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; }
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 v4: - only create a CEC notifier for HDMI connectors
Signed-off-by: Dariusz Marcinkiewicz darekm@google.com Tested-by: Hans Verkuil hverkuil-cisco@xs4all.nl --- drivers/gpu/drm/tegra/output.c | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c index bdcaa4c7168cf..34373734ff689 100644 --- a/drivers/gpu/drm/tegra/output.c +++ b/drivers/gpu/drm/tegra/output.c @@ -70,6 +70,11 @@ 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); + + if (output->cec) + cec_notifier_conn_unregister(output->cec); + drm_connector_unregister(connector); drm_connector_cleanup(connector); } @@ -163,18 +168,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 (output->hpd_gpio) free_irq(output->hpd_irq, output);
@@ -184,6 +182,7 @@ void tegra_output_remove(struct tegra_output *output)
int tegra_output_init(struct drm_device *drm, struct tegra_output *output) { + int connector_type; int err;
if (output->panel) { @@ -199,6 +198,21 @@ int tegra_output_init(struct drm_device *drm, struct tegra_output *output) if (output->hpd_gpio) enable_irq(output->hpd_irq);
+ connector_type = output->connector.connector_type; + /* + * Create a CEC notifier for HDMI connector. + */ + if (connector_type == DRM_MODE_CONNECTOR_HDMIA || + connector_type == DRM_MODE_CONNECTOR_HDMIB) { + struct cec_connector_info conn_info; + + 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; }
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 v4: - typo fix Changes since v2: - removed unnecessary NULL check before a call to cec_notifier_conn_unregister, - use cec_notifier_phys_addr_invalidate to invalidate physical address. 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 Tested-by: Hans Verkuil hverkuil-cisco@xs4all.nl --- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 36 ++++++++++++++--------- 1 file changed, 22 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index 83b94b66e464e..c00184700bb9d 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -2194,6 +2194,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; @@ -2207,6 +2209,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; }
@@ -2373,9 +2387,13 @@ 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 *notifier; + + notifier = READ_ONCE(hdmi->cec_notifier); + if (notifier) + cec_notifier_phys_addr_invalidate(notifier); + } }
if (intr_stat & HDMI_IH_PHY_STAT0_HPD) { @@ -2693,12 +2711,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 @@ -2796,9 +2808,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); @@ -2820,8 +2829,7 @@ static void __dw_hdmi_remove(struct dw_hdmi *hdmi) /* Disable all interrupts */ 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);
On 8/13/19 1:02 PM, Dariusz Marcinkiewicz wrote:
Russell's review caused me to take another look at this series, and it made wonder if cec_notifier_conn_unregister() shouldn't be called from bridge_detach?
Regards,
Hans
clk_disable_unprepare(hdmi->iahb_clk); clk_disable_unprepare(hdmi->isfr_clk);
Hi.
On Tue, Aug 13, 2019 at 1:38 PM Hans Verkuil hverkuil-cisco@xs4all.nl wrote:
Russell's review caused me to take another look at this series, and it made wonder if cec_notifier_conn_unregister() shouldn't be called from bridge_detach?
I've sent out v7 of the series where unregistration is done from bridge detach.
Regards.
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 v2: - removed unnecessary call to invalidate phys address before deregistering the notifier, - use cec_notifier_phys_addr_invalidate instead of setting invalid address on a notifier.
Signed-off-by: Dariusz Marcinkiewicz darekm@google.com Tested-by: Hans Verkuil hverkuil-cisco@xs4all.nl --- drivers/gpu/drm/exynos/exynos_hdmi.c | 31 ++++++++++++++++------------ 1 file changed, 18 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c index bc1565f1822ab..d532b468d9af5 100644 --- a/drivers/gpu/drm/exynos/exynos_hdmi.c +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c @@ -852,6 +852,10 @@ 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_conn_unregister(hdata->notifier); + drm_connector_unregister(connector); drm_connector_cleanup(connector); } @@ -935,6 +939,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; @@ -957,6 +962,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; }
@@ -1528,8 +1542,8 @@ 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_phys_addr_invalidate(hdata->notifier); return; }
@@ -2006,12 +2020,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; @@ -2023,7 +2031,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) @@ -2034,8 +2042,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: @@ -2054,12 +2061,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))
dri-devel@lists.freedesktop.org
-
Dariusz Marcinkiewicz -
Hans Verkuil -
Russell King - ARM Linux admin