| Message ID | 20250309-drm-hdmi-acr-v1-3-bb9c242f4d4b@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | drm/display: hdmi: provide common code to get Audio Clock Recovery params | expand |
On Sun, Mar 09, 2025 at 10:13:58AM +0200, Dmitry Baryshkov wrote: > From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > 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@linaro.org> > --- > 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.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > index 37238a12baa58a06a5d6f40d1ab64abc7fac60d7..f24bcc2f3a2ac39aaea061b809940978341472f4 100644 > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c > @@ -1637,6 +1637,7 @@ static void vc4_hdmi_encoder_atomic_mode_set(struct drm_encoder *encoder, > &crtc_state->adjusted_mode); > vc4_hdmi->output_bpc = conn_state->hdmi.output_bpc; > vc4_hdmi->output_format = conn_state->hdmi.output_format; > + vc4_hdmi->tmds_char_rate = conn_state->hdmi.tmds_char_rate; > mutex_unlock(&vc4_hdmi->mutex); > } > > @@ -1829,17 +1830,12 @@ static void vc4_hdmi_audio_set_mai_clock(struct vc4_hdmi *vc4_hdmi, > > static void vc4_hdmi_set_n_cts(struct vc4_hdmi *vc4_hdmi, unsigned int samplerate) > { > - const struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode; > - u32 n, cts; > - u64 tmp; > + unsigned int n, cts; > > lockdep_assert_held(&vc4_hdmi->mutex); > lockdep_assert_held(&vc4_hdmi->hw_lock); > > - n = 128 * samplerate / 1000; > - tmp = (u64)(mode->clock * 1000) * n; > - do_div(tmp, 128 * samplerate); > - cts = tmp; > + drm_hdmi_acr_get_n_cts(vc4_hdmi->tmds_char_rate, samplerate, &n, &cts); > > HDMI_WRITE(HDMI_CRP_CFG, > VC4_HDMI_CRP_CFG_EXTERNAL_CTS_EN | > 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 Maxime
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@linaro.org> > > > > 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@linaro.org> > > --- > > 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? Is it also fine to use drm_connector.mutex for protecting this? Or should we be using something like drm_connector_hdmi.infoframes.mutex (maybe after moving it from .infoframes to the top level)?
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@linaro.org> > > > > > > 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@linaro.org> > > > --- > > > 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? Maxime
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@linaro.org> > > > > > > > > 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@linaro.org> > > > > --- > > > > 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: struct drm_connector { struct { struct mutex lock; struct drm_connector_hdmi_infoframe audio_infoframe; unsigned long long tmds_char_rate; } audio; }; WDYT? If that doesn't sound appealing, I'll go the connetion_mutex and drm_connector_state way.
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@linaro.org> > > > > > > > > > > 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@linaro.org> > > > > > --- > > > > > 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
On Fri, Mar 14, 2025 at 02:46:09PM +0100, Maxime Ripard wrote: > 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@linaro.org> > > > > > > > > > > > > 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@linaro.org> > > > > > > --- > > > > > > 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. Ack, I'll take a look.
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 37238a12baa58a06a5d6f40d1ab64abc7fac60d7..f24bcc2f3a2ac39aaea061b809940978341472f4 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -1637,6 +1637,7 @@ static void vc4_hdmi_encoder_atomic_mode_set(struct drm_encoder *encoder, &crtc_state->adjusted_mode); vc4_hdmi->output_bpc = conn_state->hdmi.output_bpc; vc4_hdmi->output_format = conn_state->hdmi.output_format; + vc4_hdmi->tmds_char_rate = conn_state->hdmi.tmds_char_rate; mutex_unlock(&vc4_hdmi->mutex); } @@ -1829,17 +1830,12 @@ static void vc4_hdmi_audio_set_mai_clock(struct vc4_hdmi *vc4_hdmi, static void vc4_hdmi_set_n_cts(struct vc4_hdmi *vc4_hdmi, unsigned int samplerate) { - const struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode; - u32 n, cts; - u64 tmp; + unsigned int n, cts; lockdep_assert_held(&vc4_hdmi->mutex); lockdep_assert_held(&vc4_hdmi->hw_lock); - n = 128 * samplerate / 1000; - tmp = (u64)(mode->clock * 1000) * n; - do_div(tmp, 128 * samplerate); - cts = tmp; + drm_hdmi_acr_get_n_cts(vc4_hdmi->tmds_char_rate, samplerate, &n, &cts); HDMI_WRITE(HDMI_CRP_CFG, VC4_HDMI_CRP_CFG_EXTERNAL_CTS_EN | 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; }; #define connector_to_vc4_hdmi(_connector) \