Re: [PATCH v2] drm: exynos: Add driver for HDMI audio interface

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 18.09.2017 18:19, Sylwester Nawrocki wrote:
> 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.nawrocki@xxxxxxxxxxx>
> ---
> Patch tested on Odroid XU3 and Odroid XU4 with Ubuntu 14.04.
>
> 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 +++++++++++++++++++++++++++++------
>  2 files changed, 210 insertions(+), 44 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..dc254b7 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,19 @@ struct hdmi_driver_data {
>  	struct string_array_spec clk_muxes;
>  };
>
> +struct hdmi_audio {
> +	struct platform_device *pdev;
> +	struct hdmi_audio_infoframe infoframe;
> +	unsigned int sample_rate;
> +	unsigned int sample_width;
> +	u8 enable;

Why not bool ?

> +};
> +
>  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 +144,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 +780,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 +833,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 +1015,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;
> +	u32 data_num, sample_freq, val;
> +	u32 bit_ch = 1;
>
> -	sample_rate = 44100;
> -	bits_per_sample = 16;
>
> -	switch (bits_per_sample) {
> +	switch (hdata->audio.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 +1034,7 @@ static void hdmi_audio_init(struct hdmi_context *hdata)
>  		break;
>  	}
>
> -	hdmi_reg_acr(hdata, sample_rate);
> +	hdmi_reg_acr(hdata, hdata->audio.sample_rate);
>
>  	hdmi_reg_writeb(hdata, HDMI_I2S_MUX_CON, HDMI_I2S_IN_DISABLE
>  				| HDMI_I2S_AUD_I2S | HDMI_I2S_CUV_I2S_ENABLE
> @@ -1030,10 +1045,21 @@ static void hdmi_audio_init(struct hdmi_context *hdata)
>
>  	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;
> +	switch (hdata->audio.sample_rate) {
> +	case 32000:
> +		sample_freq = 0x3;
> +		break;
> +	case 48000:
> +		sample_freq = 0x2;
> +		break;
> +	case 96000:
> +		sample_freq = 0xa;
> +		break;
> +	case 44100:
> +	default:
> +		sample_freq = 0;
> +		break;
> +	}
>
>  	hdmi_reg_writeb(hdata, HDMI_I2S_CLK_CON, HDMI_I2S_CLK_DIS);
>  	hdmi_reg_writeb(hdata, HDMI_I2S_CLK_CON, HDMI_I2S_CLK_EN);
> @@ -1067,7 +1093,7 @@ static void hdmi_audio_init(struct hdmi_context *hdata)
>  	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_I2S_SET_SMP_FREQ(sample_freq));
>  	hdmi_reg_writeb(hdata, HDMI_I2S_CH_ST_4,
>  			HDMI_I2S_ORG_SMP_FREQ_44_1
>  			| HDMI_I2S_WORD_LEN_MAX24_24BITS

I am not audio expert, so maybe I miss something but I wonder if it
wouldn't be good to fill HDMI_I2S_CH_ST_* with content of
hdmi_codec_params.iec.status?
This way you can remove some magic code above, but maybe it could be
done in separate patch.

> @@ -1076,13 +1102,15 @@ static void hdmi_audio_init(struct hdmi_context *hdata)
>  	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 ? 2 : 0);

While at it you can replace magic '2' to HDMI_AUI_CON_EVERY_VSYNC.

> +	hdmi_reg_writemask(hdata, HDMI_CON_0, enable ?
>  			HDMI_ASP_EN : HDMI_ASP_DIS, HDMI_ASP_MASK);
>  }
>
> @@ -1398,13 +1426,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 +1460,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 +1483,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 +1509,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);

Wouldn't be enough to change it to:
mutex_lock
powered = hdata->powered;
mutex_unlock
if (!powered)
    return
...

>  }
>
>  static const struct drm_encoder_helper_funcs exynos_hdmi_encoder_helper_funcs = {
> @@ -1513,6 +1554,109 @@ 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.sample_rate = params->sample_rate;
> +	hdata->audio.sample_width = params->sample_width;
> +	hdata->audio.infoframe = params->cea;
> +
> +	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;
> +
> +	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));
> +
> +	if (IS_ERR(hdata->audio.pdev))
> +		return PTR_ERR(hdata->audio.pdev);
> +
> +	return 0;

return PTR_ERR_OR_ZERO(...) ?

Beside these nitpicks:
Reviewed-by: Andrzej Hajda <a.hajda@xxxxxxxxxxx>

 --
Regards
Andrzej

> +}
> +
> +static void hdmi_unregister_audio_device(struct hdmi_context *hdata)
> +{
> +	platform_device_unregister(hdata->audio.pdev);
> +}
> +
>  static void hdmi_hotplug_work_func(struct work_struct *work)
>  {
>  	struct hdmi_context *hdata;
> @@ -1588,11 +1732,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 +1844,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 +1852,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;
> +
>  	drm_encoder_init(drm_dev, encoder, &exynos_hdmi_encoder_funcs,
>  			 DRM_MODE_ENCODER_TMDS, NULL);
>
> @@ -1818,6 +1972,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 +2033,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 +2084,8 @@ static int hdmi_remove(struct platform_device *pdev)
>
>  	put_device(&hdata->ddc_adpt->dev);
>
> +	mutex_destroy(&hdata->mutex);
> +
>  	return 0;
>  }
>
> --
> 1.9.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel


--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux SoC Development]     [Linux Rockchip Development]     [Linux USB Development]     [Video for Linux]     [Linux Audio Users]     [Linux SCSI]     [Yosemite News]

  Powered by Linux