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

[Ying Liu]

Hi Philipp,

On Fri, Jul 22, 2016 at 4:59 PM, Philipp Zabel <p.zabel at pengutronix.de> wrote:
> 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.

It looks (connector->encoder == encoder) is simpler.

Regards,
Liu Ying

>
> regards
> Philipp
>