Re: [PATCH 06/18] drm/vc4: Rework the encoder retrieval code

Hi

Am 17.03.21 um 16:43 schrieb Maxime Ripard:

Due to a FIFO that cannot be flushed between the pixelvalve and the HDMI controllers on BCM2711, we need to carefully disable both at boot time if they were left enabled by the firmware.

However, at the time we're running that code, the struct drm_connector encoder pointer isn't set yet, and thus we cannot retrieve the encoder associated to our CRTC.

We can however make use of the fact that we have a less flexible setup than what DRM allows where we have a 1:1 relationship between our CRTCs and encoders (and connectors), and thus store the crtc associated to our encoder at boot time.

We cannot do that at the time the encoders are probed though, since the CRTCs won't be probed yet and thus we don't know at that time which CRTC index we're going to get, so let's do this in two passes: we can first bind all the components and then once they all are bound, we can iterate over all the encoders to find their associated CRTC and set the pointer.

This is similar to what we're doing to set the possible_crtcs field.

Fixes: 875a4d536842 ("drm/vc4: drv: Disable the CRTC at boot time")

Cc: stable@vger.kernel.org # v5.10+

Signed-off-by: Maxime Ripard maxime@cerno.tech

drivers/gpu/drm/vc4/vc4_crtc.c | 25 +++++++++++++++++++++-- drivers/gpu/drm/vc4/vc4_drv.c | 36 ++++++++++++++++++++++++++++++++++ drivers/gpu/drm/vc4/vc4_drv.h | 1 + 3 files changed, 60 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index f1f2e8cbce79..e2607e1f2520 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -255,6 +255,19 @@ static u32 vc4_crtc_get_fifo_full_level_bits(struct vc4_crtc *vc4_crtc, PV_CONTROL_FIFO_LEVEL); }

+struct drm_encoder *vc4_get_connector_encoder(struct drm_connector *connector) +{

• struct drm_encoder *encoder;
• if (WARN_ON(hweight32(connector->possible_encoders) != 1))

drm_WARN_ON

• return NULL;
  

• drm_connector_for_each_possible_encoder(connector, encoder)

• return encoder;
  

• return NULL;

+}

• /*
  • Returns the encoder attached to the CRTC.

@@ -269,9 +282,17 @@ static struct drm_encoder *vc4_get_crtc_encoder(struct drm_crtc *crtc)

drm_connector_list_iter_begin(crtc->dev, &conn_iter); drm_for_each_connector_iter(connector, &conn_iter) {

• if (connector->state->crtc == crtc) {
  

• struct drm_encoder *encoder;
  

• struct vc4_encoder *vc4_encoder;
  

• encoder = vc4_get_connector_encoder(connector);
  

• if (!encoder)
  

• 	continue;
  

• vc4_encoder = to_vc4_encoder(encoder);
  

• if (vc4_encoder->crtc == crtc) {
  drm_connector_list_iter_end(&conn_iter);
  

• 	return connector->encoder;
  

• 	return encoder;
  

  } } drm_connector_list_iter_end(&conn_iter);

diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c index 556ad0f02a0d..cd1fb75c66a7 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.c +++ b/drivers/gpu/drm/vc4/vc4_drv.c @@ -199,6 +199,41 @@ static int compare_dev(struct device *dev, void *data) return dev == data; }

+static struct drm_crtc *vc4_drv_find_crtc(struct drm_device *drm,

• 			  struct drm_encoder *encoder)
  

+{

• struct drm_crtc *crtc;
• if (WARN_ON(hweight32(encoder->possible_crtcs) != 1))

• return NULL;
  

• drm_for_each_crtc(crtc, drm) {

• if (!drm_encoder_crtc_ok(encoder, crtc))
  

• 	continue;
  

• return crtc;
  

• }
• return NULL;

+}

+static void vc4_drv_set_encoder_data(struct drm_device *drm) +{

• struct drm_encoder *encoder;
• drm_for_each_encoder(encoder, drm) {

• struct vc4_encoder *vc4_encoder;
  

• struct drm_crtc *crtc;
  

• crtc = vc4_drv_find_crtc(drm, encoder);
  

• if (WARN_ON(!crtc))
  

• 	return;
  

• vc4_encoder = to_vc4_encoder(encoder);
  

• vc4_encoder->crtc = crtc;
  

• }

+}

• static void vc4_match_add_drivers(struct device *dev, struct component_match **match, struct platform_driver *const *drivers,

@@ -261,6 +296,7 @@ static int vc4_drm_bind(struct device *dev) ret = component_bind_all(dev, drm); if (ret) return ret;

• vc4_drv_set_encoder_data(drm);

  ret = vc4_plane_create_additional_planes(drm); if (ret)

diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h index a7500716cf3f..1b569dcc2154 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.h +++ b/drivers/gpu/drm/vc4/vc4_drv.h @@ -438,6 +438,7 @@ enum vc4_encoder_type {

struct vc4_encoder { struct drm_encoder base;

• struct drm_crtc *crtc;

I'd probably deserves a comment why this is explicitly stored here.

enum vc4_encoder_type type; u32 clock_select;