Re: [RFC][PATCH 1/2] drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Feb 14, 2017 at 11:45:54AM -0800, John Stultz wrote:
> On Tue, Feb 14, 2017 at 11:38 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Tue, Feb 14, 2017 at 8:25 PM, John Stultz <john.stultz@linaro.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@intel.com>
> >> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> >> Cc: Sean Paul <seanpaul@chromium.org>
> >> Cc: David Airlie <airlied@linux.ie>
> >> Cc: Rob Clark <robdclark@gmail.com>
> >> Cc: Xinliang Liu <xinliang.liu@linaro.org>
> >> Cc: Xinliang Liu <z.liuxinliang@hisilicon.com>
> >> Cc: Rongrong Zou <zourongrong@gmail.com>
> >> Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com>
> >> Cc: Chen Feng <puck.chen@hisilicon.com>
> >> Cc: Archit Taneja <architt@codeaurora.org>
> >> Cc: dri-devel@lists.freedesktop.org
> >> Signed-off-by: John Stultz <john.stultz@linaro.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?

In your driver callback for ->mode_valid, call into a shared function to
validate CRTC limits. Which you also call from the crtc's ->mode_fixup
function.

In short my complain here is that this is only a partial solution of the
larger problem, specific for your driver. That kind of code is better put
into drivers, until we have a clear understanding to type up something
complete in the helpers. Shared code is imo overrated :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch