| Message ID | 20220203082546.3099-1-15330273260@189.cn (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm/lsdc: add drm driver for loongson display controller | expand |
On Thu, 3 Feb 2022 at 09:26, Sui Jingfeng <15330273260@189.cn> wrote: > > From: suijingfeng <suijingfeng@loongson.cn> > > There is a display controller in loongson's LS2K1000 SoC and LS7A1000 > bridge, and the DC in those chip is a PCI device. This patch provide > a minimal support for this display controller which is mainly for > graphic environment bring up. > > This display controller has two display pipes but with only one hardware > cursor. Each way has a DVO output interface and the CRTC is able to scanout > from 1920x1080 resolution at 60Hz. The maxmium resolution is 2048x2048@60hz. > > LS2K1000 is a SoC, only system memory is available. Therefore scanout from > system memory is the only choice. We prefer the CMA helper base solution > because the gc1000 gpu can use etnaviv driver, in this case etnaviv and > lsdc could made a compatible pair. Even through it is possible to use VRAM > helper base solution on ls2k1000 by carving out part of system memory as > VRAM. > > For LS7A1000, there are 4 gpios whos control register is located at the dc > register space which is not the geneal purpose GPIO. The 4 gpios can emulate > two way i2c. One for DVO0, another for DVO1. This is the reason the why we > are not using the drm bridge framework. > > LS2K1000 and LS2K0500 SoC don't have such hardware, they use general purpose > GPIO emulated i2c or hardware i2c adapter from other module to serve this > purpose. Drm bridge and drm panel is suitable for those SoC, we have already > implement it on our own downstream kernel. But due to the upstream kernel > don't have gpio, pwm and i2c driver support for LS2K1000. We just can not > upstream our support for the drm bridge subsystem. > > The DC in LS7A1000 can scanout from both the system memory and the dedicate > VRAM attached to the ddr3 memory controller. Sadly, only scanout from the > VRAM is proved to be a reliable solution for massive product. Scanout from > the system memory suffer from some hardware deficiency which cause the > screen flickering under RAM pressure. This is the reason why we integrate > two distict helpers into one piece of device driver. But CMA base helper is > still usable on ls7a1000 for normal usage, expecially on ls7a1000+ bridge > chip. We have also implemented demage update on top of CMA helper which > copy the demaged shadow framebuffer region from system RAM to the real > framebuffer in VRAM manually. Using "lsdc.dirty_update=1" in the commmand > line will enable this driver mode. > > LS7A1000 have a 32x32 harware cursor, we just let the two CRTC share it > simply with the help of universe plane. LS7A2000 have two 64x64 harware > cursor. Surport for LS7A2000 is on the way. > > In short, we have built-in gpio emulated i2c support, we also have hardware > cursor support. The kind of tiny drivers in drm/tiny is not suitable for us, > we are not "tiny". > > +------+ HyperTransport 3.0 > | DDR4 | | > +------+ | +------------------------------------+ > || MC0 | | LS7A1000 +------------| > +----------+ | | | DDR3 | +------+ > | LS3A4000 |<--------->| +--------+ +-------+ | memory |<->| VRAM | > | CPU |<--------->| | GC1000 | | LSDC | | controller | +------+ > +----------+ | +--------+ +-+---+-+ +------------| > || MC1 +---------------|---|----------------+ > +------+ | | > | DDR4 | +-------+ DVO0 | | DVO1 +------+ > +------+ VGA <--|ADV7125|<---------+ +------->|TFP410|--> DVI/HDMI > +-------+ +------+ > > The above picture give a simple usage of LS7A1000, note that the encoder > is not necessary adv7125 or tfp410, it is a choice of the downstream board > manufacturer. Other candicate encoder can be ch7034b, sil9022 and ite66121 > etc. Therefore, we need device tree to provide board specific information. > Besides, the dc in both ls2k1000 and ls7k1000 have the vendor:device id of > 0x0014:0x7a06, the reverison id is also same. We can't tell it apart simply > (this is the firmware engineer's mistake). But firmware already flushed to > the board and borad already sold out, we choose to resolve those issues by > introduing device tree with board specific device support. > > For lsdc, there is only a 1:1 mapping of encoders and connectors. > > +-------------------+ _________ > | | | | > | CRTC0 --> DVO0 ---------> Encoder0 --> Connector0 ---> | Monitor | > | | ^ ^ |_________| > | | | | > | <----- i2c0 ----------------+ > | LSDC IP CORE | > | <----- i2c1 ----------------+ > | | | | _________ > | | | | | | > | CRTC1 --> DVO1 ---------> Encoder1 --> Connector1 ---> | Panel | > | | |_________| > +-------------------+ > > Below is a brief introduction of loongson's CPU, bridge chip and SoC. > LS2K1000 is a double core 1.0Ghz mips64r2 compatible SoC[1]. LS7A1000 is > a bridge chip made by Loongson corporation which act as north and/or south > bridge of loongson's desktop and server level processor. It is equivalent > to AMD RS780E+SB710 or something like that. More details can be read from > its user manual[2]. > > This bridge chip is typically use with LS3A3000, LS3A4000 and LS3A5000 cpu. > LS3A3000 is 4 core 1.45gHz mips64r2 compatible cpu. > LS3A4000 is 4 core 1.8gHz mips64r5 compatible cpu. > LS3A5000 is 4 core 2.5gHz loongarch cpu[3]. > > Nearly all mordern loongson CPU's cache coherency is maintained by hardware, > except for early version of ls2k1000. So we using cached coherent memory by > default, not writecombine. > > v2: fixup warnings reported by kernel test robot > > v3: fix more grammar mistakes in Kconfig reported by Randy Dunlap and give > more details about lsdc. > > v4: > 1) Add dts required and explain why device tree is required. > 2) Give more description about lsdc and vram helper base driver. > 3) Fix warnings reported by kernel test robot. > 4) Introduce stride_alignment member into struct lsdc_chip_desc, the > stride alignment is 256 bytes for ls7a1000, ls2k1000 and ls2k0500 SoC. > But ls7a2000 improve it to 32 bytes, We are prepare for extend the > support for the on coming device. > > v5: > 1) using writel and readl replace writeq and readq, to fix kernel test > robot report build error on other archtecture > 2) set default fb format to XRGB8888 at crtc reset time. > 3) fix typos. > > v6: > 1) Explain why we are not switch to drm dridge subsystem on ls2k1000. > 2) Explain why tiny drm driver is not suitable for us. > 3) Give a short description of the trival dirty uppdate implement based > on CMA helper. > 4) code clean up > > [1] https://wiki.debian.org/InstallingDebianOn/Lemote/Loongson2K1000 > [2] https://loongson.github.io/LoongArch-Documentation/Loongson-7A1000-usermanual-EN.html > [3] https://loongson.github.io/LoongArch-Documentation/Loongson-3A5000-usermanual-EN.html > > Reported-by: Randy Dunlap <rdunlap@infradead.org> > Reported-by: kernel test robot > Signed-off-by: suijingfeng <suijingfeng@loongson.cn> > Signed-off-by: Sui Jingfeng <15330273260@189.cn> > --- > drivers/gpu/drm/Kconfig | 2 + > drivers/gpu/drm/Makefile | 1 + > drivers/gpu/drm/lsdc/Kconfig | 38 ++ > drivers/gpu/drm/lsdc/Makefile | 15 + > drivers/gpu/drm/lsdc/lsdc_connector.c | 443 ++++++++++++++ > drivers/gpu/drm/lsdc/lsdc_connector.h | 60 ++ > drivers/gpu/drm/lsdc/lsdc_crtc.c | 440 ++++++++++++++ > drivers/gpu/drm/lsdc/lsdc_drv.c | 846 ++++++++++++++++++++++++++ > drivers/gpu/drm/lsdc/lsdc_drv.h | 216 +++++++ > drivers/gpu/drm/lsdc/lsdc_encoder.c | 79 +++ > drivers/gpu/drm/lsdc/lsdc_i2c.c | 220 +++++++ > drivers/gpu/drm/lsdc/lsdc_i2c.h | 61 ++ > drivers/gpu/drm/lsdc/lsdc_irq.c | 77 +++ > drivers/gpu/drm/lsdc/lsdc_irq.h | 37 ++ > drivers/gpu/drm/lsdc/lsdc_plane.c | 681 +++++++++++++++++++++ > drivers/gpu/drm/lsdc/lsdc_pll.c | 657 ++++++++++++++++++++ > drivers/gpu/drm/lsdc/lsdc_pll.h | 109 ++++ > drivers/gpu/drm/lsdc/lsdc_regs.h | 246 ++++++++ > 18 files changed, 4228 insertions(+) > create mode 100644 drivers/gpu/drm/lsdc/Kconfig > create mode 100644 drivers/gpu/drm/lsdc/Makefile > create mode 100644 drivers/gpu/drm/lsdc/lsdc_connector.c > create mode 100644 drivers/gpu/drm/lsdc/lsdc_connector.h > create mode 100644 drivers/gpu/drm/lsdc/lsdc_crtc.c > create mode 100644 drivers/gpu/drm/lsdc/lsdc_drv.c > create mode 100644 drivers/gpu/drm/lsdc/lsdc_drv.h > create mode 100644 drivers/gpu/drm/lsdc/lsdc_encoder.c > create mode 100644 drivers/gpu/drm/lsdc/lsdc_i2c.c > create mode 100644 drivers/gpu/drm/lsdc/lsdc_i2c.h > create mode 100644 drivers/gpu/drm/lsdc/lsdc_irq.c > create mode 100644 drivers/gpu/drm/lsdc/lsdc_irq.h > create mode 100644 drivers/gpu/drm/lsdc/lsdc_plane.c > create mode 100644 drivers/gpu/drm/lsdc/lsdc_pll.c > create mode 100644 drivers/gpu/drm/lsdc/lsdc_pll.h > create mode 100644 drivers/gpu/drm/lsdc/lsdc_regs.h > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > index dfdd3ec5f793..18de1485e2ed 100644 > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -405,6 +405,8 @@ source "drivers/gpu/drm/gud/Kconfig" > > source "drivers/gpu/drm/sprd/Kconfig" > > +source "drivers/gpu/drm/lsdc/Kconfig" > + > config DRM_HYPERV > tristate "DRM Support for Hyper-V synthetic video device" > depends on DRM && PCI && MMU && HYPERV > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile > index 8675c2af7ae1..2c5a76ced323 100644 > --- a/drivers/gpu/drm/Makefile > +++ b/drivers/gpu/drm/Makefile > @@ -133,3 +133,4 @@ obj-y += xlnx/ > obj-y += gud/ > obj-$(CONFIG_DRM_HYPERV) += hyperv/ > obj-$(CONFIG_DRM_SPRD) += sprd/ > +obj-$(CONFIG_DRM_LSDC) += lsdc/ > diff --git a/drivers/gpu/drm/lsdc/Kconfig b/drivers/gpu/drm/lsdc/Kconfig > new file mode 100644 > index 000000000000..7ed1b0fdbe1b > --- /dev/null > +++ b/drivers/gpu/drm/lsdc/Kconfig > @@ -0,0 +1,38 @@ SPDX (GPL-2.0) needed. > +config DRM_LSDC > + tristate "DRM Support for loongson's display controller" > + depends on DRM && PCI > + depends on MACH_LOONGSON64 || LOONGARCH || MIPS || COMPILE_TEST > + select OF > + select CMA if HAVE_DMA_CONTIGUOUS > + select DMA_CMA if HAVE_DMA_CONTIGUOUS > + select DRM_KMS_HELPER > + select DRM_KMS_FB_HELPER > + select DRM_GEM_CMA_HELPER > + select VIDEOMODE_HELPERS > + select BACKLIGHT_PWM if CPU_LOONGSON2K > + select I2C_GPIO if CPU_LOONGSON2K > + select I2C_LS2X if CPU_LOONGSON2K > + default m > + help > + This is a KMS driver for the display controller in the LS7A1000 > + bridge chip and LS2K1000 SoC. The display controller has two > + display pipes and it is a PCI device. > + When using this driver on LS2K1000/LS2K0500 SoC, its framebuffer > + is located at system memory. > + If "M" is selected, the module will be called lsdc. > + > + If in doubt, say "Y". > + > +config DRM_LSDC_VRAM_DRIVER > + bool "vram helper based device driver support" > + depends on DRM_LSDC > + select DRM_VRAM_HELPER > + default y > + help > + When using this driver on LS7A1000 + LS3A3000/LS3A4000/LS3A5000 > + platform, the LS7A1000 bridge chip has dedicated video RAM. Using > + "lsdc.use_vram_helper=1" in the kernel command line will enable > + this driver mode and then the framebuffer will be located at the > + VRAM at the price of losing PRIME support. > + > + If in doubt, say "Y". > diff --git a/drivers/gpu/drm/lsdc/Makefile b/drivers/gpu/drm/lsdc/Makefile > new file mode 100644 > index 000000000000..342990654478 > --- /dev/null > +++ b/drivers/gpu/drm/lsdc/Makefile > @@ -0,0 +1,15 @@ SPDX > +# > +# Makefile for the lsdc drm device driver. > +# > + > +lsdc-y := \ > + lsdc_drv.o \ > + lsdc_crtc.o \ > + lsdc_irq.o \ > + lsdc_plane.o \ > + lsdc_pll.o \ > + lsdc_i2c.o \ > + lsdc_encoder.o \ > + lsdc_connector.o > + > +obj-$(CONFIG_DRM_LSDC) += lsdc.o > diff --git a/drivers/gpu/drm/lsdc/lsdc_connector.c b/drivers/gpu/drm/lsdc/lsdc_connector.c > new file mode 100644 > index 000000000000..ae5fc0c90961 > --- /dev/null > +++ b/drivers/gpu/drm/lsdc/lsdc_connector.c > @@ -0,0 +1,443 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright 2020 Loongson Corporation > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > + * copy of this software and associated documentation files (the > + * "Software"), to deal in the Software without restriction, including > + * without limitation the rights to use, copy, modify, merge, publish, > + * distribute, sub license, and/or sell copies of the Software, and to > + * permit persons to whom the Software is furnished to do so, subject to > + * the following conditions: > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL > + * THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM, > + * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR > + * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE > + * USE OR OTHER DEALINGS IN THE SOFTWARE. > + * > + * The above copyright notice and this permission notice (including the > + * next paragraph) shall be included in all copies or substantial portions > + * of the Software. This does not look like compliant with GPL-2.0. You cannot call a license GPL-2.0 and restrict it with some other provisions. Please fix the licensing. > + */ > + > +/* > + * Authors: > + * Sui Jingfeng <suijingfeng@loongson.cn> > + */ > + > +#include <drm/drm_print.h> > +#include <drm/drm_edid.h> > +#include <drm/drm_probe_helper.h> > +#include <drm/drm_atomic_helper.h> > +#include <drm/drm_crtc_helper.h> > +#include <drm/drm_connector.h> > + > +#include <video/videomode.h> > +#include <video/of_display_timing.h> > + > +#include "lsdc_drv.h" > +#include "lsdc_i2c.h" > +#include "lsdc_connector.h" > + > + > +static int lsdc_get_modes_from_edid(struct drm_connector *connector) > +{ > + struct drm_device *ddev = connector->dev; > + struct lsdc_connector *lconn = to_lsdc_connector(connector); > + struct edid *edid_p = (struct edid *)lconn->edid_data; > + int num = drm_add_edid_modes(connector, edid_p); > + > + if (num) > + drm_connector_update_edid_property(connector, edid_p); > + > + drm_dbg_kms(ddev, "%d modes added\n", num); > + > + return num; > +} > + > + Do not use double blank lines. Single line. Here and in all other places. Best regards, Krzysztof
On Thu, Feb 03, 2022 at 09:53:35AM +0100, Krzysztof Kozlowski wrote: > > diff --git a/drivers/gpu/drm/lsdc/lsdc_connector.c b/drivers/gpu/drm/lsdc/lsdc_connector.c > > new file mode 100644 > > index 000000000000..ae5fc0c90961 > > --- /dev/null > > +++ b/drivers/gpu/drm/lsdc/lsdc_connector.c > > @@ -0,0 +1,443 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * Copyright 2020 Loongson Corporation > > + * > > + * Permission is hereby granted, free of charge, to any person obtaining a > > + * copy of this software and associated documentation files (the > > + * "Software"), to deal in the Software without restriction, including > > + * without limitation the rights to use, copy, modify, merge, publish, > > + * distribute, sub license, and/or sell copies of the Software, and to > > + * permit persons to whom the Software is furnished to do so, subject to > > + * the following conditions: > > + * > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > > + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL > > + * THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM, > > + * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR > > + * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE > > + * USE OR OTHER DEALINGS IN THE SOFTWARE. > > + * > > + * The above copyright notice and this permission notice (including the > > + * next paragraph) shall be included in all copies or substantial portions > > + * of the Software. > > This does not look like compliant with GPL-2.0. You cannot call a > license GPL-2.0 and restrict it with some other provisions. That's the MIT license. It's not the GPL-2.0 license but it is compliant. regards, dan carpenter
On Thu, 3 Feb 2022 at 12:08, Dan Carpenter <dan.carpenter@oracle.com> wrote: > > On Thu, Feb 03, 2022 at 09:53:35AM +0100, Krzysztof Kozlowski wrote: > > > diff --git a/drivers/gpu/drm/lsdc/lsdc_connector.c b/drivers/gpu/drm/lsdc/lsdc_connector.c > > > new file mode 100644 > > > index 000000000000..ae5fc0c90961 > > > --- /dev/null > > > +++ b/drivers/gpu/drm/lsdc/lsdc_connector.c > > > @@ -0,0 +1,443 @@ > > > +// SPDX-License-Identifier: GPL-2.0+ > > > +/* > > > + * Copyright 2020 Loongson Corporation > > > + * > > > + * Permission is hereby granted, free of charge, to any person obtaining a > > > + * copy of this software and associated documentation files (the > > > + * "Software"), to deal in the Software without restriction, including > > > + * without limitation the rights to use, copy, modify, merge, publish, > > > + * distribute, sub license, and/or sell copies of the Software, and to > > > + * permit persons to whom the Software is furnished to do so, subject to > > > + * the following conditions: > > > + * > > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > > > + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL > > > + * THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM, > > > + * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR > > > + * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE > > > + * USE OR OTHER DEALINGS IN THE SOFTWARE. > > > + * > > > + * The above copyright notice and this permission notice (including the > > > + * next paragraph) shall be included in all copies or substantial portions > > > + * of the Software. > > > > This does not look like compliant with GPL-2.0. You cannot call a > > license GPL-2.0 and restrict it with some other provisions. > > That's the MIT license. It's not the GPL-2.0 license but it is > compliant. It's compliant when included as "OR" for example in SPDX tag. The current solution - SPDX and MIT license text - is not the proper way to describe this. Otherwise one could argue that both licenses apply at the same time and one has to fulfill both of them, which is ridiculous. There is a SPDX tag for the proper case - GPL or MIT. Best regards, Krzysztof
On Thu, Feb 03, 2022 at 12:29:11PM +0100, Krzysztof Kozlowski wrote: > On Thu, 3 Feb 2022 at 12:08, Dan Carpenter <dan.carpenter@oracle.com> wrote: > > > > > > This does not look like compliant with GPL-2.0. You cannot call a > > > license GPL-2.0 and restrict it with some other provisions. > > > > That's the MIT license. It's not the GPL-2.0 license but it is > > compliant. > > It's compliant when included as "OR" for example in SPDX tag. The > current solution - SPDX and MIT license text - is not the proper way > to describe this. Otherwise one could argue that both licenses apply > at the same time and one has to fulfill both of them, which is > ridiculous. There is a SPDX tag for the proper case - GPL or MIT. You're saying a bunch of different things. We both agree that the SPDX text is confusing because it says GPL-2.0+ but it has the text from the MIT license. "This does not look like compliant with GPL-2.0." Wrong. The MIT license is compatible with the GPL-2.0. "You cannot call a license GPL-2.0 and restrict it with some other provisions." Wrong. The MIT license just says you have to include the No Warranty text. The GPL has it's own list of requirements. But you can combine MIT and GPL code and easily comply with both requirements. That's what "compatible" means in this context. In the kernel we have MIT licensed code which is dual licensed. This means that someone can take that driver and release it as closed source software if they want. // SPDX-License-Identifier: GPL-2.0 OR MIT Then we also have code which was originally MIT licensed but now you have to comply with the GPL as well. // SPDX-License-Identifier: (GPL-2.0 WITH Linux-syscall-note) AND MIT These examples were cut and paste from Documentation/process/license-rules.rst regards, dan carpenter
On 2022/2/3 16:58, Maxime Ripard wrote: >> diff --git a/drivers/gpu/drm/lsdc/Kconfig b/drivers/gpu/drm/lsdc/Kconfig >> new file mode 100644 >> index 000000000000..7ed1b0fdbe1b >> --- /dev/null >> +++ b/drivers/gpu/drm/lsdc/Kconfig >> @@ -0,0 +1,38 @@ >> +config DRM_LSDC >> + tristate "DRM Support for loongson's display controller" >> + depends on DRM && PCI >> + depends on MACH_LOONGSON64 || LOONGARCH || MIPS || COMPILE_TEST >> + select OF >> + select CMA if HAVE_DMA_CONTIGUOUS >> + select DMA_CMA if HAVE_DMA_CONTIGUOUS >> + select DRM_KMS_HELPER >> + select DRM_KMS_FB_HELPER >> + select DRM_GEM_CMA_HELPER >> + select VIDEOMODE_HELPERS >> + select BACKLIGHT_PWM if CPU_LOONGSON2K >> + select I2C_GPIO if CPU_LOONGSON2K >> + select I2C_LS2X if CPU_LOONGSON2K >> + default m >> + help >> + This is a KMS driver for the display controller in the LS7A1000 >> + bridge chip and LS2K1000 SoC. The display controller has two >> + display pipes and it is a PCI device. >> + When using this driver on LS2K1000/LS2K0500 SoC, its framebuffer >> + is located at system memory. >> + If "M" is selected, the module will be called lsdc. >> + >> + If in doubt, say "Y". >> + >> +config DRM_LSDC_VRAM_DRIVER >> + bool "vram helper based device driver support" >> + depends on DRM_LSDC >> + select DRM_VRAM_HELPER >> + default y >> + help >> + When using this driver on LS7A1000 + LS3A3000/LS3A4000/LS3A5000 >> + platform, the LS7A1000 bridge chip has dedicated video RAM. Using >> + "lsdc.use_vram_helper=1" in the kernel command line will enable >> + this driver mode and then the framebuffer will be located at the >> + VRAM at the price of losing PRIME support. >> + >> + If in doubt, say "Y". > This doesn't sound right. The driver should make the proper decision > depending on the platform, not the user or the distribution. The LS7A1000 north bridge chip has dedicated video RAM, but the DC in LS7A1000 can also scanout from the system memory directly like a display controller in a SoC does. In fact, this display controller is envolved from early product of Loongson 2H SoC. This driver still works even if the downstream PC board manufacturer doesn't solder a video RAM on the mother board. The lsdc_should_vram_helper_based() function in lsdc_drv.c will examine if the DC device node contain a use_vram_helper property at driver loading time. If there is no use_vram_helper property, CMA helper based DRM driver will be used. Doing this way allow the user using "lsdc.use_vram_helper=0" override the default behavior even through there is a "use_vram_helper;" property in the DTS. In short, It is intend to let the command line passed from kernel has higher priority than the device tree. Otherwise the end user can not switch different driver mode through the kernel command line once the DC device node contain "use_vram_helper;" property. This driver's author already made the decision by the time when the patch is sent out, even through this**may not proper. The CMA helper based driver will be used by default, if the DC device node contain "use_vram_helper;" then VRAM based driver will be used. This is my initial intention.
>> diff --git a/drivers/gpu/drm/lsdc/Makefile b/drivers/gpu/drm/lsdc/Makefile >> new file mode 100644 >> index 000000000000..342990654478 >> --- /dev/null >> +++ b/drivers/gpu/drm/lsdc/Makefile >> @@ -0,0 +1,15 @@ >> +# >> +# Makefile for the lsdc drm device driver. >> +# >> + >> +lsdc-y := \ >> + lsdc_drv.o \ >> + lsdc_crtc.o \ >> + lsdc_irq.o \ >> + lsdc_plane.o \ >> + lsdc_pll.o \ >> + lsdc_i2c.o \ >> + lsdc_encoder.o \ >> + lsdc_connector.o >> + >> +obj-$(CONFIG_DRM_LSDC) += lsdc.o >> diff --git a/drivers/gpu/drm/lsdc/lsdc_connector.c b/drivers/gpu/drm/lsdc/lsdc_connector.c >> new file mode 100644 >> index 000000000000..ae5fc0c90961 >> --- /dev/null >> +++ b/drivers/gpu/drm/lsdc/lsdc_connector.c >> @@ -0,0 +1,443 @@ >> +// SPDX-License-Identifier: GPL-2.0+ >> +/* >> + * Copyright 2020 Loongson Corporation >> + * >> + * Permission is hereby granted, free of charge, to any person obtaining a >> + * copy of this software and associated documentation files (the >> + * "Software"), to deal in the Software without restriction, including >> + * without limitation the rights to use, copy, modify, merge, publish, >> + * distribute, sub license, and/or sell copies of the Software, and to >> + * permit persons to whom the Software is furnished to do so, subject to >> + * the following conditions: >> + * >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, >> + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL >> + * THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM, >> + * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR >> + * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE >> + * USE OR OTHER DEALINGS IN THE SOFTWARE. >> + * >> + * The above copyright notice and this permission notice (including the >> + * next paragraph) shall be included in all copies or substantial portions >> + * of the Software. >> + */ > That's the MIT license, yet you claim the driver to be licensed under > the GPLv2 or later? I just copy then paste it blindly, sorry about that. I do not know the difference, we want open the source anyway. I will correct it in next version, thanks. >> + >> +/* >> + * Authors: >> + * Sui Jingfeng <suijingfeng@loongson.cn> >> + */ >> + >> +#include <drm/drm_print.h> >> +#include <drm/drm_edid.h> >> +#include <drm/drm_probe_helper.h> >> +#include <drm/drm_atomic_helper.h> >> +#include <drm/drm_crtc_helper.h> >> +#include <drm/drm_connector.h> >> + >> +#include <video/videomode.h> >> +#include <video/of_display_timing.h> >> + >> +#include "lsdc_drv.h" >> +#include "lsdc_i2c.h" >> +#include "lsdc_connector.h" >> + >> + >> +static int lsdc_get_modes_from_edid(struct drm_connector *connector) >> +{ >> + struct drm_device *ddev = connector->dev; >> + struct lsdc_connector *lconn = to_lsdc_connector(connector); >> + struct edid *edid_p = (struct edid *)lconn->edid_data; >> + int num = drm_add_edid_modes(connector, edid_p); >> + >> + if (num) >> + drm_connector_update_edid_property(connector, edid_p); >> + >> + drm_dbg_kms(ddev, "%d modes added\n", num); >> + >> + return num; >> +} >> + >> + >> +static int lsdc_get_modes_from_timings(struct drm_connector *connector) >> +{ >> + struct drm_device *ddev = connector->dev; >> + struct lsdc_connector *lconn = to_lsdc_connector(connector); >> + struct display_timings *disp_tim = lconn->disp_tim; >> + unsigned int num = 0; >> + unsigned int i; >> + >> + for (i = 0; i < disp_tim->num_timings; i++) { >> + const struct display_timing *dt = disp_tim->timings[i]; >> + struct drm_display_mode *mode; >> + struct videomode vm; >> + >> + videomode_from_timing(dt, &vm); >> + mode = drm_mode_create(ddev); >> + if (!mode) { >> + drm_err(ddev, "failed to add mode %ux%u\n", >> + dt->hactive.typ, dt->vactive.typ); >> + continue; >> + } >> + >> + drm_display_mode_from_videomode(&vm, mode); >> + >> + mode->type |= DRM_MODE_TYPE_DRIVER; >> + >> + if (i == disp_tim->native_mode) >> + mode->type |= DRM_MODE_TYPE_PREFERRED; >> + >> + drm_mode_probed_add(connector, mode); >> + num++; >> + } >> + >> + drm_dbg_kms(ddev, "%d modes added\n", num); >> + >> + return num; >> +} >> + >> + >> +static int lsdc_get_modes_from_ddc(struct drm_connector *connector, >> + struct i2c_adapter *ddc) >> +{ >> + unsigned int num = 0; >> + struct edid *edid; >> + >> + edid = drm_get_edid(connector, ddc); >> + if (edid) { >> + drm_connector_update_edid_property(connector, edid); >> + num = drm_add_edid_modes(connector, edid); >> + kfree(edid); >> + } >> + >> + return num; >> +} >> + >> + >> +static int lsdc_get_modes(struct drm_connector *connector) >> +{ >> + struct lsdc_connector *lconn = to_lsdc_connector(connector); >> + unsigned int num = 0; >> + >> + if (lconn->has_edid) >> + return lsdc_get_modes_from_edid(connector); >> + >> + if (lconn->has_disp_tim) >> + return lsdc_get_modes_from_timings(connector); >> + >> + if (IS_ERR_OR_NULL(lconn->ddc) == false) >> + return lsdc_get_modes_from_ddc(connector, lconn->ddc); >> + >> + if (connector->connector_type == DRM_MODE_CONNECTOR_VIRTUAL) { >> + num = drm_add_modes_noedid(connector, >> + connector->dev->mode_config.max_width, >> + connector->dev->mode_config.max_height); >> + >> + drm_set_preferred_mode(connector, 1024, 768); >> + >> + return num; >> + } >> + >> + >> + /* >> + * In case we cannot retrieve the EDIDs (broken or missing i2c >> + * bus), fallback on the XGA standards, because we are for board >> + * bringup. >> + */ >> + num = drm_add_modes_noedid(connector, 1920, 1200); >> + >> + /* And prefer a mode pretty much anyone can handle */ >> + drm_set_preferred_mode(connector, 1024, 768); >> + >> + return num; >> + >> +} >> + >> + >> +static enum drm_connector_status >> +lsdc_connector_detect(struct drm_connector *connector, bool force) >> +{ >> + struct lsdc_connector *lconn = to_lsdc_connector(connector); >> + >> + if (lconn->has_edid == true) >> + return connector_status_connected; >> + >> + if (lconn->has_disp_tim == true) >> + return connector_status_connected; >> + >> + if (IS_ERR_OR_NULL(lconn->ddc) == false) >> + return drm_probe_ddc(lconn->ddc); >> + >> + if (lconn->ddc && drm_probe_ddc(lconn->ddc)) >> + return connector_status_connected; >> + >> + if (connector->connector_type == DRM_MODE_CONNECTOR_VIRTUAL) >> + return connector_status_connected; >> + >> + if ((connector->connector_type == DRM_MODE_CONNECTOR_DVIA) || >> + (connector->connector_type == DRM_MODE_CONNECTOR_DVID) || >> + (connector->connector_type == DRM_MODE_CONNECTOR_DVII)) >> + return connector_status_disconnected; >> + >> + if ((connector->connector_type == DRM_MODE_CONNECTOR_HDMIA) || >> + (connector->connector_type == DRM_MODE_CONNECTOR_HDMIB)) >> + return connector_status_disconnected; >> + >> + return connector_status_unknown; >> +} >> + >> + >> +/* >> + * @connector: point to the drm_connector structure >> + * >> + * Clean up connector resources >> + */ >> +static void lsdc_connector_destroy(struct drm_connector *connector) >> +{ >> + struct drm_device *ddev = connector->dev; >> + struct lsdc_connector *lconn = to_lsdc_connector(connector); >> + >> + if (lconn) { >> + if (lconn->ddc) >> + lsdc_destroy_i2c(connector->dev, lconn->ddc); >> + >> + drm_info(ddev, "destroying connector%u\n", lconn->index); >> + >> + devm_kfree(ddev->dev, lconn); >> + } >> + >> + drm_connector_cleanup(connector); >> +} >> + >> + >> +static const struct drm_connector_helper_funcs lsdc_connector_helpers = { >> + .get_modes = lsdc_get_modes, >> +}; >> + >> +/* >> + * These provide the minimum set of functions required to handle a connector >> + * >> + * Control connectors on a given device. >> + * >> + * Each CRTC may have one or more connectors attached to it. >> + * The functions below allow the core DRM code to control >> + * connectors, enumerate available modes, etc. >> + */ >> +static const struct drm_connector_funcs lsdc_connector_funcs = { >> + .dpms = drm_helper_connector_dpms, >> + .detect = lsdc_connector_detect, >> + .fill_modes = drm_helper_probe_single_connector_modes, >> + .destroy = lsdc_connector_destroy, >> + .reset = drm_atomic_helper_connector_reset, >> + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, >> + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, >> +}; >> + >> + >> +/* Get the simple EDID data from the device tree >> + * the length must be EDID_LENGTH, since it is simple. >> + * >> + * @np: device node contain edid data >> + * @edid_data: where the edid data to store to >> + */ >> +static bool lsdc_get_edid_from_dtb(struct device_node *np, >> + unsigned char *edid_data) >> +{ >> + int length; >> + const void *prop; >> + >> + if (np == NULL) >> + return false; >> + >> + prop = of_get_property(np, "edid", &length); >> + if (prop && (length == EDID_LENGTH)) { >> + memcpy(edid_data, prop, EDID_LENGTH); >> + return true; >> + } >> + >> + return false; >> +} > You don't have a device tree binding for that driver, this is something > that is required. And it's not clear to me why you'd want EDID in the > DTB? 1) It is left to the end user of this driver. The downstream motherboard maker may use a dpi(XRGB888) or LVDS panel which don't have DDC support either, doing this way allow them put a EDID property into the dc device node in the DTS. Then the entire system works. Note those panel usually support only one display mode. 2) That is for the display controller in ls2k1000 SoC. Currently, the upstream kernel still don't have GPIO, PWM and I2C driver support for LS2K1000 SoC. How dose you read EDID from the monitor without a I2C driver? without reading EDID the device tree support, the screen just black, the lsdc driver just stall. With reading EDID from device tree support we do not need a i2c driver to light up the monitor. This make lsdc drm driver work on various ls2k1000 development board before I2C driver and GPIO driver and PWM backlight driver is upstream. I have many local private dts with the bindings, those local change just can not upstream at this time, below is an example. 3) Again, doing this way is for graphic environment bring up. &lsdc { output-ports = <&dvo0 &dvo1>; #address-cells = <1>; #size-cells = <0>; dvo0: dvo@0 { reg = <0>; connector = "dpi-connector"; encoder = "none"; status = "ok"; display-timings { native-mode = <&mode_0_1024x600_60>; mode_0_1024x600_60: panel-timing@0 { clock-frequency = <51200000>; hactive = <1024>; vactive = <600>; hsync-len = <4>; hfront-porch = <160>; hback-porch = <156>; vfront-porch = <11>; vback-porch = <23>; vsync-len = <1>; }; mode_1_800x480_60: panel-timing@1 { clock-frequency = <30066000>; hactive = <800>; vactive = <480>; hfront-porch = <50>; hback-porch = <70>; hsync-len = <50>; vback-porch = <0>; vfront-porch = <0>; vsync-len = <50>; }; }; }; dvo1: dvo@1 { reg = <1>; connector = "hdmi-connector"; type = "a"; encoder = "sil9022"; edid = [ 00 ff ff ff ff ff ff 00 1e 6d 54 5b 0b cc 04 00 02 1c 01 03 6c 30 1b 78 ea 31 35 a5 55 4e a1 26 0c 50 54 a5 4b 00 71 4f 81 80 95 00 b3 00 a9 c0 81 00 81 c0 90 40 02 3a 80 18 71 38 2d 40 58 2c 45 00 e0 0e 11 00 00 1e 00 00 00 fd 00 38 4b 1e 53 0f 00 0a 20 20 20 20 20 20 00 00 00 fc 00 4c 47 20 46 55 4c 4c 20 48 44 0a 20 20 00 00 00 ff 00 38 30 32 4e 54 43 5a 39 38 33 37 39 0a 00 35 ]; status = "ok"; }; };
>> +static enum drm_mode_status >> +lsdc_crtc_helper_mode_valid(struct drm_crtc *crtc, >> + const struct drm_display_mode *mode) >> +{ >> + struct drm_device *ddev = crtc->dev; >> + struct lsdc_device *ldev = to_lsdc(ddev); >> + const struct lsdc_chip_desc *desc = ldev->desc; >> + >> + if (mode->hdisplay > desc->max_width) >> + return MODE_BAD_HVALUE; >> + if (mode->vdisplay > desc->max_height) >> + return MODE_BAD_VVALUE; >> + >> + if (mode->clock > desc->max_pixel_clk) { >> + drm_dbg_kms(ddev, "mode %dx%d, pixel clock=%d is too high\n", >> + mode->hdisplay, mode->vdisplay, mode->clock); >> + return MODE_CLOCK_HIGH; >> + } >> + >> + /* the crtc hardware dma take 256 bytes once a time >> + * TODO: check RGB565 support >> + */ >> + if ((mode->hdisplay * 4) % desc->stride_alignment) { >> + drm_dbg_kms(ddev, "stride is not %u bytes aligned\n", >> + desc->stride_alignment); >> + return MODE_BAD; >> + } >> + >> + return MODE_OK; >> +} > mode_valid will only prevent the mode from being advertised to the > userspace, but you need atomic_check if you want to prevent those modes > to be used by anybody. Yes, I used to change mode with mate-display-properties tools, what I though is the end user can't see it, they can't set it. I will add atomic_check() support at next version, thanks. >> + >> +static void lsdc_update_pixclk(struct drm_crtc *crtc, unsigned int pixclk, bool verbose) >> +{ >> + struct lsdc_display_pipe *dispipe; >> + struct lsdc_pll *pixpll; >> + const struct lsdc_pixpll_funcs *clkfun; >> + struct lsdc_crtc_state *priv_crtc_state; >> + >> + priv_crtc_state = to_lsdc_crtc_state(crtc->state); >> + >> + dispipe = container_of(crtc, struct lsdc_display_pipe, crtc); >> + pixpll = &dispipe->pixpll; >> + clkfun = pixpll->funcs; >> + >> + /* config the pixel pll */ >> + clkfun->update(pixpll, &priv_crtc_state->pparams); >> + >> + if (verbose) >> + clkfun->print(pixpll, pixclk); >> +} >> + >> + >> +static void lsdc_crtc_helper_mode_set_nofb(struct drm_crtc *crtc) >> +{ >> + struct drm_device *ddev = crtc->dev; >> + struct lsdc_device *ldev = to_lsdc(ddev); >> + struct drm_display_mode *mode = &crtc->state->adjusted_mode; >> + unsigned int hr = mode->hdisplay; >> + unsigned int hss = mode->hsync_start; >> + unsigned int hse = mode->hsync_end; >> + unsigned int hfl = mode->htotal; >> + unsigned int vr = mode->vdisplay; >> + unsigned int vss = mode->vsync_start; >> + unsigned int vse = mode->vsync_end; >> + unsigned int vfl = mode->vtotal; >> + unsigned int pixclock = mode->clock; >> + unsigned int index = drm_crtc_index(crtc); >> + >> + >> + if (index == 0) { >> + /* CRTC 0 */ >> + u32 hsync, vsync; >> + >> + lsdc_reg_write32(ldev, LSDC_CRTC0_FB_ORIGIN_REG, 0); >> + >> + /* 26:16 total pixels, 10:0 visiable pixels, in horizontal */ >> + lsdc_reg_write32(ldev, LSDC_CRTC0_HDISPLAY_REG, >> + (mode->crtc_htotal << 16) | mode->crtc_hdisplay); >> + >> + /* 26:16 total pixels, 10:0 visiable pixels, in vertical */ >> + lsdc_reg_write32(ldev, LSDC_CRTC0_VDISPLAY_REG, >> + (mode->crtc_vtotal << 16) | mode->crtc_vdisplay); >> + >> + /* 26:16 hsync end, 10:0 hsync start */ >> + hsync = (mode->crtc_hsync_end << 16) | mode->crtc_hsync_start; >> + >> + if (mode->flags & DRM_MODE_FLAG_NHSYNC) >> + hsync |= INV_HSYNC_BIT; >> + >> + lsdc_reg_write32(ldev, LSDC_CRTC0_HSYNC_REG, EN_HSYNC_BIT | hsync); >> + >> + /* 26:16 vsync end, 10:0 vsync start */ >> + vsync = (mode->crtc_vsync_end << 16) | mode->crtc_vsync_start; >> + >> + if (mode->flags & DRM_MODE_FLAG_NVSYNC) >> + vsync |= INV_VSYNC_BIT; >> + >> + lsdc_reg_write32(ldev, LSDC_CRTC0_VSYNC_REG, EN_VSYNC_BIT | vsync); >> + >> + } else if (index == 1) { >> + /* CRTC 1 */ >> + u32 hsync, vsync; >> + >> + lsdc_reg_write32(ldev, LSDC_CRTC1_FB_ORIGIN_REG, 0); >> + >> + /* 26:16 total pixels, 10:0 visiable pixels, in horizontal */ >> + lsdc_reg_write32(ldev, LSDC_CRTC1_HDISPLAY_REG, >> + (mode->crtc_htotal << 16) | mode->crtc_hdisplay); >> + >> + /* 26:16 total pixels, 10:0 visiable pixels, in vertical */ >> + lsdc_reg_write32(ldev, LSDC_CRTC1_VDISPLAY_REG, >> + (mode->crtc_vtotal << 16) | mode->crtc_vdisplay); >> + >> + /* 26:16 hsync end, 10:0 hsync start */ >> + hsync = (mode->crtc_hsync_end << 16) | mode->crtc_hsync_start; >> + >> + if (mode->flags & DRM_MODE_FLAG_NHSYNC) >> + hsync |= INV_HSYNC_BIT; >> + >> + lsdc_reg_write32(ldev, LSDC_CRTC1_HSYNC_REG, EN_HSYNC_BIT | hsync); >> + >> + /* 26:16 vsync end, 10:0 vsync start */ >> + vsync = (mode->crtc_vsync_end << 16) | mode->crtc_vsync_start; >> + >> + if (mode->flags & DRM_MODE_FLAG_NVSYNC) >> + vsync |= INV_VSYNC_BIT; >> + >> + lsdc_reg_write32(ldev, LSDC_CRTC1_VSYNC_REG, EN_VSYNC_BIT | vsync); >> + } >> + >> + drm_dbg_kms(ddev, "hdisplay=%d, hsync_start=%d, hsync_end=%d, htotal=%d\n", >> + hr, hss, hse, hfl); >> + >> + drm_dbg_kms(ddev, "vdisplay=%d, vsync_start=%d, vsync_end=%d, vtotal=%d\n", >> + vr, vss, vse, vfl); >> + >> + drm_dbg_kms(ddev, "%s modeset: %ux%u\n", crtc->name, hr, vr); >> + >> + lsdc_update_pixclk(crtc, pixclock, ldev->verbose); >> +} >> + >> + >> +static void lsdc_enable_display(struct lsdc_device *ldev, unsigned int index) >> +{ >> + u32 val; >> + >> + if (index == 0) { >> + val = lsdc_reg_read32(ldev, LSDC_CRTC0_CFG_REG); >> + val |= CFG_OUTPUT_EN_BIT; >> + lsdc_reg_write32(ldev, LSDC_CRTC0_CFG_REG, val); >> + } else if (index == 1) { >> + val = lsdc_reg_read32(ldev, LSDC_CRTC1_CFG_REG); >> + val |= CFG_OUTPUT_EN_BIT; >> + lsdc_reg_write32(ldev, LSDC_CRTC1_CFG_REG, val); >> + } >> +} >> + >> + >> +static void lsdc_disable_display(struct lsdc_device *ldev, unsigned int index) >> +{ >> + u32 val; >> + >> + if (index == 0) { >> + val = lsdc_reg_read32(ldev, LSDC_CRTC0_CFG_REG); >> + val &= ~CFG_OUTPUT_EN_BIT; >> + lsdc_reg_write32(ldev, LSDC_CRTC0_CFG_REG, val); >> + } else if (index == 1) { >> + val = lsdc_reg_read32(ldev, LSDC_CRTC1_CFG_REG); >> + val &= ~CFG_OUTPUT_EN_BIT; >> + lsdc_reg_write32(ldev, LSDC_CRTC1_CFG_REG, val); >> + } >> +} >> + >> + >> +static void lsdc_crtc_helper_atomic_enable(struct drm_crtc *crtc, >> + struct drm_atomic_state *state) >> +{ >> + struct drm_device *ddev = crtc->dev; >> + struct lsdc_device *ldev = to_lsdc(ddev); >> + >> + drm_crtc_vblank_on(crtc); >> + >> + lsdc_enable_display(ldev, drm_crtc_index(crtc)); >> + >> + drm_dbg_kms(ddev, "%s: enabled\n", crtc->name); >> +} >> + >> + >> +static void lsdc_crtc_helper_atomic_disable(struct drm_crtc *crtc, >> + struct drm_atomic_state *state) >> +{ >> + struct drm_device *ddev = crtc->dev; >> + struct lsdc_device *ldev = to_lsdc(ddev); >> + >> + drm_crtc_vblank_off(crtc); >> + >> + lsdc_disable_display(ldev, drm_crtc_index(crtc)); >> + >> + drm_dbg_kms(ddev, "%s: disabled\n", crtc->name); >> +} >> + >> + >> +static void lsdc_crtc_atomic_flush(struct drm_crtc *crtc, >> + struct drm_atomic_state *state) >> +{ >> + struct drm_pending_vblank_event *event = crtc->state->event; >> + >> + if (event) { >> + crtc->state->event = NULL; >> + >> + spin_lock_irq(&crtc->dev->event_lock); >> + if (drm_crtc_vblank_get(crtc) == 0) >> + drm_crtc_arm_vblank_event(crtc, event); >> + else >> + drm_crtc_send_vblank_event(crtc, event); >> + spin_unlock_irq(&crtc->dev->event_lock); >> + } >> +} >> + >> + >> +static const struct drm_crtc_helper_funcs lsdc_crtc_helper_funcs = { >> + .mode_valid = lsdc_crtc_helper_mode_valid, >> + .mode_set_nofb = lsdc_crtc_helper_mode_set_nofb, >> + .atomic_enable = lsdc_crtc_helper_atomic_enable, >> + .atomic_disable = lsdc_crtc_helper_atomic_disable, >> + .atomic_flush = lsdc_crtc_atomic_flush, >> +}; >> + >> + >> + >> +/** >> + * lsdc_crtc_init >> + * >> + * @ddev: point to the drm_device structure >> + * @index: hardware crtc index >> + * >> + * Init CRTC >> + */ >> +int lsdc_crtc_init(struct drm_device *ddev, >> + struct drm_crtc *crtc, >> + unsigned int index, >> + struct drm_plane *primary, >> + struct drm_plane *cursor) >> +{ >> + int ret; >> + >> + drm_crtc_helper_add(crtc, &lsdc_crtc_helper_funcs); >> + >> + ret = drm_mode_crtc_set_gamma_size(crtc, 256); >> + if (ret) >> + drm_warn(ddev, "set the gamma table size failed\n"); >> + >> + return drm_crtc_init_with_planes(ddev, >> + crtc, >> + primary, >> + cursor, >> + &lsdc_crtc_funcs, >> + "crtc%d", >> + index); >> +} >> diff --git a/drivers/gpu/drm/lsdc/lsdc_drv.c b/drivers/gpu/drm/lsdc/lsdc_drv.c >> new file mode 100644 >> index 000000000000..aac8901c3431 >> --- /dev/null >> +++ b/drivers/gpu/drm/lsdc/lsdc_drv.c >> @@ -0,0 +1,846 @@ >> +// SPDX-License-Identifier: GPL-2.0+ >> +/* >> + * Copyright 2020 Loongson Corporation >> + * >> + * Permission is hereby granted, free of charge, to any person obtaining a >> + * copy of this software and associated documentation files (the >> + * "Software"), to deal in the Software without restriction, including >> + * without limitation the rights to use, copy, modify, merge, publish, >> + * distribute, sub license, and/or sell copies of the Software, and to >> + * permit persons to whom the Software is furnished to do so, subject to >> + * the following conditions: >> + * >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, >> + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL >> + * THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM, >> + * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR >> + * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE >> + * USE OR OTHER DEALINGS IN THE SOFTWARE. >> + * >> + * The above copyright notice and this permission notice (including the >> + * next paragraph) shall be included in all copies or substantial portions >> + * of the Software. >> + */ >> + >> +/* >> + * Authors: >> + * Sui Jingfeng <suijingfeng@loongson.cn> >> + */ >> + >> +#include <linux/errno.h> >> +#include <linux/string.h> >> +#include <linux/module.h> >> +#include <linux/pci.h> >> +#include <linux/of_reserved_mem.h> >> + >> +#include <drm/drm_drv.h> >> +#include <drm/drm_aperture.h> >> +#include <drm/drm_of.h> >> +#include <drm/drm_plane.h> >> +#include <drm/drm_vblank.h> >> +#include <drm/drm_debugfs.h> >> +#include <drm/drm_fb_helper.h> >> +#include <drm/drm_crtc_helper.h> >> +#include <drm/drm_gem_cma_helper.h> >> +#include <drm/drm_fb_cma_helper.h> >> +#include <drm/drm_gem_framebuffer_helper.h> >> +#include <drm/drm_atomic_helper.h> >> +#include <drm/drm_damage_helper.h> >> +#include <drm/drm_probe_helper.h> >> + >> +#include "lsdc_drv.h" >> +#include "lsdc_irq.h" >> +#include "lsdc_regs.h" >> +#include "lsdc_connector.h" >> +#include "lsdc_pll.h" >> + >> + >> +#define DRIVER_AUTHOR "Sui Jingfeng <suijingfeng@loongson.cn>" >> +#define DRIVER_NAME "lsdc" >> +#define DRIVER_DESC "drm driver for loongson's display controller" >> +#define DRIVER_DATE "20200701" >> +#define DRIVER_MAJOR 1 >> +#define DRIVER_MINOR 0 >> +#define DRIVER_PATCHLEVEL 0 >> + >> +static int lsdc_modeset = 1; >> +MODULE_PARM_DESC(modeset, "Enable/disable CMA-based KMS(1 = enabled(default), 0 = disabled)"); >> +module_param_named(modeset, lsdc_modeset, int, 0644); >> + >> +static int lsdc_cached_coherent = 1; >> +MODULE_PARM_DESC(cached_coherent, "uss cached coherent mapping(1 = enabled(default), 0 = disabled)"); >> +module_param_named(cached_coherent, lsdc_cached_coherent, int, 0644); >> + >> +static int lsdc_dirty_update = -1; >> +MODULE_PARM_DESC(dirty_update, "enable dirty update(1 = enabled, 0 = disabled(default))"); >> +module_param_named(dirty_update, lsdc_dirty_update, int, 0644); >> + >> +static int lsdc_use_vram_helper = -1; >> +MODULE_PARM_DESC(use_vram_helper, "use vram helper based solution(1 = enabled, 0 = disabled(default))"); >> +module_param_named(use_vram_helper, lsdc_use_vram_helper, int, 0644); >> + >> +static int lsdc_verbose = -1; >> +MODULE_PARM_DESC(verbose, "Enable/disable print some key information"); >> +module_param_named(verbose, lsdc_verbose, int, 0644); > It's not really clear to me why you need any of those parameters. Why > would a user want to use a non coherent mapping for example? > Because we are Mips architecture. Paul Cercueil already explained it in his mmap GEM buffers cachedpatch <https://lkml.kernel.org/lkml/20200822164233.71583-1-paul@crapouillou.net/T/>. I drag part of it to here for convenient to reading: /Traditionally, GEM buffers are mapped write-combine. Writes to the buffer are accelerated, and reads are slow. Application doing lots////of alpha-blending paint inside shadow buffers, which is then memcpy'd////into the final GEM buffer./// "non coherent mapping" is actually cached and it is for CMA helpers base driver, not for VRAM helper based driver. For Loongson CPU/SoCs. The cache coherency is maintained by hardware, therefore there no need to worry about coherency problems. This is true at least for ls3a3000, ls3a4000 and ls3a5000. "non coherent" or "coherent" is not important here, the key point is that the backing memory of the framebuffer is cached with non coherent mapping, you don't need a shadow buffer layer when using X server's modesetting driver. Read and write to the framebuffer in system memory is much faster than read and write to the framebuffer in the VRAM. Why CMA helper based solution is faster than the VRAM based solution on Mips platform? Partly because of the CPU have L1, L2 and L3 cache, especially L3 cache is as large as 8MB, read and write from the cache is fast. Another reason is as Paul Cercueil said, read from VRAM with write-combine cache mode is slow. it is just uncache read. Please note that we don't have a GPU here, we are just a display controller. For the VRAM helper based driver case, the backing memory of the framebuffer is located at VRAM, When using X server's modesetting driver, we have to enable the ShadowFB option, Uncache acceleration support(at the kernel size) should also be enabled. Otherwise the performance of graphic application is just slow. Beside write-combine cache mode have bugs on our platform, a kernel side developer have disabled it. Write-combine cache mode just boil down to uncached now. See [1] and [2] [1]https://lkml.org/lkml/2020/8/10/255 [2]https://lkml.kernel.org/lkml/1617701112-14007-1-git-send-email-yangtiezhu@loongson.cn/T/ This is the reason why we prefer CMA helper base solution with non coherent mapping, simply because it is fast. As far as I know, Loongson's CPU does not has the concept of write-combine, it only support three caching mode: uncached, cached and uncache acceleration. write-combine is implemented with uncache acceleration on Mips.
>> +static int lsdc_primary_plane_atomic_check(struct drm_plane *plane, >> + struct drm_atomic_state *state) >> +{ >> + struct drm_device *ddev = plane->dev; >> + struct lsdc_device *ldev = to_lsdc(ddev); >> + struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane); >> + struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane); >> + struct drm_framebuffer *new_fb = new_plane_state->fb; >> + struct drm_framebuffer *old_fb = old_plane_state->fb; >> + struct drm_crtc *crtc = new_plane_state->crtc; >> + u32 new_format = new_fb->format->format; >> + struct drm_crtc_state *new_crtc_state; >> + struct lsdc_crtc_state *priv_crtc_state; >> + int ret; >> + >> + if (!crtc) >> + return 0; >> + >> + new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc); >> + if (WARN_ON(!new_crtc_state)) >> + return -EINVAL; >> + >> + priv_crtc_state = to_lsdc_crtc_state(new_crtc_state); >> + >> + ret = drm_atomic_helper_check_plane_state(new_plane_state, >> + new_crtc_state, >> + DRM_PLANE_HELPER_NO_SCALING, >> + DRM_PLANE_HELPER_NO_SCALING, >> + false, >> + true); >> + if (ret) >> + return ret; >> + >> + /* >> + * Require full modeset if enabling or disabling a plane, >> + * or changing its position, size, depth or format. >> + */ >> + if ((!new_fb || !old_fb || >> + old_plane_state->crtc_x != new_plane_state->crtc_x || >> + old_plane_state->crtc_y != new_plane_state->crtc_y || >> + old_plane_state->crtc_w != new_plane_state->crtc_w || >> + old_plane_state->crtc_h != new_plane_state->crtc_h || >> + old_fb->format->format != new_format)) >> + new_crtc_state->mode_changed = true; >> + >> + >> + priv_crtc_state->pix_fmt = lsdc_primary_get_default_format(crtc); > Storing the pixel format in the CRTC state is weird? What would happen > if you have a primary plane and a cursor in different formats? > > Also, reading the default format from a register doesn't look right. > atomic_check can occur at any time, including before a previous commit, > or while the hardware is disabled. You should rely on either a constant > or the previous state here. > Currently, private CRTC state(priv_crtc_state) is not get used by the cursor's atomic_check() and atomic_update(). I means this is only for the primary plane. And both and the primary and the cursor using XRGB8888 format, what I really want here is let the atomic_update to update the framebuffer's format, because the firmware (pmon) of some board set the framebuffer's format as RGB565. If the hardware's format is same with the plane state, then there is no need to update the FB's format register, save a function call, right? When the plane is disabled the drm core will call the atomic_disable() function, right? I will reconsider this, thank for your advice and i will correct other problems at the next version. Thanks for you take time review my patch again.
On Fri, Feb 04, 2022 at 12:41:37AM +0800, Sui Jingfeng wrote: > > > +static int lsdc_primary_plane_atomic_check(struct drm_plane *plane, > > > + struct drm_atomic_state *state) > > > +{ > > > + struct drm_device *ddev = plane->dev; > > > + struct lsdc_device *ldev = to_lsdc(ddev); > > > + struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane); > > > + struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane); > > > + struct drm_framebuffer *new_fb = new_plane_state->fb; > > > + struct drm_framebuffer *old_fb = old_plane_state->fb; > > > + struct drm_crtc *crtc = new_plane_state->crtc; > > > + u32 new_format = new_fb->format->format; > > > + struct drm_crtc_state *new_crtc_state; > > > + struct lsdc_crtc_state *priv_crtc_state; > > > + int ret; > > > + > > > + if (!crtc) > > > + return 0; > > > + > > > + new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc); > > > + if (WARN_ON(!new_crtc_state)) > > > + return -EINVAL; > > > + > > > + priv_crtc_state = to_lsdc_crtc_state(new_crtc_state); > > > + > > > + ret = drm_atomic_helper_check_plane_state(new_plane_state, > > > + new_crtc_state, > > > + DRM_PLANE_HELPER_NO_SCALING, > > > + DRM_PLANE_HELPER_NO_SCALING, > > > + false, > > > + true); > > > + if (ret) > > > + return ret; > > > + > > > + /* > > > + * Require full modeset if enabling or disabling a plane, > > > + * or changing its position, size, depth or format. > > > + */ > > > + if ((!new_fb || !old_fb || > > > + old_plane_state->crtc_x != new_plane_state->crtc_x || > > > + old_plane_state->crtc_y != new_plane_state->crtc_y || > > > + old_plane_state->crtc_w != new_plane_state->crtc_w || > > > + old_plane_state->crtc_h != new_plane_state->crtc_h || > > > + old_fb->format->format != new_format)) > > > + new_crtc_state->mode_changed = true; > > > + > > > + > > > + priv_crtc_state->pix_fmt = lsdc_primary_get_default_format(crtc); > > Storing the pixel format in the CRTC state is weird? What would happen > > if you have a primary plane and a cursor in different formats? > > > > Also, reading the default format from a register doesn't look right. > > atomic_check can occur at any time, including before a previous commit, > > or while the hardware is disabled. You should rely on either a constant > > or the previous state here. > > > Currently, private CRTC state(priv_crtc_state) is not get used by the cursor's > atomic_check() and atomic_update(). I means this is only for the primary plane. > And both and the primary and the cursor using XRGB8888 format, what I really > want here is let the atomic_update to update the framebuffer's format, because > the firmware (pmon) of some board set the framebuffer's format as RGB565. atomic_update will be called each time the plane state is changed, so it won't be an issue: when the first state will be committed, your atomic_update function will be called and thus you'll overwrite what was left of the firmware setup. > If the hardware's format is same with the plane state, then there is no need to > update the FB's format register, save a function call, right? My point was more about the fact that you're using the wrong abstraction there. The format is a property of the plane, not from the CRTC. In KMS (and in most drivers), you can have multiple planes with different formats all attached to the same CRTC just fine. Maxime
On Fri, Feb 04, 2022 at 12:29:39AM +0800, Sui Jingfeng wrote: > > > +static int lsdc_modeset = 1; > > > +MODULE_PARM_DESC(modeset, "Enable/disable CMA-based KMS(1 = enabled(default), 0 = disabled)"); > > > +module_param_named(modeset, lsdc_modeset, int, 0644); > > > + > > > +static int lsdc_cached_coherent = 1; > > > +MODULE_PARM_DESC(cached_coherent, "uss cached coherent mapping(1 = enabled(default), 0 = disabled)"); > > > +module_param_named(cached_coherent, lsdc_cached_coherent, int, 0644); > > > + > > > +static int lsdc_dirty_update = -1; > > > +MODULE_PARM_DESC(dirty_update, "enable dirty update(1 = enabled, 0 = disabled(default))"); > > > +module_param_named(dirty_update, lsdc_dirty_update, int, 0644); > > > + > > > +static int lsdc_use_vram_helper = -1; > > > +MODULE_PARM_DESC(use_vram_helper, "use vram helper based solution(1 = enabled, 0 = disabled(default))"); > > > +module_param_named(use_vram_helper, lsdc_use_vram_helper, int, 0644); > > > + > > > +static int lsdc_verbose = -1; > > > +MODULE_PARM_DESC(verbose, "Enable/disable print some key information"); > > > +module_param_named(verbose, lsdc_verbose, int, 0644); > > > > It's not really clear to me why you need any of those parameters. Why > > would a user want to use a non coherent mapping for example? > > > Because we are Mips architecture. Paul Cercueil already explained it > in his mmap GEM buffers cachedpatch <https://lkml.kernel.org/lkml/20200822164233.71583-1-paul@crapouillou.net/T/>. I drag part of it to here for > convenient to reading: > > /Traditionally, GEM buffers are mapped write-combine. Writes to the buffer > are accelerated, and reads are slow. Application doing lots////of > alpha-blending paint inside shadow buffers, which is then memcpy'd////into > the final GEM buffer./// > "non coherent mapping" is actually cached and it is for CMA helpers > base driver, not for VRAM helper based driver. For Loongson CPU/SoCs. > The cache coherency is maintained by hardware, therefore there no > need to worry about coherency problems. This is true at least for > ls3a3000, ls3a4000 and ls3a5000. > > "non coherent" or "coherent" is not important here, the key point is > that the backing memory of the framebuffer is cached with non coherent > mapping, you don't need a shadow buffer layer when using X server's > modesetting driver. > > Read and write to the framebuffer in system memory is much faster than > read and write to the framebuffer in the VRAM. > > Why CMA helper based solution is faster than the VRAM based solution on Mips platform? > > Partly because of the CPU have L1, L2 and L3 cache, especially L3 cache > is as large as 8MB, read and write from the cache is fast. > > Another reason is as Paul Cercueil said, read from VRAM with write-combine > cache mode is slow. it is just uncache read. > Please note that we don't have a GPU here, we are just a display controller. > > For the VRAM helper based driver case, the backing memory of the framebuffer > is located at VRAM, When using X server's modesetting driver, we have to enable > the ShadowFB option, Uncache acceleration support(at the kernel size) should > also be enabled. Otherwise the performance of graphic application is just slow. > > Beside write-combine cache mode have bugs on our platform, a kernel side > developer have disabled it. Write-combine cache mode just boil down to uncached > now. See [1] and [2] > > [1]https://lkml.org/lkml/2020/8/10/255 > [2]https://lkml.kernel.org/lkml/1617701112-14007-1-git-send-email-yangtiezhu@loongson.cn/T/ > > This is the reason why we prefer CMA helper base solution with non coherent mapping, > simply because it is fast. > > As far as I know, Loongson's CPU does not has the concept of write-combine, > it only support three caching mode: uncached, cached and uncache acceleration. > write-combine is implemented with uncache acceleration on Mips. My point wasn't just about the VRAM vs CMA stuff, it was about why do you need all those switches in the first place? Take the verbose parameter for example: it's entirely redundant with the already existing, documented, DRM logging infrastructure. Then, you have "modeset", and I'm not sure why it's supposed to be there, at all. This is a modesetting driver, why would I want to disable modesetting entirely? More fundamentally (and this extends to the CMA, caching and VRAM stuff you explained above), why can't the driver pick the right decision all the time and why would that be under the user control? You were mentioning that you need to work-around MIPS memory management. Then fine, just do that on MIPS, and don't it on the other architectures that don't need it. There's no need for a knob. Maxime
On Fri, Feb 04, 2022 at 12:04:19AM +0800, Sui Jingfeng wrote: > > > +/* Get the simple EDID data from the device tree > > > + * the length must be EDID_LENGTH, since it is simple. > > > + * > > > + * @np: device node contain edid data > > > + * @edid_data: where the edid data to store to > > > + */ > > > +static bool lsdc_get_edid_from_dtb(struct device_node *np, > > > + unsigned char *edid_data) > > > +{ > > > + int length; > > > + const void *prop; > > > + > > > + if (np == NULL) > > > + return false; > > > + > > > + prop = of_get_property(np, "edid", &length); > > > + if (prop && (length == EDID_LENGTH)) { > > > + memcpy(edid_data, prop, EDID_LENGTH); > > > + return true; > > > + } > > > + > > > + return false; > > > +} > > > > You don't have a device tree binding for that driver, this is something > > that is required. And it's not clear to me why you'd want EDID in the > > DTB? > > 1) It is left to the end user of this driver. > > The downstream motherboard maker may use a dpi(XRGB888) or LVDS panel > which don't have DDC support either, doing this way allow them put a > EDID property into the dc device node in the DTS. Then the entire system works. > Note those panel usually support only one display mode. I guess it depends on what we mean exactly by the user, but the DTB usually isn't under the (end) user control. And the drm.edid_firmware is here already to address exactly this issue. On the other end, if the board has a static panel without any DDC lines, then just put the timings in the device tree, there's no need for an EDID blob. > 2) That is for the display controller in ls2k1000 SoC. > > Currently, the upstream kernel still don't have GPIO, PWM and I2C driver support > for LS2K1000 SoC. > > How dose you read EDID from the monitor without a I2C driver? > > without reading EDID the device tree support, the screen just black, > the lsdc driver just stall. With reading EDID from device tree support > we do not need a i2c driver to light up the monitor. > > This make lsdc drm driver work on various ls2k1000 development board > before I2C driver and GPIO driver and PWM backlight driver is upstream. > > I have many local private dts with the bindings, those local change just can not > upstream at this time, below is an example. The device tree is a platform description language. It's there to let the OS know what the hardware is, but the state of hardware support in the said OS isn't a parameter we have to take into account for a new binding. If you don't have any DDC support at the moment, use the firmware mechanism above, or add fixed modes using drm_add_modes_noedid in the driver, and leave the DT out of it. Once you'll gain support for the EDID readout in the driver, then it'll just work and you won't need to change the DT again. > 3) Again, doing this way is for graphic environment bring up. > > &lsdc { > > output-ports = <&dvo0 &dvo1>; > #address-cells = <1>; > #size-cells = <0>; > dvo0: dvo@0 { > reg = <0>; > > connector = "dpi-connector"; > encoder = "none"; > status = "ok"; > > display-timings { > native-mode = <&mode_0_1024x600_60>; > > mode_0_1024x600_60: panel-timing@0 { > clock-frequency = <51200000>; > hactive = <1024>; > vactive = <600>; > hsync-len = <4>; > hfront-porch = <160>; > hback-porch = <156>; > vfront-porch = <11>; > vback-porch = <23>; > vsync-len = <1>; > }; > > mode_1_800x480_60: panel-timing@1 { > clock-frequency = <30066000>; > hactive = <800>; > vactive = <480>; > hfront-porch = <50>; > hback-porch = <70>; > hsync-len = <50>; > vback-porch = <0>; > vfront-porch = <0>; > vsync-len = <50>; > }; > }; > }; > > dvo1: dvo@1 { > reg = <1>; > > connector = "hdmi-connector"; > type = "a"; > encoder = "sil9022"; > > edid = [ 00 ff ff ff ff ff ff 00 1e 6d 54 5b 0b cc 04 00 > 02 1c 01 03 6c 30 1b 78 ea 31 35 a5 55 4e a1 26 > 0c 50 54 a5 4b 00 71 4f 81 80 95 00 b3 00 a9 c0 > 81 00 81 c0 90 40 02 3a 80 18 71 38 2d 40 58 2c > 45 00 e0 0e 11 00 00 1e 00 00 00 fd 00 38 4b 1e > 53 0f 00 0a 20 20 20 20 20 20 00 00 00 fc 00 4c > 47 20 46 55 4c 4c 20 48 44 0a 20 20 00 00 00 ff > 00 38 30 32 4e 54 43 5a 39 38 33 37 39 0a 00 35 ]; > > status = "ok"; > }; > }; Yeah, this needs to be documented with a YAML schema Maxime
On Thu, Feb 03, 2022 at 11:47:16PM +0800, Sui Jingfeng wrote: > On 2022/2/3 16:58, Maxime Ripard wrote: > > > diff --git a/drivers/gpu/drm/lsdc/Kconfig b/drivers/gpu/drm/lsdc/Kconfig > > > new file mode 100644 > > > index 000000000000..7ed1b0fdbe1b > > > --- /dev/null > > > +++ b/drivers/gpu/drm/lsdc/Kconfig > > > @@ -0,0 +1,38 @@ > > > +config DRM_LSDC > > > + tristate "DRM Support for loongson's display controller" > > > + depends on DRM && PCI > > > + depends on MACH_LOONGSON64 || LOONGARCH || MIPS || COMPILE_TEST > > > + select OF > > > + select CMA if HAVE_DMA_CONTIGUOUS > > > + select DMA_CMA if HAVE_DMA_CONTIGUOUS > > > + select DRM_KMS_HELPER > > > + select DRM_KMS_FB_HELPER > > > + select DRM_GEM_CMA_HELPER > > > + select VIDEOMODE_HELPERS > > > + select BACKLIGHT_PWM if CPU_LOONGSON2K > > > + select I2C_GPIO if CPU_LOONGSON2K > > > + select I2C_LS2X if CPU_LOONGSON2K > > > + default m > > > + help > > > + This is a KMS driver for the display controller in the LS7A1000 > > > + bridge chip and LS2K1000 SoC. The display controller has two > > > + display pipes and it is a PCI device. > > > + When using this driver on LS2K1000/LS2K0500 SoC, its framebuffer > > > + is located at system memory. > > > + If "M" is selected, the module will be called lsdc. > > > + > > > + If in doubt, say "Y". > > > + > > > +config DRM_LSDC_VRAM_DRIVER > > > + bool "vram helper based device driver support" > > > + depends on DRM_LSDC > > > + select DRM_VRAM_HELPER > > > + default y > > > + help > > > + When using this driver on LS7A1000 + LS3A3000/LS3A4000/LS3A5000 > > > + platform, the LS7A1000 bridge chip has dedicated video RAM. Using > > > + "lsdc.use_vram_helper=1" in the kernel command line will enable > > > + this driver mode and then the framebuffer will be located at the > > > + VRAM at the price of losing PRIME support. > > > + > > > + If in doubt, say "Y". > > This doesn't sound right. The driver should make the proper decision > > depending on the platform, not the user or the distribution. > > The LS7A1000 north bridge chip has dedicated video RAM, but the DC in LS7A1000 > can also scanout from the system memory directly like a display controller in a > SoC does. In fact, this display controller is envolved from early product of > Loongson 2H SoC. This driver still works even if the downstream PC board > manufacturer doesn't solder a video RAM on the mother board. > > The lsdc_should_vram_helper_based() function in lsdc_drv.c will examine > if the DC device node contain a use_vram_helper property at driver loading time. > If there is no use_vram_helper property, CMA helper based DRM driver will be used. > Doing this way allow the user using "lsdc.use_vram_helper=0" override the default > behavior even through there is a "use_vram_helper;" property in the DTS. > > In short, It is intend to let the command line passed from kernel has higher > priority than the device tree. Otherwise the end user can not switch different > driver mode through the kernel command line once the DC device node contain > "use_vram_helper;" property. > > This driver's author already made the decision by the time when the patch is > sent out, even through this**may not proper. > > The CMA helper based driver will be used by default, if the DC device node contain > "use_vram_helper;" then VRAM based driver will be used. This is my initial intention. DT isn't really a solution either. Let's take the distribution perspective there. Suppose you're a Fedora or Debian developer, and want to make a single kernel image, and ship a DT to the user for their board without any modification. How is either the Kconfig solution or DT flags solution any good there? It doesn't help them at all. Maxime
在 2022/2/9 8:52, Maxime Ripard 写道: > On Thu, Feb 03, 2022 at 11:47:16PM +0800, Sui Jingfeng wrote: [...] > DT isn't really a solution either. Let's take the distribution > perspective there. Suppose you're a Fedora or Debian developer, and want > to make a single kernel image, and ship a DT to the user for their board > without any modification. How is either the Kconfig solution or DT flags > solution any good there? It doesn't help them at all. We are working in another way. As we can tell model of the board by strings passed from the firmware, we just built-in all poosible DTs into the kernel and then tell which DT to load at boot time. So we can ensure users has smmoth experience. Thanks. - Jiaxun > > Maxime
Hi Jingfeng, Could you please keep me CCed for the for the whole thread when you respin the patch? Cuz I'm maintain MIPS/LOONGSON64 stuff and I believe this series is partially based on my work at Lemote. I will help with reviewing and explain some Loongson64 specified issue if possible. Thanks. - Jiaxun 在 2022/2/3 8:25, Sui Jingfeng 写道: > There is a display controller in loongson's LS2K1000 SoC and LS7A1000 > bridge, and the DC in those chip is a PCI device. This patch provide > a minimal support for this display controller which is mainly for > graphic environment bring up. > > This display controller has two display pipes but with only one hardware > cursor. Each way has a DVO output interface and the CRTC is able to scanout > from 1920x1080 resolution at 60Hz. The maxmium resolution is 2048x2048@60hz. > > LS2K1000 is a SoC, only system memory is available. Therefore scanout from > system memory is the only choice. We prefer the CMA helper base solution > because the gc1000 gpu can use etnaviv driver, in this case etnaviv and > lsdc could made a compatible pair. Even through it is possible to use VRAM > helper base solution on ls2k1000 by carving out part of system memory as > VRAM. > > For LS7A1000, there are 4 gpios whos control register is located at the dc > register space which is not the geneal purpose GPIO. The 4 gpios can emulate > two way i2c. One for DVO0, another for DVO1. This is the reason the why we > are not using the drm bridge framework. > > LS2K1000 and LS2K0500 SoC don't have such hardware, they use general purpose > GPIO emulated i2c or hardware i2c adapter from other module to serve this > purpose. Drm bridge and drm panel is suitable for those SoC, we have already > implement it on our own downstream kernel. But due to the upstream kernel > don't have gpio, pwm and i2c driver support for LS2K1000. We just can not > upstream our support for the drm bridge subsystem. > > The DC in LS7A1000 can scanout from both the system memory and the dedicate > VRAM attached to the ddr3 memory controller. Sadly, only scanout from the > VRAM is proved to be a reliable solution for massive product. Scanout from > the system memory suffer from some hardware deficiency which cause the > screen flickering under RAM pressure. This is the reason why we integrate > two distict helpers into one piece of device driver. But CMA base helper is > still usable on ls7a1000 for normal usage, expecially on ls7a1000+ bridge > chip. We have also implemented demage update on top of CMA helper which > copy the demaged shadow framebuffer region from system RAM to the real > framebuffer in VRAM manually. Using "lsdc.dirty_update=1" in the commmand > line will enable this driver mode. > > LS7A1000 have a 32x32 harware cursor, we just let the two CRTC share it > simply with the help of universe plane. LS7A2000 have two 64x64 harware > cursor. Surport for LS7A2000 is on the way. > > In short, we have built-in gpio emulated i2c support, we also have hardware > cursor support. The kind of tiny drivers in drm/tiny is not suitable for us, > we are not "tiny". > > +------+ HyperTransport 3.0 > | DDR4 | | > +------+ | +------------------------------------+ > || MC0 | | LS7A1000 +------------| > +----------+ | | | DDR3 | +------+ > | LS3A4000 |<--------->| +--------+ +-------+ | memory |<->| VRAM | > | CPU |<--------->| | GC1000 | | LSDC | | controller | +------+ > +----------+ | +--------+ +-+---+-+ +------------| > || MC1 +---------------|---|----------------+ > +------+ | | > | DDR4 | +-------+ DVO0 | | DVO1 +------+ > +------+ VGA <--|ADV7125|<---------+ +------->|TFP410|--> DVI/HDMI > +-------+ +------+ > > The above picture give a simple usage of LS7A1000, note that the encoder > is not necessary adv7125 or tfp410, it is a choice of the downstream board > manufacturer. Other candicate encoder can be ch7034b, sil9022 and ite66121 > etc. Therefore, we need device tree to provide board specific information. > Besides, the dc in both ls2k1000 and ls7k1000 have the vendor:device id of > 0x0014:0x7a06, the reverison id is also same. We can't tell it apart simply > (this is the firmware engineer's mistake). But firmware already flushed to > the board and borad already sold out, we choose to resolve those issues by > introduing device tree with board specific device support. > > For lsdc, there is only a 1:1 mapping of encoders and connectors. > > +-------------------+ _________ > | | | | > | CRTC0 --> DVO0 ---------> Encoder0 --> Connector0 ---> | Monitor | > | | ^ ^ |_________| > | | | | > | <----- i2c0 ----------------+ > | LSDC IP CORE | > | <----- i2c1 ----------------+ > | | | | _________ > | | | | | | > | CRTC1 --> DVO1 ---------> Encoder1 --> Connector1 ---> | Panel | > | | |_________| > +-------------------+ > > Below is a brief introduction of loongson's CPU, bridge chip and SoC. > LS2K1000 is a double core 1.0Ghz mips64r2 compatible SoC[1]. LS7A1000 is > a bridge chip made by Loongson corporation which act as north and/or south > bridge of loongson's desktop and server level processor. It is equivalent > to AMD RS780E+SB710 or something like that. More details can be read from > its user manual[2]. > > This bridge chip is typically use with LS3A3000, LS3A4000 and LS3A5000 cpu. > LS3A3000 is 4 core 1.45gHz mips64r2 compatible cpu. > LS3A4000 is 4 core 1.8gHz mips64r5 compatible cpu. > LS3A5000 is 4 core 2.5gHz loongarch cpu[3]. > > Nearly all mordern loongson CPU's cache coherency is maintained by hardware, > except for early version of ls2k1000. So we using cached coherent memory by > default, not writecombine. > > v2: fixup warnings reported by kernel test robot > > v3: fix more grammar mistakes in Kconfig reported by Randy Dunlap and give > more details about lsdc. > > v4: > 1) Add dts required and explain why device tree is required. > 2) Give more description about lsdc and vram helper base driver. > 3) Fix warnings reported by kernel test robot. > 4) Introduce stride_alignment member into struct lsdc_chip_desc, the > stride alignment is 256 bytes for ls7a1000, ls2k1000 and ls2k0500 SoC. > But ls7a2000 improve it to 32 bytes, We are prepare for extend the > support for the on coming device. > > v5: > 1) using writel and readl replace writeq and readq, to fix kernel test > robot report build error on other archtecture > 2) set default fb format to XRGB8888 at crtc reset time. > 3) fix typos. > > v6: > 1) Explain why we are not switch to drm dridge subsystem. > 2) Explain why tiny drm driver is not suitable for us. > 3) Give a short description of the trival dirty uppdate implement based > on CMA helper. > 4) code clean up. > > [1] https://wiki.debian.org/InstallingDebianOn/Lemote/Loongson2K1000 > [2] https://loongson.github.io/LoongArch-Documentation/Loongson-7A1000-usermanual-EN.html > [3] https://loongson.github.io/LoongArch-Documentation/Loongson-3A5000-usermanual-EN.html > > Reported-by: Randy Dunlap <rdunlap@infradead.org> > Reported-by: kernel test robot > Signed-off-by: suijingfeng <suijingfeng@loongson.cn> > Signed-off-by: Sui Jingfeng <15330273260@189.cn> > > suijingfeng (3): > drm/lsdc: add drm driver for loongson display controller > dt-bindings: ls2k1000: add the display controller device node > dt-bindings: mips: loongson: introduce board specific dts > > arch/mips/boot/dts/loongson/lemote_a1901.dts | 64 ++ > .../boot/dts/loongson/loongson64-2k1000.dtsi | 11 + > .../boot/dts/loongson/ls3a4000_7a1000_evb.dts | 68 ++ > arch/mips/boot/dts/loongson/ls7a-pch.dtsi | 2 +- > drivers/gpu/drm/Kconfig | 2 + > drivers/gpu/drm/Makefile | 1 + > drivers/gpu/drm/lsdc/Kconfig | 38 + > drivers/gpu/drm/lsdc/Makefile | 15 + > drivers/gpu/drm/lsdc/lsdc_connector.c | 443 +++++++++ > drivers/gpu/drm/lsdc/lsdc_connector.h | 60 ++ > drivers/gpu/drm/lsdc/lsdc_crtc.c | 440 +++++++++ > drivers/gpu/drm/lsdc/lsdc_drv.c | 846 ++++++++++++++++++ > drivers/gpu/drm/lsdc/lsdc_drv.h | 216 +++++ > drivers/gpu/drm/lsdc/lsdc_encoder.c | 79 ++ > drivers/gpu/drm/lsdc/lsdc_i2c.c | 220 +++++ > drivers/gpu/drm/lsdc/lsdc_i2c.h | 61 ++ > drivers/gpu/drm/lsdc/lsdc_irq.c | 77 ++ > drivers/gpu/drm/lsdc/lsdc_irq.h | 37 + > drivers/gpu/drm/lsdc/lsdc_plane.c | 681 ++++++++++++++ > drivers/gpu/drm/lsdc/lsdc_pll.c | 657 ++++++++++++++ > drivers/gpu/drm/lsdc/lsdc_pll.h | 109 +++ > drivers/gpu/drm/lsdc/lsdc_regs.h | 246 +++++ > 22 files changed, 4372 insertions(+), 1 deletion(-) > create mode 100644 arch/mips/boot/dts/loongson/lemote_a1901.dts > create mode 100644 arch/mips/boot/dts/loongson/ls3a4000_7a1000_evb.dts > create mode 100644 drivers/gpu/drm/lsdc/Kconfig > create mode 100644 drivers/gpu/drm/lsdc/Makefile > create mode 100644 drivers/gpu/drm/lsdc/lsdc_connector.c > create mode 100644 drivers/gpu/drm/lsdc/lsdc_connector.h > create mode 100644 drivers/gpu/drm/lsdc/lsdc_crtc.c > create mode 100644 drivers/gpu/drm/lsdc/lsdc_drv.c > create mode 100644 drivers/gpu/drm/lsdc/lsdc_drv.h > create mode 100644 drivers/gpu/drm/lsdc/lsdc_encoder.c > create mode 100644 drivers/gpu/drm/lsdc/lsdc_i2c.c > create mode 100644 drivers/gpu/drm/lsdc/lsdc_i2c.h > create mode 100644 drivers/gpu/drm/lsdc/lsdc_irq.c > create mode 100644 drivers/gpu/drm/lsdc/lsdc_irq.h > create mode 100644 drivers/gpu/drm/lsdc/lsdc_plane.c > create mode 100644 drivers/gpu/drm/lsdc/lsdc_pll.c > create mode 100644 drivers/gpu/drm/lsdc/lsdc_pll.h > create mode 100644 drivers/gpu/drm/lsdc/lsdc_regs.h >
On Wed, Feb 09, 2022 at 11:56:48AM +0000, Jiaxun Yang wrote: > > > 在 2022/2/9 8:52, Maxime Ripard 写道: > > On Thu, Feb 03, 2022 at 11:47:16PM +0800, Sui Jingfeng wrote: > [...] > > DT isn't really a solution either. Let's take the distribution > > perspective there. Suppose you're a Fedora or Debian developer, and want > > to make a single kernel image, and ship a DT to the user for their board > > without any modification. How is either the Kconfig solution or DT flags > > solution any good there? It doesn't help them at all. > > We are working in another way. As we can tell model of the board by strings > passed from the firmware, we just built-in all poosible DTs into the kernel > and then tell which DT to load at boot time. So we can ensure users has > smmoth experience. It's not really for you to say though. Once your driver is in a release, distros are going to use it. That's the whole point of you asking us to merge it. Maxime
在 2022/2/9 14:04, Maxime Ripard 写道: > On Wed, Feb 09, 2022 at 11:56:48AM +0000, Jiaxun Yang wrote: >> >> 在 2022/2/9 8:52, Maxime Ripard 写道: >>> On Thu, Feb 03, 2022 at 11:47:16PM +0800, Sui Jingfeng wrote: >> [...] >>> DT isn't really a solution either. Let's take the distribution >>> perspective there. Suppose you're a Fedora or Debian developer, and want >>> to make a single kernel image, and ship a DT to the user for their board >>> without any modification. How is either the Kconfig solution or DT flags >>> solution any good there? It doesn't help them at all. >> We are working in another way. As we can tell model of the board by strings >> passed from the firmware, we just built-in all poosible DTs into the kernel >> and then tell which DT to load at boot time. So we can ensure users has >> smmoth experience. > It's not really for you to say though. Once your driver is in a release, > distros are going to use it. That's the whole point of you asking us to > merge it. Ah I was meant to say we can determine which type of memory is best for a type of board and then hardcode it into ST accroading to the model. Though personally I want a knob in cmdline to override it at runtime. Thanks. - Jiaxun > > Maxime
On 2022/2/9 16:49, Maxime Ripard wrote: > On Fri, Feb 04, 2022 at 12:04:19AM +0800, Sui Jingfeng wrote: >>>> +/* Get the simple EDID data from the device tree >>>> + * the length must be EDID_LENGTH, since it is simple. >>>> + * >>>> + * @np: device node contain edid data >>>> + * @edid_data: where the edid data to store to >>>> + */ >>>> +static bool lsdc_get_edid_from_dtb(struct device_node *np, >>>> + unsigned char *edid_data) >>>> +{ >>>> + int length; >>>> + const void *prop; >>>> + >>>> + if (np == NULL) >>>> + return false; >>>> + >>>> + prop = of_get_property(np, "edid", &length); >>>> + if (prop && (length == EDID_LENGTH)) { >>>> + memcpy(edid_data, prop, EDID_LENGTH); >>>> + return true; >>>> + } >>>> + >>>> + return false; >>>> +} >>> You don't have a device tree binding for that driver, this is something >>> that is required. And it's not clear to me why you'd want EDID in the >>> DTB? >> 1) It is left to the end user of this driver. >> >> The downstream motherboard maker may use a dpi(XRGB888) or LVDS panel >> which don't have DDC support either, doing this way allow them put a >> EDID property into the dc device node in the DTS. Then the entire system works. >> Note those panel usually support only one display mode. > I guess it depends on what we mean exactly by the user, but the DTB > usually isn't under the (end) user control. And the drm.edid_firmware is > here already to address exactly this issue. > > On the other end, if the board has a static panel without any DDC lines, > then just put the timings in the device tree, there's no need for an > EDID blob. Loongson have a long history of using PMON firmware, The PMON firmware support flush the dtb into the the firmware before grub loading the kernel. You press 'c' key, then the PMON will give you a shell. it is much like a UEFI shell. Suppose foo.dtb is what you want to pass to the vmlinuz. Then type the follow single command can flush the dtb into the PMON firmware. |load_dtb /dev/fs/fat@usb0/foo.dtb| For our PMON firmware, it**is** totally under developer/pc board maker's control. You can flush whatever dtb every time you bootup until you satisfied. It(the pmon firmware) is designed to let downstream motherboard maker and/or customers to play easily. Support of reading EDID from the dtb is really a feature which downstream motherboard maker or customer wanted. They sometimes using eDP also whose resolution is not 1024x768. This is out of control for a graphic driver developer like me. And drm.edid_firmware have only a few limited resolution which is weak. I will consider to adding drm.edid_firmware support, thanks. >> 2) That is for the display controller in ls2k1000 SoC. >> >> Currently, the upstream kernel still don't have GPIO, PWM and I2C driver support >> for LS2K1000 SoC. >> >> How dose you read EDID from the monitor without a I2C driver? >> >> without reading EDID the device tree support, the screen just black, >> the lsdc driver just stall. With reading EDID from device tree support >> we do not need a i2c driver to light up the monitor. >> >> This make lsdc drm driver work on various ls2k1000 development board >> before I2C driver and GPIO driver and PWM backlight driver is upstream. >> >> I have many local private dts with the bindings, those local change just can not >> upstream at this time, below is an example. >> >> The device tree is a platform description language. It's there to let >> the OS know what the hardware is, but the state of hardware support in >> the said OS isn't a parameter we have to take into account for a new >> binding. >> >> If you don't have any DDC support at the moment, use the firmware >> mechanism above, or add fixed modes using drm_add_modes_noedid in the >> driver, and leave the DT out of it. Once you'll gain support for the >> EDID readout in the driver, then it'll just work and you won't need to >> change the DT again. >> The resolution will be 1024x768, it will also add a lot modes which may not supported by the specific panel. Take 1024x600 as an example, Both drm_add_modes_noedid() and firmware mechanism above will fail. Because the user supply EDID only and manufacturer of some strange panel supply EDID only. >> 3) Again, doing this way is for graphic environment bring up. >> >> &lsdc { >> >> output-ports = <&dvo0 &dvo1>; >> #address-cells = <1>; >> #size-cells = <0>; >> dvo0: dvo@0 { >> reg = <0>; >> >> connector = "dpi-connector"; >> encoder = "none"; >> status = "ok"; >> >> display-timings { >> native-mode = <&mode_0_1024x600_60>; >> >> mode_0_1024x600_60: panel-timing@0 { >> clock-frequency = <51200000>; >> hactive = <1024>; >> vactive = <600>; >> hsync-len = <4>; >> hfront-porch = <160>; >> hback-porch = <156>; >> vfront-porch = <11>; >> vback-porch = <23>; >> vsync-len = <1>; >> }; >> >> mode_1_800x480_60: panel-timing@1 { >> clock-frequency = <30066000>; >> hactive = <800>; >> vactive = <480>; >> hfront-porch = <50>; >> hback-porch = <70>; >> hsync-len = <50>; >> vback-porch = <0>; >> vfront-porch = <0>; >> vsync-len = <50>; >> }; >> }; >> }; >> >> dvo1: dvo@1 { >> reg = <1>; >> >> connector = "hdmi-connector"; >> type = "a"; >> encoder = "sil9022"; >> >> edid = [ 00 ff ff ff ff ff ff 00 1e 6d 54 5b 0b cc 04 00 >> 02 1c 01 03 6c 30 1b 78 ea 31 35 a5 55 4e a1 26 >> 0c 50 54 a5 4b 00 71 4f 81 80 95 00 b3 00 a9 c0 >> 81 00 81 c0 90 40 02 3a 80 18 71 38 2d 40 58 2c >> 45 00 e0 0e 11 00 00 1e 00 00 00 fd 00 38 4b 1e >> 53 0f 00 0a 20 20 20 20 20 20 00 00 00 fc 00 4c >> 47 20 46 55 4c 4c 20 48 44 0a 20 20 00 00 00 ff >> 00 38 30 32 4e 54 43 5a 39 38 33 37 39 0a 00 35 ]; >> >> status = "ok"; >> }; >> }; > Yeah, this needs to be documented with a YAML schema > > Maxime Yes, It takes time to learn that.
On 2022/2/9 16:43, Maxime Ripard wrote: > On Fri, Feb 04, 2022 at 12:29:39AM +0800, Sui Jingfeng wrote: >>>> +static int lsdc_modeset = 1; >>>> +MODULE_PARM_DESC(modeset, "Enable/disable CMA-based KMS(1 = enabled(default), 0 = disabled)"); >>>> +module_param_named(modeset, lsdc_modeset, int, 0644); >>>> + >>>> +static int lsdc_cached_coherent = 1; >>>> +MODULE_PARM_DESC(cached_coherent, "uss cached coherent mapping(1 = enabled(default), 0 = disabled)"); >>>> +module_param_named(cached_coherent, lsdc_cached_coherent, int, 0644); >>>> + >>>> +static int lsdc_dirty_update = -1; >>>> +MODULE_PARM_DESC(dirty_update, "enable dirty update(1 = enabled, 0 = disabled(default))"); >>>> +module_param_named(dirty_update, lsdc_dirty_update, int, 0644); >>>> + >>>> +static int lsdc_use_vram_helper = -1; >>>> +MODULE_PARM_DESC(use_vram_helper, "use vram helper based solution(1 = enabled, 0 = disabled(default))"); >>>> +module_param_named(use_vram_helper, lsdc_use_vram_helper, int, 0644); >>>> + >>>> +static int lsdc_verbose = -1; >>>> +MODULE_PARM_DESC(verbose, "Enable/disable print some key information"); >>>> +module_param_named(verbose, lsdc_verbose, int, 0644); >>> It's not really clear to me why you need any of those parameters. Why >>> would a user want to use a non coherent mapping for example? >>> >> Because we are Mips architecture. Paul Cercueil already explained it >> in his mmap GEM buffers cachedpatch <https://lkml.kernel.org/lkml/20200822164233.71583-1-paul@crapouillou.net/T/>. I drag part of it to here for >> convenient to reading: >> >> /Traditionally, GEM buffers are mapped write-combine. Writes to the buffer >> are accelerated, and reads are slow. Application doing lots////of >> alpha-blending paint inside shadow buffers, which is then memcpy'd////into >> the final GEM buffer./// >> "non coherent mapping" is actually cached and it is for CMA helpers >> base driver, not for VRAM helper based driver. For Loongson CPU/SoCs. >> The cache coherency is maintained by hardware, therefore there no >> need to worry about coherency problems. This is true at least for >> ls3a3000, ls3a4000 and ls3a5000. >> >> "non coherent" or "coherent" is not important here, the key point is >> that the backing memory of the framebuffer is cached with non coherent >> mapping, you don't need a shadow buffer layer when using X server's >> modesetting driver. >> >> Read and write to the framebuffer in system memory is much faster than >> read and write to the framebuffer in the VRAM. >> >> Why CMA helper based solution is faster than the VRAM based solution on Mips platform? >> >> Partly because of the CPU have L1, L2 and L3 cache, especially L3 cache >> is as large as 8MB, read and write from the cache is fast. >> >> Another reason is as Paul Cercueil said, read from VRAM with write-combine >> cache mode is slow. it is just uncache read. >> Please note that we don't have a GPU here, we are just a display controller. >> >> For the VRAM helper based driver case, the backing memory of the framebuffer >> is located at VRAM, When using X server's modesetting driver, we have to enable >> the ShadowFB option, Uncache acceleration support(at the kernel size) should >> also be enabled. Otherwise the performance of graphic application is just slow. >> >> Beside write-combine cache mode have bugs on our platform, a kernel side >> developer have disabled it. Write-combine cache mode just boil down to uncached >> now. See [1] and [2] >> >> [1]https://lkml.org/lkml/2020/8/10/255 >> [2]https://lkml.kernel.org/lkml/1617701112-14007-1-git-send-email-yangtiezhu@loongson.cn/T/ >> >> This is the reason why we prefer CMA helper base solution with non coherent mapping, >> simply because it is fast. >> >> As far as I know, Loongson's CPU does not has the concept of write-combine, >> it only support three caching mode: uncached, cached and uncache acceleration. >> write-combine is implemented with uncache acceleration on Mips. > My point wasn't just about the VRAM vs CMA stuff, it was about why do > you need all those switches in the first place? > > Take the verbose parameter for example: it's entirely redundant with the > already existing, documented, DRM logging infrastructure. Yes, verbose is redundant, we will use drm_dbg() instead of verbose. thanks. I am correcting. > Then, you have "modeset", and I'm not sure why it's supposed to be > there, at all. This is a modesetting driver, why would I want to disable > modesetting entirely? Something you want fbdev driver, for example simple-framebuffer driver which using the firmware passed fb (screeninfo). besides, text mode support. > More fundamentally (and this extends to the CMA, caching and VRAM stuff > you explained above), why can't the driver pick the right decision all > the time and why would that be under the user control? The right decision for ls7a1000 is to use VRAM based helper, But sometimes we need CMA helper based solution. Because: The PRIME support is lost, use lsdc with etnaviv is not possible any more. Buffer sharing with etnaviv is no longer possible, loongson display controllers are simple which require scanout buffers to be physically contiguous. We still need to develop userspace driver(say xf86-video-loongson) on ls3a4000 + ls7a1000 platform then deploy those driver to ls2k1000 platform. ls3a4000 and ls3a5000 is fast enough which can build the linux kernel directly. Build mesa, libdrm, xf86-video-loongson just a piece of cake. Is is so fast on ls3a5000+ls7a1000, developing driver on ls2k1000 is cumbersome, embarrassing slow. I means it(ls3a4000/ls3a5000 + ls7a1000) is not just target for end user, but it is a platform which can be used to develop software for other platform. The author of linux device driver told us that device driver is providing mechanism, not policy. We are able to make the decision, but we want to give the user more choice. "pick the right decision all the time" is also true, i am considering correct this, thanks. > You were mentioning that you need to work-around MIPS memory management. > Then fine, just do that on MIPS, and don't it on the other architectures > that don't need it. There's no need for a knob. > > Maxime
On Wed, Feb 09, 2022 at 10:38:41PM +0800, Sui Jingfeng wrote: > On 2022/2/9 16:49, Maxime Ripard wrote: > > On Fri, Feb 04, 2022 at 12:04:19AM +0800, Sui Jingfeng wrote: > > > > > +/* Get the simple EDID data from the device tree > > > > > + * the length must be EDID_LENGTH, since it is simple. > > > > > + * > > > > > + * @np: device node contain edid data > > > > > + * @edid_data: where the edid data to store to > > > > > + */ > > > > > +static bool lsdc_get_edid_from_dtb(struct device_node *np, > > > > > + unsigned char *edid_data) > > > > > +{ > > > > > + int length; > > > > > + const void *prop; > > > > > + > > > > > + if (np == NULL) > > > > > + return false; > > > > > + > > > > > + prop = of_get_property(np, "edid", &length); > > > > > + if (prop && (length == EDID_LENGTH)) { > > > > > + memcpy(edid_data, prop, EDID_LENGTH); > > > > > + return true; > > > > > + } > > > > > + > > > > > + return false; > > > > > +} > > > > You don't have a device tree binding for that driver, this is something > > > > that is required. And it's not clear to me why you'd want EDID in the > > > > DTB? > > > 1) It is left to the end user of this driver. > > > > > > The downstream motherboard maker may use a dpi(XRGB888) or LVDS panel > > > which don't have DDC support either, doing this way allow them put a > > > EDID property into the dc device node in the DTS. Then the entire system works. > > > Note those panel usually support only one display mode. > > I guess it depends on what we mean exactly by the user, but the DTB > > usually isn't under the (end) user control. And the drm.edid_firmware is > > here already to address exactly this issue. > > > > On the other end, if the board has a static panel without any DDC lines, > > then just put the timings in the device tree, there's no need for an > > EDID blob. > > Loongson have a long history of using PMON firmware, The PMON firmware > support flush the dtb into the the firmware before grub loading the kernel. > You press 'c' key, then the PMON will give you a shell. it is much like a > UEFI shell. Suppose foo.dtb is what you want to pass to the vmlinuz. > Then type the follow single command can flush the dtb into the PMON firmware. > > |load_dtb /dev/fs/fat@usb0/foo.dtb| > > For our PMON firmware, it**is** totally under developer/pc board maker's control. > You can flush whatever dtb every time you bootup until you satisfied. > It(the pmon firmware) is designed to let downstream motherboard maker and/or > customers to play easily. > > Support of reading EDID from the dtb is really a feature which downstream > motherboard maker or customer wanted. They sometimes using eDP also whose > resolution is not 1024x768. This is out of control for a graphic driver > developer like me. And, to reinstate, we already have a mechanism to set an EDID, and if it wasn't an option, the DT is not the place to store an EDID blob. > And drm.edid_firmware have only a few limited resolution which is weak. You're wrong. There's no limitation, it's just as limited as your solution. You put the same thing, you get the same thing out of it. The only difference is where the data are coming from. > I will consider to adding drm.edid_firmware support, thanks. It just works if you use drm_get_edid. > > > 2) That is for the display controller in ls2k1000 SoC. > > > > > > Currently, the upstream kernel still don't have GPIO, PWM and I2C driver support > > > for LS2K1000 SoC. > > > > > > How dose you read EDID from the monitor without a I2C driver? > > > > > > without reading EDID the device tree support, the screen just black, > > > the lsdc driver just stall. With reading EDID from device tree support > > > we do not need a i2c driver to light up the monitor. > > > > > > This make lsdc drm driver work on various ls2k1000 development board > > > before I2C driver and GPIO driver and PWM backlight driver is upstream. > > > > > > I have many local private dts with the bindings, those local change just can not > > > upstream at this time, below is an example. > > > > > > The device tree is a platform description language. It's there to let > > > the OS know what the hardware is, but the state of hardware support in > > > the said OS isn't a parameter we have to take into account for a new > > > binding. > > > > > > If you don't have any DDC support at the moment, use the firmware > > > mechanism above, or add fixed modes using drm_add_modes_noedid in the > > > driver, and leave the DT out of it. Once you'll gain support for the > > > EDID readout in the driver, then it'll just work and you won't need to > > > change the DT again. > > > > The resolution will be 1024x768, it will also add a lot modes which may > not supported by the specific panel. Take 1024x600 as an example, > Both drm_add_modes_noedid() and firmware mechanism above will fail. > > Because the user supply EDID only and manufacturer of some strange panel > supply EDID only. It's fairly easy to address: if the panel has some EDID, make the driver able to read it; if it doesn't, describe the mode in the DT. And if you want to be nice to your users, the firmware can even patch the DT at boot time to add the necessary bits based on whatever info it has, it doesn't have to be static. Maxime
On Wed, Feb 09, 2022 at 11:41:06PM +0800, Sui Jingfeng wrote: > > Then, you have "modeset", and I'm not sure why it's supposed to be > > there, at all. This is a modesetting driver, why would I want to disable > > modesetting entirely? > > Something you want fbdev driver, for example simple-framebuffer driver which > using the firmware passed fb (screeninfo). > > besides, text mode support. Then you want to use the generic nomodeset argument. Maxime
On 2022/2/9 20:00, Jiaxun Yang wrote: > Hi Jingfeng, > > Could you please keep me CCed for the for the whole thread when you > respin > the patch? Cuz I'm maintain MIPS/LOONGSON64 stuff and I believe this > series > is partially based on my work at Lemote. > > I will help with reviewing and explain some Loongson64 specified issue > if possible. > > Thanks. > - Jiaxun Okay, thank you.
On 2022/2/10 00:16, Maxime Ripard wrote: > On Wed, Feb 09, 2022 at 10:38:41PM +0800, Sui Jingfeng wrote: >> On 2022/2/9 16:49, Maxime Ripard wrote: >>> On Fri, Feb 04, 2022 at 12:04:19AM +0800, Sui Jingfeng wrote: >>>>>> +/* Get the simple EDID data from the device tree >>>>>> + * the length must be EDID_LENGTH, since it is simple. >>>>>> + * >>>>>> + * @np: device node contain edid data >>>>>> + * @edid_data: where the edid data to store to >>>>>> + */ >>>>>> +static bool lsdc_get_edid_from_dtb(struct device_node *np, >>>>>> + unsigned char *edid_data) >>>>>> +{ >>>>>> + int length; >>>>>> + const void *prop; >>>>>> + >>>>>> + if (np == NULL) >>>>>> + return false; >>>>>> + >>>>>> + prop = of_get_property(np, "edid", &length); >>>>>> + if (prop && (length == EDID_LENGTH)) { >>>>>> + memcpy(edid_data, prop, EDID_LENGTH); >>>>>> + return true; >>>>>> + } >>>>>> + >>>>>> + return false; >>>>>> +} >>>>> You don't have a device tree binding for that driver, this is something >>>>> that is required. And it's not clear to me why you'd want EDID in the >>>>> DTB? >>>> 1) It is left to the end user of this driver. >>>> >>>> The downstream motherboard maker may use a dpi(XRGB888) or LVDS panel >>>> which don't have DDC support either, doing this way allow them put a >>>> EDID property into the dc device node in the DTS. Then the entire system works. >>>> Note those panel usually support only one display mode. >>> I guess it depends on what we mean exactly by the user, but the DTB >>> usually isn't under the (end) user control. And the drm.edid_firmware is >>> here already to address exactly this issue. >>> >>> On the other end, if the board has a static panel without any DDC lines, >>> then just put the timings in the device tree, there's no need for an >>> EDID blob. >> Loongson have a long history of using PMON firmware, The PMON firmware >> support flush the dtb into the the firmware before grub loading the kernel. >> You press 'c' key, then the PMON will give you a shell. it is much like a >> UEFI shell. Suppose foo.dtb is what you want to pass to the vmlinuz. >> Then type the follow single command can flush the dtb into the PMON firmware. >> >> |load_dtb /dev/fs/fat@usb0/foo.dtb| >> >> For our PMON firmware, it**is** totally under developer/pc board maker's control. >> You can flush whatever dtb every time you bootup until you satisfied. >> It(the pmon firmware) is designed to let downstream motherboard maker and/or >> customers to play easily. >> >> Support of reading EDID from the dtb is really a feature which downstream >> motherboard maker or customer wanted. They sometimes using eDP also whose >> resolution is not 1024x768. This is out of control for a graphic driver >> developer like me. > And, to reinstate, we already have a mechanism to set an EDID, and if it > wasn't an option, the DT is not the place to store an EDID blob. I know, put edid blob in the dts maybe abuse, but i am not push dts with edid blob either. It is left to other people, and the ./arch/powerpc/boot/dts/ac14xx.dts already have edid blob. >> And drm.edid_firmware have only a few limited resolution which is weak. > You're wrong. There's no limitation, it's just as limited as your > solution. You put the same thing, you get the same thing out of it. The > only difference is where the data are coming from. It is extremely difficult to use, it have difficulty to specify which firmware edid is for which connector. because we have a 1024x600 panel and a 1920x1080 monitor. It require you to know the connector's name at first, it is not as intuitive as my method. I am exhausted by it. >> I will consider to adding drm.edid_firmware support, thanks. > It just works if you use drm_get_edid. > >>>> 2) That is for the display controller in ls2k1000 SoC. >>>> >>>> Currently, the upstream kernel still don't have GPIO, PWM and I2C driver support >>>> for LS2K1000 SoC. >>>> >>>> How dose you read EDID from the monitor without a I2C driver? >>>> >>>> without reading EDID the device tree support, the screen just black, >>>> the lsdc driver just stall. With reading EDID from device tree support >>>> we do not need a i2c driver to light up the monitor. >>>> >>>> This make lsdc drm driver work on various ls2k1000 development board >>>> before I2C driver and GPIO driver and PWM backlight driver is upstream. >>>> >>>> I have many local private dts with the bindings, those local change just can not >>>> upstream at this time, below is an example. >>>> >>>> The device tree is a platform description language. It's there to let >>>> the OS know what the hardware is, but the state of hardware support in >>>> the said OS isn't a parameter we have to take into account for a new >>>> binding. >>>> >>>> If you don't have any DDC support at the moment, use the firmware >>>> mechanism above, or add fixed modes using drm_add_modes_noedid in the >>>> driver, and leave the DT out of it. Once you'll gain support for the >>>> EDID readout in the driver, then it'll just work and you won't need to >>>> change the DT again. >>>> >> The resolution will be 1024x768, it will also add a lot modes which may >> not supported by the specific panel. Take 1024x600 as an example, >> Both drm_add_modes_noedid() and firmware mechanism above will fail. >> >> Because the user supply EDID only and manufacturer of some strange panel >> supply EDID only. > It's fairly easy to address: if the panel has some EDID, make the driver > able to read it; if it doesn't, describe the mode in the DT. > > And if you want to be nice to your users, the firmware can even patch > the DT at boot time to add the necessary bits based on whatever info it > has, it doesn't have to be static. > > Maxime
On Wed, Feb 9, 2022 at 11:16 AM Maxime Ripard <maxime@cerno.tech> wrote: > > On Wed, Feb 09, 2022 at 10:38:41PM +0800, Sui Jingfeng wrote: > > On 2022/2/9 16:49, Maxime Ripard wrote: > > > On Fri, Feb 04, 2022 at 12:04:19AM +0800, Sui Jingfeng wrote: > > > > > > +/* Get the simple EDID data from the device tree > > > > > > + * the length must be EDID_LENGTH, since it is simple. > > > > > > + * > > > > > > + * @np: device node contain edid data > > > > > > + * @edid_data: where the edid data to store to > > > > > > + */ > > > > > > +static bool lsdc_get_edid_from_dtb(struct device_node *np, > > > > > > + unsigned char *edid_data) > > > > > > +{ > > > > > > + int length; > > > > > > + const void *prop; > > > > > > + > > > > > > + if (np == NULL) > > > > > > + return false; > > > > > > + > > > > > > + prop = of_get_property(np, "edid", &length); > > > > > > + if (prop && (length == EDID_LENGTH)) { > > > > > > + memcpy(edid_data, prop, EDID_LENGTH); > > > > > > + return true; > > > > > > + } > > > > > > + > > > > > > + return false; > > > > > > +} > > > > > You don't have a device tree binding for that driver, this is something > > > > > that is required. And it's not clear to me why you'd want EDID in the > > > > > DTB? > > > > 1) It is left to the end user of this driver. > > > > > > > > The downstream motherboard maker may use a dpi(XRGB888) or LVDS panel > > > > which don't have DDC support either, doing this way allow them put a > > > > EDID property into the dc device node in the DTS. Then the entire system works. > > > > Note those panel usually support only one display mode. > > > I guess it depends on what we mean exactly by the user, but the DTB > > > usually isn't under the (end) user control. And the drm.edid_firmware is > > > here already to address exactly this issue. > > > > > > On the other end, if the board has a static panel without any DDC lines, > > > then just put the timings in the device tree, there's no need for an > > > EDID blob. > > > > Loongson have a long history of using PMON firmware, The PMON firmware > > support flush the dtb into the the firmware before grub loading the kernel. > > You press 'c' key, then the PMON will give you a shell. it is much like a > > UEFI shell. Suppose foo.dtb is what you want to pass to the vmlinuz. > > Then type the follow single command can flush the dtb into the PMON firmware. > > > > |load_dtb /dev/fs/fat@usb0/foo.dtb| > > > > For our PMON firmware, it**is** totally under developer/pc board maker's control. > > You can flush whatever dtb every time you bootup until you satisfied. > > It(the pmon firmware) is designed to let downstream motherboard maker and/or > > customers to play easily. > > > > Support of reading EDID from the dtb is really a feature which downstream > > motherboard maker or customer wanted. They sometimes using eDP also whose > > resolution is not 1024x768. This is out of control for a graphic driver > > developer like me. > > And, to reinstate, we already have a mechanism to set an EDID, and if it > wasn't an option, the DT is not the place to store an EDID blob. Note that it's pretty common on laptop GPUs to have the attached panel's EDID be baked into the VBIOS or ACPI (for LVDS / eDP). The hw drivers in question (e.g. nouveau, radeon, probably i915) know how to retrieve it specially. I'm no DT expert, but I'd imagine that it's meant to allow mirroring those types of configurations. Stuff like "drm.edid_firmware" isn't really meant for system-configuration-level things, more as an out in case something doesn't work as it should. Cheers, -ilia
On 2022/2/10 00:16, Maxime Ripard wrote: > And, to reinstate, we already have a mechanism to set an EDID, and if it > wasn't an option, the DT is not the place to store an EDID blob. Hi, if DT is not the place to store EDID blob, why nvidia can do that ? output->edid = of_get_property(output->of_node, "nvidia,edid", &size); [1][2] [1] ./drivers/gpu/drm/tegra/output.c [2] https://docs.nvidia.com/drive/active/5.0.10.3L/nvvib_docs/index.html#page/NVIDIA%20DRIVE%20Linux%20SDK%20Development%20Guide/Appendix/AppendixDeviceTree.html
On Wed, 9 Feb 2022 at 15:41, Sui Jingfeng <15330273260@189.cn> wrote: > On 2022/2/9 16:43, Maxime Ripard wrote: > > More fundamentally (and this extends to the CMA, caching and VRAM stuff > > you explained above), why can't the driver pick the right decision all > > the time and why would that be under the user control? > > The right decision for ls7a1000 is to use VRAM based helper, But sometimes > we need CMA helper based solution. Because: The PRIME support is lost, use > lsdc with etnaviv is not possible any more. > > Buffer sharing with etnaviv is no longer possible, loongson display controllers > are simple which require scanout buffers to be physically contiguous. Other systems have this limitation, and Mesa's 'kmsro' concept makes this work transparently, as long as your driver can export dmabufs when running in 'VRAM' mode. Cheers, Daniel
On Sun, Feb 13, 2022 at 02:11:30AM +0800, Sui Jingfeng wrote: > > On 2022/2/10 00:16, Maxime Ripard wrote: > > On Wed, Feb 09, 2022 at 10:38:41PM +0800, Sui Jingfeng wrote: > > > On 2022/2/9 16:49, Maxime Ripard wrote: > > > > On Fri, Feb 04, 2022 at 12:04:19AM +0800, Sui Jingfeng wrote: > > > > > > > +/* Get the simple EDID data from the device tree > > > > > > > + * the length must be EDID_LENGTH, since it is simple. > > > > > > > + * > > > > > > > + * @np: device node contain edid data > > > > > > > + * @edid_data: where the edid data to store to > > > > > > > + */ > > > > > > > +static bool lsdc_get_edid_from_dtb(struct device_node *np, > > > > > > > + unsigned char *edid_data) > > > > > > > +{ > > > > > > > + int length; > > > > > > > + const void *prop; > > > > > > > + > > > > > > > + if (np == NULL) > > > > > > > + return false; > > > > > > > + > > > > > > > + prop = of_get_property(np, "edid", &length); > > > > > > > + if (prop && (length == EDID_LENGTH)) { > > > > > > > + memcpy(edid_data, prop, EDID_LENGTH); > > > > > > > + return true; > > > > > > > + } > > > > > > > + > > > > > > > + return false; > > > > > > > +} > > > > > > You don't have a device tree binding for that driver, this is something > > > > > > that is required. And it's not clear to me why you'd want EDID in the > > > > > > DTB? > > > > > 1) It is left to the end user of this driver. > > > > > > > > > > The downstream motherboard maker may use a dpi(XRGB888) or LVDS panel > > > > > which don't have DDC support either, doing this way allow them put a > > > > > EDID property into the dc device node in the DTS. Then the entire system works. > > > > > Note those panel usually support only one display mode. > > > > I guess it depends on what we mean exactly by the user, but the DTB > > > > usually isn't under the (end) user control. And the drm.edid_firmware is > > > > here already to address exactly this issue. > > > > > > > > On the other end, if the board has a static panel without any DDC lines, > > > > then just put the timings in the device tree, there's no need for an > > > > EDID blob. > > > Loongson have a long history of using PMON firmware, The PMON firmware > > > support flush the dtb into the the firmware before grub loading the kernel. > > > You press 'c' key, then the PMON will give you a shell. it is much like a > > > UEFI shell. Suppose foo.dtb is what you want to pass to the vmlinuz. > > > Then type the follow single command can flush the dtb into the PMON firmware. > > > > > > |load_dtb /dev/fs/fat@usb0/foo.dtb| > > > > > > For our PMON firmware, it**is** totally under developer/pc board maker's control. > > > You can flush whatever dtb every time you bootup until you satisfied. > > > It(the pmon firmware) is designed to let downstream motherboard maker and/or > > > customers to play easily. > > > > > > Support of reading EDID from the dtb is really a feature which downstream > > > motherboard maker or customer wanted. They sometimes using eDP also whose > > > resolution is not 1024x768. This is out of control for a graphic driver > > > developer like me. > > And, to reinstate, we already have a mechanism to set an EDID, and if it > > wasn't an option, the DT is not the place to store an EDID blob. > > I know, put edid blob in the dts maybe abuse, but i am not push dts > with edid blob either. > > It is left to other people, and the > ./arch/powerpc/boot/dts/ac14xx.dts already have edid blob. There's one example across the entire tree, and that's not either documented or used by any driver. I'm not sure it was really the point you were trying to make, but the only thing it proves from my point of view is that you don't need it. > > > And drm.edid_firmware have only a few limited resolution which is weak. > > You're wrong. There's no limitation, it's just as limited as your > > solution. You put the same thing, you get the same thing out of it. The > > only difference is where the data are coming from. > > It is extremely difficult to use, it have difficulty to specify which > firmware edid is for which connector. because we have a 1024x600 panel > and a 1920x1080 monitor. > > It require you to know the connector's name at first, it is not as > intuitive as my method. I am exhausted by it. Then you always have the option to implement DDC support, or get your firmware to patch the DT at boot time with the proper display timing node. Even more so if you have a single timing to provide. Maxime
On Wed, Feb 16, 2022 at 09:34:47PM +0800, Sui Jingfeng wrote: > On 2022/2/10 00:16, Maxime Ripard wrote: > > And, to reinstate, we already have a mechanism to set an EDID, and if it > > wasn't an option, the DT is not the place to store an EDID blob. > > Hi, > > > if DT is not the place to store EDID blob, why nvidia can do that ? > > output->edid = of_get_property(output->of_node, "nvidia,edid", &size); > [1][2] Because that binding is 10 years old and never got reviewed by a DT maintainer. Things change, some things we did in the past have been mistakes that we don't want to repeat, can we move forward? Maxime
On 2022/2/16 21:46, Daniel Stone wrote: > On Wed, 9 Feb 2022 at 15:41, Sui Jingfeng <15330273260@189.cn> wrote: >> On 2022/2/9 16:43, Maxime Ripard wrote: >>> More fundamentally (and this extends to the CMA, caching and VRAM stuff >>> you explained above), why can't the driver pick the right decision all >>> the time and why would that be under the user control? >> The right decision for ls7a1000 is to use VRAM based helper, But sometimes >> we need CMA helper based solution. Because: The PRIME support is lost, use >> lsdc with etnaviv is not possible any more. >> >> Buffer sharing with etnaviv is no longer possible, loongson display controllers >> are simple which require scanout buffers to be physically contiguous. > Other systems have this limitation, and Mesa's 'kmsro' concept makes > this work transparently, as long as your driver can export dmabufs > when running in 'VRAM' mode. > > Cheers, > Daniel When using vram helper based driver, the framebuffer is locate at video ram. the backing memory fb is manage by TTM. while bo of etnaviv is locate at system ram. Currently i can't figure out how does the buffer going to be shared.
On Wed, 16 Feb 2022 at 14:13, Sui Jingfeng <15330273260@189.cn> wrote: > On 2022/2/16 21:46, Daniel Stone wrote: > > Other systems have this limitation, and Mesa's 'kmsro' concept makes > > this work transparently, as long as your driver can export dmabufs > > when running in 'VRAM' mode. > > When using vram helper based driver, the framebuffer is locate at video > ram. the backing memory fb is manage by TTM. > > while bo of etnaviv is locate at system ram. Currently i can't figure > out how does the buffer going to be shared. kmsro will allocate from the KMS device (usually using dumb buffers), export that BO as a dmabuf, then import into etnaviv. etnaviv already uses this for imx-drm. Cheers, Daniel
On 2022/2/9 16:36, Maxime Ripard wrote: > On Fri, Feb 04, 2022 at 12:41:37AM +0800, Sui Jingfeng wrote: >>>> +static int lsdc_primary_plane_atomic_check(struct drm_plane *plane, >>>> + struct drm_atomic_state *state) >>>> +{ >>>> + struct drm_device *ddev = plane->dev; >>>> + struct lsdc_device *ldev = to_lsdc(ddev); >>>> + struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane); >>>> + struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane); >>>> + struct drm_framebuffer *new_fb = new_plane_state->fb; >>>> + struct drm_framebuffer *old_fb = old_plane_state->fb; >>>> + struct drm_crtc *crtc = new_plane_state->crtc; >>>> + u32 new_format = new_fb->format->format; >>>> + struct drm_crtc_state *new_crtc_state; >>>> + struct lsdc_crtc_state *priv_crtc_state; >>>> + int ret; >>>> + >>>> + if (!crtc) >>>> + return 0; >>>> + >>>> + new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc); >>>> + if (WARN_ON(!new_crtc_state)) >>>> + return -EINVAL; >>>> + >>>> + priv_crtc_state = to_lsdc_crtc_state(new_crtc_state); >>>> + >>>> + ret = drm_atomic_helper_check_plane_state(new_plane_state, >>>> + new_crtc_state, >>>> + DRM_PLANE_HELPER_NO_SCALING, >>>> + DRM_PLANE_HELPER_NO_SCALING, >>>> + false, >>>> + true); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + /* >>>> + * Require full modeset if enabling or disabling a plane, >>>> + * or changing its position, size, depth or format. >>>> + */ >>>> + if ((!new_fb || !old_fb || >>>> + old_plane_state->crtc_x != new_plane_state->crtc_x || >>>> + old_plane_state->crtc_y != new_plane_state->crtc_y || >>>> + old_plane_state->crtc_w != new_plane_state->crtc_w || >>>> + old_plane_state->crtc_h != new_plane_state->crtc_h || >>>> + old_fb->format->format != new_format)) >>>> + new_crtc_state->mode_changed = true; >>>> + >>>> + >>>> + priv_crtc_state->pix_fmt = lsdc_primary_get_default_format(crtc); >>> Storing the pixel format in the CRTC state is weird? What would happen >>> if you have a primary plane and a cursor in different formats? >>> >>> Also, reading the default format from a register doesn't look right. >>> atomic_check can occur at any time, including before a previous commit, >>> or while the hardware is disabled. You should rely on either a constant >>> or the previous state here. >>> >> Currently, private CRTC state(priv_crtc_state) is not get used by the cursor's >> atomic_check() and atomic_update(). I means this is only for the primary plane. >> And both and the primary and the cursor using XRGB8888 format, what I really >> want here is let the atomic_update to update the framebuffer's format, because >> the firmware (pmon) of some board set the framebuffer's format as RGB565. > atomic_update will be called each time the plane state is changed, so it > won't be an issue: when the first state will be committed, your > atomic_update function will be called and thus you'll overwrite what was > left of the firmware setup. I am agree with you that the format is a property of the plane, the code using wrong abstraction. this is because i am confused by our DC hardware at that time. atomic_update is failed to update the FB format because our CRTC hardware has bug. It can only success update fb format with CRTC reset bit set in drm_crtc_funcs.reset(). Otherwise the CRTC still using RGB565 format to parse the RGBX8888 frame buffer. We finally found the hardware bug via debugfs, just dump the register value. we found that the fb format didn't get update. The hardware bug leading us using the wrong abstraction. we do not understand the DRM core fully at that time. We have revised our patch, thank you point out that. >> If the hardware's format is same with the plane state, then there is no need to >> update the FB's format register, save a function call, right? > My point was more about the fact that you're using the wrong abstraction > there. The format is a property of the plane, not from the CRTC. In KMS > (and in most drivers), you can have multiple planes with different > formats all attached to the same CRTC just fine. > > Maxime Our CRTC have cursor plane and primary plane, composite is doing by the CRTC hardware automatically. there no depth property, the cursor is always on the top of the CRTC. If the CRTC format is not same format I think we should convert the format of cursor to the format of primary plane at atomic_update, by introduce a shadow plane. Thanks for your guiding.
On 2022/2/9 22:04, Maxime Ripard wrote: > On Wed, Feb 09, 2022 at 11:56:48AM +0000, Jiaxun Yang wrote: >> >> 在 2022/2/9 8:52, Maxime Ripard 写道: >>> On Thu, Feb 03, 2022 at 11:47:16PM +0800, Sui Jingfeng wrote: >> [...] >>> DT isn't really a solution either. Let's take the distribution >>> perspective there. Suppose you're a Fedora or Debian developer, and want >>> to make a single kernel image, and ship a DT to the user for their board >>> without any modification. How is either the Kconfig solution or DT flags >>> solution any good there? It doesn't help them at all. >> We are working in another way. As we can tell model of the board by strings >> passed from the firmware, we just built-in all poosible DTs into the kernel >> and then tell which DT to load at boot time. So we can ensure users has >> smmoth experience. > It's not really for you to say though. Once your driver is in a release, > distros are going to use it. That's the whole point of you asking us to > merge it. > > Maxime Hi, Dear Maxime There are still some LS3a5000+ls7a2000 and LS3a5000+ls7a1000 boards at china market. ls7a2000 has loongson display controller + loongson self-made GPU integrated. Without a display driver, integrated graphics of the board can not be used with upstream kernel. A few ordinary player and user space developer blaming me, they need a basic usable graphic environment badly. Do you have spare time to review my patch again, The DC is lack of document even though in Chinese because the hardware engineer don't supply one. But I will provide more information required during code review as far as i can tell. https://patchwork.kernel.org/project/dri-devel/patch/20230201170403.167665-1-15330273260@189.cn/ Thanks for your kindness and your valuable time .
There is a display controller in loongson's LS2K1000 SoC and LS7A1000 bridge, and the DC in those chip is a PCI device. This patch provide a minimal support for this display controller which is mainly for graphic environment bring up. This display controller has two display pipes but with only one hardware cursor. Each way has a DVO output interface and the CRTC is able to scanout from 1920x1080 resolution at 60Hz. The maxmium resolution is 2048x2048@60hz. LS2K1000 is a SoC, only system memory is available. Therefore scanout from system memory is the only choice. We prefer the CMA helper base solution because the gc1000 gpu can use etnaviv driver, in this case etnaviv and lsdc could made a compatible pair. Even through it is possible to use VRAM helper base solution on ls2k1000 by carving out part of system memory as VRAM. For LS7A1000, there are 4 gpios whos control register is located at the dc register space which is not the geneal purpose GPIO. The 4 gpios can emulate two way i2c. One for DVO0, another for DVO1. This is the reason the why we are not using the drm bridge framework. LS2K1000 and LS2K0500 SoC don't have such hardware, they use general purpose GPIO emulated i2c or hardware i2c adapter from other module to serve this purpose. Drm bridge and drm panel is suitable for those SoC, we have already implement it on our own downstream kernel. But due to the upstream kernel don't have gpio, pwm and i2c driver support for LS2K1000. We just can not upstream our support for the drm bridge subsystem. The DC in LS7A1000 can scanout from both the system memory and the dedicate VRAM attached to the ddr3 memory controller. Sadly, only scanout from the VRAM is proved to be a reliable solution for massive product. Scanout from the system memory suffer from some hardware deficiency which cause the screen flickering under RAM pressure. This is the reason why we integrate two distict helpers into one piece of device driver. But CMA base helper is still usable on ls7a1000 for normal usage, expecially on ls7a1000+ bridge chip. We have also implemented demage update on top of CMA helper which copy the demaged shadow framebuffer region from system RAM to the real framebuffer in VRAM manually. Using "lsdc.dirty_update=1" in the commmand line will enable this driver mode. LS7A1000 have a 32x32 harware cursor, we just let the two CRTC share it simply with the help of universe plane. LS7A2000 have two 64x64 harware cursor. Surport for LS7A2000 is on the way. In short, we have built-in gpio emulated i2c support, we also have hardware cursor support. The kind of tiny drivers in drm/tiny is not suitable for us, we are not "tiny". +------+ HyperTransport 3.0 | DDR4 | | +------+ | +------------------------------------+ || MC0 | | LS7A1000 +------------| +----------+ | | | DDR3 | +------+ | LS3A4000 |<--------->| +--------+ +-------+ | memory |<->| VRAM | | CPU |<--------->| | GC1000 | | LSDC | | controller | +------+ +----------+ | +--------+ +-+---+-+ +------------| || MC1 +---------------|---|----------------+ +------+ | | | DDR4 | +-------+ DVO0 | | DVO1 +------+ +------+ VGA <--|ADV7125|<---------+ +------->|TFP410|--> DVI/HDMI +-------+ +------+ The above picture give a simple usage of LS7A1000, note that the encoder is not necessary adv7125 or tfp410, it is a choice of the downstream board manufacturer. Other candicate encoder can be ch7034b, sil9022 and ite66121 etc. Therefore, we need device tree to provide board specific information. Besides, the dc in both ls2k1000 and ls7k1000 have the vendor:device id of 0x0014:0x7a06, the reverison id is also same. We can't tell it apart simply (this is the firmware engineer's mistake). But firmware already flushed to the board and borad already sold out, we choose to resolve those issues by introduing device tree with board specific device support. For lsdc, there is only a 1:1 mapping of encoders and connectors. +-------------------+ _________ | | | | | CRTC0 --> DVO0 ---------> Encoder0 --> Connector0 ---> | Monitor | | | ^ ^ |_________| | | | | | <----- i2c0 ----------------+ | LSDC IP CORE | | <----- i2c1 ----------------+ | | | | _________ | | | | | | | CRTC1 --> DVO1 ---------> Encoder1 --> Connector1 ---> | Panel | | | |_________| +-------------------+ Below is a brief introduction of loongson's CPU, bridge chip and SoC. LS2K1000 is a double core 1.0Ghz mips64r2 compatible SoC[1]. LS7A1000 is a bridge chip made by Loongson corporation which act as north and/or south bridge of loongson's desktop and server level processor. It is equivalent to AMD RS780E+SB710 or something like that. More details can be read from its user manual[2]. This bridge chip is typically use with LS3A3000, LS3A4000 and LS3A5000 cpu. LS3A3000 is 4 core 1.45gHz mips64r2 compatible cpu. LS3A4000 is 4 core 1.8gHz mips64r5 compatible cpu. LS3A5000 is 4 core 2.5gHz loongarch cpu[3]. Nearly all mordern loongson CPU's cache coherency is maintained by hardware, except for early version of ls2k1000. So we using cached coherent memory by default, not writecombine. v2: fixup warnings reported by kernel test robot v3: fix more grammar mistakes in Kconfig reported by Randy Dunlap and give more details about lsdc. v4: 1) Add dts required and explain why device tree is required. 2) Give more description about lsdc and vram helper base driver. 3) Fix warnings reported by kernel test robot. 4) Introduce stride_alignment member into struct lsdc_chip_desc, the stride alignment is 256 bytes for ls7a1000, ls2k1000 and ls2k0500 SoC. But ls7a2000 improve it to 32 bytes, We are prepare for extend the support for the on coming device. v5: 1) using writel and readl replace writeq and readq, to fix kernel test robot report build error on other archtecture 2) set default fb format to XRGB8888 at crtc reset time. 3) fix typos. v6: 1) Explain why we are not switch to drm dridge subsystem. 2) Explain why tiny drm driver is not suitable for us. 3) Give a short description of the trival dirty uppdate implement based on CMA helper. 4) code clean up. [1] https://wiki.debian.org/InstallingDebianOn/Lemote/Loongson2K1000 [2] https://loongson.github.io/LoongArch-Documentation/Loongson-7A1000-usermanual-EN.html [3] https://loongson.github.io/LoongArch-Documentation/Loongson-3A5000-usermanual-EN.html Reported-by: Randy Dunlap <rdunlap@infradead.org> Reported-by: kernel test robot Signed-off-by: suijingfeng <suijingfeng@loongson.cn> Signed-off-by: Sui Jingfeng <15330273260@189.cn> suijingfeng (3): drm/lsdc: add drm driver for loongson display controller dt-bindings: ls2k1000: add the display controller device node dt-bindings: mips: loongson: introduce board specific dts arch/mips/boot/dts/loongson/lemote_a1901.dts | 64 ++ .../boot/dts/loongson/loongson64-2k1000.dtsi | 11 + .../boot/dts/loongson/ls3a4000_7a1000_evb.dts | 68 ++ arch/mips/boot/dts/loongson/ls7a-pch.dtsi | 2 +- drivers/gpu/drm/Kconfig | 2 + drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/lsdc/Kconfig | 38 + drivers/gpu/drm/lsdc/Makefile | 15 + drivers/gpu/drm/lsdc/lsdc_connector.c | 443 +++++++++ drivers/gpu/drm/lsdc/lsdc_connector.h | 60 ++ drivers/gpu/drm/lsdc/lsdc_crtc.c | 440 +++++++++ drivers/gpu/drm/lsdc/lsdc_drv.c | 846 ++++++++++++++++++ drivers/gpu/drm/lsdc/lsdc_drv.h | 216 +++++ drivers/gpu/drm/lsdc/lsdc_encoder.c | 79 ++ drivers/gpu/drm/lsdc/lsdc_i2c.c | 220 +++++ drivers/gpu/drm/lsdc/lsdc_i2c.h | 61 ++ drivers/gpu/drm/lsdc/lsdc_irq.c | 77 ++ drivers/gpu/drm/lsdc/lsdc_irq.h | 37 + drivers/gpu/drm/lsdc/lsdc_plane.c | 681 ++++++++++++++ drivers/gpu/drm/lsdc/lsdc_pll.c | 657 ++++++++++++++ drivers/gpu/drm/lsdc/lsdc_pll.h | 109 +++ drivers/gpu/drm/lsdc/lsdc_regs.h | 246 +++++ 22 files changed, 4372 insertions(+), 1 deletion(-) create mode 100644 arch/mips/boot/dts/loongson/lemote_a1901.dts create mode 100644 arch/mips/boot/dts/loongson/ls3a4000_7a1000_evb.dts create mode 100644 drivers/gpu/drm/lsdc/Kconfig create mode 100644 drivers/gpu/drm/lsdc/Makefile create mode 100644 drivers/gpu/drm/lsdc/lsdc_connector.c create mode 100644 drivers/gpu/drm/lsdc/lsdc_connector.h create mode 100644 drivers/gpu/drm/lsdc/lsdc_crtc.c create mode 100644 drivers/gpu/drm/lsdc/lsdc_drv.c create mode 100644 drivers/gpu/drm/lsdc/lsdc_drv.h create mode 100644 drivers/gpu/drm/lsdc/lsdc_encoder.c create mode 100644 drivers/gpu/drm/lsdc/lsdc_i2c.c create mode 100644 drivers/gpu/drm/lsdc/lsdc_i2c.h create mode 100644 drivers/gpu/drm/lsdc/lsdc_irq.c create mode 100644 drivers/gpu/drm/lsdc/lsdc_irq.h create mode 100644 drivers/gpu/drm/lsdc/lsdc_plane.c create mode 100644 drivers/gpu/drm/lsdc/lsdc_pll.c create mode 100644 drivers/gpu/drm/lsdc/lsdc_pll.h create mode 100644 drivers/gpu/drm/lsdc/lsdc_regs.h