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

[Harry Wentland]

On 2025-08-01 04:39, Timur Kristóf wrote:
> On Fri, 2025-08-01 at 03:19 -0400, Alexandre Demers wrote:
>>> 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
> 
> Hi Alexandre,
> 
> I think the confusion comes from "RGB" being a very overloaded term in
> this space, so I am in favour of clarifying the name. I am open to
> suggestion as to what is the best clarification. If you want to keep
> the "RGB" part then I propose:
> 
> SIGNAL_TYPE_RGB -> SIGNAL_TYPE_ANALOG_RGB
> 
> Which should make it very clear what it is.
> 
> Otherwise, I would like to apply Harry's suggestion to name the new
> helper function dc_is_analog_singal. Considering we don't support other
> types of analog signals, I don't think there is any confusion with
> that.
> 
> Let me know what you think.

After going through the entire series I'm not so sure
it only makes sense to rename this function to _analog_.

Either rename all of SIGNAL_TYPE_RGB (like you suggest)
or leave it all as RGB. The former creates a whole bunch
of churn and it might make sense to just leave things as
RGB. My confusion came from the fact that I've spent a
lot of time in the world of color spaces and over the
years have forgotten our terms for analog connectors.

So, no strong opinion from me. Maybe slightly in favor
of avoiding churn.

Harry

> 
> Timur
> 
> 
> 
>>>> Thanks,
>>>> Timur
>>>>
>>>>
>>>>>
>>>>>>>> +{
>>>>>>>> +	return (signal == SIGNAL_TYPE_RGB);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>  static inline bool dc_is_tmds_signal(enum signal_type
>>>>>>>> signal)
>>>>>>>>  {
>>>>>>>>  	switch (signal) {
>>