[PATCH] ALSA: hda - hdmi: Fix channel map switch not taking effect

[PATCH] ALSA: hda - hdmi: Fix channel map switch not taking effect

[Anssi Hannula]

Currently hdmi_setup_audio_infoframe() reprograms the HDA channel
mapping only when the infoframe is not up-to-date or the non-PCM flag
has changed.

However, when just the channel map has been changed, the infoframe may
still be up-to-date and non-PCM flag may not have changed, so the new
channel map is not actually programmed into the HDA codec.

Notably, this failing case is also always triggered when the device is
already in a prepared state and a new channel map is configured while
changing only the channel positions (for example, plain
"speaker-test -c2 -m FR,FL").

Fix that by keeping track of the last programmed channel map and making
hdmi_setup_audio_infoframe() always update the hardware mapping if the
channel map has changed.

Signed-off-by: Anssi Hannula <anssi.hannula@iki.fi>
Cc: <stable@vger.kernel.org>
---

BTW, is there some mechanism preventing hdmi_chmap_ctl_put() to be called
at the same time with generic_hdmi_playback_pcm_prepare() that I didn't
see with a quick look?
Both of them call hdmi_setup_audio_infoframe() which is not thread-safe.

 sound/pci/hda/patch_hdmi.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 7ea0245..09f38a1 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -74,6 +74,8 @@ struct hdmi_spec_per_pin {
 	bool non_pcm;
 	bool chmap_set;		/* channel-map override by ALSA API? */
 	unsigned char chmap[8]; /* ALSA API channel-map */
+	bool last_chmap_set;	/* channel-map override programmed? */
+	unsigned char last_chmap[8]; /* last programmed channel-map */
 	char pcm_name[8];	/* filled in build_pcm callbacks */
 };
 
@@ -955,15 +957,21 @@ static void hdmi_setup_audio_infoframe(struct hda_codec *codec,
 					    ai.bytes, sizeof(ai));
 		hdmi_start_infoframe_trans(codec, pin_nid);
 	} else {
-		/* For non-pcm audio switch, setup new channel mapping
-		 * accordingly */
-		if (per_pin->non_pcm != non_pcm)
+		/* For non-pcm audio switch or channel-map switch,
+		 * setup new channel mapping accordingly */
+		if (per_pin->non_pcm != non_pcm
+		    || per_pin->chmap_set != per_pin->last_chmap_set
+		    || (per_pin->chmap_set && memcmp(per_pin->chmap, per_pin->last_chmap,
+						     ARRAY_SIZE(per_pin->chmap)) != 0))
 			hdmi_setup_channel_mapping(codec, pin_nid, non_pcm, ca,
 						   channels, per_pin->chmap,
 						   per_pin->chmap_set);
 	}
 
 	per_pin->non_pcm = non_pcm;
+	per_pin->last_chmap_set = per_pin->chmap_set;
+	if (per_pin->chmap_set)
+		memcpy(per_pin->last_chmap, per_pin->chmap, ARRAY_SIZE(per_pin->chmap));
 }
 
 
-- 
1.8.1.5

Re: [PATCH] ALSA: hda - hdmi: Fix channel map switch not taking effect

[Takashi Iwai]

At Wed,  2 Oct 2013 21:13:32 +0300,
Anssi Hannula wrote:
> 
> Currently hdmi_setup_audio_infoframe() reprograms the HDA channel
> mapping only when the infoframe is not up-to-date or the non-PCM flag
> has changed.
> 
> However, when just the channel map has been changed, the infoframe may
> still be up-to-date and non-PCM flag may not have changed, so the new
> channel map is not actually programmed into the HDA codec.
> 
> Notably, this failing case is also always triggered when the device is
> already in a prepared state and a new channel map is configured while
> changing only the channel positions (for example, plain
> "speaker-test -c2 -m FR,FL").
> 
> Fix that by keeping track of the last programmed channel map and making
> hdmi_setup_audio_infoframe() always update the hardware mapping if the
> channel map has changed.

Hmm, maybe it's easier (and safer) to remove the check and always call
hdmi_setup_channel_mapping()?  Of course, the drawback is the
unnecessary verbs, but it's not too much.

> Signed-off-by: Anssi Hannula <anssi.hannula@iki.fi>
> Cc: <stable@vger.kernel.org>
> ---
> 
> BTW, is there some mechanism preventing hdmi_chmap_ctl_put() to be called
> at the same time with generic_hdmi_playback_pcm_prepare() that I didn't
> see with a quick look?
> Both of them call hdmi_setup_audio_infoframe() which is not thread-safe.

Yeah, we need to introduce a mutex there.


thanks,

Takashi

>  sound/pci/hda/patch_hdmi.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index 7ea0245..09f38a1 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -74,6 +74,8 @@ struct hdmi_spec_per_pin {
>  	bool non_pcm;
>  	bool chmap_set;		/* channel-map override by ALSA API? */
>  	unsigned char chmap[8]; /* ALSA API channel-map */
> +	bool last_chmap_set;	/* channel-map override programmed? */
> +	unsigned char last_chmap[8]; /* last programmed channel-map */
>  	char pcm_name[8];	/* filled in build_pcm callbacks */
>  };
>  
> @@ -955,15 +957,21 @@ static void hdmi_setup_audio_infoframe(struct hda_codec *codec,
>  					    ai.bytes, sizeof(ai));
>  		hdmi_start_infoframe_trans(codec, pin_nid);
>  	} else {
> -		/* For non-pcm audio switch, setup new channel mapping
> -		 * accordingly */
> -		if (per_pin->non_pcm != non_pcm)
> +		/* For non-pcm audio switch or channel-map switch,
> +		 * setup new channel mapping accordingly */
> +		if (per_pin->non_pcm != non_pcm
> +		    || per_pin->chmap_set != per_pin->last_chmap_set
> +		    || (per_pin->chmap_set && memcmp(per_pin->chmap, per_pin->last_chmap,
> +						     ARRAY_SIZE(per_pin->chmap)) != 0))
>  			hdmi_setup_channel_mapping(codec, pin_nid, non_pcm, ca,
>  						   channels, per_pin->chmap,
>  						   per_pin->chmap_set);
>  	}
>  
>  	per_pin->non_pcm = non_pcm;
> +	per_pin->last_chmap_set = per_pin->chmap_set;
> +	if (per_pin->chmap_set)
> +		memcpy(per_pin->last_chmap, per_pin->chmap, ARRAY_SIZE(per_pin->chmap));
>  }
>  
>  
> -- 
> 1.8.1.5
> 

Re: [PATCH] ALSA: hda - hdmi: Fix channel map switch not taking effect

[Anssi Hannula]

Takashi Iwai kirjoitti 2013-10-07 10:48:
> At Wed,  2 Oct 2013 21:13:32 +0300,
> Anssi Hannula wrote:
> 
> Currently hdmi_setup_audio_infoframe() reprograms the HDA channel
> mapping only when the infoframe is not up-to-date or the non-PCM flag
> has changed.
> 
> However, when just the channel map has been changed, the infoframe may
> still be up-to-date and non-PCM flag may not have changed, so the new
> channel map is not actually programmed into the HDA codec.
> 
> Notably, this failing case is also always triggered when the device is
> already in a prepared state and a new channel map is configured while
> changing only the channel positions (for example, plain
> "speaker-test -c2 -m FR,FL").
> 
> Fix that by keeping track of the last programmed channel map and making
> hdmi_setup_audio_infoframe() always update the hardware mapping if the
> channel map has changed.
> 
> Hmm, maybe it's easier (and safer) to remove the check and always call
> hdmi_setup_channel_mapping()?  Of course, the drawback is the
> unnecessary verbs, but it's not too much.

Easier, yes. I don't really have that good of an idea how big of a delay
do 8 write verbs cause, so I'll rely on your judgment :)

I guess I could replace the writes with read-and-write-if-differs cycles
when I add the set_slot ops for ATI/AMD, since it seems to be used for
other similar cases (like channel count)?

> Signed-off-by: Anssi Hannula <anssi.hannula@iki.fi>
> Cc: <stable@vger.kernel.org>
> ---
> 
> BTW, is there some mechanism preventing hdmi_chmap_ctl_put() to be 
> called
> at the same time with generic_hdmi_playback_pcm_prepare() that I didn't
> see with a quick look?
> Both of them call hdmi_setup_audio_infoframe() which is not 
> thread-safe.
> 
> Yeah, we need to introduce a mutex there.

I guess hdmi_present_sense() is the same, as it can also call
hdmi_setup_audio_infoframe()?

BTW, shouldn't hdmi_present_sense() call hdmi_setup_audio_infoframe() 
always
when eld_changed is true? hdmi_setup_audio_infoframe() does not do its
stuff if !monitor_present, so if one starts playback before plugging in,
the channel mappings etc will never be set even when plug-in happens
(from what I can see, that is -  did not test yet).


> thanks,
> 
> Takashi
> 
> sound/pci/hda/patch_hdmi.c | 14 +++++++++++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index 7ea0245..09f38a1 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -74,6 +74,8 @@ struct hdmi_spec_per_pin {
> 	bool non_pcm;
> 	bool chmap_set;		/* channel-map override by ALSA API? */
> 	unsigned char chmap[8]; /* ALSA API channel-map */
> +	bool last_chmap_set;	/* channel-map override programmed? */
> +	unsigned char last_chmap[8]; /* last programmed channel-map */
> 	char pcm_name[8];	/* filled in build_pcm callbacks */
> };
> 
> @@ -955,15 +957,21 @@ static void hdmi_setup_audio_infoframe(struct 
> hda_codec *codec,
> 					    ai.bytes, sizeof(ai));
> 		hdmi_start_infoframe_trans(codec, pin_nid);
> 	} else {
> -		/* For non-pcm audio switch, setup new channel mapping
> -		 * accordingly */
> -		if (per_pin->non_pcm != non_pcm)
> +		/* For non-pcm audio switch or channel-map switch,
> +		 * setup new channel mapping accordingly */
> +		if (per_pin->non_pcm != non_pcm
> +		    || per_pin->chmap_set != per_pin->last_chmap_set
> +		    || (per_pin->chmap_set && memcmp(per_pin->chmap, 
> per_pin->last_chmap,
> +						     ARRAY_SIZE(per_pin->chmap)) != 0))
> 			hdmi_setup_channel_mapping(codec, pin_nid, non_pcm, ca,
> 						   channels, per_pin->chmap,
> 						   per_pin->chmap_set);
> 	}
> 
> 	per_pin->non_pcm = non_pcm;
> +	per_pin->last_chmap_set = per_pin->chmap_set;
> +	if (per_pin->chmap_set)
> +		memcpy(per_pin->last_chmap, per_pin->chmap, 
> ARRAY_SIZE(per_pin->chmap));
> }
> 
> 
> --
> 1.8.1.5
> 

Re: [PATCH] ALSA: hda - hdmi: Fix channel map switch not taking effect

[Takashi Iwai]

At Mon, 07 Oct 2013 13:31:17 +0300,
Anssi Hannula wrote:
> 
> Takashi Iwai kirjoitti 2013-10-07 10:48:
> > At Wed,  2 Oct 2013 21:13:32 +0300,
> > Anssi Hannula wrote:
> > 
> > Currently hdmi_setup_audio_infoframe() reprograms the HDA channel
> > mapping only when the infoframe is not up-to-date or the non-PCM flag
> > has changed.
> > 
> > However, when just the channel map has been changed, the infoframe may
> > still be up-to-date and non-PCM flag may not have changed, so the new
> > channel map is not actually programmed into the HDA codec.
> > 
> > Notably, this failing case is also always triggered when the device is
> > already in a prepared state and a new channel map is configured while
> > changing only the channel positions (for example, plain
> > "speaker-test -c2 -m FR,FL").
> > 
> > Fix that by keeping track of the last programmed channel map and making
> > hdmi_setup_audio_infoframe() always update the hardware mapping if the
> > channel map has changed.
> > 
> > Hmm, maybe it's easier (and safer) to remove the check and always call
> > hdmi_setup_channel_mapping()?  Of course, the drawback is the
> > unnecessary verbs, but it's not too much.
> 
> Easier, yes. I don't really have that good of an idea how big of a delay
> do 8 write verbs cause, so I'll rely on your judgment :)
> 
> I guess I could replace the writes with read-and-write-if-differs cycles
> when I add the set_slot ops for ATI/AMD, since it seems to be used for
> other similar cases (like channel count)?

Well, this would result in more cost, as read requires a sync.

If any, we can use the cached write/update.  But this assumes that the
hardware never clears the value by some operation internally.


> > Signed-off-by: Anssi Hannula <anssi.hannula@iki.fi>
> > Cc: <stable@vger.kernel.org>
> > ---
> > 
> > BTW, is there some mechanism preventing hdmi_chmap_ctl_put() to be 
> > called
> > at the same time with generic_hdmi_playback_pcm_prepare() that I didn't
> > see with a quick look?
> > Both of them call hdmi_setup_audio_infoframe() which is not 
> > thread-safe.
> > 
> > Yeah, we need to introduce a mutex there.
> 
> I guess hdmi_present_sense() is the same, as it can also call
> hdmi_setup_audio_infoframe()?

Hm, looking at the code, we have already pin_eld->lock in
hdmi_present_sense().  This can be used in other places, too.

(And pin_eld->monitor_present change at the beginning of
 hdmi_present_sense() should be protected in the mutex, too...)

> BTW, shouldn't hdmi_present_sense() call hdmi_setup_audio_infoframe() 
> always
> when eld_changed is true? hdmi_setup_audio_infoframe() does not do its
> stuff if !monitor_present, so if one starts playback before plugging in,
> the channel mappings etc will never be set even when plug-in happens
> (from what I can see, that is -  did not test yet).

Well, there is such a call but currently specific to Haswell because
it hasn't been tested for others.  Maybe we can simply get rid of
is_haswell() check there.



Takashi


> 
> 
> > thanks,
> > 
> > Takashi
> > 
> > sound/pci/hda/patch_hdmi.c | 14 +++++++++++---
> > 1 file changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> > index 7ea0245..09f38a1 100644
> > --- a/sound/pci/hda/patch_hdmi.c
> > +++ b/sound/pci/hda/patch_hdmi.c
> > @@ -74,6 +74,8 @@ struct hdmi_spec_per_pin {
> > 	bool non_pcm;
> > 	bool chmap_set;		/* channel-map override by ALSA API? */
> > 	unsigned char chmap[8]; /* ALSA API channel-map */
> > +	bool last_chmap_set;	/* channel-map override programmed? */
> > +	unsigned char last_chmap[8]; /* last programmed channel-map */
> > 	char pcm_name[8];	/* filled in build_pcm callbacks */
> > };
> > 
> > @@ -955,15 +957,21 @@ static void hdmi_setup_audio_infoframe(struct 
> > hda_codec *codec,
> > 					    ai.bytes, sizeof(ai));
> > 		hdmi_start_infoframe_trans(codec, pin_nid);
> > 	} else {
> > -		/* For non-pcm audio switch, setup new channel mapping
> > -		 * accordingly */
> > -		if (per_pin->non_pcm != non_pcm)
> > +		/* For non-pcm audio switch or channel-map switch,
> > +		 * setup new channel mapping accordingly */
> > +		if (per_pin->non_pcm != non_pcm
> > +		    || per_pin->chmap_set != per_pin->last_chmap_set
> > +		    || (per_pin->chmap_set && memcmp(per_pin->chmap, 
> > per_pin->last_chmap,
> > +						     ARRAY_SIZE(per_pin->chmap)) != 0))
> > 			hdmi_setup_channel_mapping(codec, pin_nid, non_pcm, ca,
> > 						   channels, per_pin->chmap,
> > 						   per_pin->chmap_set);
> > 	}
> > 
> > 	per_pin->non_pcm = non_pcm;
> > +	per_pin->last_chmap_set = per_pin->chmap_set;
> > +	if (per_pin->chmap_set)
> > +		memcpy(per_pin->last_chmap, per_pin->chmap, 
> > ARRAY_SIZE(per_pin->chmap));
> > }
> > 
> > 
> > --
> > 1.8.1.5
> > 
> 

Re: [PATCH] ALSA: hda - hdmi: Fix channel map switch not taking effect

[Anssi Hannula]

Takashi Iwai kirjoitti 2013-10-07 13:43:
> At Mon, 07 Oct 2013 13:31:17 +0300,
> Anssi Hannula wrote:
> 
> Takashi Iwai kirjoitti 2013-10-07 10:48:
> > At Wed,  2 Oct 2013 21:13:32 +0300,
> > Anssi Hannula wrote:
> >
> > Currently hdmi_setup_audio_infoframe() reprograms the HDA channel
> > mapping only when the infoframe is not up-to-date or the non-PCM flag
> > has changed.
> >
> > However, when just the channel map has been changed, the infoframe may
> > still be up-to-date and non-PCM flag may not have changed, so the new
> > channel map is not actually programmed into the HDA codec.
> >
> > Notably, this failing case is also always triggered when the device is
> > already in a prepared state and a new channel map is configured while
> > changing only the channel positions (for example, plain
> > "speaker-test -c2 -m FR,FL").
> >
> > Fix that by keeping track of the last programmed channel map and making
> > hdmi_setup_audio_infoframe() always update the hardware mapping if the
> > channel map has changed.
> >
> > Hmm, maybe it's easier (and safer) to remove the check and always call
> > hdmi_setup_channel_mapping()?  Of course, the drawback is the
> > unnecessary verbs, but it's not too much.
> 
> Easier, yes. I don't really have that good of an idea how big of a 
> delay
> do 8 write verbs cause, so I'll rely on your judgment :)
> 
> I guess I could replace the writes with read-and-write-if-differs 
> cycles
> when I add the set_slot ops for ATI/AMD, since it seems to be used for
> other similar cases (like channel count)?
> 
> Well, this would result in more cost, as read requires a sync.

Hmm, so why does e.g. converter channel count set-up use that, then?

> If any, we can use the cached write/update.  But this assumes that the
> hardware never clears the value by some operation internally.

It should not, but better be safe than sorry here (at least for stable) 
:)

> > Signed-off-by: Anssi Hannula <anssi.hannula@iki.fi>
> > Cc: <stable@vger.kernel.org>
> > ---
> >
> > BTW, is there some mechanism preventing hdmi_chmap_ctl_put() to be
> > called
> > at the same time with generic_hdmi_playback_pcm_prepare() that I didn't
> > see with a quick look?
> > Both of them call hdmi_setup_audio_infoframe() which is not
> > thread-safe.
> >
> > Yeah, we need to introduce a mutex there.
> 
> I guess hdmi_present_sense() is the same, as it can also call
> hdmi_setup_audio_infoframe()?
> 
> Hm, looking at the code, we have already pin_eld->lock in
> hdmi_present_sense().  This can be used in other places, too.

Well, hdmi_setup_audio_infoframe() is non-threadsafe for other reasons 
than
eld as well (e.g. write verbs to hw), though, so it might be a bit 
confusing
as-is. Better move/rename the lock if we end up using it? Or not?

> (And pin_eld->monitor_present change at the beginning of
> hdmi_present_sense() should be protected in the mutex, too...)
> 
> BTW, shouldn't hdmi_present_sense() call hdmi_setup_audio_infoframe()
> always
> when eld_changed is true? hdmi_setup_audio_infoframe() does not do its
> stuff if !monitor_present, so if one starts playback before plugging 
> in,
> the channel mappings etc will never be set even when plug-in happens
> (from what I can see, that is -  did not test yet).
> 
> Well, there is such a call but currently specific to Haswell because
> it hasn't been tested for others.  Maybe we can simply get rid of
> is_haswell() check there.

OK, will test later and provide a patch if it works as expected.

> 
> 
> 
> 
> > thanks,
> >
> > Takashi
> >
> > sound/pci/hda/patch_hdmi.c | 14 +++++++++++---
> > 1 file changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> > index 7ea0245..09f38a1 100644
> > --- a/sound/pci/hda/patch_hdmi.c
> > +++ b/sound/pci/hda/patch_hdmi.c
> > @@ -74,6 +74,8 @@ struct hdmi_spec_per_pin {
> > 	bool non_pcm;
> > 	bool chmap_set;		/* channel-map override by ALSA API? */
> > 	unsigned char chmap[8]; /* ALSA API channel-map */
> > +	bool last_chmap_set;	/* channel-map override programmed? */
> > +	unsigned char last_chmap[8]; /* last programmed channel-map */
> > 	char pcm_name[8];	/* filled in build_pcm callbacks */
> > };
> >
> > @@ -955,15 +957,21 @@ static void hdmi_setup_audio_infoframe(struct
> > hda_codec *codec,
> > 					    ai.bytes, sizeof(ai));
> > 		hdmi_start_infoframe_trans(codec, pin_nid);
> > 	} else {
> > -		/* For non-pcm audio switch, setup new channel mapping
> > -		 * accordingly */
> > -		if (per_pin->non_pcm != non_pcm)
> > +		/* For non-pcm audio switch or channel-map switch,
> > +		 * setup new channel mapping accordingly */
> > +		if (per_pin->non_pcm != non_pcm
> > +		    || per_pin->chmap_set != per_pin->last_chmap_set
> > +		    || (per_pin->chmap_set && memcmp(per_pin->chmap,
> > per_pin->last_chmap,
> > +						     ARRAY_SIZE(per_pin->chmap)) != 0))
> > 			hdmi_setup_channel_mapping(codec, pin_nid, non_pcm, ca,
> > 						   channels, per_pin->chmap,
> > 						   per_pin->chmap_set);
> > 	}
> >
> > 	per_pin->non_pcm = non_pcm;
> > +	per_pin->last_chmap_set = per_pin->chmap_set;
> > +	if (per_pin->chmap_set)
> > +		memcpy(per_pin->last_chmap, per_pin->chmap,
> > ARRAY_SIZE(per_pin->chmap));
> > }
> >
> >
> > --
> > 1.8.1.5
> >
> 

Re: [PATCH] ALSA: hda - hdmi: Fix channel map switch not taking effect

[Takashi Iwai]

At Mon, 07 Oct 2013 15:45:09 +0300,
Anssi Hannula wrote:
> 
> Takashi Iwai kirjoitti 2013-10-07 13:43:
> > At Mon, 07 Oct 2013 13:31:17 +0300,
> > Anssi Hannula wrote:
> > 
> > Takashi Iwai kirjoitti 2013-10-07 10:48:
> > > At Wed,  2 Oct 2013 21:13:32 +0300,
> > > Anssi Hannula wrote:
> > >
> > > Currently hdmi_setup_audio_infoframe() reprograms the HDA channel
> > > mapping only when the infoframe is not up-to-date or the non-PCM flag
> > > has changed.
> > >
> > > However, when just the channel map has been changed, the infoframe may
> > > still be up-to-date and non-PCM flag may not have changed, so the new
> > > channel map is not actually programmed into the HDA codec.
> > >
> > > Notably, this failing case is also always triggered when the device is
> > > already in a prepared state and a new channel map is configured while
> > > changing only the channel positions (for example, plain
> > > "speaker-test -c2 -m FR,FL").
> > >
> > > Fix that by keeping track of the last programmed channel map and making
> > > hdmi_setup_audio_infoframe() always update the hardware mapping if the
> > > channel map has changed.
> > >
> > > Hmm, maybe it's easier (and safer) to remove the check and always call
> > > hdmi_setup_channel_mapping()?  Of course, the drawback is the
> > > unnecessary verbs, but it's not too much.
> > 
> > Easier, yes. I don't really have that good of an idea how big of a 
> > delay
> > do 8 write verbs cause, so I'll rely on your judgment :)
> > 
> > I guess I could replace the writes with read-and-write-if-differs 
> > cycles
> > when I add the set_slot ops for ATI/AMD, since it seems to be used for
> > other similar cases (like channel count)?
> > 
> > Well, this would result in more cost, as read requires a sync.
> 
> Hmm, so why does e.g. converter channel count set-up use that, then?

