Re: [RFC][PATCH v2] drm: kirin: Add mode_valid logic to avoid mode clocks we can't generate

[Jose Abreu]

Hi John,


On 18-07-2017 05:22, John Stultz wrote:
> Currently the hikey dsi logic cannot generate accurate byte
> clocks values for all pixel clock values. Thus if a mode clock
> is selected that cannot match the calculated byte clock, the
> device will boot with a blank screen.
>
> This patch uses the new mode_valid callback (many thanks to
> Jose Abreu for upstreaming it!) to ensure we don't select
> modes we cannot generate.
>
> NOTE: Stylistically I suspect there are better ways to do what
> I'm trying to do here. The encoder -> crtc bit is terrible, and
> getting the crtc adjusted mode from the encoder logic feels
> less then ideal. So feedback would be greatly appreciated!
>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> Cc: Sean Paul <seanpaul@xxxxxxxxxxxx>
> Cc: David Airlie <airlied@xxxxxxxx>
> Cc: Rob Clark <robdclark@xxxxxxxxx>
> Cc: Xinliang Liu <xinliang.liu@xxxxxxxxxx>
> Cc: Xinliang Liu <z.liuxinliang@xxxxxxxxxxxxx>
> Cc: Rongrong Zou <zourongrong@xxxxxxxxx>
> Cc: Xinwei Kong <kong.kongxinwei@xxxxxxxxxxxxx>
> Cc: Chen Feng <puck.chen@xxxxxxxxxxxxx>
> Cc: Jose Abreu <Jose.Abreu@xxxxxxxxxxxx>
> Cc: Archit Taneja <architt@xxxxxxxxxxxxxx>
> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: John Stultz <john.stultz@xxxxxxxxxx>
> ---
> v2: Reworked to calculate if modeclock matches the phy's byteclock,
>     rather then using a whitelist of known modes.

Something like Daniel suggested would be simpler maybe:

encoder_mode_valid_aux()
{
    ...
}

dsi_encoder_mode_valid(...)
{
    drm_for_each_crtc(crtc, dev) {
        crtc->mode_fixup(crtc, mode, adjusted_mode);
        ret = encoder_mode_valid_aux(adjusted_mode);
        if (ret != MODE_OK)
            return ret;
    }
}

BTW, I think at commit stage you have encoder->crtc populated,
not sure though. But at probbing stage it will not be bound. And
also if you have more than one crtc this may be wrong. (How will
you know which crtc will be bound to the encoder so that you can
get the right clock?).

Best regards,
Jose Miguel Abreu