Date:   Tue, 14 Feb 2017 11:45:54 -0800
From:   John Stultz <john.stultz@...aro.org>
To:     Daniel Vetter <daniel@...ll.ch>
Cc:     lkml <linux-kernel@...r.kernel.org>,
        Chen Feng <puck.chen@...ilicon.com>,
        Xinwei Kong <kong.kongxinwei@...ilicon.com>,
        Xinliang Liu <z.liuxinliang@...ilicon.com>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        Rongrong Zou <zourongrong@...il.com>,
        Daniel Vetter <daniel.vetter@...el.com>
Subject: Re: [RFC][PATCH 1/2] drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs

On Tue, Feb 14, 2017 at 11:38 AM, Daniel Vetter <daniel@...ll.ch> wrote:
> On Tue, Feb 14, 2017 at 8:25 PM, John Stultz <john.stultz@...aro.org> wrote:
>> Currently, on the hikey board, we have the adv7511 bridge wired
>> up to the kirin ade drm driver. Unfortunately, the kirin ade
>> core cannot generate accurate byteclocks for all pixel clock
>> values.
>>
>> Thus if a mode clock is selected that we cannot calculate a
>> matching byteclock, the device will boot with a blank screen.
>>
>> Unfortunately, currently the only place we can properly check
>> potential modes for this issue in the connector mode_valid
>> helper. Again, hikey uses the adv7511 bridge, which is shared
>> between a number of different devices, so its improper to put
>> restrictions caused by the kirin drm driver in the adv7511
>> logic.
>>
>> So this patch tries to correct for that, by adding some
>> infrastructure so that the drm_crtc_helper_funcs can optionally
>> implement a mode_valid check, so that the probe helpers can
>> check to make sure there are not any restrictions at the crtc
>> level as well.
>>
>> Cc: Daniel Vetter <daniel.vetter@...el.com>
>> Cc: Jani Nikula <jani.nikula@...ux.intel.com>
>> Cc: Sean Paul <seanpaul@...omium.org>
>> Cc: David Airlie <airlied@...ux.ie>
>> Cc: Rob Clark <robdclark@...il.com>
>> Cc: Xinliang Liu <xinliang.liu@...aro.org>
>> Cc: Xinliang Liu <z.liuxinliang@...ilicon.com>
>> Cc: Rongrong Zou <zourongrong@...il.com>
>> Cc: Xinwei Kong <kong.kongxinwei@...ilicon.com>
>> Cc: Chen Feng <puck.chen@...ilicon.com>
>> Cc: Archit Taneja <architt@...eaurora.org>
>> Cc: dri-devel@...ts.freedesktop.org
>> Signed-off-by: John Stultz <john.stultz@...aro.org>
>
> So I'm going to be super-annoying here and ask for a complete
> solution. This here is defacto what ever driver already does (or has
> too), but it doesn't really solve the overall issue of having entirely
> separate validation paths for probe and atomic_check paths. I think if
> we wan to solve this, we need to solve this properly, with a generic
> solution. That would mean:
> - still in helpers, to make it all opt-int

Just to be clear, I believe what I proposed is opt-in, but I assume
you want that in addition to the following, right?

> - covers crtc and encoders and bridges

So you'd like similar mode_valid() calls in the crtc/encoder/bridge
helpers? right?

> - allows you to implement the current mode_valid in terms of the new
> stuff (maybe as a default hook)

This bit I'm not sure I'm following. The current drm_connector's
mode_valid in terms of a new mode_valid call that also looks at
crtc/encoder/bridges? Or do you mean something else?

> - allows you to implement the current assortment of mode_fixup and/or
> atomic_check in terms of the new stuff, or at least to not have to
> duplicate logic in there

This is over my head, so I'll have to research to better understand.

> Docs for all this, especially updating all the warnings on how to use
> the existing hooks correctly.

That's fair.

> I think just pushing this specific case into the helpers, without
> rethinking the overall big picture, isn't gaining us all that much.
> For just this I'd say just put it into drivers, until we have some

Not following here. What do you mean by "put it into drivers"?  Where?

thanks
-john

Powered by blists - more mailing lists