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

Attachment: signature.asc
Description: PGP signature