On Thu, Nov 26, 2020 at 09:44:02AM +0000, Jonas Mark (BT-FIR/ENG1-Grb) wrote: > Hi Daniel, > > > > Thank you very much for your feedback. We appreciate it. > > > > > > > >>> diff --git a/drivers/gpu/drm/imx/imx-drm-core.c > > > > >>> b/drivers/gpu/drm/imx/imx-drm-core.c > > > > >>> index 9bf5ad6d18a2..2665040e11c7 100644 > > > > >>> --- a/drivers/gpu/drm/imx/imx-drm-core.c > > > > >>> +++ b/drivers/gpu/drm/imx/imx-drm-core.c > > > > >>> @@ -240,14 +240,18 @@ static int imx_drm_bind(struct device *dev) > > > > >>> legacyfb_depth = 16; > > > > >>> } > > > > >>> > > > > >>> + /* > > > > >>> + * The generic fbdev has to be setup before enabling output > > > > >>> polling. > > > > >>> + * Otherwise the fbdev client is not ready to handle delayed > > > > >>> events. > > > > >>> + */ > > > > >>> + drm_fbdev_generic_setup(drm, legacyfb_depth); > > > > >>> + > > > > >>> drm_kms_helper_poll_init(drm); > > > > >>> > > > > >>> ret = drm_dev_register(drm, 0); > > > > >>> if (ret) > > > > >>> goto err_poll_fini; > > > > >>> > > > > >>> - drm_fbdev_generic_setup(drm, legacyfb_depth); > > > > >>> - > > > > >> > > > > >> This does not work well. fbdev is supposed to be another regular > > > > >> DRM client. It has to be enabled after registering the DRM device. > > > > >> > > > > >> I'd rather improve fbdev or the driver to handle this gracefully. > > > > > > > > > > Yeah I'm not understanding the point here. Once fbcon is running, > > > > > you have a screen. Any fbdev userspace client also should do a > > > > > modeset first. And if they dont and it's expected uapi for fbdev > > > > > chardev that the display boots up enabled, then we need to fix > > > > > that in the fbdev helpers, not through clever reordering in > > > > > drivers so that a side-effect causes a modeset. > > > > > > > > > > Note that this is a bit tricky since fbdev shouldn't take over the > > > > > screen by default, so we'd need to delay this until first open of > > > > > /dev/fb0. And we should probably also delay the hotplug handling > > > > > until the first open. fbcon also fake-opens the fbdev file, so > > > > > it's the same code path. > > > > > > > > As far as I understand the commit message, the problem is that the > > > > display blanks out after registering the driver. And fbdev somewhat > > > > mitigates this by doing an early modeset. Users with fbdev disabled > > > > (most of them in embedded, I guess) would still run into the issue > > > > until userspace makes a modeset. > > > > > > > > Mark, if that's the case, an option might be to pick up the device > > > > settings instead of calling drm_mode_config_reset(). The driver > > > > would then continue to display whatever is on the screen. > > > > > > We are started using fbdev in our embedded application with Linux > > > 3.10, later updated to 4.14 and are now in the process of updating to > > > 5.4. So far the uapi appeared to us as if we could rely on an already > > > enabled fbdev. That is, none of our applications does a modeset. > > > > > > When switching to 5.4 we noticed that the fbdev uapi changed. That is, > > > the LCD is switched off until it is explicitly enabled. It could be > > > enabled by writing to /sys/class/graphics/fb0/blank. > > > > > > You are right, we are not using fbcon. fbcon will still enable the LCD > > > but in our embedded domain we have it disabled because we must not show a > > console. > > > > > > Do we understand your proposal correctly to replace the call to > > > drm_mode_config_reset() in imx_drm_bind() [imx-drm-core.c] with > > > picking up the device settings instead? > > > > > > https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Felix > > > ir.bootlin.com%2Flinux%2Fv5.10- > > rc4%2Fsource%2Fdrivers%2Fgpu%2Fdrm%2Fim > > > x%2Fimx-drm- > > core.c%23L231&data=04%7C01%7CMark.Jonas%40de.bosch.com > > > > > %7C9bbf5ede27ed40be9aaa08d88bac0c53%7C0ae51e1907c84e4bbb6d648ee > > 58410f4 > > > > > %7C0%7C0%7C637412918338819509%7CUnknown%7CTWFpbGZsb3d8eyJ > > WIjoiMC4wLjAw > > > > > MDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sd > > ata=68 > > > > > 1kOSAs2XsI1l4sOJ7j5UAGkAMciR78ma%2FgbD5jR98%3D&reserved=0 > > > > > > We are a little clueless right now: How do we pick up the device settings? > > > > Nope, not what I had in mind. > > > > Instead intercept the fb_ops->open call and in there if it's a userspace > > open > > (user parameter of the callback tells you that) and kms is not in use, then > > try to > > light up the display for the fbdev userspace to use. drm fbdev helpers > > already > > have that callback as drm_fbdev_fb_open(). I think you could try and just > > call > > drm_fbdev_client_hotplug directly, that should do the trick. Or maybe > > calling > > drm_fb_helper_dpms is the better option, not sure. fbmem.c seems to not > > store > > any blanking state at all, so this is probably all ill-defined. > > > > Important part is to do this only for the user fb_open case, since fbcon > > will do its > > own thing too. > > > > Plus I guess we need to document that this is the uapi we're having for > > fbdev > > clients, so ideally this should be cc'ed widely so we can get some acks from > > former fbdev maintainers. > > > > Also ideally we'd have an igt for this uapi to make sure it never breaks > > again. > > Something like: > > 1. open the kms driver for this, make sure display is completely off. > > 2. close kms file > > 3. open fbdev file > > 4. check (through opening kms side again) that the display has been enabled. > > > > See > > https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdri.freede > > sktop.org%2Fdocs%2Fdrm%2Fgpu%2Fdrm-uapi.html%23validating-changes- > > with- > > igt&data=04%7C01%7CMark.Jonas%40de.bosch.com%7C9bbf5ede27ed4 > > 0be9aaa08d88bac0c53%7C0ae51e1907c84e4bbb6d648ee58410f4%7C0%7C0 > > %7C637412918338819509%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLj > > AwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&am > > p;sdata=tgdaOJP2wK7eXFmOQlVdUa%2B7CRwZxOx99BCCMNE8iD0%3D&a > > mp;reserved=0 > > for some details on our validation testing, there's already a very basic > > fbdev > > testcase there. > > We had a look into the topic and came to the conclusion that we cannot do it > right now. We are lacking experience in the field and need to keep driving our > application development. > > Thus, with a heavy heart, we will instead implement a workaround which will > enable the LCD at boot time from user space. What works good for us is writing > to /sys/class/graphics/fb0/blank.
I think this makes sense, the uapi for fbdev isn't so firmly established anyway. And it definitely doesn't hurt (at least on drm-kms drivers, where we no-op out changes that change nothing). Just in general, if you hit something like this, we're definitely interested in making fbdev a more well-defined uapi that can be relied upon a bit more across drivers. 20+ years after it landed, but hey if it keeps userspace happy, it imo makes sense. -Daniel > > Greetings, > Mark > > Building Technologies, Panel Software Fire (BT-FIR/ENG1-Grb) > Bosch Sicherheitssysteme GmbH | Postfach 11 11 | 85626 Grasbrunn | GERMANY | > www.boschsecurity.com > > Sitz: Stuttgart, Registergericht: Amtsgericht Stuttgart HRB 23118 > Aufsichtsratsvorsitzender: Christian Fischer; Geschäftsführung: Tanja > Rückert, Andreas Bartz, Thomas Quante > > 100 Years Bosch Building Technologies 1920-2020 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
