Re: [Freedreno] [PATCH] drm/msm/dp: do not notify audio subsystem if sink doesn't support audio

[abhinavk@codeaurora.org]

Hi Stephen

Thanks for the review.

On 2020-11-02 13:19, Stephen Boyd wrote:
> Quoting Abhinav Kumar (2020-10-29 13:55:09)
>> For sinks that do not support audio, there is no need to notify
>> audio subsystem of the connection event.
>> 
>> This will make sure that audio routes only to the primary display
>> when connected to such sinks.
>> 
> 
> Does this need a Fixes tag? Or it's just an optimization patch?
This is an unhandled corner case ( VGA dongle ) for DP audio and will 
make
sure we do not switch audio output from primary to external when 
connected to
a sink which does not support audio.
I thought of adding a fixes tag pointing to 
https://patchwork.freedesktop.org/patch/390236/.
But at the same time, thought this can go in as a standlone patch as 
well.
If you think its required, I will add the fixes tag pointing to the base 
audio patch.
> 
>> Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
>> ---
>>  drivers/gpu/drm/msm/dp/dp_display.c | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
>> b/drivers/gpu/drm/msm/dp/dp_display.c
>> index 4a5735564be2..d970980b0ca5 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -555,8 +555,16 @@ static int dp_connect_pending_timeout(struct 
>> dp_display_private *dp, u32 data)
>>  static void dp_display_handle_plugged_change(struct msm_dp 
>> *dp_display,
>>                 bool plugged)
>>  {
>> -       if (dp_display->plugged_cb && dp_display->codec_dev)
>> -               dp_display->plugged_cb(dp_display->codec_dev, 
>> plugged);
>> +       struct dp_display_private *dp;
>> +
>> +       dp = container_of(g_dp_display,
> 
> What is g_dp_display? I guess this doesn't compile?
g_dp_display is the global dp_display pointer in the dp_display.c file. 
It does compile.
> 
>> +                       struct dp_display_private, dp_display);
>> +
>> +       if (dp_display->plugged_cb && dp_display->codec_dev) {
>> +               /* notify audio subsystem only if sink supports audio 
>> */
>> +               if (dp->audio_supported)
> 
> Can we combine this into the above if statement?
> 
>> +                       dp_display->plugged_cb(dp_display->codec_dev, 
>> plugged);
> 
> Then this isn't as nested.
Ok, will do ...
> 
>> +       }
>>  }
>> 
>>  static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 
>> data)
> _______________________________________________
> Freedreno mailing list
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno