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

["Timur Kristóf" <timur.kristof@gmail.com>]

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.

Timur



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