Re: [PATCH v4 4/4] drm/msm/dp: dp_link_parse_sink_count() return immediately if aux read failed

[khsieh]

On 2021-11-24 23:32, Dmitry Baryshkov wrote:

On 04/05/2021 07:35, Stephen Boyd wrote:

Quoting Kuogee Hsieh (2021-04-21 16:37:38)

Add checking aux read/write status at both dp_link_parse_sink_count()
and dp_link_parse_sink_status_filed() to avoid long timeout delay if


s/filed/field/


dp aux read/write failed at timeout due to cable unplugged.

Changes in V4:
-- split this patch as stand alone patch

Signed-off-by: Kuogee Hsieh 


Can this patch come before the one previously? And then some fixes tag
be added? Otherwise looks good to me.

Reviewed-by: Stephen Boyd 
Tested-by: Stephen Boyd 


Is this something that we still need to pursue/merge?

There were changes requested for this and for the previous patch, but
no new versions were posted.


It is my fault to miss this one.
The first two patches of this serial are merged.
I will rebase and re submit this one to V5.10.

Re: [PATCH v4] drm/msm/dp: do not initialize phy until plugin interrupt received

[khsieh]

On 2021-11-09 13:38, Kuogee Hsieh wrote:

From: Kuogee Hsieh 

Current DP drivers have regulators, clocks, irq and phy are grouped
together within a function and executed not in a symmetric manner.
This increase difficulty of code maintenance and limited code 
scalability.

This patch divided the driver life cycle of operation into four states,
resume (including booting up), dongle plugin, dongle unplugged and 
suspend.
Regulators, core clocks and irq are grouped together and enabled at 
resume
(or booting up) so that the DP controller is armed and ready to receive 
HPD
plugin interrupts. HPD plugin interrupt is generated when a dongle 
plugs

into DUT (device under test). Once HPD plugin interrupt is received, DP
controller will initialize phy so that dpcd read/write will function 
and

following link training can be proceeded successfully. DP phy will be
disabled after main link is teared down at end of unplugged HPD 
interrupt
handle triggered by dongle unplugged out of DUT. Finally regulators, 
code

clocks and irq are disabled at corresponding suspension.

Changes in V2:
-- removed unnecessary dp_ctrl NULL check
-- removed unnecessary phy init_count and power_count DRM_DEBUG_DP logs
-- remove flip parameter out of dp_ctrl_irq_enable()
-- add fixes tag

Changes in V3:
-- call dp_display_host_phy_init() instead of dp_ctrl_phy_init() at
dp_display_host_init() for eDP

Changes in V4:
-- rewording commit text to match this commit changes

Fixes: e91e3065a806 ("drm/msm/dp: Add DP compliance tests on
Snapdragon Chipsets")

Signed-off-by: Kuogee Hsieh 
---


any more comments?

 drivers/gpu/drm/msm/dp/dp_ctrl.c| 87 
-

 drivers/gpu/drm/msm/dp/dp_ctrl.h|  9 ++--
 drivers/gpu/drm/msm/dp/dp_display.c | 83 
++-

 3 files changed, 105 insertions(+), 74 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c 
b/drivers/gpu/drm/msm/dp/dp_ctrl.c

index 7ec155d..4788e8c 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1364,60 +1364,54 @@ static int dp_ctrl_enable_stream_clocks(struct
dp_ctrl_private *ctrl)
return ret;
 }

-int dp_ctrl_host_init(struct dp_ctrl *dp_ctrl, bool flip, bool reset)
+void dp_ctrl_irq_enable(struct dp_ctrl *dp_ctrl)
+{
+   struct dp_ctrl_private *ctrl;
+
+   ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
+
+   dp_catalog_ctrl_reset(ctrl->catalog);
+
+   dp_catalog_ctrl_enable_irq(ctrl->catalog, true);
+}
+
+void dp_ctrl_irq_disable(struct dp_ctrl *dp_ctrl)
+{
+   struct dp_ctrl_private *ctrl;
+
+   ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
+
+   dp_catalog_ctrl_reset(ctrl->catalog);
+
+   dp_catalog_ctrl_enable_irq(ctrl->catalog, false);
+}
+
+void dp_ctrl_phy_init(struct dp_ctrl *dp_ctrl)
 {
struct dp_ctrl_private *ctrl;
struct dp_io *dp_io;
struct phy *phy;

-   if (!dp_ctrl) {
-   DRM_ERROR("Invalid input data\n");
-   return -EINVAL;
-   }
-
ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
dp_io = >parser->io;
phy = dp_io->phy;

-   ctrl->dp_ctrl.orientation = flip;
-
-   if (reset)
-   dp_catalog_ctrl_reset(ctrl->catalog);
-
-   DRM_DEBUG_DP("flip=%d\n", flip);
dp_catalog_ctrl_phy_reset(ctrl->catalog);
phy_init(phy);
-   dp_catalog_ctrl_enable_irq(ctrl->catalog, true);
-
-   return 0;
 }

-/**
- * dp_ctrl_host_deinit() - Uninitialize DP controller
- * @dp_ctrl: Display Port Driver data
- *
- * Perform required steps to uninitialize DP controller
- * and its resources.
- */
-void dp_ctrl_host_deinit(struct dp_ctrl *dp_ctrl)
+void dp_ctrl_phy_exit(struct dp_ctrl *dp_ctrl)
 {
struct dp_ctrl_private *ctrl;
struct dp_io *dp_io;
struct phy *phy;

-   if (!dp_ctrl) {
-   DRM_ERROR("Invalid input data\n");
-   return;
-   }
-
ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
dp_io = >parser->io;
phy = dp_io->phy;

-   dp_catalog_ctrl_enable_irq(ctrl->catalog, false);
+   dp_catalog_ctrl_phy_reset(ctrl->catalog);
phy_exit(phy);
-
-   DRM_DEBUG_DP("Host deinitialized successfully\n");
 }

 static bool dp_ctrl_use_fixed_nvid(struct dp_ctrl_private *ctrl)
@@ -1895,8 +1889,14 @@ int dp_ctrl_off_link_stream(struct dp_ctrl 
*dp_ctrl)

return ret;
}

+   DRM_DEBUG_DP("Before, phy=%x init_count=%d power_on=%d\n",
+   (u32)(uintptr_t)phy, phy->init_count, phy->power_count);
+
phy_power_off(phy);

+   DRM_DEBUG_DP("After, phy=%x init_count=%d power_on=%d\n",
+   (u32)(uintptr_t)phy, phy->init_count, phy->power_count);
+
/* aux channel down, reinit phy */
phy_exit(phy);
phy_init(phy);
@@ -1905,23 +1905,6 @@ int dp_ctrl_off_link_stream(struct dp_ctrl 
*dp_ctrl)

Re: [PATCH] drm/msm/dp: Avoid unpowered AUX xfers that caused crashes

[khsieh]

On 2021-11-09 10:04, Douglas Anderson wrote:

If you happened to try to access `/dev/drm_dp_aux` devices provided by
the MSM DP AUX driver too early at bootup you could go boom. Let's
avoid that by only allowing AUX transfers when the controller is
powered up.

Specifically the crash that was seen (on Chrome OS 5.4 tree with
relevant backports):
  Kernel panic - not syncing: Asynchronous SError Interrupt
  CPU: 0 PID: 3131 Comm: fwupd Not tainted 5.4.144-16620-g28af11b73efb 
#1

  Hardware name: Google Lazor (rev3+) with KB Backlight (DT)
  Call trace:
   dump_backtrace+0x0/0x14c
   show_stack+0x20/0x2c
   dump_stack+0xac/0x124
   panic+0x150/0x390
   nmi_panic+0x80/0x94
   arm64_serror_panic+0x78/0x84
   do_serror+0x0/0x118
   do_serror+0xa4/0x118
   el1_error+0xbc/0x160
   dp_catalog_aux_write_data+0x1c/0x3c
   dp_aux_cmd_fifo_tx+0xf0/0x1b0
   dp_aux_transfer+0x1b0/0x2bc
   drm_dp_dpcd_access+0x8c/0x11c
   drm_dp_dpcd_read+0x64/0x10c
   auxdev_read_iter+0xd4/0x1c4

I did a little bit of tracing and found that:
* We register the AUX device very early at bootup.
* Power isn't actually turned on for my system until
  hpd_event_thread() -> dp_display_host_init() -> dp_power_init()
* You can see that dp_power_init() calls dp_aux_init() which is where
  we start allowing AUX channel requests to go through.

In general this patch is a bit of a bandaid but at least it gets us
out of the current state where userspace acting at the wrong time can
fully crash the system.
* I think the more proper fix (which requires quite a bit more
  changes) is to power stuff on while an AUX transfer is
  happening. This is like the solution we did for ti-sn65dsi86. This
  might be required for us to move to populating the panel via the
  DP-AUX bus.
* Another fix considered was to dynamically register / unregister. I
  tried that at  but it got
  ugly. Currently there's a bug where the pm_runtime() state isn't
  tracked properly and that causes us to just keep registering more
  and more.

Signed-off-by: Douglas Anderson 
---

Reviewed-by: Kuogee Hsieh 


 drivers/gpu/drm/msm/dp/dp_aux.c | 17 +
 1 file changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
b/drivers/gpu/drm/msm/dp/dp_aux.c

index eb40d8413bca..6d36f63c3338 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -33,6 +33,7 @@ struct dp_aux_private {
bool read;
bool no_send_addr;
bool no_send_stop;
+   bool initted;
u32 offset;
u32 segment;

@@ -331,6 +332,10 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux 
*dp_aux,

}

mutex_lock(>mutex);
+   if (!aux->initted) {
+   ret = -EIO;
+   goto exit;
+   }

dp_aux_update_offset_and_segment(aux, msg);
dp_aux_transfer_helper(aux, msg, true);
@@ -380,6 +385,8 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux 
*dp_aux,

}

aux->cmd_busy = false;
+
+exit:
mutex_unlock(>mutex);

return ret;
@@ -431,8 +438,13 @@ void dp_aux_init(struct drm_dp_aux *dp_aux)

aux = container_of(dp_aux, struct dp_aux_private, dp_aux);

+   mutex_lock(>mutex);
+
dp_catalog_aux_enable(aux->catalog, true);
aux->retry_cnt = 0;
+   aux->initted = true;
+
+   mutex_unlock(>mutex);
 }

 void dp_aux_deinit(struct drm_dp_aux *dp_aux)
@@ -441,7 +453,12 @@ void dp_aux_deinit(struct drm_dp_aux *dp_aux)

aux = container_of(dp_aux, struct dp_aux_private, dp_aux);

+   mutex_lock(>mutex);
+
+   aux->initted = false;
dp_catalog_aux_enable(aux->catalog, false);
+
+   mutex_unlock(>mutex);
 }

 int dp_aux_register(struct drm_dp_aux *dp_aux)

Re: [PATCH v3 6/6] drm/msm/dp: Remove the hpd init delay for eDP

[khsieh]

On 2021-10-27 23:38, Stephen Boyd wrote:

Quoting Sankeerth Billakanti (2021-10-27 18:54:48)

DP driver needs a 10 second delay before phy_init so that
the usb combo phy initializes and sets up the necessary
clocks for usb devices such as keyboard and mouse.

eDP controller uses a standalone phy and need not wait for
phy initialization from any other component. This change
will remove the delay for eDP controller.

Signed-off-by: Sankeerth Billakanti 
---
 drivers/gpu/drm/msm/dp/dp_display.c | 11 ++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c

index 61385d6..de6a1fd 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1438,7 +1439,15 @@ void msm_dp_irq_postinstall(struct msm_dp 
*dp_display)


dp_hpd_event_setup(dp);

-   dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
+   if (dp->dp_display.connector_type == DRM_MODE_CONNECTOR_eDP) {
+   /* eDP does not need any delay before phy init */
+   delay = 0;
+   } else {
+   /* DP needs 10 second delay to let usb combo phy 
initialize */


This seems to be a different approach to the patch Kuogee sent a week 
or

two ago. Can we figure out what's wrong with the DP phy starting before
the USB phy? I suppose this patch is OK as a temporary hack to keep
moving with eDP, but we really need to figure out what's wrong with DP
so this delay can be removed entirely. Has any progress been made on
that?


Sankeerth,
Can you drop this patch for now.
Let's discuss more.


+   delay = 100;
+   }
+
+   dp_add_event(dp, EV_HPD_INIT_SETUP, 0, delay);

Re: [PATCH v2 4/4] arm64: dts: qcom: sc7280: add edp display dt nodes

[khsieh]

On 2021-10-21 11:44, Stephen Boyd wrote:

Quoting Krishna Manikandan (2021-10-20 06:58:53)

From: Sankeerth Billakanti 

Add edp controller and phy DT nodes for sc7280.

Signed-off-by: Sankeerth Billakanti 
Signed-off-by: Krishna Manikandan 



Some comments below

Reviewed-by: Stephen Boyd 



Changes in v2:
- Move regulator definitions to board file (Matthias Kaehlcke)
- Move the gpio definitions to board file (Matthias Kaehlcke)
- Move the pinconf to board file (Matthias Kaehlcke)
- Move status property (Stephen Boyd)
- Drop flags from interrupts (Stephen Boyd)
- Add clock names one per line for readability (Stephen Boyd)
- Rename edp-opp-table (Stephen Boyd)
---
 arch/arm64/boot/dts/qcom/sc7280.dtsi | 107 
++-

 1 file changed, 106 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi 
b/arch/arm64/boot/dts/qcom/sc7280.dtsi

index dd35882..4450277 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -2575,7 +2575,7 @@
reg = <0 0xaf0 0 0x2>;
clocks = < RPMH_CXO_CLK>,
 < GCC_DISP_GPLL0_CLK_SRC>,
-<0>, <0>, <0>, <0>, <0>, <0>;
+<0>, <0>, <0>, <0>, <_phy 0>, 
<_phy 1>;


I can already tell this is going to be a merge mess! Can this also be
one cell per line?


 where are dsi phy? (<_phy 0>, <_phy 1>)


clock-names = "bi_tcxo", "gcc_disp_gpll0_clk",
  "dsi0_phy_pll_out_byteclk",
  "dsi0_phy_pll_out_dsiclk",
@@ -2777,6 +2784,103 @@

status = "disabled";
};
+
+   msm_edp: edp@aea {
+   compatible = "qcom,sc7280-edp";
+
+   reg = <0 0xaea 0 0x200>,
+ <0 0xaea0200 0 0x200>,
+ <0 0xaea0400 0 0xc00>,
+ <0 0xaea1000 0 0x400>;
+
+   interrupt-parent = <>;
+   interrupts = <14>;
+
+   clocks = < RPMH_CXO_CLK>,
+< GCC_EDP_CLKREF_EN>,
+< 
DISP_CC_MDSS_AHB_CLK>,
+< 
DISP_CC_MDSS_EDP_AUX_CLK>,
+< 
DISP_CC_MDSS_EDP_LINK_CLK>,
+< 
DISP_CC_MDSS_EDP_LINK_INTF_CLK>,
+< 
DISP_CC_MDSS_EDP_PIXEL_CLK>;

+   clock-names = "core_xo",
+ "core_ref",
+ "core_iface",
+ "core_aux",
+ "ctrl_link",
+ "ctrl_link_iface",
+ "stream_pixel";
+   #clock-cells = <1>;
+   assigned-clocks = < 
DISP_CC_MDSS_EDP_LINK_CLK_SRC>,
+ < 
DISP_CC_MDSS_EDP_PIXEL_CLK_SRC>;
+   assigned-clock-parents = <_phy 0>, 
<_phy 1>;

+
+   phys = <_phy>;
+   phy-names = "dp";
+
+   operating-points-v2 = 
<_opp_table>;

+   power-domains = < SC7280_CX>;
+
+
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   status = "disabled";
+
+   ports {
+   #address-cells = <1>;
+   #size-cells = <0>;
+   port@0 {
+   reg = <0>;
+   edp_in: endpoint {
+   
remote-endpoint = <_intf5_out>;

+   };
+   };
+   };
+
+   edp_opp_table: opp-table {
+   compatible = 
"operating-points-v2";

+
+   opp-16000 {
+   opp-hz = /bits/ 64 
<16000>;
+   required-opps = 
<_opp_low_svs>;

+   };
+
+   opp-27000 {
+   opp-hz = /bits/ 64 
<27000>;
+   

Re: [PATCH] drm/msm/dp: Shorten SETUP timeout

[khsieh]

On 2021-10-08 09:44, Bjorn Andersson wrote:

On Fri 08 Oct 09:07 PDT 2021, khs...@codeaurora.org wrote:


On 2021-10-07 15:34, Stephen Boyd wrote:
> Quoting khs...@codeaurora.org (2021-10-07 13:28:12)
> > On 2021-10-07 13:06, Bjorn Andersson wrote:
> > > On Thu 07 Oct 12:51 PDT 2021, khs...@codeaurora.org wrote:
> > >
> > >> On 2021-10-06 10:31, Bjorn Andersson wrote:
> > >> > On Wed 06 Oct 08:37 PDT 2021, khs...@codeaurora.org wrote:
> > >> >
> > >> > > On 2021-10-05 19:10, Bjorn Andersson wrote:
> > >> > > > On Tue 05 Oct 16:04 PDT 2021, khs...@codeaurora.org wrote:
> > >> > > >
> > >> > > > > On 2021-10-05 15:36, Stephen Boyd wrote:
> > >> > > > > > Quoting Bjorn Andersson (2021-10-05 14:40:38)
> > >> > > > > > > On Tue 05 Oct 11:45 PDT 2021, Stephen Boyd wrote:
> > >> > > > > > >
> > >> > > > > > > > Quoting Bjorn Andersson (2021-10-04 19:37:50)
> > >> > > > > > > > > Found in the middle of a patch from Sankeerth was the 
reduction of the
> > >> > > > > > > > > INIT_SETUP timeout from 10s to 100ms. Upon INIT_SETUP 
timeout the host
> > >> > > > > > > > > is initalized and HPD interrupt start to be serviced, so 
in the case of
> > >> > > > > > > > > eDP this reduction improves the user experience 
dramatically - i.e.
> > >> > > > > > > > > removes 9.9s of bland screen time at boot.
> > >> > > > > > > > >
> > >> > > > > > > > > Suggested-by: Sankeerth Billakanti 

> > >> > > > > > > > > Signed-off-by: Bjorn Andersson 

> > >> > > > > > > > > ---
> > >> > > > > > > >
> > >> > > > > > > > Any Fixes tag? BTW, the delay design is pretty convoluted. 
I had to go
> > >> > > > > > > > re-read the code a couple times to understand that it's 
waiting 100ms
> > >> > > > > > > > times the 'delay' number. What?
> > >> > > > > > > >
> > >> > > > > > >
> > >> > > > > > > I assume you're happy with the current 10s delay on the 
current
> > >> > > > > > > devices, so I don't think we should push for this to be 
backported.
> > >> > > > > > > I have no need for it to be backported on my side at least.
> > >> > > > > > >
> > >> > > > > >
> > >> > > > > > Sure. Fixes tag != backported to stable trees but it is close.
> > >> > > > > >
> > >> > > > > > > > Reviewed-by: Stephen Boyd 
> > >> > > > > > >
> > >> > > > >   dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 1); <== to 100ms
> > >> > > > >
> > >> > > > > This patch will prevent usb3 from working due to dp driver
> > >> > > > > initialize phy
> > >> > > > > earlier than usb3 which cause timeout error at power up usb3 phy
> > >> > > > > when both
> > >> > > > > edp and dp are enabled.
> > >> > > >
> > >> > > > Can you please help me understand what you mean here, I use this 
on my
> > >> > > > sc8180x with both eDP and USB-C/DP right now. What is it that 
doesn't
> > >> > > > work? Or am I just lucky in some race condition?
> > >> > > >
> > >> > > > Thanks,
> > >> > > > Bjorn
> > >> > > >
> > >> > > The problem is seen at sc7280.
> > >> > > Apple dongle have both  hdmi and usb port.
> > >> > > plug Apple dongle into type-c, then plug DP into apple's hdmi port
> > >> > > and usb
> > >> > > mouse into apple's usb port.
> > >> > > If edp enabled at this time, then usb mouse will not work due to
> > >> > > timeout at
> > >> > > phy power up.
> > >> > >
> > >> >
> > >> > Okay, so you're saying that if the DP driver invokes phy_power_on()
> > >> > before the USB driver does, USB initialization fails (or at least USB
> > >> > doesn't work)?
> > >>
> > >> if dp driver call qcom_qmp_phy_init() before usb3 call
> > >> qcom_qmp_phy_init(),
> > >> usb3 driver will timeout at readl_poll_timeout(status, val, (val &
> > >> mask) ==
> > >> ready, 10, PHY_INIT_COMPLETE_TIMEOUT) of qcom_qmp_phy_power_on().
> > >
> > > Thanks, I will try to reproduce this on my side. So the 10 seconds here
> > > is strictly to give good enough time for the dwc3 driver to probe...
> > >
> > > Any idea why you're saying that this is specific to sc7280, what
> > > changed
> > > from sc7180?
> >
> > I did not have sc7180 with edp before so that i am not sure it will
> > happen on sc7180 or not.
> > The usb3 does not work when both edp and dp enabled I just seen at
> > sc7280.
> > Current at sc7280 EC is not boot up correctly when system power up.
> > I have to manual reboot EC from linux kernel shell before DP/usb3 can
> > work.
> > I am not sure this contribute to this problem or not.
> >
>
> Can you make the usb driver into a module and only load that module
> later in boot after the DP driver calls qcom_qmp_phy_init()? That would
> be an easy way to move usb probe after DP probe and expose this problem.

we need usb calls qcom_qmp_phy_init() before dp.


Why?

I do not know the details.
But I did see below scenario,

if dp driver call qcom_qmp_phy_init() before usb3 call
qcom_qmp_phy_init(),
usb3 driver will timeout at readl_poll_timeout(status, val, (val &
mask) ==
ready, 10, PHY_INIT_COMPLETE_TIMEOUT) of qcom_qmp_phy_power_on().

Re: [PATCH] drm/msm/dp: Shorten SETUP timeout

[khsieh]

On 2021-10-07 15:34, Stephen Boyd wrote:

Quoting khs...@codeaurora.org (2021-10-07 13:28:12)

On 2021-10-07 13:06, Bjorn Andersson wrote:
> On Thu 07 Oct 12:51 PDT 2021, khs...@codeaurora.org wrote:
>
>> On 2021-10-06 10:31, Bjorn Andersson wrote:
>> > On Wed 06 Oct 08:37 PDT 2021, khs...@codeaurora.org wrote:
>> >
>> > > On 2021-10-05 19:10, Bjorn Andersson wrote:
>> > > > On Tue 05 Oct 16:04 PDT 2021, khs...@codeaurora.org wrote:
>> > > >
>> > > > > On 2021-10-05 15:36, Stephen Boyd wrote:
>> > > > > > Quoting Bjorn Andersson (2021-10-05 14:40:38)
>> > > > > > > On Tue 05 Oct 11:45 PDT 2021, Stephen Boyd wrote:
>> > > > > > >
>> > > > > > > > Quoting Bjorn Andersson (2021-10-04 19:37:50)
>> > > > > > > > > Found in the middle of a patch from Sankeerth was the 
reduction of the
>> > > > > > > > > INIT_SETUP timeout from 10s to 100ms. Upon INIT_SETUP 
timeout the host
>> > > > > > > > > is initalized and HPD interrupt start to be serviced, so in 
the case of
>> > > > > > > > > eDP this reduction improves the user experience dramatically 
- i.e.
>> > > > > > > > > removes 9.9s of bland screen time at boot.
>> > > > > > > > >
>> > > > > > > > > Suggested-by: Sankeerth Billakanti 
>> > > > > > > > > Signed-off-by: Bjorn Andersson 
>> > > > > > > > > ---
>> > > > > > > >
>> > > > > > > > Any Fixes tag? BTW, the delay design is pretty convoluted. I 
had to go
>> > > > > > > > re-read the code a couple times to understand that it's 
waiting 100ms
>> > > > > > > > times the 'delay' number. What?
>> > > > > > > >
>> > > > > > >
>> > > > > > > I assume you're happy with the current 10s delay on the current
>> > > > > > > devices, so I don't think we should push for this to be 
backported.
>> > > > > > > I have no need for it to be backported on my side at least.
>> > > > > > >
>> > > > > >
>> > > > > > Sure. Fixes tag != backported to stable trees but it is close.
>> > > > > >
>> > > > > > > > Reviewed-by: Stephen Boyd 
>> > > > > > >
>> > > > >   dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 1); <== to 100ms
>> > > > >
>> > > > > This patch will prevent usb3 from working due to dp driver
>> > > > > initialize phy
>> > > > > earlier than usb3 which cause timeout error at power up usb3 phy
>> > > > > when both
>> > > > > edp and dp are enabled.
>> > > >
>> > > > Can you please help me understand what you mean here, I use this on my
>> > > > sc8180x with both eDP and USB-C/DP right now. What is it that doesn't
>> > > > work? Or am I just lucky in some race condition?
>> > > >
>> > > > Thanks,
>> > > > Bjorn
>> > > >
>> > > The problem is seen at sc7280.
>> > > Apple dongle have both  hdmi and usb port.
>> > > plug Apple dongle into type-c, then plug DP into apple's hdmi port
>> > > and usb
>> > > mouse into apple's usb port.
>> > > If edp enabled at this time, then usb mouse will not work due to
>> > > timeout at
>> > > phy power up.
>> > >
>> >
>> > Okay, so you're saying that if the DP driver invokes phy_power_on()
>> > before the USB driver does, USB initialization fails (or at least USB
>> > doesn't work)?
>>
>> if dp driver call qcom_qmp_phy_init() before usb3 call
>> qcom_qmp_phy_init(),
>> usb3 driver will timeout at readl_poll_timeout(status, val, (val &
>> mask) ==
>> ready, 10, PHY_INIT_COMPLETE_TIMEOUT) of qcom_qmp_phy_power_on().
>
> Thanks, I will try to reproduce this on my side. So the 10 seconds here
> is strictly to give good enough time for the dwc3 driver to probe...
>
> Any idea why you're saying that this is specific to sc7280, what
> changed
> from sc7180?

I did not have sc7180 with edp before so that i am not sure it will
happen on sc7180 or not.
The usb3 does not work when both edp and dp enabled I just seen at
sc7280.
Current at sc7280 EC is not boot up correctly when system power up.
I have to manual reboot EC from linux kernel shell before DP/usb3 can
work.
I am not sure this contribute to this problem or not.



Can you make the usb driver into a module and only load that module
later in boot after the DP driver calls qcom_qmp_phy_init()? That would
be an easy way to move usb probe after DP probe and expose this 
problem.


we need usb calls qcom_qmp_phy_init() before dp.

Re: [PATCH] drm/msm/dp: Shorten SETUP timeout

[khsieh]

On 2021-10-07 13:06, Bjorn Andersson wrote:

On Thu 07 Oct 12:51 PDT 2021, khs...@codeaurora.org wrote:


On 2021-10-06 10:31, Bjorn Andersson wrote:
> On Wed 06 Oct 08:37 PDT 2021, khs...@codeaurora.org wrote:
>
> > On 2021-10-05 19:10, Bjorn Andersson wrote:
> > > On Tue 05 Oct 16:04 PDT 2021, khs...@codeaurora.org wrote:
> > >
> > > > On 2021-10-05 15:36, Stephen Boyd wrote:
> > > > > Quoting Bjorn Andersson (2021-10-05 14:40:38)
> > > > > > On Tue 05 Oct 11:45 PDT 2021, Stephen Boyd wrote:
> > > > > >
> > > > > > > Quoting Bjorn Andersson (2021-10-04 19:37:50)
> > > > > > > > Found in the middle of a patch from Sankeerth was the reduction 
of the
> > > > > > > > INIT_SETUP timeout from 10s to 100ms. Upon INIT_SETUP timeout 
the host
> > > > > > > > is initalized and HPD interrupt start to be serviced, so in the 
case of
> > > > > > > > eDP this reduction improves the user experience dramatically - 
i.e.
> > > > > > > > removes 9.9s of bland screen time at boot.
> > > > > > > >
> > > > > > > > Suggested-by: Sankeerth Billakanti 
> > > > > > > > Signed-off-by: Bjorn Andersson 
> > > > > > > > ---
> > > > > > >
> > > > > > > Any Fixes tag? BTW, the delay design is pretty convoluted. I had 
to go
> > > > > > > re-read the code a couple times to understand that it's waiting 
100ms
> > > > > > > times the 'delay' number. What?
> > > > > > >
> > > > > >
> > > > > > I assume you're happy with the current 10s delay on the current
> > > > > > devices, so I don't think we should push for this to be backported.
> > > > > > I have no need for it to be backported on my side at least.
> > > > > >
> > > > >
> > > > > Sure. Fixes tag != backported to stable trees but it is close.
> > > > >
> > > > > > > Reviewed-by: Stephen Boyd 
> > > > > >
> > > >   dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 1); <== to 100ms
> > > >
> > > > This patch will prevent usb3 from working due to dp driver
> > > > initialize phy
> > > > earlier than usb3 which cause timeout error at power up usb3 phy
> > > > when both
> > > > edp and dp are enabled.
> > >
> > > Can you please help me understand what you mean here, I use this on my
> > > sc8180x with both eDP and USB-C/DP right now. What is it that doesn't
> > > work? Or am I just lucky in some race condition?
> > >
> > > Thanks,
> > > Bjorn
> > >
> > The problem is seen at sc7280.
> > Apple dongle have both  hdmi and usb port.
> > plug Apple dongle into type-c, then plug DP into apple's hdmi port
> > and usb
> > mouse into apple's usb port.
> > If edp enabled at this time, then usb mouse will not work due to
> > timeout at
> > phy power up.
> >
>
> Okay, so you're saying that if the DP driver invokes phy_power_on()
> before the USB driver does, USB initialization fails (or at least USB
> doesn't work)?

if dp driver call qcom_qmp_phy_init() before usb3 call 
qcom_qmp_phy_init(),
usb3 driver will timeout at readl_poll_timeout(status, val, (val & 
mask) ==

ready, 10, PHY_INIT_COMPLETE_TIMEOUT) of qcom_qmp_phy_power_on().


Thanks, I will try to reproduce this on my side. So the 10 seconds here
is strictly to give good enough time for the dwc3 driver to probe...

Any idea why you're saying that this is specific to sc7280, what 
changed

from sc7180?


I did not have sc7180 with edp before so that i am not sure it will 
happen on sc7180 or not.
The usb3 does not work when both edp and dp enabled I just seen at 
sc7280.

Current at sc7280 EC is not boot up correctly when system power up.
I have to manual reboot EC from linux kernel shell before DP/usb3 can 
work.

I am not sure this contribute to this problem or not.





>
> Sounds like something we need to work out in the QMP phy driver. Do you
> have any more details about what's going wrong.
>
>
> Also, I've seen various references to said "Apple dongle", do you have a
> link to the exact one you're testing with so I can pick one up for
> testing purposes as well?

Apple A2119 hdmi+usb dongle.
https://www.amazon.com/Apple-USB-C-Digital-Multiport-Adapter/dp/B07WF96FY5/ref=sr_1_2?dchild=1=apple+a2119=1633636227=8-2



Thanks,
Bjorn


>
> Regards,
> Bjorn
>
> > > > I had prepared a patch (drm/msm/dp: do not initialize combo phy
> > > > until plugin
> > > > interrupt) to fix this problem.
> > > > Unfortunately, my patch is depend on Bjorn's patch (PATCH v3 3/5]
> > > > drm/msm/dp: Support up to 3 DP controllers).
> > > > I will submit my patch for review once Bjorn's patches merged in.
> > > > Therefore I would think this patch should go after both Bjorn's
> > > > patches and
> > > > my patch.
> > > >
> > > >
> > > >

Re: [PATCH] drm/msm/dp: Shorten SETUP timeout

[khsieh]

On 2021-10-06 10:31, Bjorn Andersson wrote:

On Wed 06 Oct 08:37 PDT 2021, khs...@codeaurora.org wrote:


On 2021-10-05 19:10, Bjorn Andersson wrote:
> On Tue 05 Oct 16:04 PDT 2021, khs...@codeaurora.org wrote:
>
> > On 2021-10-05 15:36, Stephen Boyd wrote:
> > > Quoting Bjorn Andersson (2021-10-05 14:40:38)
> > > > On Tue 05 Oct 11:45 PDT 2021, Stephen Boyd wrote:
> > > >
> > > > > Quoting Bjorn Andersson (2021-10-04 19:37:50)
> > > > > > Found in the middle of a patch from Sankeerth was the reduction of 
the
> > > > > > INIT_SETUP timeout from 10s to 100ms. Upon INIT_SETUP timeout the 
host
> > > > > > is initalized and HPD interrupt start to be serviced, so in the 
case of
> > > > > > eDP this reduction improves the user experience dramatically - i.e.
> > > > > > removes 9.9s of bland screen time at boot.
> > > > > >
> > > > > > Suggested-by: Sankeerth Billakanti 
> > > > > > Signed-off-by: Bjorn Andersson 
> > > > > > ---
> > > > >
> > > > > Any Fixes tag? BTW, the delay design is pretty convoluted. I had to go
> > > > > re-read the code a couple times to understand that it's waiting 100ms
> > > > > times the 'delay' number. What?
> > > > >
> > > >
> > > > I assume you're happy with the current 10s delay on the current
> > > > devices, so I don't think we should push for this to be backported.
> > > > I have no need for it to be backported on my side at least.
> > > >
> > >
> > > Sure. Fixes tag != backported to stable trees but it is close.
> > >
> > > > > Reviewed-by: Stephen Boyd 
> > > >
> >   dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 1); <== to 100ms
> >
> > This patch will prevent usb3 from working due to dp driver
> > initialize phy
> > earlier than usb3 which cause timeout error at power up usb3 phy
> > when both
> > edp and dp are enabled.
>
> Can you please help me understand what you mean here, I use this on my
> sc8180x with both eDP and USB-C/DP right now. What is it that doesn't
> work? Or am I just lucky in some race condition?
>
> Thanks,
> Bjorn
>
The problem is seen at sc7280.
Apple dongle have both  hdmi and usb port.
plug Apple dongle into type-c, then plug DP into apple's hdmi port and 
usb

mouse into apple's usb port.
If edp enabled at this time, then usb mouse will not work due to 
timeout at

phy power up.



Okay, so you're saying that if the DP driver invokes phy_power_on()
before the USB driver does, USB initialization fails (or at least USB
doesn't work)?


if dp driver call qcom_qmp_phy_init() before usb3 call 
qcom_qmp_phy_init(),
usb3 driver will timeout at readl_poll_timeout(status, val, (val & mask) 
== ready, 10, PHY_INIT_COMPLETE_TIMEOUT) of qcom_qmp_phy_power_on().


Sounds like something we need to work out in the QMP phy driver. Do you
have any more details about what's going wrong.


Also, I've seen various references to said "Apple dongle", do you have 
a

link to the exact one you're testing with so I can pick one up for
testing purposes as well?


Apple A2119 hdmi+usb dongle.
https://www.amazon.com/Apple-USB-C-Digital-Multiport-Adapter/dp/B07WF96FY5/ref=sr_1_2?dchild=1=apple+a2119=1633636227=8-2



Regards,
Bjorn


> > I had prepared a patch (drm/msm/dp: do not initialize combo phy
> > until plugin
> > interrupt) to fix this problem.
> > Unfortunately, my patch is depend on Bjorn's patch (PATCH v3 3/5]
> > drm/msm/dp: Support up to 3 DP controllers).
> > I will submit my patch for review once Bjorn's patches merged in.
> > Therefore I would think this patch should go after both Bjorn's
> > patches and
> > my patch.
> >
> >
> >

Re: [PATCH] drm/msm/dp: Shorten SETUP timeout

[khsieh]

On 2021-10-05 19:10, Bjorn Andersson wrote:

On Tue 05 Oct 16:04 PDT 2021, khs...@codeaurora.org wrote:


On 2021-10-05 15:36, Stephen Boyd wrote:
> Quoting Bjorn Andersson (2021-10-05 14:40:38)
> > On Tue 05 Oct 11:45 PDT 2021, Stephen Boyd wrote:
> >
> > > Quoting Bjorn Andersson (2021-10-04 19:37:50)
> > > > Found in the middle of a patch from Sankeerth was the reduction of the
> > > > INIT_SETUP timeout from 10s to 100ms. Upon INIT_SETUP timeout the host
> > > > is initalized and HPD interrupt start to be serviced, so in the case of
> > > > eDP this reduction improves the user experience dramatically - i.e.
> > > > removes 9.9s of bland screen time at boot.
> > > >
> > > > Suggested-by: Sankeerth Billakanti 
> > > > Signed-off-by: Bjorn Andersson 
> > > > ---
> > >
> > > Any Fixes tag? BTW, the delay design is pretty convoluted. I had to go
> > > re-read the code a couple times to understand that it's waiting 100ms
> > > times the 'delay' number. What?
> > >
> >
> > I assume you're happy with the current 10s delay on the current
> > devices, so I don't think we should push for this to be backported.
> > I have no need for it to be backported on my side at least.
> >
>
> Sure. Fixes tag != backported to stable trees but it is close.
>
> > > Reviewed-by: Stephen Boyd 
> >
  dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 1); <== to 100ms

This patch will prevent usb3 from working due to dp driver initialize 
phy
earlier than usb3 which cause timeout error at power up usb3 phy when 
both

edp and dp are enabled.


Can you please help me understand what you mean here, I use this on my
sc8180x with both eDP and USB-C/DP right now. What is it that doesn't
work? Or am I just lucky in some race condition?

Thanks,
Bjorn


The problem is seen at sc7280.
Apple dongle have both  hdmi and usb port.
plug Apple dongle into type-c, then plug DP into apple's hdmi port and 
usb mouse into apple's usb port.
If edp enabled at this time, then usb mouse will not work due to timeout 
at phy power up.


I had prepared a patch (drm/msm/dp: do not initialize combo phy until 
plugin

interrupt) to fix this problem.
Unfortunately, my patch is depend on Bjorn's patch (PATCH v3 3/5]
drm/msm/dp: Support up to 3 DP controllers).
I will submit my patch for review once Bjorn's patches merged in.
Therefore I would think this patch should go after both Bjorn's 
patches and

my patch.

Re: [PATCH] drm/msm/dp: Shorten SETUP timeout

[khsieh]

On 2021-10-05 15:36, Stephen Boyd wrote:

Quoting Bjorn Andersson (2021-10-05 14:40:38)

On Tue 05 Oct 11:45 PDT 2021, Stephen Boyd wrote:

> Quoting Bjorn Andersson (2021-10-04 19:37:50)
> > Found in the middle of a patch from Sankeerth was the reduction of the
> > INIT_SETUP timeout from 10s to 100ms. Upon INIT_SETUP timeout the host
> > is initalized and HPD interrupt start to be serviced, so in the case of
> > eDP this reduction improves the user experience dramatically - i.e.
> > removes 9.9s of bland screen time at boot.
> >
> > Suggested-by: Sankeerth Billakanti 
> > Signed-off-by: Bjorn Andersson 
> > ---
>
> Any Fixes tag? BTW, the delay design is pretty convoluted. I had to go
> re-read the code a couple times to understand that it's waiting 100ms
> times the 'delay' number. What?
>

I assume you're happy with the current 10s delay on the current
devices, so I don't think we should push for this to be backported.
I have no need for it to be backported on my side at least.



Sure. Fixes tag != backported to stable trees but it is close.


> Reviewed-by: Stephen Boyd 


  dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 1); <== to 100ms

This patch will prevent usb3 from working due to dp driver initialize 
phy earlier than usb3 which cause timeout error at power up usb3 phy 
when both edp and dp are enabled.
I had prepared a patch (drm/msm/dp: do not initialize combo phy until 
plugin interrupt) to fix this problem.
Unfortunately, my patch is depend on Bjorn's patch (PATCH v3 3/5] 
drm/msm/dp: Support up to 3 DP controllers).

I will submit my patch for review once Bjorn's patches merged in.
Therefore I would think this patch should go after both Bjorn's patches 
and my patch.

Re: [PATCH v2 3/5] drm/msm/dp: Support up to 3 DP controllers

[khsieh]

On 2021-08-27 10:14, Bjorn Andersson wrote:

On Fri 27 Aug 00:20 CDT 2021, Stephen Boyd wrote:


Quoting Bjorn Andersson (2021-08-25 16:42:31)
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c
> index 2c7de43f655a..4a6132c18e57 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -78,6 +78,8 @@ struct dp_display_private {
> char *name;
> int irq;
>
> +   int id;
> +
> /* state variables */
> bool core_initialized;
> bool hpd_irq_on;
> @@ -115,8 +117,19 @@ struct dp_display_private {
> struct dp_audio *audio;
>  };
>
> +
> +struct msm_dp_config {
> +   phys_addr_t io_start[3];

Can this be made into another struct, like msm_dp_desc, that also
indicates what type of DP connector it is, i.e. eDP vs DP? That would
help me understand in modetest and /sys/class/drm what sort of 
connector

is probing. dp_drm_connector_init() would need to pass the type of
connector appropriately. Right now, eDP connectors still show up as DP
instead of eDP in sysfs.



I like it, will spin a v3 with this.

Regards,
Bjorn


Hi Bjorn,

Have you spin off V3 yet?
When you expect your patches related to DP be up streamed?

Thanks,
kuogee



> +   size_t num_dp;
> +};
> +
> +static const struct msm_dp_config sc7180_dp_cfg = {
> +   .io_start = { 0x0ae9 },
> +   .num_dp = 1,
> +};
> +
>  static const struct of_device_id dp_dt_match[] = {
> -   {.compatible = "qcom,sc7180-dp"},
> +   { .compatible = "qcom,sc7180-dp", .data = _dp_cfg },
> {}
>  };
>

Re: [PATCH v3] drm/dp_mst: Fix return code on sideband message failure

[khsieh]

On 2021-08-30 09:58, Lyude Paul wrote:

On Mon, 2021-08-30 at 08:56 -0700, khs...@codeaurora.org wrote:

On 2021-08-25 09:26, Lyude Paul wrote:
> The patch was pushed yes (was part of drm-misc-next-2021-07-29), seems
> like it
> just hasn't trickled down to linus's branch quite yet.

Hi Stephen B,

Would you mind back porting this patch to V5.10 branch?
It will have lots of helps for us to support display port MST case.
Thanks,


I'm assuming you're talking to someone else? A little confused because 
I don't

see a Stephen B in this thread


Yes,
I am asking Stephen B (swb...@chromium.org) helps to back port this 
patch to v5.10.






>
> On Wed, 2021-08-25 at 09:06 -0700, khs...@codeaurora.org wrote:
> > On 2021-07-27 15:44, Lyude Paul wrote:
> > > Nice timing, you literally got me as I was 2 minutes away from leaving
> > > work
> > > for the day :P. I will go ahead and push it now.
> > >
> > Hi Lyude,
> >
> > Had you pushed this patch yet?
> > We still did not see this patch at msm-nex and v5.10 branch.
> > Thanks,
> >
> >
> > > BTW - in the future I recommend using dim to add Fixes: tags as it'll
> > > add Cc:
> > > to stable as appropriate (this patch in particular should be Cc:
> > > sta...@vger.kernel.org # v5.3+). will add these tags when I push it
> > >
> > > On Tue, 2021-07-27 at 15:41 -0700, khs...@codeaurora.org wrote:
> > > > On 2021-07-27 12:21, Lyude Paul wrote:
> > > > > On Thu, 2021-07-22 at 15:28 -0700, khs...@codeaurora.org wrote:
> > > > > >
> > > > > > It looks like this patch is good to go (mainlined).
> > > > > > Anything needed from me to do?
> > > > > > Thanks,
> > > > >
> > > > > Do you have access for pushing this patch? If not let me know and
> > > > > I
> > > > > can
> > > > > go
> > > > > ahead and push it to drm-misc-next for you.
> > > > no, I do not have access to drm-misc-next.
> > > > Please push it for me.
> > > > Thanks a lots.
> > > >
> >

Re: [PATCH v3] drm/dp_mst: Fix return code on sideband message failure

[khsieh]

On 2021-08-25 09:26, Lyude Paul wrote:
The patch was pushed yes (was part of drm-misc-next-2021-07-29), seems 
like it

just hasn't trickled down to linus's branch quite yet.


Hi Stephen B,

Would you mind back porting this patch to V5.10 branch?
It will have lots of helps for us to support display port MST case.
Thanks,





On Wed, 2021-08-25 at 09:06 -0700, khs...@codeaurora.org wrote:

On 2021-07-27 15:44, Lyude Paul wrote:
> Nice timing, you literally got me as I was 2 minutes away from leaving
> work
> for the day :P. I will go ahead and push it now.
>
Hi Lyude,

Had you pushed this patch yet?
We still did not see this patch at msm-nex and v5.10 branch.
Thanks,


> BTW - in the future I recommend using dim to add Fixes: tags as it'll
> add Cc:
> to stable as appropriate (this patch in particular should be Cc:
> sta...@vger.kernel.org # v5.3+). will add these tags when I push it
>
> On Tue, 2021-07-27 at 15:41 -0700, khs...@codeaurora.org wrote:
> > On 2021-07-27 12:21, Lyude Paul wrote:
> > > On Thu, 2021-07-22 at 15:28 -0700, khs...@codeaurora.org wrote:
> > > >
> > > > It looks like this patch is good to go (mainlined).
> > > > Anything needed from me to do?
> > > > Thanks,
> > >
> > > Do you have access for pushing this patch? If not let me know and I
> > > can
> > > go
> > > ahead and push it to drm-misc-next for you.
> > no, I do not have access to drm-misc-next.
> > Please push it for me.
> > Thanks a lots.
> >

Re: [PATCH v3] drm/dp_mst: Fix return code on sideband message failure

[khsieh]

On 2021-07-27 15:44, Lyude Paul wrote:
Nice timing, you literally got me as I was 2 minutes away from leaving 
work

for the day :P. I will go ahead and push it now.


Hi Lyude,

Had you pushed this patch yet?
We still did not see this patch at msm-nex and v5.10 branch.
Thanks,


BTW - in the future I recommend using dim to add Fixes: tags as it'll 
add Cc:

to stable as appropriate (this patch in particular should be Cc:
sta...@vger.kernel.org # v5.3+). will add these tags when I push it

On Tue, 2021-07-27 at 15:41 -0700, khs...@codeaurora.org wrote:

On 2021-07-27 12:21, Lyude Paul wrote:
> On Thu, 2021-07-22 at 15:28 -0700, khs...@codeaurora.org wrote:
> >
> > It looks like this patch is good to go (mainlined).
> > Anything needed from me to do?
> > Thanks,
>
> Do you have access for pushing this patch? If not let me know and I can
> go
> ahead and push it to drm-misc-next for you.
no, I do not have access to drm-misc-next.
Please push it for me.
Thanks a lots.

Re: [PATCH v2] drm/msm/dp: add drm debug logs to dp_pm_resume/suspend

[khsieh]

On 2021-08-10 12:23, Stephen Boyd wrote:

Quoting khs...@codeaurora.org (2021-08-10 12:18:02)

On 2021-08-10 11:33, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2021-08-10 08:29:22)
>> Changes in V2:
>> -- correct Fixes text
>> -- drop commit text
>>
>> Fixes: 601f0479c583 ("drm/msm/dp: add logs across DP driver for ease
>> of debugging")
>> Signed-off-by: Kuogee Hsieh 
>> ---
>>  drivers/gpu/drm/msm/dp/dp_display.c | 13 +
>>  1 file changed, 13 insertions(+)
>
> BTW, this conflicts with commit
> e8a767e04dbc7b201cb17ab99dca723a3488b6d4
> in msm-next. The resolution is trivial but just wanted to mention it.

I Just fetched msm-next and cherry-pick this patch over, no conflict
seen.
Is this conflict need to be fixed?



Oh sorry, I mean commit afc9b8b6bab8 ("drm/msm/dp: signal audio plugged
change at dp_pm_resume") which doesn't seem to be in msm-next. Maybe 
Rob

will resolve the conflict directly.
Yes, I just found that  commit afc9b8b6bab8 ("drm/msm/dp: signal audio 
plugged

change at dp_pm_resume") is not merged in msm-next.

Re: [PATCH v2] drm/msm/dp: add drm debug logs to dp_pm_resume/suspend

[khsieh]

On 2021-08-10 11:33, Stephen Boyd wrote:

Quoting Kuogee Hsieh (2021-08-10 08:29:22)

Changes in V2:
-- correct Fixes text
-- drop commit text

Fixes: 601f0479c583 ("drm/msm/dp: add logs across DP driver for ease 
of debugging")

Signed-off-by: Kuogee Hsieh 
---
 drivers/gpu/drm/msm/dp/dp_display.c | 13 +
 1 file changed, 13 insertions(+)


BTW, this conflicts with commit 
e8a767e04dbc7b201cb17ab99dca723a3488b6d4

in msm-next. The resolution is trivial but just wanted to mention it.


I Just fetched msm-next and cherry-pick this patch over, no conflict 
seen.

Is this conflict need to be fixed?

Re: [PATCH v3] drm/msm/dp: update is_connected status base on sink count at dp_pm_resume()

[khsieh]

On 2021-08-03 12:05, Stephen Boyd wrote:

Quoting Kuogee Hsieh (2021-08-03 09:25:13)
Currently at dp_pm_resume() is_connected state is decided base on hpd 
connection
status only. This will put is_connected in wrongly "true" state at the 
scenario
that dongle attached to DUT but without hmdi cable connecting to it. 
Fix this
problem by adding read sink count from dongle and decided is_connected 
state base

on both sink count and hpd connection status.

Changes in v2:
-- remove dp_get_sink_count() cand call drm_dp_read_sink_count()

Changes in v3:
-- delete status local variable from dp_pm_resume()

Fixes: d9aa6571b28ba ("drm/msm/dp: check sink_count before update 
is_connected status")

Signed-off-by: Kuogee Hsieh 
---
 drivers/gpu/drm/msm/dp/dp_display.c | 18 +++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c

index 78c5301..0f39256 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1313,7 +1313,7 @@ static int dp_pm_resume(struct device *dev)
struct platform_device *pdev = to_platform_device(dev);
struct msm_dp *dp_display = platform_get_drvdata(pdev);
struct dp_display_private *dp;
-   u32 status;
+   int sink_count = 0;

dp = container_of(dp_display, struct dp_display_private, 
dp_display);


@@ -1327,14 +1327,26 @@ static int dp_pm_resume(struct device *dev)

dp_catalog_ctrl_hpd_config(dp->catalog);

-   status = dp_catalog_link_is_connected(dp->catalog);
+   /*
+* set sink to normal operation mode -- D0
+* before dpcd read
+*/
+   dp_link_psm_config(dp->link, >panel->link_info, false);
+
+   /* if sink conencted, do dpcd read sink count */


s/conencted/connected/

This also just says what the code is doing. Why do we only read the 
sink

count if the link is connected? Can we read the sink count even if the
link isn't connected and then consider sink count as 0 if trying to 
read

fails?


yes, we can do that.
But it will suffer aux time out and retry.
i think it is better to avoid this overhead by check connection first.


+   if (dp_catalog_link_is_connected(dp->catalog)) {
+   sink_count = drm_dp_read_sink_count(dp->aux);
+   if (sink_count < 0)
+   sink_count = 0;
+   }

+   dp->link->sink_count = sink_count;
/*
 * can not declared display is connected unless
 * HDMI cable is plugged in and sink_count of
 * dongle become 1
 */
-   if (status && dp->link->sink_count)
+   if (dp->link->sink_count)
dp->dp_display.is_connected = true;
else
dp->dp_display.is_connected = false;

Re: [PATCH] drm/msm/dp: update is_connected status base on sink count at dp_pm_resume()

[khsieh]

On 2021-07-30 11:57, Stephen Boyd wrote:

Quoting Kuogee Hsieh (2021-07-28 14:30:54)
Currently at dp_pm_resume() is_connected state is decided base on hpd 
connection
status only. This will put is_connected in wrongly "true" state at the 
scenario
that dongle attached to DUT but without hmdi cable connecting to it. 
Fix this
problem by adding read sink count from dongle and decided is_connected 
state base

on both sink count and hpd connection status.



Please add a Fixes tag.


Signed-off-by: Kuogee Hsieh 
---
 drivers/gpu/drm/msm/dp/dp_display.c | 23 +--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c

index 2b660e9..9bcb261 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1308,6 +1308,17 @@ static int dp_display_remove(struct 
platform_device *pdev)

return 0;
 }

+static int dp_get_sink_count(struct dp_display_private *dp)
+{
+   u8 sink_count;
+
+   sink_count = drm_dp_read_sink_count(dp->aux);


drm_dp_read_sink_count() returns an int, not a u8. Comparing a u8 to
less than zero doesn't make any sense as it isn't signed.


+   if (sink_count < 0)
+   return 0;
+
+   return sink_count;
+}


We can drop this function and just have an int count in dp_pm_resume()
that is compared to < 0 and then ignored.


+
 static int dp_pm_resume(struct device *dev)
 {
struct platform_device *pdev = to_platform_device(dev);
@@ -1327,14 +1338,22 @@ static int dp_pm_resume(struct device *dev)

dp_catalog_ctrl_hpd_config(dp->catalog);

-   status = dp_catalog_link_is_connected(dp->catalog);
+   /*
+* set sink to normal operation mode -- D0
+* before dpcd read
+*/
+   dp_link_psm_config(dp->link, >panel->link_info, false);

+   if ((status = dp_catalog_link_is_connected(dp->catalog)))
+   dp->link->sink_count = dp_get_sink_count(dp);


Do we need to call drm_dp_read_sink_count_cap() as well?

no, we only need sink_count



+   else
+   dp->link->sink_count = 0;
/*
 * can not declared display is connected unless
 * HDMI cable is plugged in and sink_count of
 * dongle become 1
 */
-   if (status && dp->link->sink_count)


Is 'status' used anymore? If not, please remove it.
Yes, it still used which used to decided to perform dpcd read sink count 
or not



+   if (dp->link->sink_count)
dp->dp_display.is_connected = true;
else
dp->dp_display.is_connected = false;

Re: [PATCH v2 5/7] drm/msm/dp: return correct edid checksum after corrupted edid checksum read

[khsieh]

On 2021-07-22 12:23, Stephen Boyd wrote:

Quoting Kuogee Hsieh (2021-07-13 08:54:05)
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c 
b/drivers/gpu/drm/msm/dp/dp_panel.c

index 88196f7..0fdb551 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.c
+++ b/drivers/gpu/drm/msm/dp/dp_panel.c
@@ -303,7 +303,12 @@ void dp_panel_handle_sink_request(struct dp_panel 
*dp_panel)
panel = container_of(dp_panel, struct dp_panel_private, 
dp_panel);


if (panel->link->sink_request & DP_TEST_LINK_EDID_READ) {
-   u8 checksum = 
dp_panel_get_edid_checksum(dp_panel->edid);

+   u8 checksum;
+
+   if (dp_panel->edid)
+   checksum = 
dp_panel_get_edid_checksum(dp_panel->edid);

+   else
+   checksum = 
dp_panel->connector->real_edid_checksum;


Is there any reason why we can't use connector->real_edid_checksum all
the time?

real_edid_checksum only calculated by drm when edid checksum 
vitrification failed after edid read.
In the good edid checksum case (edid verification succeed), 
real_edid_checksum is not calculated by drm.


dp_link_send_edid_checksum(panel->link, checksum);
dp_link_send_test_response(panel->link);

Re: [PATCH v3] drm/dp_mst: Fix return code on sideband message failure

[khsieh]

On 2021-07-27 12:21, Lyude Paul wrote:

On Thu, 2021-07-22 at 15:28 -0700, khs...@codeaurora.org wrote:


It looks like this patch is good to go (mainlined).
Anything needed from me to do?
Thanks,


Do you have access for pushing this patch? If not let me know and I can 
go

ahead and push it to drm-misc-next for you.

no, I do not have access to drm-misc-next.
Please push it for me.
Thanks a lots.

Re: [PATCH v3] drm/dp_mst: Fix return code on sideband message failure

[khsieh]

On 2021-07-22 10:53, Lyude Paul wrote:

On Tue, 2021-07-13 at 15:24 -0700, khs...@codeaurora.org wrote:

On 2021-07-07 01:37, Jani Nikula wrote:
> On Tue, 06 Jul 2021, Kuogee Hsieh  wrote:
> > From: Rajkumar Subbiah 
> >
> > Commit 2f015ec6eab6 ("drm/dp_mst: Add sideband down request tracing +
> > selftests") added some debug code for sideband message tracing. But
> > it seems to have unintentionally changed the behavior on sideband
> > message
> > failure. It catches and returns failure only if DRM_UT_DP is enabled.
> > Otherwise it ignores the error code and returns success. So on an MST
> > unplug, the caller is unaware that the clear payload message failed
> > and
> > ends up waiting for 4 seconds for the response. Fixes the issue by
> > returning the proper error code.
> >
> > Changes in V2:
> > -- Revise commit text as review comment
> > -- add Fixes text
> >
> > Changes in V3:
> > -- remove "unlikely" optimization
> >
> > Fixes: 2f015ec6eab6 ("drm/dp_mst: Add sideband down request tracing +
> > selftests")
> >
> > Signed-off-by: Rajkumar Subbiah 
> > Signed-off-by: Kuogee Hsieh 
> >
> > Reviewed-by: Stephen Boyd 
>
> Reviewed-by: Jani Nikula 
>
>
> > ---
Lyude,
Any comments from you?
Thanks,


Hey! Sorry did I forget to respond to this?

Reviewed-by: Lyude Paul 



It looks like this patch is good to go (mainlined).
Anything needed from me to do?
Thanks,



> >  drivers/gpu/drm/drm_dp_mst_topology.c | 10 ++
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index 1590144..df91110 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -2887,11 +2887,13 @@ static int process_single_tx_qlock(struct
> > drm_dp_mst_topology_mgr *mgr,
> > idx += tosend + 1;
> >
> > ret = drm_dp_send_sideband_msg(mgr, up, chunk, idx);
> > -   if (unlikely(ret) && drm_debug_enabled(DRM_UT_DP)) {
> > -   struct drm_printer p = drm_debug_printer(DBG_PREFIX);
> > +   if (ret) {
> > +   if (drm_debug_enabled(DRM_UT_DP)) {
> > +   struct drm_printer p =
> > drm_debug_printer(DBG_PREFIX);
> >
> > -   drm_printf(, "sideband msg failed to send\n");
> > -   drm_dp_mst_dump_sideband_msg_tx(, txmsg);
> > +   drm_printf(, "sideband msg failed to send\n");
> > +   drm_dp_mst_dump_sideband_msg_tx(, txmsg);
> > +   }
> > return ret;
> > }

Re: [PATCH v3] drm/dp_mst: Fix return code on sideband message failure

[khsieh]

On 2021-07-07 01:37, Jani Nikula wrote:

On Tue, 06 Jul 2021, Kuogee Hsieh  wrote:

From: Rajkumar Subbiah 

Commit 2f015ec6eab6 ("drm/dp_mst: Add sideband down request tracing +
selftests") added some debug code for sideband message tracing. But
it seems to have unintentionally changed the behavior on sideband 
message

failure. It catches and returns failure only if DRM_UT_DP is enabled.
Otherwise it ignores the error code and returns success. So on an MST
unplug, the caller is unaware that the clear payload message failed 
and

ends up waiting for 4 seconds for the response. Fixes the issue by
returning the proper error code.

Changes in V2:
-- Revise commit text as review comment
-- add Fixes text

Changes in V3:
-- remove "unlikely" optimization

Fixes: 2f015ec6eab6 ("drm/dp_mst: Add sideband down request tracing + 
selftests")


Signed-off-by: Rajkumar Subbiah 
Signed-off-by: Kuogee Hsieh 

Reviewed-by: Stephen Boyd 


Reviewed-by: Jani Nikula 



---

Lyude,
Any comments from you?
Thanks,


 drivers/gpu/drm/drm_dp_mst_topology.c | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c 
b/drivers/gpu/drm/drm_dp_mst_topology.c

index 1590144..df91110 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2887,11 +2887,13 @@ static int process_single_tx_qlock(struct 
drm_dp_mst_topology_mgr *mgr,

idx += tosend + 1;

ret = drm_dp_send_sideband_msg(mgr, up, chunk, idx);
-   if (unlikely(ret) && drm_debug_enabled(DRM_UT_DP)) {
-   struct drm_printer p = drm_debug_printer(DBG_PREFIX);
+   if (ret) {
+   if (drm_debug_enabled(DRM_UT_DP)) {
+   struct drm_printer p = drm_debug_printer(DBG_PREFIX);

-   drm_printf(, "sideband msg failed to send\n");
-   drm_dp_mst_dump_sideband_msg_tx(, txmsg);
+   drm_printf(, "sideband msg failed to send\n");
+   drm_dp_mst_dump_sideband_msg_tx(, txmsg);
+   }
return ret;
}

Re: [PATCH 5/7] drm/msm/dp: return correct edid checksum after corrupted edid checksum read

[khsieh]

On 2021-07-08 00:14, Stephen Boyd wrote:

Quoting Kuogee Hsieh (2021-07-06 10:20:18)
Response with correct edid checksum saved at connector after corrupted 
edid

checksum read. This fixes Link Layer CTS cases 4.2.2.3, 4.2.2.6.

Signed-off-by: Kuogee Hsieh 
---
 drivers/gpu/drm/msm/dp/dp_panel.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c 
b/drivers/gpu/drm/msm/dp/dp_panel.c

index 88196f7..0fdb551 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.c
+++ b/drivers/gpu/drm/msm/dp/dp_panel.c
@@ -271,7 +271,7 @@ static u8 dp_panel_get_edid_checksum(struct edid 
*edid)

 {
struct edid *last_block;
u8 *raw_edid;
-   bool is_edid_corrupt;
+   bool is_edid_corrupt = false;

if (!edid) {
DRM_ERROR("invalid edid input\n");
@@ -303,7 +303,12 @@ void dp_panel_handle_sink_request(struct dp_panel 
*dp_panel)
panel = container_of(dp_panel, struct dp_panel_private, 
dp_panel);


if (panel->link->sink_request & DP_TEST_LINK_EDID_READ) {
-   u8 checksum = 
dp_panel_get_edid_checksum(dp_panel->edid);

+   u8 checksum;
+
+   if (dp_panel->edid)
+   checksum = 
dp_panel_get_edid_checksum(dp_panel->edid);

+   else
+   checksum = 
dp_panel->connector->real_edid_checksum;


dp_link_send_edid_checksum(panel->link, checksum);


It looks like this can be drm_dp_send_real_edid_checksum()? Then we
don't have to look at the connector internals sometimes and can drop
dp_panel_get_edid_checksum() entirely?
you still need to pass read edid checksum into 
drm_dp_send_real_edid_checksum().



dp_link_send_test_response(panel->link);

Re: [PATCH 2/7] drm/msm/dp: reduce link rate if failed at link training 1

[khsieh]

On 2021-07-08 00:33, Stephen Boyd wrote:

Quoting Kuogee Hsieh (2021-07-06 10:20:15)

Reduce link rate and re start link training if link training 1
failed due to loss of clock recovery done to fix Link Layer
CTS case 4.3.1.7.  Also only update voltage and pre-emphasis
swing level after link training started to fix Link Layer CTS
case 4.3.1.6.

Signed-off-by: Kuogee Hsieh 
---
 drivers/gpu/drm/msm/dp/dp_ctrl.c | 86 
++--

 1 file changed, 56 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c 
b/drivers/gpu/drm/msm/dp/dp_ctrl.c

index 27fb0f0..6f8443d 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -83,13 +83,6 @@ struct dp_ctrl_private {
struct completion video_comp;
 };

-struct dp_cr_status {
-   u8 lane_0_1;
-   u8 lane_2_3;
-};
-
-#define DP_LANE0_1_CR_DONE 0x11
-
 static int dp_aux_link_configure(struct drm_dp_aux *aux,
struct dp_link_info *link)
 {
@@ -1080,7 +1073,7 @@ static int dp_ctrl_read_link_status(struct 
dp_ctrl_private *ctrl,

 }

 static int dp_ctrl_link_train_1(struct dp_ctrl_private *ctrl,
-   struct dp_cr_status *cr, int *training_step)
+   u8 *cr, int *training_step)
 {
int tries, old_v_level, ret = 0;
u8 link_status[DP_LINK_STATUS_SIZE];
@@ -1109,8 +1102,8 @@ static int dp_ctrl_link_train_1(struct 
dp_ctrl_private *ctrl,

if (ret)
return ret;

-   cr->lane_0_1 = link_status[0];
-   cr->lane_2_3 = link_status[1];
+   cr[0] = link_status[0];
+   cr[1] = link_status[1];

if (drm_dp_clock_recovery_ok(link_status,
ctrl->link->link_params.num_lanes)) {
@@ -1188,7 +1181,7 @@ static void 
dp_ctrl_clear_training_pattern(struct dp_ctrl_private *ctrl)

 }

 static int dp_ctrl_link_train_2(struct dp_ctrl_private *ctrl,
-   struct dp_cr_status *cr, int *training_step)
+   u8 *cr, int *training_step)
 {
int tries = 0, ret = 0;
char pattern;
@@ -1204,10 +1197,6 @@ static int dp_ctrl_link_train_2(struct 
dp_ctrl_private *ctrl,

else
pattern = DP_TRAINING_PATTERN_2;

-   ret = dp_ctrl_update_vx_px(ctrl);
-   if (ret)
-   return ret;
-
ret = dp_catalog_ctrl_set_pattern(ctrl->catalog, pattern);
if (ret)
return ret;
@@ -1220,8 +1209,8 @@ static int dp_ctrl_link_train_2(struct 
dp_ctrl_private *ctrl,

ret = dp_ctrl_read_link_status(ctrl, link_status);
if (ret)
return ret;
-   cr->lane_0_1 = link_status[0];
-   cr->lane_2_3 = link_status[1];
+   cr[0] = link_status[0];
+   cr[1] = link_status[1];

if (drm_dp_channel_eq_ok(link_status,
ctrl->link->link_params.num_lanes)) {
@@ -1241,7 +1230,7 @@ static int dp_ctrl_link_train_2(struct 
dp_ctrl_private *ctrl,
 static int dp_ctrl_reinitialize_mainlink(struct dp_ctrl_private 
*ctrl);


 static int dp_ctrl_link_train(struct dp_ctrl_private *ctrl,
-   struct dp_cr_status *cr, int *training_step)
+   u8 *cr, int *training_step)
 {
int ret = 0;
u8 encoding = DP_SET_ANSI_8B10B;
@@ -1282,7 +1271,7 @@ static int dp_ctrl_link_train(struct 
dp_ctrl_private *ctrl,

 }

 static int dp_ctrl_setup_main_link(struct dp_ctrl_private *ctrl,
-   struct dp_cr_status *cr, int *training_step)
+   u8 *cr, int *training_step)
 {
int ret = 0;

@@ -1496,14 +1485,14 @@ static int 
dp_ctrl_deinitialize_mainlink(struct dp_ctrl_private *ctrl)

 static int dp_ctrl_link_maintenance(struct dp_ctrl_private *ctrl)
 {
int ret = 0;
-   struct dp_cr_status cr;
+   u8 cr_status[2];
int training_step = DP_TRAINING_NONE;

dp_ctrl_push_idle(>dp_ctrl);

ctrl->dp_ctrl.pixel_rate = 
ctrl->panel->dp_mode.drm_mode.clock;


-   ret = dp_ctrl_setup_main_link(ctrl, , _step);
+   ret = dp_ctrl_setup_main_link(ctrl, cr_status, 
_step);

if (ret)
goto end;


Do we need to extract the link status information from deep in these
functions? Why not read it again when we need to?



@@ -1634,6 +1623,41 @@ void dp_ctrl_handle_sink_request(struct dp_ctrl 
*dp_ctrl)

}
 }

+static bool dp_ctrl_any_lane_cr_done(struct dp_ctrl_private *ctrl,
+   u8 *cr_status)
+
+{
+   int i;
+   u8 status;
+   int lane = ctrl->link->link_params.num_lanes;
+
+   for (i = 0; i < lane; i++) {
+   status = cr_status[i / 2];
+   status >>= ((i % 2) * 4);
+   if (status & DP_LANE_CR_DONE)
+   return true;
+   }
+
+   return false;
+}
+
+static bool dp_ctrl_any_lane_cr_lose(struct dp_ctrl_private 

Re: [PATCH 7/7] drm/msm/dp: retrain link when loss of symbol lock detected

[khsieh]

On 2021-07-08 00:21, Stephen Boyd wrote:

Quoting Kuogee Hsieh (2021-07-06 10:20:20)

Main link symbol locked is achieved at end of link training 2. Some
dongle main link symbol may become unlocked again if host did not end
link training soon enough after completion of link training 2. Host
have to re train main link if loss of symbol lock detected before
end link training so that the coming video stream can be transmitted
to sink properly.

Signed-off-by: Kuogee Hsieh 


I guess this is a fix for the original driver, so it should be tagged
with Fixes appropriately.
Actually, this is fix on patch #6 : drm/msm/dp: do not end dp link 
training until video is ready

Should i merge patch #6 and #7 together?
Or can you suggest what should I do?



---
 drivers/gpu/drm/msm/dp/dp_ctrl.c | 34 
++

 1 file changed, 34 insertions(+)

diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c 
b/drivers/gpu/drm/msm/dp/dp_ctrl.c

index 0cb01a9..e616ab2 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1661,6 +1661,25 @@ static bool dp_ctrl_any_lane_cr_lose(struct 
dp_ctrl_private *ctrl,

return false;
 }

+static bool dp_ctrl_loss_symbol_lock(struct dp_ctrl_private *ctrl)
+{
+   u8 link_status[6];


Can we use link_status[DP_LINK_STATUS_SIZE] instead?


+   u8 status;
+   int i;
+   int lane = ctrl->link->link_params.num_lanes;


s/lane/num_lanes/

would make the code easier to read


+
+   dp_ctrl_read_link_status(ctrl, link_status);
+
+   for (i = 0; i < lane; i++) {
+   status = link_status[i / 2];
+   status >>= ((i % 2) * 4);
+   if (!(status & DP_LANE_SYMBOL_LOCKED))
+   return true;
+   }
+
+   return false;
+}
+
 int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
 {
int rc = 0;
@@ -1777,6 +1796,17 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
return rc;
 }

+static int dp_ctrl_link_retrain(struct dp_ctrl_private *ctrl)
+{
+   int ret = 0;


Please drop init of ret.


+   u8 cr_status[2];
+   int training_step = DP_TRAINING_NONE;
+
+   ret = dp_ctrl_setup_main_link(ctrl, cr_status, 
_step);


as it is assigned here.


+
+   return ret;


And indeed, it could be 'return dp_ctrl_setup_main_link()' instead.


+}
+
 int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
 {
int ret = 0;
@@ -1802,6 +1832,10 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
}
}

+   /* if loss symbol lock happen, then retaining the link */


retain or retrain? The comment seems to be saying what the code says 
"if

loss retrain", so the comment is not very useful.


+   if (dp_ctrl_loss_symbol_lock(ctrl))
+   dp_ctrl_link_retrain(ctrl);
+
/* stop txing train pattern to end link training */
dp_ctrl_clear_training_pattern(ctrl);

Re: [PATCH 3/7] drm/msm/dp: reset aux controller after dp_aux_cmd_fifo_tx() failed.

[khsieh]

On 2021-07-08 00:34, Stephen Boyd wrote:

Quoting Kuogee Hsieh (2021-07-06 10:20:16)
Aux hardware calibration sequence requires resetting the aux 
controller

in order for the new setting to take effect. However resetting the AUX
controller will also clear HPD interrupt status which may accidentally
cause pending unplug interrupt to get lost. Therefore reset aux
controller only when link is in connection state when 
dp_aux_cmd_fifo_tx()

fail. This fixes Link Layer CTS cases 4.2.1.1 and 4.2.1.2.

Signed-off-by: Kuogee Hsieh 
---
 drivers/gpu/drm/msm/dp/dp_aux.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
b/drivers/gpu/drm/msm/dp/dp_aux.c

index 4a3293b..eb40d84 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -353,6 +353,9 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux 
*dp_aux,

if (!(aux->retry_cnt % MAX_AUX_RETRIES))

dp_catalog_aux_update_cfg(aux->catalog);

}
+   /* reset aux if link is in connected state */
+   if (dp_catalog_link_is_connected(aux->catalog))


How do we avoid resetting aux when hpd is unplugged and then plugged
back in during an aux transfer?

i am not sure this is possible.
it should get unplug interrupt followed by plugin interrupt.
In this case, aux will be re set and initialized



+   dp_catalog_aux_reset(aux->catalog);
} else {
aux->retry_cnt = 0;
switch (aux->aux_error_num) {
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,

a Linux Foundation Collaborative Project

Re: [PATCH 1/7] drm/msm/dp: use dp_ctrl_off_link_stream during PHY compliance test run

[khsieh]

On 2021-07-08 00:03, Stephen Boyd wrote:

Quoting Kuogee Hsieh (2021-07-06 10:20:14)

DP cable should always connect to DPU during the entire PHY compliance
testing run. Since DP PHY compliance test is executed at irq_hpd event
context, dp_ctrl_off_link_stream() should be used instead of 
dp_ctrl_off().
dp_ctrl_off() is used for unplug event which is triggered when DP 
cable is

dis connected.

Signed-off-by: Kuogee Hsieh 
---


Is this

Fixes: f21c8a276c2d ("drm/msm/dp: handle irq_hpd with sink_count = 0 
correctly")


or

Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")


should be fixes at f21c8a276c2d ("drm/msm/dp: handle irq_hpd with 
sink_count = 0 correctly")




? It's not clear how dp_ctrl_off() was working for compliance tests
before commit f21c8a276c2d.
both dp_ctrl_off() and dp_ctrl_off_link_strea() are work for 
dp_ctrl_process_phy_test_request()
The problem is after dp_ctrl_off(), aux channel is down, hence next phy 
test will failed due to dpcd read failed.
So that cable unplugged and replug back to required to run next test 
case.
dp_ctrl_off_link_stream() will keep aux channel up and other phy test 
case can be continued.





 drivers/gpu/drm/msm/dp/dp_ctrl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c 
b/drivers/gpu/drm/msm/dp/dp_ctrl.c

index caf71fa..27fb0f0 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1530,7 +1530,7 @@ static int 
dp_ctrl_process_phy_test_request(struct dp_ctrl_private *ctrl)

 * running. Add the global reset just before disabling the
 * link clocks and core clocks.
 */
-   ret = dp_ctrl_off(>dp_ctrl);
+   ret = dp_ctrl_off_link_stream(>dp_ctrl);
if (ret) {
DRM_ERROR("failed to disable DP controller\n");
return ret;

Re: [PATCH v3] drm/msm/dp: power off DP phy at suspend

[khsieh]

On 2021-06-01 19:00, Stephen Boyd wrote:

Please add dri-devel@lists.freedesktop.org next time

Quoting Kuogee Hsieh (2021-06-01 16:50:08)

Normal DP suspend operation contains two steps, display off followed
by dp suspend, to complete system wide suspending cycle if display is
up at that time. In this case, DP phy will be powered off at display
off. However there is an exception case that depending on the timing
of dongle plug in during system wide suspending, sometimes display off
procedure may be skipped and dp suspend was called directly. In this
case, dp phy is stay at powered on (phy->power_count = 1) so that at
next resume dp driver crash at main link clock enable due to phy is
not physically powered on. This patch will call 
dp_ctrl_off_link_stream()
to tear down main link and power off phy at dp_pm_suspend() if main 
link

had been brought up.

Changes in V2:
-- stashed changes into dp_ctrl.c
-- add is_phy_on to monitor phy state

Changes in V3:
-- delete is_phy_on
-- call dp_ctrl_off_link_stream() from dp_pm_suspend()

Fixes: 0114f31a2903 ("drm/msm/dp: handle irq_hpd with sink_count = 0 
correctly)

Signed-off-by: Kuogee Hsieh 
---
 drivers/gpu/drm/msm/dp/dp_ctrl.c| 10 +-
 drivers/gpu/drm/msm/dp/dp_display.c |  4 +++-
 drivers/gpu/drm/msm/dp/dp_power.c   | 15 +++
 3 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c 
b/drivers/gpu/drm/msm/dp/dp_ctrl.c

index dbd8943..8324a453 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1414,6 +1414,7 @@ void dp_ctrl_host_deinit(struct dp_ctrl 
*dp_ctrl)

phy = dp_io->phy;

dp_catalog_ctrl_enable_irq(ctrl->catalog, false);
+
phy_exit(phy);

DRM_DEBUG_DP("Host deinitialized successfully\n");
@@ -1457,6 +1458,7 @@ static int dp_ctrl_reinitialize_mainlink(struct 
dp_ctrl_private *ctrl)

return ret;
}
phy_power_off(phy);
+
/* hw recommended delay before re-enabling clocks */
msleep(20);

@@ -1488,6 +1490,7 @@ static int dp_ctrl_deinitialize_mainlink(struct 
dp_ctrl_private *ctrl)

}

phy_power_off(phy);
+
phy_exit(phy);

return 0;


None of these hunks are useful. Can we drop them?

@@ -1816,12 +1819,16 @@ int dp_ctrl_off_link_stream(struct dp_ctrl 
*dp_ctrl)

struct dp_ctrl_private *ctrl;
struct dp_io *dp_io;
struct phy *phy;
-   int ret;
+   int ret = 0;


Drop this.



ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
dp_io = >parser->io;
phy = dp_io->phy;

+   /* main link is off */
+   if (!dp_power_clk_status(ctrl->power, DP_CTRL_PM))
+   return ret;


and then return 0?


+
/* set dongle to D3 (power off) mode */
dp_link_psm_config(ctrl->link, >panel->link_info, true);

@@ -1894,6 +1901,7 @@ int dp_ctrl_off(struct dp_ctrl *dp_ctrl)
}

phy_power_off(phy);
+
phy_exit(phy);

DRM_DEBUG_DP("DP off done\n");


Drop?

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c

index cdec0a3..5abd769 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1327,8 +1327,10 @@ static int dp_pm_suspend(struct device *dev)

mutex_lock(>event_mutex);

-   if (dp->core_initialized == true)
+   if (dp->core_initialized == true) {
+   dp_ctrl_off_link_stream(dp->ctrl);


Why not just check here for dp_power_clk_status()?


dp_display_host_deinit(dp);
+   }

dp->hpd_state = ST_SUSPENDED;

diff --git a/drivers/gpu/drm/msm/dp/dp_power.c 
b/drivers/gpu/drm/msm/dp/dp_power.c

index 9c4ea00..980924a9 100644
--- a/drivers/gpu/drm/msm/dp/dp_power.c
+++ b/drivers/gpu/drm/msm/dp/dp_power.c
@@ -262,6 +262,21 @@ int dp_power_clk_enable(struct dp_power 
*dp_power,

}
dp_power->core_clks_on = true;
}
+   } else {
+   if (pm_type == DP_CORE_PM && !dp_power->core_clks_on) 
{

+   DRM_DEBUG_DP("core clks already disabled\n");
+   return 0;
+   }
+
+   if (pm_type == DP_CTRL_PM && !dp_power->link_clks_on) 
{

+   DRM_DEBUG_DP("links clks already disabled\n");
+   return 0;
+   }
+
+   if (pm_type == DP_STREAM_PM && 
!dp_power->stream_clks_on) {

+   DRM_DEBUG_DP("pixel clks already disabled\n");
+   return 0;
+   }
}


If this happens isn't something wrong? Like we've somehow lost track of
the proper state and no we're trying to disable clks when we don't need
to. And given that clks already manage their own refcount that would be
pretty obvious if it went wrong

yes,
The problem is at suspend the link training has been done (link clk had 
been enabled)

Re: [PATCH 3/3] drm/msm/dp: Handle aux timeouts, nacks, defers

[khsieh]

On 2021-05-07 14:25, Stephen Boyd wrote:

Let's look at the irq status bits after a transfer and see if we got a
nack or a defer or a timeout, instead of telling drm layers that
everything was fine, while still printing an error message. I wasn't
sure about NACK+DEFER so I lumped all those various errors along with a
nack so that the drm core can figure out that things are just not going
well. The important thing is that we're now returning -ETIMEDOUT when
the message times out and nacks for bad addresses.

Cc: Dmitry Baryshkov 
Cc: Abhinav Kumar 
Cc: Kuogee Hsieh 
Cc: aravi...@codeaurora.org
Cc: Sean Paul 
Signed-off-by: Stephen Boyd 


Reviewed-by: Kuogee Hsieh 


---
 drivers/gpu/drm/msm/dp/dp_aux.c | 140 ++--
 drivers/gpu/drm/msm/dp/dp_aux.h |   8 --
 2 files changed, 61 insertions(+), 87 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
b/drivers/gpu/drm/msm/dp/dp_aux.c

index b49810396513..4a3293b590b0 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -9,7 +9,15 @@
 #include "dp_reg.h"
 #include "dp_aux.h"

-#define DP_AUX_ENUM_STR(x) #x
+enum msm_dp_aux_err {
+   DP_AUX_ERR_NONE,
+   DP_AUX_ERR_ADDR,
+   DP_AUX_ERR_TOUT,
+   DP_AUX_ERR_NACK,
+   DP_AUX_ERR_DEFER,
+   DP_AUX_ERR_NACK_DEFER,
+   DP_AUX_ERR_PHY,
+};

 struct dp_aux_private {
struct device *dev;
@@ -18,7 +26,7 @@ struct dp_aux_private {
struct mutex mutex;
struct completion comp;

-   u32 aux_error_num;
+   enum msm_dp_aux_err aux_error_num;
u32 retry_cnt;
bool cmd_busy;
bool native;
@@ -33,62 +41,45 @@ struct dp_aux_private {

 #define MAX_AUX_RETRIES5

-static const char *dp_aux_get_error(u32 aux_error)
-{
-   switch (aux_error) {
-   case DP_AUX_ERR_NONE:
-   return DP_AUX_ENUM_STR(DP_AUX_ERR_NONE);
-   case DP_AUX_ERR_ADDR:
-   return DP_AUX_ENUM_STR(DP_AUX_ERR_ADDR);
-   case DP_AUX_ERR_TOUT:
-   return DP_AUX_ENUM_STR(DP_AUX_ERR_TOUT);
-   case DP_AUX_ERR_NACK:
-   return DP_AUX_ENUM_STR(DP_AUX_ERR_NACK);
-   case DP_AUX_ERR_DEFER:
-   return DP_AUX_ENUM_STR(DP_AUX_ERR_DEFER);
-   case DP_AUX_ERR_NACK_DEFER:
-   return DP_AUX_ENUM_STR(DP_AUX_ERR_NACK_DEFER);
-   default:
-   return "unknown";
-   }
-}
-
-static u32 dp_aux_write(struct dp_aux_private *aux,
+static ssize_t dp_aux_write(struct dp_aux_private *aux,
struct drm_dp_aux_msg *msg)
 {
-   u32 data[4], reg, len;
+   u8 data[4];
+   u32 reg;
+   ssize_t len;
u8 *msgdata = msg->buffer;
int const AUX_CMD_FIFO_LEN = 128;
int i = 0;

if (aux->read)
-   len = 4;
+   len = 0;
else
-   len = msg->size + 4;
+   len = msg->size;

/*
 * cmd fifo only has depth of 144 bytes
 * limit buf length to 128 bytes here
 */
-   if (len > AUX_CMD_FIFO_LEN) {
+   if (len > AUX_CMD_FIFO_LEN - 4) {
DRM_ERROR("buf size greater than allowed size of 128 bytes\n");
-   return 0;
+   return -EINVAL;
}

/* Pack cmd and write to HW */
-   data[0] = (msg->address >> 16) & 0xf; /* addr[19:16] */
+   data[0] = (msg->address >> 16) & 0xf;  /* addr[19:16] */
if (aux->read)
-   data[0] |=  BIT(4); /* R/W */
+   data[0] |=  BIT(4); /* R/W */

-   data[1] = (msg->address >> 8) & 0xff;  /* addr[15:8] */
-   data[2] = msg->address & 0xff;   /* addr[7:0] */
-   data[3] = (msg->size - 1) & 0xff;/* len[7:0] */
+   data[1] = msg->address >> 8;   /* addr[15:8] */
+   data[2] = msg->address;  /* addr[7:0] */
+   data[3] = msg->size - 1; /* len[7:0] */

-   for (i = 0; i < len; i++) {
+   for (i = 0; i < len + 4; i++) {
reg = (i < 4) ? data[i] : msgdata[i - 4];
+   reg <<= DP_AUX_DATA_OFFSET;
+   reg &= DP_AUX_DATA_MASK;
+   reg |= DP_AUX_DATA_WRITE;
/* index = 0, write */
-   reg = (((reg) << DP_AUX_DATA_OFFSET)
-  & DP_AUX_DATA_MASK) | DP_AUX_DATA_WRITE;
if (i == 0)
reg |= DP_AUX_DATA_INDEX_WRITE;
aux->catalog->aux_data = reg;
@@ -116,39 +107,27 @@ static u32 dp_aux_write(struct dp_aux_private 
*aux,

return len;
 }

-static int dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
+static ssize_t dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
  struct drm_dp_aux_msg *msg)
 {
-   u32 ret, len, timeout;
-   int aux_timeout_ms = HZ/4;
+   ssize_t ret;
+   unsigned long time_left;

reinit_completion(>comp);

-   len = dp_aux_write(aux, 

Re: [PATCH 3/3] drm/msm/dp: Handle aux timeouts, nacks, defers

[khsieh]

On 2021-05-24 12:19, Stephen Boyd wrote:

Quoting khs...@codeaurora.org (2021-05-24 09:33:49)

On 2021-05-07 14:25, Stephen Boyd wrote:
> @@ -367,36 +347,38 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux
> *dp_aux,
>   }
>
>   ret = dp_aux_cmd_fifo_tx(aux, msg);
> -
>   if (ret < 0) {
>   if (aux->native) {
>   aux->retry_cnt++;
>   if (!(aux->retry_cnt % MAX_AUX_RETRIES))
>   dp_catalog_aux_update_cfg(aux->catalog);
>   }
> - usleep_range(400, 500); /* at least 400us to next try */
> - goto unlock_exit;
> - }

1) dp_catalog_aux_update_cfg(aux->catalog) will not work without
dp_catalog_aux_reset(aux->catalog);
dp_catalog_aux_reset(aux->catalog) will reset hpd control block and
potentially cause pending hpd interrupts got lost.
Therefore I think we should not do
dp_catalog_aux_update_cfg(aux->catalog) for now.
reset aux controller will reset hpd control block probolem will be 
fixed

at next chipset.
after that we can add dp_catalog_aux_update_cfg(aux->catalog) followed
by dp_catalog_aux_reset(aux->catalog) back at next chipset.


Hmm ok. So the phy calibration logic that tweaks the tuning values is
never used? Why can't the phy be tuned while it is active? I don't
understand why we would ever want to reset the aux phy when changing 
the

settings for a retry. Either way, this is not actually changing in this
patch so it would be another patch to remove this code.

ok,




2) according to DP specification, aux read/write failed have to wait 
at

least 400us before next try can start.
Otherwise, DP compliant test will failed


Yes. The caller of this function, drm_dp_dpcd_access(), has the delay
already

if (ret != 0 && ret != -ETIMEDOUT) {
usleep_range(AUX_RETRY_INTERVAL,
 AUX_RETRY_INTERVAL + 100);
}

so this delay here is redundant.

yes, you are right. This is enough.

Re: [PATCH 3/3] drm/msm/dp: Handle aux timeouts, nacks, defers

[khsieh]

On 2021-05-07 14:25, Stephen Boyd wrote:

Let's look at the irq status bits after a transfer and see if we got a
nack or a defer or a timeout, instead of telling drm layers that
everything was fine, while still printing an error message. I wasn't
sure about NACK+DEFER so I lumped all those various errors along with a
nack so that the drm core can figure out that things are just not going
well. The important thing is that we're now returning -ETIMEDOUT when
the message times out and nacks for bad addresses.

Cc: Dmitry Baryshkov 
Cc: Abhinav Kumar 
Cc: Kuogee Hsieh 
Cc: aravi...@codeaurora.org
Cc: Sean Paul 
Signed-off-by: Stephen Boyd 
---
 drivers/gpu/drm/msm/dp/dp_aux.c | 140 ++--
 drivers/gpu/drm/msm/dp/dp_aux.h |   8 --
 2 files changed, 61 insertions(+), 87 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
b/drivers/gpu/drm/msm/dp/dp_aux.c

index b49810396513..4a3293b590b0 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -9,7 +9,15 @@
 #include "dp_reg.h"
 #include "dp_aux.h"

-#define DP_AUX_ENUM_STR(x) #x
+enum msm_dp_aux_err {
+   DP_AUX_ERR_NONE,
+   DP_AUX_ERR_ADDR,
+   DP_AUX_ERR_TOUT,
+   DP_AUX_ERR_NACK,
+   DP_AUX_ERR_DEFER,
+   DP_AUX_ERR_NACK_DEFER,
+   DP_AUX_ERR_PHY,
+};

 struct dp_aux_private {
struct device *dev;
@@ -18,7 +26,7 @@ struct dp_aux_private {
struct mutex mutex;
struct completion comp;

-   u32 aux_error_num;
+   enum msm_dp_aux_err aux_error_num;
u32 retry_cnt;
bool cmd_busy;
bool native;
@@ -33,62 +41,45 @@ struct dp_aux_private {

 #define MAX_AUX_RETRIES5

-static const char *dp_aux_get_error(u32 aux_error)
-{
-   switch (aux_error) {
-   case DP_AUX_ERR_NONE:
-   return DP_AUX_ENUM_STR(DP_AUX_ERR_NONE);
-   case DP_AUX_ERR_ADDR:
-   return DP_AUX_ENUM_STR(DP_AUX_ERR_ADDR);
-   case DP_AUX_ERR_TOUT:
-   return DP_AUX_ENUM_STR(DP_AUX_ERR_TOUT);
-   case DP_AUX_ERR_NACK:
-   return DP_AUX_ENUM_STR(DP_AUX_ERR_NACK);
-   case DP_AUX_ERR_DEFER:
-   return DP_AUX_ENUM_STR(DP_AUX_ERR_DEFER);
-   case DP_AUX_ERR_NACK_DEFER:
-   return DP_AUX_ENUM_STR(DP_AUX_ERR_NACK_DEFER);
-   default:
-   return "unknown";
-   }
-}
-
-static u32 dp_aux_write(struct dp_aux_private *aux,
+static ssize_t dp_aux_write(struct dp_aux_private *aux,
struct drm_dp_aux_msg *msg)
 {
-   u32 data[4], reg, len;
+   u8 data[4];
+   u32 reg;
+   ssize_t len;
u8 *msgdata = msg->buffer;
int const AUX_CMD_FIFO_LEN = 128;
int i = 0;

if (aux->read)
-   len = 4;
+   len = 0;
else
-   len = msg->size + 4;
+   len = msg->size;

/*
 * cmd fifo only has depth of 144 bytes
 * limit buf length to 128 bytes here
 */
-   if (len > AUX_CMD_FIFO_LEN) {
+   if (len > AUX_CMD_FIFO_LEN - 4) {
DRM_ERROR("buf size greater than allowed size of 128 bytes\n");
-   return 0;
+   return -EINVAL;
}

/* Pack cmd and write to HW */
-   data[0] = (msg->address >> 16) & 0xf; /* addr[19:16] */
+   data[0] = (msg->address >> 16) & 0xf;  /* addr[19:16] */
if (aux->read)
-   data[0] |=  BIT(4); /* R/W */
+   data[0] |=  BIT(4); /* R/W */

-   data[1] = (msg->address >> 8) & 0xff;  /* addr[15:8] */
-   data[2] = msg->address & 0xff;   /* addr[7:0] */
-   data[3] = (msg->size - 1) & 0xff;/* len[7:0] */
+   data[1] = msg->address >> 8;   /* addr[15:8] */
+   data[2] = msg->address;  /* addr[7:0] */
+   data[3] = msg->size - 1; /* len[7:0] */

-   for (i = 0; i < len; i++) {
+   for (i = 0; i < len + 4; i++) {
reg = (i < 4) ? data[i] : msgdata[i - 4];
+   reg <<= DP_AUX_DATA_OFFSET;
+   reg &= DP_AUX_DATA_MASK;
+   reg |= DP_AUX_DATA_WRITE;
/* index = 0, write */
-   reg = (((reg) << DP_AUX_DATA_OFFSET)
-  & DP_AUX_DATA_MASK) | DP_AUX_DATA_WRITE;
if (i == 0)
reg |= DP_AUX_DATA_INDEX_WRITE;
aux->catalog->aux_data = reg;
@@ -116,39 +107,27 @@ static u32 dp_aux_write(struct dp_aux_private 
*aux,

return len;
 }

-static int dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
+static ssize_t dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
  struct drm_dp_aux_msg *msg)
 {
-   u32 ret, len, timeout;
-   int aux_timeout_ms = HZ/4;
+   ssize_t ret;
+   unsigned long time_left;

reinit_completion(>comp);

-   len = dp_aux_write(aux, msg);
-   if (len == 0) {
- 

Re: [PATCH 2/3] drm/msm/dp: Shrink locking area of dp_aux_transfer()

[khsieh]

On 2021-05-07 14:25, Stephen Boyd wrote:

We don't need to hold the lock to inspect the message we're going to
transfer, and we don't need to clear the busy flag either. Take the 
lock

later and bail out earlier if conditions aren't met.

Cc: Dmitry Baryshkov 
Cc: Abhinav Kumar 
Cc: Kuogee Hsieh 
Cc: aravi...@codeaurora.org
Cc: Sean Paul 
Signed-off-by: Stephen Boyd 


Reviewed-by: Kuogee Hsieh 

---
 drivers/gpu/drm/msm/dp/dp_aux.c | 19 +--
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
b/drivers/gpu/drm/msm/dp/dp_aux.c

index 91188466cece..b49810396513 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -329,30 +329,29 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux 
*dp_aux,

ssize_t ret;
int const aux_cmd_native_max = 16;
int const aux_cmd_i2c_max = 128;
-   struct dp_aux_private *aux = container_of(dp_aux,
-   struct dp_aux_private, dp_aux);
+   struct dp_aux_private *aux;

-   mutex_lock(>mutex);
+   aux = container_of(dp_aux, struct dp_aux_private, dp_aux);

 	aux->native = msg->request & (DP_AUX_NATIVE_WRITE & 
DP_AUX_NATIVE_READ);


/* Ignore address only message */
-   if ((msg->size == 0) || (msg->buffer == NULL)) {
+   if (msg->size == 0 || !msg->buffer) {
msg->reply = aux->native ?
DP_AUX_NATIVE_REPLY_ACK : DP_AUX_I2C_REPLY_ACK;
-   ret = msg->size;
-   goto unlock_exit;
+   return msg->size;
}

/* msg sanity check */
-   if ((aux->native && (msg->size > aux_cmd_native_max)) ||
-   (msg->size > aux_cmd_i2c_max)) {
+   if ((aux->native && msg->size > aux_cmd_native_max) ||
+   msg->size > aux_cmd_i2c_max) {
DRM_ERROR("%s: invalid msg: size(%zu), request(%x)\n",
__func__, msg->size, msg->request);
-   ret = -EINVAL;
-   goto unlock_exit;
+   return -EINVAL;
}

+   mutex_lock(>mutex);
+
dp_aux_update_offset_and_segment(aux, msg);
dp_aux_transfer_helper(aux, msg, true);

Re: [PATCH 1/3] drm/msm/dp: Simplify aux irq handling code

[khsieh]

On 2021-05-07 14:25, Stephen Boyd wrote:

We don't need to stash away 'isr' in the aux structure to pass to two
functions. Let's use a local variable instead. And we can complete the
completion variable in one place instead of two to simplify the code.

Cc: Dmitry Baryshkov 
Cc: Abhinav Kumar 
Cc: Kuogee Hsieh 
Cc: aravi...@codeaurora.org
Cc: Sean Paul 
Signed-off-by: Stephen Boyd 


Reviewed-by: Kuogee Hsieh 

---
 drivers/gpu/drm/msm/dp/dp_aux.c | 22 --
 drivers/gpu/drm/msm/dp/dp_catalog.c |  2 +-
 drivers/gpu/drm/msm/dp/dp_catalog.h |  2 +-
 3 files changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
b/drivers/gpu/drm/msm/dp/dp_aux.c

index 7c22bfe0fc7d..91188466cece 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -27,7 +27,6 @@ struct dp_aux_private {
bool no_send_stop;
u32 offset;
u32 segment;
-   u32 isr;

struct drm_dp_aux dp_aux;
 };
@@ -181,10 +180,8 @@ static void dp_aux_cmd_fifo_rx(struct 
dp_aux_private *aux,

}
 }

-static void dp_aux_native_handler(struct dp_aux_private *aux)
+static void dp_aux_native_handler(struct dp_aux_private *aux, u32 isr)
 {
-   u32 isr = aux->isr;
-
if (isr & DP_INTR_AUX_I2C_DONE)
aux->aux_error_num = DP_AUX_ERR_NONE;
else if (isr & DP_INTR_WRONG_ADDR)
@@ -197,14 +194,10 @@ static void dp_aux_native_handler(struct
dp_aux_private *aux)
aux->aux_error_num = DP_AUX_ERR_PHY;
dp_catalog_aux_clear_hw_interrupts(aux->catalog);
}
-
-   complete(>comp);
 }

-static void dp_aux_i2c_handler(struct dp_aux_private *aux)
+static void dp_aux_i2c_handler(struct dp_aux_private *aux, u32 isr)
 {
-   u32 isr = aux->isr;
-
if (isr & DP_INTR_AUX_I2C_DONE) {
if (isr & (DP_INTR_I2C_NACK | DP_INTR_I2C_DEFER))
aux->aux_error_num = DP_AUX_ERR_NACK;
@@ -226,8 +219,6 @@ static void dp_aux_i2c_handler(struct 
dp_aux_private *aux)

dp_catalog_aux_clear_hw_interrupts(aux->catalog);
}
}
-
-   complete(>comp);
 }

 static void dp_aux_update_offset_and_segment(struct dp_aux_private 
*aux,
@@ -412,6 +403,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux 
*dp_aux,


 void dp_aux_isr(struct drm_dp_aux *dp_aux)
 {
+   u32 isr;
struct dp_aux_private *aux;

if (!dp_aux) {
@@ -421,15 +413,17 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux)

aux = container_of(dp_aux, struct dp_aux_private, dp_aux);

-   aux->isr = dp_catalog_aux_get_irq(aux->catalog);
+   isr = dp_catalog_aux_get_irq(aux->catalog);

if (!aux->cmd_busy)
return;

if (aux->native)
-   dp_aux_native_handler(aux);
+   dp_aux_native_handler(aux, isr);
else
-   dp_aux_i2c_handler(aux);
+   dp_aux_i2c_handler(aux, isr);
+
+   complete(>comp);
 }

 void dp_aux_reconfig(struct drm_dp_aux *dp_aux)
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
b/drivers/gpu/drm/msm/dp/dp_catalog.c
index b1a9b1b98f5f..a70c238f34b0 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -292,7 +292,7 @@ void dp_catalog_dump_regs(struct dp_catalog 
*dp_catalog)

dump_regs(catalog->io->dp_controller.base + offset, len);
 }

-int dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog)
+u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog)
 {
struct dp_catalog_private *catalog = container_of(dp_catalog,
struct dp_catalog_private, dp_catalog);
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h
b/drivers/gpu/drm/msm/dp/dp_catalog.h
index 176a9020a520..502bc0dc7787 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.h
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
@@ -80,7 +80,7 @@ int dp_catalog_aux_clear_hw_interrupts(struct
dp_catalog *dp_catalog);
 void dp_catalog_aux_reset(struct dp_catalog *dp_catalog);
 void dp_catalog_aux_enable(struct dp_catalog *dp_catalog, bool 
enable);

 void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog);
-int dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog);
+u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog);

 /* DP Controller APIs */
 void dp_catalog_ctrl_state_ctrl(struct dp_catalog *dp_catalog, u32 
state);

Re: [PATCH 0/3] drm/msm/dp: Simplify aux code

[khsieh]

On 2021-05-21 14:57, Stephen Boyd wrote:

Quoting Stephen Boyd (2021-05-07 14:25:02)

Here's a few patches that simplify the aux handling code and bubble up
timeouts and nacks to the upper DRM layers. The goal is to get DRM to
know that the other side isn't there or that there's been a timeout,
instead of saying that everything is fine and putting some error 
message

into the logs.

Cc: Dmitry Baryshkov 
Cc: Abhinav Kumar 
Cc: Kuogee Hsieh 
Cc: aravi...@codeaurora.org
Cc: Sean Paul 



Kuogee, have you had a change to review this series?


Sorry  missed this one.
Will review it now.

Stephen Boyd (3):
  drm/msm/dp: Simplify aux irq handling code
  drm/msm/dp: Shrink locking area of dp_aux_transfer()
  drm/msm/dp: Handle aux timeouts, nacks, defers

 drivers/gpu/drm/msm/dp/dp_aux.c | 181 


 drivers/gpu/drm/msm/dp/dp_aux.h |   8 --
 drivers/gpu/drm/msm/dp/dp_catalog.c |   2 +-
 drivers/gpu/drm/msm/dp/dp_catalog.h |   2 +-
 4 files changed, 80 insertions(+), 113 deletions(-)


base-commit: 51595e3b4943b0079638b2657f603cf5c8ea3a66
--
https://chromeos.dev

Re: [PATCH 1/2] drm/msm/dp: service only one irq_hpd if there are multiple irq_hpd pending

[khsieh]

On 2021-04-29 20:11, Stephen Boyd wrote:

Quoting khs...@codeaurora.org (2021-04-29 10:23:31)

On 2021-04-29 02:26, Stephen Boyd wrote:
> Quoting khs...@codeaurora.org (2021-04-28 10:38:11)
>> On 2021-04-27 17:00, Stephen Boyd wrote:
>> > Quoting aravi...@codeaurora.org (2021-04-21 11:55:21)
>> >> On 2021-04-21 10:26, khs...@codeaurora.org wrote:
>> >> >>
>> >> >>> +
>> >> >>> mutex_unlock(>event_mutex);
>> >> >>>
>> >> >>> return 0;
>> >> >>> @@ -1496,6 +1502,9 @@ int msm_dp_display_disable(struct msm_dp *dp,
>> >> >>> struct drm_encoder *encoder)
>> >> >>> /* stop sentinel checking */
>> >> >>> dp_del_event(dp_display, EV_DISCONNECT_PENDING_TIMEOUT);
>> >> >>>
>> >> >>> +   /* link is down, delete pending irq_hdps */
>> >> >>> +   dp_del_event(dp_display, EV_IRQ_HPD_INT);
>> >> >>> +
>> >> >>
>> >> >> I'm becoming convinced that the whole kthread design and event queue
>> >> >> is
>> >> >> broken. These sorts of patches are working around the larger problem
>> >> >> that the kthread is running independently of the driver and irqs can
>> >> >> come in at any time but the event queue is not checked from the irq
>> >> >> handler to debounce the irq event. Is the event queue necessary at
>> >> >> all?
>> >> >> I wonder if it would be simpler to just use an irq thread and process
>> >> >> the hpd signal from there. Then we're guaranteed to not get an irq
>> >> >> again
>> >> >> until the irq thread is done processing the event. This would
>> >> >> naturally
>> >> >> debounce the irq hpd event that way.
>> >> > event q just like bottom half of irq handler. it turns irq into event
>> >> > and handle them sequentially.
>> >> > irq_hpd is asynchronous event from panel to bring up attention of hsot
>> >> > during run time of operation.
>> >> > Here, the dongle is unplugged and main link had teared down so that no
>> >> > need to service pending irq_hpd if any.
>> >> >
>> >>
>> >> As Kuogee mentioned, IRQ_HPD is a message received from the panel and
>> >> is
>> >> not like your typical HW generated IRQ. There is no guarantee that we
>> >> will not receive an IRQ_HPD until we are finished with processing of
>> >> an
>> >> earlier HPD message or an IRQ_HPD message. For example - when you run
>> >> the protocol compliance, when we get a HPD from the sink, we are
>> >> expected to start reading DPCD, EDID and proceed with link training.
>> >> As
>> >> soon as link training is finished (which is marked by a specific DPCD
>> >> register write), the sink is going to issue an IRQ_HPD. At this point,
>> >> we may not done with processing the HPD high as after link training we
>> >> would typically notify the user mode of the newly connected display,
>> >> etc.


I re-read this. I think you're saying that IRQ_HPD can come in after 
HPD
goes high and we finish link training? That sounds like we should 
enable

IRQ_HPD in the hardware once we finish link training, instead of having
it enabled all the time. Then we can finish the threaded irq handler 
and
the irq should be pending again once IRQ_HPD is sent over. Is there 
ever

a need to be processing some IRQ_HPD and then get another IRQ_HPD while
processing the first one?

yes, for example
1) plug dongle only
2) plug hdmi monitor into dongle (generated irq_hpd with sinc_count = 1)
3) unplug hdmi monitor out of the dongle (generate irq_hpd with 
sinc_count = 0)

4) go back to 2) for n times
5) unplug dongle

This patch is not fix this problem either.
The existing code has major issue which is handle irq_hpd with 
sink_count = 0 same way as handle of dongle unplugged.
I think this cause external dp display failed to work and cause crash at 
suspend/resume test case.

I will drop this patch.
I am working on handle irq_hpd with sink_count = 0 as asymmetric as 
opposite to  irq_hpd with sink_count = 1.
This means irq_hdp sink_count = 0 handle only tear down the main link 
but keep phy/aux intact.

I will re submit patch for review.





>> >
>> > Given that the irq comes in and is then forked off to processing at a
>> > later time implies that IRQ_HPD can come in at practically anytime.
>> > Case
>> > in point, this patch, which is trying to selectively search through the
>> > "event queue" and then remove the event that is no longer relevant
>> > because the display is being turned off either by userspace or because
>> > HPD has gone away. If we got rid of the queue and kthread and processed
>> > irqs in a threaded irq handler I suspect the code would be simpler and
>> > not have to search through an event queue when we disable the display.
>> > Instead while disabling the display we would make sure that the irq
>> > thread isn't running anymore with synchronize_irq() or even disable the
>> > irq entirely, but really it would be better to just disable the irq in
>> > the hardware with a register write to some irq mask register.
>> >
>> > This pushes more of the logic for HPD and connect/disconnect into the
>> > hardware and avoids 

Re: [PATCH 1/2] drm/msm/dp: service only one irq_hpd if there are multiple irq_hpd pending

[khsieh]

On 2021-04-29 02:26, Stephen Boyd wrote:

Quoting khs...@codeaurora.org (2021-04-28 10:38:11)

On 2021-04-27 17:00, Stephen Boyd wrote:
> Quoting aravi...@codeaurora.org (2021-04-21 11:55:21)
>> On 2021-04-21 10:26, khs...@codeaurora.org wrote:
>> >>
>> >>> +
>> >>> mutex_unlock(>event_mutex);
>> >>>
>> >>> return 0;
>> >>> @@ -1496,6 +1502,9 @@ int msm_dp_display_disable(struct msm_dp *dp,
>> >>> struct drm_encoder *encoder)
>> >>> /* stop sentinel checking */
>> >>> dp_del_event(dp_display, EV_DISCONNECT_PENDING_TIMEOUT);
>> >>>
>> >>> +   /* link is down, delete pending irq_hdps */
>> >>> +   dp_del_event(dp_display, EV_IRQ_HPD_INT);
>> >>> +
>> >>
>> >> I'm becoming convinced that the whole kthread design and event queue
>> >> is
>> >> broken. These sorts of patches are working around the larger problem
>> >> that the kthread is running independently of the driver and irqs can
>> >> come in at any time but the event queue is not checked from the irq
>> >> handler to debounce the irq event. Is the event queue necessary at
>> >> all?
>> >> I wonder if it would be simpler to just use an irq thread and process
>> >> the hpd signal from there. Then we're guaranteed to not get an irq
>> >> again
>> >> until the irq thread is done processing the event. This would
>> >> naturally
>> >> debounce the irq hpd event that way.
>> > event q just like bottom half of irq handler. it turns irq into event
>> > and handle them sequentially.
>> > irq_hpd is asynchronous event from panel to bring up attention of hsot
>> > during run time of operation.
>> > Here, the dongle is unplugged and main link had teared down so that no
>> > need to service pending irq_hpd if any.
>> >
>>
>> As Kuogee mentioned, IRQ_HPD is a message received from the panel and
>> is
>> not like your typical HW generated IRQ. There is no guarantee that we
>> will not receive an IRQ_HPD until we are finished with processing of
>> an
>> earlier HPD message or an IRQ_HPD message. For example - when you run
>> the protocol compliance, when we get a HPD from the sink, we are
>> expected to start reading DPCD, EDID and proceed with link training.
>> As
>> soon as link training is finished (which is marked by a specific DPCD
>> register write), the sink is going to issue an IRQ_HPD. At this point,
>> we may not done with processing the HPD high as after link training we
>> would typically notify the user mode of the newly connected display,
>> etc.
>
> Given that the irq comes in and is then forked off to processing at a
> later time implies that IRQ_HPD can come in at practically anytime.
> Case
> in point, this patch, which is trying to selectively search through the
> "event queue" and then remove the event that is no longer relevant
> because the display is being turned off either by userspace or because
> HPD has gone away. If we got rid of the queue and kthread and processed
> irqs in a threaded irq handler I suspect the code would be simpler and
> not have to search through an event queue when we disable the display.
> Instead while disabling the display we would make sure that the irq
> thread isn't running anymore with synchronize_irq() or even disable the
> irq entirely, but really it would be better to just disable the irq in
> the hardware with a register write to some irq mask register.
>
> This pushes more of the logic for HPD and connect/disconnect into the
> hardware and avoids reimplementing that in software: searching through
> the queue, checking for duplicate events, etc.

I wish we can implemented as you suggested. but it more complicate 
than

that.
Let me explain below,
we have 3 transactions defined as below,

plugin transaction: irq handle do host dp ctrl initialization and link
training. If sink_count = 0 or link train failed, then transaction
ended. otherwise send display up uevent to frame work and wait for 
frame

work thread to do mode set, start pixel clock and start video to end
transaction.


Why do we need to wait for userspace to start video? HPD is indicating
that we have something connected, so shouldn't we merely signal to
userspace that something is ready to display and then enable the irq 
for

IRQ_HPD?


yes, it is correct.
The problem is unplug happen after signal user space.
if unplug happen before user space start mode set and video, then it can 
just do nothing and return.
but if unplugged happen at the middle of user space doing mode set and 
start video?


remember we had run into problem system show in connect state when 
dongle unplugged, vice versa.







unplugged transaction: irq handle send display off uevent to frame
work and wait for frame work to disable pixel clock ,tear down main
link and dp ctrl host de initialization.


What do we do if userspace is slow and doesn't disable the display
before the cable is physically plugged in again?

plugin is not handle (re enter back into event q) until unplugged handle 
completed.


irq_hpd transaction: This only happen 

Re: [PATCH 1/2] drm/msm/dp: service only one irq_hpd if there are multiple irq_hpd pending

[khsieh]

On 2021-04-27 17:00, Stephen Boyd wrote:

Quoting aravi...@codeaurora.org (2021-04-21 11:55:21)

On 2021-04-21 10:26, khs...@codeaurora.org wrote:
>>
>>> +
>>> mutex_unlock(>event_mutex);
>>>
>>> return 0;
>>> @@ -1496,6 +1502,9 @@ int msm_dp_display_disable(struct msm_dp *dp,
>>> struct drm_encoder *encoder)
>>> /* stop sentinel checking */
>>> dp_del_event(dp_display, EV_DISCONNECT_PENDING_TIMEOUT);
>>>
>>> +   /* link is down, delete pending irq_hdps */
>>> +   dp_del_event(dp_display, EV_IRQ_HPD_INT);
>>> +
>>
>> I'm becoming convinced that the whole kthread design and event queue
>> is
>> broken. These sorts of patches are working around the larger problem
>> that the kthread is running independently of the driver and irqs can
>> come in at any time but the event queue is not checked from the irq
>> handler to debounce the irq event. Is the event queue necessary at
>> all?
>> I wonder if it would be simpler to just use an irq thread and process
>> the hpd signal from there. Then we're guaranteed to not get an irq
>> again
>> until the irq thread is done processing the event. This would
>> naturally
>> debounce the irq hpd event that way.
> event q just like bottom half of irq handler. it turns irq into event
> and handle them sequentially.
> irq_hpd is asynchronous event from panel to bring up attention of hsot
> during run time of operation.
> Here, the dongle is unplugged and main link had teared down so that no
> need to service pending irq_hpd if any.
>

As Kuogee mentioned, IRQ_HPD is a message received from the panel and 
is

not like your typical HW generated IRQ. There is no guarantee that we
will not receive an IRQ_HPD until we are finished with processing of 
an

earlier HPD message or an IRQ_HPD message. For example - when you run
the protocol compliance, when we get a HPD from the sink, we are
expected to start reading DPCD, EDID and proceed with link training. 
As

soon as link training is finished (which is marked by a specific DPCD
register write), the sink is going to issue an IRQ_HPD. At this point,
we may not done with processing the HPD high as after link training we
would typically notify the user mode of the newly connected display,
etc.


Given that the irq comes in and is then forked off to processing at a
later time implies that IRQ_HPD can come in at practically anytime. 
Case

in point, this patch, which is trying to selectively search through the
"event queue" and then remove the event that is no longer relevant
because the display is being turned off either by userspace or because
HPD has gone away. If we got rid of the queue and kthread and processed
irqs in a threaded irq handler I suspect the code would be simpler and
not have to search through an event queue when we disable the display.
Instead while disabling the display we would make sure that the irq
thread isn't running anymore with synchronize_irq() or even disable the
irq entirely, but really it would be better to just disable the irq in
the hardware with a register write to some irq mask register.

This pushes more of the logic for HPD and connect/disconnect into the
hardware and avoids reimplementing that in software: searching through
the queue, checking for duplicate events, etc.


I wish we can implemented as you suggested. but it more complicate than 
that.

Let me explain below,
we have 3 transactions defined as below,

plugin transaction: irq handle do host dp ctrl initialization and link 
training. If sink_count = 0 or link train failed, then transaction 
ended. otherwise send display up uevent to frame work and wait for frame 
work thread to do mode set, start pixel clock and start video to end 
transaction.


unplugged transaction: irq handle send display off uevent to frame work 
and wait for frame work to disable pixel clock ,tear down main link and 
dp ctrl host de initialization.


irq_hpd transaction: This only happen after plugin transaction and 
before unplug transaction. irq handle read panel dpcd register and 
perform requesting action. Action including perform dp compliant 
phy/link testing.


since dongle can be plugged/unplugged at ant time, three conditions have 
to be met to avoid race condition,

1) no irq lost
2) irq happen timing order enforced at execution
3) no irq handle done in the middle transaction

for example we do not want to see
plugin --> unplug --> plugin --> unplug become plugin --> plugin--> 
unplug


The purpose of this patch is to not handle pending irq_hpd after either 
dongle or monitor had been unplugged until next plug in.





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

Re: [PATCH v3 3/3] drm/msm/dp: check main link status before start aux read

[khsieh]

On 2021-04-20 16:38, Stephen Boyd wrote:

Quoting Kuogee Hsieh (2021-04-16 10:38:51)

Maybe when the cable is disconnected the DP phy should be shutdown and
some bit in the phy could effectively "cut off" the aux channel and 
then

NAKs would start coming through here in the DP controller I/O register
space. This patch have DP aux channel read/write to return NAK 
immediately

if DP controller connection status is in unplugged state.

Changes in V3:
-- check core_initialized before handle irq_hpd
Signed-off-by: Kuogee Hsieh 
---
 drivers/gpu/drm/msm/dp/dp_aux.c |  5 +
 drivers/gpu/drm/msm/dp/dp_display.c | 14 ++
 drivers/gpu/drm/msm/dp/dp_link.c| 20 +++-
 3 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
b/drivers/gpu/drm/msm/dp/dp_aux.c

index 7c22bfe..fae3806 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -343,6 +343,11 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux 
*dp_aux,


mutex_lock(>mutex);

+   if (!dp_catalog_link_is_connected(aux->catalog)) {
+   ret = -ETIMEDOUT;
+   goto unlock_exit;
+   }
+


This still makes me concerned. Any possibility to not do this and have
the phy cut the connection off and have this transfer timeout
immediately?

no, we have to wait hardware AUX_NACK timeout.
only this or the abort flag used last time.
Last time you have kernel crash because of service irq_hpd while clock 
is turned off.

I have add core_initialized checking wiinin irq_hpd to prevent this.
I think abort flag approach is safer.



aux->native = msg->request & (DP_AUX_NATIVE_WRITE & 
DP_AUX_NATIVE_READ);


/* Ignore address only message */
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c

index 1784e11..db3f45e 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -571,7 +571,7 @@ static int dp_hpd_plug_handle(struct 
dp_display_private *dp, u32 data)

dp->hpd_state = ST_DISCONNECTED;

if (ret == -ECONNRESET) { /* cable unplugged */
-   dp->core_initialized = false;
+   DRM_ERROR("dongle unplugged = %d\n", ret);


Is this a debug message?


}

} else {
@@ -711,9 +711,15 @@ static int dp_irq_hpd_handle(struct 
dp_display_private *dp, u32 data)

return 0;
}

-   ret = dp_display_usbpd_attention_cb(>pdev->dev);
-   if (ret == -ECONNRESET) { /* cable unplugged */
-   dp->core_initialized = false;
+   /*
+* dp core (ahb/aux clks) must be initialized before
+* irq_hpd be handled
+*/
+   if (dp->core_initialized) {
+   ret = dp_display_usbpd_attention_cb(>pdev->dev);
+   if (ret == -ECONNRESET) { /* cable unplugged */
+   DRM_ERROR("dongle unplugged = %d\n", ret);


Another debug message?


+   }
}

mutex_unlock(>event_mutex);
diff --git a/drivers/gpu/drm/msm/dp/dp_link.c 
b/drivers/gpu/drm/msm/dp/dp_link.c

index be986da..53ecae6 100644
--- a/drivers/gpu/drm/msm/dp/dp_link.c
+++ b/drivers/gpu/drm/msm/dp/dp_link.c
@@ -737,18 +737,25 @@ static int dp_link_parse_sink_count(struct 
dp_link *dp_link)

return 0;
 }

-static void dp_link_parse_sink_status_field(struct dp_link_private 
*link)
+static int dp_link_parse_sink_status_field(struct dp_link_private 
*link)

 {
int len = 0;

link->prev_sink_count = link->dp_link.sink_count;
-   dp_link_parse_sink_count(>dp_link);
+   len = dp_link_parse_sink_count(>dp_link);
+   if (len < 0) {
+   DRM_ERROR("DP parse sink count failed\n");
+   return len;
+   }

len = drm_dp_dpcd_read_link_status(link->aux,
link->link_status);
-   if (len < DP_LINK_STATUS_SIZE)
+   if (len < DP_LINK_STATUS_SIZE) {
DRM_ERROR("DP link status read failed\n");
-   dp_link_parse_request(link);
+   return len;
+   }
+
+   return dp_link_parse_request(link);
 }

 /**
@@ -1032,7 +1039,10 @@ int dp_link_process_request(struct dp_link 
*dp_link)


dp_link_reset_data(link);

-   dp_link_parse_sink_status_field(link);
+   ret = dp_link_parse_sink_status_field(link);
+   if (ret) {
+   return ret;
+   }

if (link->request.test_requested == DP_TEST_LINK_EDID_READ) {
dp_link->sink_request |= DP_TEST_LINK_EDID_READ;
--


Can you split this part off into another patch? It seems to stand on 
its

own as it makes the code more robust to transfer errors in the sink
parsing code.

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

Re: [PATCH 1/2] drm/msm/dp: service only one irq_hpd if there are multiple irq_hpd pending

[khsieh]

On 2021-04-20 15:01, Stephen Boyd wrote:

Quoting Kuogee Hsieh (2021-04-16 13:27:57)
Some dongle may generate more than one irq_hpd events in a short 
period of
time. This patch will treat those irq_hpd events as single one and 
service

only one irq_hpd event.


Why is it bad to get multiple irq_hpd events in a short period of time?
Please tell us here in the commit text.



Signed-off-by: Kuogee Hsieh 
---
 drivers/gpu/drm/msm/dp/dp_display.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c

index 5a39da6..0a7d383 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -707,6 +707,9 @@ static int dp_irq_hpd_handle(struct 
dp_display_private *dp, u32 data)

return 0;
}

+   /* only handle first irq_hpd in case of multiple irs_hpd 
pending */

+   dp_del_event(dp, EV_IRQ_HPD_INT);
+
ret = dp_display_usbpd_attention_cb(>pdev->dev);
if (ret == -ECONNRESET) { /* cable unplugged */
dp->core_initialized = false;
@@ -1300,6 +1303,9 @@ static int dp_pm_suspend(struct device *dev)
/* host_init will be called at pm_resume */
dp->core_initialized = false;

+   /* system suspended, delete pending irq_hdps */
+   dp_del_event(dp, EV_IRQ_HPD_INT);


What happens if I suspend my device and when this function is running I
toggle my monitor to use the HDMI input that is connected instead of 
some

other input, maybe the second HDMI input? Wouldn't that generate an HPD
interrupt to grab the attention of this device?

no,
At this time display is off. this mean dp controller is off and mainlink 
has teared down.
it will start with plug in interrupt to bring dp controller up and start 
link training.
irq_hpd can be generated only panel is at run time of operation mode and 
need attention from host.

If host is shutting down, then no need to service pending irq_hpd.




+
mutex_unlock(>event_mutex);

return 0;
@@ -1496,6 +1502,9 @@ int msm_dp_display_disable(struct msm_dp *dp, 
struct drm_encoder *encoder)

/* stop sentinel checking */
dp_del_event(dp_display, EV_DISCONNECT_PENDING_TIMEOUT);

+   /* link is down, delete pending irq_hdps */
+   dp_del_event(dp_display, EV_IRQ_HPD_INT);
+


I'm becoming convinced that the whole kthread design and event queue is
broken. These sorts of patches are working around the larger problem
that the kthread is running independently of the driver and irqs can
come in at any time but the event queue is not checked from the irq
handler to debounce the irq event. Is the event queue necessary at all?
I wonder if it would be simpler to just use an irq thread and process
the hpd signal from there. Then we're guaranteed to not get an irq 
again

until the irq thread is done processing the event. This would naturally
debounce the irq hpd event that way.
event q just like bottom half of irq handler. it turns irq into event 
and handle them sequentially.
irq_hpd is asynchronous event from panel to bring up attention of hsot 
during run time of operation.
Here, the dongle is unplugged and main link had teared down so that no 
need to service pending irq_hpd if any.






dp_display_disable(dp_display, 0);

rc = dp_display_unprepare(dp);

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

Re: [PATCH v2 3/3] drm/msm/dp: check main link status before start aux read

[khsieh]

On 2021-04-15 13:06, Stephen Boyd wrote:

Quoting khs...@codeaurora.org (2021-04-15 10:37:29)

On 2021-04-14 14:09, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2021-04-13 16:11:44)
>> Make sure main link is in connection state before start aux
>> read/write operation to avoid unnecessary long delay due to
>> main link had been unplugged.
>>
>> Signed-off-by: Kuogee Hsieh 
>> ---
>>  drivers/gpu/drm/msm/dp/dp_aux.c  |  5 +
>>  drivers/gpu/drm/msm/dp/dp_link.c | 20 +++-
>>  2 files changed, 20 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c
>> b/drivers/gpu/drm/msm/dp/dp_aux.c
>> index 7c22bfe..fae3806 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
>> @@ -343,6 +343,11 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux
>> *dp_aux,
>>
>> mutex_lock(>mutex);
>>
>> +   if (!dp_catalog_link_is_connected(aux->catalog)) {
>> +   ret = -ETIMEDOUT;
>> +   goto unlock_exit;
>> +   }
>
> I get a crash here sometimes when plugging and unplugging an apple HDMI
> dongle during suspend/resume. I guess device power management
> (dp_pm_suspend()) is happening in parallel with aux transfers from the
> kthread. Why doesn't the aux communication start reporting NAKs once
> the
> cable is disconnected?
>
> [  366.210058] hdmi-audio-codec hdmi-audio-codec.15.auto: calling
> platform_pm_suspend+0x0/0x60 @ 7175, parent:
> ae9.displayport-controller
> [  366.222825] hdmi-audio-codec hdmi-audio-codec.15.auto:
> platform_pm_suspend+0x0/0x60 returned 0 after 1 usecs
> [  366.232939] msm-dp-display ae9.displayport-controller: calling
> dp_pm_suspend+0x0/0x80 @ 7175, parent: ae0.mdss
> [  366.244006] msm-dp-display ae9.displayport-controller:
> dp_pm_suspend+0x0/0x80 returned 0 after 79 usecs
> [  366.254025] msm_dsi ae94000.dsi: calling
> pm_runtime_force_suspend+0x0/0xb4 @ 7175, parent: ae0.mdss
> [  366.263669] msm_dsi ae94000.dsi: pm_runtime_force_suspend+0x0/0xb4
> returned 0 after 0 usecs
> [  366.272290] panel-simple panel: calling
> platform_pm_suspend+0x0/0x60 @ 7175, parent: platform
> [  366.272501] ti_sn65dsi86 2-002d: calling
> pm_runtime_force_suspend+0x0/0xb4 @ 176, parent: i2c-2
> [  366.281055] panel-simple panel: platform_pm_suspend+0x0/0x60
> returned 0 after 0 usecs
> [  366.281081] leds mmc1::: calling led_suspend+0x0/0x4c @ 7175,
> parent: 7c4000.sdhci
> [  366.290065] ti_sn65dsi86 2-002d: pm_runtime_force_suspend+0x0/0xb4
> returned 0 after 0 usecs
> [  366.298046] leds mmc1::: led_suspend+0x0/0x4c returned 0 after 1
> usecs
> [  366.302994] Internal error: synchronous external abort: 9610
> [#1] PREEMPT SMP
> [  366.303006] Modules linked in: vhost_vsock
> vmw_vsock_virtio_transport_common vsock vhost rfcomm algif_hash
> algif_skcipher af_alg xt_cgroup uinput xt_MASQUERADE venus_enc
> hci_uart venus_dec btqca cros_ec_typec typec bluetooth qcom_spmi_adc5
> snd_soc_sc7180 qcom_spmi_temp_alarm ecdh_generic qcom_spmi_adc_tm5
> qcom_vadc_common snd_soc_qcom_common ecc snd_soc_rt5682_i2c
> snd_soc_rt5682 snd_soc_rl6231 venus_core v4l2_mem2mem
> snd_soc_lpass_sc7180 snd_soc_lpass_hdmi snd_soc_lpass_cpu
> snd_soc_lpass_platform snd_soc_max98357a fuse iio_trig_sysfs
> cros_ec_sensors cros_ec_sensors_ring cros_ec_lid_angle
> cros_ec_sensors_core industrialio_triggered_buffer kfifo_buf
> cros_ec_sensorhub lzo_rle lzo_compress zram ath10k_snoc ath10k_core
> ath mac80211 cfg80211 cdc_ether usbnet r8152 mii uvcvideo
> videobuf2_vmalloc joydev
> [  366.303211] CPU: 0 PID: 224 Comm: dp_hpd_handler Not tainted 5.4.109
> #27
> [  366.303216] Hardware name: Google Lazor (rev3+) with KB Backlight
> (DT)
> [  366.303225] pstate: 60c9 (nZCv daif +PAN +UAO)
> [  366.303234] pc : dp_catalog_link_is_connected+0x20/0x34
> [  366.303244] lr : dp_aux_transfer+0x44/0x284
> [  366.303250] sp : ffc011bfbbe0
> [  366.303254] x29: ffc011bfbbe0 x28: 
> [  366.303262] x27: 000c x26: ff896f8212bc
> [  366.303269] x25: 0001 x24: 0001
> [  366.303275] x23: 0020 x22: ff896ff82118
> [  366.303282] x21: ffc011bfbc50 x20: ff896ff82090
> [  366.303289] x19: ff896ffc3598 x18: 
> [  366.303295] x17:  x16: 0001
> [  366.303303] x15:  x14: 02ef
> [  366.303311] x13: 0400 x12: ffeb896ea060
> [  366.303317] x11:  x10: 
> [  366.303324] x9 : ff896f061100 x8 : ffc010582204
> [  366.303331] x7 : 00b2b5593519 x6 : 003033ff
> [  366.303338] x5 :  x4 : 0001
> [  366.303345] x3 : ff896ff432a1 x2 : 
> [  366.303352] x1 : 0119 x0 : ff896ffc3598
> [  366.303360] Call trace:
> [  366.303367]  dp_catalog_link_is_connected+0x20/0x34
> [  366.303374]  dp_aux_transfer+0x44/0x284mutex.
> [  366.303387]  

Re: [PATCH v2 2/3] drm/msm/dp: initialize audio_comp when audio starts

[khsieh]

On 2021-04-14 14:22, Stephen Boyd wrote:

Quoting Kuogee Hsieh (2021-04-14 14:02:50)

Initialize audio_comp when audio starts and wait for audio_comp at
dp_display_disable(). This will take care of both dongle unplugged
and display off (suspend) cases.

Changes in v2:
-- add dp_display_start_audio()

Signed-off-by: Kuogee Hsieh 


Looking better. Thanks!

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c

index 0ba71c7..8a69bcd 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -177,6 +177,14 @@ static int dp_del_event(struct dp_display_private 
*dp_priv, u32 event)


return 0;
 }
+void dp_display_start_audio(struct msm_dp *dp_display)


Please unstick this from previous function by adding a newline above.


+{
+   struct dp_display_private *dp;
+
+   dp = container_of(dp_display, struct dp_display_private, 
dp_display);

+
+   reinit_completion(>audio_comp);
+}

 void dp_display_signal_audio_complete(struct msm_dp *dp_display)
 {
@@ -648,10 +656,6 @@ static int dp_hpd_unplug_handle(struct 
dp_display_private *dp, u32 data)

/* start sentinel checking in case of missing uevent */
dp_add_event(dp, EV_DISCONNECT_PENDING_TIMEOUT, 0, 
DP_TIMEOUT_5_SECOND);


-   /* signal the disconnect event early to ensure proper teardown 
*/


This doesn't need to be done early anymore? Please mention why in the
commit text.


This is my mistake, it still need signal audio here, will fix it


-   reinit_completion(>audio_comp);
-   dp_display_handle_plugged_change(g_dp_display, false);
-
dp_catalog_hpd_config_intr(dp->catalog, 
DP_DP_HPD_PLUG_INT_MASK |

DP_DP_IRQ_HPD_INT_MASK, true);

@@ -894,7 +898,6 @@ static int dp_display_disable(struct 
dp_display_private *dp, u32 data)

/* wait only if audio was enabled */
if (dp_display->audio_enabled) {
/* signal the disconnect event */
-   reinit_completion(>audio_comp);
dp_display_handle_plugged_change(dp_display, false);
if (!wait_for_completion_timeout(>audio_comp,
HZ * 5))

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

Re: [PATCH v2 3/3] drm/msm/dp: check main link status before start aux read

[khsieh]

On 2021-04-14 14:09, Stephen Boyd wrote:

Quoting Kuogee Hsieh (2021-04-13 16:11:44)

Make sure main link is in connection state before start aux
read/write operation to avoid unnecessary long delay due to
main link had been unplugged.

Signed-off-by: Kuogee Hsieh 
---
 drivers/gpu/drm/msm/dp/dp_aux.c  |  5 +
 drivers/gpu/drm/msm/dp/dp_link.c | 20 +++-
 2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
b/drivers/gpu/drm/msm/dp/dp_aux.c

index 7c22bfe..fae3806 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -343,6 +343,11 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux 
*dp_aux,


mutex_lock(>mutex);

+   if (!dp_catalog_link_is_connected(aux->catalog)) {
+   ret = -ETIMEDOUT;
+   goto unlock_exit;
+   }


I get a crash here sometimes when plugging and unplugging an apple HDMI
dongle during suspend/resume. I guess device power management
(dp_pm_suspend()) is happening in parallel with aux transfers from the
kthread. Why doesn't the aux communication start reporting NAKs once 
the

cable is disconnected?

[  366.210058] hdmi-audio-codec hdmi-audio-codec.15.auto: calling
platform_pm_suspend+0x0/0x60 @ 7175, parent:
ae9.displayport-controller
[  366.222825] hdmi-audio-codec hdmi-audio-codec.15.auto:
platform_pm_suspend+0x0/0x60 returned 0 after 1 usecs
[  366.232939] msm-dp-display ae9.displayport-controller: calling
dp_pm_suspend+0x0/0x80 @ 7175, parent: ae0.mdss
[  366.244006] msm-dp-display ae9.displayport-controller:
dp_pm_suspend+0x0/0x80 returned 0 after 79 usecs
[  366.254025] msm_dsi ae94000.dsi: calling
pm_runtime_force_suspend+0x0/0xb4 @ 7175, parent: ae0.mdss
[  366.263669] msm_dsi ae94000.dsi: pm_runtime_force_suspend+0x0/0xb4
returned 0 after 0 usecs
[  366.272290] panel-simple panel: calling
platform_pm_suspend+0x0/0x60 @ 7175, parent: platform
[  366.272501] ti_sn65dsi86 2-002d: calling
pm_runtime_force_suspend+0x0/0xb4 @ 176, parent: i2c-2
[  366.281055] panel-simple panel: platform_pm_suspend+0x0/0x60
returned 0 after 0 usecs
[  366.281081] leds mmc1::: calling led_suspend+0x0/0x4c @ 7175,
parent: 7c4000.sdhci
[  366.290065] ti_sn65dsi86 2-002d: pm_runtime_force_suspend+0x0/0xb4
returned 0 after 0 usecs
[  366.298046] leds mmc1::: led_suspend+0x0/0x4c returned 0 after 1 
usecs

[  366.302994] Internal error: synchronous external abort: 9610
[#1] PREEMPT SMP
[  366.303006] Modules linked in: vhost_vsock
vmw_vsock_virtio_transport_common vsock vhost rfcomm algif_hash
algif_skcipher af_alg xt_cgroup uinput xt_MASQUERADE venus_enc
hci_uart venus_dec btqca cros_ec_typec typec bluetooth qcom_spmi_adc5
snd_soc_sc7180 qcom_spmi_temp_alarm ecdh_generic qcom_spmi_adc_tm5
qcom_vadc_common snd_soc_qcom_common ecc snd_soc_rt5682_i2c
snd_soc_rt5682 snd_soc_rl6231 venus_core v4l2_mem2mem
snd_soc_lpass_sc7180 snd_soc_lpass_hdmi snd_soc_lpass_cpu
snd_soc_lpass_platform snd_soc_max98357a fuse iio_trig_sysfs
cros_ec_sensors cros_ec_sensors_ring cros_ec_lid_angle
cros_ec_sensors_core industrialio_triggered_buffer kfifo_buf
cros_ec_sensorhub lzo_rle lzo_compress zram ath10k_snoc ath10k_core
ath mac80211 cfg80211 cdc_ether usbnet r8152 mii uvcvideo
videobuf2_vmalloc joydev
[  366.303211] CPU: 0 PID: 224 Comm: dp_hpd_handler Not tainted 5.4.109 
#27
[  366.303216] Hardware name: Google Lazor (rev3+) with KB Backlight 
(DT)

[  366.303225] pstate: 60c9 (nZCv daif +PAN +UAO)
[  366.303234] pc : dp_catalog_link_is_connected+0x20/0x34
[  366.303244] lr : dp_aux_transfer+0x44/0x284
[  366.303250] sp : ffc011bfbbe0
[  366.303254] x29: ffc011bfbbe0 x28: 
[  366.303262] x27: 000c x26: ff896f8212bc
[  366.303269] x25: 0001 x24: 0001
[  366.303275] x23: 0020 x22: ff896ff82118
[  366.303282] x21: ffc011bfbc50 x20: ff896ff82090
[  366.303289] x19: ff896ffc3598 x18: 
[  366.303295] x17:  x16: 0001
[  366.303303] x15:  x14: 02ef
[  366.303311] x13: 0400 x12: ffeb896ea060
[  366.303317] x11:  x10: 
[  366.303324] x9 : ff896f061100 x8 : ffc010582204
[  366.303331] x7 : 00b2b5593519 x6 : 003033ff
[  366.303338] x5 :  x4 : 0001
[  366.303345] x3 : ff896ff432a1 x2 : 
[  366.303352] x1 : 0119 x0 : ff896ffc3598
[  366.303360] Call trace:
[  366.303367]  dp_catalog_link_is_connected+0x20/0x34
[  366.303374]  dp_aux_transfer+0x44/0x284
[  366.303387]  drm_dp_dpcd_access+0x8c/0x11c
[  366.303393]  drm_dp_dpcd_read+0x64/0x10c
[  366.303399]  dp_link_process_request+0x94/0xaf8
[  366.303405]  dp_display_usbpd_attention_cb+0x38/0x140
[  366.303411]  hpd_event_thread+0x3f0/0x48c
[  366.303426]  kthread+0x140/0x17c
[  366.303439]  ret_from_fork+0x10/0x18
[  366.303458] Code: 

Re: [PATCH v2 2/3] drm/msm/dp: do not re initialize of audio_comp at display_disable()

[khsieh]

On 2021-04-13 20:17, Stephen Boyd wrote:

Quoting Kuogee Hsieh (2021-04-13 16:11:30)
At dongle unplug, dp initializes audio_comp followed by sending 
disconnect
event notification to audio and to make sure audio had shutdown 
completely
by wait for audio completion notification at display_disable(). This 
patch


Is this dp_display_disable()? Doubtful that display_disable() is the
function we're talking about.

yes


will not re initialize audio_comp at display_disable() if audio 
shutdown

is triggered by dongle unplugged.


This commit text seems to say the why before the what, where why is "dp
initializes audio_comp followed by sending disconnect.." and the what 
is

"this patch will no re-initialized audio_comp...". Can you reorder this
so the what comes before the why?


ok


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

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c

index 0ba71c7..1d71c95 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -894,8 +894,10 @@ static int dp_display_disable(struct 
dp_display_private *dp, u32 data)

/* wait only if audio was enabled */
if (dp_display->audio_enabled) {
/* signal the disconnect event */
-   reinit_completion(>audio_comp);
-   dp_display_handle_plugged_change(dp_display, false);
+   if (dp->hpd_state != ST_DISCONNECT_PENDING) {
+   reinit_completion(>audio_comp);


Why is this reinitialized here at all? Wouldn't it make more sense to
initialize the completion once at cable plug in and then not initialize
the completion anywhere else? Or initialize the completion whenever
dp_display->audio_enabled is set to true and then only wait for the
completion here if that boolean is true? Or initialize the completion
when dp_display_handle_plugged_change() is passed true for the 
'plugged'

argument?
yes, i think it is better approach, this will take care of both unplug 
and suspend.



I started reading the code and quickly got lost figuring out how
dp_display_handle_plugged_change() worked and the interaction between
the dp display code and the audio codec embedded in here. There seem to
be a couple of conditions that cut off things early, like
dp_display->audio_enabled and audio->engine_on. Why? Why does
dp_display_signal_audio_complete() call complete_all() vs. just
complete()? Please help! :(

+   dp_display_handle_plugged_change(dp_display, 
false);


I think it's this way because dp_hpd_unplug_handle() is the function
that sets the hpd_state to ST_DISCONNECT_PENDING and then reinitializes
the completion (why?) and calls dp_display_handle_plugged_change(). So
the commit text could say that reinitializing the completion again here
at dp_display_disable() is racing with the audio code in the case that
dp_hpd_unplug_handle() already called
dp_display_handle_plugged_change() and it would make more sense. But 
the

question still stands why that race even exists in the first place vs.
initializing the completion variable in only one place unconditionally
when the cable is connected, in dp_hpd_plug_handle() or
dp_display_post_enable().


+   }
if (!wait_for_completion_timeout(>audio_comp,
HZ * 5))
DRM_ERROR("audio comp timeout\n");

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

Re: [Freedreno] [PATCH v2 2/2] drm/msm/dp: add supported max link rate specified from dtsi

[khsieh]

On 2021-02-22 08:55, Sean Paul wrote:

On Mon, Feb 22, 2021 at 11:31 AM  wrote:


On 2021-02-19 14:46, Stephen Boyd wrote:
> Quoting khs...@codeaurora.org (2021-02-19 08:39:38)
>> On 2021-02-18 15:02, Stephen Boyd wrote:
>> > Quoting Kuogee Hsieh (2021-02-18 12:55:04)
>> >> Allow supported link rate to be limited to the value specified at
>> >> dtsi. If it is not specified, then link rate is derived from dpcd
>> >> directly. Below are examples,
>> >> link-rate = <162000> for max link rate limited at 1.62G
>> >> link-rate = <27> for max link rate limited at 2.7G
>> >> link-rate = <54> for max link rate limited at 5.4G
>> >> link-rate = <81> for max link rate limited at 8.1G
>> >>
>> >> Changes in V2:
>> >> -- allow supported max link rate specified from dtsi
>> >
>> > Please don't roll this into the patch that removes the limit. The
>> > previous version of this patch was fine. The part that lowers the limit
>> > back down should be another patch.
>> >
>> > We rejected link-rate in DT before and we should reject it upstream
>> > again. As far as I can tell, the maximum link rate should be determined
>> > based on the panel or the type-c port on the board. The dp controller
>> > can always achieve HBR3, so limiting it at the dp controller is
>> > incorrect. The driver should query the endpoints to figure out if they
>> > want to limit the link rate. Is that done automatically sometimes by
>> > intercepting the DPCD?
>>
>> ok, i will roll back to original patch and add the second patch for
>> max
>> link rate limited purpose.
>> panel dpcd specified max link rate it supported.
>> At driver, link rate is derived from dpcd directly since driver will
>> try
>> to use the maximum supported link rate and less lane to save power.
>> Therefore it is not possible that limit link rate base on dpcd.
>> AS i understand we are going to do max link rate limitation is due to
>> old redriver chip can not support HBR3.
>> How can I acquire which type-c port on the board so that I can trigger
>> max link rate limitation?
>>
>>
>
> The driver already seems to support lowering the link rate during link
> training. Can't we try to train at the highest rate and then downgrade
> the link speed until it trains properly? I sort of fail to see why we
> need to introduce a bunch of complexity around limiting the link rate
> on
> certain boards if the driver can figure out that link training doesn't
> work at HBR3 so it should try to train at HBR2 instead.

yes, dp driver did support down grade link rate during link training
procedure.
But link training is kind of setting up agreement between host and 
panel

with assumption that there are no other limitations in between.
The problem we are discussing here is the limitation of usb re driver
link rate support.
Since we do not know how usb re driver behavior, I am not sure link
training will work appropriately for this case.
It may end up link status keep toggling up and down.



IMO we should just fail link training if the redriver doesn't support
a link count/rate and fallback to the next count/rate. This should be
handled the same as if there were a cable incapable of achieving a
link rate. Adding the link rate to the device tree (at least on the dp
block) seems suspicious.

If you really wanted to model the redriver's limitations in software,
you'd probably want to introduce a bridge driver/connector which
rejects modes that cannot be achieved by the redriver. This should
prevent the dp driver from trying to train at the unsupported rates.

Sean


I am not familiar with drm arch that well.
Can you elaborate more how bridge can work in this case?
When dp driver received plug-in interrupt, it read panel capability dpcd 
and start link training with link rate specified at dpcd.
How bridge can propagate link rate (limited by redriver) before link 
training happen?






Both link-lane and link-rate specified at dtsi are for the limitation 
of

Trogdor hardware platform.
Both link-lane and link-rate specified at dtsi are NOT for panel since
panel have specified its capability at its DPCD.








___
Freedreno mailing list
freedr...@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

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

Re: [PATCH v2 2/2] drm/msm/dp: add supported max link rate specified from dtsi

[khsieh]

On 2021-02-19 14:46, Stephen Boyd wrote:

Quoting khs...@codeaurora.org (2021-02-19 08:39:38)

On 2021-02-18 15:02, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2021-02-18 12:55:04)
>> Allow supported link rate to be limited to the value specified at
>> dtsi. If it is not specified, then link rate is derived from dpcd
>> directly. Below are examples,
>> link-rate = <162000> for max link rate limited at 1.62G
>> link-rate = <27> for max link rate limited at 2.7G
>> link-rate = <54> for max link rate limited at 5.4G
>> link-rate = <81> for max link rate limited at 8.1G
>>
>> Changes in V2:
>> -- allow supported max link rate specified from dtsi
>
> Please don't roll this into the patch that removes the limit. The
> previous version of this patch was fine. The part that lowers the limit
> back down should be another patch.
>
> We rejected link-rate in DT before and we should reject it upstream
> again. As far as I can tell, the maximum link rate should be determined
> based on the panel or the type-c port on the board. The dp controller
> can always achieve HBR3, so limiting it at the dp controller is
> incorrect. The driver should query the endpoints to figure out if they
> want to limit the link rate. Is that done automatically sometimes by
> intercepting the DPCD?

ok, i will roll back to original patch and add the second patch for 
max

link rate limited purpose.
panel dpcd specified max link rate it supported.
At driver, link rate is derived from dpcd directly since driver will 
try

to use the maximum supported link rate and less lane to save power.
Therefore it is not possible that limit link rate base on dpcd.
AS i understand we are going to do max link rate limitation is due to
old redriver chip can not support HBR3.
How can I acquire which type-c port on the board so that I can trigger
max link rate limitation?




The driver already seems to support lowering the link rate during link
training. Can't we try to train at the highest rate and then downgrade
the link speed until it trains properly? I sort of fail to see why we
need to introduce a bunch of complexity around limiting the link rate 
on

certain boards if the driver can figure out that link training doesn't
work at HBR3 so it should try to train at HBR2 instead.


yes, dp driver did support down grade link rate during link training 
procedure.
But link training is kind of setting up agreement between host and panel 
with assumption that there are no other limitations in between.
The problem we are discussing here is the limitation of usb re driver 
link rate support.
Since we do not know how usb re driver behavior, I am not sure link 
training will work appropriately for this case.

It may end up link status keep toggling up and down.

Both link-lane and link-rate specified at dtsi are for the limitation of 
Trogdor hardware platform.
Both link-lane and link-rate specified at dtsi are NOT for panel since 
panel have specified its capability at its DPCD.









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

Re: [PATCH v2 2/2] drm/msm/dp: add supported max link rate specified from dtsi

[khsieh]

On 2021-02-18 15:02, Stephen Boyd wrote:

Quoting Kuogee Hsieh (2021-02-18 12:55:04)

Allow supported link rate to be limited to the value specified at
dtsi. If it is not specified, then link rate is derived from dpcd
directly. Below are examples,
link-rate = <162000> for max link rate limited at 1.62G
link-rate = <27> for max link rate limited at 2.7G
link-rate = <54> for max link rate limited at 5.4G
link-rate = <81> for max link rate limited at 8.1G

Changes in V2:
-- allow supported max link rate specified from dtsi


Please don't roll this into the patch that removes the limit. The
previous version of this patch was fine. The part that lowers the limit
back down should be another patch.

We rejected link-rate in DT before and we should reject it upstream
again. As far as I can tell, the maximum link rate should be determined
based on the panel or the type-c port on the board. The dp controller
can always achieve HBR3, so limiting it at the dp controller is
incorrect. The driver should query the endpoints to figure out if they
want to limit the link rate. Is that done automatically sometimes by
intercepting the DPCD?


ok, i will roll back to original patch and add the second patch for max 
link rate limited purpose.

panel dpcd specified max link rate it supported.
At driver, link rate is derived from dpcd directly since driver will try 
to use the maximum supported link rate and less lane to save power.

Therefore it is not possible that limit link rate base on dpcd.
AS i understand we are going to do max link rate limitation is due to 
old redriver chip can not support HBR3.
How can I acquire which type-c port on the board so that I can trigger 
max link rate limitation?



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

Re: [PATCH 1/2] drm/msm/dp: postpone irq_hpd event during connection pending state

[khsieh]

On 2021-01-13 16:00, Stephen Boyd wrote:

Quoting khs...@codeaurora.org (2021-01-13 15:44:32)

On 2021-01-13 12:22, Stephen Boyd wrote:
> Quoting khs...@codeaurora.org (2021-01-13 09:44:24)
>> On 2021-01-11 11:55, Stephen Boyd wrote:
>> > Quoting Kuogee Hsieh (2021-01-07 12:30:24)
>> >> irq_hpd event can only be executed at connected state. Therefore
>> >> irq_hpd event should be postponed if it happened at connection
>> >> pending state. This patch also make sure both link rate and lane
>> >
>> > Why does it happen at connection pending state?
>> plug in need two state to complete it.
>> advance to connection pending state once link training completed and
>> sent uevent notification to frame work.
>> transition to connected state after frame work provided resolution
>> timing and start transmit video panel.
>> Therefore irq_hpd should not be handled if it occurred before
>> connected
>> state.
>
> Sure that's what's going on in the patch but you didn't answer my
> question. Why does irq_hpd happen before connected state?

I have no idea why it happen this way.
during debug
https://partnerissuetracker.corp.google.com/issues/170598152
I saw two different scenario
1) irq_hpd followed by unplug with less than 20 ms in between. this 
one

fixed by this patch set.
2) plug followed by irq_hpd around 300ms in between. it does not cause
problem. but it should be handled in order (after connected state).


Ok. So nobody understands why the hardware is acting this way and we're
papering over the problem by forcing the HPD state to be what we think
it should be? That's not great.


irq_hpd is issued from dongle.
it then go through EC ps8805 driver and reach DP driver finally.
Again, to duplicate problem #1 this at my set up, i have to 
intentionally wiggling type-c connector of dongle.
But I can not duplicate problem #2 and only saw it one time from Quantan 
provide logs.







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

Re: [PATCH 2/2] drm/msm/dp: unplug interrupt missed after irq_hpd handler

[khsieh]

On 2021-01-13 12:23, Stephen Boyd wrote:

Quoting khs...@codeaurora.org (2021-01-13 09:48:25)

On 2021-01-11 11:54, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2021-01-07 12:30:25)
>> There is HPD unplug interrupts missed at scenario of an irq_hpd
>> followed by unplug interrupts with around 10 ms in between.
>> Since both AUX_SW_RESET and DP_SW_RESET clear pending HPD interrupts,
>> irq_hpd handler should not issues either aux or sw reset to avoid
>> following unplug interrupt be cleared accidentally.
>
> So the problem is that we're resetting the DP aux phy in the middle of
> the HPD state machine transitioning states?
>
yes, after reset aux, hw clear pending hpd interrupts
>>
>> Signed-off-by: Kuogee Hsieh 
>> ---
>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
>> b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> index 44f0c57..9c0ce98 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> @@ -190,6 +190,18 @@ int dp_catalog_aux_clear_hw_interrupts(struct
>> dp_catalog *dp_catalog)
>> return 0;
>>  }
>>
>> +/**
>> + * dp_catalog_aux_reset() - reset AUX controller
>> + *
>> + * @aux: DP catalog structure
>> + *
>> + * return: void
>> + *
>> + * This function reset AUX controller
>> + *
>> + * NOTE: reset AUX controller will also clear any pending HPD related
>> interrupts
>> + *
>> + */
>>  void dp_catalog_aux_reset(struct dp_catalog *dp_catalog)
>>  {
>> u32 aux_ctrl;
>> @@ -483,6 +495,18 @@ int dp_catalog_ctrl_set_pattern(struct dp_catalog
>> *dp_catalog,
>> return 0;
>>  }
>>
>> +/**
>> + * dp_catalog_ctrl_reset() - reset DP controller
>> + *
>> + * @aux: DP catalog structure
>
> It's called dp_catalog though.
registers access are through dp_catalog_


Agreed. The variable is not called 'aux' though, it's called
'dp_catalog'.


>
>> + *
>> + * return: void
>> + *
>> + * This function reset DP controller
>
> resets the
>
>> + *
>> + * NOTE: reset DP controller will also clear any pending HPD related
>> interrupts
>> + *
>> + */
>>  void dp_catalog_ctrl_reset(struct dp_catalog *dp_catalog)


Right here.


fixed at v2



>>  {
>> u32 sw_reset;
>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> index e3462f5..f96c415 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> @@ -1296,7 +1296,8 @@ static int dp_ctrl_setup_main_link(struct
>> dp_ctrl_private *ctrl,
>>  * transitioned to PUSH_IDLE. In order to start transmitting
>>  * a link training pattern, we have to first do soft reset.
>>  */
>> -   dp_catalog_ctrl_reset(ctrl->catalog);
>> +   if (*training_step != DP_TRAINING_NONE)
>
> Can we check for the positive value instead? i.e.
> DP_TRAINING_1/DP_TRAINING_2
>


Any answer?


fixed at v2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/msm/dp: postpone irq_hpd event during connection pending state

[khsieh]

On 2021-01-13 12:22, Stephen Boyd wrote:

Quoting khs...@codeaurora.org (2021-01-13 09:44:24)

On 2021-01-11 11:55, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2021-01-07 12:30:24)
>> irq_hpd event can only be executed at connected state. Therefore
>> irq_hpd event should be postponed if it happened at connection
>> pending state. This patch also make sure both link rate and lane
>
> Why does it happen at connection pending state?
plug in need two state to complete it.
advance to connection pending state once link training completed and
sent uevent notification to frame work.
transition to connected state after frame work provided resolution
timing and start transmit video panel.
Therefore irq_hpd should not be handled if it occurred before 
connected

state.


Sure that's what's going on in the patch but you didn't answer my
question. Why does irq_hpd happen before connected state?


I have no idea why it happen this way.
during debug 
https://partnerissuetracker.corp.google.com/issues/170598152

I saw two different scenario
1) irq_hpd followed by unplug with less than 20 ms in between. this one 
fixed by this patch set.
2) plug followed by irq_hpd around 300ms in between. it does not cause 
problem. but it should be handled in order (after connected state).

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

Re: [PATCH 1/2] drm/msm/dp: postpone irq_hpd event during connection pending state

[khsieh]

On 2021-01-11 11:55, Stephen Boyd wrote:

Quoting Kuogee Hsieh (2021-01-07 12:30:24)

irq_hpd event can only be executed at connected state. Therefore
irq_hpd event should be postponed if it happened at connection
pending state. This patch also make sure both link rate and lane


Why does it happen at connection pending state?

plug in need two state to complete it.
advance to connection pending state once link training completed and 
sent uevent notification to frame work.
transition to connected state after frame work provided resolution 
timing and start transmit video panel.
Therefore irq_hpd should not be handled if it occurred before connected 
state.



are valid before start link training.


Can this part about link rate and lane being valid be split off into
another patch?


ok, i will spilt this patch into two.
I will merge irq_hpd event part into 2nd patch (drm/msm/dp: unplug 
interrupt missed after irq_hpd handler).




Signed-off-by: Kuogee Hsieh 
---


Any fixes tag?


 drivers/gpu/drm/msm/dp/dp_display.c |  7 +++
 drivers/gpu/drm/msm/dp/dp_panel.c   | 12 +---
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c

index 6e971d5..3bc7ed2 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -693,6 +693,13 @@ static int dp_irq_hpd_handle(struct 
dp_display_private *dp, u32 data)

return 0;
}

+   if (state == ST_CONNECT_PENDING) {
+   /* wait until ST_CONNECTED */
+   dp_add_event(dp, EV_IRQ_HPD_INT, 0, 1); /* delay = 1 
*/

+   mutex_unlock(>event_mutex);
+   return 0;
+   }
+
ret = dp_display_usbpd_attention_cb(>pdev->dev);
if (ret == -ECONNRESET) { /* cable unplugged */
dp->core_initialized = false;

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

Re: [PATCH v2 0/2] fix missing unplug interrupt problem

[khsieh]

On 2021-01-13 12:25, Stephen Boyd wrote:

Quoting Kuogee Hsieh (2021-01-13 10:59:58)

Both AUX_SW_RESET and DP_SW_RESET clear pending HPD interrupts.
Therefore irq_hpd handler should not issues either aux or sw reset
to avoid following unplug interrupt be cleared accidentally.

Kuogee Hsieh (2):
  drm/msm/dp: return fail when both link lane and rate are 0 at dpcd
read
  drm/msm/dp: unplug interrupt missed after irq_hpd handler


It won't apply to the drm msm tree. Please rebase and resend.

Both V1 two patches are picked by Rob already.
I will drop V2 patches.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/msm/dp: unplug interrupt missed after irq_hpd handler

[khsieh]

On 2021-01-11 11:54, Stephen Boyd wrote:

Quoting Kuogee Hsieh (2021-01-07 12:30:25)

There is HPD unplug interrupts missed at scenario of an irq_hpd
followed by unplug interrupts with around 10 ms in between.
Since both AUX_SW_RESET and DP_SW_RESET clear pending HPD interrupts,
irq_hpd handler should not issues either aux or sw reset to avoid
following unplug interrupt be cleared accidentally.


So the problem is that we're resetting the DP aux phy in the middle of
the HPD state machine transitioning states?


yes, after reset aux, hw clear pending hpd interrupts


Signed-off-by: Kuogee Hsieh 
---
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c 
b/drivers/gpu/drm/msm/dp/dp_catalog.c

index 44f0c57..9c0ce98 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -190,6 +190,18 @@ int dp_catalog_aux_clear_hw_interrupts(struct 
dp_catalog *dp_catalog)

return 0;
 }

+/**
+ * dp_catalog_aux_reset() - reset AUX controller
+ *
+ * @aux: DP catalog structure
+ *
+ * return: void
+ *
+ * This function reset AUX controller
+ *
+ * NOTE: reset AUX controller will also clear any pending HPD related 
interrupts

+ *
+ */
 void dp_catalog_aux_reset(struct dp_catalog *dp_catalog)
 {
u32 aux_ctrl;
@@ -483,6 +495,18 @@ int dp_catalog_ctrl_set_pattern(struct dp_catalog 
*dp_catalog,

return 0;
 }

+/**
+ * dp_catalog_ctrl_reset() - reset DP controller
+ *
+ * @aux: DP catalog structure


It's called dp_catalog though.

registers access are through dp_catalog_



+ *
+ * return: void
+ *
+ * This function reset DP controller


resets the


+ *
+ * NOTE: reset DP controller will also clear any pending HPD related 
interrupts

+ *
+ */
 void dp_catalog_ctrl_reset(struct dp_catalog *dp_catalog)
 {
u32 sw_reset;
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c 
b/drivers/gpu/drm/msm/dp/dp_ctrl.c

index e3462f5..f96c415 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1296,7 +1296,8 @@ static int dp_ctrl_setup_main_link(struct 
dp_ctrl_private *ctrl,

 * transitioned to PUSH_IDLE. In order to start transmitting
 * a link training pattern, we have to first do soft reset.
 */
-   dp_catalog_ctrl_reset(ctrl->catalog);
+   if (*training_step != DP_TRAINING_NONE)


Can we check for the positive value instead? i.e.
DP_TRAINING_1/DP_TRAINING_2


+   dp_catalog_ctrl_reset(ctrl->catalog);

ret = dp_ctrl_link_train(ctrl, cr, training_step);


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

Re: [PATCH] drm/msm/dp: deinitialize mainlink if link training failedo

[khsieh]

On 2020-11-02 12:59, Stephen Boyd wrote:

Quoting Kuogee Hsieh (2020-10-30 16:22:53)

DP compo phy have to be enable to start link training. When
link training failed phy need to be disabled so that next
link trainng can be proceed smoothly at next plug in. This


s/trainng/training/


patch de initialize mainlink to disable phy if link training


s/de/de-/


failed. This prevent system crash due to
disp_cc_mdss_dp_link_intf_clk stuck at "off" state.  This patch
also perform checking power_on flag at dp_display_enable() and
dp_display_disable() to avoid crashing when unplug cable while
display is off.

Fixes: fdaf9a5e3c15 (drm/msm/dp: fixes wrong connection state caused 
by failure of link train




Drop newline please.


Signed-off-by: Kuogee Hsieh 
---


Can you send this as a patch series? There were three patches sent near
each other and presumably they're related.

 drivers/gpu/drm/msm/dp/dp_ctrl.c| 34 
+++--

 drivers/gpu/drm/msm/dp/dp_display.c | 13 +++
 2 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c 
b/drivers/gpu/drm/msm/dp/dp_ctrl.c

index cee161c8ecc6..904698dfc7f7 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1468,6 +1468,29 @@ static int dp_ctrl_reinitialize_mainlink(struct 
dp_ctrl_private *ctrl)

return ret;
 }

+static int dp_ctrl_deinitialize_mainlink(struct dp_ctrl_private 
*ctrl)

+{
+   struct dp_io *dp_io;
+   struct phy *phy;
+   int ret = 0;


Please drop this initialization to 0.


+
+   dp_io = >parser->io;
+   phy = dp_io->phy;
+
+   dp_catalog_ctrl_mainlink_ctrl(ctrl->catalog, false);
+
+   dp_catalog_ctrl_reset(ctrl->catalog);
+
+   ret = dp_power_clk_enable(ctrl->power, DP_CTRL_PM, false);


As it's overwritten here.


+   if (ret)
+   DRM_ERROR("Failed to disable link clocks. ret=%d\n", 
ret);

+
+   phy_power_off(phy);
+   phy_exit(phy);
+
+   return -ECONNRESET;


Isn't this an error for networking connections getting reset? Really it
should return 0 because it didn't fail.


+}
+
 static int dp_ctrl_link_maintenance(struct dp_ctrl_private *ctrl)
 {
int ret = 0;
@@ -1648,8 +1671,7 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
if (rc)
return rc;

-   while (--link_train_max_retries &&
-   !atomic_read(>dp_ctrl.aborted)) {
+   while (--link_train_max_retries) {
rc = dp_ctrl_reinitialize_mainlink(ctrl);
if (rc) {
DRM_ERROR("Failed to reinitialize mainlink. 
rc=%d\n",

@@ -1664,6 +1686,9 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
break;
} else if (training_step == DP_TRAINING_1) {
/* link train_1 failed */
+   if 
(!dp_catalog_hpd_get_state_status(ctrl->catalog))
+   break;  /* link cable 
unplugged */

+
rc = dp_ctrl_link_rate_down_shift(ctrl);
if (rc < 0) { /* already in RBR = 1.6G */
if (cr.lane_0_1 & DP_LANE0_1_CR_DONE) 
{

@@ -1683,6 +1708,9 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
}
} else if (training_step == DP_TRAINING_2) {
/* link train_2 failed, lower lane rate */
+   if 
(!dp_catalog_hpd_get_state_status(ctrl->catalog))


Maybe make a function called dp_catalog_link_disconnected()? Then the
comment isn't needed.

+   break;  /* link cable 
unplugged */

+
rc = dp_ctrl_link_lane_down_shift(ctrl);
if (rc < 0) {
/* end with failure */
@@ -1703,6 +1731,8 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
 */
if (rc == 0)  /* link train successfully */
dp_ctrl_push_idle(dp_ctrl);
+   else
+   rc = dp_ctrl_deinitialize_mainlink(ctrl);


So if it fails we deinitialize and then return success? Shouldn't we
keep the error code from the link train attempt instead of overwrite it
with (most likely) zero? I see that it returns -ECONNRESET but that's
really odd and seeing this code here means you have to look at the
function to figure out that it's still returning an error code. Please
don't do that, just ignore the error code from this function.

There are two possible failure cases at plugin request, link training 
failed  and read dpcd/edid failed.
It does not need to enable phy/pll to perform aux read/write from/to 
dpcd or edid.
on the other hand, phy/pll need to be enabled to perform link training. 
If link training failed,
then phy/pll need to be disabled so that phy/pll can be enabled next 
link training correctly.
Link training failed error has to be propagated back to the top caller 
so that dp_display_host_init()

Re: [PATCH] drm/msm/dp: promote irq_hpd handle to handle link trainign correctly

[khsieh]

On 2020-11-02 11:29, Stephen Boyd wrote:

Subject has a typo in "training".

Quoting Kuogee Hsieh (2020-10-30 16:23:24)

Some dongles, such as Apple, required link training done at irq_hpd


s/required/require/


request instead of plugin request. This patch promote irq_hpd hanlder


s/hanlder/handler/


to handle link training and setup hpd_state correctly.

Signed-off-by: Kuogee Hsieh 
---


Any Fixes tag?


 drivers/gpu/drm/msm/dp/dp_display.c | 20 ++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c

index 13b66266cd69..55627530957c 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -483,10 +485,24 @@ static int dp_display_usbpd_attention_cb(struct 
device *dev)

return -ENODEV;
}

+   hpd = dp->usbpd;
+
/* check for any test request issued by sink */
rc = dp_link_process_request(dp->link);
-   if (!rc)
-   dp_display_handle_irq_hpd(dp);
+   if (!rc) {
+   sink_request = dp->link->sink_request;
+   if (sink_request & DS_PORT_STATUS_CHANGED) {
+   dp->hpd_state = ST_CONNECT_PENDING;
+   hpd->hpd_high = 1;
+   }
+
+   rc = dp_display_handle_irq_hpd(dp);
+
+   if (rc && sink_request & DS_PORT_STATUS_CHANGED) {


Can you add parenthesis around this?

if (rc && (sink_request & DS_PORT_STATUS_CHANGED)) {


I honestly don't know what's going on in this patch. It talks about
making link training happen during irq hpd handler but this is the
attention handler and we're checking port status changed? This is
related? The code is really not clear.
irq_hpd request is generated by sinker to ask host attention that 
something has changed.
POST_STATUS_CHNAGED bit set  by sinker to indicated link had loss of 
sync. Therefore
host need to restart link retaining to fix the link loss of sync 
problem.





+   hpd->hpd_high = 0;
+   dp->hpd_state = ST_DISCONNECTED;
+   }
+   }

return rc;
 }

base-commit: 0e162b10644605428cd2596c12f8ed410cf9d2d9


What commit is this?

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

Re: [PATCH] drm/msm/dp: skip checking LINK_STATUS_UPDATED bit

[khsieh]

On 2020-10-20 15:15, Stephen Boyd wrote:

Quoting Kuogee Hsieh (2020-10-20 09:59:59)

No need to check LINK_STATuS_UPDATED bit before


LINK_STATUS_UPDATED?


return 6 bytes of link status during link training.


Why?


This patch also fix phy compliance test link rate
conversion error.


How?



Signed-off-by: Kuogee Hsieh 
---


Any Fixes: tag?


 drivers/gpu/drm/msm/dp/dp_ctrl.c | 20 ++--
 drivers/gpu/drm/msm/dp/dp_link.c | 24 +++-
 2 files changed, 17 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c 
b/drivers/gpu/drm/msm/dp/dp_ctrl.c

index 6bdaec778c4c..76e891c91c6e 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1061,23 +1061,15 @@ static bool dp_ctrl_train_pattern_set(struct 
dp_ctrl_private *ctrl,

 static int dp_ctrl_read_link_status(struct dp_ctrl_private *ctrl,
u8 *link_status)
 {
-   int len = 0;
-   u32 const offset = DP_LANE_ALIGN_STATUS_UPDATED - 
DP_LANE0_1_STATUS;

-   u32 link_status_read_max_retries = 100;
-
-   while (--link_status_read_max_retries) {
-   len = drm_dp_dpcd_read_link_status(ctrl->aux,
-   link_status);
-   if (len != DP_LINK_STATUS_SIZE) {
-   DRM_ERROR("DP link status read failed, err: 
%d\n", len);

-   return len;
-   }
+   int ret = 0, len;

-   if (!(link_status[offset] & DP_LINK_STATUS_UPDATED))
-   return 0;
+   len = drm_dp_dpcd_read_link_status(ctrl->aux, link_status);
+   if (len != DP_LINK_STATUS_SIZE) {
+   DRM_ERROR("DP link status read failed, err: %d\n", 
len);

+   ret = len;


Could this be positive if the len is greater than 0 but not
DP_LINK_STATUS_SIZE? Maybe the check should be len < 0? We certainly
don't want to return some smaller size from this function, right?



no, it should be exactly the byte number requested to read.
otherwise, it should be failed and will re read at next run.


}

-   return -ETIMEDOUT;
+   return ret;
 }

 static int dp_ctrl_link_train_1(struct dp_ctrl_private *ctrl,
diff --git a/drivers/gpu/drm/msm/dp/dp_link.c 
b/drivers/gpu/drm/msm/dp/dp_link.c

index c811da515fb3..58d65daae3b3 100644
--- a/drivers/gpu/drm/msm/dp/dp_link.c
+++ b/drivers/gpu/drm/msm/dp/dp_link.c
@@ -773,7 +773,8 @@ static int 
dp_link_process_link_training_request(struct dp_link_private *link)

link->request.test_lane_count);

link->dp_link.link_params.num_lanes = 
link->request.test_lane_count;

-   link->dp_link.link_params.rate = link->request.test_link_rate;
+   link->dp_link.link_params.rate =
+   
drm_dp_bw_code_to_link_rate(link->request.test_link_rate);


Why are we storing bw_code in test_link_rate? This looks very 
confusing.


Test_link_rate contains link rate from dpcd read. it need to be convert 
to real

rate by timing 2.7Mb before start phy compliance test.





return 0;
 }

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

Re: [PATCH v2] drm/msm/dp: add opp_table corner voting support base on dp_ink_clk rate

[khsieh]

On 2020-10-06 00:31, Rajendra Nayak wrote:

On 10/4/2020 3:56 AM, Kuogee Hsieh wrote:

Set link rate by using OPP set rate api so that CX level will be set
accordingly based on the link rate.

Changes in v2:
-- remove dev from dp_ctrl_put() parameters
-- address review comments


This needs to go below '---' and should not be part of the
change log.



Signed-off-by: Kuogee Hsieh 
---
  drivers/gpu/drm/msm/dp/dp_ctrl.c| 26 +
  drivers/gpu/drm/msm/dp/dp_display.c |  2 +-
  drivers/gpu/drm/msm/dp/dp_power.c   | 44 
++---

  drivers/gpu/drm/msm/dp/dp_power.h   |  2 +-
  4 files changed, 68 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c 
b/drivers/gpu/drm/msm/dp/dp_ctrl.c

index 2e3e1917351f..6eb9cdad1421 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -10,6 +10,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -76,6 +77,8 @@ struct dp_ctrl_private {
struct dp_parser *parser;
struct dp_catalog *catalog;
  + struct opp_table *opp_table;
+
struct completion idle_comp;
struct completion video_comp;
  };
@@ -1836,6 +1839,7 @@ struct dp_ctrl *dp_ctrl_get(struct device *dev, 
struct dp_link *link,

struct dp_parser *parser)
  {
struct dp_ctrl_private *ctrl;
+   int ret;
if (!dev || !panel || !aux ||
!link || !catalog) {
@@ -1849,6 +1853,19 @@ struct dp_ctrl *dp_ctrl_get(struct device *dev, 
struct dp_link *link,

return ERR_PTR(-ENOMEM);
}
  + ctrl->opp_table = dev_pm_opp_set_clkname(dev, "ctrl_link");
+   if (IS_ERR(ctrl->opp_table)) {
+   dev_err(dev, "invalid DP OPP table in device tree\n");


You do this regardless of an OPP table in DT, so for starters the error
message is wrong. Secondly this can return you a -EPROBE_DEFER if the
clock driver isn't ready yet.
So the ideal thing to do here, is return a PTR_ERR(ctrl->opp_table)


+   ctrl->opp_table = NULL;
+   } else {
+   /* OPP table is optional */
+   ret = dev_pm_opp_of_add_table(dev);
+   if (ret && ret != -ENODEV) {
+   dev_pm_opp_put_clkname(ctrl->opp_table);
+   ctrl->opp_table = NULL;
+   }
+   }
+
init_completion(>idle_comp);
init_completion(>video_comp);
  @@ -1866,4 +1883,13 @@ struct dp_ctrl *dp_ctrl_get(struct device 
*dev, struct dp_link *link,

void dp_ctrl_put(struct dp_ctrl *dp_ctrl)
  {
+   struct dp_ctrl_private *ctrl;
+
+   ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
+
+   if (ctrl->opp_table) {
+   dev_pm_opp_of_remove_table(ctrl->dev);
+   dev_pm_opp_put_clkname(ctrl->opp_table);
+   ctrl->opp_table = NULL;
+   }
  }
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c

index e175aa3fd3a9..269f83550b46 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -698,7 +698,7 @@ static int dp_init_sub_modules(struct 
dp_display_private *dp)

goto error;
}
  - dp->power = dp_power_get(dp->parser);
+   dp->power = dp_power_get(dev, dp->parser);
if (IS_ERR(dp->power)) {
rc = PTR_ERR(dp->power);
DRM_ERROR("failed to initialize power, rc = %d\n", rc);
diff --git a/drivers/gpu/drm/msm/dp/dp_power.c 
b/drivers/gpu/drm/msm/dp/dp_power.c

index 17c1fc6a2d44..9c4ea00a5f2a 100644
--- a/drivers/gpu/drm/msm/dp/dp_power.c
+++ b/drivers/gpu/drm/msm/dp/dp_power.c
@@ -8,12 +8,14 @@
  #include 
  #include 
  #include 
+#include 
  #include "dp_power.h"
  #include "msm_drv.h"
struct dp_power_private {
struct dp_parser *parser;
struct platform_device *pdev;
+   struct device *dev;
struct clk *link_clk_src;
struct clk *pixel_provider;
struct clk *link_provider;
@@ -148,18 +150,51 @@ static int dp_power_clk_deinit(struct 
dp_power_private *power)

return 0;
  }
  +static int dp_power_clk_set_link_rate(struct dp_power_private 
*power,

+   struct dss_clk *clk_arry, int num_clk, int enable)
+{
+   u32 rate;
+   int i, rc = 0;
+
+   for (i = 0; i < num_clk; i++) {
+   if (clk_arry[i].clk) {
+   if (clk_arry[i].type == DSS_CLK_PCLK) {
+   if (enable)
+   rate = clk_arry[i].rate;
+   else
+   rate = 0;
+
+   rc = dev_pm_opp_set_rate(power->dev, rate);


I am not sure how this is expected to work when you have multiple link 
clocks,
since you can only associate one of them with the OPP table which ends 
up

getting scaled when you do a dev_pm_opp_set_rate()
Do you really have 

Re: [PATCH] drm/msm/dp: fixes wrong connection state caused by failure of link train

[khsieh]

On 2020-10-02 19:12, Stephen Boyd wrote:

Quoting Kuogee Hsieh (2020-10-02 15:09:19)
Connection state is set incorrectly happen at either failure of link 
train

or cable plugged in while suspended. This patch fixes these problems.
This patch also replace ST_SUSPEND_PENDING with ST_DISPLAY_OFF.

Signed-off-by: Kuogee Hsieh 


Any Fixes: tag?


---
 drivers/gpu/drm/msm/dp/dp_display.c | 52 
++---

 drivers/gpu/drm/msm/dp/dp_panel.c   |  5 +++
 2 files changed, 31 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c

index 431dff9de797..898c6cc1643a 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -340,8 +340,6 @@ static int dp_display_process_hpd_high(struct 
dp_display_private *dp)

}

dp_add_event(dp, EV_USER_NOTIFICATION, true, 0);
-
-
 end:
return rc;
 }


Not sure we need this hunk


@@ -1186,19 +1180,19 @@ static int dp_pm_resume(struct device *dev)

dp = container_of(dp_display, struct dp_display_private, 
dp_display);


+   /* start from dis connection state */


disconnection? Or disconnected state?


+   atomic_set(>hpd_state, ST_DISCONNECTED);
+
dp_display_host_init(dp);

dp_catalog_ctrl_hpd_config(dp->catalog);

status = dp_catalog_hpd_get_state_status(dp->catalog);

-   if (status) {
+   if (status)
dp->dp_display.is_connected = true;
-   } else {
+   else
dp->dp_display.is_connected = false;
-   /* make sure next resume host_init be called */
-   dp->core_initialized = false;
-   }

return 0;
 }
@@ -1214,6 +1208,9 @@ static int dp_pm_suspend(struct device *dev)
if (dp_display->power_on == true)
dp_display_disable(dp, 0);

+   /* host_init will be called at pm_resume */
+   dp->core_initialized = false;
+
atomic_set(>hpd_state, ST_SUSPENDED);

return 0;
@@ -1343,6 +1340,9 @@ int msm_dp_display_enable(struct msm_dp *dp, 
struct drm_encoder *encoder)


mutex_lock(_display->event_mutex);

+   /* delete sentinel checking */


Stop sentinel checking?


+   dp_del_event(dp_display, EV_CONNECT_PENDING_TIMEOUT);
+
rc = dp_display_set_mode(dp, _display->dp_mode);
if (rc) {
DRM_ERROR("Failed to perform a mode set, rc=%d\n", 
rc);
@@ -1368,9 +1368,8 @@ int msm_dp_display_enable(struct msm_dp *dp, 
struct drm_encoder *encoder)

dp_display_unprepare(dp);
}

-   dp_del_event(dp_display, EV_CONNECT_PENDING_TIMEOUT);
-
-   if (state == ST_SUSPEND_PENDING)
+   /* manual kick off plug event to train link */
+   if (state == ST_DISPLAY_OFF)
dp_add_event(dp_display, EV_IRQ_HPD_INT, 0, 0);

/* completed connection */
@@ -1402,20 +1401,21 @@ int msm_dp_display_disable(struct msm_dp *dp, 
struct drm_encoder *encoder)


mutex_lock(_display->event_mutex);

+   /* delete sentinel checking */


Stop sentinel checking?


+   dp_del_event(dp_display, EV_DISCONNECT_PENDING_TIMEOUT);
+
dp_display_disable(dp_display, 0);

rc = dp_display_unprepare(dp);
if (rc)
DRM_ERROR("DP display unprepare failed, rc=%d\n", rc);

-   dp_del_event(dp_display, EV_DISCONNECT_PENDING_TIMEOUT);
-
state =  atomic_read(_display->hpd_state);
if (state == ST_DISCONNECT_PENDING) {


I don't understand the atomic nature of this hpd_state variable. Why is
it an atomic variable? Is taking a spinlock bad? What is to prevent the
atomic read here to not be interrupted and then this if condition check
be invalid because the variable has been updated somewhere else?
hpd_state variable updated by multiple threads. however it was protected 
by mutex.
in theory, it should also work as u32. since it was declared as atomic 
from beginning

and it does not cause any negative effects, can we keep it as it is?


/* completed disconnection */
atomic_set(_display->hpd_state, ST_DISCONNECTED);
} else {
-   atomic_set(_display->hpd_state, 
ST_SUSPEND_PENDING);

+   atomic_set(_display->hpd_state, ST_DISPLAY_OFF);

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

Re: [PATCH] drm/msm/dp: add voltage corners voting support base on dp link rate

[khsieh]

On 2020-09-30 09:24, Rajendra Nayak wrote:

On 9/30/2020 1:54 PM, Stephen Boyd wrote:

Quoting Kuogee Hsieh (2020-09-29 10:10:26)

Set link rate by using OPP set rate api so that CX level will be set
accordingly base on the link rate.


s/base/based/



Signed-off-by: Kuogee Hsieh 
---
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c 
b/drivers/gpu/drm/msm/dp/dp_ctrl.c

index 2e3e1917351f..e1595d829e04 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1849,6 +1853,21 @@ struct dp_ctrl *dp_ctrl_get(struct device 
*dev, struct dp_link *link,

 return ERR_PTR(-ENOMEM);
 }
  +   ctrl->opp_table = dev_pm_opp_set_clkname(dev, "ctrl_link");


I see that downstream has multiple DP clocks which end up voting on
CX, we don't have a
way of associating multiple OPP tables with a device upstream, so
whats usually done is
(assuming all the clocks get scaled in lock step, which I assume is
the case here) we pick
the clock with the 'highest' CX requirement and associate that with
the OPP table.
I haven't looked but I am hoping thats how we have decided to
associate "ctrl_link" clock
here?


yes, only ctrl_link use dev_pm_opp_set_rate() to set rate.


+
+   if (IS_ERR(ctrl->opp_table)) {
+   dev_err(dev, "invalid DP OPP table in device 
tree\n");

+   ctrl->opp_table = NULL;
+   } else {
+   /* OPP table is optional */
+   ret = dev_pm_opp_of_add_table(dev);
+   if (ret && ret != -ENODEV) {
+   dev_err(dev, "add DP OPP table\n");


This is debug noise right?


+   dev_pm_opp_put_clkname(ctrl->opp_table);
+   ctrl->opp_table = NULL;
+   }
+   }
+
 init_completion(>idle_comp);
 init_completion(>video_comp);
  @@ -1864,6 +1883,18 @@ struct dp_ctrl *dp_ctrl_get(struct device 
*dev, struct dp_link *link,

 return >dp_ctrl;
  }
  -void dp_ctrl_put(struct dp_ctrl *dp_ctrl)
+void dp_ctrl_put(struct device *dev, struct dp_ctrl *dp_ctrl)
  {
+   struct dp_ctrl_private *ctrl;
+
+   if (!dp_ctrl)


Can this happen?


+   return;
+
+   ctrl = container_of(dp_ctrl, struct dp_ctrl_private, 
dp_ctrl);

+
+   if (ctrl->opp_table != NULL) {


This is usually written as

if (ctrl->opp_table)


+   dev_pm_opp_of_remove_table(dev);
+   dev_pm_opp_put_clkname(ctrl->opp_table);
+   ctrl->opp_table = NULL;
+   }
  }
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h 
b/drivers/gpu/drm/msm/dp/dp_ctrl.h

index f60ba93c8678..19b412a93e02 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
@@ -31,6 +31,6 @@ struct dp_ctrl *dp_ctrl_get(struct device *dev, 
struct dp_link *link,
 struct dp_panel *panel, struct drm_dp_aux 
*aux,
 struct dp_power *power, struct dp_catalog 
*catalog,

 struct dp_parser *parser);
-void dp_ctrl_put(struct dp_ctrl *dp_ctrl);
+void dp_ctrl_put(struct device *dev, struct dp_ctrl *dp_ctrl);


Is 'dev' not inside 'dp_ctrl'?


#endif /* _DP_CTRL_H_ */
diff --git a/drivers/gpu/drm/msm/dp/dp_power.c 
b/drivers/gpu/drm/msm/dp/dp_power.c

index 17c1fc6a2d44..3d75bf09e38f 100644
--- a/drivers/gpu/drm/msm/dp/dp_power.c
+++ b/drivers/gpu/drm/msm/dp/dp_power.c
@@ -8,12 +8,14 @@
  #include 
  #include 
  #include 
+#include 
  #include "dp_power.h"
  #include "msm_drv.h"
struct dp_power_private {
 struct dp_parser *parser;
 struct platform_device *pdev;
+   struct device *dev;
 struct clk *link_clk_src;
 struct clk *pixel_provider;
 struct clk *link_provider;
@@ -148,18 +150,49 @@ static int dp_power_clk_deinit(struct 
dp_power_private *power)

 return 0;
  }
  +static int dp_power_clk_set_link_rate(struct dp_power_private 
*power,
+   struct dss_clk *clk_arry, int num_clk, int 
enable)

+{
+   u32 rate;
+   int i, rc = 0;
+
+   for (i = 0; i < num_clk; i++) {
+   if (clk_arry[i].clk) {
+   if (clk_arry[i].type == DSS_CLK_PCLK) {
+   if (enable)
+   rate = clk_arry[i].rate;
+   else
+   rate = 0;
+
+   rc = dev_pm_opp_set_rate(power->dev, 
rate);


Why do we keep going if rc is non-zero?


+   }
+
+   }
+   }
+   return rc;
+}
+
  static int dp_power_clk_set_rate(struct dp_power_private *power,
 enum dp_pm_type module, bool enable)
  {
 int rc = 0;
 struct dss_module_power *mp = >parser->mp[module];
  -   if (enable) {
-   rc = msm_dss_clk_set_rate(mp->clk_config, 
mp->num_clk);

+   if (module == DP_CTRL_PM) {
+   rc = dp_power_clk_set_link_rate(power, 

Re: [PATCH] drm/msm/dp: Sleep properly in dp_hpd_handler kthread

[khsieh]

On 2020-09-17 15:44, Stephen Boyd wrote:

We shouldn't be waiting for an event here with a timeout of 100ms when
we're not in the 'timeout' arm of the if condition. Instead we should 
be

sleeping in the interruptible state (S) until something happens and we
need to wakeup. Right now this kthread is running almost all the time
because it sleeps for 100ms, wakes up, sees there's nothing to do, and
then starts the process all over again. Looking at top it shows up in
the D state (uninterruptible) because it uses wait_event_timeout(). FIx
this up.

Cc: Tanmay Shah 
Cc: Kuogee Hsieh 
Reported-by: Douglas Anderson 
Fixes: 8ede2ecc3e5e ("drm/msm/dp: Add DP compliance tests on
Snapdragon Chipsets")
Signed-off-by: Stephen Boyd 

Reviewed-by: Kuogee Hsieh 

---

Based on msm-next-dp of https://gitlab.freedesktop.org/drm/msm.git

 drivers/gpu/drm/msm/dp/dp_display.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 05a97e097edf..e175aa3fd3a9 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -970,9 +970,8 @@ static int hpd_event_thread(void *data)
(dp_priv->event_pndx == dp_priv->event_gndx),
EVENT_TIMEOUT);
} else {
-   wait_event_timeout(dp_priv->event_q,
-   (dp_priv->event_pndx != dp_priv->event_gndx),
-   EVENT_TIMEOUT);
+   wait_event_interruptible(dp_priv->event_q,
+   (dp_priv->event_pndx != dp_priv->event_gndx));
}
spin_lock_irqsave(_priv->event_lock, flag);
todo = _priv->event_list[dp_priv->event_gndx];

base-commit: 937f941ca06f2f3ab64baebf31be2c16d57ae7b8

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

Re: [PATCH v3] drm/msm/dp: Add DP compliance tests on Snapdragon Chipsets

[khsieh]

On 2020-08-18 14:59, Stephen Boyd wrote:

Quoting Kuogee Hsieh (2020-08-18 14:15:46)

add event thread to execute events serially from event queue. Also
timeout mode is supported  which allow an event be deferred to be
executed at later time. Both link and phy compliant tests had been
done successfully.

Changes in v2:
- Fix potential deadlock by removing redundant connect_mutex
- Check and enable link clock during modeset
- Drop unused code and fix function prototypes.
- set sink power to normal operation state (D0) before DPCD read

Changes in v3:
- push idle pattern at main link before timing generator off
- add timeout handles for both connect and disconnect

This patch depends-on following series:
https://lkml.kernel.org/lkml/20200812044223.19279-1-tan...@codeaurora.org/t.atom


There's a v11 of this series. Can you rebase again?



Signed-off-by: Kuogee Hsieh 
Signed-off-by: Guenter Roeck 
Signed-off-by: Tanmay Shah 


And fix this SoB chain to be proper with Co-developed-by tags and your
tag coming last as you're the sender of the patch.

Tanmay and Guenter signed-off were added by mistake. I will remove them
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/msm/dp: Add DP compliance tests on Snapdragon Chipsets

[khsieh]

On 2020-07-20 19:57, Rob Clark wrote:
On Mon, Jul 20, 2020 at 4:32 PM Stephen Boyd  
wrote:


Quoting khs...@codeaurora.org (2020-07-20 15:48:13)
> On 2020-07-20 13:18, Stephen Boyd wrote:
> > Quoting Kuogee Hsieh (2020-07-07 11:41:25)
> >>  drivers/gpu/drm/msm/dp/dp_power.c   |  32 +-
> >>  drivers/gpu/drm/msm/dp/dp_power.h   |   1 +
> >>  drivers/gpu/drm/msm/dp/dp_reg.h |   1 +
> >>  17 files changed, 861 insertions(+), 424 deletions(-)
> >
> > It seems to spread various changes throughout the DP bits and only has
> > a
> > short description about what's changing. Given that the series above
> > isn't merged it would be better to get rid of this change and make the
> > changes in the patches that introduce these files.
> >
>
> Yes, the base DP driver is not yet merged as its still in reviews and
> has been for a while.
> While it is being reviewed, different developers are working on
> different aspects of DP such as base DP driver, DP compliance, audio etc
> to keep things going in parallel.
> To maintain the authorship of the different developers, we prefer having
> them as separate changes and not merge them.
> We can make all these changes as part of the same series if that shall
> help to keep things together but would prefer the changes themselves to
> be separate.
> Please consider this and let us know if that works.
>

I'm not the maintainer here so it's not really up to me, but this is 
why
we have the Co-developed-by tag, to show that multiple people worked 
on

some patch. The patch is supposed to logically stand on its own
regardless of how many people worked on it. Authorship is a single
person but the Co-developed-by tag helps express that more than one
person is the actual author of the patch. Can you use that tag instead
and then squash this into the other DP patches?


The dpu mega-patches are hard enough to review already.. I'd really
appreciated it if the dpu dev's sort out some way to squash later
fixups into earlier patches

BR,
-R
as per discussion on IRC, I have separated the parts of this change 
which are
unrelated to compliance and we have merged it to the base DP driver and 
added
the Co-developed-by tag there. Since this change adds supports for DP 
compliance
on MSM chipsets which is a new feature and not fixes to the base driver, 
we will
prefer to have this as a separate change as it will make it easier for 
you to

review it instead of continuing to expand the base DP driver
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/msm/dp: Add DP compliance tests on Snapdragon Chipsets

[khsieh]

On 2020-07-20 13:18, Stephen Boyd wrote:

Quoting Kuogee Hsieh (2020-07-07 11:41:25)

add event thread to execute events serially from event queue. Also
timeout mode is supported  which allow an event be deferred to be
executed at later time. Both link and phy compliant tests had been
done successfully.

This change depends-on following series:

https://lore.kernel.org/dri-devel/20200630184507.15589-1-tan...@codeaurora.org/




Can this be sent along with that series?


Signed-off-by: Kuogee Hsieh 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  12 +-
 drivers/gpu/drm/msm/dp/dp_aux.c |   4 +
 drivers/gpu/drm/msm/dp/dp_aux.h |   1 +
 drivers/gpu/drm/msm/dp/dp_catalog.c |  78 ++-
 drivers/gpu/drm/msm/dp/dp_ctrl.c| 361 +++
 drivers/gpu/drm/msm/dp/dp_ctrl.h|   3 +-
 drivers/gpu/drm/msm/dp/dp_display.c | 654 
+---

 drivers/gpu/drm/msm/dp/dp_hpd.c |   2 +-
 drivers/gpu/drm/msm/dp/dp_hpd.h |   1 +
 drivers/gpu/drm/msm/dp/dp_link.c|  22 +-
 drivers/gpu/drm/msm/dp/dp_panel.c   |  56 +-
 drivers/gpu/drm/msm/dp/dp_panel.h   |  10 +-
 drivers/gpu/drm/msm/dp/dp_parser.c  |  45 +-
 drivers/gpu/drm/msm/dp/dp_parser.h  |   2 +
 drivers/gpu/drm/msm/dp/dp_power.c   |  32 +-
 drivers/gpu/drm/msm/dp/dp_power.h   |   1 +
 drivers/gpu/drm/msm/dp/dp_reg.h |   1 +
 17 files changed, 861 insertions(+), 424 deletions(-)


It seems to spread various changes throughout the DP bits and only has 
a

short description about what's changing. Given that the series above
isn't merged it would be better to get rid of this change and make the
changes in the patches that introduce these files.



Yes, the base DP driver is not yet merged as its still in reviews and 
has been for a while.
While it is being reviewed, different developers are working on 
different aspects of DP such as base DP driver, DP compliance, audio etc 
to keep things going in parallel.
To maintain the authorship of the different developers, we prefer having 
them as separate changes and not merge them.
We can make all these changes as part of the same series if that shall 
help to keep things together but would prefer the changes themselves to 
be separate.

Please consider this and let us know if that works.



diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c

index b439e482fc80..87b291b8d7b7 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1183,13 +1183,6 @@ static void dpu_encoder_virt_disable(struct 
drm_encoder *drm_enc)

dpu_kms = to_dpu_kms(priv->kms);
global_state = dpu_kms_get_existing_global_state(dpu_kms);

-   if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && 
priv->dp) {

-   if (msm_dp_display_disable(priv->dp, drm_enc)) {
-   DPU_ERROR_ENC(dpu_enc, "dp display disable 
failed\n");

-   return;
-   }
-   }
-
trace_dpu_enc_disable(DRMID(drm_enc));

/* wait for idle */
@@ -1220,6 +1213,11 @@ static void dpu_encoder_virt_disable(struct 
drm_encoder *drm_enc)


dpu_rm_release(global_state, drm_enc);

+   if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && 
priv->dp) {

+   if (msm_dp_display_disable(priv->dp, drm_enc))
+   DPU_ERROR_ENC(dpu_enc, "dp display disable 
failed\n");

+   }
+
mutex_unlock(_enc->enc_lock);
 }

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
b/drivers/gpu/drm/msm/dp/dp_aux.c

index 696dc8741f1e..c0e8ad031895 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -189,6 +189,8 @@ static void dp_aux_native_handler(struct 
dp_aux_private *aux)

aux->aux_error_num = DP_AUX_ERR_TOUT;
if (isr & DP_INTR_NACK_DEFER)
aux->aux_error_num = DP_AUX_ERR_NACK;
+   if (isr & DP_INTR_AUX_ERROR)
+   aux->aux_error_num = DP_AUX_ERR_DPPHY_AUX;

complete(>comp);
 }
@@ -359,6 +361,8 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux 
*dp_aux,

PHY_AUX_CFG1);
dp_catalog_aux_reset(aux->catalog);
}
+   if (aux->aux_error_num == DP_AUX_ERR_DPPHY_AUX)
+   usleep_range(400, 400); /* need 400us before 
next try */


Typically usleep_range() should be a range, and not the same number
for both ends of the range.


goto unlock_exit;
}

diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c 
b/drivers/gpu/drm/msm/dp/dp_catalog.c

index ab69ae3e2dbd..367eb54c9a68 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -452,7 +452,6 @@ void dp_catalog_aux_setup(struct dp_catalog 
*dp_catalog)
dp_write_phy(catalog, REG_DP_PHY_PD_CTL,