| Message ID | 20200206102509.2.I230fd59de28e73934a91cb01424e25b9e84727f4@changeid (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 5d3c644773925c3568617435e42a9404a114c428 |
| Headers | show |
| Series | ASoC: mediatek: mt8173-rt5650: HDMI jack reporting | expand |
Hi, Tzung-Bi: On Thu, 2020-02-06 at 11:17 +0800, Tzung-Bi Shih wrote: > 1. > Provides a callback (i.e. mtk_hdmi_audio_hook_plugged_cb) to hdmi-codec. > When ASoC machine driver calls hdmi_codec_set_jack_detect(), the > callback will be invoked to save plugged_cb and codec_dev parameters. > > +---------+ set_jack_ +------------+ plugged_cb +----------+ > | machine | ----------> | hdmi-codec | ----------> | mtk-hdmi | > +---------+ detect() +------------+ codec_dev +----------+ > > 2. > When there is any jack status changes, mtk-hdmi will call the > plugged_cb() to notify hdmi-codec. And then hdmi-codec will call > snd_soc_jack_report(). > > +----------+ plugged_cb +------------+ > | mtk-hdmi | ----------> | hdmi-codec | -> snd_soc_jack_report() > +----------+ codec_dev +------------+ > connector_status > > Signed-off-by: Tzung-Bi Shih <tzungbi@google.com> > --- > drivers/gpu/drm/mediatek/mtk_hdmi.c | 34 ++++++++++++++++++++++++++--- > 1 file changed, 31 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c b/drivers/gpu/drm/mediatek/mtk_hdmi.c > index 23c2b0e8693d..fccdd975947d 100644 > --- a/drivers/gpu/drm/mediatek/mtk_hdmi.c > +++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c > @@ -169,6 +169,8 @@ struct mtk_hdmi { > bool audio_enable; > bool powered; > bool enabled; > + hdmi_codec_plugged_cb plugged_cb; > + struct device *codec_dev; > }; > > static inline struct mtk_hdmi *hdmi_ctx_from_bridge(struct drm_bridge *b) > @@ -1194,13 +1196,23 @@ static void mtk_hdmi_clk_disable_audio(struct mtk_hdmi *hdmi) > clk_disable_unprepare(hdmi->clk[MTK_HDMI_CLK_AUD_SPDIF]); > } > > +static enum drm_connector_status > +mtk_hdmi_update_plugged_status(struct mtk_hdmi *hdmi) > +{ > + bool connected = mtk_cec_hpd_high(hdmi->cec_dev); > + > + if (hdmi->plugged_cb && hdmi->codec_dev) > + hdmi->plugged_cb(hdmi->codec_dev, connected); > + > + return connected ? > + connector_status_connected : connector_status_disconnected; > +} > + > static enum drm_connector_status hdmi_conn_detect(struct drm_connector *conn, > bool force) > { > struct mtk_hdmi *hdmi = hdmi_ctx_from_conn(conn); > - > - return mtk_cec_hpd_high(hdmi->cec_dev) ? > - connector_status_connected : connector_status_disconnected; > + return mtk_hdmi_update_plugged_status(hdmi); > } > > static void hdmi_conn_destroy(struct drm_connector *conn) > @@ -1648,20 +1660,36 @@ static int mtk_hdmi_audio_get_eld(struct device *dev, void *data, uint8_t *buf, > return 0; > } > > +static int mtk_hdmi_audio_hook_plugged_cb(struct device *dev, void *data, > + hdmi_codec_plugged_cb fn, > + struct device *codec_dev) > +{ > + struct mtk_hdmi *hdmi = data; > + > + hdmi->plugged_cb = fn; > + hdmi->codec_dev = codec_dev; > + mtk_hdmi_update_plugged_status(hdmi); I think hdmi_conn_detect() and mtk_hdmi_audio_hook_plugged_cb() would be called in different thread. So it's necessary to use a mutex to protect this. Regards, CK > + > + return 0; > +} > + > static const struct hdmi_codec_ops mtk_hdmi_audio_codec_ops = { > .hw_params = mtk_hdmi_audio_hw_params, > .audio_startup = mtk_hdmi_audio_startup, > .audio_shutdown = mtk_hdmi_audio_shutdown, > .digital_mute = mtk_hdmi_audio_digital_mute, > .get_eld = mtk_hdmi_audio_get_eld, > + .hook_plugged_cb = mtk_hdmi_audio_hook_plugged_cb, > }; > > static int mtk_hdmi_register_audio_driver(struct device *dev) > { > + struct mtk_hdmi *hdmi = dev_get_drvdata(dev); > struct hdmi_codec_pdata codec_data = { > .ops = &mtk_hdmi_audio_codec_ops, > .max_i2s_channels = 2, > .i2s = 1, > + .data = hdmi, > }; > struct platform_device *pdev; >
On Wed, Feb 12, 2020 at 4:19 PM CK Hu <ck.hu@mediatek.com> wrote: > I think hdmi_conn_detect() and mtk_hdmi_audio_hook_plugged_cb() would be > called in different thread. So it's necessary to use a mutex to protect > this. Thanks for the reminder. I feel using mutex here is overkill. Please see https://patchwork.kernel.org/patch/11378413/ for my proposed solution.
Hi, Tzung-Bi: On Wed, 2020-02-12 at 19:31 +0800, Tzung-Bi Shih wrote: > On Wed, Feb 12, 2020 at 4:19 PM CK Hu <ck.hu@mediatek.com> wrote: > > I think hdmi_conn_detect() and mtk_hdmi_audio_hook_plugged_cb() would be > > called in different thread. So it's necessary to use a mutex to protect > > this. > > Thanks for the reminder. I feel using mutex here is overkill. Please > see https://patchwork.kernel.org/patch/11378413/ for my proposed > solution. > I'm not only consider the race condition of plugged_cb and codec_dev. I also care about the atomic of mtk_cec_hpd_high() and hdmi->plugged_cb(). If these two function is not an atomic operation, below is an example of problem: <Status disconnected> 1. Thread A call mtk_hdmi_audio_hook_plugged_cb() 2. Thread A call mtk_cec_hpd_high() and get disconnected. <Status connected> 3. Thread B call hdmi_conn_detect() 4. Thread B call mtk_cec_hpd_high() and get connected 5. Thread B callback plugged_cb() with connected 6. Thread A callback plugged_cb() with disconnected (Bug here) Regards, CK > _______________________________________________ > Linux-mediatek mailing list > Linux-mediatek@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-mediatek
On Thu, Feb 13, 2020 at 9:57 AM CK Hu <ck.hu@mediatek.com> wrote: > I'm not only consider the race condition of plugged_cb and codec_dev. I > also care about the atomic of mtk_cec_hpd_high() and hdmi->plugged_cb(). > If these two function is not an atomic operation, below is an example of > problem: > > <Status disconnected> > 1. Thread A call mtk_hdmi_audio_hook_plugged_cb() > 2. Thread A call mtk_cec_hpd_high() and get disconnected. > <Status connected> > 3. Thread B call hdmi_conn_detect() > 4. Thread B call mtk_cec_hpd_high() and get connected > 5. Thread B callback plugged_cb() with connected > 6. Thread A callback plugged_cb() with disconnected (Bug here) Another attempt: https://patchwork.kernel.org/patch/11379979/
diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c b/drivers/gpu/drm/mediatek/mtk_hdmi.c index 23c2b0e8693d..fccdd975947d 100644 --- a/drivers/gpu/drm/mediatek/mtk_hdmi.c +++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c @@ -169,6 +169,8 @@ struct mtk_hdmi { bool audio_enable; bool powered; bool enabled; + hdmi_codec_plugged_cb plugged_cb; + struct device *codec_dev; }; static inline struct mtk_hdmi *hdmi_ctx_from_bridge(struct drm_bridge *b) @@ -1194,13 +1196,23 @@ static void mtk_hdmi_clk_disable_audio(struct mtk_hdmi *hdmi) clk_disable_unprepare(hdmi->clk[MTK_HDMI_CLK_AUD_SPDIF]); } +static enum drm_connector_status +mtk_hdmi_update_plugged_status(struct mtk_hdmi *hdmi) +{ + bool connected = mtk_cec_hpd_high(hdmi->cec_dev); + + if (hdmi->plugged_cb && hdmi->codec_dev) + hdmi->plugged_cb(hdmi->codec_dev, connected); + + return connected ? + connector_status_connected : connector_status_disconnected; +} + static enum drm_connector_status hdmi_conn_detect(struct drm_connector *conn, bool force) { struct mtk_hdmi *hdmi = hdmi_ctx_from_conn(conn); - - return mtk_cec_hpd_high(hdmi->cec_dev) ? - connector_status_connected : connector_status_disconnected; + return mtk_hdmi_update_plugged_status(hdmi); } static void hdmi_conn_destroy(struct drm_connector *conn) @@ -1648,20 +1660,36 @@ static int mtk_hdmi_audio_get_eld(struct device *dev, void *data, uint8_t *buf, return 0; } +static int mtk_hdmi_audio_hook_plugged_cb(struct device *dev, void *data, + hdmi_codec_plugged_cb fn, + struct device *codec_dev) +{ + struct mtk_hdmi *hdmi = data; + + hdmi->plugged_cb = fn; + hdmi->codec_dev = codec_dev; + mtk_hdmi_update_plugged_status(hdmi); + + return 0; +} + static const struct hdmi_codec_ops mtk_hdmi_audio_codec_ops = { .hw_params = mtk_hdmi_audio_hw_params, .audio_startup = mtk_hdmi_audio_startup, .audio_shutdown = mtk_hdmi_audio_shutdown, .digital_mute = mtk_hdmi_audio_digital_mute, .get_eld = mtk_hdmi_audio_get_eld, + .hook_plugged_cb = mtk_hdmi_audio_hook_plugged_cb, }; static int mtk_hdmi_register_audio_driver(struct device *dev) { + struct mtk_hdmi *hdmi = dev_get_drvdata(dev); struct hdmi_codec_pdata codec_data = { .ops = &mtk_hdmi_audio_codec_ops, .max_i2s_channels = 2, .i2s = 1, + .data = hdmi, }; struct platform_device *pdev;
1. Provides a callback (i.e. mtk_hdmi_audio_hook_plugged_cb) to hdmi-codec. When ASoC machine driver calls hdmi_codec_set_jack_detect(), the callback will be invoked to save plugged_cb and codec_dev parameters. +---------+ set_jack_ +------------+ plugged_cb +----------+ | machine | ----------> | hdmi-codec | ----------> | mtk-hdmi | +---------+ detect() +------------+ codec_dev +----------+ 2. When there is any jack status changes, mtk-hdmi will call the plugged_cb() to notify hdmi-codec. And then hdmi-codec will call snd_soc_jack_report(). +----------+ plugged_cb +------------+ | mtk-hdmi | ----------> | hdmi-codec | -> snd_soc_jack_report() +----------+ codec_dev +------------+ connector_status Signed-off-by: Tzung-Bi Shih <tzungbi@google.com> --- drivers/gpu/drm/mediatek/mtk_hdmi.c | 34 ++++++++++++++++++++++++++--- 1 file changed, 31 insertions(+), 3 deletions(-)