Re: [PATCH v4 5/5] drm/ofdrm: Support big-endian scanout buffers

From: "Arnd Bergmann" <arnd@arndb.de>
To: linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH v4 5/5] drm/ofdrm: Support big-endian scanout buffers
Date: Wed, 12 Oct 2022 08:29:39 +0200	[thread overview]
Message-ID: <87fbe7bd-1160-420e-984b-5afccd5d523c@app.fastmail.com> (raw)
In-Reply-To: <20221011213841.GA28810@kitsune.suse.cz>

On Tue, Oct 11, 2022, at 11:38 PM, Michal Suchánek wrote:
> On Tue, Oct 11, 2022 at 10:06:59PM +0200, Arnd Bergmann wrote:
>> On Tue, Oct 11, 2022, at 1:30 PM, Thomas Zimmermann wrote:
>> > Am 11.10.22 um 09:46 schrieb Javier Martinez Canillas:
>> >>> +static bool display_get_big_endian_of(struct drm_device *dev, struct device_node *of_node)
>> >>> +{
>> >>> +	bool big_endian;
>> >>> +
>> >>> +#ifdef __BIG_ENDIAN
>> >>> +	big_endian = true;
>> >>> +	if (of_get_property(of_node, "little-endian", NULL))
>> >>> +		big_endian = false;
>> >>> +#else
>> >>> +	big_endian = false;
>> >>> +	if (of_get_property(of_node, "big-endian", NULL))
>> >>> +		big_endian = true;
>> >>> +#endif
>> >>> +
>> >>> +	return big_endian;
>> >>> +}
>> >>> +
>> >> 
>> >> Ah, I see. The heuristic then is whether the build is BE or LE or if the Device
>> >> Tree has an explicit node defining the endianess. The patch looks good to me:
>> >
>> > Yes. I took this test from offb.
>> 
>> Has the driver been tested with little-endian kernels though? While
>> ppc32 kernels are always BE, you can build kernels as either big-endian
>> or little-endian for most (modern) powerpc64 and arm/arm64 hardware,
>> and I don't see why that should change the defaults of the driver
>> when describing the same framebuffer hardware.
>
> The original code was added with
> commit 7f29b87a7779 ("powerpc: offb: add support for foreign endianness")
>
> The hardware is either big-endian or runtime-switchable-endian.

Are you referring to CPU hardware or framebuffer hardware here?

> It makes
> sense to assume big-endian when runnig big-endian and the DT does not
> specify endian which is likely on a historical system.

Agreed, assuming big-endian here clearly makes sense.

> It also makes sense to assume that on system with
> runtime-switchable-endian the DT specifies the framebuffer endian.
>
> If systems that only do little-endian exist or emerge later then it also
> makes sense to assume that the framebuffer matches the host if not
> specified.
>
> I don't really see a problem here.
>
> BTW is this used on arm and on what platform?

I'm not aware of any users on Arm, most likely they all use
simplefb/simpledrm or a gpu specific binding. There might be
users on sparc, but they would obviously be big-endian
as well.

> I do not see any bindings in dts.

Right, that is the real problem I see as well. I found the original
CHRP binding document at
https://www.devicetree.org/open-firmware/bindings/devices/html/lfb-1_0d.html

Unfortunately, this only specifies an 8-bit-per-pixel mode, and the
multi-byte pixel support that was added in linux-2.1.125 was
probably powermac specific without a public specification.

I think ideally we should add a binding document that describes what
the driver actually expects, but in this case I would just drop the
#ifdef check and always assume the framebuffer is big-endian unless
the "little-endian" property is set, in order to have a sensible
definition that does not depend on what OS (i.e. Linux
CONFIG_CPU_BIG_ENDIAN) you are running.

       Arnd