Skip to contentSkip to conversation

ARM: imx: Make DRM_MXSFB and FB_MXS coexist

PATCH In review Leonard Crestez 6 participants 22 messages Jun 15, 2018 – Jul 13, 2018

Subsystems (6)
DRM drivers Arm (32-bit) DRM misc Arm i.MX and Layerscape +2 more subsystems
lore
Share & tools
lore

Leonard Crestez

author
#0 lore raw

cover ARM: imx: Make DRM_MXSFB and FB_MXS coexist

Some imx boards have new drm-style bindings for lcdif devices but using them requires rebuilding the kernel with FB_MXS=n DRM_MXSFB=y.

It is relatively easy to allow both drivers to coexist by renaming drm/mxsfb to "mxsfb-drm" and making the old fbdev driver return -ENODEV instead of -ENOENT when it fails to find the display node.

This makes display on imx6sx-sdb "just work" with both styles of bindings. In order to test the old bindings I locally reverted commit 7caa59e0d40c ("ARM: dts: imx6sx-sdb: Convert from fbdev to drm bindings")

Leonard Crestez (3):
drm: mxsfb: Change driver.name to mxsfb-drm
fbdev: mxsfb: Return ENODEV on missing display node
ARM: imx_v6_v7_defconfig: Enable DRM_MXSFB

 arch/arm/configs/imx_v6_v7_defconfig | 2 ++
 drivers/gpu/drm/mxsfb/mxsfb_drv.c    | 2 +-
 drivers/video/fbdev/mxsfb.c          | 2 +-
 3 files changed, 4 insertions(+), 2 deletions(-)
Signature · 1 line
2.17.1
author
#1 lore raw

PATCH 1/3 drm: mxsfb: Change driver.name to mxsfb-drm

The FBDEV driver uses the same name and both can't be registered at the same time. Fix this by renaming the drm driver to mxsfb-drm

Signed-off-by Leonard Crestez <[email protected]>
---
 drivers/gpu/drm/mxsfb/mxsfb_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/mxsfb/mxsfb_drv.c +1 -1
index ffe5137ccaf8..dd1dd58e4956 100644
@@ -417,11 +417,11 @@ static int mxsfb_remove(struct platform_device *pdev)
 static struct platform_driver mxsfb_platform_driver = {
 	.probe		= mxsfb_probe,
 	.remove		= mxsfb_remove,
 	.id_table	= mxsfb_devtype,
 	.driver	= {
-		.name		= "mxsfb",
+		.name		= "mxsfb-drm",
 		.of_match_table	= mxsfb_dt_ids,
 	},
 };
 
 module_platform_driver(mxsfb_platform_driver);
Signature · 1 line
2.17.1
author
#3 lore raw

Re: [PATCH 1/3] drm: mxsfb: Change driver.name to mxsfb-drm

Quoted 9 lines · Fabio Estevam
Quoted 4 lines · Leonard Crestez

The FBDEV driver uses the same name and both can't be registered at the same time. Fix this by renaming the drm driver to mxsfb-drm

Signed-off-by Leonard Crestez <[email protected]>

Stefan sent the same patch a few days ago:
http://www.spinics.net/lists/dri-devel/msg179489.html

In that thread there is a proposal for removing the old fbdev/mxsfb driver entirely.

That would break old DTBs, isn't this generally considered bad? Also, are we sure the removal of fbdev/mxsfb wouldn't lose any features?

What my series does is make both drivers work with the same kernel image and turns the choice into a board-level dtb decision. Supporting everything at once seems desirable to me and it allows for a very smooth upgrade path.

The old driver could be removed later, after all users are converted.

--
Regards,
Leonard

reviewer
#4 lore raw

Re: [PATCH 1/3] drm: mxsfb: Change driver.name to mxsfb-drm

Quoted 5 lines · Leonard Crestez

In that thread there is a proposal for removing the old fbdev/mxsfb driver entirely.

That would break old DTBs, isn't this generally considered bad? Also, are we sure the removal of fbdev/mxsfb wouldn't lose any features?

Yes, I also think we should not break old dtb's.

Quoted 6 lines

What my series does is make both drivers work with the same kernel image and turns the choice into a board-level dtb decision. Supporting everything at once seems desirable to me and it allows for a very smooth upgrade path.

The old driver could be removed later, after all users are converted.

Agreed.

Maybe the mxs fbdev driver needs to warn users saying that that the driver is deprecated and the bindings need to be updated to the DRM bindings style.

maintainer
#5 lore raw

Re: [PATCH 1/3] drm: mxsfb: Change driver.name to mxsfb-drm

In reply to Leonard Crestez
Quoted 15 lines · Leonard Crestez
Quoted 9 lines · Fabio Estevam
Quoted 4 lines · Leonard Crestez

The FBDEV driver uses the same name and both can't be registered at the same time. Fix this by renaming the drm driver to mxsfb-drm

Signed-off-by Leonard Crestez <[email protected]>

Stefan sent the same patch a few days ago:
http://www.spinics.net/lists/dri-devel/msg179489.html

In that thread there is a proposal for removing the old fbdev/mxsfb driver entirely.

Quoted 7 lines

That would break old DTBs, isn't this generally considered bad? Also, are we sure the removal of fbdev/mxsfb wouldn't lose any features?

What my series does is make both drivers work with the same kernel image and turns the choice into a board-level dtb decision. Supporting everything at once seems desirable to me and it allows for a very smooth upgrade path.

