| Message ID | 20170630165155.8658-1-Liviu.Dudau@arm.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Jun 30, 2017 at 6:51 PM, Liviu Dudau <Liviu.Dudau@arm.com> wrote: > Prior to commit b0aa06e9a7fd ("drm/fb-helper: Support deferred setup"), > if no output is connected at framebuffer setup time, we get a default > 1024x768 mode that is going to be used when we first connect a monitor. > After the commit, on first connection after deferred setup, we probe > the monitor and get the preferred resolution, but no mode get set > because the drm_fb_helper_hotplug_event() function returns early > when the setup has been deferred. That is different from what happens > on a second re-connect of the monitor, when the native mode get set. > > Create a more consistent behaviour by checking in the > drm_fb_helper_hotplug_event() function if the deferred setup is still > active. If not, that means we now have a valid framebuffer that can be > used for setting the correct mode. > > Fixes: b0aa06e9a7fd ("drm/fb-helper: Support deferred setup") > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> I thought the analysis over irc was that fbcon picked a different driver as it's console, and that's why nothing shows up on the malidp output in the deferred case? That's mildly annoying, but iirc fbcon has always been rather erratic in multi-gpu setups. Although I thought that it would by default bind all fbdev drivers as consoles (and then you need to rebind the right console driver, if the right Kconfig is enabled, through sysfs). Either way if the register_framebuffer() call in initial_config isn't good enough, then we need to add the set_par in initial_config unconditionally, not just in the deferred probe case. Just disable fbcon entirely for testing, in that case even without deferred probing nothing will show up. I'd say if this is still needed in the single gpu case then we need to investigate more, but for multi-gpu it is what it is (aka fbcon is not great). -Daniel
On Fri, Jun 30, 2017 at 08:13:58PM +0200, Daniel Vetter wrote: > On Fri, Jun 30, 2017 at 6:51 PM, Liviu Dudau <Liviu.Dudau@arm.com> wrote: > > Prior to commit b0aa06e9a7fd ("drm/fb-helper: Support deferred setup"), > > if no output is connected at framebuffer setup time, we get a default > > 1024x768 mode that is going to be used when we first connect a monitor. > > After the commit, on first connection after deferred setup, we probe > > the monitor and get the preferred resolution, but no mode get set > > because the drm_fb_helper_hotplug_event() function returns early > > when the setup has been deferred. That is different from what happens > > on a second re-connect of the monitor, when the native mode get set. > > > > Create a more consistent behaviour by checking in the > > drm_fb_helper_hotplug_event() function if the deferred setup is still > > active. If not, that means we now have a valid framebuffer that can be > > used for setting the correct mode. > > > > Fixes: b0aa06e9a7fd ("drm/fb-helper: Support deferred setup") > > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > I thought the analysis over irc was that fbcon picked a different > driver as it's console, and that's why nothing shows up on the malidp > output in the deferred case? That's mildly annoying, but iirc fbcon > has always been rather erratic in multi-gpu setups. Although I thought > that it would by default bind all fbdev drivers as consoles (and then > you need to rebind the right console driver, if the right Kconfig is > enabled, through sysfs). That's right, fbcon picks a different driver and binds to it. But before the patch, on the first connection with a monitor that happened after setup (i.e. when the fbdev emulation mode set the default 1024x768 mode), a full atomic modeset was trigger for the driver that had the connection and the "card" output was enabled. Now with your patch that does not happen on the first connection, but it does after I turn off and back on the monitor. I know that is how most computer problems are solved, but still ... :) > > Either way if the register_framebuffer() call in initial_config isn't > good enough, then we need to add the set_par in initial_config > unconditionally, not just in the deferred probe case. Just disable > fbcon entirely for testing, in that case even without deferred probing > nothing will show up. Actually the un-ballanced call to set_par happens with your patch (it gets called for all hotplug events that are *not* deferred. I was trying to balance things and call set_par also for deferred setups where the __drm_fb_helper_initial_config() call succeeded. And the fact that we do call set_par in the hotplug path even when the setup is not deferred tells me that register_framebuffer() is probably not enough. > > I'd say if this is still needed in the single gpu case then we need to > investigate more, but for multi-gpu it is what it is (aka fbcon is not > great). I think single gpu case hides the problem because fbcon forces the right sequence of calls. And I think the old behaviour was correct, as I was getting output of the second "card" on hotplug, now I have to turn the monitor off and on again to get some signal sync. Best regards, Liviu > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Mon, Jul 03, 2017 at 09:44:50AM +0100, Liviu Dudau wrote: > On Fri, Jun 30, 2017 at 08:13:58PM +0200, Daniel Vetter wrote: > > On Fri, Jun 30, 2017 at 6:51 PM, Liviu Dudau <Liviu.Dudau@arm.com> wrote: > > > Prior to commit b0aa06e9a7fd ("drm/fb-helper: Support deferred setup"), > > > if no output is connected at framebuffer setup time, we get a default > > > 1024x768 mode that is going to be used when we first connect a monitor. > > > After the commit, on first connection after deferred setup, we probe > > > the monitor and get the preferred resolution, but no mode get set > > > because the drm_fb_helper_hotplug_event() function returns early > > > when the setup has been deferred. That is different from what happens > > > on a second re-connect of the monitor, when the native mode get set. > > > > > > Create a more consistent behaviour by checking in the > > > drm_fb_helper_hotplug_event() function if the deferred setup is still > > > active. If not, that means we now have a valid framebuffer that can be > > > used for setting the correct mode. > > > > > > Fixes: b0aa06e9a7fd ("drm/fb-helper: Support deferred setup") > > > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > I thought the analysis over irc was that fbcon picked a different > > driver as it's console, and that's why nothing shows up on the malidp > > output in the deferred case? That's mildly annoying, but iirc fbcon > > has always been rather erratic in multi-gpu setups. Although I thought > > that it would by default bind all fbdev drivers as consoles (and then > > you need to rebind the right console driver, if the right Kconfig is > > enabled, through sysfs). > > That's right, fbcon picks a different driver and binds to it. But > before the patch, on the first connection with a monitor that happened > after setup (i.e. when the fbdev emulation mode set the default 1024x768 > mode), a full atomic modeset was trigger for the driver that had the > connection and the "card" output was enabled. Now with your patch that > does not happen on the first connection, but it does after I turn off and > back on the monitor. I know that is how most computer problems are solved, > but still ... :) > > > > > Either way if the register_framebuffer() call in initial_config isn't > > good enough, then we need to add the set_par in initial_config > > unconditionally, not just in the deferred probe case. Just disable > > fbcon entirely for testing, in that case even without deferred probing > > nothing will show up. > > Actually the un-ballanced call to set_par happens with your patch (it > gets called for all hotplug events that are *not* deferred. I was trying > to balance things and call set_par also for deferred setups where the > __drm_fb_helper_initial_config() call succeeded. And the fact that we do > call set_par in the hotplug path even when the setup is not deferred > tells me that register_framebuffer() is probably not enough. But then you end up doing it twice if that's the fbdev fbcon will bind too. And of course we must call set_par to enable the newly hotplugged screen, it'd be black otherwise. It's just that with just 1 gpu, the register_framebuffer takes care of the initial config stuff (if you have fbcon enabled). If you boot with just 1 gpu, and hotplug that one, it will work correctly. Multi-gpu + fbcon just don't work in a predictable fashion. > > I'd say if this is still needed in the single gpu case then we need to > > investigate more, but for multi-gpu it is what it is (aka fbcon is not > > great). > > I think single gpu case hides the problem because fbcon forces the right > sequence of calls. And I think the old behaviour was correct, as I was > getting output of the second "card" on hotplug, now I have to turn the > monitor off and on again to get some signal sync. So the problem is that we rely upon register_framebuffer->fbcon to create that initial set_par. We could add an unconditional set_par to fix this bug (the condition really is pre-existing, if you'd boot with 2 gpus and both connected at start-up the 2nd one would still be off). But that comes at the cost of forcing everyone to do a 2nd, unecessary modeset right after boot-up, potentially destroying neat tricks like fastboot. So we can't just do that either, because that would regress boot-up time. Now if you have an idea how to fix this without rewriting fbcon+fbdev, we could try, but I'm not sure that's feasible really ... After all if you entirely disable fbdev emulation, your screen also stays off. And maybe you event _want_ it to stay off, even with fbdev emulation enabled. So really no idea what exactly we should do here, but adding an unconditional set_par in the initial_config path isn't the solution I think. -Daniel
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index d833eb2320d1..bb7b44d284ec 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -2444,6 +2444,7 @@ static int __drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, if (ret == -EAGAIN) { fb_helper->preferred_bpp = bpp_sel; fb_helper->deferred_setup = true; + ret = 0; } mutex_unlock(&fb_helper->lock); @@ -2565,7 +2566,13 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper) if (fb_helper->deferred_setup) { err = __drm_fb_helper_initial_config(fb_helper, fb_helper->preferred_bpp); - return err; + /* + * __drm_fb_helper_initial_config can change deferred_setup, + * if 'false' that means we can go ahead with the rest of + * the setup as normal + */ + if (fb_helper->deferred_setup) + return err; } if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
Prior to commit b0aa06e9a7fd ("drm/fb-helper: Support deferred setup"), if no output is connected at framebuffer setup time, we get a default 1024x768 mode that is going to be used when we first connect a monitor. After the commit, on first connection after deferred setup, we probe the monitor and get the preferred resolution, but no mode get set because the drm_fb_helper_hotplug_event() function returns early when the setup has been deferred. That is different from what happens on a second re-connect of the monitor, when the native mode get set. Create a more consistent behaviour by checking in the drm_fb_helper_hotplug_event() function if the deferred setup is still active. If not, that means we now have a valid framebuffer that can be used for setting the correct mode. Fixes: b0aa06e9a7fd ("drm/fb-helper: Support deferred setup") Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/drm_fb_helper.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)