NetBSD Problem Report #51492

From www@NetBSD.org  Tue Sep 20 16:02:52 2016
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(Client CN "mail.netbsd.org", Issuer "Postmaster NetBSD.org" (verified OK))
	by mollari.NetBSD.org (Postfix) with ESMTPS id E5F117A271
	for <gnats-bugs@gnats.NetBSD.org>; Tue, 20 Sep 2016 16:02:52 +0000 (UTC)
Message-Id: <20160920160251.E258C7A2A7@mollari.NetBSD.org>
Date: Tue, 20 Sep 2016 16:02:51 +0000 (UTC)
From: scole_mail@gmx.com
Reply-To: scole_mail@gmx.com
To: gnats-bugs@NetBSD.org
Subject: rasops byte order issue
X-Send-Pr-Version: www-1.0

>Number:         51492
>Notify-List:    uwe@netbsd.org
>Category:       kern
>Synopsis:       rasops byte order issue
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Tue Sep 20 16:05:00 +0000 2016
>Last-Modified:  Wed Sep 28 16:15:51 +0000 2016
>Originator:     scole_mail
>Release:        7.0
>Organization:
none
>Environment:
NetBSD dstar 7.0.1_PATCH NetBSD 7.0.1_PATCH (GENERIC) #0: Mon Sep  5 13:24:11 PDT 2016  scole@dstar:/home/scole/nbsd/cvs/7_0/obj/sys/arch/i386/compile/GENERIC i386
>Description:
There may be a byte order issue with rasops(9) where the host byte order is possibly different from the video byte order, see this thread

http://mail-index.netbsd.org/current-users/2016/09/14/msg030132.html

igsfb and platinumfb are two drivers that might be affected.  It was suggested to add a flag like RI_FB_IS_BE.

I don't have the hardware to test this anymore but can describe the issues I had with platinunfb.

RI_BSWAP and kernel option RASOPS_SMALL may be related to this issue
>How-To-Repeat:

>Fix:
This patch fixed my issue for platinumfb 16 bit only, but may not be useful to the overall problem

Apply patch with "cd .../src/sys/dev/rasops ; patch -s -p0 < .../patchfile"

Index: rasops15.c
===================================================================
RCS file: /cvsroot/src/sys/dev/rasops/rasops15.c,v
retrieving revision 1.20
diff -b -u -r1.20 rasops15.c
--- rasops15.c	17 Apr 2012 12:06:25 -0000	1.20
+++ rasops15.c	14 Sep 2016 20:31:52 -0000
@@ -217,17 +217,26 @@
 	bg = ri->ri_devcmap[((u_int)attr >> 16) & 0xf] & 0xffff;
 	stamp_attr = attr;

+	/*
+	 * XXX - someone should sanity check but think this was doing
+	 *   stamp[i] = 8|16, stamp[i+1] = 2|4 (little endian)
+	 *   stamp[i] = 4|2, stamp[i+1] = 16|8 (big endian)
+	 * where should be
+	 *   stamp[i] = 2|4, stamp[i+1] = 8|16 (little endian)
+	 *   stamp[i] = 16|8, stamp[i+1] = 4|2 (big endian)
+	 */
 	for (i = 0; i < 32; i += 2) {
 #if BYTE_ORDER == LITTLE_ENDIAN
-		stamp[i] = (i & 16 ? fg : bg);
-		stamp[i] |= ((i & 8 ? fg : bg) << 16);
-		stamp[i + 1] = (i & 4 ? fg : bg);
-		stamp[i + 1] |= ((i & 2 ? fg : bg) << 16);
+            stamp[i] = (i & 4 ? fg : bg);
+            stamp[i] |= ((i & 2 ? fg : bg) << 16);
+            stamp[i + 1] = (i & 16 ? fg : bg);
+            stamp[i + 1] |= ((i & 8 ? fg : bg) << 16);
+
 #else
-		stamp[i] = (i & 2 ? fg : bg);
-		stamp[i] |= ((i & 4 ? fg : bg) << 16);
-		stamp[i + 1] = (i & 8 ? fg : bg);
-		stamp[i + 1] |= ((i & 16 ? fg : bg) << 16);
+            stamp[i] = (i & 8 ? fg : bg);
+            stamp[i] |= ((i & 16 ? fg : bg) << 16);
+            stamp[i + 1] = (i & 2 ? fg : bg);
+            stamp[i + 1] |= ((i & 4 ? fg : bg) << 16);
 #endif
 	}
 }

>Release-Note:

>Audit-Trail:
From: Valery Ushakov <uwe@stderr.spb.ru>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/51492: rasops byte order issue
Date: Wed, 21 Sep 2016 17:24:25 +0300

 As far as I udnerstand macallan@ suggestion for the new flag, we
 already de-facto use RI_BSWAP like that in 2- 4- and 8-bit rasops.

 For rasops8 I originally did the following back in 2002:

 #if BYTE_ORDER == BIG_ENDIAN
 #define NEED_LITTLE_ENDIAN_STAMP RI_BSWAP
 #else
 #define NEED_LITTLE_ENDIAN_STAMP 0
 #endif
     if ((ri->ri_flg & RI_BSWAP) == NEED_LITTLE_ENDIAN_STAMP) {
       /* make little endian stamp */
     } else {
       /* make big endian stamp */
     }

 which kiyohara@ later used for 2- and 4- too.

 That code should probaly be simplified by building the stamp once and
 using bswap() instead of spelling out the stamp building code twice.

 For rasops15.c the logic is somewhat obscured by the fact that bswap()
 is applied to the stamp in rasops.c.  It should use the logic like in
 rasops8.c except that it can build the stamp once and let bswap do the
 right thing (as suggested in the previous paragraph for 8-bit).

 rasops24 seems to do a combination of static and dynamic checks and
 that hints that it probably must be wrong for some combinations of
 endianness and RI_BSWAP.

 So in general I think that macallan@ proposal is basicaly right, but
 that the new flag is not needed and instead existing RI_BSWAP should
 be fixed.  Then fb driver attach code can just use a macro like

     rasops_byte_order(ri, BIG_ENDIAN); // this fb is big endian

 which will statically expand to nothing or to setting RI_BSWAP
 depending on cpu's endianness.

 -uwe

From: Valery Ushakov <uwe@stderr.spb.ru>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/51492: rasops byte order issue
Date: Wed, 28 Sep 2016 17:56:41 +0300

 On Tue, Sep 20, 2016 at 16:05:00 +0000, scole_mail@gmx.com wrote:

 > +	/*
 > +	 * XXX - someone should sanity check but think this was doing
 > +	 *   stamp[i] = 8|16, stamp[i+1] = 2|4 (little endian)
 > +	 *   stamp[i] = 4|2, stamp[i+1] = 16|8 (big endian)
 > +	 * where should be
 > +	 *   stamp[i] = 2|4, stamp[i+1] = 8|16 (little endian)
 > +	 *   stamp[i] = 16|8, stamp[i+1] = 4|2 (big endian)
 > +	 */

 Actually looking at what's going on, I think that patch just inverts
 the problem and makes it work for you on big endian, while probably
 (from RTFS) breaking little endian.

 Stamp code is a bit obfuscated but here's what I think is going on
 here.  We pre-build int32_t stamp[32] array, though it's really a
 [16][2] array where the first index is a nibble (4 bits) of font
 raster and it selects an int32_t[2] array (64 bits) of 4 16-bit RGB
 values (64-bits) to write to the framebuffer.  See STAMP_SHIFT and
 STAMP_MASK macros.

 We encode font raster data with leftmost on-screen bit as the most
 significant bit, e.g

   8-bit wide font:

       0xfe,       /* [][][][][][][].. */

   12-bit wide font (padded with zero bits):

       0x07, 0xf0, /* .....******* */

 We take a nibble of font raster data and get its int32_t[2] array of
 rgb stamp data, so upper (leftmost on-screen) two bits of the nibble
 are always in the stamp[i] and the lower (rightmost on-screen) two
 bits are always in the stamp[i+1], to use rasops15_makestamp() terms.
 The corresponding putchar code is:

      so = STAMP_SHIFT(fr[0], 1) & STAMP_MASK;
      rp[0] = STAMP_READ(so);
      rp[1] = STAMP_READ(so + 4);

 which is an obfuscated way to index into stamp as if it were a
 two-dimensional array.

 Thus makestamp must always build:

     stamp[i  ] = rgb for { 16, 8 };
     stamp[i+1] = rgb for {  4, 2 };

 but it must take RI_BSWAP into account as well to build either 16|8 or
 8|16 32-bit stamp fragment.  Note, that applying bswap32() to stamp[j]
 will also swap bytes of the 16-bit rgb pixel values, but ri_devcmap
 provides them already bsawp16()'ed for RI_BSAWP (see rasops.c), so it
 will cancel out.

 -uwe

>Unformatted:

Home	PR Database Search