Having two drivers in the kernel with different set of bugs is always bad.

Quoted 1 line

The old driver could be removed later, after all users are converted.

Both drivers were in for long enough already. And let's be realistic, how many MX23/MX28 users of old DTs with new kernels are there who cannot update the DT as well ?

IMO keeping the old FBDEV driver in is just an excuse to postpone the long overdue DT update and I dislike that. Update any remaining DTs and nuke the FBDEV driver already.

Signature · 2 lines
Best regards,
Marek Vasut
reviewer
#6 lore raw

Re: [PATCH 1/3] drm: mxsfb: Change driver.name to mxsfb-drm

Quoted 1 line · Marek Vasut

Having two drivers in the kernel with different set of bugs is always bad.

Sure, but breaking dtb's is also bad.

Can the mxsfb driver be modified to handle the old style bindings?

The IPU drm driver is capable of handling both the old style where the display timing is passed in dts and the new drm style.

For example:

arch/arm/boot/dts/imx6qdl-sabresd.dtsi uses the drm style binding arch/arm/boot/dts/imx6qdl-sabreauto.dtsi uses the old style of passing the display timings in dts

Both formats are accepted by the ipu drm driver.

Can't mxsfb drm driver support both? Then we don't need to worry about breaking dtb's and could safely remove the mxs fbdev driver.

#7 lore raw

Re: [PATCH 1/3] drm: mxsfb: Change driver.name to mxsfb-drm

Quoted 5 lines · Fabio Estevam
Quoted 1 line · Marek Vasut

Having two drivers in the kernel with different set of bugs is always bad.

Sure, but breaking dtb's is also bad.

You picked only the first part of my argument, the less important one ;-)

Quoted 1 line

Can the mxsfb driver be modified to handle the old style bindings?

Maybe, but do we care ? Cfr my comment about the amount of users who will be affected by this.

Quoted 5 lines

The IPU drm driver is capable of handling both the old style where the display timing is passed in dts and the new drm style.

For example:

Quoted 8 lines

arch/arm/boot/dts/imx6qdl-sabresd.dtsi uses the drm style binding arch/arm/boot/dts/imx6qdl-sabreauto.dtsi uses the old style of passing the display timings in dts

Both formats are accepted by the ipu drm driver.

Can't mxsfb drm driver support both? Then we don't need to worry about breaking dtb's and could safely remove the mxs fbdev driver.

This might be a way forward, but again, does it justify the effort ? We will be adding compatibility code which we will have to maintain for maybe a handful of users I think.

Signature · 2 lines
Best regards,
Marek Vasut
author
#8 lore raw

Re: [PATCH 1/3] drm: mxsfb: Change driver.name to mxsfb-drm

In reply to Marek Vasut
Quoted 21 lines · Marek Vasut
Quoted 12 lines · Leonard Crestez
Quoted 7 lines · Fabio Estevam
Quoted 2 lines · Leonard Crestez

The FBDEV driver uses the same name and both can't be registered at the same time. Fix this by renaming the drm driver to mxsfb-drm

Stefan sent the same patch a few days ago:

In that thread there is a proposal for removing the old fbdev/mxsfb driver entirely.

Quoted 7 lines

That would break old DTBs, isn't this generally considered bad? Also, are we sure the removal of fbdev/mxsfb wouldn't lose any features?

What my series does is make both drivers work with the same kernel image and turns the choice into a board-level dtb decision. Supporting everything at once seems desirable to me and it allows for a very smooth upgrade path.

Quoted 7 lines

Having two drivers in the kernel with different set of bugs is always bad.

Quoted 1 line

The old driver could be removed later, after all users are converted.

Both drivers were in for long enough already. And let's be realistic, how many MX23/MX28 users of old DTs with new kernels are there who cannot update the DT as well ?

Grepping for "display =" in arch/arm/boot/dts/imx* I see that old bindings are also used by 3rd-party boards for imx6/7:
* imx6sx-nitrogen6sx
* imx6ul-geam
* imx6ul-isiot
* imx6ul-opos6uldev
* imx6ul-pico-hobbit
* imx6ul-tx6ul
* imx7d-nitrogen7

Converting everything might be quite a bit of work, and explicitly supporting old bindings is also work.

It is very confusing that there is a whole set of displays for imx6/7 which are supported by upstream but only with a non-default config. While it is extremely common in the embedded field to have custom configs the default one in the kernel should try to "just work".

Couldn't this patch series be considered a bugfix? It was also surprisingly small.

--
Regards,
Leonard

#9 lore raw

Re: [PATCH 1/3] drm: mxsfb: Change driver.name to mxsfb-drm

Quoted 30 lines · Leonard Crestez
Quoted 21 lines · Marek Vasut
Quoted 12 lines · Leonard Crestez
Quoted 7 lines · Fabio Estevam
Quoted 2 lines · Leonard Crestez

The FBDEV driver uses the same name and both can't be registered at the same time. Fix this by renaming the drm driver to mxsfb-drm

Stefan sent the same patch a few days ago:

In that thread there is a proposal for removing the old fbdev/mxsfb driver entirely.

Quoted 7 lines

That would break old DTBs, isn't this generally considered bad? Also, are we sure the removal of fbdev/mxsfb wouldn't lose any features?

What my series does is make both drivers work with the same kernel image and turns the choice into a board-level dtb decision. Supporting everything at once seems desirable to me and it allows for a very smooth upgrade path.

Quoted 7 lines

Having two drivers in the kernel with different set of bugs is always bad.

Quoted 1 line

The old driver could be removed later, after all users are converted.

Both drivers were in for long enough already. And let's be realistic, how many MX23/MX28 users of old DTs with new kernels are there who cannot update the DT as well ?

Quoted 9 lines

Grepping for "display =" in arch/arm/boot/dts/imx* I see that old bindings are also used by 3rd-party boards for imx6/7:
* imx6sx-nitrogen6sx
* imx6ul-geam
* imx6ul-isiot
* imx6ul-opos6uldev
* imx6ul-pico-hobbit
* imx6ul-tx6ul
* imx7d-nitrogen7

Er, yes, a handful of boards which could be updated :)

Quoted 2 lines

Converting everything might be quite a bit of work, and explicitly supporting old bindings is also work.

Does adding support for old bindings justify the effort invested ? I doubt so, it only adds more code to maintain.

Quoted 7 lines

It is very confusing that there is a whole set of displays for imx6/7 which are supported by upstream but only with a non-default config. While it is extremely common in the embedded field to have custom configs the default one in the kernel should try to "just work".

Couldn't this patch series be considered a bugfix? It was also surprisingly small.

I think it's just a workaround which allows you to postpone the real fix, and I don't like that.

Signature · 2 lines
Best regards,
Marek Vasut
#10 lore raw

Re: [PATCH 1/3] drm: mxsfb: Change driver.name to mxsfb-drm

Quoted 54 lines · Marek Vasut
Quoted 30 lines · Leonard Crestez
Quoted 21 lines · Marek Vasut
Quoted 12 lines · Leonard Crestez
Quoted 7 lines · Fabio Estevam
Quoted 2 lines · Leonard Crestez

The FBDEV driver uses the same name and both can't be registered at the same time. Fix this by renaming the drm driver to mxsfb-drm

Stefan sent the same patch a few days ago:

In that thread there is a proposal for removing the old fbdev/mxsfb driver entirely.

Quoted 7 lines

That would break old DTBs, isn't this generally considered bad? Also, are we sure the removal of fbdev/mxsfb wouldn't lose any features?

What my series does is make both drivers work with the same kernel image and turns the choice into a board-level dtb decision. Supporting everything at once seems desirable to me and it allows for a very smooth upgrade path.

Quoted 7 lines

Having two drivers in the kernel with different set of bugs is always bad.

Quoted 1 line

The old driver could be removed later, after all users are converted.

Both drivers were in for long enough already. And let's be realistic, how many MX23/MX28 users of old DTs with new kernels are there who cannot update the DT as well ?

Quoted 9 lines

Grepping for "display =" in arch/arm/boot/dts/imx* I see that old bindings are also used by 3rd-party boards for imx6/7:
* imx6sx-nitrogen6sx
* imx6ul-geam
* imx6ul-isiot
* imx6ul-opos6uldev
* imx6ul-pico-hobbit
* imx6ul-tx6ul
* imx7d-nitrogen7

Er, yes, a handful of boards which could be updated :)

Quoted 2 lines

Converting everything might be quite a bit of work, and explicitly supporting old bindings is also work.

Does adding support for old bindings justify the effort invested ? I doubt so, it only adds more code to maintain.

Quoted 5 lines

It is very confusing that there is a whole set of displays for imx6/7 which are supported by upstream but only with a non-default config. While it is extremely common in the embedded field to have custom configs the default one in the kernel should try to "just work".

Quoted 5 lines
Quoted 2 lines

Couldn't this patch series be considered a bugfix? It was also surprisingly small.

I think it's just a workaround which allows you to postpone the real fix, and I don't like that.

Yeah agreed, imo the proper fix here would be to either update the dts for the affected boards and/or make mxsfb accept the old dt bindings for backwards compat. Artificially extending the life of the fbdev drivers seems silly.
-Daniel

Signature · 3 lines
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
maintainer
#11 lore raw

Re: [PATCH 1/3] drm: mxsfb: Change driver.name to mxsfb-drm

Quoted 58 lines · Daniel Vetter
Quoted 57 lines · Marek Vasut
Quoted 30 lines · Leonard Crestez
Quoted 21 lines · Marek Vasut
Quoted 12 lines · Leonard Crestez
Quoted 7 lines · Fabio Estevam
Quoted 2 lines · Leonard Crestez

The FBDEV driver uses the same name and both can't be registered at the same time. Fix this by renaming the drm driver to mxsfb-drm

Stefan sent the same patch a few days ago:

In that thread there is a proposal for removing the old fbdev/mxsfb driver entirely.

Quoted 7 lines

That would break old DTBs, isn't this generally considered bad? Also, are we sure the removal of fbdev/mxsfb wouldn't lose any features?

What my series does is make both drivers work with the same kernel image and turns the choice into a board-level dtb decision. Supporting everything at once seems desirable to me and it allows for a very smooth upgrade path.

Quoted 7 lines

Having two drivers in the kernel with different set of bugs is always bad.

Quoted 1 line

The old driver could be removed later, after all users are converted.

Both drivers were in for long enough already. And let's be realistic, how many MX23/MX28 users of old DTs with new kernels are there who cannot update the DT as well ?

Quoted 9 lines

Grepping for "display =" in arch/arm/boot/dts/imx* I see that old bindings are also used by 3rd-party boards for imx6/7:
* imx6sx-nitrogen6sx
* imx6ul-geam
* imx6ul-isiot
* imx6ul-opos6uldev
* imx6ul-pico-hobbit
* imx6ul-tx6ul
* imx7d-nitrogen7

Er, yes, a handful of boards which could be updated :)

Quoted 2 lines

Converting everything might be quite a bit of work, and explicitly supporting old bindings is also work.

Does adding support for old bindings justify the effort invested ? I doubt so, it only adds more code to maintain.

Quoted 7 lines

It is very confusing that there is a whole set of displays for imx6/7 which are supported by upstream but only with a non-default config. While it is extremely common in the embedded field to have custom configs the default one in the kernel should try to "just work".

Couldn't this patch series be considered a bugfix? It was also surprisingly small.

Quoted 7 lines
Quoted 2 lines

I think it's just a workaround which allows you to postpone the real fix, and I don't like that.

Yeah agreed, imo the proper fix here would be to either update the dts for the affected boards and/or make mxsfb accept the old dt bindings for backwards compat. Artificially extending the life of the fbdev drivers seems silly.

We shouldn't have merged a DRM driver with a driver name which conflicts with an existing driver... If anything, this is artificially shortening the lifetime of the fbdev driver :-)

Again, I am ok with removing the driver. I just think it is silly to do it just because of the conflicting driver name.

Maybe Sascha, original author of the mxs fbdev driver has an opinion on this?

--
Stefan

maintainer
#12 lore raw

Re: [PATCH 1/3] drm: mxsfb: Change driver.name to mxsfb-drm

In reply to Marek Vasut
Quoted 54 lines · Marek Vasut
Quoted 30 lines · Leonard Crestez
Quoted 21 lines · Marek Vasut
Quoted 12 lines · Leonard Crestez
Quoted 7 lines · Fabio Estevam
Quoted 2 lines · Leonard Crestez

The FBDEV driver uses the same name and both can't be registered at the same time. Fix this by renaming the drm driver to mxsfb-drm

Stefan sent the same patch a few days ago:

In that thread there is a proposal for removing the old fbdev/mxsfb driver entirely.

Quoted 7 lines

That would break old DTBs, isn't this generally considered bad? Also, are we sure the removal of fbdev/mxsfb wouldn't lose any features?

What my series does is make both drivers work with the same kernel image and turns the choice into a board-level dtb decision. Supporting everything at once seems desirable to me and it allows for a very smooth upgrade path.

Quoted 7 lines

Having two drivers in the kernel with different set of bugs is always bad.

Quoted 1 line

The old driver could be removed later, after all users are converted.

Both drivers were in for long enough already. And let's be realistic, how many MX23/MX28 users of old DTs with new kernels are there who cannot update the DT as well ?

Quoted 9 lines

Grepping for "display =" in arch/arm/boot/dts/imx* I see that old bindings are also used by 3rd-party boards for imx6/7:
* imx6sx-nitrogen6sx
* imx6ul-geam
* imx6ul-isiot
* imx6ul-opos6uldev
* imx6ul-pico-hobbit
* imx6ul-tx6ul
* imx7d-nitrogen7

Er, yes, a handful of boards which could be updated :)

Quoted 2 lines

Converting everything might be quite a bit of work, and explicitly supporting old bindings is also work.

Does adding support for old bindings justify the effort invested ? I doubt so, it only adds more code to maintain.

Quoted 5 lines

It is very confusing that there is a whole set of displays for imx6/7 which are supported by upstream but only with a non-default config. While it is extremely common in the embedded field to have custom configs the default one in the kernel should try to "just work".

Quoted 5 lines
Quoted 2 lines

Couldn't this patch series be considered a bugfix? It was also surprisingly small.

I think it's just a workaround which allows you to postpone the real fix, and I don't like that.

This is one of the situation where states quo is kinda the worst situation.

Currently imx_v6_v7_defconfig and mxs_defconfig actually still uses CONFIG_FB_MXS.

I understand that you'd rather prefer to move forward. I suggest we do it in steps.

In 4.19:

- Change DRM driver.name to mxsfb-drm so we avoid conflicts for now
- Remove CONFIG_FB_MXS from imx_v6_v7_defconfig/mxs_defconfig now, and only enable CONFIG_DRM_MXSFB=y
- Add (deprecated) to CONFIG_FB_MXS

In 4.19/4.20:
- Fix the above device trees

In 4.20/4.21:
- Remove FB_MXS

Does that sound reasonable? If yes, I can send the patch set to do step 1.

--
Stefan

maintainer
#13 lore raw

Re: [PATCH 1/3] drm: mxsfb: Change driver.name to mxsfb-drm

Quoted 67 lines · Stefan Agner
Quoted 54 lines · Marek Vasut
Quoted 30 lines · Leonard Crestez
Quoted 21 lines · Marek Vasut
Quoted 12 lines · Leonard Crestez
Quoted 7 lines · Fabio Estevam
Quoted 2 lines · Leonard Crestez

The FBDEV driver uses the same name and both can't be registered at the same time. Fix this by renaming the drm driver to mxsfb-drm

Stefan sent the same patch a few days ago:

In that thread there is a proposal for removing the old fbdev/mxsfb driver entirely.

Quoted 7 lines

