drm: cec: convert DRM drivers to the new notifier API
Subsystems (11)
v6 cover drm: cec: convert DRM drivers to the new notifier API
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(-)
Signature · 6 lines
2.23.0.rc1.153.gdeed80330f-goog _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH v6 1/8 drm/i915/intel_hdmi: use cec_notifier_conn_(un)register
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.
--- drivers/gpu/drm/i915/display/intel_hdmi.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
drivers/gpu/drm/i915/display/intel_hdmi.c +9 -4
@@ -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" ); }
Signature · 6 lines
2.23.0.rc1.153.gdeed80330f-goog _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH v6 2/8 dw-hdmi-cec: use cec_notifier_cec_adap_(un)register
In reply to Dariusz MarcinkiewiczUse 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.
--- drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c +6 -7
@@ -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 ; }
Signature · 6 lines
2.23.0.rc1.153.gdeed80330f-goog _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH v6 3/8 tda9950: use cec_notifier_cec_adap_(un)register
In reply to Dariusz MarcinkiewiczUse the new cec_notifier_cec_adap_(un)register() functions to (un)register the notifier for the CEC adapter.
--- drivers/gpu/drm/i2c/tda9950.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/gpu/drm/i2c/tda9950.c +6 -6
@@ -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 ; }
Signature · 6 lines
2.23.0.rc1.153.gdeed80330f-goog _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v6 3/8] tda9950: use cec_notifier_cec_adap_(un)register
Quoted 51 lines · Dariusz Marcinkiewicz
Use the new cec_notifier_cec_adap_(un)register() functions to (un)register the notifier for the CEC adapter.
Signed-off-by Dariusz Marcinkiewicz <[email protected]>Signed-off-by Hans Verkuil <[email protected]>Tested-by Hans Verkuil <[email protected]>--- drivers/gpu/drm/i2c/tda9950.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)drivers/gpu/drm/i2c/tda9950.c +5
-5@@ -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 ; }
drivers/gpu/drm/i2c/tda9950.c static int tda9950_remove(struct i2c_client *client) · line 486
Quoted 6 lines
drivers/gpu/drm/i2c/tda9950.c +1
-1@@ -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);
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.
Quoted 7 lines
drivers/gpu/drm/i2c/tda9950.c +0
-0return 0 ; }Signature · 1 line
2.23.0.rc1.153.gdeed80330f-goog
Signature · 7 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v6 3/8] tda9950: use cec_notifier_cec_adap_(un)register
Quoted 63 lines · Russell King - ARM Linux admin
Quoted 51 lines · Dariusz Marcinkiewicz
Use the new cec_notifier_cec_adap_(un)register() functions to (un)register the notifier for the CEC adapter.
Signed-off-by Dariusz Marcinkiewicz <[email protected]>Signed-off-by Hans Verkuil <[email protected]>Tested-by Hans Verkuil <[email protected]>--- drivers/gpu/drm/i2c/tda9950.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)drivers/gpu/drm/i2c/tda9950.c +5
-5@@ -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 ; }drivers/gpu/drm/i2c/tda9950.c static int tda9950_remove(struct i2c_client *client) · line 486
Quoted 6 lines
drivers/gpu/drm/i2c/tda9950.c +1
-1@@ -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);It looks weird to have an unexpectedly different ordering of unregistration from the registration path - normally, unregistration is the reverse order of initialisation.
Quoted 6 lines
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.
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
Quoted 9 lines
Quoted 8 lines
drivers/gpu/drm/i2c/tda9950.c +0
-0return 0 ; }Signature · 1 line
2.23.0.rc1.153.gdeed80330f-goog
PATCH v6 4/8 drm: tda998x: use cec_notifier_conn_(un)register
In reply to Dariusz MarcinkiewiczUse 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.
--- drivers/gpu/drm/i2c/tda998x_drv.c | 33 +++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-)
drivers/gpu/drm/i2c/tda998x_drv.c +23 -10
@@ -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;
Signature · 6 lines
2.23.0.rc1.153.gdeed80330f-goog _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v6 4/8] drm: tda998x: use cec_notifier_conn_(un)register
Quoted 55 lines · Dariusz Marcinkiewicz
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_invalidatewhere appropriate,
- don't check for NULL notifier before callingcec_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 <[email protected]>Tested-by Hans Verkuil <[email protected]>--- drivers/gpu/drm/i2c/tda998x_drv.c | 33 +++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-)drivers/gpu/drm/i2c/tda998x_drv.c +11
-2@@ -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); +
drivers/gpu/drm/i2c/tda998x_drv.c static int tda998x_connector_init(struct tda998x_priv *priv, · line 1368
Quoted 10 lines
drivers/gpu/drm/i2c/tda998x_drv.c +10
-0+ 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;
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.
drivers/gpu/drm/i2c/tda998x_drv.c static void tda998x_destroy(struct device *dev) · line 1813
Quoted 11 lines
drivers/gpu/drm/i2c/tda998x_drv.c +2
-2+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);
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().
Quoted 20 lines
drivers/gpu/drm/i2c/tda998x_drv.c +0
-6}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;Signature · 1 line
2.23.0.rc1.153.gdeed80330f-goog
Signature · 3 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up
Re: [PATCH v6 4/8] drm: tda998x: use cec_notifier_conn_(un)register
Hello.
Quoted 16 lines · Russell King - ARM Linux admin
This also doesn't make sense:
tda998x_destroy()is the opposite oftda998x_create(). However,tda998x_connector_destroy()is the opposite oftda998x_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()
Quoted 3 lines
Anything you create in
tda998x_connector_create()must be cleaned up bytda998x_connector_destroy().
Thank you.
I've just sent out another revision of the patch, where registration and deregistration is symmetric. Please take a look.
Best regards.
PATCH v6 5/8 drm: sti: use cec_notifier_conn_(un)register
In reply to Dariusz MarcinkiewiczUse 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.
--- drivers/gpu/drm/sti/sti_hdmi.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)
drivers/gpu/drm/sti/sti_hdmi.c +12 -7
@@ -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 ; }
Signature · 6 lines
2.23.0.rc1.153.gdeed80330f-goog _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH v6 6/8 drm: tegra: use cec_notifier_conn_(un)register
In reply to Dariusz MarcinkiewiczUse 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
--- drivers/gpu/drm/tegra/output.c | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-)
drivers/gpu/drm/tegra/output.c +21 -7
@@ -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 ; }
Signature · 6 lines
2.23.0.rc1.153.gdeed80330f-goog _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH v6 7/8 drm: dw-hdmi: use cec_notifier_conn_(un)register
In reply to Dariusz MarcinkiewiczUse 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.
--- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 36 ++++++++++++++--------- 1 file changed, 22 insertions(+), 14 deletions(-)
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +22 -14
@@ -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);
Signature · 6 lines
2.23.0.rc1.153.gdeed80330f-goog _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v6 7/8] drm: dw-hdmi: use cec_notifier_conn_(un)register
Quoted 95 lines · Dariusz Marcinkiewicz
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 tocec_notifier_conn_unregister,
- usecec_notifier_phys_addr_invalidateto 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 <[email protected]>Tested-by Hans Verkuil <[email protected]>--- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 36 ++++++++++++++--------- 1 file changed, 22 insertions(+), 14 deletions(-)drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +21
-12@@ -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);
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c static void __dw_hdmi_remove(struct dw_hdmi *hdmi) · line 2832
Quoted 6 lines
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +1
-2@@ -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);
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
Quoted 4 lines
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +0
-0clk_disable_unprepare (hdmi->iahb_clk);clk_disable_unprepare (hdmi->isfr_clk);
Re: [PATCH v6 7/8] drm: dw-hdmi: use cec_notifier_conn_(un)register
Hi.
Quoted 4 lines · Hans Verkuil
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 frombridge_detach?
I've sent out v7 of the series where unregistration is done from bridge detach.
Regards.
PATCH v6 8/8 drm: exynos: exynos_hdmi: use cec_notifier_conn_(un)register
In reply to Dariusz MarcinkiewiczUse 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.
--- drivers/gpu/drm/exynos/exynos_hdmi.c | 31 ++++++++++++++++------------ 1 file changed, 18 insertions(+), 13 deletions(-)
drivers/gpu/drm/exynos/exynos_hdmi.c +18 -13
@@ -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))
Signature · 6 lines
2.23.0.rc1.153.gdeed80330f-goog _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel