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

From: Philipp Zabel <p.zabel@pengutronix.de>
To: Steve Longerbeam <slongerbeam@gmail.com>, linux-media@vger.kernel.org
Cc: Tim Harvey <tharvey@gateworks.com>,
	"open list:DRM DRIVERS FOR FREESCALE IMX" 
	<dri-devel@lists.freedesktop.org>,
	open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v4 1/4] gpu: ipu-v3: ipu-ic: Rename yuv2rgb encoding matrices
Date: Wed, 13 Feb 2019 11:35:09 +0100	[thread overview]
Message-ID: <1550054109.3937.1.camel@pengutronix.de> (raw)
In-Reply-To: <7d4c5935-ffa1-2320-1632-136e1ce89350@gmail.com>

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