Message-ID: <8b7bcb57-4450-ed67-bd5f-d8e8f2e74510@189.cn>
Date:   Sun, 13 Feb 2022 02:11:30 +0800
From:   Sui Jingfeng <15330273260@....cn>
To:     Maxime Ripard <maxime@...no.tech>
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 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.

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

Powered by blists - more mailing lists