[PATCH V2 4/9] drm/exynos: add exynos_dp_panel driver registration to drm driver

[Ajay kumar]

On Thu, Aug 21, 2014 at 1:55 PM, Stéphane Marchesin
<stephane.marchesin at gmail.com> wrote:
> On Thu, Aug 21, 2014 at 12:36 AM, Thierry Reding
> <thierry.reding at gmail.com> wrote:
>> On Tue, Aug 19, 2014 at 09:02:39PM -0700, Stéphane Marchesin wrote:
>>> On Tue, Apr 22, 2014 at 8:26 AM, Thierry Reding
>>> <thierry.reding at gmail.com> wrote:
>>> > On Tue, Apr 22, 2014 at 08:33:23PM +0530, Ajay kumar wrote:
>>> >> Hi Thierry,
>>> >>
>>> >> On Tue, Apr 22, 2014 at 2:03 PM, Thierry Reding
>>> >> <thierry.reding at gmail.com> wrote:
>>> >> > On Tue, Apr 22, 2014 at 04:09:13AM +0530, Ajay Kumar wrote:
>>> >> >> Register exynos_dp_panel before the list of exynos crtcs and
>>> >> >> connectors are probed.
>>> >> >>
>>> >> >> This is needed because exynos_dp_panel should be registered to
>>> >> >> the drm_panel list via panel-exynos-dp probe, i.e much before
>>> >> >> exynos_dp_bind calls of_drm_find_panel().
>>> >> >>
>>> >> >> Signed-off-by: Ajay Kumar <ajaykumar.rs at samsung.com>
>>> >> >> ---
>>> >> >> Changes since V1:
>>> >> >>       Added platform_driver_unregister(&exynos_dp_panel_driver) to
>>> >> >>       exynos_drm_platform_remove as per Jingoo Han's correction
>>> >> >>
>>> >> >>  drivers/gpu/drm/exynos/exynos_drm_drv.c |   15 +++++++++++++++
>>> >> >>  drivers/gpu/drm/exynos/exynos_drm_drv.h |    1 +
>>> >> >>  2 files changed, 16 insertions(+)
>>> >> >>
>>> >> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
>>> >> >> index 1d653f8..2db7f67 100644
>>> >> >> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
>>> >> >> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
>>> >> >> @@ -530,12 +530,23 @@ static int exynos_drm_platform_probe(struct platform_device *pdev)
>>> >> >>               goto err_unregister_ipp_drv;
>>> >> >>  #endif
>>> >> >>
>>> >> >> +#ifdef CONFIG_DRM_PANEL_EXYNOS_DP
>>> >> >> +     ret = platform_driver_register(&exynos_dp_panel_driver);
>>> >> >> +     if (ret < 0)
>>> >> >> +             goto err_unregister_dp_panel;
>>> >> >> +#endif
>>> >> >
>>> >> > No, this is not how you're supposed to use DRM panel drivers. The idea
>>> >> > is that you write a standalone driver for a given panel.
>>> >> >
>>> >> > What you do here has a number of problems. For one it's a driver that's
>>> >> > tightly coupled to Exynos SoCs. But if I have a different SoC that uses
>>> >> > the same panel I want to be able to use the same driver, and not have to
>>> >> > rewrite the driver for my SoC.
>>> >> >
>>> >> > Another problem is that you're assuming here that the driver is built in
>>> >> > and it will break if you try to build either Exynos DRM or the panel
>>> >> > driver as a module. This is perhaps nothing you care about right now,
>>> >> > but eventually people will want to ship a single kernel that can run on
>>> >> > a number of SoCs. But if we keep adding things like this, that kernel
>>> >> > will keep growing in size until it no longer fits in any kind of memory.
>>> >> >
>>> >> > Thierry
>>> >>
>>> >> I completely agree with you in this!
>>> >>
>>> >> Yes, this is not acceptable, but I want to know an "acceptable"
>>> >> workaround for the situation below:
>>> >> I register the driver using module_init().
>>> >> And, exynos_drm gets probed much before the panel driver probe happens.
>>> >> So, the panel driver hasn't probed yet, but exynos_dp via exynos_drm
>>> >> tries to call
>>> >> "of_drm_find_panel" which always returns NULL.
>>> >
>>> > That's a situation that your driver needs to be able to deal with. The
>>> > driver registration order doesn't matter one bit. It may happen to work
>>> > most of the time, but as soon as one of the resources that your panel
>>> > driver needs isn't there when the panel is probed, then it won't be
>>> > registered and of_drm_find_panel() will still return NULL.
>>> >
>>> > Usually the right thing to do in that case would be to return (and
>>> > propagate) -EPROBE_DEFER so that your driver's probe is deferred and
>>> > retried when other drivers have been probed. That way it should
>>> > eventually get a non-NULL panel.
>>>
>>> So I just gave this (drm_panel + probe deferring) a shot on exynos,
>>> and correctly reacting to -EPROBE_DEFER postpones DP initialization by
>>> approximately 1.5 second. Is there a good way to handle that? As it
>>> stands, this isn't usable.
>>
>> How much is 1.5 seconds compared to the overall boot time of the device?
>
> 1.5s is 15-20% of my boot time (if you count the boot time from
> firmware start to login prompt, otherwise it's more). Note that on
> other platforms, we've seen this take as much as 5 or 6s, but for the
> exynos case it is "only" 1.5s.
>
>> What exactly is causing this 1.5 second delay?
>
> A regulator isn't ready, and then drm_panel returns defer. Then the
> whole drm driver init is deferred.
>
>>
>> This really is a fundamental issue with deferred probing and the issue
>> has come up several times in the past. A couple of possible solutions
>> have been proposed, with the latest being here[0] I think. That ended in
>> a bit of a debacle, unfortunately, but on of the outcomes was that a lot
>> of the ordering problems could be fixed by using phandle references to
>> track dependencies. I'm not aware of anyone working on that right now,
>> presumably because everyone is busy getting features merged rather than
>> optimizing boot speed.
>
> Yes, I don't believe boot time ordering will ever happen upstream, but
> then the current implementation with EPROBE_DEFER isn't usable either.
> Any ideas? ATM it seems like the only way out is to just write my
> own dt format for the panel and ignore drm_panel.
Something like this:
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.8/drivers/gpu/drm/exynos/exynos_dp_core.c

With this way also, we would expect the regulator to come up early(before drm).

Ajay