Hi Sylwester,
2017년 09월 26일 23:17에 Sylwester Nawrocki 이(가) 쓴 글:
> The hdmi-codec interface added in this patch is required to properly
> support HDMI audio. Currently the audio part of the SoC internal
> HDMI transmitter is configured with fixed values, which makes HDMI
> audio working by chance, only on boards having an external audio
> codec connected in parallel with the HDMI audio transmitter's input
> I2S interface.
>
> Signed-off-by: Sylwester Nawrocki <s.nawro...@samsung.com>
> Reviewed-by: Andrzej Hajda <a.ha...@samsung.com>
>
> ---
> Tested on Odroid XU3 and Odroid XU4 with Ubuntu 14.04.
>
> Changes since v2:
> - u8 replaced with bool type,
> - the HDMI codec iec.status data used directly for setting up
> the HDMI controller registers rather than using hard coded
> constants,
> - constants used for configuring the HDMI_AUI_CON register
> instead of plain numbers,
> - if/IS_ERR/return replaced with PTR_ERR_OR_ZERO().
>
> Changes since v1:
> - rebased onto v4.14-rc1 and adapted locking
>
> Changes since RFC version:
> - fixed hdmi-codec locking issue
> - added a comment documenting struct hdmi_contex::mutex
> ---
> drivers/gpu/drm/exynos/Kconfig | 1 +
> drivers/gpu/drm/exynos/exynos_hdmi.c | 253
> ++++++++++++++++++++++++++---------
> drivers/gpu/drm/exynos/regs-hdmi.h | 8 +-
> 3 files changed, 197 insertions(+), 65 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/Kconfig b/drivers/gpu/drm/exynos/Kconfig
> index 305dc3d..5a7c9d8 100644
> --- a/drivers/gpu/drm/exynos/Kconfig
> +++ b/drivers/gpu/drm/exynos/Kconfig
> @@ -3,6 +3,7 @@ config DRM_EXYNOS
> depends on OF && DRM && (ARCH_S3C64XX || ARCH_EXYNOS ||
> ARCH_MULTIPLATFORM)
> select DRM_KMS_HELPER
> select VIDEOMODE_HELPERS
> + select SND_SOC_HDMI_CODEC if SND_SOC
> help
> Choose this option if you have a Samsung SoC EXYNOS chipset.
> If M is selected the module will be called exynosdrm.
> diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c
> b/drivers/gpu/drm/exynos/exynos_hdmi.c
> index 214fa5e..d2b389a 100644
> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
> @@ -40,7 +40,7 @@
> #include <linux/component.h>
> #include <linux/mfd/syscon.h>
> #include <linux/regmap.h>
> -
> +#include <sound/hdmi-codec.h>
> #include <drm/exynos_drm.h>
>
> #include <media/cec-notifier.h>
> @@ -111,12 +111,18 @@ struct hdmi_driver_data {
> struct string_array_spec clk_muxes;
> };
>
> +struct hdmi_audio {
> + struct platform_device *pdev;
> + struct hdmi_audio_infoframe infoframe;
> + struct hdmi_codec_params params;
> + bool enable;
> +};
> +
> struct hdmi_context {
> struct drm_encoder encoder;
> struct device *dev;
> struct drm_device *drm_dev;
> struct drm_connector connector;
> - bool powered;
> bool dvi_mode;
> struct delayed_work hotplug_work;
> struct drm_display_mode current_mode;
> @@ -137,6 +143,11 @@ struct hdmi_context {
> struct regulator *reg_hdmi_en;
> struct exynos_drm_clk phy_clk;
> struct drm_bridge *bridge;
> +
> + /* mutex protecting subsequent fields below */
> + struct mutex mutex;
> + struct hdmi_audio audio;
> + bool powered;
> };
>
> static inline struct hdmi_context *encoder_to_hdmi(struct drm_encoder *e)
> @@ -768,6 +779,22 @@ static int hdmi_clk_set_parents(struct hdmi_context
> *hdata, bool to_phy)
> return ret;
> }
>
> +static int hdmi_audio_infoframe_apply(struct hdmi_context *hdata)
> +{
> + struct hdmi_audio_infoframe *infoframe = &hdata->audio.infoframe;
> + u8 buf[HDMI_INFOFRAME_SIZE(AUDIO)];
> + int len;
> +
> + len = hdmi_audio_infoframe_pack(infoframe, buf, sizeof(buf));
> + if (len < 0)
> + return len;
> +
> + hdmi_reg_writeb(hdata, HDMI_AUI_CON, HDMI_AUI_CON_EVERY_VSYNC);
> + hdmi_reg_write_buf(hdata, HDMI_AUI_HEADER0, buf, len);
> +
> + return 0;
> +}
> +
> static void hdmi_reg_infoframes(struct hdmi_context *hdata)
> {
> union hdmi_infoframe frm;
> @@ -805,15 +832,7 @@ static void hdmi_reg_infoframes(struct hdmi_context
> *hdata)
> hdmi_reg_write_buf(hdata, HDMI_VSI_DATA(0), buf + 3, ret - 3);
> }
>
> - ret = hdmi_audio_infoframe_init(&frm.audio);
> - if (!ret) {
> - frm.audio.channels = 2;
> - ret = hdmi_audio_infoframe_pack(&frm.audio, buf, sizeof(buf));
> - }
> - if (ret > 0) {
> - hdmi_reg_writeb(hdata, HDMI_AUI_CON, HDMI_AUI_CON_EVERY_VSYNC);
> - hdmi_reg_write_buf(hdata, HDMI_AUI_HEADER0, buf, ret);
> - }
> + hdmi_audio_infoframe_apply(hdata);
> }
>
> static enum drm_connector_status hdmi_detect(struct drm_connector *connector,
> @@ -995,23 +1014,18 @@ static void hdmi_reg_acr(struct hdmi_context *hdata,
> u32 freq)
> hdmi_reg_writeb(hdata, HDMI_ACR_CON, 4);
> }
>
> -static void hdmi_audio_init(struct hdmi_context *hdata)
> +static void hdmi_audio_config(struct hdmi_context *hdata)
> {
> - u32 sample_rate, bits_per_sample;
> - u32 data_num, bit_ch, sample_frq;
> - u32 val;
> -
> - sample_rate = 44100;
> - bits_per_sample = 16;
> + u32 bit_ch = 1;
> + u32 data_num, val;
> + int i;
>
> - switch (bits_per_sample) {
> + switch (hdata->audio.params.sample_width) {
> case 20:
> data_num = 2;
> - bit_ch = 1;
> break;
> case 24:
> data_num = 3;
> - bit_ch = 1;
> break;
> default:
> data_num = 1;
> @@ -1019,7 +1033,7 @@ static void hdmi_audio_init(struct hdmi_context *hdata)
> break;
> }
>
> - hdmi_reg_acr(hdata, sample_rate);
> + hdmi_reg_acr(hdata, hdata->audio.params.sample_rate);
>
> hdmi_reg_writeb(hdata, HDMI_I2S_MUX_CON, HDMI_I2S_IN_DISABLE
> | HDMI_I2S_AUD_I2S | HDMI_I2S_CUV_I2S_ENABLE
> @@ -1029,12 +1043,6 @@ static void hdmi_audio_init(struct hdmi_context *hdata)
> | HDMI_I2S_CH1_EN | HDMI_I2S_CH2_EN);
>
> hdmi_reg_writeb(hdata, HDMI_I2S_MUX_CUV, HDMI_I2S_CUV_RL_EN);
> -
> - sample_frq = (sample_rate == 44100) ? 0 :
> - (sample_rate == 48000) ? 2 :
> - (sample_rate == 32000) ? 3 :
> - (sample_rate == 96000) ? 0xa : 0x0;
> -
> hdmi_reg_writeb(hdata, HDMI_I2S_CLK_CON, HDMI_I2S_CLK_DIS);
> hdmi_reg_writeb(hdata, HDMI_I2S_CLK_CON, HDMI_I2S_CLK_EN);
>
> @@ -1058,31 +1066,24 @@ static void hdmi_audio_init(struct hdmi_context
> *hdata)
> | HDMI_I2S_SET_SDATA_BIT(data_num)
> | HDMI_I2S_BASIC_FORMAT);
>
> - /* Configure register related to CUV information */
> - hdmi_reg_writeb(hdata, HDMI_I2S_CH_ST_0, HDMI_I2S_CH_STATUS_MODE_0
> - | HDMI_I2S_2AUD_CH_WITHOUT_PREEMPH
> - | HDMI_I2S_COPYRIGHT
> - | HDMI_I2S_LINEAR_PCM
> - | HDMI_I2S_CONSUMER_FORMAT);
> - hdmi_reg_writeb(hdata, HDMI_I2S_CH_ST_1, HDMI_I2S_CD_PLAYER);
> - hdmi_reg_writeb(hdata, HDMI_I2S_CH_ST_2, HDMI_I2S_SET_SOURCE_NUM(0));
> - hdmi_reg_writeb(hdata, HDMI_I2S_CH_ST_3, HDMI_I2S_CLK_ACCUR_LEVEL_2
> - | HDMI_I2S_SET_SMP_FREQ(sample_frq));
> - hdmi_reg_writeb(hdata, HDMI_I2S_CH_ST_4,
> - HDMI_I2S_ORG_SMP_FREQ_44_1
> - | HDMI_I2S_WORD_LEN_MAX24_24BITS
> - | HDMI_I2S_WORD_LEN_MAX_24BITS);
> + /* Configure registers related to CUV information */
> + for (i = 0; i < HDMI_I2S_CH_ST_MAXNUM; i++)
> + hdmi_reg_writeb(hdata, HDMI_I2S_CH_ST(i),
> + hdata->audio.params.iec.status[i]);
>
> hdmi_reg_writeb(hdata, HDMI_I2S_CH_ST_CON, HDMI_I2S_CH_STATUS_RELOAD);
> }
>
> -static void hdmi_audio_control(struct hdmi_context *hdata, bool onoff)
> +static void hdmi_audio_control(struct hdmi_context *hdata)
> {
> + bool enable = hdata->audio.enable;
> +
> if (hdata->dvi_mode)
> return;
>
> - hdmi_reg_writeb(hdata, HDMI_AUI_CON, onoff ? 2 : 0);
> - hdmi_reg_writemask(hdata, HDMI_CON_0, onoff ?
> + hdmi_reg_writeb(hdata, HDMI_AUI_CON, enable ?
> + HDMI_AVI_CON_EVERY_VSYNC : HDMI_AUI_CON_NO_TRAN);
> + hdmi_reg_writemask(hdata, HDMI_CON_0, enable ?
> HDMI_ASP_EN : HDMI_ASP_DIS, HDMI_ASP_MASK);
> }
>
> @@ -1398,13 +1399,14 @@ static void hdmiphy_conf_apply(struct hdmi_context
> *hdata)
> hdmiphy_wait_for_pll(hdata);
> }
>
> +/* Should be called with hdata->mutex mutex held */
> static void hdmi_conf_apply(struct hdmi_context *hdata)
> {
> hdmi_start(hdata, false);
> hdmi_conf_init(hdata);
> - hdmi_audio_init(hdata);
> + hdmi_audio_config(hdata);
> hdmi_mode_apply(hdata);
> - hdmi_audio_control(hdata, true);
> + hdmi_audio_control(hdata);
> }
>
> static void hdmi_mode_set(struct drm_encoder *encoder,
> @@ -1431,6 +1433,7 @@ static void hdmi_set_refclk(struct hdmi_context *hdata,
> bool on)
> SYSREG_HDMI_REFCLK_INT_CLK, on ? ~0 : 0);
> }
>
> +/* Should be called with hdata->mutex mutex held. */
> static void hdmiphy_enable(struct hdmi_context *hdata)
> {
> if (hdata->powered)
> @@ -1453,6 +1456,7 @@ static void hdmiphy_enable(struct hdmi_context *hdata)
> hdata->powered = true;
> }
>
> +/* Should be called with hdata->mutex mutex held. */
> static void hdmiphy_disable(struct hdmi_context *hdata)
> {
> if (!hdata->powered)
> @@ -1478,28 +1482,38 @@ static void hdmi_enable(struct drm_encoder *encoder)
> {
> struct hdmi_context *hdata = encoder_to_hdmi(encoder);
>
> + mutex_lock(&hdata->mutex);
> +
> hdmiphy_enable(hdata);
> hdmi_conf_apply(hdata);
> +
> + mutex_unlock(&hdata->mutex);
> }
>
> static void hdmi_disable(struct drm_encoder *encoder)
> {
> struct hdmi_context *hdata = encoder_to_hdmi(encoder);
>
> - if (!hdata->powered)
> + mutex_lock(&hdata->mutex);
> +
> + if (hdata->powered) {
> + /*
> + * The SFRs of VP and Mixer are updated by Vertical Sync of
> + * Timing generator which is a part of HDMI so the sequence
> + * to disable TV Subsystem should be as following,
> + * VP -> Mixer -> HDMI
> + *
> + * To achieve such sequence HDMI is disabled together with
> + * HDMI PHY, via pipe clock callback.
> + */
> + mutex_unlock(&hdata->mutex);
> + cancel_delayed_work(&hdata->hotplug_work);
> + cec_notifier_set_phys_addr(hdata->notifier,
> + CEC_PHYS_ADDR_INVALID);
> return;
> + }
>
> - /*
> - * The SFRs of VP and Mixer are updated by Vertical Sync of
> - * Timing generator which is a part of HDMI so the sequence
> - * to disable TV Subsystem should be as following,
> - * VP -> Mixer -> HDMI
> - *
> - * To achieve such sequence HDMI is disabled together with HDMI PHY, via
> - * pipe clock callback.
> - */
> - cancel_delayed_work(&hdata->hotplug_work);
> - cec_notifier_set_phys_addr(hdata->notifier, CEC_PHYS_ADDR_INVALID);
> + mutex_unlock(&hdata->mutex);
> }
>
> static const struct drm_encoder_helper_funcs
> exynos_hdmi_encoder_helper_funcs = {
> @@ -1513,6 +1527,104 @@ static void hdmi_disable(struct drm_encoder *encoder)
> .destroy = drm_encoder_cleanup,
> };
>
> +static void hdmi_audio_shutdown(struct device *dev, void *data)
> +{
> + struct hdmi_context *hdata = dev_get_drvdata(dev);
> +
> + mutex_lock(&hdata->mutex);
> +
> + hdata->audio.enable = false;
> +
> + if (hdata->powered)
> + hdmi_audio_control(hdata);
> +
> + mutex_unlock(&hdata->mutex);
> +}
> +
> +static int hdmi_audio_hw_params(struct device *dev, void *data,
> + struct hdmi_codec_daifmt *daifmt,
> + struct hdmi_codec_params *params)
> +{
> + struct hdmi_context *hdata = dev_get_drvdata(dev);
> +
> + if (daifmt->fmt != HDMI_I2S || daifmt->bit_clk_inv ||
> + daifmt->frame_clk_inv || daifmt->bit_clk_master ||
> + daifmt->frame_clk_master) {
> + dev_err(dev, "%s: Bad flags %d %d %d %d\n", __func__,
> + daifmt->bit_clk_inv, daifmt->frame_clk_inv,
> + daifmt->bit_clk_master,
> + daifmt->frame_clk_master);
> + return -EINVAL;
> + }
> +
> + mutex_lock(&hdata->mutex);
> +
> + hdata->audio.params = *params;
> +
> + if (hdata->powered) {
> + hdmi_audio_config(hdata);
> + hdmi_audio_infoframe_apply(hdata);
> + }
> +
> + mutex_unlock(&hdata->mutex);
> +
> + return 0;
> +}
> +
> +static int hdmi_audio_digital_mute(struct device *dev, void *data, bool mute)
> +{
> + struct hdmi_context *hdata = dev_get_drvdata(dev);
> +
> + mutex_lock(&hdata->mutex);
> +
> + hdata->audio.enable = !mute;
Wouldn't it be better to keep 'mute' instead of 'enable'? 'hdata->audio.enable'
is used by only hdmi_adio_control function and whether hdmi is power on or not
is already checked by 'hdata->powered'
> +
> + if (hdata->powered)
> + hdmi_audio_control(hdata);
> +
> + mutex_unlock(&hdata->mutex);
> +
> + return 0;
> +}
> +
> +static int hdmi_audio_get_eld(struct device *dev, void *data, uint8_t *buf,
> + size_t len)
> +{
> + struct hdmi_context *hdata = dev_get_drvdata(dev);
> + struct drm_connector *connector = &hdata->connector;
> +
> + memcpy(buf, connector->eld, min(sizeof(connector->eld), len));
> +
> + return 0;
> +}
> +
> +static const struct hdmi_codec_ops audio_codec_ops = {
> + .hw_params = hdmi_audio_hw_params,
> + .audio_shutdown = hdmi_audio_shutdown,
> + .digital_mute = hdmi_audio_digital_mute,
> + .get_eld = hdmi_audio_get_eld,
> +};
> +
> +static int hdmi_register_audio_device(struct hdmi_context *hdata)
> +{
> + struct hdmi_codec_pdata codec_data = {
> + .ops = &audio_codec_ops,
> + .max_i2s_channels = 6,
> + .i2s = 1,
> + };
> +
> + hdata->audio.pdev = platform_device_register_data(
> + hdata->dev, HDMI_CODEC_DRV_NAME, PLATFORM_DEVID_AUTO,
> + &codec_data, sizeof(codec_data));
> +
> + return PTR_ERR_OR_ZERO(hdata->audio.pdev);
> +}
> +
> +static void hdmi_unregister_audio_device(struct hdmi_context *hdata)
> +{
> + platform_device_unregister(hdata->audio.pdev);
> +}
This function is unnecessary wrapper. You can use
platform_device_unregister(hdata->audio.pdev) at probe callback.
And you would need to unregister audio device at remove callback.
> +
> static void hdmi_hotplug_work_func(struct work_struct *work)
> {
> struct hdmi_context *hdata;
> @@ -1588,11 +1700,14 @@ static void hdmiphy_clk_enable(struct exynos_drm_clk
> *clk, bool enable)
> {
> struct hdmi_context *hdata = container_of(clk, struct hdmi_context,
> phy_clk);
> + mutex_lock(&hdata->mutex);
>
> if (enable)
> hdmiphy_enable(hdata);
> else
> hdmiphy_disable(hdata);
> +
> + mutex_unlock(&hdata->mutex);
> }
>
> static int hdmi_bridge_init(struct hdmi_context *hdata)
> @@ -1697,6 +1812,7 @@ static int hdmi_bind(struct device *dev, struct device
> *master, void *data)
> struct drm_device *drm_dev = data;
> struct hdmi_context *hdata = dev_get_drvdata(dev);
> struct drm_encoder *encoder = &hdata->encoder;
> + struct hdmi_audio_infoframe *audio_infoframe = &hdata->audio.infoframe;
> struct exynos_drm_crtc *crtc;
> int ret;
>
> @@ -1704,6 +1820,12 @@ static int hdmi_bind(struct device *dev, struct device
> *master, void *data)
>
> hdata->phy_clk.enable = hdmiphy_clk_enable;
>
> + hdmi_audio_infoframe_init(audio_infoframe);
> + audio_infoframe->coding_type = HDMI_AUDIO_CODING_TYPE_STREAM;
> + audio_infoframe->sample_size = HDMI_AUDIO_SAMPLE_SIZE_STREAM;
> + audio_infoframe->sample_frequency = HDMI_AUDIO_SAMPLE_FREQUENCY_STREAM;
> + audio_infoframe->channels = 2;
Audio device is registered at probe callback so it would be better to move
above code into probe callback.
I coudln't see initializing audio infoframe should be done here.
Thanks,
Inki Dae
> +
> drm_encoder_init(drm_dev, encoder, &exynos_hdmi_encoder_funcs,
> DRM_MODE_ENCODER_TMDS, NULL);
>
> @@ -1818,6 +1940,8 @@ static int hdmi_probe(struct platform_device *pdev)
>
> hdata->dev = dev;
>
> + mutex_init(&hdata->mutex);
> +
> ret = hdmi_resources_init(hdata);
> if (ret) {
> if (ret != -EPROBE_DEFER)
> @@ -1877,12 +2001,19 @@ static int hdmi_probe(struct platform_device *pdev)
>
> pm_runtime_enable(dev);
>
> - ret = component_add(&pdev->dev, &hdmi_component_ops);
> + ret = hdmi_register_audio_device(hdata);
> if (ret)
> goto err_notifier_put;
>
> + ret = component_add(&pdev->dev, &hdmi_component_ops);
> + if (ret)
> + goto err_unregister_audio;
> +
> return ret;
>
> +err_unregister_audio:
> + hdmi_unregister_audio_device(hdata);
> +
> err_notifier_put:
> cec_notifier_put(hdata->notifier);
> pm_runtime_disable(dev);
> @@ -1921,6 +2052,8 @@ static int hdmi_remove(struct platform_device *pdev)
>
> put_device(&hdata->ddc_adpt->dev);
>
> + mutex_destroy(&hdata->mutex);
> +
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/exynos/regs-hdmi.h
> b/drivers/gpu/drm/exynos/regs-hdmi.h
> index a0507dc..d843b1f 100644
> --- a/drivers/gpu/drm/exynos/regs-hdmi.h
> +++ b/drivers/gpu/drm/exynos/regs-hdmi.h
> @@ -419,11 +419,9 @@
> #define HDMI_I2S_DSD_CON HDMI_I2S_BASE(0x01c)
> #define HDMI_I2S_MUX_CON HDMI_I2S_BASE(0x020)
> #define HDMI_I2S_CH_ST_CON HDMI_I2S_BASE(0x024)
> -#define HDMI_I2S_CH_ST_0 HDMI_I2S_BASE(0x028)
> -#define HDMI_I2S_CH_ST_1 HDMI_I2S_BASE(0x02c)
> -#define HDMI_I2S_CH_ST_2 HDMI_I2S_BASE(0x030)
> -#define HDMI_I2S_CH_ST_3 HDMI_I2S_BASE(0x034)
> -#define HDMI_I2S_CH_ST_4 HDMI_I2S_BASE(0x038)
> +/* n must be withing range 0...(HDMI_I2S_CH_ST_MAXNUM - 1) */
> +#define HDMI_I2S_CH_ST_MAXNUM 5
> +#define HDMI_I2S_CH_ST(n) HDMI_I2S_BASE(0x028 + 4 * (n))
> #define HDMI_I2S_CH_ST_SH_0 HDMI_I2S_BASE(0x03c)
> #define HDMI_I2S_CH_ST_SH_1 HDMI_I2S_BASE(0x040)
> #define HDMI_I2S_CH_ST_SH_2 HDMI_I2S_BASE(0x044)
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc"
> in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
