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