Re: [Freedreno] [v2] drm/msm/disp/dpu1: add safe lut config in dpu driver
On 2021-08-04 01:43, Stephen Boyd wrote:
Quoting Kalyan Thota (2021-08-03 03:41:47)
Add safe lut configuration for all the targets in dpu
driver as per QOS recommendation.
Issue reported on SC7280:
With wait-for-safe feature in smmu enabled, RT client
buffer levels are checked to be safe before smmu invalidation.
Since display was always set to unsafe it was delaying the
invalidaiton process thus impacting the performance on NRT clients
such as eMMC and NVMe.
Validated this change on SC7280, With this change eMMC performance
has improved significantly.
Changes in v1:
- Add fixes tag (Sai)
- CC stable kernel (Dimtry)
Fixes: cfacf946a464d4(drm/msm/disp/dpu1: add support for display for
SC7280 target)
This is wrong format and commit hash
My bad, i'll fix it right away
- KT
Fixes: 591e34a091d1 ("drm/msm/disp/dpu1: add support for display for
SC7280 target")
Signed-off-by: Kalyan Thota
Tested-by: Sai Prakash Ranjan
(sc7280, sc7180)
Re: [Freedreno] [v1] drm/msm/disp/dpu1: fix warn stack reported during dpu resume
On 2021-04-01 19:01, Dmitry Baryshkov wrote:
On Thu, 1 Apr 2021 at 16:19, wrote:
On 2021-04-01 07:37, Dmitry Baryshkov wrote:
> On 01/04/2021 01:47, Rob Clark wrote:
>> On Wed, Mar 31, 2021 at 9:03 AM Dmitry Baryshkov
>> wrote:
>>>
>>> On 31/03/2021 14:27, Kalyan Thota wrote:
WARN_ON was introduced by the below commit to catch runtime resumes
that are getting triggered before icc path was set.
"drm/msm/disp/dpu1: icc path needs to be set before dpu runtime
resume"
For the targets where the bw scaling is not enabled, this WARN_ON is
a false alarm. Fix the WARN condition appropriately.
>>>
>>> Should we change all DPU targets to use bw scaling to the mdp from
>>> the
>>> mdss nodes? The limitation to sc7180 looks artificial.
>>
>> yes, we should, this keeps biting us on 845
>
> Done,
>
https://lore.kernel.org/linux-arm-msm/20210401020533.3956787-2-dmitry.barysh...@linaro.org/
Hi Dmitry,
https://lore.kernel.org/linux-arm-msm/20210401020533.3956787-2-dmitry.barysh...@linaro.org/
you need to add clk_inefficiency_factor, bw_inefficiency_factor in the
catalogue for the new
targets where bw scaling is being enabled. please reuse sc7180 values.
Done in patch 1 in that series.
got it.
secondly, the AXI clock needs to be moved from mdss to mdp device like
as in sc7180 dt if its not done already.
Is this enough:
sm8250 has < GCC_DISP_HF_AXI_CLK> both in mdss and mdp nodes
sdm845 has < GCC_DISP_AXI_CLK> in mdss node and <
DISP_CC_MDSS_AXI_CLK> in the mdp node.
Since there is no BW vote in mdss, we need to move the AXI clock ON to
mdp node.
for sm8250
remove GCC_DISP_HF_AXI_CLK from mdss device and add it in mdp device.
for sdm845
remove GCC_DISP_AXI_CLK from mdss device and add it in mdp device before
we turn on DISP_CC_MDSS_AXI_CLK, i.e as first item.
lastly, if you are planning to remove the static votes from dpu_mdss,
do
you also want to move the
interconnect paths from mdss device to mdp device in the dt ?
I have no strong opinion on this. So far I did not change dt to be
compatible with the current device trees.
Thanks,
Kalyan
>
>>
Reported-by: Steev Klimaszewski
>>
>> Please add Fixes: tag as well
Adding Fixes tag above my sign-off, should i push another version or
can
it be picked from here ?
Fixes: 627dc55c273d ("drm/msm/disp/dpu1: icc path needs to be set
before
dpu runtime resume")
>>
Signed-off-by: Kalyan Thota
---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 8 +---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 9 +
drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c | 11 ++-
3 files changed, 20 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index cab387f..0071a4d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -294,6 +294,9 @@ static int
dpu_kms_parse_data_bus_icc_path(struct dpu_kms *dpu_kms)
struct icc_path *path1;
struct drm_device *dev = dpu_kms->dev;
+ if (!dpu_supports_bw_scaling(dev))
+ return 0;
+
path0 = of_icc_get(dev->dev, "mdp0-mem");
path1 = of_icc_get(dev->dev, "mdp1-mem");
@@ -934,8 +937,7 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
DPU_DEBUG("REG_DMA is not defined");
}
- if (of_device_is_compatible(dev->dev->of_node,
"qcom,sc7180-mdss"))
- dpu_kms_parse_data_bus_icc_path(dpu_kms);
+ dpu_kms_parse_data_bus_icc_path(dpu_kms);
pm_runtime_get_sync(_kms->pdev->dev);
@@ -1198,7 +1200,7 @@ static int __maybe_unused
dpu_runtime_resume(struct device *dev)
ddev = dpu_kms->dev;
- WARN_ON(!(dpu_kms->num_paths));
+ WARN_ON((dpu_supports_bw_scaling(ddev) &&
!dpu_kms->num_paths));
/* Min vote of BW is required before turning on AXI clk */
for (i = 0; i < dpu_kms->num_paths; i++)
icc_set_bw(dpu_kms->path[i], 0,
Bps_to_icc(MIN_IB_BW));
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
index d6717d6..f7bcc0a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
@@ -154,6 +154,15 @@ struct vsync_info {
#define to_dpu_global_state(x) container_of(x, struct
dpu_global_state, base)
+/**
+ * dpu_supports_bw_scaling: returns true for drivers that support
bw scaling.
+ * @dev: Pointer to drm_device structure
+ */
+static inline int dpu_supports_bw_scaling(struct drm_device *dev)
+{
+ return of_device_is_compatible(dev->dev->of_node,
"qcom,sc7180-mdss");
+}
+
/* Global private object state for tracking
Re: [Freedreno] [v1] drm/msm/disp/dpu1: fix warn stack reported during dpu resume
On 2021-04-01 07:37, Dmitry Baryshkov wrote:
On 01/04/2021 01:47, Rob Clark wrote:
On Wed, Mar 31, 2021 at 9:03 AM Dmitry Baryshkov
wrote:
On 31/03/2021 14:27, Kalyan Thota wrote:
WARN_ON was introduced by the below commit to catch runtime resumes
that are getting triggered before icc path was set.
"drm/msm/disp/dpu1: icc path needs to be set before dpu runtime
resume"
For the targets where the bw scaling is not enabled, this WARN_ON is
a false alarm. Fix the WARN condition appropriately.
Should we change all DPU targets to use bw scaling to the mdp from
the
mdss nodes? The limitation to sc7180 looks artificial.
yes, we should, this keeps biting us on 845
Done,
https://lore.kernel.org/linux-arm-msm/20210401020533.3956787-2-dmitry.barysh...@linaro.org/
Hi Dmitry,
https://lore.kernel.org/linux-arm-msm/20210401020533.3956787-2-dmitry.barysh...@linaro.org/
you need to add clk_inefficiency_factor, bw_inefficiency_factor in the
catalogue for the new
targets where bw scaling is being enabled. please reuse sc7180 values.
secondly, the AXI clock needs to be moved from mdss to mdp device like
as in sc7180 dt if its not done already.
lastly, if you are planning to remove the static votes from dpu_mdss, do
you also want to move the
interconnect paths from mdss device to mdp device in the dt ?
Thanks,
Kalyan
Reported-by: Steev Klimaszewski
Please add Fixes: tag as well
Adding Fixes tag above my sign-off, should i push another version or can
it be picked from here ?
Fixes: Id252b9c2887 ("drm/msm/disp/dpu1: icc path needs to be set before
dpu runtime resume")
Signed-off-by: Kalyan Thota
---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 8 +---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 9 +
drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c | 11 ++-
3 files changed, 20 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index cab387f..0071a4d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -294,6 +294,9 @@ static int
dpu_kms_parse_data_bus_icc_path(struct dpu_kms *dpu_kms)
struct icc_path *path1;
struct drm_device *dev = dpu_kms->dev;
+ if (!dpu_supports_bw_scaling(dev))
+ return 0;
+
path0 = of_icc_get(dev->dev, "mdp0-mem");
path1 = of_icc_get(dev->dev, "mdp1-mem");
@@ -934,8 +937,7 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
DPU_DEBUG("REG_DMA is not defined");
}
- if (of_device_is_compatible(dev->dev->of_node,
"qcom,sc7180-mdss"))
- dpu_kms_parse_data_bus_icc_path(dpu_kms);
+ dpu_kms_parse_data_bus_icc_path(dpu_kms);
pm_runtime_get_sync(_kms->pdev->dev);
@@ -1198,7 +1200,7 @@ static int __maybe_unused
dpu_runtime_resume(struct device *dev)
ddev = dpu_kms->dev;
- WARN_ON(!(dpu_kms->num_paths));
+ WARN_ON((dpu_supports_bw_scaling(ddev) &&
!dpu_kms->num_paths));
/* Min vote of BW is required before turning on AXI clk */
for (i = 0; i < dpu_kms->num_paths; i++)
icc_set_bw(dpu_kms->path[i], 0,
Bps_to_icc(MIN_IB_BW));
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
index d6717d6..f7bcc0a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
@@ -154,6 +154,15 @@ struct vsync_info {
#define to_dpu_global_state(x) container_of(x, struct
dpu_global_state, base)
+/**
+ * dpu_supports_bw_scaling: returns true for drivers that support
bw scaling.
+ * @dev: Pointer to drm_device structure
+ */
+static inline int dpu_supports_bw_scaling(struct drm_device *dev)
+{
+ return of_device_is_compatible(dev->dev->of_node,
"qcom,sc7180-mdss");
+}
+
/* Global private object state for tracking resources that are
shared across
* multiple kms objects (planes/crtcs/etc).
*/
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
index cd40788..8cd712c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
@@ -41,6 +41,9 @@ static int dpu_mdss_parse_data_bus_icc_path(struct
drm_device *dev,
struct icc_path *path0 = of_icc_get(dev->dev, "mdp0-mem");
struct icc_path *path1 = of_icc_get(dev->dev, "mdp1-mem");
+ if (dpu_supports_bw_scaling(dev))
+ return 0;
+
if (IS_ERR_OR_NULL(path0))
return PTR_ERR_OR_ZERO(path0);
@@ -276,11 +279,9 @@ int dpu_mdss_init(struct drm_device *dev)
DRM_DEBUG("mapped mdss address space @%pK\n",
dpu_mdss->mmio);
- if (!of_device_is_compatible(dev->dev->of_node,
"qcom,sc7180-mdss")) {
- ret = dpu_mdss_parse_data_bus_icc_path(dev, dpu_mdss);
- if (ret)
- return ret;
- }
+ ret = dpu_mdss_parse_data_bus_icc_path(dev, dpu_mdss);
+ if (ret)
+
Re: [Freedreno] [v1] drm/msm/disp/dpu1: icc path needs to be set before dpu runtime resume
On 2021-03-31 00:04, Steev Klimaszewski wrote:
On 3/22/21 4:17 AM, Kalyan Thota wrote:
From: Kalyan Thota
DPU runtime resume will request for a min vote on the AXI bus as
it is a necessary step before turning ON the AXI clock.
The change does below
1) Move the icc path set before requesting runtime get_sync.
2) remove the dependency of hw catalog for min ib vote
as it is initialized at a later point.
Signed-off-by: Kalyan Thota
---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index ed636f1..cab387f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -44,6 +44,8 @@
#define DPU_DEBUGFS_DIR "msm_dpu"
#define DPU_DEBUGFS_HWMASKNAME "hw_log_mask"
+#define MIN_IB_BW 4ULL /* Min ib vote 400MB */
+
static int dpu_kms_hw_init(struct msm_kms *kms);
static void _dpu_kms_mmu_destroy(struct dpu_kms *dpu_kms);
@@ -932,6 +934,9 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
DPU_DEBUG("REG_DMA is not defined");
}
+ if (of_device_is_compatible(dev->dev->of_node, "qcom,sc7180-mdss"))
+ dpu_kms_parse_data_bus_icc_path(dpu_kms);
+
pm_runtime_get_sync(_kms->pdev->dev);
dpu_kms->core_rev = readl_relaxed(dpu_kms->mmio + 0x0);
@@ -1037,9 +1042,6 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
dpu_vbif_init_memtypes(dpu_kms);
- if (of_device_is_compatible(dev->dev->of_node, "qcom,sc7180-mdss"))
- dpu_kms_parse_data_bus_icc_path(dpu_kms);
-
pm_runtime_put_sync(_kms->pdev->dev);
return 0;
@@ -1196,10 +1198,10 @@ static int __maybe_unused
dpu_runtime_resume(struct device *dev)
ddev = dpu_kms->dev;
+ WARN_ON(!(dpu_kms->num_paths));
/* Min vote of BW is required before turning on AXI clk */
for (i = 0; i < dpu_kms->num_paths; i++)
- icc_set_bw(dpu_kms->path[i], 0,
- dpu_kms->catalog->perf.min_dram_ib);
+ icc_set_bw(dpu_kms->path[i], 0, Bps_to_icc(MIN_IB_BW));
rc = msm_dss_enable_clk(mp->clk_config, mp->num_clk, true);
if (rc) {
With this patch now applied to 5.12-rc5, I am seeing the following when
booting the Lenovo Yoga C630 -
Mar 30 13:16:03 c630 kernel: [2.038491] [ cut here
]
Mar 30 13:16:03 c630 kernel: [2.038495] WARNING: CPU: 3 PID: 125
at drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c:1196
dpu_runtime_resume+0xc0/0xf0 [msm]
Mar 30 13:16:03 c630 kernel: [2.038551] Modules linked in:
ti_sn65dsi86 i2c_hid_of crct10dif_ce msm rtc_pm8xxx llcc_qcom ocmem
drm_kms_helper i2c_qcom_geni phy_qcom_qusb2 ipa(+) qcom_common
qcom_glink_smem qmi_helpers mdt_loader panel_simple drm pwm_bl
Mar 30 13:16:03 c630 kernel: [2.038599] CPU: 3 PID: 125 Comm:
kworker/3:1 Not tainted 5.12.0-rc5 #1
Mar 30 13:16:03 c630 kernel: [2.038605] Hardware name: LENOVO
81JL/LNVNB161216, BIOS 9UCN33WW(V2.06) 06/ 4/2019
Mar 30 13:16:03 c630 kernel: [2.038610] Workqueue: events
deferred_probe_work_func
Mar 30 13:16:03 c630 kernel: [2.038621] pstate: 6045 (nZCv
daif +PAN -UAO -TCO BTYPE=--)
Mar 30 13:16:03 c630 kernel: [2.038627] pc :
dpu_runtime_resume+0xc0/0xf0 [msm]
Mar 30 13:16:03 c630 kernel: [2.038674] lr :
pm_generic_runtime_resume+0x30/0x50
Mar 30 13:16:03 c630 kernel: [2.038683] sp : 800010b9b7e0
Mar 30 13:16:03 c630 kernel: [2.038685] x29: 800010b9b7e0 x28:
Mar 30 13:16:03 c630 kernel: [2.038692] x27: x26:
6b42c0c16cf4
Mar 30 13:16:03 c630 kernel: [2.038698] x25: 7965f7df x24:
0001
Mar 30 13:16:03 c630 kernel: [2.038705] x23: 6b42c0a34180 x22:
da2e0cc5b3d0
Mar 30 13:16:03 c630 kernel: [2.038712] x21: da2e0b3ed6a0 x20:
6b42c6845000
Mar 30 13:16:03 c630 kernel: [2.038718] x19: 6b42c6851080 x18:
da2e0cce1220
Mar 30 13:16:03 c630 kernel: [2.038725] x17: da2e0cce1238 x16:
da2e0b23e5f0
Mar 30 13:16:03 c630 kernel: [2.038731] x15: 4000 x14:
Mar 30 13:16:03 c630 kernel: [2.038738] x13: 6b42c5f0b5b0 x12:
Mar 30 13:16:03 c630 kernel: [2.038744] x11: 0001 x10:
3fff
Mar 30 13:16:03 c630 kernel: [2.038750] x9 : x8 :
Mar 30 13:16:03 c630 kernel: [2.038755] x7 : x6 :
0c473b7e
Mar 30 13:16:03 c630 kernel: [2.038761] x5 : 00ff x4 :
00221806fff8f800
Mar 30 13:16:03 c630 kernel: [2.038768] x3 : 0018 x2 :
da2dc3d34320
Mar 30 13:16:03 c630 kernel: [2.038774] x1 : x0 :
Mar 30 13:16:03 c630 kernel: [2.038781] Call trace:
Mar 30 13:16:03 c630 kernel: [2.038784]
dpu_runtime_resume+0xc0/0xf0 [msm]
Mar 30 13:16:03
Re: [Freedreno] [v4] drm/msm/disp/dpu1: turn off vblank irqs aggressively in dpu driver
On 2021-02-22 21:38, Rob Clark wrote:
On Thu, Feb 18, 2021 at 4:36 AM Kalyan Thota
wrote:
Set the flag vblank_disable_immediate = true to turn off vblank irqs
immediately as soon as drm_vblank_put is requested so that there are
no irqs triggered during idle state. This will reduce cpu wakeups
and help in power saving.
To enable vblank_disable_immediate flag the underlying KMS driver
needs to support high precision vblank timestamping and also a
reliable way of providing vblank counter which is incrementing
at the leading edge of vblank.
This patch also brings in changes to support vblank_disable_immediate
requirement in dpu driver.
Changes in v1:
- Specify reason to add vblank timestamp support. (Rob).
- Add changes to provide vblank counter from dpu driver.
Changes in v2:
- Fix warn stack reported by Rob Clark with v2 patch.
Changes in v3:
- Move back to HW frame counter (Rob).
could you let me know what the delta was in v4? (No need to resend
yet, if needed I can amend the commit msg when applying)
- Frame count mismatch was causing a DRM WARN stack spew.
DPU HW will increment the frame count at the end of
the sync, where as vblank will be triggered at the
fetch_start counter which is calculated as v_total - vfp.
This is to start fetching early for panels with low
vbp w.r.t hw latency lines.
Add logic to detect the line count if it falls between
vactive and v_total then return incremented frame count value.
BR,
-R
Signed-off-by: Kalyan Thota
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 80
++
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c| 30
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h| 11 +++
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 1 +
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 26 +++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c| 1 +
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h| 1 +
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c| 5 ++
8 files changed, 155 insertions(+)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index d4662e8..9a80981 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -65,6 +65,83 @@ static void dpu_crtc_destroy(struct drm_crtc *crtc)
kfree(dpu_crtc);
}
+static struct drm_encoder *get_encoder_from_crtc(struct drm_crtc
*crtc)
+{
+ struct drm_device *dev = crtc->dev;
+ struct drm_encoder *encoder;
+
+ drm_for_each_encoder(encoder, dev)
+ if (encoder->crtc == crtc)
+ return encoder;
+
+ return NULL;
+}
+
+static u32 dpu_crtc_get_vblank_counter(struct drm_crtc *crtc)
+{
+ struct drm_encoder *encoder;
+
+ encoder = get_encoder_from_crtc(crtc);
+ if (!encoder) {
+ DRM_ERROR("no encoder found for crtc %d\n",
crtc->index);
+ return false;
+ }
+
+ return dpu_encoder_get_frame_count(encoder);
+}
+
+static bool dpu_crtc_get_scanout_position(struct drm_crtc *crtc,
+ bool in_vblank_irq,
+ int *vpos, int *hpos,
+ ktime_t *stime, ktime_t
*etime,
+ const struct
drm_display_mode *mode)
+{
+ unsigned int pipe = crtc->index;
+ struct drm_encoder *encoder;
+ int line, vsw, vbp, vactive_start, vactive_end, vfp_end;
+
+ encoder = get_encoder_from_crtc(crtc);
+ if (!encoder) {
+ DRM_ERROR("no encoder found for crtc %d\n", pipe);
+ return false;
+ }
+
+ vsw = mode->crtc_vsync_end - mode->crtc_vsync_start;
+ vbp = mode->crtc_vtotal - mode->crtc_vsync_end;
+
+ /*
+* the line counter is 1 at the start of the VSYNC pulse and
VTOTAL at
+* the end of VFP. Translate the porch values relative to the
line
+* counter positions.
+*/
+
+ vactive_start = vsw + vbp + 1;
+ vactive_end = vactive_start + mode->crtc_vdisplay;
+
+ /* last scan line before VSYNC */
+ vfp_end = mode->crtc_vtotal;
+
+ if (stime)
+ *stime = ktime_get();
+
+ line = dpu_encoder_get_linecount(encoder);
+
+ if (line < vactive_start)
+ line -= vactive_start;
+ else if (line > vactive_end)
+ line = line - vfp_end - vactive_start;
+ else
+ line -= vactive_start;
+
+ *vpos = line;
+ *hpos = 0;
+
+ if (etime)
+ *etime = ktime_get();
+
+ return true;
+}
+
static void _dpu_crtc_setup_blend_cfg(struct dpu_crtc_mixer *mixer,
struct dpu_plane_state *pstate, struct dpu_format
*format)
{
@@ -1243,6 +1320,8 @@ static const struct drm_crtc_funcs
dpu_crtc_funcs = {
.early_unregister =
Re: [Freedreno] [v2] drm/msm/disp/dpu1: turn off vblank irqs aggressively in dpu driver
On 2021-02-12 23:19, Rob Clark wrote: On Thu, Feb 11, 2021 at 7:31 AM wrote: On 2021-02-11 01:56, Rob Clark wrote: > On Wed, Feb 10, 2021 at 3:41 AM wrote: >> >> On 2021-02-01 00:46, Rob Clark wrote: >> > On Fri, Dec 18, 2020 at 2:27 AM Kalyan Thota >> > wrote: >> >> >> >> Set the flag vblank_disable_immediate = true to turn off vblank irqs >> >> immediately as soon as drm_vblank_put is requested so that there are >> >> no irqs triggered during idle state. This will reduce cpu wakeups >> >> and help in power saving. >> >> >> >> To enable vblank_disable_immediate flag the underlying KMS driver >> >> needs to support high precision vblank timestamping and also a >> >> reliable way of providing vblank counter which is incrementing >> >> at the leading edge of vblank. >> >> >> >> This patch also brings in changes to support vblank_disable_immediate >> >> requirement in dpu driver. >> >> >> >> Changes in v1: >> >> - Specify reason to add vblank timestamp support. (Rob) >> >> - Add changes to provide vblank counter from dpu driver. >> >> >> >> Signed-off-by: Kalyan Thota >> > >> > This seems to be triggering: >> > >> > [ +0.032668] [ cut here ] >> > [ +0.004759] msm ae0.mdss: drm_WARN_ON_ONCE(cur_vblank != >> > vblank->last) >> > [ +0.24] WARNING: CPU: 0 PID: 362 at >> > drivers/gpu/drm/drm_vblank.c:354 drm_update_vblank_count+0x1e4/0x258 >> > [ +0.017154] Modules linked in: joydev >> > [ +0.003784] CPU: 0 PID: 362 Comm: frecon Not tainted >> > 5.11.0-rc5-00037-g33d3504871dd #2 >> > [ +0.008135] Hardware name: Google Lazor (rev1 - 2) with LTE (DT) >> > [ +0.006167] pstate: 60400089 (nZCv daIf +PAN -UAO -TCO BTYPE=--) >> > [ +0.006169] pc : drm_update_vblank_count+0x1e4/0x258 >> > [ +0.005105] lr : drm_update_vblank_count+0x1e4/0x258 >> > [ +0.005106] sp : ffc010003b70 >> > [ +0.003409] x29: ffc010003b70 x28: ff80855d9d98 >> > [ +0.005466] x27: x26: 00fe502a >> > [ +0.005458] x25: 0001 x24: 0001 >> > [ +0.005466] x23: 0001 x22: ff808561ce80 >> > [ +0.005465] x21: x20: >> > [ +0.005468] x19: ff80850d6800 x18: >> > [ +0.005466] x17: x16: >> > [ +0.005465] x15: 000a x14: 263b >> > [ +0.005466] x13: 0006 x12: >> > [ +0.005465] x11: 0010 x10: ffc090003797 >> > [ +0.005466] x9 : ffed200e2a8c x8 : >> > [ +0.005466] x7 : x6 : ffed213b2b51 >> > [ +0.005465] x5 : c000dfff x4 : ffed21218048 >> > [ +0.005465] x3 : x2 : >> > [ +0.005465] x1 : x0 : >> > [ +0.005466] Call trace: >> > [ +0.002520] drm_update_vblank_count+0x1e4/0x258 >> > [ +0.004748] drm_handle_vblank+0xd0/0x35c >> > [ +0.004130] drm_crtc_handle_vblank+0x24/0x30 >> > [ +0.004487] dpu_crtc_vblank_callback+0x3c/0xc4 >> > [ +0.004662] dpu_encoder_vblank_callback+0x70/0xc4 >> > [ +0.004931] dpu_encoder_phys_vid_vblank_irq+0x50/0x12c >> > [ +0.005378] dpu_core_irq_callback_handler+0xf4/0xfc >> > [ +0.005107] dpu_hw_intr_dispatch_irq+0x100/0x120 >> > [ +0.004834] dpu_core_irq+0x44/0x5c >> > [ +0.003597] dpu_irq+0x1c/0x28 >> > [ +0.003141] msm_irq+0x34/0x40 >> > [ +0.003153] __handle_irq_event_percpu+0xfc/0x254 >> > [ +0.004838] handle_irq_event_percpu+0x3c/0x94 >> > [ +0.004574] handle_irq_event+0x54/0x98 >> > [ +0.003944] handle_level_irq+0xa0/0xd0 >> > [ +0.003943] generic_handle_irq+0x30/0x48 >> > [ +0.004131] dpu_mdss_irq+0xe4/0x118 >> > [ +0.003684] generic_handle_irq+0x30/0x48 >> > [ +0.004127] __handle_domain_irq+0xa8/0xac >> > [ +0.004215] gic_handle_irq+0xdc/0x150 >> > [ +0.003856] el1_irq+0xb4/0x180 >> > [ +0.003237] dpu_encoder_vsync_time+0x78/0x230 >> > [ +0.004574] dpu_encoder_kickoff+0x190/0x354 >> > [ +0.004386] dpu_crtc_commit_kickoff+0x194/0x1a0 >> > [ +0.004748] dpu_kms_flush_commit+0xf4/0x108 >> > [ +0.004390] msm_atomic_commit_tail+0x2e8/0x384 >> > [ +0.004661] commit_tail+0x80/0x108 >> > [ +0.003588] drm_atomic_helper_commit+0x118/0x11c >> > [ +0.004834] drm_atomic_commit+0x58/0x68 >> > [ +0.004033] drm_atomic_helper_set_config+0x70/0x9c >> > [ +0.005018] drm_mode_setcrtc+0x390/0x584 >> > [ +0.004131] drm_ioctl_kernel+0xc8/0x11c >> > [ +0.004035] drm_ioctl+0x2f8/0x34c >> > [ +0.003500] drm_compat_ioctl+0x48/0xe8 >> > [ +0.003945] __arm64_compat_sys_ioctl+0xe8/0x104 >> > [ +0.004750] el0_svc_common.constprop.0+0x114/0x188 >> > [ +0.005019] do_el0_svc_compat+0x28/0x38 >> > [ +0.004031] el0_svc_compat+0x20/0x30 >> > [ +0.003772] el0_sync_compat_handler+0x104/0x18c >> > [ +0.004749] el0_sync_compat+0x178/0x180 >> > [ +0.004034] ---[ end trace 2959d178e74f2555 ]--- >> > >> > >> > BR, >> > -R >> > >> Hi Rob, >> >> on DPU HW, with prefetch enabled, the
Re: [Freedreno] [v2] drm/msm/disp/dpu1: turn off vblank irqs aggressively in dpu driver
On 2021-02-11 01:56, Rob Clark wrote: On Wed, Feb 10, 2021 at 3:41 AM wrote: On 2021-02-01 00:46, Rob Clark wrote: > On Fri, Dec 18, 2020 at 2:27 AM Kalyan Thota > wrote: >> >> Set the flag vblank_disable_immediate = true to turn off vblank irqs >> immediately as soon as drm_vblank_put is requested so that there are >> no irqs triggered during idle state. This will reduce cpu wakeups >> and help in power saving. >> >> To enable vblank_disable_immediate flag the underlying KMS driver >> needs to support high precision vblank timestamping and also a >> reliable way of providing vblank counter which is incrementing >> at the leading edge of vblank. >> >> This patch also brings in changes to support vblank_disable_immediate >> requirement in dpu driver. >> >> Changes in v1: >> - Specify reason to add vblank timestamp support. (Rob) >> - Add changes to provide vblank counter from dpu driver. >> >> Signed-off-by: Kalyan Thota > > This seems to be triggering: > > [ +0.032668] [ cut here ] > [ +0.004759] msm ae0.mdss: drm_WARN_ON_ONCE(cur_vblank != > vblank->last) > [ +0.24] WARNING: CPU: 0 PID: 362 at > drivers/gpu/drm/drm_vblank.c:354 drm_update_vblank_count+0x1e4/0x258 > [ +0.017154] Modules linked in: joydev > [ +0.003784] CPU: 0 PID: 362 Comm: frecon Not tainted > 5.11.0-rc5-00037-g33d3504871dd #2 > [ +0.008135] Hardware name: Google Lazor (rev1 - 2) with LTE (DT) > [ +0.006167] pstate: 60400089 (nZCv daIf +PAN -UAO -TCO BTYPE=--) > [ +0.006169] pc : drm_update_vblank_count+0x1e4/0x258 > [ +0.005105] lr : drm_update_vblank_count+0x1e4/0x258 > [ +0.005106] sp : ffc010003b70 > [ +0.003409] x29: ffc010003b70 x28: ff80855d9d98 > [ +0.005466] x27: x26: 00fe502a > [ +0.005458] x25: 0001 x24: 0001 > [ +0.005466] x23: 0001 x22: ff808561ce80 > [ +0.005465] x21: x20: > [ +0.005468] x19: ff80850d6800 x18: > [ +0.005466] x17: x16: > [ +0.005465] x15: 000a x14: 263b > [ +0.005466] x13: 0006 x12: > [ +0.005465] x11: 0010 x10: ffc090003797 > [ +0.005466] x9 : ffed200e2a8c x8 : > [ +0.005466] x7 : x6 : ffed213b2b51 > [ +0.005465] x5 : c000dfff x4 : ffed21218048 > [ +0.005465] x3 : x2 : > [ +0.005465] x1 : x0 : > [ +0.005466] Call trace: > [ +0.002520] drm_update_vblank_count+0x1e4/0x258 > [ +0.004748] drm_handle_vblank+0xd0/0x35c > [ +0.004130] drm_crtc_handle_vblank+0x24/0x30 > [ +0.004487] dpu_crtc_vblank_callback+0x3c/0xc4 > [ +0.004662] dpu_encoder_vblank_callback+0x70/0xc4 > [ +0.004931] dpu_encoder_phys_vid_vblank_irq+0x50/0x12c > [ +0.005378] dpu_core_irq_callback_handler+0xf4/0xfc > [ +0.005107] dpu_hw_intr_dispatch_irq+0x100/0x120 > [ +0.004834] dpu_core_irq+0x44/0x5c > [ +0.003597] dpu_irq+0x1c/0x28 > [ +0.003141] msm_irq+0x34/0x40 > [ +0.003153] __handle_irq_event_percpu+0xfc/0x254 > [ +0.004838] handle_irq_event_percpu+0x3c/0x94 > [ +0.004574] handle_irq_event+0x54/0x98 > [ +0.003944] handle_level_irq+0xa0/0xd0 > [ +0.003943] generic_handle_irq+0x30/0x48 > [ +0.004131] dpu_mdss_irq+0xe4/0x118 > [ +0.003684] generic_handle_irq+0x30/0x48 > [ +0.004127] __handle_domain_irq+0xa8/0xac > [ +0.004215] gic_handle_irq+0xdc/0x150 > [ +0.003856] el1_irq+0xb4/0x180 > [ +0.003237] dpu_encoder_vsync_time+0x78/0x230 > [ +0.004574] dpu_encoder_kickoff+0x190/0x354 > [ +0.004386] dpu_crtc_commit_kickoff+0x194/0x1a0 > [ +0.004748] dpu_kms_flush_commit+0xf4/0x108 > [ +0.004390] msm_atomic_commit_tail+0x2e8/0x384 > [ +0.004661] commit_tail+0x80/0x108 > [ +0.003588] drm_atomic_helper_commit+0x118/0x11c > [ +0.004834] drm_atomic_commit+0x58/0x68 > [ +0.004033] drm_atomic_helper_set_config+0x70/0x9c > [ +0.005018] drm_mode_setcrtc+0x390/0x584 > [ +0.004131] drm_ioctl_kernel+0xc8/0x11c > [ +0.004035] drm_ioctl+0x2f8/0x34c > [ +0.003500] drm_compat_ioctl+0x48/0xe8 > [ +0.003945] __arm64_compat_sys_ioctl+0xe8/0x104 > [ +0.004750] el0_svc_common.constprop.0+0x114/0x188 > [ +0.005019] do_el0_svc_compat+0x28/0x38 > [ +0.004031] el0_svc_compat+0x20/0x30 > [ +0.003772] el0_sync_compat_handler+0x104/0x18c > [ +0.004749] el0_sync_compat+0x178/0x180 > [ +0.004034] ---[ end trace 2959d178e74f2555 ]--- > > > BR, > -R > Hi Rob, on DPU HW, with prefetch enabled, the frame count increment and vsync irq are not happening at same instance. This is causing the frame count to mismatch. Example: |###--^--|###--^--| for the above vsync cycle with prefetch enabled "^" --> marks a fetch counter where in we are asking the hw to start fetching in the front porch so that we will have more time to fetch data by first active line of next
Re: [Freedreno] [v2] drm/msm/disp/dpu1: turn off vblank irqs aggressively in dpu driver
On 2021-02-01 00:46, Rob Clark wrote:
On Fri, Dec 18, 2020 at 2:27 AM Kalyan Thota
wrote:
Set the flag vblank_disable_immediate = true to turn off vblank irqs
immediately as soon as drm_vblank_put is requested so that there are
no irqs triggered during idle state. This will reduce cpu wakeups
and help in power saving.
To enable vblank_disable_immediate flag the underlying KMS driver
needs to support high precision vblank timestamping and also a
reliable way of providing vblank counter which is incrementing
at the leading edge of vblank.
This patch also brings in changes to support vblank_disable_immediate
requirement in dpu driver.
Changes in v1:
- Specify reason to add vblank timestamp support. (Rob)
- Add changes to provide vblank counter from dpu driver.
Signed-off-by: Kalyan Thota
This seems to be triggering:
[ +0.032668] [ cut here ]
[ +0.004759] msm ae0.mdss: drm_WARN_ON_ONCE(cur_vblank !=
vblank->last)
[ +0.24] WARNING: CPU: 0 PID: 362 at
drivers/gpu/drm/drm_vblank.c:354 drm_update_vblank_count+0x1e4/0x258
[ +0.017154] Modules linked in: joydev
[ +0.003784] CPU: 0 PID: 362 Comm: frecon Not tainted
5.11.0-rc5-00037-g33d3504871dd #2
[ +0.008135] Hardware name: Google Lazor (rev1 - 2) with LTE (DT)
[ +0.006167] pstate: 60400089 (nZCv daIf +PAN -UAO -TCO BTYPE=--)
[ +0.006169] pc : drm_update_vblank_count+0x1e4/0x258
[ +0.005105] lr : drm_update_vblank_count+0x1e4/0x258
[ +0.005106] sp : ffc010003b70
[ +0.003409] x29: ffc010003b70 x28: ff80855d9d98
[ +0.005466] x27: x26: 00fe502a
[ +0.005458] x25: 0001 x24: 0001
[ +0.005466] x23: 0001 x22: ff808561ce80
[ +0.005465] x21: x20:
[ +0.005468] x19: ff80850d6800 x18:
[ +0.005466] x17: x16:
[ +0.005465] x15: 000a x14: 263b
[ +0.005466] x13: 0006 x12:
[ +0.005465] x11: 0010 x10: ffc090003797
[ +0.005466] x9 : ffed200e2a8c x8 :
[ +0.005466] x7 : x6 : ffed213b2b51
[ +0.005465] x5 : c000dfff x4 : ffed21218048
[ +0.005465] x3 : x2 :
[ +0.005465] x1 : x0 :
[ +0.005466] Call trace:
[ +0.002520] drm_update_vblank_count+0x1e4/0x258
[ +0.004748] drm_handle_vblank+0xd0/0x35c
[ +0.004130] drm_crtc_handle_vblank+0x24/0x30
[ +0.004487] dpu_crtc_vblank_callback+0x3c/0xc4
[ +0.004662] dpu_encoder_vblank_callback+0x70/0xc4
[ +0.004931] dpu_encoder_phys_vid_vblank_irq+0x50/0x12c
[ +0.005378] dpu_core_irq_callback_handler+0xf4/0xfc
[ +0.005107] dpu_hw_intr_dispatch_irq+0x100/0x120
[ +0.004834] dpu_core_irq+0x44/0x5c
[ +0.003597] dpu_irq+0x1c/0x28
[ +0.003141] msm_irq+0x34/0x40
[ +0.003153] __handle_irq_event_percpu+0xfc/0x254
[ +0.004838] handle_irq_event_percpu+0x3c/0x94
[ +0.004574] handle_irq_event+0x54/0x98
[ +0.003944] handle_level_irq+0xa0/0xd0
[ +0.003943] generic_handle_irq+0x30/0x48
[ +0.004131] dpu_mdss_irq+0xe4/0x118
[ +0.003684] generic_handle_irq+0x30/0x48
[ +0.004127] __handle_domain_irq+0xa8/0xac
[ +0.004215] gic_handle_irq+0xdc/0x150
[ +0.003856] el1_irq+0xb4/0x180
[ +0.003237] dpu_encoder_vsync_time+0x78/0x230
[ +0.004574] dpu_encoder_kickoff+0x190/0x354
[ +0.004386] dpu_crtc_commit_kickoff+0x194/0x1a0
[ +0.004748] dpu_kms_flush_commit+0xf4/0x108
[ +0.004390] msm_atomic_commit_tail+0x2e8/0x384
[ +0.004661] commit_tail+0x80/0x108
[ +0.003588] drm_atomic_helper_commit+0x118/0x11c
[ +0.004834] drm_atomic_commit+0x58/0x68
[ +0.004033] drm_atomic_helper_set_config+0x70/0x9c
[ +0.005018] drm_mode_setcrtc+0x390/0x584
[ +0.004131] drm_ioctl_kernel+0xc8/0x11c
[ +0.004035] drm_ioctl+0x2f8/0x34c
[ +0.003500] drm_compat_ioctl+0x48/0xe8
[ +0.003945] __arm64_compat_sys_ioctl+0xe8/0x104
[ +0.004750] el0_svc_common.constprop.0+0x114/0x188
[ +0.005019] do_el0_svc_compat+0x28/0x38
[ +0.004031] el0_svc_compat+0x20/0x30
[ +0.003772] el0_sync_compat_handler+0x104/0x18c
[ +0.004749] el0_sync_compat+0x178/0x180
[ +0.004034] ---[ end trace 2959d178e74f2555 ]---
BR,
-R
Hi Rob,
on DPU HW, with prefetch enabled, the frame count increment and vsync
irq are not happening at same instance. This is causing the frame count
to mismatch.
Example:
|###--^--|###--^--|
for the above vsync cycle with prefetch enabled "^" --> marks a fetch
counter where in we are asking the hw to start fetching in the front
porch so that we will have more time to fetch data by first active line
of next frame.
In this case, the vsync irq will be triggered at fetch start marker
("^") so that double buffered updates are submitted to HW and the frame
count update will happen at the end of front porch ("|")
to handle this, can we fallback on the SW vblank counter
Re: [Freedreno] [PATCH 3/3] drm/msm/dpu: add support for clk and bw scaling for display
On 2020-11-08 23:25, Amit Pundir wrote: On Tue, 4 Aug 2020 at 21:09, Rob Clark wrote: On Thu, Jul 16, 2020 at 4:36 AM Kalyan Thota wrote: > > This change adds support to scale src clk and bandwidth as > per composition requirements. > > Interconnect registration for bw has been moved to mdp > device node from mdss to facilitate the scaling. > > Changes in v1: > - Address armv7 compilation issues with the patch (Rob) > > Signed-off-by: Kalyan Thota Reviewed-by: Rob Clark Hi Kalyan, Rob, This patch broke the display on the PocoF1 phone (sdm845-xiaomi-beryllium.dts) running AOSP. I can boot to UI but the display is frozen soon after that and dmesg is full of following errors: [drm:dpu_core_perf_crtc_update:397] [dpu error]crtc-65: failed to update bus bw vote [drm:dpu_core_perf_crtc_check:203] [dpu error]exceeds bandwidth: 7649746kb > 680kb [drm:dpu_crtc_atomic_check:969] [dpu error]crtc65 failed performance check -7 [drm:dpu_core_perf_crtc_check:203] [dpu error]exceeds bandwidth: 7649746kb > 680kb [drm:dpu_crtc_atomic_check:969] [dpu error]crtc65 failed performance check -7 [drm:dpu_core_perf_crtc_check:203] [dpu error]exceeds bandwidth: 7649746kb > 680kb [drm:dpu_crtc_atomic_check:969] [dpu error]crtc65 failed performance check -7 Here is the full dmesg https://pastebin.ubuntu.com/p/PcSdNgMnYw/. Georgi pointed out following patch but it didn't help, https://lore.kernel.org/dri-devel/20201027102304.945424-1-dmitry.barysh...@linaro.org/ Am I missing any other followup fix? Regards, Amit Pundir __ Hi Amit, Apologies for the delay. I have gone through the logs and referred to the below panel file for the timings. https://github.com/Matheus-Garbelini/Kernel-Sphinx-Pocophone-F1/blob/master/arch/arm64/boot/dts/qcom/dsi-panel-tianma-fhd-nt36672a-video.dtsi if the above is correct file, then below could be the possible root cause. The panel back porch and pw is less and it is causing the prefill bw requirement to shoot up per layer as currently we are not considering front porch in the calculation. can you please try the attached patch in the email as a solution and provide me the feedback, i'll post it as a formal change. Thanks, Kalyan _ Freedreno mailing list freedr...@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno From 028fb47ccc5a3f8f8e51513bd2719aa14c68ac09 Mon Sep 17 00:00:00 2001 From: Kalyan Thota Date: Tue, 24 Nov 2020 02:39:52 -0800 Subject: [PATCH] drm: msm: dpu: consider front porch in the prefill calculation In case of panels with low vertical back porch and pw, the prefill bw will increase as we will have less time to fetch and fill all the hw latency buffers. for ex: hw_latnecy_lines = 24, and if vbp+pw = 10 then we need to fetch 24 lines of data in 10 line times. This will increase prefill bw requirement. DPU hw can fetch data during front porch also provided prefetch is enabled. Use front porch also into the prefill caluculation as driver enables prefetch if the blanking is not sufficient to fill the latency lines. Signed-off-by: Kalyan Thota --- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 11 +-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c index 7ea90d2..315b999 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c @@ -151,7 +151,7 @@ static void _dpu_plane_calc_bw(struct drm_plane *plane, u64 plane_bw; u32 hw_latency_lines; u64 scale_factor; - int vbp, vpw; + int vbp, vpw, vfp; pstate = to_dpu_plane_state(plane->state); mode = >state->crtc->mode; @@ -164,6 +164,7 @@ static void _dpu_plane_calc_bw(struct drm_plane *plane, fps = drm_mode_vrefresh(mode); vbp = mode->vtotal - mode->vsync_end; vpw = mode->vsync_end - mode->vsync_start; + vfp = mode->vsync_start - mode->vdisplay; hw_latency_lines = dpu_kms->catalog->perf.min_prefill_lines; scale_factor = src_height > dst_height ? mult_frac(src_height, 1, dst_height) : 1; @@ -176,7 +177,13 @@ static void _dpu_plane_calc_bw(struct drm_plane *plane, src_width * hw_latency_lines * fps * fmt->bpp * scale_factor * mode->vtotal; - do_div(plane_prefill_bw, (vbp+vpw)); + if ((vbp+vpw) > hw_latency_lines) + do_div(plane_prefill_bw, (vbp+vpw)); + else if ((vbp+vpw+vfp) < hw_latency_lines) + do_div(plane_prefill_bw, (vbp+vpw+vfp)); + else + do_div(plane_prefill_bw, hw_latency_lines); + pstate->plane_fetch_bw = max(plane_bw, plane_prefill_bw); } -- 2.7.4 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Freedreno] [v1] drm/msm/dpu: Fix reservation failures in modeset
On 2020-08-06 22:15, Rob Clark wrote:
On Thu, Aug 6, 2020 at 7:46 AM wrote:
On 2020-08-05 21:18, Rob Clark wrote:
> On Wed, Aug 5, 2020 at 6:34 AM Kalyan Thota
> wrote:
>>
>> In TEST_ONLY commit, rm global_state will duplicate the
>> object and request for new reservations, once they pass
>> then the new state will be swapped with the old and will
>> be available for the Atomic Commit.
>>
>> This patch fixes some of missing links in the resource
>> reservation sequence mentioned above.
>>
>> 1) Creation of a duplicate state in test_only commit (Rob)
>> 2) Allow resource release only during crtc_active false.
>>
>> For #2
>> In a modeset operation, swap state happens well before disable.
>> Hence clearing reservations in disable will cause failures
>> in modeset enable.
>>
>> Sequence:
>> Swap state --> old, new
>> modeset disables --> virt disable
>> modeset enable --> virt modeset
>>
>> Allow reservations to be cleared only when crtc active is false
>> as in that case there wont be any modeset enable after disable.
>>
>> Signed-off-by: Kalyan Thota
>> ---
>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 7 +--
>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 63976dc..b85a576 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -582,7 +582,7 @@ static int dpu_encoder_virt_atomic_check(
>> dpu_kms = to_dpu_kms(priv->kms);
>> mode = _state->mode;
>> adj_mode = _state->adjusted_mode;
>> - global_state = dpu_kms_get_existing_global_state(dpu_kms);
>> + global_state = dpu_kms_get_global_state(crtc_state->state);
>> trace_dpu_enc_atomic_check(DRMID(drm_enc));
>>
>> /*
>> @@ -1172,6 +1172,7 @@ static void dpu_encoder_virt_disable(struct
>> drm_encoder *drm_enc)
>> struct msm_drm_private *priv;
>> struct dpu_kms *dpu_kms;
>> struct dpu_global_state *global_state;
>> + struct drm_crtc_state *crtc_state;
>> int i = 0;
>>
>> if (!drm_enc) {
>> @@ -1191,6 +1192,7 @@ static void dpu_encoder_virt_disable(struct
>> drm_encoder *drm_enc)
>> priv = drm_enc->dev->dev_private;
>> dpu_kms = to_dpu_kms(priv->kms);
>> global_state = dpu_kms_get_existing_global_state(dpu_kms);
>> + crtc_state = drm_enc->crtc->state;
>>
>> trace_dpu_enc_disable(DRMID(drm_enc));
>>
>> @@ -1220,7 +1222,8 @@ static void dpu_encoder_virt_disable(struct
>> drm_encoder *drm_enc)
>>
>> DPU_DEBUG_ENC(dpu_enc, "encoder disabled\n");
>>
>> - dpu_rm_release(global_state, drm_enc);
>> + if (crtc_state->active_changed && !crtc_state->active)
>> + dpu_rm_release(global_state, drm_enc);
>
> I still think releasing the state in the atomic_commit() path is the
> wrong thing to do. In the commit path, the various state objects
> should be immutable.. ie. in the atomic_test() path you derive the new
> hw state (including assignment/release of resources), and
> atomic_commit() is simply pushing the state down to the hw.
>
> Otherwise, this looks better than v1.
>
> BR,
> -R
>
okay. Should we avoid reservation all together if active=0 on that
crtc
and trigger rm_release on the enc during atomic_check ?
how do you see the approach ?
Yeah, I suppose something like:
if (drm_atomic_crtc_needs_modeset()) {
reserve()
} else if (active_changed && !active) {
release()
}
I think it could happen (at least with atomic api) that you get a
modeset without active_changed, so we might need to release() and then
reserve() in that case? (This is probably where starting to run more
IGT tests would be useful)
BR,
-R
Thanks Rob, please review the v2 version.
-Kalyan
>>
>> mutex_unlock(_enc->enc_lock);
>> }
>> --
>> 1.9.1
>>
> ___
> Freedreno mailing list
> freedr...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Freedreno] [v1] drm/msm/dpu: Fix reservation failures in modeset
On 2020-08-05 21:18, Rob Clark wrote:
On Wed, Aug 5, 2020 at 6:34 AM Kalyan Thota
wrote:
In TEST_ONLY commit, rm global_state will duplicate the
object and request for new reservations, once they pass
then the new state will be swapped with the old and will
be available for the Atomic Commit.
This patch fixes some of missing links in the resource
reservation sequence mentioned above.
1) Creation of a duplicate state in test_only commit (Rob)
2) Allow resource release only during crtc_active false.
For #2
In a modeset operation, swap state happens well before disable.
Hence clearing reservations in disable will cause failures
in modeset enable.
Sequence:
Swap state --> old, new
modeset disables --> virt disable
modeset enable --> virt modeset
Allow reservations to be cleared only when crtc active is false
as in that case there wont be any modeset enable after disable.
Signed-off-by: Kalyan Thota
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 63976dc..b85a576 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -582,7 +582,7 @@ static int dpu_encoder_virt_atomic_check(
dpu_kms = to_dpu_kms(priv->kms);
mode = _state->mode;
adj_mode = _state->adjusted_mode;
- global_state = dpu_kms_get_existing_global_state(dpu_kms);
+ global_state = dpu_kms_get_global_state(crtc_state->state);
trace_dpu_enc_atomic_check(DRMID(drm_enc));
/*
@@ -1172,6 +1172,7 @@ static void dpu_encoder_virt_disable(struct
drm_encoder *drm_enc)
struct msm_drm_private *priv;
struct dpu_kms *dpu_kms;
struct dpu_global_state *global_state;
+ struct drm_crtc_state *crtc_state;
int i = 0;
if (!drm_enc) {
@@ -1191,6 +1192,7 @@ static void dpu_encoder_virt_disable(struct
drm_encoder *drm_enc)
priv = drm_enc->dev->dev_private;
dpu_kms = to_dpu_kms(priv->kms);
global_state = dpu_kms_get_existing_global_state(dpu_kms);
+ crtc_state = drm_enc->crtc->state;
trace_dpu_enc_disable(DRMID(drm_enc));
@@ -1220,7 +1222,8 @@ static void dpu_encoder_virt_disable(struct
drm_encoder *drm_enc)
DPU_DEBUG_ENC(dpu_enc, "encoder disabled\n");
- dpu_rm_release(global_state, drm_enc);
+ if (crtc_state->active_changed && !crtc_state->active)
+ dpu_rm_release(global_state, drm_enc);
I still think releasing the state in the atomic_commit() path is the
wrong thing to do. In the commit path, the various state objects
should be immutable.. ie. in the atomic_test() path you derive the new
hw state (including assignment/release of resources), and
atomic_commit() is simply pushing the state down to the hw.
Otherwise, this looks better than v1.
BR,
-R
okay. Should we avoid reservation all together if active=0 on that crtc
and trigger rm_release on the enc during atomic_check ?
how do you see the approach ?
-Kalyan
mutex_unlock(_enc->enc_lock);
}
--
1.9.1
___
Freedreno mailing list
freedr...@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Freedreno] [v1] drm/msm/dpu: update reservations in commit path
On 2020-08-05 01:02, Rob Clark wrote:
On Tue, Aug 4, 2020 at 4:32 AM Kalyan Thota
wrote:
DPU resources reserved in the atomic_check path gets unwinded
during modeset operation before commit happens in a non seamless
transition.
Update the reservations in the commit path to avoid resource
failures. Secondly have dummy reservations in atomic_check path
so that we can gracefully fail the composition if resources are
not available.
Signed-off-by: Kalyan Thota
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 15 +++
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 63976dc..c6b8254 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -565,7 +565,7 @@ static int dpu_encoder_virt_atomic_check(
const struct drm_display_mode *mode;
struct drm_display_mode *adj_mode;
struct msm_display_topology topology;
- struct dpu_global_state *global_state;
+ struct dpu_global_state tmp_resv_state;
int i = 0;
int ret = 0;
@@ -582,7 +582,7 @@ static int dpu_encoder_virt_atomic_check(
dpu_kms = to_dpu_kms(priv->kms);
mode = _state->mode;
adj_mode = _state->adjusted_mode;
- global_state = dpu_kms_get_existing_global_state(dpu_kms);
+ memset(_resv_state, 0, sizeof(tmp_resv_state));
I think what you want to do is dpu_kms_get_global_state().. that will
clone/duplicate the existing global state (or return the already
duplicated global state if it is called multiple times).
Thanks Rob, realized the same after posting patch. Made changes in the
new patch
accordingly.
It is safe to modify this global state in the atomic_check() path.. in
fact that is the intention. For a TEST_ONLY atomic commit, or if any
of the atomic_check()'s fail, this new duplicated global state is
discarded. If all the checks succeed and the atomic update is
committed to hw, this new global state replaces the existing global
state.
Posted a new change kindly review.
BR,
-R
trace_dpu_enc_atomic_check(DRMID(drm_enc));
/*
@@ -621,7 +621,7 @@ static int dpu_encoder_virt_atomic_check(
* info may not be available to complete reservation.
*/
if (drm_atomic_crtc_needs_modeset(crtc_state)) {
- ret = dpu_rm_reserve(_kms->rm,
global_state,
+ ret = dpu_rm_reserve(_kms->rm,
_resv_state,
drm_enc, crtc_state,
topology);
}
}
@@ -966,7 +966,7 @@ static void dpu_encoder_virt_mode_set(struct
drm_encoder *drm_enc,
struct dpu_hw_blk *hw_lm[MAX_CHANNELS_PER_ENC];
struct dpu_hw_blk *hw_dspp[MAX_CHANNELS_PER_ENC] = { NULL };
int num_lm, num_ctl, num_pp, num_dspp;
- int i, j;
+ int i, j, rc;
if (!drm_enc) {
DPU_ERROR("invalid encoder\n");
@@ -1006,6 +1006,13 @@ static void dpu_encoder_virt_mode_set(struct
drm_encoder *drm_enc,
topology = dpu_encoder_get_topology(dpu_enc, dpu_kms,
adj_mode);
+ rc = dpu_rm_reserve(_kms->rm, global_state, drm_enc,
+ drm_crtc->state, topology);
+ if (rc) {
+ DPU_ERROR_ENC(dpu_enc, "Failed to reserve
resources\n");
+ return;
+ }
+
/* Query resource that have been reserved in atomic check
step. */
num_pp = dpu_rm_get_assigned_resources(_kms->rm,
global_state,
drm_enc->base.id, DPU_HW_BLK_PINGPONG, hw_pp,
--
1.9.1
___
Freedreno mailing list
freedr...@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Freedreno] [PATCH] drm/msm/dpu: fix/enable 6bpc dither with split-lm
On 2020-07-20 20:53, Rob Clark wrote:
On Mon, Jul 20, 2020 at 5:53 AM wrote:
On 2020-07-16 03:49, Rob Clark wrote:
> From: Rob Clark
>
> If split-lm is used (for ex, on sdm845), we can have multiple ping-
> pongs, but only a single phys encoder. We need to configure dithering
> on each of them.
>
> Signed-off-by: Rob Clark
Reviewed-by: Kalyan Thota
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 22 ++-
> .../gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 3 +--
> 2 files changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 46df0ff75b85..9b98b63c77fb 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -212,14 +212,14 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = {
> 15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10
> };
>
> -static void _dpu_encoder_setup_dither(struct dpu_encoder_phys *phys)
> +static void _dpu_encoder_setup_dither(struct dpu_hw_pingpong *hw_pp,
> unsigned bpc)
> {
> struct dpu_hw_dither_cfg dither_cfg = { 0 };
>
> - if (!phys->hw_pp || !phys->hw_pp->ops.setup_dither)
> + if (!hw_pp->ops.setup_dither)
> return;
>
> - switch (phys->connector->display_info.bpc) {
> + switch (bpc) {
> case 6:
> dither_cfg.c0_bitdepth = 6;
> dither_cfg.c1_bitdepth = 6;
> @@ -228,14 +228,14 @@ static void _dpu_encoder_setup_dither(struct
> dpu_encoder_phys *phys)
> dither_cfg.temporal_en = 0;
> break;
> default:
> - phys->hw_pp->ops.setup_dither(phys->hw_pp, NULL);
> + hw_pp->ops.setup_dither(hw_pp, NULL);
> return;
> }
>
> memcpy(_cfg.matrix, dither_matrix,
> sizeof(u32) * DITHER_MATRIX_SZ);
>
> - phys->hw_pp->ops.setup_dither(phys->hw_pp, _cfg);
> + hw_pp->ops.setup_dither(hw_pp, _cfg);
> }
>
> void dpu_encoder_helper_report_irq_timeout(struct dpu_encoder_phys
> *phys_enc,
> @@ -1132,11 +1132,13 @@ static void
> _dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
>
> _dpu_encoder_update_vsync_source(dpu_enc, _enc->disp_info);
>
> - if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {
> - for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> - struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
> -
> - _dpu_encoder_setup_dither(phys);
> + if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI &&
> + !WARN_ON(dpu_enc->num_phys_encs == 0)) {
> + unsigned bpc =
dpu_enc->phys_encs[0]->connector->display_info.bpc;
> + for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {
> + if (!dpu_enc->hw_pp[i])
> + continue;
> + _dpu_encoder_setup_dither(dpu_enc->hw_pp[i], bpc);
> }
> }
> }
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> index 7411ab6bf6af..bea4ab5c58c5 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> @@ -231,8 +231,7 @@ static void _setup_pingpong_ops(struct
> dpu_hw_pingpong *c,
> c->ops.poll_timeout_wr_ptr = dpu_hw_pp_poll_timeout_wr_ptr;
> c->ops.get_line_count = dpu_hw_pp_get_line_count;
>
> - if (test_bit(DPU_PINGPONG_DITHER, ) &&
> - IS_SC7180_TARGET(c->hw.hwversion))
> + if (test_bit(DPU_PINGPONG_DITHER, ))
> c->ops.setup_dither = dpu_hw_pp_setup_dither;
> };
Change looks good to me
Does that count as a Reviewed-by?
Sure i have added the tag.
BR,
-R
- Kalyan
___
Freedreno mailing list
freedr...@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Freedreno] [PATCH] drm/msm/dpu: fix/enable 6bpc dither with split-lm
On 2020-07-16 03:49, Rob Clark wrote:
From: Rob Clark
If split-lm is used (for ex, on sdm845), we can have multiple ping-
pongs, but only a single phys encoder. We need to configure dithering
on each of them.
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 22 ++-
.../gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 3 +--
2 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 46df0ff75b85..9b98b63c77fb 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -212,14 +212,14 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = {
15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10
};
-static void _dpu_encoder_setup_dither(struct dpu_encoder_phys *phys)
+static void _dpu_encoder_setup_dither(struct dpu_hw_pingpong *hw_pp,
unsigned bpc)
{
struct dpu_hw_dither_cfg dither_cfg = { 0 };
- if (!phys->hw_pp || !phys->hw_pp->ops.setup_dither)
+ if (!hw_pp->ops.setup_dither)
return;
- switch (phys->connector->display_info.bpc) {
+ switch (bpc) {
case 6:
dither_cfg.c0_bitdepth = 6;
dither_cfg.c1_bitdepth = 6;
@@ -228,14 +228,14 @@ static void _dpu_encoder_setup_dither(struct
dpu_encoder_phys *phys)
dither_cfg.temporal_en = 0;
break;
default:
- phys->hw_pp->ops.setup_dither(phys->hw_pp, NULL);
+ hw_pp->ops.setup_dither(hw_pp, NULL);
return;
}
memcpy(_cfg.matrix, dither_matrix,
sizeof(u32) * DITHER_MATRIX_SZ);
- phys->hw_pp->ops.setup_dither(phys->hw_pp, _cfg);
+ hw_pp->ops.setup_dither(hw_pp, _cfg);
}
void dpu_encoder_helper_report_irq_timeout(struct dpu_encoder_phys
*phys_enc,
@@ -1132,11 +1132,13 @@ static void
_dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
_dpu_encoder_update_vsync_source(dpu_enc, _enc->disp_info);
- if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {
- for (i = 0; i < dpu_enc->num_phys_encs; i++) {
- struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
-
- _dpu_encoder_setup_dither(phys);
+ if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI &&
+ !WARN_ON(dpu_enc->num_phys_encs == 0)) {
+ unsigned bpc =
dpu_enc->phys_encs[0]->connector->display_info.bpc;
+ for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {
+ if (!dpu_enc->hw_pp[i])
+ continue;
+ _dpu_encoder_setup_dither(dpu_enc->hw_pp[i], bpc);
}
}
}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
index 7411ab6bf6af..bea4ab5c58c5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
@@ -231,8 +231,7 @@ static void _setup_pingpong_ops(struct
dpu_hw_pingpong *c,
c->ops.poll_timeout_wr_ptr = dpu_hw_pp_poll_timeout_wr_ptr;
c->ops.get_line_count = dpu_hw_pp_get_line_count;
- if (test_bit(DPU_PINGPONG_DITHER, ) &&
- IS_SC7180_TARGET(c->hw.hwversion))
+ if (test_bit(DPU_PINGPONG_DITHER, ))
c->ops.setup_dither = dpu_hw_pp_setup_dither;
};
Change looks good to me
- Kalyan
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Freedreno] [v1] drm/msm/dpu: add support for clk and bw scaling for display
On 2020-07-13 22:50, Rob Clark wrote:
On Mon, Jul 13, 2020 at 8:59 AM wrote:
On 2020-07-10 22:38, Rob Clark wrote:
> On Thu, Jun 18, 2020 at 7:09 AM Kalyan Thota
> wrote:
>>
>> This change adds support to scale src clk and bandwidth as
>> per composition requirements.
>>
>> Interconnect registration for bw has been moved to mdp
>> device node from mdss to facilitate the scaling.
>>
>> Changes in v1:
>> - Address armv7 compilation issues with the patch (Rob)
>>
>> Signed-off-by: Kalyan Thota
>> ---
>> drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 109
>> +
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 5 +-
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 4 +
>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c| 37 -
>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h| 4 +
>> drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c | 9 +-
>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 84
>> +++
>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 4 +
>> 8 files changed, 233 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>> index 7c230f7..e52bc44 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>> @@ -29,6 +29,74 @@ enum dpu_perf_mode {
>> DPU_PERF_MODE_MAX
>> };
>>
>> +/**
>> + * @_dpu_core_perf_calc_bw() - to calculate BW per crtc
>> + * @kms - pointer to the dpu_kms
>> + * @crtc - pointer to a crtc
>> + * Return: returns aggregated BW for all planes in crtc.
>> + */
>> +static u64 _dpu_core_perf_calc_bw(struct dpu_kms *kms,
>> + struct drm_crtc *crtc)
>> +{
>> + struct drm_plane *plane;
>> + struct dpu_plane_state *pstate;
>> + u64 crtc_plane_bw = 0;
>> + u32 bw_factor;
>> +
>> + drm_atomic_crtc_for_each_plane(plane, crtc) {
>> + pstate = to_dpu_plane_state(plane->state);
>> + if (!pstate)
>> + continue;
>> +
>> + crtc_plane_bw += pstate->plane_fetch_bw;
>> + }
>> +
>> + bw_factor = kms->catalog->perf.bw_inefficiency_factor;
>> + if (bw_factor) {
>> + crtc_plane_bw *= bw_factor;
>> + do_div(crtc_plane_bw, 100);
>> + }
>> +
>> + return crtc_plane_bw;
>> +}
>> +
>> +/**
>> + * _dpu_core_perf_calc_clk() - to calculate clock per crtc
>> + * @kms - pointer to the dpu_kms
>> + * @crtc - pointer to a crtc
>> + * @state - pointer to a crtc state
>> + * Return: returns max clk for all planes in crtc.
>> + */
>> +static u64 _dpu_core_perf_calc_clk(struct dpu_kms *kms,
>> + struct drm_crtc *crtc, struct drm_crtc_state *state)
>> +{
>> + struct drm_plane *plane;
>> + struct dpu_plane_state *pstate;
>> + struct drm_display_mode *mode;
>> + u64 crtc_clk;
>> + u32 clk_factor;
>> +
>> + mode = >adjusted_mode;
>> +
>> + crtc_clk = mode->vtotal * mode->hdisplay *
>> drm_mode_vrefresh(mode);
>> +
>> + drm_atomic_crtc_for_each_plane(plane, crtc) {
>> + pstate = to_dpu_plane_state(plane->state);
>> + if (!pstate)
>> + continue;
>> +
>> + crtc_clk = max(pstate->plane_clk, crtc_clk);
>> + }
>> +
>> + clk_factor = kms->catalog->perf.clk_inefficiency_factor;
>> + if (clk_factor) {
>> + crtc_clk *= clk_factor;
>> + do_div(crtc_clk, 100);
>> + }
>> +
>> + return crtc_clk;
>> +}
>> +
>> static struct dpu_kms *_dpu_crtc_get_kms(struct drm_crtc *crtc)
>> {
>> struct msm_drm_private *priv;
>> @@ -51,12 +119,7 @@ static void _dpu_core_perf_calc_crtc(struct
>> dpu_kms *kms,
>> dpu_cstate = to_dpu_crtc_state(state);
>> memset(perf, 0, sizeof(struct dpu_core_perf_params));
>>
>> - if (!dpu_cstate->bw_control) {
>> - perf->bw_ctl = kms->catalog->perf.max_bw_high *
>> - 1000ULL;
>> - perf->max_per_pipe_ib = perf->bw_ctl;
>> - perf->core_clk_rate = kms->perf.max_core_clk_rate;
>> - } else if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM)
>> {
>> + if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
>> perf->bw_ctl = 0;
>> perf->max_per_pipe_ib = 0;
>> perf->core_clk_rate = 0;
>> @@ -64,6 +127,10 @@ static void _dpu_core_perf_calc_crtc(struct
>> dpu_kms *kms,
>> perf->bw_ctl = kms->perf.fix_core_ab_vote;
>> perf->max_per_pipe_ib = kms->perf.fix_core_ib_vote;
>> perf->core_clk_rate = kms->perf.fix_core_clk_rate;
>> + } else {
>> + perf->bw_ctl = _dpu_core_perf_calc_bw(kms, crtc);
>> + perf->max_per_pipe_ib =
>> kms->catalog->perf.min_dram_ib;
>> + perf->core_clk_rate =
Re: [v1] drm/msm/dpu: add support for clk and bw scaling for display
On 2020-07-14 06:42, Matthias Kaehlcke wrote: On Thu, Jun 18, 2020 at 07:38:41PM +0530, Kalyan Thota wrote: This change adds support to scale src clk and bandwidth as per composition requirements. Interconnect registration for bw has been moved to mdp device node from mdss to facilitate the scaling. Changes in v1: - Address armv7 compilation issues with the patch (Rob) Signed-off-by: Kalyan Thota It seems this is an evolution of this series: https://patchwork.kernel.org/project/linux-arm-msm/list/?series=265351 Are the DT bits of the series still valid? If so please include them in the series, otherwise please add DT patches to allow folks to test and review, and get them landed in Bjorn's tree after the driver changes have landed. Hi, Yes the patch is dependent on the DT changes, should i add them with depends tag in the commit text ? https://patchwork.kernel.org/patch/11470785/ https://patchwork.kernel.org/patch/11470789/ - Kalyan ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Freedreno] [v1] drm/msm/dpu: enumerate second cursor pipe for external interface
On 2020-07-10 22:19, Rob Clark wrote:
On Thu, Jun 25, 2020 at 5:46 AM Kalyan Thota
wrote:
Setup an RGB HW pipe as cursor which can be used on
secondary interface.
For SC7180 2 HW pipes are enumerated as cursors
1 - primary interface
2 - secondary interface
Signed-off-by: Kalyan Thota
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
index 8f2357d..23061fd 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -117,10 +117,10 @@
.reg_off = 0x2AC, .bit_off = 0},
.clk_ctrls[DPU_CLK_CTRL_DMA0] = {
.reg_off = 0x2AC, .bit_off = 8},
- .clk_ctrls[DPU_CLK_CTRL_DMA1] = {
- .reg_off = 0x2B4, .bit_off = 8},
.clk_ctrls[DPU_CLK_CTRL_CURSOR0] = {
- .reg_off = 0x2BC, .bit_off = 8},
+ .reg_off = 0x2B4, .bit_off = 8},
+ .clk_ctrls[DPU_CLK_CTRL_CURSOR1] = {
+ .reg_off = 0x2C4, .bit_off = 8},
It looks like you shifted the register offset here from 0x2bc to
0x2c4, was that intentional?
BR,
-R
Yes Rob, the offset was wrong which i corrected in this patch.
},
};
@@ -272,10 +272,10 @@
sc7180_vig_sblk_0, 0, SSPP_TYPE_VIG,
DPU_CLK_CTRL_VIG0),
SSPP_BLK("sspp_8", SSPP_DMA0, 0x24000, DMA_SDM845_MASK,
sdm845_dma_sblk_0, 1, SSPP_TYPE_DMA,
DPU_CLK_CTRL_DMA0),
- SSPP_BLK("sspp_9", SSPP_DMA1, 0x26000, DMA_SDM845_MASK,
- sdm845_dma_sblk_1, 5, SSPP_TYPE_DMA,
DPU_CLK_CTRL_DMA1),
+ SSPP_BLK("sspp_9", SSPP_DMA1, 0x26000,
DMA_CURSOR_SDM845_MASK,
+ sdm845_dma_sblk_1, 5, SSPP_TYPE_DMA,
DPU_CLK_CTRL_CURSOR0),
SSPP_BLK("sspp_10", SSPP_DMA2, 0x28000,
DMA_CURSOR_SDM845_MASK,
- sdm845_dma_sblk_2, 9, SSPP_TYPE_DMA,
DPU_CLK_CTRL_CURSOR0),
+ sdm845_dma_sblk_2, 9, SSPP_TYPE_DMA,
DPU_CLK_CTRL_CURSOR1),
};
/*
--
1.9.1
___
Freedreno mailing list
freedr...@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Freedreno] [v1] drm/msm/dpu: add support for clk and bw scaling for display
On 2020-07-10 22:38, Rob Clark wrote:
On Thu, Jun 18, 2020 at 7:09 AM Kalyan Thota
wrote:
This change adds support to scale src clk and bandwidth as
per composition requirements.
Interconnect registration for bw has been moved to mdp
device node from mdss to facilitate the scaling.
Changes in v1:
- Address armv7 compilation issues with the patch (Rob)
Signed-off-by: Kalyan Thota
---
drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 109
+
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 5 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 4 +
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c| 37 -
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h| 4 +
drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c | 9 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 84
+++
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 4 +
8 files changed, 233 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index 7c230f7..e52bc44 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -29,6 +29,74 @@ enum dpu_perf_mode {
DPU_PERF_MODE_MAX
};
+/**
+ * @_dpu_core_perf_calc_bw() - to calculate BW per crtc
+ * @kms - pointer to the dpu_kms
+ * @crtc - pointer to a crtc
+ * Return: returns aggregated BW for all planes in crtc.
+ */
+static u64 _dpu_core_perf_calc_bw(struct dpu_kms *kms,
+ struct drm_crtc *crtc)
+{
+ struct drm_plane *plane;
+ struct dpu_plane_state *pstate;
+ u64 crtc_plane_bw = 0;
+ u32 bw_factor;
+
+ drm_atomic_crtc_for_each_plane(plane, crtc) {
+ pstate = to_dpu_plane_state(plane->state);
+ if (!pstate)
+ continue;
+
+ crtc_plane_bw += pstate->plane_fetch_bw;
+ }
+
+ bw_factor = kms->catalog->perf.bw_inefficiency_factor;
+ if (bw_factor) {
+ crtc_plane_bw *= bw_factor;
+ do_div(crtc_plane_bw, 100);
+ }
+
+ return crtc_plane_bw;
+}
+
+/**
+ * _dpu_core_perf_calc_clk() - to calculate clock per crtc
+ * @kms - pointer to the dpu_kms
+ * @crtc - pointer to a crtc
+ * @state - pointer to a crtc state
+ * Return: returns max clk for all planes in crtc.
+ */
+static u64 _dpu_core_perf_calc_clk(struct dpu_kms *kms,
+ struct drm_crtc *crtc, struct drm_crtc_state *state)
+{
+ struct drm_plane *plane;
+ struct dpu_plane_state *pstate;
+ struct drm_display_mode *mode;
+ u64 crtc_clk;
+ u32 clk_factor;
+
+ mode = >adjusted_mode;
+
+ crtc_clk = mode->vtotal * mode->hdisplay *
drm_mode_vrefresh(mode);
+
+ drm_atomic_crtc_for_each_plane(plane, crtc) {
+ pstate = to_dpu_plane_state(plane->state);
+ if (!pstate)
+ continue;
+
+ crtc_clk = max(pstate->plane_clk, crtc_clk);
+ }
+
+ clk_factor = kms->catalog->perf.clk_inefficiency_factor;
+ if (clk_factor) {
+ crtc_clk *= clk_factor;
+ do_div(crtc_clk, 100);
+ }
+
+ return crtc_clk;
+}
+
static struct dpu_kms *_dpu_crtc_get_kms(struct drm_crtc *crtc)
{
struct msm_drm_private *priv;
@@ -51,12 +119,7 @@ static void _dpu_core_perf_calc_crtc(struct
dpu_kms *kms,
dpu_cstate = to_dpu_crtc_state(state);
memset(perf, 0, sizeof(struct dpu_core_perf_params));
- if (!dpu_cstate->bw_control) {
- perf->bw_ctl = kms->catalog->perf.max_bw_high *
- 1000ULL;
- perf->max_per_pipe_ib = perf->bw_ctl;
- perf->core_clk_rate = kms->perf.max_core_clk_rate;
- } else if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM)
{
+ if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
perf->bw_ctl = 0;
perf->max_per_pipe_ib = 0;
perf->core_clk_rate = 0;
@@ -64,6 +127,10 @@ static void _dpu_core_perf_calc_crtc(struct
dpu_kms *kms,
perf->bw_ctl = kms->perf.fix_core_ab_vote;
perf->max_per_pipe_ib = kms->perf.fix_core_ib_vote;
perf->core_clk_rate = kms->perf.fix_core_clk_rate;
+ } else {
+ perf->bw_ctl = _dpu_core_perf_calc_bw(kms, crtc);
+ perf->max_per_pipe_ib =
kms->catalog->perf.min_dram_ib;
+ perf->core_clk_rate = _dpu_core_perf_calc_clk(kms,
crtc, state);
}
DPU_DEBUG(
@@ -115,11 +182,7 @@ int dpu_core_perf_crtc_check(struct drm_crtc
*crtc,
DPU_DEBUG("crtc:%d bw:%llu ctrl:%d\n",
tmp_crtc->base.id,
tmp_cstate->new_perf.bw_ctl,
tmp_cstate->bw_control);
- /*
-* For bw check only use the bw if the
-
Re: [Freedreno] [PATCH] drm/msm/dpu: add support for dither block in display
On 2020-06-25 01:55, Rob Clark wrote:
On Wed, Jun 24, 2020 at 4:57 AM Kalyan Thota
wrote:
This change enables dither block for primary interface
in display.
Enabled for 6bpc in the current version.
Signed-off-by: Kalyan Thota
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 45
+
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 66
+
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.h | 28 +++
3 files changed, 130 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 63976dc..26e870a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -208,6 +208,42 @@ struct dpu_encoder_virt {
#define to_dpu_encoder_virt(x) container_of(x, struct
dpu_encoder_virt, base)
+static u32 dither_matrix[DITHER_MATRIX_SZ] = {
+ 15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10
+};
+
+static void _dpu_encoder_setup_dither(struct dpu_encoder_phys *phys)
+{
+ struct dpu_hw_dither_cfg dither_cfg = { 0 };
+ struct drm_display_info *info;
+
+ if (!phys || !phys->connector || !phys->hw_pp ||
+ !phys->hw_pp->ops.setup_dither)
+ return;
it looks like other than phys->hw_pp->ops.setup_dither, you shouldn't
need to check most of these conditions.
+
+ info = >connector->display_info;
+ if (!info)
and definitely not this one
+ return;
+
+ switch (phys->connector->display_info.bpc) {
+ case 6:
+ dither_cfg.c0_bitdepth = 6;
+ dither_cfg.c1_bitdepth = 6;
+ dither_cfg.c2_bitdepth = 6;
+ dither_cfg.c3_bitdepth = 6;
+ dither_cfg.temporal_en = 0;
+ break;
+ default:
+ phys->hw_pp->ops.setup_dither(phys->hw_pp, NULL);
+ return;
+ }
+
+ memcpy(_cfg.matrix, dither_matrix,
+ sizeof(u32) * DITHER_MATRIX_SZ);
+
+ phys->hw_pp->ops.setup_dither(phys->hw_pp, _cfg);
+}
+
void dpu_encoder_helper_report_irq_timeout(struct dpu_encoder_phys
*phys_enc,
enum dpu_intr_idx intr_idx)
{
@@ -1082,6 +1118,7 @@ static void
_dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
struct dpu_encoder_virt *dpu_enc = NULL;
struct msm_drm_private *priv;
struct dpu_kms *dpu_kms;
+ int i;
if (!drm_enc || !drm_enc->dev) {
DPU_ERROR("invalid parameters\n");
@@ -1104,6 +1141,14 @@ static void
_dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
dpu_kms->catalog);
_dpu_encoder_update_vsync_source(dpu_enc,
_enc->disp_info);
+
+ if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {
+ for (i = 0; i < dpu_enc->num_phys_encs; i++) {
+ struct dpu_encoder_phys *phys =
dpu_enc->phys_encs[i];
+
+ _dpu_encoder_setup_dither(phys);
+ }
+ }
}
void dpu_encoder_virt_runtime_resume(struct drm_encoder *drm_enc)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
index d110a40..cf7603d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
@@ -28,6 +28,16 @@
#define PP_FBC_BUDGET_CTL 0x038
#define PP_FBC_LOSSY_MODE 0x03C
+#define PP_DITHER_EN 0x000
+#define PP_DITHER_BITDEPTH 0x004
+#define PP_DITHER_MATRIX 0x008
+
+#define DITHER_DEPTH_MAP_INDEX 9
+
+static u32 dither_depth_map[DITHER_DEPTH_MAP_INDEX] = {
+ 0, 0, 0, 0, 0, 0, 0, 1, 2
+};
+
static const struct dpu_pingpong_cfg *_pingpong_offset(enum
dpu_pingpong pp,
const struct dpu_mdss_cfg *m,
void __iomem *addr,
@@ -49,6 +59,40 @@ static const struct dpu_pingpong_cfg
*_pingpong_offset(enum dpu_pingpong pp,
return ERR_PTR(-EINVAL);
}
+static void dpu_hw_pp_setup_dither(struct dpu_hw_pingpong *pp,
+ struct dpu_hw_dither_cfg *cfg)
+{
+ struct dpu_hw_blk_reg_map *c;
+ u32 i, base, data = 0;
+
+ if (!pp)
+ return;
can this ever be NULL.. at least currently you are checking this both
here and in _dpu_encoder_setup_dither()
BR,
-R
Sure, will fix them. I guess got habituated to add checks so that static
code analyzers don't complain :-)
Kalyan
+
+ c = >hw;
+ base = pp->caps->sblk->dither.base;
+ if (!cfg) {
+ DPU_REG_WRITE(c, base + PP_DITHER_EN, 0);
+ return;
+ }
+
+ data = dither_depth_map[cfg->c0_bitdepth] & REG_MASK(2);
+ data |= (dither_depth_map[cfg->c1_bitdepth] & REG_MASK(2)) <<
2;
+ data |= (dither_depth_map[cfg->c2_bitdepth] & REG_MASK(2)) <<
4;
+ data |=
Re: [Freedreno] [PATCH] drm/msm/dpu: ensure device suspend happens during PM sleep
On 2020-05-28 03:41, Doug Anderson wrote: Hi, On Fri, May 15, 2020 at 9:37 AM Doug Anderson wrote: Hi, On Fri, May 15, 2020 at 5:06 AM wrote: > > On 2020-05-14 21:47, Doug Anderson wrote: > > Hi, > > > > On Fri, May 1, 2020 at 6:31 AM Kalyan Thota > > wrote: > >> > >> "The PM core always increments the runtime usage counter > >> before calling the ->suspend() callback and decrements it > >> after calling the ->resume() callback" > >> > >> DPU and DSI are managed as runtime devices. When > >> suspend is triggered, PM core adds a refcount on all the > >> devices and calls device suspend, since usage count is > >> already incremented, runtime suspend was not getting called > >> and it kept the clocks on which resulted in target not > >> entering into XO shutdown. > >> > >> Add changes to force suspend on runtime devices during pm sleep. > >> > >> Changes in v1: > >> - Remove unnecessary checks in the function > >> _dpu_kms_disable_dpu (Rob Clark). > >> > >> Changes in v2: > >> - Avoid using suspend_late to reset the usagecount > >>as suspend_late might not be called during suspend > >>call failures (Doug). > >> > >> Changes in v3: > >> - Use force suspend instead of managing device usage_count > >>via runtime put and get API's to trigger callbacks (Doug). > >> > >> Changes in v4: > >> - Check the return values of pm_runtime_force_suspend and > >>pm_runtime_force_resume API's and pass appropriately (Doug). > >> > >> Changes in v5: > > > > Can you please put the version number properly in your subject? It's > > really hard to tell one version of your patch from another. > > > > > >> - With v4 patch, test cycle has uncovered issues in device resume. > >> > >>On bubs: cmd tx failures were seen as SW is sending panel off > >>commands when the dsi resources are turned off. > >> > >>Upon suspend, DRM driver will issue a NULL composition to the > >>dpu, followed by turning off all the HW blocks. > >> > >>v5 changes will serialize the NULL commit and resource unwinding > >>by handling them under PM prepare and PM complete phases there by > >>ensuring that clks are on when panel off commands are being > >>processed. > > > > I'm still most definitely not an expert in how all the DRM pieces all > > hook up together, but the solution you have in this patch seems wrong > > to me. As far as I can tell the "prepare" state isn't supposed to be > > actually doing the suspend work and here that's exactly what you're > > doing. I think you should find a different solution to ensure > > ordering is correct. > > > > -Doug > > > > Hi, Quite honestly I'm probably not the right person to be reviewing this code. I mostly just noticed one of your early patches and it looked strange to me. Hopefully someone with actual experience in how all the DRM components work together can actually review and see if this makes sense. Maybe Sean would know better? That being said, let me at least look at what you're saying... > Prepare and Complete are callbacks defined as part of Sleep and Resume > sequence > > Entering PM SUSPEND the phases are : prepare --> suspend --> > suspend_late --> suspend_noirq. > While leaving PM SUSPEND the phases are: resume_noirq --> resume_early > --> resume --> complete. Sure, it's part of the sequence. It's also documented in pm.h as: * The principal role of this callback is to prevent new children of * the device from being registered after it has returned (the driver's * subsystem and generally the rest of the kernel is supposed to prevent * new calls to the probe method from being made too once @prepare() has * succeeded). It does not feel like that matches your usage of this call. > The reason to push drm suspend handling to PM prepare phase is that > parent here will trigger a modeset to turn off the timing and > subsequently the panel. > the child devices should not turn of their clocks before parent unwinds > the composition. Hence they are serialized as per the sequence mentioned > above. So the general model in Linux is that children suspend before their parents, right? So you're saying that, in this case, the parent needs to act on the child before the child suspends. Is that correct? Rather than hijacking the prepare/complete, I'd be at least slightly inclined to move the other driver to turn off its clocks in suspend_late and to turn them back on in resume_early? That seems to be what was done in "analogix_dp-rockchip.c" to solve a similar problem. > A similar approach is taken by other driver that use drm framework. In > this driver, the device registers for prepare and complete callbacks to > handle drm_suspend and drm_resume. > https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/exynos/exynos_drm_drv.c#L163 OK, if there is another driver in DRM then I guess I won't object too strongly. Note that when searching for other drivers I noticed this bit in todo.rst: * Most drivers (except
Re: [Freedreno] [PATCH] drm/msm/dpu: ensure device suspend happens during PM sleep
On 2020-05-14 21:47, Doug Anderson wrote: Hi, On Fri, May 1, 2020 at 6:31 AM Kalyan Thota wrote: "The PM core always increments the runtime usage counter before calling the ->suspend() callback and decrements it after calling the ->resume() callback" DPU and DSI are managed as runtime devices. When suspend is triggered, PM core adds a refcount on all the devices and calls device suspend, since usage count is already incremented, runtime suspend was not getting called and it kept the clocks on which resulted in target not entering into XO shutdown. Add changes to force suspend on runtime devices during pm sleep. Changes in v1: - Remove unnecessary checks in the function _dpu_kms_disable_dpu (Rob Clark). Changes in v2: - Avoid using suspend_late to reset the usagecount as suspend_late might not be called during suspend call failures (Doug). Changes in v3: - Use force suspend instead of managing device usage_count via runtime put and get API's to trigger callbacks (Doug). Changes in v4: - Check the return values of pm_runtime_force_suspend and pm_runtime_force_resume API's and pass appropriately (Doug). Changes in v5: Can you please put the version number properly in your subject? It's really hard to tell one version of your patch from another. - With v4 patch, test cycle has uncovered issues in device resume. On bubs: cmd tx failures were seen as SW is sending panel off commands when the dsi resources are turned off. Upon suspend, DRM driver will issue a NULL composition to the dpu, followed by turning off all the HW blocks. v5 changes will serialize the NULL commit and resource unwinding by handling them under PM prepare and PM complete phases there by ensuring that clks are on when panel off commands are being processed. I'm still most definitely not an expert in how all the DRM pieces all hook up together, but the solution you have in this patch seems wrong to me. As far as I can tell the "prepare" state isn't supposed to be actually doing the suspend work and here that's exactly what you're doing. I think you should find a different solution to ensure ordering is correct. -Doug Hi, Prepare and Complete are callbacks defined as part of Sleep and Resume sequence Entering PM SUSPEND the phases are : prepare --> suspend --> suspend_late --> suspend_noirq. While leaving PM SUSPEND the phases are: resume_noirq --> resume_early --> resume --> complete. The reason to push drm suspend handling to PM prepare phase is that parent here will trigger a modeset to turn off the timing and subsequently the panel. the child devices should not turn of their clocks before parent unwinds the composition. Hence they are serialized as per the sequence mentioned above. A similar approach is taken by other driver that use drm framework. In this driver, the device registers for prepare and complete callbacks to handle drm_suspend and drm_resume. https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/exynos/exynos_drm_drv.c#L163 Kalyan ___ Freedreno mailing list freedr...@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/msm/dpu: ensure device suspend happens during PM sleep
On 2020-03-31 21:30, Doug Anderson wrote:
Hi,
On Tue, Mar 31, 2020 at 6:58 AM Kalyan Thota
wrote:
"The PM core always increments the runtime usage counter
before calling the ->suspend() callback and decrements it
after calling the ->resume() callback"
DPU and DSI are managed as runtime devices. When
suspend is triggered, PM core adds a refcount on all the
devices and calls device suspend, since usage count is
already incremented, runtime suspend was not getting called
and it kept the clocks on which resulted in target not
entering into XO shutdown.
Add changes to force suspend on runtime devices during pm sleep.
Changes in v1:
- Remove unnecessary checks in the function
_dpu_kms_disable_dpu (Rob Clark).
Changes in v2:
- Avoid using suspend_late to reset the usagecount
as suspend_late might not be called during suspend
call failures (Doug).
Changes in v3:
- Use force suspend instead of managing device usage_count
via runtime put and get API's to trigger callbacks (Doug).
Signed-off-by: Kalyan Thota
---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 2 ++
drivers/gpu/drm/msm/dsi/dsi.c | 2 ++
drivers/gpu/drm/msm/msm_drv.c | 4
3 files changed, 8 insertions(+)
This looks much saner to me. Thanks! I assume it still works fine
for you? I'm still no expert on how all the pieces of DRM drivers
work together, but at least there's not a bunch of strange fiddling
with pm_runtime state and hopefully it will avoid weird corner
cases...
--- Yes, verified the change on trogdor device, and display can suspend
with the change.
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index ce19f1d..b886d9d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -1123,6 +1123,8 @@ static int __maybe_unused
dpu_runtime_resume(struct device *dev)
static const struct dev_pm_ops dpu_pm_ops = {
SET_RUNTIME_PM_OPS(dpu_runtime_suspend, dpu_runtime_resume,
NULL)
+ SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+ pm_runtime_force_resume)
};
static const struct of_device_id dpu_dt_match[] = {
diff --git a/drivers/gpu/drm/msm/dsi/dsi.c
b/drivers/gpu/drm/msm/dsi/dsi.c
index 55ea4bc2..62704885 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c
@@ -161,6 +161,8 @@ static int dsi_dev_remove(struct platform_device
*pdev)
static const struct dev_pm_ops dsi_pm_ops = {
SET_RUNTIME_PM_OPS(msm_dsi_runtime_suspend,
msm_dsi_runtime_resume, NULL)
+ SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+ pm_runtime_force_resume)
};
static struct platform_driver dsi_driver = {
diff --git a/drivers/gpu/drm/msm/msm_drv.c
b/drivers/gpu/drm/msm/msm_drv.c
index 7d985f8..2b8c99c 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -1051,6 +1051,8 @@ static int msm_pm_suspend(struct device *dev)
return ret;
}
+ pm_runtime_force_suspend(dev);
nit: check return value of pm_runtime_force_suspend()?
+
return 0;
}
@@ -1063,6 +1065,8 @@ static int msm_pm_resume(struct device *dev)
if (WARN_ON(!priv->pm_state))
return -ENOENT;
+ pm_runtime_force_resume(dev);
nit: check return value of pm_runtime_force_resume()?
-Doug
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Freedreno] [PATCH] drm/msm/dpu: ensure device suspend happens during PM sleep
On 2020-03-31 00:25, Doug Anderson wrote:
Hi,
On Mon, Mar 30, 2020 at 2:04 AM Kalyan Thota
wrote:
"The PM core always increments the runtime usage counter
before calling the ->suspend() callback and decrements it
after calling the ->resume() callback"
DPU and DSI are managed as runtime devices. When
suspend is triggered, PM core adds a refcount on all the
devices and calls device suspend, since usage count is
already incremented, runtime suspend was not getting called
and it kept the clocks on which resulted in target not
entering into XO shutdown.
Add changes to manage runtime devices during pm sleep.
Changes in v1:
- Remove unnecessary checks in the function
_dpu_kms_disable_dpu (Rob Clark).
Changes in v2:
- Avoid using suspend_late to reset the usagecount
as suspend_late might not be called during suspend
call failures (Doug).
Signed-off-by: Kalyan Thota
---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 33
+
drivers/gpu/drm/msm/msm_drv.c | 4
drivers/gpu/drm/msm/msm_kms.h | 2 ++
3 files changed, 39 insertions(+)
I am still 100% baffled by your patch and I never did quite understand
your response to my previous comments [1]. I think you're saying that
the problem you were facing is that if you call "suspend" but never
called "runtime_suspend" that the device stays active. Is that right?
If that's true, did you try something like this suggestion I made?
SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
pm_runtime_force_resume)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index ce19f1d..2343cbd 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -26,6 +26,7 @@
#include "dpu_encoder.h"
#include "dpu_plane.h"
#include "dpu_crtc.h"
+#include "dsi.h"
#define CREATE_TRACE_POINTS
#include "dpu_trace.h"
@@ -325,6 +326,37 @@ static void dpu_kms_disable_commit(struct msm_kms
*kms)
pm_runtime_put_sync(_kms->pdev->dev);
}
+static void _dpu_kms_disable_dpu(struct msm_kms *kms)
+{
+ struct dpu_kms *dpu_kms = to_dpu_kms(kms);
+ struct drm_device *dev = dpu_kms->dev;
+ struct msm_drm_private *priv = dev->dev_private;
+ struct msm_dsi *dsi;
+ int i;
+
+ dpu_kms_disable_commit(kms);
+
+ for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {
+ if (!priv->dsi[i])
+ continue;
+ dsi = priv->dsi[i];
+ pm_runtime_put_sync(>pdev->dev);
+ }
+ pm_runtime_put_sync(dev->dev);
+
+ /* Increment the usagecount without triggering a resume */
+ pm_runtime_get_noresume(dev->dev);
+
+ pm_runtime_get_noresume(_kms->pdev->dev);
+
+ for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {
+ if (!priv->dsi[i])
+ continue;
+ dsi = priv->dsi[i];
+ pm_runtime_get_noresume(>pdev->dev);
+ }
+}
My pm_runtime knowledge is pretty weak sometimes, but the above
function looks crazy. Maybe it's just me not understanding, but can
you please summarize what you're trying to accomplish?
-- I was trying to get the runtime callbacks via controlling the device
usage_count
Since the usage_count was already incremented by PM core, i was
decrementing and incrementing (without resume)
so that callbacks are triggered.
I have taken your suggestion on forcing the suspend instead of managing
it via usage_count.
i'll follow it up in the next patchset.
-Doug
[1]
https://lore.kernel.org/r/114130f68c494f83303c51157e2c5...@codeaurora.org
___
Freedreno mailing list
freedr...@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/msm/dpu: ensure device suspend happens during PM sleep
On 2020-03-25 21:20, Doug Anderson wrote:
Hi,
On Wed, Mar 25, 2020 at 8:40 AM Rob Clark wrote:
On Tue, Mar 24, 2020 at 7:35 AM Doug Anderson
wrote:
>
> Hi,
>
> On Sun, Mar 22, 2020 at 11:14 PM Kalyan Thota wrote:
> >
> > "The PM core always increments the runtime usage counter
> > before calling the ->suspend() callback and decrements it
> > after calling the ->resume() callback"
> >
> > DPU and DSI are managed as runtime devices. When
> > suspend is triggered, PM core adds a refcount on all the
> > devices and calls device suspend, since usage count is
> > already incremented, runtime suspend was not getting called
> > and it kept the clocks on which resulted in target not
> > entering into XO shutdown.
> >
> > Add changes to manage runtime devices during pm sleep.
> >
> > Changes in v1:
> > - Remove unnecessary checks in the function
> > _dpu_kms_disable_dpu (Rob Clark).
>
> I'm wondering what happened with my feedback on v1, AKA:
>
>
https://lore.kernel.org/r/CAD=FV=VxzEV40g+ieuEN+7o=34+wm8mho8o7t5za1yosx7s...@mail.gmail.com
>
> Maybe you didn't see it? ...or if you or Rob think I'm way off base
> (always possible) then please tell me so.
>
-- I didn't notice your comments earlier. Apologies !!
At least w/ the current patch, disable_dpu should not be called for
screen-off (although I'd hope if all the screens are off the device
would suspend).
OK, that's good.
-- Rob has answered it, with current change disable_dpu will only be
called during pm_suspend.
But I won't claim to be a pm expert.. so not really
sure if this is the best approach or not. I don't think our
arrangement of sub-devices under a parent is completely abnormal, so
it does feel like there should be a simpler solution..
I think the other arguments about asymmetry are still valid and I've
fixed bugs around this type of thing in the past. For instance, see
commit f7ccbed656f7 ("drm/rockchip: Suspend DP late").
* What happens if suspend is aborted partway through (by getting a
wakeup even as you're suspending, for instance)? In such a case some
of the normal suspend calls will be called but "suspend_late" won't be
called. Does that mess up your counting?
-- I understand this concern, i'll explore a bit more on how to handle
"failed to suspend","early awake"
cases (to restore the usage_count) since suspend_late wont be called.
*From your description, it sure seems like this part of the
runtime_pm.rst doc is relevant to you:
Did I misunderstand and this isn't what you want? Looking a bit
further, maybe the right thing is to use the "SMART_SUSPEND" flag?
-- if you notice in the device_prepare
(https://elixir.bootlin.com/linux/latest/source/drivers/base/power/main.c#L1913)
there is a pm_runtime_get_noresume at L1931, which will increment the
usagecount before triggering client prepare call, hence implementing
prepare wont fetch us much.
This appears to be more for the cases when device is runtime suspended
and suspend followed later
"one example usecase that i can think of, is screen timeout after that
suspend is triggered"
currently the problem i am looking at is that
PM Core does +1 in device prepare
DPU driver does -1 in suspend
DPU driver does +1 in suspend late ( look for right place )
PM core does -1 in device complete
i'll get back after exploring a bit.
-Doug
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
