Messages in this thread | | | Date | Mon, 31 Jul 2023 13:57:41 +0200 | Subject | Re: [PATCH RESEND v6 09/11] drm/mediatek: gamma: Add support for 12-bit LUT and MT8195 | From | Alexandre Mergnat <> |
| |
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@baylibre.com>
-- Regards, Alexandre
| |