It can be optimized as well :)

> > If any, we can use the cached write/update.  But this assumes that the
> > hardware never clears the value by some operation internally.
> 
> It should not, but better be safe than sorry here (at least for stable) 
> :)
> 
> > > Signed-off-by: Anssi Hannula <anssi.hannula@iki.fi>
> > > Cc: <stable@vger.kernel.org>
> > > ---
> > >
> > > BTW, is there some mechanism preventing hdmi_chmap_ctl_put() to be
> > > called
> > > at the same time with generic_hdmi_playback_pcm_prepare() that I didn't
> > > see with a quick look?
> > > Both of them call hdmi_setup_audio_infoframe() which is not
> > > thread-safe.
> > >
> > > Yeah, we need to introduce a mutex there.
> > 
> > I guess hdmi_present_sense() is the same, as it can also call
> > hdmi_setup_audio_infoframe()?
> > 
> > Hm, looking at the code, we have already pin_eld->lock in
> > hdmi_present_sense().  This can be used in other places, too.
> 
> Well, hdmi_setup_audio_infoframe() is non-threadsafe for other reasons 
> than
> eld as well (e.g. write verbs to hw), though, so it might be a bit 
> confusing
> as-is. Better move/rename the lock if we end up using it? Or not?

Yeah, it'd make sense.

> > (And pin_eld->monitor_present change at the beginning of
> > hdmi_present_sense() should be protected in the mutex, too...)
> > 
> > BTW, shouldn't hdmi_present_sense() call hdmi_setup_audio_infoframe()
> > always
> > when eld_changed is true? hdmi_setup_audio_infoframe() does not do its
> > stuff if !monitor_present, so if one starts playback before plugging 
> > in,
> > the channel mappings etc will never be set even when plug-in happens
> > (from what I can see, that is -  did not test yet).
> > 
> > Well, there is such a call but currently specific to Haswell because
> > it hasn't been tested for others.  Maybe we can simply get rid of
> > is_haswell() check there.
> 
> OK, will test later and provide a patch if it works as expected.

Thanks.


Takashi


> 
> > 
> > 
> > 
> > 
> > > thanks,
> > >
> > > Takashi
> > >
> > > sound/pci/hda/patch_hdmi.c | 14 +++++++++++---
> > > 1 file changed, 11 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> > > index 7ea0245..09f38a1 100644
> > > --- a/sound/pci/hda/patch_hdmi.c
> > > +++ b/sound/pci/hda/patch_hdmi.c
> > > @@ -74,6 +74,8 @@ struct hdmi_spec_per_pin {
> > > 	bool non_pcm;
> > > 	bool chmap_set;		/* channel-map override by ALSA API? */
> > > 	unsigned char chmap[8]; /* ALSA API channel-map */
> > > +	bool last_chmap_set;	/* channel-map override programmed? */
> > > +	unsigned char last_chmap[8]; /* last programmed channel-map */
> > > 	char pcm_name[8];	/* filled in build_pcm callbacks */
> > > };
> > >
> > > @@ -955,15 +957,21 @@ static void hdmi_setup_audio_infoframe(struct
> > > hda_codec *codec,
> > > 					    ai.bytes, sizeof(ai));
> > > 		hdmi_start_infoframe_trans(codec, pin_nid);
> > > 	} else {
> > > -		/* For non-pcm audio switch, setup new channel mapping
> > > -		 * accordingly */
> > > -		if (per_pin->non_pcm != non_pcm)
> > > +		/* For non-pcm audio switch or channel-map switch,
> > > +		 * setup new channel mapping accordingly */
> > > +		if (per_pin->non_pcm != non_pcm
> > > +		    || per_pin->chmap_set != per_pin->last_chmap_set
> > > +		    || (per_pin->chmap_set && memcmp(per_pin->chmap,
> > > per_pin->last_chmap,
> > > +						     ARRAY_SIZE(per_pin->chmap)) != 0))
> > > 			hdmi_setup_channel_mapping(codec, pin_nid, non_pcm, ca,
> > > 						   channels, per_pin->chmap,
> > > 						   per_pin->chmap_set);
> > > 	}
> > >
> > > 	per_pin->non_pcm = non_pcm;
> > > +	per_pin->last_chmap_set = per_pin->chmap_set;
> > > +	if (per_pin->chmap_set)
> > > +		memcpy(per_pin->last_chmap, per_pin->chmap,
> > > ARRAY_SIZE(per_pin->chmap));
> > > }
> > >
> > >
> > > --
> > > 1.8.1.5
> > >
> > 
> 

[PATCH v2] ALSA: hda - hdmi: Fix channel map switch not taking effect

[Anssi Hannula]

Currently hdmi_setup_audio_infoframe() reprograms the HDA channel
mapping only when the infoframe is not up-to-date or the non-PCM flag
has changed.

However, when just the channel map has been changed, the infoframe may
still be up-to-date and non-PCM flag may not have changed, so the new
channel map is not actually programmed into the HDA codec.

Notably, this failing case is also always triggered when the device is
already in a prepared state and a new channel map is configured while
changing only the channel positions (for example, plain
"speaker-test -c2 -m FR,FL").

Fix that by always programming the channel map in
hdmi_setup_audio_infoframe(). Tested on Intel HDMI.

Signed-off-by: Anssi Hannula <anssi.hannula@iki.fi>
Cc: <stable@vger.kernel.org>
---

Here is a simpler version of the map switch fix.

 sound/pci/hda/patch_hdmi.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 7ea0245..50173d4 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -937,6 +937,14 @@ static void hdmi_setup_audio_infoframe(struct hda_codec *codec,
 	}
 
 	/*
+	 * always configure channel mapping, it may have been changed by the
+	 * user in the meantime
+	 */
+	hdmi_setup_channel_mapping(codec, pin_nid, non_pcm, ca,
+				   channels, per_pin->chmap,
+				   per_pin->chmap_set);
+
+	/*
 	 * sizeof(ai) is used instead of sizeof(*hdmi_ai) or
 	 * sizeof(*dp_ai) to avoid partial match/update problems when
 	 * the user switches between HDMI/DP monitors.
@@ -947,20 +955,10 @@ static void hdmi_setup_audio_infoframe(struct hda_codec *codec,
 			    "pin=%d channels=%d\n",
 			    pin_nid,
 			    channels);
-		hdmi_setup_channel_mapping(codec, pin_nid, non_pcm, ca,
-					   channels, per_pin->chmap,
-					   per_pin->chmap_set);
 		hdmi_stop_infoframe_trans(codec, pin_nid);
 		hdmi_fill_audio_infoframe(codec, pin_nid,
 					    ai.bytes, sizeof(ai));
 		hdmi_start_infoframe_trans(codec, pin_nid);
-	} else {
-		/* For non-pcm audio switch, setup new channel mapping
-		 * accordingly */
-		if (per_pin->non_pcm != non_pcm)
-			hdmi_setup_channel_mapping(codec, pin_nid, non_pcm, ca,
-						   channels, per_pin->chmap,
-						   per_pin->chmap_set);
 	}
 
 	per_pin->non_pcm = non_pcm;
-- 
1.8.1.5

Re: [PATCH v2] ALSA: hda - hdmi: Fix channel map switch not taking effect

[Takashi Iwai]

