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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

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?

Regards,
Angelo