Re: [PATCH v2 3/3] drm/msm/dp: check main link status before start aux read
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
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))
Re: [PATCH v2 3/3] drm/msm/dp: check main link status before start aux read
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()
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");
Re: [PATCH v2 1/2] phy/qualcomm: add hbr3_hbr2 voltage and premphasis swing table
On 2021-02-24 11:39, Stephen Boyd wrote:
Quoting Stephen Boyd (2021-02-18 10:46:17)
Quoting Kuogee Hsieh (2021-02-18 08:51:10)
> Add hbr3_hbr2 voltage and premphasis swing table to support
> HBR3 link rate.
>
> Changes in V2:
> -- replaced upper case with lower case at hbr3_hbr2 table
>
> Signed-off-by: Kuogee Hsieh
> ---
Reviewed-by: Stephen Boyd
BTW, the DP driver already set rates for HBR2, so does that mean this
is
fixing the voltage and preemphasis settings for HBR2? If so we should
backport this to stable trees and mark it as fixing commit
52e013d0bffa
("phy: qcom-qmp: Add support for DP in USB3+DP combo phy").
Yes? No?
yes
Re: [Freedreno] [PATCH v2 2/2] drm/msm/dp: add supported max link rate specified from dtsi
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
Re: [PATCH v2 2/2] drm/msm/dp: add supported max link rate specified from dtsi
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.
Re: [PATCH v2 2/2] drm/msm/dp: add supported max link rate specified from dtsi
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?
Re: [PATCH 1/2] drm/msm/dp: postpone irq_hpd event during connection pending state
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.
Re: [PATCH 1/2] drm/msm/dp: postpone irq_hpd event during connection pending state
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).
Re: [PATCH 2/2] drm/msm/dp: unplug interrupt missed after irq_hpd handler
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
Re: [PATCH v2 0/2] fix missing unplug interrupt problem
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.
Re: [PATCH 2/2] drm/msm/dp: unplug interrupt missed after irq_hpd handler
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);
Re: [PATCH 1/2] drm/msm/dp: postpone irq_hpd event during connection pending state
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;
Re: [PATCH] drm/msm/dp: deinitialize mainlink if link training failedo
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
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?
Re: [PATCH] drm/msm/dp: skip checking LINK_STATUS_UPDATED bit
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;
}
Re: [PATCH v2] drm/msm/dp: add opp_table corner voting support base on dp_ink_clk rate
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
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);
Re: [PATCH] drm/msm/dp: add voltage corners voting support base on dp link rate
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
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
Re: [PATCH v3] drm/msm/dp: Add DP compliance tests on Snapdragon Chipsets
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
Re: [PATCH] drm/msm/dp: Add DP compliance tests on Snapdragon Chipsets
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
Re: [PATCH] drm/msm/dp: Add DP compliance tests on Snapdragon Chipsets
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,
