Re: [Freedreno] [v4,1/3] drm/msm: Add support for GPU cooling
On 10/30/2020 2:18 AM, m...@chromium.org wrote: On Thu, Oct 29, 2020 at 01:37:19PM +0530, Akhil P Oommen wrote: Register GPU as a devfreq cooling device so that it can be passively cooled by the thermal framework. Signed-off-by: Akhil P Oommen Reviewed-by: Matthias Kaehlcke Wait, I did not post a 'Reviewed-by' tag for this patch! I think the patch should be ok, but I'm still not super happy about the resource management involving devfreq in general (see discussion on https://patchwork.freedesktop.org/patch/394291/?series=82476=1). It's not really something introduced by this patch, but if it ever gets fixed releasing the cooling device at the end of msm_gpu_cleanup() after everything else might cause trouble. In summary, I'm supportive of landing this patch, but reluctant to 'sign it off' because of the above. In any case: Tested-by: Matthias Kaehlcke Sorry, Matthias. My mistake. You shared the reviewed tag for the dt-bindings update. Will fix this ASAP. Thanks for verifying this. -Akhil. ___ dri-devel mailing list dri-de...@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 1/4] drm: allow drm_atomic_print_state() to accept any drm_printer
Hi Daniel
On 2020-10-22 03:38, Daniel Vetter wrote:
On Wed, Oct 21, 2020 at 10:01:45PM -0700, Abhinav Kumar wrote:
Currently drm_atomic_print_state() internally allocates and uses a
drm_info printer. Allow it to accept any drm_printer type so that
the API can be leveraged even for taking drm snapshot.
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/drm_atomic.c| 17 -
drivers/gpu/drm/drm_atomic_uapi.c | 4 +++-
drivers/gpu/drm/drm_crtc_internal.h | 4 +++-
3 files changed, 18 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c
b/drivers/gpu/drm/drm_atomic.c
index 58527f151984..e7079a5f439c 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1,6 +1,7 @@
/*
* Copyright (C) 2014 Red Hat
* Copyright (C) 2014 Intel Corp.
+ * Copyright (c) 2020, The Linux Foundation. All rights reserved.
*
* Permission is hereby granted, free of charge, to any person
obtaining a
* copy of this software and associated documentation files (the
"Software"),
@@ -1543,9 +1544,9 @@ int __drm_atomic_helper_set_config(struct
drm_mode_set *set,
}
EXPORT_SYMBOL(__drm_atomic_helper_set_config);
-void drm_atomic_print_state(const struct drm_atomic_state *state)
+void drm_atomic_print_state(const struct drm_atomic_state *state,
+ struct drm_printer *p)
Please add a nice kerneldoc for this newly exported function.
Specifically
this kerneldoc needs to include a warning that state updates after call
drm_atomic_state_helper_commit_hw_done() is unsafe to print using this
function, because it looks at the new state objects. Only the old state
structures will stay like this.
So maybe rename the function to say print_new_state() to make this
completely clear. That way we can eventually add a print_old_state()
when
needed.
Otherwise I think this makes sense, and nicely avoids the locking issue
of
looking at ->state pointers without the right locking.
-Daniel
Thanks for the review, I have addressed these comments and posted a V2.
-Abhinav
{
- struct drm_printer p = drm_info_printer(state->dev->dev);
struct drm_plane *plane;
struct drm_plane_state *plane_state;
struct drm_crtc *crtc;
@@ -1554,17 +1555,23 @@ void drm_atomic_print_state(const struct
drm_atomic_state *state)
struct drm_connector_state *connector_state;
int i;
+ if (!p) {
+ DRM_ERROR("invalid drm printer\n");
+ return;
+ }
+
DRM_DEBUG_ATOMIC("checking %p\n", state);
for_each_new_plane_in_state(state, plane, plane_state, i)
- drm_atomic_plane_print_state(, plane_state);
+ drm_atomic_plane_print_state(p, plane_state);
for_each_new_crtc_in_state(state, crtc, crtc_state, i)
- drm_atomic_crtc_print_state(, crtc_state);
+ drm_atomic_crtc_print_state(p, crtc_state);
for_each_new_connector_in_state(state, connector, connector_state,
i)
- drm_atomic_connector_print_state(, connector_state);
+ drm_atomic_connector_print_state(p, connector_state);
}
+EXPORT_SYMBOL(drm_atomic_print_state);
static void __drm_state_dump(struct drm_device *dev, struct
drm_printer *p,
bool take_locks)
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
b/drivers/gpu/drm/drm_atomic_uapi.c
index 25c269bc4681..d9ae86c92608 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -2,6 +2,7 @@
* Copyright (C) 2014 Red Hat
* Copyright (C) 2014 Intel Corp.
* Copyright (C) 2018 Intel Corp.
+ * Copyright (c) 2020, The Linux Foundation. All rights reserved.
*
* Permission is hereby granted, free of charge, to any person
obtaining a
* copy of this software and associated documentation files (the
"Software"),
@@ -1294,6 +1295,7 @@ int drm_mode_atomic_ioctl(struct drm_device
*dev,
struct drm_out_fence_state *fence_state;
int ret = 0;
unsigned int i, j, num_fences;
+ struct drm_printer p = drm_info_printer(dev->dev);
/* disallow for drivers not supporting atomic: */
if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
@@ -1413,7 +1415,7 @@ int drm_mode_atomic_ioctl(struct drm_device
*dev,
ret = drm_atomic_nonblocking_commit(state);
} else {
if (drm_debug_enabled(DRM_UT_STATE))
- drm_atomic_print_state(state);
+ drm_atomic_print_state(state, );
ret = drm_atomic_commit(state);
}
diff --git a/drivers/gpu/drm/drm_crtc_internal.h
b/drivers/gpu/drm/drm_crtc_internal.h
index da96b2f64d7e..d34215366936 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -5,6 +5,7 @@
* Jesse Barnes
* Copyright © 2014 Intel Corporation
* Daniel Vetter
+ * Copyright (c) 2020, The Linux Foundation. All rights reserved.
*
* Permission is
[Freedreno] [PATCH v2 4/4] drm/msm/dpu: add dpu_dbg points across dpu driver
Add dpu_dbg points across dpu driver to trigger dumps when critical
errors are hit.
changes in v2: none
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 12 ++--
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 5 +++--
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 5 -
3 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index f7f5c258b553..a2ee1af73c9f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: GPL-2.0-only
/*
- * Copyright (c) 2014-2018, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2014-2018, 2020 The Linux Foundation. All rights reserved.
* Copyright (C) 2013 Red Hat
* Author: Rob Clark
*/
@@ -26,6 +26,7 @@
#include "dpu_crtc.h"
#include "dpu_trace.h"
#include "dpu_core_irq.h"
+#include "dpu_dbg.h"
#define DPU_DEBUG_ENC(e, fmt, ...) DPU_DEBUG("enc%d " fmt,\
(e) ? (e)->base.base.id : -1, ##__VA_ARGS__)
@@ -1313,6 +1314,11 @@ static void dpu_encoder_underrun_callback(struct
drm_encoder *drm_enc,
DPU_ATRACE_BEGIN("encoder_underrun_callback");
atomic_inc(_enc->underrun_cnt);
+
+ /* trigger dump only on the first underrun */
+ if (atomic_read(_enc->underrun_cnt) == 1)
+ DPU_DBG_DUMP("all");
+
trace_dpu_enc_underrun_cb(DRMID(drm_enc),
atomic_read(_enc->underrun_cnt));
DPU_ATRACE_END("encoder_underrun_callback");
@@ -1553,8 +1559,10 @@ static void dpu_encoder_helper_hw_reset(struct
dpu_encoder_phys *phys_enc)
ctl->idx);
rc = ctl->ops.reset(ctl);
- if (rc)
+ if (rc) {
DPU_ERROR_ENC(dpu_enc, "ctl %d reset failure\n", ctl->idx);
+ DPU_DBG_DUMP("all");
+ }
phys_enc->enable_state = DPU_ENC_ENABLED;
}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index 8493d68ad841..58f79557b560 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: GPL-2.0-only
/*
- * Copyright (c) 2015-2018 The Linux Foundation. All rights reserved.
+ * Copyright (c) 2015-2018, 2020 The Linux Foundation. All rights reserved.
*/
#define pr_fmt(fmt)"[drm:%s:%d] " fmt, __func__, __LINE__
@@ -9,6 +9,7 @@
#include "dpu_core_irq.h"
#include "dpu_formats.h"
#include "dpu_trace.h"
+#include "dpu_dbg.h"
#define DPU_DEBUG_CMDENC(e, fmt, ...) DPU_DEBUG("enc%d intf%d " fmt, \
(e) && (e)->base.parent ? \
@@ -213,7 +214,7 @@ static int _dpu_encoder_phys_cmd_handle_ppdone_timeout(
phys_enc->hw_ctl->idx - CTL_0,
cmd_enc->pp_timeout_report_cnt,
atomic_read(_enc->pending_kickoff_cnt));
-
+ DPU_DBG_DUMP("all");
dpu_encoder_helper_unregister_irq(phys_enc, INTR_IDX_RDPTR);
}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
index 805e059b50b7..46c5320150fa 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: GPL-2.0-only
-/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
+/* Copyright (c) 2015-2018, 2020 The Linux Foundation. All rights reserved.
*/
#define pr_fmt(fmt)"[drm:%s:%d] " fmt, __func__, __LINE__
@@ -8,6 +8,7 @@
#include "dpu_core_irq.h"
#include "dpu_formats.h"
#include "dpu_trace.h"
+#include "dpu_dbg.h"
#define DPU_DEBUG_VIDENC(e, fmt, ...) DPU_DEBUG("enc%d intf%d " fmt, \
(e) && (e)->parent ? \
@@ -467,6 +468,7 @@ static void dpu_encoder_phys_vid_enable(struct
dpu_encoder_phys *phys_enc)
"update pending flush ctl %d flush_mask 0%x intf_mask 0x%x\n",
ctl->idx - CTL_0, flush_mask, intf_flush_mask);
+ atomic_set(_enc->underrun_cnt, 0);
/* ctl_flush & timing engine enable will be triggered by framework */
if (phys_enc->enable_state == DPU_ENC_DISABLED)
@@ -549,6 +551,7 @@ static void dpu_encoder_phys_vid_prepare_for_kickoff(
if (rc) {
DPU_ERROR_VIDENC(phys_enc, "ctl %d reset failure: %d\n",
ctl->idx, rc);
+ DPU_DBG_DUMP("all");
dpu_encoder_helper_unregister_irq(phys_enc, INTR_IDX_VSYNC);
}
}
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
[Freedreno] [PATCH v2 3/4] drm/msm: register the base address with dpu_dbg module
Register the base address of various dpu sub-modules with the
dpu_dbg module so that it can be dumped out during error scenarios.
changes in v2:
- Fix an issue where the same dsi client was getting registered
multiple times to the dpu_dbg module
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.c | 4 +--
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c| 6 -
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dspp.c | 7 +++--
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 5 +++-
.../gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 6 -
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 8 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c| 7 -
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 12 +++--
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 4 ++-
drivers/gpu/drm/msm/dp/dp_catalog.c | 12 +
drivers/gpu/drm/msm/dp/dp_catalog.h | 4 +++
drivers/gpu/drm/msm/dp/dp_display.c | 2 ++
drivers/gpu/drm/msm/dsi/dsi.c | 1 +
drivers/gpu/drm/msm/dsi/dsi.h | 1 +
drivers/gpu/drm/msm/dsi/dsi_host.c| 15 ++-
drivers/gpu/drm/msm/msm_drv.c | 26 ++-
drivers/gpu/drm/msm/msm_drv.h | 3 ++-
17 files changed, 108 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.c
index f83682668e87..06d28efb45c9 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.c
@@ -263,7 +263,7 @@ int dpu_dbg_reg_register_base(const char *name, void
__iomem *base,
/* Initialize list to make sure check for null list will be valid */
INIT_LIST_HEAD(_base->sub_range_list);
- pr_debug("%s base: %pK max_offset 0x%zX\n", reg_base->name,
+ pr_info("%s base: %pK max_offset 0x%zX\n", reg_base->name,
reg_base->base, reg_base->max_offset);
list_add(_base->reg_base_head, _base->reg_base_list);
@@ -310,7 +310,7 @@ void dpu_dbg_reg_register_dump_range(const char *base_name,
range->xin_id = xin_id;
list_add_tail(>head, _base->sub_range_list);
- pr_debug("base %s, range %s, start 0x%X, end 0x%X\n",
+ pr_info("base_name %s, range_name %s, start 0x%X, end 0x%X\n",
base_name, range->range_name,
range->offset.start, range->offset.end);
}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
index 758c355b4fd8..3a827ea96723 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: GPL-2.0-only
-/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
+/* Copyright (c) 2015-2018, 2020 The Linux Foundation. All rights reserved.
*/
#include
@@ -7,6 +7,7 @@
#include "dpu_hw_ctl.h"
#include "dpu_kms.h"
#include "dpu_trace.h"
+#include "dpu_dbg.h"
#define CTL_LAYER(lm) \
(((lm) == LM_5) ? (0x024) : (((lm) - LM_0) * 0x004))
@@ -588,6 +589,9 @@ struct dpu_hw_ctl *dpu_hw_ctl_init(enum dpu_ctl idx,
dpu_hw_blk_init(>base, DPU_HW_BLK_CTL, idx, _hw_ops);
+ dpu_dbg_reg_register_dump_range(DPU_DBG_NAME, cfg->name, c->hw.blk_off,
+ c->hw.blk_off + c->hw.length, c->hw.xin_id);
+
return c;
}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dspp.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dspp.c
index a7a24539921f..ee9ae02f5e7f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dspp.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dspp.c
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: GPL-2.0-only
-/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
+/* Copyright (c) 2015-2018, 2020 The Linux Foundation. All rights reserved.
*/
#include "dpu_hwio.h"
@@ -7,7 +7,7 @@
#include "dpu_hw_lm.h"
#include "dpu_hw_dspp.h"
#include "dpu_kms.h"
-
+#include "dpu_dbg.h"
/* DSPP_PCC */
#define PCC_EN BIT(0)
@@ -115,6 +115,9 @@ struct dpu_hw_dspp *dpu_hw_dspp_init(enum dpu_dspp idx,
dpu_hw_blk_init(>base, DPU_HW_BLK_DSPP, idx, _hw_ops);
+ dpu_dbg_reg_register_dump_range(DPU_DBG_NAME, cfg->name, c->hw.blk_off,
+ c->hw.blk_off + c->hw.length, c->hw.xin_id);
+
return c;
}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
index 6f0f54588124..d8198d4d42bc 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
@@ -1,11 +1,12 @@
// SPDX-License-Identifier: GPL-2.0-only
-/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
+/* Copyright (c) 2015-2020, The Linux Foundation. All rights reserved.
*/
#include "dpu_hwio.h"
#include "dpu_hw_catalog.h"
#include "dpu_hw_intf.h"
#include "dpu_kms.h"
+#include "dpu_dbg.h"
#define
[Freedreno] [PATCH v2 2/4] drm/msm/dpu: add support to dump dpu registers
Add the dpu_dbg module which adds supports to dump dpu registers
which can be used in case of error conditions.
changes in v2: Fix kbot errors
Reported-by: kernel test robot
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/msm/Makefile | 2 +
drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.c | 316 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.h | 273 +++
drivers/gpu/drm/msm/disp/dpu1/dpu_dbg_util.c | 314 +
.../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h| 4 +-
drivers/gpu/drm/msm/msm_drv.c | 6 +-
6 files changed, 913 insertions(+), 2 deletions(-)
create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.c
create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.h
create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_dbg_util.c
diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
index 340682cd0f32..96bd1398edac 100644
--- a/drivers/gpu/drm/msm/Makefile
+++ b/drivers/gpu/drm/msm/Makefile
@@ -54,6 +54,8 @@ msm-y := \
disp/dpu1/dpu_core_irq.o \
disp/dpu1/dpu_core_perf.o \
disp/dpu1/dpu_crtc.o \
+ disp/dpu1/dpu_dbg.o \
+ disp/dpu1/dpu_dbg_util.o \
disp/dpu1/dpu_encoder.o \
disp/dpu1/dpu_encoder_phys_cmd.o \
disp/dpu1/dpu_encoder_phys_vid.o \
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.c
new file mode 100644
index ..f83682668e87
--- /dev/null
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.c
@@ -0,0 +1,316 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2009-2020, The Linux Foundation. All rights reserved.
+ */
+
+#define pr_fmt(fmt)"[drm:%s:%d] " fmt, __func__, __LINE__
+
+#include "dpu_dbg.h"
+#include "dpu_hw_catalog.h"
+
+/* global dpu debug base structure */
+static struct dpu_dbg_base dpu_dbg;
+
+
+#ifdef CONFIG_DEV_COREDUMP
+static ssize_t dpu_devcoredump_read(char *buffer, loff_t offset,
+ size_t count, void *data, size_t datalen)
+{
+ struct drm_print_iterator iter;
+ struct drm_printer p;
+
+ iter.data = buffer;
+ iter.offset = 0;
+ iter.start = offset;
+ iter.remain = count;
+
+ p = drm_coredump_printer();
+
+ drm_printf(, "---\n");
+
+ drm_printf(, "module: " KBUILD_MODNAME "\n");
+ drm_printf(, "dpu devcoredump\n");
+ drm_printf(, "timestamp %lld\n", ktime_to_ns(dpu_dbg.timestamp));
+
+ dpu_dbg.dpu_dbg_printer =
+ dpu_dbg.enable_reg_dump = DPU_DBG_DUMP_IN_COREDUMP;
+
+ drm_printf(, "===dpu regs\n");
+
+ _dpu_dump_array(_dbg, dpu_dbg.req_dump_blks,
+ ARRAY_SIZE(dpu_dbg.req_dump_blks),
+ dpu_dbg.work_panic, "evtlog_workitem",
+ dpu_dbg.dump_all);
+
+ drm_printf(, "===dpu drm state\n");
+
+ if (dpu_dbg.atomic_state)
+ drm_atomic_print_new_state(dpu_dbg.atomic_state,
+ );
+
+ return count - iter.remain;
+}
+
+static void dpu_devcoredump_free(void *data)
+{
+ if (dpu_dbg.atomic_state) {
+ drm_atomic_state_put(dpu_dbg.atomic_state);
+ dpu_dbg.atomic_state = NULL;
+ }
+ dpu_dbg.coredump_pending = false;
+}
+
+static void dpu_devcoredump_capture_state(void)
+{
+ struct drm_device *ddev;
+ struct drm_modeset_acquire_ctx ctx;
+
+ dpu_dbg.timestamp = ktime_get();
+
+ ddev = dpu_dbg.drm_dev;
+
+ drm_modeset_acquire_init(, 0);
+
+ while (drm_modeset_lock_all_ctx(ddev, ) != 0)
+ drm_modeset_backoff();
+
+ dpu_dbg.atomic_state = drm_atomic_helper_duplicate_state(ddev,
+ );
+ drm_modeset_drop_locks();
+ drm_modeset_acquire_fini();
+}
+#else
+static void dpu_devcoredump_capture_state(void)
+{
+}
+#endif /* CONFIG_DEV_COREDUMP */
+
+/**
+ * _dpu_dump_work - deferred dump work function
+ * @work: work structure
+ */
+static void _dpu_dump_work(struct kthread_work *work)
+{
+ /* reset the enable_reg_dump to default before every dump */
+ dpu_dbg.enable_reg_dump = DEFAULT_REGDUMP;
+
+ _dpu_dump_array(_dbg, dpu_dbg.req_dump_blks,
+ ARRAY_SIZE(dpu_dbg.req_dump_blks),
+ dpu_dbg.work_panic, "evtlog_workitem",
+ dpu_dbg.dump_all);
+
+ dpu_devcoredump_capture_state();
+
+#ifdef CONFIG_DEV_COREDUMP
+ if (dpu_dbg.enable_reg_dump & DPU_DBG_DUMP_IN_MEM) {
+ dev_coredumpm(dpu_dbg.dev, THIS_MODULE, _dbg, 0, GFP_KERNEL,
+ dpu_devcoredump_read, dpu_devcoredump_free);
+ dpu_dbg.coredump_pending = true;
+ }
+#endif
+}
+
+void dpu_dbg_dump(enum dpu_dbg_dump_context dump_mode, const char *name, ...)
+{
+ int i, index = 0;
+ bool do_panic = false;
+ bool dump_all = false;
+ va_list args;
+ char *blk_name = NULL;
+
[Freedreno] [PATCH v2 1/4] drm: allow drm_atomic_print_state() to accept any drm_printer
Currently drm_atomic_print_state() internally allocates and uses a
drm_info printer. Allow it to accept any drm_printer type so that
the API can be leveraged even for taking drm snapshot.
Rename the drm_atomic_print_state() to drm_atomic_print_new_state()
so that it reflects its functionality better.
changes in v2:
- Rename the function drm_atomic_print_state to
drm_atomic_print_new_state and update the commit text
- Fix kbot errors
- Add kernel doc for the newly exported function
Reported-by: kernel test robot
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/drm_atomic.c | 29 +++
drivers/gpu/drm/drm_atomic_uapi.c | 4 ++-
drivers/gpu/drm/drm_crtc_internal.h | 4 ++-
.../gpu/drm/selftests/test-drm_framebuffer.c | 1 +
4 files changed, 31 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 58527f151984..5df7b67ced78 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1,6 +1,7 @@
/*
* Copyright (C) 2014 Red Hat
* Copyright (C) 2014 Intel Corp.
+ * Copyright (c) 2020, The Linux Foundation. All rights reserved.
*
* Permission is hereby granted, free of charge, to any person obtaining a
* copy of this software and associated documentation files (the "Software"),
@@ -1543,9 +1544,21 @@ int __drm_atomic_helper_set_config(struct drm_mode_set
*set,
}
EXPORT_SYMBOL(__drm_atomic_helper_set_config);
-void drm_atomic_print_state(const struct drm_atomic_state *state)
+/**
+ * drm_atomic_print_new_state - prints drm atomic state
+ * @state: atomic configuration to check
+ * @p: drm printer
+ *
+ * This functions prints the drm atomic state snapshot using the drm printer
+ * which is passed to it. This snapshot can be used for debugging purposes.
+ *
+ * Note that this function looks into the new state objects and hence its not
+ * safe to be used after the call to drm_atomic_helper_commit_hw_done().
+ *
+ */
+void drm_atomic_print_new_state(const struct drm_atomic_state *state,
+ struct drm_printer *p)
{
- struct drm_printer p = drm_info_printer(state->dev->dev);
struct drm_plane *plane;
struct drm_plane_state *plane_state;
struct drm_crtc *crtc;
@@ -1554,17 +1567,23 @@ void drm_atomic_print_state(const struct
drm_atomic_state *state)
struct drm_connector_state *connector_state;
int i;
+ if (!p) {
+ DRM_ERROR("invalid drm printer\n");
+ return;
+ }
+
DRM_DEBUG_ATOMIC("checking %p\n", state);
for_each_new_plane_in_state(state, plane, plane_state, i)
- drm_atomic_plane_print_state(, plane_state);
+ drm_atomic_plane_print_state(p, plane_state);
for_each_new_crtc_in_state(state, crtc, crtc_state, i)
- drm_atomic_crtc_print_state(, crtc_state);
+ drm_atomic_crtc_print_state(p, crtc_state);
for_each_new_connector_in_state(state, connector, connector_state, i)
- drm_atomic_connector_print_state(, connector_state);
+ drm_atomic_connector_print_state(p, connector_state);
}
+EXPORT_SYMBOL(drm_atomic_print_new_state);
static void __drm_state_dump(struct drm_device *dev, struct drm_printer *p,
bool take_locks)
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
b/drivers/gpu/drm/drm_atomic_uapi.c
index 25c269bc4681..b4b3cb28a8ea 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -2,6 +2,7 @@
* Copyright (C) 2014 Red Hat
* Copyright (C) 2014 Intel Corp.
* Copyright (C) 2018 Intel Corp.
+ * Copyright (c) 2020, The Linux Foundation. All rights reserved.
*
* Permission is hereby granted, free of charge, to any person obtaining a
* copy of this software and associated documentation files (the "Software"),
@@ -1294,6 +1295,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
struct drm_out_fence_state *fence_state;
int ret = 0;
unsigned int i, j, num_fences;
+ struct drm_printer p = drm_info_printer(dev->dev);
/* disallow for drivers not supporting atomic: */
if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
@@ -1413,7 +1415,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
ret = drm_atomic_nonblocking_commit(state);
} else {
if (drm_debug_enabled(DRM_UT_STATE))
- drm_atomic_print_state(state);
+ drm_atomic_print_new_state(state, );
ret = drm_atomic_commit(state);
}
diff --git a/drivers/gpu/drm/drm_crtc_internal.h
b/drivers/gpu/drm/drm_crtc_internal.h
index da96b2f64d7e..2bd56ec9fb0e 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -5,6 +5,7 @@
* Jesse Barnes
* Copyright © 2014 Intel Corporation
* Daniel Vetter
+ * Copyright (c) 2020, The Linux
[Freedreno] [PATCH v2 0/4] Add devcoredump support for DPU
This series adds support to use devcoredump for DPU driver. It introduces the dpu_dbg module which assists in the capturing of register dumps during error scenarios. When a display related error happens, the dpu_dbg module captures all the relevant register dumps along with the snapshot of the drm atomic state and triggers a devcoredump. changes in v2: - Fix kbot errors - Rename drm_atomic_print_state function and add kernel doc for it - Fix multiple dsi registration issue with dpu_dbg Abhinav Kumar (4): drm: allow drm_atomic_print_state() to accept any drm_printer drm/msm/dpu: add support to dump dpu registers drm/msm: register the base address with dpu_dbg module drm/msm/dpu: add dpu_dbg points across dpu driver drivers/gpu/drm/drm_atomic.c | 29 +- drivers/gpu/drm/drm_atomic_uapi.c | 4 +- drivers/gpu/drm/drm_crtc_internal.h | 4 +- drivers/gpu/drm/msm/Makefile | 2 + drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.c | 316 ++ drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.h | 273 +++ drivers/gpu/drm/msm/disp/dpu1/dpu_dbg_util.c | 314 + drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 12 +- .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 5 +- .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 5 +- .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h| 4 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c| 6 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dspp.c | 7 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 5 +- .../gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 6 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 8 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c| 7 +- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 12 +- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 4 +- drivers/gpu/drm/msm/dp/dp_catalog.c | 12 + drivers/gpu/drm/msm/dp/dp_catalog.h | 4 + drivers/gpu/drm/msm/dp/dp_display.c | 2 + drivers/gpu/drm/msm/dsi/dsi.c | 1 + drivers/gpu/drm/msm/dsi/dsi.h | 1 + drivers/gpu/drm/msm/dsi/dsi_host.c| 15 +- drivers/gpu/drm/msm/msm_drv.c | 30 +- drivers/gpu/drm/msm/msm_drv.h | 3 +- .../gpu/drm/selftests/test-drm_framebuffer.c | 1 + 28 files changed, 1066 insertions(+), 26 deletions(-) create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.c create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_dbg.h create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_dbg_util.c -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH] drm/msm/dp: do not notify audio subsystem if sink doesn't support audio
For sinks that do not support audio, there is no need to notify
audio subsystem of the connection event.
This will make sure that audio routes only to the primary display
when connected to such sinks.
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/msm/dp/dp_display.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 4a5735564be2..d970980b0ca5 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -555,8 +555,16 @@ static int dp_connect_pending_timeout(struct
dp_display_private *dp, u32 data)
static void dp_display_handle_plugged_change(struct msm_dp *dp_display,
bool plugged)
{
- if (dp_display->plugged_cb && dp_display->codec_dev)
- dp_display->plugged_cb(dp_display->codec_dev, plugged);
+ struct dp_display_private *dp;
+
+ dp = container_of(g_dp_display,
+ struct dp_display_private, dp_display);
+
+ if (dp_display->plugged_cb && dp_display->codec_dev) {
+ /* notify audio subsystem only if sink supports audio */
+ if (dp->audio_supported)
+ dp_display->plugged_cb(dp_display->codec_dev, plugged);
+ }
}
static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [v4,1/3] drm/msm: Add support for GPU cooling
On Thu, Oct 29, 2020 at 01:37:19PM +0530, Akhil P Oommen wrote: > Register GPU as a devfreq cooling device so that it can be passively > cooled by the thermal framework. > > Signed-off-by: Akhil P Oommen > Reviewed-by: Matthias Kaehlcke Wait, I did not post a 'Reviewed-by' tag for this patch! I think the patch should be ok, but I'm still not super happy about the resource management involving devfreq in general (see discussion on https://patchwork.freedesktop.org/patch/394291/?series=82476=1). It's not really something introduced by this patch, but if it ever gets fixed releasing the cooling device at the end of msm_gpu_cleanup() after everything else might cause trouble. In summary, I'm supportive of landing this patch, but reluctant to 'sign it off' because of the above. In any case: Tested-by: Matthias Kaehlcke ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH 1/2] drm: msm: adreno: use DEFINE_DEBUGFS_ATTRIBUTE with debugfs_create_file_unsafe()
Using DEFINE_DEBUGFS_ATTRIBUTE macro with debugfs_create_file_unsafe()
function in place of the debugfs_create_file() function will make the
file operation struct "reset" aware of the file's lifetime. Additional
details here: https://lists.archive.carbon60.com/linux/kernel/2369498
Issue reported by Coccinelle script:
scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci
Signed-off-by: Deepak R Varma
---
Please Note: This is project task specific patch.
drivers/gpu/drm/msm/adreno/a5xx_debugfs.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a5xx_debugfs.c
b/drivers/gpu/drm/msm/adreno/a5xx_debugfs.c
index fc2c905b6c9e..ffe1fb9be155 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_debugfs.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_debugfs.c
@@ -138,7 +138,7 @@ reset_set(void *data, u64 val)
return 0;
}
-DEFINE_SIMPLE_ATTRIBUTE(reset_fops, NULL, reset_set, "%llx\n");
+DEFINE_DEBUGFS_ATTRIBUTE(reset_fops, NULL, reset_set, "%llx\n");
void a5xx_debugfs_init(struct msm_gpu *gpu, struct drm_minor *minor)
@@ -154,6 +154,6 @@ void a5xx_debugfs_init(struct msm_gpu *gpu, struct
drm_minor *minor)
ARRAY_SIZE(a5xx_debugfs_list),
minor->debugfs_root, minor);
- debugfs_create_file("reset", S_IWUGO, minor->debugfs_root, dev,
- _fops);
+ debugfs_create_file_unsafe("reset", S_IWUGO, minor->debugfs_root, dev,
+ _fops);
}
--
2.25.1
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH 2/2] drm: msm: adreno: improve code indentation & alignment
Align instructions split across multiple lines as per the coding
standards. Issue flagged by checkpatch script.
Signed-off-by: Deepak R Varma
---
Please note: This is a project task specific patch.
drivers/gpu/drm/msm/adreno/a5xx_debugfs.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a5xx_debugfs.c
b/drivers/gpu/drm/msm/adreno/a5xx_debugfs.c
index ffe1fb9be155..ac9296f314be 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_debugfs.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_debugfs.c
@@ -20,7 +20,7 @@ static void pfp_print(struct msm_gpu *gpu, struct drm_printer
*p)
for (i = 0; i < 36; i++) {
gpu_write(gpu, REG_A5XX_CP_PFP_STAT_ADDR, i);
drm_printf(p, " %02x: %08x\n", i,
- gpu_read(gpu, REG_A5XX_CP_PFP_STAT_DATA));
+ gpu_read(gpu, REG_A5XX_CP_PFP_STAT_DATA));
}
}
@@ -33,7 +33,7 @@ static void me_print(struct msm_gpu *gpu, struct drm_printer
*p)
for (i = 0; i < 29; i++) {
gpu_write(gpu, REG_A5XX_CP_ME_STAT_ADDR, i);
drm_printf(p, " %02x: %08x\n", i,
- gpu_read(gpu, REG_A5XX_CP_ME_STAT_DATA));
+ gpu_read(gpu, REG_A5XX_CP_ME_STAT_DATA));
}
}
@@ -46,7 +46,7 @@ static void meq_print(struct msm_gpu *gpu, struct drm_printer
*p)
for (i = 0; i < 64; i++) {
drm_printf(p, " %02x: %08x\n", i,
- gpu_read(gpu, REG_A5XX_CP_MEQ_DBG_DATA));
+ gpu_read(gpu, REG_A5XX_CP_MEQ_DBG_DATA));
}
}
@@ -63,7 +63,7 @@ static void roq_print(struct msm_gpu *gpu, struct drm_printer
*p)
for (j = 0; j < 4; j++)
val[j] = gpu_read(gpu, REG_A5XX_CP_ROQ_DBG_DATA);
drm_printf(p, " %02x: %08x %08x %08x %08x\n", i,
- val[0], val[1], val[2], val[3]);
+ val[0], val[1], val[2], val[3]);
}
}
@@ -155,5 +155,5 @@ void a5xx_debugfs_init(struct msm_gpu *gpu, struct
drm_minor *minor)
minor->debugfs_root, minor);
debugfs_create_file_unsafe("reset", S_IWUGO, minor->debugfs_root, dev,
- _fops);
+ _fops);
}
--
2.25.1
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v4 23/23] drm/msm: Don't implicit-sync if only a single ring
On Thu, Oct 29, 2020 at 09:59:09AM -0700, Rob Clark wrote:
> On Thu, Oct 29, 2020 at 9:14 AM Daniel Vetter wrote:
> >
> > On Mon, Oct 26, 2020 at 10:34 AM Daniel Vetter wrote:
> > >
> > > On Fri, Oct 23, 2020 at 08:49:14PM -0700, Rob Clark wrote:
> > > > On Fri, Oct 23, 2020 at 11:20 AM Lucas Stach
> > > > wrote:
> > > > >
> > > > > On Fr, 2020-10-23 at 09:51 -0700, Rob Clark wrote:
> > > > > > From: Rob Clark
> > > > > >
> > > > > > If there is only a single ring (no-preemption), everything is FIFO
> > > > > > order
> > > > > > and there is no need to implicit-sync.
> > > > > >
> > > > > > Mesa should probably just always use MSM_SUBMIT_NO_IMPLICIT, as
> > > > > > behavior
> > > > > > is undefined when fences are not used to synchronize buffer usage
> > > > > > across
> > > > > > contexts (which is the only case where multiple different priority
> > > > > > rings
> > > > > > could come into play).
> > > > >
> > > > > Really, doesn't this break cross-device implicit sync? Okay, you may
> > > > > not have many peripherals that rely on implicit sync on devices where
> > > > > Adreno is usually found, but it seems rather heavy-handed.
> > > > >
> > > > > Wouldn't it be better to only ignore fences from your own ring context
> > > > > in the implicit sync, like we do in the common DRM scheduler
> > > > > (drm_sched_dependency_optimized)?
> > > >
> > > > we already do this.. as was discussed on an earlier iteration of this
> > > > patchset
> > > >
> > > > But I'm not aware of any other non-gpu related implicit sync use-case
> > > > (even on imx devices where display is decoupled from gpu).. I'll
> > > > revert the patch if someone comes up with one, but otherwise lets let
> > > > the implicit sync baggage die
> > >
> > > The thing is, dma_resv won't die, even if implicit sync is dead. We're
> > > using internally for activity tracking and memory management. If you don't
> > > set these, then we can't share generic code with msm, and I think everyone
> > > inventing their own memory management is a bit a mistake.
> > >
> > > Now you only kill the implicit write sync stuff here, but I'm not sure
> > > that's worth much since you still install all the read fences for
> > > consistency. And if userspace doesn't want to be synced, they can set the
> > > flag and do this on their own: I think you should be able to achieve
> > > exactly the same thing in mesa.
> > >
> > > Aside: If you're worried about overhead, you can do O(1) submit if you
> > > manage your ppgtt like amdgpu does.
> >
> > So just remember a use-case which is maybe a bit yucky, but it is
> > actually possible to implement race-free. If you have implicit sync.
> >
> > There's screen-capture tool in mplayer and obs which capture your
> > compositor by running getfb2 in a loop. It works, and after some
> > initial screaming I realized it does actually work race-free. If you
> > have implicit sync.
> >
> > I really don't think you can sunset this, as much as you want to. And
> > sunsetting it inconsistently is probably the worst.
>
> For the case where you only have a single ring, as long as it is
> importing the fb in to egl to read it (which it would need to do to
> get a linear view), this would still all work
Hm right we still have the implicit sync of the ringbuffer. At least until
you add a submit scheduler to msm ...
> (but I may drop this patch because it is just a micro-optimization and
> seems to cause more confusion)
Yeah I'd say without numbers to justify it it feels a bit on thin ice :-)
-Daniel
>
> BR,
> -R
>
>
> > -Daniel
> >
> > > -Daniel
> > >
> > > >
> > > > BR,
> > > > -R
> > > >
> > > >
> > > >
> > > > >
> > > > > Regards,
> > > > > Lucas
> > > > >
> > > > > > Signed-off-by: Rob Clark
> > > > > > Reviewed-by: Kristian H. Kristensen
> > > > > > ---
> > > > > > drivers/gpu/drm/msm/msm_gem_submit.c | 7 ---
> > > > > > 1 file changed, 4 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c
> > > > > > b/drivers/gpu/drm/msm/msm_gem_submit.c
> > > > > > index d04c349d8112..b6babc7f9bb8 100644
> > > > > > --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> > > > > > +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> > > > > > @@ -283,7 +283,7 @@ static int submit_lock_objects(struct
> > > > > > msm_gem_submit *submit)
> > > > > > return ret;
> > > > > > }
> > > > > >
> > > > > > -static int submit_fence_sync(struct msm_gem_submit *submit, bool
> > > > > > no_implicit)
> > > > > > +static int submit_fence_sync(struct msm_gem_submit *submit, bool
> > > > > > implicit_sync)
> > > > > > {
> > > > > > int i, ret = 0;
> > > > > >
> > > > > > @@ -303,7 +303,7 @@ static int submit_fence_sync(struct
> > > > > > msm_gem_submit *submit, bool no_implicit)
> > > > > > return ret;
> > > > > > }
> > > > > >
> > > > > > - if (no_implicit)
> > > > > > + if (!implicit_sync)
> > > > > >
Re: [Freedreno] [PATCH v4 23/23] drm/msm: Don't implicit-sync if only a single ring
On Thu, Oct 29, 2020 at 9:14 AM Daniel Vetter wrote:
>
> On Mon, Oct 26, 2020 at 10:34 AM Daniel Vetter wrote:
> >
> > On Fri, Oct 23, 2020 at 08:49:14PM -0700, Rob Clark wrote:
> > > On Fri, Oct 23, 2020 at 11:20 AM Lucas Stach
> > > wrote:
> > > >
> > > > On Fr, 2020-10-23 at 09:51 -0700, Rob Clark wrote:
> > > > > From: Rob Clark
> > > > >
> > > > > If there is only a single ring (no-preemption), everything is FIFO
> > > > > order
> > > > > and there is no need to implicit-sync.
> > > > >
> > > > > Mesa should probably just always use MSM_SUBMIT_NO_IMPLICIT, as
> > > > > behavior
> > > > > is undefined when fences are not used to synchronize buffer usage
> > > > > across
> > > > > contexts (which is the only case where multiple different priority
> > > > > rings
> > > > > could come into play).
> > > >
> > > > Really, doesn't this break cross-device implicit sync? Okay, you may
> > > > not have many peripherals that rely on implicit sync on devices where
> > > > Adreno is usually found, but it seems rather heavy-handed.
> > > >
> > > > Wouldn't it be better to only ignore fences from your own ring context
> > > > in the implicit sync, like we do in the common DRM scheduler
> > > > (drm_sched_dependency_optimized)?
> > >
> > > we already do this.. as was discussed on an earlier iteration of this
> > > patchset
> > >
> > > But I'm not aware of any other non-gpu related implicit sync use-case
> > > (even on imx devices where display is decoupled from gpu).. I'll
> > > revert the patch if someone comes up with one, but otherwise lets let
> > > the implicit sync baggage die
> >
> > The thing is, dma_resv won't die, even if implicit sync is dead. We're
> > using internally for activity tracking and memory management. If you don't
> > set these, then we can't share generic code with msm, and I think everyone
> > inventing their own memory management is a bit a mistake.
> >
> > Now you only kill the implicit write sync stuff here, but I'm not sure
> > that's worth much since you still install all the read fences for
> > consistency. And if userspace doesn't want to be synced, they can set the
> > flag and do this on their own: I think you should be able to achieve
> > exactly the same thing in mesa.
> >
> > Aside: If you're worried about overhead, you can do O(1) submit if you
> > manage your ppgtt like amdgpu does.
>
> So just remember a use-case which is maybe a bit yucky, but it is
> actually possible to implement race-free. If you have implicit sync.
>
> There's screen-capture tool in mplayer and obs which capture your
> compositor by running getfb2 in a loop. It works, and after some
> initial screaming I realized it does actually work race-free. If you
> have implicit sync.
>
> I really don't think you can sunset this, as much as you want to. And
> sunsetting it inconsistently is probably the worst.
For the case where you only have a single ring, as long as it is
importing the fb in to egl to read it (which it would need to do to
get a linear view), this would still all work
(but I may drop this patch because it is just a micro-optimization and
seems to cause more confusion)
BR,
-R
> -Daniel
>
> > -Daniel
> >
> > >
> > > BR,
> > > -R
> > >
> > >
> > >
> > > >
> > > > Regards,
> > > > Lucas
> > > >
> > > > > Signed-off-by: Rob Clark
> > > > > Reviewed-by: Kristian H. Kristensen
> > > > > ---
> > > > > drivers/gpu/drm/msm/msm_gem_submit.c | 7 ---
> > > > > 1 file changed, 4 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c
> > > > > b/drivers/gpu/drm/msm/msm_gem_submit.c
> > > > > index d04c349d8112..b6babc7f9bb8 100644
> > > > > --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> > > > > +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> > > > > @@ -283,7 +283,7 @@ static int submit_lock_objects(struct
> > > > > msm_gem_submit *submit)
> > > > > return ret;
> > > > > }
> > > > >
> > > > > -static int submit_fence_sync(struct msm_gem_submit *submit, bool
> > > > > no_implicit)
> > > > > +static int submit_fence_sync(struct msm_gem_submit *submit, bool
> > > > > implicit_sync)
> > > > > {
> > > > > int i, ret = 0;
> > > > >
> > > > > @@ -303,7 +303,7 @@ static int submit_fence_sync(struct
> > > > > msm_gem_submit *submit, bool no_implicit)
> > > > > return ret;
> > > > > }
> > > > >
> > > > > - if (no_implicit)
> > > > > + if (!implicit_sync)
> > > > > continue;
> > > > >
> > > > > ret = msm_gem_sync_object(_obj->base,
> > > > > submit->ring->fctx,
> > > > > @@ -774,7 +774,8 @@ int msm_ioctl_gem_submit(struct drm_device *dev,
> > > > > void *data,
> > > > > if (ret)
> > > > > goto out;
> > > > >
> > > > > - ret = submit_fence_sync(submit, !!(args->flags &
> > > > > MSM_SUBMIT_NO_IMPLICIT));
> > > > > + ret = submit_fence_sync(submit, (gpu->nr_rings > 1) &&
> > > > > +
Re: [Freedreno] [PATCH v4 23/23] drm/msm: Don't implicit-sync if only a single ring
On Mon, Oct 26, 2020 at 10:34 AM Daniel Vetter wrote:
>
> On Fri, Oct 23, 2020 at 08:49:14PM -0700, Rob Clark wrote:
> > On Fri, Oct 23, 2020 at 11:20 AM Lucas Stach wrote:
> > >
> > > On Fr, 2020-10-23 at 09:51 -0700, Rob Clark wrote:
> > > > From: Rob Clark
> > > >
> > > > If there is only a single ring (no-preemption), everything is FIFO order
> > > > and there is no need to implicit-sync.
> > > >
> > > > Mesa should probably just always use MSM_SUBMIT_NO_IMPLICIT, as behavior
> > > > is undefined when fences are not used to synchronize buffer usage across
> > > > contexts (which is the only case where multiple different priority rings
> > > > could come into play).
> > >
> > > Really, doesn't this break cross-device implicit sync? Okay, you may
> > > not have many peripherals that rely on implicit sync on devices where
> > > Adreno is usually found, but it seems rather heavy-handed.
> > >
> > > Wouldn't it be better to only ignore fences from your own ring context
> > > in the implicit sync, like we do in the common DRM scheduler
> > > (drm_sched_dependency_optimized)?
> >
> > we already do this.. as was discussed on an earlier iteration of this
> > patchset
> >
> > But I'm not aware of any other non-gpu related implicit sync use-case
> > (even on imx devices where display is decoupled from gpu).. I'll
> > revert the patch if someone comes up with one, but otherwise lets let
> > the implicit sync baggage die
>
> The thing is, dma_resv won't die, even if implicit sync is dead. We're
> using internally for activity tracking and memory management. If you don't
> set these, then we can't share generic code with msm, and I think everyone
> inventing their own memory management is a bit a mistake.
>
> Now you only kill the implicit write sync stuff here, but I'm not sure
> that's worth much since you still install all the read fences for
> consistency. And if userspace doesn't want to be synced, they can set the
> flag and do this on their own: I think you should be able to achieve
> exactly the same thing in mesa.
>
> Aside: If you're worried about overhead, you can do O(1) submit if you
> manage your ppgtt like amdgpu does.
So just remember a use-case which is maybe a bit yucky, but it is
actually possible to implement race-free. If you have implicit sync.
There's screen-capture tool in mplayer and obs which capture your
compositor by running getfb2 in a loop. It works, and after some
initial screaming I realized it does actually work race-free. If you
have implicit sync.
I really don't think you can sunset this, as much as you want to. And
sunsetting it inconsistently is probably the worst.
-Daniel
> -Daniel
>
> >
> > BR,
> > -R
> >
> >
> >
> > >
> > > Regards,
> > > Lucas
> > >
> > > > Signed-off-by: Rob Clark
> > > > Reviewed-by: Kristian H. Kristensen
> > > > ---
> > > > drivers/gpu/drm/msm/msm_gem_submit.c | 7 ---
> > > > 1 file changed, 4 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c
> > > > b/drivers/gpu/drm/msm/msm_gem_submit.c
> > > > index d04c349d8112..b6babc7f9bb8 100644
> > > > --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> > > > +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> > > > @@ -283,7 +283,7 @@ static int submit_lock_objects(struct
> > > > msm_gem_submit *submit)
> > > > return ret;
> > > > }
> > > >
> > > > -static int submit_fence_sync(struct msm_gem_submit *submit, bool
> > > > no_implicit)
> > > > +static int submit_fence_sync(struct msm_gem_submit *submit, bool
> > > > implicit_sync)
> > > > {
> > > > int i, ret = 0;
> > > >
> > > > @@ -303,7 +303,7 @@ static int submit_fence_sync(struct msm_gem_submit
> > > > *submit, bool no_implicit)
> > > > return ret;
> > > > }
> > > >
> > > > - if (no_implicit)
> > > > + if (!implicit_sync)
> > > > continue;
> > > >
> > > > ret = msm_gem_sync_object(_obj->base,
> > > > submit->ring->fctx,
> > > > @@ -774,7 +774,8 @@ int msm_ioctl_gem_submit(struct drm_device *dev,
> > > > void *data,
> > > > if (ret)
> > > > goto out;
> > > >
> > > > - ret = submit_fence_sync(submit, !!(args->flags &
> > > > MSM_SUBMIT_NO_IMPLICIT));
> > > > + ret = submit_fence_sync(submit, (gpu->nr_rings > 1) &&
> > > > + !(args->flags & MSM_SUBMIT_NO_IMPLICIT));
> > > > if (ret)
> > > > goto out;
> > > >
> > >
> > ___
> > dri-devel mailing list
> > dri-de...@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH v4 3/3] dt-bindings: drm/msm/gpu: Add cooling device support
Add cooling device support to gpu. A cooling device is bound to a
thermal zone to allow thermal mitigation.
Signed-off-by: Akhil P Oommen
---
Documentation/devicetree/bindings/display/msm/gpu.txt | 7 +++
1 file changed, 7 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/msm/gpu.txt
b/Documentation/devicetree/bindings/display/msm/gpu.txt
index 1af0ff1..090dcb3 100644
--- a/Documentation/devicetree/bindings/display/msm/gpu.txt
+++ b/Documentation/devicetree/bindings/display/msm/gpu.txt
@@ -39,6 +39,10 @@ Required properties:
a4xx Snapdragon SoCs. See
Documentation/devicetree/bindings/sram/qcom,ocmem.yaml.
+Optional properties:
+- #cooling-cells: The value must be 2. For details, please refer
+ Documentation/devicetree/bindings/thermal/thermal-cooling-devices.yaml.
+
Example 3xx/4xx:
/ {
@@ -61,6 +65,7 @@ Example 3xx/4xx:
power-domains = < OXILICX_GDSC>;
operating-points-v2 = <_opp_table>;
iommus = <_iommu 0>;
+ #cooling-cells = <2>;
};
gpu_sram: ocmem@fdd0 {
@@ -98,6 +103,8 @@ Example a6xx (with GMU):
reg = <0x500 0x4>, <0x509e000 0x10>;
reg-names = "kgsl_3d0_reg_memory", "cx_mem";
+ #cooling-cells = <2>;
+
/*
* Look ma, no clocks! The GPU clocks and power are
* controlled entirely by the GMU
--
2.7.4
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH v4 2/3] arm64: dts: qcom: sc7180: Add gpu cooling support
Add cooling-cells property and the cooling maps for the gpu tzones
to support GPU cooling.
Signed-off-by: Akhil P Oommen
Reviewed-by: Matthias Kaehlcke
---
arch/arm64/boot/dts/qcom/sc7180.dtsi | 30 +++---
1 file changed, 23 insertions(+), 7 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi
b/arch/arm64/boot/dts/qcom/sc7180.dtsi
index d46b383..8e2000c 100644
--- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
@@ -2,7 +2,7 @@
/*
* SC7180 SoC device tree source
*
- * Copyright (c) 2019, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2019-20, The Linux Foundation. All rights reserved.
*/
#include
@@ -1886,6 +1886,8 @@
operating-points-v2 = <_opp_table>;
qcom,gmu = <>;
+ #cooling-cells = <2>;
+
interconnects = <_noc MASTER_GFX3D _virt
SLAVE_EBI1>;
interconnect-names = "gfx-mem";
@@ -3825,16 +3827,16 @@
};
gpuss0-thermal {
- polling-delay-passive = <0>;
+ polling-delay-passive = <100>;
polling-delay = <0>;
thermal-sensors = < 13>;
trips {
gpuss0_alert0: trip-point0 {
- temperature = <9>;
+ temperature = <95000>;
hysteresis = <2000>;
- type = "hot";
+ type = "passive";
};
gpuss0_crit: gpuss0_crit {
@@ -3843,19 +3845,26 @@
type = "critical";
};
};
+
+ cooling-maps {
+ map0 {
+ trip = <_alert0>;
+ cooling-device = < THERMAL_NO_LIMIT
THERMAL_NO_LIMIT>;
+ };
+ };
};
gpuss1-thermal {
- polling-delay-passive = <0>;
+ polling-delay-passive = <100>;
polling-delay = <0>;
thermal-sensors = < 14>;
trips {
gpuss1_alert0: trip-point0 {
- temperature = <9>;
+ temperature = <95000>;
hysteresis = <2000>;
- type = "hot";
+ type = "passive";
};
gpuss1_crit: gpuss1_crit {
@@ -3864,6 +3873,13 @@
type = "critical";
};
};
+
+ cooling-maps {
+ map0 {
+ trip = <_alert0>;
+ cooling-device = < THERMAL_NO_LIMIT
THERMAL_NO_LIMIT>;
+ };
+ };
};
aoss1-thermal {
--
2.7.4
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH v4 1/3] drm/msm: Add support for GPU cooling
Register GPU as a devfreq cooling device so that it can be passively
cooled by the thermal framework.
Signed-off-by: Akhil P Oommen
Reviewed-by: Matthias Kaehlcke
---
Changes in v4:
1. Fix gpu cooling map.
2. Add mka's Reviewed-by tag.
Changes in v3:
1. Minor fix in binding documentation (RobH)
Changes in v2:
1. Update the dt bindings documentation
drivers/gpu/drm/msm/msm_gpu.c | 12
drivers/gpu/drm/msm/msm_gpu.h | 2 ++
2 files changed, 14 insertions(+)
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index 55d1648..9f9db46 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -14,6 +14,7 @@
#include
#include
#include
+#include
#include
#include
@@ -107,9 +108,18 @@ static void msm_devfreq_init(struct msm_gpu *gpu)
if (IS_ERR(gpu->devfreq.devfreq)) {
DRM_DEV_ERROR(>pdev->dev, "Couldn't initialize GPU
devfreq\n");
gpu->devfreq.devfreq = NULL;
+ return;
}
devfreq_suspend_device(gpu->devfreq.devfreq);
+
+ gpu->cooling = of_devfreq_cooling_register(gpu->pdev->dev.of_node,
+ gpu->devfreq.devfreq);
+ if (IS_ERR(gpu->cooling)) {
+ DRM_DEV_ERROR(>pdev->dev,
+ "Couldn't register GPU cooling device\n");
+ gpu->cooling = NULL;
+ }
}
static int enable_pwrrail(struct msm_gpu *gpu)
@@ -1005,4 +1015,6 @@ void msm_gpu_cleanup(struct msm_gpu *gpu)
gpu->aspace->mmu->funcs->detach(gpu->aspace->mmu);
msm_gem_address_space_put(gpu->aspace);
}
+
+ devfreq_cooling_unregister(gpu->cooling);
}
diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
index 6c9e1fd..9a8f20d 100644
--- a/drivers/gpu/drm/msm/msm_gpu.h
+++ b/drivers/gpu/drm/msm/msm_gpu.h
@@ -147,6 +147,8 @@ struct msm_gpu {
struct msm_gpu_state *crashstate;
/* True if the hardware supports expanded apriv (a650 and newer) */
bool hw_apriv;
+
+ struct thermal_cooling_device *cooling;
};
static inline struct msm_gpu *dev_to_gpu(struct device *dev)
--
2.7.4
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [v3, 2/3] arm64: dts: qcom: sc7180: Add gpu cooling support
On 10/29/2020 6:09 AM, m...@chromium.org wrote:
Hi Akhil,
On Wed, Oct 28, 2020 at 07:09:53PM +0530, Akhil P Oommen wrote:
Add cooling-cells property and the cooling maps for the gpu tzones
to support GPU cooling.
Signed-off-by: Akhil P Oommen
---
arch/arm64/boot/dts/qcom/sc7180.dtsi | 30 +++---
1 file changed, 23 insertions(+), 7 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi
b/arch/arm64/boot/dts/qcom/sc7180.dtsi
index d46b383..a7ea029 100644
--- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
@@ -2,7 +2,7 @@
/*
* SC7180 SoC device tree source
*
- * Copyright (c) 2019, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2019-20, The Linux Foundation. All rights reserved.
*/
#include
@@ -1886,6 +1886,8 @@
operating-points-v2 = <_opp_table>;
qcom,gmu = <>;
+ #cooling-cells = <2>;
+
interconnects = <_noc MASTER_GFX3D _virt
SLAVE_EBI1>;
interconnect-names = "gfx-mem";
@@ -3825,16 +3827,16 @@
};
gpuss0-thermal {
- polling-delay-passive = <0>;
+ polling-delay-passive = <100>;
polling-delay = <0>;
thermal-sensors = < 13>;
trips {
gpuss0_alert0: trip-point0 {
- temperature = <9>;
+ temperature = <95000>;
hysteresis = <2000>;
- type = "hot";
+ type = "passive";
};
gpuss0_crit: gpuss0_crit {
@@ -3843,19 +3845,26 @@
type = "critical";
};
};
+
+ cooling-maps {
+ map0 {
+ trip = <_alert0>;
+ cooling-device = < THERMAL_NO_LIMIT
THERMAL_NO_LIMIT>;
+ };
+ };
};
gpuss1-thermal {
- polling-delay-passive = <0>;
+ polling-delay-passive = <100>;
polling-delay = <0>;
thermal-sensors = < 14>;
trips {
gpuss1_alert0: trip-point0 {
- temperature = <9>;
+ temperature = <95000>;
hysteresis = <2000>;
- type = "hot";
+ type = "passive";
};
gpuss1_crit: gpuss1_crit {
@@ -3864,6 +3873,13 @@
type = "critical";
};
};
+
+ cooling-maps {
+ map0 {
+ trip = <_alert0>;
Copy & paste error, this should be 'gpuss1_alert0'.
aah! you are correct.
--Akhil
+ cooling-device = < THERMAL_NO_LIMIT
THERMAL_NO_LIMIT>;
+ };
+ };
};
aoss1-thermal {
Other than the C error:
Reviewed-by: Matthias Kaehlcke
___
dri-devel mailing list
dri-de...@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
