mbox series

[00/11] drm/vc4: hdmi: Enable Channel Mapping, IEC958, HBR Passthrough using hdmi-codec

Message ID 20210507140334.204865-1-maxime@cerno.tech (mailing list archive)
Headers show
Series drm/vc4: hdmi: Enable Channel Mapping, IEC958, HBR Passthrough using hdmi-codec | expand

Message

Maxime Ripard May 7, 2021, 2:03 p.m. UTC 
Hi,

hdmi-codec allows to have a lot of HDMI-audio related infrastructure in place,
it's missing a few controls to be able to provide HBR passthrough. This series
adds more infrastructure for the drivers, and leverages it in the vc4 HDMI
controller driver.

One thing that felt a bit weird is that even though
https://www.kernel.org/doc/html/latest/sound/kernel-api/writing-an-alsa-driver.html#iec958-s-pdif
mentions that the iec958 mask control should be a mixer control and the
default control should be a PCM one, it feels a bit weird to have two different
control type for two controls so similar, and other drivers are pretty
inconsistent with this. Should we update the documentation?

Thanks!
Maxime

Dom Cobley (5):
  drm/vc4: hdmi: Set HD_CTL_WHOLSMP and HD_CTL_CHALIGN_SET
  drm/vc4: hdmi: Set HDMI_MAI_FMT
  drm/vc4: hdmi: Set VC4_HDMI_MAI_CONFIG_FORMAT_REVERSE
  drm/vc4: hdmi: Remove firmware logic for MAI threshold setting
  ARM: dts: bcm2711: Tune DMA parameters for HDMI audio

Maxime Ripard (6):
  snd: iec958: split status creation and fill
  ASoC: hdmi-codec: Rework to support more controls
  ASoC: hdmi-codec: Add iec958 controls
  ASoC: hdmi-codec: Add a prepare hook
  drm/vc4: hdmi: Register HDMI codec
  drm/vc4: hdmi: Remove redundant variables

 arch/arm/boot/dts/bcm2711.dtsi |   4 +-
 drivers/gpu/drm/vc4/Kconfig    |   1 +
 drivers/gpu/drm/vc4/vc4_hdmi.c | 322 ++++++++++++++-------------------
 drivers/gpu/drm/vc4/vc4_hdmi.h |   5 +-
 drivers/gpu/drm/vc4/vc4_regs.h |  30 +++
 include/sound/hdmi-codec.h     |  12 +-
 include/sound/pcm_iec958.h     |   8 +
 sound/core/pcm_iec958.c        | 131 +++++++++-----
 sound/soc/codecs/hdmi-codec.c  | 219 +++++++++++++++++-----
 9 files changed, 456 insertions(+), 276 deletions(-)

Comments

Maxime Ripard May 24, 2021, 1:39 p.m. UTC | #1 
Hi,

On Fri, May 07, 2021 at 04:03:23PM +0200, Maxime Ripard wrote:
> Hi,
> 
> hdmi-codec allows to have a lot of HDMI-audio related infrastructure in place,
> it's missing a few controls to be able to provide HBR passthrough. This series
> adds more infrastructure for the drivers, and leverages it in the vc4 HDMI
> controller driver.
> 
> One thing that felt a bit weird is that even though
> https://www.kernel.org/doc/html/latest/sound/kernel-api/writing-an-alsa-driver.html#iec958-s-pdif
> mentions that the iec958 mask control should be a mixer control and the
> default control should be a PCM one, it feels a bit weird to have two different
> control type for two controls so similar, and other drivers are pretty
> inconsistent with this. Should we update the documentation?

Any comments on this series?
Thanks!
Maxime
Takashi Iwai May 25, 2021, 8:35 a.m. UTC | #2 
On Mon, 24 May 2021 15:39:04 +0200,
Maxime Ripard wrote:
> 
> Hi,
> 
> On Fri, May 07, 2021 at 04:03:23PM +0200, Maxime Ripard wrote:
> > Hi,
> > 
> > hdmi-codec allows to have a lot of HDMI-audio related infrastructure in place,
> > it's missing a few controls to be able to provide HBR passthrough. This series
> > adds more infrastructure for the drivers, and leverages it in the vc4 HDMI
> > controller driver.
> > 
> > One thing that felt a bit weird is that even though
> > https://www.kernel.org/doc/html/latest/sound/kernel-api/writing-an-alsa-driver.html#iec958-s-pdif
> > mentions that the iec958 mask control should be a mixer control and the
> > default control should be a PCM one, it feels a bit weird to have two different
> > control type for two controls so similar, and other drivers are pretty
> > inconsistent with this. Should we update the documentation?
> 
> Any comments on this series?

A patch for updating the documentation is welcome.
Currently, as de facto standard, we allow both MIXER and PCM ifaces
for all IEC958-related controls, and it's unlikely that we would
change that in future.


thanks,

Takashi
Maxime Ripard May 25, 2021, 9:23 a.m. UTC | #3 
Hi Takashi,

On Tue, May 25, 2021 at 10:35:14AM +0200, Takashi Iwai wrote:
> On Mon, 24 May 2021 15:39:04 +0200,
> Maxime Ripard wrote:
> > 
> > Hi,
> > 
> > On Fri, May 07, 2021 at 04:03:23PM +0200, Maxime Ripard wrote:
> > > Hi,
> > > 
> > > hdmi-codec allows to have a lot of HDMI-audio related infrastructure in place,
> > > it's missing a few controls to be able to provide HBR passthrough. This series
> > > adds more infrastructure for the drivers, and leverages it in the vc4 HDMI
> > > controller driver.
> > > 
> > > One thing that felt a bit weird is that even though
> > > https://www.kernel.org/doc/html/latest/sound/kernel-api/writing-an-alsa-driver.html#iec958-s-pdif
> > > mentions that the iec958 mask control should be a mixer control and the
> > > default control should be a PCM one, it feels a bit weird to have two different
> > > control type for two controls so similar, and other drivers are pretty
> > > inconsistent with this. Should we update the documentation?
> > 
> > Any comments on this series?
> 
> A patch for updating the documentation is welcome.
> Currently, as de facto standard, we allow both MIXER and PCM ifaces
> for all IEC958-related controls, and it's unlikely that we would
> change that in future.

Ok, I'll write a patch for the documentation make it clearer then :)

Do we want to make sure that all the iec958 controls are on the same
iface, or is it also left to the driver (or should we just leave the
existing drivers as is but encourage a consistent use in the future)?

Maxime
Takashi Iwai May 25, 2021, 9:27 a.m. UTC | #4 
On Tue, 25 May 2021 11:23:53 +0200,
Maxime Ripard wrote:
> 
> Hi Takashi,
> 
> On Tue, May 25, 2021 at 10:35:14AM +0200, Takashi Iwai wrote:
> > On Mon, 24 May 2021 15:39:04 +0200,
> > Maxime Ripard wrote:
> > > 
> > > Hi,
> > > 
> > > On Fri, May 07, 2021 at 04:03:23PM +0200, Maxime Ripard wrote:
> > > > Hi,
> > > > 
> > > > hdmi-codec allows to have a lot of HDMI-audio related infrastructure in place,
> > > > it's missing a few controls to be able to provide HBR passthrough. This series
> > > > adds more infrastructure for the drivers, and leverages it in the vc4 HDMI
> > > > controller driver.
> > > > 
> > > > One thing that felt a bit weird is that even though
> > > > https://www.kernel.org/doc/html/latest/sound/kernel-api/writing-an-alsa-driver.html#iec958-s-pdif
> > > > mentions that the iec958 mask control should be a mixer control and the
> > > > default control should be a PCM one, it feels a bit weird to have two different
> > > > control type for two controls so similar, and other drivers are pretty
> > > > inconsistent with this. Should we update the documentation?
> > > 
> > > Any comments on this series?
> > 
> > A patch for updating the documentation is welcome.
> > Currently, as de facto standard, we allow both MIXER and PCM ifaces
> > for all IEC958-related controls, and it's unlikely that we would
> > change that in future.
> 
> Ok, I'll write a patch for the documentation make it clearer then :)
> 
> Do we want to make sure that all the iec958 controls are on the same
> iface, or is it also left to the driver (or should we just leave the
> existing drivers as is but encourage a consistent use in the future)?

I'd leave the existing drivers as-is.  Changing the iface is basically
an incompatible change, and although most of applications and alsa-lib
should look at both ifaces, there can be any surprise by that change.


thanks,

Takashi