[PATCH 0/2] drm/vc4: hdmi: Get rid of encoder->crtc, take 2

[PATCH 0/2] drm/vc4: hdmi: Get rid of encoder->crtc, take 2

[Maxime Ripard]

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(-)

-- 
2.31.1

[PATCH 1/2] drm/vc4: hdmi: Check the device state in prepare()

[Maxime Ripard]

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);
 
-- 
2.31.1

Re: [PATCH 1/2] drm/vc4: hdmi: Check the device state in prepare()

[Sudip Mukherjee]

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>


-- 
Regards
Sudip

[PATCH 2/2] drm/vc4: hdmi: Remove drm_encoder->crtc usage

[Maxime Ripard]

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;
 
 	/*
-- 
2.31.1

Re: [PATCH 2/2] drm/vc4: hdmi: Remove drm_encoder->crtc usage

[Sudip Mukherjee]

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>


-- 
Regards
Sudip