At Mon,  7 Oct 2013 19:24:52 +0300,
Anssi Hannula wrote:
> 
> Currently hdmi_setup_audio_infoframe() reprograms the HDA channel
> mapping only when the infoframe is not up-to-date or the non-PCM flag
> has changed.
> 
> However, when just the channel map has been changed, the infoframe may
> still be up-to-date and non-PCM flag may not have changed, so the new
> channel map is not actually programmed into the HDA codec.
> 
> Notably, this failing case is also always triggered when the device is
> already in a prepared state and a new channel map is configured while
> changing only the channel positions (for example, plain
> "speaker-test -c2 -m FR,FL").
> 
> Fix that by always programming the channel map in
> hdmi_setup_audio_infoframe(). Tested on Intel HDMI.
> 
> Signed-off-by: Anssi Hannula <anssi.hannula@iki.fi>
> Cc: <stable@vger.kernel.org>
> ---
> 
> Here is a simpler version of the map switch fix.

Thanks, applied now.

The for-linus branch including this fix was merged back to for-next
branch, too, so you can use for-next branch for the further
development.


Takashi

> 
>  sound/pci/hda/patch_hdmi.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index 7ea0245..50173d4 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -937,6 +937,14 @@ static void hdmi_setup_audio_infoframe(struct hda_codec *codec,
>  	}
>  
>  	/*
> +	 * always configure channel mapping, it may have been changed by the
> +	 * user in the meantime
> +	 */
> +	hdmi_setup_channel_mapping(codec, pin_nid, non_pcm, ca,
> +				   channels, per_pin->chmap,
> +				   per_pin->chmap_set);
> +
> +	/*
>  	 * sizeof(ai) is used instead of sizeof(*hdmi_ai) or
>  	 * sizeof(*dp_ai) to avoid partial match/update problems when
>  	 * the user switches between HDMI/DP monitors.
> @@ -947,20 +955,10 @@ static void hdmi_setup_audio_infoframe(struct hda_codec *codec,
>  			    "pin=%d channels=%d\n",
>  			    pin_nid,
>  			    channels);
> -		hdmi_setup_channel_mapping(codec, pin_nid, non_pcm, ca,
> -					   channels, per_pin->chmap,
> -					   per_pin->chmap_set);
>  		hdmi_stop_infoframe_trans(codec, pin_nid);
>  		hdmi_fill_audio_infoframe(codec, pin_nid,
>  					    ai.bytes, sizeof(ai));
>  		hdmi_start_infoframe_trans(codec, pin_nid);
> -	} else {
> -		/* For non-pcm audio switch, setup new channel mapping
> -		 * accordingly */
> -		if (per_pin->non_pcm != non_pcm)
> -			hdmi_setup_channel_mapping(codec, pin_nid, non_pcm, ca,
> -						   channels, per_pin->chmap,
> -						   per_pin->chmap_set);
>  	}
>  
>  	per_pin->non_pcm = non_pcm;
> -- 
> 1.8.1.5
> 

Re: [PATCH] ALSA: hda - hdmi: Fix channel map switch not taking effect

[Takashi Iwai]

At Mon, 07 Oct 2013 15:09:15 +0200,
Takashi Iwai wrote:
> 
> At Mon, 07 Oct 2013 15:45:09 +0300,
> Anssi Hannula wrote:
> > 
> > Takashi Iwai kirjoitti 2013-10-07 13:43:
> > > At Mon, 07 Oct 2013 13:31:17 +0300,
> > > Anssi Hannula wrote:
> > > 
> > > Takashi Iwai kirjoitti 2013-10-07 10:48:
> > > > At Wed,  2 Oct 2013 21:13:32 +0300,
> > > > Anssi Hannula wrote:
> > > >
> > > > Currently hdmi_setup_audio_infoframe() reprograms the HDA channel
> > > > mapping only when the infoframe is not up-to-date or the non-PCM flag
> > > > has changed.
> > > >
> > > > However, when just the channel map has been changed, the infoframe may
> > > > still be up-to-date and non-PCM flag may not have changed, so the new
> > > > channel map is not actually programmed into the HDA codec.
> > > >
> > > > Notably, this failing case is also always triggered when the device is
> > > > already in a prepared state and a new channel map is configured while
> > > > changing only the channel positions (for example, plain
> > > > "speaker-test -c2 -m FR,FL").
> > > >
> > > > Fix that by keeping track of the last programmed channel map and making
> > > > hdmi_setup_audio_infoframe() always update the hardware mapping if the
> > > > channel map has changed.
> > > >
> > > > Hmm, maybe it's easier (and safer) to remove the check and always call
> > > > hdmi_setup_channel_mapping()?  Of course, the drawback is the
> > > > unnecessary verbs, but it's not too much.
> > > 
> > > Easier, yes. I don't really have that good of an idea how big of a 
> > > delay
> > > do 8 write verbs cause, so I'll rely on your judgment :)
> > > 
> > > I guess I could replace the writes with read-and-write-if-differs 
> > > cycles
> > > when I add the set_slot ops for ATI/AMD, since it seems to be used for
> > > other similar cases (like channel count)?
> > > 
> > > Well, this would result in more cost, as read requires a sync.
> > 
> > Hmm, so why does e.g. converter channel count set-up use that, then?
> 
> It can be optimized as well :)
> 
> > > If any, we can use the cached write/update.  But this assumes that the
> > > hardware never clears the value by some operation internally.
> > 
> > It should not, but better be safe than sorry here (at least for stable) 
> > :)
> > 
> > > > Signed-off-by: Anssi Hannula <anssi.hannula@iki.fi>
> > > > Cc: <stable@vger.kernel.org>
> > > > ---
> > > >
> > > > BTW, is there some mechanism preventing hdmi_chmap_ctl_put() to be
> > > > called
> > > > at the same time with generic_hdmi_playback_pcm_prepare() that I didn't
> > > > see with a quick look?
> > > > Both of them call hdmi_setup_audio_infoframe() which is not
> > > > thread-safe.
> > > >
> > > > Yeah, we need to introduce a mutex there.
> > > 
> > > I guess hdmi_present_sense() is the same, as it can also call
> > > hdmi_setup_audio_infoframe()?
> > > 
> > > Hm, looking at the code, we have already pin_eld->lock in
> > > hdmi_present_sense().  This can be used in other places, too.
> > 
> > Well, hdmi_setup_audio_infoframe() is non-threadsafe for other reasons 
> > than
> > eld as well (e.g. write verbs to hw), though, so it might be a bit 
> > confusing
> > as-is. Better move/rename the lock if we end up using it? Or not?
> 
> Yeah, it'd make sense.
> 
> > > (And pin_eld->monitor_present change at the beginning of
> > > hdmi_present_sense() should be protected in the mutex, too...)
> > > 
> > > BTW, shouldn't hdmi_present_sense() call hdmi_setup_audio_infoframe()
> > > always
> > > when eld_changed is true? hdmi_setup_audio_infoframe() does not do its
> > > stuff if !monitor_present, so if one starts playback before plugging 
> > > in,
> > > the channel mappings etc will never be set even when plug-in happens
> > > (from what I can see, that is -  did not test yet).
> > > 
> > > Well, there is such a call but currently specific to Haswell because
> > > it hasn't been tested for others.  Maybe we can simply get rid of
> > > is_haswell() check there.
> > 
> > OK, will test later and provide a patch if it works as expected.
> 
> Thanks.

[dropped stable from Cc]

Anssi, is the lock fix/cleanup patch ready?

I already have patche to extend mutex lock over other places and
move it into per_pin, but if you have already similar patches, I'll
take yours, hopefully together with other fixes.


thanks,

Takashi

Re: [PATCH] ALSA: hda - hdmi: Fix channel map switch not taking effect

[Anssi Hannula]

15.10.2013 18:32, Takashi Iwai kirjoitti:
> At Mon, 07 Oct 2013 15:09:15 +0200,
> Takashi Iwai wrote:
>>
>> At Mon, 07 Oct 2013 15:45:09 +0300,
>> Anssi Hannula wrote:
>>>
>>> Takashi Iwai kirjoitti 2013-10-07 13:43:
>>>> At Mon, 07 Oct 2013 13:31:17 +0300,
>>>> Anssi Hannula wrote:
>>>>
>>>> Takashi Iwai kirjoitti 2013-10-07 10:48:
>>>>> At Wed,  2 Oct 2013 21:13:32 +0300,
>>>>> Anssi Hannula wrote:
>>>>>
>>>>> Currently hdmi_setup_audio_infoframe() reprograms the HDA channel
>>>>> mapping only when the infoframe is not up-to-date or the non-PCM flag
>>>>> has changed.
>>>>>
>>>>> However, when just the channel map has been changed, the infoframe may
>>>>> still be up-to-date and non-PCM flag may not have changed, so the new
>>>>> channel map is not actually programmed into the HDA codec.
>>>>>
>>>>> Notably, this failing case is also always triggered when the device is
>>>>> already in a prepared state and a new channel map is configured while
>>>>> changing only the channel positions (for example, plain
>>>>> "speaker-test -c2 -m FR,FL").
>>>>>
>>>>> Fix that by keeping track of the last programmed channel map and making
>>>>> hdmi_setup_audio_infoframe() always update the hardware mapping if the
>>>>> channel map has changed.
>>>>>
>>>>> Hmm, maybe it's easier (and safer) to remove the check and always call
>>>>> hdmi_setup_channel_mapping()?  Of course, the drawback is the
>>>>> unnecessary verbs, but it's not too much.
>>>>
>>>> Easier, yes. I don't really have that good of an idea how big of a 
>>>> delay
>>>> do 8 write verbs cause, so I'll rely on your judgment :)
>>>>
>>>> I guess I could replace the writes with read-and-write-if-differs 
>>>> cycles
>>>> when I add the set_slot ops for ATI/AMD, since it seems to be used for
>>>> other similar cases (like channel count)?
>>>>
>>>> Well, this would result in more cost, as read requires a sync.
>>>
>>> Hmm, so why does e.g. converter channel count set-up use that, then?
>>
>> It can be optimized as well :)
>>
>>>> If any, we can use the cached write/update.  But this assumes that the
>>>> hardware never clears the value by some operation internally.
>>>
>>> It should not, but better be safe than sorry here (at least for stable) 
>>> :)
>>>
>>>>> Signed-off-by: Anssi Hannula <anssi.hannula@iki.fi>
>>>>> Cc: <stable@vger.kernel.org>
>>>>> ---
>>>>>
>>>>> BTW, is there some mechanism preventing hdmi_chmap_ctl_put() to be
>>>>> called
>>>>> at the same time with generic_hdmi_playback_pcm_prepare() that I didn't
>>>>> see with a quick look?
>>>>> Both of them call hdmi_setup_audio_infoframe() which is not
>>>>> thread-safe.
>>>>>
>>>>> Yeah, we need to introduce a mutex there.
>>>>
>>>> I guess hdmi_present_sense() is the same, as it can also call
>>>> hdmi_setup_audio_infoframe()?
>>>>
>>>> Hm, looking at the code, we have already pin_eld->lock in
>>>> hdmi_present_sense().  This can be used in other places, too.
>>>
>>> Well, hdmi_setup_audio_infoframe() is non-threadsafe for other reasons 
>>> than
>>> eld as well (e.g. write verbs to hw), though, so it might be a bit 
>>> confusing
>>> as-is. Better move/rename the lock if we end up using it? Or not?
>>
>> Yeah, it'd make sense.
>>
>>>> (And pin_eld->monitor_present change at the beginning of
>>>> hdmi_present_sense() should be protected in the mutex, too...)
>>>>
>>>> BTW, shouldn't hdmi_present_sense() call hdmi_setup_audio_infoframe()
>>>> always
>>>> when eld_changed is true? hdmi_setup_audio_infoframe() does not do its
>>>> stuff if !monitor_present, so if one starts playback before plugging 
>>>> in,
>>>> the channel mappings etc will never be set even when plug-in happens
>>>> (from what I can see, that is -  did not test yet).
>>>>
>>>> Well, there is such a call but currently specific to Haswell because
>>>> it hasn't been tested for others.  Maybe we can simply get rid of
>>>> is_haswell() check there.
>>>
>>> OK, will test later and provide a patch if it works as expected.
>>
>> Thanks.
> 
> [dropped stable from Cc]
> 
> Anssi, is the lock fix/cleanup patch ready?
> 
> I already have patche to extend mutex lock over other places and
> move it into per_pin, but if you have already similar patches, I'll
> take yours, hopefully together with other fixes.

Nope, haven't had the time to work on it yet (my excuse is that I was at
XBMC DevCon last weekend :) ).

So feel free to apply yours and I'll work on top of that (at some point
later this week I guess).

-- 
Anssi Hannula

Re: [PATCH] ALSA: hda - hdmi: Fix channel map switch not taking effect

[Takashi Iwai]

At Tue, 15 Oct 2013 19:22:00 +0300,
Anssi Hannula wrote:
> 
> 15.10.2013 18:32, Takashi Iwai kirjoitti:
> > At Mon, 07 Oct 2013 15:09:15 +0200,
> > Takashi Iwai wrote:
> >>
> >> At Mon, 07 Oct 2013 15:45:09 +0300,
> >> Anssi Hannula wrote:
> >>>
> >>> Takashi Iwai kirjoitti 2013-10-07 13:43:
> >>>> At Mon, 07 Oct 2013 13:31:17 +0300,
> >>>> Anssi Hannula wrote:
> >>>>
> >>>> Takashi Iwai kirjoitti 2013-10-07 10:48:
> >>>>> At Wed,  2 Oct 2013 21:13:32 +0300,
> >>>>> Anssi Hannula wrote:
> >>>>>
> >>>>> Currently hdmi_setup_audio_infoframe() reprograms the HDA channel
> >>>>> mapping only when the infoframe is not up-to-date or the non-PCM flag
> >>>>> has changed.
> >>>>>
> >>>>> However, when just the channel map has been changed, the infoframe may
> >>>>> still be up-to-date and non-PCM flag may not have changed, so the new
> >>>>> channel map is not actually programmed into the HDA codec.
> >>>>>
> >>>>> Notably, this failing case is also always triggered when the device is
> >>>>> already in a prepared state and a new channel map is configured while
> >>>>> changing only the channel positions (for example, plain
> >>>>> "speaker-test -c2 -m FR,FL").
> >>>>>
> >>>>> Fix that by keeping track of the last programmed channel map and making
> >>>>> hdmi_setup_audio_infoframe() always update the hardware mapping if the
> >>>>> channel map has changed.
> >>>>>
> >>>>> Hmm, maybe it's easier (and safer) to remove the check and always call
> >>>>> hdmi_setup_channel_mapping()?  Of course, the drawback is the
> >>>>> unnecessary verbs, but it's not too much.
> >>>>
> >>>> Easier, yes. I don't really have that good of an idea how big of a 
> >>>> delay
> >>>> do 8 write verbs cause, so I'll rely on your judgment :)
> >>>>
> >>>> I guess I could replace the writes with read-and-write-if-differs 
> >>>> cycles
> >>>> when I add the set_slot ops for ATI/AMD, since it seems to be used for
> >>>> other similar cases (like channel count)?
> >>>>
> >>>> Well, this would result in more cost, as read requires a sync.
> >>>
> >>> Hmm, so why does e.g. converter channel count set-up use that, then?
> >>
> >> It can be optimized as well :)
> >>
> >>>> If any, we can use the cached write/update.  But this assumes that the
> >>>> hardware never clears the value by some operation internally.
> >>>
> >>> It should not, but better be safe than sorry here (at least for stable) 
> >>> :)
> >>>
> >>>>> Signed-off-by: Anssi Hannula <anssi.hannula@iki.fi>
> >>>>> Cc: <stable@vger.kernel.org>
> >>>>> ---
> >>>>>
> >>>>> BTW, is there some mechanism preventing hdmi_chmap_ctl_put() to be
> >>>>> called
> >>>>> at the same time with generic_hdmi_playback_pcm_prepare() that I didn't
> >>>>> see with a quick look?
> >>>>> Both of them call hdmi_setup_audio_infoframe() which is not
> >>>>> thread-safe.
> >>>>>
> >>>>> Yeah, we need to introduce a mutex there.
> >>>>
> >>>> I guess hdmi_present_sense() is the same, as it can also call
> >>>> hdmi_setup_audio_infoframe()?
> >>>>
> >>>> Hm, looking at the code, we have already pin_eld->lock in
> >>>> hdmi_present_sense().  This can be used in other places, too.
> >>>
> >>> Well, hdmi_setup_audio_infoframe() is non-threadsafe for other reasons 
> >>> than
> >>> eld as well (e.g. write verbs to hw), though, so it might be a bit 
> >>> confusing
> >>> as-is. Better move/rename the lock if we end up using it? Or not?
> >>
> >> Yeah, it'd make sense.
> >>
> >>>> (And pin_eld->monitor_present change at the beginning of
> >>>> hdmi_present_sense() should be protected in the mutex, too...)
> >>>>
> >>>> BTW, shouldn't hdmi_present_sense() call hdmi_setup_audio_infoframe()
> >>>> always
> >>>> when eld_changed is true? hdmi_setup_audio_infoframe() does not do its
> >>>> stuff if !monitor_present, so if one starts playback before plugging 
> >>>> in,
> >>>> the channel mappings etc will never be set even when plug-in happens
> >>>> (from what I can see, that is -  did not test yet).
> >>>>
> >>>> Well, there is such a call but currently specific to Haswell because
> >>>> it hasn't been tested for others.  Maybe we can simply get rid of
> >>>> is_haswell() check there.
> >>>
> >>> OK, will test later and provide a patch if it works as expected.
> >>
> >> Thanks.
> > 
> > [dropped stable from Cc]
> > 
> > Anssi, is the lock fix/cleanup patch ready?
> > 
> > I already have patche to extend mutex lock over other places and
> > move it into per_pin, but if you have already similar patches, I'll
> > take yours, hopefully together with other fixes.
> 
> Nope, haven't had the time to work on it yet (my excuse is that I was at
> XBMC DevCon last weekend :) ).
> 
> So feel free to apply yours and I'll work on top of that (at some point
> later this week I guess).

