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

[Philipp Zabel]

Hi Liu,

thank you for your comments.

Am Freitag, den 22.07.2016, 12:01 +0800 schrieb 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.

Yes. I initially stored ldb_bus_format in imx_ldb_ch, then fixed that,
and forgot to change the subject.

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

Will do.

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

I suppose I should keep the connector walk then. I'll send a new
version.

[...]
> 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.

I could also use drm_for_each_connector and just keep the
(connector->state->crtc == encoder->crtc) check in the loop.

regards
Philipp