On 31/07/2023 12:27, AngeloGioacchino Del Regno wrote:
Il 28/07/23 14:58, Alexandre Mergnat ha scritto:
Hi Angelo
On 27/07/2023 15:06, AngeloGioacchino Del Regno wrote:
+/* For 10 bit LUT layout, R/G/B are in the same register */
#define DISP_GAMMA_LUT_10BIT_R GENMASK(29, 20)
#define DISP_GAMMA_LUT_10BIT_G GENMASK(19, 10)
#define DISP_GAMMA_LUT_10BIT_B GENMASK(9, 0)
+/* For 12 bit LUT layout, R/G are in LUT, B is in LUT1 */
As I understood from the application processor registers (v0.4), R/G
are in LUT, B is in LUT1 for 10bit and 12bit for MT8195. Can you
check please to be sure ?
That's right, but here I'm implying that 10-bit LUT is only for older
SoCs, and
all of them have got the same register layout with one LUT register
for R, G, B,
while all the new SoCs, which have got 12-bits LUT support, have got
the new
register layout with two LUT registers (and multiple banks).
Infact, the MT8195 SoC was added here with 12-bits LUT support only
(as the LUT
parameters extraction is easily handled by the
drm_color_lut_extract() function).
The alternative would've been to add two compatibles, like
"mediatek,mt8195-disp-gamma-10bits" and
"mediatek,mt8195-disp-gamma-12bits",
or a boolean property like "mediatek,lut-12bits" which would appear
literally
everywhere starting from a certain point in time (since there's no
reason to
use 10-bits LUT on MT8195, that starts now!).
Even then, consider the complication in code, where
mtk_gamma_set_common()
would have to handle:
- 10-bits, layout A
- 10-bits, layout B -> but fallback to layout A if this is AAL
- 12-bits layout
is_aal = !(gamma && gamma->data);
for_each_bank()
{
if (num_lut_banks > 1) write_num_bank();
for (i = 0; i < lut_bank_size; i++) {
.......
if (!lut_diff || (i % 2 == 0)) {
if (lut_bits == 12 || (lut_bits == 10 && layout_b)) {
... setup word[0],[1] ...
} else if (layout_b && !is_aal) {
...setup word[0],[1]...
} else {
...setup word[0]
}
} else {
^^^ almost repeat the same ^^^
}
writel(word[0], (...));
if (lut_bits == 12 || (lut_bits == 10 && layout_b) && !is_aal)
writel(word[i] (....));
}
}
probe() {
if (of_property_read_bool(dev->of_node, "mediatek,lut-12bits") ||
data->supports_only_12bits)
priv->lut_bits = 12;
else
priv->lut_bits = 10;
}
...at least, that's the implementation that I would do to solve your
concern,
which isn't *too bad*, but still, a big question arises here...
Why should we care about supporting *both* 10-bit and 12-bit Gamma
LUTs on
the *same* SoC?
A 12-bit LUT gives us more precision and there's no penalty if we
want to
convert a 10-bit LUT to a 12-bits one, as we're simply "ignoring" the
value
of two bits per component (no expensive calculation involved)...
Is there anything that I'm underestimating here?
Thanks for you explanation !
I think your choice is not bad, but it's not clear that MT8195 10 bit
LUT isn't supported at all.
So, IMHO, the first solution is to support it like you explained it
above, and the second solution is to add comment somewhere to clarify
that driver doesn't support 10 bit LUT if the SoC is able to use 12
bit LUT, like MT8195 10 bit.
Is that relevant ? :D
Even though the same as whhat I'm doing here was already done before, as
the
current 10-bits LUT support ignores 9-bits LUT support, I can add a
comment to
the code:
/*
* SoCs supporting 12-bits LUTs are using a new register layout that does
* always support (by HW) both 12-bits and 10-bits LUT but, on those, we
* ignore the support for 10-bits in this driver and always use 12-bits.
*
* Summarizing:
* - SoC HW support 9/10-bits LUT only
* - Old register layout
* - 10-bits LUT supported
* - 9-bits LUT not supported
* - SoC HW support both 10/12bits LUT
* - New register layout
* - 12-bits LUT supported
* - 10-its LUT not supported
*/
Where the SoCs supporting 9-bits and 10-bits: mt6795, 8173, 8192,others and
12-bits are 8195, 8186, others.. of course.
Would that work for you?
Sound good for me. After that:
Reviewed-by: Alexandre Mergnat <amergnat@xxxxxxxxxxxx>
--
Regards,
Alexandre
