Re: [PATCH RESEND v6 09/11] drm/mediatek: gamma: Add support for 12-bit LUT and MT8195

[Alexandre Mergnat <amergnat@baylibre.com>]

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