[Intel-gfx] [PATCH v3 07/10] drm/exynos: Remove custom FB helper deferred setup

[Daniel Vetter]

On Tue, Mar 21, 2017 at 12:13:26PM +0100, Andrzej Hajda wrote:
> On 21.03.2017 11:53, Thierry Reding wrote:
> > On Tue, Mar 21, 2017 at 11:42:21AM +0100, Andrzej Hajda wrote:
> >> On 21.03.2017 11:20, Daniel Vetter wrote:
> >>> On Tue, Mar 21, 2017 at 10:58:43AM +0100, Andrzej Hajda wrote:
> >>>> On 21.03.2017 09:13, Thierry Reding wrote:
> >>>>> From: Thierry Reding <treding at nvidia.com>
> >>>>>
> >>>>> The FB helper core now supports deferred setup, so the driver's custom
> >>>>> implementation can be removed.
> >>>>>
> >>>>> Signed-off-by: Thierry Reding <treding at nvidia.com>
> >>>>> ---
> >>>>>  drivers/gpu/drm/exynos/exynos_drm_drv.c   | 6 ++++--
> >>>>>  drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 2 --
> >>>>>  2 files changed, 4 insertions(+), 4 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> >>>>> index 09d3c4c3c858..c5a37dda8d1b 100644
> >>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> >>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> >>>>> @@ -399,8 +399,9 @@ static int exynos_drm_bind(struct device *dev)
> >>>>>  	/* init kms poll for handling hpd */
> >>>>>  	drm_kms_helper_poll_init(drm);
> >>>>>  
> >>>>> -	/* force connectors detection */
> >>>>> -	drm_helper_hpd_irq_event(drm);
> >>>>> +	ret = exynos_drm_fbdev_init(dev);
> >>>>> +	if (ret)
> >>>>> +		goto err_cleanup_poll;
> >>>> It should be rather:
> >>>>     ret = exynos_drm_fbdev_init(drm);
> >>>>
> >>>> Even with this change it does not work, I will try to track down the
> >>>> problem.
> >> The solution was to remove custom equivalent of
> >> drm_fb_helper_maybe_connected - exynos_drm_fbdev_is_anything_connected,
> >> it is called earlier and it was a part of custom deferred fbdev.
> > Cool, thanks for figuring that out. I can add that to this patch for v4.
> >
> >>> We might need the multi-stage fbdev setup here, i.e. init fbdev, run
> >>> hpd_irq_event() (I suspect that kicks all the connectors to start
> >>> probing), then initial_config for fbdev.
> >>>
> >>> Probably simplest way to achieve this is to keep the hpd_irq_event call,
> >>> but place it _after_ the fbdev_init.
> >>> -Daniel
> >> I wonder if it wouldn't be sufficient to check if there is anything
> >> connected, instead of checking if there is anything not-disconnected, in
> >> drm_fb_helper_maybe_connected.
> > My recollection is that this had been discussed when I first sent this
> > out. VGA outputs are somewhat of a special case in that you can't always
> > properly detect that it's connected or not. So effectively we need to
> > take into account the connector_status_unknown. There's also some more
> > information about this in the kerneldoc for enum drm_connector_status.
> 
> OK, I forgot about it and assumed that it is only used for initial state
> of connector.

unknown does indeed mean "I couldn't figure this out", which kinda only
happesn for VGA on platforms where load detect doesn't exist (or doesn't
reliably exist). We should probably document this somewhere ... hint :-)

> Returning to Daniel's proposition about hpd_irq_event(), it seems
> unnecessary then: before calling drm_fb_helper_maybe_connected
> drm_fb_helper_probe_connector_modes is called, which calls fill_modes
> callback which should perform connector probing anyway, am I right?

Yeah if you can fix this by removing the exynos copy of maybe_connected,
then we indeed don't need the hpd_irq. Needing it would indicate a bug in
exynos anyway.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch