Re: [PATCH v4 04/12] drm/vc4: crtc: Fix vc4_get_crtc_encoder logic

[Dave Stevenson]

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
>