Message-ID: <20170228093435.5drrweusvgz7d5hm@phenom.ffwll.local>
Date:   Tue, 28 Feb 2017 10:34:35 +0100
From:   Daniel Vetter <daniel@...ll.ch>
To:     John Stultz <john.stultz@...aro.org>
Cc:     Daniel Vetter <daniel@...ll.ch>,
        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 Mon, Feb 27, 2017 at 10:03:20PM -0800, John Stultz wrote:
> On Mon, Feb 20, 2017 at 2:32 PM, Daniel Vetter <daniel@...ll.ch> wrote:
> > I thought about this some more, I think what we need to fix this mess
> > properly is:
> > - mode_valid helper callbacks for crtc, encoder, bridges, with the
> > same interface as for connectors.
> > - calling all these new mode_valid hooks from the probe helpers, but
> > with the restriction that we only reject a mode if all possible
> > crtc/encoders combos reject it. We need to filter by
> > possible_encoders/crtcs for these checks. Bridges have a fixed routing
> > to their encoder, so those are easy.
> 
> So... my ignorance here my have complicated things a bit.
> 
> Xinliang can correct me here, if I get this wrong. :)
> 
> So in the past, when we ran into this issue, we just hacked in a mode
> white list into the adv7511 bridge mode_valid check, as it was the
> easiest place. But as I understood it, the limitation came from logic
> in (what is now an early version of) the kirin ade driver.
> 
> However, since kirin made it upstream, the code was refactored and I
> believe the specific code was moved to the dsi_calc_phy_rate() which
> is part of the dsi encoder, not the crtc code.
> 
> So in my limited understanding I initially tried to add the mode_check
> logic in the ade crtc driver, but to me (and I could have this wrong
> again) it seems it really ought to be in the encoder (where we
> probably could do better then using a white list by doing the mode
> calculations and seeing if we get a value that matches what was
> requested).
> 
> I'm not sure if that simplifies your proposal (possibly avoiding the
> crtc/encoder combo checks?), or if you think we still need the big
> grand solution?

We'll need the grand plan for other drivers I think. Or at least that's
what I'm trying to volunteer you for :-)

If you just want to fix up your specific combo, then you can just hack up
that driver. You don't need any core/helper changes for that.

> > - add calls to mode_valid in the atomic helpers, right before we call
> > mode_fixup or atomic_check in drm_atomic_helper_check_modesets.
> > - convert drivers to move code from mode_fixup into mode_valid
> > wherever possible, to make sure we can share as much of the check
> > logic between probe and atomic comit code.
> > - update docs for all the hooks, plus update the overview sections accordingly.
> >
> > I think this should give us a good approximation of nirvana. For I
> > long time I thought we could get away without adding mode_valid
> > everywhere, but in the probe paths we really can't fake the
> > adjusted_mode (and other atomic state), so adding dedicated hooks
> > which are called from both places is probably the only option.
> 
> So my mental map of the DRM code is all pretty hazy and limited here,
> but the above sounds reasonable. I'll try to trace through and better
> understand what specifically you're asking for and get something
> started here.

Yeah, we're still lacking the nice overview graphs for the drm docs. I
have them here, but the kbuild scripts need polish :(
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Powered by blists - more mailing lists