Re: [RFC PATCH v5 4/6] drm/tidss: Add support to configure OLDI mode for am625-dss.

2022-10-24 Thread Tomi Valkeinen 

On 18/10/2022 10:00, Aradhya Bhatia wrote:

Hi Tomi

Thank you for the comprehensive feedback across all the patches. I am
working on them.

I do have some concerns which I have talked about, below.

On 12-Oct-22 17:53, Tomi Valkeinen wrote:

On 28/09/2022 20:52, Aradhya Bhatia wrote:

The newer version of DSS (AM625-DSS) has 2 OLDI TXes at its disposal.
These can be configured to support the following modes:

1. OLDI_SINGLE_LINK_SINGLE_MODE
Single Output over OLDI 0.
+--+    +-+  +---+
|  |    | |  |   |
| CRTC +--->+ ENCODER +->| PANEL |
|  |    | |  |   |
+--+    +-+  +---+


Can you have single link on OLDI 1 (OLDI 0 off)? I don't know if that 
make sense on this platform, but if the pins for OLDI 0 and 1 are 
different, there might be a reason on some cases for that.


HW does not support a case where single link is enabled over OLDI 1 with
OLDI 0 off, even though the pins are different.

One could still put 2 panel nodes in DT to set OLDI in a Clone Mode and
simply not use OLDI 0 pins, but I dont think that is a valid case that
should be supported.




2. OLDI_SINGLE_LINK_CLONE_MODE
Duplicate Output over OLDI 0 and 1.
+--+    +-+  +---+
|  |    | |  |   |
| CRTC +---+--->| ENCODER +->| PANEL |
|  |   |    | |  |   |
+--+   |    +-+  +---+
   |


I think you've got a tab in the line above, but otherwise use spaces.


    |    +-+  +---+
    |    | |  |   |
    +--->| ENCODER +->| PANEL |
 | |  |   |
 +-+  +---+

3. OLDI_DUAL_LINK_MODE
Combined Output over OLDI 0 and 1.
+--+    +-+  +---+
|  |    | +->|   |
| CRTC +--->+ ENCODER |  | PANEL |
|  |    | +->|   |
+--+    +-+  +---+

Following the above pathways for different modes, 2 encoder/panel-bridge
pipes get created for clone mode, and 1 pipe in cases of single link and
dual link mode.

Add support for confgure the OLDI modes using of and lvds DRM helper


"configuring"


functions.

Signed-off-by: Aradhya Bhatia 
---
  drivers/gpu/drm/tidss/tidss_dispc.c |  11 +++
  drivers/gpu/drm/tidss/tidss_dispc.h |   8 ++
  drivers/gpu/drm/tidss/tidss_drv.h   |   3 +
  drivers/gpu/drm/tidss/tidss_kms.c   | 146 +++-
  4 files changed, 145 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c 
b/drivers/gpu/drm/tidss/tidss_dispc.c

index 34f0da4bb3e3..88008ad39b55 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -354,6 +354,8 @@ struct dispc_device {
  bool is_enabled;
+    enum dispc_oldi_modes oldi_mode;
+
  struct dss_vp_data vp_data[TIDSS_MAX_PORTS];
  u32 *fourccs;
@@ -1958,6 +1960,15 @@ const u32 *dispc_plane_formats(struct 
dispc_device *dispc, unsigned int *len)

  return dispc->fourccs;
  }
+int dispc_configure_oldi_mode(struct dispc_device *dispc,
+  enum dispc_oldi_modes oldi_mode)
+{
+    WARN_ON(!dispc);
+
+    dispc->oldi_mode = oldi_mode;
+    return 0;
+}


I think "configure" means more than just storing the value. Maybe 
dispc_set_oldi_mode(). And an empty line above the return.



+
  static s32 pixinc(int pixels, u8 ps)
  {
  if (pixels == 1)
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h 
b/drivers/gpu/drm/tidss/tidss_dispc.h

index b66418e583ee..45cce1054832 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.h
+++ b/drivers/gpu/drm/tidss/tidss_dispc.h
@@ -64,6 +64,13 @@ enum dispc_dss_subrevision {
  DISPC_AM625,
  };
+enum dispc_oldi_modes {
+    OLDI_MODE_OFF,    /* OLDI turned off / tied off in 
IP. */
+    OLDI_SINGLE_LINK_SINGLE_MODE,    /* Single Output over OLDI 
0. */
+    OLDI_SINGLE_LINK_CLONE_MODE,    /* Duplicate Output over 
OLDI 0 and 1. */
+    OLDI_DUAL_LINK_MODE,    /* Combined Output over OLDI 0 
and 1. */

+};
+
  struct dispc_features {
  int min_pclk_khz;
  int max_pclk_khz[DISPC_VP_MAX_BUS_TYPE];
@@ -131,6 +138,7 @@ int dispc_plane_setup(struct dispc_device *dispc, 
u32 hw_plane,

    u32 hw_videoport);
  int dispc_plane_enable(struct dispc_device *dispc, u32 hw_plane, 
bool enable);
  const u32 *dispc_plane_formats(struct dispc_device *dispc, unsigned 
int *len);
+int dispc_configure_oldi_mode(struct dispc_device *dispc, enum 
dispc_oldi_modes oldi_mode);

  int dispc_init(struct tidss_device *tidss);
  void dispc_remove(struct tidss_device *tidss);
diff --git a/drivers/gpu/drm/tidss/tidss_drv.h 
b/drivers/gpu/drm/tidss/tidss_drv.h

index d7f27b0b0315..2252ba0222ca 100644
--- a/drivers/gpu/drm/tidss/tidss_drv.h
+++ b/drivers/gpu/drm/tidss/tidss_drv.h
@@ -12,6 +12,9 @@
  #define TIDSS_MAX_PORTS 4
  #define

Re: [RFC PATCH v5 4/6] drm/tidss: Add support to configure OLDI mode for am625-dss.

2022-10-18 Thread Aradhya Bhatia 

Hi Tomi

Thank you for the comprehensive feedback across all the patches. I am
working on them.

I do have some concerns which I have talked about, below.

On 12-Oct-22 17:53, Tomi Valkeinen wrote:

On 28/09/2022 20:52, Aradhya Bhatia wrote:

The newer version of DSS (AM625-DSS) has 2 OLDI TXes at its disposal.
These can be configured to support the following modes:

1. OLDI_SINGLE_LINK_SINGLE_MODE
Single Output over OLDI 0.
+--+    +-+  +---+
|  |    | |  |   |
| CRTC +--->+ ENCODER +->| PANEL |
|  |    | |  |   |
+--+    +-+  +---+


Can you have single link on OLDI 1 (OLDI 0 off)? I don't know if that 
make sense on this platform, but if the pins for OLDI 0 and 1 are 
different, there might be a reason on some cases for that.


HW does not support a case where single link is enabled over OLDI 1 with
OLDI 0 off, even though the pins are different.

One could still put 2 panel nodes in DT to set OLDI in a Clone Mode and
simply not use OLDI 0 pins, but I dont think that is a valid case that
should be supported.




2. OLDI_SINGLE_LINK_CLONE_MODE
Duplicate Output over OLDI 0 and 1.
+--+    +-+  +---+
|  |    | |  |   |
| CRTC +---+--->| ENCODER +->| PANEL |
|  |   |    | |  |   |
+--+   |    +-+  +---+
   |


I think you've got a tab in the line above, but otherwise use spaces.


    |    +-+  +---+
    |    | |  |   |
    +--->| ENCODER +->| PANEL |
 | |  |   |
 +-+  +---+

3. OLDI_DUAL_LINK_MODE
Combined Output over OLDI 0 and 1.
+--+    +-+  +---+
|  |    | +->|   |
| CRTC +--->+ ENCODER |  | PANEL |
|  |    | +->|   |
+--+    +-+  +---+

Following the above pathways for different modes, 2 encoder/panel-bridge
pipes get created for clone mode, and 1 pipe in cases of single link and
dual link mode.

Add support for confgure the OLDI modes using of and lvds DRM helper


"configuring"


functions.

Signed-off-by: Aradhya Bhatia 
---
  drivers/gpu/drm/tidss/tidss_dispc.c |  11 +++
  drivers/gpu/drm/tidss/tidss_dispc.h |   8 ++
  drivers/gpu/drm/tidss/tidss_drv.h   |   3 +
  drivers/gpu/drm/tidss/tidss_kms.c   | 146 +++-
  4 files changed, 145 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c 
b/drivers/gpu/drm/tidss/tidss_dispc.c

index 34f0da4bb3e3..88008ad39b55 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -354,6 +354,8 @@ struct dispc_device {
  bool is_enabled;
+    enum dispc_oldi_modes oldi_mode;
+
  struct dss_vp_data vp_data[TIDSS_MAX_PORTS];
  u32 *fourccs;
@@ -1958,6 +1960,15 @@ const u32 *dispc_plane_formats(struct 
dispc_device *dispc, unsigned int *len)

  return dispc->fourccs;
  }
+int dispc_configure_oldi_mode(struct dispc_device *dispc,
+  enum dispc_oldi_modes oldi_mode)
+{
+    WARN_ON(!dispc);
+
+    dispc->oldi_mode = oldi_mode;
+    return 0;
+}


I think "configure" means more than just storing the value. Maybe 
dispc_set_oldi_mode(). And an empty line above the return.



+
  static s32 pixinc(int pixels, u8 ps)
  {
  if (pixels == 1)
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h 
b/drivers/gpu/drm/tidss/tidss_dispc.h

index b66418e583ee..45cce1054832 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.h
+++ b/drivers/gpu/drm/tidss/tidss_dispc.h
@@ -64,6 +64,13 @@ enum dispc_dss_subrevision {
  DISPC_AM625,
  };
+enum dispc_oldi_modes {
+    OLDI_MODE_OFF,    /* OLDI turned off / tied off in 
IP. */
+    OLDI_SINGLE_LINK_SINGLE_MODE,    /* Single Output over OLDI 
0. */
+    OLDI_SINGLE_LINK_CLONE_MODE,    /* Duplicate Output over OLDI 
0 and 1. */
+    OLDI_DUAL_LINK_MODE,    /* Combined Output over OLDI 0 
and 1. */

+};
+
  struct dispc_features {
  int min_pclk_khz;
  int max_pclk_khz[DISPC_VP_MAX_BUS_TYPE];
@@ -131,6 +138,7 @@ int dispc_plane_setup(struct dispc_device *dispc, 
u32 hw_plane,

    u32 hw_videoport);
  int dispc_plane_enable(struct dispc_device *dispc, u32 hw_plane, 
bool enable);
  const u32 *dispc_plane_formats(struct dispc_device *dispc, unsigned 
int *len);
+int dispc_configure_oldi_mode(struct dispc_device *dispc, enum 
dispc_oldi_modes oldi_mode);

  int dispc_init(struct tidss_device *tidss);
  void dispc_remove(struct tidss_device *tidss);
diff --git a/drivers/gpu/drm/tidss/tidss_drv.h 
b/drivers/gpu/drm/tidss/tidss_drv.h

index d7f27b0b0315..2252ba0222ca 100644
--- a/drivers/gpu/drm/tidss/tidss_drv.h
+++ b/drivers/gpu/drm/tidss/tidss_drv.h
@@ -12,6 +12,9 @@
  #define TIDSS_MAX_PORTS 4
  #define TIDSS_MAX_PLANES 4
+/* For AM625-DSS with 2 OLDI TXes */

Re: [RFC PATCH v5 4/6] drm/tidss: Add support to configure OLDI mode for am625-dss.

2022-10-12 Thread Tomi Valkeinen 

On 28/09/2022 20:52, Aradhya Bhatia wrote:

The newer version of DSS (AM625-DSS) has 2 OLDI TXes at its disposal.
These can be configured to support the following modes:

1. OLDI_SINGLE_LINK_SINGLE_MODE
Single Output over OLDI 0.
+--++-+  +---+
|  || |  |   |
| CRTC +--->+ ENCODER +->| PANEL |
|  || |  |   |
+--++-+  +---+


Can you have single link on OLDI 1 (OLDI 0 off)? I don't know if that 
make sense on this platform, but if the pins for OLDI 0 and 1 are 
different, there might be a reason on some cases for that.



2. OLDI_SINGLE_LINK_CLONE_MODE
Duplicate Output over OLDI 0 and 1.
+--++-+  +---+
|  || |  |   |
| CRTC +---+--->| ENCODER +->| PANEL |
|  |   || |  |   |
+--+   |+-+  +---+
   |


I think you've got a tab in the line above, but otherwise use spaces.


|+-+  +---+
|| |  |   |
+--->| ENCODER +->| PANEL |
 | |  |   |
 +-+  +---+

3. OLDI_DUAL_LINK_MODE
Combined Output over OLDI 0 and 1.
+--++-+  +---+
|  || +->|   |
| CRTC +--->+ ENCODER |  | PANEL |
|  || +->|   |
+--++-+  +---+

Following the above pathways for different modes, 2 encoder/panel-bridge
pipes get created for clone mode, and 1 pipe in cases of single link and
dual link mode.

Add support for confgure the OLDI modes using of and lvds DRM helper


"configuring"


functions.

Signed-off-by: Aradhya Bhatia 
---
  drivers/gpu/drm/tidss/tidss_dispc.c |  11 +++
  drivers/gpu/drm/tidss/tidss_dispc.h |   8 ++
  drivers/gpu/drm/tidss/tidss_drv.h   |   3 +
  drivers/gpu/drm/tidss/tidss_kms.c   | 146 +++-
  4 files changed, 145 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c 
b/drivers/gpu/drm/tidss/tidss_dispc.c
index 34f0da4bb3e3..88008ad39b55 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -354,6 +354,8 @@ struct dispc_device {
  
  	bool is_enabled;
  
+	enum dispc_oldi_modes oldi_mode;

+
struct dss_vp_data vp_data[TIDSS_MAX_PORTS];
  
  	u32 *fourccs;

@@ -1958,6 +1960,15 @@ const u32 *dispc_plane_formats(struct dispc_device 
*dispc, unsigned int *len)
return dispc->fourccs;
  }
  
+int dispc_configure_oldi_mode(struct dispc_device *dispc,

+ enum dispc_oldi_modes oldi_mode)
+{
+   WARN_ON(!dispc);
+
+   dispc->oldi_mode = oldi_mode;
+   return 0;
+}


I think "configure" means more than just storing the value. Maybe 
dispc_set_oldi_mode(). And an empty line above the return.



+
  static s32 pixinc(int pixels, u8 ps)
  {
if (pixels == 1)
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h 
b/drivers/gpu/drm/tidss/tidss_dispc.h
index b66418e583ee..45cce1054832 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.h
+++ b/drivers/gpu/drm/tidss/tidss_dispc.h
@@ -64,6 +64,13 @@ enum dispc_dss_subrevision {
DISPC_AM625,
  };
  
+enum dispc_oldi_modes {

+   OLDI_MODE_OFF,  /* OLDI turned off / tied off 
in IP. */
+   OLDI_SINGLE_LINK_SINGLE_MODE,   /* Single Output over OLDI 0. */
+   OLDI_SINGLE_LINK_CLONE_MODE,/* Duplicate Output over OLDI 0 
and 1. */
+   OLDI_DUAL_LINK_MODE,/* Combined Output over OLDI 0 
and 1. */
+};
+
  struct dispc_features {
int min_pclk_khz;
int max_pclk_khz[DISPC_VP_MAX_BUS_TYPE];
@@ -131,6 +138,7 @@ int dispc_plane_setup(struct dispc_device *dispc, u32 
hw_plane,
  u32 hw_videoport);
  int dispc_plane_enable(struct dispc_device *dispc, u32 hw_plane, bool enable);
  const u32 *dispc_plane_formats(struct dispc_device *dispc, unsigned int *len);
+int dispc_configure_oldi_mode(struct dispc_device *dispc, enum 
dispc_oldi_modes oldi_mode);
  
  int dispc_init(struct tidss_device *tidss);

  void dispc_remove(struct tidss_device *tidss);
diff --git a/drivers/gpu/drm/tidss/tidss_drv.h 
b/drivers/gpu/drm/tidss/tidss_drv.h
index d7f27b0b0315..2252ba0222ca 100644
--- a/drivers/gpu/drm/tidss/tidss_drv.h
+++ b/drivers/gpu/drm/tidss/tidss_drv.h
@@ -12,6 +12,9 @@
  #define TIDSS_MAX_PORTS 4
  #define TIDSS_MAX_PLANES 4
  
+/* For AM625-DSS with 2 OLDI TXes */

+#define TIDSS_MAX_BRIDGE_PER_PIPE  2


"BRIDGES"?


+
  typedef u32 dispc_irq_t;
  
  struct tidss_device {

diff --git a/drivers/gpu/drm/tidss/tidss_kms.c 
b/drivers/gpu/drm/tidss/tidss_kms.c
index 666e527a0acf..73afe390f36d 100644
--- a/drivers/gpu/drm/tidss/tidss_kms.c
+++ b/drivers/gpu/drm/tidss/tidss_kms.c
@@ -107,32 +107,84 @@ static const struct drm_mode_config_funcs 
mode_config_funcs

[RFC PATCH v5 4/6] drm/tidss: Add support to configure OLDI mode for am625-dss.

2022-09-28 Thread Aradhya Bhatia 
The newer version of DSS (AM625-DSS) has 2 OLDI TXes at its disposal.
These can be configured to support the following modes:

1. OLDI_SINGLE_LINK_SINGLE_MODE
Single Output over OLDI 0.
+--++-+  +---+
|  || |  |   |
| CRTC +--->+ ENCODER +->| PANEL |
|  || |  |   |
+--++-+  +---+

2. OLDI_SINGLE_LINK_CLONE_MODE
Duplicate Output over OLDI 0 and 1.
+--++-+  +---+
|  || |  |   |
| CRTC +---+--->| ENCODER +->| PANEL |
|  |   || |  |   |
+--+   |+-+  +---+
   |
   |+-+  +---+
   || |  |   |
   +--->| ENCODER +->| PANEL |
| |  |   |
+-+  +---+

3. OLDI_DUAL_LINK_MODE
Combined Output over OLDI 0 and 1.
+--++-+  +---+
|  || +->|   |
| CRTC +--->+ ENCODER |  | PANEL |
|  || +->|   |
+--++-+  +---+

Following the above pathways for different modes, 2 encoder/panel-bridge
pipes get created for clone mode, and 1 pipe in cases of single link and
dual link mode.

Add support for confgure the OLDI modes using of and lvds DRM helper
functions.

Signed-off-by: Aradhya Bhatia 
---
 drivers/gpu/drm/tidss/tidss_dispc.c |  11 +++
 drivers/gpu/drm/tidss/tidss_dispc.h |   8 ++
 drivers/gpu/drm/tidss/tidss_drv.h   |   3 +
 drivers/gpu/drm/tidss/tidss_kms.c   | 146 +++-
 4 files changed, 145 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c 
b/drivers/gpu/drm/tidss/tidss_dispc.c
index 34f0da4bb3e3..88008ad39b55 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -354,6 +354,8 @@ struct dispc_device {
 
bool is_enabled;
 
+   enum dispc_oldi_modes oldi_mode;
+
struct dss_vp_data vp_data[TIDSS_MAX_PORTS];
 
u32 *fourccs;
@@ -1958,6 +1960,15 @@ const u32 *dispc_plane_formats(struct dispc_device 
*dispc, unsigned int *len)
return dispc->fourccs;
 }
 
+int dispc_configure_oldi_mode(struct dispc_device *dispc,
+ enum dispc_oldi_modes oldi_mode)
+{
+   WARN_ON(!dispc);
+
+   dispc->oldi_mode = oldi_mode;
+   return 0;
+}
+
 static s32 pixinc(int pixels, u8 ps)
 {
if (pixels == 1)
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h 
b/drivers/gpu/drm/tidss/tidss_dispc.h
index b66418e583ee..45cce1054832 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.h
+++ b/drivers/gpu/drm/tidss/tidss_dispc.h
@@ -64,6 +64,13 @@ enum dispc_dss_subrevision {
DISPC_AM625,
 };
 
+enum dispc_oldi_modes {
+   OLDI_MODE_OFF,  /* OLDI turned off / tied off 
in IP. */
+   OLDI_SINGLE_LINK_SINGLE_MODE,   /* Single Output over OLDI 0. */
+   OLDI_SINGLE_LINK_CLONE_MODE,/* Duplicate Output over OLDI 0 
and 1. */
+   OLDI_DUAL_LINK_MODE,/* Combined Output over OLDI 0 
and 1. */
+};
+
 struct dispc_features {
int min_pclk_khz;
int max_pclk_khz[DISPC_VP_MAX_BUS_TYPE];
@@ -131,6 +138,7 @@ int dispc_plane_setup(struct dispc_device *dispc, u32 
hw_plane,
  u32 hw_videoport);
 int dispc_plane_enable(struct dispc_device *dispc, u32 hw_plane, bool enable);
 const u32 *dispc_plane_formats(struct dispc_device *dispc, unsigned int *len);
+int dispc_configure_oldi_mode(struct dispc_device *dispc, enum 
dispc_oldi_modes oldi_mode);
 
 int dispc_init(struct tidss_device *tidss);
 void dispc_remove(struct tidss_device *tidss);
diff --git a/drivers/gpu/drm/tidss/tidss_drv.h 
b/drivers/gpu/drm/tidss/tidss_drv.h
index d7f27b0b0315..2252ba0222ca 100644
--- a/drivers/gpu/drm/tidss/tidss_drv.h
+++ b/drivers/gpu/drm/tidss/tidss_drv.h
@@ -12,6 +12,9 @@
 #define TIDSS_MAX_PORTS 4
 #define TIDSS_MAX_PLANES 4
 
+/* For AM625-DSS with 2 OLDI TXes */
+#define TIDSS_MAX_BRIDGE_PER_PIPE  2
+
 typedef u32 dispc_irq_t;
 
 struct tidss_device {
diff --git a/drivers/gpu/drm/tidss/tidss_kms.c 
b/drivers/gpu/drm/tidss/tidss_kms.c
index 666e527a0acf..73afe390f36d 100644
--- a/drivers/gpu/drm/tidss/tidss_kms.c
+++ b/drivers/gpu/drm/tidss/tidss_kms.c
@@ -107,32 +107,84 @@ static const struct drm_mode_config_funcs 
mode_config_funcs = {
.atomic_commit = drm_atomic_helper_commit,
 };
 
+static int tidss_get_oldi_mode(struct tidss_device *tidss)
+{
+   int pixel_order;
+   struct device_node *dss_ports, *oldi0_port, *oldi1_port;
+
+   dss_ports = of_get_next_child(tidss->dev->of_node, NULL);
+   oldi0_port = of_graph_get_port_by_id(dss_ports, 0);
+   oldi1_port = of_graph_get_port_by_id(dss_ports, 2);
+
+   if (!(oldi0_port && oldi1_port))
+   return OLDI_SINGLE_LINK_SINGLE_MODE;
+
+