* [PATCH] drm: imx: Move fbdev setup to before output polling @ 2020-11-17 15:52 Mark Jonas 2020-11-18 8:10 ` Thomas Zimmermann 0 siblings, 1 reply; 8+ messages in thread From: Mark Jonas @ 2020-11-17 15:52 UTC (permalink / raw) To: Philipp Zabel, David Airlie, Daniel Vetter, Shawn Guo, Sascha Hauer, Fabio Estevam Cc: Pengutronix Kernel Team, NXP Linux Team, dri-devel, linux-arm-kernel, linux-kernel, Leo Ruan, Mark Jonas From: Leo Ruan <tingquan.ruan@cn.bosch.com> The generic fbdev has to be setup before enabling output polling. Otherwise the fbdev client is not ready to handle delayed events. Since f53705fd, the generic fbdev is setup after the output polling init. During fbdev setup, when fbdev probes attached outputs and a status changes from unknown to connected, the delayed event is marked and the output_poll_work thread is scheduled without delay. If output_poll_execute() is runs immediately, the delayed event is handled without actually polling the output because the fbdev is not registered yet. So the delayed event is lost. This leads to a dark screen until a KMS application (or fbcon) sets the screen mode. This patch fixes the issue by moving the setup of generic fbdev before initializing and enabling output polling. Signed-off-by: Leo Ruan <tingquan.ruan@cn.bosch.com> Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com> --- drivers/gpu/drm/imx/imx-drm-core.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) 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); - return 0; err_poll_fini: -- 2.17.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] drm: imx: Move fbdev setup to before output polling 2020-11-17 15:52 [PATCH] drm: imx: Move fbdev setup to before output polling Mark Jonas @ 2020-11-18 8:10 ` Thomas Zimmermann 2020-11-18 8:59 ` Daniel Vetter 0 siblings, 1 reply; 8+ messages in thread From: Thomas Zimmermann @ 2020-11-18 8:10 UTC (permalink / raw) To: Mark Jonas, Philipp Zabel, David Airlie, Daniel Vetter, Shawn Guo, Sascha Hauer, Fabio Estevam Cc: Leo Ruan, linux-kernel, dri-devel, NXP Linux Team, Pengutronix Kernel Team, linux-arm-kernel Hi Am 17.11.20 um 16:52 schrieb Mark Jonas: > From: Leo Ruan <tingquan.ruan@cn.bosch.com> > > The generic fbdev has to be setup before enabling output polling. > Otherwise the fbdev client is not ready to handle delayed events. > > Since f53705fd, the generic fbdev is setup after the output polling > init. During fbdev setup, when fbdev probes attached outputs and a > status changes from unknown to connected, the delayed event is > marked and the output_poll_work thread is scheduled without delay. > If output_poll_execute() is runs immediately, the delayed event > is handled without actually polling the output because the fbdev is not > registered yet. So the delayed event is lost. This leads to a dark > screen until a KMS application (or fbcon) sets the screen mode. > > This patch fixes the issue by moving the setup of generic fbdev before > initializing and enabling output polling. > > Signed-off-by: Leo Ruan <tingquan.ruan@cn.bosch.com> > Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com> > --- > drivers/gpu/drm/imx/imx-drm-core.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > 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. Best regards Thomas > return 0; > > err_poll_fini: > -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm: imx: Move fbdev setup to before output polling 2020-11-18 8:10 ` Thomas Zimmermann @ 2020-11-18 8:59 ` Daniel Vetter 2020-11-18 9:12 ` Thomas Zimmermann 0 siblings, 1 reply; 8+ messages in thread From: Daniel Vetter @ 2020-11-18 8:59 UTC (permalink / raw) To: Thomas Zimmermann Cc: Mark Jonas, Philipp Zabel, David Airlie, Shawn Guo, Sascha Hauer, Fabio Estevam, Leo Ruan, Linux Kernel Mailing List, dri-devel, NXP Linux Team, Pengutronix Kernel Team, Linux ARM On Wed, Nov 18, 2020 at 9:10 AM Thomas Zimmermann <tzimmermann@suse.de> wrote: > > Hi > > Am 17.11.20 um 16:52 schrieb Mark Jonas: > > From: Leo Ruan <tingquan.ruan@cn.bosch.com> > > > > The generic fbdev has to be setup before enabling output polling. > > Otherwise the fbdev client is not ready to handle delayed events. > > > > Since f53705fd, the generic fbdev is setup after the output polling > > init. During fbdev setup, when fbdev probes attached outputs and a > > status changes from unknown to connected, the delayed event is > > marked and the output_poll_work thread is scheduled without delay. > > If output_poll_execute() is runs immediately, the delayed event > > is handled without actually polling the output because the fbdev is not > > registered yet. So the delayed event is lost. This leads to a dark > > screen until a KMS application (or fbcon) sets the screen mode. > > > > This patch fixes the issue by moving the setup of generic fbdev before > > initializing and enabling output polling. > > > > Signed-off-by: Leo Ruan <tingquan.ruan@cn.bosch.com> > > Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com> > > --- > > drivers/gpu/drm/imx/imx-drm-core.c | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > 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. -Daniel > > Best regards > Thomas > > > return 0; > > > > err_poll_fini: > > > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Felix Imendörffer -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm: imx: Move fbdev setup to before output polling 2020-11-18 8:59 ` Daniel Vetter @ 2020-11-18 9:12 ` Thomas Zimmermann 2020-11-18 9:47 ` AW: " Jonas Mark (BT-FIR/ENG1-Grb) 0 siblings, 1 reply; 8+ messages in thread From: Thomas Zimmermann @ 2020-11-18 9:12 UTC (permalink / raw) To: Daniel Vetter Cc: Leo Ruan, Pengutronix Kernel Team, David Airlie, Sascha Hauer, Linux Kernel Mailing List, dri-devel, NXP Linux Team, Mark Jonas, Shawn Guo, Linux ARM [-- Attachment #1.1.1: Type: text/plain, Size: 4153 bytes --] Hi Am 18.11.20 um 09:59 schrieb Daniel Vetter: > On Wed, Nov 18, 2020 at 9:10 AM Thomas Zimmermann <tzimmermann@suse.de> wrote: >> >> Hi >> >> Am 17.11.20 um 16:52 schrieb Mark Jonas: >>> From: Leo Ruan <tingquan.ruan@cn.bosch.com> >>> >>> The generic fbdev has to be setup before enabling output polling. >>> Otherwise the fbdev client is not ready to handle delayed events. >>> >>> Since f53705fd, the generic fbdev is setup after the output polling >>> init. During fbdev setup, when fbdev probes attached outputs and a >>> status changes from unknown to connected, the delayed event is >>> marked and the output_poll_work thread is scheduled without delay. >>> If output_poll_execute() is runs immediately, the delayed event >>> is handled without actually polling the output because the fbdev is not >>> registered yet. So the delayed event is lost. This leads to a dark >>> screen until a KMS application (or fbcon) sets the screen mode. >>> >>> This patch fixes the issue by moving the setup of generic fbdev before >>> initializing and enabling output polling. >>> >>> Signed-off-by: Leo Ruan <tingquan.ruan@cn.bosch.com> >>> Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com> >>> --- >>> drivers/gpu/drm/imx/imx-drm-core.c | 8 ++++++-- >>> 1 file changed, 6 insertions(+), 2 deletions(-) >>> >>> 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. Best regards Thomas > -Daniel > >> >> Best regards >> Thomas >> >>> return 0; >>> >>> err_poll_fini: >>> >> >> -- >> Thomas Zimmermann >> Graphics Driver Developer >> SUSE Software Solutions Germany GmbH >> Maxfeldstr. 5, 90409 Nürnberg, Germany >> (HRB 36809, AG Nürnberg) >> Geschäftsführer: Felix Imendörffer > > > -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer [-- Attachment #1.1.2: OpenPGP_0x680DC11D530B7A23.asc --] [-- Type: application/pgp-keys, Size: 7535 bytes --] [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 840 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* AW: [PATCH] drm: imx: Move fbdev setup to before output polling 2020-11-18 9:12 ` Thomas Zimmermann @ 2020-11-18 9:47 ` Jonas Mark (BT-FIR/ENG1-Grb) 2020-11-18 10:23 ` Daniel Vetter 0 siblings, 1 reply; 8+ messages in thread From: Jonas Mark (BT-FIR/ENG1-Grb) @ 2020-11-18 9:47 UTC (permalink / raw) To: Thomas Zimmermann, Daniel Vetter Cc: RUAN Tingquan (BT-FIR/ENG1-Zhu), Pengutronix Kernel Team, David Airlie, Sascha Hauer, Linux Kernel Mailing List, dri-devel, NXP Linux Team, Shawn Guo, Linux ARM, Jonas Mark (BT-FIR/ENG1-Grb) Hi Thomas and 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://elixir.bootlin.com/linux/v5.10-rc4/source/drivers/gpu/drm/imx/imx-drm-core.c#L231 We are a little clueless right now: How do we pick up the device settings? Greetings, Mark ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm: imx: Move fbdev setup to before output polling 2020-11-18 9:47 ` AW: " Jonas Mark (BT-FIR/ENG1-Grb) @ 2020-11-18 10:23 ` Daniel Vetter 2020-11-26 9:44 ` AW: " Jonas Mark (BT-FIR/ENG1-Grb) 0 siblings, 1 reply; 8+ messages in thread From: Daniel Vetter @ 2020-11-18 10:23 UTC (permalink / raw) To: Jonas Mark (BT-FIR/ENG1-Grb) Cc: Thomas Zimmermann, RUAN Tingquan (BT-FIR/ENG1-Zhu), Pengutronix Kernel Team, David Airlie, Sascha Hauer, Linux Kernel Mailing List, dri-devel, NXP Linux Team, Shawn Guo, Linux ARM On Wed, Nov 18, 2020 at 10:47 AM Jonas Mark (BT-FIR/ENG1-Grb) <Mark.Jonas@de.bosch.com> wrote: > > Hi Thomas and 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://elixir.bootlin.com/linux/v5.10-rc4/source/drivers/gpu/drm/imx/imx-drm-core.c#L231 > > 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://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#validating-changes-with-igt for some details on our validation testing, there's already a very basic fbdev testcase there. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 8+ messages in thread
* AW: [PATCH] drm: imx: Move fbdev setup to before output polling 2020-11-18 10:23 ` Daniel Vetter @ 2020-11-26 9:44 ` Jonas Mark (BT-FIR/ENG1-Grb) 2020-11-27 14:50 ` Daniel Vetter 0 siblings, 1 reply; 8+ messages in thread From: Jonas Mark (BT-FIR/ENG1-Grb) @ 2020-11-26 9:44 UTC (permalink / raw) To: Daniel Vetter Cc: Thomas Zimmermann, RUAN Tingquan (BT-FIR/ENG1-Zhu), Pengutronix Kernel Team, David Airlie, Sascha Hauer, Linux Kernel Mailing List, dri-devel, NXP Linux Team, Shawn Guo, Linux ARM 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. 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm: imx: Move fbdev setup to before output polling 2020-11-26 9:44 ` AW: " Jonas Mark (BT-FIR/ENG1-Grb) @ 2020-11-27 14:50 ` Daniel Vetter 0 siblings, 0 replies; 8+ messages in thread From: Daniel Vetter @ 2020-11-27 14:50 UTC (permalink / raw) To: Jonas Mark (BT-FIR/ENG1-Grb) Cc: Daniel Vetter, Thomas Zimmermann, RUAN Tingquan (BT-FIR/ENG1-Zhu), Pengutronix Kernel Team, David Airlie, Sascha Hauer, Linux Kernel Mailing List, dri-devel, NXP Linux Team, Shawn Guo, Linux ARM 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-11-27 14:51 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-17 15:52 [PATCH] drm: imx: Move fbdev setup to before output polling Mark Jonas 2020-11-18 8:10 ` Thomas Zimmermann 2020-11-18 8:59 ` Daniel Vetter 2020-11-18 9:12 ` Thomas Zimmermann 2020-11-18 9:47 ` AW: " Jonas Mark (BT-FIR/ENG1-Grb) 2020-11-18 10:23 ` Daniel Vetter 2020-11-26 9:44 ` AW: " Jonas Mark (BT-FIR/ENG1-Grb) 2020-11-27 14:50 ` Daniel Vetter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).