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@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@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