Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.

[Ilia Mirkin]

On Sat, Apr 22, 2017 at 9:48 AM, Ilia Mirkin <imirkin@xxxxxxxxxxxx> wrote:
> On Sat, Apr 22, 2017 at 9:40 AM, Ilia Mirkin <imirkin@xxxxxxxxxxxx> wrote:
>> On Sat, Apr 22, 2017 at 5:50 AM, Ville SyrjÃlÃ
>> <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
>>> On Sat, Apr 22, 2017 at 01:07:57AM -0400, Ilia Mirkin wrote:
>>>> On Fri, Apr 21, 2017 at 12:59 PM, Ville SyrjÃlÃ
>>>> <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
>>>> > On Fri, Apr 21, 2017 at 10:49:49AM -0400, Ilia Mirkin wrote:
>>>> >> On Fri, Apr 21, 2017 at 3:58 AM, Gerd Hoffmann <kraxel@xxxxxxxxxx> wrote:
>>>> >> > While working on graphics support for virtual machines on ppc64 (which
>>>> >> > exists in both little and big endian variants) I've figured the comments
>>>> >> > for various drm fourcc formats in the header file don't match reality.
>>>> >> >
>>>> >> > Comments says the RGB formats are little endian, but in practice they
>>>> >> > are native endian.  Look at the drm_mode_legacy_fb_format() helper.  It
>>>> >> > maps -- for example -- bpp/depth 32/24 to DRM_FORMAT_XRGB8888, no matter
>>>> >> > whenever the machine is little endian or big endian.  The users of this
>>>> >> > function (fbdev emulation, DRM_IOCTL_MODE_ADDFB) expect the framebuffer
>>>> >> > is native endian, not little endian.  Most userspace also operates on
>>>> >> > native endian only.
>>>> >> >
>>>> >> > So, go update the comments for all 16+24+32 bpp RGB formats.
>>>> >> >
>>>> >> > Leaving the yuv formats as-is.  I have no idea if and how those are used
>>>> >> > on bigendian machines.
>>>> >>
>>>> >> I think this is premature. The current situation is that I can't get
>>>> >> modetest to work *at all* on my NV34 / BE setup (I mean, it runs, just
>>>> >> the colors displayed are wrong). I believe that currently it packs
>>>> >> things in "cpu native endian". I've tried futzing with that without
>>>> >> much success, although I didn't spend too much time on it. I have a
>>>> >> NV34 plugged into my LE setup as well although I haven't tested to
>>>> >> double-check that it all works there. However I'm quite sure it used
>>>> >> to, as I used modetest to help develop the YUV overlay support for
>>>> >> those GPUs.
>>>> >
>>>> > I just took a quick stab at fixing modetest to respect the current
>>>> > wording in drm_fourcc.h:
>>>> >
>>>> > git://github.com/vsyrjala/libdrm.git modetest_endian
>>>>
>>>> Looks like there was some careless testing on my part :( So ... it
>>>> looks like the current modetest without those changes does, in fact,
>>>> work on NV34/BE. With the changes, it breaks (and the handling of the
>>>> b* modes is a little broken in those patches -- they're not selectable
>>>> from the cmdline.) Which means that, as Michel & co predicted, it
>>>> appears to be taking BE input not LE input. This is very surprising to
>>>> me, but it is what it is. As I mentioned before, the details of how
>>>> the "BE" mode works on the GPUs is largely unknown to us beyond a few
>>>> basics. Note that only XR24 works, AR24 ends up with all black
>>>> displayed. This also happens on LE.
>>>
>>> Did you try 8bpp or 16bpp formats? I expect that if you've just blindly
>>> enabled some magic byte swapper in the hardware it will only for
>>> a specific pixel size.
>>
>> Thankfully dispnv04 exposes no such madness - just XR24 (and AR24,
>> although that doesn't appear functional). Yes, it's likely that
>> there's a byteswap happening somewhere. In fact the copy engines have
>> parameters somewhere to tell how the swap should be done (basically
>> what the element size is). I don't quite know how to set that though
>> on this generation. I should poke at VRAM via the mmio peephole and
>> see what's actually being stored. Although of course MMIO accesses are
>> also auto-byteswapped. It's all just one big massive headache.
>
> Or it could just be the obvious:
>
> static void nv_crtc_commit(struct drm_crtc *crtc)
> {
>         struct drm_device *dev = crtc->dev;
>         const struct drm_crtc_helper_funcs *funcs = crtc->helper_private;
>         struct nouveau_crtc *nv_crtc = nouveau_crtc(crtc);
>
>         nouveau_hw_load_state(dev, nv_crtc->index,
> &nv04_display(dev)->mode_reg);
>         nv04_crtc_mode_set_base(crtc, crtc->x, crtc->y, NULL);
>
> #ifdef __BIG_ENDIAN
>         /* turn on LFB swapping */
>         {
>                 uint8_t tmp = NVReadVgaCrtc(dev, nv_crtc->index,
> NV_CIO_CRE_RCR);
>                 tmp |= MASK(NV_CIO_CRE_RCR_ENDIAN_BIG);
>                 NVWriteVgaCrtc(dev, nv_crtc->index, NV_CIO_CRE_RCR, tmp);
>         }
> #endif
>
>         funcs->dpms(crtc, DRM_MODE_DPMS_ON);
>         drm_crtc_vblank_on(crtc);
> }
>
> So presumably instead of that __BIG_ENDIAN thing, it should instead be
> if crtc->primary->fb->fourcc & BIG_ENDIAN. (Although that's probably
> going to break the universe.) There's also some questionable support
> for 16-bit modes in the code, I'm going to see how easy it is to flip
> on.

OK, so just to follow up, I'd like to note a few things that were not
obvious to me, but perhaps were to all of you. I'll say them anyways.
As a preface, this is all addressing the pre-nv50 nouveau support.
This support is a port of a port of a port of the (rather old)
xf86-video-nv code, which exists in no less than 3 places in the
kernel, among other things. It largely lives in
drivers/gpu/drm/nouveau/dispnv04. The reason for this is that my G5
PowerMac7,3 only has PCI and AGP slots, and I don't have any nv50+ hw
that'll fit. I'll try to look for some though, I hear it exists (and
according to some very rare reports, also has BE issues).

First of all, while nv50+ nouveau supports all the new formats/etc
hotness, dispnv04 code was using drm_crtc_init. This is a helper that
is used by many drivers and provides XRGB8888 and ARGB8888 formats, no
questions asked. My guess is that this happened *well* after the code
was ported to drm. As-is, these are the only formats exposed to the
world.

The dispnv04 backend logic supports both XRGB8888, RGB565 (not
exposed), and XRGB1555 (not exposed). The ARGB variants don't appear
to work, although I haven't investigated why. It is also configured
to, based on a compile time setting, enable the byteswap logic in the
CRTC unit (or RAMDAC, who knows what the full pipeline is). I tested
this works seemingly correctly for all three formats, by adapting
Ville's updated libdrm branch with modetest. (There may theoretically
be a paletted mode as well, but since it's 8-bit and not properly set
up, figured it wasn't worth trying to work out.)

The drm helper code isn't ready for DRM_FORMAT_BIG_ENDIAN support.
Among other things, drm_format_info() fails to find the format, and
even if it's fixed to ignore that bit, whether the fb is in a pixel
format with that flag defined is lost after the fb is created. fbdev
also creates fb's that expect cpu endianness, as disabling the
byteswap logic caused a green fbcon terminal to show up. (So at least
something somewhere in the fbcon -> nouveau's fbdev emulation pipeline
is expecting cpu endianness. This happens both with nouveau's fbdev
accel logic and without.)

So I think the current situation, at least wrt pre-nv50 nouveau, is
that XRGB/ARGB8888 are "special", since they are the only things
exposed by drm_crtc_init. I believe those definitions should be
updated to note that they're cpu-endian-specific (or another way of
phrasing it more diplomatically is that they're array formats rather
than packed formats). The rest of the formats aren't needed/used by
the legacy world, and can have whatever proper definitions we want,
including better support for the BIG_ENDIAN flag. We can also define
new RGBA8 formats that are defined to be packed in the declared
endianness. But I don't see a way out of dealing with the fact that
every legacy driver (which, like it or not, is mostly what BE hw gets
stuck with), expects these to be in cpu-endian formats, and as does
the rest of the existing userspace stack.

Cheers,

  -ilia