linux-kernel - Re: [PATCH v4 1/4] gpu: ipu-v3: ipu-ic: Rename yuv2rgb encoding matrices

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1550054109.3937.1.camel@pengutronix.de>
Date:   Wed, 13 Feb 2019 11:35:09 +0100
From:   Philipp Zabel <p.zabel@...gutronix.de>
To:     Steve Longerbeam <slongerbeam@...il.com>,
        linux-media@...r.kernel.org
Cc:     Tim Harvey <tharvey@...eworks.com>,
        "open list:DRM DRIVERS FOR FREESCALE IMX" 
        <dri-devel@...ts.freedesktop.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 1/4] gpu: ipu-v3: ipu-ic: Rename yuv2rgb encoding
 matrices

On Tue, 2019-02-12 at 09:42 -0800, Steve Longerbeam wrote:
[...]
> > > But what about this "SAT_MODE" field in the IC task parameter memory?
> > 
> > That just controls the saturation. The result after the matrix
> > multiplication is either saturated to [0..255] or to [16..235]/[16..240]
> > when converting from the internal representation to the 8 bit output.
> 
> By saturation I think you mean clipped to those ranges?

Yes, thanks. I didn't realize it sounds weird to use saturated this way.
See: https://en.wikipedia.org/wiki/Saturation_arithmetic

> > > According to the manual the hardware will automatically convert the
> > > written coefficients to the correct limited ranges.
> > 
> > Where did you get that from? "The final calculation result is limited
> > according to the SAT_MODE parameter and rounded to 8 bits." I see no
> > mention of coefficients being modified.
> 
> Well, as is often the case with this manual, I was interpreting based on 
> poorly written information. By "final calculation result is limited 
> according to the SAT_MODE parameter" I interpreted that to mean the 
> hardware enables scaling from full range to limited range. But I concede 
> that it more likely means it clips the output to those ranges.

Ok, with this manual I'm never sure that there aren't some conflicting
statements somewhere else that I might have overlooked. Good to see that
we are at least basing our interpretations on the same text.

> > > I see there is a "sat" field defined in the struct but is not being
> > > set in the tables.
> > > 
> > > So what should we do, define the full range coefficients, and make use
> > > of SAT_MODE h/w feature, or scale/offset the coefficients ourselves and
> > > not use SAT_MODE? I'm inclined to do the former.
> > 
> > SAT_MODE should be set for conversions to YUV limited range so that the
> > coefficients can be rounded to the closest value.
> 
> Well, we have already rounded the coefficients to the nearest int in the 
> tables. Do you mean the final result (coeff * color component + offset) 
> is rounded?

The manual says so: "The final calculation result is limited according
to the SAT_MODE parameter and rounded to 8 bits", but that's not what I
meant. Still, I might have been mistaken.

I think due to the fact that the coefficients are multiplied by up to
255 (max pixel value) and then effectively divided by 256 when
converting to 8 bit, the only way to overflow limited range is if two
coefficients are rounded away from zero in the calculation of a single
component. This doesn't seem to happen in practice.

A constructed example, conversion to YUV limited range with carefully
chosen coefficients.

  Y = R * .1817 + G * .6153 + B * .0618 + 16;

Note that .1817 + .6153 + .0618 < 219/255.
With rounded coefficients though:

  Y = (R * 47 + G * 158 + B * 16 + (64 << 6)) / 256 = 236.136

Now 47 + 158 + 16 > 219, and the result is out of range.

regards
Philipp