That would break old DTBs, isn't this generally considered bad? Also, are we sure the removal of fbdev/mxsfb wouldn't lose any features?

What my series does is make both drivers work with the same kernel image and turns the choice into a board-level dtb decision. Supporting everything at once seems desirable to me and it allows for a very smooth upgrade path.

Quoted 7 lines

Having two drivers in the kernel with different set of bugs is always bad.

Quoted 1 line

The old driver could be removed later, after all users are converted.

Both drivers were in for long enough already. And let's be realistic, how many MX23/MX28 users of old DTs with new kernels are there who cannot update the DT as well ?

Quoted 9 lines

Grepping for "display =" in arch/arm/boot/dts/imx* I see that old bindings are also used by 3rd-party boards for imx6/7:
* imx6sx-nitrogen6sx
* imx6ul-geam
* imx6ul-isiot
* imx6ul-opos6uldev
* imx6ul-pico-hobbit
* imx6ul-tx6ul
* imx7d-nitrogen7

Er, yes, a handful of boards which could be updated :)

Quoted 2 lines

Converting everything might be quite a bit of work, and explicitly supporting old bindings is also work.

Does adding support for old bindings justify the effort invested ? I doubt so, it only adds more code to maintain.

Quoted 5 lines

It is very confusing that there is a whole set of displays for imx6/7 which are supported by upstream but only with a non-default config. While it is extremely common in the embedded field to have custom configs the default one in the kernel should try to "just work".

Quoted 5 lines
Quoted 2 lines

Couldn't this patch series be considered a bugfix? It was also surprisingly small.

I think it's just a workaround which allows you to postpone the real fix, and I don't like that.

This is one of the situation where states quo is kinda the worst situation.

Currently imx_v6_v7_defconfig and mxs_defconfig actually still uses CONFIG_FB_MXS.

Quoted 6 lines

I understand that you'd rather prefer to move forward. I suggest we do it in steps.

In 4.19:

- Change DRM driver.name to mxsfb-drm so we avoid conflicts for now

But this will break mesa if it depends on mxsfb name for ie. etnaviv binding.

Quoted 4 lines

- Remove CONFIG_FB_MXS from imx_v6_v7_defconfig/mxs_defconfig now, and only enable CONFIG_DRM_MXSFB=y
- Add (deprecated) to CONFIG_FB_MXS

Quoted 8 lines

In 4.19/4.20:
- Fix the above device trees

In 4.20/4.21:
- Remove FB_MXS

Does that sound reasonable? If yes, I can send the patch set to do step 1.

Can you fix the DTs for 4.19 too ?

Signature · 2 lines
Best regards,
Marek Vasut
maintainer
#14 lore raw

Re: [PATCH 1/3] drm: mxsfb: Change driver.name to mxsfb-drm

Quoted 71 lines · Marek Vasut
Quoted 70 lines · Stefan Agner
Quoted 54 lines · Marek Vasut
Quoted 30 lines · Leonard Crestez
Quoted 21 lines · Marek Vasut
Quoted 12 lines · Leonard Crestez
Quoted 7 lines · Fabio Estevam
Quoted 2 lines · Leonard Crestez

The FBDEV driver uses the same name and both can't be registered at the same time. Fix this by renaming the drm driver to mxsfb-drm

Stefan sent the same patch a few days ago:

In that thread there is a proposal for removing the old fbdev/mxsfb driver entirely.

Quoted 7 lines

That would break old DTBs, isn't this generally considered bad? Also, are we sure the removal of fbdev/mxsfb wouldn't lose any features?

What my series does is make both drivers work with the same kernel image and turns the choice into a board-level dtb decision. Supporting everything at once seems desirable to me and it allows for a very smooth upgrade path.

Quoted 7 lines

Having two drivers in the kernel with different set of bugs is always bad.

Quoted 1 line

The old driver could be removed later, after all users are converted.

Both drivers were in for long enough already. And let's be realistic, how many MX23/MX28 users of old DTs with new kernels are there who cannot update the DT as well ?

Quoted 9 lines

Grepping for "display =" in arch/arm/boot/dts/imx* I see that old bindings are also used by 3rd-party boards for imx6/7:
* imx6sx-nitrogen6sx
* imx6ul-geam
* imx6ul-isiot
* imx6ul-opos6uldev
* imx6ul-pico-hobbit
* imx6ul-tx6ul
* imx7d-nitrogen7

Er, yes, a handful of boards which could be updated :)

Quoted 2 lines

Converting everything might be quite a bit of work, and explicitly supporting old bindings is also work.

Does adding support for old bindings justify the effort invested ? I doubt so, it only adds more code to maintain.

Quoted 5 lines

It is very confusing that there is a whole set of displays for imx6/7 which are supported by upstream but only with a non-default config. While it is extremely common in the embedded field to have custom configs the default one in the kernel should try to "just work".

Quoted 5 lines
Quoted 2 lines

Couldn't this patch series be considered a bugfix? It was also surprisingly small.

I think it's just a workaround which allows you to postpone the real fix, and I don't like that.

This is one of the situation where states quo is kinda the worst situation.

Currently imx_v6_v7_defconfig and mxs_defconfig actually still uses CONFIG_FB_MXS.

I understand that you'd rather prefer to move forward. I suggest we do it in steps.

Quoted 7 lines
Quoted 3 lines

In 4.19:

- Change DRM driver.name to mxsfb-drm so we avoid conflicts for now

