[PATCH 01/17] dt-bindings: display: renesas: du: Increase indent in output table

[PATCH 01/17] dt-bindings: display: renesas: du: Increase indent in output table

[Kieran Bingham]

The DU output table lists the port combinations for each supported DU
type.  Newer models of R-Car Gen3 platforms have an increased string
length.

Increase the table indentation in preparation for supporting new target
types.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 .../bindings/display/renesas,du.txt           | 26 +++++++++----------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/renesas,du.txt b/Documentation/devicetree/bindings/display/renesas,du.txt
index c9cd17f99702..a36a6e7ee54f 100644
--- a/Documentation/devicetree/bindings/display/renesas,du.txt
+++ b/Documentation/devicetree/bindings/display/renesas,du.txt
@@ -47,20 +47,20 @@ bindings specified in Documentation/devicetree/bindings/graph.txt.
 The following table lists for each supported model the port number
 corresponding to each DU output.
 
-                      Port0          Port1          Port2          Port3
+                        Port0          Port1          Port2          Port3
 -----------------------------------------------------------------------------
- R8A7743 (RZ/G1M)     DPAD 0         LVDS 0         -              -
- R8A7745 (RZ/G1E)     DPAD 0         DPAD 1         -              -
- R8A7779 (R-Car H1)   DPAD 0         DPAD 1         -              -
- R8A7790 (R-Car H2)   DPAD 0         LVDS 0         LVDS 1         -
- R8A7791 (R-Car M2-W) DPAD 0         LVDS 0         -              -
- R8A7792 (R-Car V2H)  DPAD 0         DPAD 1         -              -
- R8A7793 (R-Car M2-N) DPAD 0         LVDS 0         -              -
- R8A7794 (R-Car E2)   DPAD 0         DPAD 1         -              -
- R8A7795 (R-Car H3)   DPAD 0         HDMI 0         HDMI 1         LVDS 0
- R8A7796 (R-Car M3-W) DPAD 0         HDMI 0         LVDS 0         -
- R8A77970 (R-Car V3M) DPAD 0         LVDS 0         -              -
- R8A77995 (R-Car D3)  DPAD 0         LVDS 0         LVDS 1         -
+ R8A7743 (RZ/G1M)       DPAD 0         LVDS 0         -              -
+ R8A7745 (RZ/G1E)       DPAD 0         DPAD 1         -              -
+ R8A7779 (R-Car H1)     DPAD 0         DPAD 1         -              -
+ R8A7790 (R-Car H2)     DPAD 0         LVDS 0         LVDS 1         -
+ R8A7791 (R-Car M2-W)   DPAD 0         LVDS 0         -              -
+ R8A7792 (R-Car V2H)    DPAD 0         DPAD 1         -              -
+ R8A7793 (R-Car M2-N)   DPAD 0         LVDS 0         -              -
+ R8A7794 (R-Car E2)     DPAD 0         DPAD 1         -              -
+ R8A7795 (R-Car H3)     DPAD 0         HDMI 0         HDMI 1         LVDS 0
+ R8A7796 (R-Car M3-W)   DPAD 0         HDMI 0         LVDS 0         -
+ R8A77970 (R-Car V3M)   DPAD 0         LVDS 0         -              -
+ R8A77995 (R-Car D3)    DPAD 0         LVDS 0         LVDS 1         -
 
 
 Example: R8A7795 (R-Car H3) ES2.0 DU
-- 
2.17.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/17] dt-bindings: display: renesas: du: Increase indent in output table

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Thursday, 26 April 2018 19:53:30 EEST Kieran Bingham wrote:
> The DU output table lists the port combinations for each supported DU
> type.  Newer models of R-Car Gen3 platforms have an increased string
> length.
> 
> Increase the table indentation in preparation for supporting new target
> types.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

and applied to my tree.

> ---
>  .../bindings/display/renesas,du.txt           | 26 +++++++++----------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/renesas,du.txt
> b/Documentation/devicetree/bindings/display/renesas,du.txt index
> c9cd17f99702..a36a6e7ee54f 100644
> --- a/Documentation/devicetree/bindings/display/renesas,du.txt
> +++ b/Documentation/devicetree/bindings/display/renesas,du.txt
> @@ -47,20 +47,20 @@ bindings specified in
> Documentation/devicetree/bindings/graph.txt. The following table lists for
> each supported model the port number corresponding to each DU output.
> 
> -                      Port0          Port1          Port2          Port3
> +                        Port0          Port1          Port2          Port3
>  ---------------------------------------------------------------------------
> -- - R8A7743 (RZ/G1M)     DPAD 0         LVDS 0         -              - -
> R8A7745 (RZ/G1E)     DPAD 0         DPAD 1         -              - -
> R8A7779 (R-Car H1)   DPAD 0         DPAD 1         -              - -
> R8A7790 (R-Car H2)   DPAD 0         LVDS 0         LVDS 1         - -
> R8A7791 (R-Car M2-W) DPAD 0         LVDS 0         -              - -
> R8A7792 (R-Car V2H)  DPAD 0         DPAD 1         -              - -
> R8A7793 (R-Car M2-N) DPAD 0         LVDS 0         -              - -
> R8A7794 (R-Car E2)   DPAD 0         DPAD 1         -              - -
> R8A7795 (R-Car H3)   DPAD 0         HDMI 0         HDMI 1         LVDS 0 -
> R8A7796 (R-Car M3-W) DPAD 0         HDMI 0         LVDS 0         - -
> R8A77970 (R-Car V3M) DPAD 0         LVDS 0         -              - -
> R8A77995 (R-Car D3)  DPAD 0         LVDS 0         LVDS 1         - +
> R8A7743 (RZ/G1M)       DPAD 0         LVDS 0         -              - +
> R8A7745 (RZ/G1E)       DPAD 0         DPAD 1         -              - +
> R8A7779 (R-Car H1)     DPAD 0         DPAD 1         -              - +
> R8A7790 (R-Car H2)     DPAD 0         LVDS 0         LVDS 1         - +
> R8A7791 (R-Car M2-W)   DPAD 0         LVDS 0         -              - +
> R8A7792 (R-Car V2H)    DPAD 0         DPAD 1         -              - +
> R8A7793 (R-Car M2-N)   DPAD 0         LVDS 0         -              - +
> R8A7794 (R-Car E2)     DPAD 0         DPAD 1         -              - +
> R8A7795 (R-Car H3)     DPAD 0         HDMI 0         HDMI 1         LVDS 0
> + R8A7796 (R-Car M3-W)   DPAD 0         HDMI 0         LVDS 0         - +
> R8A77970 (R-Car V3M)   DPAD 0         LVDS 0         -              - +
> R8A77995 (R-Car D3)    DPAD 0         LVDS 0         LVDS 1         -
> 
> 
>  Example: R8A7795 (R-Car H3) ES2.0 DU


-- 
Regards,

Laurent Pinchart



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 02/17] dt-bindings: display: renesas: du: Document the R8A77965 bindings

[Kieran Bingham]

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 Documentation/devicetree/bindings/display/renesas,du.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/renesas,du.txt b/Documentation/devicetree/bindings/display/renesas,du.txt
index a36a6e7ee54f..7c6854bd0a04 100644
--- a/Documentation/devicetree/bindings/display/renesas,du.txt
+++ b/Documentation/devicetree/bindings/display/renesas,du.txt
@@ -13,6 +13,7 @@ Required Properties:
     - "renesas,du-r8a7794" for R8A7794 (R-Car E2) compatible DU
     - "renesas,du-r8a7795" for R8A7795 (R-Car H3) compatible DU
     - "renesas,du-r8a7796" for R8A7796 (R-Car M3-W) compatible DU
+    - "renesas,du-r8a77965" for R8A77965 (R-Car M3-N) compatible DU
     - "renesas,du-r8a77970" for R8A77970 (R-Car V3M) compatible DU
     - "renesas,du-r8a77995" for R8A77995 (R-Car D3) compatible DU
 
@@ -59,6 +60,7 @@ corresponding to each DU output.
  R8A7794 (R-Car E2)     DPAD 0         DPAD 1         -              -
  R8A7795 (R-Car H3)     DPAD 0         HDMI 0         HDMI 1         LVDS 0
  R8A7796 (R-Car M3-W)   DPAD 0         HDMI 0         LVDS 0         -
+ R8A77965 (R-Car M3-N)  DPAD 0         HDMI 0         LVDS 0         -
  R8A77970 (R-Car V3M)   DPAD 0         LVDS 0         -              -
  R8A77995 (R-Car D3)    DPAD 0         LVDS 0         LVDS 1         -
 
-- 
2.17.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 02/17] dt-bindings: display: renesas: du: Document the R8A77965 bindings

[Kieran Bingham]

Ahem - this one seems to have lost it's commit message.

Apologies :)

--
Kieran


On 26/04/18 17:53, Kieran Bingham wrote:
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  Documentation/devicetree/bindings/display/renesas,du.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/renesas,du.txt b/Documentation/devicetree/bindings/display/renesas,du.txt
> index a36a6e7ee54f..7c6854bd0a04 100644
> --- a/Documentation/devicetree/bindings/display/renesas,du.txt
> +++ b/Documentation/devicetree/bindings/display/renesas,du.txt
> @@ -13,6 +13,7 @@ Required Properties:
>      - "renesas,du-r8a7794" for R8A7794 (R-Car E2) compatible DU
>      - "renesas,du-r8a7795" for R8A7795 (R-Car H3) compatible DU
>      - "renesas,du-r8a7796" for R8A7796 (R-Car M3-W) compatible DU
> +    - "renesas,du-r8a77965" for R8A77965 (R-Car M3-N) compatible DU
>      - "renesas,du-r8a77970" for R8A77970 (R-Car V3M) compatible DU
>      - "renesas,du-r8a77995" for R8A77995 (R-Car D3) compatible DU
>  
> @@ -59,6 +60,7 @@ corresponding to each DU output.
>   R8A7794 (R-Car E2)     DPAD 0         DPAD 1         -              -
>   R8A7795 (R-Car H3)     DPAD 0         HDMI 0         HDMI 1         LVDS 0
>   R8A7796 (R-Car M3-W)   DPAD 0         HDMI 0         LVDS 0         -
> + R8A77965 (R-Car M3-N)  DPAD 0         HDMI 0         LVDS 0         -
>   R8A77970 (R-Car V3M)   DPAD 0         LVDS 0         -              -
>   R8A77995 (R-Car D3)    DPAD 0         LVDS 0         LVDS 1         -
>  
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 02/17] dt-bindings: display: renesas: du: Document the R8A77965 bindings

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Thursday, 26 April 2018 19:57:32 EEST Kieran Bingham wrote:
> Ahem - this one seems to have lost it's commit message.
> 
> Apologies :)

Apart from that, this looks good to me.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

and applied to my tree with the commit message

Document the M3-N (r8a77965) SoC in the R-Car DU bindings

Let me know if you would like a different message.

