Re: [PATCH 3/4] drm/vc4: use new helper to get ACR values

[Maxime Ripard]

On Tue, Mar 11, 2025 at 06:28:50PM +0200, Dmitry Baryshkov wrote:
> On Tue, Mar 11, 2025 at 09:07:10AM +0100, Maxime Ripard wrote:
> > On Mon, Mar 10, 2025 at 10:18:04PM +0200, Dmitry Baryshkov wrote:
> > > On Mon, Mar 10, 2025 at 03:51:53PM +0100, Maxime Ripard wrote:
> > > > On Sun, Mar 09, 2025 at 10:13:58AM +0200, Dmitry Baryshkov wrote:
> > > > > From: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > > > > 
> > > > > Use drm_hdmi_acr_get_n_cts() helper instead of calculating N and CTS
> > > > > values in the VC4 driver.
> > > > > 
> > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > > > > ---
> > > > >  drivers/gpu/drm/vc4/vc4_hdmi.c | 10 +++-------
> > > > >  drivers/gpu/drm/vc4/vc4_hdmi.h |  7 +++++++
> > > > >  2 files changed, 10 insertions(+), 7 deletions(-)
> > > > > 
> > > 
> > > > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
> > > > > index e3d989ca302b72533c374dfa3fd0d5bd7fe64a82..0a775dbfe99d45521f3d0a2016555aefa81d7934 100644
> > > > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.h
> > > > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
> > > > > @@ -211,6 +211,13 @@ struct vc4_hdmi {
> > > > >  	 * KMS hooks. Protected by @mutex.
> > > > >  	 */
> > > > >  	enum hdmi_colorspace output_format;
> > > > > +
> > > > > +	/**
> > > > > +	 * @tmds_char_rate: Copy of
> > > > > +	 * @drm_connector_state.hdmi.tmds_char_rate for use outside of
> > > > > +	 * KMS hooks. Protected by @mutex.
> > > > > +	 */
> > > > > +	unsigned long long tmds_char_rate;
> > > > >  };
> > > > 
> > > > This should be in drm_connector_hdmi if it's useful
> > > 
> > > That would mean bringing the state to a non-state structure on the
> > > framework level. Is it fine from your POV?
> > 
> > Sorry, I'm changing my mind a little bit, but it's pretty much the same
> > case than for accessing the infoframes from debugfs: we want to get some
> > information stored in the state from outside of KMS.
> > 
> > What we did for the infoframes is that we're actually just taking the
> > connection_mutex from the DRM device and access the drm_connector->state
> > pointer.
> > 
> > I guess it would also work for ALSA?
> 
> I'd really prefer to follow the drm_connector.infoframes.audio. It makes
> sense to group all ALSA-related functionality together. Maybe I should
> refactor it to:

That's the thing though: the tmds_char_rate has nothing to do with ALSA.
It's useful to derive the parameters, but KMS controls it, it's part of
its state, and that's where it belongs.

Just like any infoframe but the audio one.

Maxime

Attachment: signature.asc
Description: PGP signature