[PATCH 00/11] Add HDMI support for RK3128

[PATCH 00/11] Add HDMI support for RK3128

[Alex Bee]

This series aims to add support for the display controller (VOP) and the
HDMI controller block of RK3128 (which is very similar to the one found in
RK3036).

The VOP part is very simple -  everything we need for HDMI support is
already there. I only needed to split the output selection registers from
RK3036.
The VOP has an IOMMU attached, but it has a serious silicon bug:
Registers can only be written, but not be read. As it's not possible to use
it with the IOMMU driver in it's current state I'm not adding it here and
we have to live with CMA for now - which works fine also. Andy, maybe you
can shed a light on whether there is any way to not have to write the
registers only and hope for the best (a.k.a rockchip,skip-mmu-read). I have
it working here locally - but nothing for upstream (yet).

The inno-hdmi driver currently gets a lot of attention [0-2] and I'm
hooking in now also. I did only the bare minimum to get a correctly colored
picture and a stable signal for RK3128. I leave the cleanups to those other
series and will come back with sound/CEC support later. It shouldn't
interfere to much, only the csc-part must not get dropped completely and
the mode-check-hooks are no longer dead.

Note: Patches are based and tested on next-20231213.

[0] https://lore.kernel.org/all/20231207-kms-hdmi-connector-state-v5-0-6538e19d634d@kernel.org
[1] https://lore.kernel.org/all/20231204123315.28456-1-keith.zhao@starfivetech.com
[2] https://lore.kernel.org/all/2601b669-c570-f39d-8cf9-bff56c939912@gmail.com

Alex Bee (11):
  dt-bindings: display: rockchip,inno-hdmi: Document RK3128 compatible
  drm/rockchip: vop: Add output selection registers for RK312x
  drm/rockchip: inno_hdmi: Fix video timing
  drm/rockchip: inno_hdmi: Correctly setup HDMI quantization range
  drm/rockchip: inno_hdmi: Add variant support
  drm/rockchip: inno_hdmi: Add RK3128 support
  drm/rockchip: inno_hdmi: Add basic mode validation
  drm/rockchip: inno_hdmi: Drop custom fill_modes hook
  ARM: dts: rockchip: Add display subsystem for RK3128
  ARM: dts rockchip: Add HDMI node for RK3128
  ARM: dts: rockchip: Enable HDMI output for XPI-3128

 .../display/rockchip/rockchip,inno-hdmi.yaml  |  30 ++-
 .../arm/boot/dts/rockchip/rk3128-xpi-3128.dts |  29 +++
 arch/arm/boot/dts/rockchip/rk3128.dtsi        |  60 ++++++
 drivers/gpu/drm/rockchip/inno_hdmi.c          | 203 +++++++++++++++---
 drivers/gpu/drm/rockchip/rockchip_vop_reg.c   |  13 +-
 drivers/gpu/drm/rockchip/rockchip_vop_reg.h   |   3 +
 6 files changed, 309 insertions(+), 29 deletions(-)


base-commit: 48e8992e33abf054bcc0bb2e77b2d43bb899212e
-- 
2.43.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 01/11] dt-bindings: display: rockchip,inno-hdmi: Document RK3128 compatible

[Alex Bee]

Document the compatible for RK3128's HDMI controller block.
The integration for this SoC is somewhat different here: It needs the PHY's
reference clock rate to calculate the ddc bus frequency correctly. This
clock is part of a power-domain (PD_VIO), so this gets added as an optional
property too.

Signed-off-by: Alex Bee <knaerzche@gmail.com>
---
 .../display/rockchip/rockchip,inno-hdmi.yaml  | 30 +++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,inno-hdmi.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,inno-hdmi.yaml
index 96889c86849a..9f00abcbfb38 100644
--- a/Documentation/devicetree/bindings/display/rockchip/rockchip,inno-hdmi.yaml
+++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,inno-hdmi.yaml
@@ -14,6 +14,7 @@ properties:
   compatible:
     enum:
       - rockchip,rk3036-inno-hdmi
+      - rockchip,rk3128-inno-hdmi
 
   reg:
     maxItems: 1
@@ -22,10 +23,21 @@ properties:
     maxItems: 1
 
   clocks:
-    maxItems: 1
+    minItems: 1
+    items:
+      - description: The HDMI controller main clock
+      - description: The HDMI PHY reference clock
 
   clock-names:
-    const: pclk
+    minItems: 1
+    items:
+      - const: pclk
+      - enum:
+          - pclk
+          - ref
+
+  power-domains:
+    maxItems: 1
 
   ports:
     $ref: /schemas/graph.yaml#/properties/ports
@@ -55,6 +67,20 @@ required:
   - pinctrl-names
   - ports
 
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: rockchip,rk3128-inno-hdmi
+
+    then:
+      properties:
+        clocks:
+          minItems: 2
+        clock-names:
+          minItems: 2
+
 additionalProperties: false
 
 examples:
-- 
2.43.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 01/11] dt-bindings: display: rockchip,inno-hdmi: Document RK3128 compatible

[Krzysztof Kozlowski]

On 13/12/2023 20:51, Alex Bee wrote:
> Document the compatible for RK3128's HDMI controller block.
> The integration for this SoC is somewhat different here: It needs the PHY's

Please wrap commit message according to Linux coding style / submission
process (neither too early nor over the limit):
https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597


> reference clock rate to calculate the ddc bus frequency correctly. This
> clock is part of a power-domain (PD_VIO), so this gets added as an optional
> property too.

If clock is part of power domain, then the power domain must be in the
clock controller, not here. So either you put power domain in wrong
place or you used incorrect reason for a change.

> 
> Signed-off-by: Alex Bee <knaerzche@gmail.com>
> ---
>  .../display/rockchip/rockchip,inno-hdmi.yaml  | 30 +++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,inno-hdmi.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,inno-hdmi.yaml
> index 96889c86849a..9f00abcbfb38 100644
> --- a/Documentation/devicetree/bindings/display/rockchip/rockchip,inno-hdmi.yaml
> +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,inno-hdmi.yaml
> @@ -14,6 +14,7 @@ properties:
>    compatible:
>      enum:
>        - rockchip,rk3036-inno-hdmi
> +      - rockchip,rk3128-inno-hdmi
>  
>    reg:
>      maxItems: 1
> @@ -22,10 +23,21 @@ properties:
>      maxItems: 1
>  
>    clocks:
> -    maxItems: 1
> +    minItems: 1
> +    items:
> +      - description: The HDMI controller main clock
> +      - description: The HDMI PHY reference clock
>  
>    clock-names:
> -    const: pclk
> +    minItems: 1
> +    items:
> +      - const: pclk
> +      - enum:
> +          - pclk
> +          - ref

That's way overcomplicated. Just items listing the names and minItems:
1. See other bindings how this is done.

> +
> +  power-domains:
> +    maxItems: 1

Is it relevant to existing device?

>  
>    ports:
>      $ref: /schemas/graph.yaml#/properties/ports
> @@ -55,6 +67,20 @@ required:
>    - pinctrl-names
>    - ports


Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 01/11] dt-bindings: display: rockchip,inno-hdmi: Document RK3128 compatible

[Alex Bee]

Am 14.12.23 um 08:53 schrieb Krzysztof Kozlowski:
> On 13/12/2023 20:51, Alex Bee wrote:
>> Document the compatible for RK3128's HDMI controller block.
>> The integration for this SoC is somewhat different here: It needs the PHY's
> Please wrap commit message according to Linux coding style / submission
> process (neither too early nor over the limit):
> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597
OK. Not sure why checkpatch --strict  didn't tell me that I'm over the 
limit here.
>
>> reference clock rate to calculate the ddc bus frequency correctly. This
>> clock is part of a power-domain (PD_VIO), so this gets added as an optional
>> property too.
> If clock is part of power domain, then the power domain must be in the
> clock controller, not here. So either you put power domain in wrong
> place or you used incorrect reason for a change.
  Rockchip defines it's powerdomains per clock and I was little to much 
in that world when writing this. Actually the controller itself is part 
of the powerdomain. Will rephrase.
>> Signed-off-by: Alex Bee <knaerzche@gmail.com>
>> ---
>>   .../display/rockchip/rockchip,inno-hdmi.yaml  | 30 +++++++++++++++++--
>>   1 file changed, 28 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,inno-hdmi.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,inno-hdmi.yaml
>> index 96889c86849a..9f00abcbfb38 100644
>> --- a/Documentation/devicetree/bindings/display/rockchip/rockchip,inno-hdmi.yaml
>> +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,inno-hdmi.yaml
>> @@ -14,6 +14,7 @@ properties:
>>     compatible:
>>       enum:
>>         - rockchip,rk3036-inno-hdmi
>> +      - rockchip,rk3128-inno-hdmi
>>   
>>     reg:
>>       maxItems: 1
>> @@ -22,10 +23,21 @@ properties:
>>       maxItems: 1
>>   
>>     clocks:
>> -    maxItems: 1
>> +    minItems: 1
>> +    items:
>> +      - description: The HDMI controller main clock
>> +      - description: The HDMI PHY reference clock
>>   
>>     clock-names:
>> -    const: pclk
>> +    minItems: 1
>> +    items:
>> +      - const: pclk
>> +      - enum:
>> +          - pclk
>> +          - ref
> That's way overcomplicated. Just items listing the names and minItems:
> 1. See other bindings how this is done.
OK.
>> +
>> +  power-domains:
>> +    maxItems: 1
> Is it relevant to existing device?

Will move to new variant only.

Alex

>>   
>>     ports:
>>       $ref: /schemas/graph.yaml#/properties/ports
>> @@ -55,6 +67,20 @@ required:
>>     - pinctrl-names
>>     - ports
>
> Best regards,
> Krzysztof
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 01/11] dt-bindings: display: rockchip,inno-hdmi: Document RK3128 compatible

[Krzysztof Kozlowski]

On 14/12/2023 16:22, Alex Bee wrote:
> 
> Am 14.12.23 um 08:53 schrieb Krzysztof Kozlowski:
>> On 13/12/2023 20:51, Alex Bee wrote:
>>> Document the compatible for RK3128's HDMI controller block.
>>> The integration for this SoC is somewhat different here: It needs the PHY's
>> Please wrap commit message according to Linux coding style / submission
>> process (neither too early nor over the limit):
>> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597
> OK. Not sure why checkpatch --strict  didn't tell me that I'm over the 
> limit here.
>>
>>> reference clock rate to calculate the ddc bus frequency correctly. This
>>> clock is part of a power-domain (PD_VIO), so this gets added as an optional
>>> property too.
>> If clock is part of power domain, then the power domain must be in the
>> clock controller, not here. So either you put power domain in wrong
>> place or you used incorrect reason for a change.
>   Rockchip defines it's powerdomains per clock and I was little to much 
> in that world when writing this. Actually the controller itself is part 
> of the powerdomain. Will rephrase.

Does it mean you have like 200 different power domains in one SoC? Then
how are they different than clock if there is one-to-one mapping?

>>> Signed-off-by: Alex Bee <knaerzche@gmail.com>
>>> ---
>>>   .../display/rockchip/rockchip,inno-hdmi.yaml  | 30 +++++++++++++++++--
>>>   1 file changed, 28 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,inno-hdmi.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,inno-hdmi.yaml
>>> index 96889c86849a..9f00abcbfb38 100644
>>> --- a/Documentation/devicetree/bindings/display/rockchip/rockchip,inno-hdmi.yaml
>>> +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,inno-hdmi.yaml
>>> @@ -14,6 +14,7 @@ properties:
>>>     compatible:
>>>       enum:
>>>         - rockchip,rk3036-inno-hdmi
>>> +      - rockchip,rk3128-inno-hdmi
>>>   
>>>     reg:
>>>       maxItems: 1
>>> @@ -22,10 +23,21 @@ properties:
>>>       maxItems: 1
>>>   
>>>     clocks:
>>> -    maxItems: 1
>>> +    minItems: 1
>>> +    items:
>>> +      - description: The HDMI controller main clock
>>> +      - description: The HDMI PHY reference clock
>>>   
>>>     clock-names:
>>> -    const: pclk
>>> +    minItems: 1
>>> +    items:
>>> +      - const: pclk
>>> +      - enum:
>>> +          - pclk
>>> +          - ref
>> That's way overcomplicated. Just items listing the names and minItems:
>> 1. See other bindings how this is done.
> OK.
>>> +
>>> +  power-domains:
>>> +    maxItems: 1
>> Is it relevant to existing device?
> 
> Will move to new variant only.
> 

Definition should be here, but in if:then: it should be disallowed
(:false) for other variants.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 01/11] dt-bindings: display: rockchip,inno-hdmi: Document RK3128 compatible

[Heiko Stübner]

Am Donnerstag, 14. Dezember 2023, 17:07:27 CET schrieb Krzysztof Kozlowski:
> On 14/12/2023 16:22, Alex Bee wrote:
> > 
> > Am 14.12.23 um 08:53 schrieb Krzysztof Kozlowski:
> >> On 13/12/2023 20:51, Alex Bee wrote:
> >>> Document the compatible for RK3128's HDMI controller block.
> >>> The integration for this SoC is somewhat different here: It needs the PHY's
> >> Please wrap commit message according to Linux coding style / submission
> >> process (neither too early nor over the limit):
> >> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597
> > OK. Not sure why checkpatch --strict  didn't tell me that I'm over the 
> > limit here.
> >>
> >>> reference clock rate to calculate the ddc bus frequency correctly. This
> >>> clock is part of a power-domain (PD_VIO), so this gets added as an optional
> >>> property too.
> >> If clock is part of power domain, then the power domain must be in the
> >> clock controller, not here. So either you put power domain in wrong
> >> place or you used incorrect reason for a change.
> >   Rockchip defines it's powerdomains per clock and I was little to much 
> > in that world when writing this. Actually the controller itself is part 
> > of the powerdomain. Will rephrase.
> 
> Does it mean you have like 200 different power domains in one SoC? Then
> how are they different than clock if there is one-to-one mapping?

It's more like the other way around. Controllers and their clocks belong
to specific power-domains. So there are of course more clocks than domains.





_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 01/11] dt-bindings: display: rockchip,inno-hdmi: Document RK3128 compatible

[Krzysztof Kozlowski]

On 14/12/2023 17:20, Heiko Stübner wrote:
> Am Donnerstag, 14. Dezember 2023, 17:07:27 CET schrieb Krzysztof Kozlowski:
>> On 14/12/2023 16:22, Alex Bee wrote:
>>>
>>> Am 14.12.23 um 08:53 schrieb Krzysztof Kozlowski:
>>>> On 13/12/2023 20:51, Alex Bee wrote:
>>>>> Document the compatible for RK3128's HDMI controller block.
>>>>> The integration for this SoC is somewhat different here: It needs the PHY's
>>>> Please wrap commit message according to Linux coding style / submission
>>>> process (neither too early nor over the limit):
>>>> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597
>>> OK. Not sure why checkpatch --strict  didn't tell me that I'm over the 
>>> limit here.
>>>>
>>>>> reference clock rate to calculate the ddc bus frequency correctly. This
>>>>> clock is part of a power-domain (PD_VIO), so this gets added as an optional
>>>>> property too.
>>>> If clock is part of power domain, then the power domain must be in the
>>>> clock controller, not here. So either you put power domain in wrong
>>>> place or you used incorrect reason for a change.
>>>   Rockchip defines it's powerdomains per clock and I was little to much 
>>> in that world when writing this. Actually the controller itself is part 
>>> of the powerdomain. Will rephrase.
>>
>> Does it mean you have like 200 different power domains in one SoC? Then
>> how are they different than clock if there is one-to-one mapping?
> 
> It's more like the other way around. Controllers and their clocks belong
> to specific power-domains. So there are of course more clocks than domains.

That's fine and expected. Here the comment was suggested that you need
to add power-domain because clock is in power-domain. That would be
clearly wrong and instead the clock controller should model the power
domain relationship.
> 
> 
> 
> 

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 02/11] drm/rockchip: vop: Add output selection registers for RK312x

[Alex Bee]

In contrast to RK3036, RK312x SoCs have multiple output channels such as
RGB (i.e. LVDS TTL), LVDS, DSI and HDMI.

In order to support that, this adds a new vop_output struct for rk3126_vop
with the registers required to enable the appropriate output and setup the
correct polarity.

Signed-off-by: Alex Bee <knaerzche@gmail.com>
---
 drivers/gpu/drm/rockchip/rockchip_vop_reg.c | 13 ++++++++++++-
 drivers/gpu/drm/rockchip/rockchip_vop_reg.h |  3 +++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
index c51ca82320cb..b9ee02061d5b 100644
--- a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
+++ b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
@@ -227,11 +227,22 @@ static const struct vop_win_data rk3126_vop_win_data[] = {
 	  .type = DRM_PLANE_TYPE_CURSOR },
 };
 
+static const struct vop_output rk3126_output = {
+	.pin_pol = VOP_REG(RK3036_DSP_CTRL0, 0xf, 4),
+	.hdmi_pin_pol = VOP_REG(RK3126_INT_SCALER, 0x7, 4),
+	.hdmi_en = VOP_REG(RK3036_AXI_BUS_CTRL, 0x1, 22),
+	.hdmi_dclk_pol = VOP_REG(RK3036_AXI_BUS_CTRL, 0x1, 23),
+	.rgb_en = VOP_REG(RK3036_AXI_BUS_CTRL, 0x1, 24),
+	.rgb_dclk_pol = VOP_REG(RK3036_AXI_BUS_CTRL, 0x1, 25),
+	.mipi_en = VOP_REG(RK3036_AXI_BUS_CTRL, 0x1, 28),
+	.mipi_dclk_pol = VOP_REG(RK3036_AXI_BUS_CTRL, 0x1, 29),
+};
+
 static const struct vop_data rk3126_vop = {
 	.intr = &rk3036_intr,
 	.common = &rk3036_common,
 	.modeset = &rk3036_modeset,
-	.output = &rk3036_output,
+	.output = &rk3126_output,
 	.win = rk3126_vop_win_data,
 	.win_size = ARRAY_SIZE(rk3126_vop_win_data),
 	.max_output = { 1920, 1080 },
diff --git a/drivers/gpu/drm/rockchip/rockchip_vop_reg.h b/drivers/gpu/drm/rockchip/rockchip_vop_reg.h
index 406e981c75bd..fbf1bcc68625 100644
--- a/drivers/gpu/drm/rockchip/rockchip_vop_reg.h
+++ b/drivers/gpu/drm/rockchip/rockchip_vop_reg.h
@@ -872,6 +872,9 @@
 /* rk3036 register definition end */
 
 /* rk3126 register definition */
+#define RK3126_INT_SCALER		0x0c
+
+/* win1 register */
 #define RK3126_WIN1_MST			0x4c
 #define RK3126_WIN1_DSP_INFO		0x50
 #define RK3126_WIN1_DSP_ST		0x54
-- 
2.43.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 03/11] drm/rockchip: inno_hdmi: Fix video timing

[Alex Bee]

The controller wants the difference between *total and *sync_start in
the HDMI_VIDEO_EXT_*DELAY registers. Otherwise the signal is very
unstable for certain non-VIC modes.
See downstream commit [0]

[0] https://github.com/rockchip-linux/kernel/commit/8eb559f2502c

Fixes: 412d4ae6b7a5 ("drm/rockchip: hdmi: add Innosilicon HDMI support")
Co-developed-by: Zheng Yang <zhengyang@rock-chips.com>
Signed-off-by: Zheng Yang <zhengyang@rock-chips.com>
Signed-off-by: Alex Bee <knaerzche@gmail.com>
---
 drivers/gpu/drm/rockchip/inno_hdmi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
index 6e5b922a121e..345253e033c5 100644
--- a/drivers/gpu/drm/rockchip/inno_hdmi.c
+++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
@@ -412,7 +412,7 @@ static int inno_hdmi_config_video_timing(struct inno_hdmi *hdmi,
 	hdmi_writeb(hdmi, HDMI_VIDEO_EXT_HBLANK_L, value & 0xFF);
 	hdmi_writeb(hdmi, HDMI_VIDEO_EXT_HBLANK_H, (value >> 8) & 0xFF);
 
-	value = mode->hsync_start - mode->hdisplay;
+	value = mode->htotal - mode->hsync_start;
 	hdmi_writeb(hdmi, HDMI_VIDEO_EXT_HDELAY_L, value & 0xFF);
 	hdmi_writeb(hdmi, HDMI_VIDEO_EXT_HDELAY_H, (value >> 8) & 0xFF);
 
@@ -427,7 +427,7 @@ static int inno_hdmi_config_video_timing(struct inno_hdmi *hdmi,
 	value = mode->vtotal - mode->vdisplay;
 	hdmi_writeb(hdmi, HDMI_VIDEO_EXT_VBLANK, value & 0xFF);
 
-	value = mode->vsync_start - mode->vdisplay;
+	value = mode->vtotal - mode->vsync_start;
 	hdmi_writeb(hdmi, HDMI_VIDEO_EXT_VDELAY, value & 0xFF);
 
 	value = mode->vsync_end - mode->vsync_start;
-- 
2.43.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 04/11] drm/rockchip: inno_hdmi: Correctly setup HDMI quantization range

[Alex Bee]

The display controller will always give full range RGB regardless of the
mode set, but HDMI requires certain modes to be transmitted in limited
range RGB. This is especially required for HDMI sinks which do not support
non-standard quantization ranges.

This enables color space conversion for those modes and sets the
quantization range accordingly in the AVI infoframe.

Fixes: 412d4ae6b7a5 ("drm/rockchip: hdmi: add Innosilicon HDMI support")
Signed-off-by: Alex Bee <knaerzche@gmail.com>
---
 drivers/gpu/drm/rockchip/inno_hdmi.c | 40 ++++++++++++++++++++++------
 1 file changed, 32 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
index 345253e033c5..32626a75723c 100644
--- a/drivers/gpu/drm/rockchip/inno_hdmi.c
+++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
@@ -33,6 +33,7 @@ struct hdmi_data_info {
 	unsigned int enc_in_format;
 	unsigned int enc_out_format;
 	unsigned int colorimetry;
+	bool rgb_limited_range;
 };
 
 struct inno_hdmi_i2c {
@@ -308,6 +309,18 @@ static int inno_hdmi_config_video_avi(struct inno_hdmi *hdmi,
 	else
 		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
 
+	if (hdmi->hdmi_data.enc_out_format == HDMI_COLORSPACE_RGB) {
+		drm_hdmi_avi_infoframe_quant_range(&frame.avi,
+						   &hdmi->connector, mode,
+						   hdmi->hdmi_data.rgb_limited_range ?
+						   HDMI_QUANTIZATION_RANGE_LIMITED :
+						   HDMI_QUANTIZATION_RANGE_FULL);
+	} else {
+		frame.avi.quantization_range = HDMI_QUANTIZATION_RANGE_DEFAULT;
+		frame.avi.ycc_quantization_range =
+			HDMI_YCC_QUANTIZATION_RANGE_LIMITED;
+	}
+
 	return inno_hdmi_upload_frame(hdmi, rc, &frame, INFOFRAME_AVI, 0, 0, 0);
 }
 
@@ -334,14 +347,22 @@ static int inno_hdmi_config_video_csc(struct inno_hdmi *hdmi)
 	if (data->enc_in_format == data->enc_out_format) {
 		if ((data->enc_in_format == HDMI_COLORSPACE_RGB) ||
 		    (data->enc_in_format >= HDMI_COLORSPACE_YUV444)) {
-			value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
-			hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
-
-			hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
-				  m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
-				  v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
-				  v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
-			return 0;
+			if (data->enc_in_format == HDMI_COLORSPACE_RGB &&
+			    data->enc_out_format == HDMI_COLORSPACE_RGB &&
+			    hdmi->hdmi_data.rgb_limited_range) {
+				csc_mode = CSC_RGB_0_255_TO_RGB_16_235_8BIT;
+				auto_csc = AUTO_CSC_DISABLE;
+				c0_c2_change = C0_C2_CHANGE_DISABLE;
+				csc_enable = v_CSC_ENABLE;
+			} else {
+				value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
+				hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
+				hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
+					  m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
+					  v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
+					  v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
+				return 0;
+			}
 		}
 	}
 
@@ -458,6 +479,9 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
 	else
 		hdmi->hdmi_data.colorimetry = HDMI_COLORIMETRY_ITU_709;
 
+	hdmi->hdmi_data.rgb_limited_range =
+		drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_LIMITED;
+
 	/* Mute video and audio output */
 	hdmi_modb(hdmi, HDMI_AV_MUTE, m_AUDIO_MUTE | m_VIDEO_BLACK,
 		  v_AUDIO_MUTE(1) | v_VIDEO_MUTE(1));
-- 
2.43.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 04/11] drm/rockchip: inno_hdmi: Correctly setup HDMI quantization range

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 3961 bytes --]

Hi Alex,

Thanks for working on this!

On Wed, Dec 13, 2023 at 08:51:18PM +0100, Alex Bee wrote:
> The display controller will always give full range RGB regardless of the
> mode set, but HDMI requires certain modes to be transmitted in limited
> range RGB. This is especially required for HDMI sinks which do not support
> non-standard quantization ranges.
> 
> This enables color space conversion for those modes and sets the
> quantization range accordingly in the AVI infoframe.
> 
> Fixes: 412d4ae6b7a5 ("drm/rockchip: hdmi: add Innosilicon HDMI support")
> Signed-off-by: Alex Bee <knaerzche@gmail.com>
> ---
>  drivers/gpu/drm/rockchip/inno_hdmi.c | 40 ++++++++++++++++++++++------
>  1 file changed, 32 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
> index 345253e033c5..32626a75723c 100644
> --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
> +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
> @@ -33,6 +33,7 @@ struct hdmi_data_info {
>  	unsigned int enc_in_format;
>  	unsigned int enc_out_format;
>  	unsigned int colorimetry;
> +	bool rgb_limited_range;
>  };
>  
>  struct inno_hdmi_i2c {
> @@ -308,6 +309,18 @@ static int inno_hdmi_config_video_avi(struct inno_hdmi *hdmi,
>  	else
>  		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
>  
> +	if (hdmi->hdmi_data.enc_out_format == HDMI_COLORSPACE_RGB) {
> +		drm_hdmi_avi_infoframe_quant_range(&frame.avi,
> +						   &hdmi->connector, mode,
> +						   hdmi->hdmi_data.rgb_limited_range ?
> +						   HDMI_QUANTIZATION_RANGE_LIMITED :
> +						   HDMI_QUANTIZATION_RANGE_FULL);
> +	} else {
> +		frame.avi.quantization_range = HDMI_QUANTIZATION_RANGE_DEFAULT;
> +		frame.avi.ycc_quantization_range =
> +			HDMI_YCC_QUANTIZATION_RANGE_LIMITED;
> +	}
> +
>  	return inno_hdmi_upload_frame(hdmi, rc, &frame, INFOFRAME_AVI, 0, 0, 0);
>  }
>  
> @@ -334,14 +347,22 @@ static int inno_hdmi_config_video_csc(struct inno_hdmi *hdmi)
>  	if (data->enc_in_format == data->enc_out_format) {
>  		if ((data->enc_in_format == HDMI_COLORSPACE_RGB) ||
>  		    (data->enc_in_format >= HDMI_COLORSPACE_YUV444)) {
> -			value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
> -			hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
> -
> -			hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
> -				  m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
> -				  v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
> -				  v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
> -			return 0;
> +			if (data->enc_in_format == HDMI_COLORSPACE_RGB &&
> +			    data->enc_out_format == HDMI_COLORSPACE_RGB &&
> +			    hdmi->hdmi_data.rgb_limited_range) {
> +				csc_mode = CSC_RGB_0_255_TO_RGB_16_235_8BIT;
> +				auto_csc = AUTO_CSC_DISABLE;
> +				c0_c2_change = C0_C2_CHANGE_DISABLE;
> +				csc_enable = v_CSC_ENABLE;
> +			} else {
> +				value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
> +				hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
> +				hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
> +					  m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
> +					  v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
> +					  v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
> +				return 0;
> +			}
>  		}
>  	}
>  
> @@ -458,6 +479,9 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
>  	else
>  		hdmi->hdmi_data.colorimetry = HDMI_COLORIMETRY_ITU_709;
>  
> +	hdmi->hdmi_data.rgb_limited_range =
> +		drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_LIMITED;
> +

This patch conflicts heavily with my inno_hdmi patches here (patches 22 to 38):
https://lore.kernel.org/dri-devel/20231207-kms-hdmi-connector-state-v5-0-6538e19d634d@kernel.org/

I would appreciate if you could test and merge them into your series.

In particular, there's no need to store the range here: enc_out_format
is always RGB, so you'll always end up calling
drm_hdmi_avi_infoframe_quant_range(), and you'll always have the same csc values.

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 04/11] drm/rockchip: inno_hdmi: Correctly setup HDMI quantization range

[Alex Bee]

Hi Maxime

Am 14.12.23 um 08:56 schrieb Maxime Ripard:
> Hi Alex,
>
> Thanks for working on this!
>
> On Wed, Dec 13, 2023 at 08:51:18PM +0100, Alex Bee wrote:
>> The display controller will always give full range RGB regardless of the
>> mode set, but HDMI requires certain modes to be transmitted in limited
>> range RGB. This is especially required for HDMI sinks which do not support
>> non-standard quantization ranges.
>>
>> This enables color space conversion for those modes and sets the
>> quantization range accordingly in the AVI infoframe.
>>
>> Fixes: 412d4ae6b7a5 ("drm/rockchip: hdmi: add Innosilicon HDMI support")
>> Signed-off-by: Alex Bee <knaerzche@gmail.com>
>> ---
>>   drivers/gpu/drm/rockchip/inno_hdmi.c | 40 ++++++++++++++++++++++------
>>   1 file changed, 32 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
>> index 345253e033c5..32626a75723c 100644
>> --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
>> +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
>> @@ -33,6 +33,7 @@ struct hdmi_data_info {
>>   	unsigned int enc_in_format;
>>   	unsigned int enc_out_format;
>>   	unsigned int colorimetry;
>> +	bool rgb_limited_range;
>>   };
>>   
>>   struct inno_hdmi_i2c {
>> @@ -308,6 +309,18 @@ static int inno_hdmi_config_video_avi(struct inno_hdmi *hdmi,
>>   	else
>>   		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
>>   
>> +	if (hdmi->hdmi_data.enc_out_format == HDMI_COLORSPACE_RGB) {
>> +		drm_hdmi_avi_infoframe_quant_range(&frame.avi,
>> +						   &hdmi->connector, mode,
>> +						   hdmi->hdmi_data.rgb_limited_range ?
>> +						   HDMI_QUANTIZATION_RANGE_LIMITED :
>> +						   HDMI_QUANTIZATION_RANGE_FULL);
>> +	} else {
>> +		frame.avi.quantization_range = HDMI_QUANTIZATION_RANGE_DEFAULT;
>> +		frame.avi.ycc_quantization_range =
>> +			HDMI_YCC_QUANTIZATION_RANGE_LIMITED;
>> +	}
>> +
>>   	return inno_hdmi_upload_frame(hdmi, rc, &frame, INFOFRAME_AVI, 0, 0, 0);
>>   }
>>   
>> @@ -334,14 +347,22 @@ static int inno_hdmi_config_video_csc(struct inno_hdmi *hdmi)
>>   	if (data->enc_in_format == data->enc_out_format) {
>>   		if ((data->enc_in_format == HDMI_COLORSPACE_RGB) ||
>>   		    (data->enc_in_format >= HDMI_COLORSPACE_YUV444)) {
>> -			value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
>> -			hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
>> -
>> -			hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
>> -				  m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
>> -				  v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
>> -				  v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
>> -			return 0;
>> +			if (data->enc_in_format == HDMI_COLORSPACE_RGB &&
>> +			    data->enc_out_format == HDMI_COLORSPACE_RGB &&
>> +			    hdmi->hdmi_data.rgb_limited_range) {
>> +				csc_mode = CSC_RGB_0_255_TO_RGB_16_235_8BIT;
>> +				auto_csc = AUTO_CSC_DISABLE;
>> +				c0_c2_change = C0_C2_CHANGE_DISABLE;
>> +				csc_enable = v_CSC_ENABLE;
>> +			} else {
>> +				value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
>> +				hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
>> +				hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
>> +					  m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
>> +					  v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
>> +					  v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
>> +				return 0;
>> +			}
>>   		}
>>   	}
>>   
>> @@ -458,6 +479,9 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
>>   	else
>>   		hdmi->hdmi_data.colorimetry = HDMI_COLORIMETRY_ITU_709;
>>   
>> +	hdmi->hdmi_data.rgb_limited_range =
>> +		drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_LIMITED;
>> +
> This patch conflicts heavily with my inno_hdmi patches here (patches 22 to 38):
> https://lore.kernel.org/dri-devel/20231207-kms-hdmi-connector-state-v5-0-6538e19d634d@kernel.org/
I'm aware of that and I mentioned it in the cover letter. Your series is 
not merged yet and it didn't get much feedback so far. What is the 
status there? Especially because you are removing things from inno-hdmi 
driver (which aren't really required to remove there) which will I have 
to reintroduce.

> I would appreciate if you could test and merge them into your series.
>
> In particular, there's no need to store the range here: enc_out_format
rgb_limited_range is currently not only used for csc, but also for for 
infoframe creation. So it makes sense to have this stored  to avoid 
calling drm_default_rgb_quant_range twice.

> is always RGB, so you'll always end up calling
> drm_hdmi_avi_infoframe_quant_range(), and you'll always have the same csc values.

I don't think that's true. Non-VIC modes like 800x600 or 1024x764 want 
full range RGB and csc must not be done in those cases.

Alex

>
> Maxime

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 04/11] drm/rockchip: inno_hdmi: Correctly setup HDMI quantization range

[Heiko Stübner]

Am Donnerstag, 14. Dezember 2023, 12:12:08 CET schrieb Alex Bee:
> Hi Maxime
> 
> Am 14.12.23 um 08:56 schrieb Maxime Ripard:
> > Hi Alex,
> >
> > Thanks for working on this!
> >
> > On Wed, Dec 13, 2023 at 08:51:18PM +0100, Alex Bee wrote:
> >> The display controller will always give full range RGB regardless of the
> >> mode set, but HDMI requires certain modes to be transmitted in limited
> >> range RGB. This is especially required for HDMI sinks which do not support
> >> non-standard quantization ranges.
> >>
> >> This enables color space conversion for those modes and sets the
> >> quantization range accordingly in the AVI infoframe.
> >>
> >> Fixes: 412d4ae6b7a5 ("drm/rockchip: hdmi: add Innosilicon HDMI support")
> >> Signed-off-by: Alex Bee <knaerzche@gmail.com>
> >> ---
> >>   drivers/gpu/drm/rockchip/inno_hdmi.c | 40 ++++++++++++++++++++++------
> >>   1 file changed, 32 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
> >> index 345253e033c5..32626a75723c 100644
> >> --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
> >> +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
> >> @@ -33,6 +33,7 @@ struct hdmi_data_info {
> >>   	unsigned int enc_in_format;
> >>   	unsigned int enc_out_format;
> >>   	unsigned int colorimetry;
> >> +	bool rgb_limited_range;
> >>   };
> >>   
> >>   struct inno_hdmi_i2c {
> >> @@ -308,6 +309,18 @@ static int inno_hdmi_config_video_avi(struct inno_hdmi *hdmi,
> >>   	else
> >>   		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
> >>   
> >> +	if (hdmi->hdmi_data.enc_out_format == HDMI_COLORSPACE_RGB) {
> >> +		drm_hdmi_avi_infoframe_quant_range(&frame.avi,
> >> +						   &hdmi->connector, mode,
> >> +						   hdmi->hdmi_data.rgb_limited_range ?
> >> +						   HDMI_QUANTIZATION_RANGE_LIMITED :
> >> +						   HDMI_QUANTIZATION_RANGE_FULL);
> >> +	} else {
> >> +		frame.avi.quantization_range = HDMI_QUANTIZATION_RANGE_DEFAULT;
> >> +		frame.avi.ycc_quantization_range =
> >> +			HDMI_YCC_QUANTIZATION_RANGE_LIMITED;
> >> +	}
> >> +
> >>   	return inno_hdmi_upload_frame(hdmi, rc, &frame, INFOFRAME_AVI, 0, 0, 0);
> >>   }
> >>   
> >> @@ -334,14 +347,22 @@ static int inno_hdmi_config_video_csc(struct inno_hdmi *hdmi)
> >>   	if (data->enc_in_format == data->enc_out_format) {
> >>   		if ((data->enc_in_format == HDMI_COLORSPACE_RGB) ||
> >>   		    (data->enc_in_format >= HDMI_COLORSPACE_YUV444)) {
> >> -			value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
> >> -			hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
> >> -
> >> -			hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
> >> -				  m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
> >> -				  v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
> >> -				  v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
> >> -			return 0;
> >> +			if (data->enc_in_format == HDMI_COLORSPACE_RGB &&
> >> +			    data->enc_out_format == HDMI_COLORSPACE_RGB &&
> >> +			    hdmi->hdmi_data.rgb_limited_range) {
> >> +				csc_mode = CSC_RGB_0_255_TO_RGB_16_235_8BIT;
> >> +				auto_csc = AUTO_CSC_DISABLE;
> >> +				c0_c2_change = C0_C2_CHANGE_DISABLE;
> >> +				csc_enable = v_CSC_ENABLE;
> >> +			} else {
> >> +				value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
> >> +				hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
> >> +				hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
> >> +					  m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
> >> +					  v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
> >> +					  v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
> >> +				return 0;
> >> +			}
> >>   		}
> >>   	}
> >>   
> >> @@ -458,6 +479,9 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
> >>   	else
> >>   		hdmi->hdmi_data.colorimetry = HDMI_COLORIMETRY_ITU_709;
> >>   
> >> +	hdmi->hdmi_data.rgb_limited_range =
> >> +		drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_LIMITED;
> >> +
> > This patch conflicts heavily with my inno_hdmi patches here (patches 22 to 38):
> > https://lore.kernel.org/dri-devel/20231207-kms-hdmi-connector-state-v5-0-6538e19d634d@kernel.org/
> I'm aware of that and I mentioned it in the cover letter. Your series is 
> not merged yet and it didn't get much feedback so far. What is the 
> status there? Especially because you are removing things from inno-hdmi 
> driver (which aren't really required to remove there) which will I have 
> to reintroduce.

Sadly I haven't found the time to look closer at Maxime's series so far,
but I got the impression that it separates into multiple cleanup steps
for a number of controllers.

So the sentence below suggests that Maxime wanted you to pick the
appropriate patches from there and incorporate them into your series
(as it looks like you developed a nice working knowledge of the inno-hdmi
driver). So there is no need to first drop and then reintroduce stuff, but
there may be other interesting cleanups there.

> > I would appreciate if you could test and merge them into your series.


Heiko

> > In particular, there's no need to store the range here: enc_out_format
> rgb_limited_range is currently not only used for csc, but also for for 
> infoframe creation. So it makes sense to have this stored  to avoid 
> calling drm_default_rgb_quant_range twice.
> 
> > is always RGB, so you'll always end up calling
> > drm_hdmi_avi_infoframe_quant_range(), and you'll always have the same csc values.
> 
> I don't think that's true. Non-VIC modes like 800x600 or 1024x764 want 
> full range RGB and csc must not be done in those cases.





_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 04/11] drm/rockchip: inno_hdmi: Correctly setup HDMI quantization range

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 5180 bytes --]

On Thu, Dec 14, 2023 at 12:17:34PM +0100, Heiko Stübner wrote:
> Am Donnerstag, 14. Dezember 2023, 12:12:08 CET schrieb Alex Bee:
> > Hi Maxime
> > 
> > Am 14.12.23 um 08:56 schrieb Maxime Ripard:
> > > Hi Alex,
> > >
> > > Thanks for working on this!
> > >
> > > On Wed, Dec 13, 2023 at 08:51:18PM +0100, Alex Bee wrote:
> > >> The display controller will always give full range RGB regardless of the
> > >> mode set, but HDMI requires certain modes to be transmitted in limited
> > >> range RGB. This is especially required for HDMI sinks which do not support
> > >> non-standard quantization ranges.
> > >>
> > >> This enables color space conversion for those modes and sets the
> > >> quantization range accordingly in the AVI infoframe.
> > >>
> > >> Fixes: 412d4ae6b7a5 ("drm/rockchip: hdmi: add Innosilicon HDMI support")
> > >> Signed-off-by: Alex Bee <knaerzche@gmail.com>
> > >> ---
> > >>   drivers/gpu/drm/rockchip/inno_hdmi.c | 40 ++++++++++++++++++++++------
> > >>   1 file changed, 32 insertions(+), 8 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
> > >> index 345253e033c5..32626a75723c 100644
> > >> --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
> > >> +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
> > >> @@ -33,6 +33,7 @@ struct hdmi_data_info {
> > >>   	unsigned int enc_in_format;
> > >>   	unsigned int enc_out_format;
> > >>   	unsigned int colorimetry;
> > >> +	bool rgb_limited_range;
> > >>   };
> > >>   
> > >>   struct inno_hdmi_i2c {
> > >> @@ -308,6 +309,18 @@ static int inno_hdmi_config_video_avi(struct inno_hdmi *hdmi,
> > >>   	else
> > >>   		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
> > >>   
> > >> +	if (hdmi->hdmi_data.enc_out_format == HDMI_COLORSPACE_RGB) {
> > >> +		drm_hdmi_avi_infoframe_quant_range(&frame.avi,
> > >> +						   &hdmi->connector, mode,
> > >> +						   hdmi->hdmi_data.rgb_limited_range ?
> > >> +						   HDMI_QUANTIZATION_RANGE_LIMITED :
> > >> +						   HDMI_QUANTIZATION_RANGE_FULL);
> > >> +	} else {
> > >> +		frame.avi.quantization_range = HDMI_QUANTIZATION_RANGE_DEFAULT;
> > >> +		frame.avi.ycc_quantization_range =
> > >> +			HDMI_YCC_QUANTIZATION_RANGE_LIMITED;
> > >> +	}
> > >> +
> > >>   	return inno_hdmi_upload_frame(hdmi, rc, &frame, INFOFRAME_AVI, 0, 0, 0);
> > >>   }
> > >>   
> > >> @@ -334,14 +347,22 @@ static int inno_hdmi_config_video_csc(struct inno_hdmi *hdmi)
> > >>   	if (data->enc_in_format == data->enc_out_format) {
> > >>   		if ((data->enc_in_format == HDMI_COLORSPACE_RGB) ||
> > >>   		    (data->enc_in_format >= HDMI_COLORSPACE_YUV444)) {
> > >> -			value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
> > >> -			hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
> > >> -
> > >> -			hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
> > >> -				  m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
> > >> -				  v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
> > >> -				  v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
> > >> -			return 0;
> > >> +			if (data->enc_in_format == HDMI_COLORSPACE_RGB &&
> > >> +			    data->enc_out_format == HDMI_COLORSPACE_RGB &&
> > >> +			    hdmi->hdmi_data.rgb_limited_range) {
> > >> +				csc_mode = CSC_RGB_0_255_TO_RGB_16_235_8BIT;
> > >> +				auto_csc = AUTO_CSC_DISABLE;
> > >> +				c0_c2_change = C0_C2_CHANGE_DISABLE;
> > >> +				csc_enable = v_CSC_ENABLE;
> > >> +			} else {
> > >> +				value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
> > >> +				hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
> > >> +				hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
> > >> +					  m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
> > >> +					  v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
> > >> +					  v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
> > >> +				return 0;
> > >> +			}
> > >>   		}
> > >>   	}
> > >>   
> > >> @@ -458,6 +479,9 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
> > >>   	else
> > >>   		hdmi->hdmi_data.colorimetry = HDMI_COLORIMETRY_ITU_709;
> > >>   
> > >> +	hdmi->hdmi_data.rgb_limited_range =
> > >> +		drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_LIMITED;
> > >> +
> > > This patch conflicts heavily with my inno_hdmi patches here (patches 22 to 38):
> > > https://lore.kernel.org/dri-devel/20231207-kms-hdmi-connector-state-v5-0-6538e19d634d@kernel.org/
> > I'm aware of that and I mentioned it in the cover letter. Your series is 
> > not merged yet and it didn't get much feedback so far. What is the 
> > status there? Especially because you are removing things from inno-hdmi 
> > driver (which aren't really required to remove there) which will I have 
> > to reintroduce.
> 
> Sadly I haven't found the time to look closer at Maxime's series so far,
> but I got the impression that it separates into multiple cleanup steps
> for a number of controllers.

Yeah, one of the previous version comment was to support more
controllers than vc4, which is fair. So I ended up reworking and
converting multiple controllers, but most of the clean up changes can be
applied outside of that series just fine.

I just didn't find someone to test / review them yet :)

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re:Re: [PATCH 04/11] drm/rockchip: inno_hdmi: Correctly setup HDMI quantization range

[Andy Yan]

Hi：

在 2023-12-14 19:36:05，"Maxime Ripard" <mripard@kernel.org> 写道：
>On Thu, Dec 14, 2023 at 12:17:34PM +0100, Heiko Stübner wrote:
>> Am Donnerstag, 14. Dezember 2023, 12:12:08 CET schrieb Alex Bee:
>> > Hi Maxime
>> > 
>> > Am 14.12.23 um 08:56 schrieb Maxime Ripard:
>> > > Hi Alex,
>> > >
>> > > Thanks for working on this!
>> > >
>> > > On Wed, Dec 13, 2023 at 08:51:18PM +0100, Alex Bee wrote:
>> > >> The display controller will always give full range RGB regardless of the
>> > >> mode set, but HDMI requires certain modes to be transmitted in limited
>> > >> range RGB. This is especially required for HDMI sinks which do not support
>> > >> non-standard quantization ranges.
>> > >>
>> > >> This enables color space conversion for those modes and sets the
>> > >> quantization range accordingly in the AVI infoframe.
>> > >>
>> > >> Fixes: 412d4ae6b7a5 ("drm/rockchip: hdmi: add Innosilicon HDMI support")
>> > >> Signed-off-by: Alex Bee <knaerzche@gmail.com>
>> > >> ---
>> > >>   drivers/gpu/drm/rockchip/inno_hdmi.c | 40 ++++++++++++++++++++++------
>> > >>   1 file changed, 32 insertions(+), 8 deletions(-)
>> > >>
>> > >> diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
>> > >> index 345253e033c5..32626a75723c 100644
>> > >> --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
>> > >> +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
>> > >> @@ -33,6 +33,7 @@ struct hdmi_data_info {
>> > >>   	unsigned int enc_in_format;
>> > >>   	unsigned int enc_out_format;
>> > >>   	unsigned int colorimetry;
>> > >> +	bool rgb_limited_range;
>> > >>   };
>> > >>   
>> > >>   struct inno_hdmi_i2c {
>> > >> @@ -308,6 +309,18 @@ static int inno_hdmi_config_video_avi(struct inno_hdmi *hdmi,
>> > >>   	else
>> > >>   		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
>> > >>   
>> > >> +	if (hdmi->hdmi_data.enc_out_format == HDMI_COLORSPACE_RGB) {
>> > >> +		drm_hdmi_avi_infoframe_quant_range(&frame.avi,
>> > >> +						   &hdmi->connector, mode,
>> > >> +						   hdmi->hdmi_data.rgb_limited_range ?
>> > >> +						   HDMI_QUANTIZATION_RANGE_LIMITED :
>> > >> +						   HDMI_QUANTIZATION_RANGE_FULL);
>> > >> +	} else {
>> > >> +		frame.avi.quantization_range = HDMI_QUANTIZATION_RANGE_DEFAULT;
>> > >> +		frame.avi.ycc_quantization_range =
>> > >> +			HDMI_YCC_QUANTIZATION_RANGE_LIMITED;
>> > >> +	}
>> > >> +
>> > >>   	return inno_hdmi_upload_frame(hdmi, rc, &frame, INFOFRAME_AVI, 0, 0, 0);
>> > >>   }
>> > >>   
>> > >> @@ -334,14 +347,22 @@ static int inno_hdmi_config_video_csc(struct inno_hdmi *hdmi)
>> > >>   	if (data->enc_in_format == data->enc_out_format) {
>> > >>   		if ((data->enc_in_format == HDMI_COLORSPACE_RGB) ||
>> > >>   		    (data->enc_in_format >= HDMI_COLORSPACE_YUV444)) {
>> > >> -			value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
>> > >> -			hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
>> > >> -
>> > >> -			hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
>> > >> -				  m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
>> > >> -				  v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
>> > >> -				  v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
>> > >> -			return 0;
>> > >> +			if (data->enc_in_format == HDMI_COLORSPACE_RGB &&
>> > >> +			    data->enc_out_format == HDMI_COLORSPACE_RGB &&
>> > >> +			    hdmi->hdmi_data.rgb_limited_range) {
>> > >> +				csc_mode = CSC_RGB_0_255_TO_RGB_16_235_8BIT;
>> > >> +				auto_csc = AUTO_CSC_DISABLE;
>> > >> +				c0_c2_change = C0_C2_CHANGE_DISABLE;
>> > >> +				csc_enable = v_CSC_ENABLE;
>> > >> +			} else {
>> > >> +				value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
>> > >> +				hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
>> > >> +				hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
>> > >> +					  m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
>> > >> +					  v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
>> > >> +					  v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
>> > >> +				return 0;
>> > >> +			}
>> > >>   		}
>> > >>   	}
>> > >>   
>> > >> @@ -458,6 +479,9 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
>> > >>   	else
>> > >>   		hdmi->hdmi_data.colorimetry = HDMI_COLORIMETRY_ITU_709;
>> > >>   
>> > >> +	hdmi->hdmi_data.rgb_limited_range =
>> > >> +		drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_LIMITED;
>> > >> +
>> > > This patch conflicts heavily with my inno_hdmi patches here (patches 22 to 38):
>> > > https://lore.kernel.org/dri-devel/20231207-kms-hdmi-connector-state-v5-0-6538e19d634d@kernel.org/
>> > I'm aware of that and I mentioned it in the cover letter. Your series is 
>> > not merged yet and it didn't get much feedback so far. What is the 
>> > status there? Especially because you are removing things from inno-hdmi 
>> > driver (which aren't really required to remove there) which will I have 
>> > to reintroduce.
>> 
>> Sadly I haven't found the time to look closer at Maxime's series so far,
>> but I got the impression that it separates into multiple cleanup steps
>> for a number of controllers.
>
>Yeah, one of the previous version comment was to support more
>controllers than vc4, which is fair. So I ended up reworking and
>converting multiple controllers, but most of the clean up changes can be
>applied outside of that series just fine.
>
>I just didn't find someone to test / review them yet :)

I will try to bring up my rk3036 kylin board whith mainline kernel this weekend， then I can do some tests.

>
>Maxime
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 04/11] drm/rockchip: inno_hdmi: Correctly setup HDMI quantization range

[Alex Bee]

Hi Heiko, Hi Maxime,

Am 14.12.23 um 12:36 schrieb Maxime Ripard:
> On Thu, Dec 14, 2023 at 12:17:34PM +0100, Heiko Stübner wrote:
>> Am Donnerstag, 14. Dezember 2023, 12:12:08 CET schrieb Alex Bee:
>>> Hi Maxime
>>>
>>> Am 14.12.23 um 08:56 schrieb Maxime Ripard:
>>>> Hi Alex,
>>>>
>>>> Thanks for working on this!
>>>>
>>>> On Wed, Dec 13, 2023 at 08:51:18PM +0100, Alex Bee wrote:
>>>>> The display controller will always give full range RGB regardless of the
>>>>> mode set, but HDMI requires certain modes to be transmitted in limited
>>>>> range RGB. This is especially required for HDMI sinks which do not support
>>>>> non-standard quantization ranges.
>>>>>
>>>>> This enables color space conversion for those modes and sets the
>>>>> quantization range accordingly in the AVI infoframe.
>>>>>
>>>>> Fixes: 412d4ae6b7a5 ("drm/rockchip: hdmi: add Innosilicon HDMI support")
>>>>> Signed-off-by: Alex Bee <knaerzche@gmail.com>
>>>>> ---
>>>>>    drivers/gpu/drm/rockchip/inno_hdmi.c | 40 ++++++++++++++++++++++------
>>>>>    1 file changed, 32 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
>>>>> index 345253e033c5..32626a75723c 100644
>>>>> --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
>>>>> +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
>>>>> @@ -33,6 +33,7 @@ struct hdmi_data_info {
>>>>>    	unsigned int enc_in_format;
>>>>>    	unsigned int enc_out_format;
>>>>>    	unsigned int colorimetry;
>>>>> +	bool rgb_limited_range;
>>>>>    };
>>>>>    
>>>>>    struct inno_hdmi_i2c {
>>>>> @@ -308,6 +309,18 @@ static int inno_hdmi_config_video_avi(struct inno_hdmi *hdmi,
>>>>>    	else
>>>>>    		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
>>>>>    
>>>>> +	if (hdmi->hdmi_data.enc_out_format == HDMI_COLORSPACE_RGB) {
>>>>> +		drm_hdmi_avi_infoframe_quant_range(&frame.avi,
>>>>> +						   &hdmi->connector, mode,
>>>>> +						   hdmi->hdmi_data.rgb_limited_range ?
>>>>> +						   HDMI_QUANTIZATION_RANGE_LIMITED :
>>>>> +						   HDMI_QUANTIZATION_RANGE_FULL);
>>>>> +	} else {
>>>>> +		frame.avi.quantization_range = HDMI_QUANTIZATION_RANGE_DEFAULT;
>>>>> +		frame.avi.ycc_quantization_range =
>>>>> +			HDMI_YCC_QUANTIZATION_RANGE_LIMITED;
>>>>> +	}
>>>>> +
>>>>>    	return inno_hdmi_upload_frame(hdmi, rc, &frame, INFOFRAME_AVI, 0, 0, 0);
>>>>>    }
>>>>>    
>>>>> @@ -334,14 +347,22 @@ static int inno_hdmi_config_video_csc(struct inno_hdmi *hdmi)
>>>>>    	if (data->enc_in_format == data->enc_out_format) {
>>>>>    		if ((data->enc_in_format == HDMI_COLORSPACE_RGB) ||
>>>>>    		    (data->enc_in_format >= HDMI_COLORSPACE_YUV444)) {
>>>>> -			value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
>>>>> -			hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
>>>>> -
>>>>> -			hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
>>>>> -				  m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
>>>>> -				  v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
>>>>> -				  v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
>>>>> -			return 0;
>>>>> +			if (data->enc_in_format == HDMI_COLORSPACE_RGB &&
>>>>> +			    data->enc_out_format == HDMI_COLORSPACE_RGB &&
>>>>> +			    hdmi->hdmi_data.rgb_limited_range) {
>>>>> +				csc_mode = CSC_RGB_0_255_TO_RGB_16_235_8BIT;
>>>>> +				auto_csc = AUTO_CSC_DISABLE;
>>>>> +				c0_c2_change = C0_C2_CHANGE_DISABLE;
>>>>> +				csc_enable = v_CSC_ENABLE;
>>>>> +			} else {
>>>>> +				value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
>>>>> +				hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
>>>>> +				hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
>>>>> +					  m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
>>>>> +					  v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
>>>>> +					  v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
>>>>> +				return 0;
>>>>> +			}
>>>>>    		}
>>>>>    	}
>>>>>    
>>>>> @@ -458,6 +479,9 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
>>>>>    	else
>>>>>    		hdmi->hdmi_data.colorimetry = HDMI_COLORIMETRY_ITU_709;
>>>>>    
>>>>> +	hdmi->hdmi_data.rgb_limited_range =
>>>>> +		drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_LIMITED;
>>>>> +
>>>> This patch conflicts heavily with my inno_hdmi patches here (patches 22 to 38):
>>>> https://lore.kernel.org/dri-devel/20231207-kms-hdmi-connector-state-v5-0-6538e19d634d@kernel.org/
>>> I'm aware of that and I mentioned it in the cover letter. Your series is
>>> not merged yet and it didn't get much feedback so far. What is the
>>> status there? Especially because you are removing things from inno-hdmi
>>> driver (which aren't really required to remove there) which will I have
>>> to reintroduce.
>> Sadly I haven't found the time to look closer at Maxime's series so far,
>> but I got the impression that it separates into multiple cleanup steps
>> for a number of controllers.
> Yeah, one of the previous version comment was to support more
> controllers than vc4, which is fair. So I ended up reworking and
> converting multiple controllers, but most of the clean up changes can be
> applied outside of that series just fine.
>
> I just didn't find someone to test / review them yet :)

I'm not exactly sure how to proceed here. Do you want me to:

- base my patches on top of Maxime's series and reintroduce csc things 
(of course only those which touch inno-hdmi w/o the framwork changes)

- only apply the patches that do not touch csc things and base my 
series  on top of that

- adapt Maxime's patches so that RGB full to RGB limited stays in there

- wait with resend until Maxime's series is merged and reintroduce csc 
after that

- something else

?

Please advise.

Thanks,

Alex

> Maxime

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 04/11] drm/rockchip: inno_hdmi: Correctly setup HDMI quantization range

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 6318 bytes --]

On Thu, Dec 14, 2023 at 05:34:59PM +0100, Alex Bee wrote:
> Hi Heiko, Hi Maxime,
> 
> Am 14.12.23 um 12:36 schrieb Maxime Ripard:
> > On Thu, Dec 14, 2023 at 12:17:34PM +0100, Heiko Stübner wrote:
> > > Am Donnerstag, 14. Dezember 2023, 12:12:08 CET schrieb Alex Bee:
> > > > Hi Maxime
> > > > 
> > > > Am 14.12.23 um 08:56 schrieb Maxime Ripard:
> > > > > Hi Alex,
> > > > > 
> > > > > Thanks for working on this!
> > > > > 
> > > > > On Wed, Dec 13, 2023 at 08:51:18PM +0100, Alex Bee wrote:
> > > > > > The display controller will always give full range RGB regardless of the
> > > > > > mode set, but HDMI requires certain modes to be transmitted in limited
> > > > > > range RGB. This is especially required for HDMI sinks which do not support
> > > > > > non-standard quantization ranges.
> > > > > > 
> > > > > > This enables color space conversion for those modes and sets the
> > > > > > quantization range accordingly in the AVI infoframe.
> > > > > > 
> > > > > > Fixes: 412d4ae6b7a5 ("drm/rockchip: hdmi: add Innosilicon HDMI support")
> > > > > > Signed-off-by: Alex Bee <knaerzche@gmail.com>
> > > > > > ---
> > > > > >    drivers/gpu/drm/rockchip/inno_hdmi.c | 40 ++++++++++++++++++++++------
> > > > > >    1 file changed, 32 insertions(+), 8 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
> > > > > > index 345253e033c5..32626a75723c 100644
> > > > > > --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
> > > > > > +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
> > > > > > @@ -33,6 +33,7 @@ struct hdmi_data_info {
> > > > > >    	unsigned int enc_in_format;
> > > > > >    	unsigned int enc_out_format;
> > > > > >    	unsigned int colorimetry;
> > > > > > +	bool rgb_limited_range;
> > > > > >    };
> > > > > >    struct inno_hdmi_i2c {
> > > > > > @@ -308,6 +309,18 @@ static int inno_hdmi_config_video_avi(struct inno_hdmi *hdmi,
> > > > > >    	else
> > > > > >    		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
> > > > > > +	if (hdmi->hdmi_data.enc_out_format == HDMI_COLORSPACE_RGB) {
> > > > > > +		drm_hdmi_avi_infoframe_quant_range(&frame.avi,
> > > > > > +						   &hdmi->connector, mode,
> > > > > > +						   hdmi->hdmi_data.rgb_limited_range ?
> > > > > > +						   HDMI_QUANTIZATION_RANGE_LIMITED :
> > > > > > +						   HDMI_QUANTIZATION_RANGE_FULL);
> > > > > > +	} else {
> > > > > > +		frame.avi.quantization_range = HDMI_QUANTIZATION_RANGE_DEFAULT;
> > > > > > +		frame.avi.ycc_quantization_range =
> > > > > > +			HDMI_YCC_QUANTIZATION_RANGE_LIMITED;
> > > > > > +	}
> > > > > > +
> > > > > >    	return inno_hdmi_upload_frame(hdmi, rc, &frame, INFOFRAME_AVI, 0, 0, 0);
> > > > > >    }
> > > > > > @@ -334,14 +347,22 @@ static int inno_hdmi_config_video_csc(struct inno_hdmi *hdmi)
> > > > > >    	if (data->enc_in_format == data->enc_out_format) {
> > > > > >    		if ((data->enc_in_format == HDMI_COLORSPACE_RGB) ||
> > > > > >    		    (data->enc_in_format >= HDMI_COLORSPACE_YUV444)) {
> > > > > > -			value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
> > > > > > -			hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
> > > > > > -
> > > > > > -			hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
> > > > > > -				  m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
> > > > > > -				  v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
> > > > > > -				  v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
> > > > > > -			return 0;
> > > > > > +			if (data->enc_in_format == HDMI_COLORSPACE_RGB &&
> > > > > > +			    data->enc_out_format == HDMI_COLORSPACE_RGB &&
> > > > > > +			    hdmi->hdmi_data.rgb_limited_range) {
> > > > > > +				csc_mode = CSC_RGB_0_255_TO_RGB_16_235_8BIT;
> > > > > > +				auto_csc = AUTO_CSC_DISABLE;
> > > > > > +				c0_c2_change = C0_C2_CHANGE_DISABLE;
> > > > > > +				csc_enable = v_CSC_ENABLE;
> > > > > > +			} else {
> > > > > > +				value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
> > > > > > +				hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
> > > > > > +				hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
> > > > > > +					  m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
> > > > > > +					  v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
> > > > > > +					  v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
> > > > > > +				return 0;
> > > > > > +			}
> > > > > >    		}
> > > > > >    	}
> > > > > > @@ -458,6 +479,9 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
> > > > > >    	else
> > > > > >    		hdmi->hdmi_data.colorimetry = HDMI_COLORIMETRY_ITU_709;
> > > > > > +	hdmi->hdmi_data.rgb_limited_range =
> > > > > > +		drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_LIMITED;
> > > > > > +
> > > > > This patch conflicts heavily with my inno_hdmi patches here (patches 22 to 38):
> > > > > https://lore.kernel.org/dri-devel/20231207-kms-hdmi-connector-state-v5-0-6538e19d634d@kernel.org/
> > > > I'm aware of that and I mentioned it in the cover letter. Your series is
> > > > not merged yet and it didn't get much feedback so far. What is the
> > > > status there? Especially because you are removing things from inno-hdmi
> > > > driver (which aren't really required to remove there) which will I have
> > > > to reintroduce.
> > > Sadly I haven't found the time to look closer at Maxime's series so far,
> > > but I got the impression that it separates into multiple cleanup steps
> > > for a number of controllers.
> > Yeah, one of the previous version comment was to support more
> > controllers than vc4, which is fair. So I ended up reworking and
> > converting multiple controllers, but most of the clean up changes can be
> > applied outside of that series just fine.
> > 
> > I just didn't find someone to test / review them yet :)
> 
> I'm not exactly sure how to proceed here. Do you want me to:
> 
> - base my patches on top of Maxime's series and reintroduce csc things (of
> course only those which touch inno-hdmi w/o the framwork changes)
> 
> - only apply the patches that do not touch csc things and base my series  on
> top of that
> 
> - adapt Maxime's patches so that RGB full to RGB limited stays in there
> 
> - wait with resend until Maxime's series is merged and reintroduce csc after
> that
> 
> - something else
> 
> ?

2 or 3, at your discretion

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 04/11] drm/rockchip: inno_hdmi: Correctly setup HDMI quantization range

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 5510 bytes --]

On Thu, Dec 14, 2023 at 12:12:08PM +0100, Alex Bee wrote:
> Hi Maxime
> 
> Am 14.12.23 um 08:56 schrieb Maxime Ripard:
> > Hi Alex,
> > 
> > Thanks for working on this!
> > 
> > On Wed, Dec 13, 2023 at 08:51:18PM +0100, Alex Bee wrote:
> > > The display controller will always give full range RGB regardless of the
> > > mode set, but HDMI requires certain modes to be transmitted in limited
> > > range RGB. This is especially required for HDMI sinks which do not support
> > > non-standard quantization ranges.
> > > 
> > > This enables color space conversion for those modes and sets the
> > > quantization range accordingly in the AVI infoframe.
> > > 
> > > Fixes: 412d4ae6b7a5 ("drm/rockchip: hdmi: add Innosilicon HDMI support")
> > > Signed-off-by: Alex Bee <knaerzche@gmail.com>
> > > ---
> > >   drivers/gpu/drm/rockchip/inno_hdmi.c | 40 ++++++++++++++++++++++------
> > >   1 file changed, 32 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
> > > index 345253e033c5..32626a75723c 100644
> > > --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
> > > +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
> > > @@ -33,6 +33,7 @@ struct hdmi_data_info {
> > >   	unsigned int enc_in_format;
> > >   	unsigned int enc_out_format;
> > >   	unsigned int colorimetry;
> > > +	bool rgb_limited_range;
> > >   };
> > >   struct inno_hdmi_i2c {
> > > @@ -308,6 +309,18 @@ static int inno_hdmi_config_video_avi(struct inno_hdmi *hdmi,
> > >   	else
> > >   		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
> > > +	if (hdmi->hdmi_data.enc_out_format == HDMI_COLORSPACE_RGB) {
> > > +		drm_hdmi_avi_infoframe_quant_range(&frame.avi,
> > > +						   &hdmi->connector, mode,
> > > +						   hdmi->hdmi_data.rgb_limited_range ?
> > > +						   HDMI_QUANTIZATION_RANGE_LIMITED :
> > > +						   HDMI_QUANTIZATION_RANGE_FULL);
> > > +	} else {
> > > +		frame.avi.quantization_range = HDMI_QUANTIZATION_RANGE_DEFAULT;
> > > +		frame.avi.ycc_quantization_range =
> > > +			HDMI_YCC_QUANTIZATION_RANGE_LIMITED;
> > > +	}
> > > +
> > >   	return inno_hdmi_upload_frame(hdmi, rc, &frame, INFOFRAME_AVI, 0, 0, 0);
> > >   }
> > > @@ -334,14 +347,22 @@ static int inno_hdmi_config_video_csc(struct inno_hdmi *hdmi)
> > >   	if (data->enc_in_format == data->enc_out_format) {
> > >   		if ((data->enc_in_format == HDMI_COLORSPACE_RGB) ||
> > >   		    (data->enc_in_format >= HDMI_COLORSPACE_YUV444)) {
> > > -			value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
> > > -			hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
> > > -
> > > -			hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
> > > -				  m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
> > > -				  v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
> > > -				  v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
> > > -			return 0;
> > > +			if (data->enc_in_format == HDMI_COLORSPACE_RGB &&
> > > +			    data->enc_out_format == HDMI_COLORSPACE_RGB &&
> > > +			    hdmi->hdmi_data.rgb_limited_range) {
> > > +				csc_mode = CSC_RGB_0_255_TO_RGB_16_235_8BIT;
> > > +				auto_csc = AUTO_CSC_DISABLE;
> > > +				c0_c2_change = C0_C2_CHANGE_DISABLE;
> > > +				csc_enable = v_CSC_ENABLE;
> > > +			} else {
> > > +				value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
> > > +				hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
> > > +				hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
> > > +					  m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
> > > +					  v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
> > > +					  v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
> > > +				return 0;
> > > +			}
> > >   		}
> > >   	}
> > > @@ -458,6 +479,9 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
> > >   	else
> > >   		hdmi->hdmi_data.colorimetry = HDMI_COLORIMETRY_ITU_709;
> > > +	hdmi->hdmi_data.rgb_limited_range =
> > > +		drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_LIMITED;
> > > +
> > This patch conflicts heavily with my inno_hdmi patches here (patches 22 to 38):
> > https://lore.kernel.org/dri-devel/20231207-kms-hdmi-connector-state-v5-0-6538e19d634d@kernel.org/
>
> I'm aware of that and I mentioned it in the cover letter.

Sorry, I missed that part.

> Your series is not merged yet and it didn't get much feedback so far.
> What is the status there?

It didn't have much reviews, but I'll hope to change that. For the
patches 22 to 38 though, it doesn't really matter. Those changes are
self-contained and can be applied as is outside of the series.

> Especially because you are removing things from inno-hdmi driver (which
> aren't really required to remove there) which will I have to reintroduce.

I'm not entirely sure which part I remove that are actually going to be
used here.

> > I would appreciate if you could test and merge them into your series.
> > 
> > In particular, there's no need to store the range here: enc_out_format
>
> rgb_limited_range is currently not only used for csc, but also for for
> infoframe creation. So it makes sense to have this stored  to avoid calling
> drm_default_rgb_quant_range twice.

You're right, I missed one. Still, it shouldn't be stored in the
hdmi_data_info structure, it's tied to the mode, and the mode is part of
the state, so it's not a property to a given device, but it's tied to
the connector state.

So if you want to do so, you should really create a custom state
structure and store the range there, just like vc4 is doing for example.

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 04/11] drm/rockchip: inno_hdmi: Correctly setup HDMI quantization range

[Alex Bee]

Am 14.12.23 um 12:33 schrieb Maxime Ripard:
> On Thu, Dec 14, 2023 at 12:12:08PM +0100, Alex Bee wrote:
>> Hi Maxime
>>
>> Am 14.12.23 um 08:56 schrieb Maxime Ripard:
>>> Hi Alex,
>>>
>>> Thanks for working on this!
>>>
>>> On Wed, Dec 13, 2023 at 08:51:18PM +0100, Alex Bee wrote:
>>>> The display controller will always give full range RGB regardless of the
>>>> mode set, but HDMI requires certain modes to be transmitted in limited
>>>> range RGB. This is especially required for HDMI sinks which do not support
>>>> non-standard quantization ranges.
>>>>
>>>> This enables color space conversion for those modes and sets the
>>>> quantization range accordingly in the AVI infoframe.
>>>>
>>>> Fixes: 412d4ae6b7a5 ("drm/rockchip: hdmi: add Innosilicon HDMI support")
>>>> Signed-off-by: Alex Bee <knaerzche@gmail.com>
>>>> ---
>>>>    drivers/gpu/drm/rockchip/inno_hdmi.c | 40 ++++++++++++++++++++++------
>>>>    1 file changed, 32 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
>>>> index 345253e033c5..32626a75723c 100644
>>>> --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
>>>> +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
>>>> @@ -33,6 +33,7 @@ struct hdmi_data_info {
>>>>    	unsigned int enc_in_format;
>>>>    	unsigned int enc_out_format;
>>>>    	unsigned int colorimetry;
>>>> +	bool rgb_limited_range;
>>>>    };
>>>>    struct inno_hdmi_i2c {
>>>> @@ -308,6 +309,18 @@ static int inno_hdmi_config_video_avi(struct inno_hdmi *hdmi,
>>>>    	else
>>>>    		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
>>>> +	if (hdmi->hdmi_data.enc_out_format == HDMI_COLORSPACE_RGB) {
>>>> +		drm_hdmi_avi_infoframe_quant_range(&frame.avi,
>>>> +						   &hdmi->connector, mode,
>>>> +						   hdmi->hdmi_data.rgb_limited_range ?
>>>> +						   HDMI_QUANTIZATION_RANGE_LIMITED :
>>>> +						   HDMI_QUANTIZATION_RANGE_FULL);
>>>> +	} else {
>>>> +		frame.avi.quantization_range = HDMI_QUANTIZATION_RANGE_DEFAULT;
>>>> +		frame.avi.ycc_quantization_range =
>>>> +			HDMI_YCC_QUANTIZATION_RANGE_LIMITED;
>>>> +	}
>>>> +
>>>>    	return inno_hdmi_upload_frame(hdmi, rc, &frame, INFOFRAME_AVI, 0, 0, 0);
>>>>    }
>>>> @@ -334,14 +347,22 @@ static int inno_hdmi_config_video_csc(struct inno_hdmi *hdmi)
>>>>    	if (data->enc_in_format == data->enc_out_format) {
>>>>    		if ((data->enc_in_format == HDMI_COLORSPACE_RGB) ||
>>>>    		    (data->enc_in_format >= HDMI_COLORSPACE_YUV444)) {
>>>> -			value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
>>>> -			hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
>>>> -
>>>> -			hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
>>>> -				  m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
>>>> -				  v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
>>>> -				  v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
>>>> -			return 0;
>>>> +			if (data->enc_in_format == HDMI_COLORSPACE_RGB &&
>>>> +			    data->enc_out_format == HDMI_COLORSPACE_RGB &&
>>>> +			    hdmi->hdmi_data.rgb_limited_range) {
>>>> +				csc_mode = CSC_RGB_0_255_TO_RGB_16_235_8BIT;
>>>> +				auto_csc = AUTO_CSC_DISABLE;
>>>> +				c0_c2_change = C0_C2_CHANGE_DISABLE;
>>>> +				csc_enable = v_CSC_ENABLE;
>>>> +			} else {
>>>> +				value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
>>>> +				hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
>>>> +				hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
>>>> +					  m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
>>>> +					  v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
>>>> +					  v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
>>>> +				return 0;
>>>> +			}
>>>>    		}
>>>>    	}
>>>> @@ -458,6 +479,9 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
>>>>    	else
>>>>    		hdmi->hdmi_data.colorimetry = HDMI_COLORIMETRY_ITU_709;
>>>> +	hdmi->hdmi_data.rgb_limited_range =
>>>> +		drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_LIMITED;
>>>> +
>>> This patch conflicts heavily with my inno_hdmi patches here (patches 22 to 38):
>>> https://lore.kernel.org/dri-devel/20231207-kms-hdmi-connector-state-v5-0-6538e19d634d@kernel.org/
>> I'm aware of that and I mentioned it in the cover letter.
> Sorry, I missed that part.
>
>> Your series is not merged yet and it didn't get much feedback so far.
>> What is the status there?
> It didn't have much reviews, but I'll hope to change that. For the
> patches 22 to 38 though, it doesn't really matter. Those changes are
> self-contained and can be applied as is outside of the series.
>
>> Especially because you are removing things from inno-hdmi driver (which
>> aren't really required to remove there) which will I have to reintroduce.
> I'm not entirely sure which part I remove that are actually going to be
> used here.
I'm refering to [PATCH v5 33/44] which completly removes csc coeffs but 
this series needs the CSC_RGB_0_255_TO_RGB_16_235_8BIT  coeffs and  
[PATCH v5 29/44] which removes writing csc_coeffs to the hardware.
>
>>> I would appreciate if you could test and merge them into your series.
>>>
>>> In particular, there's no need to store the range here: enc_out_format
>> rgb_limited_range is currently not only used for csc, but also for for
>> infoframe creation. So it makes sense to have this stored  to avoid calling
>> drm_default_rgb_quant_range twice.
> You're right, I missed one. Still, it shouldn't be stored in the
> hdmi_data_info structure, it's tied to the mode, and the mode is part of
> the state, so it's not a property to a given device, but it's tied to
> the connector state.
>
> So if you want to do so, you should really create a custom state
> structure and store the range there, just like vc4 is doing for example.

OK - I'll check.

Alex

>
> Maxime

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 04/11] drm/rockchip: inno_hdmi: Correctly setup HDMI quantization range

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 5564 bytes --]

On Thu, Dec 14, 2023 at 03:05:59PM +0100, Alex Bee wrote:
> 
> Am 14.12.23 um 12:33 schrieb Maxime Ripard:
> > On Thu, Dec 14, 2023 at 12:12:08PM +0100, Alex Bee wrote:
> > > Hi Maxime
> > > 
> > > Am 14.12.23 um 08:56 schrieb Maxime Ripard:
> > > > Hi Alex,
> > > > 
> > > > Thanks for working on this!
> > > > 
> > > > On Wed, Dec 13, 2023 at 08:51:18PM +0100, Alex Bee wrote:
> > > > > The display controller will always give full range RGB regardless of the
> > > > > mode set, but HDMI requires certain modes to be transmitted in limited
> > > > > range RGB. This is especially required for HDMI sinks which do not support
> > > > > non-standard quantization ranges.
> > > > > 
> > > > > This enables color space conversion for those modes and sets the
> > > > > quantization range accordingly in the AVI infoframe.
> > > > > 
> > > > > Fixes: 412d4ae6b7a5 ("drm/rockchip: hdmi: add Innosilicon HDMI support")
> > > > > Signed-off-by: Alex Bee <knaerzche@gmail.com>
> > > > > ---
> > > > >    drivers/gpu/drm/rockchip/inno_hdmi.c | 40 ++++++++++++++++++++++------
> > > > >    1 file changed, 32 insertions(+), 8 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
> > > > > index 345253e033c5..32626a75723c 100644
> > > > > --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
> > > > > +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
> > > > > @@ -33,6 +33,7 @@ struct hdmi_data_info {
> > > > >    	unsigned int enc_in_format;
> > > > >    	unsigned int enc_out_format;
> > > > >    	unsigned int colorimetry;
> > > > > +	bool rgb_limited_range;
> > > > >    };
> > > > >    struct inno_hdmi_i2c {
> > > > > @@ -308,6 +309,18 @@ static int inno_hdmi_config_video_avi(struct inno_hdmi *hdmi,
> > > > >    	else
> > > > >    		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
> > > > > +	if (hdmi->hdmi_data.enc_out_format == HDMI_COLORSPACE_RGB) {
> > > > > +		drm_hdmi_avi_infoframe_quant_range(&frame.avi,
> > > > > +						   &hdmi->connector, mode,
> > > > > +						   hdmi->hdmi_data.rgb_limited_range ?
> > > > > +						   HDMI_QUANTIZATION_RANGE_LIMITED :
> > > > > +						   HDMI_QUANTIZATION_RANGE_FULL);
> > > > > +	} else {
> > > > > +		frame.avi.quantization_range = HDMI_QUANTIZATION_RANGE_DEFAULT;
> > > > > +		frame.avi.ycc_quantization_range =
> > > > > +			HDMI_YCC_QUANTIZATION_RANGE_LIMITED;
> > > > > +	}
> > > > > +
> > > > >    	return inno_hdmi_upload_frame(hdmi, rc, &frame, INFOFRAME_AVI, 0, 0, 0);
> > > > >    }
> > > > > @@ -334,14 +347,22 @@ static int inno_hdmi_config_video_csc(struct inno_hdmi *hdmi)
> > > > >    	if (data->enc_in_format == data->enc_out_format) {
> > > > >    		if ((data->enc_in_format == HDMI_COLORSPACE_RGB) ||
> > > > >    		    (data->enc_in_format >= HDMI_COLORSPACE_YUV444)) {
> > > > > -			value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
> > > > > -			hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
> > > > > -
> > > > > -			hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
> > > > > -				  m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
> > > > > -				  v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
> > > > > -				  v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
> > > > > -			return 0;
> > > > > +			if (data->enc_in_format == HDMI_COLORSPACE_RGB &&
> > > > > +			    data->enc_out_format == HDMI_COLORSPACE_RGB &&
> > > > > +			    hdmi->hdmi_data.rgb_limited_range) {
> > > > > +				csc_mode = CSC_RGB_0_255_TO_RGB_16_235_8BIT;
> > > > > +				auto_csc = AUTO_CSC_DISABLE;
> > > > > +				c0_c2_change = C0_C2_CHANGE_DISABLE;
> > > > > +				csc_enable = v_CSC_ENABLE;
> > > > > +			} else {
> > > > > +				value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
> > > > > +				hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
> > > > > +				hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
> > > > > +					  m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
> > > > > +					  v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
> > > > > +					  v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
> > > > > +				return 0;
> > > > > +			}
> > > > >    		}
> > > > >    	}
> > > > > @@ -458,6 +479,9 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
> > > > >    	else
> > > > >    		hdmi->hdmi_data.colorimetry = HDMI_COLORIMETRY_ITU_709;
> > > > > +	hdmi->hdmi_data.rgb_limited_range =
> > > > > +		drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_LIMITED;
> > > > > +
> > > > This patch conflicts heavily with my inno_hdmi patches here (patches 22 to 38):
> > > > https://lore.kernel.org/dri-devel/20231207-kms-hdmi-connector-state-v5-0-6538e19d634d@kernel.org/
> > > I'm aware of that and I mentioned it in the cover letter.
> > Sorry, I missed that part.
> > 
> > > Your series is not merged yet and it didn't get much feedback so far.
> > > What is the status there?
> > It didn't have much reviews, but I'll hope to change that. For the
> > patches 22 to 38 though, it doesn't really matter. Those changes are
> > self-contained and can be applied as is outside of the series.
> > 
> > > Especially because you are removing things from inno-hdmi driver (which
> > > aren't really required to remove there) which will I have to reintroduce.
> > I'm not entirely sure which part I remove that are actually going to be
> > used here.
>
> I'm refering to [PATCH v5 33/44] which completly removes csc coeffs but this
> series needs the CSC_RGB_0_255_TO_RGB_16_235_8BIT  coeffs and  [PATCH v5
> 29/44] which removes writing csc_coeffs to the hardware.

Oh, right, feel free to drop those

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 05/11] drm/rockchip: inno_hdmi: Add variant support

[Alex Bee]

In preparation to support RK3128's integration of the controller, this
patch adds a simple variant implementation. They mainly differ in the
phy configuration required, so those are part of the match_data.
The values have been taken from downstream. The pixelclocks in there are
meant to be max-inclusive.

Signed-off-by: Alex Bee <knaerzche@gmail.com>
---
 drivers/gpu/drm/rockchip/inno_hdmi.c | 68 +++++++++++++++++++++++++++-
 1 file changed, 66 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
index 32626a75723c..593b184bd0ad 100644
--- a/drivers/gpu/drm/rockchip/inno_hdmi.c
+++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
@@ -27,6 +27,17 @@
 
 #include "inno_hdmi.h"
 
+struct inno_hdmi_phy_config {
+	unsigned int pixelclock;
+	u8 pre_emphasis;
+	u8 voltage_level_control;
+};
+
+struct inno_hdmi_variant {
+	struct inno_hdmi_phy_config *phy_configs;
+	struct inno_hdmi_phy_config *default_phy_config;
+};
+
 struct hdmi_data_info {
 	int vic;
 	bool sink_has_audio;
@@ -64,6 +75,14 @@ struct inno_hdmi {
 
 	struct hdmi_data_info	hdmi_data;
 	struct drm_display_mode previous_mode;
+
+	const struct inno_hdmi_variant *variant;
+};
+
+static struct inno_hdmi_phy_config rk3036_hdmi_phy_configs[] = {
+	{ 74250000,  0x3f, 0xbb },
+	{ 165000000, 0x6f, 0xbb },
+	{ ~0UL,	     0x00, 0x00 }
 };
 
 static struct inno_hdmi *encoder_to_inno_hdmi(struct drm_encoder *encoder)
@@ -176,6 +195,24 @@ static inline void hdmi_modb(struct inno_hdmi *hdmi, u16 offset,
 	hdmi_writeb(hdmi, offset, temp);
 }
 
+static int inno_hdmi_find_phy_config(struct inno_hdmi *hdmi,
+				     unsigned int pixelclk)
+{
+	const struct inno_hdmi_phy_config *phy_configs =
+						hdmi->variant->phy_configs;
+	int i;
+
+	for (i = 0; phy_configs[i].pixelclock != ~0UL; i++) {
+		if (pixelclk <= phy_configs[i].pixelclock)
+			return i;
+	}
+
+	DRM_DEV_DEBUG(hdmi->dev, "No phy configuration for pixelclock %u\n",
+		      pixelclk);
+
+	return -EINVAL;
+}
+
 static void inno_hdmi_i2c_init(struct inno_hdmi *hdmi)
 {
 	int ddc_bus_freq;
@@ -200,12 +237,25 @@ static void inno_hdmi_sys_power(struct inno_hdmi *hdmi, bool enable)
 
 static void inno_hdmi_set_pwr_mode(struct inno_hdmi *hdmi, int mode)
 {
+	int ret;
+	struct inno_hdmi_phy_config *phy_config;
+
 	switch (mode) {
 	case NORMAL:
 		inno_hdmi_sys_power(hdmi, false);
 
-		hdmi_writeb(hdmi, HDMI_PHY_PRE_EMPHASIS, 0x6f);
-		hdmi_writeb(hdmi, HDMI_PHY_DRIVER, 0xbb);
+		ret = inno_hdmi_find_phy_config(hdmi, hdmi->tmds_rate);
+		if (ret < 0) {
+			phy_config = hdmi->variant->default_phy_config;
+			DRM_DEV_ERROR(hdmi->dev,
+				      "Using default phy configuration for TMDS rate %u",
+				      hdmi->tmds_rate);
+		} else {
+			phy_config = &hdmi->variant->phy_configs[ret];
+		}
+
+		hdmi_writeb(hdmi, HDMI_PHY_PRE_EMPHASIS, phy_config->pre_emphasis);
+		hdmi_writeb(hdmi, HDMI_PHY_DRIVER, phy_config->voltage_level_control);
 
 		hdmi_writeb(hdmi, HDMI_PHY_SYS_CTL, 0x15);
 		hdmi_writeb(hdmi, HDMI_PHY_SYS_CTL, 0x14);
@@ -845,6 +895,8 @@ static int inno_hdmi_bind(struct device *dev, struct device *master,
 	struct platform_device *pdev = to_platform_device(dev);
 	struct drm_device *drm = data;
 	struct inno_hdmi *hdmi;
+	const struct inno_hdmi_variant *variant;
+
 	int irq;
 	int ret;
 
@@ -853,6 +905,12 @@ static int inno_hdmi_bind(struct device *dev, struct device *master,
 		return -ENOMEM;
 
 	hdmi->dev = dev;
+
+	variant = of_device_get_match_data(hdmi->dev);
+	if (!variant)
+		return -EINVAL;
+
+	hdmi->variant = variant;
 	hdmi->drm_dev = drm;
 
 	hdmi->regs = devm_platform_ioremap_resource(pdev, 0);
@@ -949,8 +1007,14 @@ static void inno_hdmi_remove(struct platform_device *pdev)
 	component_del(&pdev->dev, &inno_hdmi_ops);
 }
 
+static const struct inno_hdmi_variant rk3036_inno_hdmi_variant = {
+	.phy_configs = rk3036_hdmi_phy_configs,
+	.default_phy_config = &rk3036_hdmi_phy_configs[1],
+};
+
 static const struct of_device_id inno_hdmi_dt_ids[] = {
 	{ .compatible = "rockchip,rk3036-inno-hdmi",
+	  .data = &rk3036_inno_hdmi_variant,
 	},
 	{},
 };
-- 
2.43.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 06/11] drm/rockchip: inno_hdmi: Add RK3128 support

[Alex Bee]

This variant requires the phy reference clock to be enabled before the
DDC block can work and the (initial) DDC bus frequency is calculated
based on the rate of this clock.
Besides the only difference is phy configuration which is required to make
the driver working for this variant as well.

Signed-off-by: Alex Bee <knaerzche@gmail.com>
---
 drivers/gpu/drm/rockchip/inno_hdmi.c | 46 +++++++++++++++++++++++++---
 1 file changed, 41 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
index 593b184bd0ad..f7f0bec725f9 100644
--- a/drivers/gpu/drm/rockchip/inno_hdmi.c
+++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
@@ -63,6 +63,7 @@ struct inno_hdmi {
 
 	int irq;
 	struct clk *pclk;
+	struct clk *refclk;
 	void __iomem *regs;
 
 	struct drm_connector	connector;
@@ -85,6 +86,12 @@ static struct inno_hdmi_phy_config rk3036_hdmi_phy_configs[] = {
 	{ ~0UL,	     0x00, 0x00 }
 };
 
+static struct inno_hdmi_phy_config rk3128_hdmi_phy_configs[] = {
+	{ 74250000,  0x3f, 0xaa },
+	{ 165000000, 0x5f, 0xaa },
+	{ ~0UL,	     0x00, 0x00 }
+};
+
 static struct inno_hdmi *encoder_to_inno_hdmi(struct drm_encoder *encoder)
 {
 	struct rockchip_encoder *rkencoder = to_rockchip_encoder(encoder);
@@ -930,6 +937,20 @@ static int inno_hdmi_bind(struct device *dev, struct device *master,
 		return ret;
 	}
 
+	hdmi->refclk = devm_clk_get_optional(hdmi->dev, "ref");
+	if (IS_ERR(hdmi->refclk)) {
+		DRM_DEV_ERROR(hdmi->dev, "Unable to get HDMI reference clock\n");
+		ret = PTR_ERR(hdmi->refclk);
+		goto err_disable_pclk;
+	}
+
+	ret = clk_prepare_enable(hdmi->refclk);
+	if (ret) {
+		DRM_DEV_ERROR(hdmi->dev,
+			      "Cannot enable HDMI reference clock: %d\n", ret);
+		goto err_disable_pclk;
+	}
+
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0) {
 		ret = irq;
@@ -946,12 +967,16 @@ static int inno_hdmi_bind(struct device *dev, struct device *master,
 	}
 
 	/*
-	 * When IP controller haven't configured to an accurate video
-	 * timing, then the TMDS clock source would be switched to
-	 * PCLK_HDMI, so we need to init the TMDS rate to PCLK rate,
-	 * and reconfigure the DDC clock.
+	 * When IP controller isn't configured to an accurate
+	 * video timing and there is no reference clock available,
+	 * then the TMDS clock source would be switched to PCLK_HDMI,
+	 * so we need to init the TMDS rate to PCLK rate, and
+	 * reconfigure the DDC clock.
 	 */
-	hdmi->tmds_rate = clk_get_rate(hdmi->pclk);
+	if (hdmi->refclk)
+		hdmi->tmds_rate = clk_get_rate(hdmi->refclk);
+	else
+		hdmi->tmds_rate = clk_get_rate(hdmi->pclk);
 	inno_hdmi_i2c_init(hdmi);
 
 	ret = inno_hdmi_register(drm, hdmi);
@@ -976,6 +1001,8 @@ static int inno_hdmi_bind(struct device *dev, struct device *master,
 err_put_adapter:
 	i2c_put_adapter(hdmi->ddc);
 err_disable_clk:
+	clk_disable_unprepare(hdmi->refclk);
+err_disable_pclk:
 	clk_disable_unprepare(hdmi->pclk);
 	return ret;
 }
@@ -989,6 +1016,7 @@ static void inno_hdmi_unbind(struct device *dev, struct device *master,
 	hdmi->encoder.encoder.funcs->destroy(&hdmi->encoder.encoder);
 
 	i2c_put_adapter(hdmi->ddc);
+	clk_disable_unprepare(hdmi->refclk);
 	clk_disable_unprepare(hdmi->pclk);
 }
 
@@ -1012,10 +1040,18 @@ static const struct inno_hdmi_variant rk3036_inno_hdmi_variant = {
 	.default_phy_config = &rk3036_hdmi_phy_configs[1],
 };
 
+static const struct inno_hdmi_variant rk3128_inno_hdmi_variant = {
+	.phy_configs = rk3128_hdmi_phy_configs,
+	.default_phy_config = &rk3128_hdmi_phy_configs[1],
+};
+
 static const struct of_device_id inno_hdmi_dt_ids[] = {
 	{ .compatible = "rockchip,rk3036-inno-hdmi",
 	  .data = &rk3036_inno_hdmi_variant,
 	},
+	{ .compatible = "rockchip,rk3128-inno-hdmi",
+	  .data = &rk3128_inno_hdmi_variant,
+	},
 	{},
 };
 MODULE_DEVICE_TABLE(of, inno_hdmi_dt_ids);
-- 
2.43.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 07/11] drm/rockchip: inno_hdmi: Add basic mode validation

[Alex Bee]

As per TRM this controller supports pixelclocks starting from 25 MHz. The
maximum supported pixelclocks are defined by the phy configurations we
have. Also it can't support modes that require doubled clocks.
If there is a phy reference clock we can additionally validate against
VESA DMT's recommendations.
Those checks are added to the mode_valid hook of the connector and
encoder's mode_fixup hook.

Signed-off-by: Alex Bee <knaerzche@gmail.com>
---
 drivers/gpu/drm/rockchip/inno_hdmi.c | 38 ++++++++++++++++++++++++++--
 1 file changed, 36 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
index f7f0bec725f9..2f839ff31c1c 100644
--- a/drivers/gpu/drm/rockchip/inno_hdmi.c
+++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
@@ -38,6 +38,8 @@ struct inno_hdmi_variant {
 	struct inno_hdmi_phy_config *default_phy_config;
 };
 
+#define INNO_HDMI_MIN_TMDS_CLOCK  25000000U
+
 struct hdmi_data_info {
 	int vic;
 	bool sink_has_audio;
@@ -572,6 +574,34 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
 	return 0;
 }
 
+static enum drm_mode_status inno_hdmi_mode_valid(struct inno_hdmi *hdmi,
+						 struct drm_display_mode *mode)
+{
+	/* No support for double-clock modes */
+	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
+		return MODE_BAD;
+
+	unsigned int mpixelclk = mode->clock * 1000;
+
+	if (mpixelclk < INNO_HDMI_MIN_TMDS_CLOCK)
+		return MODE_CLOCK_LOW;
+
+	if (inno_hdmi_find_phy_config(hdmi, mpixelclk) < 0)
+		return MODE_CLOCK_HIGH;
+
+	if (hdmi->refclk) {
+		long refclk = clk_round_rate(hdmi->refclk, mpixelclk);
+		unsigned int max_tolerance = mpixelclk / 5000;
+
+		/* Vesa DMT standard mentions +/- 0.5% max tolerance */
+		if (abs(refclk - mpixelclk) > max_tolerance ||
+		    mpixelclk - refclk > max_tolerance)
+			return MODE_NOCLOCK;
+	}
+
+	return MODE_OK;
+}
+
 static void inno_hdmi_encoder_mode_set(struct drm_encoder *encoder,
 				       struct drm_display_mode *mode,
 				       struct drm_display_mode *adj_mode)
@@ -602,7 +632,9 @@ static bool inno_hdmi_encoder_mode_fixup(struct drm_encoder *encoder,
 					 const struct drm_display_mode *mode,
 					 struct drm_display_mode *adj_mode)
 {
-	return true;
+	struct inno_hdmi *hdmi = encoder_to_inno_hdmi(encoder);
+
+	return inno_hdmi_mode_valid(hdmi, adj_mode) == MODE_OK;
 }
 
 static int
@@ -659,7 +691,9 @@ static enum drm_mode_status
 inno_hdmi_connector_mode_valid(struct drm_connector *connector,
 			       struct drm_display_mode *mode)
 {
-	return MODE_OK;
+	struct inno_hdmi *hdmi = connector_to_inno_hdmi(connector);
+
+	return inno_hdmi_mode_valid(hdmi, mode);
 }
 
 static int
-- 
2.43.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 07/11] drm/rockchip: inno_hdmi: Add basic mode validation

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 3189 bytes --]

Hi,

On Wed, Dec 13, 2023 at 08:51:21PM +0100, Alex Bee wrote:
> As per TRM this controller supports pixelclocks starting from 25 MHz. The
> maximum supported pixelclocks are defined by the phy configurations we
> have. Also it can't support modes that require doubled clocks.
> If there is a phy reference clock we can additionally validate against
> VESA DMT's recommendations.
> Those checks are added to the mode_valid hook of the connector and
> encoder's mode_fixup hook.
> 
> Signed-off-by: Alex Bee <knaerzche@gmail.com>
> ---
>  drivers/gpu/drm/rockchip/inno_hdmi.c | 38 ++++++++++++++++++++++++++--
>  1 file changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
> index f7f0bec725f9..2f839ff31c1c 100644
> --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
> +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
> @@ -38,6 +38,8 @@ struct inno_hdmi_variant {
>  	struct inno_hdmi_phy_config *default_phy_config;
>  };
>  
> +#define INNO_HDMI_MIN_TMDS_CLOCK  25000000U
> +
>  struct hdmi_data_info {
>  	int vic;
>  	bool sink_has_audio;
> @@ -572,6 +574,34 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
>  	return 0;
>  }
>  
> +static enum drm_mode_status inno_hdmi_mode_valid(struct inno_hdmi *hdmi,
> +						 struct drm_display_mode *mode)
> +{

So, mode_valid is only called to filter out the modes retrieved by
get_modes, but it won't be called when userspace programs a mode. That's
atomic_check's job.

So you probably want to create a shared function between atomic_check
and mode_valid, and call it from both places (or call mode_valid from
atomic_check).

> +	/* No support for double-clock modes */
> +	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
> +		return MODE_BAD;
> +
> +	unsigned int mpixelclk = mode->clock * 1000;

Variables should be declared at the top of the function.

> +	if (mpixelclk < INNO_HDMI_MIN_TMDS_CLOCK)
> +		return MODE_CLOCK_LOW;

You probably want to check the max TMDS clock too?

> +	if (inno_hdmi_find_phy_config(hdmi, mpixelclk) < 0)
> +		return MODE_CLOCK_HIGH;
> +
> +	if (hdmi->refclk) {
> +		long refclk = clk_round_rate(hdmi->refclk, mpixelclk);
> +		unsigned int max_tolerance = mpixelclk / 5000;
> +
> +		/* Vesa DMT standard mentions +/- 0.5% max tolerance */
> +		if (abs(refclk - mpixelclk) > max_tolerance ||
> +		    mpixelclk - refclk > max_tolerance;
> +			return MODE_NOCLOCK;

You should use abs_diff here. abs() will get confused by the unsigned vs
signed comparison.

> +	}
> +
> +	return MODE_OK;
> +}
> +
>  static void inno_hdmi_encoder_mode_set(struct drm_encoder *encoder,
>  				       struct drm_display_mode *mode,
>  				       struct drm_display_mode *adj_mode)
> @@ -602,7 +632,9 @@ static bool inno_hdmi_encoder_mode_fixup(struct drm_encoder *encoder,
>  					 const struct drm_display_mode *mode,
>  					 struct drm_display_mode *adj_mode)
>  {
> -	return true;
> +	struct inno_hdmi *hdmi = encoder_to_inno_hdmi(encoder);
> +
> +	return inno_hdmi_mode_valid(hdmi, adj_mode) == MODE_OK;
>  }

Why do you call mode_valid in mode_fixup?

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 07/11] drm/rockchip: inno_hdmi: Add basic mode validation

[Alex Bee]

Hi Maxime,

Am 14.12.23 um 09:05 schrieb Maxime Ripard:
> Hi,
>
> On Wed, Dec 13, 2023 at 08:51:21PM +0100, Alex Bee wrote:
>> As per TRM this controller supports pixelclocks starting from 25 MHz. The
>> maximum supported pixelclocks are defined by the phy configurations we
>> have. Also it can't support modes that require doubled clocks.
>> If there is a phy reference clock we can additionally validate against
>> VESA DMT's recommendations.
>> Those checks are added to the mode_valid hook of the connector and
>> encoder's mode_fixup hook.
>>
>> Signed-off-by: Alex Bee <knaerzche@gmail.com>
>> ---
>>   drivers/gpu/drm/rockchip/inno_hdmi.c | 38 ++++++++++++++++++++++++++--
>>   1 file changed, 36 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
>> index f7f0bec725f9..2f839ff31c1c 100644
>> --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
>> +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
>> @@ -38,6 +38,8 @@ struct inno_hdmi_variant {
>>   	struct inno_hdmi_phy_config *default_phy_config;
>>   };
>>   
>> +#define INNO_HDMI_MIN_TMDS_CLOCK  25000000U
>> +
>>   struct hdmi_data_info {
>>   	int vic;
>>   	bool sink_has_audio;
>> @@ -572,6 +574,34 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
>>   	return 0;
>>   }
>>   
>> +static enum drm_mode_status inno_hdmi_mode_valid(struct inno_hdmi *hdmi,
>> +						 struct drm_display_mode *mode)
>> +{
> So, mode_valid is only called to filter out the modes retrieved by
> get_modes, but it won't be called when userspace programs a mode. That's
> atomic_check's job.
>
> So you probably want to create a shared function between atomic_check
> and mode_valid, and call it from both places (or call mode_valid from
> atomic_check).
This actually _is_ a shared function called in 
inno_hdmi_connector_mode_valid and inno_hdmi_encoder_mode_fixup. Yes, I  
probably should use it in atomic_check _also_.
>
>> +	/* No support for double-clock modes */
>> +	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
>> +		return MODE_BAD;
>> +
>> +	unsigned int mpixelclk = mode->clock * 1000;
> Variables should be declared at the top of the function.
Oookay ... can move them.
>> +	if (mpixelclk < INNO_HDMI_MIN_TMDS_CLOCK)
>> +		return MODE_CLOCK_LOW;
> You probably want to check the max TMDS clock too?
Not sure what you mean here. For the currently supported formats of this 
driver (rgb only) tmds clock and pixel clock are always the same.
>
>> +	if (inno_hdmi_find_phy_config(hdmi, mpixelclk) < 0)
>> +		return MODE_CLOCK_HIGH;
>> +
>> +	if (hdmi->refclk) {
>> +		long refclk = clk_round_rate(hdmi->refclk, mpixelclk);
>> +		unsigned int max_tolerance = mpixelclk / 5000;
>> +
>> +		/* Vesa DMT standard mentions +/- 0.5% max tolerance */
>> +		if (abs(refclk - mpixelclk) > max_tolerance ||
>> +		    mpixelclk - refclk > max_tolerance;
>> +			return MODE_NOCLOCK;
> You should use abs_diff here. abs() will get confused by the unsigned vs
> signed comparison.
Ack.
>
>> +	}
>> +
>> +	return MODE_OK;
>> +}
>> +
>>   static void inno_hdmi_encoder_mode_set(struct drm_encoder *encoder,
>>   				       struct drm_display_mode *mode,
>>   				       struct drm_display_mode *adj_mode)
>> @@ -602,7 +632,9 @@ static bool inno_hdmi_encoder_mode_fixup(struct drm_encoder *encoder,
>>   					 const struct drm_display_mode *mode,
>>   					 struct drm_display_mode *adj_mode)
>>   {
>> -	return true;
>> +	struct inno_hdmi *hdmi = encoder_to_inno_hdmi(encoder);
>> +
>> +	return inno_hdmi_mode_valid(hdmi, adj_mode) == MODE_OK;
>>   }
> Why do you call mode_valid in mode_fixup?

I'm calling the shared function you asked me to introduce 
(inno_hdmi_connector_mode_valid != inno_mode_valid)

Alex

>
> Maxime

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 07/11] drm/rockchip: inno_hdmi: Add basic mode validation

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 3923 bytes --]

On Thu, Dec 14, 2023 at 12:17:39PM +0100, Alex Bee wrote:
> Hi Maxime,
> 
> Am 14.12.23 um 09:05 schrieb Maxime Ripard:
> > Hi,
> > 
> > On Wed, Dec 13, 2023 at 08:51:21PM +0100, Alex Bee wrote:
> > > As per TRM this controller supports pixelclocks starting from 25 MHz. The
> > > maximum supported pixelclocks are defined by the phy configurations we
> > > have. Also it can't support modes that require doubled clocks.
> > > If there is a phy reference clock we can additionally validate against
> > > VESA DMT's recommendations.
> > > Those checks are added to the mode_valid hook of the connector and
> > > encoder's mode_fixup hook.
> > > 
> > > Signed-off-by: Alex Bee <knaerzche@gmail.com>
> > > ---
> > >   drivers/gpu/drm/rockchip/inno_hdmi.c | 38 ++++++++++++++++++++++++++--
> > >   1 file changed, 36 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
> > > index f7f0bec725f9..2f839ff31c1c 100644
> > > --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
> > > +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
> > > @@ -38,6 +38,8 @@ struct inno_hdmi_variant {
> > >   	struct inno_hdmi_phy_config *default_phy_config;
> > >   };
> > > +#define INNO_HDMI_MIN_TMDS_CLOCK  25000000U
> > > +
> > >   struct hdmi_data_info {
> > >   	int vic;
> > >   	bool sink_has_audio;
> > > @@ -572,6 +574,34 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
> > >   	return 0;
> > >   }
> > > +static enum drm_mode_status inno_hdmi_mode_valid(struct inno_hdmi *hdmi,
> > > +						 struct drm_display_mode *mode)
> > > +{
> > So, mode_valid is only called to filter out the modes retrieved by
> > get_modes, but it won't be called when userspace programs a mode. That's
> > atomic_check's job.
> > 
> > So you probably want to create a shared function between atomic_check
> > and mode_valid, and call it from both places (or call mode_valid from
> > atomic_check).
>
> This actually _is_ a shared function called in
> inno_hdmi_connector_mode_valid and inno_hdmi_encoder_mode_fixup. Yes, I 
> probably should use it in atomic_check _also_.

Yeah, I saw that later and forgot to rephrase.

> > > +	/* No support for double-clock modes */
> > > +	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
> > > +		return MODE_BAD;
> > > +
> > > +	unsigned int mpixelclk = mode->clock * 1000;
> > Variables should be declared at the top of the function.
> Oookay ... can move them.
> > > +	if (mpixelclk < INNO_HDMI_MIN_TMDS_CLOCK)
> > > +		return MODE_CLOCK_LOW;
> > You probably want to check the max TMDS clock too?
>
> Not sure what you mean here. For the currently supported formats of this
> driver (rgb only) tmds clock and pixel clock are always the same.

I mean that your controller has a maximum TMDS rate it supports too
(probably something like 340MHz). You should also filter out the modes
that have a pixel clock higher than the one you can reach.

> > > @@ -602,7 +632,9 @@ static bool inno_hdmi_encoder_mode_fixup(struct drm_encoder *encoder,
> > >   					 const struct drm_display_mode *mode,
> > >   					 struct drm_display_mode *adj_mode)
> > >   {
> > > -	return true;
> > > +	struct inno_hdmi *hdmi = encoder_to_inno_hdmi(encoder);
> > > +
> > > +	return inno_hdmi_mode_valid(hdmi, adj_mode) == MODE_OK;
> > >   }
> > Why do you call mode_valid in mode_fixup?
> 
> I'm calling the shared function you asked me to introduce
> (inno_hdmi_connector_mode_valid != inno_mode_valid)

That's the mode_fixup part that I'm focused on :)

mode_fixup is the legacy function to adjust the mode to the controller
capabilities. It's optional, and you're not adjusting anything here,
just testing the same thing mode_valid did.

mode_valid has been superseeded by atomic_check anyway, so just drop
mode_valid and use your function in atomic_check like we discussed.

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 07/11] drm/rockchip: inno_hdmi: Add basic mode validation

[Alex Bee]

Am 14.12.23 um 12:40 schrieb Maxime Ripard:
> On Thu, Dec 14, 2023 at 12:17:39PM +0100, Alex Bee wrote:
>> Hi Maxime,
>>
>> Am 14.12.23 um 09:05 schrieb Maxime Ripard:
>>> Hi,
>>>
>>> On Wed, Dec 13, 2023 at 08:51:21PM +0100, Alex Bee wrote:
>>>> As per TRM this controller supports pixelclocks starting from 25 MHz. The
>>>> maximum supported pixelclocks are defined by the phy configurations we
>>>> have. Also it can't support modes that require doubled clocks.
>>>> If there is a phy reference clock we can additionally validate against
>>>> VESA DMT's recommendations.
>>>> Those checks are added to the mode_valid hook of the connector and
>>>> encoder's mode_fixup hook.
>>>>
>>>> Signed-off-by: Alex Bee <knaerzche@gmail.com>
>>>> ---
>>>>    drivers/gpu/drm/rockchip/inno_hdmi.c | 38 ++++++++++++++++++++++++++--
>>>>    1 file changed, 36 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
>>>> index f7f0bec725f9..2f839ff31c1c 100644
>>>> --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
>>>> +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
>>>> @@ -38,6 +38,8 @@ struct inno_hdmi_variant {
>>>>    	struct inno_hdmi_phy_config *default_phy_config;
>>>>    };
>>>> +#define INNO_HDMI_MIN_TMDS_CLOCK  25000000U
>>>> +
>>>>    struct hdmi_data_info {
>>>>    	int vic;
>>>>    	bool sink_has_audio;
>>>> @@ -572,6 +574,34 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
>>>>    	return 0;
>>>>    }
>>>> +static enum drm_mode_status inno_hdmi_mode_valid(struct inno_hdmi *hdmi,
>>>> +						 struct drm_display_mode *mode)
>>>> +{
>>> So, mode_valid is only called to filter out the modes retrieved by
>>> get_modes, but it won't be called when userspace programs a mode. That's
>>> atomic_check's job.
>>>
>>> So you probably want to create a shared function between atomic_check
>>> and mode_valid, and call it from both places (or call mode_valid from
>>> atomic_check).
>> This actually _is_ a shared function called in
>> inno_hdmi_connector_mode_valid and inno_hdmi_encoder_mode_fixup. Yes, I
>> probably should use it in atomic_check _also_.
> Yeah, I saw that later and forgot to rephrase.
>
>>>> +	/* No support for double-clock modes */
>>>> +	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
>>>> +		return MODE_BAD;
>>>> +
>>>> +	unsigned int mpixelclk = mode->clock * 1000;
>>> Variables should be declared at the top of the function.
>> Oookay ... can move them.
>>>> +	if (mpixelclk < INNO_HDMI_MIN_TMDS_CLOCK)
>>>> +		return MODE_CLOCK_LOW;
>>> You probably want to check the max TMDS clock too?
>> Not sure what you mean here. For the currently supported formats of this
>> driver (rgb only) tmds clock and pixel clock are always the same.
> I mean that your controller has a maximum TMDS rate it supports too
> (probably something like 340MHz). You should also filter out the modes
> that have a pixel clock higher than the one you can reach.
Yes it does have it and it is defined by the phy configurations that do 
exist. The mode is validated against those exactly below this statement. 
(See commit message, btw.)
>
>>>> @@ -602,7 +632,9 @@ static bool inno_hdmi_encoder_mode_fixup(struct drm_encoder *encoder,
>>>>    					 const struct drm_display_mode *mode,
>>>>    					 struct drm_display_mode *adj_mode)
>>>>    {
>>>> -	return true;
>>>> +	struct inno_hdmi *hdmi = encoder_to_inno_hdmi(encoder);
>>>> +
>>>> +	return inno_hdmi_mode_valid(hdmi, adj_mode) == MODE_OK;
>>>>    }
>>> Why do you call mode_valid in mode_fixup?
>> I'm calling the shared function you asked me to introduce
>> (inno_hdmi_connector_mode_valid != inno_mode_valid)
> That's the mode_fixup part that I'm focused on :)
>
> mode_fixup is the legacy function to adjust the mode to the controller
> capabilities. It's optional, and you're not adjusting anything here,
> just testing the same thing mode_valid did.
>
> mode_valid has been superseeded by atomic_check anyway, so just drop
> mode_valid and use your function in atomic_check like we discussed.

OK.

I just read that mode_fixup won't be called at all if atomic_check 
exists. I will drop here and call in atomic_check only.

Alex

> Maxime

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 08/11] drm/rockchip: inno_hdmi: Drop custom fill_modes hook

[Alex Bee]

Now that we have proper pixelclock-based validation we can drop the
custom fill_modes hook.
CRTC size validation for the display controller has been added with
Commit 8e140cb60270 ("drm/rockchip: vop: limit maximum resolution to
hardware capabilities")

Signed-off-by: Alex Bee <knaerzche@gmail.com>
---
 drivers/gpu/drm/rockchip/inno_hdmi.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
index 2f839ff31c1c..84520da8c995 100644
--- a/drivers/gpu/drm/rockchip/inno_hdmi.c
+++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
@@ -696,13 +696,6 @@ inno_hdmi_connector_mode_valid(struct drm_connector *connector,
 	return inno_hdmi_mode_valid(hdmi, mode);
 }
 
-static int
-inno_hdmi_probe_single_connector_modes(struct drm_connector *connector,
-				       uint32_t maxX, uint32_t maxY)
-{
-	return drm_helper_probe_single_connector_modes(connector, 1920, 1080);
-}
-
 static void inno_hdmi_connector_destroy(struct drm_connector *connector)
 {
 	drm_connector_unregister(connector);
@@ -710,7 +703,7 @@ static void inno_hdmi_connector_destroy(struct drm_connector *connector)
 }
 
 static const struct drm_connector_funcs inno_hdmi_connector_funcs = {
-	.fill_modes = inno_hdmi_probe_single_connector_modes,
+	.fill_modes = drm_helper_probe_single_connector_modes,
 	.detect = inno_hdmi_connector_detect,
 	.destroy = inno_hdmi_connector_destroy,
 	.reset = drm_atomic_helper_connector_reset,
-- 
2.43.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 09/11] ARM: dts: rockchip: Add display subsystem for RK3128

[Alex Bee]

Add vop and display-subsysem nodes to RK3128's device tree.

Signed-off-by: Alex Bee <knaerzche@gmail.com>
---
 arch/arm/boot/dts/rockchip/rk3128.dtsi | 27 ++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/arch/arm/boot/dts/rockchip/rk3128.dtsi b/arch/arm/boot/dts/rockchip/rk3128.dtsi
index e2264c40b924..1a3bc8b2bc6e 100644
--- a/arch/arm/boot/dts/rockchip/rk3128.dtsi
+++ b/arch/arm/boot/dts/rockchip/rk3128.dtsi
@@ -115,6 +115,12 @@ opp-1200000000 {
 		};
 	};
 
+	display_subsystem: display-subsystem {
+		compatible = "rockchip,display-subsystem";
+		ports = <&vop_out>;
+		status = "disabled";
+	};
+
 	gpu_opp_table: opp-table-1 {
 		compatible = "operating-points-v2";
 
@@ -246,6 +252,27 @@ power-domain@RK3128_PD_GPU {
 		};
 	};
 
+	vop: vop@1010e000 {
+		compatible = "rockchip,rk3126-vop";
+		reg = <0x1010e000 0x300>;
+		interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&cru ACLK_LCDC0>, <&cru DCLK_VOP>,
+			 <&cru HCLK_LCDC0>;
+		clock-names = "aclk_vop", "dclk_vop",
+			      "hclk_vop";
+		resets = <&cru SRST_VOP_A>, <&cru SRST_VOP_H>,
+			 <&cru SRST_VOP_D>;
+		reset-names = "axi", "ahb",
+			      "dclk";
+		power-domains = <&power RK3128_PD_VIO>;
+		status = "disabled";
+
+		vop_out: port {
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
+	};
+
 	qos_gpu: qos@1012d000 {
 		compatible = "rockchip,rk3128-qos", "syscon";
 		reg = <0x1012d000 0x20>;
-- 
2.43.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 10/11] ARM: dts rockchip: Add HDMI node for RK3128

[Alex Bee]

RK3128 has Innosilicon based HDMI TX controller similar to the one found in
RK3036.
Add it and the respective port nodes to the SoC device tree.

Signed-off-by: Alex Bee <knaerzche@gmail.com>
---
 arch/arm/boot/dts/rockchip/rk3128.dtsi | 33 ++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/arch/arm/boot/dts/rockchip/rk3128.dtsi b/arch/arm/boot/dts/rockchip/rk3128.dtsi
index 1a3bc8b2bc6e..fb98873fd94e 100644
--- a/arch/arm/boot/dts/rockchip/rk3128.dtsi
+++ b/arch/arm/boot/dts/rockchip/rk3128.dtsi
@@ -270,6 +270,11 @@ vop: vop@1010e000 {
 		vop_out: port {
 			#address-cells = <1>;
 			#size-cells = <0>;
+
+			vop_out_hdmi: endpoint@0 {
+				reg = <0>;
+				remote-endpoint = <&hdmi_in_vop>;
+			};
 		};
 	};
 
@@ -463,6 +468,34 @@ usb2phy_otg: otg-port {
 		};
 	};
 
+	hdmi: hdmi@20034000 {
+		compatible = "rockchip,rk3128-inno-hdmi";
+		reg = <0x20034000 0x4000>;
+		interrupts = <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&cru PCLK_HDMI>, <&cru DCLK_VOP>;
+		clock-names = "pclk", "ref";
+		pinctrl-names = "default";
+		pinctrl-0 = <&hdmii2c_xfer &hdmi_hpd &hdmi_cec>;
+		power-domains = <&power RK3128_PD_VIO>;
+		status = "disabled";
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			hdmi_in: port@0 {
+				reg = <0>;
+				hdmi_in_vop: endpoint {
+					remote-endpoint = <&vop_out_hdmi>;
+				};
+			};
+
+			hdmi_out: port@1 {
+				reg = <1>;
+			};
+		};
+	};
+
 	timer0: timer@20044000 {
 		compatible = "rockchip,rk3128-timer", "rockchip,rk3288-timer";
 		reg = <0x20044000 0x20>;
-- 
2.43.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 11/11] ARM: dts: rockchip: Enable HDMI output for XPI-3128

[Alex Bee]

Add an hdmi-connector node and enable the hdmi, display-subsystem and vop
nodes.

Signed-off-by: Alex Bee <knaerzche@gmail.com>
---
 .../arm/boot/dts/rockchip/rk3128-xpi-3128.dts | 29 +++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/arch/arm/boot/dts/rockchip/rk3128-xpi-3128.dts b/arch/arm/boot/dts/rockchip/rk3128-xpi-3128.dts
index 03a97881519a..21c1678f4e91 100644
--- a/arch/arm/boot/dts/rockchip/rk3128-xpi-3128.dts
+++ b/arch/arm/boot/dts/rockchip/rk3128-xpi-3128.dts
@@ -47,6 +47,17 @@ dc_5v: dc-5v-regulator {
 		regulator-boot-on;
 	};
 
+	hdmi-connnector {
+		compatible = "hdmi-connector";
+		type = "a";
+
+		port {
+			hdmi_connector_in: endpoint {
+				remote-endpoint = <&hdmi_connector_out>;
+			};
+		};
+	};
+
 	/*
 	 * This is a vbus-supply, which also supplies the GL852G usb hub,
 	 * thus has to be always-on
@@ -239,6 +250,10 @@ &cpu0 {
 	cpu-supply = <&vdd_arm>;
 };
 
+&display_subsystem {
+	status = "okay";
+};
+
 &emmc {
 	bus-width = <8>;
 	vmmc-supply = <&vcc_io>;
@@ -328,6 +343,16 @@ &gpu {
 	status = "okay";
 };
 
+&hdmi {
+	status = "okay";
+};
+
+&hdmi_out {
+	hdmi_connector_out: endpoint {
+		remote-endpoint = <&hdmi_connector_in>;
+	};
+};
+
 &mdio {
 	phy0: ethernet-phy@1 {
 		compatible = "ethernet-phy-ieee802.3-c22";
@@ -423,3 +448,7 @@ &usb2phy_host {
 &usb2phy_otg {
 	status = "okay";
 };
+
+&vop {
+	status = "okay";
+};
-- 
2.43.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel