| Message ID | d99e90932aad1a644fa32ff22fd2cb56b830341b.1390813481.git.moinejf@free.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jan 27, 2014 at 09:48:54AM +0100, Jean-Francois Moine wrote: > + eld_ver = eld[0] >> 3; > + if (eld_ver != 2 && eld_ver != 31) > + return 0; > + > + mnl = eld[4] & 0x1f; > + if (mnl > 16) > + return 0; > + > + sad_count = eld[5] >> 4; > + sad = eld + 20 + mnl; Can this parsing code be factored out - it (or large parts of it) should be usable by other HDMI devices shouldn't it?
On Mon, Jan 27, 2014 at 08:44:41PM +0000, Mark Brown wrote: > On Mon, Jan 27, 2014 at 09:48:54AM +0100, Jean-Francois Moine wrote: > > > + eld_ver = eld[0] >> 3; > > + if (eld_ver != 2 && eld_ver != 31) > > + return 0; > > + > > + mnl = eld[4] & 0x1f; > > + if (mnl > 16) > > + return 0; > > + > > + sad_count = eld[5] >> 4; > > + sad = eld + 20 + mnl; > > Can this parsing code be factored out - it (or large parts of it) should > be usable by other HDMI devices shouldn't it? Yes, preferably as a generic ALSA helper rather than an ASoC helper - I don't see any need for this to be ASoC specific (I have a pure ALSA driver which has very similar code in it.)
On Mon, Jan 27, 2014 at 08:49:15PM +0000, Russell King - ARM Linux wrote: > On Mon, Jan 27, 2014 at 08:44:41PM +0000, Mark Brown wrote: > > Can this parsing code be factored out - it (or large parts of it) should > > be usable by other HDMI devices shouldn't it? > Yes, preferably as a generic ALSA helper rather than an ASoC helper - > I don't see any need for this to be ASoC specific (I have a pure ALSA > driver which has very similar code in it.) Indeed, definitely ALSA generic - ideally we could factor a lot of the integration with the video side out.
At Mon, 27 Jan 2014 20:54:37 +0000, Mark Brown wrote: > > On Mon, Jan 27, 2014 at 08:49:15PM +0000, Russell King - ARM Linux wrote: > > On Mon, Jan 27, 2014 at 08:44:41PM +0000, Mark Brown wrote: > > > > Can this parsing code be factored out - it (or large parts of it) should > > > be usable by other HDMI devices shouldn't it? > > > Yes, preferably as a generic ALSA helper rather than an ASoC helper - > > I don't see any need for this to be ASoC specific (I have a pure ALSA > > driver which has very similar code in it.) > > Indeed, definitely ALSA generic - ideally we could factor a lot of the > integration with the video side out. Yes, indeed. OTOH, as discussed recently, we're heading to move from ELD parsing to more direct communication between video and audio drivers for HD-audio. ELD will be still provided to user-space, but not evaluated any longer in the new scenario. Takashi
On Tue, Jan 28, 2014 at 10:23:57AM +0100, Takashi Iwai wrote: > Mark Brown wrote: > > On Mon, Jan 27, 2014 at 08:49:15PM +0000, Russell King - ARM Linux wrote: > > > Yes, preferably as a generic ALSA helper rather than an ASoC helper - > > > I don't see any need for this to be ASoC specific (I have a pure ALSA > > > driver which has very similar code in it.) > > Indeed, definitely ALSA generic - ideally we could factor a lot of the > > integration with the video side out. > Yes, indeed. > OTOH, as discussed recently, we're heading to move from ELD parsing to > more direct communication between video and audio drivers for > HD-audio. ELD will be still provided to user-space, but not evaluated > any longer in the new scenario. That sort of refactoring being one of the best reasons to keep things out of individual drivers! Having said all this I don't know if it's worth blocking Jean-Francois' work on that, it's an improvement in itself. Splitting the code out a bit would be good to help prepare but having the full refactoring done might be too much of a blocker.
At Tue, 28 Jan 2014 11:00:51 +0000, Mark Brown wrote: > > On Tue, Jan 28, 2014 at 10:23:57AM +0100, Takashi Iwai wrote: > > Mark Brown wrote: > > > On Mon, Jan 27, 2014 at 08:49:15PM +0000, Russell King - ARM Linux wrote: > > > > > Yes, preferably as a generic ALSA helper rather than an ASoC helper - > > > > I don't see any need for this to be ASoC specific (I have a pure ALSA > > > > driver which has very similar code in it.) > > > > Indeed, definitely ALSA generic - ideally we could factor a lot of the > > > integration with the video side out. > > > Yes, indeed. > > > OTOH, as discussed recently, we're heading to move from ELD parsing to > > more direct communication between video and audio drivers for > > HD-audio. ELD will be still provided to user-space, but not evaluated > > any longer in the new scenario. > > That sort of refactoring being one of the best reasons to keep things > out of individual drivers! Having said all this I don't know if it's > worth blocking Jean-Francois' work on that, it's an improvement in > itself. Splitting the code out a bit would be good to help prepare but > having the full refactoring done might be too much of a blocker. I just rather wanted to point out the general direction for the further development, didn't mean NAK. Jean-Francois' patch itself looks simple enough, so I see no problem to take it for now as is. Takashi
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 186c751..3c8b4d7 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -45,6 +45,8 @@ struct tda998x_priv { wait_queue_head_t wq_edid; volatile int wq_edid_wait; struct drm_encoder *encoder; + + u8 *eld; }; #define to_tda998x_priv(x) ((struct tda998x_priv *)to_encoder_slave(x)->slave_priv) @@ -731,6 +733,14 @@ tda998x_configure_audio(struct tda998x_priv *priv, } /* tda998x codec interface */ +u8 *tda998x_audio_get_eld(struct i2c_client *client) +{ + struct tda998x_priv *priv = i2c_get_clientdata(client); + + return priv->eld; +} +EXPORT_SYMBOL_GPL(tda998x_audio_get_eld); + void tda998x_audio_update(struct i2c_client *client, int format, int port) @@ -1186,6 +1196,11 @@ tda998x_encoder_get_modes(struct drm_encoder *encoder, drm_mode_connector_update_edid_property(connector, edid); n = drm_add_edid_modes(connector, edid); priv->is_hdmi_sink = drm_detect_hdmi_monitor(edid); + + /* keep the EDID as ELD for the audio subsystem */ + drm_edid_to_eld(connector, edid); + priv->eld = connector->eld; + kfree(edid); } diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h index 0459931..64aaaa8 100644 --- a/include/drm/i2c/tda998x.h +++ b/include/drm/i2c/tda998x.h @@ -26,6 +26,7 @@ struct tda998x_encoder_params { unsigned audio_sample_rate; }; +u8 *tda998x_audio_get_eld(struct i2c_client *client); void tda998x_audio_update(struct i2c_client *client, int format, int port); diff --git a/sound/soc/codecs/tda998x.c b/sound/soc/codecs/tda998x.c index d724f7d..500ac61 100644 --- a/sound/soc/codecs/tda998x.c +++ b/sound/soc/codecs/tda998x.c @@ -91,10 +91,79 @@ static int tda_startup(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { struct tda_priv *priv = snd_soc_codec_get_drvdata(dai->codec); + u8 *eld = NULL; + static unsigned rates_mask[] = { + SNDRV_PCM_RATE_32000, + SNDRV_PCM_RATE_44100, + SNDRV_PCM_RATE_48000, + SNDRV_PCM_RATE_88200, + SNDRV_PCM_RATE_96000, + SNDRV_PCM_RATE_176400, + SNDRV_PCM_RATE_192000, + }; /* memorize the used DAI */ priv->dai_id = dai->id; + /* get the ELD from the tda998x driver */ + if (!priv->i2c_client) + tda_get_encoder(priv); + if (priv->i2c_client) + eld = tda998x_audio_get_eld(priv->i2c_client); + + /* adjust the hw params from the ELD (EDID) */ + if (eld) { + struct snd_soc_dai_driver *dai_drv = dai->driver; + struct snd_soc_pcm_stream *stream = &dai_drv->playback; + u8 *sad; + int sad_count; + unsigned eld_ver, mnl, rates, rate_mask, i; + unsigned max_channels, fmt; + u64 formats; + + eld_ver = eld[0] >> 3; + if (eld_ver != 2 && eld_ver != 31) + return 0; + + mnl = eld[4] & 0x1f; + if (mnl > 16) + return 0; + + sad_count = eld[5] >> 4; + sad = eld + 20 + mnl; + + /* Start from the basic audio settings */ + max_channels = 2; + rates = 0; + fmt = 0; + while (sad_count--) { + switch (sad[0] & 0x78) { + case 0x08: /* PCM */ + max_channels = max(max_channels, (sad[0] & 7) + 1u); + rates |= sad[1]; + fmt |= sad[2] & 0x07; + break; + } + sad += 3; + } + + for (rate_mask = i = 0; i < ARRAY_SIZE(rates_mask); i++) + if (rates & 1 << i) + rate_mask |= rates_mask[i]; + formats = 0; + if (fmt & 1) + formats |= SNDRV_PCM_FMTBIT_S16_LE; + if (fmt & 2) + formats |= SNDRV_PCM_FMTBIT_S20_3LE; + if (fmt & 4) + formats |= SNDRV_PCM_FMTBIT_S24_LE; + + /* change the snd_soc_pcm_stream values of the driver */ + stream->rates = rate_mask; + stream->channels_max = max_channels; + stream->formats = formats; + } + /* start the TDA998x audio */ return tda_start_stop(priv, 1); } @@ -193,9 +262,17 @@ static const struct snd_soc_codec_driver soc_codec_tda998x = { static int tda998x_dev_probe(struct platform_device *pdev) { + struct snd_soc_dai_driver *dai_drv; + + /* copy the DAI driver to a writable area */ + dai_drv = devm_kzalloc(&pdev->dev, sizeof(tda998x_dai), GFP_KERNEL); + if (!dai_drv) + return -ENOMEM; + memcpy(dai_drv, tda998x_dai, sizeof(tda998x_dai)); + return snd_soc_register_codec(&pdev->dev, &soc_codec_tda998x, - tda998x_dai, ARRAY_SIZE(tda998x_dai)); + dai_drv, ARRAY_SIZE(tda998x_dai)); } static int tda998x_dev_remove(struct platform_device *pdev)
The supported audio parameters are described in the EDID which is received by the HDMI transmitter from the connected screen. Use these ones to adjust the audio stream parameters. Signed-off-by: Jean-Francois Moine <moinejf@free.fr> --- drivers/gpu/drm/i2c/tda998x_drv.c | 15 ++++++++ include/drm/i2c/tda998x.h | 1 + sound/soc/codecs/tda998x.c | 79 ++++++++++++++++++++++++++++++++++++++- 3 files changed, 94 insertions(+), 1 deletion(-)