Re: [PATCH v6 1/3] drm/lsdc: add drm driver for loongson display controller

[Daniel Stone]

On Wed, 16 Feb 2022 at 14:13, Sui Jingfeng <15330273...@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

Re: [PATCH v6 1/3] drm/lsdc: add drm driver for loongson display controller

[Sui Jingfeng]

On 2022/2/16 21:46, Daniel Stone wrote:

On Wed, 9 Feb 2022 at 15:41, Sui Jingfeng <15330273...@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.

Re: [PATCH v6 1/3] drm/lsdc: add drm driver for loongson display controller

[Maxime Ripard]

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


signature.asc
Description: PGP signature

Re: [PATCH v6 1/3] drm/lsdc: add drm driver for loongson display controller

[Maxime Ripard]

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


signature.asc
Description: PGP signature

Re: [PATCH v6 1/3] drm/lsdc: add drm driver for loongson display controller

[Daniel Stone]

On Wed, 9 Feb 2022 at 15:41, Sui Jingfeng <15330273...@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

Re: [PATCH v6 1/3] drm/lsdc: add drm driver for loongson display controller

[Sui Jingfeng]

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

Re: [PATCH v6 1/3] drm/lsdc: add drm driver for loongson display controller

[Ilia Mirkin]

On Wed, Feb 9, 2022 at 11:16 AM 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.

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

Re: [PATCH v6 1/3] drm/lsdc: add drm driver for loongson display controller

[Sui Jingfeng]

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 onl

Re: [PATCH v6 1/3] drm/lsdc: add drm driver for loongson display controller

[Jiaxun Yang]

在 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

Re: [PATCH v6 1/3] drm/lsdc: add drm driver for loongson display controller

[Jiaxun Yang]

在 2022/2/9 14:04, Maxime Ripard 写道:

On Wed, Feb 09, 2022 at 11:56:48AM +, 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

Re: [PATCH v6 1/3] drm/lsdc: add drm driver for loongson display controller

[Maxime Ripard]

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


signature.asc
Description: PGP signature

Re: [PATCH v6 1/3] drm/lsdc: add drm driver for loongson display controller

[Maxime Ripard]

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 su

Re: [PATCH v6 1/3] drm/lsdc: add drm driver for loongson display controller

[Sui Jingfeng]

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  
. 
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 lotsof
alpha-blending paint inside shadow buffers, which is then memcpy'dinto
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-yangtie...@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-lo

Re: [PATCH v6 1/3] drm/lsdc: add drm driver for loongson display controller

[Sui Jingfeng]

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 = <5120>;
             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 

Re: [PATCH v6 1/3] drm/lsdc: add drm driver for loongson display controller

[Maxime Ripard]

On Wed, Feb 09, 2022 at 11:56:48AM +, 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


signature.asc
Description: PGP signature

Re: [PATCH v6 1/3] drm/lsdc: add drm driver for loongson display controller

[Maxime Ripard]

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 ..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


signature.asc
Description: PGP signature

Re: [PATCH v6 1/3] drm/lsdc: add drm driver for loongson display controller

[Maxime Ripard]

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 = <5120>;
>             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


signature.asc
Description: PGP signature

Re: [PATCH v6 1/3] drm/lsdc: add drm driver for loongson display controller

[Maxime Ripard]

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  
> .
>  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 lotsof
> alpha-blending paint inside shadow buffers, which is then memcpy'dinto
> 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-yangtie...@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


signature.asc
Description: PGP signature

Re: [PATCH v6 1/3] drm/lsdc: add drm driver for loongson display controller

[Maxime Ripard]

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  XRGB 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


signature.asc
Description: PGP signature

Re: [PATCH v6 1/3] drm/lsdc: add drm driver for loongson display controller

[Sui Jingfeng]

+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  XRGB 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.

Re: [PATCH v6 1/3] drm/lsdc: add drm driver for loongson display controller

[Sui Jingfeng]

+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 vsy

Re: [PATCH v6 1/3] drm/lsdc: add drm driver for loongson display controller

[Sui Jingfeng]

diff --git a/drivers/gpu/drm/lsdc/Makefile b/drivers/gpu/drm/lsdc/Makefile
new file mode 100644
index ..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 ..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 
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+
+#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)
+ 

Re: [PATCH v6 1/3] drm/lsdc: add drm driver for loongson display controller

[Sui Jingfeng]

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 ..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.

Re: [PATCH v6 1/3] drm/lsdc: add drm driver for loongson display controller

[Sui Jingfeng]

On 2022/2/3 16:58, Maxime Ripard wrote:

Hi,

On Thu, Feb 03, 2022 at 04:25:44PM +0800, Sui Jingfeng wrote:

From: suijingfeng

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 -> E

Re: [PATCH v6 1/3] drm/lsdc: add drm driver for loongson display controller

[Dan Carpenter]

On Thu, Feb 03, 2022 at 12:29:11PM +0100, Krzysztof Kozlowski wrote:
> On Thu, 3 Feb 2022 at 12:08, Dan Carpenter  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

Re: [PATCH v6 1/3] drm/lsdc: add drm driver for loongson display controller

[Krzysztof Kozlowski]

On Thu, 3 Feb 2022 at 12:08, Dan Carpenter  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 ..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

Re: [PATCH v6 1/3] drm/lsdc: add drm driver for loongson display controller

[Dan Carpenter]

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 ..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

Re: [PATCH v6 1/3] drm/lsdc: add drm driver for loongson display controller

[Maxime Ripard]

Hi,

On Thu, Feb 03, 2022 at 04:25:44PM +0800, Sui Jingfeng wrote:
> From: suijingfeng 
> 
> 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 +
>  |   |   || _
>  

Re: [PATCH v6 1/3] drm/lsdc: add drm driver for loongson display controller

[Krzysztof Kozlowski]

On Thu, 3 Feb 2022 at 09:26, Sui Jingfeng <15330273...@189.cn> wrote:
>
> From: suijingfeng 
>
> 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 +
>  |   |   || _
>  |  

[PATCH v6 1/3] drm/lsdc: add drm driver for loongson display controller

[Sui Jingfeng]

From: suijingfeng 

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  |
 |   | |_|
 +-