But this will break mesa if it depends on mxsfb name for ie. etnaviv binding.

Does it? grep -r -e mxsfb in libdrm and mesa master returns nothing.

There is also .name in struct drm_driver, which is already set to mxsfb-drm... Is that the one exposed to user space?

Quoted 7 lines
Quoted 7 lines

- Remove CONFIG_FB_MXS from imx_v6_v7_defconfig/mxs_defconfig now, and only enable CONFIG_DRM_MXSFB=y
- Add (deprecated) to CONFIG_FB_MXS

In 4.19/4.20:
- Fix the above device trees

Quoted 7 lines
Quoted 5 lines

In 4.20/4.21:
- Remove FB_MXS

Does that sound reasonable? If yes, I can send the patch set to do step 1.

Can you fix the DTs for 4.19 too ?

Getting tight, but will try.

--
Stefan

author
#15 lore raw

Re: [PATCH 1/3] drm: mxsfb: Change driver.name to mxsfb-drm

Quoted 15 lines · Stefan Agner
Quoted 14 lines · Marek Vasut
Quoted 12 lines · Stefan Agner

This is one of the situation where states quo is kinda the worst situation.

Currently imx_v6_v7_defconfig and mxs_defconfig actually still uses CONFIG_FB_MXS.

I understand that you'd rather prefer to move forward. I suggest we do it in steps.

In 4.19:

- Change DRM driver.name to mxsfb-drm so we avoid conflicts for now

Quoted 7 lines
Quoted 2 lines

But this will break mesa if it depends on mxsfb name for ie. etnaviv binding.

Does it? grep -r -e mxsfb in libdrm and mesa master returns nothing.

There is also .name in struct drm_driver, which is already set to mxsfb-drm... Is that the one exposed to user space?

Running etnaviv+x11 with a renamed mxsfb driver works fine on imx6sx- sdb.

Tools like modetest already need -M mxsfb-drm, the drm_driver.name seems to be what matters.

Quoted 2 lines

- Remove CONFIG_FB_MXS from imx_v6_v7_defconfig/mxs_defconfig now, and only enable CONFIG_DRM_MXSFB=y

If one of the drivers is renamed then they can coexist: since the bindings are distinct one driver will return a probe error and the other will bind successfully. This can even be adjusted so that it doesn't even print ugly scary errors.

This can last until somebody implements support for old bindings in the drm driver and then FB_MXS can just be deleted.

maintainer
#16 lore raw

Re: [PATCH 1/3] drm: mxsfb: Change driver.name to mxsfb-drm

Quoted 33 lines · Leonard Crestez
Quoted 15 lines · Stefan Agner
Quoted 14 lines · Marek Vasut
Quoted 12 lines · Stefan Agner

This is one of the situation where states quo is kinda the worst situation.

Currently imx_v6_v7_defconfig and mxs_defconfig actually still uses CONFIG_FB_MXS.

I understand that you'd rather prefer to move forward. I suggest we do it in steps.

In 4.19:

- Change DRM driver.name to mxsfb-drm so we avoid conflicts for now

Quoted 7 lines
Quoted 2 lines

But this will break mesa if it depends on mxsfb name for ie. etnaviv binding.

Does it? grep -r -e mxsfb in libdrm and mesa master returns nothing.

There is also .name in struct drm_driver, which is already set to mxsfb-drm... Is that the one exposed to user space?

Running etnaviv+x11 with a renamed mxsfb driver works fine on imx6sx- sdb.

Tools like modetest already need -M mxsfb-drm, the drm_driver.name seems to be what matters.

Quoted 2 lines

- Remove CONFIG_FB_MXS from imx_v6_v7_defconfig/mxs_defconfig now, and only enable CONFIG_DRM_MXSFB=y

Quoted 7 lines

If one of the drivers is renamed then they can coexist: since the bindings are distinct one driver will return a probe error and the other will bind successfully. This can even be adjusted so that it doesn't even print ugly scary errors.

This can last until somebody implements support for old bindings in the drm driver and then FB_MXS can just be deleted.

So why don't we just convert the DT bindings on boards supported upstream and zap the old driver ? What is the problem with that?

Realistically, how many MXS boards in the field use old DT and new kernel ?

Signature · 2 lines
Best regards,
Marek Vasut
#17 lore raw

Re: [PATCH 1/3] drm: mxsfb: Change driver.name to mxsfb-drm

Quoted 39 lines · Marek Vasut
Quoted 38 lines · Leonard Crestez
Quoted 15 lines · Stefan Agner
Quoted 14 lines · Marek Vasut
Quoted 12 lines · Stefan Agner

This is one of the situation where states quo is kinda the worst situation.

Currently imx_v6_v7_defconfig and mxs_defconfig actually still uses CONFIG_FB_MXS.

I understand that you'd rather prefer to move forward. I suggest we do it in steps.

In 4.19:

- Change DRM driver.name to mxsfb-drm so we avoid conflicts for now

Quoted 7 lines
Quoted 2 lines

But this will break mesa if it depends on mxsfb name for ie. etnaviv binding.

Does it? grep -r -e mxsfb in libdrm and mesa master returns nothing.

There is also .name in struct drm_driver, which is already set to mxsfb-drm... Is that the one exposed to user space?

Running etnaviv+x11 with a renamed mxsfb driver works fine on imx6sx- sdb.

Tools like modetest already need -M mxsfb-drm, the drm_driver.name seems to be what matters.

Quoted 2 lines

- Remove CONFIG_FB_MXS from imx_v6_v7_defconfig/mxs_defconfig now, and only enable CONFIG_DRM_MXSFB=y

If one of the drivers is renamed then they can coexist: since the bindings are distinct one driver will return a probe error and the other will bind successfully. This can even be adjusted so that it doesn't even print ugly scary errors.

Quoted 5 lines
Quoted 2 lines

This can last until somebody implements support for old bindings in the drm driver and then FB_MXS can just be deleted.

So why don't we just convert the DT bindings on boards supported upstream and zap the old driver ? What is the problem with that?

+1 on zapping drivers :-)

Quoted 1 line

Realistically, how many MXS boards in the field use old DT and new kernel ?

Yeah I think occasionally that entire "DT is API, can't ever change it" song is overblown. The rule with regressions isn't that you're never allowed to break anything, but that you're only allowed to break stuff no one will notice and report.

If there's realistically no users, go ahead and break (instead of huge and drawn-out compat plan).

If there's a solid (and automatic enough) fallback like the drm driver, go ahead and break.

We're doing this all the time in graphics with userspace ABI, I don't think DT is any different. Every once in a while there's a bit of regrets and a revert because we missed something, but overal it's much less effort than always trying to do a perfect job with backwards compat.

Is it really a regression if no one reports it? No.

Cheers, Daniel

Signature · 3 lines
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
maintainer
#18 lore raw

Re: [PATCH 1/3] drm: mxsfb: Change driver.name to mxsfb-drm

In reply to Leonard Crestez
Quoted 21 lines · Leonard Crestez
Quoted 20 lines · Stefan Agner
Quoted 10 lines · Marek Vasut
Quoted 9 lines · Stefan Agner

This is one of the situation where states quo is kinda the worst situation.

Currently imx_v6_v7_defconfig and mxs_defconfig actually still uses CONFIG_FB_MXS.

I understand that you'd rather prefer to move forward. I suggest we do it in steps.

Quoted 6 lines
Quoted 3 lines

In 4.19:

- Change DRM driver.name to mxsfb-drm so we avoid conflicts for now

But this will break mesa if it depends on mxsfb name for ie. etnaviv binding.

Does it? grep -r -e mxsfb in libdrm and mesa master returns nothing.

Quoted 8 lines
Quoted 2 lines

There is also .name in struct drm_driver, which is already set to mxsfb-drm... Is that the one exposed to user space?

Running etnaviv+x11 with a renamed mxsfb driver works fine on imx6sx- sdb.

Tools like modetest already need -M mxsfb-drm, the drm_driver.name seems to be what matters.

Ok, almost thought so, thanks for the confirmation! So we should be good.

Quoted 11 lines
Quoted 3 lines

- Remove CONFIG_FB_MXS from imx_v6_v7_defconfig/mxs_defconfig now, and only enable CONFIG_DRM_MXSFB=y

If one of the drivers is renamed then they can coexist: since the bindings are distinct one driver will return a probe error and the other will bind successfully. This can even be adjusted so that it doesn't even print ugly scary errors.

This can last until somebody implements support for old bindings in the drm driver and then FB_MXS can just be deleted.

Yeah I guess that is what Marek don't want because it promotes using FB_MXS for longer than needed.

I don't care as much since we anyway use the MXSFB DRM driver. However, what I really dislike is that a kernel compiled with both drivers currently leads to MXSFB DRM being unusable (because fbdev gets probed first). I feel removing the MXS_FB is rather harsh, so I *really* would love to see the MXSFB DRM driver renamed.

--
Stefan

maintainer
#19 lore raw

Re: [PATCH 1/3] drm: mxsfb: Change driver.name to mxsfb-drm

In reply to Marek Vasut

[adding Authors of the problematic device trees]

Quoted 85 lines · Marek Vasut
Quoted 67 lines · Stefan Agner
Quoted 54 lines · Marek Vasut
Quoted 30 lines · Leonard Crestez
Quoted 21 lines · Marek Vasut
Quoted 12 lines · Leonard Crestez
Quoted 7 lines · Fabio Estevam
Quoted 2 lines · Leonard Crestez

The FBDEV driver uses the same name and both can't be registered at the same time. Fix this by renaming the drm driver to mxsfb-drm

Stefan sent the same patch a few days ago:

In that thread there is a proposal for removing the old fbdev/mxsfb driver entirely.

Quoted 7 lines

That would break old DTBs, isn't this generally considered bad? Also, are we sure the removal of fbdev/mxsfb wouldn't lose any features?

What my series does is make both drivers work with the same kernel image and turns the choice into a board-level dtb decision. Supporting everything at once seems desirable to me and it allows for a very smooth upgrade path.

Quoted 7 lines

Having two drivers in the kernel with different set of bugs is always bad.

Quoted 1 line

The old driver could be removed later, after all users are converted.

Both drivers were in for long enough already. And let's be realistic, how many MX23/MX28 users of old DTs with new kernels are there who cannot update the DT as well ?

Quoted 9 lines

Grepping for "display =" in arch/arm/boot/dts/imx* I see that old bindings are also used by 3rd-party boards for imx6/7:
* imx6sx-nitrogen6sx
* imx6ul-geam
* imx6ul-isiot
* imx6ul-opos6uldev
* imx6ul-pico-hobbit
* imx6ul-tx6ul
* imx7d-nitrogen7

