Date:   Wed, 16 Feb 2022 15:08:31 +0100
From:   Maxime Ripard <maxime@...no.tech>
To:     Sui Jingfeng <15330273260@....cn>
Cc:     Dan Carpenter <dan.carpenter@...cle.com>,
        Lucas Stach <l.stach@...gutronix.de>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Roland Scheidegger <sroland@...are.com>,
        Zack Rusin <zackr@...are.com>,
        Christian Gmeiner <christian.gmeiner@...il.com>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Rob Herring <robh+dt@...nel.org>,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Andrey Zhizhikin <andrey.zhizhikin@...ca-geosystems.com>,
        Sam Ravnborg <sam@...nborg.org>,
        suijingfeng <suijingfeng@...ngson.cn>,
        linux-mips@...r.kernel.org, linux-kernel@...r.kernel.org,
        dri-devel@...ts.freedesktop.org,
        Randy Dunlap <rdunlap@...radead.org>
Subject: Re: [PATCH v6 1/3] drm/lsdc: add drm driver for loongson display
 controller

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@...0/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

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists