[PATCH] drm/imx: imx-ldb: store LVDS bus configuration in ldb channel

[Ying Liu]

Hi Philipp,

This patch's headline doesn't exactly reflect what the patch actually
does - retrieve lvds bus format from imx_crtc_state during encoder
mode_set.

On Thu, Jul 21, 2016 at 9:25 PM, Philipp Zabel <p.zabel at pengutronix.de> wrote:
> The code in imx_ldb_encoder_mode_set crashes trying to access the
> crtc->state->state drm_atomic_state pointer if that was previously
> cleared by drm_atomic_helper_swap_state.

Nit: Providing a crash log might be good.

> Instead of trying to walk all connectors during encoder mode_set to
> determine the LVDS bus format from panel or external bridge connector
> display info, store it in imx_crtc_state during encoder atomic_check,
> where it is already known, and just retrieve it from imx_crtc_state
> during encoder mode_set.
>
> Fixes: 49f98bc4d44a4 ("drm/imx: store internal bus configuration in crtc state")
> Signed-off-by: Philipp Zabel <p.zabel at pengutronix.de>
> ---
>  drivers/gpu/drm/imx/imx-drm.h |  1 +
>  drivers/gpu/drm/imx/imx-ldb.c | 22 +++++-----------------
>  2 files changed, 6 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/imx/imx-drm.h b/drivers/gpu/drm/imx/imx-drm.h
> index bdaa381..5e2f68a 100644
> --- a/drivers/gpu/drm/imx/imx-drm.h
> +++ b/drivers/gpu/drm/imx/imx-drm.h
> @@ -19,6 +19,7 @@ struct imx_crtc_state {
>         u32                                     bus_flags;
>         int                                     di_hsync_pin;
>         int                                     di_vsync_pin;
> +       u32                                     ldb_bus_format;

Pretty much a hack here, I think.
Comparing to the other members of imx_crtc_state,
ldb_bus_format is less likely to be a member, since
crtc just doesn't like to know the LVDS bus format...

>  };
>
>  static inline struct imx_crtc_state *to_imx_crtc_state(struct drm_crtc_state *s)
> diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
> index dc2b420..871dff9 100644
> --- a/drivers/gpu/drm/imx/imx-ldb.c
> +++ b/drivers/gpu/drm/imx/imx-ldb.c
> @@ -262,7 +262,10 @@ static void imx_ldb_encoder_mode_set(struct drm_encoder *encoder,
>         unsigned long serial_clk;
>         unsigned long di_clk = mode->clock * 1000;
>         int mux = drm_of_encoder_active_port_id(imx_ldb_ch->child, encoder);
> -       u32 bus_format = imx_ldb_ch->bus_format;
> +       struct drm_crtc_state *crtc_state = encoder->crtc->state;
> +       struct imx_crtc_state *imx_crtc_state = to_imx_crtc_state(crtc_state);
> +       u32 bus_format = imx_ldb_ch->bus_format ?:
> +                        imx_crtc_state->ldb_bus_format;

Instead of introducing imx_crtc_state->ldb_bus_format to get the
lvds bus format, you may get the connector via
crtc_state->connector_mask(verify connector->encoder to be
the encoder we are talking about). connector_mask is maintained
well by the atomic core I assume.

Regards,
Liu Ying

>
>         if (mode->clock > 170000) {
>                 dev_warn(ldb->dev,
> @@ -297,22 +300,6 @@ static void imx_ldb_encoder_mode_set(struct drm_encoder *encoder,
>                         ldb->ldb_ctrl &= ~LDB_DI1_VS_POL_ACT_LOW;
>         }
>
> -       if (!bus_format) {
> -               struct drm_connector_state *conn_state;
> -               struct drm_connector *connector;
> -               int i;
> -
> -               for_each_connector_in_state(encoder->crtc->state->state,
> -                                           connector, conn_state, i) {
> -                       struct drm_display_info *di = &connector->display_info;
> -
> -                       if (conn_state->crtc == encoder->crtc &&
> -                           di->num_bus_formats) {
> -                               bus_format = di->bus_formats[0];
> -                               break;
> -                       }
> -               }
> -       }
>         imx_ldb_ch_set_bus_format(imx_ldb_ch, bus_format);
>  }
>
> @@ -404,6 +391,7 @@ static int imx_ldb_encoder_atomic_check(struct drm_encoder *encoder,
>
>         imx_crtc_state->di_hsync_pin = 2;
>         imx_crtc_state->di_vsync_pin = 3;
> +       imx_crtc_state->ldb_bus_format = bus_format;
>
>         return 0;
>  }
> --
> 2.8.1
>