Er, yes, a handful of boards which could be updated :)

Quoted 2 lines

Converting everything might be quite a bit of work, and explicitly supporting old bindings is also work.

Does adding support for old bindings justify the effort invested ? I doubt so, it only adds more code to maintain.

Quoted 5 lines

It is very confusing that there is a whole set of displays for imx6/7 which are supported by upstream but only with a non-default config. While it is extremely common in the embedded field to have custom configs the default one in the kernel should try to "just work".

Quoted 5 lines
Quoted 2 lines

Couldn't this patch series be considered a bugfix? It was also surprisingly small.

I think it's just a workaround which allows you to postpone the real fix, and I don't like that.

This is one of the situation where states quo is kinda the worst situation.

Currently imx_v6_v7_defconfig and mxs_defconfig actually still uses CONFIG_FB_MXS.

Quoted 6 lines

I understand that you'd rather prefer to move forward. I suggest we do it in steps.

In 4.19:

- Change DRM driver.name to mxsfb-drm so we avoid conflicts for now

But this will break mesa if it depends on mxsfb name for ie. etnaviv binding.

Quoted 7 lines

- Remove CONFIG_FB_MXS from imx_v6_v7_defconfig/mxs_defconfig now, and only enable CONFIG_DRM_MXSFB=y
- Add (deprecated) to CONFIG_FB_MXS

In 4.19/4.20:
- Fix the above device trees

Quoted 7 lines
Quoted 5 lines

In 4.20/4.21:
- Remove FB_MXS

Does that sound reasonable? If yes, I can send the patch set to do step 1.

Can you fix the DTs for 4.19 too ?

Unfortunately updating the device trees turned out to be a non trivial task especially as an outsider. The display needs to be supported in drivers/gpu/drm/panel/panel-simple.c. Some displays might be already in place, but if not, the display need to be added. Since for panel bindings vendor and product information are needed, it is not possible as an outsiders to make the conversion.

Jagan, Sebastien, Fabio, Lothar and Gary, could you maybe update the respective device trees?

A conversion to the new bindings similar to commit a027d49fc193 ("ARM:
dts: imx7-colibri: use OF graph to describe the display") is what we are looking for.

This will make sure that the boards keep working using the new MXSFB DRM driver.

--
Stefan

author
#20 lore raw

PATCH 2/3 fbdev: mxsfb: Return ENODEV on missing display node

In reply to Leonard Crestez

When this driver encounters drm-style bindings it returns -ENOENT and this reports a probe error. Make it return -ENODEV to signal an explicit rejection instead.

This allows peaceful coexistence between the DRM_MXSFB and FB_MXS config options.

Signed-off-by Leonard Crestez <[email protected]>
---
 drivers/video/fbdev/mxsfb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/video/fbdev/mxsfb.c +1 -1
index 12c8bd1d24d5..9ec8882d7b3d 100644
@@ -736,11 +736,11 @@ static int mxsfb_init_fbinfo_dt(struct fb_info *fb_info,
 	int ret;
 
 	display_np = of_parse_phandle(np, "display", 0);
 	if (!display_np) {
 		dev_err(dev, "failed to find display phandle\n");
-		return -ENOENT;
+		return -ENODEV;
 	}
 
 	ret = of_property_read_u32(display_np, "bus-width", &width);
 	if (ret < 0) {
 		dev_err(dev, "failed to get property bus-width\n");
Signature · 1 line
2.17.1
author
#21 lore raw

PATCH 3/3 ARM: imx_v6_v7_defconfig: Enable DRM_MXSFB

In reply to Leonard Crestez

Multiple boards have drm-style bindings on imx lcdif nodes, enable the driver so that they work by default.

Also enable DRM_PANEL_SEIKO_43WVF1G, it is one of the supported display attachments for imx development boards.

Signed-off-by Leonard Crestez <[email protected]>
---
 arch/arm/configs/imx_v6_v7_defconfig | 2 ++
 1 file changed, 2 insertions(+)
arch/arm/configs/imx_v6_v7_defconfig +2 -0
index f70507ab91ee..dd0f64da2243 100644
@@ -256,18 +256,20 @@ CONFIG_VIDEO_ADV7180=m
 CONFIG_VIDEO_OV5640=m
 CONFIG_IMX_IPUV3_CORE=y
 CONFIG_DRM=y
 CONFIG_DRM_PANEL_LVDS=y
 CONFIG_DRM_PANEL_SIMPLE=y
+CONFIG_DRM_PANEL_SEIKO_43WVF1G=y
 CONFIG_DRM_DW_HDMI_AHB_AUDIO=m
 CONFIG_DRM_DW_HDMI_CEC=y
 CONFIG_DRM_IMX=y
 CONFIG_DRM_IMX_PARALLEL_DISPLAY=y
 CONFIG_DRM_IMX_TVE=y
 CONFIG_DRM_IMX_LDB=y
 CONFIG_DRM_IMX_HDMI=y
 CONFIG_DRM_ETNAVIV=y
+CONFIG_DRM_MXSFB=y
 CONFIG_FB_MXS=y
 CONFIG_LCD_CLASS_DEVICE=y
 CONFIG_LCD_L4F00242T03=y
 CONFIG_LCD_PLATFORM=y
 CONFIG_BACKLIGHT_PWM=y
Signature · 1 line
2.17.1