| Message ID | 1b15025671d9099863a3091346536e45891e4a26.1391274628.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: > + /* change the snd_soc_pcm_stream values of the driver */ > + stream->rates = rate_mask; > + stream->channels_max = max_channels; > + stream->formats = formats; > + /* 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)); > + The code should be doing this by setting constraints based on the current setup rather than by editing the data structure - the expecation is very much that the data won't change so this prevents surprises with future work on the core.
On Tue, 4 Feb 2014 18:06:25 +0000 Mark Brown <broonie@kernel.org> wrote: > On Mon, Jan 27, 2014 at 09:48:54AM +0100, Jean-Francois Moine wrote: > > > + /* change the snd_soc_pcm_stream values of the driver */ > > + stream->rates = rate_mask; > > + stream->channels_max = max_channels; > > + stream->formats = formats; > > > + /* 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)); > > + > > The code should be doing this by setting constraints based on the > current setup rather than by editing the data structure - the expecation > is very much that the data won't change so this prevents surprises with > future work on the core. As it is done in the soc core, in soc_pcm_open(), the runtime hw_params are initialized after the call to the CODEC startup, and the next CODEC event is hw_params() when the user has already chosen all the parameters. So, in the CODEC, I don't see how I could update the parameters dictated by the EDID otherwise in changing the DAI driver parameters.
On 02/05/2014 10:11 AM, Jean-Francois Moine wrote: > On Tue, 4 Feb 2014 18:06:25 +0000 > Mark Brown <broonie@kernel.org> wrote: > >> On Mon, Jan 27, 2014 at 09:48:54AM +0100, Jean-Francois Moine wrote: >> >>> + /* change the snd_soc_pcm_stream values of the driver */ >>> + stream->rates = rate_mask; >>> + stream->channels_max = max_channels; >>> + stream->formats = formats; >> >>> + /* 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)); >>> + >> >> The code should be doing this by setting constraints based on the >> current setup rather than by editing the data structure - the expecation >> is very much that the data won't change so this prevents surprises with >> future work on the core. > > As it is done in the soc core, in soc_pcm_open(), the runtime hw_params > are initialized after the call to the CODEC startup, and the next CODEC > event is hw_params() when the user has already chosen all the parameters. > > So, in the CODEC, I don't see how I could update the parameters > dictated by the EDID otherwise in changing the DAI driver parameters. > The startup function is the right place. But instead of modifying the DAI use snd_pcm_hw_constraint_mask64(), snd_pcm_hw_constraint_list(), etc. to setup the additional constraints that come from the EDID. Bonus points for making this a generic helper function that takes a runtime and a EDID and then applies the EDID constraints on the runtime. - Lars
On Wed, Feb 05, 2014 at 10:19:22AM +0100, Lars-Peter Clausen wrote: > On 02/05/2014 10:11 AM, Jean-Francois Moine wrote: > >So, in the CODEC, I don't see how I could update the parameters > >dictated by the EDID otherwise in changing the DAI driver parameters. > The startup function is the right place. But instead of modifying > the DAI use snd_pcm_hw_constraint_mask64(), > snd_pcm_hw_constraint_list(), etc. to setup the additional > constraints that come from the EDID. Right. > Bonus points for making this a generic helper function that takes a > runtime and a EDID and then applies the EDID constraints on the > runtime. ...as previously requested. I know there was some discusion of broader moves to factor this stuff out but it'd still be good to keep it separated out even prior to a final non-EDID based solution so it's easier to refactor.
On 02/05/2014 12:18 PM, Mark Brown wrote: > On Wed, Feb 05, 2014 at 10:19:22AM +0100, Lars-Peter Clausen wrote: [..] >> Bonus points for making this a generic helper function that takes a >> runtime and a EDID and then applies the EDID constraints on the >> runtime. > > ...as previously requested. I know there was some discusion of broader > moves to factor this stuff out but it'd still be good to keep it > separated out even prior to a final non-EDID based solution so it's > easier to refactor. I think it will always be EDID (or ELD) based. As I understood it the re-factoring Takashi was talking about is related to how this data is passed from the graphics driver to the audio driver. The way things work right now in HDA land is that the graphics driver reads the EDID from the monitor, converts it to ELD and writes it to a special memory region in the graphics controller. This generates an interrupt in the audio driver and the audio driver reads the ELD from the hardware and sets up the constraints based on that. And I think that the plan is to change this to pass the EDID directly from the graphics driver to the audio driver without taking the detour through the hardware. This is what we'll need for embedded systems anyway. A system that allows to associate a sound driver with a specific HDMI port and status updates for that port, e.g. new EDID or cable connected/disconnected are passed from the graphics driver handling that port to the audio driver. - Lars
On Wed, Feb 05, 2014 at 02:31:09PM +0100, Lars-Peter Clausen wrote: > On 02/05/2014 12:18 PM, Mark Brown wrote: > >...as previously requested. I know there was some discusion of broader > >moves to factor this stuff out but it'd still be good to keep it > >separated out even prior to a final non-EDID based solution so it's > >easier to refactor. > I think it will always be EDID (or ELD) based. As I understood it > the re-factoring Takashi was talking about is related to how this > data is passed from the graphics driver to the audio driver. The way Right, the data of course has to come from there originally, it's about how we pass the data and updates to it around.
On Wed, 05 Feb 2014 10:19:22 +0100 Lars-Peter Clausen <lars@metafoo.de> wrote: > > So, in the CODEC, I don't see how I could update the parameters > > dictated by the EDID otherwise in changing the DAI driver parameters. > > > > The startup function is the right place. But instead of modifying the DAI > use snd_pcm_hw_constraint_mask64(), snd_pcm_hw_constraint_list(), etc. to > setup the additional constraints that come from the EDID. It is more complicated, but it works. Nevertheless, I have 2 problems: - snd_pcm_hw_constraint_list() keeps a pointer to the list, so, it cannot be in the stack. It fix this with static struct and rate array. - snd_pcm_hw_constraint_mask64() is not exported. Is there an other way to set constraints on the formats/sample widths?
On 02/05/2014 07:07 PM, Jean-Francois Moine wrote: > On Wed, 05 Feb 2014 10:19:22 +0100 > Lars-Peter Clausen <lars@metafoo.de> wrote: > >>> So, in the CODEC, I don't see how I could update the parameters >>> dictated by the EDID otherwise in changing the DAI driver parameters. >>> >> >> The startup function is the right place. But instead of modifying the DAI >> use snd_pcm_hw_constraint_mask64(), snd_pcm_hw_constraint_list(), etc. to >> setup the additional constraints that come from the EDID. > > It is more complicated, but it works. Nevertheless, I have 2 problems: > > - snd_pcm_hw_constraint_list() keeps a pointer to the list, so, it > cannot be in the stack. It fix this with static struct and rate array. > Right. If the struct is modified though it should be per device and not global. I think the best way to implement this is to make the array static and specify a mask for the constraint based on the EDID. E.g. static unsigned int hdmi_rates[] = { 32000, 44100, 48000, 88200, 96000, 176400, 192000, }; rate_mask = 0; while (...) { ... rate_mask |= 1 << sad[1]; } rate_constraints->list = hdmi_rates; rate_constraints->count = ARRAY_SIZE(hdmi_rates); rate_constraints->mask = rate_mask; > - snd_pcm_hw_constraint_mask64() is not exported. > Is there an other way to set constraints on the formats/sample widths? I think that's a bug. Both snd_pcm_hw_constraint_mask() and snd_pcm_hw_constraint_mask64() should be exported. Can you send a patch? - Lars
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 68f0b7b..b833fa5 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -47,6 +47,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) @@ -733,6 +735,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) @@ -1187,6 +1197,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 7e4806d..99387ae 100644 --- a/include/drm/i2c/tda998x.h +++ b/include/drm/i2c/tda998x.h @@ -27,6 +27,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 34d7086..0493163 100644 --- a/sound/soc/codecs/tda998x.c +++ b/sound/soc/codecs/tda998x.c @@ -64,10 +64,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); } @@ -182,9 +251,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(-)