date:20220425

[Freedreno] [PATCH v2] drm/msm/dp: remove fail safe mode related code

2022-04-25 Thread Kuogee Hsieh

Current DP driver implementation has adding safe mode done at
dp_hpd_plug_handle() which is expected to be executed under event
thread context.

However there is possible circular locking happen (see blow stack trace)
after edp driver call dp_hpd_plug_handle() from dp_bridge_enable() which
is executed under drm_thread context.

After review all possibilities methods and as discussed on
https://patchwork.freedesktop.org/patch/483155/, supporting EDID
compliance tests in the driver is quite hacky. As seen with other
vendor drivers, supporting these will be much easier with IGT. Hence
removing all the related fail safe code for it so that no possibility
of circular lock will happen.

==
 WARNING: possible circular locking dependency detected
 5.15.35-lockdep #6 Tainted: GW
 --
 frecon/429 is trying to acquire lock:
 ff808dc3c4e8 (>mode_config.mutex){+.+.}-{3:3}, at:
dp_panel_add_fail_safe_mode+0x4c/0xa0

 but task is already holding lock:
 ff808dc441e0 (>commit_lock[i]){+.+.}-{3:3}, at: lock_crtcs+0xb4/0x124

 which lock already depends on the new lock.

 the existing dependency chain (in reverse order) is:

 -> #3 (>commit_lock[i]){+.+.}-{3:3}:
__mutex_lock_common+0x174/0x1a64
mutex_lock_nested+0x98/0xac
lock_crtcs+0xb4/0x124
msm_atomic_commit_tail+0x330/0x748
commit_tail+0x19c/0x278
drm_atomic_helper_commit+0x1dc/0x1f0
drm_atomic_commit+0xc0/0xd8
drm_atomic_helper_set_config+0xb4/0x134
drm_mode_setcrtc+0x688/0x1248
drm_ioctl_kernel+0x1e4/0x338
drm_ioctl+0x3a4/0x684
__arm64_sys_ioctl+0x118/0x154
invoke_syscall+0x78/0x224
el0_svc_common+0x178/0x200
do_el0_svc+0x94/0x13c
el0_svc+0x5c/0xec
el0t_64_sync_handler+0x78/0x108
el0t_64_sync+0x1a4/0x1a8

 -> #2 (crtc_ww_class_mutex){+.+.}-{3:3}:
__mutex_lock_common+0x174/0x1a64
ww_mutex_lock+0xb8/0x278
modeset_lock+0x304/0x4ac
drm_modeset_lock+0x4c/0x7c
drmm_mode_config_init+0x4a8/0xc50
msm_drm_init+0x274/0xac0
msm_drm_bind+0x20/0x2c
try_to_bring_up_master+0x3dc/0x470
__component_add+0x18c/0x3c0
component_add+0x1c/0x28
dp_display_probe+0x954/0xa98
platform_probe+0x124/0x15c
really_probe+0x1b0/0x5f8
__driver_probe_device+0x174/0x20c
driver_probe_device+0x70/0x134
__device_attach_driver+0x130/0x1d0
bus_for_each_drv+0xfc/0x14c
__device_attach+0x1bc/0x2bc
device_initial_probe+0x1c/0x28
bus_probe_device+0x94/0x178
deferred_probe_work_func+0x1a4/0x1f0
process_one_work+0x5d4/0x9dc
worker_thread+0x898/0xccc
kthread+0x2d4/0x3d4
ret_from_fork+0x10/0x20

 -> #1 (crtc_ww_class_acquire){+.+.}-{0:0}:
ww_acquire_init+0x1c4/0x2c8
drm_modeset_acquire_init+0x44/0xc8
drm_helper_probe_single_connector_modes+0xb0/0x12dc
drm_mode_getconnector+0x5dc/0xfe8
drm_ioctl_kernel+0x1e4/0x338
drm_ioctl+0x3a4/0x684
__arm64_sys_ioctl+0x118/0x154
invoke_syscall+0x78/0x224
el0_svc_common+0x178/0x200
do_el0_svc+0x94/0x13c
el0_svc+0x5c/0xec
el0t_64_sync_handler+0x78/0x108
el0t_64_sync+0x1a4/0x1a8

 -> #0 (>mode_config.mutex){+.+.}-{3:3}:
__lock_acquire+0x2650/0x672c
lock_acquire+0x1b4/0x4ac
__mutex_lock_common+0x174/0x1a64
mutex_lock_nested+0x98/0xac
dp_panel_add_fail_safe_mode+0x4c/0xa0
dp_hpd_plug_handle+0x1f0/0x280
dp_bridge_enable+0x94/0x2b8
drm_atomic_bridge_chain_enable+0x11c/0x168
drm_atomic_helper_commit_modeset_enables+0x500/0x740
msm_atomic_commit_tail+0x3e4/0x748
commit_tail+0x19c/0x278
drm_atomic_helper_commit+0x1dc/0x1f0
drm_atomic_commit+0xc0/0xd8
drm_atomic_helper_set_config+0xb4/0x134
drm_mode_setcrtc+0x688/0x1248
drm_ioctl_kernel+0x1e4/0x338
drm_ioctl+0x3a4/0x684
__arm64_sys_ioctl+0x118/0x154
invoke_syscall+0x78/0x224
el0_svc_common+0x178/0x200
do_el0_svc+0x94/0x13c
el0_svc+0x5c/0xec
el0t_64_sync_handler+0x78/0x108
el0t_64_sync+0x1a4/0x1a8

Changes in v2:
-- re text commit title
-- remove all fail safe mode

Signed-off-by: Kuogee Hsieh 
---
 drivers/gpu/drm/msm/dp/dp_display.c |  6 --
 drivers/gpu/drm/msm/dp/dp_panel.c   | 11 ---
 2 files changed, 17 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c
index 92cd50f..01453db 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -555,12 +555,6 @@ static int dp_hpd_plug_handle(struct dp_display_private 
*dp, u32 data)
 
mutex_unlock(>event_mutex);
 
-   /*
-* add fail safe

Re: [Freedreno] [PATCH v9 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP

2022-04-25 Thread Sankeerth Billakanti (QUIC)

Hi Stephen,

>> >>  int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog)
>> >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>> >> b/drivers/gpu/drm/msm/dp/dp_display.c
>> >> index 055681a..dea4de9 100644
>> >> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> >> @@ -1096,6 +1097,13 @@ static void dp_display_config_hpd(struct
>> >dp_display_private *dp)
>> >> dp_display_host_init(dp);
>> >> dp_catalog_ctrl_hpd_config(dp->catalog);
>> >>
>> >> +   /* Enable plug and unplug interrupts only for external 
>> >> DisplayPort */
>> >> +   if (!dp->dp_display.is_edp)
>> >> +   dp_catalog_hpd_config_intr(dp->catalog,
>> >> +   DP_DP_HPD_PLUG_INT_MASK |
>> >> +   DP_DP_HPD_UNPLUG_INT_MASK,
>> >> +   true);
>> >> +
>> >
>> >It seems like only the plug and unplug is enabled for DP here. Is
>> >replug enabled for eDP when it shouldn't be?
>> >
>>
>> By default, all the interrupts are masked. This function is not
>> executed for eDP anymore because the host_init, phy_init and
>> enable_irq are all done from modeset_init for eDP with aux_bus. So,
>> none of the eDP hpd interrupts are enabled or unmasked before pre-
>enable.
>>
>> The replug interrupt is unmasked for DP and eDP from the
>> dp_hpd_plug_handle() and masked from dp_hpd_unplug_handle().
>
>Why is replug enabled for eDP?

As the eDP panel is assumed to be always connected, just enabling the IRQ_HPD 
is sufficient.

The REPLUG is enabled or unmasked along with IRQ_HPD in code.

I did not remove the REPLUG event support for eDP so that we have an option to 
see if the eDP panel
can undergo a short disconnect/connect cycle after pre-enable (while the panel 
power is enabled).

REPLUG can be generated for eDP if,
a) the panel power turns off and on OR 
b) the sink itself issues a fast disconnect-connect.

REPLUG event initiated by sink is rare and we observed it only during the DP 
compliance test.

Some more information on HPD events generated by the source:

Replug interrupt is something our controller HW supports and not part of the 
DP/eDP specification.

The programmed values for HPD on the HW controller indicates the following:

1. The HOTPLUG interrupt will be generated if the HPD line is continuously high 
for 100ms.
2. Similarly, UNPLUG interrupt will be generated when the HPD line transitions 
from high to low and remains low for 100ms.
3. IRQ_HPD will be generated when the HPD line transitions from high to low and 
remains low for less than 2ms.
4. REPLUG will be generated if the HPD line remains low for more than 2ms but 
less than 100ms.

According to the DP spec, replug event should be considered as a disconnect and 
then connect.

To answer your question, I did not remove REPLUG support for eDP because I felt 
it will not affect the eDP normal functioning in anyway.

Thank you,
Sankeerth

Re: [Freedreno] [PATCH] drm/msm/dp: move add fail safe mode to dp_connector_get_mode()

2022-04-25 Thread Abhinav Kumar





On 4/25/2022 7:18 PM, Doug Anderson wrote:

Hi,

On Mon, Apr 25, 2022 at 6:42 PM Abhinav Kumar  wrote:



2) When there was a valid EDID but no 640x480 mode

This is the equipment specific case and the one even I was a bit
surprised. There is a DP compliance equipment we have in-house and while
validation, it was found that in its list of modes , it did not have any
modes which chromebook supported ( due to 2 lanes ). But my
understanding was that, all sinks should have atleast 640x480 but
apparently this one did not have that. So to handle this DP compliance
equipment behavior, we had to do this.


That doesn't seem right. If there's a valid EDID and the valid EDID
doesn't contain 640x480, are you _sure_ you're supposed to be adding
640x480? That doesn't sound right to me. I've got a tiny display in
front of me for testing that only has one mode:

#0 800x480 65.68 800 840 888 928 480 493 496 525 32000



As I had wrote, DRM core kicks in only when the count of modes is 0.
Here what is happening is the count was not 0 but 640x480 was not
present in the EDID. So we had to add it explicitly.

Your tiny display is a display port display?

I am referring to only display port monitors. If your tiny display is
DP, it should have had 640x480 in its list of modes.


My tiny display is actually a HDMI display hooked up to a HDMI to DP
(active) adapter.

...but this is a legal and common thing to have. I suppose possibly my
HDMI display is "illegal"?

OK, so reading through the spec more carefully, I do see that the DP
spec makes numerous mentions of the fact that DP sinks _must_ support
640x480. Even going back to DP 1.4, I see section "5.2.1.2 Video
Timing Format" says that we must support 640x480. It seems like that's
_intended_ to be used only if the EDID read fails, though or if we
somehow have to output video without knowledge of the EDID. It seems
hard to believe that there's a great reason to assume a display will
support 640x480 if we have more accurate knowledge.

In any case, I guess I would still say that adding this mode belongs
in the DRM core. The core should notice that it's a DP connection
(bridge->type == DRM_MODE_CONNECTOR_DisplayPort) and that 640x480 was
left out and it should add it. We should also make sure it's not
"preferred" and is last in the list so we never accidentally pick it.
If DP truly says that we should always give the user 640x480 then
that's true for everyone, not just Qualcomm. We should add it in the
core. If, later, someone wants to hide this from the UI it would be
much easier if they only needed to modify one place.



So I debugged with kuogee just now using the DP compliance equipment.
It turns out, the issue is not that 640x480 mode is not present.

The issue is that it is not marked as preferred.

Hence we missed this part during debugging this equipment failure.

We still have to figure out the best way to either mark 640x480 as 
preferred or eliminate other modes during the test-case so that 640x480 
is actually picked by usermode.


Now that being said, the fix still doesn't belong in the framework. It 
has to be in the msm/dp code.


Different vendors handle this failure case differently looks like.

Lets take below snippet from i915 as example.

3361if (intel_connector->detect_edid == NULL ||
3362connector->edid_corrupt ||
3363intel_dp->aux.i2c_defer_count > 6) {
3364/* Check EDID read for NACKs, DEFERs and corruption
3365 * (DP CTS 1.2 Core r1.1)
3366 *4.2.2.4 : Failed EDID read, I2C_NAK
3367 *4.2.2.5 : Failed EDID read, I2C_DEFER
3368 *4.2.2.6 : EDID corruption detected
3369 * Use failsafe mode for all cases
3370 */
3371if (intel_dp->aux.i2c_nack_count > 0 ||
3372intel_dp->aux.i2c_defer_count > 0)
3373drm_dbg_kms(>drm,
3374"EDID read had %d NACKs, %d DEFERs\n",
3375intel_dp->aux.i2c_nack_count,
3376intel_dp->aux.i2c_defer_count);
3377intel_dp->compliance.test_data.edid = 
INTEL_DP_RESOLUTION_FAILSAFE;

This marks the fail safe mode and IGT test case reads this to set this 
mode and hence the test passes.


We rely on the chromeOS usermode to output pixel data for this test-case 
and not IGT. We use IGT only for video pattern CTS today but this is a 
different test-case which is failing.


ChromeOS usermode will not pick 640x480 unless we mark it as preferred 
or other modes are eliminated.


So we have to come up with the right way for the usermode to pick 640x480.

We will discuss this a bit more and come up with a different change.




So IMO we _shouldn't_ land ${SUBJECT} patch.

Just for testing, I also tried a hack to make EDID reading fail
(return -EIO in the MSM dp_aux_transfer() function if msg->request <
8). Before ${SUBJECT} patch I'd see these modes:

#0 1024x768 60.00

Re: [Freedreno] [PATCH] drm/msm/dp: move add fail safe mode to dp_connector_get_mode()

2022-04-25 Thread Doug Anderson

Hi,

On Mon, Apr 25, 2022 at 6:42 PM Abhinav Kumar  wrote:
>
> >> 2) When there was a valid EDID but no 640x480 mode
> >>
> >> This is the equipment specific case and the one even I was a bit
> >> surprised. There is a DP compliance equipment we have in-house and while
> >> validation, it was found that in its list of modes , it did not have any
> >> modes which chromebook supported ( due to 2 lanes ). But my
> >> understanding was that, all sinks should have atleast 640x480 but
> >> apparently this one did not have that. So to handle this DP compliance
> >> equipment behavior, we had to do this.
> >
> > That doesn't seem right. If there's a valid EDID and the valid EDID
> > doesn't contain 640x480, are you _sure_ you're supposed to be adding
> > 640x480? That doesn't sound right to me. I've got a tiny display in
> > front of me for testing that only has one mode:
> >
> >#0 800x480 65.68 800 840 888 928 480 493 496 525 32000
> >
>
> As I had wrote, DRM core kicks in only when the count of modes is 0.
> Here what is happening is the count was not 0 but 640x480 was not
> present in the EDID. So we had to add it explicitly.
>
> Your tiny display is a display port display?
>
> I am referring to only display port monitors. If your tiny display is
> DP, it should have had 640x480 in its list of modes.

My tiny display is actually a HDMI display hooked up to a HDMI to DP
(active) adapter.

...but this is a legal and common thing to have. I suppose possibly my
HDMI display is "illegal"?

OK, so reading through the spec more carefully, I do see that the DP
spec makes numerous mentions of the fact that DP sinks _must_ support
640x480. Even going back to DP 1.4, I see section "5.2.1.2 Video
Timing Format" says that we must support 640x480. It seems like that's
_intended_ to be used only if the EDID read fails, though or if we
somehow have to output video without knowledge of the EDID. It seems
hard to believe that there's a great reason to assume a display will
support 640x480 if we have more accurate knowledge.

In any case, I guess I would still say that adding this mode belongs
in the DRM core. The core should notice that it's a DP connection
(bridge->type == DRM_MODE_CONNECTOR_DisplayPort) and that 640x480 was
left out and it should add it. We should also make sure it's not
"preferred" and is last in the list so we never accidentally pick it.
If DP truly says that we should always give the user 640x480 then
that's true for everyone, not just Qualcomm. We should add it in the
core. If, later, someone wants to hide this from the UI it would be
much easier if they only needed to modify one place.

> > So IMO we _shouldn't_ land ${SUBJECT} patch.
> >
> > Just for testing, I also tried a hack to make EDID reading fail
> > (return -EIO in the MSM dp_aux_transfer() function if msg->request <
> > 8). Before ${SUBJECT} patch I'd see these modes:
> >
> >#0 1024x768 60.00 1024 1048 1184 1344 768 771 777 806 65000
> >#1 800x600 60.32 800 840 968 1056 600 601 605 628 4
> >#2 800x600 56.25 800 824 896 1024 600 601 603 625 36000
> >#3 848x480 60.00 848 864 976 1088 480 486 494 517 33750
> >#4 640x480 59.94 640 656 752 800 480 490 492 525 25175
> >
> > ...and after ${SUBJECT} patch I'd see:
> >
> >#0 640x480 59.94 640 656 752 800 480 490 492 525 25175
> >#1 1024x768 60.00 1024 1048 1184 1344 768 771 777 806 65000
> >#2 800x600 60.32 800 840 968 1056 600 601 605 628 4
> >#3 800x600 56.25 800 824 896 1024 600 601 603 625 36000
> >#4 848x480 60.00 848 864 976 1088 480 486 494 517 33750
> >
> > ...so your patch causes 640x480 to be prioritized. That also doesn't
> > seem ideal. If it was ideal, the DRM core should have listed 640x480
> > first.
>
> So this is a different display or these modes are coming due to the
> drm_add_modes_noedid() call because of the EDID read fail right?

Right, it's from the !edid case.

-Doug

Re: [Freedreno] [PATCH v5 15/19] drm/msm/dpu: initialize dpu encoder and connector for writeback

2022-04-25 Thread Dmitry Baryshkov


On 26/04/2022 03:32, Laurent Pinchart wrote:

Hi Abhinav,

On Sun, Apr 24, 2022 at 05:32:06PM -0700, Abhinav Kumar wrote:

Initialize dpu encoder and connector for writeback if the
target supports it in the catalog.

changes in v2:
- start initialing the encoder for writeback since we
have migrated to using drm_writeback_connector_init_with_encoder()
- instead of checking for WB_2 inside _dpu_kms_initialize_writeback
call it only when its WB_2
- rebase on tip of msm-next and remove usage of priv->encoders

changes in v3:
- none

changes in v4:
- fix copyright years order

Signed-off-by: Abhinav Kumar 
Reviewed-by: Dmitry Baryshkov 
---
  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 27 +
  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 61 -
  2 files changed, 80 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 24870eb..2d79002 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -2102,7 +2102,7 @@ static void dpu_encoder_early_unregister(struct 
drm_encoder *encoder)
  }
  
  static int dpu_encoder_virt_add_phys_encs(

-   u32 display_caps,
+   struct msm_display_info *disp_info,
struct dpu_encoder_virt *dpu_enc,
struct dpu_enc_phys_init_params *params)
  {
@@ -2121,7 +2121,7 @@ static int dpu_encoder_virt_add_phys_encs(
return -EINVAL;
}
  
-	if (display_caps & MSM_DISPLAY_CAP_VID_MODE) {

+   if (disp_info->capabilities & MSM_DISPLAY_CAP_VID_MODE) {
enc = dpu_encoder_phys_vid_init(params);
  
  		if (IS_ERR_OR_NULL(enc)) {

@@ -2134,7 +2134,7 @@ static int dpu_encoder_virt_add_phys_encs(
++dpu_enc->num_phys_encs;
}
  
-	if (display_caps & MSM_DISPLAY_CAP_CMD_MODE) {

+   if (disp_info->capabilities & MSM_DISPLAY_CAP_CMD_MODE) {
enc = dpu_encoder_phys_cmd_init(params);
  
  		if (IS_ERR_OR_NULL(enc)) {

@@ -2147,6 +2147,19 @@ static int dpu_encoder_virt_add_phys_encs(
++dpu_enc->num_phys_encs;
}
  
+	if (disp_info->intf_type == DRM_MODE_ENCODER_VIRTUAL) {

+   enc = dpu_encoder_phys_wb_init(params);
+
+   if (IS_ERR_OR_NULL(enc)) {
+   DPU_ERROR_ENC(dpu_enc, "failed to init wb enc: %ld\n",
+   PTR_ERR(enc));
+   return enc == NULL ? -EINVAL : PTR_ERR(enc);
+   }
+
+   dpu_enc->phys_encs[dpu_enc->num_phys_encs] = enc;
+   ++dpu_enc->num_phys_encs;
+   }
+
if (params->split_role == ENC_ROLE_SLAVE)
dpu_enc->cur_slave = enc;
else
@@ -2248,9 +2261,8 @@ static int dpu_encoder_setup_display(struct 
dpu_encoder_virt *dpu_enc,
}
  
  		if (!ret) {

-   ret = 
dpu_encoder_virt_add_phys_encs(disp_info->capabilities,
-   
 dpu_enc,
-   
 _params);
+   ret = dpu_encoder_virt_add_phys_encs(disp_info,
+   dpu_enc, _params);
if (ret)
DPU_ERROR_ENC(dpu_enc, "failed to add phys 
encs\n");
}
@@ -2367,8 +2379,9 @@ struct drm_encoder *dpu_encoder_init(struct drm_device 
*dev,
if (!dpu_enc)
return ERR_PTR(-ENOMEM);
  
+

rc = drm_encoder_init(dev, _enc->base, _encoder_funcs,
-   drm_enc_mode, NULL);
+ drm_enc_mode, NULL);
if (rc) {
devm_kfree(dev->dev, dpu_enc);
return ERR_PTR(rc);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index c683cab..9a406e1 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -1,7 +1,9 @@
  // SPDX-License-Identifier: GPL-2.0-only
  /*
- * Copyright (c) 2014-2018, The Linux Foundation. All rights reserved.
   * Copyright (C) 2013 Red Hat
+ * Copyright (c) 2014-2018, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
+ *
   * Author: Rob Clark 
   */
  
@@ -15,6 +17,7 @@

  #include 
  #include 
  #include 
+#include 
  
  #include "msm_drv.h"

  #include "msm_mmu.h"
@@ -29,6 +32,7 @@
  #include "dpu_kms.h"
  #include "dpu_plane.h"
  #include "dpu_vbif.h"
+#include "dpu_writeback.h"
  
  #define CREATE_TRACE_POINTS

  #include "dpu_trace.h"
@@ -648,6 +652,45 @@ static int _dpu_kms_initialize_displayport(struct 
drm_device *dev,
return 0;
  }
  
+static int

Re: [Freedreno] [PATCH v9 1/4] drm/msm/dp: Add eDP support via aux_bus

2022-04-25 Thread Sankeerth Billakanti (QUIC)

Hi Stephen,

 Quoting Sankeerth Billakanti (2022-04-22 02:11:03)
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index d7a19d6..055681a 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c

 Some nitpicks

 Reviewed-by: Stephen Boyd 

> @@ -1508,7 +1509,8 @@ 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_display->is_edp)
> +   dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);

 Did it turn out that in fact DP isn't ready still to setup even
 after delaying the irq?

>>>
>>> The host_init, config_hpd, phy_init and enable_irq are happening in
>modeset_init already for eDP.
>>> So, I am not scheduling the EV_HPD_INIT_SETUP event for eDP. I am not
>modifying the delay for DP.
>>
>> Cool. That didn't answer my question though. Why does DP still need
>> the delay? I thought recent changes made it unnecessary.
>
>I'd say that if it is not necessary, it should be changed in the separate 
>commit.
>The question is valid nevertheless.
>

Yes, that is right. The delay is unnecessary with the recent changes.
Like Dmitry rightly suggested, we will remove the delay in a separate commit.

>
>--
>With best wishes
>Dmitry

Re: [Freedreno] [PATCH] drm/msm/dp: move add fail safe mode to dp_connector_get_mode()

2022-04-25 Thread Abhinav Kumar


Hi Doug

On 4/25/2022 5:26 PM, Doug Anderson wrote:

Hi,

On Sat, Apr 23, 2022 at 8:34 AM Abhinav Kumar  wrote:


On 4/22/2022 11:25 PM, Dmitry Baryshkov wrote:

On Sat, 23 Apr 2022 at 03:12, Abhinav Kumar  wrote:




On 4/22/2022 5:07 PM, Dmitry Baryshkov wrote:

On 23/04/2022 02:45, Kuogee Hsieh wrote:

Current DP driver implementation has adding safe mode done at
dp_hpd_plug_handle() which is expected to be executed under event
thread context.

However there is possible circular locking happen (see blow stack trace)
after edp driver call dp_hpd_plug_handle() from dp_bridge_enable() which
is executed under drm_thread context.

To break this circular locking, this patch have safe mode added at
dp_connector_get_mode() which is executed under drm thread context.
Therefore no lock acquired required for >mode_config.mutex while
adding fail safe mode since it has been hold by drm thread already.

==
WARNING: possible circular locking dependency detected
5.15.35-lockdep #6 Tainted: GW
--
frecon/429 is trying to acquire lock:
ff808dc3c4e8 (>mode_config.mutex){+.+.}-{3:3}, at:
dp_panel_add_fail_safe_mode+0x4c/0xa0

but task is already holding lock:
ff808dc441e0 (>commit_lock[i]){+.+.}-{3:3}, at:
lock_crtcs+0xb4/0x124

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #3 (>commit_lock[i]){+.+.}-{3:3}:
   __mutex_lock_common+0x174/0x1a64
   mutex_lock_nested+0x98/0xac
   lock_crtcs+0xb4/0x124
   msm_atomic_commit_tail+0x330/0x748
   commit_tail+0x19c/0x278
   drm_atomic_helper_commit+0x1dc/0x1f0
   drm_atomic_commit+0xc0/0xd8
   drm_atomic_helper_set_config+0xb4/0x134
   drm_mode_setcrtc+0x688/0x1248
   drm_ioctl_kernel+0x1e4/0x338
   drm_ioctl+0x3a4/0x684
   __arm64_sys_ioctl+0x118/0x154
   invoke_syscall+0x78/0x224
   el0_svc_common+0x178/0x200
   do_el0_svc+0x94/0x13c
   el0_svc+0x5c/0xec
   el0t_64_sync_handler+0x78/0x108
   el0t_64_sync+0x1a4/0x1a8

-> #2 (crtc_ww_class_mutex){+.+.}-{3:3}:
   __mutex_lock_common+0x174/0x1a64
   ww_mutex_lock+0xb8/0x278
   modeset_lock+0x304/0x4ac
   drm_modeset_lock+0x4c/0x7c
   drmm_mode_config_init+0x4a8/0xc50
   msm_drm_init+0x274/0xac0
   msm_drm_bind+0x20/0x2c
   try_to_bring_up_master+0x3dc/0x470
   __component_add+0x18c/0x3c0
   component_add+0x1c/0x28
   dp_display_probe+0x954/0xa98
   platform_probe+0x124/0x15c
   really_probe+0x1b0/0x5f8
   __driver_probe_device+0x174/0x20c
   driver_probe_device+0x70/0x134
   __device_attach_driver+0x130/0x1d0
   bus_for_each_drv+0xfc/0x14c
   __device_attach+0x1bc/0x2bc
   device_initial_probe+0x1c/0x28
   bus_probe_device+0x94/0x178
   deferred_probe_work_func+0x1a4/0x1f0
   process_one_work+0x5d4/0x9dc
   worker_thread+0x898/0xccc
   kthread+0x2d4/0x3d4
   ret_from_fork+0x10/0x20

-> #1 (crtc_ww_class_acquire){+.+.}-{0:0}:
   ww_acquire_init+0x1c4/0x2c8
   drm_modeset_acquire_init+0x44/0xc8
   drm_helper_probe_single_connector_modes+0xb0/0x12dc
   drm_mode_getconnector+0x5dc/0xfe8
   drm_ioctl_kernel+0x1e4/0x338
   drm_ioctl+0x3a4/0x684
   __arm64_sys_ioctl+0x118/0x154
   invoke_syscall+0x78/0x224
   el0_svc_common+0x178/0x200
   do_el0_svc+0x94/0x13c
   el0_svc+0x5c/0xec
   el0t_64_sync_handler+0x78/0x108
   el0t_64_sync+0x1a4/0x1a8

-> #0 (>mode_config.mutex){+.+.}-{3:3}:
   __lock_acquire+0x2650/0x672c
   lock_acquire+0x1b4/0x4ac
   __mutex_lock_common+0x174/0x1a64
   mutex_lock_nested+0x98/0xac
   dp_panel_add_fail_safe_mode+0x4c/0xa0
   dp_hpd_plug_handle+0x1f0/0x280
   dp_bridge_enable+0x94/0x2b8
   drm_atomic_bridge_chain_enable+0x11c/0x168
   drm_atomic_helper_commit_modeset_enables+0x500/0x740
   msm_atomic_commit_tail+0x3e4/0x748
   commit_tail+0x19c/0x278
   drm_atomic_helper_commit+0x1dc/0x1f0
   drm_atomic_commit+0xc0/0xd8
   drm_atomic_helper_set_config+0xb4/0x134
   drm_mode_setcrtc+0x688/0x1248
   drm_ioctl_kernel+0x1e4/0x338
   drm_ioctl+0x3a4/0x684
   __arm64_sys_ioctl+0x118/0x154
   invoke_syscall+0x78/0x224
   el0_svc_common+0x178/0x200
   do_el0_svc+0x94/0x13c
   el0_svc+0x5c/0xec
   el0t_64_sync_handler+0x78/0x108
   el0t_64_sync+0x1a4/0x1a8

Signed-off-by: Kuogee Hsieh 
---
drivers/gpu/drm/msm/dp/dp_display.c |  6 --

[Freedreno] [PULL] drm/msm display pull request

2022-04-25 Thread Dmitry Baryshkov

Hi Rob,

This is a pull request over the patches accumulated, reviewed and tested for
the 5.19 merge window. This pull request contains following changes:

 - DPU: DSC (Display Stream Compression) support
 - DPU: inline rotation support on SC7280
 - DPU: update DP timings to follow vendor recommendations
 - DP, DPU: add support for wide bus (on newer chipsets)
 - DP: eDP support
 - Merge DPU1 and MDP5 MDSS driver, make dpu/mdp device the master
   component
 - MDSS: optionally reset the IP block at the bootup to drop
   bootloader state
 - Properly register and unregister internal bridges in the DRM framework
 - Complete DPU IRQ cleanup
 - DP: conversion to use drm_bridge and drm_bridge_connector
 - Misc small fixes

This request still comes as a merge of several local branches. If you wish, I
can rebase them into a single stream of patches.

There are several other series which are very close to the merge (e.g. the
writeback support), so if the time permits I might send another pull request
later.

The following changes since commit 78f815c1cf8fc5f05dc5cec29eb1895cb53470e9:

  drm/msm: return the average load over the polling period (2022-04-21 15:05:23 
-0700)

are available in the Git repository at:

  https://gitlab.freedesktop.org/lumag/msm.git msm-next-lumag

for you to fetch changes up to 7e7657dc3f145df9990494f17aebcbea163f4a84:

  Merge branches 'msm-next-lumag-core', 'msm-next-lumag-dpu', 
'msm-next-lumag-dp', 'msm-next-lumag-dsi', 'msm-next-lumag-hdmi', 
'msm-next-lumag-mdp5' and 'msm-next-lumag-mdp4' into msm-next-lumag (2022-04-26 
04:08:20 +0300)


Abhinav Kumar (1):
  drm/msm: remove unused hotplug and edid macros from msm_drv.h

Bjorn Andersson (2):
  dt-bindings: display: msm: Add optional resets
  drm/msm/dpu: Issue MDSS reset during initialization

Chia-I Wu (1):
  drm/msm: add trace_dma_fence_emit to msm_gpu_submit

Dmitry Baryshkov (30):
  drm/msm: unify MDSS drivers
  drm/msm: remove extra indirection for msm_mdss
  drm/msm: split the main platform driver
  drm/msm: stop using device's match data pointer
  drm/msm: allow compile time selection of driver components
  drm/msm: make mdp5/dpu devices master components
  drm/msm: properly add and remove internal bridges
  drm/msm/dpu: remove manual destruction of DRM objects
  drm/msm: loop over encoders using drm_for_each_encoder()
  drm/msm: don't store created planes, connectors and encoders
  drm/msm: remove unused plane_property field from msm_drm_private
  drm/msm/dpu: don't use merge_3d if DSC merge topology is used
  drm/msm/dp: replace dp_connector with drm_bridge_connector
  drm/msm/dp: remove extra wrappers and public functions
  drm/msm/dp: drop dp_mode argument from dp_panel_get_modes()
  drm/msm/dp: simplify dp_connector_get_modes()
  drm/msm/dp: remove max_pclk_khz field from dp_panel/dp_display
  drm/msm/dpu: remove extra wrappers around dpu_core_irq
  drm/msm/dpu: remove always-true argument of dpu_core_irq_read()
  drm/msm/dpu: allow just single IRQ callback
  drm/msm/dpu: get rid of dpu_encoder_helper_(un)register_irq
  drm/msm/dpu: remove struct dpu_encoder_irq
  drm/msm/dpu: pass irq to dpu_encoder_helper_wait_for_irq()
  drm/msm/dpu: document INTF_EDP/INTF_DP difference
  drm/msm/dpu: drop INTF_TYPE_MAX symbol
  drm/msm/dpu: drop obsolete INTF_EDP comment
  drm/msm/dpu: drop INTF_EDP from interface type conditions
  drm/msm/dsi: fix error checks and return values for DSI xmit functions
  drm/msm: select DRM_DP_AUX_BUS for the AUX bus support
  Merge branches 'msm-next-lumag-core', 'msm-next-lumag-dpu', 
'msm-next-lumag-dp', 'msm-next-lumag-dsi', 'msm-next-lumag-hdmi', 
'msm-next-lumag-mdp5' and 'msm-next-lumag-mdp4' into msm-next-lumag

Guo Zhengkui (1):
  drm/msm: fix returnvar.cocci warning

Haowen Bai (1):
  drm/msm/mdp5: Eliminate useless code

Kuogee Hsieh (7):
  drm/msm/dpu: adjust display_v_end for eDP and DP
  drm/msm/dpu: replace BIT(x) with correspond marco define string
  drm/msm/dpu: revise timing engine programming to support widebus feature
  drm/msm/dp: enable widebus feature for display port
  drm/msm/dp: replace DRM_DEBUG_DP marco with drm_dbg_dp
  drm/msm/dp: stop event kernel thread when DP unbind
  drm/msm/dp: tear down main link at unplug handle immediately

Lv Ruyi (4):
  drm: msm: fix error check return value of irq_of_parse_and_map()
  drm/msm/dp: fix error check return value of irq_of_parse_and_map()
  drm/msm/hdmi: fix error check return value of irq_of_parse_and_map()
  drm/msm/dpu: fix error check return value of irq_of_parse_and_map()

Marijn Suijten (1):
  drm/msm/dpu: Bind pingpong block to intf on active ctls in cmd encoder

Sankeerth Billakanti (4):
  drm/msm/dp: Add eDP support via aux_bus
  drm/msm/dp: Support only IRQ_HPD

Re: [Freedreno] [PATCH v5 10/19] drm/msm/dpu: make changes to dpu_encoder to support virtual encoder

2022-04-25 Thread Dmitry Baryshkov


On 26/04/2022 03:44, Abhinav Kumar wrote:

Hi Dmitry

On 4/25/2022 5:21 PM, Dmitry Baryshkov wrote:

On 25/04/2022 03:32, Abhinav Kumar wrote:

Make changes to dpu_encoder to support virtual encoder needed
to support writeback for dpu.

changes in v4:
- squash dpu_encoder pieces from [1]

[1] https://patchwork.freedesktop.org/patch/483099/?series=102964=2

Signed-off-by: Abhinav Kumar 
---
  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c  | 94 
+++-

  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h |  7 ++
  2 files changed, 83 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c

index 25c7eda..d1e92d89 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1013,9 +1013,18 @@ static void 
dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,

  if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX)
  phys->hw_intf = dpu_rm_get_intf(_kms->rm, 
phys->intf_idx);

-    if (!phys->hw_intf) {
+    if (phys->wb_idx >= WB_0 && phys->wb_idx < WB_MAX)
+    phys->hw_wb = dpu_rm_get_wb(_kms->rm, phys->wb_idx);
+
+    if (!phys->hw_intf && !phys->hw_wb) {
  DPU_ERROR_ENC(dpu_enc,
-  "no intf block assigned at idx: %d\n", i);
+  "no intf or wb block assigned at idx: %d\n", i);
+    return;
+    }
+
+    if (phys->hw_intf && phys->hw_wb) {
+    DPU_ERROR_ENC(dpu_enc,
+    "invalid phys both intf and wb block at idx: 
%d\n", i);

  return;
  }
@@ -1163,16 +1172,35 @@ static enum dpu_intf 
dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog,

  {
  int i = 0;
-    for (i = 0; i < catalog->intf_count; i++) {
-    if (catalog->intf[i].type == type
-    && catalog->intf[i].controller_id == controller_id) {
-    return catalog->intf[i].id;
+    if (type != INTF_WB) {
+    for (i = 0; i < catalog->intf_count; i++) {
+    if (catalog->intf[i].type == type
+    && catalog->intf[i].controller_id == controller_id) {
+    return catalog->intf[i].id;
+    }
  }
  }
  return INTF_MAX;
  }
+static enum dpu_wb dpu_encoder_get_wb(struct dpu_mdss_cfg *catalog,
+    enum dpu_intf_type type, u32 controller_id)
+{
+    int i = 0;
+
+    if (type != INTF_WB)
+    goto end;
+
+    for (i = 0; i < catalog->wb_count; i++) {
+    if (catalog->wb[i].id == controller_id)
+    return catalog->wb[i].id;
+    }
+
+end:
+    return WB_MAX;
+}
+
  static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
  struct dpu_encoder_phys *phy_enc)
  {
@@ -1887,16 +1915,32 @@ void dpu_encoder_helper_phys_cleanup(struct 
dpu_encoder_phys *phys_enc)

  dpu_encoder_helper_reset_mixers(phys_enc);
-    for (i = 0; i < dpu_enc->num_phys_encs; i++) {
-    if (dpu_enc->phys_encs[i] && 
phys_enc->hw_intf->ops.bind_pingpong_blk)

-    phys_enc->hw_intf->ops.bind_pingpong_blk(
-    dpu_enc->phys_encs[i]->hw_intf, false,
-    dpu_enc->phys_encs[i]->hw_pp->idx);
-
-    /* mark INTF flush as pending */
-    if (phys_enc->hw_ctl->ops.update_pending_flush_intf)
- phys_enc->hw_ctl->ops.update_pending_flush_intf(phys_enc->hw_ctl,
-    dpu_enc->phys_encs[i]->hw_intf->idx);
+    /*
+ * TODO: move the once-only operation like CTL flush/trigger
+ * into dpu_encoder_virt_disable() and all operations which need
+ * to be done per phys encoder into the phys_disable() op.
+ */
+    if (phys_enc->hw_wb) {
+    /* disable the PP block */
+    if (phys_enc->hw_wb->ops.bind_pingpong_blk)
+    phys_enc->hw_wb->ops.bind_pingpong_blk(phys_enc->hw_wb, 
false,

+    phys_enc->hw_pp->idx);
+
+    /* mark WB flush as pending */
+    if (phys_enc->hw_ctl->ops.update_pending_flush_wb)
+    phys_enc->hw_ctl->ops.update_pending_flush_wb(ctl, 
phys_enc->hw_wb->idx);

+    } else {
+    for (i = 0; i < dpu_enc->num_phys_encs; i++) {
+    if (dpu_enc->phys_encs[i] && 
phys_enc->hw_intf->ops.bind_pingpong_blk)

+    phys_enc->hw_intf->ops.bind_pingpong_blk(
+    dpu_enc->phys_encs[i]->hw_intf, false,
+    dpu_enc->phys_encs[i]->hw_pp->idx);
+
+    /* mark INTF flush as pending */
+    if (phys_enc->hw_ctl->ops.update_pending_flush_intf)
+ phys_enc->hw_ctl->ops.update_pending_flush_intf(phys_enc->hw_ctl,
+    dpu_enc->phys_encs[i]->hw_intf->idx);
+    }
  }
  /* reset the merge 3D HW block */
@@ -2112,6 +2156,9 @@ static int dpu_encoder_setup_display(struct 
dpu_encoder_virt *dpu_enc,

  case DRM_MODE_ENCODER_TMDS:
  intf_type = INTF_DP;
  break;
+    case DRM_MODE_ENCODER_VIRTUAL:
+

Re: [Freedreno] [PATCH v5 10/19] drm/msm/dpu: make changes to dpu_encoder to support virtual encoder

2022-04-25 Thread Abhinav Kumar


Hi Dmitry

On 4/25/2022 5:21 PM, Dmitry Baryshkov wrote:

On 25/04/2022 03:32, Abhinav Kumar wrote:

Make changes to dpu_encoder to support virtual encoder needed
to support writeback for dpu.

changes in v4:
- squash dpu_encoder pieces from [1]

[1] https://patchwork.freedesktop.org/patch/483099/?series=102964=2

Signed-off-by: Abhinav Kumar 
---
  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c  | 94 
+++-

  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h |  7 ++
  2 files changed, 83 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c

index 25c7eda..d1e92d89 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1013,9 +1013,18 @@ static void 
dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,

  if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX)
  phys->hw_intf = dpu_rm_get_intf(_kms->rm, 
phys->intf_idx);

-    if (!phys->hw_intf) {
+    if (phys->wb_idx >= WB_0 && phys->wb_idx < WB_MAX)
+    phys->hw_wb = dpu_rm_get_wb(_kms->rm, phys->wb_idx);
+
+    if (!phys->hw_intf && !phys->hw_wb) {
  DPU_ERROR_ENC(dpu_enc,
-  "no intf block assigned at idx: %d\n", i);
+  "no intf or wb block assigned at idx: %d\n", i);
+    return;
+    }
+
+    if (phys->hw_intf && phys->hw_wb) {
+    DPU_ERROR_ENC(dpu_enc,
+    "invalid phys both intf and wb block at idx: 
%d\n", i);

  return;
  }
@@ -1163,16 +1172,35 @@ static enum dpu_intf 
dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog,

  {
  int i = 0;
-    for (i = 0; i < catalog->intf_count; i++) {
-    if (catalog->intf[i].type == type
-    && catalog->intf[i].controller_id == controller_id) {
-    return catalog->intf[i].id;
+    if (type != INTF_WB) {
+    for (i = 0; i < catalog->intf_count; i++) {
+    if (catalog->intf[i].type == type
+    && catalog->intf[i].controller_id == controller_id) {
+    return catalog->intf[i].id;
+    }
  }
  }
  return INTF_MAX;
  }
+static enum dpu_wb dpu_encoder_get_wb(struct dpu_mdss_cfg *catalog,
+    enum dpu_intf_type type, u32 controller_id)
+{
+    int i = 0;
+
+    if (type != INTF_WB)
+    goto end;
+
+    for (i = 0; i < catalog->wb_count; i++) {
+    if (catalog->wb[i].id == controller_id)
+    return catalog->wb[i].id;
+    }
+
+end:
+    return WB_MAX;
+}
+
  static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
  struct dpu_encoder_phys *phy_enc)
  {
@@ -1887,16 +1915,32 @@ void dpu_encoder_helper_phys_cleanup(struct 
dpu_encoder_phys *phys_enc)

  dpu_encoder_helper_reset_mixers(phys_enc);
-    for (i = 0; i < dpu_enc->num_phys_encs; i++) {
-    if (dpu_enc->phys_encs[i] && 
phys_enc->hw_intf->ops.bind_pingpong_blk)

-    phys_enc->hw_intf->ops.bind_pingpong_blk(
-    dpu_enc->phys_encs[i]->hw_intf, false,
-    dpu_enc->phys_encs[i]->hw_pp->idx);
-
-    /* mark INTF flush as pending */
-    if (phys_enc->hw_ctl->ops.update_pending_flush_intf)
-
phys_enc->hw_ctl->ops.update_pending_flush_intf(phys_enc->hw_ctl,

-    dpu_enc->phys_encs[i]->hw_intf->idx);
+    /*
+ * TODO: move the once-only operation like CTL flush/trigger
+ * into dpu_encoder_virt_disable() and all operations which need
+ * to be done per phys encoder into the phys_disable() op.
+ */
+    if (phys_enc->hw_wb) {
+    /* disable the PP block */
+    if (phys_enc->hw_wb->ops.bind_pingpong_blk)
+    phys_enc->hw_wb->ops.bind_pingpong_blk(phys_enc->hw_wb, 
false,

+    phys_enc->hw_pp->idx);
+
+    /* mark WB flush as pending */
+    if (phys_enc->hw_ctl->ops.update_pending_flush_wb)
+    phys_enc->hw_ctl->ops.update_pending_flush_wb(ctl, 
phys_enc->hw_wb->idx);

+    } else {
+    for (i = 0; i < dpu_enc->num_phys_encs; i++) {
+    if (dpu_enc->phys_encs[i] && 
phys_enc->hw_intf->ops.bind_pingpong_blk)

+    phys_enc->hw_intf->ops.bind_pingpong_blk(
+    dpu_enc->phys_encs[i]->hw_intf, false,
+    dpu_enc->phys_encs[i]->hw_pp->idx);
+
+    /* mark INTF flush as pending */
+    if (phys_enc->hw_ctl->ops.update_pending_flush_intf)
+
phys_enc->hw_ctl->ops.update_pending_flush_intf(phys_enc->hw_ctl,

+    dpu_enc->phys_encs[i]->hw_intf->idx);
+    }
  }
  /* reset the merge 3D HW block */
@@ -2112,6 +2156,9 @@ static int dpu_encoder_setup_display(struct 
dpu_encoder_virt *dpu_enc,

  case DRM_MODE_ENCODER_TMDS:
  intf_type = INTF_DP;
  break;
+    case DRM_MODE_ENCODER_VIRTUAL:
+    intf_type = INTF_WB;

Re: [Freedreno] [PATCH v5 15/19] drm/msm/dpu: initialize dpu encoder and connector for writeback

2022-04-25 Thread Abhinav Kumar


Hi Laurent

On 4/25/2022 5:32 PM, Laurent Pinchart wrote:

Hi Abhinav,

On Sun, Apr 24, 2022 at 05:32:06PM -0700, Abhinav Kumar wrote:

Initialize dpu encoder and connector for writeback if the
target supports it in the catalog.

changes in v2:
- start initialing the encoder for writeback since we
have migrated to using drm_writeback_connector_init_with_encoder()
- instead of checking for WB_2 inside _dpu_kms_initialize_writeback
call it only when its WB_2
- rebase on tip of msm-next and remove usage of priv->encoders

changes in v3:
- none

changes in v4:
- fix copyright years order

Signed-off-by: Abhinav Kumar 
Reviewed-by: Dmitry Baryshkov 
---
  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 27 +
  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 61 -
  2 files changed, 80 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 24870eb..2d79002 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -2102,7 +2102,7 @@ static void dpu_encoder_early_unregister(struct 
drm_encoder *encoder)
  }
  
  static int dpu_encoder_virt_add_phys_encs(

-   u32 display_caps,
+   struct msm_display_info *disp_info,
struct dpu_encoder_virt *dpu_enc,
struct dpu_enc_phys_init_params *params)
  {
@@ -2121,7 +2121,7 @@ static int dpu_encoder_virt_add_phys_encs(
return -EINVAL;
}
  
-	if (display_caps & MSM_DISPLAY_CAP_VID_MODE) {

+   if (disp_info->capabilities & MSM_DISPLAY_CAP_VID_MODE) {
enc = dpu_encoder_phys_vid_init(params);
  
  		if (IS_ERR_OR_NULL(enc)) {

@@ -2134,7 +2134,7 @@ static int dpu_encoder_virt_add_phys_encs(
++dpu_enc->num_phys_encs;
}
  
-	if (display_caps & MSM_DISPLAY_CAP_CMD_MODE) {

+   if (disp_info->capabilities & MSM_DISPLAY_CAP_CMD_MODE) {
enc = dpu_encoder_phys_cmd_init(params);
  
  		if (IS_ERR_OR_NULL(enc)) {

@@ -2147,6 +2147,19 @@ static int dpu_encoder_virt_add_phys_encs(
++dpu_enc->num_phys_encs;
}
  
+	if (disp_info->intf_type == DRM_MODE_ENCODER_VIRTUAL) {

+   enc = dpu_encoder_phys_wb_init(params);
+
+   if (IS_ERR_OR_NULL(enc)) {
+   DPU_ERROR_ENC(dpu_enc, "failed to init wb enc: %ld\n",
+   PTR_ERR(enc));
+   return enc == NULL ? -EINVAL : PTR_ERR(enc);
+   }
+
+   dpu_enc->phys_encs[dpu_enc->num_phys_encs] = enc;
+   ++dpu_enc->num_phys_encs;
+   }
+
if (params->split_role == ENC_ROLE_SLAVE)
dpu_enc->cur_slave = enc;
else
@@ -2248,9 +2261,8 @@ static int dpu_encoder_setup_display(struct 
dpu_encoder_virt *dpu_enc,
}
  
  		if (!ret) {

-   ret = 
dpu_encoder_virt_add_phys_encs(disp_info->capabilities,
-   
 dpu_enc,
-   
 _params);
+   ret = dpu_encoder_virt_add_phys_encs(disp_info,
+   dpu_enc, _params);
if (ret)
DPU_ERROR_ENC(dpu_enc, "failed to add phys 
encs\n");
}
@@ -2367,8 +2379,9 @@ struct drm_encoder *dpu_encoder_init(struct drm_device 
*dev,
if (!dpu_enc)
return ERR_PTR(-ENOMEM);
  
+

rc = drm_encoder_init(dev, _enc->base, _encoder_funcs,
-   drm_enc_mode, NULL);
+ drm_enc_mode, NULL);
if (rc) {
devm_kfree(dev->dev, dpu_enc);
return ERR_PTR(rc);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index c683cab..9a406e1 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -1,7 +1,9 @@
  // SPDX-License-Identifier: GPL-2.0-only
  /*
- * Copyright (c) 2014-2018, The Linux Foundation. All rights reserved.
   * Copyright (C) 2013 Red Hat
+ * Copyright (c) 2014-2018, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
+ *
   * Author: Rob Clark 
   */
  
@@ -15,6 +17,7 @@

  #include 
  #include 
  #include 
+#include 
  
  #include "msm_drv.h"

  #include "msm_mmu.h"
@@ -29,6 +32,7 @@
  #include "dpu_kms.h"
  #include "dpu_plane.h"
  #include "dpu_vbif.h"
+#include "dpu_writeback.h"
  
  #define CREATE_TRACE_POINTS

  #include "dpu_trace.h"
@@ -648,6 +652,45 @@ static int _dpu_kms_initialize_displayport(struct 
drm_device *dev,
return 0;
  }
  
+static int

Re: [Freedreno] [PATCH v5 15/19] drm/msm/dpu: initialize dpu encoder and connector for writeback

2022-04-25 Thread Laurent Pinchart

Hi Abhinav,

On Sun, Apr 24, 2022 at 05:32:06PM -0700, Abhinav Kumar wrote:
> Initialize dpu encoder and connector for writeback if the
> target supports it in the catalog.
> 
> changes in v2:
>   - start initialing the encoder for writeback since we
>   have migrated to using drm_writeback_connector_init_with_encoder()
>   - instead of checking for WB_2 inside _dpu_kms_initialize_writeback
>   call it only when its WB_2
>   - rebase on tip of msm-next and remove usage of priv->encoders
> 
> changes in v3:
>   - none
> 
> changes in v4:
>   - fix copyright years order
> 
> Signed-off-by: Abhinav Kumar 
> Reviewed-by: Dmitry Baryshkov 
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 27 +
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 61 
> -
>  2 files changed, 80 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 24870eb..2d79002 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -2102,7 +2102,7 @@ static void dpu_encoder_early_unregister(struct 
> drm_encoder *encoder)
>  }
>  
>  static int dpu_encoder_virt_add_phys_encs(
> - u32 display_caps,
> + struct msm_display_info *disp_info,
>   struct dpu_encoder_virt *dpu_enc,
>   struct dpu_enc_phys_init_params *params)
>  {
> @@ -2121,7 +2121,7 @@ static int dpu_encoder_virt_add_phys_encs(
>   return -EINVAL;
>   }
>  
> - if (display_caps & MSM_DISPLAY_CAP_VID_MODE) {
> + if (disp_info->capabilities & MSM_DISPLAY_CAP_VID_MODE) {
>   enc = dpu_encoder_phys_vid_init(params);
>  
>   if (IS_ERR_OR_NULL(enc)) {
> @@ -2134,7 +2134,7 @@ static int dpu_encoder_virt_add_phys_encs(
>   ++dpu_enc->num_phys_encs;
>   }
>  
> - if (display_caps & MSM_DISPLAY_CAP_CMD_MODE) {
> + if (disp_info->capabilities & MSM_DISPLAY_CAP_CMD_MODE) {
>   enc = dpu_encoder_phys_cmd_init(params);
>  
>   if (IS_ERR_OR_NULL(enc)) {
> @@ -2147,6 +2147,19 @@ static int dpu_encoder_virt_add_phys_encs(
>   ++dpu_enc->num_phys_encs;
>   }
>  
> + if (disp_info->intf_type == DRM_MODE_ENCODER_VIRTUAL) {
> + enc = dpu_encoder_phys_wb_init(params);
> +
> + if (IS_ERR_OR_NULL(enc)) {
> + DPU_ERROR_ENC(dpu_enc, "failed to init wb enc: %ld\n",
> + PTR_ERR(enc));
> + return enc == NULL ? -EINVAL : PTR_ERR(enc);
> + }
> +
> + dpu_enc->phys_encs[dpu_enc->num_phys_encs] = enc;
> + ++dpu_enc->num_phys_encs;
> + }
> +
>   if (params->split_role == ENC_ROLE_SLAVE)
>   dpu_enc->cur_slave = enc;
>   else
> @@ -2248,9 +2261,8 @@ static int dpu_encoder_setup_display(struct 
> dpu_encoder_virt *dpu_enc,
>   }
>  
>   if (!ret) {
> - ret = 
> dpu_encoder_virt_add_phys_encs(disp_info->capabilities,
> - 
>  dpu_enc,
> - 
>  _params);
> + ret = dpu_encoder_virt_add_phys_encs(disp_info,
> + dpu_enc, _params);
>   if (ret)
>   DPU_ERROR_ENC(dpu_enc, "failed to add phys 
> encs\n");
>   }
> @@ -2367,8 +2379,9 @@ struct drm_encoder *dpu_encoder_init(struct drm_device 
> *dev,
>   if (!dpu_enc)
>   return ERR_PTR(-ENOMEM);
>  
> +
>   rc = drm_encoder_init(dev, _enc->base, _encoder_funcs,
> - drm_enc_mode, NULL);
> +   drm_enc_mode, NULL);
>   if (rc) {
>   devm_kfree(dev->dev, dpu_enc);
>   return ERR_PTR(rc);
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index c683cab..9a406e1 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -1,7 +1,9 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /*
> - * Copyright (c) 2014-2018, The Linux Foundation. All rights reserved.
>   * Copyright (C) 2013 Red Hat
> + * Copyright (c) 2014-2018, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> + *
>   * Author: Rob Clark 
>   */
>  
> @@ -15,6 +17,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include "msm_drv.h"
>  #include "msm_mmu.h"
> @@ -29,6 +32,7 @@
>  #include "dpu_kms.h"
>  #include "dpu_plane.h"
>  #include "dpu_vbif.h"
> +#include "dpu_writeback.h"
>  
>  #define CREATE_TRACE_POINTS
>  #include "dpu_trace.h"
> @@

Re: [Freedreno] [PATCH] drm/msm/dp: move add fail safe mode to dp_connector_get_mode()

2022-04-25 Thread Doug Anderson

Hi,

On Sat, Apr 23, 2022 at 8:34 AM Abhinav Kumar  wrote:
>
> On 4/22/2022 11:25 PM, Dmitry Baryshkov wrote:
> > On Sat, 23 Apr 2022 at 03:12, Abhinav Kumar  
> > wrote:
> >>
> >>
> >>
> >> On 4/22/2022 5:07 PM, Dmitry Baryshkov wrote:
> >>> On 23/04/2022 02:45, Kuogee Hsieh wrote:
>  Current DP driver implementation has adding safe mode done at
>  dp_hpd_plug_handle() which is expected to be executed under event
>  thread context.
> 
>  However there is possible circular locking happen (see blow stack trace)
>  after edp driver call dp_hpd_plug_handle() from dp_bridge_enable() which
>  is executed under drm_thread context.
> 
>  To break this circular locking, this patch have safe mode added at
>  dp_connector_get_mode() which is executed under drm thread context.
>  Therefore no lock acquired required for >mode_config.mutex while
>  adding fail safe mode since it has been hold by drm thread already.
> 
>  ==
> WARNING: possible circular locking dependency detected
> 5.15.35-lockdep #6 Tainted: GW
> --
> frecon/429 is trying to acquire lock:
> ff808dc3c4e8 (>mode_config.mutex){+.+.}-{3:3}, at:
>  dp_panel_add_fail_safe_mode+0x4c/0xa0
> 
> but task is already holding lock:
> ff808dc441e0 (>commit_lock[i]){+.+.}-{3:3}, at:
>  lock_crtcs+0xb4/0x124
> 
> which lock already depends on the new lock.
> 
> the existing dependency chain (in reverse order) is:
> 
> -> #3 (>commit_lock[i]){+.+.}-{3:3}:
>    __mutex_lock_common+0x174/0x1a64
>    mutex_lock_nested+0x98/0xac
>    lock_crtcs+0xb4/0x124
>    msm_atomic_commit_tail+0x330/0x748
>    commit_tail+0x19c/0x278
>    drm_atomic_helper_commit+0x1dc/0x1f0
>    drm_atomic_commit+0xc0/0xd8
>    drm_atomic_helper_set_config+0xb4/0x134
>    drm_mode_setcrtc+0x688/0x1248
>    drm_ioctl_kernel+0x1e4/0x338
>    drm_ioctl+0x3a4/0x684
>    __arm64_sys_ioctl+0x118/0x154
>    invoke_syscall+0x78/0x224
>    el0_svc_common+0x178/0x200
>    do_el0_svc+0x94/0x13c
>    el0_svc+0x5c/0xec
>    el0t_64_sync_handler+0x78/0x108
>    el0t_64_sync+0x1a4/0x1a8
> 
> -> #2 (crtc_ww_class_mutex){+.+.}-{3:3}:
>    __mutex_lock_common+0x174/0x1a64
>    ww_mutex_lock+0xb8/0x278
>    modeset_lock+0x304/0x4ac
>    drm_modeset_lock+0x4c/0x7c
>    drmm_mode_config_init+0x4a8/0xc50
>    msm_drm_init+0x274/0xac0
>    msm_drm_bind+0x20/0x2c
>    try_to_bring_up_master+0x3dc/0x470
>    __component_add+0x18c/0x3c0
>    component_add+0x1c/0x28
>    dp_display_probe+0x954/0xa98
>    platform_probe+0x124/0x15c
>    really_probe+0x1b0/0x5f8
>    __driver_probe_device+0x174/0x20c
>    driver_probe_device+0x70/0x134
>    __device_attach_driver+0x130/0x1d0
>    bus_for_each_drv+0xfc/0x14c
>    __device_attach+0x1bc/0x2bc
>    device_initial_probe+0x1c/0x28
>    bus_probe_device+0x94/0x178
>    deferred_probe_work_func+0x1a4/0x1f0
>    process_one_work+0x5d4/0x9dc
>    worker_thread+0x898/0xccc
>    kthread+0x2d4/0x3d4
>    ret_from_fork+0x10/0x20
> 
> -> #1 (crtc_ww_class_acquire){+.+.}-{0:0}:
>    ww_acquire_init+0x1c4/0x2c8
>    drm_modeset_acquire_init+0x44/0xc8
>    drm_helper_probe_single_connector_modes+0xb0/0x12dc
>    drm_mode_getconnector+0x5dc/0xfe8
>    drm_ioctl_kernel+0x1e4/0x338
>    drm_ioctl+0x3a4/0x684
>    __arm64_sys_ioctl+0x118/0x154
>    invoke_syscall+0x78/0x224
>    el0_svc_common+0x178/0x200
>    do_el0_svc+0x94/0x13c
>    el0_svc+0x5c/0xec
>    el0t_64_sync_handler+0x78/0x108
>    el0t_64_sync+0x1a4/0x1a8
> 
> -> #0 (>mode_config.mutex){+.+.}-{3:3}:
>    __lock_acquire+0x2650/0x672c
>    lock_acquire+0x1b4/0x4ac
>    __mutex_lock_common+0x174/0x1a64
>    mutex_lock_nested+0x98/0xac
>    dp_panel_add_fail_safe_mode+0x4c/0xa0
>    dp_hpd_plug_handle+0x1f0/0x280
>    dp_bridge_enable+0x94/0x2b8
>    drm_atomic_bridge_chain_enable+0x11c/0x168
>    drm_atomic_helper_commit_modeset_enables+0x500/0x740
>    msm_atomic_commit_tail+0x3e4/0x748
>    commit_tail+0x19c/0x278
>

Re: [Freedreno] [PATCH v5 10/19] drm/msm/dpu: make changes to dpu_encoder to support virtual encoder

2022-04-25 Thread Dmitry Baryshkov


On 25/04/2022 03:32, Abhinav Kumar wrote:

Make changes to dpu_encoder to support virtual encoder needed
to support writeback for dpu.

changes in v4:
- squash dpu_encoder pieces from [1]

[1] https://patchwork.freedesktop.org/patch/483099/?series=102964=2

Signed-off-by: Abhinav Kumar 
---
  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c  | 94 +++-
  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h |  7 ++
  2 files changed, 83 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 25c7eda..d1e92d89 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1013,9 +1013,18 @@ static void dpu_encoder_virt_atomic_mode_set(struct 
drm_encoder *drm_enc,
if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX)
phys->hw_intf = dpu_rm_get_intf(_kms->rm, 
phys->intf_idx);
  
-		if (!phys->hw_intf) {

+   if (phys->wb_idx >= WB_0 && phys->wb_idx < WB_MAX)
+   phys->hw_wb = dpu_rm_get_wb(_kms->rm, phys->wb_idx);
+
+   if (!phys->hw_intf && !phys->hw_wb) {
DPU_ERROR_ENC(dpu_enc,
- "no intf block assigned at idx: %d\n", i);
+ "no intf or wb block assigned at idx: 
%d\n", i);
+   return;
+   }
+
+   if (phys->hw_intf && phys->hw_wb) {
+   DPU_ERROR_ENC(dpu_enc,
+   "invalid phys both intf and wb block at idx: 
%d\n", i);
return;
}
  
@@ -1163,16 +1172,35 @@ static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog,

  {
int i = 0;
  
-	for (i = 0; i < catalog->intf_count; i++) {

-   if (catalog->intf[i].type == type
-   && catalog->intf[i].controller_id == controller_id) {
-   return catalog->intf[i].id;
+   if (type != INTF_WB) {
+   for (i = 0; i < catalog->intf_count; i++) {
+   if (catalog->intf[i].type == type
+   && catalog->intf[i].controller_id == 
controller_id) {
+   return catalog->intf[i].id;
+   }
}
}
  
  	return INTF_MAX;

  }
  
+static enum dpu_wb dpu_encoder_get_wb(struct dpu_mdss_cfg *catalog,

+   enum dpu_intf_type type, u32 controller_id)
+{
+   int i = 0;
+
+   if (type != INTF_WB)
+   goto end;
+
+   for (i = 0; i < catalog->wb_count; i++) {
+   if (catalog->wb[i].id == controller_id)
+   return catalog->wb[i].id;
+   }
+
+end:
+   return WB_MAX;
+}
+
  static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
struct dpu_encoder_phys *phy_enc)
  {
@@ -1887,16 +1915,32 @@ void dpu_encoder_helper_phys_cleanup(struct 
dpu_encoder_phys *phys_enc)
  
  	dpu_encoder_helper_reset_mixers(phys_enc);
  
-	for (i = 0; i < dpu_enc->num_phys_encs; i++) {

-   if (dpu_enc->phys_encs[i] && 
phys_enc->hw_intf->ops.bind_pingpong_blk)
-   phys_enc->hw_intf->ops.bind_pingpong_blk(
-   dpu_enc->phys_encs[i]->hw_intf, false,
-   dpu_enc->phys_encs[i]->hw_pp->idx);
-
-   /* mark INTF flush as pending */
-   if (phys_enc->hw_ctl->ops.update_pending_flush_intf)
-   
phys_enc->hw_ctl->ops.update_pending_flush_intf(phys_enc->hw_ctl,
-   dpu_enc->phys_encs[i]->hw_intf->idx);
+   /*
+* TODO: move the once-only operation like CTL flush/trigger
+* into dpu_encoder_virt_disable() and all operations which need
+* to be done per phys encoder into the phys_disable() op.
+*/
+   if (phys_enc->hw_wb) {
+   /* disable the PP block */
+   if (phys_enc->hw_wb->ops.bind_pingpong_blk)
+   phys_enc->hw_wb->ops.bind_pingpong_blk(phys_enc->hw_wb, 
false,
+   phys_enc->hw_pp->idx);
+
+   /* mark WB flush as pending */
+   if (phys_enc->hw_ctl->ops.update_pending_flush_wb)
+   phys_enc->hw_ctl->ops.update_pending_flush_wb(ctl, 
phys_enc->hw_wb->idx);
+   } else {
+   for (i = 0; i < dpu_enc->num_phys_encs; i++) {
+   if (dpu_enc->phys_encs[i] && 
phys_enc->hw_intf->ops.bind_pingpong_blk)
+   phys_enc->hw_intf->ops.bind_pingpong_blk(
+   dpu_enc->phys_encs[i]->hw_intf, 
false,
+   
dpu_enc->phys_encs[i]->hw_pp->idx);
+
+   /* mark INTF flush as

Re: [Freedreno] [PATCH v5 14/19] drm/msm/dpu: add the writeback connector layer

2022-04-25 Thread Dmitry Baryshkov


On 25/04/2022 03:32, Abhinav Kumar wrote:

Introduce the dpu_writeback module which serves as the
interface between dpu operations and the drm_writeback.

This module manages the connector related operations for
dpu writeback.

changes in v2:
- start using drm_writeback_connector_init_with_encoder()
- drop unnecessary arguments from dpu_writeback_init()
- rebase on msm-next tip and remove usage of priv->connectors

changes in v3:
- none

changes in v4:
- none

changes in v5:
- store the drm_enc in the dpu_wb_conn to be used while
  using dpu_encoder APIs

Signed-off-by: Abhinav Kumar 


Reviewed-by: Dmitry Baryshkov 


---
  drivers/gpu/drm/msm/Makefile  |  1 +
  drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c | 76 +++
  drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h | 31 +++
  3 files changed, 108 insertions(+)
  create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c
  create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h

diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
index 0387f22..66395ee 100644
--- a/drivers/gpu/drm/msm/Makefile
+++ b/drivers/gpu/drm/msm/Makefile
@@ -80,6 +80,7 @@ msm-$(CONFIG_DRM_MSM_DPU) += \
disp/dpu1/dpu_plane.o \
disp/dpu1/dpu_rm.o \
disp/dpu1/dpu_vbif.o \
+   disp/dpu1/dpu_writeback.o
  
  msm-$(CONFIG_DRM_MSM_MDSS) += \

msm_mdss.o \
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c
new file mode 100644
index 000..7620ffe
--- /dev/null
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c
@@ -0,0 +1,76 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include "dpu_writeback.h"
+
+static int dpu_wb_conn_get_modes(struct drm_connector *connector)
+{
+   struct drm_device *dev = connector->dev;
+
+   return drm_add_modes_noedid(connector, dev->mode_config.max_width,
+   dev->mode_config.max_height);
+}
+
+static const struct drm_connector_funcs dpu_wb_conn_funcs = {
+   .reset = drm_atomic_helper_connector_reset,
+   .fill_modes = drm_helper_probe_single_connector_modes,
+   .destroy = drm_connector_cleanup,
+   .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+   .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+static int dpu_wb_conn_prepare_job(struct drm_writeback_connector *connector,
+   struct drm_writeback_job *job)
+{
+
+   struct dpu_wb_connector *dpu_wb_conn = to_dpu_wb_conn(connector);
+
+   if (!job->fb)
+   return 0;
+
+   dpu_encoder_prepare_wb_job(dpu_wb_conn->wb_enc, job);
+
+   return 0;
+}
+
+static void dpu_wb_conn_cleanup_job(struct drm_writeback_connector *connector,
+   struct drm_writeback_job *job)
+{
+   struct dpu_wb_connector *dpu_wb_conn = to_dpu_wb_conn(connector);
+
+   if (!job->fb)
+   return;
+
+   dpu_encoder_cleanup_wb_job(dpu_wb_conn->wb_enc, job);
+}
+
+static const struct drm_connector_helper_funcs dpu_wb_conn_helper_funcs = {
+   .get_modes = dpu_wb_conn_get_modes,
+   .prepare_writeback_job = dpu_wb_conn_prepare_job,
+   .cleanup_writeback_job = dpu_wb_conn_cleanup_job,
+};
+
+int dpu_writeback_init(struct drm_device *dev, struct drm_encoder *enc,
+   const u32 *format_list, u32 num_formats)
+{
+   struct dpu_wb_connector *dpu_wb_conn;
+   int rc = 0;
+
+   dpu_wb_conn = devm_kzalloc(dev->dev, sizeof(*dpu_wb_conn), GFP_KERNEL);
+
+   drm_connector_helper_add(_wb_conn->base.base, 
_wb_conn_helper_funcs);
+
+   /* DPU initializes the encoder and sets it up completely for writeback
+* cases and hence should use the new API 
drm_writeback_connector_init_with_encoder
+* to initialize the writeback connector
+*/
+   rc = drm_writeback_connector_init_with_encoder(dev, _wb_conn->base, 
enc,
+   _wb_conn_funcs, format_list, num_formats);
+
+   if (!rc)
+   dpu_wb_conn->wb_enc = enc;
+
+   return rc;
+}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h
new file mode 100644
index 000..5a75ea9
--- /dev/null
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#ifndef _DPU_WRITEBACK_H
+#define _DPU_WRITEBACK_H
+
+#include 
+#include 
+#include 
+#include 
+
+#include "msm_drv.h"
+#include "dpu_kms.h"
+#include "dpu_encoder_phys.h"
+
+struct dpu_wb_connector {
+   struct drm_writeback_connector base;
+   struct drm_encoder *wb_enc;
+};
+
+static inline struct dpu_wb_connector *to_dpu_wb_conn(struct 
drm_writeback_connector *conn)
+{
+

Re: [Freedreno] [PATCH v4 11/20] drm/msm/dpu: make changes to dpu_encoder to support virtual encoder

2022-04-25 Thread Dmitry Baryshkov


On 23/04/2022 02:06, Abhinav Kumar wrote:

Make changes to dpu_encoder to support virtual encoder needed
to support writeback for dpu.

changes in v4:
- squash dpu_encoder pieces from [1]

[1] https://patchwork.freedesktop.org/patch/483099/?series=102964=2

Signed-off-by: Abhinav Kumar 
---
  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c  | 94 +++-
  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h |  7 ++
  2 files changed, 83 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 25c7eda..d1e92d89 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1013,9 +1013,18 @@ static void dpu_encoder_virt_atomic_mode_set(struct 
drm_encoder *drm_enc,
if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX)
phys->hw_intf = dpu_rm_get_intf(_kms->rm, 
phys->intf_idx);
  
-		if (!phys->hw_intf) {

+   if (phys->wb_idx >= WB_0 && phys->wb_idx < WB_MAX)
+   phys->hw_wb = dpu_rm_get_wb(_kms->rm, phys->wb_idx);
+
+   if (!phys->hw_intf && !phys->hw_wb) {
DPU_ERROR_ENC(dpu_enc,
- "no intf block assigned at idx: %d\n", i);
+ "no intf or wb block assigned at idx: 
%d\n", i);
+   return;
+   }
+
+   if (phys->hw_intf && phys->hw_wb) {
+   DPU_ERROR_ENC(dpu_enc,
+   "invalid phys both intf and wb block at idx: 
%d\n", i);
return;
}
  
@@ -1163,16 +1172,35 @@ static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog,

  {
int i = 0;
  
-	for (i = 0; i < catalog->intf_count; i++) {

-   if (catalog->intf[i].type == type
-   && catalog->intf[i].controller_id == controller_id) {
-   return catalog->intf[i].id;
+   if (type != INTF_WB) {
+   for (i = 0; i < catalog->intf_count; i++) {
+   if (catalog->intf[i].type == type
+   && catalog->intf[i].controller_id == 
controller_id) {
+   return catalog->intf[i].id;
+   }
}
}
  
  	return INTF_MAX;

  }
  
+static enum dpu_wb dpu_encoder_get_wb(struct dpu_mdss_cfg *catalog,

+   enum dpu_intf_type type, u32 controller_id)
+{
+   int i = 0;
+
+   if (type != INTF_WB)
+   goto end;
+
+   for (i = 0; i < catalog->wb_count; i++) {
+   if (catalog->wb[i].id == controller_id)
+   return catalog->wb[i].id;
+   }
+
+end:
+   return WB_MAX;
+}
+
  static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
struct dpu_encoder_phys *phy_enc)
  {
@@ -1887,16 +1915,32 @@ void dpu_encoder_helper_phys_cleanup(struct 
dpu_encoder_phys *phys_enc)
  
  	dpu_encoder_helper_reset_mixers(phys_enc);
  
-	for (i = 0; i < dpu_enc->num_phys_encs; i++) {

-   if (dpu_enc->phys_encs[i] && 
phys_enc->hw_intf->ops.bind_pingpong_blk)
-   phys_enc->hw_intf->ops.bind_pingpong_blk(
-   dpu_enc->phys_encs[i]->hw_intf, false,
-   dpu_enc->phys_encs[i]->hw_pp->idx);
-
-   /* mark INTF flush as pending */
-   if (phys_enc->hw_ctl->ops.update_pending_flush_intf)
-   
phys_enc->hw_ctl->ops.update_pending_flush_intf(phys_enc->hw_ctl,
-   dpu_enc->phys_encs[i]->hw_intf->idx);
+   /*
+* TODO: move the once-only operation like CTL flush/trigger
+* into dpu_encoder_virt_disable() and all operations which need
+* to be done per phys encoder into the phys_disable() op.
+*/
+   if (phys_enc->hw_wb) {
+   /* disable the PP block */
+   if (phys_enc->hw_wb->ops.bind_pingpong_blk)
+   phys_enc->hw_wb->ops.bind_pingpong_blk(phys_enc->hw_wb, 
false,
+   phys_enc->hw_pp->idx);
+
+   /* mark WB flush as pending */
+   if (phys_enc->hw_ctl->ops.update_pending_flush_wb)
+   phys_enc->hw_ctl->ops.update_pending_flush_wb(ctl, 
phys_enc->hw_wb->idx);
+   } else {
+   for (i = 0; i < dpu_enc->num_phys_encs; i++) {
+   if (dpu_enc->phys_encs[i] && 
phys_enc->hw_intf->ops.bind_pingpong_blk)
+   phys_enc->hw_intf->ops.bind_pingpong_blk(
+   dpu_enc->phys_encs[i]->hw_intf, 
false,
+   
dpu_enc->phys_encs[i]->hw_pp->idx);
+
+   /* mark INTF flush as

[Freedreno] [PATCH v2 2/2] drm/msm/dp: do not stop transmitting phy test pattern during DP phy compliance test

2022-04-25 Thread Kuogee Hsieh

At normal operation, transmit phy test pattern has to be terminated before
DP controller switch to video ready state. However during phy compliance
testing, transmit phy test pattern should not be terminated until end of
compliance test which usually indicated by unplugged interrupt.

Only stop sending the train pattern in dp_ctrl_on_stream() if we're not
doing compliance testing. We also no longer reset 'p_level' and
'v_level' within dp_ctrl_on_link() due to both 'p_level' and 'v_level'
are acquired from link status at previous dpcd read and we like to use
those level to start link training.

Changes in v2:
-- add more details commit text
-- correct Fixes

Fixes: 2e0adc765d88 ("drm/msm/dp: do not end dp link training until video is 
ready")
Signed-off-by: Kuogee Hsieh 
---
 drivers/gpu/drm/msm/dp/dp_ctrl.c | 15 +++
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index 193cc1a..f99e173 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1699,8 +1699,6 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
ctrl->link->link_params.rate,
ctrl->link->link_params.num_lanes, ctrl->dp_ctrl.pixel_rate);
 
-   ctrl->link->phy_params.p_level = 0;
-   ctrl->link->phy_params.v_level = 0;
 
rc = dp_ctrl_enable_mainlink_clocks(ctrl);
if (rc)
@@ -1822,12 +1820,6 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
}
}
 
-   if (!dp_ctrl_channel_eq_ok(ctrl))
-   dp_ctrl_link_retrain(ctrl);
-
-   /* stop txing train pattern to end link training */
-   dp_ctrl_clear_training_pattern(ctrl);
-
ret = dp_ctrl_enable_stream_clocks(ctrl);
if (ret) {
DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
@@ -1839,6 +1831,13 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
return 0;
}
 
+   if (!dp_ctrl_channel_eq_ok(ctrl)) {
+   dp_ctrl_link_retrain(ctrl);
+   }
+
+   /* stop txing train pattern to end link training */
+   dp_ctrl_clear_training_pattern(ctrl);
+
/*
 * Set up transfer unit values and set controller state to send
 * video.
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[Freedreno] [PATCH v2 1/2] drm/msm/dp: reset DP controller before transmit phy test pattern

2022-04-25 Thread Kuogee Hsieh

DP controller state can not switch from video ready state to
transmit phy pattern state at run time. DP mainlink has to be
teared down followed by reset controller to default state to have
DP controller switch to transmit phy test pattern state and start
generate specified phy test pattern to sinker once main link setup
again.

Changes in v2:
-- correct Fixes's commit id

Fixes: 52352fe2f866 ("drm/msm/dp: use dp_ctrl_off_link_stream during PHY 
compliance test run")
Signed-off-by: Kuogee Hsieh 
Reviewed-by: Stephen Boyd 
---
 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 5356856..193cc1a 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1532,7 +1532,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_link_stream(>dp_ctrl);
+   ret = dp_ctrl_off(>dp_ctrl);
if (ret) {
DRM_ERROR("failed to disable DP controller\n");
return ret;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[Freedreno] [PATCH v2 0/2] fix DP phy compliance test

2022-04-25 Thread Kuogee Hsieh

Current DP phy compliance test failed due to test pattern generation was
terminated premature.

Kuogee Hsieh (2):
  drm/msm/dp: reset DP controller before transmit phy test pattern
  drm/msm/dp: do not stop transmitting phy test pattern during DP phy
compliance test

 drivers/gpu/drm/msm/dp/dp_ctrl.c | 17 -
 1 file changed, 8 insertions(+), 9 deletions(-)

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

Re: [Freedreno] [PATCH v1 2/2] drm/msm/dp: do not stop transmitting phy test pattern during DP phy compliance test

2022-04-25 Thread Stephen Boyd

Quoting Kuogee Hsieh (2022-04-25 15:11:03)
> At normal operation, transmit phy test pattern has to be terminated before
> DP controller switch to video ready state. However during phy compliance
> testing, transmit phy test pattern should not be terminated until end of
> compliance test which usually indicated by unplugged interrupt.

And what does the patch do to fix it? We need more details in the commit
text. I think it should continue with:

Only stop sending the train pattern in dp_ctrl_on_stream() if we're not
doing compliance testing. We also no longer reset 'p_level' and
'v_level' because XYZ and it's OK/better to retrain the link after
enabling the stream clks because XYZ.

>
> Fixes: 64e190e720a7 ("drm/msm/dp: DisplayPort PHY compliance tests fixup")

Should be

Fixes: 6625e2637d93 ("drm/msm/dp: DisplayPort PHY compliance tests fixup")

> Signed-off-by: Kuogee Hsieh 
> ---
>  drivers/gpu/drm/msm/dp/dp_ctrl.c | 15 +++
>  1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c 
> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 193cc1a..f99e173 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -1699,8 +1699,6 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
> ctrl->link->link_params.rate,
> ctrl->link->link_params.num_lanes, ctrl->dp_ctrl.pixel_rate);
>
> -   ctrl->link->phy_params.p_level = 0;
> -   ctrl->link->phy_params.v_level = 0;

Why is this removed? Can you mention in the commit text why we don't
need to zero it out anymore?

>
> rc = dp_ctrl_enable_mainlink_clocks(ctrl);
> if (rc)
> @@ -1822,12 +1820,6 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
> }
> }
>
> -   if (!dp_ctrl_channel_eq_ok(ctrl))
> -   dp_ctrl_link_retrain(ctrl);
> -
> -   /* stop txing train pattern to end link training */
> -   dp_ctrl_clear_training_pattern(ctrl);
> -
> ret = dp_ctrl_enable_stream_clocks(ctrl);
> if (ret) {
> DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
> @@ -1839,6 +1831,13 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
> return 0;
> }
>
> +   if (!dp_ctrl_channel_eq_ok(ctrl)) {
> +   dp_ctrl_link_retrain(ctrl);
> +   }

Braces not needed. Please remove.

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

Re: [Freedreno] [PATCH v1 1/2] drm/msm/dp: reset DP controller before transmit phy test pattern

2022-04-25 Thread Stephen Boyd

Quoting Kuogee Hsieh (2022-04-25 15:11:02)
> DP controller state can not switch from video ready state to
> transmit phy pattern state at run time. DP mainlink has to be
> teared down followed by reset controller to default state to have
> DP controller switch to transmit phy test pattern state and start
> generate specified phy test pattern to sinker once main link setup
> again.
>
> Fixes: ee35444be0c8 ("drm/msm/dp: use dp_ctrl_off_link_stream during PHY 
> compliance test run")

What commit is this? I think it's supposed to be

Fixes: 52352fe2f866 ("drm/msm/dp: use dp_ctrl_off_link_stream during
PHY compliance test run")

> Signed-off-by: Kuogee Hsieh 
> ---

Otherwise,

Reviewed-by: Stephen Boyd

Re: [Freedreno] [PATCH] drm/msm/dp: tear down main link at unplug handle immediately

2022-04-25 Thread Stephen Boyd

Quoting Kuogee Hsieh (2022-04-25 15:29:30)
>
> On 4/20/2022 3:38 PM, Stephen Boyd wrote:
> > Quoting Kuogee Hsieh (2022-04-14 14:03:43)
> >
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
> >> b/drivers/gpu/drm/msm/dp/dp_display.c
> >> index 01453db..f5bd8f5 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >> @@ -615,24 +598,21 @@ static int dp_hpd_unplug_handle(struct 
> >> dp_display_private *dp, u32 data)
> >>  if (dp->link->sink_count == 0) {
> >>  dp_display_host_phy_exit(dp);
> >>  }
> >> +   dp_display_notify_disconnect(>pdev->dev);
> >>  mutex_unlock(>event_mutex);
> >>  return 0;
> >> -   }
> >> -
> >> -   if (state == ST_DISCONNECT_PENDING) {
> >> +   } else if (state == ST_DISCONNECT_PENDING) {
> >>  mutex_unlock(>event_mutex);
> >>  return 0;
> >> -   }
> >> -
> >> -   if (state == ST_CONNECT_PENDING) {
> >> -   /* wait until CONNECTED */
> >> -   dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 1); /* delay = 1 */
> >> +   } else if (state == ST_CONNECT_PENDING) {
> > I take it that ST_CONNECT_PENDING is sort of like "userspace hasn't
> > handled the uevent yet and modeset hasn't been called but the link is
> > setup and now we want to tear it down". The state name may want to be
> > changed to something else.
> yes, how about change to  ST_MAINLINK_READY?

Sure.

> >> @@ -1529,8 +1480,11 @@ int msm_dp_display_enable(struct msm_dp *dp, struct 
> >> drm_encoder *encoder)
> >>
> >>  mutex_lock(_display->event_mutex);
> >>
> >> -   /* stop sentinel checking */
> >> -   dp_del_event(dp_display, EV_CONNECT_PENDING_TIMEOUT);
> >> +   state = dp_display->hpd_state;
> >> +   if (state != ST_DISPLAY_OFF && state != ST_CONNECT_PENDING) {
> > Is this to guard against userspace doing an atomic commit when the
> > display isn't connected? Is that even possible?
>
> No, it used to guard follow scenario in timing order,
>
> 1) plugin had been handled and mainlink is ready,
>
> 2)  userspace hasn't handled the uevent yet and modeset hasn't been called
>
> 3) unplugged happen, mainlink be teared down
>
> 4) user space start to response to uevent  and try to enable display.
> (it too late since mainlink had been teared down)
>

Ok. Thanks for clarifying.

Re: [Freedreno] [PATCH v2] drm/msm/dp: add fail safe mode outside of event_mutex context

2022-04-25 Thread Dmitry Baryshkov


On 09/04/2022 00:04, Kuogee Hsieh wrote:

There is possible circular locking dependency detected on event_mutex
(see below logs). This is due to set fail safe mode is done at
dp_panel_read_sink_caps() within event_mutex scope. To break this
possible circular locking, this patch move setting fail safe mode
out of event_mutex scope.

[   23.958078] ==
[   23.964430] WARNING: possible circular locking dependency detected
[   23.970777] 5.17.0-rc2-lockdep-00088-g05241de1f69e #148 Not tainted
[   23.977219] --
[   23.983570] DrmThread/1574 is trying to acquire lock:
[   23.988763] ff808423aab0 (>event_mutex){+.+.}-{3:3}, at: 
msm_dp_displ   
  ay_enable+0x58/0x164
[   23.997895]
[   23.997895] but task is already holding lock:
[   24.003895] ff808420b280 (>commit_lock[i]/1){+.+.}-{3:3}, at: 
lock_c 
rtcs+0x80/0x8c
[   24.012495]
[   24.012495] which lock already depends on the new lock.
[   24.012495]
[   24.020886]
[   24.020886] the existing dependency chain (in reverse order) is:
[   24.028570]
[   24.028570] -> #5 (>commit_lock[i]/1){+.+.}-{3:3}:
[   24.035472]__mutex_lock+0xc8/0x384
[   24.039695]mutex_lock_nested+0x54/0x74
[   24.044272]lock_crtcs+0x80/0x8c
[   24.048222]msm_atomic_commit_tail+0x1e8/0x3d0
[   24.053413]commit_tail+0x7c/0xfc
[   24.057452]drm_atomic_helper_commit+0x158/0x15c
[   24.062826]drm_atomic_commit+0x60/0x74
[   24.067403]drm_mode_atomic_ioctl+0x6b0/0x908
[   24.072508]drm_ioctl_kernel+0xe8/0x168
[   24.077086]drm_ioctl+0x320/0x370
[   24.081123]drm_compat_ioctl+0x40/0xdc
[   24.085602]__arm64_compat_sys_ioctl+0xe0/0x150
[   24.090895]invoke_syscall+0x80/0x114
[   24.095294]el0_svc_common.constprop.3+0xc4/0xf8
[   24.100668]do_el0_svc_compat+0x2c/0x54
[   24.105242]el0_svc_compat+0x4c/0xe4
[   24.109548]el0t_32_sync_handler+0xc4/0xf4
[   24.114381]el0t_32_sync+0x178
[   24.118688]
[   24.118688] -> #4 (>commit_lock[i]){+.+.}-{3:3}:
[   24.125408]__mutex_lock+0xc8/0x384
[   24.129628]mutex_lock_nested+0x54/0x74
[   24.134204]lock_crtcs+0x80/0x8c
[   24.138155]msm_atomic_commit_tail+0x1e8/0x3d0
[   24.143345]commit_tail+0x7c/0xfc
[   24.147382]drm_atomic_helper_commit+0x158/0x15c
[   24.152755]drm_atomic_commit+0x60/0x74
[   24.157323]drm_atomic_helper_set_config+0x68/0x90
[   24.162869]drm_mode_setcrtc+0x394/0x648
[   24.167535]drm_ioctl_kernel+0xe8/0x168
[   24.172102]drm_ioctl+0x320/0x370
[   24.176135]drm_compat_ioctl+0x40/0xdc
[   24.180621]__arm64_compat_sys_ioctl+0xe0/0x150
[   24.185904]invoke_syscall+0x80/0x114
[   24.190302]el0_svc_common.constprop.3+0xc4/0xf8
[   24.195673]do_el0_svc_compat+0x2c/0x54
[   24.200241]el0_svc_compat+0x4c/0xe4
[   24.204544]el0t_32_sync_handler+0xc4/0xf4
[   24.209378]el0t_32_sync+0x174/0x178
[   24.213680] -> #3 (crtc_ww_class_mutex){+.+.}-{3:3}:
[   24.220308]__ww_mutex_lock.constprop.20+0xe8/0x878
[   24.225951]ww_mutex_lock+0x60/0xd0
[   24.230166]modeset_lock+0x190/0x19c
[   24.234467]drm_modeset_lock+0x34/0x54
[   24.238953]drmm_mode_config_init+0x550/0x764
[   24.244065]msm_drm_bind+0x170/0x59c
[   24.248374]try_to_bring_up_master+0x244/0x294
[   24.253572]__component_add+0xf4/0x14c
[   24.258057]component_add+0x2c/0x38
[   24.262273]dsi_dev_attach+0x2c/0x38
[   24.266575]dsi_host_attach+0xc4/0x120
[   24.271060]mipi_dsi_attach+0x34/0x48
[   24.275456]devm_mipi_dsi_attach+0x28/0x68
[   24.280298]ti_sn_bridge_probe+0x2b4/0x2dc
[   24.285137]auxiliary_bus_probe+0x78/0x90
[   24.289893]really_probe+0x1e4/0x3d8
[   24.294194]__driver_probe_device+0x14c/0x164
[   24.299298]driver_probe_device+0x54/0xf8
[   24.304043]__device_attach_driver+0xb4/0x118
[   24.309145]bus_for_each_drv+0xb0/0xd4
[   24.313628]__device_attach+0xcc/0x158
[   24.318112]device_initial_probe+0x24/0x30
[   24.322954]bus_probe_device+0x38/0x9c
[   24.327439]deferred_probe_work_func+0xd4/0xf0
[   24.332628]process_one_work+0x2f0/0x498
[   24.337289]process_scheduled_works+0x44/0x48
[   24.342391]worker_thread+0x1e4/0x26c
[   24.346788]kthread+0xe4/0xf4
[   24.350470]ret_from_fork+0x10/0x20
[   24.354683]
[   24.354683]
[   24.354683] -> #2 (crtc_ww_class_acquire){+.+.}-{0:0}:
[   24.361489]drm_modeset_acquire_init+0xe4/0x138
[   24.366777]drm_helper_probe_detect_ctx+0x44/0x114
[

Re: [Freedreno] [PATCH] drm/msm/dp: move add fail safe mode to dp_connector_get_mode()

2022-04-25 Thread Dmitry Baryshkov


On 23/04/2022 18:33, Abhinav Kumar wrote:



On 4/22/2022 11:25 PM, Dmitry Baryshkov wrote:
On Sat, 23 Apr 2022 at 03:12, Abhinav Kumar 
 wrote:




On 4/22/2022 5:07 PM, Dmitry Baryshkov wrote:

On 23/04/2022 02:45, Kuogee Hsieh wrote:

Current DP driver implementation has adding safe mode done at
dp_hpd_plug_handle() which is expected to be executed under event
thread context.

However there is possible circular locking happen (see blow stack 
trace)
after edp driver call dp_hpd_plug_handle() from dp_bridge_enable() 
which

is executed under drm_thread context.

To break this circular locking, this patch have safe mode added at
dp_connector_get_mode() which is executed under drm thread context.
Therefore no lock acquired required for >mode_config.mutex while
adding fail safe mode since it has been hold by drm thread already.

==
   WARNING: possible circular locking dependency detected
   5.15.35-lockdep #6 Tainted: G    W
   --
   frecon/429 is trying to acquire lock:
   ff808dc3c4e8 (>mode_config.mutex){+.+.}-{3:3}, at:
dp_panel_add_fail_safe_mode+0x4c/0xa0

   but task is already holding lock:
   ff808dc441e0 (>commit_lock[i]){+.+.}-{3:3}, at:
lock_crtcs+0xb4/0x124

   which lock already depends on the new lock.

   the existing dependency chain (in reverse order) is:

   -> #3 (>commit_lock[i]){+.+.}-{3:3}:
  __mutex_lock_common+0x174/0x1a64
  mutex_lock_nested+0x98/0xac
  lock_crtcs+0xb4/0x124
  msm_atomic_commit_tail+0x330/0x748
  commit_tail+0x19c/0x278
  drm_atomic_helper_commit+0x1dc/0x1f0
  drm_atomic_commit+0xc0/0xd8
  drm_atomic_helper_set_config+0xb4/0x134
  drm_mode_setcrtc+0x688/0x1248
  drm_ioctl_kernel+0x1e4/0x338
  drm_ioctl+0x3a4/0x684
  __arm64_sys_ioctl+0x118/0x154
  invoke_syscall+0x78/0x224
  el0_svc_common+0x178/0x200
  do_el0_svc+0x94/0x13c
  el0_svc+0x5c/0xec
  el0t_64_sync_handler+0x78/0x108
  el0t_64_sync+0x1a4/0x1a8

   -> #2 (crtc_ww_class_mutex){+.+.}-{3:3}:
  __mutex_lock_common+0x174/0x1a64
  ww_mutex_lock+0xb8/0x278
  modeset_lock+0x304/0x4ac
  drm_modeset_lock+0x4c/0x7c
  drmm_mode_config_init+0x4a8/0xc50
  msm_drm_init+0x274/0xac0
  msm_drm_bind+0x20/0x2c
  try_to_bring_up_master+0x3dc/0x470
  __component_add+0x18c/0x3c0
  component_add+0x1c/0x28
  dp_display_probe+0x954/0xa98
  platform_probe+0x124/0x15c
  really_probe+0x1b0/0x5f8
  __driver_probe_device+0x174/0x20c
  driver_probe_device+0x70/0x134
  __device_attach_driver+0x130/0x1d0
  bus_for_each_drv+0xfc/0x14c
  __device_attach+0x1bc/0x2bc
  device_initial_probe+0x1c/0x28
  bus_probe_device+0x94/0x178
  deferred_probe_work_func+0x1a4/0x1f0
  process_one_work+0x5d4/0x9dc
  worker_thread+0x898/0xccc
  kthread+0x2d4/0x3d4
  ret_from_fork+0x10/0x20

   -> #1 (crtc_ww_class_acquire){+.+.}-{0:0}:
  ww_acquire_init+0x1c4/0x2c8
  drm_modeset_acquire_init+0x44/0xc8
  drm_helper_probe_single_connector_modes+0xb0/0x12dc
  drm_mode_getconnector+0x5dc/0xfe8
  drm_ioctl_kernel+0x1e4/0x338
  drm_ioctl+0x3a4/0x684
  __arm64_sys_ioctl+0x118/0x154
  invoke_syscall+0x78/0x224
  el0_svc_common+0x178/0x200
  do_el0_svc+0x94/0x13c
  el0_svc+0x5c/0xec
  el0t_64_sync_handler+0x78/0x108
  el0t_64_sync+0x1a4/0x1a8

   -> #0 (>mode_config.mutex){+.+.}-{3:3}:
  __lock_acquire+0x2650/0x672c
  lock_acquire+0x1b4/0x4ac
  __mutex_lock_common+0x174/0x1a64
  mutex_lock_nested+0x98/0xac
  dp_panel_add_fail_safe_mode+0x4c/0xa0
  dp_hpd_plug_handle+0x1f0/0x280
  dp_bridge_enable+0x94/0x2b8
  drm_atomic_bridge_chain_enable+0x11c/0x168
  drm_atomic_helper_commit_modeset_enables+0x500/0x740
  msm_atomic_commit_tail+0x3e4/0x748
  commit_tail+0x19c/0x278
  drm_atomic_helper_commit+0x1dc/0x1f0
  drm_atomic_commit+0xc0/0xd8
  drm_atomic_helper_set_config+0xb4/0x134
  drm_mode_setcrtc+0x688/0x1248
  drm_ioctl_kernel+0x1e4/0x338
  drm_ioctl+0x3a4/0x684
  __arm64_sys_ioctl+0x118/0x154
  invoke_syscall+0x78/0x224
  el0_svc_common+0x178/0x200
  do_el0_svc+0x94/0x13c
  el0_svc+0x5c/0xec
  el0t_64_sync_handler+0x78/0x108
  el0t_64_sync+0x1a4/0x1a8

Signed-off-by: Kuogee Hsieh 
---
   drivers/gpu/drm/msm/dp/dp_display.c |  6 --
   drivers/gpu/drm/msm/dp/dp_panel.c   | 23 +--
   2 files changed, 13 insertions(+), 16 deletions(-)

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

[Freedreno] [PATCH v2] drm/msm/dp: tear down main link at unplug handle immediately

2022-04-25 Thread Kuogee Hsieh

Two stages are required to setup up main link to be ready to transmit
video stream.
Stage 1: dp_hpd_plug_handle() perform link training to set up main link
stage 2: user space framework (msm_dp_display_enable()) to enable pixel
clock and transfer main link to video ready state.

At current implementation, when dongle unplugged dp_hdp_unplug_handle()
has to wait until stage 2 completed before it can send link down uevent
to user space framework to disable pixel clock followed by tearing down
main link.  This introduce unnecessary latency if dongle unplugged happen
after stage 1 and before stage 2. It also has possibility leave main link
stay at ready state after dongle unplugged if framework does not response
to link down uevent notification. This will prevent next dongle plug in
from working. This scenario could possibly happen when dongle unplug while
system in the middle of suspending.

This patch allow unplug handle to tear down main link and notify
framework link down immediately if dongle unplugged happen after
stage 1 and before stage 2. With this approach, dp driver is much
more resilient to any different scenarios. Also redundant both
dp_connect_pending_timeout() and dp_disconnect_pending_timeout()
are removed to reduce logic complexity.

Changes in V2:
-- return -EINVAL at msm_dp_display_enable() if not in correct state
-- replace ST_CONNECT_PENDING with ST_MAINLINK_READY

Fixes: 8ede2ecc3e5e ("drm/msm/dp: Add DP compliance tests on Snapdragon 
Chipsets")
Signed-off-by: Kuogee Hsieh 
Reviewed-by: Stephen Boyd 
---
 drivers/gpu/drm/msm/dp/dp_ctrl.c|  29 +
 drivers/gpu/drm/msm/dp/dp_ctrl.h|   1 +
 drivers/gpu/drm/msm/dp/dp_display.c | 113 +++-
 3 files changed, 63 insertions(+), 80 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index 5356856..5fb7b42 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1910,6 +1910,35 @@ int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl)
return ret;
 }
 
+int dp_ctrl_off_link(struct dp_ctrl *dp_ctrl)
+{
+   struct dp_ctrl_private *ctrl;
+   struct dp_io *dp_io;
+   struct phy *phy;
+   int ret;
+
+   ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
+   dp_io = >parser->io;
+   phy = dp_io->phy;
+
+   dp_catalog_ctrl_mainlink_ctrl(ctrl->catalog, false);
+
+   ret = dp_power_clk_enable(ctrl->power, DP_CTRL_PM, false);
+   if (ret) {
+   DRM_ERROR("Failed to disable link clocks. ret=%d\n", ret);
+   }
+
+   DRM_DEBUG_DP("Before, phy=%p init_count=%d power_on=%d\n",
+   phy, phy->init_count, phy->power_count);
+
+   phy_power_off(phy);
+
+   DRM_DEBUG_DP("After, phy=%p init_count=%d power_on=%d\n",
+   phy, phy->init_count, phy->power_count);
+
+   return ret;
+}
+
 int dp_ctrl_off(struct dp_ctrl *dp_ctrl)
 {
struct dp_ctrl_private *ctrl;
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h
index 2433edb..ffafe17 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
@@ -22,6 +22,7 @@ struct dp_ctrl {
 int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl);
 int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl);
 int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl);
+int dp_ctrl_off_link(struct dp_ctrl *dp_ctrl);
 int dp_ctrl_off(struct dp_ctrl *dp_ctrl);
 void dp_ctrl_push_idle(struct dp_ctrl *dp_ctrl);
 void dp_ctrl_isr(struct dp_ctrl *dp_ctrl);
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c
index 178b774..56c548b 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -42,7 +42,7 @@ enum {
 /* event thread connection state */
 enum {
ST_DISCONNECTED,
-   ST_CONNECT_PENDING,
+   ST_MAINLINK_READY,
ST_CONNECTED,
ST_DISCONNECT_PENDING,
ST_DISPLAY_OFF,
@@ -57,14 +57,11 @@ enum {
EV_IRQ_HPD_INT,
EV_HPD_UNPLUG_INT,
EV_USER_NOTIFICATION,
-   EV_CONNECT_PENDING_TIMEOUT,
-   EV_DISCONNECT_PENDING_TIMEOUT,
 };
 
 #define EVENT_TIMEOUT  (HZ/10) /* 100ms */
 #define DP_EVENT_Q_MAX 8
 
-#define DP_TIMEOUT_5_SECOND(5000/EVENT_TIMEOUT)
 #define DP_TIMEOUT_NONE0
 
 #define WAIT_FOR_RESUME_TIMEOUT_JIFFIES (HZ / 2)
@@ -451,6 +448,11 @@ static int dp_display_usbpd_configure_cb(struct device 
*dev)
 
 static int dp_display_usbpd_disconnect_cb(struct device *dev)
 {
+   return 0;
+}
+
+static int dp_display_notify_disconnect(struct device *dev)
+{
struct dp_display_private *dp = dev_get_dp_display_private(dev);
 
dp_add_event(dp, EV_USER_NOTIFICATION, false, 0);
@@ -478,7 +480,7 @@ static int dp_display_handle_port_ststus_changed(struct 
dp_display_private *dp)
}
} else {
if (dp->hpd_state == ST_DISCONNECTED) {
-   dp->hpd_state =

Re: [Freedreno] [PATCH] drm/msm/dp: tear down main link at unplug handle immediately

2022-04-25 Thread Kuogee Hsieh




On 4/20/2022 3:38 PM, Stephen Boyd wrote:

Quoting Kuogee Hsieh (2022-04-14 14:03:43)

Two stages are required to setup up main link to be ready to transmit
video stream.
Stage 1: dp_hpd_plug_handle() perform link training to set up main link
stage 2: user space framework (msm_dp_display_enable()) to enable pixel
clock and transfer main link to video ready state.

At current implementation, when dongle unplugged dp_hdp_unplug_handle()
has to wait until stage 2 completed before it can send link down uevent
to user space framework to disable pixel clock followed by tearing down
main link.  This introduce unnecessary latency if dongle unplugged happen
after stage 1 and before stage 2. It also has possibility leave main link
stay at ready state after dongle unplugged if framework does not response
to link down uevent notification. This will prevent next dongle plug in
from working. This scenario could possibly happen when dongle unplug while
system in the middle of suspending.

This patch allow unplug handle to tear down main link and notify
framework link down immediately if dongle unplugged happen after
stage 1 and before stage 2. With this approach, dp driver is much
more resilient to any different scenarios. Also redundant both
dp_connect_pending_timeout() and dp_disconnect_pending_timeout()
are removed to reduce logic complexity.

Fixes: 8ede2ecc3e5e ("drm/msm/dp: Add DP compliance tests on Snapdragon 
Chipsets")
Signed-off-by: Kuogee Hsieh 

Reviewed-by: Stephen Boyd 

Some questions below but doesn't seem like it will hold up this patch.


diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c
index 01453db..f5bd8f5 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -615,24 +598,21 @@ static int dp_hpd_unplug_handle(struct dp_display_private 
*dp, u32 data)
 if (dp->link->sink_count == 0) {
 dp_display_host_phy_exit(dp);
 }
+   dp_display_notify_disconnect(>pdev->dev);
 mutex_unlock(>event_mutex);
 return 0;
-   }
-
-   if (state == ST_DISCONNECT_PENDING) {
+   } else if (state == ST_DISCONNECT_PENDING) {
 mutex_unlock(>event_mutex);
 return 0;
-   }
-
-   if (state == ST_CONNECT_PENDING) {
-   /* wait until CONNECTED */
-   dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 1); /* delay = 1 */
+   } else if (state == ST_CONNECT_PENDING) {

I take it that ST_CONNECT_PENDING is sort of like "userspace hasn't
handled the uevent yet and modeset hasn't been called but the link is
setup and now we want to tear it down". The state name may want to be
changed to something else.

yes, how about change to  ST_MAINLINK_READY?



+   dp_ctrl_off_link(dp->ctrl);
+   dp_display_host_phy_exit(dp);
+   dp->hpd_state = ST_DISCONNECTED;
+   dp_display_notify_disconnect(>pdev->dev);
 mutex_unlock(>event_mutex);
 return 0;
 }

-   dp->hpd_state = ST_DISCONNECT_PENDING;
-
 /* disable HPD plug interrupts */
 dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, 
false);

@@ -640,10 +620,13 @@ static int dp_hpd_unplug_handle(struct dp_display_private 
*dp, u32 data)
  * We don't need separate work for disconnect as
  * connect/attention interrupts are disabled
  */
-   dp_display_usbpd_disconnect_cb(>pdev->dev);
+   dp_display_notify_disconnect(>pdev->dev);

-   /* start sentinel checking in case of missing uevent */
-   dp_add_event(dp, EV_DISCONNECT_PENDING_TIMEOUT, 0, DP_TIMEOUT_5_SECOND);
+   if (state == ST_DISPLAY_OFF) {
+   dp->hpd_state = ST_DISCONNECTED;
+   } else {
+   dp->hpd_state = ST_DISCONNECT_PENDING;
+   }

Nitpick: No braces needed for single line if clauses.


 DRM_DEBUG_DP("hpd_state=%d\n", state);
 /* signal the disconnect event early to ensure proper teardown */
@@ -1529,8 +1480,11 @@ int msm_dp_display_enable(struct msm_dp *dp, struct 
drm_encoder *encoder)

 mutex_lock(_display->event_mutex);

-   /* stop sentinel checking */
-   dp_del_event(dp_display, EV_CONNECT_PENDING_TIMEOUT);
+   state = dp_display->hpd_state;
+   if (state != ST_DISPLAY_OFF && state != ST_CONNECT_PENDING) {

Is this to guard against userspace doing an atomic commit when the
display isn't connected? Is that even possible?


No, it used to guard follow scenario in timing order,

1) plugin had been handled and mainlink is ready,

2)  userspace hasn't handled the uevent yet and modeset hasn't been called

3) unplugged happen, mainlink be teared down

4) user space start to response to uevent  and try to enable display. 
(it too late since mainlink had been teared down)




+   mutex_unlock(_display->event_mutex);
+   return rc;
+

Re: [Freedreno] [PATCH v2] drm/msm/dp: fix error check return value of irq_of_parse_and_map()

2022-04-25 Thread Dmitry Baryshkov


On 24/04/2022 06:24, cgel@gmail.com wrote:

From: Lv Ruyi 

The irq_of_parse_and_map() function returns 0 on failure, and does not
return an negative value.

Fixes: 8ede2ecc3e5e ("drm/msm/dp: Add DP compliance tests on Snapdragon 
Chipsets")
Reported-by: Zeal Robot 
Signed-off-by: Lv Ruyi 


Reviewed-by: Dmitry Baryshkov 


---
v2: don't print rc, and return -EINVAL rather than 0
---
  drivers/gpu/drm/msm/dp/dp_display.c | 7 +++
  1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c
index a42732b67349..c3566e6564b1 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1239,10 +1239,9 @@ int dp_display_request_irq(struct msm_dp *dp_display)
dp = container_of(dp_display, struct dp_display_private, dp_display);
  
  	dp->irq = irq_of_parse_and_map(dp->pdev->dev.of_node, 0);

-   if (dp->irq < 0) {
-   rc = dp->irq;
-   DRM_ERROR("failed to get irq: %d\n", rc);
-   return rc;
+   if (!dp->irq) {
+   DRM_ERROR("failed to get irq\n");
+   return -EINVAL;
}
  
  	rc = devm_request_irq(>pdev->dev, dp->irq,



--
With best wishes
Dmitry

Re: [Freedreno] [PATCH v2] drm: msm: fix error check return value of irq_of_parse_and_map()

2022-04-25 Thread Dmitry Baryshkov


On 24/04/2022 06:19, cgel@gmail.com wrote:

From: Lv Ruyi 

The irq_of_parse_and_map() function returns 0 on failure, and does not
return an negative value.

Reported-by: Zeal Robot 
Signed-off-by: Lv Ruyi 


Reviewed-by: Dmitry Baryshkov 


---
v2: don't print irq, and return ERR_PTR(-EINVAL)
---
  drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c 
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index 3b92372e7bdf..44e395e59df9 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -570,9 +570,9 @@ struct msm_kms *mdp5_kms_init(struct drm_device *dev)
}
  
  	irq = irq_of_parse_and_map(pdev->dev.of_node, 0);

-   if (irq < 0) {
-   ret = irq;
-   DRM_DEV_ERROR(>dev, "failed to get irq: %d\n", ret);
+   if (!irq) {
+   ret = -EINVAL;
+   DRM_DEV_ERROR(>dev, "failed to get irq\n");
goto fail;
}
  



--
With best wishes
Dmitry

Re: [Freedreno] [PATCH] drm/msm: change msm_sched_ops from global to static

2022-04-25 Thread Dmitry Baryshkov


On 21/04/2022 16:15, Tom Rix wrote:

Smatch reports this issue
msm_ringbuffer.c:43:36: warning: symbol 'msm_sched_ops' was not declared. 
Should it be static?

msm_sched_ops is only used in msm_ringbuffer.c so change its
storage-class specifier to static.

Signed-off-by: Tom Rix 


Reviewed-by: Dmitry Baryshkov 


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

diff --git a/drivers/gpu/drm/msm/msm_ringbuffer.c 
b/drivers/gpu/drm/msm/msm_ringbuffer.c
index 367a6aaa3a20..66f4ec09ef67 100644
--- a/drivers/gpu/drm/msm/msm_ringbuffer.c
+++ b/drivers/gpu/drm/msm/msm_ringbuffer.c
@@ -40,7 +40,7 @@ static void msm_job_free(struct drm_sched_job *job)
msm_gem_submit_put(submit);
  }
  
-const struct drm_sched_backend_ops msm_sched_ops = {

+static const struct drm_sched_backend_ops msm_sched_ops = {
.run_job = msm_job_run,
.free_job = msm_job_free
  };



--
With best wishes
Dmitry

[Freedreno] [PATCH v1 2/2] drm/msm/dp: do not stop transmitting phy test pattern during DP phy compliance test

2022-04-25 Thread Kuogee Hsieh

At normal operation, transmit phy test pattern has to be terminated before
DP controller switch to video ready state. However during phy compliance
testing, transmit phy test pattern should not be terminated until end of
compliance test which usually indicated by unplugged interrupt.

Fixes: 64e190e720a7 ("drm/msm/dp: DisplayPort PHY compliance tests fixup")
Signed-off-by: Kuogee Hsieh 
---
 drivers/gpu/drm/msm/dp/dp_ctrl.c | 15 +++
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index 193cc1a..f99e173 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1699,8 +1699,6 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
ctrl->link->link_params.rate,
ctrl->link->link_params.num_lanes, ctrl->dp_ctrl.pixel_rate);
 
-   ctrl->link->phy_params.p_level = 0;
-   ctrl->link->phy_params.v_level = 0;
 
rc = dp_ctrl_enable_mainlink_clocks(ctrl);
if (rc)
@@ -1822,12 +1820,6 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
}
}
 
-   if (!dp_ctrl_channel_eq_ok(ctrl))
-   dp_ctrl_link_retrain(ctrl);
-
-   /* stop txing train pattern to end link training */
-   dp_ctrl_clear_training_pattern(ctrl);
-
ret = dp_ctrl_enable_stream_clocks(ctrl);
if (ret) {
DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
@@ -1839,6 +1831,13 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
return 0;
}
 
+   if (!dp_ctrl_channel_eq_ok(ctrl)) {
+   dp_ctrl_link_retrain(ctrl);
+   }
+
+   /* stop txing train pattern to end link training */
+   dp_ctrl_clear_training_pattern(ctrl);
+
/*
 * Set up transfer unit values and set controller state to send
 * video.
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[Freedreno] [PATCH v1 1/2] drm/msm/dp: reset DP controller before transmit phy test pattern

2022-04-25 Thread Kuogee Hsieh

DP controller state can not switch from video ready state to
transmit phy pattern state at run time. DP mainlink has to be
teared down followed by reset controller to default state to have
DP controller switch to transmit phy test pattern state and start
generate specified phy test pattern to sinker once main link setup
again.

Fixes: ee35444be0c8 ("drm/msm/dp: use dp_ctrl_off_link_stream during PHY 
compliance test run")
Signed-off-by: Kuogee Hsieh 
---
 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 5356856..193cc1a 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1532,7 +1532,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_link_stream(>dp_ctrl);
+   ret = dp_ctrl_off(>dp_ctrl);
if (ret) {
DRM_ERROR("failed to disable DP controller\n");
return ret;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[Freedreno] [PATCH v1 0/2] fix DP phy compliance test

2022-04-25 Thread Kuogee Hsieh

Current DP phy compliance test failed due to test pattern generation was
terminated premature.

Kuogee Hsieh (2):
  drm/msm/dp: reset DP controller before transmit phy test pattern
  drm/msm/dp: do not stop transmitting phy test pattern during DP phy
compliance test

 drivers/gpu/drm/msm/dp/dp_ctrl.c | 17 -
 1 file changed, 8 insertions(+), 9 deletions(-)

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

Re: [Freedreno] [PATCH v4 2/2] drm/msm/dpu: Issue MDSS reset during initialization

2022-04-25 Thread Dmitry Baryshkov


On 21/04/2022 07:15, Bjorn Andersson wrote:

It's typical for the bootloader to bring up the display for showing a
boot splash or efi framebuffer. But in some cases the kernel driver ends
up only partially configuring (in particular) the DPU, which might
result in e.g. that two different data paths attempts to push data to
the interface - with resulting graphical artifacts.

Naturally the end goal would be to inherit the bootloader's
configuration and provide the user with a glitch free handover from the
boot configuration to a running DPU.

But as implementing seamless transition from the bootloader
configuration to the running OS will be a considerable effort, start by
simply resetting the entire MDSS to its power-on state, to avoid the
partial configuration.

Signed-off-by: Bjorn Andersson 


Reviewed-by: Dmitry Baryshkov 


---

Changes since v3:
- Rebased upon the mdss dpu/mdp restructuring 
(https://patchwork.freedesktop.org/series/98525/)

  drivers/gpu/drm/msm/msm_mdss.c | 32 
  1 file changed, 32 insertions(+)

diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
index f6f0d0fa5ab2..20f154dda9cf 100644
--- a/drivers/gpu/drm/msm/msm_mdss.c
+++ b/drivers/gpu/drm/msm/msm_mdss.c
@@ -4,11 +4,13 @@
   */
  
  #include 

+#include 
  #include 
  #include 
  #include 
  #include 
  #include 
+#include 
  
  #include "msm_drv.h"

  #include "msm_kms.h"
@@ -193,6 +195,32 @@ static void msm_mdss_destroy(struct msm_mdss *msm_mdss)
irq_set_chained_handler_and_data(irq, NULL, NULL);
  }
  
+static int msm_mdss_reset(struct device *dev)

+{
+   struct reset_control *reset;
+
+   reset = reset_control_get_optional_exclusive(dev, NULL);
+   if (!reset) {
+   /* Optional reset not specified */
+   return 0;
+   } else if (IS_ERR(reset)) {
+   return dev_err_probe(dev, PTR_ERR(reset),
+"failed to acquire mdss reset\n");
+   }
+
+   reset_control_assert(reset);
+   /*
+* Tests indicate that reset has to be held for some period of time,
+* make it one frame in a typical system
+*/
+   msleep(20);
+   reset_control_deassert(reset);
+
+   reset_control_put(reset);
+
+   return 0;
+}
+
  /*
   * MDP5 MDSS uses at most three specified clocks.
   */
@@ -229,6 +257,10 @@ static struct msm_mdss *msm_mdss_init(struct 
platform_device *pdev, bool is_mdp5
int ret;
int irq;
  
+	ret = msm_mdss_reset(>dev);

+   if (ret)
+   return ERR_PTR(ret);
+
msm_mdss = devm_kzalloc(>dev, sizeof(*msm_mdss), GFP_KERNEL);
if (!msm_mdss)
return ERR_PTR(-ENOMEM);



--
With best wishes
Dmitry

Re: [Freedreno] [PATCH 1/2] dt-bindings: msm/dp: List supplies in the bindings

2022-04-25 Thread Doug Anderson

Hi,

On Mon, Apr 25, 2022 at 2:14 PM Stephen Boyd  wrote:
>
> Quoting Douglas Anderson (2022-04-25 14:06:42)
> > We're supposed to list the supplies in the dt bindings but there are
> > none in the DP controller bindings. Looking at the Linux driver and
> > existing device trees, we can see that two supplies are expected:
> > - vdda-0p9-supply
> > - vdda-1p2-supply
> >
> > Let's list them both in the bindings. Note that the datasheet for
> > sc7280 doesn't describe these supplies very verbosely. For the 0p9
> > supply, for instance, it says "Power for eDP 0.9 V circuits". This
> > this is obvious from the property name, we don't bother cluttering the
> > bindings with a description.
> >
> > Signed-off-by: Douglas Anderson 
> > ---
> >
> >  .../devicetree/bindings/display/msm/dp-controller.yaml  | 6 ++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git 
> > a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml 
> > b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> > index cd05cfd76536..dba31108db51 100644
> > --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> > +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> > @@ -76,6 +76,9 @@ properties:
> >"#sound-dai-cells":
> >  const: 0
> >
> > +  vdda-0p9-supply: true
> > +  vdda-1p2-supply: true
> > +
> >ports:
> >  $ref: /schemas/graph.yaml#/properties/ports
> >  properties:
> > @@ -137,6 +140,9 @@ examples:
> >
> >  power-domains = < SC7180_CX>;
> >
> > +vdda-0p9-supply = <_usb_ss_dp_core>;
>
> Having 'a' in 'vdda' typically means 'analog' for 'analog' circuits, so
> I'd expect this to only matter for the phy that contains the analog
> circuitry. It would be great to remove the regulator code from
> drivers/gpu/drm/msm/dp/dp_power.c and move the regulator_set_load() call
> to the phy driver if possible. Hopefully qcom folks can help clarify
> here.

Interesting. Oddly enough, the sc7280 datasheet doesn't list the "_A".
It calls these "VDD_VREF_0P9" and "VDD_VREF_1P2". However, on the
schematic in front of me someone labeled these pins on the sc7280 with
the "A". ...and the driver looks for a supply with the "a". :-/

It would be good to get clarification from someone with better information.

-Doug

Re: [Freedreno] [PATCH 1/2] dt-bindings: msm/dp: List supplies in the bindings

2022-04-25 Thread Stephen Boyd

Quoting Douglas Anderson (2022-04-25 14:06:42)
> We're supposed to list the supplies in the dt bindings but there are
> none in the DP controller bindings. Looking at the Linux driver and
> existing device trees, we can see that two supplies are expected:
> - vdda-0p9-supply
> - vdda-1p2-supply
>
> Let's list them both in the bindings. Note that the datasheet for
> sc7280 doesn't describe these supplies very verbosely. For the 0p9
> supply, for instance, it says "Power for eDP 0.9 V circuits". This
> this is obvious from the property name, we don't bother cluttering the
> bindings with a description.
>
> Signed-off-by: Douglas Anderson 
> ---
>
>  .../devicetree/bindings/display/msm/dp-controller.yaml  | 6 ++
>  1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml 
> b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> index cd05cfd76536..dba31108db51 100644
> --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> @@ -76,6 +76,9 @@ properties:
>"#sound-dai-cells":
>  const: 0
>
> +  vdda-0p9-supply: true
> +  vdda-1p2-supply: true
> +
>ports:
>  $ref: /schemas/graph.yaml#/properties/ports
>  properties:
> @@ -137,6 +140,9 @@ examples:
>
>  power-domains = < SC7180_CX>;
>
> +vdda-0p9-supply = <_usb_ss_dp_core>;

Having 'a' in 'vdda' typically means 'analog' for 'analog' circuits, so
I'd expect this to only matter for the phy that contains the analog
circuitry. It would be great to remove the regulator code from
drivers/gpu/drm/msm/dp/dp_power.c and move the regulator_set_load() call
to the phy driver if possible. Hopefully qcom folks can help clarify
here.

> +vdda-1p2-supply = <_usb_ss_dp_1p2>;

Re: [Freedreno] [PATCH v9 1/4] drm/msm/dp: Add eDP support via aux_bus

2022-04-25 Thread Dmitry Baryshkov


On 25/04/2022 23:26, Stephen Boyd wrote:

Quoting Sankeerth Billakanti (QUIC) (2022-04-25 02:39:43)

Hi Stephen,


Quoting Sankeerth Billakanti (2022-04-22 02:11:03)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index d7a19d6..055681a 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c


Some nitpicks

Reviewed-by: Stephen Boyd 


@@ -1508,7 +1509,8 @@ 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_display->is_edp)
+   dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);


Did it turn out that in fact DP isn't ready still to setup even after delaying 
the
irq?



The host_init, config_hpd, phy_init and enable_irq are happening in 
modeset_init already for eDP.
So, I am not scheduling the EV_HPD_INIT_SETUP event for eDP. I am not modifying 
the delay for DP.


Cool. That didn't answer my question though. Why does DP still need the
delay? I thought recent changes made it unnecessary.


I'd say that if it is not necessary, it should be changed in the 
separate commit. The question is valid nevertheless.



--
With best wishes
Dmitry

Re: [Freedreno] [PATCH] drm/msm/hdmi: fix error check return value of irq_of_parse_and_map()

2022-04-25 Thread Dmitry Baryshkov


On 25/04/2022 23:11, Stephen Boyd wrote:

Quoting cgel@gmail.com (2022-04-25 02:18:31)

From: Lv Ruyi 

The irq_of_parse_and_map() function returns 0 on failure, and does not
return a negative value anyhow, so never enter this conditional branch.

Fixes: f6a8eaca0ea1 ("drm/msm/mdp5: use irqdomains")
Reported-by: Zeal Robot 
Signed-off-by: Lv Ruyi 
---


This one fixes a commit that moved away from platform APIs!


The change was introduced in 2014 in the commit f6a8eaca0ea1 
("drm/msm/mdp5: use irqdomains"). I can suspect that at that time 
platform irq code wasn't fully compatible with IRQ domains.



Reviewed-by: Stephen Boyd 



--
With best wishes
Dmitry

Re: [Freedreno] [PATCH] drm/msm/dpu: fix error check return value of irq_of_parse_and_map()

2022-04-25 Thread Dmitry Baryshkov


On 25/04/2022 23:10, Stephen Boyd wrote:

Quoting cgel@gmail.com (2022-04-25 02:09:47)

From: Lv Ruyi 

The irq_of_parse_and_map() function returns 0 on failure, and does not
return a negative value anyhow, so never enter this conditional branch.

Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
Reported-by: Zeal Robot 
Signed-off-by: Lv Ruyi 
---


Question still stands why we can't use platform device APIs.


Let's do a separate pass and replace all of functions with platform 
device API.



Reviewed-by: Stephen Boyd 



--
With best wishes
Dmitry

[Freedreno] [PATCH 2/2] dt-bindings: phy: List supplies for qcom, edp-phy

2022-04-25 Thread Douglas Anderson

We're supposed to list the supplies in the dt bindings but there are
none in the eDP PHY bindings.

Looking at the driver in Linux, I can see that there seem to be two
relevant supplies: "vdda-phy" and "vdda-pll". Let's add those to the
bindings.

NOTE: from looking at the Qualcomm datasheet for sc7280, it's not
immediately clear how to figure out how to fill in these supplies. The
only two eDP related supplies are simply described as "power for eDP
0.9V circuits" and "power for eDP 1.2V circuits". From guessing and
from comparing how a similar PHY is hooked up on other similar
Qualcomm boards, I'll make the educated guess that the 1.2V supply
goes to "vdda-phy" and the 0.9V supply goes to "vdda-pll" and I'll use
that in the example here.

Signed-off-by: Douglas Anderson 
---

 Documentation/devicetree/bindings/phy/qcom,edp-phy.yaml | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/phy/qcom,edp-phy.yaml 
b/Documentation/devicetree/bindings/phy/qcom,edp-phy.yaml
index a5850ff529f8..cf9e9b8011cb 100644
--- a/Documentation/devicetree/bindings/phy/qcom,edp-phy.yaml
+++ b/Documentation/devicetree/bindings/phy/qcom,edp-phy.yaml
@@ -41,6 +41,9 @@ properties:
   "#phy-cells":
 const: 0
 
+  vdda-phy-supply: true
+  vdda-pll-supply: true
+
 required:
   - compatible
   - reg
@@ -65,5 +68,8 @@ examples:
 
   #clock-cells = <1>;
   #phy-cells = <0>;
+
+  vdda-phy-supply = <_a_edp_0_1p2>;
+  vdda-pll-supply = <_a_edp_0_0p9>;
 };
 ...
-- 
2.36.0.rc2.479.g8af0fa9b8e-goog

[Freedreno] [PATCH 1/2] dt-bindings: msm/dp: List supplies in the bindings

2022-04-25 Thread Douglas Anderson

We're supposed to list the supplies in the dt bindings but there are
none in the DP controller bindings. Looking at the Linux driver and
existing device trees, we can see that two supplies are expected:
- vdda-0p9-supply
- vdda-1p2-supply

Let's list them both in the bindings. Note that the datasheet for
sc7280 doesn't describe these supplies very verbosely. For the 0p9
supply, for instance, it says "Power for eDP 0.9 V circuits". This
this is obvious from the property name, we don't bother cluttering the
bindings with a description.

Signed-off-by: Douglas Anderson 
---

 .../devicetree/bindings/display/msm/dp-controller.yaml  | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml 
b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
index cd05cfd76536..dba31108db51 100644
--- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
+++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
@@ -76,6 +76,9 @@ properties:
   "#sound-dai-cells":
 const: 0
 
+  vdda-0p9-supply: true
+  vdda-1p2-supply: true
+
   ports:
 $ref: /schemas/graph.yaml#/properties/ports
 properties:
@@ -137,6 +140,9 @@ examples:
 
 power-domains = < SC7180_CX>;
 
+vdda-0p9-supply = <_usb_ss_dp_core>;
+vdda-1p2-supply = <_usb_ss_dp_1p2>;
+
 ports {
 #address-cells = <1>;
 #size-cells = <0>;
-- 
2.36.0.rc2.479.g8af0fa9b8e-goog

[Freedreno] [PATCH 0/2] dt-bindings: List supplies needed for sc7280 eDP

2022-04-25 Thread Douglas Anderson

While looking through dts changes for sc7280 eDP I found that none of
the regulators that were being defined were listed in the
bindings. That being said, the current Linux drivers _were_ looking
for regulators. This series tries to document the reality to the best
of my ability.

If someone from Qualcomm has better documentation than I do and wants
to correct anything here then I'd be more than happy. For the PHY
patch especially I don't have a great way to map what I see in
datasheets / schematics to figure out which is the supply for the
"phy" and the "pll".

Assuming these look OK, I'd expect the PHY patch to land through the
PHY tree and the display patch to land through msm-next. I can split
the series up if need be--the two patches are just in one series
because they have a similar topic--there are no actual dependencies
here.


Douglas Anderson (2):
  dt-bindings: msm/dp: List supplies in the bindings
  dt-bindings: phy: List supplies for qcom,edp-phy

 .../devicetree/bindings/display/msm/dp-controller.yaml  | 6 ++
 Documentation/devicetree/bindings/phy/qcom,edp-phy.yaml | 6 ++
 2 files changed, 12 insertions(+)

-- 
2.36.0.rc2.479.g8af0fa9b8e-goog

Re: [Freedreno] [PATCH v9 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP

2022-04-25 Thread Stephen Boyd

Quoting Sankeerth Billakanti (QUIC) (2022-04-24 19:55:29)
> >Quoting Sankeerth Billakanti (2022-04-22 02:11:04)
> >
> >>  int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog) diff
> >> --git a/drivers/gpu/drm/msm/dp/dp_display.c
> >> b/drivers/gpu/drm/msm/dp/dp_display.c
> >> index 055681a..dea4de9 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >> @@ -1096,6 +1097,13 @@ static void dp_display_config_hpd(struct
> >dp_display_private *dp)
> >> dp_display_host_init(dp);
> >> dp_catalog_ctrl_hpd_config(dp->catalog);
> >>
> >> +   /* Enable plug and unplug interrupts only for external DisplayPort 
> >> */
> >> +   if (!dp->dp_display.is_edp)
> >> +   dp_catalog_hpd_config_intr(dp->catalog,
> >> +   DP_DP_HPD_PLUG_INT_MASK |
> >> +   DP_DP_HPD_UNPLUG_INT_MASK,
> >> +   true);
> >> +
> >
> >It seems like only the plug and unplug is enabled for DP here. Is replug
> >enabled for eDP when it shouldn't be?
> >
>
> By default, all the interrupts are masked. This function is not executed for 
> eDP
> anymore because the host_init, phy_init and enable_irq are all done from
> modeset_init for eDP with aux_bus. So, none of the eDP hpd interrupts are
> enabled or unmasked before pre-enable.
>
> The replug interrupt is unmasked for DP and eDP from the dp_hpd_plug_handle()
> and masked from dp_hpd_unplug_handle().

Why is replug enabled for eDP?

Re: [Freedreno] [PATCH v9 1/4] drm/msm/dp: Add eDP support via aux_bus

2022-04-25 Thread Stephen Boyd

Quoting Sankeerth Billakanti (QUIC) (2022-04-25 02:39:43)
> Hi Stephen,
>
> >Quoting Sankeerth Billakanti (2022-04-22 02:11:03)
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> >> b/drivers/gpu/drm/msm/dp/dp_display.c
> >> index d7a19d6..055681a 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >
> >Some nitpicks
> >
> >Reviewed-by: Stephen Boyd 
> >
> >> @@ -1508,7 +1509,8 @@ 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_display->is_edp)
> >> +   dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
> >
> >Did it turn out that in fact DP isn't ready still to setup even after 
> >delaying the
> >irq?
> >
>
> The host_init, config_hpd, phy_init and enable_irq are happening in 
> modeset_init already for eDP.
> So, I am not scheduling the EV_HPD_INIT_SETUP event for eDP. I am not 
> modifying the delay for DP.

Cool. That didn't answer my question though. Why does DP still need the
delay? I thought recent changes made it unnecessary.

Re: [Freedreno] [PATCH] drm/msm: fix returnvar.cocci warning

2022-04-25 Thread Stephen Boyd

Quoting Guo Zhengkui (2022-04-25 05:22:21)
> Fix the following coccicheck warning:
> drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c:24:5-8: Unneeded variable:
> "ret". Return "0" on line 75.
>
> Signed-off-by: Guo Zhengkui 
> ---

Reviewed-by: Stephen Boyd

Re: [Freedreno] [PATCH] drm/msm/hdmi: fix error check return value of irq_of_parse_and_map()

2022-04-25 Thread Stephen Boyd

Quoting cgel@gmail.com (2022-04-25 02:18:31)
> From: Lv Ruyi 
>
> The irq_of_parse_and_map() function returns 0 on failure, and does not
> return a negative value anyhow, so never enter this conditional branch.
>
> Fixes: f6a8eaca0ea1 ("drm/msm/mdp5: use irqdomains")
> Reported-by: Zeal Robot 
> Signed-off-by: Lv Ruyi 
> ---

This one fixes a commit that moved away from platform APIs!

Reviewed-by: Stephen Boyd

Re: [Freedreno] [PATCH] drm/msm/dpu: fix error check return value of irq_of_parse_and_map()

2022-04-25 Thread Stephen Boyd

Quoting cgel@gmail.com (2022-04-25 02:09:47)
> From: Lv Ruyi 
>
> The irq_of_parse_and_map() function returns 0 on failure, and does not
> return a negative value anyhow, so never enter this conditional branch.
>
> Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
> Reported-by: Zeal Robot 
> Signed-off-by: Lv Ruyi 
> ---

Question still stands why we can't use platform device APIs.

Reviewed-by: Stephen Boyd

Re: [Freedreno] [PATCH v2 1/3] drm/msm/dpu: index dpu_kms->hw_vbif using vbif_idx

2022-04-25 Thread Abhinav Kumar


+ Vinod

Hi Dmitry

Can we also absorb https://patchwork.freedesktop.org/patch/483255/ into 
this change?


Looks like they are touching the same code and can be absorbed easily.

Thanks

Abhinav


On 4/19/2022 9:20 AM, Dmitry Baryshkov wrote:

Remove loops over hw_vbif. Instead always VBIF's idx as an index in the
array. This fixes an error in dpu_kms_hw_init(), where we fill
dpu_kms->hw_vbif[i], but check for an error pointer at
dpu_kms->hw_vbif[vbif_idx].

Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
Signed-off-by: Dmitry Baryshkov 
---
  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  | 10 
  drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c | 29 +++-
  2 files changed, 17 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index e29796c4f27b..aadf032a190b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -790,11 +790,9 @@ static void _dpu_kms_hw_destroy(struct dpu_kms *dpu_kms)
_dpu_kms_mmu_destroy(dpu_kms);
  
  	if (dpu_kms->catalog) {

-   for (i = 0; i < dpu_kms->catalog->vbif_count; i++) {
-   u32 vbif_idx = dpu_kms->catalog->vbif[i].id;
-
-   if ((vbif_idx < VBIF_MAX) && dpu_kms->hw_vbif[vbif_idx])
-   dpu_hw_vbif_destroy(dpu_kms->hw_vbif[vbif_idx]);
+   for (i = 0; i < ARRAY_SIZE(dpu_kms->hw_vbif); i++) {
+   if (dpu_kms->hw_vbif[i])
+   dpu_hw_vbif_destroy(dpu_kms->hw_vbif[i]);
}
}
  
@@ -1102,7 +1100,7 @@ static int dpu_kms_hw_init(struct msm_kms *kms)

for (i = 0; i < dpu_kms->catalog->vbif_count; i++) {
u32 vbif_idx = dpu_kms->catalog->vbif[i].id;
  
-		dpu_kms->hw_vbif[i] = dpu_hw_vbif_init(vbif_idx,

+   dpu_kms->hw_vbif[vbif_idx] = dpu_hw_vbif_init(vbif_idx,
dpu_kms->vbif[vbif_idx], dpu_kms->catalog);
if (IS_ERR_OR_NULL(dpu_kms->hw_vbif[vbif_idx])) {
rc = PTR_ERR(dpu_kms->hw_vbif[vbif_idx]);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
index 21d20373eb8b..a18fb649301c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
@@ -11,6 +11,14 @@
  #include "dpu_hw_vbif.h"
  #include "dpu_trace.h"
  
+static struct dpu_hw_vbif *dpu_get_vbif(struct dpu_kms *dpu_kms, enum dpu_vbif vbif_idx)

+{
+   if (vbif_idx < ARRAY_SIZE(dpu_kms->hw_vbif))
+   return dpu_kms->hw_vbif[vbif_idx];
+
+   return NULL;
+}
+
  /**
   * _dpu_vbif_wait_for_xin_halt - wait for the xin to halt
   * @vbif: Pointer to hardware vbif driver
@@ -148,20 +156,15 @@ static u32 _dpu_vbif_get_ot_limit(struct dpu_hw_vbif 
*vbif,
  void dpu_vbif_set_ot_limit(struct dpu_kms *dpu_kms,
struct dpu_vbif_set_ot_params *params)
  {
-   struct dpu_hw_vbif *vbif = NULL;
+   struct dpu_hw_vbif *vbif;
struct dpu_hw_mdp *mdp;
bool forced_on = false;
u32 ot_lim;
-   int ret, i;
+   int ret;
  
  	mdp = dpu_kms->hw_mdp;
  
-	for (i = 0; i < ARRAY_SIZE(dpu_kms->hw_vbif); i++) {

-   if (dpu_kms->hw_vbif[i] &&
-   dpu_kms->hw_vbif[i]->idx == params->vbif_idx)
-   vbif = dpu_kms->hw_vbif[i];
-   }
-
+   vbif = dpu_get_vbif(dpu_kms, params->vbif_idx);
if (!vbif || !mdp) {
DRM_DEBUG_ATOMIC("invalid arguments vbif %d mdp %d\n",
vbif != NULL, mdp != NULL);
@@ -204,7 +207,7 @@ void dpu_vbif_set_ot_limit(struct dpu_kms *dpu_kms,
  void dpu_vbif_set_qos_remap(struct dpu_kms *dpu_kms,
struct dpu_vbif_set_qos_params *params)
  {
-   struct dpu_hw_vbif *vbif = NULL;
+   struct dpu_hw_vbif *vbif;
struct dpu_hw_mdp *mdp;
bool forced_on = false;
const struct dpu_vbif_qos_tbl *qos_tbl;
@@ -216,13 +219,7 @@ void dpu_vbif_set_qos_remap(struct dpu_kms *dpu_kms,
}
mdp = dpu_kms->hw_mdp;
  
-	for (i = 0; i < ARRAY_SIZE(dpu_kms->hw_vbif); i++) {

-   if (dpu_kms->hw_vbif[i] &&
-   dpu_kms->hw_vbif[i]->idx == params->vbif_idx) {
-   vbif = dpu_kms->hw_vbif[i];
-   break;
-   }
-   }
+   vbif = dpu_get_vbif(dpu_kms, params->vbif_idx);
  
  	if (!vbif || !vbif->cap) {

DPU_ERROR("invalid vbif %d\n", params->vbif_idx);

Re: [Freedreno] [PATCH v4 03/20] drm: allow real encoder to be passed for drm_writeback_connector

2022-04-25 Thread Laurent Pinchart

On Mon, Apr 25, 2022 at 10:48:07AM -0700, Abhinav Kumar wrote:
> On 4/25/2022 10:32 AM, Laurent Pinchart wrote:
> > On Mon, Apr 25, 2022 at 01:50:43PM +0300, Dmitry Baryshkov wrote:
> >> On Sun, 24 Apr 2022 at 22:59, Laurent Pinchart wrote:
> >>> On Sun, Apr 24, 2022 at 11:23:20AM -0700, Abhinav Kumar wrote:
>  On 4/24/2022 11:12 AM, Abhinav Kumar wrote:
> > On 4/24/2022 7:50 AM, Laurent Pinchart wrote:
> >> On Fri, Apr 22, 2022 at 04:06:38PM -0700, Abhinav Kumar wrote:
> >>> For some vendor driver implementations, display hardware can
> >>> be shared between the encoder used for writeback and the physical
> >>> display.
> >>>
> >>> In addition resources such as clocks and interrupts can
> >>> also be shared between writeback and the real encoder.
> >>>
> >>> To accommodate such vendor drivers and hardware, allow
> >>> real encoder to be passed for drm_writeback_connector.
> >>>
> >>> For existing clients, drm_writeback_connector_init() will use
> >>> an internal_encoder under the hood and hence no changes will
> >>> be needed.
> >>>
> >>> changes in v7:
> >>>  - move this change before the vc4 change in the series
> >>>to minimize the changes to vendor drivers in drm core
> >>>changes
> >>
> >> Why is this needed ? The drm_writeback_connector functions don't need
> >> the drm_encoder after drm_writeback_connector_init() (or
> >> drm_writeback_connector_init_with_encoder()) returns.
> >>
> >
> > Sorry I didnt follow this comment. This change log is incorrect, so
> > after changing the previous change in the series and modifying this, no
> > further changes are needed to vc4, so I decided to drop the next change.
> > So this change log is incorrect. I can remove this.
> >
> > Is that what you meant?
> 
>  So till the previous change, the only user of
>  drm_writeback_connector_init_with_encoder() was
>  drm_writeback_connector_init() which was still passing its own
>  internal_encoder.
> 
>  Only if the wb_connector->encoder is changed to a pointer, other vendor
>  drivers can pass their own encoder to
>  drm_writeback_connector_init_with_encoder().
> 
>  Hence you are right that drm_writeback_connector functions do not need
>  drm_encoder after init() returns, but till this change is done, other
>  vendor drivers cannot directly call
>  drm_writeback_connector_init_with_encoder() because the encoder will not
>  be valid till then.
> >>>
> >>> Users of drm_writeback_connector_init_with_encoder() handle the encoder
> >>> themselves, they can simply ignore drm_writeback_connector.encoder. The
> >>> documentation of the encoder field needs to be updated though (I'd do so
> >>> in the previous patch), to clearly mention that the field is valid only
> >>> when using drm_writeback_connector_init(), not when calling
> >>> drm_writeback_connector_init_with_encoder().
> >>
> >> If we allow it to be unitialized, it might end with hard-to-trace
> >> bugs, additional conditions, etc.
> >> In my opnion we should:
> >>   - either make drm_writeback_connector.encoder a valid pointer
> >>   - or drop the field completely.
> >>
> >> And up to some point I'd vote for the second option. The code using
> >> internal_encoder can continue using it directly. The code using
> >> drm_writeback_connector_init_with_encoder() will manage encoder on
> >> their own. We will loose a single entry point for wb's encoder, but do
> >> we really need it? (Frankly speaking I didn't check.)
> > 
> > As far as I understand, we went for the second option as Abhinav dropped
> > this patch for the next version.
> 
> I dropped the patch as there was no agreement yet and I didnt want to 
> block the series as its not really needed now but thats not option 2 
> because wb_conn->encoder field remains there, just that its unused for 
> users using drm_writeback_connector_init_with_encoder().
> 
> I guess what Dmitry is suggesting is just drop that 
> wb_connector->encoder field completely.

I don't think we can do that, as that field is used for the internal
encoder :-) It could be renamed to internal_encoder if desired, but not
dropped. I don't mind much either way, as long as we make it clear that
drivers must never reference it directly.

>  Hope this clarifies it.
> 
> >>> Signed-off-by: Abhinav Kumar 
> >>> Reviewed-by: Dmitry Baryshkov 
> >>> ---
> >>>drivers/gpu/drm/drm_writeback.c | 18 --
> >>>drivers/gpu/drm/vc4/vc4_txp.c   |  4 ++--
> >>>include/drm/drm_writeback.h | 22 --
> >>>3 files changed, 34 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_writeback.c
> >>> b/drivers/gpu/drm/drm_writeback.c
> >>> index 92658ad..0538674 100644
> >>> --- a/drivers/gpu/drm/drm_writeback.c
> >>> +++

Re: [Freedreno] [PATCH v4 03/20] drm: allow real encoder to be passed for drm_writeback_connector

2022-04-25 Thread Abhinav Kumar


Hi Laurent

On 4/25/2022 10:32 AM, Laurent Pinchart wrote:

On Mon, Apr 25, 2022 at 01:50:43PM +0300, Dmitry Baryshkov wrote:

On Sun, 24 Apr 2022 at 22:59, Laurent Pinchart wrote:

On Sun, Apr 24, 2022 at 11:23:20AM -0700, Abhinav Kumar wrote:

On 4/24/2022 11:12 AM, Abhinav Kumar wrote:

On 4/24/2022 7:50 AM, Laurent Pinchart wrote:

On Fri, Apr 22, 2022 at 04:06:38PM -0700, Abhinav Kumar wrote:

For some vendor driver implementations, display hardware can
be shared between the encoder used for writeback and the physical
display.

In addition resources such as clocks and interrupts can
also be shared between writeback and the real encoder.

To accommodate such vendor drivers and hardware, allow
real encoder to be passed for drm_writeback_connector.

For existing clients, drm_writeback_connector_init() will use
an internal_encoder under the hood and hence no changes will
be needed.

changes in v7:
 - move this change before the vc4 change in the series
   to minimize the changes to vendor drivers in drm core
   changes


Why is this needed ? The drm_writeback_connector functions don't need
the drm_encoder after drm_writeback_connector_init() (or
drm_writeback_connector_init_with_encoder()) returns.



Sorry I didnt follow this comment. This change log is incorrect, so
after changing the previous change in the series and modifying this, no
further changes are needed to vc4, so I decided to drop the next change.
So this change log is incorrect. I can remove this.

Is that what you meant?


So till the previous change, the only user of
drm_writeback_connector_init_with_encoder() was
drm_writeback_connector_init() which was still passing its own
internal_encoder.

Only if the wb_connector->encoder is changed to a pointer, other vendor
drivers can pass their own encoder to
drm_writeback_connector_init_with_encoder().

Hence you are right that drm_writeback_connector functions do not need
drm_encoder after init() returns, but till this change is done, other
vendor drivers cannot directly call
drm_writeback_connector_init_with_encoder() because the encoder will not
be valid till then.


Users of drm_writeback_connector_init_with_encoder() handle the encoder
themselves, they can simply ignore drm_writeback_connector.encoder. The
documentation of the encoder field needs to be updated though (I'd do so
in the previous patch), to clearly mention that the field is valid only
when using drm_writeback_connector_init(), not when calling
drm_writeback_connector_init_with_encoder().


If we allow it to be unitialized, it might end with hard-to-trace
bugs, additional conditions, etc.
In my opnion we should:
  - either make drm_writeback_connector.encoder a valid pointer
  - or drop the field completely.

And up to some point I'd vote for the second option. The code using
internal_encoder can continue using it directly. The code using
drm_writeback_connector_init_with_encoder() will manage encoder on
their own. We will loose a single entry point for wb's encoder, but do
we really need it? (Frankly speaking I didn't check.)


As far as I understand, we went for the second option as Abhinav dropped
this patch for the next version.



I dropped the patch as there was no agreement yet and I didnt want to 
block the series as its not really needed now but thats not option 2 
because wb_conn->encoder field remains there, just that its unused for 
users using drm_writeback_connector_init_with_encoder().


I guess what Dmitry is suggesting is just drop that 
wb_connector->encoder field completely.



Hope this clarifies it.


Signed-off-by: Abhinav Kumar 
Reviewed-by: Dmitry Baryshkov 
---
   drivers/gpu/drm/drm_writeback.c | 18 --
   drivers/gpu/drm/vc4/vc4_txp.c   |  4 ++--
   include/drm/drm_writeback.h | 22 --
   3 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_writeback.c
b/drivers/gpu/drm/drm_writeback.c
index 92658ad..0538674 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -180,21 +180,21 @@ int drm_writeback_connector_init(struct
drm_device *dev,
   {
   int ret = 0;
-drm_encoder_helper_add(_connector->encoder, enc_helper_funcs);
+drm_encoder_helper_add(_connector->internal_encoder, enc_helper_funcs);
-wb_connector->encoder.possible_crtcs = possible_crtcs;
+wb_connector->internal_encoder.possible_crtcs = possible_crtcs;
-ret = drm_encoder_init(dev, _connector->encoder,
+ret = drm_encoder_init(dev, _connector->internal_encoder,
  _writeback_encoder_funcs,
  DRM_MODE_ENCODER_VIRTUAL, NULL);
   if (ret)
   return ret;
-ret = drm_writeback_connector_init_with_encoder(dev, wb_connector, 
_connector->encoder,
-con_funcs, formats, n_formats);
+ret = drm_writeback_connector_init_with_encoder(dev, wb_connector,
+_connector->internal_encoder, con_funcs, formats, n_formats);
   if (ret)
-

Re: [Freedreno] [PATCH v4 03/20] drm: allow real encoder to be passed for drm_writeback_connector

2022-04-25 Thread Laurent Pinchart

On Mon, Apr 25, 2022 at 01:50:43PM +0300, Dmitry Baryshkov wrote:
> On Sun, 24 Apr 2022 at 22:59, Laurent Pinchart wrote:
> > On Sun, Apr 24, 2022 at 11:23:20AM -0700, Abhinav Kumar wrote:
> > > On 4/24/2022 11:12 AM, Abhinav Kumar wrote:
> > > > On 4/24/2022 7:50 AM, Laurent Pinchart wrote:
> > > >> On Fri, Apr 22, 2022 at 04:06:38PM -0700, Abhinav Kumar wrote:
> > > >>> For some vendor driver implementations, display hardware can
> > > >>> be shared between the encoder used for writeback and the physical
> > > >>> display.
> > > >>>
> > > >>> In addition resources such as clocks and interrupts can
> > > >>> also be shared between writeback and the real encoder.
> > > >>>
> > > >>> To accommodate such vendor drivers and hardware, allow
> > > >>> real encoder to be passed for drm_writeback_connector.
> > > >>>
> > > >>> For existing clients, drm_writeback_connector_init() will use
> > > >>> an internal_encoder under the hood and hence no changes will
> > > >>> be needed.
> > > >>>
> > > >>> changes in v7:
> > > >>> - move this change before the vc4 change in the series
> > > >>>   to minimize the changes to vendor drivers in drm core
> > > >>>   changes
> > > >>
> > > >> Why is this needed ? The drm_writeback_connector functions don't need
> > > >> the drm_encoder after drm_writeback_connector_init() (or
> > > >> drm_writeback_connector_init_with_encoder()) returns.
> > > >>
> > > >
> > > > Sorry I didnt follow this comment. This change log is incorrect, so
> > > > after changing the previous change in the series and modifying this, no
> > > > further changes are needed to vc4, so I decided to drop the next change.
> > > > So this change log is incorrect. I can remove this.
> > > >
> > > > Is that what you meant?
> > >
> > > So till the previous change, the only user of
> > > drm_writeback_connector_init_with_encoder() was
> > > drm_writeback_connector_init() which was still passing its own
> > > internal_encoder.
> > >
> > > Only if the wb_connector->encoder is changed to a pointer, other vendor
> > > drivers can pass their own encoder to
> > > drm_writeback_connector_init_with_encoder().
> > >
> > > Hence you are right that drm_writeback_connector functions do not need
> > > drm_encoder after init() returns, but till this change is done, other
> > > vendor drivers cannot directly call
> > > drm_writeback_connector_init_with_encoder() because the encoder will not
> > > be valid till then.
> >
> > Users of drm_writeback_connector_init_with_encoder() handle the encoder
> > themselves, they can simply ignore drm_writeback_connector.encoder. The
> > documentation of the encoder field needs to be updated though (I'd do so
> > in the previous patch), to clearly mention that the field is valid only
> > when using drm_writeback_connector_init(), not when calling
> > drm_writeback_connector_init_with_encoder().
> 
> If we allow it to be unitialized, it might end with hard-to-trace
> bugs, additional conditions, etc.
> In my opnion we should:
>  - either make drm_writeback_connector.encoder a valid pointer
>  - or drop the field completely.
> 
> And up to some point I'd vote for the second option. The code using
> internal_encoder can continue using it directly. The code using
> drm_writeback_connector_init_with_encoder() will manage encoder on
> their own. We will loose a single entry point for wb's encoder, but do
> we really need it? (Frankly speaking I didn't check.)

As far as I understand, we went for the second option as Abhinav dropped
this patch for the next version.

> > > Hope this clarifies it.
> > >
> > > >>> Signed-off-by: Abhinav Kumar 
> > > >>> Reviewed-by: Dmitry Baryshkov 
> > > >>> ---
> > > >>>   drivers/gpu/drm/drm_writeback.c | 18 --
> > > >>>   drivers/gpu/drm/vc4/vc4_txp.c   |  4 ++--
> > > >>>   include/drm/drm_writeback.h | 22 --
> > > >>>   3 files changed, 34 insertions(+), 10 deletions(-)
> > > >>>
> > > >>> diff --git a/drivers/gpu/drm/drm_writeback.c
> > > >>> b/drivers/gpu/drm/drm_writeback.c
> > > >>> index 92658ad..0538674 100644
> > > >>> --- a/drivers/gpu/drm/drm_writeback.c
> > > >>> +++ b/drivers/gpu/drm/drm_writeback.c
> > > >>> @@ -180,21 +180,21 @@ int drm_writeback_connector_init(struct
> > > >>> drm_device *dev,
> > > >>>   {
> > > >>>   int ret = 0;
> > > >>> -drm_encoder_helper_add(_connector->encoder, enc_helper_funcs);
> > > >>> +drm_encoder_helper_add(_connector->internal_encoder, 
> > > >>> enc_helper_funcs);
> > > >>> -wb_connector->encoder.possible_crtcs = possible_crtcs;
> > > >>> +wb_connector->internal_encoder.possible_crtcs = possible_crtcs;
> > > >>> -ret = drm_encoder_init(dev, _connector->encoder,
> > > >>> +ret = drm_encoder_init(dev, _connector->internal_encoder,
> > > >>>  _writeback_encoder_funcs,
> > > >>>  DRM_MODE_ENCODER_VIRTUAL, NULL);
> > > >>>   if (ret)
> > > >>>   return ret;
> > > >>> -ret =

Re: [Freedreno] [PATCH v4 03/20] drm: allow real encoder to be passed for drm_writeback_connector

2022-04-25 Thread Abhinav Kumar





On 4/25/2022 3:50 AM, Dmitry Baryshkov wrote:

On Sun, 24 Apr 2022 at 22:59, Laurent Pinchart
 wrote:


Hi Abhinav,

On Sun, Apr 24, 2022 at 11:23:20AM -0700, Abhinav Kumar wrote:

On 4/24/2022 11:12 AM, Abhinav Kumar wrote:

On 4/24/2022 7:50 AM, Laurent Pinchart wrote:

On Fri, Apr 22, 2022 at 04:06:38PM -0700, Abhinav Kumar wrote:

For some vendor driver implementations, display hardware can
be shared between the encoder used for writeback and the physical
display.

In addition resources such as clocks and interrupts can
also be shared between writeback and the real encoder.

To accommodate such vendor drivers and hardware, allow
real encoder to be passed for drm_writeback_connector.

For existing clients, drm_writeback_connector_init() will use
an internal_encoder under the hood and hence no changes will
be needed.

changes in v7:
 - move this change before the vc4 change in the series
   to minimize the changes to vendor drivers in drm core
   changes


Why is this needed ? The drm_writeback_connector functions don't need
the drm_encoder after drm_writeback_connector_init() (or
drm_writeback_connector_init_with_encoder()) returns.



Sorry I didnt follow this comment. This change log is incorrect, so
after changing the previous change in the series and modifying this, no
further changes are needed to vc4, so I decided to drop the next change.
So this change log is incorrect. I can remove this.

Is that what you meant?


So till the previous change, the only user of
drm_writeback_connector_init_with_encoder() was
drm_writeback_connector_init() which was still passing its own
internal_encoder.

Only if the wb_connector->encoder is changed to a pointer, other vendor
drivers can pass their own encoder to
drm_writeback_connector_init_with_encoder().

Hence you are right that drm_writeback_connector functions do not need
drm_encoder after init() returns, but till this change is done, other
vendor drivers cannot directly call
drm_writeback_connector_init_with_encoder() because the encoder will not
be valid till then.


Users of drm_writeback_connector_init_with_encoder() handle the encoder
themselves, they can simply ignore drm_writeback_connector.encoder. The
documentation of the encoder field needs to be updated though (I'd do so
in the previous patch), to clearly mention that the field is valid only
when using drm_writeback_connector_init(), not when calling
drm_writeback_connector_init_with_encoder().


If we allow it to be unitialized, it might end with hard-to-trace
bugs, additional conditions, etc.
In my opnion we should:
  - either make drm_writeback_connector.encoder a valid pointer
  - or drop the field completely.

And up to some point I'd vote for the second option. The code using
internal_encoder can continue using it directly. The code using
drm_writeback_connector_init_with_encoder() will manage encoder on
their own. We will loose a single entry point for wb's encoder, but do
we really need it? (Frankly speaking I didn't check.)

Agree with your comment Dmitry and thats why I was suggesting to let 
drm_writeback_connector.encoder remain a valid pointer to avoid 
discrepancies.


If we go with option (1), this change as-it-is is the best way to go.

If we go with option (2), to drop the encoder field and just rename 
encoder ---> internal_encoder, it will still work for existing users.


But I would prefer that goes as a separate change then because MSM 
doesn't really need this ( and at this point even other drivers don't )


I am still ready to post the change separately and do basic validation 
with our boards.





Hope this clarifies it.


Signed-off-by: Abhinav Kumar 
Reviewed-by: Dmitry Baryshkov 
---
   drivers/gpu/drm/drm_writeback.c | 18 --
   drivers/gpu/drm/vc4/vc4_txp.c   |  4 ++--
   include/drm/drm_writeback.h | 22 --
   3 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_writeback.c
b/drivers/gpu/drm/drm_writeback.c
index 92658ad..0538674 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -180,21 +180,21 @@ int drm_writeback_connector_init(struct
drm_device *dev,
   {
   int ret = 0;
-drm_encoder_helper_add(_connector->encoder, enc_helper_funcs);
+drm_encoder_helper_add(_connector->internal_encoder, enc_helper_funcs);
-wb_connector->encoder.possible_crtcs = possible_crtcs;
+wb_connector->internal_encoder.possible_crtcs = possible_crtcs;
-ret = drm_encoder_init(dev, _connector->encoder,
+ret = drm_encoder_init(dev, _connector->internal_encoder,
  _writeback_encoder_funcs,
  DRM_MODE_ENCODER_VIRTUAL, NULL);
   if (ret)
   return ret;
-ret = drm_writeback_connector_init_with_encoder(dev, wb_connector, 
_connector->encoder,
-con_funcs, formats, n_formats);
+ret = drm_writeback_connector_init_with_encoder(dev, wb_connector,
+

[Freedreno] [PATCH] drm/msm: fix returnvar.cocci warning

2022-04-25 Thread Guo Zhengkui

Fix the following coccicheck warning:
drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c:24:5-8: Unneeded variable:
"ret". Return "0" on line 75.

Signed-off-by: Guo Zhengkui 
---
 drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c 
b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
index 3cf476c55158..8ae66facc095 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
@@ -21,7 +21,6 @@ static int mdp4_hw_init(struct msm_kms *kms)
struct drm_device *dev = mdp4_kms->dev;
u32 dmap_cfg, vg_cfg;
unsigned long clk;
-   int ret = 0;
 
pm_runtime_get_sync(dev->dev);
 
@@ -72,7 +71,7 @@ static int mdp4_hw_init(struct msm_kms *kms)
 
pm_runtime_put_sync(dev->dev);
 
-   return ret;
+   return 0;
 }
 
 static void mdp4_enable_commit(struct msm_kms *kms)
-- 
2.20.1

[Freedreno] [PATCH v10 4/4] drm/msm/dp: Support the eDP modes given by panel

2022-04-25 Thread Sankeerth Billakanti

The eDP controller does not have a reliable way keep panel
powered on to read the sink capabilities. So, the controller
driver cannot validate if a mode can be supported by the
source. We will rely on the panel driver to populate only
the supported modes for now.

Signed-off-by: Sankeerth Billakanti 
Reviewed-by: Douglas Anderson 
Reviewed-by: Stephen Boyd 
---
Changes in v10:
  - none 

Changes in v9:
  - none

Changes in v8:
  - add the drm/msm/dp tag in the commit title

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

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c
index fd1dddb9..637fb63 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -998,6 +998,14 @@ enum drm_mode_status dp_bridge_mode_valid(struct 
drm_bridge *bridge,
return -EINVAL;
}
 
+   /*
+* The eDP controller currently does not have a reliable way of
+* enabling panel power to read sink capabilities. So, we rely
+* on the panel driver to populate only supported modes for now.
+*/
+   if (dp->is_edp)
+   return MODE_OK;
+
if ((dp->max_pclk_khz <= 0) ||
(dp->max_pclk_khz > DP_MAX_PIXEL_CLK_KHZ) ||
(mode->clock > dp->max_pclk_khz))
-- 
2.7.4

[Freedreno] [PATCH v10 3/4] drm/msm/dp: wait for hpd high before aux transaction

2022-04-25 Thread Sankeerth Billakanti

The source device should ensure the sink is ready before proceeding to
read the sink capability or perform any aux transactions. The sink
will indicate its readiness by asserting the HPD line. The controller
driver needs to wait for the hpd line to be asserted by the sink before
it performs any aux transactions.

The eDP sink is assumed to be always connected. It needs power from the
source and its HPD line will be asserted only after the panel is powered
on. The panel power will be enabled from the panel-edp driver and only
after that, the hpd line will be asserted.

Whereas for DP, the sink can be hotplugged and unplugged anytime. The hpd
line gets asserted to indicate the sink is connected and ready. Hence
there is no need to wait for the hpd line to be asserted for a DP sink.

Signed-off-by: Sankeerth Billakanti 
Reviewed-by: Douglas Anderson 
Reviewed-by: Stephen Boyd 
---
These changes may be handled in is_hpd_asserted when
https://lore.kernel.org/r/20220408193536.RFC.3.Icf57bb12233a47727013c6ab69eebf803e22ebc1@changeid/
is accepted upstream.

Changes in v10:
  - none

Changes in v9:
  - none

Changes in v8:
  - correct the indentation

Changes in v7:
  - add a comment to say why the wait is done for eDP
  - correct the commit text

Changes in v6:
  - Wait for hpd high only for eDP
  - Split into smaller patches

 drivers/gpu/drm/msm/dp/dp_aux.c | 21 -
 drivers/gpu/drm/msm/dp/dp_aux.h |  3 ++-
 drivers/gpu/drm/msm/dp/dp_catalog.c | 13 +
 drivers/gpu/drm/msm/dp/dp_catalog.h |  1 +
 drivers/gpu/drm/msm/dp/dp_display.c |  2 +-
 5 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index 6d36f63..d030a93 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -34,6 +34,7 @@ struct dp_aux_private {
bool no_send_addr;
bool no_send_stop;
bool initted;
+   bool is_edp;
u32 offset;
u32 segment;
 
@@ -337,6 +338,22 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
goto exit;
}
 
+   /*
+* For eDP it's important to give a reasonably long wait here for HPD
+* to be asserted. This is because the panel driver may have _just_
+* turned on the panel and then tried to do an AUX transfer. The panel
+* driver has no way of knowing when the panel is ready, so it's up
+* to us to wait. For DP we never get into this situation so let's
+* avoid ever doing the extra long wait for DP.
+*/
+   if (aux->is_edp) {
+   ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog);
+   if (ret) {
+   DRM_DEBUG_DP("Panel not ready for aux transactions\n");
+   goto exit;
+   }
+   }
+
dp_aux_update_offset_and_segment(aux, msg);
dp_aux_transfer_helper(aux, msg, true);
 
@@ -491,7 +508,8 @@ void dp_aux_unregister(struct drm_dp_aux *dp_aux)
drm_dp_aux_unregister(dp_aux);
 }
 
-struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog)
+struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog,
+ bool is_edp)
 {
struct dp_aux_private *aux;
 
@@ -506,6 +524,7 @@ struct drm_dp_aux *dp_aux_get(struct device *dev, struct 
dp_catalog *catalog)
 
init_completion(>comp);
aux->cmd_busy = false;
+   aux->is_edp = is_edp;
mutex_init(>mutex);
 
aux->dev = dev;
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.h b/drivers/gpu/drm/msm/dp/dp_aux.h
index 82afc8d..5a50c08 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.h
+++ b/drivers/gpu/drm/msm/dp/dp_aux.h
@@ -16,7 +16,8 @@ void dp_aux_init(struct drm_dp_aux *dp_aux);
 void dp_aux_deinit(struct drm_dp_aux *dp_aux);
 void dp_aux_reconfig(struct drm_dp_aux *dp_aux);
 
-struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog);
+struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog,
+ bool is_edp);
 void dp_aux_put(struct drm_dp_aux *aux);
 
 #endif /*__DP_AUX_H_*/
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c 
b/drivers/gpu/drm/msm/dp/dp_catalog.c
index df9670d..0c6a96e 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -242,6 +242,19 @@ void dp_catalog_aux_update_cfg(struct dp_catalog 
*dp_catalog)
phy_calibrate(phy);
 }
 
+int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog)
+{
+   u32 state;
+   struct dp_catalog_private *catalog = container_of(dp_catalog,
+   struct dp_catalog_private, dp_catalog);
+
+   /* poll for hpd connected status every 2ms and timeout after 500ms */
+   return readl_poll_timeout(catalog->io->dp_controller.aux.base +
+   REG_DP_DP_HPD_INT_STATUS,
+

[Freedreno] [PATCH v10 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP

2022-04-25 Thread Sankeerth Billakanti

The panel-edp enables the eDP panel power during probe, get_modes
and pre-enable. The eDP connect and disconnect interrupts for the eDP/DP
controller are directly dependent on panel power. As eDP display can be
assumed as always connected, the controller driver can skip the eDP
connect and disconnect interrupts. Any disruption in the link status
will be indicated via the IRQ_HPD interrupts.

So, the eDP controller driver can just enable the IRQ_HPD and replug
interrupts. The DP controller driver still needs to enable all the
interrupts.

Signed-off-by: Sankeerth Billakanti 
Reviewed-by: Douglas Anderson 
Reviewed-by: Stephen Boyd 
---
Changes in v10:
  - none

Changes in v9:
  - add comment explaining the interrupt status register

Changes in v8:
  - add comment explaining the interrupt status return

Changes in v7:
  - reordered the patch in the series
  - modified the return statement for isr
  - connector check modified to just check for eDP

 drivers/gpu/drm/msm/dp/dp_catalog.c | 16 ++--
 drivers/gpu/drm/msm/dp/dp_display.c | 22 +-
 2 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c 
b/drivers/gpu/drm/msm/dp/dp_catalog.c
index fac815f..df9670d 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -569,10 +569,6 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog 
*dp_catalog)
 
u32 reftimer = dp_read_aux(catalog, REG_DP_DP_HPD_REFTIMER);
 
-   /* enable HPD plug and unplug interrupts */
-   dp_catalog_hpd_config_intr(dp_catalog,
-   DP_DP_HPD_PLUG_INT_MASK | DP_DP_HPD_UNPLUG_INT_MASK, true);
-
/* Configure REFTIMER and enable it */
reftimer |= DP_DP_HPD_REFTIMER_ENABLE;
dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer);
@@ -599,13 +595,21 @@ u32 dp_catalog_hpd_get_intr_status(struct dp_catalog 
*dp_catalog)
 {
struct dp_catalog_private *catalog = container_of(dp_catalog,
struct dp_catalog_private, dp_catalog);
-   int isr = 0;
+   int isr, mask;
 
isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS);
dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK,
 (isr & DP_DP_HPD_INT_MASK));
+   mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK);
 
-   return isr;
+   /*
+* We only want to return interrupts that are unmasked to the caller.
+* However, the interrupt status field also contains other
+* informational bits about the HPD state status, so we only mask
+* out the part of the register that tells us about which interrupts
+* are pending.
+*/
+   return isr & (mask | ~DP_DP_HPD_INT_MASK);
 }
 
 int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c
index f772d84..d25fa1f 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -683,7 +683,8 @@ static int dp_hpd_unplug_handle(struct dp_display_private 
*dp, u32 data)
dp_display_handle_plugged_change(>dp_display, false);
 
/* enable HDP plug interrupt to prepare for next plugin */
-   dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true);
+   if (!dp->dp_display.is_edp)
+   dp_catalog_hpd_config_intr(dp->catalog, 
DP_DP_HPD_PLUG_INT_MASK, true);
 
DRM_DEBUG_DP("After, type=%d hpd_state=%d\n",
dp->dp_display.connector_type, state);
@@ -1096,6 +1097,13 @@ static void dp_display_config_hpd(struct 
dp_display_private *dp)
dp_display_host_init(dp);
dp_catalog_ctrl_hpd_config(dp->catalog);
 
+   /* Enable plug and unplug interrupts only for external DisplayPort */
+   if (!dp->dp_display.is_edp)
+   dp_catalog_hpd_config_intr(dp->catalog,
+   DP_DP_HPD_PLUG_INT_MASK |
+   DP_DP_HPD_UNPLUG_INT_MASK,
+   true);
+
/* Enable interrupt first time
 * we are leaving dp clocks on during disconnect
 * and never disable interrupt
@@ -1381,6 +1389,12 @@ static int dp_pm_resume(struct device *dev)
dp_catalog_ctrl_hpd_config(dp->catalog);
 
 
+   if (!dp->dp_display.is_edp)
+   dp_catalog_hpd_config_intr(dp->catalog,
+   DP_DP_HPD_PLUG_INT_MASK |
+   DP_DP_HPD_UNPLUG_INT_MASK,
+   true);
+
if (dp_catalog_link_is_connected(dp->catalog)) {
/*
 * set sink to normal operation mode -- D0
@@ -1658,6 +1672,9 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge)
return;
}
 
+   if (dp->is_edp)
+   dp_hpd_plug_handle(dp_display, 0);
+
mutex_lock(_display->event_mutex);
 
/* stop sentinel checking

[Freedreno] [PATCH v10 1/4] drm/msm/dp: Add eDP support via aux_bus

2022-04-25 Thread Sankeerth Billakanti

This patch adds support for generic eDP sink through aux_bus. The eDP/DP
controller driver should support aux transactions originating from the
panel-edp driver and hence should be initialized and ready.

The panel bridge supporting the panel should be ready before the bridge
connector is initialized. The generic panel probe needs the controller
resources to be enabled to support the aux transactions originating from
the panel probe.

Signed-off-by: Sankeerth Billakanti 
Reviewed-by: Douglas Anderson 
Reviewed-by: Stephen Boyd 
---
Changes in v10:
  - modify the error handling condition
  - modify the kernel doc

Changes in v9:
  - add comments for panel probe
  - modify the error handling checks

Changes in v8:
  - handle corner cases
  - add comment for the bridge ops

Changes in v7:
  - aux_bus is mandatory for eDP
  - connector type check modified to just check for eDP

Changes in v6:
  - Remove initialization
  - Fix aux_bus node leak
  - Split the patches

 drivers/gpu/drm/msm/dp/dp_display.c | 72 ++---
 drivers/gpu/drm/msm/dp/dp_display.h |  1 +
 drivers/gpu/drm/msm/dp/dp_drm.c | 21 ---
 drivers/gpu/drm/msm/dp/dp_parser.c  | 23 ++--
 drivers/gpu/drm/msm/dp/dp_parser.h  | 14 +++-
 5 files changed, 101 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c
index d7a19d6..f772d84 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -10,6 +10,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "msm_drv.h"
 #include "msm_kms.h"
@@ -259,14 +260,12 @@ static int dp_display_bind(struct device *dev, struct 
device *master,
dp->dp_display.drm_dev = drm;
priv->dp[dp->id] = >dp_display;
 
-   rc = dp->parser->parse(dp->parser, dp->dp_display.connector_type);
+   rc = dp->parser->parse(dp->parser);
if (rc) {
DRM_ERROR("device tree parsing failed\n");
goto end;
}
 
-   dp->dp_display.next_bridge = dp->parser->next_bridge;
-
dp->aux->drm_dev = drm;
rc = dp_aux_register(dp->aux);
if (rc) {
@@ -1319,6 +1318,8 @@ static int dp_display_probe(struct platform_device *pdev)
dp->pdev = pdev;
dp->name = "drm_dp";
dp->dp_display.connector_type = desc->connector_type;
+   dp->dp_display.is_edp =
+   (dp->dp_display.connector_type == DRM_MODE_CONNECTOR_eDP);
 
rc = dp_init_sub_modules(dp);
if (rc) {
@@ -1508,7 +1509,8 @@ 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_display->is_edp)
+   dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
 }
 
 void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
@@ -1530,6 +1532,64 @@ void msm_dp_debugfs_init(struct msm_dp *dp_display, 
struct drm_minor *minor)
}
 }
 
+static int dp_display_get_next_bridge(struct msm_dp *dp)
+{
+   int rc;
+   struct dp_display_private *dp_priv;
+   struct device_node *aux_bus;
+   struct device *dev;
+
+   dp_priv = container_of(dp, struct dp_display_private, dp_display);
+   dev = _priv->pdev->dev;
+   aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
+
+   if (aux_bus && dp->is_edp) {
+   dp_display_host_init(dp_priv);
+   dp_catalog_ctrl_hpd_config(dp_priv->catalog);
+   dp_display_host_phy_init(dp_priv);
+   enable_irq(dp_priv->irq);
+
+   /*
+* The code below assumes that the panel will finish probing
+* by the time devm_of_dp_aux_populate_ep_devices() returns.
+* This isn't a great assumption since it will fail if the
+* panel driver is probed asynchronously but is the best we
+* can do without a bigger driver reorganization.
+*/
+   rc = devm_of_dp_aux_populate_ep_devices(dp_priv->aux);
+   of_node_put(aux_bus);
+   if (rc)
+   goto error;
+   } else if (dp->is_edp) {
+   DRM_ERROR("eDP aux_bus not found\n");
+   return -ENODEV;
+   }
+
+   /*
+* External bridges are mandatory for eDP interfaces: one has to
+* provide at least an eDP panel (which gets wrapped into panel-bridge).
+*
+* For DisplayPort interfaces external bridges are optional, so
+* silently ignore an error if one is not present (-ENODEV).
+*/
+   rc = dp_parser_find_next_bridge(dp_priv->parser);
+   if (!dp->is_edp && rc == -ENODEV)
+   return 0;
+
+   if (!rc) {
+   dp->next_bridge = dp_priv->parser->next_bridge;
+   return 0;
+   }
+
+error:
+   if (dp->is_edp) {
+   disable_irq(dp_priv->irq);
+

[Freedreno] [PATCH v10 0/4] Add support for the eDP panel over aux_bus

2022-04-25 Thread Sankeerth Billakanti

This series adds support for generic eDP panel over aux_bus.

These changes are dependent on the following patches:
https://patchwork.kernel.org/project/linux-arm-msm/patch/20220211224006.1797846-5-dmitry.barysh...@linaro.org/
https://patchwork.kernel.org/project/linux-arm-msm/patch/20220211224006.1797846-6-dmitry.barysh...@linaro.org/

Sankeerth Billakanti (4):
  drm/msm/dp: Add eDP support via aux_bus
  drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP
  drm/msm/dp: wait for hpd high before aux transaction
  drm/msm/dp: Support the eDP modes given by panel

 drivers/gpu/drm/msm/dp/dp_aux.c |  21 +++-
 drivers/gpu/drm/msm/dp/dp_aux.h |   3 +-
 drivers/gpu/drm/msm/dp/dp_catalog.c |  29 +++---
 drivers/gpu/drm/msm/dp/dp_catalog.h |   1 +
 drivers/gpu/drm/msm/dp/dp_display.c | 104 +---
 drivers/gpu/drm/msm/dp/dp_display.h |   1 +
 drivers/gpu/drm/msm/dp/dp_drm.c |  21 ++--
 drivers/gpu/drm/msm/dp/dp_parser.c  |  23 +---
 drivers/gpu/drm/msm/dp/dp_parser.h  |  14 -
 9 files changed, 177 insertions(+), 40 deletions(-)

-- 
2.7.4

Re: [Freedreno] [PATCH v4 03/20] drm: allow real encoder to be passed for drm_writeback_connector

2022-04-25 Thread Dmitry Baryshkov

On Sun, 24 Apr 2022 at 22:59, Laurent Pinchart
 wrote:
>
> Hi Abhinav,
>
> On Sun, Apr 24, 2022 at 11:23:20AM -0700, Abhinav Kumar wrote:
> > On 4/24/2022 11:12 AM, Abhinav Kumar wrote:
> > > On 4/24/2022 7:50 AM, Laurent Pinchart wrote:
> > >> On Fri, Apr 22, 2022 at 04:06:38PM -0700, Abhinav Kumar wrote:
> > >>> For some vendor driver implementations, display hardware can
> > >>> be shared between the encoder used for writeback and the physical
> > >>> display.
> > >>>
> > >>> In addition resources such as clocks and interrupts can
> > >>> also be shared between writeback and the real encoder.
> > >>>
> > >>> To accommodate such vendor drivers and hardware, allow
> > >>> real encoder to be passed for drm_writeback_connector.
> > >>>
> > >>> For existing clients, drm_writeback_connector_init() will use
> > >>> an internal_encoder under the hood and hence no changes will
> > >>> be needed.
> > >>>
> > >>> changes in v7:
> > >>> - move this change before the vc4 change in the series
> > >>>   to minimize the changes to vendor drivers in drm core
> > >>>   changes
> > >>
> > >> Why is this needed ? The drm_writeback_connector functions don't need
> > >> the drm_encoder after drm_writeback_connector_init() (or
> > >> drm_writeback_connector_init_with_encoder()) returns.
> > >>
> > >
> > > Sorry I didnt follow this comment. This change log is incorrect, so
> > > after changing the previous change in the series and modifying this, no
> > > further changes are needed to vc4, so I decided to drop the next change.
> > > So this change log is incorrect. I can remove this.
> > >
> > > Is that what you meant?
> >
> > So till the previous change, the only user of
> > drm_writeback_connector_init_with_encoder() was
> > drm_writeback_connector_init() which was still passing its own
> > internal_encoder.
> >
> > Only if the wb_connector->encoder is changed to a pointer, other vendor
> > drivers can pass their own encoder to
> > drm_writeback_connector_init_with_encoder().
> >
> > Hence you are right that drm_writeback_connector functions do not need
> > drm_encoder after init() returns, but till this change is done, other
> > vendor drivers cannot directly call
> > drm_writeback_connector_init_with_encoder() because the encoder will not
> > be valid till then.
>
> Users of drm_writeback_connector_init_with_encoder() handle the encoder
> themselves, they can simply ignore drm_writeback_connector.encoder. The
> documentation of the encoder field needs to be updated though (I'd do so
> in the previous patch), to clearly mention that the field is valid only
> when using drm_writeback_connector_init(), not when calling
> drm_writeback_connector_init_with_encoder().

If we allow it to be unitialized, it might end with hard-to-trace
bugs, additional conditions, etc.
In my opnion we should:
 - either make drm_writeback_connector.encoder a valid pointer
 - or drop the field completely.

And up to some point I'd vote for the second option. The code using
internal_encoder can continue using it directly. The code using
drm_writeback_connector_init_with_encoder() will manage encoder on
their own. We will loose a single entry point for wb's encoder, but do
we really need it? (Frankly speaking I didn't check.)

>
> > Hope this clarifies it.
> >
> > >>> Signed-off-by: Abhinav Kumar 
> > >>> Reviewed-by: Dmitry Baryshkov 
> > >>> ---
> > >>>   drivers/gpu/drm/drm_writeback.c | 18 --
> > >>>   drivers/gpu/drm/vc4/vc4_txp.c   |  4 ++--
> > >>>   include/drm/drm_writeback.h | 22 --
> > >>>   3 files changed, 34 insertions(+), 10 deletions(-)
> > >>>
> > >>> diff --git a/drivers/gpu/drm/drm_writeback.c
> > >>> b/drivers/gpu/drm/drm_writeback.c
> > >>> index 92658ad..0538674 100644
> > >>> --- a/drivers/gpu/drm/drm_writeback.c
> > >>> +++ b/drivers/gpu/drm/drm_writeback.c
> > >>> @@ -180,21 +180,21 @@ int drm_writeback_connector_init(struct
> > >>> drm_device *dev,
> > >>>   {
> > >>>   int ret = 0;
> > >>> -drm_encoder_helper_add(_connector->encoder, enc_helper_funcs);
> > >>> +drm_encoder_helper_add(_connector->internal_encoder, 
> > >>> enc_helper_funcs);
> > >>> -wb_connector->encoder.possible_crtcs = possible_crtcs;
> > >>> +wb_connector->internal_encoder.possible_crtcs = possible_crtcs;
> > >>> -ret = drm_encoder_init(dev, _connector->encoder,
> > >>> +ret = drm_encoder_init(dev, _connector->internal_encoder,
> > >>>  _writeback_encoder_funcs,
> > >>>  DRM_MODE_ENCODER_VIRTUAL, NULL);
> > >>>   if (ret)
> > >>>   return ret;
> > >>> -ret = drm_writeback_connector_init_with_encoder(dev, wb_connector, 
> > >>> _connector->encoder,
> > >>> -con_funcs, formats, n_formats);
> > >>> +ret = drm_writeback_connector_init_with_encoder(dev, wb_connector,
> > >>> +_connector->internal_encoder, con_funcs, formats, 
> > >>> n_formats);
> > >>>   if (ret)
> > >>> -

Re: [Freedreno] [PATCH v9 1/4] drm/msm/dp: Add eDP support via aux_bus

2022-04-25 Thread Sankeerth Billakanti (QUIC)

Hi Stephen,

>Quoting Sankeerth Billakanti (2022-04-22 02:11:03)
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>> b/drivers/gpu/drm/msm/dp/dp_display.c
>> index d7a19d6..055681a 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>
>Some nitpicks
>
>Reviewed-by: Stephen Boyd 
>
>> @@ -1508,7 +1509,8 @@ 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_display->is_edp)
>> +   dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
>
>Did it turn out that in fact DP isn't ready still to setup even after delaying 
>the
>irq?
>

The host_init, config_hpd, phy_init and enable_irq are happening in 
modeset_init already for eDP.
So, I am not scheduling the EV_HPD_INIT_SETUP event for eDP. I am not modifying 
the delay for DP.

>>  }
>>
>>  void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor
>> *minor) @@ -1530,6 +1532,65 @@ void msm_dp_debugfs_init(struct
>msm_dp *dp_display, struct drm_minor *minor)
>> }
>>  }
>>
>> +static int dp_display_get_next_bridge(struct msm_dp *dp) {
>> +   int rc;
>> +   struct dp_display_private *dp_priv;
>> +   struct device_node *aux_bus;
>> +   struct device *dev;
>> +
>> +   dp_priv = container_of(dp, struct dp_display_private, dp_display);
>> +   dev = _priv->pdev->dev;
>> +   aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
>> +
>> +   if (aux_bus && dp->is_edp) {
>> +   dp_display_host_init(dp_priv);
>> +   dp_catalog_ctrl_hpd_config(dp_priv->catalog);
>> +   dp_display_host_phy_init(dp_priv);
>> +   enable_irq(dp_priv->irq);
>> +
>> +   /*
>> +* The code below assumes that the panel will finish probing
>> +* by the time devm_of_dp_aux_populate_ep_devices() returns.
>> +* This isn't a great assumption since it will fail if the
>> +* panel driver is probed asynchronously but is the best we
>> +* can do without a bigger driver reorganization.
>> +*/
>> +   rc = devm_of_dp_aux_populate_ep_devices(dp_priv->aux);
>> +   of_node_put(aux_bus);
>> +   if (rc)
>> +   goto error;
>> +   } else if (dp->is_edp) {
>> +   DRM_ERROR("eDP aux_bus not found\n");
>> +   return -ENODEV;
>> +   }
>> +
>> +   /*
>> +* External bridges are mandatory for eDP interfaces: one has to
>> +* provide at least an eDP panel (which gets wrapped into panel-
>bridge).
>> +*
>> +* For DisplayPort interfaces external bridges are optional, so
>> +* silently ignore an error if one is not present (-ENODEV).
>> +*/
>> +   rc = dp_parser_find_next_bridge(dp_priv->parser);
>> +   if (!dp->is_edp && rc == -ENODEV)
>> +   return 0;
>> +   else if (rc)
>
>Just an if because there's a return above.
>

Okay

>> +   goto error;
>> +
>> +   dp->next_bridge = dp_priv->parser->next_bridge;
>
>In which case it almost feels like it could be written
>
>   if (!dp->is_edp && rc == -ENODEV)
>   return 0;
>   if (!rc) {
>   dp->next_bridge = dp_priv->parser->next_bridge;
>   return 0;
>   }
>error:
>   if (dp->is_edp) {
>
>but I'm not worried either way, besides removing the else from the else-if.
>

Okay

>> +
>> +   return 0;
>> +
>> +error:
>> +   if (dp->is_edp) {
>> +   disable_irq(dp_priv->irq);
>> +   dp_display_host_phy_exit(dp_priv);
>> +   dp_display_host_deinit(dp_priv);
>> +   }
>> +   return rc;
>> +}
>> +
>>  int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device
>*dev,
>> struct drm_encoder *encoder)  { diff --git
>> a/drivers/gpu/drm/msm/dp/dp_parser.h
>> b/drivers/gpu/drm/msm/dp/dp_parser.h
>> index d371bae..950416c 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_parser.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_parser.h
>> @@ -125,7 +125,7 @@ struct dp_parser {
>> u32 max_dp_lanes;
>> struct drm_bridge *next_bridge;
>>
>> -   int (*parse)(struct dp_parser *parser, int connector_type);
>> +   int (*parse)(struct dp_parser *parser);
>>  };
>>
>>  /**
>> @@ -141,4 +141,15 @@ struct dp_parser {
>>   */
>>  struct dp_parser *dp_parser_get(struct platform_device *pdev);
>>
>> +/**
>> + * dp_parser_find_next_bridge() - find an additional bridge to DP
>> + *
>> + * @parser: dp_parser data from client
>> + * return: 0 if able to get the bridge else return an error code
>
>return comes after the description below. Also should be capitalized.
>You can check this by compiling with W=1 I believe, or run the kernel doc
>format script on the file..
>

Okay

>> + *
>> + * This function is used to find any

[Freedreno] [PATCH] drm/msm/hdmi: fix error check return value of irq_of_parse_and_map()

2022-04-25 Thread cgel . zte

From: Lv Ruyi 

The irq_of_parse_and_map() function returns 0 on failure, and does not
return a negative value anyhow, so never enter this conditional branch.

Fixes: f6a8eaca0ea1 ("drm/msm/mdp5: use irqdomains")
Reported-by: Zeal Robot 
Signed-off-by: Lv Ruyi 
---
 drivers/gpu/drm/msm/hdmi/hdmi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index ec324352e862..c3b661c2932d 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -298,9 +298,9 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
drm_connector_attach_encoder(hdmi->connector, hdmi->encoder);
 
hdmi->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
-   if (hdmi->irq < 0) {
-   ret = hdmi->irq;
-   DRM_DEV_ERROR(dev->dev, "failed to get irq: %d\n", ret);
+   if (!hdmi->irq) {
+   ret = -EINVAL;
+   DRM_DEV_ERROR(dev->dev, "failed to get irq\n");
goto fail;
}
 
-- 
2.25.1

[Freedreno] [PATCH] drm/msm/dpu: fix error check return value of irq_of_parse_and_map()

2022-04-25 Thread cgel . zte

From: Lv Ruyi 

The irq_of_parse_and_map() function returns 0 on failure, and does not
return a negative value anyhow, so never enter this conditional branch.

Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
Reported-by: Zeal Robot 
Signed-off-by: Lv Ruyi 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index e29796c4f27b..36eeeae7fe45 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -1187,9 +1187,9 @@ struct msm_kms *dpu_kms_init(struct drm_device *dev)
dpu_kms = to_dpu_kms(priv->kms);
 
irq = irq_of_parse_and_map(dpu_kms->pdev->dev.of_node, 0);
-   if (irq < 0) {
-   DPU_ERROR("failed to get irq: %d\n", irq);
-   return ERR_PTR(irq);
+   if (!irq) {
+   DPU_ERROR("failed to get irq\n");
+   return ERR_PTR(-EINVAL);
}
dpu_kms->base.irq = irq;
 
-- 
2.25.1

61 matches

Mail list logo