[PATCH] ALSA: hda - Remove temporary buffer for ELD update

[PATCH] ALSA: hda - Remove temporary buffer for ELD update

[Hyungwon Hwang]

With the introduce of pcm_lock and per_pin->lock, this temporary buffer
is not needed anymore.

Signed-off-by: Hyungwon Hwang <hyungwon.hwang7@gmail.com>
---
 sound/pci/hda/patch_hdmi.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 5af372d..a24d5b4 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -1395,8 +1395,7 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
 	struct hda_jack_tbl *jack;
 	struct hda_codec *codec = per_pin->codec;
 	struct hdmi_spec *spec = codec->spec;
-	struct hdmi_eld *eld = &spec->temp_eld;
-	struct hdmi_eld *pin_eld = &per_pin->sink_eld;
+	struct hdmi_eld *eld = &per_pin->sink_eld;
 	hda_nid_t pin_nid = per_pin->pin_nid;
 	/*
 	 * Always execute a GetPinSense verb here, even when called from
@@ -1413,15 +1412,15 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
 	present = snd_hda_pin_sense(codec, pin_nid);
 
 	mutex_lock(&per_pin->lock);
-	pin_eld->monitor_present = !!(present & AC_PINSENSE_PRESENCE);
-	if (pin_eld->monitor_present)
+	eld->monitor_present = !!(present & AC_PINSENSE_PRESENCE);
+	if (eld->monitor_present)
 		eld->eld_valid  = !!(present & AC_PINSENSE_ELDV);
 	else
 		eld->eld_valid = false;
 
 	codec_dbg(codec,
 		"HDMI status: Codec=%d Pin=%d Presence_Detect=%d ELD_Valid=%d\n",
-		codec->addr, pin_nid, pin_eld->monitor_present, eld->eld_valid);
+		codec->addr, pin_nid, eld->monitor_present, eld->eld_valid);
 
 	if (eld->eld_valid) {
 		if (spec->ops.pin_get_eld(codec, pin_nid, eld->eld_buffer,
@@ -1441,7 +1440,7 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
 	else
 		update_eld(codec, per_pin, eld);
 
-	ret = !repoll || !pin_eld->monitor_present || pin_eld->eld_valid;
+	ret = !repoll || !eld->monitor_present || eld->eld_valid;
 
 	jack = snd_hda_jack_tbl_get(codec, pin_nid);
 	if (jack)
-- 
2.5.0

Re: [PATCH] ALSA: hda - Remove temporary buffer for ELD update

[Takashi Iwai]

On Mon, 11 Apr 2016 19:10:51 +0200,
Hyungwon Hwang wrote:
> 
> With the introduce of pcm_lock and per_pin->lock, this temporary buffer
> is not needed anymore.

The use of a temporary buffer is not only to avoid the race.  It's
also for avoiding the corruption due to an erroneous read.  That is,
pin_get_eld() might overwrite some bogus data while it returns an
error.  Since the driver retries upon the error, we need to keep the
old good data as much as possible.


thanks,

Takashi

> 
> Signed-off-by: Hyungwon Hwang <hyungwon.hwang7@gmail.com>
> ---
>  sound/pci/hda/patch_hdmi.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index 5af372d..a24d5b4 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -1395,8 +1395,7 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
>  	struct hda_jack_tbl *jack;
>  	struct hda_codec *codec = per_pin->codec;
>  	struct hdmi_spec *spec = codec->spec;
> -	struct hdmi_eld *eld = &spec->temp_eld;
> -	struct hdmi_eld *pin_eld = &per_pin->sink_eld;
> +	struct hdmi_eld *eld = &per_pin->sink_eld;
>  	hda_nid_t pin_nid = per_pin->pin_nid;
>  	/*
>  	 * Always execute a GetPinSense verb here, even when called from
> @@ -1413,15 +1412,15 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
>  	present = snd_hda_pin_sense(codec, pin_nid);
>  
>  	mutex_lock(&per_pin->lock);
> -	pin_eld->monitor_present = !!(present & AC_PINSENSE_PRESENCE);
> -	if (pin_eld->monitor_present)
> +	eld->monitor_present = !!(present & AC_PINSENSE_PRESENCE);
> +	if (eld->monitor_present)
>  		eld->eld_valid  = !!(present & AC_PINSENSE_ELDV);
>  	else
>  		eld->eld_valid = false;
>  
>  	codec_dbg(codec,
>  		"HDMI status: Codec=%d Pin=%d Presence_Detect=%d ELD_Valid=%d\n",
> -		codec->addr, pin_nid, pin_eld->monitor_present, eld->eld_valid);
> +		codec->addr, pin_nid, eld->monitor_present, eld->eld_valid);
>  
>  	if (eld->eld_valid) {
>  		if (spec->ops.pin_get_eld(codec, pin_nid, eld->eld_buffer,
> @@ -1441,7 +1440,7 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
>  	else
>  		update_eld(codec, per_pin, eld);
>  
> -	ret = !repoll || !pin_eld->monitor_present || pin_eld->eld_valid;
> +	ret = !repoll || !eld->monitor_present || eld->eld_valid;
>  
>  	jack = snd_hda_jack_tbl_get(codec, pin_nid);
>  	if (jack)
> -- 
> 2.5.0
> 
> 

Re: [PATCH] ALSA: hda - Remove temporary buffer for ELD update

[Hyungwon Hwang]

Dear Takashi,

2016년 04월 12일 18:32에 Takashi Iwai 이(가) 쓴 글:
> On Mon, 11 Apr 2016 19:10:51 +0200,
> Hyungwon Hwang wrote:
>>
>> With the introduce of pcm_lock and per_pin->lock, this temporary buffer
>> is not needed anymore.
> 
> The use of a temporary buffer is not only to avoid the race.  It's
> also for avoiding the corruption due to an erroneous read.  That is,
> pin_get_eld() might overwrite some bogus data while it returns an
> error.  Since the driver retries upon the error, we need to keep the
> old good data as much as possible.

I missed that pin_get_eld() can ruin the structure. I agree with you.
Then I will send another patch which set eld->monitor_present by the
value of pin_eld->monitor_present. So proc can show the correct current
status.

Thanks,
Hyungwon Hwang

> 
> 
> thanks,
> 
> Takashi
> 
>>
>> Signed-off-by: Hyungwon Hwang <hyungwon.hwang7@gmail.com>
>> ---
>>  sound/pci/hda/patch_hdmi.c | 11 +++++------
>>  1 file changed, 5 insertions(+), 6 deletions(-)
>>
>> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
>> index 5af372d..a24d5b4 100644
>> --- a/sound/pci/hda/patch_hdmi.c
>> +++ b/sound/pci/hda/patch_hdmi.c
>> @@ -1395,8 +1395,7 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
>>  	struct hda_jack_tbl *jack;
>>  	struct hda_codec *codec = per_pin->codec;
>>  	struct hdmi_spec *spec = codec->spec;
>> -	struct hdmi_eld *eld = &spec->temp_eld;
>> -	struct hdmi_eld *pin_eld = &per_pin->sink_eld;
>> +	struct hdmi_eld *eld = &per_pin->sink_eld;
>>  	hda_nid_t pin_nid = per_pin->pin_nid;
>>  	/*
>>  	 * Always execute a GetPinSense verb here, even when called from
>> @@ -1413,15 +1412,15 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
>>  	present = snd_hda_pin_sense(codec, pin_nid);
>>  
>>  	mutex_lock(&per_pin->lock);
>> -	pin_eld->monitor_present = !!(present & AC_PINSENSE_PRESENCE);
>> -	if (pin_eld->monitor_present)
>> +	eld->monitor_present = !!(present & AC_PINSENSE_PRESENCE);
>> +	if (eld->monitor_present)
>>  		eld->eld_valid  = !!(present & AC_PINSENSE_ELDV);
>>  	else
>>  		eld->eld_valid = false;
>>  
>>  	codec_dbg(codec,
>>  		"HDMI status: Codec=%d Pin=%d Presence_Detect=%d ELD_Valid=%d\n",
>> -		codec->addr, pin_nid, pin_eld->monitor_present, eld->eld_valid);
>> +		codec->addr, pin_nid, eld->monitor_present, eld->eld_valid);
>>  
>>  	if (eld->eld_valid) {
>>  		if (spec->ops.pin_get_eld(codec, pin_nid, eld->eld_buffer,
>> @@ -1441,7 +1440,7 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
>>  	else
>>  		update_eld(codec, per_pin, eld);
>>  
>> -	ret = !repoll || !pin_eld->monitor_present || pin_eld->eld_valid;
>> +	ret = !repoll || !eld->monitor_present || eld->eld_valid;
>>  
>>  	jack = snd_hda_jack_tbl_get(codec, pin_nid);
>>  	if (jack)
>> -- 
>> 2.5.0
>>
>>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel