[PATCH v2 2/3] drm/msm/dp: do not re initialize of audio_comp at display_disable()

[PATCH v2 2/3] drm/msm/dp: do not re initialize of audio_comp at display_disable()

[Kuogee Hsieh]

At dongle unplug, dp initializes audio_comp followed by sending disconnect
event notification to audio and to make sure audio had shutdown completely
by wait for audio completion notification at display_disable(). This patch
will not re initialize audio_comp at display_disable() if audio shutdown
is triggered by dongle unplugged.

Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 0ba71c7..1d71c95 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -894,8 +894,10 @@ static int dp_display_disable(struct dp_display_private *dp, u32 data)
 	/* wait only if audio was enabled */
 	if (dp_display->audio_enabled) {
 		/* signal the disconnect event */
-		reinit_completion(&dp->audio_comp);
-		dp_display_handle_plugged_change(dp_display, false);
+		if (dp->hpd_state != ST_DISCONNECT_PENDING) {
+			reinit_completion(&dp->audio_comp);
+			dp_display_handle_plugged_change(dp_display, false);
+		}
 		if (!wait_for_completion_timeout(&dp->audio_comp,
 				HZ * 5))
 			DRM_ERROR("audio comp timeout\n");
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 2/3] drm/msm/dp: do not re initialize of audio_comp at display_disable()

[Stephen Boyd]

Quoting Kuogee Hsieh (2021-04-13 16:11:30)
> At dongle unplug, dp initializes audio_comp followed by sending disconnect
> event notification to audio and to make sure audio had shutdown completely
> by wait for audio completion notification at display_disable(). This patch

Is this dp_display_disable()? Doubtful that display_disable() is the
function we're talking about.

> will not re initialize audio_comp at display_disable() if audio shutdown
> is triggered by dongle unplugged.

This commit text seems to say the why before the what, where why is "dp
initializes audio_comp followed by sending disconnect.." and the what is
"this patch will no re-initialized audio_comp...". Can you reorder this
so the what comes before the why?

> 
> Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/dp/dp_display.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 0ba71c7..1d71c95 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -894,8 +894,10 @@ static int dp_display_disable(struct dp_display_private *dp, u32 data)
>         /* wait only if audio was enabled */
>         if (dp_display->audio_enabled) {
>                 /* signal the disconnect event */
> -               reinit_completion(&dp->audio_comp);
> -               dp_display_handle_plugged_change(dp_display, false);
> +               if (dp->hpd_state != ST_DISCONNECT_PENDING) {
> +                       reinit_completion(&dp->audio_comp);

Why is this reinitialized here at all? Wouldn't it make more sense to
initialize the completion once at cable plug in and then not initialize
the completion anywhere else? Or initialize the completion whenever
dp_display->audio_enabled is set to true and then only wait for the
completion here if that boolean is true? Or initialize the completion
when dp_display_handle_plugged_change() is passed true for the 'plugged'
argument?

I started reading the code and quickly got lost figuring out how
dp_display_handle_plugged_change() worked and the interaction between
the dp display code and the audio codec embedded in here. There seem to
be a couple of conditions that cut off things early, like
dp_display->audio_enabled and audio->engine_on. Why? Why does
dp_display_signal_audio_complete() call complete_all() vs. just
complete()? Please help! :(

> +                       dp_display_handle_plugged_change(dp_display, false);

I think it's this way because dp_hpd_unplug_handle() is the function
that sets the hpd_state to ST_DISCONNECT_PENDING and then reinitializes
the completion (why?) and calls dp_display_handle_plugged_change(). So
the commit text could say that reinitializing the completion again here
at dp_display_disable() is racing with the audio code in the case that
dp_hpd_unplug_handle() already called
dp_display_handle_plugged_change() and it would make more sense. But the
question still stands why that race even exists in the first place vs.
initializing the completion variable in only one place unconditionally
when the cable is connected, in dp_hpd_plug_handle() or
dp_display_post_enable().

> +               }
>                 if (!wait_for_completion_timeout(&dp->audio_comp,
>                                 HZ * 5))
>                         DRM_ERROR("audio comp timeout\n");

Re: [PATCH v2 2/3] drm/msm/dp: do not re initialize of audio_comp at display_disable()

[khsieh]

On 2021-04-13 20:17, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2021-04-13 16:11:30)
>> At dongle unplug, dp initializes audio_comp followed by sending 
>> disconnect
>> event notification to audio and to make sure audio had shutdown 
>> completely
>> by wait for audio completion notification at display_disable(). This 
>> patch
> 
> Is this dp_display_disable()? Doubtful that display_disable() is the
> function we're talking about.
yes
> 
>> will not re initialize audio_comp at display_disable() if audio 
>> shutdown
>> is triggered by dongle unplugged.
> 
> This commit text seems to say the why before the what, where why is "dp
> initializes audio_comp followed by sending disconnect.." and the what 
> is
> "this patch will no re-initialized audio_comp...". Can you reorder this
> so the what comes before the why?
> 
ok
>> 
>> Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>
>> ---
>>  drivers/gpu/drm/msm/dp/dp_display.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
>> b/drivers/gpu/drm/msm/dp/dp_display.c
>> index 0ba71c7..1d71c95 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -894,8 +894,10 @@ static int dp_display_disable(struct 
>> dp_display_private *dp, u32 data)
>>         /* wait only if audio was enabled */
>>         if (dp_display->audio_enabled) {
>>                 /* signal the disconnect event */
>> -               reinit_completion(&dp->audio_comp);
>> -               dp_display_handle_plugged_change(dp_display, false);
>> +               if (dp->hpd_state != ST_DISCONNECT_PENDING) {
>> +                       reinit_completion(&dp->audio_comp);
> 
> Why is this reinitialized here at all? Wouldn't it make more sense to
> initialize the completion once at cable plug in and then not initialize
> the completion anywhere else? Or initialize the completion whenever
> dp_display->audio_enabled is set to true and then only wait for the
> completion here if that boolean is true? Or initialize the completion
> when dp_display_handle_plugged_change() is passed true for the 
> 'plugged'
> argument?
> yes, i think it is better approach, this will take care of both unplug 
> and suspend.

> I started reading the code and quickly got lost figuring out how
> dp_display_handle_plugged_change() worked and the interaction between
> the dp display code and the audio codec embedded in here. There seem to
> be a couple of conditions that cut off things early, like
> dp_display->audio_enabled and audio->engine_on. Why? Why does
> dp_display_signal_audio_complete() call complete_all() vs. just
> complete()? Please help! :(
> 
>> +                       dp_display_handle_plugged_change(dp_display, 
>> false);
> 
> I think it's this way because dp_hpd_unplug_handle() is the function
> that sets the hpd_state to ST_DISCONNECT_PENDING and then reinitializes
> the completion (why?) and calls dp_display_handle_plugged_change(). So
> the commit text could say that reinitializing the completion again here
> at dp_display_disable() is racing with the audio code in the case that
> dp_hpd_unplug_handle() already called
> dp_display_handle_plugged_change() and it would make more sense. But 
> the
> question still stands why that race even exists in the first place vs.
> initializing the completion variable in only one place unconditionally
> when the cable is connected, in dp_hpd_plug_handle() or
> dp_display_post_enable().
> 
>> +               }
>>                 if (!wait_for_completion_timeout(&dp->audio_comp,
>>                                 HZ * 5))
>>                         DRM_ERROR("audio comp timeout\n");