Anssi, do you have the merge-ready version of AMD HDMI fix patches?
Since 3.13 merge window will be opened soon, I'd like to merge such
big (and good) things now if possible.


Takashi

Re: [PATCH] ALSA: hda - hdmi: Fix channel map switch not taking effect

[Anssi Hannula]

24.10.2013 14:01, Takashi Iwai kirjoitti:
> At Tue, 15 Oct 2013 19:22:00 +0300,
> Anssi Hannula wrote:
>>
>> 15.10.2013 18:32, Takashi Iwai kirjoitti:
>>> At Mon, 07 Oct 2013 15:09:15 +0200,
>>> Takashi Iwai wrote:
>>>>
>>>> At Mon, 07 Oct 2013 15:45:09 +0300,
>>>> Anssi Hannula wrote:
>>>>>
>>>>> Takashi Iwai kirjoitti 2013-10-07 13:43:
>>>>>> At Mon, 07 Oct 2013 13:31:17 +0300,
>>>>>> Anssi Hannula wrote:
>>>>>>
>>>>>> Takashi Iwai kirjoitti 2013-10-07 10:48:
>>>>>>> At Wed,  2 Oct 2013 21:13:32 +0300,
>>>>>>> Anssi Hannula wrote:
>>>>>>>
>>>>>>> Currently hdmi_setup_audio_infoframe() reprograms the HDA channel
>>>>>>> mapping only when the infoframe is not up-to-date or the non-PCM flag
>>>>>>> has changed.
>>>>>>>
>>>>>>> However, when just the channel map has been changed, the infoframe may
>>>>>>> still be up-to-date and non-PCM flag may not have changed, so the new
>>>>>>> channel map is not actually programmed into the HDA codec.
>>>>>>>
>>>>>>> Notably, this failing case is also always triggered when the device is
>>>>>>> already in a prepared state and a new channel map is configured while
>>>>>>> changing only the channel positions (for example, plain
>>>>>>> "speaker-test -c2 -m FR,FL").
>>>>>>>
>>>>>>> Fix that by keeping track of the last programmed channel map and making
>>>>>>> hdmi_setup_audio_infoframe() always update the hardware mapping if the
>>>>>>> channel map has changed.
>>>>>>>
>>>>>>> Hmm, maybe it's easier (and safer) to remove the check and always call
>>>>>>> hdmi_setup_channel_mapping()?  Of course, the drawback is the
>>>>>>> unnecessary verbs, but it's not too much.
>>>>>>
>>>>>> Easier, yes. I don't really have that good of an idea how big of a 
>>>>>> delay
>>>>>> do 8 write verbs cause, so I'll rely on your judgment :)
>>>>>>
>>>>>> I guess I could replace the writes with read-and-write-if-differs 
>>>>>> cycles
>>>>>> when I add the set_slot ops for ATI/AMD, since it seems to be used for
>>>>>> other similar cases (like channel count)?
>>>>>>
>>>>>> Well, this would result in more cost, as read requires a sync.
>>>>>
>>>>> Hmm, so why does e.g. converter channel count set-up use that, then?
>>>>
>>>> It can be optimized as well :)
>>>>
>>>>>> If any, we can use the cached write/update.  But this assumes that the
>>>>>> hardware never clears the value by some operation internally.
>>>>>
>>>>> It should not, but better be safe than sorry here (at least for stable) 
>>>>> :)
>>>>>
>>>>>>> Signed-off-by: Anssi Hannula <anssi.hannula@iki.fi>
>>>>>>> Cc: <stable@vger.kernel.org>
>>>>>>> ---
>>>>>>>
>>>>>>> BTW, is there some mechanism preventing hdmi_chmap_ctl_put() to be
>>>>>>> called
>>>>>>> at the same time with generic_hdmi_playback_pcm_prepare() that I didn't
>>>>>>> see with a quick look?
>>>>>>> Both of them call hdmi_setup_audio_infoframe() which is not
>>>>>>> thread-safe.
>>>>>>>
>>>>>>> Yeah, we need to introduce a mutex there.
>>>>>>
>>>>>> I guess hdmi_present_sense() is the same, as it can also call
>>>>>> hdmi_setup_audio_infoframe()?
>>>>>>
>>>>>> Hm, looking at the code, we have already pin_eld->lock in
>>>>>> hdmi_present_sense().  This can be used in other places, too.
>>>>>
>>>>> Well, hdmi_setup_audio_infoframe() is non-threadsafe for other reasons 
>>>>> than
>>>>> eld as well (e.g. write verbs to hw), though, so it might be a bit 
>>>>> confusing
>>>>> as-is. Better move/rename the lock if we end up using it? Or not?
>>>>
>>>> Yeah, it'd make sense.
>>>>
>>>>>> (And pin_eld->monitor_present change at the beginning of
>>>>>> hdmi_present_sense() should be protected in the mutex, too...)
>>>>>>
>>>>>> BTW, shouldn't hdmi_present_sense() call hdmi_setup_audio_infoframe()
>>>>>> always
>>>>>> when eld_changed is true? hdmi_setup_audio_infoframe() does not do its
>>>>>> stuff if !monitor_present, so if one starts playback before plugging 
>>>>>> in,
>>>>>> the channel mappings etc will never be set even when plug-in happens
>>>>>> (from what I can see, that is -  did not test yet).
>>>>>>
>>>>>> Well, there is such a call but currently specific to Haswell because
>>>>>> it hasn't been tested for others.  Maybe we can simply get rid of
>>>>>> is_haswell() check there.
>>>>>
>>>>> OK, will test later and provide a patch if it works as expected.
>>>>
>>>> Thanks.
>>>
>>> [dropped stable from Cc]
>>>
>>> Anssi, is the lock fix/cleanup patch ready?
>>>
>>> I already have patche to extend mutex lock over other places and
>>> move it into per_pin, but if you have already similar patches, I'll
>>> take yours, hopefully together with other fixes.
>>
>> Nope, haven't had the time to work on it yet (my excuse is that I was at
>> XBMC DevCon last weekend :) ).
>>
>> So feel free to apply yours and I'll work on top of that (at some point
>> later this week I guess).
> 
> Anssi, do you have the merge-ready version of AMD HDMI fix patches?
> Since 3.13 merge window will be opened soon, I'd like to merge such
> big (and good) things now if possible.

