| Message ID | 20170321081358.27237-8-thierry.reding@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 21.03.2017 09:13, Thierry Reding wrote: > From: Thierry Reding <treding@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@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. Regards Andrzej > > /* register the DRM device */ > ret = drm_dev_register(drm, 0); > @@ -411,6 +412,7 @@ static int exynos_drm_bind(struct device *dev) > > err_cleanup_fbdev: > exynos_drm_fbdev_fini(drm); > +err_cleanup_poll: > drm_kms_helper_poll_fini(drm); > exynos_drm_device_subdrv_remove(drm); > err_cleanup_vblank: > diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c > index 641531243e04..a020fa70c825 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c > @@ -306,6 +306,4 @@ void exynos_drm_output_poll_changed(struct drm_device *dev) > > if (fb_helper) > drm_fb_helper_hotplug_event(fb_helper); > - else > - exynos_drm_fbdev_init(dev); > }
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@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@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. 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 > > Regards > Andrzej > > > > > > /* register the DRM device */ > > ret = drm_dev_register(drm, 0); > > @@ -411,6 +412,7 @@ static int exynos_drm_bind(struct device *dev) > > > > err_cleanup_fbdev: > > exynos_drm_fbdev_fini(drm); > > +err_cleanup_poll: > > drm_kms_helper_poll_fini(drm); > > exynos_drm_device_subdrv_remove(drm); > > err_cleanup_vblank: > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c > > index 641531243e04..a020fa70c825 100644 > > --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c > > +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c > > @@ -306,6 +306,4 @@ void exynos_drm_output_poll_changed(struct drm_device *dev) > > > > if (fb_helper) > > drm_fb_helper_hotplug_event(fb_helper); > > - else > > - exynos_drm_fbdev_init(dev); > > } > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
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@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@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. > 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. Regards Andrzej > >> Regards >> Andrzej >> >> >>> >>> /* register the DRM device */ >>> ret = drm_dev_register(drm, 0); >>> @@ -411,6 +412,7 @@ static int exynos_drm_bind(struct device *dev) >>> >>> err_cleanup_fbdev: >>> exynos_drm_fbdev_fini(drm); >>> +err_cleanup_poll: >>> drm_kms_helper_poll_fini(drm); >>> exynos_drm_device_subdrv_remove(drm); >>> err_cleanup_vblank: >>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c >>> index 641531243e04..a020fa70c825 100644 >>> --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c >>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c >>> @@ -306,6 +306,4 @@ void exynos_drm_output_poll_changed(struct drm_device *dev) >>> >>> if (fb_helper) >>> drm_fb_helper_hotplug_event(fb_helper); >>> - else >>> - exynos_drm_fbdev_init(dev); >>> } >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
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@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@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. Thierry
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@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@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. 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? Regards Andrzej > > Thierry
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@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@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. > 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? Yes, that matches my understanding of the call sequence. Thierry
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@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@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
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; /* register the DRM device */ ret = drm_dev_register(drm, 0); @@ -411,6 +412,7 @@ static int exynos_drm_bind(struct device *dev) err_cleanup_fbdev: exynos_drm_fbdev_fini(drm); +err_cleanup_poll: drm_kms_helper_poll_fini(drm); exynos_drm_device_subdrv_remove(drm); err_cleanup_vblank: diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c index 641531243e04..a020fa70c825 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c @@ -306,6 +306,4 @@ void exynos_drm_output_poll_changed(struct drm_device *dev) if (fb_helper) drm_fb_helper_hotplug_event(fb_helper); - else - exynos_drm_fbdev_init(dev); }