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

Fri Aug 1 15:09:45 UTC 2025

On Fri, 2025-08-01 at 10:55 -0400, Harry Wentland wrote:
> 
> 
> 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

Hi Harry,

It sounds like both Alexandre and yourself are slightly in favour of
keeping the old names, SIGNAL_TYPE_RGB and dc_is_rgb_signal.

So, I'll just leave them as they are.
I will soon send a v2 for the series addressing the feedback.

Thanks,
Timur

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