> On 26/04/18 17:53, Kieran Bingham wrote:
> > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > ---
> > 
> >  Documentation/devicetree/bindings/display/renesas,du.txt | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/display/renesas,du.txt
> > b/Documentation/devicetree/bindings/display/renesas,du.txt index
> > a36a6e7ee54f..7c6854bd0a04 100644
> > --- a/Documentation/devicetree/bindings/display/renesas,du.txt
> > +++ b/Documentation/devicetree/bindings/display/renesas,du.txt
> > 
> > @@ -13,6 +13,7 @@ Required Properties:
> >      - "renesas,du-r8a7794" for R8A7794 (R-Car E2) compatible DU
> >      - "renesas,du-r8a7795" for R8A7795 (R-Car H3) compatible DU
> >      - "renesas,du-r8a7796" for R8A7796 (R-Car M3-W) compatible DU
> > +    - "renesas,du-r8a77965" for R8A77965 (R-Car M3-N) compatible DU
> >      - "renesas,du-r8a77970" for R8A77970 (R-Car V3M) compatible DU
> >      - "renesas,du-r8a77995" for R8A77995 (R-Car D3) compatible DU
> > 
> > @@ -59,6 +60,7 @@ corresponding to each DU output.
> > 
> >   R8A7794 (R-Car E2)     DPAD 0         DPAD 1         -              -
> >   R8A7795 (R-Car H3)     DPAD 0         HDMI 0         HDMI 1         LVDS
> >   0
> >   R8A7796 (R-Car M3-W)   DPAD 0         HDMI 0         LVDS 0         -
> > + R8A77965 (R-Car M3-N)  DPAD 0         HDMI 0         LVDS 0         -
> >   R8A77970 (R-Car V3M)   DPAD 0         LVDS 0         -              -
> >   R8A77995 (R-Car D3)    DPAD 0         LVDS 0         LVDS 1         -

-- 
Regards,

Laurent Pinchart



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 02/17] dt-bindings: display: renesas: du: Document the R8A77965 bindings

[Kieran Bingham]

[-- Attachment #1.1.1: Type: text/plain, Size: 2231 bytes --]

Hi Laurent,

On 26/04/18 21:10, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Thursday, 26 April 2018 19:57:32 EEST Kieran Bingham wrote:
>> Ahem - this one seems to have lost it's commit message.
>>
>> Apologies :)
> 
> Apart from that, this looks good to me.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> and applied to my tree with the commit message
> 
> Document the M3-N (r8a77965) SoC in the R-Car DU bindings
> 

That's perfect, thanks - and saves me sending a v1.1

Regards

Kieran

> Let me know if you would like a different message.
> 
>> On 26/04/18 17:53, Kieran Bingham wrote:
>>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>> ---
>>>
>>>  Documentation/devicetree/bindings/display/renesas,du.txt | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/display/renesas,du.txt
>>> b/Documentation/devicetree/bindings/display/renesas,du.txt index
>>> a36a6e7ee54f..7c6854bd0a04 100644
>>> --- a/Documentation/devicetree/bindings/display/renesas,du.txt
>>> +++ b/Documentation/devicetree/bindings/display/renesas,du.txt
>>>
>>> @@ -13,6 +13,7 @@ Required Properties:
>>>      - "renesas,du-r8a7794" for R8A7794 (R-Car E2) compatible DU
>>>      - "renesas,du-r8a7795" for R8A7795 (R-Car H3) compatible DU
>>>      - "renesas,du-r8a7796" for R8A7796 (R-Car M3-W) compatible DU
>>> +    - "renesas,du-r8a77965" for R8A77965 (R-Car M3-N) compatible DU
>>>      - "renesas,du-r8a77970" for R8A77970 (R-Car V3M) compatible DU
>>>      - "renesas,du-r8a77995" for R8A77995 (R-Car D3) compatible DU
>>>
>>> @@ -59,6 +60,7 @@ corresponding to each DU output.
>>>
>>>   R8A7794 (R-Car E2)     DPAD 0         DPAD 1         -              -
>>>   R8A7795 (R-Car H3)     DPAD 0         HDMI 0         HDMI 1         LVDS
>>>   0
>>>   R8A7796 (R-Car M3-W)   DPAD 0         HDMI 0         LVDS 0         -
>>> + R8A77965 (R-Car M3-N)  DPAD 0         HDMI 0         LVDS 0         -
>>>   R8A77970 (R-Car V3M)   DPAD 0         LVDS 0         -              -
>>>   R8A77995 (R-Car D3)    DPAD 0         LVDS 0         LVDS 1         -
> 


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 04/17] drm: rcar-du: Use the correct naming for ODPM fields in DEFR6

[Kieran Bingham]

The naming of the fields for the ODPM signals in the DU extensional
function control register 6 (DEFR6) is incorrect against the data sheets
for both R-Car Gen2 and R-Car Gen3.

Rename the fields to match the datasheet.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_group.c |  4 ++--
 drivers/gpu/drm/rcar-du/rcar_du_regs.h  | 16 ++++++++--------
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c b/drivers/gpu/drm/rcar-du/rcar_du_group.c
index 2f37ea901873..eead202c95c7 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
@@ -46,10 +46,10 @@ void rcar_du_group_write(struct rcar_du_group *rgrp, u32 reg, u32 data)
 
 static void rcar_du_group_setup_pins(struct rcar_du_group *rgrp)
 {
-	u32 defr6 = DEFR6_CODE | DEFR6_ODPM12_DISP;
+	u32 defr6 = DEFR6_CODE | DEFR6_ODPM02_DISP;
 
 	if (rgrp->num_crtcs > 1)
-		defr6 |= DEFR6_ODPM22_DISP;
+		defr6 |= DEFR6_ODPM12_DISP;
 
 	rcar_du_group_write(rgrp, DEFR6, defr6);
 }
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_regs.h b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
index d5bae99d3cfe..9dfd220ceda1 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_regs.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
@@ -187,14 +187,14 @@
 
 #define DEFR6			0x000e8
 #define DEFR6_CODE		(0x7778 << 16)
-#define DEFR6_ODPM22_DSMR	(0 << 10)
-#define DEFR6_ODPM22_DISP	(2 << 10)
-#define DEFR6_ODPM22_CDE	(3 << 10)
-#define DEFR6_ODPM22_MASK	(3 << 10)
-#define DEFR6_ODPM12_DSMR	(0 << 8)
-#define DEFR6_ODPM12_DISP	(2 << 8)
-#define DEFR6_ODPM12_CDE	(3 << 8)
-#define DEFR6_ODPM12_MASK	(3 << 8)
+#define DEFR6_ODPM12_DSMR	(0 << 10)
+#define DEFR6_ODPM12_DISP	(2 << 10)
+#define DEFR6_ODPM12_CDE	(3 << 10)
+#define DEFR6_ODPM12_MASK	(3 << 10)
+#define DEFR6_ODPM02_DSMR	(0 << 8)
+#define DEFR6_ODPM02_DISP	(2 << 8)
+#define DEFR6_ODPM02_CDE	(3 << 8)
+#define DEFR6_ODPM02_MASK	(3 << 8)
 #define DEFR6_TCNE1		(1 << 6)
 #define DEFR6_TCNE0		(1 << 4)
 #define DEFR6_MLOS1		(1 << 2)
-- 
2.17.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 04/17] drm: rcar-du: Use the correct naming for ODPM fields in DEFR6

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Thursday, 26 April 2018 19:53:33 EEST Kieran Bingham wrote:
> The naming of the fields for the ODPM signals in the DU extensional
> function control register 6 (DEFR6) is incorrect against the data sheets
> for both R-Car Gen2 and R-Car Gen3.
> 
> Rename the fields to match the datasheet.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

and taken in my tree.

> ---
>  drivers/gpu/drm/rcar-du/rcar_du_group.c |  4 ++--
>  drivers/gpu/drm/rcar-du/rcar_du_regs.h  | 16 ++++++++--------
>  2 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c
> b/drivers/gpu/drm/rcar-du/rcar_du_group.c index 2f37ea901873..eead202c95c7
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> @@ -46,10 +46,10 @@ void rcar_du_group_write(struct rcar_du_group *rgrp, u32
> reg, u32 data)
> 
>  static void rcar_du_group_setup_pins(struct rcar_du_group *rgrp)
>  {
> -	u32 defr6 = DEFR6_CODE | DEFR6_ODPM12_DISP;
> +	u32 defr6 = DEFR6_CODE | DEFR6_ODPM02_DISP;
> 
>  	if (rgrp->num_crtcs > 1)
> -		defr6 |= DEFR6_ODPM22_DISP;
> +		defr6 |= DEFR6_ODPM12_DISP;
> 
>  	rcar_du_group_write(rgrp, DEFR6, defr6);
>  }
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_regs.h
> b/drivers/gpu/drm/rcar-du/rcar_du_regs.h index d5bae99d3cfe..9dfd220ceda1
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_regs.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
> @@ -187,14 +187,14 @@
> 
>  #define DEFR6			0x000e8
>  #define DEFR6_CODE		(0x7778 << 16)
> -#define DEFR6_ODPM22_DSMR	(0 << 10)
> -#define DEFR6_ODPM22_DISP	(2 << 10)
> -#define DEFR6_ODPM22_CDE	(3 << 10)
> -#define DEFR6_ODPM22_MASK	(3 << 10)
> -#define DEFR6_ODPM12_DSMR	(0 << 8)
> -#define DEFR6_ODPM12_DISP	(2 << 8)
> -#define DEFR6_ODPM12_CDE	(3 << 8)
> -#define DEFR6_ODPM12_MASK	(3 << 8)
> +#define DEFR6_ODPM12_DSMR	(0 << 10)
> +#define DEFR6_ODPM12_DISP	(2 << 10)
> +#define DEFR6_ODPM12_CDE	(3 << 10)
> +#define DEFR6_ODPM12_MASK	(3 << 10)
> +#define DEFR6_ODPM02_DSMR	(0 << 8)
> +#define DEFR6_ODPM02_DISP	(2 << 8)
> +#define DEFR6_ODPM02_CDE	(3 << 8)
> +#define DEFR6_ODPM02_MASK	(3 << 8)
>  #define DEFR6_TCNE1		(1 << 6)
>  #define DEFR6_TCNE0		(1 << 4)
>  #define DEFR6_MLOS1		(1 << 2)

-- 
Regards,

Laurent Pinchart

[PATCH 05/17] drm: rcar-du: Split CRTC handling to support hardware indexing

[Kieran Bingham]

The DU CRTC driver does not support distinguishing between a hardware
index, and a software (CRTC) index in the event that a DU channel might
not be populated by the hardware.

Support this by adapting the rcar_du_device_info structure to store a
bitmask of available channels rather than a count of CRTCs. The count
can then be obtained by determining the hamming weight of the bitmask.

This allows the rcar_du_crtc_create() function to distinguish between
both index types, and non-populated DU channels will be skipped without
leaving a gap in the software CRTC indexes.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 26 ++++++++++++++------------
 drivers/gpu/drm/rcar-du/rcar_du_crtc.h |  3 ++-
 drivers/gpu/drm/rcar-du/rcar_du_drv.c  | 20 ++++++++++----------
 drivers/gpu/drm/rcar-du/rcar_du_drv.h  |  4 ++--
 drivers/gpu/drm/rcar-du/rcar_du_kms.c  | 17 ++++++++++++-----
 5 files changed, 40 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 5a15dfd66343..36ce194c13b5 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -902,7 +902,8 @@ static irqreturn_t rcar_du_crtc_irq(int irq, void *arg)
  * Initialization
  */
 
-int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int index)
+int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int swindex,
+			unsigned int hwindex)
 {
 	static const unsigned int mmio_offsets[] = {
 		DU0_REG_OFFSET, DU1_REG_OFFSET, DU2_REG_OFFSET, DU3_REG_OFFSET
@@ -910,7 +911,7 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int index)
 
 	struct rcar_du_device *rcdu = rgrp->dev;
 	struct platform_device *pdev = to_platform_device(rcdu->dev);
-	struct rcar_du_crtc *rcrtc = &rcdu->crtcs[index];
+	struct rcar_du_crtc *rcrtc = &rcdu->crtcs[swindex];
 	struct drm_crtc *crtc = &rcrtc->crtc;
 	struct drm_plane *primary;
 	unsigned int irqflags;
@@ -922,7 +923,7 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int index)
 
 	/* Get the CRTC clock and the optional external clock. */
 	if (rcar_du_has(rcdu, RCAR_DU_FEATURE_CRTC_IRQ_CLOCK)) {
-		sprintf(clk_name, "du.%u", index);
+		sprintf(clk_name, "du.%u", hwindex);
 		name = clk_name;
 	} else {
 		name = NULL;
@@ -930,16 +931,16 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int index)
 
 	rcrtc->clock = devm_clk_get(rcdu->dev, name);
 	if (IS_ERR(rcrtc->clock)) {
-		dev_err(rcdu->dev, "no clock for CRTC %u\n", index);
+		dev_err(rcdu->dev, "no clock for CRTC %u\n", swindex);
 		return PTR_ERR(rcrtc->clock);
 	}
 
-	sprintf(clk_name, "dclkin.%u", index);
+	sprintf(clk_name, "dclkin.%u", hwindex);
 	clk = devm_clk_get(rcdu->dev, clk_name);
 	if (!IS_ERR(clk)) {
 		rcrtc->extclock = clk;
 	} else if (PTR_ERR(rcrtc->clock) == -EPROBE_DEFER) {
-		dev_info(rcdu->dev, "can't get external clock %u\n", index);
+		dev_info(rcdu->dev, "can't get external clock %u\n", hwindex);
 		return -EPROBE_DEFER;
 	}
 
@@ -948,13 +949,13 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int index)
 	spin_lock_init(&rcrtc->vblank_lock);
 
 	rcrtc->group = rgrp;
-	rcrtc->mmio_offset = mmio_offsets[index];
-	rcrtc->index = index;
+	rcrtc->mmio_offset = mmio_offsets[hwindex];
+	rcrtc->index = hwindex;
 
 	if (rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE))
 		primary = &rcrtc->vsp->planes[rcrtc->vsp_pipe].plane;
 	else
-		primary = &rgrp->planes[index % 2].plane;
+		primary = &rgrp->planes[hwindex % 2].plane;
 
 	ret = drm_crtc_init_with_planes(rcdu->ddev, crtc, primary, NULL,
 					rcdu->info->gen <= 2 ?
@@ -970,7 +971,8 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int index)
 
 	/* Register the interrupt handler. */
 	if (rcar_du_has(rcdu, RCAR_DU_FEATURE_CRTC_IRQ_CLOCK)) {
-		irq = platform_get_irq(pdev, index);
+		/* The IRQ's are associated with the CRTC (sw)index */
+		irq = platform_get_irq(pdev, swindex);
 		irqflags = 0;
 	} else {
 		irq = platform_get_irq(pdev, 0);
@@ -978,7 +980,7 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int index)
 	}
 
 	if (irq < 0) {
-		dev_err(rcdu->dev, "no IRQ for CRTC %u\n", index);
+		dev_err(rcdu->dev, "no IRQ for CRTC %u\n", swindex);
 		return irq;
 	}
 
@@ -986,7 +988,7 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int index)
 			       dev_name(rcdu->dev), rcrtc);
 	if (ret < 0) {
 		dev_err(rcdu->dev,
-			"failed to register IRQ for CRTC %u\n", index);
+			"failed to register IRQ for CRTC %u\n", swindex);
 		return ret;
 	}
 
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
index 518ee2c60eb8..5f003a16abc5 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
@@ -99,7 +99,8 @@ enum rcar_du_output {
 	RCAR_DU_OUTPUT_MAX,
 };
 
-int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int index);
+int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int swindex,
+			unsigned int hwindex);
 void rcar_du_crtc_suspend(struct rcar_du_crtc *rcrtc);
 void rcar_du_crtc_resume(struct rcar_du_crtc *rcrtc);
 
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
index 05745e86d73e..d6ebc628fc22 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
@@ -40,7 +40,7 @@ static const struct rcar_du_device_info rzg1_du_r8a7743_info = {
 	.gen = 2,
 	.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
 		  | RCAR_DU_FEATURE_EXT_CTRL_REGS,
-	.num_crtcs = 2,
+	.channel_mask = BIT(0) | BIT(1),
 	.routes = {
 		/*
 		 * R8A7743 has one RGB output and one LVDS output
@@ -61,7 +61,7 @@ static const struct rcar_du_device_info rzg1_du_r8a7745_info = {
 	.gen = 2,
 	.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
 		  | RCAR_DU_FEATURE_EXT_CTRL_REGS,
-	.num_crtcs = 2,
+	.channel_mask = BIT(0) | BIT(1),
 	.routes = {
 		/*
 		 * R8A7745 has two RGB outputs
@@ -80,7 +80,7 @@ static const struct rcar_du_device_info rzg1_du_r8a7745_info = {
 static const struct rcar_du_device_info rcar_du_r8a7779_info = {
 	.gen = 2,
 	.features = 0,
-	.num_crtcs = 2,
+	.channel_mask = BIT(0) | BIT(1),
 	.routes = {
 		/*
 		 * R8A7779 has two RGB outputs and one (currently unsupported)
@@ -102,7 +102,7 @@ static const struct rcar_du_device_info rcar_du_r8a7790_info = {
 	.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
 		  | RCAR_DU_FEATURE_EXT_CTRL_REGS,
 	.quirks = RCAR_DU_QUIRK_ALIGN_128B,
-	.num_crtcs = 3,
+	.channel_mask = BIT(0) | BIT(1) | BIT(2),
 	.routes = {
 		/*
 		 * R8A7790 has one RGB output, two LVDS outputs and one
@@ -129,7 +129,7 @@ static const struct rcar_du_device_info rcar_du_r8a7791_info = {
 	.gen = 2,
 	.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
 		  | RCAR_DU_FEATURE_EXT_CTRL_REGS,
-	.num_crtcs = 2,
+	.channel_mask = BIT(0) | BIT(1),
 	.routes = {
 		/*
 		 * R8A779[13] has one RGB output, one LVDS output and one
@@ -151,7 +151,7 @@ static const struct rcar_du_device_info rcar_du_r8a7792_info = {
 	.gen = 2,
 	.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
 		  | RCAR_DU_FEATURE_EXT_CTRL_REGS,
-	.num_crtcs = 2,
+	.channel_mask = BIT(0) | BIT(1),
 	.routes = {
 		/* R8A7792 has two RGB outputs. */
 		[RCAR_DU_OUTPUT_DPAD0] = {
@@ -169,7 +169,7 @@ static const struct rcar_du_device_info rcar_du_r8a7794_info = {
 	.gen = 2,
 	.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
 		  | RCAR_DU_FEATURE_EXT_CTRL_REGS,
-	.num_crtcs = 2,
+	.channel_mask = BIT(0) | BIT(1),
 	.routes = {
 		/*
 		 * R8A7794 has two RGB outputs and one (currently unsupported)
@@ -191,7 +191,7 @@ static const struct rcar_du_device_info rcar_du_r8a7795_info = {
 	.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
 		  | RCAR_DU_FEATURE_EXT_CTRL_REGS
 		  | RCAR_DU_FEATURE_VSP1_SOURCE,
-	.num_crtcs = 4,
+	.channel_mask = BIT(0) | BIT(1) | BIT(2) | BIT(3),
 	.routes = {
 		/*
 		 * R8A7795 has one RGB output, two HDMI outputs and one
@@ -223,7 +223,7 @@ static const struct rcar_du_device_info rcar_du_r8a7796_info = {
 	.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
 		  | RCAR_DU_FEATURE_EXT_CTRL_REGS
 		  | RCAR_DU_FEATURE_VSP1_SOURCE,
-	.num_crtcs = 3,
+	.channel_mask = BIT(0) | BIT(1) | BIT(2),
 	.routes = {
 		/*
 		 * R8A7796 has one RGB output, one LVDS output and one HDMI
@@ -251,7 +251,7 @@ static const struct rcar_du_device_info rcar_du_r8a77970_info = {
 	.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
 		  | RCAR_DU_FEATURE_EXT_CTRL_REGS
 		  | RCAR_DU_FEATURE_VSP1_SOURCE,
-	.num_crtcs = 1,
+	.channel_mask = BIT(0),
 	.routes = {
 		/* R8A77970 has one RGB output and one LVDS output. */
 		[RCAR_DU_OUTPUT_DPAD0] = {
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
index 5c7ec15818c7..7a5de66deec2 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
@@ -52,7 +52,7 @@ struct rcar_du_output_routing {
  * @gen: device generation (2 or 3)
  * @features: device features (RCAR_DU_FEATURE_*)
  * @quirks: device quirks (RCAR_DU_QUIRK_*)
- * @num_crtcs: total number of CRTCs
+ * @channel_mask: bit mask of supported DU channels
  * @routes: array of CRTC to output routes, indexed by output (RCAR_DU_OUTPUT_*)
  * @num_lvds: number of internal LVDS encoders
  */
@@ -60,7 +60,7 @@ struct rcar_du_device_info {
 	unsigned int gen;
 	unsigned int features;
 	unsigned int quirks;
-	unsigned int num_crtcs;
+	unsigned int channel_mask;
 	struct rcar_du_output_routing routes[RCAR_DU_OUTPUT_MAX];
 	unsigned int num_lvds;
 	unsigned int dpll_ch;
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
index cf5b422fc753..19a445fbc879 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -559,6 +559,7 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
 	struct drm_fbdev_cma *fbdev;
 	unsigned int num_encoders;
 	unsigned int num_groups;
+	unsigned int swi, hwi;
 	unsigned int i;
 	int ret;
 
@@ -571,7 +572,7 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
 	dev->mode_config.funcs = &rcar_du_mode_config_funcs;
 	dev->mode_config.helper_private = &rcar_du_mode_config_helper;
 
-	rcdu->num_crtcs = rcdu->info->num_crtcs;
+	rcdu->num_crtcs = hweight8(rcdu->info->channel_mask);
 
 	ret = rcar_du_properties_init(rcdu);
 	if (ret < 0)
@@ -581,7 +582,7 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
 	 * Initialize vertical blanking interrupts handling. Start with vblank
 	 * disabled for all CRTCs.
 	 */
-	ret = drm_vblank_init(dev, (1 << rcdu->info->num_crtcs) - 1);
+	ret = drm_vblank_init(dev, (1 << rcdu->num_crtcs) - 1);
 	if (ret < 0)
 		return ret;
 
@@ -623,10 +624,16 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
 	}
 
 	/* Create the CRTCs. */
-	for (i = 0; i < rcdu->num_crtcs; ++i) {
-		struct rcar_du_group *rgrp = &rcdu->groups[i / 2];
+	for (swi = 0, hwi = 0; swi < rcdu->num_crtcs; ++hwi) {
+		struct rcar_du_group *rgrp;
+
+		/* Skip unpopulated DU channels */
+		if (!(rcdu->info->channel_mask & BIT(hwi)))
+			continue;
+
+		rgrp = &rcdu->groups[hwi / 2];
 
-		ret = rcar_du_crtc_create(rgrp, i);
+		ret = rcar_du_crtc_create(rgrp, swi++, hwi);
 		if (ret < 0)
 			return ret;
 	}
-- 
2.17.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 05/17] drm: rcar-du: Split CRTC handling to support hardware indexing

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Thursday, 26 April 2018 19:53:34 EEST Kieran Bingham wrote:
> The DU CRTC driver does not support distinguishing between a hardware
> index, and a software (CRTC) index in the event that a DU channel might
> not be populated by the hardware.
> 
> Support this by adapting the rcar_du_device_info structure to store a
> bitmask of available channels rather than a count of CRTCs. The count
> can then be obtained by determining the hamming weight of the bitmask.
> 
> This allows the rcar_du_crtc_create() function to distinguish between
> both index types, and non-populated DU channels will be skipped without
> leaving a gap in the software CRTC indexes.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 26 ++++++++++++++------------
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h |  3 ++-
>  drivers/gpu/drm/rcar-du/rcar_du_drv.c  | 20 ++++++++++----------
>  drivers/gpu/drm/rcar-du/rcar_du_drv.h  |  4 ++--
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c  | 17 ++++++++++++-----
>  5 files changed, 40 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 5a15dfd66343..36ce194c13b5
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -902,7 +902,8 @@ static irqreturn_t rcar_du_crtc_irq(int irq, void *arg)
>   * Initialization
>   */
> 
> -int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int index)
> +int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int swindex,
> +			unsigned int hwindex)
>  {
>  	static const unsigned int mmio_offsets[] = {
>  		DU0_REG_OFFSET, DU1_REG_OFFSET, DU2_REG_OFFSET, DU3_REG_OFFSET
> @@ -910,7 +911,7 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp,
> unsigned int index)
> 
>  	struct rcar_du_device *rcdu = rgrp->dev;
>  	struct platform_device *pdev = to_platform_device(rcdu->dev);
> -	struct rcar_du_crtc *rcrtc = &rcdu->crtcs[index];
> +	struct rcar_du_crtc *rcrtc = &rcdu->crtcs[swindex];
>  	struct drm_crtc *crtc = &rcrtc->crtc;
>  	struct drm_plane *primary;
>  	unsigned int irqflags;
> @@ -922,7 +923,7 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp,
> unsigned int index)
> 
>  	/* Get the CRTC clock and the optional external clock. */
>  	if (rcar_du_has(rcdu, RCAR_DU_FEATURE_CRTC_IRQ_CLOCK)) {
> -		sprintf(clk_name, "du.%u", index);
> +		sprintf(clk_name, "du.%u", hwindex);
>  		name = clk_name;
>  	} else {
>  		name = NULL;
> @@ -930,16 +931,16 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp,
> unsigned int index)
> 
>  	rcrtc->clock = devm_clk_get(rcdu->dev, name);
>  	if (IS_ERR(rcrtc->clock)) {
> -		dev_err(rcdu->dev, "no clock for CRTC %u\n", index);
> +		dev_err(rcdu->dev, "no clock for CRTC %u\n", swindex);

How about

		dev_err(rcdu->dev, "no clock for DU channel %u\n", hwindex);

I think that would be clearer, because at this stage we're dealing with 
hardware resources, so matching the datasheet numbers seems better to me.

>  		return PTR_ERR(rcrtc->clock);
>  	}
> 
> -	sprintf(clk_name, "dclkin.%u", index);
> +	sprintf(clk_name, "dclkin.%u", hwindex);
>  	clk = devm_clk_get(rcdu->dev, clk_name);
>  	if (!IS_ERR(clk)) {
>  		rcrtc->extclock = clk;
>  	} else if (PTR_ERR(rcrtc->clock) == -EPROBE_DEFER) {
> -		dev_info(rcdu->dev, "can't get external clock %u\n", index);
> +		dev_info(rcdu->dev, "can't get external clock %u\n", hwindex);
>  		return -EPROBE_DEFER;
>  	}
> 
> @@ -948,13 +949,13 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp,
> unsigned int index) spin_lock_init(&rcrtc->vblank_lock);
> 
>  	rcrtc->group = rgrp;
> -	rcrtc->mmio_offset = mmio_offsets[index];
> -	rcrtc->index = index;
> +	rcrtc->mmio_offset = mmio_offsets[hwindex];
> +	rcrtc->index = hwindex;
> 
>  	if (rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE))
>  		primary = &rcrtc->vsp->planes[rcrtc->vsp_pipe].plane;
>  	else
> -		primary = &rgrp->planes[index % 2].plane;
> +		primary = &rgrp->planes[hwindex % 2].plane;

This shouldn't make a difference because when RCAR_DU_FEATURE_VSP1_SOURCE 
isn't set we're running on Gen2, and don't need to deal with indices, but from 
a conceptual point of view, wouldn't the software index be better here ? 
Missing hardware channels won't be visible from userspace, so taking the first 
plane of the group as the primary plane would seem better to me.

>  	ret = drm_crtc_init_with_planes(rcdu->ddev, crtc, primary, NULL,
>  					rcdu->info->gen <= 2 ?
> @@ -970,7 +971,8 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp,
> unsigned int index)
> 
>  	/* Register the interrupt handler. */
>  	if (rcar_du_has(rcdu, RCAR_DU_FEATURE_CRTC_IRQ_CLOCK)) {
> -		irq = platform_get_irq(pdev, index);
> +		/* The IRQ's are associated with the CRTC (sw)index */

s/index/index./

> +		irq = platform_get_irq(pdev, swindex);
>  		irqflags = 0;
>  	} else {
>  		irq = platform_get_irq(pdev, 0);
> @@ -978,7 +980,7 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp,
> unsigned int index) }
> 
>  	if (irq < 0) {
> -		dev_err(rcdu->dev, "no IRQ for CRTC %u\n", index);
> +		dev_err(rcdu->dev, "no IRQ for CRTC %u\n", swindex);
>  		return irq;
>  	}
> 
> @@ -986,7 +988,7 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp,
> unsigned int index) dev_name(rcdu->dev), rcrtc);
>  	if (ret < 0) {
>  		dev_err(rcdu->dev,
> -			"failed to register IRQ for CRTC %u\n", index);
> +			"failed to register IRQ for CRTC %u\n", swindex);
>  		return ret;
>  	}
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h index 518ee2c60eb8..5f003a16abc5
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> @@ -99,7 +99,8 @@ enum rcar_du_output {
>  	RCAR_DU_OUTPUT_MAX,
>  };
> 
> -int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int index);
> +int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int swindex,
> +			unsigned int hwindex);
>  void rcar_du_crtc_suspend(struct rcar_du_crtc *rcrtc);
>  void rcar_du_crtc_resume(struct rcar_du_crtc *rcrtc);
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 05745e86d73e..d6ebc628fc22
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> @@ -40,7 +40,7 @@ static const struct rcar_du_device_info
> rzg1_du_r8a7743_info = { .gen = 2,
>  	.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
>  		  | RCAR_DU_FEATURE_EXT_CTRL_REGS,
> -	.num_crtcs = 2,
> +	.channel_mask = BIT(0) | BIT(1),

I'd write it BIT(1) | BIT(0) to match the usual little-endian order. Same 
comment for the other info structure instances.

>  	.routes = {
>  		/*
>  		 * R8A7743 has one RGB output and one LVDS output
> @@ -61,7 +61,7 @@ static const struct rcar_du_device_info
> rzg1_du_r8a7745_info = { .gen = 2,
>  	.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
>  		  | RCAR_DU_FEATURE_EXT_CTRL_REGS,
> -	.num_crtcs = 2,
> +	.channel_mask = BIT(0) | BIT(1),
>  	.routes = {
>  		/*
>  		 * R8A7745 has two RGB outputs
> @@ -80,7 +80,7 @@ static const struct rcar_du_device_info
> rzg1_du_r8a7745_info = { static const struct rcar_du_device_info
> rcar_du_r8a7779_info = {
>  	.gen = 2,
>  	.features = 0,
> -	.num_crtcs = 2,
> +	.channel_mask = BIT(0) | BIT(1),
>  	.routes = {
>  		/*
>  		 * R8A7779 has two RGB outputs and one (currently unsupported)
> @@ -102,7 +102,7 @@ static const struct rcar_du_device_info
> rcar_du_r8a7790_info = { .features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
>  		  | RCAR_DU_FEATURE_EXT_CTRL_REGS,
>  	.quirks = RCAR_DU_QUIRK_ALIGN_128B,
> -	.num_crtcs = 3,
> +	.channel_mask = BIT(0) | BIT(1) | BIT(2),
>  	.routes = {
>  		/*
>  		 * R8A7790 has one RGB output, two LVDS outputs and one
> @@ -129,7 +129,7 @@ static const struct rcar_du_device_info
> rcar_du_r8a7791_info = { .gen = 2,
>  	.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
>  		  | RCAR_DU_FEATURE_EXT_CTRL_REGS,
> -	.num_crtcs = 2,
> +	.channel_mask = BIT(0) | BIT(1),
>  	.routes = {
>  		/*
>  		 * R8A779[13] has one RGB output, one LVDS output and one
> @@ -151,7 +151,7 @@ static const struct rcar_du_device_info
> rcar_du_r8a7792_info = { .gen = 2,
>  	.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
>  		  | RCAR_DU_FEATURE_EXT_CTRL_REGS,
> -	.num_crtcs = 2,
> +	.channel_mask = BIT(0) | BIT(1),
>  	.routes = {
>  		/* R8A7792 has two RGB outputs. */
>  		[RCAR_DU_OUTPUT_DPAD0] = {
> @@ -169,7 +169,7 @@ static const struct rcar_du_device_info
> rcar_du_r8a7794_info = { .gen = 2,
>  	.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
>  		  | RCAR_DU_FEATURE_EXT_CTRL_REGS,
> -	.num_crtcs = 2,
> +	.channel_mask = BIT(0) | BIT(1),
>  	.routes = {
>  		/*
>  		 * R8A7794 has two RGB outputs and one (currently unsupported)
> @@ -191,7 +191,7 @@ static const struct rcar_du_device_info
> rcar_du_r8a7795_info = { .features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
>  		  | RCAR_DU_FEATURE_EXT_CTRL_REGS
>  		  | RCAR_DU_FEATURE_VSP1_SOURCE,
> -	.num_crtcs = 4,
> +	.channel_mask = BIT(0) | BIT(1) | BIT(2) | BIT(3),
>  	.routes = {
>  		/*
>  		 * R8A7795 has one RGB output, two HDMI outputs and one
> @@ -223,7 +223,7 @@ static const struct rcar_du_device_info
> rcar_du_r8a7796_info = { .features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
>  		  | RCAR_DU_FEATURE_EXT_CTRL_REGS
>  		  | RCAR_DU_FEATURE_VSP1_SOURCE,
> -	.num_crtcs = 3,
> +	.channel_mask = BIT(0) | BIT(1) | BIT(2),
>  	.routes = {
>  		/*
>  		 * R8A7796 has one RGB output, one LVDS output and one HDMI
> @@ -251,7 +251,7 @@ static const struct rcar_du_device_info
> rcar_du_r8a77970_info = { .features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
>  		  | RCAR_DU_FEATURE_EXT_CTRL_REGS
>  		  | RCAR_DU_FEATURE_VSP1_SOURCE,
> -	.num_crtcs = 1,
> +	.channel_mask = BIT(0),
>  	.routes = {
>  		/* R8A77970 has one RGB output and one LVDS output. */
>  		[RCAR_DU_OUTPUT_DPAD0] = {
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> b/drivers/gpu/drm/rcar-du/rcar_du_drv.h index 5c7ec15818c7..7a5de66deec2
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> @@ -52,7 +52,7 @@ struct rcar_du_output_routing {
>   * @gen: device generation (2 or 3)
>   * @features: device features (RCAR_DU_FEATURE_*)
>   * @quirks: device quirks (RCAR_DU_QUIRK_*)
> - * @num_crtcs: total number of CRTCs
> + * @channel_mask: bit mask of supported DU channels

Nitpicking, how about channels_mask ?

>   * @routes: array of CRTC to output routes, indexed by output
> (RCAR_DU_OUTPUT_*) * @num_lvds: number of internal LVDS encoders
>   */
> @@ -60,7 +60,7 @@ struct rcar_du_device_info {
>  	unsigned int gen;
>  	unsigned int features;
>  	unsigned int quirks;
> -	unsigned int num_crtcs;
> +	unsigned int channel_mask;
>  	struct rcar_du_output_routing routes[RCAR_DU_OUTPUT_MAX];
>  	unsigned int num_lvds;
>  	unsigned int dpll_ch;
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index cf5b422fc753..19a445fbc879
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -559,6 +559,7 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
>  	struct drm_fbdev_cma *fbdev;
>  	unsigned int num_encoders;
>  	unsigned int num_groups;
> +	unsigned int swi, hwi;

One variable per line please. I would also call them swindex and hwindex, that 
would be clearer in my opinion.

>  	unsigned int i;
>  	int ret;
> 
> @@ -571,7 +572,7 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
>  	dev->mode_config.funcs = &rcar_du_mode_config_funcs;
>  	dev->mode_config.helper_private = &rcar_du_mode_config_helper;
> 
> -	rcdu->num_crtcs = rcdu->info->num_crtcs;
> +	rcdu->num_crtcs = hweight8(rcdu->info->channel_mask);
> 
>  	ret = rcar_du_properties_init(rcdu);
>  	if (ret < 0)
> @@ -581,7 +582,7 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
>  	 * Initialize vertical blanking interrupts handling. Start with vblank
>  	 * disabled for all CRTCs.
>  	 */
> -	ret = drm_vblank_init(dev, (1 << rcdu->info->num_crtcs) - 1);
> +	ret = drm_vblank_init(dev, (1 << rcdu->num_crtcs) - 1);
>  	if (ret < 0)
>  		return ret;
> 
> @@ -623,10 +624,16 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
>  	}
> 
>  	/* Create the CRTCs. */
> -	for (i = 0; i < rcdu->num_crtcs; ++i) {
> -		struct rcar_du_group *rgrp = &rcdu->groups[i / 2];
> +	for (swi = 0, hwi = 0; swi < rcdu->num_crtcs; ++hwi) {
> +		struct rcar_du_group *rgrp;
> +
> +		/* Skip unpopulated DU channels */

s/channels/channels./

> +		if (!(rcdu->info->channel_mask & BIT(hwi)))
> +			continue;
> +
> +		rgrp = &rcdu->groups[hwi / 2];
> 
> -		ret = rcar_du_crtc_create(rgrp, i);
> +		ret = rcar_du_crtc_create(rgrp, swi++, hwi);
>  		if (ret < 0)
>  			return ret;
>  	}

This is going to turn into an infinite loop if we ever get the num_crtcs 
calculation wrong, but I don't see why that should be the case, so I'm OK with 
the implementation.

With all those small issues fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

-- 
Regards,

Laurent Pinchart



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 05/17] drm: rcar-du: Split CRTC handling to support hardware indexing

[Kieran Bingham]

Hi Laurent,

On 26/04/18 21:30, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Thursday, 26 April 2018 19:53:34 EEST Kieran Bingham wrote:
>> The DU CRTC driver does not support distinguishing between a hardware
>> index, and a software (CRTC) index in the event that a DU channel might
>> not be populated by the hardware.
>>
>> Support this by adapting the rcar_du_device_info structure to store a
>> bitmask of available channels rather than a count of CRTCs. The count
>> can then be obtained by determining the hamming weight of the bitmask.
>>
>> This allows the rcar_du_crtc_create() function to distinguish between
>> both index types, and non-populated DU channels will be skipped without
>> leaving a gap in the software CRTC indexes.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>> ---
>>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 26 ++++++++++++++------------
>>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h |  3 ++-
>>  drivers/gpu/drm/rcar-du/rcar_du_drv.c  | 20 ++++++++++----------
>>  drivers/gpu/drm/rcar-du/rcar_du_drv.h  |  4 ++--
>>  drivers/gpu/drm/rcar-du/rcar_du_kms.c  | 17 ++++++++++++-----
>>  5 files changed, 40 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 5a15dfd66343..36ce194c13b5
>> 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> @@ -902,7 +902,8 @@ static irqreturn_t rcar_du_crtc_irq(int irq, void *arg)
>>   * Initialization
>>   */
>>
>> -int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int index)
>> +int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int swindex,
>> +			unsigned int hwindex)
>>  {
>>  	static const unsigned int mmio_offsets[] = {
>>  		DU0_REG_OFFSET, DU1_REG_OFFSET, DU2_REG_OFFSET, DU3_REG_OFFSET
>> @@ -910,7 +911,7 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp,
>> unsigned int index)
>>
>>  	struct rcar_du_device *rcdu = rgrp->dev;
>>  	struct platform_device *pdev = to_platform_device(rcdu->dev);
>> -	struct rcar_du_crtc *rcrtc = &rcdu->crtcs[index];
>> +	struct rcar_du_crtc *rcrtc = &rcdu->crtcs[swindex];
>>  	struct drm_crtc *crtc = &rcrtc->crtc;
>>  	struct drm_plane *primary;
>>  	unsigned int irqflags;
>> @@ -922,7 +923,7 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp,
>> unsigned int index)
>>
>>  	/* Get the CRTC clock and the optional external clock. */
>>  	if (rcar_du_has(rcdu, RCAR_DU_FEATURE_CRTC_IRQ_CLOCK)) {
>> -		sprintf(clk_name, "du.%u", index);
>> +		sprintf(clk_name, "du.%u", hwindex);
>>  		name = clk_name;
>>  	} else {
>>  		name = NULL;
>> @@ -930,16 +931,16 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp,
>> unsigned int index)
>>
>>  	rcrtc->clock = devm_clk_get(rcdu->dev, name);
>>  	if (IS_ERR(rcrtc->clock)) {
>> -		dev_err(rcdu->dev, "no clock for CRTC %u\n", index);
>> +		dev_err(rcdu->dev, "no clock for CRTC %u\n", swindex);
> 
> How about
> 
> 		dev_err(rcdu->dev, "no clock for DU channel %u\n", hwindex);
> 
> I think that would be clearer, because at this stage we're dealing with 
> hardware resources, so matching the datasheet numbers seems better to me.

Yes, I agree.
Changed.


>>  		return PTR_ERR(rcrtc->clock);
>>  	}
>>
>> -	sprintf(clk_name, "dclkin.%u", index);
>> +	sprintf(clk_name, "dclkin.%u", hwindex);
>>  	clk = devm_clk_get(rcdu->dev, clk_name);
>>  	if (!IS_ERR(clk)) {
>>  		rcrtc->extclock = clk;
>>  	} else if (PTR_ERR(rcrtc->clock) == -EPROBE_DEFER) {
>> -		dev_info(rcdu->dev, "can't get external clock %u\n", index);
>> +		dev_info(rcdu->dev, "can't get external clock %u\n", hwindex);
>>  		return -EPROBE_DEFER;
>>  	}
>>
>> @@ -948,13 +949,13 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp,
>> unsigned int index) spin_lock_init(&rcrtc->vblank_lock);
>>
>>  	rcrtc->group = rgrp;
>> -	rcrtc->mmio_offset = mmio_offsets[index];
>> -	rcrtc->index = index;
>> +	rcrtc->mmio_offset = mmio_offsets[hwindex];
>> +	rcrtc->index = hwindex;
>>
>>  	if (rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE))
>>  		primary = &rcrtc->vsp->planes[rcrtc->vsp_pipe].plane;
>>  	else
>> -		primary = &rgrp->planes[index % 2].plane;
>> +		primary = &rgrp->planes[hwindex % 2].plane;
> 
> This shouldn't make a difference because when RCAR_DU_FEATURE_VSP1_SOURCE 
> isn't set we're running on Gen2, and don't need to deal with indices, but from 
> a conceptual point of view, wouldn't the software index be better here ? 
> Missing hardware channels won't be visible from userspace, so taking the first 
> plane of the group as the primary plane would seem better to me.

That's fine by me - updated.


> 
>>  	ret = drm_crtc_init_with_planes(rcdu->ddev, crtc, primary, NULL,
>>  					rcdu->info->gen <= 2 ?
>> @@ -970,7 +971,8 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp,
>> unsigned int index)
>>
>>  	/* Register the interrupt handler. */
>>  	if (rcar_du_has(rcdu, RCAR_DU_FEATURE_CRTC_IRQ_CLOCK)) {
>> -		irq = platform_get_irq(pdev, index);
>> +		/* The IRQ's are associated with the CRTC (sw)index */
> 
> s/index/index./
> 
>> +		irq = platform_get_irq(pdev, swindex);
>>  		irqflags = 0;
>>  	} else {
>>  		irq = platform_get_irq(pdev, 0);
>> @@ -978,7 +980,7 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp,
>> unsigned int index) }
>>
>>  	if (irq < 0) {
>> -		dev_err(rcdu->dev, "no IRQ for CRTC %u\n", index);
>> +		dev_err(rcdu->dev, "no IRQ for CRTC %u\n", swindex);
>>  		return irq;
>>  	}
>>
>> @@ -986,7 +988,7 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp,
>> unsigned int index) dev_name(rcdu->dev), rcrtc);
>>  	if (ret < 0) {
>>  		dev_err(rcdu->dev,
>> -			"failed to register IRQ for CRTC %u\n", index);
>> +			"failed to register IRQ for CRTC %u\n", swindex);
>>  		return ret;
>>  	}
>>
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h index 518ee2c60eb8..5f003a16abc5
>> 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>> @@ -99,7 +99,8 @@ enum rcar_du_output {
>>  	RCAR_DU_OUTPUT_MAX,
>>  };
>>
>> -int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int index);
>> +int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int swindex,
>> +			unsigned int hwindex);
>>  void rcar_du_crtc_suspend(struct rcar_du_crtc *rcrtc);
>>  void rcar_du_crtc_resume(struct rcar_du_crtc *rcrtc);
>>
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
>> b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 05745e86d73e..d6ebc628fc22
>> 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
>> @@ -40,7 +40,7 @@ static const struct rcar_du_device_info
>> rzg1_du_r8a7743_info = { .gen = 2,
>>  	.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
>>  		  | RCAR_DU_FEATURE_EXT_CTRL_REGS,
>> -	.num_crtcs = 2,
>> +	.channel_mask = BIT(0) | BIT(1),
> 
> I'd write it BIT(1) | BIT(0) to match the usual little-endian order. Same 
> comment for the other info structure instances.
> 

Not a fan - but it's ok with me. :) Changed.

This is different to the usage on the .dpll_ch though ...

>>  	.routes = {
>>  		/*
>>  		 * R8A7743 has one RGB output and one LVDS output
>> @@ -61,7 +61,7 @@ static const struct rcar_du_device_info
>> rzg1_du_r8a7745_info = { .gen = 2,
>>  	.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
>>  		  | RCAR_DU_FEATURE_EXT_CTRL_REGS,
>> -	.num_crtcs = 2,
>> +	.channel_mask = BIT(0) | BIT(1),
>>  	.routes = {
>>  		/*
>>  		 * R8A7745 has two RGB outputs
>> @@ -80,7 +80,7 @@ static const struct rcar_du_device_info
>> rzg1_du_r8a7745_info = { static const struct rcar_du_device_info
>> rcar_du_r8a7779_info = {
>>  	.gen = 2,
>>  	.features = 0,
>> -	.num_crtcs = 2,
>> +	.channel_mask = BIT(0) | BIT(1),
>>  	.routes = {
>>  		/*
>>  		 * R8A7779 has two RGB outputs and one (currently unsupported)
>> @@ -102,7 +102,7 @@ static const struct rcar_du_device_info
>> rcar_du_r8a7790_info = { .features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
>>  		  | RCAR_DU_FEATURE_EXT_CTRL_REGS,
>>  	.quirks = RCAR_DU_QUIRK_ALIGN_128B,
>> -	.num_crtcs = 3,
>> +	.channel_mask = BIT(0) | BIT(1) | BIT(2),
>>  	.routes = {
>>  		/*
>>  		 * R8A7790 has one RGB output, two LVDS outputs and one
>> @@ -129,7 +129,7 @@ static const struct rcar_du_device_info
>> rcar_du_r8a7791_info = { .gen = 2,
>>  	.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
>>  		  | RCAR_DU_FEATURE_EXT_CTRL_REGS,
>> -	.num_crtcs = 2,
>> +	.channel_mask = BIT(0) | BIT(1),
>>  	.routes = {
>>  		/*
>>  		 * R8A779[13] has one RGB output, one LVDS output and one
>> @@ -151,7 +151,7 @@ static const struct rcar_du_device_info
>> rcar_du_r8a7792_info = { .gen = 2,
>>  	.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
>>  		  | RCAR_DU_FEATURE_EXT_CTRL_REGS,
>> -	.num_crtcs = 2,
>> +	.channel_mask = BIT(0) | BIT(1),
>>  	.routes = {
>>  		/* R8A7792 has two RGB outputs. */
>>  		[RCAR_DU_OUTPUT_DPAD0] = {
>> @@ -169,7 +169,7 @@ static const struct rcar_du_device_info
>> rcar_du_r8a7794_info = { .gen = 2,
>>  	.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
>>  		  | RCAR_DU_FEATURE_EXT_CTRL_REGS,
>> -	.num_crtcs = 2,
>> +	.channel_mask = BIT(0) | BIT(1),
>>  	.routes = {
>>  		/*
>>  		 * R8A7794 has two RGB outputs and one (currently unsupported)
>> @@ -191,7 +191,7 @@ static const struct rcar_du_device_info
>> rcar_du_r8a7795_info = { .features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
>>  		  | RCAR_DU_FEATURE_EXT_CTRL_REGS
>>  		  | RCAR_DU_FEATURE_VSP1_SOURCE,
>> -	.num_crtcs = 4,
>> +	.channel_mask = BIT(0) | BIT(1) | BIT(2) | BIT(3),
>>  	.routes = {
>>  		/*
>>  		 * R8A7795 has one RGB output, two HDMI outputs and one
>> @@ -223,7 +223,7 @@ static const struct rcar_du_device_info
>> rcar_du_r8a7796_info = { .features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
>>  		  | RCAR_DU_FEATURE_EXT_CTRL_REGS
>>  		  | RCAR_DU_FEATURE_VSP1_SOURCE,
>> -	.num_crtcs = 3,
>> +	.channel_mask = BIT(0) | BIT(1) | BIT(2),
>>  	.routes = {
>>  		/*
>>  		 * R8A7796 has one RGB output, one LVDS output and one HDMI
>> @@ -251,7 +251,7 @@ static const struct rcar_du_device_info
>> rcar_du_r8a77970_info = { .features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
>>  		  | RCAR_DU_FEATURE_EXT_CTRL_REGS
>>  		  | RCAR_DU_FEATURE_VSP1_SOURCE,
>> -	.num_crtcs = 1,
>> +	.channel_mask = BIT(0),
>>  	.routes = {
>>  		/* R8A77970 has one RGB output and one LVDS output. */
>>  		[RCAR_DU_OUTPUT_DPAD0] = {
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
>> b/drivers/gpu/drm/rcar-du/rcar_du_drv.h index 5c7ec15818c7..7a5de66deec2
>> 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
>> @@ -52,7 +52,7 @@ struct rcar_du_output_routing {
>>   * @gen: device generation (2 or 3)
>>   * @features: device features (RCAR_DU_FEATURE_*)
>>   * @quirks: device quirks (RCAR_DU_QUIRK_*)
>> - * @num_crtcs: total number of CRTCs
>> + * @channel_mask: bit mask of supported DU channels
> 
> Nitpicking, how about channels_mask ?
> 
>>   * @routes: array of CRTC to output routes, indexed by output
>> (RCAR_DU_OUTPUT_*) * @num_lvds: number of internal LVDS encoders
>>   */
>> @@ -60,7 +60,7 @@ struct rcar_du_device_info {
>>  	unsigned int gen;
>>  	unsigned int features;
>>  	unsigned int quirks;
>> -	unsigned int num_crtcs;
>> +	unsigned int channel_mask;
>>  	struct rcar_du_output_routing routes[RCAR_DU_OUTPUT_MAX];
>>  	unsigned int num_lvds;
>>  	unsigned int dpll_ch;
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
>> b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index cf5b422fc753..19a445fbc879
>> 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
>> @@ -559,6 +559,7 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
>>  	struct drm_fbdev_cma *fbdev;
>>  	unsigned int num_encoders;
>>  	unsigned int num_groups;
>> +	unsigned int swi, hwi;
> 
> One variable per line please. I would also call them swindex and hwindex, that 
> would be clearer in my opinion.
> 

Fixed (on both accounts)


>>  	unsigned int i;
>>  	int ret;
>>
>> @@ -571,7 +572,7 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
>>  	dev->mode_config.funcs = &rcar_du_mode_config_funcs;
>>  	dev->mode_config.helper_private = &rcar_du_mode_config_helper;
>>
>> -	rcdu->num_crtcs = rcdu->info->num_crtcs;
>> +	rcdu->num_crtcs = hweight8(rcdu->info->channel_mask);
>>
>>  	ret = rcar_du_properties_init(rcdu);
>>  	if (ret < 0)
>> @@ -581,7 +582,7 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
>>  	 * Initialize vertical blanking interrupts handling. Start with vblank
>>  	 * disabled for all CRTCs.
>>  	 */
>> -	ret = drm_vblank_init(dev, (1 << rcdu->info->num_crtcs) - 1);
>> +	ret = drm_vblank_init(dev, (1 << rcdu->num_crtcs) - 1);
>>  	if (ret < 0)
>>  		return ret;
>>
>> @@ -623,10 +624,16 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
>>  	}
>>
>>  	/* Create the CRTCs. */
>> -	for (i = 0; i < rcdu->num_crtcs; ++i) {
>> -		struct rcar_du_group *rgrp = &rcdu->groups[i / 2];
>> +	for (swi = 0, hwi = 0; swi < rcdu->num_crtcs; ++hwi) {
>> +		struct rcar_du_group *rgrp;
>> +
>> +		/* Skip unpopulated DU channels */
> 
> s/channels/channels./

Done.


> 
>> +		if (!(rcdu->info->channel_mask & BIT(hwi)))
>> +			continue;
>> +
>> +		rgrp = &rcdu->groups[hwi / 2];
>>
>> -		ret = rcar_du_crtc_create(rgrp, i);
>> +		ret = rcar_du_crtc_create(rgrp, swi++, hwi);
>>  		if (ret < 0)
>>  			return ret;
>>  	}
> 
> This is going to turn into an infinite loop if we ever get the num_crtcs 
> calculation wrong, but I don't see why that should be the case, so I'm OK with 
> the implementation.

Yes, I have considered this. I actually started out by making the loop condition
based on "swi < hweight8(rcdu->info->channel_mask)" because of this. Thus that
would ensure that if the channel_mask was ever unset or 0 then the loop would exit.

However then I figured there's no point duplicating the hweight8 call and that
the num_crtc's should represent the same value.

As long as no one tries to increment the num_crtc's arbitrarily - this should be
safe even if the rcdu->info->channel_mask is blank.


> With all those small issues fixed,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks, Tag collected.

--
Kieran

[PATCH 06/17] drm: rcar-du: Allow DU groups to work with hardware indexing

[Kieran Bingham]

The group objects assume linear indexing, and more so always assume that
channel 0 of any active group is used.

Now that the CRTC objects support non-linear indexing, adapt the groups
to remove assumptions that channel 0 is utilised in each group by using
the channel mask provided in the device structures.

Finally ensure that the RGB routing is determined from the index of the
CRTC object (which represents the hardware DU channel index).

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_group.c | 14 +++++++++-----
 drivers/gpu/drm/rcar-du/rcar_du_group.h |  2 ++
 drivers/gpu/drm/rcar-du/rcar_du_kms.c   |  5 ++++-
 3 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c b/drivers/gpu/drm/rcar-du/rcar_du_group.c
index eead202c95c7..c52091fe02ba 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
@@ -46,9 +46,12 @@ void rcar_du_group_write(struct rcar_du_group *rgrp, u32 reg, u32 data)
 
 static void rcar_du_group_setup_pins(struct rcar_du_group *rgrp)
 {
-	u32 defr6 = DEFR6_CODE | DEFR6_ODPM02_DISP;
+	u32 defr6 = DEFR6_CODE;
 
-	if (rgrp->num_crtcs > 1)
+	if (rgrp->channel_mask & BIT(0))
+		defr6 |= DEFR6_ODPM02_DISP;
+
+	if (rgrp->channel_mask & BIT(1))
 		defr6 |= DEFR6_ODPM12_DISP;
 
 	rcar_du_group_write(rgrp, DEFR6, defr6);
@@ -80,10 +83,11 @@ static void rcar_du_group_setup_defr8(struct rcar_du_group *rgrp)
 		 * On Gen3 VSPD routing can't be configured, but DPAD routing
 		 * needs to be set despite having a single option available.
 		 */
-		u32 crtc = ffs(possible_crtcs) - 1;
+		unsigned int rgb_crtc = ffs(possible_crtcs) - 1;
+		struct rcar_du_crtc *crtc = &rcdu->crtcs[rgb_crtc];
 
-		if (crtc / 2 == rgrp->index)
-			defr8 |= DEFR8_DRGBS_DU(crtc);
+		if (crtc->index / 2 == rgrp->index)
+			defr8 |= DEFR8_DRGBS_DU(crtc->index);
 	}
 
 	rcar_du_group_write(rgrp, DEFR8, defr8);
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.h b/drivers/gpu/drm/rcar-du/rcar_du_group.h
index 5e3adc6b31b5..d29a68e006a7 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_group.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_group.h
@@ -25,6 +25,7 @@ struct rcar_du_device;
  * @dev: the DU device
  * @mmio_offset: registers offset in the device memory map
  * @index: group index
+ * @channel_mask: bitmask of populated DU channels in this group
  * @num_crtcs: number of CRTCs in this group (1 or 2)
  * @use_count: number of users of the group (rcar_du_group_(get|put))
  * @used_crtcs: number of CRTCs currently in use
@@ -39,6 +40,7 @@ struct rcar_du_group {
 	unsigned int mmio_offset;
 	unsigned int index;
 
+	unsigned int channel_mask;
 	unsigned int num_crtcs;
 	unsigned int use_count;
 	unsigned int used_crtcs;
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
index 19a445fbc879..45fb554fd3c7 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -597,7 +597,10 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
 		rgrp->dev = rcdu;
 		rgrp->mmio_offset = mmio_offsets[i];
 		rgrp->index = i;
-		rgrp->num_crtcs = min(rcdu->num_crtcs - 2 * i, 2U);
+		/* Extract the channel mask for this group only */
+		rgrp->channel_mask = (rcdu->info->channel_mask >> (2 * i))
+				   & GENMASK(1, 0);
+		rgrp->num_crtcs = hweight8(rgrp->channel_mask);
 
 		/*
 		 * If we have more than one CRTCs in this group pre-associate
-- 
2.17.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 06/17] drm: rcar-du: Allow DU groups to work with hardware indexing

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Thursday, 26 April 2018 19:53:35 EEST Kieran Bingham wrote:
> The group objects assume linear indexing, and more so always assume that
> channel 0 of any active group is used.
> 
> Now that the CRTC objects support non-linear indexing, adapt the groups
> to remove assumptions that channel 0 is utilised in each group by using
> the channel mask provided in the device structures.
> 
> Finally ensure that the RGB routing is determined from the index of the
> CRTC object (which represents the hardware DU channel index).
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_group.c | 14 +++++++++-----
>  drivers/gpu/drm/rcar-du/rcar_du_group.h |  2 ++
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c   |  5 ++++-
>  3 files changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c
> b/drivers/gpu/drm/rcar-du/rcar_du_group.c index eead202c95c7..c52091fe02ba
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> @@ -46,9 +46,12 @@ void rcar_du_group_write(struct rcar_du_group *rgrp, u32
> reg, u32 data)
> 
>  static void rcar_du_group_setup_pins(struct rcar_du_group *rgrp)
>  {
> -	u32 defr6 = DEFR6_CODE | DEFR6_ODPM02_DISP;
> +	u32 defr6 = DEFR6_CODE;
> 
> -	if (rgrp->num_crtcs > 1)
> +	if (rgrp->channel_mask & BIT(0))
> +		defr6 |= DEFR6_ODPM02_DISP;
> +
> +	if (rgrp->channel_mask & BIT(1))
>  		defr6 |= DEFR6_ODPM12_DISP;

So much cleaner with the channels mask, I like this.

>  	rcar_du_group_write(rgrp, DEFR6, defr6);
> @@ -80,10 +83,11 @@ static void rcar_du_group_setup_defr8(struct
> rcar_du_group *rgrp) * On Gen3 VSPD routing can't be configured, but DPAD
> routing
>  		 * needs to be set despite having a single option available.
>  		 */
> -		u32 crtc = ffs(possible_crtcs) - 1;
> +		unsigned int rgb_crtc = ffs(possible_crtcs) - 1;
> +		struct rcar_du_crtc *crtc = &rcdu->crtcs[rgb_crtc];
> 
> -		if (crtc / 2 == rgrp->index)
> -			defr8 |= DEFR8_DRGBS_DU(crtc);
> +		if (crtc->index / 2 == rgrp->index)
> +			defr8 |= DEFR8_DRGBS_DU(crtc->index);
>  	}
> 
>  	rcar_du_group_write(rgrp, DEFR8, defr8);
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.h
> b/drivers/gpu/drm/rcar-du/rcar_du_group.h index 5e3adc6b31b5..d29a68e006a7
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_group.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.h
> @@ -25,6 +25,7 @@ struct rcar_du_device;
>   * @dev: the DU device
>   * @mmio_offset: registers offset in the device memory map
>   * @index: group index
> + * @channel_mask: bitmask of populated DU channels in this group
>   * @num_crtcs: number of CRTCs in this group (1 or 2)
>   * @use_count: number of users of the group (rcar_du_group_(get|put))
>   * @used_crtcs: number of CRTCs currently in use
> @@ -39,6 +40,7 @@ struct rcar_du_group {
>  	unsigned int mmio_offset;
>  	unsigned int index;
> 
> +	unsigned int channel_mask;

Depending on how you like my suggestion in patch 05/17, this might be better 
called channels_mask.

>  	unsigned int num_crtcs;
>  	unsigned int use_count;
>  	unsigned int used_crtcs;
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index 19a445fbc879..45fb554fd3c7
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -597,7 +597,10 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
>  		rgrp->dev = rcdu;
>  		rgrp->mmio_offset = mmio_offsets[i];
>  		rgrp->index = i;
> -		rgrp->num_crtcs = min(rcdu->num_crtcs - 2 * i, 2U);
> +		/* Extract the channel mask for this group only */

s/only/only./

> +		rgrp->channel_mask = (rcdu->info->channel_mask >> (2 * i))
> +				   & GENMASK(1, 0);
> +		rgrp->num_crtcs = hweight8(rgrp->channel_mask);

You could optimize this by computing it as

	rgrp->num_crtcs = (rgrp->channel_mask >> 1)
			| (rgrp->channel_mask & 1);

as you know that only two bits at most can be set. Up to you.

>  		/*
>  		 * If we have more than one CRTCs in this group pre-associate

With those small issues fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

-- 
Regards,

Laurent Pinchart



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 06/17] drm: rcar-du: Allow DU groups to work with hardware indexing

[Kieran Bingham]

Hi Laurent,

On 26/04/18 21:36, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Thursday, 26 April 2018 19:53:35 EEST Kieran Bingham wrote:
>> The group objects assume linear indexing, and more so always assume that
>> channel 0 of any active group is used.
>>
>> Now that the CRTC objects support non-linear indexing, adapt the groups
>> to remove assumptions that channel 0 is utilised in each group by using
>> the channel mask provided in the device structures.
>>
>> Finally ensure that the RGB routing is determined from the index of the
>> CRTC object (which represents the hardware DU channel index).
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>> ---
>>  drivers/gpu/drm/rcar-du/rcar_du_group.c | 14 +++++++++-----
>>  drivers/gpu/drm/rcar-du/rcar_du_group.h |  2 ++
>>  drivers/gpu/drm/rcar-du/rcar_du_kms.c   |  5 ++++-
>>  3 files changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c
>> b/drivers/gpu/drm/rcar-du/rcar_du_group.c index eead202c95c7..c52091fe02ba
>> 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
>> @@ -46,9 +46,12 @@ void rcar_du_group_write(struct rcar_du_group *rgrp, u32
>> reg, u32 data)
>>
>>  static void rcar_du_group_setup_pins(struct rcar_du_group *rgrp)
>>  {
>> -	u32 defr6 = DEFR6_CODE | DEFR6_ODPM02_DISP;
>> +	u32 defr6 = DEFR6_CODE;
>>
>> -	if (rgrp->num_crtcs > 1)
>> +	if (rgrp->channel_mask & BIT(0))
>> +		defr6 |= DEFR6_ODPM02_DISP;
>> +
>> +	if (rgrp->channel_mask & BIT(1))
>>  		defr6 |= DEFR6_ODPM12_DISP;
> 
> So much cleaner with the channels mask, I like this.

:-D

> 
>>  	rcar_du_group_write(rgrp, DEFR6, defr6);
>> @@ -80,10 +83,11 @@ static void rcar_du_group_setup_defr8(struct
>> rcar_du_group *rgrp) * On Gen3 VSPD routing can't be configured, but DPAD
>> routing
>>  		 * needs to be set despite having a single option available.
>>  		 */
>> -		u32 crtc = ffs(possible_crtcs) - 1;
>> +		unsigned int rgb_crtc = ffs(possible_crtcs) - 1;
>> +		struct rcar_du_crtc *crtc = &rcdu->crtcs[rgb_crtc];
>>
>> -		if (crtc / 2 == rgrp->index)
>> -			defr8 |= DEFR8_DRGBS_DU(crtc);
>> +		if (crtc->index / 2 == rgrp->index)
>> +			defr8 |= DEFR8_DRGBS_DU(crtc->index);
>>  	}
>>
>>  	rcar_du_group_write(rgrp, DEFR8, defr8);
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.h
>> b/drivers/gpu/drm/rcar-du/rcar_du_group.h index 5e3adc6b31b5..d29a68e006a7
>> 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_group.h
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.h
>> @@ -25,6 +25,7 @@ struct rcar_du_device;
>>   * @dev: the DU device
>>   * @mmio_offset: registers offset in the device memory map
>>   * @index: group index
>> + * @channel_mask: bitmask of populated DU channels in this group
>>   * @num_crtcs: number of CRTCs in this group (1 or 2)
>>   * @use_count: number of users of the group (rcar_du_group_(get|put))
>>   * @used_crtcs: number of CRTCs currently in use
>> @@ -39,6 +40,7 @@ struct rcar_du_group {
>>  	unsigned int mmio_offset;
>>  	unsigned int index;
>>
>> +	unsigned int channel_mask;
> 
> Depending on how you like my suggestion in patch 05/17, this might be better 
> called channels_mask.

Done.

> 
>>  	unsigned int num_crtcs;
>>  	unsigned int use_count;
>>  	unsigned int used_crtcs;
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
>> b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index 19a445fbc879..45fb554fd3c7
>> 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
>> @@ -597,7 +597,10 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
>>  		rgrp->dev = rcdu;
>>  		rgrp->mmio_offset = mmio_offsets[i];
>>  		rgrp->index = i;
>> -		rgrp->num_crtcs = min(rcdu->num_crtcs - 2 * i, 2U);
>> +		/* Extract the channel mask for this group only */
> 
> s/only/only./
> 
>> +		rgrp->channel_mask = (rcdu->info->channel_mask >> (2 * i))
>> +				   & GENMASK(1, 0);
>> +		rgrp->num_crtcs = hweight8(rgrp->channel_mask);
> 
> You could optimize this by computing it as
> 
> 	rgrp->num_crtcs = (rgrp->channel_mask >> 1)
> 			| (rgrp->channel_mask & 1);
> 
> as you know that only two bits at most can be set. Up to you.

Hrm... that looks like a neat trick - but I might leave this as hweight if you
don't object.
We're not on a hot-path here, and hweight is purposefully designed to count
bits, and thus self documenting ... whereas bit-magic is ... magic :D

 (Don't get me wrong though I am a fan of magic :D, and I love good bit-tricks)

> 
>>  		/*
>>  		 * If we have more than one CRTCs in this group pre-associate
> 
> With those small issues fixed,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>


Thanks, collected.

Kieran

[PATCH 07/17] drm: rcar-du: Add R8A77965 support

[Kieran Bingham]

The R8A77965 (M3-N) SoC provides VGA, HDMI and LVDS output.

This platform is unusual in that the VGA is connected to DU3 leaving DU2
unpopulated. This is reflected by the channel_mask accordingly.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_drv.c | 29 +++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
index d6ebc628fc22..4d195ff8c569 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
@@ -246,6 +246,34 @@ static const struct rcar_du_device_info rcar_du_r8a7796_info = {
 	.dpll_ch =  BIT(1),
 };
 
+static const struct rcar_du_device_info rcar_du_r8a77965_info = {
+	.gen = 3,
+	.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
+		  | RCAR_DU_FEATURE_EXT_CTRL_REGS
+		  | RCAR_DU_FEATURE_VSP1_SOURCE,
+	.channel_mask = BIT(0) | BIT(1) | BIT(3),
+	.routes = {
+		/*
+		 * R8A77965 has one RGB output, one LVDS output and one HDMI
+		 * output.
+		 */
+		[RCAR_DU_OUTPUT_DPAD0] = {
+			.possible_crtcs = BIT(2),
+			.port = 0,
+		},
+		[RCAR_DU_OUTPUT_HDMI0] = {
+			.possible_crtcs = BIT(1),
+			.port = 1,
+		},
+		[RCAR_DU_OUTPUT_LVDS0] = {
+			.possible_crtcs = BIT(0),
+			.port = 2,
+		},
+	},
+	.num_lvds = 1,
+	.dpll_ch =  BIT(1),
+};
+
 static const struct rcar_du_device_info rcar_du_r8a77970_info = {
 	.gen = 3,
 	.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
@@ -277,6 +305,7 @@ static const struct of_device_id rcar_du_of_table[] = {
 	{ .compatible = "renesas,du-r8a7794", .data = &rcar_du_r8a7794_info },
 	{ .compatible = "renesas,du-r8a7795", .data = &rcar_du_r8a7795_info },
 	{ .compatible = "renesas,du-r8a7796", .data = &rcar_du_r8a7796_info },
+	{ .compatible = "renesas,du-r8a77965", .data = &rcar_du_r8a77965_info },
 	{ .compatible = "renesas,du-r8a77970", .data = &rcar_du_r8a77970_info },
 	{ }
 };
-- 
2.17.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 07/17] drm: rcar-du: Add R8A77965 support

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Thursday, 26 April 2018 19:53:36 EEST Kieran Bingham wrote:
> The R8A77965 (M3-N) SoC provides VGA, HDMI and LVDS output.
> 
> This platform is unusual in that the VGA is connected to DU3 leaving DU2
> unpopulated. This is reflected by the channel_mask accordingly.

I'd write s/VGA/DPAD/g (or s/VGA/RGB/g) as the DPAD output can be used for 
other purposes than VGA.

> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_drv.c | 29 +++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index d6ebc628fc22..4d195ff8c569
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> @@ -246,6 +246,34 @@ static const struct rcar_du_device_info
> rcar_du_r8a7796_info = { .dpll_ch =  BIT(1),
>  };
> 
> +static const struct rcar_du_device_info rcar_du_r8a77965_info = {
> +	.gen = 3,
> +	.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
> +		  | RCAR_DU_FEATURE_EXT_CTRL_REGS
> +		  | RCAR_DU_FEATURE_VSP1_SOURCE,
> +	.channel_mask = BIT(0) | BIT(1) | BIT(3),

Depending on what you think of my suggestions for patch 05/17, you might want 
to reverse the bit order here.

> +	.routes = {
> +		/*
> +		 * R8A77965 has one RGB output, one LVDS output and one HDMI
> +		 * output.
> +		 */
> +		[RCAR_DU_OUTPUT_DPAD0] = {
> +			.possible_crtcs = BIT(2),
> +			.port = 0,
> +		},
> +		[RCAR_DU_OUTPUT_HDMI0] = {
> +			.possible_crtcs = BIT(1),
> +			.port = 1,
> +		},
> +		[RCAR_DU_OUTPUT_LVDS0] = {
> +			.possible_crtcs = BIT(0),

I wonder whether it wouldn't be easier to read if we replaced possible_crtcs 
with possible_channels, as this structure describes the hardware and had its 
num_crtcs field replaced with a channel_mask. This would require converting 
the possible_channels field to a possible_crtcs field in 
rcar_du_modeset_init(), and I think that no change would be needed in 
rcar_du_group_setup_defr8() (but please double check). On the other hand, no 
code would be simplified, and rcar_du_modeset_init() would gain some 
additional complexity, so it might not be worth it.

Either way this patch looks good to me.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +			.port = 2,
> +		},
> +	},
> +	.num_lvds = 1,
> +	.dpll_ch =  BIT(1),
> +};
> +
>  static const struct rcar_du_device_info rcar_du_r8a77970_info = {
>  	.gen = 3,
>  	.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
> @@ -277,6 +305,7 @@ static const struct of_device_id rcar_du_of_table[] = {
>  	{ .compatible = "renesas,du-r8a7794", .data = &rcar_du_r8a7794_info },
>  	{ .compatible = "renesas,du-r8a7795", .data = &rcar_du_r8a7795_info },
>  	{ .compatible = "renesas,du-r8a7796", .data = &rcar_du_r8a7796_info },
> +	{ .compatible = "renesas,du-r8a77965", .data = &rcar_du_r8a77965_info },
>  	{ .compatible = "renesas,du-r8a77970", .data = &rcar_du_r8a77970_info },
>  	{ }
>  };

-- 
Regards,

Laurent Pinchart



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 07/17] drm: rcar-du: Add R8A77965 support

[Kieran Bingham]

On 26/04/18 21:43, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Thursday, 26 April 2018 19:53:36 EEST Kieran Bingham wrote:
>> The R8A77965 (M3-N) SoC provides VGA, HDMI and LVDS output.
>>
>> This platform is unusual in that the VGA is connected to DU3 leaving DU2
>> unpopulated. This is reflected by the channel_mask accordingly.
> 
> I'd write s/VGA/DPAD/g (or s/VGA/RGB/g) as the DPAD output can be used for 
> other purposes than VGA.
> 
>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>> ---
>>  drivers/gpu/drm/rcar-du/rcar_du_drv.c | 29 +++++++++++++++++++++++++++
>>  1 file changed, 29 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
>> b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index d6ebc628fc22..4d195ff8c569
>> 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
>> @@ -246,6 +246,34 @@ static const struct rcar_du_device_info
>> rcar_du_r8a7796_info = { .dpll_ch =  BIT(1),
>>  };
>>
>> +static const struct rcar_du_device_info rcar_du_r8a77965_info = {
>> +	.gen = 3,
>> +	.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
>> +		  | RCAR_DU_FEATURE_EXT_CTRL_REGS
>> +		  | RCAR_DU_FEATURE_VSP1_SOURCE,
>> +	.channel_mask = BIT(0) | BIT(1) | BIT(3),
> 
> Depending on what you think of my suggestions for patch 05/17, you might want 
> to reverse the bit order here.

Done.

> 
>> +	.routes = {
>> +		/*
>> +		 * R8A77965 has one RGB output, one LVDS output and one HDMI
>> +		 * output.
>> +		 */
>> +		[RCAR_DU_OUTPUT_DPAD0] = {
>> +			.possible_crtcs = BIT(2),
>> +			.port = 0,
>> +		},
>> +		[RCAR_DU_OUTPUT_HDMI0] = {
>> +			.possible_crtcs = BIT(1),
>> +			.port = 1,
>> +		},
>> +		[RCAR_DU_OUTPUT_LVDS0] = {
>> +			.possible_crtcs = BIT(0),
> 
> I wonder whether it wouldn't be easier to read if we replaced possible_crtcs 
> with possible_channels, as this structure describes the hardware and had its 
> num_crtcs field replaced with a channel_mask. This would require converting 
> the possible_channels field to a possible_crtcs field in 
> rcar_du_modeset_init(), and I think that no change would be needed in 
> rcar_du_group_setup_defr8() (but please double check). On the other hand, no 
> code would be simplified, and rcar_du_modeset_init() would gain some 
> additional complexity, so it might not be worth it.

I think we can leave this for now and consider it later if worth while.

> 
> Either way this patch looks good to me.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks, collected.

--
Kieran


> 
>> +			.port = 2,
>> +		},
>> +	},
>> +	.num_lvds = 1,
>> +	.dpll_ch =  BIT(1),
>> +};
>> +
>>  static const struct rcar_du_device_info rcar_du_r8a77970_info = {
>>  	.gen = 3,
>>  	.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
>> @@ -277,6 +305,7 @@ static const struct of_device_id rcar_du_of_table[] = {
>>  	{ .compatible = "renesas,du-r8a7794", .data = &rcar_du_r8a7794_info },
>>  	{ .compatible = "renesas,du-r8a7795", .data = &rcar_du_r8a7795_info },
>>  	{ .compatible = "renesas,du-r8a7796", .data = &rcar_du_r8a7796_info },
>> +	{ .compatible = "renesas,du-r8a77965", .data = &rcar_du_r8a77965_info },
>>  	{ .compatible = "renesas,du-r8a77970", .data = &rcar_du_r8a77970_info },
>>  	{ }
>>  };
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel