| Message ID | 20250309-drm-hdmi-acr-v1-3-bb9c242f4d4b@linaro.org (mailing list archive) |
|---|---|
| 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.
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) \