sof/hda rework to share more of patch_hdmi.c logic

sof/hda rework to share more of patch_hdmi.c logic

[Kai Vehmanen]

Hi Takashi,

I notice you are doing a lot of cleanups to HDA code. Just FYI I'm looking 
into modifying the SOF Intel backend to use 
snd-hda-codec-hdmi/patch_hdmi.c for HDMI/DP audio support, i.e. to be able 
to share this code between snd-hda-intel and SOF Intel (and not using 
hdac-hdmi).

Let me know if this clashes with something you are already looking into. I 
have a very rough version up and running, but it still needs some work. If 
the general idea seems ok to you, I'll continue to work on a RFC patch and 
send for review.

This will change how HDMI is exposed to user-space with SOF Intel drivers, 
so we need to be extra careful how this is introduced. But this really 
seems to be the best way to go to avoid the duplicated maintenance work 
with two drivers that we now have.

PS Tracked in SOF github as 
   https://github.com/thesofproject/linux/issues/1123

Br, Kai

Re: sof/hda rework to share more of patch_hdmi.c logic

[Takashi Iwai]

On Thu, 15 Aug 2019 16:28:02 +0200,
Kai Vehmanen wrote:
> 
> Hi Takashi,
> 
> I notice you are doing a lot of cleanups to HDA code. Just FYI I'm looking 
> into modifying the SOF Intel backend to use 
> snd-hda-codec-hdmi/patch_hdmi.c for HDMI/DP audio support, i.e. to be able 
> to share this code between snd-hda-intel and SOF Intel (and not using 
> hdac-hdmi).
> 
> Let me know if this clashes with something you are already looking into. I 
> have a very rough version up and running, but it still needs some work. If 
> the general idea seems ok to you, I'll continue to work on a RFC patch and 
> send for review.

That sounds like a good idea.

Currently I've almost done for HD-audio rework for 5.4, so feel free
to go ahead.

> This will change how HDMI is exposed to user-space with SOF Intel drivers, 
> so we need to be extra careful how this is introduced. But this really 
> seems to be the best way to go to avoid the duplicated maintenance work 
> with two drivers that we now have.

Agreed.  I guess the biggest difference is the handling of the
DP-MST.  The legacy HD-audio HDMI driver takes a different approach
for DP-MST, namely, it chooses dynamically the pin that is connected
with a monitor.  It's for keeping the compatibility (more or less)
with old behavior; the program just needs to open the device that
corresponds to the notification via jack ctl without fiddling with
other extra routing.

> PS Tracked in SOF github as 
>    https://github.com/thesofproject/linux/issues/1123

OK, will watch out.


thanks,

Takashi

Re: sof/hda rework to share more of patch_hdmi.c logic

[Kai Vehmanen]

Hi,

On Thu, 15 Aug 2019, Takashi Iwai wrote:
> Kai Vehmanen wrote:
>> into modifying the SOF Intel backend to use 
>> snd-hda-codec-hdmi/patch_hdmi.c for HDMI/DP audio support, i.e. to be able 
>> to share this code between snd-hda-intel and SOF Intel (and not using 
>> hdac-hdmi).
[..]
>> This will change how HDMI is exposed to user-space with SOF Intel 
>> drivers, so we need to be extra careful how this is introduced. But 

> Agreed.  I guess the biggest difference is the handling of the 
> DP-MST.  The legacy HD-audio HDMI driver takes a different approach
> for DP-MST, namely, it chooses dynamically the pin that is connected
> with a monitor.  It's for keeping the compatibility (more or less)
> with old behavior; the program just needs to open the device that
> corresponds to the notification via jack ctl without fiddling with
> other extra routing.

indeed. I've now got to a point where I have the key functionality
in place. And after some reworks, the changes to patch_hdmi.c 
are pretty minimal, which is very nice (I started with a much more 
evasive patch).

But, but. The DP-MST handling is indeed iffy. I tried a few approaches, 
but it is hard to reconcile concept of "backup PCMs" of patch_hdmi.c with 
concepts of ASoC and ALSA topology, where the PCM and DAIs are supposed to 
be defined in the topology file. This gets worse with SOF (and any similar 
usage) which allow you to have arbitrary DSP processing between a PCM and 
the HDMI/DP DAIs. So this seems like a dead-end.

What I ended up doing was to make a new mode to patch_hdmi.c that limits 
PCM count to actual converter count (and this is aligned with topology), 
and still supporting DP-MST by always mapping monitors to a free PCM. I've 
tested some complex DP-MST scenarios and this seems to work pretty well. 
The jack detection will still be able to tell which of the PCMs have a 
monitor detected.

