Hi Maxime
On Fri, 7 May 2021 at 16:05, Maxime Ripard <maxime@xxxxxxxxxx> wrote:
>
> The vc4_get_crtc_encoder function currently only works when the
> connector->state->crtc pointer is set, which is only true when the
> connector is currently enabled.
>
> However, we use it as part of the disable path as well, and our lookup
> will fail in that case, resulting in it returning a null pointer we
> can't act on.
>
> We can access the connector that used to be connected to that crtc
> though using the old connector state in the disable path.
>
> Since we want to support both the enable and disable path, we can
> support it by passing the state accessor variant as a function pointer,
> together with the atomic state.
>
> Fixes: 792c3132bc1b ("drm/vc4: encoder: Add finer-grained encoder callbacks")
> Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx>
Reviewed-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/vc4/vc4_crtc.c | 21 ++++++++++++++++-----
> 1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> index 8a19d6c76605..36ea684a349b 100644
> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> @@ -262,14 +262,22 @@ static u32 vc4_crtc_get_fifo_full_level_bits(struct vc4_crtc *vc4_crtc,
> * allows drivers to push pixels to more than one encoder from the
> * same CRTC.
> */
> -static struct drm_encoder *vc4_get_crtc_encoder(struct drm_crtc *crtc)
> +static struct drm_encoder *vc4_get_crtc_encoder(struct drm_crtc *crtc,
> + struct drm_atomic_state *state,
> + struct drm_connector_state *(*get_state)(struct drm_atomic_state *state,
> + struct drm_connector *connector))
> {
> struct drm_connector *connector;
> struct drm_connector_list_iter conn_iter;
>
> drm_connector_list_iter_begin(crtc->dev, &conn_iter);
> drm_for_each_connector_iter(connector, &conn_iter) {
> - if (connector->state->crtc == crtc) {
> + struct drm_connector_state *conn_state = get_state(state, connector);
> +
> + if (!conn_state)
> + continue;
> +
> + if (conn_state->crtc == crtc) {
> drm_connector_list_iter_end(&conn_iter);
> return connector->encoder;
> }
> @@ -292,7 +300,8 @@ static void vc4_crtc_config_pv(struct drm_crtc *crtc, struct drm_atomic_state *s
> {
> struct drm_device *dev = crtc->dev;
> struct vc4_dev *vc4 = to_vc4_dev(dev);
> - struct drm_encoder *encoder = vc4_get_crtc_encoder(crtc);
> + struct drm_encoder *encoder = vc4_get_crtc_encoder(crtc, state,
> + drm_atomic_get_new_connector_state);
> struct vc4_encoder *vc4_encoder = to_vc4_encoder(encoder);
> struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
> const struct vc4_pv_data *pv_data = vc4_crtc_to_vc4_pv_data(vc4_crtc);
> @@ -407,7 +416,8 @@ static int vc4_crtc_disable(struct drm_crtc *crtc,
> struct drm_atomic_state *state,
> unsigned int channel)
> {
> - struct drm_encoder *encoder = vc4_get_crtc_encoder(crtc);
> + struct drm_encoder *encoder = vc4_get_crtc_encoder(crtc, state,
> + drm_atomic_get_old_connector_state);
> struct vc4_encoder *vc4_encoder = to_vc4_encoder(encoder);
> struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
> struct drm_device *dev = crtc->dev;
> @@ -507,7 +517,8 @@ static void vc4_crtc_atomic_enable(struct drm_crtc *crtc,
> {
> struct drm_device *dev = crtc->dev;
> struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
> - struct drm_encoder *encoder = vc4_get_crtc_encoder(crtc);
> + struct drm_encoder *encoder = vc4_get_crtc_encoder(crtc, state,
> + drm_atomic_get_new_connector_state);
> struct vc4_encoder *vc4_encoder = to_vc4_encoder(encoder);
>
> require_hvs_enabled(dev);
> --
> 2.31.1
>
