Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 16 Feb 2022 15:08:31 +0100
From:   Maxime Ripard <maxime@cerno.tech>
To:     Sui Jingfeng <15330273260@189.cn>
Cc:     Dan Carpenter <dan.carpenter@oracle.com>,
        Lucas Stach <l.stach@pengutronix.de>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Roland Scheidegger <sroland@vmware.com>,
        Zack Rusin <zackr@vmware.com>,
        Christian Gmeiner <christian.gmeiner@gmail.com>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        Rob Herring <robh+dt@kernel.org>,
        Thomas Bogendoerfer <tsbogend@alpha.franken.de>,
        Krzysztof Kozlowski <krzk@kernel.org>,
        Andrey Zhizhikin <andrey.zhizhikin@leica-geosystems.com>,
        Sam Ravnborg <sam@ravnborg.org>,
        suijingfeng <suijingfeng@loongson.cn>,
        linux-mips@vger.kernel.org, linux-kernel@vger.kernel.org,
        dri-devel@lists.freedesktop.org,
        Randy Dunlap <rdunlap@infradead.org>
Subject: Re: [PATCH v6 1/3] drm/lsdc: add drm driver for loongson display
 controller
Message-ID: <20220216140831.gnevfwudj7635lj5@houat>
References: <20220203082546.3099-1-15330273260@189.cn>
 <20220203082546.3099-2-15330273260@189.cn>
 <20220203085851.yqstkfgt4dz7rcnw@houat>
 <11ac5696-29e3-fefa-31c0-b7b86c88bbdc@189.cn>
 <20220209084908.kub4bs64rzhvpvon@houat>
 <84bfb2fc-595c-3bae-e8a0-c19ccbcfcfd8@189.cn>
 <20220209161624.42ijbnhanaaari46@houat>
 <8b7bcb57-4450-ed67-bd5f-d8e8f2e74510@189.cn>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="5po5qlzkqgek6g72"
Content-Disposition: inline
In-Reply-To: <8b7bcb57-4450-ed67-bd5f-d8e8f2e74510@189.cn>
Precedence: bulk


--5po5qlzkqgek6g72
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Sun, Feb 13, 2022 at 02:11:30AM +0800, Sui Jingfeng wrote:
>=20
> 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 =3D=3D NULL)
> > > > > > > +		return false;
> > > > > > > +
> > > > > > > +	prop =3D of_get_property(np, "edid", &length);
> > > > > > > +	if (prop && (length =3D=3D 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 s=
omething
> > > > > > 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.
> > > > >=20
> > > > > The downstream motherboard maker may use a dpi(XRGB888) or LVDS p=
anel
> > > > > which don't have DDC support either, doing this way allow them pu=
t 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_firmwa=
re is
> > > > here already to address exactly this issue.
> > > >=20
> > > > On the other end, if the board has a static panel without any DDC l=
ines,
> > > > 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 k=
ernel.
> > > You press 'c' key, then the PMON will give you a shell. it is much li=
ke 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 f=
irmware.
> > >=20
> > > |load_dtb /dev/fs/fat@usb0/foo.dtb|
> > >=20
> > > For our PMON firmware, it**is**  totally under developer/pc board mak=
er'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.
> > >=20
> > > Support of reading EDID from the dtb is really a feature which downst=
ream
> > > motherboard maker or customer wanted. They sometimes using eDP also w=
hose
> > > resolution is not 1024x768. This is out of control for a graphic driv=
er
> > > 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.
>=20
> I know, put edid blob in the dts maybe abuse, but i am not push dts
> with edid blob either.
>=20
> It is left to other people, and the
> ./arch/powerpc/boot/dts/ac14xx.dts=A0already 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 wea=
k.
> > 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.
>=20
> 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.
>=20
> 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

--5po5qlzkqgek6g72
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYKAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCYg0FXwAKCRDj7w1vZxhR
xfmBAQCrXZ3zzbaw3jHLn6vYrzlJ3K3ueudXqrU+n5s+nh1g8wD+M2JQezLoWbiT
5VUHgMDoAk2PZM6gdsOAgZM6nCofhA8=
=yuvU
-----END PGP SIGNATURE-----

--5po5qlzkqgek6g72--