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

[John Stultz]

On Tue, Jul 18, 2017 at 4:10 AM, Jose Abreu <Jose.Abreu@xxxxxxxxxxxx> wrote:
> 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?).


Thanks so much for the suggestion above and the explanation. I only
have a rough sense of the components and not as much sense of an
overview on how they are all initialized, so this is very helpful!

And yes, my case is fairly limited since the encoder and crtc are used
together for this driver, but I know it can be more complex with
others, so I'll re-implement as you've suggested!

One thing that does worry me with trying to validate modes without
knowing which path is to be used, is that if there were two crtcs (and
again, on my hardware, thankfully there isn't), and for a given mode,
one adjusted the mode and one didn't.  So then for that given mode the
encoder could only generate a valid mode for one of the crtcs, its not
clear if that is MODE_OK or MODE_BAD.  We don't want to prune modes
that could possibly work with other crtcs, but we don't want a mode
that cannot be generated be selected either.  To me it seems we should
have the path figured out before we go pruning modes. Obviously there
needs to be a probe step that checks if any mode works with a given
pipeline (so that we can validate the pipeline), but it seems like
there should be a second step to ensure once we have a pipeline bound
we don't try to use modes it cannot support. Am I further
misunderstanding something here?

Thanks again!
-john