Re: [PATCH 07/20] drm/amd/display: Don't use stereo sync and audio on RGB signals

[Alexandre Demers <alexandre.f.demers@gmail.com>]

>On 2025-07-30 13:08, Timur Kristóf wrote:
>> On Wed, 2025-07-30 at 10:34 -0400, Harry Wentland wrote:
>>>
>>>
>>> On 2025-07-30 04:19, Timur Kristóf wrote:
>>>> On Tue, 2025-07-29 at 14:21 -0400, Harry Wentland wrote:
>>>>>
>>>>>
>>>>> On 2025-07-23 11:58, Timur Kristóf wrote:
>>>>>> Features like stereo sync and audio are not supported by RGB
>>>>>> signals, so don't try to use them.
>>>>>>
>>>>>
>>>>> Where does it say that?
>>>>>
>>>>> Harry
>>>>
>>>> 1. Audio
>>>>
>>>> VGA ports (and the analog part of DVI-I ports) simply cannot carry
>>>> audio. So there is no hardware to control any audio, therefore
>>>> there is
>>>> nothing for this code to enable, which is why I added those ifs to
>>>> not
>>>> even try to enable audio on analog video signals.
>>>>
>>>
>>> My bad, I was thinking RGB as opposed to YCbCr. Forgot that we use
>>> RGB signal to refer to VGA.
>> 
>> Sorry for the confusion.
>> 
>>>
>>>> As a side note, DVI-D ports (and the digital part of DVI-I ports)
>>>> may
>>>> have a non-standard extension to carry digital audio signals, but
>>>> that
>>>> is not revelant to supporting analog displays.
>>>>
>>>> 2. Stereo sync
>>>>
>>>> With regards to stereo sync, I didn't find any reference to this in
>>>> the
>>>> legacy display code, so I assumed either it is unsupported or the
>>>> VBIOS
>>>> already sets it up correctly. At least, considering that the legacy
>>>> code didn't bother setting it up, we don't lose any functionality
>>>> if we
>>>> leave it out of DC as well.
>>>>
>>>> That being said, upon some further digging in the DCE register
>>>> files, I
>>>> found a register called DAC_STEREOSYNC_SELECT so maybe I could
>>>> investigate using that. Maybe it would be better to work with the
>>>> registers directly instead of the VBIOS? Would it be okay to
>>>> investigate that further in a future patch series once this one is
>>>> merged?
>>>>
>>>
>>> I don't think DC supports stereo sync currently. I'm not sure there
>>> is
>>> much value in pursuing that.
>> 
>> If stereo sync is not supported, what does setup_stereo_sync() do?
>> 
> 
> My bad. Not sure then. But no objection if you want to explore it.
> 
> Harry
> >>>
>>>>>> diff --git a/drivers/gpu/drm/amd/display/include/signal_types.h
>>>>>> b/drivers/gpu/drm/amd/display/include/signal_types.h
>>>>>> index a10d6b988aab..825a08fcb125 100644
>>>>>> --- a/drivers/gpu/drm/amd/display/include/signal_types.h
>>>>>> +++ b/drivers/gpu/drm/amd/display/include/signal_types.h
>>>>>> @@ -118,6 +118,11 @@ static inline bool dc_is_dvi_signal(enum
>>>>>> signal_type signal)
>>>>>>  	}
>>>>>>  }
>>>>>>  
>>>>>> +static inline bool dc_is_rgb_signal(enum signal_type signal)
>>>
>>> To avoid confusion with people that haven't worked on analog
>>> signals in years (or ever) it might be better to name this
>>> dc_is_analog_signal.
>>>
>>> Harry
>> 
>> Sounds good, I'll rename it in the next version of the series.
>> To further ease the confusion, what do you think about renaming
>> SIGNAL_TYPE_RGB to SIGNAL_TYPE_ANALOG?
I think Harry hasn't answered your proposition. I must say that the 
first time I looked for VGA in the legacy code, I stumbled upon the RGB 
usage. But then, it began to make sense (I'm not completely sure if 
signals and connector types are used properly everywhere), because we 
are mostly translating DRM signal types to supported connector 
types.That being said, while both dc_is_rgb_signal() and 
dc_is_analog_signal() could be used here, we are specifically querying 
the signal type and this signal type is RGB. Because of this, I would be 
in favor of keeping dc_is_rgb_signal() unless there is another analog 
type that could be queried and not rename SIGNAL_TYPE_RGB to 
SIGNAL_TYPE_ANALOG in any case. Cheers, Alexandre >>
>> Thanks,
>> Timur
>> 
>> 
>>>
>>>>>> +{
>>>>>> +	return (signal == SIGNAL_TYPE_RGB);
>>>>>> +}
>>>>>> +
>>>>>>  static inline bool dc_is_tmds_signal(enum signal_type signal)
>>>>>>  {
>>>>>>  	switch (signal) {