Hi,
Following the report from Sudip Mukherjee, the previous version of that patch got reverted until a fix was found.
While it's not clear yet why we end up in that situation, the culprit is that the original patch in its ASoC prepare hook was calling the vc4_hdmi_set_n_cts function that in turned relied on the connector->state->crtc pointer being non-NULL.
However, no particular caution was being done to make sure that was the case, eventually leading to a NULL pointer dereference under the "right" circumstances.
We did however had some checks for the pointers sanity in the original patch, but they were only enforced when the device was opened, and we were only checking for the connector->state pointer.
The fix is then two-fold: First, we check that we can actually perform audio operations in both startup and prepare, since the situation could have changed between the time the device was opened and the time when we actually start streaming. Then, the encoder->crtc conversion patch has been changed to check on connector->state->crtc as well in that sanity check to avoid dereferencing it if it's NULL.
Let me know what you think, Maxime
Maxime Ripard (2): drm/vc4: hdmi: Check the device state in prepare() drm/vc4: hdmi: Remove drm_encoder->crtc usage
drivers/gpu/drm/vc4/vc4_hdmi.c | 75 ++++++++++++++++++++++++++-------- 1 file changed, 57 insertions(+), 18 deletions(-)
Even though we already check that the encoder->crtc pointer is there during in startup(), which is part of the open() path in ASoC, nothing guarantees that our encoder state won't change between the time when we open the device and the time we prepare it.
Move the sanity checks we do in startup() to a helper and call it from prepare().
Fixes: 91e99e113929 ("drm/vc4: hdmi: Register HDMI codec") Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_hdmi.c | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index b4b4653fe301..74f9df38815e 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -1227,17 +1227,31 @@ static inline struct vc4_hdmi *dai_to_hdmi(struct snd_soc_dai *dai) return snd_soc_card_get_drvdata(card); }
+static bool vc4_hdmi_audio_can_stream(struct vc4_hdmi *vc4_hdmi) +{ + struct drm_encoder *encoder = &vc4_hdmi->encoder.base.base; + + /* + * The encoder doesn't have a CRTC until the first modeset. + */ + if (!encoder->crtc) + return false; + + /* + * If the encoder is currently in DVI mode, treat the codec DAI + * as missing. + */ + if (!(HDMI_READ(HDMI_RAM_PACKET_CONFIG) & VC4_HDMI_RAM_PACKET_ENABLE)) + return false; + + return true; +} + static int vc4_hdmi_audio_startup(struct device *dev, void *data) { struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev); - struct drm_encoder *encoder = &vc4_hdmi->encoder.base.base;
- /* - * If the HDMI encoder hasn't probed, or the encoder is - * currently in DVI mode, treat the codec dai as missing. - */ - if (!encoder->crtc || !(HDMI_READ(HDMI_RAM_PACKET_CONFIG) & - VC4_HDMI_RAM_PACKET_ENABLE)) + if (!vc4_hdmi_audio_can_stream(vc4_hdmi)) return -ENODEV;
vc4_hdmi->audio.streaming = true; @@ -1339,6 +1353,9 @@ static int vc4_hdmi_audio_prepare(struct device *dev, void *data, u32 mai_audio_format; u32 mai_sample_rate;
+ if (!vc4_hdmi_audio_can_stream(vc4_hdmi)) + return -EINVAL; + dev_dbg(dev, "%s: %u Hz, %d bit, %d channels\n", __func__, sample_rate, params->sample_width, channels);
On Fri, Sep 24, 2021 at 2:55 PM Maxime Ripard maxime@cerno.tech wrote:
Even though we already check that the encoder->crtc pointer is there during in startup(), which is part of the open() path in ASoC, nothing guarantees that our encoder state won't change between the time when we open the device and the time we prepare it.
Move the sanity checks we do in startup() to a helper and call it from prepare().
Fixes: 91e99e113929 ("drm/vc4: hdmi: Register HDMI codec") Signed-off-by: Maxime Ripard maxime@cerno.tech
Tested-by: Sudip Mukherjee sudipm.mukherjee@gmail.com
The drm_encoder crtc pointer isn't really fit for an atomic driver, let's rely on the connector state instead.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_hdmi.c | 50 ++++++++++++++++++++++++---------- 1 file changed, 36 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 74f9df38815e..4bd01178ef9a 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -432,7 +432,7 @@ static void vc4_hdmi_set_avi_infoframe(struct drm_encoder *encoder) struct vc4_hdmi_encoder *vc4_encoder = to_vc4_hdmi_encoder(encoder); struct drm_connector *connector = &vc4_hdmi->connector; struct drm_connector_state *cstate = connector->state; - struct drm_crtc *crtc = encoder->crtc; + struct drm_crtc *crtc = cstate->crtc; const struct drm_display_mode *mode = &crtc->state->adjusted_mode; union hdmi_infoframe frame; int ret; @@ -537,8 +537,11 @@ static bool vc4_hdmi_supports_scrambling(struct drm_encoder *encoder,
static void vc4_hdmi_enable_scrambling(struct drm_encoder *encoder) { - struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode; struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); + struct drm_connector *connector = &vc4_hdmi->connector; + struct drm_connector_state *cstate = connector->state; + struct drm_crtc *crtc = cstate->crtc; + struct drm_display_mode *mode = &crtc->state->adjusted_mode;
if (!vc4_hdmi_supports_scrambling(encoder, mode)) return; @@ -559,17 +562,18 @@ static void vc4_hdmi_enable_scrambling(struct drm_encoder *encoder) static void vc4_hdmi_disable_scrambling(struct drm_encoder *encoder) { struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); - struct drm_crtc *crtc = encoder->crtc; + struct drm_connector *connector = &vc4_hdmi->connector; + struct drm_connector_state *cstate = connector->state;
/* - * At boot, encoder->crtc will be NULL. Since we don't know the + * At boot, connector->state will be NULL. Since we don't know the * state of the scrambler and in order to avoid any * inconsistency, let's disable it all the time. */ - if (crtc && !vc4_hdmi_supports_scrambling(encoder, &crtc->mode)) + if (cstate && !vc4_hdmi_supports_scrambling(encoder, &cstate->crtc->mode)) return;
- if (crtc && !vc4_hdmi_mode_needs_scrambling(&crtc->mode)) + if (cstate && !vc4_hdmi_mode_needs_scrambling(&cstate->crtc->mode)) return;
if (delayed_work_pending(&vc4_hdmi->scrambling_work)) @@ -891,7 +895,9 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder, vc4_hdmi_encoder_get_connector_state(encoder, state); struct vc4_hdmi_connector_state *vc4_conn_state = conn_state_to_vc4_hdmi_conn_state(conn_state); - struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode; + struct drm_crtc_state *crtc_state = + drm_atomic_get_new_crtc_state(state, conn_state->crtc); + struct drm_display_mode *mode = &crtc_state->adjusted_mode; struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); unsigned long bvb_rate, pixel_rate, hsm_rate; int ret; @@ -985,7 +991,11 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder, static void vc4_hdmi_encoder_pre_crtc_enable(struct drm_encoder *encoder, struct drm_atomic_state *state) { - struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode; + struct drm_connector_state *conn_state = + vc4_hdmi_encoder_get_connector_state(encoder, state); + struct drm_crtc_state *crtc_state = + drm_atomic_get_new_crtc_state(state, conn_state->crtc); + struct drm_display_mode *mode = &crtc_state->adjusted_mode; struct vc4_hdmi_encoder *vc4_encoder = to_vc4_hdmi_encoder(encoder); struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
@@ -1008,7 +1018,11 @@ static void vc4_hdmi_encoder_pre_crtc_enable(struct drm_encoder *encoder, static void vc4_hdmi_encoder_post_crtc_enable(struct drm_encoder *encoder, struct drm_atomic_state *state) { - struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode; + struct drm_connector_state *conn_state = + vc4_hdmi_encoder_get_connector_state(encoder, state); + struct drm_crtc_state *crtc_state = + drm_atomic_get_new_crtc_state(state, conn_state->crtc); + struct drm_display_mode *mode = &crtc_state->adjusted_mode; struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); struct vc4_hdmi_encoder *vc4_encoder = to_vc4_hdmi_encoder(encoder); bool hsync_pos = mode->flags & DRM_MODE_FLAG_PHSYNC; @@ -1196,8 +1210,8 @@ 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) { - struct drm_encoder *encoder = &vc4_hdmi->encoder.base.base; - struct drm_crtc *crtc = encoder->crtc; + struct drm_connector *connector = &vc4_hdmi->connector; + struct drm_crtc *crtc = connector->state->crtc; const struct drm_display_mode *mode = &crtc->state->adjusted_mode; u32 n, cts; u64 tmp; @@ -1229,12 +1243,20 @@ static inline struct vc4_hdmi *dai_to_hdmi(struct snd_soc_dai *dai)
static bool vc4_hdmi_audio_can_stream(struct vc4_hdmi *vc4_hdmi) { - struct drm_encoder *encoder = &vc4_hdmi->encoder.base.base; + struct drm_connector *connector = &vc4_hdmi->connector;
/* - * The encoder doesn't have a CRTC until the first modeset. + * The HDMI connector doesn't have a state until + * drm_mode_config_reset() is called. */ - if (!encoder->crtc) + if (!connector->state) + return false; + + /* + * If the connector is disabled, its state won't have a pointer + * to the CRTC. + */ + if (!connector->state->crtc) return false;
/*
On Fri, Sep 24, 2021 at 2:55 PM Maxime Ripard maxime@cerno.tech wrote:
The drm_encoder crtc pointer isn't really fit for an atomic driver, let's rely on the connector state instead.
Signed-off-by: Maxime Ripard maxime@cerno.tech
Tested-by: Sudip Mukherjee sudipm.mukherjee@gmail.com
dri-devel@lists.freedesktop.org