From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1426698AbdDVTYm (ORCPT ); Sat, 22 Apr 2017 15:24:42 -0400 Received: from mail-qt0-f195.google.com ([209.85.216.195]:34870 "EHLO mail-qt0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757355AbdDVTYj (ORCPT ); Sat, 22 Apr 2017 15:24:39 -0400 MIME-Version: 1.0 In-Reply-To: References: <20170421075825.6307-1-kraxel@redhat.com> <20170421165907.GQ30290@intel.com> <20170422095056.GR30290@intel.com> From: Ilia Mirkin Date: Sat, 22 Apr 2017 15:24:37 -0400 X-Google-Sender-Auth: ZhV2bbCw_04qpJr07JN59_eKxhw Message-ID: Subject: Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality. To: =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= Cc: Gerd Hoffmann , "dri-devel@lists.freedesktop.org" , Daniel Vetter , Pekka Paalanen , =?UTF-8?Q?Michel_D=C3=A4nzer?= , Alex Deucher , amd-gfx@lists.freedesktop.org, Jani Nikula , Sean Paul , David Airlie , open list Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id v3MJOtph005278 On Sat, Apr 22, 2017 at 9:48 AM, Ilia Mirkin wrote: > On Sat, Apr 22, 2017 at 9:40 AM, Ilia Mirkin wrote: >> On Sat, Apr 22, 2017 at 5:50 AM, Ville Syrjälä >> 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ä >>>> 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 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