Well, I have finished the hdmi_ops conversion, but the updated patchset
has not yet been tested on ATI/AMD. But I guess I could post the updated
patchset anyway (today), and fix any possible remaining issues afterwards...

-- 
Anssi Hannula

Re: [PATCH] ALSA: hda - hdmi: Fix channel map switch not taking effect

[Takashi Iwai]

At Thu, 24 Oct 2013 17:20:09 +0300,
Anssi Hannula wrote:
> 
> 24.10.2013 14:01, Takashi Iwai kirjoitti:
> > At Tue, 15 Oct 2013 19:22:00 +0300,
> > Anssi Hannula wrote:
> >>
> >> 15.10.2013 18:32, Takashi Iwai kirjoitti:
> >>> At Mon, 07 Oct 2013 15:09:15 +0200,
> >>> Takashi Iwai wrote:
> >>>>
> >>>> At Mon, 07 Oct 2013 15:45:09 +0300,
> >>>> Anssi Hannula wrote:
> >>>>>
> >>>>> Takashi Iwai kirjoitti 2013-10-07 13:43:
> >>>>>> At Mon, 07 Oct 2013 13:31:17 +0300,
> >>>>>> Anssi Hannula wrote:
> >>>>>>
> >>>>>> Takashi Iwai kirjoitti 2013-10-07 10:48:
> >>>>>>> At Wed,  2 Oct 2013 21:13:32 +0300,
> >>>>>>> Anssi Hannula wrote:
> >>>>>>>
> >>>>>>> Currently hdmi_setup_audio_infoframe() reprograms the HDA channel
> >>>>>>> mapping only when the infoframe is not up-to-date or the non-PCM flag
> >>>>>>> has changed.
> >>>>>>>
> >>>>>>> However, when just the channel map has been changed, the infoframe may
> >>>>>>> still be up-to-date and non-PCM flag may not have changed, so the new
> >>>>>>> channel map is not actually programmed into the HDA codec.
> >>>>>>>
> >>>>>>> Notably, this failing case is also always triggered when the device is
> >>>>>>> already in a prepared state and a new channel map is configured while
> >>>>>>> changing only the channel positions (for example, plain
> >>>>>>> "speaker-test -c2 -m FR,FL").
> >>>>>>>
> >>>>>>> Fix that by keeping track of the last programmed channel map and making
> >>>>>>> hdmi_setup_audio_infoframe() always update the hardware mapping if the
> >>>>>>> channel map has changed.
> >>>>>>>
> >>>>>>> Hmm, maybe it's easier (and safer) to remove the check and always call
> >>>>>>> hdmi_setup_channel_mapping()?  Of course, the drawback is the
> >>>>>>> unnecessary verbs, but it's not too much.
> >>>>>>
> >>>>>> Easier, yes. I don't really have that good of an idea how big of a 
> >>>>>> delay
> >>>>>> do 8 write verbs cause, so I'll rely on your judgment :)
> >>>>>>
> >>>>>> I guess I could replace the writes with read-and-write-if-differs 
> >>>>>> cycles
> >>>>>> when I add the set_slot ops for ATI/AMD, since it seems to be used for
> >>>>>> other similar cases (like channel count)?
> >>>>>>
> >>>>>> Well, this would result in more cost, as read requires a sync.
> >>>>>
> >>>>> Hmm, so why does e.g. converter channel count set-up use that, then?
> >>>>
> >>>> It can be optimized as well :)
> >>>>
> >>>>>> If any, we can use the cached write/update.  But this assumes that the
> >>>>>> hardware never clears the value by some operation internally.
> >>>>>
> >>>>> It should not, but better be safe than sorry here (at least for stable) 
> >>>>> :)
> >>>>>
> >>>>>>> Signed-off-by: Anssi Hannula <anssi.hannula@iki.fi>
> >>>>>>> Cc: <stable@vger.kernel.org>
> >>>>>>> ---
> >>>>>>>
> >>>>>>> BTW, is there some mechanism preventing hdmi_chmap_ctl_put() to be
> >>>>>>> called
> >>>>>>> at the same time with generic_hdmi_playback_pcm_prepare() that I didn't
> >>>>>>> see with a quick look?
> >>>>>>> Both of them call hdmi_setup_audio_infoframe() which is not
> >>>>>>> thread-safe.
> >>>>>>>
> >>>>>>> Yeah, we need to introduce a mutex there.
> >>>>>>
> >>>>>> I guess hdmi_present_sense() is the same, as it can also call
> >>>>>> hdmi_setup_audio_infoframe()?
> >>>>>>
> >>>>>> Hm, looking at the code, we have already pin_eld->lock in
> >>>>>> hdmi_present_sense().  This can be used in other places, too.
> >>>>>
> >>>>> Well, hdmi_setup_audio_infoframe() is non-threadsafe for other reasons 
> >>>>> than
> >>>>> eld as well (e.g. write verbs to hw), though, so it might be a bit 
> >>>>> confusing
> >>>>> as-is. Better move/rename the lock if we end up using it? Or not?
> >>>>
> >>>> Yeah, it'd make sense.
> >>>>
> >>>>>> (And pin_eld->monitor_present change at the beginning of
> >>>>>> hdmi_present_sense() should be protected in the mutex, too...)
> >>>>>>
> >>>>>> BTW, shouldn't hdmi_present_sense() call hdmi_setup_audio_infoframe()
> >>>>>> always
> >>>>>> when eld_changed is true? hdmi_setup_audio_infoframe() does not do its
> >>>>>> stuff if !monitor_present, so if one starts playback before plugging 
> >>>>>> in,
> >>>>>> the channel mappings etc will never be set even when plug-in happens
> >>>>>> (from what I can see, that is -  did not test yet).
> >>>>>>
> >>>>>> Well, there is such a call but currently specific to Haswell because
> >>>>>> it hasn't been tested for others.  Maybe we can simply get rid of
> >>>>>> is_haswell() check there.
> >>>>>
> >>>>> OK, will test later and provide a patch if it works as expected.
> >>>>
> >>>> Thanks.
> >>>
> >>> [dropped stable from Cc]
> >>>
> >>> Anssi, is the lock fix/cleanup patch ready?
> >>>
> >>> I already have patche to extend mutex lock over other places and
> >>> move it into per_pin, but if you have already similar patches, I'll
> >>> take yours, hopefully together with other fixes.
> >>
> >> Nope, haven't had the time to work on it yet (my excuse is that I was at
> >> XBMC DevCon last weekend :) ).
> >>
> >> So feel free to apply yours and I'll work on top of that (at some point
> >> later this week I guess).
> > 
> > Anssi, do you have the merge-ready version of AMD HDMI fix patches?
> > Since 3.13 merge window will be opened soon, I'd like to merge such
> > big (and good) things now if possible.
> 
> Well, I have finished the hdmi_ops conversion, but the updated patchset
> has not yet been tested on ATI/AMD. But I guess I could post the updated
> patchset anyway (today), and fix any possible remaining issues afterwards...

Yeah, I know that the stuff already worked, so it'd be easy to fix
even if something went wrong by rebasing.


Takashi