| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <519F4435.5010703@digi.com>
Date: Fri, 24 May 2013 12:43:01 +0200
From: Hector Palacios <hector.palacios@...i.com>
To: Juergen Beisert <jbe@...gutronix.de>
CC: "linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"maxime.ripard@...e-electrons.com" <maxime.ripard@...e-electrons.com>,
"fabio.estevam@...escale.com" <fabio.estevam@...escale.com>,
"brian@...stalfontz.com" <brian@...stalfontz.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Alexandre Belloni <alexandre.belloni@...e-electrons.com>
Subject: Re: mxsfb: DATA_FORMAT_24_BIT flag outputs invalid colours
Hi Juergen,
On 05/24/2013 12:28 PM, Juergen Beisert wrote:
> Hector Palacios wrote:
>> Hi Juergen,
>>
>> On 05/23/2013 03:31 PM, Juergen Beisert wrote:
>>> Hi Maxime,
>>>
>>> maxime.ripard@...e-electrons.com wrote:
>>>> On Thu, May 23, 2013 at 01:55:28PM +0200, Hector Palacios wrote:
>>>>> I'm using an i.MX28 based board with lcd connected with 18bits data
>>>>> bus. My platform uses 32 bits per pixel:
>>>>>
>>>>> mxsfb_pdata.default_bpp = 32;
>>>>> mxsfb_pdata.ld_intf_width = STMLCDIF_18BIT;
>>>>>
>>>>> With these settings the mxsfb.c driver sets flag DATA_FORMAT_24_BIT
>>>>> at HW_LCDIF_CTRL register in function mxsfb_set_par():
>>>>>
>>>>> case 32:
>>>>> dev_dbg(&host->pdev->dev, "Setting up RGB888/666 mode\n");
>>>>> ctrl |= CTRL_SET_WORD_LENGTH(3);
>>>>> switch (host->ld_intf_width) {
>>>>> case STMLCDIF_8BIT:
>>>>> dev_dbg(&host->pdev->dev,
>>>>> "Unsupported LCD bus width mapping\n");
>>>>> return -EINVAL;
>>>>> case STMLCDIF_16BIT:
>>>>> case STMLCDIF_18BIT:
>>>>> /* 24 bit to 18 bit mapping */
>>>>> ctrl |= CTRL_DF24; /* ignore the upper 2 bits in
>>>>> * each colour component
>>>>> */
>>>>> break;
>>>>> case STMLCDIF_24BIT:
>>>>> /* real 24 bit */
>>>>> break;
>>>>> }
>>>>>
>>>>> According to the manual, this flag does:
>>>>> 0x0: ALL_24_BITS_VALID: Data input to the block is in 24 bpp
>>>>> format, such that all RGB 888 data is contained in 24 bits.
>>>>> 0x1: DROP_UPPER_2_BITS_PER_BYTE — Data input to the block is
>>>>> actually RGB 18 bpp, but there is 1 colour per byte, hence the upper
>>>>> 2 bits in each byte do not contain any useful data, and should be
>>>>> dropped.
>>>>>
>>>>> The setting of this flag is producing bad colours with true colour
>>>>> images (i.e. the Linux penguin is displayed ok, but QT applications
>>>>> or images displayed with fbv are not).
>>>>> I believe the setting of this flag is not correct (after all, if my
>>>>> bpp is 32, then all 24bit colours are useful and dropping the upper
>>>>> 2 bits is a bad idea).
>>>>> If I don't set it, then true colour images are displayed correctly.
>>>>> The only problem is that the Linux penguin is displayed much darker
>>>>> than usual (correct colours, but darker). Perhaps the 224 colour
>>>>> format of this image justifies it?
>>>>>
>>>>> I noticed the cfa10049 platform also uses the same configuration (18
>>>>> bits data bus and 32bpp) and was wondering if true colour images are
>>>>> correctly displayed in this platform with this flag set (for example
>>>>> with fbv application [1]).
>>>>
>>>> I had the exact same problem, and suggested the exact same solution a
>>>> few weeks back.
>>>>
>>>> https://patchwork.kernel.org/patch/2470441/
>>>>
>>>> The conclusion of that discussion what that the userspace applications
>>>> were not honouring the bitfield correctly set by the mxsfb driver, and
>>>> as such, it was not a bug in the driver.
>>>>
>>>> While this is correct, I wonder, now that since we had that same problem
>>>> in a very short amount of time, if we couldn't set this behaviour
>>>> dependant of some (dt? kernel argument?) property so that one could
>>>> customise it anyway he want.
>>>>
>>>> Maxime
>>>
>>> i.MX2[3|8] LCD1 LCD2 LCD3
>>> 24bit 18bit 18bit
>>> --------------------------------------------
>>> LCD_D0 B0 B0 --
>>> LCD_D1 B1 B1 --
>>> LCD_D2 B2 B2 B0
>>> LCD_D3 B3 B3 B1
>>> LCD_D4 B4 B4 B2
>>> LCD_D5 B5 B5 B3
>>> LCD_D6 B6 G0 B4
>>> LCD_D7 B7 G1 B5
>>>
>>> LCD_D8 G0 G2 --
>>> LCD_D9 G1 G3 --
>>> LCD_D10 G2 G4 G0
>>> LCD_D11 G3 G5 G1
>>> LCD_D12 G4 R0 G2
>>> LCD_D13 G5 R1 G3
>>> LCD_D14 G6 R2 G4
>>> LCD_D15 G7 R3 G5
>>>
>>> LCD_D16 R0 R4 --
>>> LCD_D17 R1 R5 --
>>> LCD_D18 R2 R0
>>> LCD_D19 R3 R1
>>> LCD_D20 R4 R2
>>> LCD_D21 R5 R3
>>> LCD_D22 R6 R4
>>> LCD_D23 R7 R5
>>>
>>> Is your display connected like LCD2 or LCD3? LCD3 must still handled like
>>> a 24 bit display shown in LCD1, while only the LCD2-case is the "24 bit
>>> to 18 bit mapping" case.
>>>
>>> At least my current tests with an i.MX23 and a connection like LCD2 are
>>> working here with a Qt application. Qt honours the pixel bitfield
>>> description. And I'm using the "bits-per-pixel = <32>" and "bus-width =
>>> <18>" entries in the device tree.
>>
>> I have a 24bit LCD display but my connection to it is done at 18bits data
>> width. Represented below as LCD4.
>> NOTE: In my LCD4 column, notation Rx/Gx/Bx represent the color bit in
>> memory as well as the display data line.
>> Since we use 32bpp each channel has 8 bits (R7..R0, etc.).
>> I understand that you have an 18bit display and that your notation in LCD2
>> column represents the display data lines, not the color bit indexes in
>> memory.
>>
>> i.MX2[3|8] LCD1 LCD2 LCD3 LCD4
>> 24bit 18bit 18bit 24bit connected at 18bit
>> -------------------------------------------------------
>> LCD_D0 B0 B0 -- B2
>> LCD_D1 B1 B1 -- B3
>> LCD_D2 B2 B2 B0 B4
>> LCD_D3 B3 B3 B1 B5
>> LCD_D4 B4 B4 B2 B6
>> LCD_D5 B5 B5 B3 B7
>> LCD_D6 B6 G0 B4 G2
>> LCD_D7 B7 G1 B5 G3
>>
>> LCD_D8 G0 G2 -- G4
>> LCD_D9 G1 G3 -- G5
>> LCD_D10 G2 G4 G0 G6
>> LCD_D11 G3 G5 G1 G7
>> LCD_D12 G4 R0 G2 R2
>> LCD_D13 G5 R1 G3 R3
>> LCD_D14 G6 R2 G4 R4
>> LCD_D15 G7 R3 G5 R5
>>
>> LCD_D16 R0 R4 -- R6
>> LCD_D17 R1 R5 -- R7
>> LCD_D18 R2 R0
>> LCD_D19 R3 R1
>> LCD_D20 R4 R2
>> LCD_D21 R5 R3
>> LCD_D22 R6 R4
>> LCD_D23 R7 R5
>>
>> For 32bpp (RGB888) and 18bit data bus I would expect the LCD controller to
>> take the six *most significant* bits [7..2] from each color byte out to the
>> LCD data bus (LCD_D17..D0) in the order depicted in my LCD4 column.
>>
>> I'm not sure what the DATA_FORMAT_24_BIT flag is doing, but dropping the
>> two most significant bits of color in memory doesn't seem to be a good idea
>> unless (maybe) color is in 18bpp. Previous kernels did not even touch this
>> flag.
>>
>> Does the following patch make sense?
>>
>> diff --git a/drivers/video/mxsfb.c b/drivers/video/mxsfb.c
>> index b1c1a80..bb0a4e1 100644
>> --- a/drivers/video/mxsfb.c
>> +++ b/drivers/video/mxsfb.c
>> @@ -298,9 +298,6 @@ static int mxsfb_check_var(struct fb_var_screeninfo
>> *var, break;
>> case STMLCDIF_16BIT:
>> case STMLCDIF_18BIT:
>> - /* 24 bit to 18 bit mapping */
>> - rgb = def_rgb666;
>> - break;
>> case STMLCDIF_24BIT:
>> /* real 24 bit */
>> rgb = def_rgb888;
>> @@ -424,11 +421,6 @@ static int mxsfb_set_par(struct fb_info *fb_info)
>> return -EINVAL;
>> case STMLCDIF_16BIT:
>> case STMLCDIF_18BIT:
>> - /* 24 bit to 18 bit mapping */
>> - ctrl |= CTRL_DF24; /* ignore the upper 2 bits in
>> - * each colour component
>> - */
>> - break;
>> case STMLCDIF_24BIT:
>> /* real 24 bit */
>> break;
>>
>> The setting of def_rgb666 for a 32bpp color depth does not make sense to me
>> because the color in memory is really rgb888.
>>
>> With this patch, my true color images are displayed ok and so does the
>> penguin logo. I don't know however how other displays connections at 18bit
>> will do.
>
> Your 24 bit display is connected like a regular 18 bit display. So they should
> work in the same way with the same settings. You *must* always skip two data
> bits from the memory RGB888 to form a RGB666 value.
>
> I just did some measurement here with my i.MX23 based hardware.
>
> I'm using 32 bits per pixel and the 18 bit display interface (LCD_D[0..17]).
>
> And surprise, surprise: the i.MX23 *always* maps the 24 bit input data to the
> 18 bit interface. And what does the DATA_FORMAT_24_BIT aka. CTRL_DF24 do???
> Just simple: it seems its meaning changes with the interface width. The
> documentation says:
>
> 0 = all 24 bits are valid
> 1 = drop upper 2 bits per byte
>
> This text seems valid for a real 24 bit display (but I cannot test it). From
> my measurement its meaning changes when used with an 18 bit display to:
>
> 0 = drop lower 2 bits per byte
> 1 = drop upper 2 bits per byte
>
> When this bit is 0:
>
> red green blue
> 10000001|00001111|10001111 (memory layout)
>
> 100000..|000011..|100011.. (at the display)
>
> When this bit is 1:
>
> red green blue
> 10000001|00001111|10001111 (memory layout)
>
> ..000001|..001111|..001111 (at the display)
And how can the setting of this flag be useful?
In the example above, you had a red component of 0x81 (half way through the scale),
which is converted to a 0x01 (almost black) at the display. Isn't this wrong?
This only looks right to me if you set a 18bpp but even in that case masking the upper
bits isn't really needed at all.
Regards,
--
Héctor Palacios
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists