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

From: Steve Longerbeam <slongerbeam@gmail.com>
To: Philipp Zabel <p.zabel@pengutronix.de>, 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: Tue, 12 Feb 2019 09:42:08 -0800	[thread overview]
Message-ID: <7d4c5935-ffa1-2320-1632-136e1ce89350@gmail.com> (raw)
In-Reply-To: <1549966666.4800.3.camel@pengutronix.de>

On 2/12/19 2:17 AM, Philipp Zabel wrote:
> Hi Steve,
>
> On Mon, 2019-02-11 at 10:24 -0800, Steve Longerbeam wrote:
> [...]
>> Looking more closely at these coefficients now, I see you are right,
>> they are the BT.601 YUV full-range coefficients (Y range 0 to 1, U and V
>> range -0.5 to 0.5). Well, not even that -- the coefficients are not
>> being scaled to the limited ranges, but the 0.5 offset (128) _is_ being
>> added to U/V, but no offset for Y. So it is even more messed up.
>>
>> Your corrected coefficients and offsets look correct to me: Y
>> coefficients scaled to (235 - 16) / 255 and U/V coefficients scaled to
>> (240 - 16)  / 255, and add the offsets for both Y and U/V.
>>
>> 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?

>
>> 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.

>
>> 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?

>   Otherwise we'd have to
> round towards zero, possibly with a larger error, to make sure the
> results are inside the valid ranges.

Makes sense, I will turn on that bit for limited range YUV output for 
v5, so that the final color component result is clipped to limited range 
and rounded.

Steve