[PATCH v1 0/3] fix primary corruption issue

[PATCH v1 0/3] fix primary corruption issue

[Kuogee Hsieh]

fix primary corruption :
1) move struc of msm_display_info to msm_drv.h
2) decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table
3) place edp at head of drm bridge chain to fix screen corruption


Kuogee Hsieh (3):
  drm/msm/dp: move struc of msm_display_info to msm_drv.h
  drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg
    table
  drm/msm/dp: place edp at head of drm bridge chain to fix screen
    corruption

 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 20 -------------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 16 +++++++++------
 drivers/gpu/drm/msm/dp/dp_display.c         | 30 ++++++++++++++++++++++-------
 drivers/gpu/drm/msm/msm_drv.h               | 21 ++++++++++++++++++++
 4 files changed, 54 insertions(+), 33 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v1 0/3] fix primary corruption issue

[Kuogee Hsieh]

fix primary corruption :
1) move struc of msm_display_info to msm_drv.h
2) decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table
3) place edp at head of drm bridge chain to fix screen corruption


Kuogee Hsieh (3):
  drm/msm/dp: move struc of msm_display_info to msm_drv.h
  drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg
    table
  drm/msm/dp: place edp at head of drm bridge chain to fix screen
    corruption

 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 20 -------------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 16 +++++++++------
 drivers/gpu/drm/msm/dp/dp_display.c         | 30 ++++++++++++++++++++++-------
 drivers/gpu/drm/msm/msm_drv.h               | 21 ++++++++++++++++++++
 4 files changed, 54 insertions(+), 33 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v1 1/3] drm/msm/dp: move struc of msm_display_info to msm_drv.h

[Kuogee Hsieh]

With current implementation, communication between interface driver and
upper mdss encoder layer are implemented through function calls. This
increase code complexity. Since struct msm_display_info contains msm
generic display information, it can be expended to contains more useful
information, such as widebus and dcs, in future to serve as communication
channel purpose between interface driver and upper mdss encoder layer so
that existing function calls can be eliminated.
This patch more struct msm_display_info to msm_drv.h to be visible by
whole msm scope.

Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 20 --------------------
 drivers/gpu/drm/msm/msm_drv.h               | 19 +++++++++++++++++++
 2 files changed, 19 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index 781d41c..6b604c5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -19,26 +19,6 @@
 #define IDLE_TIMEOUT	(66 - 16/2)
 
 /**
- * struct msm_display_info - defines display properties
- * @intf_type:          DRM_MODE_ENCODER_ type
- * @capabilities:       Bitmask of display flags
- * @num_of_h_tiles:     Number of horizontal tiles in case of split interface
- * @h_tile_instance:    Controller instance used per tile. Number of elements is
- *                      based on num_of_h_tiles
- * @is_te_using_watchdog_timer:  Boolean to indicate watchdog TE is
- *				 used instead of panel TE in cmd mode panels
- * @dsc:		DSC configuration data for DSC-enabled displays
- */
-struct msm_display_info {
-	int intf_type;
-	uint32_t capabilities;
-	uint32_t num_of_h_tiles;
-	uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
-	bool is_te_using_watchdog_timer;
-	struct msm_display_dsc_config *dsc;
-};
-
-/**
  * dpu_encoder_assign_crtc - Link the encoder to the crtc it's assigned to
  * @encoder:	encoder pointer
  * @crtc:	crtc pointer
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index fdbaad5..f9c263b 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -106,11 +106,30 @@ struct msm_drm_thread {
 	struct kthread_worker *worker;
 };
 
+<<<<<<< HEAD
 /* DSC config */
 struct msm_display_dsc_config {
 	struct drm_dsc_config *drm;
 };
 
+/**
+ * struct msm_display_info - defines display properties
+ * @intf_type:          DRM_MODE_ENCODER_ type
+ * @capabilities:       Bitmask of display flags
+ * @num_of_h_tiles:     Number of horizontal tiles in case of split interface
+ * @h_tile_instance:    Controller instance used per tile. Number of elements is
+ *                      based on num_of_h_tiles
+ * @is_te_using_watchdog_timer:  Boolean to indicate watchdog TE is
+ *				 used instead of panel TE in cmd mode panels
+ */
+struct msm_display_info {
+	int intf_type;
+	uint32_t capabilities;
+	uint32_t num_of_h_tiles;
+	uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
+	bool is_te_using_watchdog_timer;
+};
+
 struct msm_drm_private {
 
 	struct drm_device *dev;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v1 1/3] drm/msm/dp: move struc of msm_display_info to msm_drv.h

[Kuogee Hsieh]

With current implementation, communication between interface driver and
upper mdss encoder layer are implemented through function calls. This
increase code complexity. Since struct msm_display_info contains msm
generic display information, it can be expended to contains more useful
information, such as widebus and dcs, in future to serve as communication
channel purpose between interface driver and upper mdss encoder layer so
that existing function calls can be eliminated.
This patch more struct msm_display_info to msm_drv.h to be visible by
whole msm scope.

Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 20 --------------------
 drivers/gpu/drm/msm/msm_drv.h               | 19 +++++++++++++++++++
 2 files changed, 19 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index 781d41c..6b604c5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -19,26 +19,6 @@
 #define IDLE_TIMEOUT	(66 - 16/2)
 
 /**
- * struct msm_display_info - defines display properties
- * @intf_type:          DRM_MODE_ENCODER_ type
- * @capabilities:       Bitmask of display flags
- * @num_of_h_tiles:     Number of horizontal tiles in case of split interface
- * @h_tile_instance:    Controller instance used per tile. Number of elements is
- *                      based on num_of_h_tiles
- * @is_te_using_watchdog_timer:  Boolean to indicate watchdog TE is
- *				 used instead of panel TE in cmd mode panels
- * @dsc:		DSC configuration data for DSC-enabled displays
- */
-struct msm_display_info {
-	int intf_type;
-	uint32_t capabilities;
-	uint32_t num_of_h_tiles;
-	uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
-	bool is_te_using_watchdog_timer;
-	struct msm_display_dsc_config *dsc;
-};
-
-/**
  * dpu_encoder_assign_crtc - Link the encoder to the crtc it's assigned to
  * @encoder:	encoder pointer
  * @crtc:	crtc pointer
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index fdbaad5..f9c263b 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -106,11 +106,30 @@ struct msm_drm_thread {
 	struct kthread_worker *worker;
 };
 
+<<<<<<< HEAD
 /* DSC config */
 struct msm_display_dsc_config {
 	struct drm_dsc_config *drm;
 };
 
+/**
+ * struct msm_display_info - defines display properties
+ * @intf_type:          DRM_MODE_ENCODER_ type
+ * @capabilities:       Bitmask of display flags
+ * @num_of_h_tiles:     Number of horizontal tiles in case of split interface
+ * @h_tile_instance:    Controller instance used per tile. Number of elements is
+ *                      based on num_of_h_tiles
+ * @is_te_using_watchdog_timer:  Boolean to indicate watchdog TE is
+ *				 used instead of panel TE in cmd mode panels
+ */
+struct msm_display_info {
+	int intf_type;
+	uint32_t capabilities;
+	uint32_t num_of_h_tiles;
+	uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
+	bool is_te_using_watchdog_timer;
+};
+
 struct msm_drm_private {
 
 	struct drm_device *dev;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v1 1/3] drm/msm/dp: move struc of msm_display_info to msm_drv.h

[Doug Anderson]

Hi,

On Fri, Jun 24, 2022 at 10:15 AM Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
> With current implementation, communication between interface driver and
> upper mdss encoder layer are implemented through function calls. This
> increase code complexity. Since struct msm_display_info contains msm
> generic display information, it can be expended to contains more useful
> information, such as widebus and dcs, in future to serve as communication
> channel purpose between interface driver and upper mdss encoder layer so
> that existing function calls can be eliminated.
> This patch more struct msm_display_info to msm_drv.h to be visible by
> whole msm scope.
>
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 20 --------------------
>  drivers/gpu/drm/msm/msm_drv.h               | 19 +++++++++++++++++++
>  2 files changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> index 781d41c..6b604c5 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -19,26 +19,6 @@
>  #define IDLE_TIMEOUT   (66 - 16/2)
>
>  /**
> - * struct msm_display_info - defines display properties
> - * @intf_type:          DRM_MODE_ENCODER_ type
> - * @capabilities:       Bitmask of display flags
> - * @num_of_h_tiles:     Number of horizontal tiles in case of split interface
> - * @h_tile_instance:    Controller instance used per tile. Number of elements is
> - *                      based on num_of_h_tiles
> - * @is_te_using_watchdog_timer:  Boolean to indicate watchdog TE is
> - *                              used instead of panel TE in cmd mode panels
> - * @dsc:               DSC configuration data for DSC-enabled displays
> - */
> -struct msm_display_info {
> -       int intf_type;
> -       uint32_t capabilities;
> -       uint32_t num_of_h_tiles;
> -       uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
> -       bool is_te_using_watchdog_timer;
> -       struct msm_display_dsc_config *dsc;

So in the structure you're "moving" there's this member called "dsc".


> -};
> -
> -/**
>   * dpu_encoder_assign_crtc - Link the encoder to the crtc it's assigned to
>   * @encoder:   encoder pointer
>   * @crtc:      crtc pointer
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index fdbaad5..f9c263b 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -106,11 +106,30 @@ struct msm_drm_thread {
>         struct kthread_worker *worker;
>  };
>
> +<<<<<<< HEAD

The above doesn't look like valid C to me.


>  /* DSC config */
>  struct msm_display_dsc_config {
>         struct drm_dsc_config *drm;
>  };
>
> +/**
> + * struct msm_display_info - defines display properties
> + * @intf_type:          DRM_MODE_ENCODER_ type
> + * @capabilities:       Bitmask of display flags
> + * @num_of_h_tiles:     Number of horizontal tiles in case of split interface
> + * @h_tile_instance:    Controller instance used per tile. Number of elements is
> + *                      based on num_of_h_tiles
> + * @is_te_using_watchdog_timer:  Boolean to indicate watchdog TE is
> + *                              used instead of panel TE in cmd mode panels
> + */
> +struct msm_display_info {
> +       int intf_type;
> +       uint32_t capabilities;
> +       uint32_t num_of_h_tiles;
> +       uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
> +       bool is_te_using_watchdog_timer;

...but then when you "move" the structure to its new location, which
should be a noop, then <poof> the "dsc" variable vanishes (along with
the kernel doc description of it before the structure).

-Doug

Re: [PATCH v1 1/3] drm/msm/dp: move struc of msm_display_info to msm_drv.h

[Doug Anderson]

Hi,

On Fri, Jun 24, 2022 at 10:15 AM Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
> With current implementation, communication between interface driver and
> upper mdss encoder layer are implemented through function calls. This
> increase code complexity. Since struct msm_display_info contains msm
> generic display information, it can be expended to contains more useful
> information, such as widebus and dcs, in future to serve as communication
> channel purpose between interface driver and upper mdss encoder layer so
> that existing function calls can be eliminated.
> This patch more struct msm_display_info to msm_drv.h to be visible by
> whole msm scope.
>
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 20 --------------------
>  drivers/gpu/drm/msm/msm_drv.h               | 19 +++++++++++++++++++
>  2 files changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> index 781d41c..6b604c5 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -19,26 +19,6 @@
>  #define IDLE_TIMEOUT   (66 - 16/2)
>
>  /**
> - * struct msm_display_info - defines display properties
> - * @intf_type:          DRM_MODE_ENCODER_ type
> - * @capabilities:       Bitmask of display flags
> - * @num_of_h_tiles:     Number of horizontal tiles in case of split interface
> - * @h_tile_instance:    Controller instance used per tile. Number of elements is
> - *                      based on num_of_h_tiles
> - * @is_te_using_watchdog_timer:  Boolean to indicate watchdog TE is
> - *                              used instead of panel TE in cmd mode panels
> - * @dsc:               DSC configuration data for DSC-enabled displays
> - */
> -struct msm_display_info {
> -       int intf_type;
> -       uint32_t capabilities;
> -       uint32_t num_of_h_tiles;
> -       uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
> -       bool is_te_using_watchdog_timer;
> -       struct msm_display_dsc_config *dsc;

So in the structure you're "moving" there's this member called "dsc".


> -};
> -
> -/**
>   * dpu_encoder_assign_crtc - Link the encoder to the crtc it's assigned to
>   * @encoder:   encoder pointer
>   * @crtc:      crtc pointer
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index fdbaad5..f9c263b 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -106,11 +106,30 @@ struct msm_drm_thread {
>         struct kthread_worker *worker;
>  };
>
> +<<<<<<< HEAD

The above doesn't look like valid C to me.


>  /* DSC config */
>  struct msm_display_dsc_config {
>         struct drm_dsc_config *drm;
>  };
>
> +/**
> + * struct msm_display_info - defines display properties
> + * @intf_type:          DRM_MODE_ENCODER_ type
> + * @capabilities:       Bitmask of display flags
> + * @num_of_h_tiles:     Number of horizontal tiles in case of split interface
> + * @h_tile_instance:    Controller instance used per tile. Number of elements is
> + *                      based on num_of_h_tiles
> + * @is_te_using_watchdog_timer:  Boolean to indicate watchdog TE is
> + *                              used instead of panel TE in cmd mode panels
> + */
> +struct msm_display_info {
> +       int intf_type;
> +       uint32_t capabilities;
> +       uint32_t num_of_h_tiles;
> +       uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
> +       bool is_te_using_watchdog_timer;

...but then when you "move" the structure to its new location, which
should be a noop, then <poof> the "dsc" variable vanishes (along with
the kernel doc description of it before the structure).

-Doug

Re: [PATCH v1 1/3] drm/msm/dp: move struc of msm_display_info to msm_drv.h

[Kuogee Hsieh]

On 6/24/2022 2:40 PM, Doug Anderson wrote:
> Hi,
>
> On Fri, Jun 24, 2022 at 10:15 AM Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>> With current implementation, communication between interface driver and
>> upper mdss encoder layer are implemented through function calls. This
>> increase code complexity. Since struct msm_display_info contains msm
>> generic display information, it can be expended to contains more useful
>> information, such as widebus and dcs, in future to serve as communication
>> channel purpose between interface driver and upper mdss encoder layer so
>> that existing function calls can be eliminated.
>> This patch more struct msm_display_info to msm_drv.h to be visible by
>> whole msm scope.
>>
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 20 --------------------
>>   drivers/gpu/drm/msm/msm_drv.h               | 19 +++++++++++++++++++
>>   2 files changed, 19 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> index 781d41c..6b604c5 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> @@ -19,26 +19,6 @@
>>   #define IDLE_TIMEOUT   (66 - 16/2)
>>
>>   /**
>> - * struct msm_display_info - defines display properties
>> - * @intf_type:          DRM_MODE_ENCODER_ type
>> - * @capabilities:       Bitmask of display flags
>> - * @num_of_h_tiles:     Number of horizontal tiles in case of split interface
>> - * @h_tile_instance:    Controller instance used per tile. Number of elements is
>> - *                      based on num_of_h_tiles
>> - * @is_te_using_watchdog_timer:  Boolean to indicate watchdog TE is
>> - *                              used instead of panel TE in cmd mode panels
>> - * @dsc:               DSC configuration data for DSC-enabled displays
>> - */
>> -struct msm_display_info {
>> -       int intf_type;
>> -       uint32_t capabilities;
>> -       uint32_t num_of_h_tiles;
>> -       uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
>> -       bool is_te_using_watchdog_timer;
>> -       struct msm_display_dsc_config *dsc;
> So in the structure you're "moving" there's this member called "dsc".
>
>
>> -};
>> -
>> -/**
>>    * dpu_encoder_assign_crtc - Link the encoder to the crtc it's assigned to
>>    * @encoder:   encoder pointer
>>    * @crtc:      crtc pointer
>> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
>> index fdbaad5..f9c263b 100644
>> --- a/drivers/gpu/drm/msm/msm_drv.h
>> +++ b/drivers/gpu/drm/msm/msm_drv.h
>> @@ -106,11 +106,30 @@ struct msm_drm_thread {
>>          struct kthread_worker *worker;
>>   };
>>
>> +<<<<<<< HEAD
> The above doesn't look like valid C to me.
>
>
>>   /* DSC config */
>>   struct msm_display_dsc_config {
>>          struct drm_dsc_config *drm;
>>   };
>>
>> +/**
>> + * struct msm_display_info - defines display properties
>> + * @intf_type:          DRM_MODE_ENCODER_ type
>> + * @capabilities:       Bitmask of display flags
>> + * @num_of_h_tiles:     Number of horizontal tiles in case of split interface
>> + * @h_tile_instance:    Controller instance used per tile. Number of elements is
>> + *                      based on num_of_h_tiles
>> + * @is_te_using_watchdog_timer:  Boolean to indicate watchdog TE is
>> + *                              used instead of panel TE in cmd mode panels
>> + */
>> +struct msm_display_info {
>> +       int intf_type;
>> +       uint32_t capabilities;
>> +       uint32_t num_of_h_tiles;
>> +       uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
>> +       bool is_te_using_watchdog_timer;
> ...but then when you "move" the structure to its new location, which
> should be a noop, then <poof> the "dsc" variable vanishes (along with
> the kernel doc description of it before the structure).

Sorry, i did not resolve the conflicts correctly  when i cherry-pick 
them to msm-next tree.

Will fix them.

> -Doug

Re: [PATCH v1 1/3] drm/msm/dp: move struc of msm_display_info to msm_drv.h

[Kuogee Hsieh]

On 6/24/2022 2:40 PM, Doug Anderson wrote:
> Hi,
>
> On Fri, Jun 24, 2022 at 10:15 AM Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>> With current implementation, communication between interface driver and
>> upper mdss encoder layer are implemented through function calls. This
>> increase code complexity. Since struct msm_display_info contains msm
>> generic display information, it can be expended to contains more useful
>> information, such as widebus and dcs, in future to serve as communication
>> channel purpose between interface driver and upper mdss encoder layer so
>> that existing function calls can be eliminated.
>> This patch more struct msm_display_info to msm_drv.h to be visible by
>> whole msm scope.
>>
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 20 --------------------
>>   drivers/gpu/drm/msm/msm_drv.h               | 19 +++++++++++++++++++
>>   2 files changed, 19 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> index 781d41c..6b604c5 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> @@ -19,26 +19,6 @@
>>   #define IDLE_TIMEOUT   (66 - 16/2)
>>
>>   /**
>> - * struct msm_display_info - defines display properties
>> - * @intf_type:          DRM_MODE_ENCODER_ type
>> - * @capabilities:       Bitmask of display flags
>> - * @num_of_h_tiles:     Number of horizontal tiles in case of split interface
>> - * @h_tile_instance:    Controller instance used per tile. Number of elements is
>> - *                      based on num_of_h_tiles
>> - * @is_te_using_watchdog_timer:  Boolean to indicate watchdog TE is
>> - *                              used instead of panel TE in cmd mode panels
>> - * @dsc:               DSC configuration data for DSC-enabled displays
>> - */
>> -struct msm_display_info {
>> -       int intf_type;
>> -       uint32_t capabilities;
>> -       uint32_t num_of_h_tiles;
>> -       uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
>> -       bool is_te_using_watchdog_timer;
>> -       struct msm_display_dsc_config *dsc;
> So in the structure you're "moving" there's this member called "dsc".
>
>
>> -};
>> -
>> -/**
>>    * dpu_encoder_assign_crtc - Link the encoder to the crtc it's assigned to
>>    * @encoder:   encoder pointer
>>    * @crtc:      crtc pointer
>> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
>> index fdbaad5..f9c263b 100644
>> --- a/drivers/gpu/drm/msm/msm_drv.h
>> +++ b/drivers/gpu/drm/msm/msm_drv.h
>> @@ -106,11 +106,30 @@ struct msm_drm_thread {
>>          struct kthread_worker *worker;
>>   };
>>
>> +<<<<<<< HEAD
> The above doesn't look like valid C to me.
>
>
>>   /* DSC config */
>>   struct msm_display_dsc_config {
>>          struct drm_dsc_config *drm;
>>   };
>>
>> +/**
>> + * struct msm_display_info - defines display properties
>> + * @intf_type:          DRM_MODE_ENCODER_ type
>> + * @capabilities:       Bitmask of display flags
>> + * @num_of_h_tiles:     Number of horizontal tiles in case of split interface
>> + * @h_tile_instance:    Controller instance used per tile. Number of elements is
>> + *                      based on num_of_h_tiles
>> + * @is_te_using_watchdog_timer:  Boolean to indicate watchdog TE is
>> + *                              used instead of panel TE in cmd mode panels
>> + */
>> +struct msm_display_info {
>> +       int intf_type;
>> +       uint32_t capabilities;
>> +       uint32_t num_of_h_tiles;
>> +       uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
>> +       bool is_te_using_watchdog_timer;
> ...but then when you "move" the structure to its new location, which
> should be a noop, then <poof> the "dsc" variable vanishes (along with
> the kernel doc description of it before the structure).

Sorry, i did not resolve the conflicts correctly  when i cherry-pick 
them to msm-next tree.

Will fix them.

> -Doug

Re: [PATCH v1 1/3] drm/msm/dp: move struc of msm_display_info to msm_drv.h

[Dmitry Baryshkov]

On Sat, 25 Jun 2022 at 00:51, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
> On 6/24/2022 2:40 PM, Doug Anderson wrote:
> > On Fri, Jun 24, 2022 at 10:15 AM Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:

> >> +struct msm_display_info {
> >> +       int intf_type;
> >> +       uint32_t capabilities;
> >> +       uint32_t num_of_h_tiles;
> >> +       uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
> >> +       bool is_te_using_watchdog_timer;
> > ...but then when you "move" the structure to its new location, which
> > should be a noop, then <poof> the "dsc" variable vanishes (along with
> > the kernel doc description of it before the structure).
>
> Sorry, i did not resolve the conflicts correctly  when i cherry-pick
> them to msm-next tree.
>
> Will fix them.

I would strongly suggest doing development on top of msm/next or
linux-next. Using any other tree results in lots of problems starting
from the lame Fixes tags that we have been constantly seeing for the
last few months, conflicts when the patch is being rebased or
cherry-picked and ending up with the patches not being tested with the
tree that they are being applied to.

-- 
With best wishes
Dmitry

Re: [PATCH v1 1/3] drm/msm/dp: move struc of msm_display_info to msm_drv.h

[Dmitry Baryshkov]

On Sat, 25 Jun 2022 at 00:51, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
> On 6/24/2022 2:40 PM, Doug Anderson wrote:
> > On Fri, Jun 24, 2022 at 10:15 AM Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:

> >> +struct msm_display_info {
> >> +       int intf_type;
> >> +       uint32_t capabilities;
> >> +       uint32_t num_of_h_tiles;
> >> +       uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
> >> +       bool is_te_using_watchdog_timer;
> > ...but then when you "move" the structure to its new location, which
> > should be a noop, then <poof> the "dsc" variable vanishes (along with
> > the kernel doc description of it before the structure).
>
> Sorry, i did not resolve the conflicts correctly  when i cherry-pick
> them to msm-next tree.
>
> Will fix them.

I would strongly suggest doing development on top of msm/next or
linux-next. Using any other tree results in lots of problems starting
from the lame Fixes tags that we have been constantly seeing for the
last few months, conflicts when the patch is being rebased or
cherry-picked and ending up with the patches not being tested with the
tree that they are being applied to.

-- 
With best wishes
Dmitry

Re: [PATCH v1 1/3] drm/msm/dp: move struc of msm_display_info to msm_drv.h

[Dmitry Baryshkov]

On Fri, 24 Jun 2022 at 20:15, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
> With current implementation, communication between interface driver and
> upper mdss encoder layer are implemented through function calls. This
> increase code complexity. Since struct msm_display_info contains msm
> generic display information, it can be expended to contains more useful
> information, such as widebus and dcs, in future to serve as communication
> channel purpose between interface driver and upper mdss encoder layer so
> that existing function calls can be eliminated.
> This patch more struct msm_display_info to msm_drv.h to be visible by
> whole msm scope.

NAK.

The msm_display_info contains information used by (and useful to) DPU
only, it is not 'msm generic' info. For this reason it has been moved
from msm_drv.h to dpu_encoder.h inIn commit b7420739f112 ("drm/msm:
move struct msm_display_info to dpu driver") . Neither mdp5 nor mdp4
are going to use this structure.

At some point I thought too that we might be able to create a set of
data and functions to describe encoder backends (dsi, hdmi, dp). This
has failed for me. After musing over the msm_drv.h part containing
functions published by the backends, I could not end up with a set of
them being good enough. The only common part seems to be the
modeset_init, snapshot and (once DP gets the DSC interface)
get_dsc_config. The rest is backend-specific.

I would suggest returning to this topic if/once DSI gets wide bus
support or DP starts using bonded interfaces. Before that I don't
foresee that common data structure would simplify things rather than
complicating them.

>
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 20 --------------------
>  drivers/gpu/drm/msm/msm_drv.h               | 19 +++++++++++++++++++
>  2 files changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> index 781d41c..6b604c5 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -19,26 +19,6 @@
>  #define IDLE_TIMEOUT   (66 - 16/2)
>
>  /**
> - * struct msm_display_info - defines display properties
> - * @intf_type:          DRM_MODE_ENCODER_ type
> - * @capabilities:       Bitmask of display flags
> - * @num_of_h_tiles:     Number of horizontal tiles in case of split interface
> - * @h_tile_instance:    Controller instance used per tile. Number of elements is
> - *                      based on num_of_h_tiles
> - * @is_te_using_watchdog_timer:  Boolean to indicate watchdog TE is
> - *                              used instead of panel TE in cmd mode panels
> - * @dsc:               DSC configuration data for DSC-enabled displays
> - */
> -struct msm_display_info {
> -       int intf_type;
> -       uint32_t capabilities;
> -       uint32_t num_of_h_tiles;
> -       uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
> -       bool is_te_using_watchdog_timer;
> -       struct msm_display_dsc_config *dsc;
> -};
> -
> -/**
>   * dpu_encoder_assign_crtc - Link the encoder to the crtc it's assigned to
>   * @encoder:   encoder pointer
>   * @crtc:      crtc pointer
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index fdbaad5..f9c263b 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -106,11 +106,30 @@ struct msm_drm_thread {
>         struct kthread_worker *worker;
>  };
>
> +<<<<<<< HEAD

Not to mention that this patch is broken per se.

>  /* DSC config */
>  struct msm_display_dsc_config {
>         struct drm_dsc_config *drm;
>  };
>
> +/**
> + * struct msm_display_info - defines display properties
> + * @intf_type:          DRM_MODE_ENCODER_ type
> + * @capabilities:       Bitmask of display flags
> + * @num_of_h_tiles:     Number of horizontal tiles in case of split interface
> + * @h_tile_instance:    Controller instance used per tile. Number of elements is
> + *                      based on num_of_h_tiles
> + * @is_te_using_watchdog_timer:  Boolean to indicate watchdog TE is
> + *                              used instead of panel TE in cmd mode panels
> + */
> +struct msm_display_info {
> +       int intf_type;
> +       uint32_t capabilities;
> +       uint32_t num_of_h_tiles;
> +       uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
> +       bool is_te_using_watchdog_timer;
> +};
> +
>  struct msm_drm_private {
>
>         struct drm_device *dev;
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
--
With best wishes
Dmitry

Re: [PATCH v1 1/3] drm/msm/dp: move struc of msm_display_info to msm_drv.h

[Dmitry Baryshkov]

On Fri, 24 Jun 2022 at 20:15, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
> With current implementation, communication between interface driver and
> upper mdss encoder layer are implemented through function calls. This
> increase code complexity. Since struct msm_display_info contains msm
> generic display information, it can be expended to contains more useful
> information, such as widebus and dcs, in future to serve as communication
> channel purpose between interface driver and upper mdss encoder layer so
> that existing function calls can be eliminated.
> This patch more struct msm_display_info to msm_drv.h to be visible by
> whole msm scope.

NAK.

The msm_display_info contains information used by (and useful to) DPU
only, it is not 'msm generic' info. For this reason it has been moved
from msm_drv.h to dpu_encoder.h inIn commit b7420739f112 ("drm/msm:
move struct msm_display_info to dpu driver") . Neither mdp5 nor mdp4
are going to use this structure.

At some point I thought too that we might be able to create a set of
data and functions to describe encoder backends (dsi, hdmi, dp). This
has failed for me. After musing over the msm_drv.h part containing
functions published by the backends, I could not end up with a set of
them being good enough. The only common part seems to be the
modeset_init, snapshot and (once DP gets the DSC interface)
get_dsc_config. The rest is backend-specific.

I would suggest returning to this topic if/once DSI gets wide bus
support or DP starts using bonded interfaces. Before that I don't
foresee that common data structure would simplify things rather than
complicating them.

>
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 20 --------------------
>  drivers/gpu/drm/msm/msm_drv.h               | 19 +++++++++++++++++++
>  2 files changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> index 781d41c..6b604c5 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -19,26 +19,6 @@
>  #define IDLE_TIMEOUT   (66 - 16/2)
>
>  /**
> - * struct msm_display_info - defines display properties
> - * @intf_type:          DRM_MODE_ENCODER_ type
> - * @capabilities:       Bitmask of display flags
> - * @num_of_h_tiles:     Number of horizontal tiles in case of split interface
> - * @h_tile_instance:    Controller instance used per tile. Number of elements is
> - *                      based on num_of_h_tiles
> - * @is_te_using_watchdog_timer:  Boolean to indicate watchdog TE is
> - *                              used instead of panel TE in cmd mode panels
> - * @dsc:               DSC configuration data for DSC-enabled displays
> - */
> -struct msm_display_info {
> -       int intf_type;
> -       uint32_t capabilities;
> -       uint32_t num_of_h_tiles;
> -       uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
> -       bool is_te_using_watchdog_timer;
> -       struct msm_display_dsc_config *dsc;
> -};
> -
> -/**
>   * dpu_encoder_assign_crtc - Link the encoder to the crtc it's assigned to
>   * @encoder:   encoder pointer
>   * @crtc:      crtc pointer
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index fdbaad5..f9c263b 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -106,11 +106,30 @@ struct msm_drm_thread {
>         struct kthread_worker *worker;
>  };
>
> +<<<<<<< HEAD

Not to mention that this patch is broken per se.

>  /* DSC config */
>  struct msm_display_dsc_config {
>         struct drm_dsc_config *drm;
>  };
>
> +/**
> + * struct msm_display_info - defines display properties
> + * @intf_type:          DRM_MODE_ENCODER_ type
> + * @capabilities:       Bitmask of display flags
> + * @num_of_h_tiles:     Number of horizontal tiles in case of split interface
> + * @h_tile_instance:    Controller instance used per tile. Number of elements is
> + *                      based on num_of_h_tiles
> + * @is_te_using_watchdog_timer:  Boolean to indicate watchdog TE is
> + *                              used instead of panel TE in cmd mode panels
> + */
> +struct msm_display_info {
> +       int intf_type;
> +       uint32_t capabilities;
> +       uint32_t num_of_h_tiles;
> +       uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
> +       bool is_te_using_watchdog_timer;
> +};
> +
>  struct msm_drm_private {
>
>         struct drm_device *dev;
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
--
With best wishes
Dmitry

[PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Kuogee Hsieh]

Current the index (dp->id) of DP descriptor table (scxxxx_dp_cfg[]) are tightly
coupled with DP controller_id. This means DP use controller id 0 must be placed
at first entry of DP descriptor table (scxxxx_dp_cfg[]). Otherwise the internal
INTF will mismatch controller_id. This will cause controller kickoff wrong
interface timing engine and cause dpu_encoder_phys_vid_wait_for_commit_done
vblank timeout error.

This patch add controller_id field into struct msm_dp_desc to break the tightly
coupled relationship between index (dp->id) of DP descriptor table with DP
controller_id.

Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 16 ++++++++++------
 drivers/gpu/drm/msm/dp/dp_display.c     | 30 +++++++++++++++++++++++-------
 drivers/gpu/drm/msm/msm_drv.h           |  2 ++
 3 files changed, 35 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 2b9d931..8feeb89 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -615,7 +615,7 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev,
 					    struct dpu_kms *dpu_kms)
 {
 	struct drm_encoder *encoder = NULL;
-	struct msm_display_info info;
+	struct msm_display_info *info;
 	int rc;
 	int i;
 
@@ -637,11 +637,15 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev,
 			return rc;
 		}
 
-		info.num_of_h_tiles = 1;
-		info.h_tile_instance[0] = i;
-		info.capabilities = MSM_DISPLAY_CAP_VID_MODE;
-		info.intf_type = encoder->encoder_type;
-		rc = dpu_encoder_setup(dev, encoder, &info);
+		info = &priv->info[i];
+		info->intf_type = encoder->encoder_type;
+		/*
+		 * info->capabilities, info->num_of_h_tiles and
+		 * info->h_tile_instance are populated at
+		 * dp_display_bind()
+		 */
+
+		rc = dpu_encoder_setup(dev, encoder, info);
 		if (rc) {
 			DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
 				  encoder->base.id, rc);
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index da5c03a..a87a9d8 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -77,6 +77,7 @@ struct dp_display_private {
 	int irq;
 
 	unsigned int id;
+	unsigned int controller_id;
 
 	/* state variables */
 	bool core_initialized;
@@ -123,6 +124,7 @@ struct dp_display_private {
 struct msm_dp_desc {
 	phys_addr_t io_start;
 	unsigned int connector_type;
+	unsigned int controller_id;
 	bool wide_bus_en;
 };
 
@@ -133,31 +135,38 @@ struct msm_dp_config {
 
 static const struct msm_dp_config sc7180_dp_cfg = {
 	.descs = (const struct msm_dp_desc[]) {
-		[MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
+		{ .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort,
+		.controller_id = MSM_DP_CONTROLLER_0 },
 	},
 	.num_descs = 1,
 };
 
 static const struct msm_dp_config sc7280_dp_cfg = {
 	.descs = (const struct msm_dp_desc[]) {
-		[MSM_DP_CONTROLLER_0] =	{ .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
-		[MSM_DP_CONTROLLER_1] =	{ .io_start = 0x0aea0000, .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
+		{ .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort,
+		.controller_id = MSM_DP_CONTROLLER_0, .wide_bus_en = true },
+		{ .io_start = 0x0aea0000, .connector_type = DRM_MODE_CONNECTOR_eDP, 
+		.controller_id = MSM_DP_CONTROLLER_1, .wide_bus_en = true },
 	},
 	.num_descs = 2,
 };
 
 static const struct msm_dp_config sc8180x_dp_cfg = {
 	.descs = (const struct msm_dp_desc[]) {
-		[MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
-		[MSM_DP_CONTROLLER_1] = { .io_start = 0x0ae98000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
-		[MSM_DP_CONTROLLER_2] = { .io_start = 0x0ae9a000, .connector_type = DRM_MODE_CONNECTOR_eDP },
+		{.io_start = 0x0ae9a000, .connector_type = DRM_MODE_CONNECTOR_eDP,
+		.controller_id = MSM_DP_CONTROLLER_2 },
+		{ .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort,
+		.controller_id = MSM_DP_CONTROLLER_0 },
+		{ .io_start = 0x0ae98000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort,
+		.controller_id = MSM_DP_CONTROLLER_1 },
 	},
 	.num_descs = 3,
 };
 
 static const struct msm_dp_config sm8350_dp_cfg = {
 	.descs = (const struct msm_dp_desc[]) {
-		[MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
+		{ .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort,
+		.controller_id = MSM_DP_CONTROLLER_0 },
 	},
 	.num_descs = 1,
 };
@@ -260,10 +269,16 @@ static int dp_display_bind(struct device *dev, struct device *master,
 	struct dp_display_private *dp = dev_get_dp_display_private(dev);
 	struct msm_drm_private *priv = dev_get_drvdata(master);
 	struct drm_device *drm = priv->dev;
+	struct msm_display_info *info;
 
 	dp->dp_display.drm_dev = drm;
 	priv->dp[dp->id] = &dp->dp_display;
 
+	info = &priv->info[dp->id];
+	info->num_of_h_tiles = 1;
+	info->h_tile_instance[0] = dp->controller_id;
+	info->capabilities = MSM_DISPLAY_CAP_VID_MODE;
+	
 	rc = dp->parser->parse(dp->parser);
 	if (rc) {
 		DRM_ERROR("device tree parsing failed\n");
@@ -1308,6 +1323,7 @@ static int dp_display_probe(struct platform_device *pdev)
 	dp->pdev = pdev;
 	dp->name = "drm_dp";
 	dp->dp_display.connector_type = desc->connector_type;
+	dp->controller_id =  desc->controller_id;
 	dp->wide_bus_en = desc->wide_bus_en;
 	dp->dp_display.is_edp =
 		(dp->dp_display.connector_type == DRM_MODE_CONNECTOR_eDP);
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index f9c263b..71ab699 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -150,6 +150,8 @@ struct msm_drm_private {
 
 	struct msm_dp *dp[MSM_DP_CONTROLLER_COUNT];
 
+	struct msm_display_info info[MSM_DP_CONTROLLER_COUNT];
+
 	/* when we have more than one 'msm_gpu' these need to be an array: */
 	struct msm_gpu *gpu;
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Kuogee Hsieh]

Current the index (dp->id) of DP descriptor table (scxxxx_dp_cfg[]) are tightly
coupled with DP controller_id. This means DP use controller id 0 must be placed
at first entry of DP descriptor table (scxxxx_dp_cfg[]). Otherwise the internal
INTF will mismatch controller_id. This will cause controller kickoff wrong
interface timing engine and cause dpu_encoder_phys_vid_wait_for_commit_done
vblank timeout error.

This patch add controller_id field into struct msm_dp_desc to break the tightly
coupled relationship between index (dp->id) of DP descriptor table with DP
controller_id.

Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 16 ++++++++++------
 drivers/gpu/drm/msm/dp/dp_display.c     | 30 +++++++++++++++++++++++-------
 drivers/gpu/drm/msm/msm_drv.h           |  2 ++
 3 files changed, 35 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 2b9d931..8feeb89 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -615,7 +615,7 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev,
 					    struct dpu_kms *dpu_kms)
 {
 	struct drm_encoder *encoder = NULL;
-	struct msm_display_info info;
+	struct msm_display_info *info;
 	int rc;
 	int i;
 
@@ -637,11 +637,15 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev,
 			return rc;
 		}
 
-		info.num_of_h_tiles = 1;
-		info.h_tile_instance[0] = i;
-		info.capabilities = MSM_DISPLAY_CAP_VID_MODE;
-		info.intf_type = encoder->encoder_type;
-		rc = dpu_encoder_setup(dev, encoder, &info);
+		info = &priv->info[i];
+		info->intf_type = encoder->encoder_type;
+		/*
+		 * info->capabilities, info->num_of_h_tiles and
+		 * info->h_tile_instance are populated at
+		 * dp_display_bind()
+		 */
+
+		rc = dpu_encoder_setup(dev, encoder, info);
 		if (rc) {
 			DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
 				  encoder->base.id, rc);
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index da5c03a..a87a9d8 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -77,6 +77,7 @@ struct dp_display_private {
 	int irq;
 
 	unsigned int id;
+	unsigned int controller_id;
 
 	/* state variables */
 	bool core_initialized;
@@ -123,6 +124,7 @@ struct dp_display_private {
 struct msm_dp_desc {
 	phys_addr_t io_start;
 	unsigned int connector_type;
+	unsigned int controller_id;
 	bool wide_bus_en;
 };
 
@@ -133,31 +135,38 @@ struct msm_dp_config {
 
 static const struct msm_dp_config sc7180_dp_cfg = {
 	.descs = (const struct msm_dp_desc[]) {
-		[MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
+		{ .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort,
+		.controller_id = MSM_DP_CONTROLLER_0 },
 	},
 	.num_descs = 1,
 };
 
 static const struct msm_dp_config sc7280_dp_cfg = {
 	.descs = (const struct msm_dp_desc[]) {
-		[MSM_DP_CONTROLLER_0] =	{ .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
-		[MSM_DP_CONTROLLER_1] =	{ .io_start = 0x0aea0000, .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
+		{ .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort,
+		.controller_id = MSM_DP_CONTROLLER_0, .wide_bus_en = true },
+		{ .io_start = 0x0aea0000, .connector_type = DRM_MODE_CONNECTOR_eDP, 
+		.controller_id = MSM_DP_CONTROLLER_1, .wide_bus_en = true },
 	},
 	.num_descs = 2,
 };
 
 static const struct msm_dp_config sc8180x_dp_cfg = {
 	.descs = (const struct msm_dp_desc[]) {
-		[MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
-		[MSM_DP_CONTROLLER_1] = { .io_start = 0x0ae98000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
-		[MSM_DP_CONTROLLER_2] = { .io_start = 0x0ae9a000, .connector_type = DRM_MODE_CONNECTOR_eDP },
+		{.io_start = 0x0ae9a000, .connector_type = DRM_MODE_CONNECTOR_eDP,
+		.controller_id = MSM_DP_CONTROLLER_2 },
+		{ .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort,
+		.controller_id = MSM_DP_CONTROLLER_0 },
+		{ .io_start = 0x0ae98000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort,
+		.controller_id = MSM_DP_CONTROLLER_1 },
 	},
 	.num_descs = 3,
 };
 
 static const struct msm_dp_config sm8350_dp_cfg = {
 	.descs = (const struct msm_dp_desc[]) {
-		[MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
+		{ .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort,
+		.controller_id = MSM_DP_CONTROLLER_0 },
 	},
 	.num_descs = 1,
 };
@@ -260,10 +269,16 @@ static int dp_display_bind(struct device *dev, struct device *master,
 	struct dp_display_private *dp = dev_get_dp_display_private(dev);
 	struct msm_drm_private *priv = dev_get_drvdata(master);
 	struct drm_device *drm = priv->dev;
+	struct msm_display_info *info;
 
 	dp->dp_display.drm_dev = drm;
 	priv->dp[dp->id] = &dp->dp_display;
 
+	info = &priv->info[dp->id];
+	info->num_of_h_tiles = 1;
+	info->h_tile_instance[0] = dp->controller_id;
+	info->capabilities = MSM_DISPLAY_CAP_VID_MODE;
+	
 	rc = dp->parser->parse(dp->parser);
 	if (rc) {
 		DRM_ERROR("device tree parsing failed\n");
@@ -1308,6 +1323,7 @@ static int dp_display_probe(struct platform_device *pdev)
 	dp->pdev = pdev;
 	dp->name = "drm_dp";
 	dp->dp_display.connector_type = desc->connector_type;
+	dp->controller_id =  desc->controller_id;
 	dp->wide_bus_en = desc->wide_bus_en;
 	dp->dp_display.is_edp =
 		(dp->dp_display.connector_type == DRM_MODE_CONNECTOR_eDP);
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index f9c263b..71ab699 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -150,6 +150,8 @@ struct msm_drm_private {
 
 	struct msm_dp *dp[MSM_DP_CONTROLLER_COUNT];
 
+	struct msm_display_info info[MSM_DP_CONTROLLER_COUNT];
+
 	/* when we have more than one 'msm_gpu' these need to be an array: */
 	struct msm_gpu *gpu;
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Stephen Boyd]

Quoting Kuogee Hsieh (2022-06-24 10:15:11)
> Current the index (dp->id) of DP descriptor table (scxxxx_dp_cfg[]) are tightly
> coupled with DP controller_id. This means DP use controller id 0 must be placed
> at first entry of DP descriptor table (scxxxx_dp_cfg[]). Otherwise the internal
> INTF will mismatch controller_id. This will cause controller kickoff wrong
> interface timing engine and cause dpu_encoder_phys_vid_wait_for_commit_done
> vblank timeout error.
>
> This patch add controller_id field into struct msm_dp_desc to break the tightly
> coupled relationship between index (dp->id) of DP descriptor table with DP
> controller_id.

Please no. This reverts the intention of commit bb3de286d992
("drm/msm/dp: Support up to 3 DP controllers")

    A new enum is introduced to document the connection between the
    instances referenced in the dpu_intf_cfg array and the controllers in
    the DP driver and sc7180 is updated.

It sounds like the intent of that commit failed to make a strong enough
connection. Now it needs to match the INTF number as well? I can't
really figure out what is actually wrong, because this patch undoes that
intentional tight coupling. Is the next patch the important part that
flips the order of the two interfaces?

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Stephen Boyd]

Quoting Kuogee Hsieh (2022-06-24 10:15:11)
> Current the index (dp->id) of DP descriptor table (scxxxx_dp_cfg[]) are tightly
> coupled with DP controller_id. This means DP use controller id 0 must be placed
> at first entry of DP descriptor table (scxxxx_dp_cfg[]). Otherwise the internal
> INTF will mismatch controller_id. This will cause controller kickoff wrong
> interface timing engine and cause dpu_encoder_phys_vid_wait_for_commit_done
> vblank timeout error.
>
> This patch add controller_id field into struct msm_dp_desc to break the tightly
> coupled relationship between index (dp->id) of DP descriptor table with DP
> controller_id.

Please no. This reverts the intention of commit bb3de286d992
("drm/msm/dp: Support up to 3 DP controllers")

    A new enum is introduced to document the connection between the
    instances referenced in the dpu_intf_cfg array and the controllers in
    the DP driver and sc7180 is updated.

It sounds like the intent of that commit failed to make a strong enough
connection. Now it needs to match the INTF number as well? I can't
really figure out what is actually wrong, because this patch undoes that
intentional tight coupling. Is the next patch the important part that
flips the order of the two interfaces?

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Kuogee Hsieh]

On 6/24/2022 1:00 PM, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2022-06-24 10:15:11)
>> Current the index (dp->id) of DP descriptor table (scxxxx_dp_cfg[]) are tightly
>> coupled with DP controller_id. This means DP use controller id 0 must be placed
>> at first entry of DP descriptor table (scxxxx_dp_cfg[]). Otherwise the internal
>> INTF will mismatch controller_id. This will cause controller kickoff wrong
>> interface timing engine and cause dpu_encoder_phys_vid_wait_for_commit_done
>> vblank timeout error.
>>
>> This patch add controller_id field into struct msm_dp_desc to break the tightly
>> coupled relationship between index (dp->id) of DP descriptor table with DP
>> controller_id.
> Please no. This reverts the intention of commit bb3de286d992
> ("drm/msm/dp: Support up to 3 DP controllers")
>
>      A new enum is introduced to document the connection between the
>      instances referenced in the dpu_intf_cfg array and the controllers in
>      the DP driver and sc7180 is updated.
>
> It sounds like the intent of that commit failed to make a strong enough
> connection. Now it needs to match the INTF number as well? I can't
> really figure out what is actually wrong, because this patch undoes that
> intentional tight coupling. Is the next patch the important part that
> flips the order of the two interfaces?

The commit bb3de286d992have two problems,

1)  The below sc7280_dp_cfg will not work, if eDP use 
MSM_DP_CONTROLLER_2 instead of  MSM_DP_CONTROLLER_1

since it have num_descs =2 but eDP is at index 2 (CONTROLLER_2) which 
never be reached.

static const struct msm_dp_config sc7280_dp_cfg = {
         .descs = (const struct msm_dp_desc[]) {
                 [MSM_DP_CONTROLLER_2] = { .io_start = 0x0aea0000, 
.connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
                 [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, 
.connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
         },
         .num_descs = 2,
};

2)  DP always has index of 0 (dp->id = 0) and the first one to call 
msm_dp_modeset_init(). This make DP always place at head of bridge chain.

At next patch eDP must be placed at head of bridge chain to fix eDP 
corruption issue. This is the purpose of this patch. I will revise the 
commit text.

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Kuogee Hsieh]

On 6/24/2022 1:00 PM, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2022-06-24 10:15:11)
>> Current the index (dp->id) of DP descriptor table (scxxxx_dp_cfg[]) are tightly
>> coupled with DP controller_id. This means DP use controller id 0 must be placed
>> at first entry of DP descriptor table (scxxxx_dp_cfg[]). Otherwise the internal
>> INTF will mismatch controller_id. This will cause controller kickoff wrong
>> interface timing engine and cause dpu_encoder_phys_vid_wait_for_commit_done
>> vblank timeout error.
>>
>> This patch add controller_id field into struct msm_dp_desc to break the tightly
>> coupled relationship between index (dp->id) of DP descriptor table with DP
>> controller_id.
> Please no. This reverts the intention of commit bb3de286d992
> ("drm/msm/dp: Support up to 3 DP controllers")
>
>      A new enum is introduced to document the connection between the
>      instances referenced in the dpu_intf_cfg array and the controllers in
>      the DP driver and sc7180 is updated.
>
> It sounds like the intent of that commit failed to make a strong enough
> connection. Now it needs to match the INTF number as well? I can't
> really figure out what is actually wrong, because this patch undoes that
> intentional tight coupling. Is the next patch the important part that
> flips the order of the two interfaces?

The commit bb3de286d992have two problems,

1)  The below sc7280_dp_cfg will not work, if eDP use 
MSM_DP_CONTROLLER_2 instead of  MSM_DP_CONTROLLER_1

since it have num_descs =2 but eDP is at index 2 (CONTROLLER_2) which 
never be reached.

static const struct msm_dp_config sc7280_dp_cfg = {
         .descs = (const struct msm_dp_desc[]) {
                 [MSM_DP_CONTROLLER_2] = { .io_start = 0x0aea0000, 
.connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
                 [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, 
.connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
         },
         .num_descs = 2,
};

2)  DP always has index of 0 (dp->id = 0) and the first one to call 
msm_dp_modeset_init(). This make DP always place at head of bridge chain.

At next patch eDP must be placed at head of bridge chain to fix eDP 
corruption issue. This is the purpose of this patch. I will revise the 
commit text.

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Stephen Boyd]

Quoting Kuogee Hsieh (2022-06-24 14:17:50)
>
> On 6/24/2022 1:00 PM, Stephen Boyd wrote:
> > Quoting Kuogee Hsieh (2022-06-24 10:15:11)
> >> Current the index (dp->id) of DP descriptor table (scxxxx_dp_cfg[]) are tightly
> >> coupled with DP controller_id. This means DP use controller id 0 must be placed
> >> at first entry of DP descriptor table (scxxxx_dp_cfg[]). Otherwise the internal
> >> INTF will mismatch controller_id. This will cause controller kickoff wrong
> >> interface timing engine and cause dpu_encoder_phys_vid_wait_for_commit_done
> >> vblank timeout error.
> >>
> >> This patch add controller_id field into struct msm_dp_desc to break the tightly
> >> coupled relationship between index (dp->id) of DP descriptor table with DP
> >> controller_id.
> > Please no. This reverts the intention of commit bb3de286d992
> > ("drm/msm/dp: Support up to 3 DP controllers")
> >
> >      A new enum is introduced to document the connection between the
> >      instances referenced in the dpu_intf_cfg array and the controllers in
> >      the DP driver and sc7180 is updated.
> >
> > It sounds like the intent of that commit failed to make a strong enough
> > connection. Now it needs to match the INTF number as well? I can't
> > really figure out what is actually wrong, because this patch undoes that
> > intentional tight coupling. Is the next patch the important part that
> > flips the order of the two interfaces?
>
> The commit bb3de286d992have two problems,
>
> 1)  The below sc7280_dp_cfg will not work, if eDP use
> MSM_DP_CONTROLLER_2 instead of  MSM_DP_CONTROLLER_1

Why would we use three indices for an soc that only has two indices
possible? This is not a real problem?

>
> since it have num_descs =2 but eDP is at index 2 (CONTROLLER_2) which
> never be reached.
>
> static const struct msm_dp_config sc7280_dp_cfg = {
>          .descs = (const struct msm_dp_desc[]) {
>                  [MSM_DP_CONTROLLER_2] = { .io_start = 0x0aea0000,
> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>                  [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
>          },
>          .num_descs = 2,
> };
>
> 2)  DP always has index of 0 (dp->id = 0) and the first one to call
> msm_dp_modeset_init(). This make DP always place at head of bridge chain.

What does this mean? Are you talking about the list of bridges in drm
core, i.e. 'bridge_list'?

>
> At next patch eDP must be placed at head of bridge chain to fix eDP
> corruption issue. This is the purpose of this patch. I will revise the
> commit text.
>

Wouldn't that be "broken" again if we decided to change drm_bridge_add()
to add to the list head instead of list tail? Or if somehow
msm_dp_modeset_init() was called in a different order so that the DP
bridge was added before the eDP bridge?

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Stephen Boyd]

Quoting Kuogee Hsieh (2022-06-24 14:17:50)
>
> On 6/24/2022 1:00 PM, Stephen Boyd wrote:
> > Quoting Kuogee Hsieh (2022-06-24 10:15:11)
> >> Current the index (dp->id) of DP descriptor table (scxxxx_dp_cfg[]) are tightly
> >> coupled with DP controller_id. This means DP use controller id 0 must be placed
> >> at first entry of DP descriptor table (scxxxx_dp_cfg[]). Otherwise the internal
> >> INTF will mismatch controller_id. This will cause controller kickoff wrong
> >> interface timing engine and cause dpu_encoder_phys_vid_wait_for_commit_done
> >> vblank timeout error.
> >>
> >> This patch add controller_id field into struct msm_dp_desc to break the tightly
> >> coupled relationship between index (dp->id) of DP descriptor table with DP
> >> controller_id.
> > Please no. This reverts the intention of commit bb3de286d992
> > ("drm/msm/dp: Support up to 3 DP controllers")
> >
> >      A new enum is introduced to document the connection between the
> >      instances referenced in the dpu_intf_cfg array and the controllers in
> >      the DP driver and sc7180 is updated.
> >
> > It sounds like the intent of that commit failed to make a strong enough
> > connection. Now it needs to match the INTF number as well? I can't
> > really figure out what is actually wrong, because this patch undoes that
> > intentional tight coupling. Is the next patch the important part that
> > flips the order of the two interfaces?
>
> The commit bb3de286d992have two problems,
>
> 1)  The below sc7280_dp_cfg will not work, if eDP use
> MSM_DP_CONTROLLER_2 instead of  MSM_DP_CONTROLLER_1

Why would we use three indices for an soc that only has two indices
possible? This is not a real problem?

>
> since it have num_descs =2 but eDP is at index 2 (CONTROLLER_2) which
> never be reached.
>
> static const struct msm_dp_config sc7280_dp_cfg = {
>          .descs = (const struct msm_dp_desc[]) {
>                  [MSM_DP_CONTROLLER_2] = { .io_start = 0x0aea0000,
> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>                  [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
>          },
>          .num_descs = 2,
> };
>
> 2)  DP always has index of 0 (dp->id = 0) and the first one to call
> msm_dp_modeset_init(). This make DP always place at head of bridge chain.

What does this mean? Are you talking about the list of bridges in drm
core, i.e. 'bridge_list'?

>
> At next patch eDP must be placed at head of bridge chain to fix eDP
> corruption issue. This is the purpose of this patch. I will revise the
> commit text.
>

Wouldn't that be "broken" again if we decided to change drm_bridge_add()
to add to the list head instead of list tail? Or if somehow
msm_dp_modeset_init() was called in a different order so that the DP
bridge was added before the eDP bridge?

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Kuogee Hsieh]

On 6/24/2022 2:40 PM, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2022-06-24 14:17:50)
>> On 6/24/2022 1:00 PM, Stephen Boyd wrote:
>>> Quoting Kuogee Hsieh (2022-06-24 10:15:11)
>>>> Current the index (dp->id) of DP descriptor table (scxxxx_dp_cfg[]) are tightly
>>>> coupled with DP controller_id. This means DP use controller id 0 must be placed
>>>> at first entry of DP descriptor table (scxxxx_dp_cfg[]). Otherwise the internal
>>>> INTF will mismatch controller_id. This will cause controller kickoff wrong
>>>> interface timing engine and cause dpu_encoder_phys_vid_wait_for_commit_done
>>>> vblank timeout error.
>>>>
>>>> This patch add controller_id field into struct msm_dp_desc to break the tightly
>>>> coupled relationship between index (dp->id) of DP descriptor table with DP
>>>> controller_id.
>>> Please no. This reverts the intention of commit bb3de286d992
>>> ("drm/msm/dp: Support up to 3 DP controllers")
>>>
>>>       A new enum is introduced to document the connection between the
>>>       instances referenced in the dpu_intf_cfg array and the controllers in
>>>       the DP driver and sc7180 is updated.
>>>
>>> It sounds like the intent of that commit failed to make a strong enough
>>> connection. Now it needs to match the INTF number as well? I can't
>>> really figure out what is actually wrong, because this patch undoes that
>>> intentional tight coupling. Is the next patch the important part that
>>> flips the order of the two interfaces?
>> The commit bb3de286d992have two problems,
>>
>> 1)  The below sc7280_dp_cfg will not work, if eDP use
>> MSM_DP_CONTROLLER_2 instead of  MSM_DP_CONTROLLER_1
> Why would we use three indices for an soc that only has two indices
> possible? This is not a real problem?

I do not what will happen at future, it may have more dp controller use 
late.

at current soc, below table has only one eDP will not work either.

static const struct msm_dp_config sc7280_dp_cfg = {
          .descs = (const struct msm_dp_desc[]) {
                  [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000,
.connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },

          .num_descs = 1,
};

>
>> since it have num_descs =2 but eDP is at index 2 (CONTROLLER_2) which
>> never be reached.
>>
>> static const struct msm_dp_config sc7280_dp_cfg = {
>>           .descs = (const struct msm_dp_desc[]) {
>>                   [MSM_DP_CONTROLLER_2] = { .io_start = 0x0aea0000,
>> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>>                   [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
>>           },
>>           .num_descs = 2,
>> };
>>
>> 2)  DP always has index of 0 (dp->id = 0) and the first one to call
>> msm_dp_modeset_init(). This make DP always place at head of bridge chain.
> What does this mean? Are you talking about the list of bridges in drm
> core, i.e. 'bridge_list'?
yes,
>
>> At next patch eDP must be placed at head of bridge chain to fix eDP
>> corruption issue. This is the purpose of this patch. I will revise the
>> commit text.
>>
> Wouldn't that be "broken" again if we decided to change drm_bridge_add()
> to add to the list head instead of list tail? Or if somehow
> msm_dp_modeset_init() was called in a different order so that the DP
> bridge was added before the eDP bridge?

we have no control of drm_bridge_add().

Since drm perform screen update following bridge chain sequentially, we 
have to make sure primary always update first.

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Kuogee Hsieh]

On 6/24/2022 2:40 PM, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2022-06-24 14:17:50)
>> On 6/24/2022 1:00 PM, Stephen Boyd wrote:
>>> Quoting Kuogee Hsieh (2022-06-24 10:15:11)
>>>> Current the index (dp->id) of DP descriptor table (scxxxx_dp_cfg[]) are tightly
>>>> coupled with DP controller_id. This means DP use controller id 0 must be placed
>>>> at first entry of DP descriptor table (scxxxx_dp_cfg[]). Otherwise the internal
>>>> INTF will mismatch controller_id. This will cause controller kickoff wrong
>>>> interface timing engine and cause dpu_encoder_phys_vid_wait_for_commit_done
>>>> vblank timeout error.
>>>>
>>>> This patch add controller_id field into struct msm_dp_desc to break the tightly
>>>> coupled relationship between index (dp->id) of DP descriptor table with DP
>>>> controller_id.
>>> Please no. This reverts the intention of commit bb3de286d992
>>> ("drm/msm/dp: Support up to 3 DP controllers")
>>>
>>>       A new enum is introduced to document the connection between the
>>>       instances referenced in the dpu_intf_cfg array and the controllers in
>>>       the DP driver and sc7180 is updated.
>>>
>>> It sounds like the intent of that commit failed to make a strong enough
>>> connection. Now it needs to match the INTF number as well? I can't
>>> really figure out what is actually wrong, because this patch undoes that
>>> intentional tight coupling. Is the next patch the important part that
>>> flips the order of the two interfaces?
>> The commit bb3de286d992have two problems,
>>
>> 1)  The below sc7280_dp_cfg will not work, if eDP use
>> MSM_DP_CONTROLLER_2 instead of  MSM_DP_CONTROLLER_1
> Why would we use three indices for an soc that only has two indices
> possible? This is not a real problem?

I do not what will happen at future, it may have more dp controller use 
late.

at current soc, below table has only one eDP will not work either.

static const struct msm_dp_config sc7280_dp_cfg = {
          .descs = (const struct msm_dp_desc[]) {
                  [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000,
.connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },

          .num_descs = 1,
};

>
>> since it have num_descs =2 but eDP is at index 2 (CONTROLLER_2) which
>> never be reached.
>>
>> static const struct msm_dp_config sc7280_dp_cfg = {
>>           .descs = (const struct msm_dp_desc[]) {
>>                   [MSM_DP_CONTROLLER_2] = { .io_start = 0x0aea0000,
>> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>>                   [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
>>           },
>>           .num_descs = 2,
>> };
>>
>> 2)  DP always has index of 0 (dp->id = 0) and the first one to call
>> msm_dp_modeset_init(). This make DP always place at head of bridge chain.
> What does this mean? Are you talking about the list of bridges in drm
> core, i.e. 'bridge_list'?
yes,
>
>> At next patch eDP must be placed at head of bridge chain to fix eDP
>> corruption issue. This is the purpose of this patch. I will revise the
>> commit text.
>>
> Wouldn't that be "broken" again if we decided to change drm_bridge_add()
> to add to the list head instead of list tail? Or if somehow
> msm_dp_modeset_init() was called in a different order so that the DP
> bridge was added before the eDP bridge?

we have no control of drm_bridge_add().

Since drm perform screen update following bridge chain sequentially, we 
have to make sure primary always update first.

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Stephen Boyd]

Quoting Kuogee Hsieh (2022-06-24 14:49:57)
>
> On 6/24/2022 2:40 PM, Stephen Boyd wrote:
> > Quoting Kuogee Hsieh (2022-06-24 14:17:50)
> >> On 6/24/2022 1:00 PM, Stephen Boyd wrote:
> >>> Quoting Kuogee Hsieh (2022-06-24 10:15:11)
> >>>> Current the index (dp->id) of DP descriptor table (scxxxx_dp_cfg[]) are tightly
> >>>> coupled with DP controller_id. This means DP use controller id 0 must be placed
> >>>> at first entry of DP descriptor table (scxxxx_dp_cfg[]). Otherwise the internal
> >>>> INTF will mismatch controller_id. This will cause controller kickoff wrong
> >>>> interface timing engine and cause dpu_encoder_phys_vid_wait_for_commit_done
> >>>> vblank timeout error.
> >>>>
> >>>> This patch add controller_id field into struct msm_dp_desc to break the tightly
> >>>> coupled relationship between index (dp->id) of DP descriptor table with DP
> >>>> controller_id.
> >>> Please no. This reverts the intention of commit bb3de286d992
> >>> ("drm/msm/dp: Support up to 3 DP controllers")
> >>>
> >>>       A new enum is introduced to document the connection between the
> >>>       instances referenced in the dpu_intf_cfg array and the controllers in
> >>>       the DP driver and sc7180 is updated.
> >>>
> >>> It sounds like the intent of that commit failed to make a strong enough
> >>> connection. Now it needs to match the INTF number as well? I can't
> >>> really figure out what is actually wrong, because this patch undoes that
> >>> intentional tight coupling. Is the next patch the important part that
> >>> flips the order of the two interfaces?
> >> The commit bb3de286d992have two problems,
> >>
> >> 1)  The below sc7280_dp_cfg will not work, if eDP use
> >> MSM_DP_CONTROLLER_2 instead of  MSM_DP_CONTROLLER_1
> > Why would we use three indices for an soc that only has two indices
> > possible? This is not a real problem?
>
> I do not what will happen at future, it may have more dp controller use
> late.
>
> at current soc, below table has only one eDP will not work either.
>
> static const struct msm_dp_config sc7280_dp_cfg = {
>           .descs = (const struct msm_dp_desc[]) {
>                   [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000,
> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>
>           .num_descs = 1,

So the MSM_DP_CONTROLLER_* number needs to match what exactly?

>
> >
> >> since it have num_descs =2 but eDP is at index 2 (CONTROLLER_2) which
> >> never be reached.
> >>
> >> static const struct msm_dp_config sc7280_dp_cfg = {
> >>           .descs = (const struct msm_dp_desc[]) {
> >>                   [MSM_DP_CONTROLLER_2] = { .io_start = 0x0aea0000,
> >> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
> >>                   [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
> >> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
> >>           },
> >>           .num_descs = 2,
> >> };
> >>
> >> 2)  DP always has index of 0 (dp->id = 0) and the first one to call
> >> msm_dp_modeset_init(). This make DP always place at head of bridge chain.
> > What does this mean? Are you talking about the list of bridges in drm
> > core, i.e. 'bridge_list'?
> yes,

I changed the drm_bridge_add() API and that doesn't make any difference.
The corruption is still seen. That would imply it is not the order of
the list of bridges.

---8<---
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index e275b4ca344b..e3518101b65e 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -165,7 +165,7 @@ void drm_bridge_add(struct drm_bridge *bridge)
 	mutex_init(&bridge->hpd_mutex);

 	mutex_lock(&bridge_lock);
-	list_add_tail(&bridge->list, &bridge_list);
+	list_add(&bridge->list, &bridge_list);
 	mutex_unlock(&bridge_lock);
 }
 EXPORT_SYMBOL(drm_bridge_add);

> >
> >> At next patch eDP must be placed at head of bridge chain to fix eDP
> >> corruption issue. This is the purpose of this patch. I will revise the
> >> commit text.
> >>
> > Wouldn't that be "broken" again if we decided to change drm_bridge_add()
> > to add to the list head instead of list tail? Or if somehow
> > msm_dp_modeset_init() was called in a different order so that the DP
> > bridge was added before the eDP bridge?
>
> we have no control of drm_bridge_add().
>
> Since drm perform screen update following bridge chain sequentially, we
> have to make sure primary always update first.
>

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Stephen Boyd]

Quoting Kuogee Hsieh (2022-06-24 14:49:57)
>
> On 6/24/2022 2:40 PM, Stephen Boyd wrote:
> > Quoting Kuogee Hsieh (2022-06-24 14:17:50)
> >> On 6/24/2022 1:00 PM, Stephen Boyd wrote:
> >>> Quoting Kuogee Hsieh (2022-06-24 10:15:11)
> >>>> Current the index (dp->id) of DP descriptor table (scxxxx_dp_cfg[]) are tightly
> >>>> coupled with DP controller_id. This means DP use controller id 0 must be placed
> >>>> at first entry of DP descriptor table (scxxxx_dp_cfg[]). Otherwise the internal
> >>>> INTF will mismatch controller_id. This will cause controller kickoff wrong
> >>>> interface timing engine and cause dpu_encoder_phys_vid_wait_for_commit_done
> >>>> vblank timeout error.
> >>>>
> >>>> This patch add controller_id field into struct msm_dp_desc to break the tightly
> >>>> coupled relationship between index (dp->id) of DP descriptor table with DP
> >>>> controller_id.
> >>> Please no. This reverts the intention of commit bb3de286d992
> >>> ("drm/msm/dp: Support up to 3 DP controllers")
> >>>
> >>>       A new enum is introduced to document the connection between the
> >>>       instances referenced in the dpu_intf_cfg array and the controllers in
> >>>       the DP driver and sc7180 is updated.
> >>>
> >>> It sounds like the intent of that commit failed to make a strong enough
> >>> connection. Now it needs to match the INTF number as well? I can't
> >>> really figure out what is actually wrong, because this patch undoes that
> >>> intentional tight coupling. Is the next patch the important part that
> >>> flips the order of the two interfaces?
> >> The commit bb3de286d992have two problems,
> >>
> >> 1)  The below sc7280_dp_cfg will not work, if eDP use
> >> MSM_DP_CONTROLLER_2 instead of  MSM_DP_CONTROLLER_1
> > Why would we use three indices for an soc that only has two indices
> > possible? This is not a real problem?
>
> I do not what will happen at future, it may have more dp controller use
> late.
>
> at current soc, below table has only one eDP will not work either.
>
> static const struct msm_dp_config sc7280_dp_cfg = {
>           .descs = (const struct msm_dp_desc[]) {
>                   [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000,
> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>
>           .num_descs = 1,

So the MSM_DP_CONTROLLER_* number needs to match what exactly?

>
> >
> >> since it have num_descs =2 but eDP is at index 2 (CONTROLLER_2) which
> >> never be reached.
> >>
> >> static const struct msm_dp_config sc7280_dp_cfg = {
> >>           .descs = (const struct msm_dp_desc[]) {
> >>                   [MSM_DP_CONTROLLER_2] = { .io_start = 0x0aea0000,
> >> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
> >>                   [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
> >> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
> >>           },
> >>           .num_descs = 2,
> >> };
> >>
> >> 2)  DP always has index of 0 (dp->id = 0) and the first one to call
> >> msm_dp_modeset_init(). This make DP always place at head of bridge chain.
> > What does this mean? Are you talking about the list of bridges in drm
> > core, i.e. 'bridge_list'?
> yes,

I changed the drm_bridge_add() API and that doesn't make any difference.
The corruption is still seen. That would imply it is not the order of
the list of bridges.

---8<---
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index e275b4ca344b..e3518101b65e 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -165,7 +165,7 @@ void drm_bridge_add(struct drm_bridge *bridge)
 	mutex_init(&bridge->hpd_mutex);

 	mutex_lock(&bridge_lock);
-	list_add_tail(&bridge->list, &bridge_list);
+	list_add(&bridge->list, &bridge_list);
 	mutex_unlock(&bridge_lock);
 }
 EXPORT_SYMBOL(drm_bridge_add);

> >
> >> At next patch eDP must be placed at head of bridge chain to fix eDP
> >> corruption issue. This is the purpose of this patch. I will revise the
> >> commit text.
> >>
> > Wouldn't that be "broken" again if we decided to change drm_bridge_add()
> > to add to the list head instead of list tail? Or if somehow
> > msm_dp_modeset_init() was called in a different order so that the DP
> > bridge was added before the eDP bridge?
>
> we have no control of drm_bridge_add().
>
> Since drm perform screen update following bridge chain sequentially, we
> have to make sure primary always update first.
>

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Kuogee Hsieh]

On 6/24/2022 3:19 PM, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2022-06-24 14:49:57)
>> On 6/24/2022 2:40 PM, Stephen Boyd wrote:
>>> Quoting Kuogee Hsieh (2022-06-24 14:17:50)
>>>> On 6/24/2022 1:00 PM, Stephen Boyd wrote:
>>>>> Quoting Kuogee Hsieh (2022-06-24 10:15:11)
>>>>>> Current the index (dp->id) of DP descriptor table (scxxxx_dp_cfg[]) are tightly
>>>>>> coupled with DP controller_id. This means DP use controller id 0 must be placed
>>>>>> at first entry of DP descriptor table (scxxxx_dp_cfg[]). Otherwise the internal
>>>>>> INTF will mismatch controller_id. This will cause controller kickoff wrong
>>>>>> interface timing engine and cause dpu_encoder_phys_vid_wait_for_commit_done
>>>>>> vblank timeout error.
>>>>>>
>>>>>> This patch add controller_id field into struct msm_dp_desc to break the tightly
>>>>>> coupled relationship between index (dp->id) of DP descriptor table with DP
>>>>>> controller_id.
>>>>> Please no. This reverts the intention of commit bb3de286d992
>>>>> ("drm/msm/dp: Support up to 3 DP controllers")
>>>>>
>>>>>        A new enum is introduced to document the connection between the
>>>>>        instances referenced in the dpu_intf_cfg array and the controllers in
>>>>>        the DP driver and sc7180 is updated.
>>>>>
>>>>> It sounds like the intent of that commit failed to make a strong enough
>>>>> connection. Now it needs to match the INTF number as well? I can't
>>>>> really figure out what is actually wrong, because this patch undoes that
>>>>> intentional tight coupling. Is the next patch the important part that
>>>>> flips the order of the two interfaces?
>>>> The commit bb3de286d992have two problems,
>>>>
>>>> 1)  The below sc7280_dp_cfg will not work, if eDP use
>>>> MSM_DP_CONTROLLER_2 instead of  MSM_DP_CONTROLLER_1
>>> Why would we use three indices for an soc that only has two indices
>>> possible? This is not a real problem?
>> I do not what will happen at future, it may have more dp controller use
>> late.
>>
>> at current soc, below table has only one eDP will not work either.
>>
>> static const struct msm_dp_config sc7280_dp_cfg = {
>>            .descs = (const struct msm_dp_desc[]) {
>>                    [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000,
>> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>>
>>            .num_descs = 1,
> So the MSM_DP_CONTROLLER_* number needs to match what exactly?MSM

MSM_DP_CONTROLLER_1 need to match to the index = 1 of sc7280_dp_cfg[] <== This is correct

The problem is sc7280_dp_cfg[] have two entries since eDP place at index 
of MSM_DP_CONTROLLER_1.

but .num_desc = 1  <== this said only have one entry at sc7280_dp_cfg[] 
table. Therefore eDP will never be found at for loop  at 
_dpu_kms_initialize_displayport().


>
>>>> since it have num_descs =2 but eDP is at index 2 (CONTROLLER_2) which
>>>> never be reached.
>>>>
>>>> static const struct msm_dp_config sc7280_dp_cfg = {
>>>>            .descs = (const struct msm_dp_desc[]) {
>>>>                    [MSM_DP_CONTROLLER_2] = { .io_start = 0x0aea0000,
>>>> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>>>>                    [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
>>>>            },
>>>>            .num_descs = 2,
>>>> };
>>>>
>>>> 2)  DP always has index of 0 (dp->id = 0) and the first one to call
>>>> msm_dp_modeset_init(). This make DP always place at head of bridge chain.
>>> What does this mean? Are you talking about the list of bridges in drm
>>> core, i.e. 'bridge_list'?
>> yes,
> I changed the drm_bridge_add() API and that doesn't make any difference.
> The corruption is still seen. That would imply it is not the order of
> the list of bridges.

Sorry, my mistake. it is not in drm_bridge_add.

It should be in dpu_encoder_init() of _dpu_kms_initialize_displayport().

can you make below changes (patch) to _dpu_kms_initialize_displayport().

kuogee: go backward for dp modeset_init

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 3a4da0d..b271a4b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -611,9 +611,15 @@ static int _dpu_kms_initialize_displayport(struct 
drm_device *dev,
         struct drm_encoder *encoder = NULL;
         struct msm_display_info info;
         int rc;
-       int i;
+       int i,num;
+
+       num = ARRAY_SIZE(priv->dp);

+#ifdef XXXX
         for (i = 0; i < ARRAY_SIZE(priv->dp); i++) {
+#else
+       for (i = num - 1; i >= 0 ; i--) {
+#endif
                 if (!priv->dp[i])
                         continue;


>
> ---8<---
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index e275b4ca344b..e3518101b65e 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -165,7 +165,7 @@ void drm_bridge_add(struct drm_bridge *bridge)
>   	mutex_init(&bridge->hpd_mutex);
>
>   	mutex_lock(&bridge_lock);
> -	list_add_tail(&bridge->list, &bridge_list);
> +	list_add(&bridge->list, &bridge_list);
>   	mutex_unlock(&bridge_lock);
>   }
>   EXPORT_SYMBOL(drm_bridge_add);
>
>>>> At next patch eDP must be placed at head of bridge chain to fix eDP
>>>> corruption issue. This is the purpose of this patch. I will revise the
>>>> commit text.
>>>>
>>> Wouldn't that be "broken" again if we decided to change drm_bridge_add()
>>> to add to the list head instead of list tail? Or if somehow
>>> msm_dp_modeset_init() was called in a different order so that the DP
>>> bridge was added before the eDP bridge?
>> we have no control of drm_bridge_add().
>>
>> Since drm perform screen update following bridge chain sequentially, we
>> have to make sure primary always update first.
>>

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Kuogee Hsieh]

On 6/24/2022 3:19 PM, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2022-06-24 14:49:57)
>> On 6/24/2022 2:40 PM, Stephen Boyd wrote:
>>> Quoting Kuogee Hsieh (2022-06-24 14:17:50)
>>>> On 6/24/2022 1:00 PM, Stephen Boyd wrote:
>>>>> Quoting Kuogee Hsieh (2022-06-24 10:15:11)
>>>>>> Current the index (dp->id) of DP descriptor table (scxxxx_dp_cfg[]) are tightly
>>>>>> coupled with DP controller_id. This means DP use controller id 0 must be placed
>>>>>> at first entry of DP descriptor table (scxxxx_dp_cfg[]). Otherwise the internal
>>>>>> INTF will mismatch controller_id. This will cause controller kickoff wrong
>>>>>> interface timing engine and cause dpu_encoder_phys_vid_wait_for_commit_done
>>>>>> vblank timeout error.
>>>>>>
>>>>>> This patch add controller_id field into struct msm_dp_desc to break the tightly
>>>>>> coupled relationship between index (dp->id) of DP descriptor table with DP
>>>>>> controller_id.
>>>>> Please no. This reverts the intention of commit bb3de286d992
>>>>> ("drm/msm/dp: Support up to 3 DP controllers")
>>>>>
>>>>>        A new enum is introduced to document the connection between the
>>>>>        instances referenced in the dpu_intf_cfg array and the controllers in
>>>>>        the DP driver and sc7180 is updated.
>>>>>
>>>>> It sounds like the intent of that commit failed to make a strong enough
>>>>> connection. Now it needs to match the INTF number as well? I can't
>>>>> really figure out what is actually wrong, because this patch undoes that
>>>>> intentional tight coupling. Is the next patch the important part that
>>>>> flips the order of the two interfaces?
>>>> The commit bb3de286d992have two problems,
>>>>
>>>> 1)  The below sc7280_dp_cfg will not work, if eDP use
>>>> MSM_DP_CONTROLLER_2 instead of  MSM_DP_CONTROLLER_1
>>> Why would we use three indices for an soc that only has two indices
>>> possible? This is not a real problem?
>> I do not what will happen at future, it may have more dp controller use
>> late.
>>
>> at current soc, below table has only one eDP will not work either.
>>
>> static const struct msm_dp_config sc7280_dp_cfg = {
>>            .descs = (const struct msm_dp_desc[]) {
>>                    [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000,
>> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>>
>>            .num_descs = 1,
> So the MSM_DP_CONTROLLER_* number needs to match what exactly?MSM

MSM_DP_CONTROLLER_1 need to match to the index = 1 of sc7280_dp_cfg[] <== This is correct

The problem is sc7280_dp_cfg[] have two entries since eDP place at index 
of MSM_DP_CONTROLLER_1.

but .num_desc = 1  <== this said only have one entry at sc7280_dp_cfg[] 
table. Therefore eDP will never be found at for loop  at 
_dpu_kms_initialize_displayport().


>
>>>> since it have num_descs =2 but eDP is at index 2 (CONTROLLER_2) which
>>>> never be reached.
>>>>
>>>> static const struct msm_dp_config sc7280_dp_cfg = {
>>>>            .descs = (const struct msm_dp_desc[]) {
>>>>                    [MSM_DP_CONTROLLER_2] = { .io_start = 0x0aea0000,
>>>> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>>>>                    [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
>>>>            },
>>>>            .num_descs = 2,
>>>> };
>>>>
>>>> 2)  DP always has index of 0 (dp->id = 0) and the first one to call
>>>> msm_dp_modeset_init(). This make DP always place at head of bridge chain.
>>> What does this mean? Are you talking about the list of bridges in drm
>>> core, i.e. 'bridge_list'?
>> yes,
> I changed the drm_bridge_add() API and that doesn't make any difference.
> The corruption is still seen. That would imply it is not the order of
> the list of bridges.

Sorry, my mistake. it is not in drm_bridge_add.

It should be in dpu_encoder_init() of _dpu_kms_initialize_displayport().

can you make below changes (patch) to _dpu_kms_initialize_displayport().

kuogee: go backward for dp modeset_init

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 3a4da0d..b271a4b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -611,9 +611,15 @@ static int _dpu_kms_initialize_displayport(struct 
drm_device *dev,
         struct drm_encoder *encoder = NULL;
         struct msm_display_info info;
         int rc;
-       int i;
+       int i,num;
+
+       num = ARRAY_SIZE(priv->dp);

+#ifdef XXXX
         for (i = 0; i < ARRAY_SIZE(priv->dp); i++) {
+#else
+       for (i = num - 1; i >= 0 ; i--) {
+#endif
                 if (!priv->dp[i])
                         continue;


>
> ---8<---
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index e275b4ca344b..e3518101b65e 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -165,7 +165,7 @@ void drm_bridge_add(struct drm_bridge *bridge)
>   	mutex_init(&bridge->hpd_mutex);
>
>   	mutex_lock(&bridge_lock);
> -	list_add_tail(&bridge->list, &bridge_list);
> +	list_add(&bridge->list, &bridge_list);
>   	mutex_unlock(&bridge_lock);
>   }
>   EXPORT_SYMBOL(drm_bridge_add);
>
>>>> At next patch eDP must be placed at head of bridge chain to fix eDP
>>>> corruption issue. This is the purpose of this patch. I will revise the
>>>> commit text.
>>>>
>>> Wouldn't that be "broken" again if we decided to change drm_bridge_add()
>>> to add to the list head instead of list tail? Or if somehow
>>> msm_dp_modeset_init() was called in a different order so that the DP
>>> bridge was added before the eDP bridge?
>> we have no control of drm_bridge_add().
>>
>> Since drm perform screen update following bridge chain sequentially, we
>> have to make sure primary always update first.
>>

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Stephen Boyd]

Quoting Kuogee Hsieh (2022-06-24 15:53:45)
>
> MSM_DP_CONTROLLER_1 need to match to the index = 1 of sc7280_dp_cfg[] <== This is correct
>
> The problem is sc7280_dp_cfg[] have two entries since eDP place at index
> of MSM_DP_CONTROLLER_1.
>
> but .num_desc = 1  <== this said only have one entry at sc7280_dp_cfg[]
> table. Therefore eDP will never be found at for loop  at
> _dpu_kms_initialize_displayport().
>

Yes, but what else does the MSM_DP_CONTROLLER_1 need to match? Because
the intention of the previous commit was to make it so the order of
sc7280_dp_cfg couldn't be messed up and not match the
MSM_DP_CONTROLLER_1 value that lives in sc7280_intf[].

>
> Sorry, my mistake. it is not in drm_bridge_add.
>
> It should be in dpu_encoder_init() of _dpu_kms_initialize_displayport().
>
> can you make below changes (patch) to _dpu_kms_initialize_displayport().
>

Yes, I've made that change to try to understand the problem. I still
don't understand, sadly. Does flipping the order of iteration through
'priv->dp' somehow mean that the crtc that is assigned to the eDP
connector is left unchanged? Whereas without registering the eDP encoder
first means we have to change the crtc for the eDP encoder and that
can't be done atomically?

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Stephen Boyd]

Quoting Kuogee Hsieh (2022-06-24 15:53:45)
>
> MSM_DP_CONTROLLER_1 need to match to the index = 1 of sc7280_dp_cfg[] <== This is correct
>
> The problem is sc7280_dp_cfg[] have two entries since eDP place at index
> of MSM_DP_CONTROLLER_1.
>
> but .num_desc = 1  <== this said only have one entry at sc7280_dp_cfg[]
> table. Therefore eDP will never be found at for loop  at
> _dpu_kms_initialize_displayport().
>

Yes, but what else does the MSM_DP_CONTROLLER_1 need to match? Because
the intention of the previous commit was to make it so the order of
sc7280_dp_cfg couldn't be messed up and not match the
MSM_DP_CONTROLLER_1 value that lives in sc7280_intf[].

>
> Sorry, my mistake. it is not in drm_bridge_add.
>
> It should be in dpu_encoder_init() of _dpu_kms_initialize_displayport().
>
> can you make below changes (patch) to _dpu_kms_initialize_displayport().
>

Yes, I've made that change to try to understand the problem. I still
don't understand, sadly. Does flipping the order of iteration through
'priv->dp' somehow mean that the crtc that is assigned to the eDP
connector is left unchanged? Whereas without registering the eDP encoder
first means we have to change the crtc for the eDP encoder and that
can't be done atomically?

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Kuogee Hsieh]

On 6/24/2022 4:12 PM, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2022-06-24 15:53:45)
>> MSM_DP_CONTROLLER_1 need to match to the index = 1 of sc7280_dp_cfg[] <== This is correct
>>
>> The problem is sc7280_dp_cfg[] have two entries since eDP place at index
>> of MSM_DP_CONTROLLER_1.
>>
>> but .num_desc = 1  <== this said only have one entry at sc7280_dp_cfg[]
>> table. Therefore eDP will never be found at for loop  at
>> _dpu_kms_initialize_displayport().
>>
> Yes, but what else does the MSM_DP_CONTROLLER_1 need to match? Because
> the intention of the previous commit was to make it so the order of
> sc7280_dp_cfg couldn't be messed up and not match the
> MSM_DP_CONTROLLER_1 value that lives in sc7280_intf[].


at  _dpu_kms_initialize_displayport()

> -		info.h_tile_instance[0] = i; <== assign i to become dp controller id, "i" is index of scxxxx_dp_cfg[]

This what I mean MSM_DP_CONTROLLER_1 need to match to index = 1 of 
scxxxx_dp_cfg[].

it it is not match, then MSM_DP_CONTROLLER_1 with match to different INTF.


>
>> Sorry, my mistake. it is not in drm_bridge_add.
>>
>> It should be in dpu_encoder_init() of _dpu_kms_initialize_displayport().
>>
>> can you make below changes (patch) to _dpu_kms_initialize_displayport().
>>
> Yes, I've made that change to try to understand the problem. I still
> don't understand, sadly. Does flipping the order of iteration through
> 'priv->dp' somehow mean that the crtc that is assigned to the eDP
> connector is left unchanged? Whereas without registering the eDP encoder
> first means we have to change the crtc for the eDP encoder and that
> can't be done atomically?

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Kuogee Hsieh]

On 6/24/2022 4:12 PM, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2022-06-24 15:53:45)
>> MSM_DP_CONTROLLER_1 need to match to the index = 1 of sc7280_dp_cfg[] <== This is correct
>>
>> The problem is sc7280_dp_cfg[] have two entries since eDP place at index
>> of MSM_DP_CONTROLLER_1.
>>
>> but .num_desc = 1  <== this said only have one entry at sc7280_dp_cfg[]
>> table. Therefore eDP will never be found at for loop  at
>> _dpu_kms_initialize_displayport().
>>
> Yes, but what else does the MSM_DP_CONTROLLER_1 need to match? Because
> the intention of the previous commit was to make it so the order of
> sc7280_dp_cfg couldn't be messed up and not match the
> MSM_DP_CONTROLLER_1 value that lives in sc7280_intf[].


at  _dpu_kms_initialize_displayport()

> -		info.h_tile_instance[0] = i; <== assign i to become dp controller id, "i" is index of scxxxx_dp_cfg[]

This what I mean MSM_DP_CONTROLLER_1 need to match to index = 1 of 
scxxxx_dp_cfg[].

it it is not match, then MSM_DP_CONTROLLER_1 with match to different INTF.


>
>> Sorry, my mistake. it is not in drm_bridge_add.
>>
>> It should be in dpu_encoder_init() of _dpu_kms_initialize_displayport().
>>
>> can you make below changes (patch) to _dpu_kms_initialize_displayport().
>>
> Yes, I've made that change to try to understand the problem. I still
> don't understand, sadly. Does flipping the order of iteration through
> 'priv->dp' somehow mean that the crtc that is assigned to the eDP
> connector is left unchanged? Whereas without registering the eDP encoder
> first means we have to change the crtc for the eDP encoder and that
> can't be done atomically?

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Stephen Boyd]

Quoting Kuogee Hsieh (2022-06-24 16:30:59)
>
> On 6/24/2022 4:12 PM, Stephen Boyd wrote:
> > Quoting Kuogee Hsieh (2022-06-24 15:53:45)
> >> MSM_DP_CONTROLLER_1 need to match to the index = 1 of sc7280_dp_cfg[] <== This is correct
> >>
> >> The problem is sc7280_dp_cfg[] have two entries since eDP place at index
> >> of MSM_DP_CONTROLLER_1.
> >>
> >> but .num_desc = 1  <== this said only have one entry at sc7280_dp_cfg[]
> >> table. Therefore eDP will never be found at for loop  at
> >> _dpu_kms_initialize_displayport().
> >>
> > Yes, but what else does the MSM_DP_CONTROLLER_1 need to match? Because
> > the intention of the previous commit was to make it so the order of
> > sc7280_dp_cfg couldn't be messed up and not match the
> > MSM_DP_CONTROLLER_1 value that lives in sc7280_intf[].
>
>
> at  _dpu_kms_initialize_displayport()
>
> > -             info.h_tile_instance[0] = i; <== assign i to become dp controller id, "i" is index of scxxxx_dp_cfg[]
>
> This what I mean MSM_DP_CONTROLLER_1 need to match to index = 1 of
> scxxxx_dp_cfg[].
>
> it it is not match, then MSM_DP_CONTROLLER_1 with match to different INTF.

I thought we matched the INTF instance by searching through
sc7280_intf[] for a matching MSM_DP_CONTROLLER_1 and then returning that
INTF number. See dpu_encoder_get_intf() and the caller.

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Stephen Boyd]

Quoting Kuogee Hsieh (2022-06-24 16:30:59)
>
> On 6/24/2022 4:12 PM, Stephen Boyd wrote:
> > Quoting Kuogee Hsieh (2022-06-24 15:53:45)
> >> MSM_DP_CONTROLLER_1 need to match to the index = 1 of sc7280_dp_cfg[] <== This is correct
> >>
> >> The problem is sc7280_dp_cfg[] have two entries since eDP place at index
> >> of MSM_DP_CONTROLLER_1.
> >>
> >> but .num_desc = 1  <== this said only have one entry at sc7280_dp_cfg[]
> >> table. Therefore eDP will never be found at for loop  at
> >> _dpu_kms_initialize_displayport().
> >>
> > Yes, but what else does the MSM_DP_CONTROLLER_1 need to match? Because
> > the intention of the previous commit was to make it so the order of
> > sc7280_dp_cfg couldn't be messed up and not match the
> > MSM_DP_CONTROLLER_1 value that lives in sc7280_intf[].
>
>
> at  _dpu_kms_initialize_displayport()
>
> > -             info.h_tile_instance[0] = i; <== assign i to become dp controller id, "i" is index of scxxxx_dp_cfg[]
>
> This what I mean MSM_DP_CONTROLLER_1 need to match to index = 1 of
> scxxxx_dp_cfg[].
>
> it it is not match, then MSM_DP_CONTROLLER_1 with match to different INTF.

I thought we matched the INTF instance by searching through
sc7280_intf[] for a matching MSM_DP_CONTROLLER_1 and then returning that
INTF number. See dpu_encoder_get_intf() and the caller.

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Dmitry Baryshkov]

On Sat, 25 Jun 2022 at 02:45, Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Kuogee Hsieh (2022-06-24 16:30:59)
> >
> > On 6/24/2022 4:12 PM, Stephen Boyd wrote:
> > > Quoting Kuogee Hsieh (2022-06-24 15:53:45)
> > >> MSM_DP_CONTROLLER_1 need to match to the index = 1 of sc7280_dp_cfg[] <== This is correct
> > >>
> > >> The problem is sc7280_dp_cfg[] have two entries since eDP place at index
> > >> of MSM_DP_CONTROLLER_1.
> > >>
> > >> but .num_desc = 1  <== this said only have one entry at sc7280_dp_cfg[]
> > >> table. Therefore eDP will never be found at for loop  at
> > >> _dpu_kms_initialize_displayport().
> > >>
> > > Yes, but what else does the MSM_DP_CONTROLLER_1 need to match? Because
> > > the intention of the previous commit was to make it so the order of
> > > sc7280_dp_cfg couldn't be messed up and not match the
> > > MSM_DP_CONTROLLER_1 value that lives in sc7280_intf[].
> >
> >
> > at  _dpu_kms_initialize_displayport()
> >
> > > -             info.h_tile_instance[0] = i; <== assign i to become dp controller id, "i" is index of scxxxx_dp_cfg[]
> >
> > This what I mean MSM_DP_CONTROLLER_1 need to match to index = 1 of
> > scxxxx_dp_cfg[].
> >
> > it it is not match, then MSM_DP_CONTROLLER_1 with match to different INTF.
>
> I thought we matched the INTF instance by searching through
> sc7280_intf[] for a matching MSM_DP_CONTROLLER_1 and then returning that
> INTF number. See dpu_encoder_get_intf() and the caller.

You both are correct here. We are searching through the _intf[] array
for the corresponding controller_id. And yes, the controller_ids are
being passed through the h_tile_instance[] array.

-- 
With best wishes
Dmitry

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Dmitry Baryshkov]

On Sat, 25 Jun 2022 at 02:45, Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Kuogee Hsieh (2022-06-24 16:30:59)
> >
> > On 6/24/2022 4:12 PM, Stephen Boyd wrote:
> > > Quoting Kuogee Hsieh (2022-06-24 15:53:45)
> > >> MSM_DP_CONTROLLER_1 need to match to the index = 1 of sc7280_dp_cfg[] <== This is correct
> > >>
> > >> The problem is sc7280_dp_cfg[] have two entries since eDP place at index
> > >> of MSM_DP_CONTROLLER_1.
> > >>
> > >> but .num_desc = 1  <== this said only have one entry at sc7280_dp_cfg[]
> > >> table. Therefore eDP will never be found at for loop  at
> > >> _dpu_kms_initialize_displayport().
> > >>
> > > Yes, but what else does the MSM_DP_CONTROLLER_1 need to match? Because
> > > the intention of the previous commit was to make it so the order of
> > > sc7280_dp_cfg couldn't be messed up and not match the
> > > MSM_DP_CONTROLLER_1 value that lives in sc7280_intf[].
> >
> >
> > at  _dpu_kms_initialize_displayport()
> >
> > > -             info.h_tile_instance[0] = i; <== assign i to become dp controller id, "i" is index of scxxxx_dp_cfg[]
> >
> > This what I mean MSM_DP_CONTROLLER_1 need to match to index = 1 of
> > scxxxx_dp_cfg[].
> >
> > it it is not match, then MSM_DP_CONTROLLER_1 with match to different INTF.
>
> I thought we matched the INTF instance by searching through
> sc7280_intf[] for a matching MSM_DP_CONTROLLER_1 and then returning that
> INTF number. See dpu_encoder_get_intf() and the caller.

You both are correct here. We are searching through the _intf[] array
for the corresponding controller_id. And yes, the controller_ids are
being passed through the h_tile_instance[] array.

-- 
With best wishes
Dmitry

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Kuogee Hsieh]

On 6/24/2022 4:45 PM, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2022-06-24 16:30:59)
>> On 6/24/2022 4:12 PM, Stephen Boyd wrote:
>>> Quoting Kuogee Hsieh (2022-06-24 15:53:45)
>>>> MSM_DP_CONTROLLER_1 need to match to the index = 1 of sc7280_dp_cfg[] <== This is correct
>>>>
>>>> The problem is sc7280_dp_cfg[] have two entries since eDP place at index
>>>> of MSM_DP_CONTROLLER_1.
>>>>
>>>> but .num_desc = 1  <== this said only have one entry at sc7280_dp_cfg[]
>>>> table. Therefore eDP will never be found at for loop  at
>>>> _dpu_kms_initialize_displayport().
>>>>
>>> Yes, but what else does the MSM_DP_CONTROLLER_1 need to match? Because
>>> the intention of the previous commit was to make it so the order of
>>> sc7280_dp_cfg couldn't be messed up and not match the
>>> MSM_DP_CONTROLLER_1 value that lives in sc7280_intf[].
>>
>> at  _dpu_kms_initialize_displayport()
>>
>>> -             info.h_tile_instance[0] = i; <== assign i to become dp controller id, "i" is index of scxxxx_dp_cfg[]
>> This what I mean MSM_DP_CONTROLLER_1 need to match to index = 1 of
>> scxxxx_dp_cfg[].
>>
>> it it is not match, then MSM_DP_CONTROLLER_1 with match to different INTF.
> I thought we matched the INTF instance by searching through
> sc7280_intf[] for a matching MSM_DP_CONTROLLER_1 and then returning that
> INTF number. See dpu_encoder_get_intf() and the caller.

yes, but the controller_id had been over written by dp->id.

u32 controller_id = disp_info->h_tile_instance[i];


See below code.


>          for (i = 0; i < disp_info->num_of_h_tiles && !ret; i++) {
>                  /*
>                   * Left-most tile is at index 0, content is controller id
>                   * h_tile_instance_ids[2] = {0, 1}; DSI0 = left, DSI1 = right
>                   * h_tile_instance_ids[2] = {1, 0}; DSI1 = left, DSI0 = right
>                   */
>                  u32 controller_id = disp_info->h_tile_instance[i];   <== kuogee assign dp->id to controller_id
>
>                  if (disp_info->num_of_h_tiles > 1) {
>                          if (i == 0)
>                                  phys_params.split_role = ENC_ROLE_MASTER;
>                          else
>                                  phys_params.split_role = ENC_ROLE_SLAVE;
>                  } else {
>                          phys_params.split_role = ENC_ROLE_SOLO;
>                  }
>
>                  DPU_DEBUG("h_tile_instance %d = %d, split_role %d\n",
>                                  i, controller_id, phys_params.split_role);
>
>                  phys_params.intf_idx = dpu_encoder_get_intf(dpu_kms->catalog,
>
>                intf_type,
>
>                controller_id);

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Kuogee Hsieh]

On 6/24/2022 4:45 PM, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2022-06-24 16:30:59)
>> On 6/24/2022 4:12 PM, Stephen Boyd wrote:
>>> Quoting Kuogee Hsieh (2022-06-24 15:53:45)
>>>> MSM_DP_CONTROLLER_1 need to match to the index = 1 of sc7280_dp_cfg[] <== This is correct
>>>>
>>>> The problem is sc7280_dp_cfg[] have two entries since eDP place at index
>>>> of MSM_DP_CONTROLLER_1.
>>>>
>>>> but .num_desc = 1  <== this said only have one entry at sc7280_dp_cfg[]
>>>> table. Therefore eDP will never be found at for loop  at
>>>> _dpu_kms_initialize_displayport().
>>>>
>>> Yes, but what else does the MSM_DP_CONTROLLER_1 need to match? Because
>>> the intention of the previous commit was to make it so the order of
>>> sc7280_dp_cfg couldn't be messed up and not match the
>>> MSM_DP_CONTROLLER_1 value that lives in sc7280_intf[].
>>
>> at  _dpu_kms_initialize_displayport()
>>
>>> -             info.h_tile_instance[0] = i; <== assign i to become dp controller id, "i" is index of scxxxx_dp_cfg[]
>> This what I mean MSM_DP_CONTROLLER_1 need to match to index = 1 of
>> scxxxx_dp_cfg[].
>>
>> it it is not match, then MSM_DP_CONTROLLER_1 with match to different INTF.
> I thought we matched the INTF instance by searching through
> sc7280_intf[] for a matching MSM_DP_CONTROLLER_1 and then returning that
> INTF number. See dpu_encoder_get_intf() and the caller.

yes, but the controller_id had been over written by dp->id.

u32 controller_id = disp_info->h_tile_instance[i];


See below code.


>          for (i = 0; i < disp_info->num_of_h_tiles && !ret; i++) {
>                  /*
>                   * Left-most tile is at index 0, content is controller id
>                   * h_tile_instance_ids[2] = {0, 1}; DSI0 = left, DSI1 = right
>                   * h_tile_instance_ids[2] = {1, 0}; DSI1 = left, DSI0 = right
>                   */
>                  u32 controller_id = disp_info->h_tile_instance[i];   <== kuogee assign dp->id to controller_id
>
>                  if (disp_info->num_of_h_tiles > 1) {
>                          if (i == 0)
>                                  phys_params.split_role = ENC_ROLE_MASTER;
>                          else
>                                  phys_params.split_role = ENC_ROLE_SLAVE;
>                  } else {
>                          phys_params.split_role = ENC_ROLE_SOLO;
>                  }
>
>                  DPU_DEBUG("h_tile_instance %d = %d, split_role %d\n",
>                                  i, controller_id, phys_params.split_role);
>
>                  phys_params.intf_idx = dpu_encoder_get_intf(dpu_kms->catalog,
>
>                intf_type,
>
>                controller_id);

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Abhinav Kumar]

Hi Stephen / Dmitry

Let me try to explain the issue kuogee is trying to fix below:

On 6/24/2022 4:56 PM, Kuogee Hsieh wrote:
> 
> On 6/24/2022 4:45 PM, Stephen Boyd wrote:
>> Quoting Kuogee Hsieh (2022-06-24 16:30:59)
>>> On 6/24/2022 4:12 PM, Stephen Boyd wrote:
>>>> Quoting Kuogee Hsieh (2022-06-24 15:53:45)
>>>>> MSM_DP_CONTROLLER_1 need to match to the index = 1 of 
>>>>> sc7280_dp_cfg[] <== This is correct
>>>>>
>>>>> The problem is sc7280_dp_cfg[] have two entries since eDP place at 
>>>>> index
>>>>> of MSM_DP_CONTROLLER_1.
>>>>>
>>>>> but .num_desc = 1  <== this said only have one entry at 
>>>>> sc7280_dp_cfg[]
>>>>> table. Therefore eDP will never be found at for loop  at
>>>>> _dpu_kms_initialize_displayport().
>>>>>
>>>> Yes, but what else does the MSM_DP_CONTROLLER_1 need to match? Because
>>>> the intention of the previous commit was to make it so the order of
>>>> sc7280_dp_cfg couldn't be messed up and not match the
>>>> MSM_DP_CONTROLLER_1 value that lives in sc7280_intf[].
>>>
>>> at  _dpu_kms_initialize_displayport()
>>>
>>>> -             info.h_tile_instance[0] = i; <== assign i to become dp 
>>>> controller id, "i" is index of scxxxx_dp_cfg[]
>>> This what I mean MSM_DP_CONTROLLER_1 need to match to index = 1 of
>>> scxxxx_dp_cfg[].
>>>
>>> it it is not match, then MSM_DP_CONTROLLER_1 with match to different 
>>> INTF.
>> I thought we matched the INTF instance by searching through
>> sc7280_intf[] for a matching MSM_DP_CONTROLLER_1 and then returning that
>> INTF number. See dpu_encoder_get_intf() and the caller.
> 
> yes, but the controller_id had been over written by dp->id.
> 
> u32 controller_id = disp_info->h_tile_instance[i];
> 
> 
> See below code.
> 
> 
>>          for (i = 0; i < disp_info->num_of_h_tiles && !ret; i++) {
>>                  /*
>>                   * Left-most tile is at index 0, content is 
>> controller id
>>                   * h_tile_instance_ids[2] = {0, 1}; DSI0 = left, DSI1 
>> = right
>>                   * h_tile_instance_ids[2] = {1, 0}; DSI1 = left, DSI0 
>> = right
>>                   */
>>                  u32 controller_id = disp_info->h_tile_instance[i];   
>> <== kuogee assign dp->id to controller_id
>>
>>                  if (disp_info->num_of_h_tiles > 1) {
>>                          if (i == 0)
>>                                  phys_params.split_role = 
>> ENC_ROLE_MASTER;
>>                          else
>>                                  phys_params.split_role = ENC_ROLE_SLAVE;
>>                  } else {
>>                          phys_params.split_role = ENC_ROLE_SOLO;
>>                  }
>>
>>                  DPU_DEBUG("h_tile_instance %d = %d, split_role %d\n",
>>                                  i, controller_id, 
>> phys_params.split_role);
>>
>>                  phys_params.intf_idx = 
>> dpu_encoder_get_intf(dpu_kms->catalog,
>>
>>                intf_type,
>>
>>                controller_id);


So let me try to explain this as this is what i understood from the 
patch and how kuogee explained me.

The ordering of the array still matters here and thats what he is trying 
to address with the second change.

So as per him, he tried to swap the order of entries like below and that 
did not work and that is incorrect behavior because he still retained 
the MSM_DP_CONTROLLER_x field for the table like below:

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c
index dcd80c8a794c..7816e82452ca 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -140,8 +140,8 @@ static const struct msm_dp_config sc7180_dp_cfg = {

  static const struct msm_dp_config sc7280_dp_cfg = {
         .descs = (const struct msm_dp_desc[]) {
-               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, 
.connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
                 [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000, 
.connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
+               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, 
.connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
         },
         .num_descs = 2,
  };


The reason order is important is because  in this function below, even 
though it matches the address to find which one to use it loops through 
the array and so the value of *id will change depending on which one is 
located where.

static const struct msm_dp_desc *dp_display_get_desc(struct 
platform_device *pdev,
                              unsigned int *id)
{
     const struct msm_dp_config *cfg = of_device_get_match_data(&pdev->dev);
     struct resource *res;
     int i;

     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
     if (!res)
         return NULL;

     for (i = 0; i < cfg->num_descs; i++) {
         if (cfg->descs[i].io_start == res->start) {
             *id = i;
             return &cfg->descs[i];
         }
     }

In dp_display_bind(), dp->id is used as the index of assigning the 
dp_display,

priv->dp[dp->id] = &dp->dp_display;

And now in _dpu_kms_initialize_displayport(), in the array this will 
decide the value of info.h_tile_instance[0] which will be assigned to 
just the index i.

info.h_tile_instance[0] is then used as the controller id to find from 
the catalog table.

So if this order is not retained it does not work.

Thats the issue he is trying to address to make the order of entries 
irrelevant in the table in dp_display.c

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Abhinav Kumar]

Hi Stephen / Dmitry

Let me try to explain the issue kuogee is trying to fix below:

On 6/24/2022 4:56 PM, Kuogee Hsieh wrote:
> 
> On 6/24/2022 4:45 PM, Stephen Boyd wrote:
>> Quoting Kuogee Hsieh (2022-06-24 16:30:59)
>>> On 6/24/2022 4:12 PM, Stephen Boyd wrote:
>>>> Quoting Kuogee Hsieh (2022-06-24 15:53:45)
>>>>> MSM_DP_CONTROLLER_1 need to match to the index = 1 of 
>>>>> sc7280_dp_cfg[] <== This is correct
>>>>>
>>>>> The problem is sc7280_dp_cfg[] have two entries since eDP place at 
>>>>> index
>>>>> of MSM_DP_CONTROLLER_1.
>>>>>
>>>>> but .num_desc = 1  <== this said only have one entry at 
>>>>> sc7280_dp_cfg[]
>>>>> table. Therefore eDP will never be found at for loop  at
>>>>> _dpu_kms_initialize_displayport().
>>>>>
>>>> Yes, but what else does the MSM_DP_CONTROLLER_1 need to match? Because
>>>> the intention of the previous commit was to make it so the order of
>>>> sc7280_dp_cfg couldn't be messed up and not match the
>>>> MSM_DP_CONTROLLER_1 value that lives in sc7280_intf[].
>>>
>>> at  _dpu_kms_initialize_displayport()
>>>
>>>> -             info.h_tile_instance[0] = i; <== assign i to become dp 
>>>> controller id, "i" is index of scxxxx_dp_cfg[]
>>> This what I mean MSM_DP_CONTROLLER_1 need to match to index = 1 of
>>> scxxxx_dp_cfg[].
>>>
>>> it it is not match, then MSM_DP_CONTROLLER_1 with match to different 
>>> INTF.
>> I thought we matched the INTF instance by searching through
>> sc7280_intf[] for a matching MSM_DP_CONTROLLER_1 and then returning that
>> INTF number. See dpu_encoder_get_intf() and the caller.
> 
> yes, but the controller_id had been over written by dp->id.
> 
> u32 controller_id = disp_info->h_tile_instance[i];
> 
> 
> See below code.
> 
> 
>>          for (i = 0; i < disp_info->num_of_h_tiles && !ret; i++) {
>>                  /*
>>                   * Left-most tile is at index 0, content is 
>> controller id
>>                   * h_tile_instance_ids[2] = {0, 1}; DSI0 = left, DSI1 
>> = right
>>                   * h_tile_instance_ids[2] = {1, 0}; DSI1 = left, DSI0 
>> = right
>>                   */
>>                  u32 controller_id = disp_info->h_tile_instance[i];   
>> <== kuogee assign dp->id to controller_id
>>
>>                  if (disp_info->num_of_h_tiles > 1) {
>>                          if (i == 0)
>>                                  phys_params.split_role = 
>> ENC_ROLE_MASTER;
>>                          else
>>                                  phys_params.split_role = ENC_ROLE_SLAVE;
>>                  } else {
>>                          phys_params.split_role = ENC_ROLE_SOLO;
>>                  }
>>
>>                  DPU_DEBUG("h_tile_instance %d = %d, split_role %d\n",
>>                                  i, controller_id, 
>> phys_params.split_role);
>>
>>                  phys_params.intf_idx = 
>> dpu_encoder_get_intf(dpu_kms->catalog,
>>
>>                intf_type,
>>
>>                controller_id);


So let me try to explain this as this is what i understood from the 
patch and how kuogee explained me.

The ordering of the array still matters here and thats what he is trying 
to address with the second change.

So as per him, he tried to swap the order of entries like below and that 
did not work and that is incorrect behavior because he still retained 
the MSM_DP_CONTROLLER_x field for the table like below:

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c
index dcd80c8a794c..7816e82452ca 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -140,8 +140,8 @@ static const struct msm_dp_config sc7180_dp_cfg = {

  static const struct msm_dp_config sc7280_dp_cfg = {
         .descs = (const struct msm_dp_desc[]) {
-               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, 
.connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
                 [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000, 
.connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
+               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, 
.connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
         },
         .num_descs = 2,
  };


The reason order is important is because  in this function below, even 
though it matches the address to find which one to use it loops through 
the array and so the value of *id will change depending on which one is 
located where.

static const struct msm_dp_desc *dp_display_get_desc(struct 
platform_device *pdev,
                              unsigned int *id)
{
     const struct msm_dp_config *cfg = of_device_get_match_data(&pdev->dev);
     struct resource *res;
     int i;

     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
     if (!res)
         return NULL;

     for (i = 0; i < cfg->num_descs; i++) {
         if (cfg->descs[i].io_start == res->start) {
             *id = i;
             return &cfg->descs[i];
         }
     }

In dp_display_bind(), dp->id is used as the index of assigning the 
dp_display,

priv->dp[dp->id] = &dp->dp_display;

And now in _dpu_kms_initialize_displayport(), in the array this will 
decide the value of info.h_tile_instance[0] which will be assigned to 
just the index i.

info.h_tile_instance[0] is then used as the controller id to find from 
the catalog table.

So if this order is not retained it does not work.

Thats the issue he is trying to address to make the order of entries 
irrelevant in the table in dp_display.c

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Stephen Boyd]

Quoting Abhinav Kumar (2022-06-24 17:03:37)
> Hi Stephen / Dmitry
>
> Let me try to explain the issue kuogee is trying to fix below:
>
> On 6/24/2022 4:56 PM, Kuogee Hsieh wrote:
> >
> > On 6/24/2022 4:45 PM, Stephen Boyd wrote:
> >> Quoting Kuogee Hsieh (2022-06-24 16:30:59)
> >>> On 6/24/2022 4:12 PM, Stephen Boyd wrote:
> >>>> Quoting Kuogee Hsieh (2022-06-24 15:53:45)
> >>>>> MSM_DP_CONTROLLER_1 need to match to the index = 1 of
> >>>>> sc7280_dp_cfg[] <== This is correct
> >>>>>
> >>>>> The problem is sc7280_dp_cfg[] have two entries since eDP place at
> >>>>> index
> >>>>> of MSM_DP_CONTROLLER_1.
> >>>>>
> >>>>> but .num_desc = 1  <== this said only have one entry at
> >>>>> sc7280_dp_cfg[]
> >>>>> table. Therefore eDP will never be found at for loop  at
> >>>>> _dpu_kms_initialize_displayport().
> >>>>>
> >>>> Yes, but what else does the MSM_DP_CONTROLLER_1 need to match? Because
> >>>> the intention of the previous commit was to make it so the order of
> >>>> sc7280_dp_cfg couldn't be messed up and not match the
> >>>> MSM_DP_CONTROLLER_1 value that lives in sc7280_intf[].
> >>>
> >>> at  _dpu_kms_initialize_displayport()
> >>>
> >>>> -             info.h_tile_instance[0] = i; <== assign i to become dp
> >>>> controller id, "i" is index of scxxxx_dp_cfg[]
> >>> This what I mean MSM_DP_CONTROLLER_1 need to match to index = 1 of
> >>> scxxxx_dp_cfg[].
> >>>
> >>> it it is not match, then MSM_DP_CONTROLLER_1 with match to different
> >>> INTF.
> >> I thought we matched the INTF instance by searching through
> >> sc7280_intf[] for a matching MSM_DP_CONTROLLER_1 and then returning that
> >> INTF number. See dpu_encoder_get_intf() and the caller.
> >
> > yes, but the controller_id had been over written by dp->id.
> >
> > u32 controller_id = disp_info->h_tile_instance[i];
> >
> >
> > See below code.
> >
> >
> >>          for (i = 0; i < disp_info->num_of_h_tiles && !ret; i++) {
> >>                  /*
> >>                   * Left-most tile is at index 0, content is
> >> controller id
> >>                   * h_tile_instance_ids[2] = {0, 1}; DSI0 = left, DSI1
> >> = right
> >>                   * h_tile_instance_ids[2] = {1, 0}; DSI1 = left, DSI0
> >> = right
> >>                   */
> >>                  u32 controller_id = disp_info->h_tile_instance[i];
> >> <== kuogee assign dp->id to controller_id
> >>
> >>                  if (disp_info->num_of_h_tiles > 1) {
> >>                          if (i == 0)
> >>                                  phys_params.split_role =
> >> ENC_ROLE_MASTER;
> >>                          else
> >>                                  phys_params.split_role = ENC_ROLE_SLAVE;
> >>                  } else {
> >>                          phys_params.split_role = ENC_ROLE_SOLO;
> >>                  }
> >>
> >>                  DPU_DEBUG("h_tile_instance %d = %d, split_role %d\n",
> >>                                  i, controller_id,
> >> phys_params.split_role);
> >>
> >>                  phys_params.intf_idx =
> >> dpu_encoder_get_intf(dpu_kms->catalog,
> >>
> >>                intf_type,
> >>
> >>                controller_id);
>
>
> So let me try to explain this as this is what i understood from the
> patch and how kuogee explained me.
>
> The ordering of the array still matters here and thats what he is trying
> to address with the second change.

The order of the array should not matter. That's the problem.

>
> So as per him, he tried to swap the order of entries like below and that
> did not work and that is incorrect behavior because he still retained
> the MSM_DP_CONTROLLER_x field for the table like below:
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index dcd80c8a794c..7816e82452ca 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -140,8 +140,8 @@ static const struct msm_dp_config sc7180_dp_cfg = {
>
>   static const struct msm_dp_config sc7280_dp_cfg = {
>          .descs = (const struct msm_dp_desc[]) {
> -               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
>                  [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000,
> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
> +               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
>          },
>          .num_descs = 2,
>   };
>
>
> The reason order is important is because  in this function below, even
> though it matches the address to find which one to use it loops through
> the array and so the value of *id will change depending on which one is
> located where.
>
> static const struct msm_dp_desc *dp_display_get_desc(struct
> platform_device *pdev,
>                               unsigned int *id)

Thanks! We should fix this function to not overwrite the id.

> {
>      const struct msm_dp_config *cfg = of_device_get_match_data(&pdev->dev);
>      struct resource *res;
>      int i;
>
>      res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>      if (!res)
>          return NULL;
>
>      for (i = 0; i < cfg->num_descs; i++) {
>          if (cfg->descs[i].io_start == res->start) {
>              *id = i;
>              return &cfg->descs[i];
>          }
>      }
>
> In dp_display_bind(), dp->id is used as the index of assigning the
> dp_display,
>
> priv->dp[dp->id] = &dp->dp_display;
>
> And now in _dpu_kms_initialize_displayport(), in the array this will
> decide the value of info.h_tile_instance[0] which will be assigned to
> just the index i.
>
> info.h_tile_instance[0] is then used as the controller id to find from
> the catalog table.
>
> So if this order is not retained it does not work.
>
> Thats the issue he is trying to address to make the order of entries
> irrelevant in the table in dp_display.c

The code seems to be brittle. This sort of explanation would have been
helpful to have in the commit text.

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Stephen Boyd]

Quoting Abhinav Kumar (2022-06-24 17:03:37)
> Hi Stephen / Dmitry
>
> Let me try to explain the issue kuogee is trying to fix below:
>
> On 6/24/2022 4:56 PM, Kuogee Hsieh wrote:
> >
> > On 6/24/2022 4:45 PM, Stephen Boyd wrote:
> >> Quoting Kuogee Hsieh (2022-06-24 16:30:59)
> >>> On 6/24/2022 4:12 PM, Stephen Boyd wrote:
> >>>> Quoting Kuogee Hsieh (2022-06-24 15:53:45)
> >>>>> MSM_DP_CONTROLLER_1 need to match to the index = 1 of
> >>>>> sc7280_dp_cfg[] <== This is correct
> >>>>>
> >>>>> The problem is sc7280_dp_cfg[] have two entries since eDP place at
> >>>>> index
> >>>>> of MSM_DP_CONTROLLER_1.
> >>>>>
> >>>>> but .num_desc = 1  <== this said only have one entry at
> >>>>> sc7280_dp_cfg[]
> >>>>> table. Therefore eDP will never be found at for loop  at
> >>>>> _dpu_kms_initialize_displayport().
> >>>>>
> >>>> Yes, but what else does the MSM_DP_CONTROLLER_1 need to match? Because
> >>>> the intention of the previous commit was to make it so the order of
> >>>> sc7280_dp_cfg couldn't be messed up and not match the
> >>>> MSM_DP_CONTROLLER_1 value that lives in sc7280_intf[].
> >>>
> >>> at  _dpu_kms_initialize_displayport()
> >>>
> >>>> -             info.h_tile_instance[0] = i; <== assign i to become dp
> >>>> controller id, "i" is index of scxxxx_dp_cfg[]
> >>> This what I mean MSM_DP_CONTROLLER_1 need to match to index = 1 of
> >>> scxxxx_dp_cfg[].
> >>>
> >>> it it is not match, then MSM_DP_CONTROLLER_1 with match to different
> >>> INTF.
> >> I thought we matched the INTF instance by searching through
> >> sc7280_intf[] for a matching MSM_DP_CONTROLLER_1 and then returning that
> >> INTF number. See dpu_encoder_get_intf() and the caller.
> >
> > yes, but the controller_id had been over written by dp->id.
> >
> > u32 controller_id = disp_info->h_tile_instance[i];
> >
> >
> > See below code.
> >
> >
> >>          for (i = 0; i < disp_info->num_of_h_tiles && !ret; i++) {
> >>                  /*
> >>                   * Left-most tile is at index 0, content is
> >> controller id
> >>                   * h_tile_instance_ids[2] = {0, 1}; DSI0 = left, DSI1
> >> = right
> >>                   * h_tile_instance_ids[2] = {1, 0}; DSI1 = left, DSI0
> >> = right
> >>                   */
> >>                  u32 controller_id = disp_info->h_tile_instance[i];
> >> <== kuogee assign dp->id to controller_id
> >>
> >>                  if (disp_info->num_of_h_tiles > 1) {
> >>                          if (i == 0)
> >>                                  phys_params.split_role =
> >> ENC_ROLE_MASTER;
> >>                          else
> >>                                  phys_params.split_role = ENC_ROLE_SLAVE;
> >>                  } else {
> >>                          phys_params.split_role = ENC_ROLE_SOLO;
> >>                  }
> >>
> >>                  DPU_DEBUG("h_tile_instance %d = %d, split_role %d\n",
> >>                                  i, controller_id,
> >> phys_params.split_role);
> >>
> >>                  phys_params.intf_idx =
> >> dpu_encoder_get_intf(dpu_kms->catalog,
> >>
> >>                intf_type,
> >>
> >>                controller_id);
>
>
> So let me try to explain this as this is what i understood from the
> patch and how kuogee explained me.
>
> The ordering of the array still matters here and thats what he is trying
> to address with the second change.

The order of the array should not matter. That's the problem.

>
> So as per him, he tried to swap the order of entries like below and that
> did not work and that is incorrect behavior because he still retained
> the MSM_DP_CONTROLLER_x field for the table like below:
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index dcd80c8a794c..7816e82452ca 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -140,8 +140,8 @@ static const struct msm_dp_config sc7180_dp_cfg = {
>
>   static const struct msm_dp_config sc7280_dp_cfg = {
>          .descs = (const struct msm_dp_desc[]) {
> -               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
>                  [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000,
> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
> +               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
>          },
>          .num_descs = 2,
>   };
>
>
> The reason order is important is because  in this function below, even
> though it matches the address to find which one to use it loops through
> the array and so the value of *id will change depending on which one is
> located where.
>
> static const struct msm_dp_desc *dp_display_get_desc(struct
> platform_device *pdev,
>                               unsigned int *id)

Thanks! We should fix this function to not overwrite the id.

> {
>      const struct msm_dp_config *cfg = of_device_get_match_data(&pdev->dev);
>      struct resource *res;
>      int i;
>
>      res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>      if (!res)
>          return NULL;
>
>      for (i = 0; i < cfg->num_descs; i++) {
>          if (cfg->descs[i].io_start == res->start) {
>              *id = i;
>              return &cfg->descs[i];
>          }
>      }
>
> In dp_display_bind(), dp->id is used as the index of assigning the
> dp_display,
>
> priv->dp[dp->id] = &dp->dp_display;
>
> And now in _dpu_kms_initialize_displayport(), in the array this will
> decide the value of info.h_tile_instance[0] which will be assigned to
> just the index i.
>
> info.h_tile_instance[0] is then used as the controller id to find from
> the catalog table.
>
> So if this order is not retained it does not work.
>
> Thats the issue he is trying to address to make the order of entries
> irrelevant in the table in dp_display.c

The code seems to be brittle. This sort of explanation would have been
helpful to have in the commit text.

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Stephen Boyd]

Quoting Stephen Boyd (2022-06-24 17:11:01)
> Quoting Abhinav Kumar (2022-06-24 17:03:37)
> >
> > So let me try to explain this as this is what i understood from the
> > patch and how kuogee explained me.
> >
> > The ordering of the array still matters here and thats what he is trying
> > to address with the second change.
>
> The order of the array should not matter. That's the problem.

It seems like somewhere else the order of the array matters, presumably
while setting up encoders?

>
> >
> > So as per him, he tried to swap the order of entries like below and that
> > did not work and that is incorrect behavior because he still retained
> > the MSM_DP_CONTROLLER_x field for the table like below:
> >
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> > b/drivers/gpu/drm/msm/dp/dp_display.c
> > index dcd80c8a794c..7816e82452ca 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > @@ -140,8 +140,8 @@ static const struct msm_dp_config sc7180_dp_cfg = {
> >
> >   static const struct msm_dp_config sc7280_dp_cfg = {
> >          .descs = (const struct msm_dp_desc[]) {
> > -               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
> > .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
> >                  [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000,
> > .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
> > +               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
> > .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
> >          },
> >          .num_descs = 2,
> >   };
> >
> >
> > The reason order is important is because  in this function below, even
> > though it matches the address to find which one to use it loops through
> > the array and so the value of *id will change depending on which one is
> > located where.
> >
> > static const struct msm_dp_desc *dp_display_get_desc(struct
> > platform_device *pdev,
> >                               unsigned int *id)
>
> Thanks! We should fix this function to not overwrite the id.
>

Ah nevermind. I mixed up dp->id and h_tile_instance thinking one was
overwriting the other but that doesn't make any sense.

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Stephen Boyd]

Quoting Stephen Boyd (2022-06-24 17:11:01)
> Quoting Abhinav Kumar (2022-06-24 17:03:37)
> >
> > So let me try to explain this as this is what i understood from the
> > patch and how kuogee explained me.
> >
> > The ordering of the array still matters here and thats what he is trying
> > to address with the second change.
>
> The order of the array should not matter. That's the problem.

It seems like somewhere else the order of the array matters, presumably
while setting up encoders?

>
> >
> > So as per him, he tried to swap the order of entries like below and that
> > did not work and that is incorrect behavior because he still retained
> > the MSM_DP_CONTROLLER_x field for the table like below:
> >
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> > b/drivers/gpu/drm/msm/dp/dp_display.c
> > index dcd80c8a794c..7816e82452ca 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > @@ -140,8 +140,8 @@ static const struct msm_dp_config sc7180_dp_cfg = {
> >
> >   static const struct msm_dp_config sc7280_dp_cfg = {
> >          .descs = (const struct msm_dp_desc[]) {
> > -               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
> > .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
> >                  [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000,
> > .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
> > +               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
> > .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
> >          },
> >          .num_descs = 2,
> >   };
> >
> >
> > The reason order is important is because  in this function below, even
> > though it matches the address to find which one to use it loops through
> > the array and so the value of *id will change depending on which one is
> > located where.
> >
> > static const struct msm_dp_desc *dp_display_get_desc(struct
> > platform_device *pdev,
> >                               unsigned int *id)
>
> Thanks! We should fix this function to not overwrite the id.
>

Ah nevermind. I mixed up dp->id and h_tile_instance thinking one was
overwriting the other but that doesn't make any sense.

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Abhinav Kumar]

On 6/24/2022 5:23 PM, Stephen Boyd wrote:
> Quoting Stephen Boyd (2022-06-24 17:11:01)
>> Quoting Abhinav Kumar (2022-06-24 17:03:37)
>>>
>>> So let me try to explain this as this is what i understood from the
>>> patch and how kuogee explained me.
>>>
>>> The ordering of the array still matters here and thats what he is trying
>>> to address with the second change.
>>
>> The order of the array should not matter. That's the problem.
> 
> It seems like somewhere else the order of the array matters, presumably
> while setting up encoders?
> 
>>
>>>
>>> So as per him, he tried to swap the order of entries like below and that
>>> did not work and that is incorrect behavior because he still retained
>>> the MSM_DP_CONTROLLER_x field for the table like below:
>>>
>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>>> b/drivers/gpu/drm/msm/dp/dp_display.c
>>> index dcd80c8a794c..7816e82452ca 100644
>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>> @@ -140,8 +140,8 @@ static const struct msm_dp_config sc7180_dp_cfg = {
>>>
>>>    static const struct msm_dp_config sc7280_dp_cfg = {
>>>           .descs = (const struct msm_dp_desc[]) {
>>> -               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
>>>                   [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000,
>>> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>>> +               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
>>>           },
>>>           .num_descs = 2,
>>>    };
>>>
>>>
>>> The reason order is important is because  in this function below, even
>>> though it matches the address to find which one to use it loops through
>>> the array and so the value of *id will change depending on which one is
>>> located where.
>>>
>>> static const struct msm_dp_desc *dp_display_get_desc(struct
>>> platform_device *pdev,
>>>                                unsigned int *id)
>>
>> Thanks! We should fix this function to not overwrite the id.
>>
> 
> Ah nevermind. I mixed up dp->id and h_tile_instance thinking one was
> overwriting the other but that doesn't make any sense.

Yes, I also misunderstood one point.

Even if we re-order like above, we are still retaining the index 
correctly so that will still work.

I checked with kuogee again now, he mentioned he needs this only for 
patch 3.

He is not sure of the root-cause of why turning ON the first display 
fixes the issue . I think that needs to be debugged correctly to answers 
questions posted by you / Dmitry. Lets hold on these patches till we 
have the answers.

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Abhinav Kumar]

On 6/24/2022 5:23 PM, Stephen Boyd wrote:
> Quoting Stephen Boyd (2022-06-24 17:11:01)
>> Quoting Abhinav Kumar (2022-06-24 17:03:37)
>>>
>>> So let me try to explain this as this is what i understood from the
>>> patch and how kuogee explained me.
>>>
>>> The ordering of the array still matters here and thats what he is trying
>>> to address with the second change.
>>
>> The order of the array should not matter. That's the problem.
> 
> It seems like somewhere else the order of the array matters, presumably
> while setting up encoders?
> 
>>
>>>
>>> So as per him, he tried to swap the order of entries like below and that
>>> did not work and that is incorrect behavior because he still retained
>>> the MSM_DP_CONTROLLER_x field for the table like below:
>>>
>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>>> b/drivers/gpu/drm/msm/dp/dp_display.c
>>> index dcd80c8a794c..7816e82452ca 100644
>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>> @@ -140,8 +140,8 @@ static const struct msm_dp_config sc7180_dp_cfg = {
>>>
>>>    static const struct msm_dp_config sc7280_dp_cfg = {
>>>           .descs = (const struct msm_dp_desc[]) {
>>> -               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
>>>                   [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000,
>>> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>>> +               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
>>>           },
>>>           .num_descs = 2,
>>>    };
>>>
>>>
>>> The reason order is important is because  in this function below, even
>>> though it matches the address to find which one to use it loops through
>>> the array and so the value of *id will change depending on which one is
>>> located where.
>>>
>>> static const struct msm_dp_desc *dp_display_get_desc(struct
>>> platform_device *pdev,
>>>                                unsigned int *id)
>>
>> Thanks! We should fix this function to not overwrite the id.
>>
> 
> Ah nevermind. I mixed up dp->id and h_tile_instance thinking one was
> overwriting the other but that doesn't make any sense.

Yes, I also misunderstood one point.

Even if we re-order like above, we are still retaining the index 
correctly so that will still work.

I checked with kuogee again now, he mentioned he needs this only for 
patch 3.

He is not sure of the root-cause of why turning ON the first display 
fixes the issue . I think that needs to be debugged correctly to answers 
questions posted by you / Dmitry. Lets hold on these patches till we 
have the answers.

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Dmitry Baryshkov]

On Sat, 25 Jun 2022 at 03:03, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
> Hi Stephen / Dmitry
>
> Let me try to explain the issue kuogee is trying to fix below:
>
> On 6/24/2022 4:56 PM, Kuogee Hsieh wrote:
> >
> > On 6/24/2022 4:45 PM, Stephen Boyd wrote:
> >> Quoting Kuogee Hsieh (2022-06-24 16:30:59)
> >>> On 6/24/2022 4:12 PM, Stephen Boyd wrote:
> >>>> Quoting Kuogee Hsieh (2022-06-24 15:53:45)
> >>>>> MSM_DP_CONTROLLER_1 need to match to the index = 1 of
> >>>>> sc7280_dp_cfg[] <== This is correct
> >>>>>
> >>>>> The problem is sc7280_dp_cfg[] have two entries since eDP place at
> >>>>> index
> >>>>> of MSM_DP_CONTROLLER_1.
> >>>>>
> >>>>> but .num_desc = 1  <== this said only have one entry at
> >>>>> sc7280_dp_cfg[]
> >>>>> table. Therefore eDP will never be found at for loop  at
> >>>>> _dpu_kms_initialize_displayport().
> >>>>>
> >>>> Yes, but what else does the MSM_DP_CONTROLLER_1 need to match? Because
> >>>> the intention of the previous commit was to make it so the order of
> >>>> sc7280_dp_cfg couldn't be messed up and not match the
> >>>> MSM_DP_CONTROLLER_1 value that lives in sc7280_intf[].
> >>>
> >>> at  _dpu_kms_initialize_displayport()
> >>>
> >>>> -             info.h_tile_instance[0] = i; <== assign i to become dp
> >>>> controller id, "i" is index of scxxxx_dp_cfg[]
> >>> This what I mean MSM_DP_CONTROLLER_1 need to match to index = 1 of
> >>> scxxxx_dp_cfg[].
> >>>
> >>> it it is not match, then MSM_DP_CONTROLLER_1 with match to different
> >>> INTF.
> >> I thought we matched the INTF instance by searching through
> >> sc7280_intf[] for a matching MSM_DP_CONTROLLER_1 and then returning that
> >> INTF number. See dpu_encoder_get_intf() and the caller.
> >
> > yes, but the controller_id had been over written by dp->id.
> >
> > u32 controller_id = disp_info->h_tile_instance[i];
> >
> >
> > See below code.
> >
> >
> >>          for (i = 0; i < disp_info->num_of_h_tiles && !ret; i++) {
> >>                  /*
> >>                   * Left-most tile is at index 0, content is
> >> controller id
> >>                   * h_tile_instance_ids[2] = {0, 1}; DSI0 = left, DSI1
> >> = right
> >>                   * h_tile_instance_ids[2] = {1, 0}; DSI1 = left, DSI0
> >> = right
> >>                   */
> >>                  u32 controller_id = disp_info->h_tile_instance[i];
> >> <== kuogee assign dp->id to controller_id
> >>
> >>                  if (disp_info->num_of_h_tiles > 1) {
> >>                          if (i == 0)
> >>                                  phys_params.split_role =
> >> ENC_ROLE_MASTER;
> >>                          else
> >>                                  phys_params.split_role = ENC_ROLE_SLAVE;
> >>                  } else {
> >>                          phys_params.split_role = ENC_ROLE_SOLO;
> >>                  }
> >>
> >>                  DPU_DEBUG("h_tile_instance %d = %d, split_role %d\n",
> >>                                  i, controller_id,
> >> phys_params.split_role);
> >>
> >>                  phys_params.intf_idx =
> >> dpu_encoder_get_intf(dpu_kms->catalog,
> >>
> >>                intf_type,
> >>
> >>                controller_id);
>
>
> So let me try to explain this as this is what i understood from the
> patch and how kuogee explained me.
>
> The ordering of the array still matters here and thats what he is trying
> to address with the second change.
>
> So as per him, he tried to swap the order of entries like below and that
> did not work and that is incorrect behavior because he still retained
> the MSM_DP_CONTROLLER_x field for the table like below:

I'd like to understand why did he try to change the order in the first place.

>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index dcd80c8a794c..7816e82452ca 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -140,8 +140,8 @@ static const struct msm_dp_config sc7180_dp_cfg = {
>
>   static const struct msm_dp_config sc7280_dp_cfg = {
>          .descs = (const struct msm_dp_desc[]) {
> -               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
>                  [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000,
> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
> +               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
>          },
>          .num_descs = 2,
>   };
>
>
> The reason order is important is because  in this function below, even
> though it matches the address to find which one to use it loops through
> the array and so the value of *id will change depending on which one is
> located where.
>
> static const struct msm_dp_desc *dp_display_get_desc(struct
> platform_device *pdev,
>                               unsigned int *id)
> {
>      const struct msm_dp_config *cfg = of_device_get_match_data(&pdev->dev);
>      struct resource *res;
>      int i;
>
>      res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>      if (!res)
>          return NULL;
>
>      for (i = 0; i < cfg->num_descs; i++) {
>          if (cfg->descs[i].io_start == res->start) {
>              *id = i;

The id is set to the index of the corresponding DP instance in the
descs array, which is MSM_DP_CONTROLLER_n. Correct up to now.

>              return &cfg->descs[i];
>          }
>      }
>
> In dp_display_bind(), dp->id is used as the index of assigning the
> dp_display,
>
> priv->dp[dp->id] = &dp->dp_display;

dp->id earlier is set to the id returned by dp_display_get_desc.
So the priv->dp is now indexed by MSM_DP_CONTROLLER_n. Again, correct.

>
> And now in _dpu_kms_initialize_displayport(), in the array this will
> decide the value of info.h_tile_instance[0] which will be assigned to
> just the index i.

i is iterated over priv->dp indices (MSM_DP_CONTROLLER_n, see above),
which means that that h_tile_instance[0] is now set to the
MSM_DP_CONTROLLER_n. Still correct.

> info.h_tile_instance[0] is then used as the controller id to find from
> the catalog table.

This sounds good.

> So if this order is not retained it does not work.
>
> Thats the issue he is trying to address to make the order of entries
> irrelevant in the table in dp_display.c



-- 
With best wishes
Dmitry

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Dmitry Baryshkov]

On Sat, 25 Jun 2022 at 03:03, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
> Hi Stephen / Dmitry
>
> Let me try to explain the issue kuogee is trying to fix below:
>
> On 6/24/2022 4:56 PM, Kuogee Hsieh wrote:
> >
> > On 6/24/2022 4:45 PM, Stephen Boyd wrote:
> >> Quoting Kuogee Hsieh (2022-06-24 16:30:59)
> >>> On 6/24/2022 4:12 PM, Stephen Boyd wrote:
> >>>> Quoting Kuogee Hsieh (2022-06-24 15:53:45)
> >>>>> MSM_DP_CONTROLLER_1 need to match to the index = 1 of
> >>>>> sc7280_dp_cfg[] <== This is correct
> >>>>>
> >>>>> The problem is sc7280_dp_cfg[] have two entries since eDP place at
> >>>>> index
> >>>>> of MSM_DP_CONTROLLER_1.
> >>>>>
> >>>>> but .num_desc = 1  <== this said only have one entry at
> >>>>> sc7280_dp_cfg[]
> >>>>> table. Therefore eDP will never be found at for loop  at
> >>>>> _dpu_kms_initialize_displayport().
> >>>>>
> >>>> Yes, but what else does the MSM_DP_CONTROLLER_1 need to match? Because
> >>>> the intention of the previous commit was to make it so the order of
> >>>> sc7280_dp_cfg couldn't be messed up and not match the
> >>>> MSM_DP_CONTROLLER_1 value that lives in sc7280_intf[].
> >>>
> >>> at  _dpu_kms_initialize_displayport()
> >>>
> >>>> -             info.h_tile_instance[0] = i; <== assign i to become dp
> >>>> controller id, "i" is index of scxxxx_dp_cfg[]
> >>> This what I mean MSM_DP_CONTROLLER_1 need to match to index = 1 of
> >>> scxxxx_dp_cfg[].
> >>>
> >>> it it is not match, then MSM_DP_CONTROLLER_1 with match to different
> >>> INTF.
> >> I thought we matched the INTF instance by searching through
> >> sc7280_intf[] for a matching MSM_DP_CONTROLLER_1 and then returning that
> >> INTF number. See dpu_encoder_get_intf() and the caller.
> >
> > yes, but the controller_id had been over written by dp->id.
> >
> > u32 controller_id = disp_info->h_tile_instance[i];
> >
> >
> > See below code.
> >
> >
> >>          for (i = 0; i < disp_info->num_of_h_tiles && !ret; i++) {
> >>                  /*
> >>                   * Left-most tile is at index 0, content is
> >> controller id
> >>                   * h_tile_instance_ids[2] = {0, 1}; DSI0 = left, DSI1
> >> = right
> >>                   * h_tile_instance_ids[2] = {1, 0}; DSI1 = left, DSI0
> >> = right
> >>                   */
> >>                  u32 controller_id = disp_info->h_tile_instance[i];
> >> <== kuogee assign dp->id to controller_id
> >>
> >>                  if (disp_info->num_of_h_tiles > 1) {
> >>                          if (i == 0)
> >>                                  phys_params.split_role =
> >> ENC_ROLE_MASTER;
> >>                          else
> >>                                  phys_params.split_role = ENC_ROLE_SLAVE;
> >>                  } else {
> >>                          phys_params.split_role = ENC_ROLE_SOLO;
> >>                  }
> >>
> >>                  DPU_DEBUG("h_tile_instance %d = %d, split_role %d\n",
> >>                                  i, controller_id,
> >> phys_params.split_role);
> >>
> >>                  phys_params.intf_idx =
> >> dpu_encoder_get_intf(dpu_kms->catalog,
> >>
> >>                intf_type,
> >>
> >>                controller_id);
>
>
> So let me try to explain this as this is what i understood from the
> patch and how kuogee explained me.
>
> The ordering of the array still matters here and thats what he is trying
> to address with the second change.
>
> So as per him, he tried to swap the order of entries like below and that
> did not work and that is incorrect behavior because he still retained
> the MSM_DP_CONTROLLER_x field for the table like below:

I'd like to understand why did he try to change the order in the first place.

>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index dcd80c8a794c..7816e82452ca 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -140,8 +140,8 @@ static const struct msm_dp_config sc7180_dp_cfg = {
>
>   static const struct msm_dp_config sc7280_dp_cfg = {
>          .descs = (const struct msm_dp_desc[]) {
> -               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
>                  [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000,
> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
> +               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
>          },
>          .num_descs = 2,
>   };
>
>
> The reason order is important is because  in this function below, even
> though it matches the address to find which one to use it loops through
> the array and so the value of *id will change depending on which one is
> located where.
>
> static const struct msm_dp_desc *dp_display_get_desc(struct
> platform_device *pdev,
>                               unsigned int *id)
> {
>      const struct msm_dp_config *cfg = of_device_get_match_data(&pdev->dev);
>      struct resource *res;
>      int i;
>
>      res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>      if (!res)
>          return NULL;
>
>      for (i = 0; i < cfg->num_descs; i++) {
>          if (cfg->descs[i].io_start == res->start) {
>              *id = i;

The id is set to the index of the corresponding DP instance in the
descs array, which is MSM_DP_CONTROLLER_n. Correct up to now.

>              return &cfg->descs[i];
>          }
>      }
>
> In dp_display_bind(), dp->id is used as the index of assigning the
> dp_display,
>
> priv->dp[dp->id] = &dp->dp_display;

dp->id earlier is set to the id returned by dp_display_get_desc.
So the priv->dp is now indexed by MSM_DP_CONTROLLER_n. Again, correct.

>
> And now in _dpu_kms_initialize_displayport(), in the array this will
> decide the value of info.h_tile_instance[0] which will be assigned to
> just the index i.

i is iterated over priv->dp indices (MSM_DP_CONTROLLER_n, see above),
which means that that h_tile_instance[0] is now set to the
MSM_DP_CONTROLLER_n. Still correct.

> info.h_tile_instance[0] is then used as the controller id to find from
> the catalog table.

This sounds good.

> So if this order is not retained it does not work.
>
> Thats the issue he is trying to address to make the order of entries
> irrelevant in the table in dp_display.c



-- 
With best wishes
Dmitry

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Kuogee Hsieh]

On 6/24/2022 5:11 PM, Dmitry Baryshkov wrote:
> On Sat, 25 Jun 2022 at 03:03, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>> Hi Stephen / Dmitry
>>
>> Let me try to explain the issue kuogee is trying to fix below:
>>
>> On 6/24/2022 4:56 PM, Kuogee Hsieh wrote:
>>> On 6/24/2022 4:45 PM, Stephen Boyd wrote:
>>>> Quoting Kuogee Hsieh (2022-06-24 16:30:59)
>>>>> On 6/24/2022 4:12 PM, Stephen Boyd wrote:
>>>>>> Quoting Kuogee Hsieh (2022-06-24 15:53:45)
>>>>>>> MSM_DP_CONTROLLER_1 need to match to the index = 1 of
>>>>>>> sc7280_dp_cfg[] <== This is correct
>>>>>>>
>>>>>>> The problem is sc7280_dp_cfg[] have two entries since eDP place at
>>>>>>> index
>>>>>>> of MSM_DP_CONTROLLER_1.
>>>>>>>
>>>>>>> but .num_desc = 1  <== this said only have one entry at
>>>>>>> sc7280_dp_cfg[]
>>>>>>> table. Therefore eDP will never be found at for loop  at
>>>>>>> _dpu_kms_initialize_displayport().
>>>>>>>
>>>>>> Yes, but what else does the MSM_DP_CONTROLLER_1 need to match? Because
>>>>>> the intention of the previous commit was to make it so the order of
>>>>>> sc7280_dp_cfg couldn't be messed up and not match the
>>>>>> MSM_DP_CONTROLLER_1 value that lives in sc7280_intf[].
>>>>> at  _dpu_kms_initialize_displayport()
>>>>>
>>>>>> -             info.h_tile_instance[0] = i; <== assign i to become dp
>>>>>> controller id, "i" is index of scxxxx_dp_cfg[]
>>>>> This what I mean MSM_DP_CONTROLLER_1 need to match to index = 1 of
>>>>> scxxxx_dp_cfg[].
>>>>>
>>>>> it it is not match, then MSM_DP_CONTROLLER_1 with match to different
>>>>> INTF.
>>>> I thought we matched the INTF instance by searching through
>>>> sc7280_intf[] for a matching MSM_DP_CONTROLLER_1 and then returning that
>>>> INTF number. See dpu_encoder_get_intf() and the caller.
>>> yes, but the controller_id had been over written by dp->id.
>>>
>>> u32 controller_id = disp_info->h_tile_instance[i];
>>>
>>>
>>> See below code.
>>>
>>>
>>>>           for (i = 0; i < disp_info->num_of_h_tiles && !ret; i++) {
>>>>                   /*
>>>>                    * Left-most tile is at index 0, content is
>>>> controller id
>>>>                    * h_tile_instance_ids[2] = {0, 1}; DSI0 = left, DSI1
>>>> = right
>>>>                    * h_tile_instance_ids[2] = {1, 0}; DSI1 = left, DSI0
>>>> = right
>>>>                    */
>>>>                   u32 controller_id = disp_info->h_tile_instance[i];
>>>> <== kuogee assign dp->id to controller_id
>>>>
>>>>                   if (disp_info->num_of_h_tiles > 1) {
>>>>                           if (i == 0)
>>>>                                   phys_params.split_role =
>>>> ENC_ROLE_MASTER;
>>>>                           else
>>>>                                   phys_params.split_role = ENC_ROLE_SLAVE;
>>>>                   } else {
>>>>                           phys_params.split_role = ENC_ROLE_SOLO;
>>>>                   }
>>>>
>>>>                   DPU_DEBUG("h_tile_instance %d = %d, split_role %d\n",
>>>>                                   i, controller_id,
>>>> phys_params.split_role);
>>>>
>>>>                   phys_params.intf_idx =
>>>> dpu_encoder_get_intf(dpu_kms->catalog,
>>>>
>>>>                 intf_type,
>>>>
>>>>                 controller_id);
>>
>> So let me try to explain this as this is what i understood from the
>> patch and how kuogee explained me.
>>
>> The ordering of the array still matters here and thats what he is trying
>> to address with the second change.
>>
>> So as per him, he tried to swap the order of entries like below and that
>> did not work and that is incorrect behavior because he still retained
>> the MSM_DP_CONTROLLER_x field for the table like below:
> I'd like to understand why did he try to change the order in the first place.
>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>> b/drivers/gpu/drm/msm/dp/dp_display.c
>> index dcd80c8a794c..7816e82452ca 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -140,8 +140,8 @@ static const struct msm_dp_config sc7180_dp_cfg = {
>>
>>    static const struct msm_dp_config sc7280_dp_cfg = {
>>           .descs = (const struct msm_dp_desc[]) {
>> -               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
>>                   [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000,
>> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>> +               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
>>           },
>>           .num_descs = 2,
>>    };
>>
>>
>> The reason order is important is because  in this function below, even
>> though it matches the address to find which one to use it loops through
>> the array and so the value of *id will change depending on which one is
>> located where.
>>
>> static const struct msm_dp_desc *dp_display_get_desc(struct
>> platform_device *pdev,
>>                                unsigned int *id)
>> {
>>       const struct msm_dp_config *cfg = of_device_get_match_data(&pdev->dev);
>>       struct resource *res;
>>       int i;
>>
>>       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>       if (!res)
>>           return NULL;
>>
>>       for (i = 0; i < cfg->num_descs; i++) {
>>           if (cfg->descs[i].io_start == res->start) {
>>               *id = i;
> The id is set to the index of the corresponding DP instance in the
> descs array, which is MSM_DP_CONTROLLER_n. Correct up to now.
>
>>               return &cfg->descs[i];
>>           }
>>       }
>>
>> In dp_display_bind(), dp->id is used as the index of assigning the
>> dp_display,
>>
>> priv->dp[dp->id] = &dp->dp_display;
> dp->id earlier is set to the id returned by dp_display_get_desc.
> So the priv->dp is now indexed by MSM_DP_CONTROLLER_n. Again, correct.
>
>> And now in _dpu_kms_initialize_displayport(), in the array this will
>> decide the value of info.h_tile_instance[0] which will be assigned to
>> just the index i.
> i is iterated over priv->dp indices (MSM_DP_CONTROLLER_n, see above),
> which means that that h_tile_instance[0] is now set to the
> MSM_DP_CONTROLLER_n. Still correct.
>
>> info.h_tile_instance[0] is then used as the controller id to find from
>> the catalog table.
> This sounds good.
How can I have eDP call dpu_encoder_init() before DP calls with 
_dpu_kms_initialize_displayport()?
>> So if this order is not retained it does not work.
>>
>> Thats the issue he is trying to address to make the order of entries
>> irrelevant in the table in dp_display.c
>
>

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Kuogee Hsieh]

On 6/24/2022 5:11 PM, Dmitry Baryshkov wrote:
> On Sat, 25 Jun 2022 at 03:03, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>> Hi Stephen / Dmitry
>>
>> Let me try to explain the issue kuogee is trying to fix below:
>>
>> On 6/24/2022 4:56 PM, Kuogee Hsieh wrote:
>>> On 6/24/2022 4:45 PM, Stephen Boyd wrote:
>>>> Quoting Kuogee Hsieh (2022-06-24 16:30:59)
>>>>> On 6/24/2022 4:12 PM, Stephen Boyd wrote:
>>>>>> Quoting Kuogee Hsieh (2022-06-24 15:53:45)
>>>>>>> MSM_DP_CONTROLLER_1 need to match to the index = 1 of
>>>>>>> sc7280_dp_cfg[] <== This is correct
>>>>>>>
>>>>>>> The problem is sc7280_dp_cfg[] have two entries since eDP place at
>>>>>>> index
>>>>>>> of MSM_DP_CONTROLLER_1.
>>>>>>>
>>>>>>> but .num_desc = 1  <== this said only have one entry at
>>>>>>> sc7280_dp_cfg[]
>>>>>>> table. Therefore eDP will never be found at for loop  at
>>>>>>> _dpu_kms_initialize_displayport().
>>>>>>>
>>>>>> Yes, but what else does the MSM_DP_CONTROLLER_1 need to match? Because
>>>>>> the intention of the previous commit was to make it so the order of
>>>>>> sc7280_dp_cfg couldn't be messed up and not match the
>>>>>> MSM_DP_CONTROLLER_1 value that lives in sc7280_intf[].
>>>>> at  _dpu_kms_initialize_displayport()
>>>>>
>>>>>> -             info.h_tile_instance[0] = i; <== assign i to become dp
>>>>>> controller id, "i" is index of scxxxx_dp_cfg[]
>>>>> This what I mean MSM_DP_CONTROLLER_1 need to match to index = 1 of
>>>>> scxxxx_dp_cfg[].
>>>>>
>>>>> it it is not match, then MSM_DP_CONTROLLER_1 with match to different
>>>>> INTF.
>>>> I thought we matched the INTF instance by searching through
>>>> sc7280_intf[] for a matching MSM_DP_CONTROLLER_1 and then returning that
>>>> INTF number. See dpu_encoder_get_intf() and the caller.
>>> yes, but the controller_id had been over written by dp->id.
>>>
>>> u32 controller_id = disp_info->h_tile_instance[i];
>>>
>>>
>>> See below code.
>>>
>>>
>>>>           for (i = 0; i < disp_info->num_of_h_tiles && !ret; i++) {
>>>>                   /*
>>>>                    * Left-most tile is at index 0, content is
>>>> controller id
>>>>                    * h_tile_instance_ids[2] = {0, 1}; DSI0 = left, DSI1
>>>> = right
>>>>                    * h_tile_instance_ids[2] = {1, 0}; DSI1 = left, DSI0
>>>> = right
>>>>                    */
>>>>                   u32 controller_id = disp_info->h_tile_instance[i];
>>>> <== kuogee assign dp->id to controller_id
>>>>
>>>>                   if (disp_info->num_of_h_tiles > 1) {
>>>>                           if (i == 0)
>>>>                                   phys_params.split_role =
>>>> ENC_ROLE_MASTER;
>>>>                           else
>>>>                                   phys_params.split_role = ENC_ROLE_SLAVE;
>>>>                   } else {
>>>>                           phys_params.split_role = ENC_ROLE_SOLO;
>>>>                   }
>>>>
>>>>                   DPU_DEBUG("h_tile_instance %d = %d, split_role %d\n",
>>>>                                   i, controller_id,
>>>> phys_params.split_role);
>>>>
>>>>                   phys_params.intf_idx =
>>>> dpu_encoder_get_intf(dpu_kms->catalog,
>>>>
>>>>                 intf_type,
>>>>
>>>>                 controller_id);
>>
>> So let me try to explain this as this is what i understood from the
>> patch and how kuogee explained me.
>>
>> The ordering of the array still matters here and thats what he is trying
>> to address with the second change.
>>
>> So as per him, he tried to swap the order of entries like below and that
>> did not work and that is incorrect behavior because he still retained
>> the MSM_DP_CONTROLLER_x field for the table like below:
> I'd like to understand why did he try to change the order in the first place.
>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>> b/drivers/gpu/drm/msm/dp/dp_display.c
>> index dcd80c8a794c..7816e82452ca 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -140,8 +140,8 @@ static const struct msm_dp_config sc7180_dp_cfg = {
>>
>>    static const struct msm_dp_config sc7280_dp_cfg = {
>>           .descs = (const struct msm_dp_desc[]) {
>> -               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
>>                   [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000,
>> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>> +               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
>>           },
>>           .num_descs = 2,
>>    };
>>
>>
>> The reason order is important is because  in this function below, even
>> though it matches the address to find which one to use it loops through
>> the array and so the value of *id will change depending on which one is
>> located where.
>>
>> static const struct msm_dp_desc *dp_display_get_desc(struct
>> platform_device *pdev,
>>                                unsigned int *id)
>> {
>>       const struct msm_dp_config *cfg = of_device_get_match_data(&pdev->dev);
>>       struct resource *res;
>>       int i;
>>
>>       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>       if (!res)
>>           return NULL;
>>
>>       for (i = 0; i < cfg->num_descs; i++) {
>>           if (cfg->descs[i].io_start == res->start) {
>>               *id = i;
> The id is set to the index of the corresponding DP instance in the
> descs array, which is MSM_DP_CONTROLLER_n. Correct up to now.
>
>>               return &cfg->descs[i];
>>           }
>>       }
>>
>> In dp_display_bind(), dp->id is used as the index of assigning the
>> dp_display,
>>
>> priv->dp[dp->id] = &dp->dp_display;
> dp->id earlier is set to the id returned by dp_display_get_desc.
> So the priv->dp is now indexed by MSM_DP_CONTROLLER_n. Again, correct.
>
>> And now in _dpu_kms_initialize_displayport(), in the array this will
>> decide the value of info.h_tile_instance[0] which will be assigned to
>> just the index i.
> i is iterated over priv->dp indices (MSM_DP_CONTROLLER_n, see above),
> which means that that h_tile_instance[0] is now set to the
> MSM_DP_CONTROLLER_n. Still correct.
>
>> info.h_tile_instance[0] is then used as the controller id to find from
>> the catalog table.
> This sounds good.
How can I have eDP call dpu_encoder_init() before DP calls with 
_dpu_kms_initialize_displayport()?
>> So if this order is not retained it does not work.
>>
>> Thats the issue he is trying to address to make the order of entries
>> irrelevant in the table in dp_display.c
>
>

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Dmitry Baryshkov]

On Sat, 25 Jun 2022 at 03:19, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
> How can I have eDP call dpu_encoder_init() before DP calls with
> _dpu_kms_initialize_displayport()?

Why do you want to do it? They are two different encoders.

-- 
With best wishes
Dmitry

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Dmitry Baryshkov]

On Sat, 25 Jun 2022 at 03:19, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
> How can I have eDP call dpu_encoder_init() before DP calls with
> _dpu_kms_initialize_displayport()?

Why do you want to do it? They are two different encoders.

-- 
With best wishes
Dmitry

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Kuogee Hsieh]

On 6/24/2022 5:21 PM, Dmitry Baryshkov wrote:
> On Sat, 25 Jun 2022 at 03:19, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>> How can I have eDP call dpu_encoder_init() before DP calls with
>> _dpu_kms_initialize_displayport()?
> Why do you want to do it? They are two different encoders.

eDP is primary display which in normal case should be bring up first if 
DP is also presented.

We want eDP encoder  help functions be executed before DP.

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Kuogee Hsieh]

On 6/24/2022 5:21 PM, Dmitry Baryshkov wrote:
> On Sat, 25 Jun 2022 at 03:19, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>> How can I have eDP call dpu_encoder_init() before DP calls with
>> _dpu_kms_initialize_displayport()?
> Why do you want to do it? They are two different encoders.

eDP is primary display which in normal case should be bring up first if 
DP is also presented.

We want eDP encoder  help functions be executed before DP.

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Dmitry Baryshkov]

On Sat, 25 Jun 2022 at 03:23, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
> On 6/24/2022 5:21 PM, Dmitry Baryshkov wrote:
> > On Sat, 25 Jun 2022 at 03:19, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
> >> How can I have eDP call dpu_encoder_init() before DP calls with
> >> _dpu_kms_initialize_displayport()?
> > Why do you want to do it? They are two different encoders.
>
> eDP is primary display which in normal case should be bring up first if
> DP is also presented.

I do not like the concept of primary display. It is the user, who must
decide which display is primary to him. I have seen people using just
external monitors and ignoring built-in eDP completely.

Also, why does the bring up order matters here? What do you gain by
bringing up eDP before the DP?

>
> We want eDP encoder  help functions be executed before DP.

Why?

-- 
With best wishes
Dmitry

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Dmitry Baryshkov]

On Sat, 25 Jun 2022 at 03:23, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
> On 6/24/2022 5:21 PM, Dmitry Baryshkov wrote:
> > On Sat, 25 Jun 2022 at 03:19, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
> >> How can I have eDP call dpu_encoder_init() before DP calls with
> >> _dpu_kms_initialize_displayport()?
> > Why do you want to do it? They are two different encoders.
>
> eDP is primary display which in normal case should be bring up first if
> DP is also presented.

I do not like the concept of primary display. It is the user, who must
decide which display is primary to him. I have seen people using just
external monitors and ignoring built-in eDP completely.

Also, why does the bring up order matters here? What do you gain by
bringing up eDP before the DP?

>
> We want eDP encoder  help functions be executed before DP.

Why?

-- 
With best wishes
Dmitry

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Dmitry Baryshkov]

On Sat, 25 Jun 2022 at 03:28, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Sat, 25 Jun 2022 at 03:23, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
> > On 6/24/2022 5:21 PM, Dmitry Baryshkov wrote:
> > > On Sat, 25 Jun 2022 at 03:19, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
> > >> How can I have eDP call dpu_encoder_init() before DP calls with
> > >> _dpu_kms_initialize_displayport()?
> > > Why do you want to do it? They are two different encoders.
> >
> > eDP is primary display which in normal case should be bring up first if
> > DP is also presented.
>
> I do not like the concept of primary display. It is the user, who must
> decide which display is primary to him. I have seen people using just
> external monitors and ignoring built-in eDP completely.
>
> Also, why does the bring up order matters here? What do you gain by
> bringing up eDP before the DP?

I should probably rephrase my question to be more clear. How does
changing the order of DP vs eDP bringup help you 'to fix screen
corruption'.

-- 
With best wishes
Dmitry

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Dmitry Baryshkov]

On Sat, 25 Jun 2022 at 03:28, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Sat, 25 Jun 2022 at 03:23, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
> > On 6/24/2022 5:21 PM, Dmitry Baryshkov wrote:
> > > On Sat, 25 Jun 2022 at 03:19, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
> > >> How can I have eDP call dpu_encoder_init() before DP calls with
> > >> _dpu_kms_initialize_displayport()?
> > > Why do you want to do it? They are two different encoders.
> >
> > eDP is primary display which in normal case should be bring up first if
> > DP is also presented.
>
> I do not like the concept of primary display. It is the user, who must
> decide which display is primary to him. I have seen people using just
> external monitors and ignoring built-in eDP completely.
>
> Also, why does the bring up order matters here? What do you gain by
> bringing up eDP before the DP?

I should probably rephrase my question to be more clear. How does
changing the order of DP vs eDP bringup help you 'to fix screen
corruption'.

-- 
With best wishes
Dmitry

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Kuogee Hsieh]

On 6/24/2022 5:46 PM, Dmitry Baryshkov wrote:
> On Sat, 25 Jun 2022 at 03:28, Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
>> On Sat, 25 Jun 2022 at 03:23, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>>> On 6/24/2022 5:21 PM, Dmitry Baryshkov wrote:
>>>> On Sat, 25 Jun 2022 at 03:19, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>>>>> How can I have eDP call dpu_encoder_init() before DP calls with
>>>>> _dpu_kms_initialize_displayport()?
>>>> Why do you want to do it? They are two different encoders.
>>> eDP is primary display which in normal case should be bring up first if
>>> DP is also presented.
>> I do not like the concept of primary display. It is the user, who must
>> decide which display is primary to him. I have seen people using just
>> external monitors and ignoring built-in eDP completely.from

>> Also, why does the bring up order matters here? What do you gain by
>> bringing up eDP before the DP?
> I should probably rephrase my question to be more clear. How does
> changing the order of DP vs eDP bringup help you 'to fix screen
> corruption'.

it did fix the primary display correction issue if edp go first and i do 
not know the root cause yet.

We are still investigating it.

However I do think currently msm_dp_config sc7280_dp_cfg has issues need 
be addressed.



>

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Kuogee Hsieh]

On 6/24/2022 5:46 PM, Dmitry Baryshkov wrote:
> On Sat, 25 Jun 2022 at 03:28, Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
>> On Sat, 25 Jun 2022 at 03:23, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>>> On 6/24/2022 5:21 PM, Dmitry Baryshkov wrote:
>>>> On Sat, 25 Jun 2022 at 03:19, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>>>>> How can I have eDP call dpu_encoder_init() before DP calls with
>>>>> _dpu_kms_initialize_displayport()?
>>>> Why do you want to do it? They are two different encoders.
>>> eDP is primary display which in normal case should be bring up first if
>>> DP is also presented.
>> I do not like the concept of primary display. It is the user, who must
>> decide which display is primary to him. I have seen people using just
>> external monitors and ignoring built-in eDP completely.from

>> Also, why does the bring up order matters here? What do you gain by
>> bringing up eDP before the DP?
> I should probably rephrase my question to be more clear. How does
> changing the order of DP vs eDP bringup help you 'to fix screen
> corruption'.

it did fix the primary display correction issue if edp go first and i do 
not know the root cause yet.

We are still investigating it.

However I do think currently msm_dp_config sc7280_dp_cfg has issues need 
be addressed.



>

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Stephen Boyd]

Quoting Kuogee Hsieh (2022-06-24 18:02:50)
>
> On 6/24/2022 5:46 PM, Dmitry Baryshkov wrote:
> > On Sat, 25 Jun 2022 at 03:28, Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> >> On Sat, 25 Jun 2022 at 03:23, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
> >>> On 6/24/2022 5:21 PM, Dmitry Baryshkov wrote:
> >>>> On Sat, 25 Jun 2022 at 03:19, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
> >>>>> How can I have eDP call dpu_encoder_init() before DP calls with
> >>>>> _dpu_kms_initialize_displayport()?
> >>>> Why do you want to do it? They are two different encoders.
> >>> eDP is primary display which in normal case should be bring up first if
> >>> DP is also presented.
> >> I do not like the concept of primary display. It is the user, who must
> >> decide which display is primary to him. I have seen people using just
> >> external monitors and ignoring built-in eDP completely.from
>
> >> Also, why does the bring up order matters here? What do you gain by
> >> bringing up eDP before the DP?
> > I should probably rephrase my question to be more clear. How does
> > changing the order of DP vs eDP bringup help you 'to fix screen
> > corruption'.
>
> it did fix the primary display correction issue if edp go first and i do
> not know the root cause yet.
>
> We are still investigating it.
>
> However I do think currently msm_dp_config sc7280_dp_cfg has issues need
> be addressed.
>

What issues exist with sc7280_dp_cfg? It looks correct to me.

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Stephen Boyd]

Quoting Kuogee Hsieh (2022-06-24 18:02:50)
>
> On 6/24/2022 5:46 PM, Dmitry Baryshkov wrote:
> > On Sat, 25 Jun 2022 at 03:28, Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> >> On Sat, 25 Jun 2022 at 03:23, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
> >>> On 6/24/2022 5:21 PM, Dmitry Baryshkov wrote:
> >>>> On Sat, 25 Jun 2022 at 03:19, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
> >>>>> How can I have eDP call dpu_encoder_init() before DP calls with
> >>>>> _dpu_kms_initialize_displayport()?
> >>>> Why do you want to do it? They are two different encoders.
> >>> eDP is primary display which in normal case should be bring up first if
> >>> DP is also presented.
> >> I do not like the concept of primary display. It is the user, who must
> >> decide which display is primary to him. I have seen people using just
> >> external monitors and ignoring built-in eDP completely.from
>
> >> Also, why does the bring up order matters here? What do you gain by
> >> bringing up eDP before the DP?
> > I should probably rephrase my question to be more clear. How does
> > changing the order of DP vs eDP bringup help you 'to fix screen
> > corruption'.
>
> it did fix the primary display correction issue if edp go first and i do
> not know the root cause yet.
>
> We are still investigating it.
>
> However I do think currently msm_dp_config sc7280_dp_cfg has issues need
> be addressed.
>

What issues exist with sc7280_dp_cfg? It looks correct to me.

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Kuogee Hsieh]

On 6/24/2022 6:15 PM, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2022-06-24 18:02:50)
>> On 6/24/2022 5:46 PM, Dmitry Baryshkov wrote:
>>> On Sat, 25 Jun 2022 at 03:28, Dmitry Baryshkov
>>> <dmitry.baryshkov@linaro.org> wrote:
>>>> On Sat, 25 Jun 2022 at 03:23, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>>>>> On 6/24/2022 5:21 PM, Dmitry Baryshkov wrote:
>>>>>> On Sat, 25 Jun 2022 at 03:19, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>>>>>>> How can I have eDP call dpu_encoder_init() before DP calls with
>>>>>>> _dpu_kms_initialize_displayport()?
>>>>>> Why do you want to do it? They are two different encoders.
>>>>> eDP is primary display which in normal case should be bring up first if
>>>>> DP is also presented.
>>>> I do not like the concept of primary display. It is the user, who must
>>>> decide which display is primary to him. I have seen people using just
>>>> external monitors and ignoring built-in eDP completely.from
>>>> Also, why does the bring up order matters here? What do you gain by
>>>> bringing up eDP before the DP?
>>> I should probably rephrase my question to be more clear. How does
>>> changing the order of DP vs eDP bringup help you 'to fix screen
>>> corruption'.
>> it did fix the primary display correction issue if edp go first and i do
>> not know the root cause yet.
>>
>> We are still investigating it.
>>
>> However I do think currently msm_dp_config sc7280_dp_cfg has issues need
>> be addressed.
>>
> What issues exist with sc7280_dp_cfg? It looks correct to me.


If we are going to bring up a new chipset with edp as primary only, i am 
not sure the below configuration will work?

> static const struct msm_dp_config sc7280_dp_cfg = {
>          .descs = (const struct msm_dp_desc[]) {
>                  [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000, .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>          },
>          .num_descs = 1,
> };

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Kuogee Hsieh]

On 6/24/2022 6:15 PM, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2022-06-24 18:02:50)
>> On 6/24/2022 5:46 PM, Dmitry Baryshkov wrote:
>>> On Sat, 25 Jun 2022 at 03:28, Dmitry Baryshkov
>>> <dmitry.baryshkov@linaro.org> wrote:
>>>> On Sat, 25 Jun 2022 at 03:23, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>>>>> On 6/24/2022 5:21 PM, Dmitry Baryshkov wrote:
>>>>>> On Sat, 25 Jun 2022 at 03:19, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>>>>>>> How can I have eDP call dpu_encoder_init() before DP calls with
>>>>>>> _dpu_kms_initialize_displayport()?
>>>>>> Why do you want to do it? They are two different encoders.
>>>>> eDP is primary display which in normal case should be bring up first if
>>>>> DP is also presented.
>>>> I do not like the concept of primary display. It is the user, who must
>>>> decide which display is primary to him. I have seen people using just
>>>> external monitors and ignoring built-in eDP completely.from
>>>> Also, why does the bring up order matters here? What do you gain by
>>>> bringing up eDP before the DP?
>>> I should probably rephrase my question to be more clear. How does
>>> changing the order of DP vs eDP bringup help you 'to fix screen
>>> corruption'.
>> it did fix the primary display correction issue if edp go first and i do
>> not know the root cause yet.
>>
>> We are still investigating it.
>>
>> However I do think currently msm_dp_config sc7280_dp_cfg has issues need
>> be addressed.
>>
> What issues exist with sc7280_dp_cfg? It looks correct to me.


If we are going to bring up a new chipset with edp as primary only, i am 
not sure the below configuration will work?

> static const struct msm_dp_config sc7280_dp_cfg = {
>          .descs = (const struct msm_dp_desc[]) {
>                  [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000, .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>          },
>          .num_descs = 1,
> };

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Dmitry Baryshkov]

On Mon, 27 Jun 2022 at 18:33, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
>
> On 6/24/2022 6:15 PM, Stephen Boyd wrote:
> > Quoting Kuogee Hsieh (2022-06-24 18:02:50)
> >> On 6/24/2022 5:46 PM, Dmitry Baryshkov wrote:
> >>> On Sat, 25 Jun 2022 at 03:28, Dmitry Baryshkov
> >>> <dmitry.baryshkov@linaro.org> wrote:
> >>>> On Sat, 25 Jun 2022 at 03:23, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
> >>>>> On 6/24/2022 5:21 PM, Dmitry Baryshkov wrote:
> >>>>>> On Sat, 25 Jun 2022 at 03:19, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
> >>>>>>> How can I have eDP call dpu_encoder_init() before DP calls with
> >>>>>>> _dpu_kms_initialize_displayport()?
> >>>>>> Why do you want to do it? They are two different encoders.
> >>>>> eDP is primary display which in normal case should be bring up first if
> >>>>> DP is also presented.
> >>>> I do not like the concept of primary display. It is the user, who must
> >>>> decide which display is primary to him. I have seen people using just
> >>>> external monitors and ignoring built-in eDP completely.from
> >>>> Also, why does the bring up order matters here? What do you gain by
> >>>> bringing up eDP before the DP?
> >>> I should probably rephrase my question to be more clear. How does
> >>> changing the order of DP vs eDP bringup help you 'to fix screen
> >>> corruption'.
> >> it did fix the primary display correction issue if edp go first and i do
> >> not know the root cause yet.
> >>
> >> We are still investigating it.
> >>
> >> However I do think currently msm_dp_config sc7280_dp_cfg has issues need
> >> be addressed.
> >>
> > What issues exist with sc7280_dp_cfg? It looks correct to me.
>
>
> If we are going to bring up a new chipset with edp as primary only, i am
> not sure the below configuration will work?
>
> > static const struct msm_dp_config sc7280_dp_cfg = {
> >          .descs = (const struct msm_dp_desc[]) {
> >                  [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000, .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
> >          },
> >          .num_descs = 1,
> > };

As I wrote in one of the comments, there is an issue with num_descs
being not obvious (in your example it should be 2, not 1). I thought
about dropping it and looping until the MSM_DP_CONTROLLER_COUNT, but
this would result in other kinds of hard-to-catch issues. Let me muse
about it.

-- 
With best wishes
Dmitry

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Dmitry Baryshkov]

On Mon, 27 Jun 2022 at 18:33, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
>
> On 6/24/2022 6:15 PM, Stephen Boyd wrote:
> > Quoting Kuogee Hsieh (2022-06-24 18:02:50)
> >> On 6/24/2022 5:46 PM, Dmitry Baryshkov wrote:
> >>> On Sat, 25 Jun 2022 at 03:28, Dmitry Baryshkov
> >>> <dmitry.baryshkov@linaro.org> wrote:
> >>>> On Sat, 25 Jun 2022 at 03:23, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
> >>>>> On 6/24/2022 5:21 PM, Dmitry Baryshkov wrote:
> >>>>>> On Sat, 25 Jun 2022 at 03:19, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
> >>>>>>> How can I have eDP call dpu_encoder_init() before DP calls with
> >>>>>>> _dpu_kms_initialize_displayport()?
> >>>>>> Why do you want to do it? They are two different encoders.
> >>>>> eDP is primary display which in normal case should be bring up first if
> >>>>> DP is also presented.
> >>>> I do not like the concept of primary display. It is the user, who must
> >>>> decide which display is primary to him. I have seen people using just
> >>>> external monitors and ignoring built-in eDP completely.from
> >>>> Also, why does the bring up order matters here? What do you gain by
> >>>> bringing up eDP before the DP?
> >>> I should probably rephrase my question to be more clear. How does
> >>> changing the order of DP vs eDP bringup help you 'to fix screen
> >>> corruption'.
> >> it did fix the primary display correction issue if edp go first and i do
> >> not know the root cause yet.
> >>
> >> We are still investigating it.
> >>
> >> However I do think currently msm_dp_config sc7280_dp_cfg has issues need
> >> be addressed.
> >>
> > What issues exist with sc7280_dp_cfg? It looks correct to me.
>
>
> If we are going to bring up a new chipset with edp as primary only, i am
> not sure the below configuration will work?
>
> > static const struct msm_dp_config sc7280_dp_cfg = {
> >          .descs = (const struct msm_dp_desc[]) {
> >                  [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000, .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
> >          },
> >          .num_descs = 1,
> > };

As I wrote in one of the comments, there is an issue with num_descs
being not obvious (in your example it should be 2, not 1). I thought
about dropping it and looping until the MSM_DP_CONTROLLER_COUNT, but
this would result in other kinds of hard-to-catch issues. Let me muse
about it.

-- 
With best wishes
Dmitry

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Kuogee Hsieh]

On 6/27/2022 8:38 AM, Dmitry Baryshkov wrote:
> On Mon, 27 Jun 2022 at 18:33, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>>
>> On 6/24/2022 6:15 PM, Stephen Boyd wrote:
>>> Quoting Kuogee Hsieh (2022-06-24 18:02:50)
>>>> On 6/24/2022 5:46 PM, Dmitry Baryshkov wrote:
>>>>> On Sat, 25 Jun 2022 at 03:28, Dmitry Baryshkov
>>>>> <dmitry.baryshkov@linaro.org> wrote:
>>>>>> On Sat, 25 Jun 2022 at 03:23, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>>>>>>> On 6/24/2022 5:21 PM, Dmitry Baryshkov wrote:
>>>>>>>> On Sat, 25 Jun 2022 at 03:19, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>>>>>>>>> How can I have eDP call dpu_encoder_init() before DP calls with
>>>>>>>>> _dpu_kms_initialize_displayport()?
>>>>>>>> Why do you want to do it? They are two different encoders.
>>>>>>> eDP is primary display which in normal case should be bring up first if
>>>>>>> DP is also presented.
>>>>>> I do not like the concept of primary display. It is the user, who must
>>>>>> decide which display is primary to him. I have seen people using just
>>>>>> external monitors and ignoring built-in eDP completely.from
>>>>>> Also, why does the bring up order matters here? What do you gain by
>>>>>> bringing up eDP before the DP?
>>>>> I should probably rephrase my question to be more clear. How does
>>>>> changing the order of DP vs eDP bringup help you 'to fix screen
>>>>> corruption'.
>>>> it did fix the primary display correction issue if edp go first and i do
>>>> not know the root cause yet.
>>>>
>>>> We are still investigating it.
>>>>
>>>> However I do think currently msm_dp_config sc7280_dp_cfg has issues need
>>>> be addressed.
>>>>
>>> What issues exist with sc7280_dp_cfg? It looks correct to me.
>>
>> If we are going to bring up a new chipset with edp as primary only, i am
>> not sure the below configuration will work?
>>
>>> static const struct msm_dp_config sc7280_dp_cfg = {
>>>           .descs = (const struct msm_dp_desc[]) {
>>>                   [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000, .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>>>           },
>>>           .num_descs = 1,
>>> };
> As I wrote in one of the comments, there is an issue with num_descs
> being not obvious (in your example it should be 2, not 1). I thought
> about dropping it and looping until the MSM_DP_CONTROLLER_COUNT, but
> this would result in other kinds of hard-to-catch issues. Let me muse
> about it.

Thanks for consideration.

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Kuogee Hsieh]

On 6/27/2022 8:38 AM, Dmitry Baryshkov wrote:
> On Mon, 27 Jun 2022 at 18:33, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>>
>> On 6/24/2022 6:15 PM, Stephen Boyd wrote:
>>> Quoting Kuogee Hsieh (2022-06-24 18:02:50)
>>>> On 6/24/2022 5:46 PM, Dmitry Baryshkov wrote:
>>>>> On Sat, 25 Jun 2022 at 03:28, Dmitry Baryshkov
>>>>> <dmitry.baryshkov@linaro.org> wrote:
>>>>>> On Sat, 25 Jun 2022 at 03:23, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>>>>>>> On 6/24/2022 5:21 PM, Dmitry Baryshkov wrote:
>>>>>>>> On Sat, 25 Jun 2022 at 03:19, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>>>>>>>>> How can I have eDP call dpu_encoder_init() before DP calls with
>>>>>>>>> _dpu_kms_initialize_displayport()?
>>>>>>>> Why do you want to do it? They are two different encoders.
>>>>>>> eDP is primary display which in normal case should be bring up first if
>>>>>>> DP is also presented.
>>>>>> I do not like the concept of primary display. It is the user, who must
>>>>>> decide which display is primary to him. I have seen people using just
>>>>>> external monitors and ignoring built-in eDP completely.from
>>>>>> Also, why does the bring up order matters here? What do you gain by
>>>>>> bringing up eDP before the DP?
>>>>> I should probably rephrase my question to be more clear. How does
>>>>> changing the order of DP vs eDP bringup help you 'to fix screen
>>>>> corruption'.
>>>> it did fix the primary display correction issue if edp go first and i do
>>>> not know the root cause yet.
>>>>
>>>> We are still investigating it.
>>>>
>>>> However I do think currently msm_dp_config sc7280_dp_cfg has issues need
>>>> be addressed.
>>>>
>>> What issues exist with sc7280_dp_cfg? It looks correct to me.
>>
>> If we are going to bring up a new chipset with edp as primary only, i am
>> not sure the below configuration will work?
>>
>>> static const struct msm_dp_config sc7280_dp_cfg = {
>>>           .descs = (const struct msm_dp_desc[]) {
>>>                   [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000, .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>>>           },
>>>           .num_descs = 1,
>>> };
> As I wrote in one of the comments, there is an issue with num_descs
> being not obvious (in your example it should be 2, not 1). I thought
> about dropping it and looping until the MSM_DP_CONTROLLER_COUNT, but
> this would result in other kinds of hard-to-catch issues. Let me muse
> about it.

Thanks for consideration.

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Abhinav Kumar]

On 6/24/2022 5:11 PM, Dmitry Baryshkov wrote:
> On Sat, 25 Jun 2022 at 03:03, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>> Hi Stephen / Dmitry
>>
>> Let me try to explain the issue kuogee is trying to fix below:
>>
>> On 6/24/2022 4:56 PM, Kuogee Hsieh wrote:
>>>
>>> On 6/24/2022 4:45 PM, Stephen Boyd wrote:
>>>> Quoting Kuogee Hsieh (2022-06-24 16:30:59)
>>>>> On 6/24/2022 4:12 PM, Stephen Boyd wrote:
>>>>>> Quoting Kuogee Hsieh (2022-06-24 15:53:45)
>>>>>>> MSM_DP_CONTROLLER_1 need to match to the index = 1 of
>>>>>>> sc7280_dp_cfg[] <== This is correct
>>>>>>>
>>>>>>> The problem is sc7280_dp_cfg[] have two entries since eDP place at
>>>>>>> index
>>>>>>> of MSM_DP_CONTROLLER_1.
>>>>>>>
>>>>>>> but .num_desc = 1  <== this said only have one entry at
>>>>>>> sc7280_dp_cfg[]
>>>>>>> table. Therefore eDP will never be found at for loop  at
>>>>>>> _dpu_kms_initialize_displayport().
>>>>>>>
>>>>>> Yes, but what else does the MSM_DP_CONTROLLER_1 need to match? Because
>>>>>> the intention of the previous commit was to make it so the order of
>>>>>> sc7280_dp_cfg couldn't be messed up and not match the
>>>>>> MSM_DP_CONTROLLER_1 value that lives in sc7280_intf[].
>>>>>
>>>>> at  _dpu_kms_initialize_displayport()
>>>>>
>>>>>> -             info.h_tile_instance[0] = i; <== assign i to become dp
>>>>>> controller id, "i" is index of scxxxx_dp_cfg[]
>>>>> This what I mean MSM_DP_CONTROLLER_1 need to match to index = 1 of
>>>>> scxxxx_dp_cfg[].
>>>>>
>>>>> it it is not match, then MSM_DP_CONTROLLER_1 with match to different
>>>>> INTF.
>>>> I thought we matched the INTF instance by searching through
>>>> sc7280_intf[] for a matching MSM_DP_CONTROLLER_1 and then returning that
>>>> INTF number. See dpu_encoder_get_intf() and the caller.
>>>
>>> yes, but the controller_id had been over written by dp->id.
>>>
>>> u32 controller_id = disp_info->h_tile_instance[i];
>>>
>>>
>>> See below code.
>>>
>>>
>>>>           for (i = 0; i < disp_info->num_of_h_tiles && !ret; i++) {
>>>>                   /*
>>>>                    * Left-most tile is at index 0, content is
>>>> controller id
>>>>                    * h_tile_instance_ids[2] = {0, 1}; DSI0 = left, DSI1
>>>> = right
>>>>                    * h_tile_instance_ids[2] = {1, 0}; DSI1 = left, DSI0
>>>> = right
>>>>                    */
>>>>                   u32 controller_id = disp_info->h_tile_instance[i];
>>>> <== kuogee assign dp->id to controller_id
>>>>
>>>>                   if (disp_info->num_of_h_tiles > 1) {
>>>>                           if (i == 0)
>>>>                                   phys_params.split_role =
>>>> ENC_ROLE_MASTER;
>>>>                           else
>>>>                                   phys_params.split_role = ENC_ROLE_SLAVE;
>>>>                   } else {
>>>>                           phys_params.split_role = ENC_ROLE_SOLO;
>>>>                   }
>>>>
>>>>                   DPU_DEBUG("h_tile_instance %d = %d, split_role %d\n",
>>>>                                   i, controller_id,
>>>> phys_params.split_role);
>>>>
>>>>                   phys_params.intf_idx =
>>>> dpu_encoder_get_intf(dpu_kms->catalog,
>>>>
>>>>                 intf_type,
>>>>
>>>>                 controller_id);
>>
>>
>> So let me try to explain this as this is what i understood from the
>> patch and how kuogee explained me.
>>
>> The ordering of the array still matters here and thats what he is trying
>> to address with the second change.
>>
>> So as per him, he tried to swap the order of entries like below and that
>> did not work and that is incorrect behavior because he still retained
>> the MSM_DP_CONTROLLER_x field for the table like below:
> 
> I'd like to understand why did he try to change the order in the first place.
> 
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>> b/drivers/gpu/drm/msm/dp/dp_display.c
>> index dcd80c8a794c..7816e82452ca 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -140,8 +140,8 @@ static const struct msm_dp_config sc7180_dp_cfg = {
>>
>>    static const struct msm_dp_config sc7280_dp_cfg = {
>>           .descs = (const struct msm_dp_desc[]) {
>> -               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
>>                   [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000,
>> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>> +               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
>>           },
>>           .num_descs = 2,
>>    };
>>
>>
>> The reason order is important is because  in this function below, even
>> though it matches the address to find which one to use it loops through
>> the array and so the value of *id will change depending on which one is
>> located where.
>>
>> static const struct msm_dp_desc *dp_display_get_desc(struct
>> platform_device *pdev,
>>                                unsigned int *id)
>> {
>>       const struct msm_dp_config *cfg = of_device_get_match_data(&pdev->dev);
>>       struct resource *res;
>>       int i;
>>
>>       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>       if (!res)
>>           return NULL;
>>
>>       for (i = 0; i < cfg->num_descs; i++) {
>>           if (cfg->descs[i].io_start == res->start) {
>>               *id = i;
> 
> The id is set to the index of the corresponding DP instance in the
> descs array, which is MSM_DP_CONTROLLER_n. Correct up to now.

Right, this is where I misunderstood his explanation.

Even if we swap the order, but retain the index correctly it will still 
work today.

Hes not sure of the root-cause of why turning on the primary display 
first fixes the issue.

I think till we root-cause that, lets put this on hold.

> 
>>               return &cfg->descs[i];
>>           }
>>       }
>>
>> In dp_display_bind(), dp->id is used as the index of assigning the
>> dp_display,
>>
>> priv->dp[dp->id] = &dp->dp_display;
> 
> dp->id earlier is set to the id returned by dp_display_get_desc.
> So the priv->dp is now indexed by MSM_DP_CONTROLLER_n. Again, correct.
> 
>>
>> And now in _dpu_kms_initialize_displayport(), in the array this will
>> decide the value of info.h_tile_instance[0] which will be assigned to
>> just the index i.
> 
> i is iterated over priv->dp indices (MSM_DP_CONTROLLER_n, see above),
> which means that that h_tile_instance[0] is now set to the
> MSM_DP_CONTROLLER_n. Still correct.
> 
>> info.h_tile_instance[0] is then used as the controller id to find from
>> the catalog table.
> 
> This sounds good.
> 
>> So if this order is not retained it does not work.
>>
>> Thats the issue he is trying to address to make the order of entries
>> irrelevant in the table in dp_display.c
> 
> 
> 

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Abhinav Kumar]

On 6/24/2022 5:11 PM, Dmitry Baryshkov wrote:
> On Sat, 25 Jun 2022 at 03:03, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>> Hi Stephen / Dmitry
>>
>> Let me try to explain the issue kuogee is trying to fix below:
>>
>> On 6/24/2022 4:56 PM, Kuogee Hsieh wrote:
>>>
>>> On 6/24/2022 4:45 PM, Stephen Boyd wrote:
>>>> Quoting Kuogee Hsieh (2022-06-24 16:30:59)
>>>>> On 6/24/2022 4:12 PM, Stephen Boyd wrote:
>>>>>> Quoting Kuogee Hsieh (2022-06-24 15:53:45)
>>>>>>> MSM_DP_CONTROLLER_1 need to match to the index = 1 of
>>>>>>> sc7280_dp_cfg[] <== This is correct
>>>>>>>
>>>>>>> The problem is sc7280_dp_cfg[] have two entries since eDP place at
>>>>>>> index
>>>>>>> of MSM_DP_CONTROLLER_1.
>>>>>>>
>>>>>>> but .num_desc = 1  <== this said only have one entry at
>>>>>>> sc7280_dp_cfg[]
>>>>>>> table. Therefore eDP will never be found at for loop  at
>>>>>>> _dpu_kms_initialize_displayport().
>>>>>>>
>>>>>> Yes, but what else does the MSM_DP_CONTROLLER_1 need to match? Because
>>>>>> the intention of the previous commit was to make it so the order of
>>>>>> sc7280_dp_cfg couldn't be messed up and not match the
>>>>>> MSM_DP_CONTROLLER_1 value that lives in sc7280_intf[].
>>>>>
>>>>> at  _dpu_kms_initialize_displayport()
>>>>>
>>>>>> -             info.h_tile_instance[0] = i; <== assign i to become dp
>>>>>> controller id, "i" is index of scxxxx_dp_cfg[]
>>>>> This what I mean MSM_DP_CONTROLLER_1 need to match to index = 1 of
>>>>> scxxxx_dp_cfg[].
>>>>>
>>>>> it it is not match, then MSM_DP_CONTROLLER_1 with match to different
>>>>> INTF.
>>>> I thought we matched the INTF instance by searching through
>>>> sc7280_intf[] for a matching MSM_DP_CONTROLLER_1 and then returning that
>>>> INTF number. See dpu_encoder_get_intf() and the caller.
>>>
>>> yes, but the controller_id had been over written by dp->id.
>>>
>>> u32 controller_id = disp_info->h_tile_instance[i];
>>>
>>>
>>> See below code.
>>>
>>>
>>>>           for (i = 0; i < disp_info->num_of_h_tiles && !ret; i++) {
>>>>                   /*
>>>>                    * Left-most tile is at index 0, content is
>>>> controller id
>>>>                    * h_tile_instance_ids[2] = {0, 1}; DSI0 = left, DSI1
>>>> = right
>>>>                    * h_tile_instance_ids[2] = {1, 0}; DSI1 = left, DSI0
>>>> = right
>>>>                    */
>>>>                   u32 controller_id = disp_info->h_tile_instance[i];
>>>> <== kuogee assign dp->id to controller_id
>>>>
>>>>                   if (disp_info->num_of_h_tiles > 1) {
>>>>                           if (i == 0)
>>>>                                   phys_params.split_role =
>>>> ENC_ROLE_MASTER;
>>>>                           else
>>>>                                   phys_params.split_role = ENC_ROLE_SLAVE;
>>>>                   } else {
>>>>                           phys_params.split_role = ENC_ROLE_SOLO;
>>>>                   }
>>>>
>>>>                   DPU_DEBUG("h_tile_instance %d = %d, split_role %d\n",
>>>>                                   i, controller_id,
>>>> phys_params.split_role);
>>>>
>>>>                   phys_params.intf_idx =
>>>> dpu_encoder_get_intf(dpu_kms->catalog,
>>>>
>>>>                 intf_type,
>>>>
>>>>                 controller_id);
>>
>>
>> So let me try to explain this as this is what i understood from the
>> patch and how kuogee explained me.
>>
>> The ordering of the array still matters here and thats what he is trying
>> to address with the second change.
>>
>> So as per him, he tried to swap the order of entries like below and that
>> did not work and that is incorrect behavior because he still retained
>> the MSM_DP_CONTROLLER_x field for the table like below:
> 
> I'd like to understand why did he try to change the order in the first place.
> 
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>> b/drivers/gpu/drm/msm/dp/dp_display.c
>> index dcd80c8a794c..7816e82452ca 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -140,8 +140,8 @@ static const struct msm_dp_config sc7180_dp_cfg = {
>>
>>    static const struct msm_dp_config sc7280_dp_cfg = {
>>           .descs = (const struct msm_dp_desc[]) {
>> -               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
>>                   [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000,
>> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>> +               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
>>           },
>>           .num_descs = 2,
>>    };
>>
>>
>> The reason order is important is because  in this function below, even
>> though it matches the address to find which one to use it loops through
>> the array and so the value of *id will change depending on which one is
>> located where.
>>
>> static const struct msm_dp_desc *dp_display_get_desc(struct
>> platform_device *pdev,
>>                                unsigned int *id)
>> {
>>       const struct msm_dp_config *cfg = of_device_get_match_data(&pdev->dev);
>>       struct resource *res;
>>       int i;
>>
>>       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>       if (!res)
>>           return NULL;
>>
>>       for (i = 0; i < cfg->num_descs; i++) {
>>           if (cfg->descs[i].io_start == res->start) {
>>               *id = i;
> 
> The id is set to the index of the corresponding DP instance in the
> descs array, which is MSM_DP_CONTROLLER_n. Correct up to now.

Right, this is where I misunderstood his explanation.

Even if we swap the order, but retain the index correctly it will still 
work today.

Hes not sure of the root-cause of why turning on the primary display 
first fixes the issue.

I think till we root-cause that, lets put this on hold.

> 
>>               return &cfg->descs[i];
>>           }
>>       }
>>
>> In dp_display_bind(), dp->id is used as the index of assigning the
>> dp_display,
>>
>> priv->dp[dp->id] = &dp->dp_display;
> 
> dp->id earlier is set to the id returned by dp_display_get_desc.
> So the priv->dp is now indexed by MSM_DP_CONTROLLER_n. Again, correct.
> 
>>
>> And now in _dpu_kms_initialize_displayport(), in the array this will
>> decide the value of info.h_tile_instance[0] which will be assigned to
>> just the index i.
> 
> i is iterated over priv->dp indices (MSM_DP_CONTROLLER_n, see above),
> which means that that h_tile_instance[0] is now set to the
> MSM_DP_CONTROLLER_n. Still correct.
> 
>> info.h_tile_instance[0] is then used as the controller id to find from
>> the catalog table.
> 
> This sounds good.
> 
>> So if this order is not retained it does not work.
>>
>> Thats the issue he is trying to address to make the order of entries
>> irrelevant in the table in dp_display.c
> 
> 
> 

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Dmitry Baryshkov]

On Sat, 25 Jun 2022 at 04:23, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> On 6/24/2022 5:11 PM, Dmitry Baryshkov wrote:
> > On Sat, 25 Jun 2022 at 03:03, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >> On 6/24/2022 4:56 PM, Kuogee Hsieh wrote:
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> >> b/drivers/gpu/drm/msm/dp/dp_display.c
> >> index dcd80c8a794c..7816e82452ca 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >> @@ -140,8 +140,8 @@ static const struct msm_dp_config sc7180_dp_cfg = {
> >>
> >>    static const struct msm_dp_config sc7280_dp_cfg = {
> >>           .descs = (const struct msm_dp_desc[]) {
> >> -               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
> >> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
> >>                   [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000,
> >> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
> >> +               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
> >> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
> >>           },
> >>           .num_descs = 2,
> >>    };
> >>
> >>
> >> The reason order is important is because  in this function below, even
> >> though it matches the address to find which one to use it loops through
> >> the array and so the value of *id will change depending on which one is
> >> located where.
> >>
> >> static const struct msm_dp_desc *dp_display_get_desc(struct
> >> platform_device *pdev,
> >>                                unsigned int *id)
> >> {
> >>       const struct msm_dp_config *cfg = of_device_get_match_data(&pdev->dev);
> >>       struct resource *res;
> >>       int i;
> >>
> >>       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >>       if (!res)
> >>           return NULL;
> >>
> >>       for (i = 0; i < cfg->num_descs; i++) {
> >>           if (cfg->descs[i].io_start == res->start) {
> >>               *id = i;
> >
> > The id is set to the index of the corresponding DP instance in the
> > descs array, which is MSM_DP_CONTROLLER_n. Correct up to now.
>
> Right, this is where I misunderstood his explanation.
>
> Even if we swap the order, but retain the index correctly it will still
> work today.
>
> Hes not sure of the root-cause of why turning on the primary display
> first fixes the issue.
>
> I think till we root-cause that, lets put this on hold.

Agreed. Let's find the root cause.


-- 
With best wishes
Dmitry

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Dmitry Baryshkov]

On Sat, 25 Jun 2022 at 04:23, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> On 6/24/2022 5:11 PM, Dmitry Baryshkov wrote:
> > On Sat, 25 Jun 2022 at 03:03, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >> On 6/24/2022 4:56 PM, Kuogee Hsieh wrote:
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> >> b/drivers/gpu/drm/msm/dp/dp_display.c
> >> index dcd80c8a794c..7816e82452ca 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >> @@ -140,8 +140,8 @@ static const struct msm_dp_config sc7180_dp_cfg = {
> >>
> >>    static const struct msm_dp_config sc7280_dp_cfg = {
> >>           .descs = (const struct msm_dp_desc[]) {
> >> -               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
> >> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
> >>                   [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000,
> >> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
> >> +               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
> >> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
> >>           },
> >>           .num_descs = 2,
> >>    };
> >>
> >>
> >> The reason order is important is because  in this function below, even
> >> though it matches the address to find which one to use it loops through
> >> the array and so the value of *id will change depending on which one is
> >> located where.
> >>
> >> static const struct msm_dp_desc *dp_display_get_desc(struct
> >> platform_device *pdev,
> >>                                unsigned int *id)
> >> {
> >>       const struct msm_dp_config *cfg = of_device_get_match_data(&pdev->dev);
> >>       struct resource *res;
> >>       int i;
> >>
> >>       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >>       if (!res)
> >>           return NULL;
> >>
> >>       for (i = 0; i < cfg->num_descs; i++) {
> >>           if (cfg->descs[i].io_start == res->start) {
> >>               *id = i;
> >
> > The id is set to the index of the corresponding DP instance in the
> > descs array, which is MSM_DP_CONTROLLER_n. Correct up to now.
>
> Right, this is where I misunderstood his explanation.
>
> Even if we swap the order, but retain the index correctly it will still
> work today.
>
> Hes not sure of the root-cause of why turning on the primary display
> first fixes the issue.
>
> I think till we root-cause that, lets put this on hold.

Agreed. Let's find the root cause.


-- 
With best wishes
Dmitry

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Doug Anderson]

Hi,

On Sat, Jun 25, 2022 at 1:48 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Sat, 25 Jun 2022 at 04:23, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> > On 6/24/2022 5:11 PM, Dmitry Baryshkov wrote:
> > > On Sat, 25 Jun 2022 at 03:03, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> > >> On 6/24/2022 4:56 PM, Kuogee Hsieh wrote:
> > >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> > >> b/drivers/gpu/drm/msm/dp/dp_display.c
> > >> index dcd80c8a794c..7816e82452ca 100644
> > >> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > >> @@ -140,8 +140,8 @@ static const struct msm_dp_config sc7180_dp_cfg = {
> > >>
> > >>    static const struct msm_dp_config sc7280_dp_cfg = {
> > >>           .descs = (const struct msm_dp_desc[]) {
> > >> -               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
> > >> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
> > >>                   [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000,
> > >> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
> > >> +               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
> > >> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
> > >>           },
> > >>           .num_descs = 2,
> > >>    };
> > >>
> > >>
> > >> The reason order is important is because  in this function below, even
> > >> though it matches the address to find which one to use it loops through
> > >> the array and so the value of *id will change depending on which one is
> > >> located where.
> > >>
> > >> static const struct msm_dp_desc *dp_display_get_desc(struct
> > >> platform_device *pdev,
> > >>                                unsigned int *id)
> > >> {
> > >>       const struct msm_dp_config *cfg = of_device_get_match_data(&pdev->dev);
> > >>       struct resource *res;
> > >>       int i;
> > >>
> > >>       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > >>       if (!res)
> > >>           return NULL;
> > >>
> > >>       for (i = 0; i < cfg->num_descs; i++) {
> > >>           if (cfg->descs[i].io_start == res->start) {
> > >>               *id = i;
> > >
> > > The id is set to the index of the corresponding DP instance in the
> > > descs array, which is MSM_DP_CONTROLLER_n. Correct up to now.
> >
> > Right, this is where I misunderstood his explanation.
> >
> > Even if we swap the order, but retain the index correctly it will still
> > work today.
> >
> > Hes not sure of the root-cause of why turning on the primary display
> > first fixes the issue.
> >
> > I think till we root-cause that, lets put this on hold.
>
> Agreed. Let's find the root cause.

FWIW, I was poking a little bit about the glitch that Kuogee was
trying to fix here. Through a bunch of hacky heuristics I think the
dpu_hw_ctl_trigger_flush_v1() is the function that "causes" the
corruption. Specifically I managed to do something like:

if (hacky_heuristic)
  pr_info("About to call flush\n);
  mdelay(2000);
}
ctl->ops.trigger_flush(ctl)
if (hacky_heuristic)
  pr_info("Finished calling flush\n);
  mdelay(2000);
  pr_info("Finished calling flush delay done\n);
}

I then watched my display and reproduced the problem. When I saw the
problem I looked over at the console and saw "Finished calling flush"
was the last thing printed.

I don't know if this helps much. Presumably we're flushing a bunch of
previous operations so whatever we had queued up probably matters
more, but maybe it'll give a clue?


Other notes FWIW:

* If you increase the amount of time of the glitching, you can
actually see that we are glitching both the internal and external
displays.

* You can actually make the glitch stay on the screen "permanently" by
unplugging the external display while the internal screen is off. I
don't know why it doesn't always happen, but it seems to happen often
enough. Presumably if someone knew the display controller well they
could try to figure out what state it was in and debug the problem.


-Doug

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Doug Anderson]

Hi,

On Sat, Jun 25, 2022 at 1:48 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Sat, 25 Jun 2022 at 04:23, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> > On 6/24/2022 5:11 PM, Dmitry Baryshkov wrote:
> > > On Sat, 25 Jun 2022 at 03:03, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> > >> On 6/24/2022 4:56 PM, Kuogee Hsieh wrote:
> > >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> > >> b/drivers/gpu/drm/msm/dp/dp_display.c
> > >> index dcd80c8a794c..7816e82452ca 100644
> > >> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > >> @@ -140,8 +140,8 @@ static const struct msm_dp_config sc7180_dp_cfg = {
> > >>
> > >>    static const struct msm_dp_config sc7280_dp_cfg = {
> > >>           .descs = (const struct msm_dp_desc[]) {
> > >> -               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
> > >> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
> > >>                   [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000,
> > >> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
> > >> +               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
> > >> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
> > >>           },
> > >>           .num_descs = 2,
> > >>    };
> > >>
> > >>
> > >> The reason order is important is because  in this function below, even
> > >> though it matches the address to find which one to use it loops through
> > >> the array and so the value of *id will change depending on which one is
> > >> located where.
> > >>
> > >> static const struct msm_dp_desc *dp_display_get_desc(struct
> > >> platform_device *pdev,
> > >>                                unsigned int *id)
> > >> {
> > >>       const struct msm_dp_config *cfg = of_device_get_match_data(&pdev->dev);
> > >>       struct resource *res;
> > >>       int i;
> > >>
> > >>       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > >>       if (!res)
> > >>           return NULL;
> > >>
> > >>       for (i = 0; i < cfg->num_descs; i++) {
> > >>           if (cfg->descs[i].io_start == res->start) {
> > >>               *id = i;
> > >
> > > The id is set to the index of the corresponding DP instance in the
> > > descs array, which is MSM_DP_CONTROLLER_n. Correct up to now.
> >
> > Right, this is where I misunderstood his explanation.
> >
> > Even if we swap the order, but retain the index correctly it will still
> > work today.
> >
> > Hes not sure of the root-cause of why turning on the primary display
> > first fixes the issue.
> >
> > I think till we root-cause that, lets put this on hold.
>
> Agreed. Let's find the root cause.

FWIW, I was poking a little bit about the glitch that Kuogee was
trying to fix here. Through a bunch of hacky heuristics I think the
dpu_hw_ctl_trigger_flush_v1() is the function that "causes" the
corruption. Specifically I managed to do something like:

if (hacky_heuristic)
  pr_info("About to call flush\n);
  mdelay(2000);
}
ctl->ops.trigger_flush(ctl)
if (hacky_heuristic)
  pr_info("Finished calling flush\n);
  mdelay(2000);
  pr_info("Finished calling flush delay done\n);
}

I then watched my display and reproduced the problem. When I saw the
problem I looked over at the console and saw "Finished calling flush"
was the last thing printed.

I don't know if this helps much. Presumably we're flushing a bunch of
previous operations so whatever we had queued up probably matters
more, but maybe it'll give a clue?


Other notes FWIW:

* If you increase the amount of time of the glitching, you can
actually see that we are glitching both the internal and external
displays.

* You can actually make the glitch stay on the screen "permanently" by
unplugging the external display while the internal screen is off. I
don't know why it doesn't always happen, but it seems to happen often
enough. Presumably if someone knew the display controller well they
could try to figure out what state it was in and debug the problem.


-Doug

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Kuogee Hsieh]

On 6/27/2022 4:20 PM, Doug Anderson wrote:
> Hi,
>
> On Sat, Jun 25, 2022 at 1:48 AM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
>> On Sat, 25 Jun 2022 at 04:23, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>> On 6/24/2022 5:11 PM, Dmitry Baryshkov wrote:
>>>> On Sat, 25 Jun 2022 at 03:03, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>> On 6/24/2022 4:56 PM, Kuogee Hsieh wrote:
>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>>>>> b/drivers/gpu/drm/msm/dp/dp_display.c
>>>>> index dcd80c8a794c..7816e82452ca 100644
>>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>>>> @@ -140,8 +140,8 @@ static const struct msm_dp_config sc7180_dp_cfg = {
>>>>>
>>>>>     static const struct msm_dp_config sc7280_dp_cfg = {
>>>>>            .descs = (const struct msm_dp_desc[]) {
>>>>> -               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
>>>>>                    [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000,
>>>>> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>>>>> +               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
>>>>>            },
>>>>>            .num_descs = 2,
>>>>>     };
>>>>>
>>>>>
>>>>> The reason order is important is because  in this function below, even
>>>>> though it matches the address to find which one to use it loops through
>>>>> the array and so the value of *id will change depending on which one is
>>>>> located where.
>>>>>
>>>>> static const struct msm_dp_desc *dp_display_get_desc(struct
>>>>> platform_device *pdev,
>>>>>                                 unsigned int *id)
>>>>> {
>>>>>        const struct msm_dp_config *cfg = of_device_get_match_data(&pdev->dev);
>>>>>        struct resource *res;
>>>>>        int i;
>>>>>
>>>>>        res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>>>        if (!res)
>>>>>            return NULL;
>>>>>
>>>>>        for (i = 0; i < cfg->num_descs; i++) {
>>>>>            if (cfg->descs[i].io_start == res->start) {
>>>>>                *id = i;
>>>> The id is set to the index of the corresponding DP instance in the
>>>> descs array, which is MSM_DP_CONTROLLER_n. Correct up to now.
>>> Right, this is where I misunderstood his explanation.
>>>
>>> Even if we swap the order, but retain the index correctly it will still
>>> work today.
>>>
>>> Hes not sure of the root-cause of why turning on the primary display
>>> first fixes the issue.
>>>
>>> I think till we root-cause that, lets put this on hold.
>> Agreed. Let's find the root cause.
> FWIW, I was poking a little bit about the glitch that Kuogee was
> trying to fix here. Through a bunch of hacky heuristics I think the
> dpu_hw_ctl_trigger_flush_v1() is the function that "causes" the
> corruption. Specifically I managed to do something like:
>
> if (hacky_heuristic)
>    pr_info("About to call flush\n);
>    mdelay(2000);
> }
> ctl->ops.trigger_flush(ctl)
> if (hacky_heuristic)
>    pr_info("Finished calling flush\n);
>    mdelay(2000);
>    pr_info("Finished calling flush delay done\n);
> }

flush bit need to up update at real time.

otherwise unexpected side effects will happen.

i try same thing, but I got fence timeout error.

Anyway, I had submit new patch to fix corruption issue.

Thanks for your efforts and helps.

> I then watched my display and reproduced the problem. When I saw the
> problem I looked over at the console and saw "Finished calling flush"
> was the last thing printed.
>
> I don't know if this helps much. Presumably we're flushing a bunch of
> previous operations so whatever we had queued up probably matters
> more, but maybe it'll give a clue?
>
>
> Other notes FWIW:
>
> * If you increase the amount of time of the glitching, you can
> actually see that we are glitching both the internal and external
> displays.
>
> * You can actually make the glitch stay on the screen "permanently" by
> unplugging the external display while the internal screen is off. I
> don't know why it doesn't always happen, but it seems to happen often
> enough. Presumably if someone knew the display controller well they
> could try to figure out what state it was in and debug the problem.
>
>
> -Doug

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Kuogee Hsieh]

On 6/27/2022 4:20 PM, Doug Anderson wrote:
> Hi,
>
> On Sat, Jun 25, 2022 at 1:48 AM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
>> On Sat, 25 Jun 2022 at 04:23, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>> On 6/24/2022 5:11 PM, Dmitry Baryshkov wrote:
>>>> On Sat, 25 Jun 2022 at 03:03, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>> On 6/24/2022 4:56 PM, Kuogee Hsieh wrote:
>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>>>>> b/drivers/gpu/drm/msm/dp/dp_display.c
>>>>> index dcd80c8a794c..7816e82452ca 100644
>>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>>>> @@ -140,8 +140,8 @@ static const struct msm_dp_config sc7180_dp_cfg = {
>>>>>
>>>>>     static const struct msm_dp_config sc7280_dp_cfg = {
>>>>>            .descs = (const struct msm_dp_desc[]) {
>>>>> -               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
>>>>>                    [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000,
>>>>> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>>>>> +               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
>>>>>            },
>>>>>            .num_descs = 2,
>>>>>     };
>>>>>
>>>>>
>>>>> The reason order is important is because  in this function below, even
>>>>> though it matches the address to find which one to use it loops through
>>>>> the array and so the value of *id will change depending on which one is
>>>>> located where.
>>>>>
>>>>> static const struct msm_dp_desc *dp_display_get_desc(struct
>>>>> platform_device *pdev,
>>>>>                                 unsigned int *id)
>>>>> {
>>>>>        const struct msm_dp_config *cfg = of_device_get_match_data(&pdev->dev);
>>>>>        struct resource *res;
>>>>>        int i;
>>>>>
>>>>>        res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>>>        if (!res)
>>>>>            return NULL;
>>>>>
>>>>>        for (i = 0; i < cfg->num_descs; i++) {
>>>>>            if (cfg->descs[i].io_start == res->start) {
>>>>>                *id = i;
>>>> The id is set to the index of the corresponding DP instance in the
>>>> descs array, which is MSM_DP_CONTROLLER_n. Correct up to now.
>>> Right, this is where I misunderstood his explanation.
>>>
>>> Even if we swap the order, but retain the index correctly it will still
>>> work today.
>>>
>>> Hes not sure of the root-cause of why turning on the primary display
>>> first fixes the issue.
>>>
>>> I think till we root-cause that, lets put this on hold.
>> Agreed. Let's find the root cause.
> FWIW, I was poking a little bit about the glitch that Kuogee was
> trying to fix here. Through a bunch of hacky heuristics I think the
> dpu_hw_ctl_trigger_flush_v1() is the function that "causes" the
> corruption. Specifically I managed to do something like:
>
> if (hacky_heuristic)
>    pr_info("About to call flush\n);
>    mdelay(2000);
> }
> ctl->ops.trigger_flush(ctl)
> if (hacky_heuristic)
>    pr_info("Finished calling flush\n);
>    mdelay(2000);
>    pr_info("Finished calling flush delay done\n);
> }

flush bit need to up update at real time.

otherwise unexpected side effects will happen.

i try same thing, but I got fence timeout error.

Anyway, I had submit new patch to fix corruption issue.

Thanks for your efforts and helps.

> I then watched my display and reproduced the problem. When I saw the
> problem I looked over at the console and saw "Finished calling flush"
> was the last thing printed.
>
> I don't know if this helps much. Presumably we're flushing a bunch of
> previous operations so whatever we had queued up probably matters
> more, but maybe it'll give a clue?
>
>
> Other notes FWIW:
>
> * If you increase the amount of time of the glitching, you can
> actually see that we are glitching both the internal and external
> displays.
>
> * You can actually make the glitch stay on the screen "permanently" by
> unplugging the external display while the internal screen is off. I
> don't know why it doesn't always happen, but it seems to happen often
> enough. Presumably if someone knew the display controller well they
> could try to figure out what state it was in and debug the problem.
>
>
> -Doug

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Dmitry Baryshkov]

On Sat, 25 Jun 2022 at 00:17, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
>
> On 6/24/2022 1:00 PM, Stephen Boyd wrote:
> > Quoting Kuogee Hsieh (2022-06-24 10:15:11)
> >> Current the index (dp->id) of DP descriptor table (scxxxx_dp_cfg[]) are tightly
> >> coupled with DP controller_id. This means DP use controller id 0 must be placed
> >> at first entry of DP descriptor table (scxxxx_dp_cfg[]). Otherwise the internal
> >> INTF will mismatch controller_id. This will cause controller kickoff wrong
> >> interface timing engine and cause dpu_encoder_phys_vid_wait_for_commit_done
> >> vblank timeout error.
> >>
> >> This patch add controller_id field into struct msm_dp_desc to break the tightly
> >> coupled relationship between index (dp->id) of DP descriptor table with DP
> >> controller_id.
> > Please no. This reverts the intention of commit bb3de286d992
> > ("drm/msm/dp: Support up to 3 DP controllers")
> >
> >      A new enum is introduced to document the connection between the
> >      instances referenced in the dpu_intf_cfg array and the controllers in
> >      the DP driver and sc7180 is updated.
> >
> > It sounds like the intent of that commit failed to make a strong enough
> > connection. Now it needs to match the INTF number as well? I can't
> > really figure out what is actually wrong, because this patch undoes that
> > intentional tight coupling. Is the next patch the important part that
> > flips the order of the two interfaces?
>
> The commit bb3de286d992have two problems,
>
> 1)  The below sc7280_dp_cfg will not work, if eDP use
> MSM_DP_CONTROLLER_2 instead of  MSM_DP_CONTROLLER_1
>
> since it have num_descs =2 but eDP is at index 2 (CONTROLLER_2) which
> never be reached.
>
> static const struct msm_dp_config sc7280_dp_cfg = {
>          .descs = (const struct msm_dp_desc[]) {
>                  [MSM_DP_CONTROLLER_2] = { .io_start = 0x0aea0000,
> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>                  [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
>          },
>          .num_descs = 2,

Please change num_descs to 3. Or better eliminate it completely and
iterate up to MSM_DP_CONTROLLER_MAX, checking whether the entry
contains real values or is just a zero sentinel entry.

> };
>
> 2)  DP always has index of 0 (dp->id = 0) and the first one to call
> msm_dp_modeset_init(). This make DP always place at head of bridge chain.
>
> At next patch eDP must be placed at head of bridge chain to fix eDP
> corruption issue. This is the purpose of this patch. I will revise the
> commit text.

This text doesn't make sense to me. The dp->id has nothing to do with
the bridge chains. Each dp entry is a head of the corresponding bridge
chain. DP with dp->id = 0 and eDP with dp->id = whatever will be parts
of different encoder -> bridges -> connector chains.

-- 
With best wishes
Dmitry

Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Dmitry Baryshkov]

On Sat, 25 Jun 2022 at 00:17, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
>
> On 6/24/2022 1:00 PM, Stephen Boyd wrote:
> > Quoting Kuogee Hsieh (2022-06-24 10:15:11)
> >> Current the index (dp->id) of DP descriptor table (scxxxx_dp_cfg[]) are tightly
> >> coupled with DP controller_id. This means DP use controller id 0 must be placed
> >> at first entry of DP descriptor table (scxxxx_dp_cfg[]). Otherwise the internal
> >> INTF will mismatch controller_id. This will cause controller kickoff wrong
> >> interface timing engine and cause dpu_encoder_phys_vid_wait_for_commit_done
> >> vblank timeout error.
> >>
> >> This patch add controller_id field into struct msm_dp_desc to break the tightly
> >> coupled relationship between index (dp->id) of DP descriptor table with DP
> >> controller_id.
> > Please no. This reverts the intention of commit bb3de286d992
> > ("drm/msm/dp: Support up to 3 DP controllers")
> >
> >      A new enum is introduced to document the connection between the
> >      instances referenced in the dpu_intf_cfg array and the controllers in
> >      the DP driver and sc7180 is updated.
> >
> > It sounds like the intent of that commit failed to make a strong enough
> > connection. Now it needs to match the INTF number as well? I can't
> > really figure out what is actually wrong, because this patch undoes that
> > intentional tight coupling. Is the next patch the important part that
> > flips the order of the two interfaces?
>
> The commit bb3de286d992have two problems,
>
> 1)  The below sc7280_dp_cfg will not work, if eDP use
> MSM_DP_CONTROLLER_2 instead of  MSM_DP_CONTROLLER_1
>
> since it have num_descs =2 but eDP is at index 2 (CONTROLLER_2) which
> never be reached.
>
> static const struct msm_dp_config sc7280_dp_cfg = {
>          .descs = (const struct msm_dp_desc[]) {
>                  [MSM_DP_CONTROLLER_2] = { .io_start = 0x0aea0000,
> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>                  [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
>          },
>          .num_descs = 2,

Please change num_descs to 3. Or better eliminate it completely and
iterate up to MSM_DP_CONTROLLER_MAX, checking whether the entry
contains real values or is just a zero sentinel entry.

> };
>
> 2)  DP always has index of 0 (dp->id = 0) and the first one to call
> msm_dp_modeset_init(). This make DP always place at head of bridge chain.
>
> At next patch eDP must be placed at head of bridge chain to fix eDP
> corruption issue. This is the purpose of this patch. I will revise the
> commit text.

This text doesn't make sense to me. The dp->id has nothing to do with
the bridge chains. Each dp entry is a head of the corresponding bridge
chain. DP with dp->id = 0 and eDP with dp->id = whatever will be parts
of different encoder -> bridges -> connector chains.

-- 
With best wishes
Dmitry

[PATCH v1 3/3] drm/msm/dp: place edp at head of drm bridge chain to fix screen corruption

[Kuogee Hsieh]

The msm_dp_modeset_init() is used to attach DP driver to drm bridge chain.
msm_dp_modeset_init() is executed in the order of index (dp->id) of DP
descriptor table.

Currently, DP is placed at first entry (dp->id = 0) of descriptor table
and eDP is placed at secondary entry (dp->id = 1 ) of descriptor table.
This means DP will be placed at head of bridge chain and eDP will be
placed right after DP at bridge chain.

Drm screen update is happen sequentially in the order from head to tail
of bridge chain. Therefore external DP display will have screen updated
happen before primary eDP display if external DP display presented.
This is wrong screen update order and cause one frame time screen
corruption happen at primary display during external DP plugged in.

This patch place eDP at first entry (dp->id = 0) of descriptor table and
place DP at secondary entry (dp->id = 1) to have primary eDP locate at
head of bridge chain. This correct screen update order and eliminated
the one frame time screen corruption happen d at primary display.

Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index a87a9d8..2755ff3 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -143,10 +143,10 @@ static const struct msm_dp_config sc7180_dp_cfg = {
 
 static const struct msm_dp_config sc7280_dp_cfg = {
 	.descs = (const struct msm_dp_desc[]) {
-		{ .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort,
-		.controller_id = MSM_DP_CONTROLLER_0, .wide_bus_en = true },
 		{ .io_start = 0x0aea0000, .connector_type = DRM_MODE_CONNECTOR_eDP, 
 		.controller_id = MSM_DP_CONTROLLER_1, .wide_bus_en = true },
+		{ .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort,
+		.controller_id = MSM_DP_CONTROLLER_0, .wide_bus_en = true },
 	},
 	.num_descs = 2,
 };
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v1 3/3] drm/msm/dp: place edp at head of drm bridge chain to fix screen corruption

[Kuogee Hsieh]

The msm_dp_modeset_init() is used to attach DP driver to drm bridge chain.
msm_dp_modeset_init() is executed in the order of index (dp->id) of DP
descriptor table.

Currently, DP is placed at first entry (dp->id = 0) of descriptor table
and eDP is placed at secondary entry (dp->id = 1 ) of descriptor table.
This means DP will be placed at head of bridge chain and eDP will be
placed right after DP at bridge chain.

Drm screen update is happen sequentially in the order from head to tail
of bridge chain. Therefore external DP display will have screen updated
happen before primary eDP display if external DP display presented.
This is wrong screen update order and cause one frame time screen
corruption happen at primary display during external DP plugged in.

This patch place eDP at first entry (dp->id = 0) of descriptor table and
place DP at secondary entry (dp->id = 1) to have primary eDP locate at
head of bridge chain. This correct screen update order and eliminated
the one frame time screen corruption happen d at primary display.

Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index a87a9d8..2755ff3 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -143,10 +143,10 @@ static const struct msm_dp_config sc7180_dp_cfg = {
 
 static const struct msm_dp_config sc7280_dp_cfg = {
 	.descs = (const struct msm_dp_desc[]) {
-		{ .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort,
-		.controller_id = MSM_DP_CONTROLLER_0, .wide_bus_en = true },
 		{ .io_start = 0x0aea0000, .connector_type = DRM_MODE_CONNECTOR_eDP, 
 		.controller_id = MSM_DP_CONTROLLER_1, .wide_bus_en = true },
+		{ .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort,
+		.controller_id = MSM_DP_CONTROLLER_0, .wide_bus_en = true },
 	},
 	.num_descs = 2,
 };
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v1 3/3] drm/msm/dp: place edp at head of drm bridge chain to fix screen corruption

[Dmitry Baryshkov]

On Fri, 24 Jun 2022 at 20:15, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
> The msm_dp_modeset_init() is used to attach DP driver to drm bridge chain.
> msm_dp_modeset_init() is executed in the order of index (dp->id) of DP
> descriptor table.
>
> Currently, DP is placed at first entry (dp->id = 0) of descriptor table
> and eDP is placed at secondary entry (dp->id = 1 ) of descriptor table.
> This means DP will be placed at head of bridge chain and eDP will be
> placed right after DP at bridge chain.

No, the dp->ids do not have anything to do with the bridge chains.

> Drm screen update is happen sequentially in the order from head to tail
> of bridge chain. Therefore external DP display will have screen updated
> happen before primary eDP display if external DP display presented.
> This is wrong screen update order and cause one frame time screen
> corruption happen at primary display during external DP plugged in.
>
> This patch place eDP at first entry (dp->id = 0) of descriptor table and
> place DP at secondary entry (dp->id = 1) to have primary eDP locate at
> head of bridge chain. This correct screen update order and eliminated
> the one frame time screen corruption happen d at primary display.
>
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>  drivers/gpu/drm/msm/dp/dp_display.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index a87a9d8..2755ff3 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -143,10 +143,10 @@ static const struct msm_dp_config sc7180_dp_cfg = {
>
>  static const struct msm_dp_config sc7280_dp_cfg = {
>         .descs = (const struct msm_dp_desc[]) {
> -               { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort,
> -               .controller_id = MSM_DP_CONTROLLER_0, .wide_bus_en = true },
>                 { .io_start = 0x0aea0000, .connector_type = DRM_MODE_CONNECTOR_eDP,
>                 .controller_id = MSM_DP_CONTROLLER_1, .wide_bus_en = true },
> +               { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort,
> +               .controller_id = MSM_DP_CONTROLLER_0, .wide_bus_en = true },

If the correctness of DP display depends on the order of entries in
the dp_desc table, something is terribly wrong in the driver. Please
fix that rather than working around it by shuffling the entries in the
array.

>         },
>         .num_descs = 2,
>  };
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>


-- 
With best wishes
Dmitry

Re: [PATCH v1 3/3] drm/msm/dp: place edp at head of drm bridge chain to fix screen corruption

[Dmitry Baryshkov]

On Fri, 24 Jun 2022 at 20:15, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
> The msm_dp_modeset_init() is used to attach DP driver to drm bridge chain.
> msm_dp_modeset_init() is executed in the order of index (dp->id) of DP
> descriptor table.
>
> Currently, DP is placed at first entry (dp->id = 0) of descriptor table
> and eDP is placed at secondary entry (dp->id = 1 ) of descriptor table.
> This means DP will be placed at head of bridge chain and eDP will be
> placed right after DP at bridge chain.

No, the dp->ids do not have anything to do with the bridge chains.

> Drm screen update is happen sequentially in the order from head to tail
> of bridge chain. Therefore external DP display will have screen updated
> happen before primary eDP display if external DP display presented.
> This is wrong screen update order and cause one frame time screen
> corruption happen at primary display during external DP plugged in.
>
> This patch place eDP at first entry (dp->id = 0) of descriptor table and
> place DP at secondary entry (dp->id = 1) to have primary eDP locate at
> head of bridge chain. This correct screen update order and eliminated
> the one frame time screen corruption happen d at primary display.
>
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>  drivers/gpu/drm/msm/dp/dp_display.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index a87a9d8..2755ff3 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -143,10 +143,10 @@ static const struct msm_dp_config sc7180_dp_cfg = {
>
>  static const struct msm_dp_config sc7280_dp_cfg = {
>         .descs = (const struct msm_dp_desc[]) {
> -               { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort,
> -               .controller_id = MSM_DP_CONTROLLER_0, .wide_bus_en = true },
>                 { .io_start = 0x0aea0000, .connector_type = DRM_MODE_CONNECTOR_eDP,
>                 .controller_id = MSM_DP_CONTROLLER_1, .wide_bus_en = true },
> +               { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort,
> +               .controller_id = MSM_DP_CONTROLLER_0, .wide_bus_en = true },

If the correctness of DP display depends on the order of entries in
the dp_desc table, something is terribly wrong in the driver. Please
fix that rather than working around it by shuffling the entries in the
array.

>         },
>         .num_descs = 2,
>  };
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>


-- 
With best wishes
Dmitry