I wonder if this would be an acceptable approach, given 
the reuse benefits we get. Downsides:
- assignment of monitors to PCMs will depend on ELD update ordering
- in SOF we need to align PCM numbering scheme in all topologies 
  we convert over to patch_hdmi.c

I did not yet figure out how to toggle the new DP-MST mode in patch_hdmi.c 
in a nice way, so didn't sent patches to list yet. I'll do that early next 
week. If you want a sneak preview, I uploaded the series at:

https://github.com/thesofproject/linux/pull/1155

Br, Kai

Re: sof/hda rework to share more of patch_hdmi.c logic

[Takashi Iwai]

On Fri, 23 Aug 2019 16:29:40 +0200,
Kai Vehmanen wrote:
> 
> Hi,
> 
> On Thu, 15 Aug 2019, Takashi Iwai wrote:
> > Kai Vehmanen wrote:
> >> into modifying the SOF Intel backend to use 
> >> snd-hda-codec-hdmi/patch_hdmi.c for HDMI/DP audio support, i.e. to be able 
> >> to share this code between snd-hda-intel and SOF Intel (and not using 
> >> hdac-hdmi).
> [..]
> >> This will change how HDMI is exposed to user-space with SOF Intel 
> >> drivers, so we need to be extra careful how this is introduced. But 
> 
> > Agreed.  I guess the biggest difference is the handling of the 
> > DP-MST.  The legacy HD-audio HDMI driver takes a different approach
> > for DP-MST, namely, it chooses dynamically the pin that is connected
> > with a monitor.  It's for keeping the compatibility (more or less)
> > with old behavior; the program just needs to open the device that
> > corresponds to the notification via jack ctl without fiddling with
> > other extra routing.
> 
> indeed. I've now got to a point where I have the key functionality
> in place. And after some reworks, the changes to patch_hdmi.c 
> are pretty minimal, which is very nice (I started with a much more 
> evasive patch).
> 
> But, but. The DP-MST handling is indeed iffy. I tried a few approaches, 
> but it is hard to reconcile concept of "backup PCMs" of patch_hdmi.c with 
> concepts of ASoC and ALSA topology, where the PCM and DAIs are supposed to 
> be defined in the topology file. This gets worse with SOF (and any similar 
> usage) which allow you to have arbitrary DSP processing between a PCM and 
> the HDMI/DP DAIs. So this seems like a dead-end.
> 
> What I ended up doing was to make a new mode to patch_hdmi.c that limits 
> PCM count to actual converter count (and this is aligned with topology), 
> and still supporting DP-MST by always mapping monitors to a free PCM. I've 
> tested some complex DP-MST scenarios and this seems to work pretty well. 
> The jack detection will still be able to tell which of the PCMs have a 
> monitor detected.
> 
> I wonder if this would be an acceptable approach, given 
> the reuse benefits we get. Downsides:
> - assignment of monitors to PCMs will depend on ELD update ordering

This is true in anyway for the legacy HD-audio, too.

> - in SOF we need to align PCM numbering scheme in all topologies 
>   we convert over to patch_hdmi.c

I don't think this can be a problem.

Practically seen, the number of PCMs could be just 3 on Skylake and
co, even with DP MST, because i915 can drive only up to 3 monitors.
The driver *should* receive the unplug event before the plug event to
a new monitor, so hda_detach_hda_pcm() gets called before
hda_attach_hda_pcm().

The current patch_hdmi.c implementation is based on the theoretical
possibility, and limitation to the reduced PCM streams would work, I
suppose.


thanks,

Takashi

> 
> I did not yet figure out how to toggle the new DP-MST mode in patch_hdmi.c 
> in a nice way, so didn't sent patches to list yet. I'll do that early next 
> week. If you want a sneak preview, I uploaded the series at:
> 
> https://github.com/thesofproject/linux/pull/1155
> 
> Br, Kai
> 

Re: sof/hda rework to share more of patch_hdmi.c logic

[Takashi Iwai]

On Fri, 23 Aug 2019 16:55:09 +0200,
Takashi Iwai wrote:
> 
> On Fri, 23 Aug 2019 16:29:40 +0200,
> Kai Vehmanen wrote:
> > 
> > Hi,
> > 
> > On Thu, 15 Aug 2019, Takashi Iwai wrote:
> > > Kai Vehmanen wrote:
> > >> into modifying the SOF Intel backend to use 
> > >> snd-hda-codec-hdmi/patch_hdmi.c for HDMI/DP audio support, i.e. to be able 
> > >> to share this code between snd-hda-intel and SOF Intel (and not using 
> > >> hdac-hdmi).
> > [..]
> > >> This will change how HDMI is exposed to user-space with SOF Intel 
> > >> drivers, so we need to be extra careful how this is introduced. But 
> > 
> > > Agreed.  I guess the biggest difference is the handling of the 
> > > DP-MST.  The legacy HD-audio HDMI driver takes a different approach
> > > for DP-MST, namely, it chooses dynamically the pin that is connected
> > > with a monitor.  It's for keeping the compatibility (more or less)
> > > with old behavior; the program just needs to open the device that
> > > corresponds to the notification via jack ctl without fiddling with
> > > other extra routing.
> > 
> > indeed. I've now got to a point where I have the key functionality
> > in place. And after some reworks, the changes to patch_hdmi.c 
> > are pretty minimal, which is very nice (I started with a much more 
> > evasive patch).
> > 
> > But, but. The DP-MST handling is indeed iffy. I tried a few approaches, 
> > but it is hard to reconcile concept of "backup PCMs" of patch_hdmi.c with 
> > concepts of ASoC and ALSA topology, where the PCM and DAIs are supposed to 
> > be defined in the topology file. This gets worse with SOF (and any similar 
> > usage) which allow you to have arbitrary DSP processing between a PCM and 
> > the HDMI/DP DAIs. So this seems like a dead-end.
> > 
> > What I ended up doing was to make a new mode to patch_hdmi.c that limits 
> > PCM count to actual converter count (and this is aligned with topology), 
> > and still supporting DP-MST by always mapping monitors to a free PCM. I've 
> > tested some complex DP-MST scenarios and this seems to work pretty well. 
> > The jack detection will still be able to tell which of the PCMs have a 
> > monitor detected.
> > 
> > I wonder if this would be an acceptable approach, given 
> > the reuse benefits we get. Downsides:
> > - assignment of monitors to PCMs will depend on ELD update ordering
> 
> This is true in anyway for the legacy HD-audio, too.
> 
> > - in SOF we need to align PCM numbering scheme in all topologies 
> >   we convert over to patch_hdmi.c
> 
> I don't think this can be a problem.
> 
> Practically seen, the number of PCMs could be just 3 on Skylake and
> co, even with DP MST, because i915 can drive only up to 3 monitors.
> The driver *should* receive the unplug event before the plug event to
> a new monitor, so hda_detach_hda_pcm() gets called before
> hda_attach_hda_pcm().
> 
> The current patch_hdmi.c implementation is based on the theoretical
> possibility, and limitation to the reduced PCM streams would work, I
> suppose.

For more correctness: the patch_hdmi.c implementation actually would
work even without the backup PCM streams.  The backup PCM streams are
there for assuring the compatible behavior for old applications that
access to the fixed PCM device (e.g. hw:1,1).  But, it's hardly
possible to get more than three audio-monitors active in the real
scenario, we've almost never seen this necessity.

The actual behavior can be found in the description of commit
a76056f2e57e:

    When monitor is connected, find a proper PCM for the monitor.
    When monitor is disconnected, unbind the PCM from the pin.
    
    The binding policy (use Intel platform as example) is:
    1. Try to use the legacy pin-pcm mapping for the device entry 0
       of the pin.
    2. If step 1 fails, try to bind pin to the backup PCMs. For example,
       on Intel platform, if DP MST is enabled, 5 PCMs will be created.
       PCM 3, PCM 7, PCM 8 are supposed to be used by device entry 0 of
       pin 5, pin 6 and pin 7. PCM 9 and PCM 10 are the backup PCMs.
    3. If step 2 fails, try to find any PCM to bind to the pin.
    
Removing the backup streams means the removal of step 2, but the
driver will keep working in step 3.


thanks,

Takashi

Re: sof/hda rework to share more of patch_hdmi.c logic

[Kai Vehmanen]

Hey,


On Fri, 23 Aug 2019, Takashi Iwai wrote:
> On Fri, 23 Aug 2019, Takashi Iwai wrote:
> > The current patch_hdmi.c implementation is based on the theoretical
> > possibility, and limitation to the reduced PCM streams would work, I
> > suppose.
[...]
> access to the fixed PCM device (e.g. hw:1,1).  But, it's hardly
> possible to get more than three audio-monitors active in the real
> scenario, we've almost never seen this necessity.

ack. This does feel very unlikely to be a problem. And one could implement 
a wait list -> delay PCM attach if all audio converters are taken when the 
new monitor ELD update happens. Upon next disconnect of an already 
attached audio enabled monitor, one could check the wait list and hook up 
monitors to the freed converters. But yeah, at least on i915 I don't see 
the need for this.

This does raise the question whether we should change the behaviour 
for the non-DSP HD driver as well...? Tempting, but the risk for breaking 
apps may be too high.

> The actual behavior can be found in the description of commit
> a76056f2e57e:
> 
>     When monitor is connected, find a proper PCM for the monitor.
>     When monitor is disconnected, unbind the PCM from the pin.
>     
>     The binding policy (use Intel platform as example) is:
>     1. Try to use the legacy pin-pcm mapping for the device entry 0
>        of the pin.
>     2. If step 1 fails, try to bind pin to the backup PCMs. For example,
>        on Intel platform, if DP MST is enabled, 5 PCMs will be created.
>        PCM 3, PCM 7, PCM 8 are supposed to be used by device entry 0 of
>        pin 5, pin 6 and pin 7. PCM 9 and PCM 10 are the backup PCMs.
>     3. If step 2 fails, try to find any PCM to bind to the pin.
>     
> Removing the backup streams means the removal of step 2, but the
> driver will keep working in step 3.

Yes, I didn't have to actually change this code at all. I just limit the 
PCM count to number of converters, and the above code works without 
modifications (and skips (2)). Only problem left is how to change to 
toggle the different rules for calculing pcm count. Unless we change the 
behaviour for non-DSP HD driver as well, there needs to be some dynamic 
configuration of patch_hdmi. Kconfig/module-param won't do, I assume 
distros want to enable both options with same kernel.

Thanks for the quick response!

Br, Kai

Re: sof/hda rework to share more of patch_hdmi.c logic

[Takashi Iwai]

On Fri, 23 Aug 2019 17:55:05 +0200,
Kai Vehmanen wrote:
> 
> Hey,
> 
> 
> On Fri, 23 Aug 2019, Takashi Iwai wrote:
> > On Fri, 23 Aug 2019, Takashi Iwai wrote:
> > > The current patch_hdmi.c implementation is based on the theoretical
> > > possibility, and limitation to the reduced PCM streams would work, I
> > > suppose.
> [...]
> > access to the fixed PCM device (e.g. hw:1,1).  But, it's hardly
> > possible to get more than three audio-monitors active in the real
> > scenario, we've almost never seen this necessity.
> 
> ack. This does feel very unlikely to be a problem. And one could implement 
> a wait list -> delay PCM attach if all audio converters are taken when the 
> new monitor ELD update happens. Upon next disconnect of an already 
> attached audio enabled monitor, one could check the wait list and hook up 
> monitors to the freed converters. But yeah, at least on i915 I don't see 
> the need for this.
> 
> This does raise the question whether we should change the behaviour 
> for the non-DSP HD driver as well...? Tempting, but the risk for breaking 
> apps may be too high.

It's a good question.  On one hand, it'd be good to clean up a bit,
but we never know the use of backup PCM ever happened.  No news is a
good news, after all.

> > The actual behavior can be found in the description of commit
> > a76056f2e57e:
> > 
> >     When monitor is connected, find a proper PCM for the monitor.
> >     When monitor is disconnected, unbind the PCM from the pin.
> >     
> >     The binding policy (use Intel platform as example) is:
> >     1. Try to use the legacy pin-pcm mapping for the device entry 0
> >        of the pin.
> >     2. If step 1 fails, try to bind pin to the backup PCMs. For example,
> >        on Intel platform, if DP MST is enabled, 5 PCMs will be created.
> >        PCM 3, PCM 7, PCM 8 are supposed to be used by device entry 0 of
> >        pin 5, pin 6 and pin 7. PCM 9 and PCM 10 are the backup PCMs.
> >     3. If step 2 fails, try to find any PCM to bind to the pin.
> >     
> > Removing the backup streams means the removal of step 2, but the
> > driver will keep working in step 3.
> 
> Yes, I didn't have to actually change this code at all. I just limit the 
> PCM count to number of converters, and the above code works without 
> modifications (and skips (2)). Only problem left is how to change to 
> toggle the different rules for calculing pcm count. Unless we change the 
> behaviour for non-DSP HD driver as well, there needs to be some dynamic 
> configuration of patch_hdmi. Kconfig/module-param won't do, I assume 
> distros want to enable both options with same kernel.

We can introduce some new flag in struct hda_codec indicating this
restriction, and set it in hdac_hda_codec_probe() beforehand.

Or, we may introduce a new Kconfig and disable statically.  The new
Kconfig may have "depends on SOF=n" or whatever to be sure.
Although we have no 100% guarantee as mentioned in the above, this
shouldn't be a big problem.  Worth for try.

Honestly speaking, I'm not entirely sure which is a better way to go.
Both would be fairly small changes.

You can try them out and see which looks saner.  I really don't mind
either way.


thanks,

Takashi