RE: [PATCH v2] drm/dp: Fix aux->transfer NULL pointer dereference on drm_dp_dpcd_access
[AMD Official Use Only] Hi Jani: > -Original Message- > From: Jani Nikula > Sent: Wednesday, November 3, 2021 7:31 PM > To: Yuan, Perry ; Maarten Lankhorst > ; Maxime Ripard > ; Thomas Zimmermann ; > David Airlie ; Daniel Vetter > Cc: dri-devel@lists.freedesktop.org; linux-ker...@vger.kernel.org; Huang, > Shimmer ; Huang, Ray > Subject: RE: [PATCH v2] drm/dp: Fix aux->transfer NULL pointer dereference > on drm_dp_dpcd_access > > [CAUTION: External Email] > > On Wed, 03 Nov 2021, "Yuan, Perry" wrote: > > [AMD Official Use Only] > > > > Hi Jani: > > > >> -Original Message- > >> From: Jani Nikula > >> Sent: Tuesday, November 2, 2021 4:40 PM > >> To: Yuan, Perry ; Maarten Lankhorst > >> ; Maxime Ripard > >> ; Thomas Zimmermann ; > David > >> Airlie ; Daniel Vetter > >> Cc: dri-devel@lists.freedesktop.org; linux-ker...@vger.kernel.org; > >> Huang, Shimmer ; Huang, Ray > > >> Subject: RE: [PATCH v2] drm/dp: Fix aux->transfer NULL pointer > >> dereference on drm_dp_dpcd_access > >> > >> [CAUTION: External Email] > >> > >> On Tue, 02 Nov 2021, "Yuan, Perry" wrote: > >> > [AMD Official Use Only] > >> > > >> > Hi Jani: > >> > Thanks for your comments. > >> > > >> >> -Original Message- > >> >> From: Jani Nikula > >> >> Sent: Monday, November 1, 2021 9:07 PM > >> >> To: Yuan, Perry ; Maarten Lankhorst > >> >> ; Maxime Ripard > >> >> ; Thomas Zimmermann > ; > >> David > >> >> Airlie ; Daniel Vetter > >> >> Cc: Yuan, Perry ; > >> >> dri-devel@lists.freedesktop.org; linux- ker...@vger.kernel.org; > >> >> Huang, Shimmer ; Huang, Ray > >> > >> >> Subject: Re: [PATCH v2] drm/dp: Fix aux->transfer NULL pointer > >> >> dereference on drm_dp_dpcd_access > >> >> > >> >> [CAUTION: External Email] > >> >> > >> >> On Mon, 01 Nov 2021, Perry Yuan wrote: > >> >> > Fix below crash by adding a check in the drm_dp_dpcd_access > >> >> > which ensures that aux->transfer was actually initialized earlier. > >> >> > >> >> Gut feeling says this is papering over a real usage issue > >> >> somewhere else. Why is the aux being used for transfers before > >> >> ->transfer has been set? Why should the dp helper be defensive > >> >> against all kinds of > >> misprogramming? > >> >> > >> >> > >> >> BR, > >> >> Jani. > >> >> > >> > > >> > The issue was found by Intel IGT test suite, graphic by pass test case. > >> > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fg > >> > itl > >> > ab.freedesktop.org%2Fdrm%2Figt-gpu- > >> tools&data=04%7C01%7CPerry.Yuan > >> > %40amd.com%7C83d011acfe65437c0fa808d99ddc65b0%7C3dd8961fe4 > 884e6 > >> 08e11a8 > >> > > >> > 2d994e183d%7C0%7C0%7C637714392203200313%7CUnknown%7CTWFpbG > Zsb > >> 3d8eyJWIj > >> > > >> > oiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1 > 00 > >> 0&am > >> > > >> > p;sdata=snPpRYLGeJtTpNGle1YHZAvevcABbgLkgOsffiNzQPw%3D&reser > ved > >> =0 > >> > normally use case will not see the issue. > >> > To avoid this issue happy again when we run the test case , it will > >> > be nice to > >> add a check before the transfer is called. > >> > And we can see that it really needs to have a check here to make > >> > ITG &kernel > >> happy. > >> > >> You're missing my point. What is the root cause? Why do you have the > >> aux device or connector registered before ->transfer function is > >> initialized. I don't think you should do that. > >> > >> BR, > >> Jani. > >> > > > > One potential IGT fix patch to resolve the test case failure is: > > > > tests/amdgpu/amd_bypass.c > > data->pipe_crc = igt_pipe_crc_new(data->drm_fd, data->pipe_id, > >- AMDGPU_PIPE_CRC_SOURCE_DPRX); > >+ INTEL_PIPE_CRC_SOURCE_AUTO); > > The kernel panic error gone after change "dprx" to "auto" in the IGT test. > > > > In my view ,the IGT amdgpu bypass test will do some common setup work > including crc piple, source. > > When the IGT sets up a new CRC pipe capture source for amdgpu bypass > test, the SOURCE was set as "dprx" instead of "auto" > > It makes "amdgpu_dm_crtc_set_crc_source()" failed to set correct AUX > and it's transfer function invalid . > > The system I tested use HDMI port connected to monitor . > > > > amdgpu_dm_crtc_set_crc_source->(aux = (aconn->port) ? &aconn- > >port->aux : &aconn->dm_dp_aux.aux;) > >drm_dp_start_crc -> > > drm_dp_dpcd_readb-> aux->transfer is NULL, issue here. > > The fix will use the "auto" keyword, which will let the driver select a > default source of frame CRCs for this CRTC. > > > > Correct me if have some wrong points. > > Apparently I'm completely failing to communicate my POV to you. > > If you have a kernel oops, the fix needs to be in the kernel, not IGT. > > The question is, why is it possible for IGT (or any userspace) to trigger AUX > communication when the ->transfer function is not set? In my opinion, the > kernel driver should not have exposed the interface at all if the ->transfer >
[PULL] drm-misc-next-fixes
Hi Dave, Daniel, Here's this week drm-misc-next-fixes PR Thanks! Maxime drm-misc-next-fixes-2021-11-05: A refcounting fix for outstanding fence callbacks. The following changes since commit b3ec8cdf457e5e63d396fe1346cc788cf7c1b578: fbdev: Garbage collect fbdev scrolling acceleration, part 1 (from TODO list) (2021-10-13 15:29:23 +0200) are available in the Git repository at: git://anongit.freedesktop.org/drm/drm-misc tags/drm-misc-next-fixes-2021-11-05 for you to fetch changes up to ff2d23843f7fb4f13055be5a4a9a20ddd04e6e9c: dma-buf/poll: Get a file reference for outstanding fence callbacks (2021-11-04 09:18:57 +0100) A refcounting fix for outstanding fence callbacks. Michel Dänzer (1): dma-buf/poll: Get a file reference for outstanding fence callbacks drivers/dma-buf/dma-buf.c | 18 -- 1 file changed, 12 insertions(+), 6 deletions(-) signature.asc Description: PGP signature
[PATCH] omapfb: panel-dsi-cm: Replace snprintf in show functions with sysfs_emit
From: Jing Yao coccicheck complains about the use of snprintf() in sysfs show functions: WARNING use scnprintf or sprintf Use sysfs_emit instead of scnprintf, snprintf or sprintf makes more sense. Reported-by: Zeal Robot Signed-off-by: Jing Yao --- drivers/video/fbdev/omap2/omapfb/displays/panel-dsi-cm.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/video/fbdev/omap2/omapfb/displays/panel-dsi-cm.c b/drivers/video/fbdev/omap2/omapfb/displays/panel-dsi-cm.c index 4b0793abdd84..a2c7c5cb1523 100644 --- a/drivers/video/fbdev/omap2/omapfb/displays/panel-dsi-cm.c +++ b/drivers/video/fbdev/omap2/omapfb/displays/panel-dsi-cm.c @@ -409,7 +409,7 @@ static ssize_t dsicm_num_errors_show(struct device *dev, if (r) return r; - return snprintf(buf, PAGE_SIZE, "%d\n", errors); + return sysfs_emit(buf, "%d\n", errors); } static ssize_t dsicm_hw_revision_show(struct device *dev, @@ -439,7 +439,7 @@ static ssize_t dsicm_hw_revision_show(struct device *dev, if (r) return r; - return snprintf(buf, PAGE_SIZE, "%02x.%02x.%02x\n", id1, id2, id3); + return sysfs_emit(buf, "%02x.%02x.%02x\n", id1, id2, id3); } static ssize_t dsicm_store_ulps(struct device *dev, @@ -487,7 +487,7 @@ static ssize_t dsicm_show_ulps(struct device *dev, t = ddata->ulps_enabled; mutex_unlock(&ddata->lock); - return snprintf(buf, PAGE_SIZE, "%u\n", t); + return sysfs_emit(buf, "%u\n", t); } static ssize_t dsicm_store_ulps_timeout(struct device *dev, @@ -532,7 +532,7 @@ static ssize_t dsicm_show_ulps_timeout(struct device *dev, t = ddata->ulps_timeout; mutex_unlock(&ddata->lock); - return snprintf(buf, PAGE_SIZE, "%u\n", t); + return sysfs_emit(buf, "%u\n", t); } static DEVICE_ATTR(num_dsi_errors, S_IRUGO, dsicm_num_errors_show, NULL); -- 2.25.1
[PATCH] omapfb: panel-tpo-td043mtea1: Replace snprintf in show functions with sysfs_emit
From: Jing Yao
coccicheck complains about the use of snprintf() in sysfs show
functions:
WARNING use scnprintf or sprintf
Use sysfs_emit instead of scnprintf, snprintf or sprintf makes more
sense.
Reported-by: Zeal Robot
Signed-off-by: Jing Yao
---
.../video/fbdev/omap2/omapfb/displays/panel-tpo-td043mtea1.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/video/fbdev/omap2/omapfb/displays/panel-tpo-td043mtea1.c
b/drivers/video/fbdev/omap2/omapfb/displays/panel-tpo-td043mtea1.c
index afac1d9445aa..57b7d1f49096 100644
--- a/drivers/video/fbdev/omap2/omapfb/displays/panel-tpo-td043mtea1.c
+++ b/drivers/video/fbdev/omap2/omapfb/displays/panel-tpo-td043mtea1.c
@@ -169,7 +169,7 @@ static ssize_t tpo_td043_vmirror_show(struct device *dev,
{
struct panel_drv_data *ddata = dev_get_drvdata(dev);
- return snprintf(buf, PAGE_SIZE, "%d\n", ddata->vmirror);
+ return sysfs_emit(buf, "%d\n", ddata->vmirror);
}
static ssize_t tpo_td043_vmirror_store(struct device *dev,
@@ -199,7 +199,7 @@ static ssize_t tpo_td043_mode_show(struct device *dev,
{
struct panel_drv_data *ddata = dev_get_drvdata(dev);
- return snprintf(buf, PAGE_SIZE, "%d\n", ddata->mode);
+ return sysfs_emit(buf, "%d\n", ddata->mode);
}
static ssize_t tpo_td043_mode_store(struct device *dev,
--
2.25.1
Re: [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled
Hi
Am 04.11.21 um 21:09 schrieb Javier Martinez Canillas:
Hello Jani,
On 11/4/21 20:57, Jani Nikula wrote:
On Thu, 04 Nov 2021, Javier Martinez Canillas wrote:
+/**
+ * drm_drv_enabled - Checks if a DRM driver can be enabled
+ * @driver: DRM driver to check
+ *
+ * Checks whether a DRM driver can be enabled or not. This may be the case
+ * if the "nomodeset" kernel command line parameter is used.
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
+int drm_drv_enabled(const struct drm_driver *driver)
Jani mentioned that i915 absolutely wants this to run from the
module_init function. Best is to drop the parameter.
+{
+ if (vgacon_text_force()) {
+ DRM_INFO("%s driver is disabled\n", driver->name);
+ return -ENODEV;
+ }
If we run this from within a module_init function, we'd get plenty of
these warnings if drivers are compiled into the kernel. Maybe simply
remove the message. There's already a warning printed by the nomodeset
handler.
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_drv_enabled);
The name implies a bool return, but it's not.
if (drm_drv_enabled(...)) {
/* surprise, it's disabled! */
}
It used to return a bool in v2 but Thomas suggested an int instead to
have consistency on the errno code that was returned by the callers.
I should probably name that function differently to avoid confusion.
Yes, please.
Best regards
Thomas
But I think you are correct and this change is caused too much churn
for not that much benefit, specially since is unclear that there might
be another condition to prevent a DRM driver to load besides nomodeset.
I'll just drop this patch and post only #2 but making drivers to test
using the drm_check_modeset() function (which doesn't have a name that
implies a bool return).
BR,
Jani.
Best regards,
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
OpenPGP_signature
Description: OpenPGP digital signature
Re: [GIT PULL] drm/tegra: Changes for v5.16-rc1
On Fri, Oct 08, 2021 at 10:23:34PM +0200, Thierry Reding wrote: > Hi Dave, Daniel, > > The following changes since commit c3dbfb9c49eef7d07904e5fd5e158dd6688bbab3: > > gpu: host1x: Plug potential memory leak (2021-09-16 18:06:52 +0200) > > are available in the Git repository at: > > ssh://git.freedesktop.org/git/tegra/linux.git tags/drm/tegra/for-5.16-rc1 > > for you to fetch changes up to 5dccbc9de8f0071eb731b4de81d0638ea6c06a53: > > drm/tegra: dc: rgb: Allow changing PLLD rate on Tegra30+ (2021-10-08 > 21:17:38 +0200) > > This is based on the drm/tegra/for-5.15-rc3 tag that you pulled a couple > of weeks ago. As mentioned last time already, the userspace for the new > NVDEC driver can be found here: > > https://github.com/cyndis/vaapi-tegra-driver > > I'm sending this a week earlier than usual because I'm out of office > next week. Hi guys, I haven't seen this show up in drm-next yet. Did this fall through the cracks or was there something that you wanted to see addressed? Thanks, Thierry signature.asc Description: PGP signature
Re: [PATCH v2 2/2] drm: Move nomodeset kernel parameter to the DRM subsystem
Hi
Am 04.11.21 um 17:07 schrieb Javier Martinez Canillas:
The "nomodeset" kernel cmdline parameter is handled by the vgacon driver
but the exported vgacon_text_force() symbol is only used by DRM drivers.
It makes much more sense for the parameter logic to be in the subsystem
of the drivers that are making use of it.
Let's move the vgacon_text_force() function and related logic to the DRM
subsystem. While doing that, rename the function to drm_check_modeset()
which better reflects what the function is really used to test for.
Suggested-by: Daniel Vetter
Signed-off-by: Javier Martinez Canillas
---
Changes in v2:
- Conditionally build drm_nomodeset.o if CONFIG_VGA_CONSOLE is set.
- Squash patches to move nomodeset logic to DRM and do the renaming.
- Name the function drm_check_modeset() and make it return -ENODEV.
drivers/gpu/drm/Makefile| 2 ++
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 1 -
drivers/gpu/drm/ast/ast_drv.c | 1 -
drivers/gpu/drm/drm_drv.c | 9 +
drivers/gpu/drm/drm_nomodeset.c | 26 +
drivers/gpu/drm/i915/i915_module.c | 2 --
drivers/gpu/drm/mgag200/mgag200_drv.c | 1 -
drivers/gpu/drm/nouveau/nouveau_drm.c | 1 -
drivers/gpu/drm/qxl/qxl_drv.c | 1 -
drivers/gpu/drm/radeon/radeon_drv.c | 1 -
drivers/gpu/drm/tiny/bochs.c| 1 -
drivers/gpu/drm/tiny/cirrus.c | 1 -
drivers/gpu/drm/vboxvideo/vbox_drv.c| 1 -
drivers/gpu/drm/virtio/virtgpu_drv.c| 1 -
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 1 -
drivers/video/console/vgacon.c | 21
include/drm/drm_mode_config.h | 6 ++
include/linux/console.h | 6 --
18 files changed, 39 insertions(+), 44 deletions(-)
create mode 100644 drivers/gpu/drm/drm_nomodeset.c
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 1c41156deb5f..c74810c285af 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -33,6 +33,8 @@ drm-$(CONFIG_DRM_PRIVACY_SCREEN) += drm_privacy_screen.o
drm_privacy_screen_x86.
obj-$(CONFIG_DRM_DP_AUX_BUS) += drm_dp_aux_bus.o
+obj-$(CONFIG_VGA_CONSOLE) += drm_nomodeset.o
+
This now depends on the VGA textmode console. Even if you have no VGA
console, you'd want drm_nomodeset.o. Simpledrm might be built-in and can
provide graphics. Non-PC systems don't even have a VGA device.
I think we really want a separate boolean config option that gets
selected by CONFIG_DRM.
drm_cma_helper-y := drm_gem_cma_helper.o
obj-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_cma_helper.o
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 7fde40d06181..b4b6993861e6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -31,7 +31,6 @@
#include "amdgpu_drv.h"
#include
-#include
#include
#include
#include
diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index 802063279b86..6222082c3082 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -26,7 +26,6 @@
* Authors: Dave Airlie
*/
-#include
#include
#include
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 3fb567d62881..80b85b8ea776 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -986,12 +986,13 @@ EXPORT_SYMBOL(drm_dev_set_unique);
*/
int drm_drv_enabled(const struct drm_driver *driver)
{
- if (vgacon_text_force()) {
+ int ret;
+
+ ret = drm_check_modeset();
+ if (ret)
DRM_INFO("%s driver is disabled\n", driver->name);
- return -ENODEV;
- }
- return 0;
+ return ret;
}
EXPORT_SYMBOL(drm_drv_enabled);
diff --git a/drivers/gpu/drm/drm_nomodeset.c b/drivers/gpu/drm/drm_nomodeset.c
new file mode 100644
index ..6683e396d2c5
--- /dev/null
+++ b/drivers/gpu/drm/drm_nomodeset.c
@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include
+#include
+
+static bool drm_nomodeset;
+
+int drm_check_modeset(void)
+{
+ return drm_nomodeset ? -ENODEV : 0;
+}
+EXPORT_SYMBOL(drm_check_modeset);
+
+static int __init disable_modeset(char *str)
+{
+ drm_nomodeset = true;
+
+ pr_warn("You have booted with nomodeset. This means your GPU drivers are
DISABLED\n");
+ pr_warn("Any video related functionality will be severely degraded, and you
may not even be able to suspend the system properly\n");
+ pr_warn("Unless you actually understand what nomodeset does, you should
reboot without enabling it\n");
I'd update this text to be less sensational.
+
+ return 1;
+}
+
+/* Disable kernel modesetting */
+__setup("nomodeset", disable_modeset);
diff --git a/drivers/gpu/drm/i915/i915_module.c
b/drivers/gpu/drm/i915/i915_module.c
index 45cb3e540eff..c890c1ca20c4 100644
--- a/drivers
[PATCH] drm/prime/gem: drm_gem_object_release_handle by handle
The drm_gem_handle_delete is called by DRM_IOCTL_GEM_CLOSE from
userspace.
drm_gem_handle_delete(handle)
drm_gem_object_release_handle(handle)
drm_gem_remove_prime_handles()
drm_prime_remove_buf_handle_locked()
if (member->dma_buf == dma_buf)
free member
return
The api description of drm_gem_handle_delete says to delete the given
file-private handle.
But the drm_gem_remove_prime_handles seems to remove *handles* from
rbtree of drm file-private structure.
And then the drm_gem_remove_prime_handles only remove the first handle
lookuped from rbtree, as following codes:
rb = prime_fpriv->dmabufs.rb_node;
while (rb) {
struct drm_prime_member *member;
member = rb_entry(rb, struct drm_prime_member, dmabuf_rb);
if (member->dma_buf == dma_buf) {
rb_erase(&member->handle_rb, &prime_fpriv->handles);
rb_erase(&member->dmabuf_rb, &prime_fpriv->dmabufs);
dma_buf_put(dma_buf);
kfree(member);
return;
}
This patch fixes to remove the rbtree member by given handle.
Test
handle_1 = drm_alloc(1MB)
fd_1 = drm_handle_to_fd(handle_1)
name_1 = drm get flink name from handle(handle_1)//
DRM_IOCTL_GEM_FLINK
handle_2 = drm get handle,size_2 from flink name(name_1) // DRM_IOCTL_GEM_OPEN
fd_2 = drm_handle_to_fd(handle_2)
handle_3 = drm get handle,size_3 from flink name(name_1) // DRM_IOCTL_GEM_OPEN
fd_3 = drm_handle_to_fd(handle_3)
drm close handle(handle_3) // DRM_IOCTL_GEM_CLOSE
handle_4 = drm_alloc(4MB)
fd_4 = drm_handle_to_fd(handle_4)
We find that the fd_4 dmabuf size is 1MB. Tested by following:
handle_5 = drm_fd_to_handle(fd_4)
name_5 = drm get flink name from handle(handle_5)//
DRM_IOCTL_GEM_FLINK
handle_3 = drm get handle,size_5 from flink name(name_5) // DRM_IOCTL_GEM_OPEN
Without this patch, the size_5 = 1MB
With this patch, the size_5 = 4MB
Signed-off-by: Jianqun Xu
---
drivers/gpu/drm/drm_gem.c | 7 ---
drivers/gpu/drm/drm_internal.h | 2 +-
drivers/gpu/drm/drm_prime.c| 4 ++--
3 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 09c820045859..bfa5637f54d2 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -168,7 +168,8 @@ void drm_gem_private_object_init(struct drm_device *dev,
EXPORT_SYMBOL(drm_gem_private_object_init);
static void
-drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct drm_file *filp)
+drm_gem_remove_prime_handle(struct drm_gem_object *obj, struct drm_file *filp,
+ int handle)
{
/*
* Note: obj->dma_buf can't disappear as long as we still hold a
@@ -177,7 +178,7 @@ drm_gem_remove_prime_handles(struct drm_gem_object *obj,
struct drm_file *filp)
mutex_lock(&filp->prime.lock);
if (obj->dma_buf) {
drm_prime_remove_buf_handle_locked(&filp->prime,
- obj->dma_buf);
+ obj->dma_buf, handle);
}
mutex_unlock(&filp->prime.lock);
}
@@ -252,7 +253,7 @@ drm_gem_object_release_handle(int id, void *ptr, void *data)
if (obj->funcs->close)
obj->funcs->close(obj, file_priv);
- drm_gem_remove_prime_handles(obj, file_priv);
+ drm_gem_remove_prime_handle(obj, file_priv, id);
drm_vma_node_revoke(&obj->vma_node, file_priv);
drm_gem_object_handle_put_unlocked(obj);
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index 17f3548c8ed2..17c4f6cac21c 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -75,7 +75,7 @@ int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void
*data,
void drm_prime_init_file_private(struct drm_prime_file_private *prime_fpriv);
void drm_prime_destroy_file_private(struct drm_prime_file_private
*prime_fpriv);
void drm_prime_remove_buf_handle_locked(struct drm_prime_file_private
*prime_fpriv,
- struct dma_buf *dma_buf);
+ struct dma_buf *dma_bufi, int handle);
/* drm_drv.c */
struct drm_minor *drm_minor_acquire(unsigned int minor_id);
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index deb23dbec8b5..080476296715 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -188,7 +188,7 @@ static int drm_prime_lookup_buf_handle(struct
drm_prime_file_private *prime_fpri
}
void drm_prime_remove_buf_handle_locked(struct drm_prime_file_private
*prime_fpriv,
- struct dma_buf *dma_buf)
+ struct dma_buf *dma_buf, int handle)
{
struct rb_node *rb;
@@ -197,7 +197,7 @@ void
Re: [PATCH v2 2/2] drm: Move nomodeset kernel parameter to the DRM subsystem
On Fri, 05 Nov 2021, Thomas Zimmermann wrote:
> Hi
>
> Am 04.11.21 um 17:07 schrieb Javier Martinez Canillas:
>> The "nomodeset" kernel cmdline parameter is handled by the vgacon driver
>> but the exported vgacon_text_force() symbol is only used by DRM drivers.
>>
>> It makes much more sense for the parameter logic to be in the subsystem
>> of the drivers that are making use of it.
>>
>> Let's move the vgacon_text_force() function and related logic to the DRM
>> subsystem. While doing that, rename the function to drm_check_modeset()
>> which better reflects what the function is really used to test for.
>>
>> Suggested-by: Daniel Vetter
>> Signed-off-by: Javier Martinez Canillas
>> ---
>>
>> Changes in v2:
>> - Conditionally build drm_nomodeset.o if CONFIG_VGA_CONSOLE is set.
>> - Squash patches to move nomodeset logic to DRM and do the renaming.
>> - Name the function drm_check_modeset() and make it return -ENODEV.
>>
>> drivers/gpu/drm/Makefile| 2 ++
>> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 1 -
>> drivers/gpu/drm/ast/ast_drv.c | 1 -
>> drivers/gpu/drm/drm_drv.c | 9 +
>> drivers/gpu/drm/drm_nomodeset.c | 26 +
>> drivers/gpu/drm/i915/i915_module.c | 2 --
>> drivers/gpu/drm/mgag200/mgag200_drv.c | 1 -
>> drivers/gpu/drm/nouveau/nouveau_drm.c | 1 -
>> drivers/gpu/drm/qxl/qxl_drv.c | 1 -
>> drivers/gpu/drm/radeon/radeon_drv.c | 1 -
>> drivers/gpu/drm/tiny/bochs.c| 1 -
>> drivers/gpu/drm/tiny/cirrus.c | 1 -
>> drivers/gpu/drm/vboxvideo/vbox_drv.c| 1 -
>> drivers/gpu/drm/virtio/virtgpu_drv.c| 1 -
>> drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 1 -
>> drivers/video/console/vgacon.c | 21
>> include/drm/drm_mode_config.h | 6 ++
>> include/linux/console.h | 6 --
>> 18 files changed, 39 insertions(+), 44 deletions(-)
>> create mode 100644 drivers/gpu/drm/drm_nomodeset.c
>>
>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>> index 1c41156deb5f..c74810c285af 100644
>> --- a/drivers/gpu/drm/Makefile
>> +++ b/drivers/gpu/drm/Makefile
>> @@ -33,6 +33,8 @@ drm-$(CONFIG_DRM_PRIVACY_SCREEN) += drm_privacy_screen.o
>> drm_privacy_screen_x86.
>>
>> obj-$(CONFIG_DRM_DP_AUX_BUS) += drm_dp_aux_bus.o
>>
>> +obj-$(CONFIG_VGA_CONSOLE) += drm_nomodeset.o
>> +
>
> This now depends on the VGA textmode console. Even if you have no VGA
> console, you'd want drm_nomodeset.o. Simpledrm might be built-in and can
> provide graphics. Non-PC systems don't even have a VGA device.
This was discussed in an earlier version, which had this builtin but the
header still had a stub for CONFIG_VGA_CONSOLE=n.
> I think we really want a separate boolean config option that gets
> selected by CONFIG_DRM.
Perhaps that should be a separate change on top.
BR,
Jani.
>
>
>> drm_cma_helper-y := drm_gem_cma_helper.o
>> obj-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_cma_helper.o
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> index 7fde40d06181..b4b6993861e6 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> @@ -31,7 +31,6 @@
>> #include "amdgpu_drv.h"
>>
>> #include
>> -#include
>> #include
>> #include
>> #include
>> diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
>> index 802063279b86..6222082c3082 100644
>> --- a/drivers/gpu/drm/ast/ast_drv.c
>> +++ b/drivers/gpu/drm/ast/ast_drv.c
>> @@ -26,7 +26,6 @@
>>* Authors: Dave Airlie
>>*/
>>
>> -#include
>> #include
>> #include
>>
>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> index 3fb567d62881..80b85b8ea776 100644
>> --- a/drivers/gpu/drm/drm_drv.c
>> +++ b/drivers/gpu/drm/drm_drv.c
>> @@ -986,12 +986,13 @@ EXPORT_SYMBOL(drm_dev_set_unique);
>>*/
>> int drm_drv_enabled(const struct drm_driver *driver)
>> {
>> -if (vgacon_text_force()) {
>> +int ret;
>> +
>> +ret = drm_check_modeset();
>> +if (ret)
>> DRM_INFO("%s driver is disabled\n", driver->name);
>> -return -ENODEV;
>> -}
>>
>> -return 0;
>> +return ret;
>> }
>> EXPORT_SYMBOL(drm_drv_enabled);
>>
>> diff --git a/drivers/gpu/drm/drm_nomodeset.c
>> b/drivers/gpu/drm/drm_nomodeset.c
>> new file mode 100644
>> index ..6683e396d2c5
>> --- /dev/null
>> +++ b/drivers/gpu/drm/drm_nomodeset.c
>> @@ -0,0 +1,26 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +#include
>> +#include
>> +
>> +static bool drm_nomodeset;
>> +
>> +int drm_check_modeset(void)
>> +{
>> +return drm_nomodeset ? -ENODEV : 0;
>> +}
>> +EXPORT_SYMBOL(drm_check_modeset);
>> +
>> +static int __init disable_modeset(char *str)
>> +{
>> +drm_nomodeset = true;
>> +
>> +pr_warn("You have booted with nomodeset. T
[PATCH 1/3] drm/shmem-helper: Unexport drm_gem_shmem_create_with_handle()
Turn drm_gem_shmem_create_with_handle() into an internal helper function. It's not used outside of the compilation unit. Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/drm_gem_shmem_helper.c | 3 +-- include/drm/drm_gem_shmem_helper.h | 5 - 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c index a5b743a83ce9..cd93e91b3487 100644 --- a/drivers/gpu/drm/drm_gem_shmem_helper.c +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c @@ -410,7 +410,7 @@ void drm_gem_shmem_vunmap(struct drm_gem_object *obj, struct dma_buf_map *map) } EXPORT_SYMBOL(drm_gem_shmem_vunmap); -struct drm_gem_shmem_object * +static struct drm_gem_shmem_object * drm_gem_shmem_create_with_handle(struct drm_file *file_priv, struct drm_device *dev, size_t size, uint32_t *handle) @@ -434,7 +434,6 @@ drm_gem_shmem_create_with_handle(struct drm_file *file_priv, return shmem; } -EXPORT_SYMBOL(drm_gem_shmem_create_with_handle); /* Update madvise status, returns true if not purged, else * false or -errno. diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h index 434328d8a0d9..6b47eb7d9f76 100644 --- a/include/drm/drm_gem_shmem_helper.h +++ b/include/drm/drm_gem_shmem_helper.h @@ -128,11 +128,6 @@ static inline bool drm_gem_shmem_is_purgeable(struct drm_gem_shmem_object *shmem void drm_gem_shmem_purge_locked(struct drm_gem_object *obj); bool drm_gem_shmem_purge(struct drm_gem_object *obj); -struct drm_gem_shmem_object * -drm_gem_shmem_create_with_handle(struct drm_file *file_priv, -struct drm_device *dev, size_t size, -uint32_t *handle); - int drm_gem_shmem_dumb_create(struct drm_file *file, struct drm_device *dev, struct drm_mode_create_dumb *args); -- 2.33.1
[PATCH 0/3] drm/shmem-helper: Cleanup public interface
The interface of GEM SHMEM helpers inconsistently uses either struct drm_gem_object or drm_gem_shmem_object for the GEM object. Convert GEM SHMEM functions to accept struct drm_gem_shmem_object, provide small wrappers for GEM object callbacks and update all users. Converting all GEM object functions to use drm_gem_shmem_object enables type checking by the C compiler. Previous callers could have passed any implementation of drm_gem_object to the GEM SHMEM helpers. It also removes upcasting in the GEM functions and simplifies the caller side. No functional changes. For GEM object callbacks, the SHMEM helper library now provides a number of small wrappers that do the necessary upcasting. Again no functional changes. Thomas Zimmermann (3): drm/shmem-helper: Unexport drm_gem_shmem_create_with_handle() drm/shmem-helper: Export dedicated wrappers for GEM object functions drm/shmem-helper: Pass GEM shmem object in public interfaces drivers/gpu/drm/drm_gem_shmem_helper.c| 126 + drivers/gpu/drm/lima/lima_gem.c | 18 +- drivers/gpu/drm/lima/lima_sched.c | 4 +- drivers/gpu/drm/panfrost/panfrost_drv.c | 2 +- drivers/gpu/drm/panfrost/panfrost_gem.c | 20 ++- .../gpu/drm/panfrost/panfrost_gem_shrinker.c | 2 +- drivers/gpu/drm/panfrost/panfrost_mmu.c | 5 +- drivers/gpu/drm/panfrost/panfrost_perfcnt.c | 6 +- drivers/gpu/drm/v3d/v3d_bo.c | 22 +-- drivers/gpu/drm/virtio/virtgpu_object.c | 27 ++- include/drm/drm_gem_shmem_helper.h| 168 +++--- 11 files changed, 251 insertions(+), 149 deletions(-) -- 2.33.1
[PATCH 2/3] drm/shmem-helper: Export dedicated wrappers for GEM object functions
Wrap GEM SHMEM functions for struct drm_gem_object_funcs and update
all callers. This will allow for an update of the public interfaces
of the GEM SHMEM helper library.
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/drm_gem_shmem_helper.c | 45 -
drivers/gpu/drm/lima/lima_gem.c | 8 +-
drivers/gpu/drm/panfrost/panfrost_gem.c | 12 +--
drivers/gpu/drm/v3d/v3d_bo.c| 14 +--
drivers/gpu/drm/virtio/virtgpu_object.c | 15 ++-
include/drm/drm_gem_shmem_helper.h | 120
6 files changed, 161 insertions(+), 53 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
b/drivers/gpu/drm/drm_gem_shmem_helper.c
index cd93e91b3487..72ac263f20be 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -30,14 +30,14 @@
*/
static const struct drm_gem_object_funcs drm_gem_shmem_funcs = {
- .free = drm_gem_shmem_free_object,
- .print_info = drm_gem_shmem_print_info,
- .pin = drm_gem_shmem_pin,
- .unpin = drm_gem_shmem_unpin,
- .get_sg_table = drm_gem_shmem_get_sg_table,
- .vmap = drm_gem_shmem_vmap,
- .vunmap = drm_gem_shmem_vunmap,
- .mmap = drm_gem_shmem_mmap,
+ .free = drm_gem_shmem_object_free,
+ .print_info = drm_gem_shmem_object_print_info,
+ .pin = drm_gem_shmem_object_pin,
+ .unpin = drm_gem_shmem_object_unpin,
+ .get_sg_table = drm_gem_shmem_object_get_sg_table,
+ .vmap = drm_gem_shmem_object_vmap,
+ .vunmap = drm_gem_shmem_object_vunmap,
+ .mmap = drm_gem_shmem_object_mmap,
};
static struct drm_gem_shmem_object *
@@ -121,8 +121,7 @@ EXPORT_SYMBOL_GPL(drm_gem_shmem_create);
* @obj: GEM object to free
*
* This function cleans up the GEM object state and frees the memory used to
- * store the object itself. It should be used to implement
- * &drm_gem_object_funcs.free.
+ * store the object itself.
*/
void drm_gem_shmem_free_object(struct drm_gem_object *obj)
{
@@ -248,8 +247,7 @@ EXPORT_SYMBOL(drm_gem_shmem_put_pages);
* @obj: GEM object
*
* This function makes sure the backing pages are pinned in memory while the
- * buffer is exported. It should only be used to implement
- * &drm_gem_object_funcs.pin.
+ * buffer is exported.
*
* Returns:
* 0 on success or a negative error code on failure.
@@ -269,7 +267,7 @@ EXPORT_SYMBOL(drm_gem_shmem_pin);
* @obj: GEM object
*
* This function removes the requirement that the backing pages are pinned in
- * memory. It should only be used to implement &drm_gem_object_funcs.unpin.
+ * memory.
*/
void drm_gem_shmem_unpin(struct drm_gem_object *obj)
{
@@ -340,11 +338,8 @@ static int drm_gem_shmem_vmap_locked(struct
drm_gem_shmem_object *shmem, struct
* store.
*
* This function makes sure that a contiguous kernel virtual address mapping
- * exists for the buffer backing the shmem GEM object.
- *
- * This function can be used to implement &drm_gem_object_funcs.vmap. But it
can
- * also be called by drivers directly, in which case it will hide the
- * differences between dma-buf imported and natively allocated objects.
+ * exists for the buffer backing the shmem GEM object. It hides the differences
+ * between dma-buf imported and natively allocated objects.
*
* Acquired mappings should be cleaned up by calling drm_gem_shmem_vunmap().
*
@@ -396,9 +391,8 @@ static void drm_gem_shmem_vunmap_locked(struct
drm_gem_shmem_object *shmem,
* drm_gem_shmem_vmap(). The mapping is only removed when the use count drops
to
* zero.
*
- * This function can be used to implement &drm_gem_object_funcs.vmap. But it
can
- * also be called by drivers directly, in which case it will hide the
- * differences between dma-buf imported and natively allocated objects.
+ * This function hides the differences between dma-buf imported and natively
+ * allocated objects.
*/
void drm_gem_shmem_vunmap(struct drm_gem_object *obj, struct dma_buf_map *map)
{
@@ -604,8 +598,7 @@ static const struct vm_operations_struct
drm_gem_shmem_vm_ops = {
* @vma: VMA for the area to be mapped
*
* This function implements an augmented version of the GEM DRM file mmap
- * operation for shmem objects. Drivers which employ the shmem helpers should
- * use this function as their &drm_gem_object_funcs.mmap handler.
+ * operation for shmem objects.
*
* Returns:
* 0 on success or a negative error code on failure.
@@ -646,8 +639,6 @@ EXPORT_SYMBOL_GPL(drm_gem_shmem_mmap);
* @p: DRM printer
* @indent: Tab indentation level
* @obj: GEM object
- *
- * This implements the &drm_gem_object_funcs.info callback.
*/
void drm_gem_shmem_print_info(struct drm_printer *p, unsigned int indent,
const struct drm_gem_object *obj)
@@ -666,9 +657,7 @@ EXPORT_SYMBOL(drm_gem_shmem_print_info);
* @obj: GEM object
*
* This function exports a scatter/gather table suitable for PRIME usage by
- * calling the standard DMA mapp
[PATCH 3/3] drm/shmem-helper: Pass GEM shmem object in public interfaces
Change all GEM SHMEM object functions that receive a GEM object
of type struct drm_gem_object to expect an object of type
struct drm_gem_shmem_object instead.
This change reduces the number of upcasts from struct drm_gem_object
by moving them into callers. The C compiler can now verify that the
GEM SHMEM functions are called with the correct type.
For consistency, the patch also renames drm_gem_shmem_free_object to
drm_gem_shmem_free. It further updates documentation for a number of
functions.
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/drm_gem_shmem_helper.c| 78 ---
drivers/gpu/drm/lima/lima_gem.c | 10 +--
drivers/gpu/drm/lima/lima_sched.c | 4 +-
drivers/gpu/drm/panfrost/panfrost_drv.c | 2 +-
drivers/gpu/drm/panfrost/panfrost_gem.c | 8 +-
.../gpu/drm/panfrost/panfrost_gem_shrinker.c | 2 +-
drivers/gpu/drm/panfrost/panfrost_mmu.c | 5 +-
drivers/gpu/drm/panfrost/panfrost_perfcnt.c | 6 +-
drivers/gpu/drm/v3d/v3d_bo.c | 8 +-
drivers/gpu/drm/virtio/virtgpu_object.c | 12 +--
include/drm/drm_gem_shmem_helper.h| 65 +---
11 files changed, 100 insertions(+), 100 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
b/drivers/gpu/drm/drm_gem_shmem_helper.c
index 72ac263f20be..6521b0604a0f 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -117,15 +117,15 @@ struct drm_gem_shmem_object *drm_gem_shmem_create(struct
drm_device *dev, size_t
EXPORT_SYMBOL_GPL(drm_gem_shmem_create);
/**
- * drm_gem_shmem_free_object - Free resources associated with a shmem GEM
object
- * @obj: GEM object to free
+ * drm_gem_shmem_free - Free resources associated with a shmem GEM object
+ * @shmem: shmem GEM object to free
*
* This function cleans up the GEM object state and frees the memory used to
* store the object itself.
*/
-void drm_gem_shmem_free_object(struct drm_gem_object *obj)
+void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem)
{
- struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
+ struct drm_gem_object *obj = &shmem->base;
WARN_ON(shmem->vmap_use_count);
@@ -149,7 +149,7 @@ void drm_gem_shmem_free_object(struct drm_gem_object *obj)
mutex_destroy(&shmem->vmap_lock);
kfree(shmem);
}
-EXPORT_SYMBOL_GPL(drm_gem_shmem_free_object);
+EXPORT_SYMBOL_GPL(drm_gem_shmem_free);
static int drm_gem_shmem_get_pages_locked(struct drm_gem_shmem_object *shmem)
{
@@ -244,7 +244,7 @@ EXPORT_SYMBOL(drm_gem_shmem_put_pages);
/**
* drm_gem_shmem_pin - Pin backing pages for a shmem GEM object
- * @obj: GEM object
+ * @shmem: shmem GEM object
*
* This function makes sure the backing pages are pinned in memory while the
* buffer is exported.
@@ -252,10 +252,8 @@ EXPORT_SYMBOL(drm_gem_shmem_put_pages);
* Returns:
* 0 on success or a negative error code on failure.
*/
-int drm_gem_shmem_pin(struct drm_gem_object *obj)
+int drm_gem_shmem_pin(struct drm_gem_shmem_object *shmem)
{
- struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
-
WARN_ON(shmem->base.import_attach);
return drm_gem_shmem_get_pages(shmem);
@@ -264,15 +262,13 @@ EXPORT_SYMBOL(drm_gem_shmem_pin);
/**
* drm_gem_shmem_unpin - Unpin backing pages for a shmem GEM object
- * @obj: GEM object
+ * @shmem: shmem GEM object
*
* This function removes the requirement that the backing pages are pinned in
* memory.
*/
-void drm_gem_shmem_unpin(struct drm_gem_object *obj)
+void drm_gem_shmem_unpin(struct drm_gem_shmem_object *shmem)
{
- struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
-
WARN_ON(shmem->base.import_attach);
drm_gem_shmem_put_pages(shmem);
@@ -346,9 +342,8 @@ static int drm_gem_shmem_vmap_locked(struct
drm_gem_shmem_object *shmem, struct
* Returns:
* 0 on success or a negative error code on failure.
*/
-int drm_gem_shmem_vmap(struct drm_gem_object *obj, struct dma_buf_map *map)
+int drm_gem_shmem_vmap(struct drm_gem_shmem_object *shmem, struct dma_buf_map
*map)
{
- struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
int ret;
ret = mutex_lock_interruptible(&shmem->vmap_lock);
@@ -394,10 +389,8 @@ static void drm_gem_shmem_vunmap_locked(struct
drm_gem_shmem_object *shmem,
* This function hides the differences between dma-buf imported and natively
* allocated objects.
*/
-void drm_gem_shmem_vunmap(struct drm_gem_object *obj, struct dma_buf_map *map)
+void drm_gem_shmem_vunmap(struct drm_gem_shmem_object *shmem, struct
dma_buf_map *map)
{
- struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
-
mutex_lock(&shmem->vmap_lock);
drm_gem_shmem_vunmap_locked(shmem, map);
mutex_unlock(&shmem->vmap_lock);
@@ -432,10 +425,8 @@ drm_gem_shmem_create_with_handle(struct drm_file
*file_priv,
/* Update madvise status,
Re: [PATCH v2 2/2] drm: Move nomodeset kernel parameter to the DRM subsystem
Hi
Am 05.11.21 um 10:22 schrieb Jani Nikula:
On Fri, 05 Nov 2021, Thomas Zimmermann wrote:
Hi
Am 04.11.21 um 17:07 schrieb Javier Martinez Canillas:
The "nomodeset" kernel cmdline parameter is handled by the vgacon driver
but the exported vgacon_text_force() symbol is only used by DRM drivers.
It makes much more sense for the parameter logic to be in the subsystem
of the drivers that are making use of it.
Let's move the vgacon_text_force() function and related logic to the DRM
subsystem. While doing that, rename the function to drm_check_modeset()
which better reflects what the function is really used to test for.
Suggested-by: Daniel Vetter
Signed-off-by: Javier Martinez Canillas
---
Changes in v2:
- Conditionally build drm_nomodeset.o if CONFIG_VGA_CONSOLE is set.
- Squash patches to move nomodeset logic to DRM and do the renaming.
- Name the function drm_check_modeset() and make it return -ENODEV.
drivers/gpu/drm/Makefile| 2 ++
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 1 -
drivers/gpu/drm/ast/ast_drv.c | 1 -
drivers/gpu/drm/drm_drv.c | 9 +
drivers/gpu/drm/drm_nomodeset.c | 26 +
drivers/gpu/drm/i915/i915_module.c | 2 --
drivers/gpu/drm/mgag200/mgag200_drv.c | 1 -
drivers/gpu/drm/nouveau/nouveau_drm.c | 1 -
drivers/gpu/drm/qxl/qxl_drv.c | 1 -
drivers/gpu/drm/radeon/radeon_drv.c | 1 -
drivers/gpu/drm/tiny/bochs.c| 1 -
drivers/gpu/drm/tiny/cirrus.c | 1 -
drivers/gpu/drm/vboxvideo/vbox_drv.c| 1 -
drivers/gpu/drm/virtio/virtgpu_drv.c| 1 -
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 1 -
drivers/video/console/vgacon.c | 21
include/drm/drm_mode_config.h | 6 ++
include/linux/console.h | 6 --
18 files changed, 39 insertions(+), 44 deletions(-)
create mode 100644 drivers/gpu/drm/drm_nomodeset.c
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 1c41156deb5f..c74810c285af 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -33,6 +33,8 @@ drm-$(CONFIG_DRM_PRIVACY_SCREEN) += drm_privacy_screen.o
drm_privacy_screen_x86.
obj-$(CONFIG_DRM_DP_AUX_BUS) += drm_dp_aux_bus.o
+obj-$(CONFIG_VGA_CONSOLE) += drm_nomodeset.o
+
This now depends on the VGA textmode console. Even if you have no VGA
console, you'd want drm_nomodeset.o. Simpledrm might be built-in and can
provide graphics. Non-PC systems don't even have a VGA device.
This was discussed in an earlier version, which had this builtin but the
header still had a stub for CONFIG_VGA_CONSOLE=n.
I think we really want a separate boolean config option that gets
selected by CONFIG_DRM.
Perhaps that should be a separate change on top.
Sure, make it a separate patch.
We want to make this work on ARM systems. I even have a request to
replace offb on Power architecture by simpledrm. So the final config has
to be system agnostic.
Best regards
Thomas
BR,
Jani.
drm_cma_helper-y := drm_gem_cma_helper.o
obj-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_cma_helper.o
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 7fde40d06181..b4b6993861e6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -31,7 +31,6 @@
#include "amdgpu_drv.h"
#include
-#include
#include
#include
#include
diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index 802063279b86..6222082c3082 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -26,7 +26,6 @@
* Authors: Dave Airlie
*/
-#include
#include
#include
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 3fb567d62881..80b85b8ea776 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -986,12 +986,13 @@ EXPORT_SYMBOL(drm_dev_set_unique);
*/
int drm_drv_enabled(const struct drm_driver *driver)
{
- if (vgacon_text_force()) {
+ int ret;
+
+ ret = drm_check_modeset();
+ if (ret)
DRM_INFO("%s driver is disabled\n", driver->name);
- return -ENODEV;
- }
- return 0;
+ return ret;
}
EXPORT_SYMBOL(drm_drv_enabled);
diff --git a/drivers/gpu/drm/drm_nomodeset.c b/drivers/gpu/drm/drm_nomodeset.c
new file mode 100644
index ..6683e396d2c5
--- /dev/null
+++ b/drivers/gpu/drm/drm_nomodeset.c
@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include
+#include
+
+static bool drm_nomodeset;
+
+int drm_check_modeset(void)
+{
+ return drm_nomodeset ? -ENODEV : 0;
+}
+EXPORT_SYMBOL(drm_check_modeset);
+
+static int __init disable_modeset(char *str)
+{
+ drm_nomodeset = true;
+
+ pr_warn("You have booted with nomodeset. This means your GPU drivers are
DISABLE
Re: [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled
Hello Thomas,
On 11/5/21 09:43, Thomas Zimmermann wrote:
> Hi
>
> Am 04.11.21 um 21:09 schrieb Javier Martinez Canillas:
>> Hello Jani,
>>
>> On 11/4/21 20:57, Jani Nikula wrote:
>>> On Thu, 04 Nov 2021, Javier Martinez Canillas wrote:
+/**
+ * drm_drv_enabled - Checks if a DRM driver can be enabled
+ * @driver: DRM driver to check
+ *
+ * Checks whether a DRM driver can be enabled or not. This may be the case
+ * if the "nomodeset" kernel command line parameter is used.
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
+int drm_drv_enabled(const struct drm_driver *driver)
>
> Jani mentioned that i915 absolutely wants this to run from the
> module_init function. Best is to drop the parameter.
>
Ok. I now wonder though how much value would add this function since
it will just be a wrapper around the nomodeset check.
We talked about adding a new DRIVER_GENERIC feature flag and check for
this, but as danvet mentioned that is not really needed. We just need
to avoid testing for nomodeset in the simpledrm driver.
Do you envision other condition that could be added later to disable a
DRM driver ? Or do you think that just from a code readability point of
view makes worth it ?
+{
+ if (vgacon_text_force()) {
+ DRM_INFO("%s driver is disabled\n", driver->name);
+ return -ENODEV;
+ }
>
> If we run this from within a module_init function, we'd get plenty of
> these warnings if drivers are compiled into the kernel. Maybe simply
> remove the message. There's already a warning printed by the nomodeset
> handler.
>
Indeed. I'll just drop it.
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_drv_enabled);
>>>
>>> The name implies a bool return, but it's not.
>>>
>>> if (drm_drv_enabled(...)) {
>>> /* surprise, it's disabled! */
>>> }
>>>
>>
>> It used to return a bool in v2 but Thomas suggested an int instead to
>> have consistency on the errno code that was returned by the callers.
>>
>> I should probably name that function differently to avoid confusion.
>
> Yes, please.
>
drm_driver_check() maybe ?
Best regards,
--
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH v2 2/2] drm: Move nomodeset kernel parameter to the DRM subsystem
On 11/5/21 10:00, Thomas Zimmermann wrote:
[snip]
>> +
>> +static int __init disable_modeset(char *str)
>> +{
>> +drm_nomodeset = true;
>> +
>> +pr_warn("You have booted with nomodeset. This means your GPU drivers
>> are DISABLED\n");
>> +pr_warn("Any video related functionality will be severely degraded, and
>> you may not even be able to suspend the system properly\n");
>> +pr_warn("Unless you actually understand what nomodeset does, you should
>> reboot without enabling it\n");
>
> I'd update this text to be less sensational.
>
This is indeed quite sensational. But think we can do it as a follow-up patch.
Since we will have to stick with nomodeset for backward compatibility, I was
planning to add documentation to explain what this parameter is about and what
is the actual effect of setting it.
So I think we can change this as a part of that patch-set.
>> +
>> +return 1;
>> +}
>> +
>> +/* Disable kernel modesetting */
>> +__setup("nomodeset", disable_modeset);
>> diff --git a/drivers/gpu/drm/i915/i915_module.c
>> b/drivers/gpu/drm/i915/i915_module.c
>> index 45cb3e540eff..c890c1ca20c4 100644
>> --- a/drivers/gpu/drm/i915/i915_module.c
>> +++ b/drivers/gpu/drm/i915/i915_module.c
>> @@ -4,8 +4,6 @@
>>* Copyright © 2021 Intel Corporation
>>*/
>>
>> -#include
>> -
>
> These changes should be in patch 1?
>
Yes, I forgot to move these when changed the order of the patches.
Best regards, --
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH v2 2/2] drm: Move nomodeset kernel parameter to the DRM subsystem
On 11/5/21 10:39, Thomas Zimmermann wrote: [snip] +obj-$(CONFIG_VGA_CONSOLE) += drm_nomodeset.o + >>> >>> This now depends on the VGA textmode console. Even if you have no VGA >>> console, you'd want drm_nomodeset.o. Simpledrm might be built-in and can >>> provide graphics. Non-PC systems don't even have a VGA device. >> >> This was discussed in an earlier version, which had this builtin but the >> header still had a stub for CONFIG_VGA_CONSOLE=n. >> >>> I think we really want a separate boolean config option that gets >>> selected by CONFIG_DRM. >> >> Perhaps that should be a separate change on top. > > Sure, make it a separate patch. > Agreed. I was planning to do it as a follow-up as well and drop the #ifdef CONFIG_VGA_CONSOLE guard in the header. > We want to make this work on ARM systems. I even have a request to > replace offb on Power architecture by simpledrm. So the final config has > to be system agnostic. > Same, since we want to drop the fbdev drivers in Fedora, for all arches. > Best regards > Thomas > Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled
On Fri, 05 Nov 2021, Javier Martinez Canillas wrote:
> Hello Thomas,
>
> On 11/5/21 09:43, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 04.11.21 um 21:09 schrieb Javier Martinez Canillas:
>>> Hello Jani,
>>>
>>> On 11/4/21 20:57, Jani Nikula wrote:
On Thu, 04 Nov 2021, Javier Martinez Canillas wrote:
> +/**
> + * drm_drv_enabled - Checks if a DRM driver can be enabled
> + * @driver: DRM driver to check
> + *
> + * Checks whether a DRM driver can be enabled or not. This may be the
> case
> + * if the "nomodeset" kernel command line parameter is used.
> + *
> + * Return: 0 on success or a negative error code on failure.
> + */
> +int drm_drv_enabled(const struct drm_driver *driver)
>>
>> Jani mentioned that i915 absolutely wants this to run from the
>> module_init function. Best is to drop the parameter.
>>
>
> Ok. I now wonder though how much value would add this function since
> it will just be a wrapper around the nomodeset check.
>
> We talked about adding a new DRIVER_GENERIC feature flag and check for
> this, but as danvet mentioned that is not really needed. We just need
> to avoid testing for nomodeset in the simpledrm driver.
>
> Do you envision other condition that could be added later to disable a
> DRM driver ? Or do you think that just from a code readability point of
> view makes worth it ?
Taking a step back for perspective.
I think there's broad consensus in moving the parameter to drm, naming
the check function to drm_something_something(), and breaking the ties
to CONFIG_VGA_CONSOLE. I appreciate the work you're doing to that
effect.
I think everything beyond that is still a bit vague and/or
contentious. So how about making the first 2-3 patches just that?
Something we can all agree on, makes good progress, improves the kernel,
and gives us something to build on?
BR,
Jani.
>
> +{
> + if (vgacon_text_force()) {
> + DRM_INFO("%s driver is disabled\n", driver->name);
> + return -ENODEV;
> + }
>>
>> If we run this from within a module_init function, we'd get plenty of
>> these warnings if drivers are compiled into the kernel. Maybe simply
>> remove the message. There's already a warning printed by the nomodeset
>> handler.
>>
>
> Indeed. I'll just drop it.
>
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_drv_enabled);
The name implies a bool return, but it's not.
if (drm_drv_enabled(...)) {
/* surprise, it's disabled! */
}
>>>
>>> It used to return a bool in v2 but Thomas suggested an int instead to
>>> have consistency on the errno code that was returned by the callers.
>>>
>>> I should probably name that function differently to avoid confusion.
>>
>> Yes, please.
>>
>
> drm_driver_check() maybe ?
>
> Best regards,
--
Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH v5 2/3] drm: Add Gamma and Degamma LUT sizes props to drm_crtc to validate.
On Thu, Nov 04, 2021 at 05:15:43PM -0400, Ilia Mirkin wrote:
> On Thu, Nov 4, 2021 at 4:03 PM Mark Yacoub wrote:
> >
> > From: Mark Yacoub
> >
> > [Why]
> > 1. drm_atomic_helper_check doesn't check for the LUT sizes of either Gamma
> > or Degamma props in the new CRTC state, allowing any invalid size to
> > be passed on.
> > 2. Each driver has its own LUT size, which could also be different for
> > legacy users.
> >
> > [How]
> > 1. Create |degamma_lut_size| and |gamma_lut_size| to save the LUT sizes
> > assigned by the driver when it's initializing its color and CTM
> > management.
> > 2. Create drm_atomic_helper_check_crtc which is called by
> > drm_atomic_helper_check to check the LUT sizes saved in drm_crtc that
> > they match the sizes in the new CRTC state.
> > 3. As the LUT size check now happens in drm_atomic_helper_check, remove
> > the lut check in intel_color.c
> >
> > Resolves: igt@kms_color@pipe-A-invalid-gamma-lut-sizes on MTK
> > Tested on Zork (amdgpu) and Jacuzzi (mediatek), volteer (TGL)
> >
> > v3:
> > 1. Check for lut pointer inside drm_check_lut_size.
> >
> > v2:
> > 1. Remove the rename to a parent commit.
> > 2. Create a drm drm_check_lut_size instead of intel only function.
> >
> > v1:
> > 1. Fix typos
> > 2. Remove the LUT size check from intel driver
> > 3. Rename old LUT check to indicate it's a channel change
> >
> > Signed-off-by: Mark Yacoub
> > ---
> > drivers/gpu/drm/drm_atomic_helper.c| 52 ++
> > drivers/gpu/drm/drm_color_mgmt.c | 19
> > drivers/gpu/drm/i915/display/intel_color.c | 32 ++---
> > include/drm/drm_atomic_helper.h| 1 +
> > include/drm/drm_color_mgmt.h | 3 ++
> > include/drm/drm_crtc.h | 11 +
> > 6 files changed, 99 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > b/drivers/gpu/drm/drm_atomic_helper.c
> > index bc3487964fb5e..548e5d8221fb4 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -929,6 +929,54 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
> > }
> > EXPORT_SYMBOL(drm_atomic_helper_check_planes);
> >
> > +/**
> > + * drm_atomic_helper_check_crtcs - validate state object for CRTC changes
> > + * @state: the driver state object
> > + *
> > + * Check the CRTC state object such as the Gamma/Degamma LUT sizes if the
> > new
> > + * state holds them.
> > + *
> > + * RETURNS:
> > + * Zero for success or -errno
> > + */
> > +int drm_atomic_helper_check_crtcs(struct drm_atomic_state *state)
> > +{
> > + struct drm_crtc *crtc;
> > + struct drm_crtc_state *new_crtc_state;
> > + int i;
> > +
> > + for_each_new_crtc_in_state (state, crtc, new_crtc_state, i) {
> > + if (!new_crtc_state->color_mgmt_changed)
> > + continue;
> > +
> > + if (drm_check_lut_size(new_crtc_state->gamma_lut,
> > + crtc->gamma_lut_size) ||
> > + drm_check_lut_size(new_crtc_state->gamma_lut,
> > + crtc->gamma_size)) {
> > + drm_dbg_state(
> > + state->dev,
> > + "Invalid Gamma LUT size. Expected %u/%u,
> > got %u.\n",
> > + crtc->gamma_lut_size, crtc->gamma_size,
> > +
> > drm_color_lut_size(new_crtc_state->gamma_lut));
> > + return -EINVAL;
> > + }
> > +
> > + if (drm_check_lut_size(new_crtc_state->degamma_lut,
> > + crtc->degamma_lut_size)) {
> > + drm_dbg_state(
> > + state->dev,
> > + "Invalid Degamma LUT size. Expected %u, got
> > %u.\n",
> > + crtc->degamma_lut_size,
> > + drm_color_lut_size(
> > + new_crtc_state->degamma_lut));
> > + return -EINVAL;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(drm_atomic_helper_check_crtcs);
> > +
> > /**
> > * drm_atomic_helper_check - validate state object
> > * @dev: DRM device
> > @@ -974,6 +1022,10 @@ int drm_atomic_helper_check(struct drm_device *dev,
> > if (ret)
> > return ret;
> >
> > + ret = drm_atomic_helper_check_crtcs(state);
> > + if (ret)
> > + return ret;
> > +
> > if (state->legacy_cursor_update)
> > state->async_update = !drm_atomic_helper_async_check(dev,
> > state);
> >
> > diff --git a/drivers/gpu/drm/drm_color_mgmt.c
> > b/drivers/gpu/drm/drm_color_mgmt.c
> > index 16a07f84948f3..c85094223b535 100644
> > --- a/drivers/gpu/drm/drm_color_mgmt.c
> > +++ b/drivers/gpu/drm/drm_color_mgmt.c
> > @@ -16
Re: [PATCH v2] drm/ttm: Do not put non-struct page memory into PUD/PMDs
On Thu, Oct 28, 2021 at 04:42:56PM -0300, Jason Gunthorpe wrote: > On Thu, Oct 28, 2021 at 05:14:52PM +0200, Daniel Vetter wrote: > > Hm totally lost this, I'm trying to not be too responsible for mm changes > > since it scares me :-) Plus dropping this very late in the release feels a > > bit risky. > > > > Ok if I stuff this into drm-next instead? > > Sure Finally got around to this, should show up in the merge window. Apologies for being everywhere except on dri-devel. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH] drm/udl: fix control-message timeout
On Mon, Oct 25, 2021 at 01:53:53PM +0200, Johan Hovold wrote:
> USB control-message timeouts are specified in milliseconds and should
> specifically not vary with CONFIG_HZ.
>
> Fixes: 5320918b9a87 ("drm/udl: initial UDL driver (v4)")
> Cc: sta...@vger.kernel.org # 3.4
> Signed-off-by: Johan Hovold
Thanks for patch, queued up for the merge window.
-Daniel
> ---
> drivers/gpu/drm/udl/udl_connector.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/udl/udl_connector.c
> b/drivers/gpu/drm/udl/udl_connector.c
> index 3750fd216131..930574ad2bca 100644
> --- a/drivers/gpu/drm/udl/udl_connector.c
> +++ b/drivers/gpu/drm/udl/udl_connector.c
> @@ -30,7 +30,7 @@ static int udl_get_edid_block(void *data, u8 *buf, unsigned
> int block,
> int bval = (i + block * EDID_LENGTH) << 8;
> ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
> 0x02, (0x80 | (0x02 << 5)), bval,
> - 0xA1, read_buff, 2, HZ);
> + 0xA1, read_buff, 2, 1000);
> if (ret < 1) {
> DRM_ERROR("Read EDID byte %d failed err %x\n", i, ret);
> kfree(read_buff);
> --
> 2.32.0
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Re: [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled
Hi
Am 05.11.21 um 10:48 schrieb Javier Martinez Canillas:
Hello Thomas,
On 11/5/21 09:43, Thomas Zimmermann wrote:
Hi
Am 04.11.21 um 21:09 schrieb Javier Martinez Canillas:
Hello Jani,
On 11/4/21 20:57, Jani Nikula wrote:
On Thu, 04 Nov 2021, Javier Martinez Canillas wrote:
+/**
+ * drm_drv_enabled - Checks if a DRM driver can be enabled
+ * @driver: DRM driver to check
+ *
+ * Checks whether a DRM driver can be enabled or not. This may be the case
+ * if the "nomodeset" kernel command line parameter is used.
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
+int drm_drv_enabled(const struct drm_driver *driver)
Jani mentioned that i915 absolutely wants this to run from the
module_init function. Best is to drop the parameter.
Ok. I now wonder though how much value would add this function since
it will just be a wrapper around the nomodeset check.
We talked about adding a new DRIVER_GENERIC feature flag and check for
this, but as danvet mentioned that is not really needed. We just need
to avoid testing for nomodeset in the simpledrm driver.
Do you envision other condition that could be added later to disable a
DRM driver ? Or do you think that just from a code readability point of
view makes worth it ?
DRIVER_GENERIC would have been nice, but it's not happening now.
I suggest to move over the nomodeset parameter (i.e., this patchset),
then make the config option system agnostic, and finally add the test to
all drivers expect simpledrm. That should solve the imminent problem.
Best regards
Thomas
+{
+ if (vgacon_text_force()) {
+ DRM_INFO("%s driver is disabled\n", driver->name);
+ return -ENODEV;
+ }
If we run this from within a module_init function, we'd get plenty of
these warnings if drivers are compiled into the kernel. Maybe simply
remove the message. There's already a warning printed by the nomodeset
handler.
Indeed. I'll just drop it.
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_drv_enabled);
The name implies a bool return, but it's not.
if (drm_drv_enabled(...)) {
/* surprise, it's disabled! */
}
It used to return a bool in v2 but Thomas suggested an int instead to
have consistency on the errno code that was returned by the callers.
I should probably name that function differently to avoid confusion.
Yes, please.
drm_driver_check() maybe ?
Best regards,
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH] MAINTAINERS: dri-devel is for all of drivers/gpu
On Thu, Oct 28, 2021 at 01:36:58PM -0400, Alex Deucher wrote: > On Thu, Oct 28, 2021 at 1:09 PM Daniel Vetter wrote: > > > > Somehow we only have a list of subdirectories, which apparently made > > it harder for folks to find the gpu maintainers. Fix that. > > > > References: > > https://lore.kernel.org/dri-devel/YXrAAZlxxStNFG%2FK@phenom.ffwll.local/ > > Signed-off-by: Daniel Vetter > > Cc: David Airlie > > Cc: Daniel Vetter > > Cc: Steven Rostedt > > Reviewed-by: Alex Deucher 'tis should be good enough, I stuffed this into drm-misc-next-fixes. -Daniel > > > --- > > MAINTAINERS | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 98aa1f55ed41..fdb1f91c6bb9 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -6153,8 +6153,7 @@ T:git git://anongit.freedesktop.org/drm/drm > > F: Documentation/devicetree/bindings/display/ > > F: Documentation/devicetree/bindings/gpu/ > > F: Documentation/gpu/ > > -F: drivers/gpu/drm/ > > -F: drivers/gpu/vga/ > > +F: drivers/gpu/ > > F: include/drm/ > > F: include/linux/vga* > > F: include/uapi/drm/ > > -- > > 2.33.0 > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH v6] backlight: lp855x: Switch to atomic PWM API
Hello, On Thu, Nov 04, 2021 at 02:58:38PM -0300, Maíra Canal wrote: > Remove legacy PWM interface (pwm_config, pwm_enable, pwm_disable) and > replace it for the atomic PWM API. > > Signed-off-by: Maíra Canal LGTM, Reviewed-by: Uwe Kleine-König Thanks Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: linux-next: build failure after merge of the drm-misc tree
On Fri, 05 Nov 2021, Stephen Rothwell wrote:
> Hi all,
>
> On Mon, 1 Nov 2021 19:42:23 +1100 Stephen Rothwell
> wrote:
>>
>> On Fri, 15 Oct 2021 20:26:48 +1100 Stephen Rothwell
>> wrote:
>> >
>> > After merging the drm-misc tree, today's linux-next build (arm
>> > multi_v7_defconfig) failed like this:
>> >
>> > drivers/gpu/drm/drm_modeset_lock.c:111:29: error: conflicting types for
>> > '__stack_depot_save'
>> > 111 | static depot_stack_handle_t __stack_depot_save(void)
>> > | ^~
>> > In file included from include/linux/page_ext.h:7,
>> > from include/linux/mm.h:25,
>> > from include/linux/kallsyms.h:13,
>> > from include/linux/bpf.h:20,
>> > from include/linux/bpf-cgroup.h:5,
>> > from include/linux/cgroup-defs.h:22,
>> > from include/linux/cgroup.h:28,
>> > from include/linux/memcontrol.h:13,
>> > from include/linux/swap.h:9,
>> > from include/linux/suspend.h:5,
>> > from include/linux/regulator/consumer.h:35,
>> > from include/linux/i2c.h:18,
>> > from include/drm/drm_crtc.h:28,
>> > from include/drm/drm_atomic.h:31,
>> > from drivers/gpu/drm/drm_modeset_lock.c:24:
>> > include/linux/stackdepot.h:18:22: note: previous declaration of
>> > '__stack_depot_save' was here
>> >18 | depot_stack_handle_t __stack_depot_save(unsigned long *entries,
>> > | ^~
>> >
>> > Caused by commit
>> >
>> > cd06ab2fd48f ("drm/locking: add backtrace for locking contended locks
>> > without backoff")
>> >
>> > This may only have been revealed because of another fix I have had to
>> > apply today.
>> >
>> > I have applied the following patch for today.
>> >
>> > From: Stephen Rothwell
>> > Date: Fri, 15 Oct 2021 20:17:52 +1100
>> > Subject: [PATCH] drm/locking: fix for name conflict
>> >
>> > Fixes: cd06ab2fd48f ("drm/locking: add backtrace for locking contended
>> > locks without backoff")
>> > Signed-off-by: Stephen Rothwell
>> > ---
>> > drivers/gpu/drm/drm_modeset_lock.c | 6 +++---
>> > 1 file changed, 3 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/drm_modeset_lock.c
>> > b/drivers/gpu/drm/drm_modeset_lock.c
>> > index 4d32b61fa1fd..ee36dd20900d 100644
>> > --- a/drivers/gpu/drm/drm_modeset_lock.c
>> > +++ b/drivers/gpu/drm/drm_modeset_lock.c
>> > @@ -79,7 +79,7 @@
>> > static DEFINE_WW_CLASS(crtc_ww_class);
>> >
>> > #if IS_ENABLED(CONFIG_DRM_DEBUG_MODESET_LOCK)
>> > -static noinline depot_stack_handle_t __stack_depot_save(void)
>> > +static noinline depot_stack_handle_t __drm_stack_depot_save(void)
>> > {
>> >unsigned long entries[8];
>> >unsigned int n;
>> > @@ -108,7 +108,7 @@ static void __stack_depot_print(depot_stack_handle_t
>> > stack_depot)
>> >kfree(buf);
>> > }
>> > #else /* CONFIG_DRM_DEBUG_MODESET_LOCK */
>> > -static depot_stack_handle_t __stack_depot_save(void)
>> > +static depot_stack_handle_t __drm_stack_depot_save(void)
>> > {
>> >return 0;
>> > }
>> > @@ -317,7 +317,7 @@ static inline int modeset_lock(struct drm_modeset_lock
>> > *lock,
>> >ret = 0;
>> >} else if (ret == -EDEADLK) {
>> >ctx->contended = lock;
>> > - ctx->stack_depot = __stack_depot_save();
>> > + ctx->stack_depot = __drm_stack_depot_save();
>> >}
>> >
>> >return ret;
>>
>> This has reappeared today. I don't know what happened to the drm-misc
>> tree over the weeked :-(
>>
>> I have reapplied the above fix.
>
> So the above drm-misc commit is now in the drm tree, but its fix up
> commit vanished from the drm-misc tree over the past weekend :-(
Cc: drm-misc maintainers.
We normally point drm-misc/for-linux-next at drm-misc-next, *except* to
drm-misc-next-fixes during the merge window. This is because
drm-misc-next already starts accumulating stuff that's headed to one
release later, e.g. currently v5.17. I think that's part of the reason.
I probably should have pushed c4f08d7246a5 ("drm/locking: fix
__stack_depot_* name conflict") to drm-misc-next-fixes.
There's still something funny going on, because the drm-misc-next pull
request [1] isn't part of the drm pull request for v5.16 [2]. Is there
going to be another drm pull?
BR,
Jani.
[1] https://lore.kernel.org/r/20211014120452.2wicnt6hobu3kbwb@gilmour
[2]
https://lore.kernel.org/r/CAPM=9tyOyz4_-OdjDduFkponSXycO6maBDFsWGTLv+j=_vp...@mail.gmail.com
--
Jani Nikula, Intel Open Source Graphics Center
Re: [RFC v2 02/22] drm: Add Enhanced Gamma and color lut range attributes
On Thu, Nov 04, 2021 at 12:27:56PM -0400, Harry Wentland wrote:
>
>
> On 2021-11-04 04:38, Pekka Paalanen wrote:
> > On Wed, 3 Nov 2021 11:08:13 -0400
> > Harry Wentland wrote:
> >
> >> On 2021-09-06 17:38, Uma Shankar wrote:
> >>> Existing LUT precision structure is having only 16 bit
> >>> precision. This is not enough for upcoming enhanced hardwares
> >>> and advance usecases like HDR processing. Hence added a new
> >>> structure with 32 bit precision values.
> >>>
> >>> This also defines a new structure to define color lut ranges,
> >>> along with related macro definitions and enums. This will help
> >>> describe multi segmented lut ranges in the hardware.
> >>>
> >>> Signed-off-by: Uma Shankar
> >>> ---
> >>> include/uapi/drm/drm_mode.h | 58 +
> >>> 1 file changed, 58 insertions(+)
> >>>
> >>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> >>> index 90c55383f1ee..1079794c86c3 100644
> >>> --- a/include/uapi/drm/drm_mode.h
> >>> +++ b/include/uapi/drm/drm_mode.h
> >>> @@ -903,6 +903,64 @@ struct hdr_output_metadata {
> >>> };
> >>> };
> >>>
> >>> +/*
> >>> + * DRM_MODE_LUT_GAMMA|DRM_MODE_LUT_DEGAMMA is legal and means the LUT
> >>> + * can be used for either purpose, but not simultaneously. To expose
> >>> + * modes that support gamma and degamma simultaneously the gamma mode
> >>> + * must declare distinct DRM_MODE_LUT_GAMMA and DRM_MODE_LUT_DEGAMMA
> >>> + * ranges.
> >>> + */
> >>> +/* LUT is for gamma (after CTM) */
> >>> +#define DRM_MODE_LUT_GAMMA BIT(0)
> >>> +/* LUT is for degamma (before CTM) */
> >>> +#define DRM_MODE_LUT_DEGAMMA BIT(1)
> >>> +/* linearly interpolate between the points */
> >>> +#define DRM_MODE_LUT_INTERPOLATE BIT(2)
> >>> +/*
> >>> + * the last value of the previous range is the
> >>> + * first value of the current range.
> >>> + */
> >>> +#define DRM_MODE_LUT_REUSE_LAST BIT(3)
> >>> +/* the curve must be non-decreasing */
> >>> +#define DRM_MODE_LUT_NON_DECREASING BIT(4)
> >>> +/* the curve is reflected across origin for negative inputs */
> >>> +#define DRM_MODE_LUT_REFLECT_NEGATIVE BIT(5)
> >>> +/* the same curve (red) is used for blue and green channels as well */
> >>> +#define DRM_MODE_LUT_SINGLE_CHANNEL BIT(6)
> >>> +
> >>> +struct drm_color_lut_range {
> >>> + /* DRM_MODE_LUT_* */
> >>> + __u32 flags;
> >>> + /* number of points on the curve */
> >>> + __u16 count;
> >>> + /* input/output bits per component */
> >>> + __u8 input_bpc, output_bpc;
> >>> + /* input start/end values */
> >>> + __s32 start, end;
> >>> + /* output min/max values */
> >>> + __s32 min, max;
> >>> +};
> >>> +
> >>> +enum lut_type {
> >>> + LUT_TYPE_DEGAMMA = 0,
> >>> + LUT_TYPE_GAMMA = 1,
> >>> +};
> >>> +
> >>> +/*
> >>> + * Creating 64 bit palette entries for better data
> >>> + * precision. This will be required for HDR and
> >>> + * similar color processing usecases.
> >>> + */
> >>> +struct drm_color_lut_ext {
> >>> + /*
> >>> + * Data is U32.32 fixed point format.
> >>> + */
> >>> + __u64 red;
> >>> + __u64 green;
> >>> + __u64 blue;
> >>> + __u64 reserved;
> >>> +};
> >>
> >> I've been drawing out examples of drm_color_lut_range defined PWLs
> >> and understand a bit better what you and Ville are trying to accomplish
> >> with it. It actually makes a lot of sense and would allow for a generic
> >> way to populate different PWL definitions with a generic function.
> >>
> >> But I still have some key questions that either are not answered in these
> >> patch sets or that I somehow overlooked.
> >>
> >> Can you explain how the U32.32 fixed point format relates to the input_bpc
> >> and output_bpc in drm_color_lut_range, as we as to the pixel coming in from
> >> the framebuffer.
> >>
> >> E.g. if we have ARGB2101010 what happens to a 0x3ff red value (assuming
> >> alpha
> >> is non-multiplied)?
> >>
> >> If the drm_color_lut_range segments are defined with input_bpc of 24 bpc
> >> will
> >> 0x3ff be zero-expanded to 24-bit? Is the 24 bpc an integer? I.e. would our
> >> 3xff
> >> be interpreted as 0x3ff << (24-10)?
> >>
> >> Assuming the output_bpc is 16 bpc and the programmed LUT makes this 1-to-1
> >> would
> >> the output value be 0x3ff << (16-10)?
> >>
> >> On AMD HW the pipe-internal format is a custom floating point format. We
> >> could
> >> probably express that in terms of input/output_bpc and do the translation
> >> in our
> >> driver between that and the internal floating point format, depending on
> >> the
> >> framebuffer format, but there is the added complication of the magnitude
> >> of the
> >> pixel data and correlating HDR with SDR planes.
> >>
> >> E.g. any SDR data would map from 0.0 to 1.0 floating point, while HDR
> >> content would
> >> map from 0.0 to some value larger than 1.0. I don't (yet) have a clear
> >> picture how
> >> to represent that with the drm_color_lut_range definition.
> >
> >
> > Hi Harry,
> >
> > I think you just would not. Conceptually an SDR plane gets it
Re: [PATCH 0/9] drm/vc4: Introduce locking and remove !KMS state access
On Mon, 25 Oct 2021 16:11:04 +0200, Maxime Ripard wrote: > This is a follow-up of the series here: > https://lore.kernel.org/all/20210924135530.1036564-1-max...@cerno.tech/ > > and the discussion that occured here: > https://lore.kernel.org/all/YWgteNaNeaS9uWDe@phenom.ffwll.local/ > > The original series aimed at getting rid of the encoder->crtc pointer usage in > the vc4 HDMI driver, some regression was noticed and the following discussion > pointed out that we were doing a fair number of KMS state access outside of > the > mode set path, which is disallowed and unsafe. > > [...] Applied to drm/drm-misc (drm-misc-next). Thanks! Maxime
Re: linux-next: build failure after merge of the drm-misc tree
Hi,
On Fri, Nov 05, 2021 at 01:03:43PM +0200, Jani Nikula wrote:
> On Fri, 05 Nov 2021, Stephen Rothwell wrote:
> > Hi all,
> >
> > On Mon, 1 Nov 2021 19:42:23 +1100 Stephen Rothwell
> > wrote:
> >>
> >> On Fri, 15 Oct 2021 20:26:48 +1100 Stephen Rothwell
> >> wrote:
> >> >
> >> > After merging the drm-misc tree, today's linux-next build (arm
> >> > multi_v7_defconfig) failed like this:
> >> >
> >> > drivers/gpu/drm/drm_modeset_lock.c:111:29: error: conflicting types for
> >> > '__stack_depot_save'
> >> > 111 | static depot_stack_handle_t __stack_depot_save(void)
> >> > | ^~
> >> > In file included from include/linux/page_ext.h:7,
> >> > from include/linux/mm.h:25,
> >> > from include/linux/kallsyms.h:13,
> >> > from include/linux/bpf.h:20,
> >> > from include/linux/bpf-cgroup.h:5,
> >> > from include/linux/cgroup-defs.h:22,
> >> > from include/linux/cgroup.h:28,
> >> > from include/linux/memcontrol.h:13,
> >> > from include/linux/swap.h:9,
> >> > from include/linux/suspend.h:5,
> >> > from include/linux/regulator/consumer.h:35,
> >> > from include/linux/i2c.h:18,
> >> > from include/drm/drm_crtc.h:28,
> >> > from include/drm/drm_atomic.h:31,
> >> > from drivers/gpu/drm/drm_modeset_lock.c:24:
> >> > include/linux/stackdepot.h:18:22: note: previous declaration of
> >> > '__stack_depot_save' was here
> >> >18 | depot_stack_handle_t __stack_depot_save(unsigned long *entries,
> >> > | ^~
> >> >
> >> > Caused by commit
> >> >
> >> > cd06ab2fd48f ("drm/locking: add backtrace for locking contended locks
> >> > without backoff")
> >> >
> >> > This may only have been revealed because of another fix I have had to
> >> > apply today.
> >> >
> >> > I have applied the following patch for today.
> >> >
> >> > From: Stephen Rothwell
> >> > Date: Fri, 15 Oct 2021 20:17:52 +1100
> >> > Subject: [PATCH] drm/locking: fix for name conflict
> >> >
> >> > Fixes: cd06ab2fd48f ("drm/locking: add backtrace for locking contended
> >> > locks without backoff")
> >> > Signed-off-by: Stephen Rothwell
> >> > ---
> >> > drivers/gpu/drm/drm_modeset_lock.c | 6 +++---
> >> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/drm_modeset_lock.c
> >> > b/drivers/gpu/drm/drm_modeset_lock.c
> >> > index 4d32b61fa1fd..ee36dd20900d 100644
> >> > --- a/drivers/gpu/drm/drm_modeset_lock.c
> >> > +++ b/drivers/gpu/drm/drm_modeset_lock.c
> >> > @@ -79,7 +79,7 @@
> >> > static DEFINE_WW_CLASS(crtc_ww_class);
> >> >
> >> > #if IS_ENABLED(CONFIG_DRM_DEBUG_MODESET_LOCK)
> >> > -static noinline depot_stack_handle_t __stack_depot_save(void)
> >> > +static noinline depot_stack_handle_t __drm_stack_depot_save(void)
> >> > {
> >> > unsigned long entries[8];
> >> > unsigned int n;
> >> > @@ -108,7 +108,7 @@ static void __stack_depot_print(depot_stack_handle_t
> >> > stack_depot)
> >> > kfree(buf);
> >> > }
> >> > #else /* CONFIG_DRM_DEBUG_MODESET_LOCK */
> >> > -static depot_stack_handle_t __stack_depot_save(void)
> >> > +static depot_stack_handle_t __drm_stack_depot_save(void)
> >> > {
> >> > return 0;
> >> > }
> >> > @@ -317,7 +317,7 @@ static inline int modeset_lock(struct
> >> > drm_modeset_lock *lock,
> >> > ret = 0;
> >> > } else if (ret == -EDEADLK) {
> >> > ctx->contended = lock;
> >> > -ctx->stack_depot = __stack_depot_save();
> >> > +ctx->stack_depot = __drm_stack_depot_save();
> >> > }
> >> >
> >> > return ret;
> >>
> >> This has reappeared today. I don't know what happened to the drm-misc
> >> tree over the weeked :-(
> >>
> >> I have reapplied the above fix.
> >
> > So the above drm-misc commit is now in the drm tree, but its fix up
> > commit vanished from the drm-misc tree over the past weekend :-(
>
> Cc: drm-misc maintainers.
>
> We normally point drm-misc/for-linux-next at drm-misc-next, *except* to
> drm-misc-next-fixes during the merge window. This is because
> drm-misc-next already starts accumulating stuff that's headed to one
> release later, e.g. currently v5.17. I think that's part of the reason.
Indeed
> I probably should have pushed c4f08d7246a5 ("drm/locking: fix
> __stack_depot_* name conflict") to drm-misc-next-fixes.
>
> There's still something funny going on, because the drm-misc-next pull
> request [1] isn't part of the drm pull request for v5.16 [2]. Is there
> going to be another drm pull?
The last drm-misc-next PR for some reason didn't got logged into
patchwork, and Dave missed it.
We found out yesterday, and he pulled it today so I assume there will be
a second PR with that last
Re: [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled
On 11/5/21 11:04, Jani Nikula wrote: > On Fri, 05 Nov 2021, Javier Martinez Canillas wrote: [snip] >> >> Do you envision other condition that could be added later to disable a >> DRM driver ? Or do you think that just from a code readability point of >> view makes worth it ? > > Taking a step back for perspective. > > I think there's broad consensus in moving the parameter to drm, naming > the check function to drm_something_something(), and breaking the ties > to CONFIG_VGA_CONSOLE. I appreciate the work you're doing to that > effect. > Thanks, I appreciate your feedback and comments. > I think everything beyond that is still a bit vague and/or > contentious. So how about making the first 2-3 patches just that? > Something we can all agree on, makes good progress, improves the kernel, > and gives us something to build on? > That works for me. Thomas, do you agree with that approach ? Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
Re: Questions about KMS flip
On Thu, Nov 04, 2021 at 12:44:34PM -0400, Harry Wentland wrote: > +Nick > > It looks to be the old drm_plane_state->fb holds that reference. See > dm_plane_helper_cleanup_fb() in amdgpu_dm.c. BTW looks like you have a possible leak during fb init; amdgpu_display_framebuffer_init() grabs the refs to the BOs, but drm_framebuffer_init() might still fail (at least theoretically) which will then leak those BO refs. > > Harry > > On 2021-11-04 08:51, Christian König wrote: > > Hi guys, > > > > adding the usual suspects which might know that of hand: When we do a KMS > > page flip, who keeps the reference to the BO while it is scanned out? > > > > We are running into warning backtraces from TTM which look more than odd. > > > > Thanks, > > Christian. -- Ville Syrjälä Intel
Re: [RFC v2 05/22] drm/i915/xelpd: Define Degamma Lut range struct for HDR planes
On Wed, Nov 03, 2021 at 11:10:37AM -0400, Harry Wentland wrote:
>
>
> On 2021-09-06 17:38, Uma Shankar wrote:
> > Define the structure with XE_LPD degamma lut ranges. HDR and SDR
> > planes have different capabilities, implemented respective
> > structure for the HDR planes.
> >
> > Signed-off-by: Uma Shankar
> > ---
> > drivers/gpu/drm/i915/display/intel_color.c | 52 ++
> > 1 file changed, 52 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_color.c
> > b/drivers/gpu/drm/i915/display/intel_color.c
> > index afcb4bf3826c..6403bd74324b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_color.c
> > +++ b/drivers/gpu/drm/i915/display/intel_color.c
> > @@ -2092,6 +2092,58 @@ static void icl_read_luts(struct intel_crtc_state
> > *crtc_state)
> > }
> > }
> >
> > + /* FIXME input bpc? */
> > +__maybe_unused
> > +static const struct drm_color_lut_range d13_degamma_hdr[] = {
> > + /* segment 1 */
> > + {
> > + .flags = (DRM_MODE_LUT_GAMMA |
> > + DRM_MODE_LUT_REFLECT_NEGATIVE |
> > + DRM_MODE_LUT_INTERPOLATE |
> > + DRM_MODE_LUT_NON_DECREASING),
> > + .count = 128,
>
> Is the distribution of the 128 entries uniform?
I guess this is the plane gamma thing despite being in intel_color.c,
so yeah I think that's correct.
> If so, is a
> uniform distribution of 128 points across most of the LUT
> good enough for HDR with 128 entries?
No idea how good this actually is. It is .24 so at least
it does have a fair bit of precision.
>
> > + .input_bpc = 24, .output_bpc = 16,
> > + .start = 0, .end = (1 << 24) - 1,
> > + .min = 0, .max = (1 << 24) - 1,
> > + },
> > + /* segment 2 */
> > + {
> > + .flags = (DRM_MODE_LUT_GAMMA |
> > + DRM_MODE_LUT_REFLECT_NEGATIVE |
> > + DRM_MODE_LUT_INTERPOLATE |
> > + DRM_MODE_LUT_REUSE_LAST |
> > + DRM_MODE_LUT_NON_DECREASING),
> > + .count = 1,
> > + .input_bpc = 24, .output_bpc = 16,
> > + .start = (1 << 24) - 1, .end = 1 << 24,
>
> .start and .end are only a single entry apart. Is this correct?
One think I wanted to do is simplify this stuff by getting rid of
.end entirely. So I think this should just be '.start=1<<24' (or
whatever way we decide to specify the input precision, which is
I think another slightly open question).
So for this thing we could just have:
{ .count = 128, .min = 0, .max = (1 << 24) - 1, .start = 0 },
{ .count = 1, .min = 0, .max = (7 << 24) - 1, .start = 1 << 24 },
{ .count = 1, .min = 0, .max = (7 << 24) - 1, .start = 3 << 24 },
{ .count = 1, .min = 0, .max = (7 << 24) - 1, .start = 7 << 24 },
+ flags/etc. which I left out for brevity.
So that is trying to indicate that the first 129 entries are equally
spaced, and would be used to interpolate for input values [0.0,1.0).
Input values [1.0,3.0) would interpolate between entry 128 and 129,
and [3.0,7.0) would interpolate between entry 129 and 130.
--
Ville Syrjälä
Intel
Re: [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled
Hi Am 05.11.21 um 13:00 schrieb Javier Martinez Canillas: On 11/5/21 11:04, Jani Nikula wrote: On Fri, 05 Nov 2021, Javier Martinez Canillas wrote: [snip] Do you envision other condition that could be added later to disable a DRM driver ? Or do you think that just from a code readability point of view makes worth it ? Taking a step back for perspective. I think there's broad consensus in moving the parameter to drm, naming the check function to drm_something_something(), and breaking the ties to CONFIG_VGA_CONSOLE. I appreciate the work you're doing to that effect. Thanks, I appreciate your feedback and comments. I think everything beyond that is still a bit vague and/or contentious. So how about making the first 2-3 patches just that? Something we can all agree on, makes good progress, improves the kernel, and gives us something to build on? That works for me. Thomas, do you agree with that approach ? Sure. I think that's more or less what I proposed in my reply to that mail. Best regards Thomas Best regards, -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev OpenPGP_signature Description: OpenPGP digital signature
[PATCH] drm/i915/gem: Fix gem_madvise for ttm+shmem objects
Gem-TTM objects that are backed by shmem might have populated
page-vectors without having the Gem pages set. Those objects
aren't moved to the correct shrinker / purge list by the
gem_madvise. Furthermore they are purged directly on
MADV_DONTNEED rather than waiting for the shrinker to do that.
For such objects, identified by having the
_SELF_MANAGED_SHRINK_LIST set, make sure they end up on the
correct list and defer purging to the shrinker.
Signed-off-by: Thomas Hellström
---
drivers/gpu/drm/i915/i915_gem.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d0e642c82064..da972c8d45b1 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1005,7 +1005,8 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void *data,
obj->ops->adjust_lru(obj);
}
- if (i915_gem_object_has_pages(obj)) {
+ if (i915_gem_object_has_pages(obj) ||
+ i915_gem_object_has_self_managed_shrink_list(obj)) {
unsigned long flags;
spin_lock_irqsave(&i915->mm.obj_lock, flags);
@@ -1024,7 +1025,8 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void *data,
/* if the object is no longer attached, discard its backing storage */
if (obj->mm.madv == I915_MADV_DONTNEED &&
- !i915_gem_object_has_pages(obj))
+ !i915_gem_object_has_pages(obj) &&
+ !i915_gem_object_has_self_managed_shrink_list(obj))
i915_gem_object_truncate(obj);
args->retained = obj->mm.madv != __I915_MADV_PURGED;
--
2.31.1
Re: [PATCH v6] backlight: lp855x: Switch to atomic PWM API
On Thu, Nov 04, 2021 at 02:58:38PM -0300, Maíra Canal wrote: > Remove legacy PWM interface (pwm_config, pwm_enable, pwm_disable) and > replace it for the atomic PWM API. > > Signed-off-by: Maíra Canal Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH] drm/i915/gem: Fix gem_madvise for ttm+shmem objects
On 05/11/2021 13:03, Thomas Hellström wrote:
Gem-TTM objects that are backed by shmem might have populated
page-vectors without having the Gem pages set. Those objects
aren't moved to the correct shrinker / purge list by the
gem_madvise. Furthermore they are purged directly on
MADV_DONTNEED rather than waiting for the shrinker to do that.
For such objects, identified by having the
_SELF_MANAGED_SHRINK_LIST set, make sure they end up on the
correct list and defer purging to the shrinker.
Signed-off-by: Thomas Hellström
---
drivers/gpu/drm/i915/i915_gem.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d0e642c82064..da972c8d45b1 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1005,7 +1005,8 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void *data,
obj->ops->adjust_lru(obj);
}
- if (i915_gem_object_has_pages(obj)) {
+ if (i915_gem_object_has_pages(obj) ||
+ i915_gem_object_has_self_managed_shrink_list(obj)) {
Makes sense.
unsigned long flags;
spin_lock_irqsave(&i915->mm.obj_lock, flags);
@@ -1024,7 +1025,8 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void *data,
/* if the object is no longer attached, discard its backing storage */
if (obj->mm.madv == I915_MADV_DONTNEED &&
- !i915_gem_object_has_pages(obj))
+ !i915_gem_object_has_pages(obj) &&
+ !i915_gem_object_has_self_managed_shrink_list(obj))
i915_gem_object_truncate(obj);
And it looks like this also matches the workings of lmem, where under
memory pressure we also just purge such objects, instead of moving them,
making sure to keep them first in the LRU?
One thing is to maybe immediately discard already swapped-out objects
here, since the shrinker will be oblivious to them, and they sort of
just linger in swap until the object is destroyed?
args->retained = obj->mm.madv != __I915_MADV_PURGED;
Re: [PATCH 2/2] x86/mm: nuke PAGE_KERNEL_IO
Hi, gentle ping on this. Is it something that could go through the tip
tree?
thanks
Lucas De Marchi
On Thu, Oct 21, 2021 at 11:15:11AM -0700, Lucas De Marchi wrote:
PAGE_KERNEL_IO is only defined for x86 and nowadays is the same as
PAGE_KERNEL. It was different for some time, OR'ing a `_PAGE_IOMAP` flag
in commit be43d72835ba ("x86: add _PAGE_IOMAP pte flag for IO
mappings"). This got removed in commit f955371ca9d3 ("x86: remove the
Xen-specific _PAGE_IOMAP PTE flag"), so today they are just the same.
With the last users outside arch/x86 being remove we can now remove
PAGE_KERNEL_IO.
Signed-off-by: Lucas De Marchi
---
arch/x86/include/asm/fixmap.h| 2 +-
arch/x86/include/asm/pgtable_types.h | 7 ---
arch/x86/mm/ioremap.c| 2 +-
arch/x86/xen/setup.c | 2 +-
include/asm-generic/fixmap.h | 2 +-
5 files changed, 4 insertions(+), 11 deletions(-)
diff --git a/arch/x86/include/asm/fixmap.h b/arch/x86/include/asm/fixmap.h
index d0dcefb5cc59..5e186a69db10 100644
--- a/arch/x86/include/asm/fixmap.h
+++ b/arch/x86/include/asm/fixmap.h
@@ -173,7 +173,7 @@ static inline void __set_fixmap(enum fixed_addresses idx,
* supported for MMIO addresses, so make sure that the memory encryption
* mask is not part of the page attributes.
*/
-#define FIXMAP_PAGE_NOCACHE PAGE_KERNEL_IO_NOCACHE
+#define FIXMAP_PAGE_NOCACHE PAGE_KERNEL_NOCACHE
/*
* Early memremap routines used for in-place encryption. The mappings created
diff --git a/arch/x86/include/asm/pgtable_types.h
b/arch/x86/include/asm/pgtable_types.h
index 40497a9020c6..a87224767ff3 100644
--- a/arch/x86/include/asm/pgtable_types.h
+++ b/arch/x86/include/asm/pgtable_types.h
@@ -199,10 +199,6 @@ enum page_cache_mode {
#define __PAGE_KERNEL_WP (__PP|__RW| 0|___A|__NX|___D| 0|___G| __WP)
-#define __PAGE_KERNEL_IO __PAGE_KERNEL
-#define __PAGE_KERNEL_IO_NOCACHE __PAGE_KERNEL_NOCACHE
-
-
#ifndef __ASSEMBLY__
#define __PAGE_KERNEL_ENC (__PAGE_KERNEL| _ENC)
@@ -223,9 +219,6 @@ enum page_cache_mode {
#define PAGE_KERNEL_LARGE_EXEC __pgprot_mask(__PAGE_KERNEL_LARGE_EXEC | _ENC)
#define PAGE_KERNEL_VVAR__pgprot_mask(__PAGE_KERNEL_VVAR | _ENC)
-#define PAGE_KERNEL_IO __pgprot_mask(__PAGE_KERNEL_IO)
-#define PAGE_KERNEL_IO_NOCACHE __pgprot_mask(__PAGE_KERNEL_IO_NOCACHE)
-
#endif /* __ASSEMBLY__ */
/* xwr */
diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index 026031b3b782..3102dda4b152 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -243,7 +243,7 @@ __ioremap_caller(resource_size_t phys_addr, unsigned long
size,
* make sure the memory encryption attribute is enabled in the
* resulting mapping.
*/
- prot = PAGE_KERNEL_IO;
+ prot = PAGE_KERNEL;
if ((io_desc.flags & IORES_MAP_ENCRYPTED) || encrypted)
prot = pgprot_encrypted(prot);
diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
index 8bfc10330107..5dc0771a50f3 100644
--- a/arch/x86/xen/setup.c
+++ b/arch/x86/xen/setup.c
@@ -435,7 +435,7 @@ static unsigned long __init
xen_set_identity_and_remap_chunk(
for (pfn = start_pfn; pfn <= max_pfn_mapped && pfn < end_pfn; pfn++)
(void)HYPERVISOR_update_va_mapping(
(unsigned long)__va(pfn << PAGE_SHIFT),
- mfn_pte(pfn, PAGE_KERNEL_IO), 0);
+ mfn_pte(pfn, PAGE_KERNEL), 0);
return remap_pfn;
}
diff --git a/include/asm-generic/fixmap.h b/include/asm-generic/fixmap.h
index 8cc7b09c1bc7..f1b0c6f3d0be 100644
--- a/include/asm-generic/fixmap.h
+++ b/include/asm-generic/fixmap.h
@@ -54,7 +54,7 @@ static inline unsigned long virt_to_fix(const unsigned long
vaddr)
#define FIXMAP_PAGE_NOCACHE PAGE_KERNEL_NOCACHE
#endif
#ifndef FIXMAP_PAGE_IO
-#define FIXMAP_PAGE_IO PAGE_KERNEL_IO
+#define FIXMAP_PAGE_IO PAGE_KERNEL
#endif
#ifndef FIXMAP_PAGE_CLEAR
#define FIXMAP_PAGE_CLEAR __pgprot(0)
--
2.33.1
Re: [PATCH] drm/msm/devfreq: Fix OPP refcnt leak
Hi,
On Thu, Nov 4, 2021 at 9:32 PM Steev Klimaszewski wrote:
>
>
> On 11/4/21 5:28 PM, Rob Clark wrote:
> > From: Rob Clark
> >
> > Reported-by: Douglas Anderson
> > Fixes: 9bc95570175a ("drm/msm: Devfreq tuning")
> > Signed-off-by: Rob Clark
> > ---
> > drivers/gpu/drm/msm/msm_gpu_devfreq.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> > b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> > index d32b729b4616..9bf8600b6eea 100644
> > --- a/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> > +++ b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> > @@ -20,8 +20,6 @@ static int msm_devfreq_target(struct device *dev,
> > unsigned long *freq,
> > struct msm_gpu *gpu = dev_to_gpu(dev);
> > struct dev_pm_opp *opp;
> >
> > - opp = devfreq_recommended_opp(dev, freq, flags);
> > -
> > /*
> >* If the GPU is idle, devfreq is not aware, so just ignore
> >* it's requests
> > @@ -31,6 +29,8 @@ static int msm_devfreq_target(struct device *dev,
> > unsigned long *freq,
> > return 0;
> > }
> >
> > + opp = devfreq_recommended_opp(dev, freq, flags);
> > +
> > if (IS_ERR(opp))
> > return PTR_ERR(opp);
> >
>
> Testing this here on the Lenovo Yoga C630, and I'm starting to see in my
> dmesg output
>
> [ 36.337061] devfreq 500.gpu: Couldn't update frequency transition
> information.
> [ 36.388122] devfreq 500.gpu: Couldn't update frequency transition
> information.
Ah, I think this makes sense. We're now storing a frequency which
might not match an actual "opp" and I suppose that we must return it
in some cases.
I guess a simple fix is to still call devfreq_recommended_opp() in the
idle case but just call dev_pm_opp_put() to fix the leak.
-Doug
Re: [PATCH] drm/fb-helper: Fix restore_fbdev when there are pending delayed hotplug
On 2021-11-04 17:32, Jocelyn Falempe wrote:
> When using Xorg/Logind and an external monitor connected with an MST dock.
> After disconnecting the external monitor, switching to VT may not work,
> the (internal) monitor sill display Xorg, and you can't see what you are
> typing in the VT.
>
> This is related to commit e2809c7db818df6bbd0edf843e1beb2fbc9d8541 which
> introduced delayed hotplug for MST, and commit
> dc5bdb68b5b369d5bc7d1de96fa64cc1737a6320 which introduced a workaround for
> Xorg and logind, and add a force parameter to
> __drm_fb_helper_restore_fbdev_mode_unlocked() in this case.
>
> When switching to VT, with Xorg and logind, if there
> are pending hotplug event (like MST unplugged), the hotplug path
> may not be fulfilled, because logind may drop the master a bit later.
> It leads to the console not showing up on the monitor.
>
> So in this case, forward the "force" parameter to the hotplug event,
> and ignore if there is a drm master in this case.
I'm worried that this leaves a race condition which allows the still-master
(which causes drm_fb_helper_hotplug_event to bail without this patch) to modify
the state set by __drm_fb_helper_hotplug_event, which could still result in
similar symptoms.
I wonder if something like calling drm_fb_helper_hotplug_event when master is
dropped and fb_helper->delayed_hotplug == true could work instead.
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 8e7a124d6c5a..c07080f661b1 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -82,6 +82,8 @@ MODULE_PARM_DESC(drm_leak_fbdev_smem,
> static LIST_HEAD(kernel_fb_helper_list);
> static DEFINE_MUTEX(kernel_fb_helper_lock);
>
> +static int __drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper,
> bool force);
> +
> /**
> * DOC: fbdev helpers
> *
> @@ -258,7 +260,7 @@ __drm_fb_helper_restore_fbdev_mode_unlocked(struct
> drm_fb_helper *fb_helper,
> mutex_unlock(&fb_helper->lock);
>
> if (do_delayed)
> - drm_fb_helper_hotplug_event(fb_helper);
> + __drm_fb_helper_hotplug_event(fb_helper, force);
>
> return ret;
> }
> @@ -1930,6 +1932,38 @@ int drm_fb_helper_initial_config(struct drm_fb_helper
> *fb_helper, int bpp_sel)
> }
> EXPORT_SYMBOL(drm_fb_helper_initial_config);
>
> +static int __drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper,
> bool force)
> +{
> + int err = 0;
> +
> + if (!drm_fbdev_emulation || !fb_helper)
> + return 0;
> +
> + mutex_lock(&fb_helper->lock);
> + if (fb_helper->deferred_setup) {
> + err = __drm_fb_helper_initial_config_and_unlock(fb_helper,
> + fb_helper->preferred_bpp);
> + return err;
> + }
> + if (!force) {
> + if (!fb_helper->fb ||
> !drm_master_internal_acquire(fb_helper->dev)) {
> + fb_helper->delayed_hotplug = true;
> + mutex_unlock(&fb_helper->lock);
> + return err;
> + }
> + drm_master_internal_release(fb_helper->dev);
> + }
> + drm_dbg_kms(fb_helper->dev, "\n");
> +
> + drm_client_modeset_probe(&fb_helper->client, fb_helper->fb->width,
> fb_helper->fb->height);
> + drm_setup_crtcs_fb(fb_helper);
> + mutex_unlock(&fb_helper->lock);
> +
> + drm_fb_helper_set_par(fb_helper->fbdev);
> +
> + return 0;
> +}
> +
> /**
> * drm_fb_helper_hotplug_event - respond to a hotplug notification by
> * probing all the outputs attached to the fb
> @@ -1953,35 +1987,7 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config);
> */
> int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
> {
> - int err = 0;
> -
> - if (!drm_fbdev_emulation || !fb_helper)
> - return 0;
> -
> - mutex_lock(&fb_helper->lock);
> - if (fb_helper->deferred_setup) {
> - err = __drm_fb_helper_initial_config_and_unlock(fb_helper,
> - fb_helper->preferred_bpp);
> - return err;
> - }
> -
> - if (!fb_helper->fb || !drm_master_internal_acquire(fb_helper->dev)) {
> - fb_helper->delayed_hotplug = true;
> - mutex_unlock(&fb_helper->lock);
> - return err;
> - }
> -
> - drm_master_internal_release(fb_helper->dev);
> -
> - drm_dbg_kms(fb_helper->dev, "\n");
> -
> - drm_client_modeset_probe(&fb_helper->client, fb_helper->fb->width,
> fb_helper->fb->height);
> - drm_setup_crtcs_fb(fb_helper);
> - mutex_unlock(&fb_helper->lock);
> -
> - drm_fb_helper_set_par(fb_helper->fbdev);
> -
> - return 0;
> + return __drm_fb_helper_hotplug_event(fb_helper, false);
> }
> EXPORT_SYMBOL(drm_fb_helper_hotplug_event);
>
>
--
Earthling Michel Dänzer| https://redhat.com
Libre software enthusiast | Mesa and Xwayland developer
[Bug 214921] amdgpu hangs HP Laptop on shutdown
https://bugzilla.kernel.org/show_bug.cgi?id=214921 Paul Gover (pmw.go...@yahoo.co.uk) changed: What|Removed |Added CC||pmw.go...@yahoo.co.uk --- Comment #2 from Paul Gover (pmw.go...@yahoo.co.uk) --- Spasswolf's comment doesn't seem in any way related to this bug report. I presume it was filed against the wrong bug! I can confirm on my setup, HP laptop with an AMD "Stoney" chipset, on kernel 5.15.0 the system doesn't shutdown when you use KDE power menu "shutdown", nor if you issue the "halt" or "poweroff" commands, nor "shutdown -P now". The keyboard is dead, so Ctl-Alt-Del doesn't reboot, all that's left is holding the power button down. Behaviour was correct on kernel 5.14 (at least, up to 5.14.14). -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
Re: [PATCH] drm/msm/dp: do not initialize phy until plugin interrupt received
On Fri 05 Nov 10:28 PDT 2021, Kuogee Hsieh wrote:
> From: Kuogee Hsieh
>
> Combo phy supports both USB and DP simultaneously. There may has a
> possible conflict during phy initialization phase between USB and
> DP driver which may cause USB phy timeout when USB tries to power
> up its phy. This patch has the DP driver not initialize its phy
> during DP driver initialization phase. Instead DP driver only enable
> required regulators and clocks so that it is able to receive HPD
> interrupts after completion of initialization phase. DP driver will
> initialize its phy when HPD plug-in interrupt received.
Is this a hardware requirement, or is this a issue with the current
implementation of the QMP combo phy driver? We should not hack up the DP
driver to circumvent the latter.
Also, I don't suppose there's anything here that prevents the HPD to
come before the USB PHY is powered up? Even though that seems less
likely in practice...
> This patch also provides a positive side effects which balance regulator
> enable count since regulator only enabled at initialize phase and resume
> and disabled at followed suspend.
>
Is this something that needs to be fixed separately, so that it can be
backported to stable kernels?
Regards,
Bjorn
> Signed-off-by: Kuogee Hsieh
> ---
> drivers/gpu/drm/msm/dp/dp_ctrl.c| 97
> ++---
> drivers/gpu/drm/msm/dp/dp_ctrl.h| 9 ++--
> drivers/gpu/drm/msm/dp/dp_display.c | 71 ---
> 3 files changed, 119 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 7ec155d..e0e5dc9 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -1364,7 +1364,41 @@ static int dp_ctrl_enable_stream_clocks(struct
> dp_ctrl_private *ctrl)
> return ret;
> }
>
> -int dp_ctrl_host_init(struct dp_ctrl *dp_ctrl, bool flip, bool reset)
> +void dp_ctrl_irq_enable(struct dp_ctrl *dp_ctrl, bool flip)
> +{
> + struct dp_ctrl_private *ctrl;
> +
> + if (!dp_ctrl) {
> + DRM_ERROR("Invalid input data\n");
> + return;
> + }
> +
> + ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
> +
> + ctrl->dp_ctrl.orientation = flip;
> +
> + dp_catalog_ctrl_reset(ctrl->catalog);
> +
> + dp_catalog_ctrl_enable_irq(ctrl->catalog, true);
> +}
> +
> +void dp_ctrl_irq_disable(struct dp_ctrl *dp_ctrl)
> +{
> + struct dp_ctrl_private *ctrl;
> +
> + if (!dp_ctrl) {
> + DRM_ERROR("Invalid input data\n");
> + return;
> + }
> +
> + ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
> +
> + dp_catalog_ctrl_reset(ctrl->catalog);
> +
> + dp_catalog_ctrl_enable_irq(ctrl->catalog, false);
> +}
> +
> +void dp_ctrl_phy_init(struct dp_ctrl *dp_ctrl)
> {
> struct dp_ctrl_private *ctrl;
> struct dp_io *dp_io;
> @@ -1372,34 +1406,24 @@ int dp_ctrl_host_init(struct dp_ctrl *dp_ctrl, bool
> flip, bool reset)
>
> if (!dp_ctrl) {
> DRM_ERROR("Invalid input data\n");
> - return -EINVAL;
> + return;
> }
>
> ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
> dp_io = &ctrl->parser->io;
> phy = dp_io->phy;
>
> - ctrl->dp_ctrl.orientation = flip;
> -
> - if (reset)
> - dp_catalog_ctrl_reset(ctrl->catalog);
> + DRM_DEBUG_DP("Before, phy=%x init_count=%d power_on=%d\n",
> + (u32)(uintptr_t)phy, phy->init_count, phy->power_count);
>
> - DRM_DEBUG_DP("flip=%d\n", flip);
> dp_catalog_ctrl_phy_reset(ctrl->catalog);
> phy_init(phy);
> - dp_catalog_ctrl_enable_irq(ctrl->catalog, true);
>
> - return 0;
> + DRM_DEBUG_DP("After, phy=%x init_count=%d power_on=%d\n",
> + (u32)(uintptr_t)phy, phy->init_count, phy->power_count);
> }
>
> -/**
> - * dp_ctrl_host_deinit() - Uninitialize DP controller
> - * @dp_ctrl: Display Port Driver data
> - *
> - * Perform required steps to uninitialize DP controller
> - * and its resources.
> - */
> -void dp_ctrl_host_deinit(struct dp_ctrl *dp_ctrl)
> +void dp_ctrl_phy_exit(struct dp_ctrl *dp_ctrl)
> {
> struct dp_ctrl_private *ctrl;
> struct dp_io *dp_io;
> @@ -1414,10 +1438,14 @@ void dp_ctrl_host_deinit(struct dp_ctrl *dp_ctrl)
> dp_io = &ctrl->parser->io;
> phy = dp_io->phy;
>
> - dp_catalog_ctrl_enable_irq(ctrl->catalog, false);
> + DRM_DEBUG_DP("Before, phy=%x init_count=%d power_on=%d\n",
> + (u32)(uintptr_t)phy, phy->init_count, phy->power_count);
> +
> + dp_catalog_ctrl_phy_reset(ctrl->catalog);
> phy_exit(phy);
>
> - DRM_DEBUG_DP("Host deinitialized successfully\n");
> + DRM_DEBUG_DP("After, phy=%x init_count=%d power_on=%d\n",
> + (u32)(uintptr_t)phy, phy->init_count, phy->power_count);
> }
>
> static bool dp_ctrl_use_fixed_nvid(struct dp_ctrl_private *c
Re: [PATCH] drm/fb-helper: Fix restore_fbdev when there are pending delayed hotplug
On 05/11/2021 16:50, Michel Dänzer wrote:
On 2021-11-04 17:32, Jocelyn Falempe wrote:
When using Xorg/Logind and an external monitor connected with an MST dock.
After disconnecting the external monitor, switching to VT may not work,
the (internal) monitor sill display Xorg, and you can't see what you are
typing in the VT.
This is related to commit e2809c7db818df6bbd0edf843e1beb2fbc9d8541 which
introduced delayed hotplug for MST, and commit
dc5bdb68b5b369d5bc7d1de96fa64cc1737a6320 which introduced a workaround for
Xorg and logind, and add a force parameter to
__drm_fb_helper_restore_fbdev_mode_unlocked() in this case.
When switching to VT, with Xorg and logind, if there
are pending hotplug event (like MST unplugged), the hotplug path
may not be fulfilled, because logind may drop the master a bit later.
It leads to the console not showing up on the monitor.
So in this case, forward the "force" parameter to the hotplug event,
and ignore if there is a drm master in this case.
I'm worried that this leaves a race condition which allows the still-master
(which causes drm_fb_helper_hotplug_event to bail without this patch) to modify
the state set by __drm_fb_helper_hotplug_event, which could still result in
similar symptoms.
I wonder if something like calling drm_fb_helper_hotplug_event when master is
dropped and fb_helper->delayed_hotplug == true could work instead.
Ok, I will try to make a new patch and call
drm_fb_helper_hotplug_event() from drm_drop_master() instead.
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 8e7a124d6c5a..c07080f661b1 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -82,6 +82,8 @@ MODULE_PARM_DESC(drm_leak_fbdev_smem,
static LIST_HEAD(kernel_fb_helper_list);
static DEFINE_MUTEX(kernel_fb_helper_lock);
+static int __drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper, bool force);
+
/**
* DOC: fbdev helpers
*
@@ -258,7 +260,7 @@ __drm_fb_helper_restore_fbdev_mode_unlocked(struct
drm_fb_helper *fb_helper,
mutex_unlock(&fb_helper->lock);
if (do_delayed)
- drm_fb_helper_hotplug_event(fb_helper);
+ __drm_fb_helper_hotplug_event(fb_helper, force);
return ret;
}
@@ -1930,6 +1932,38 @@ int drm_fb_helper_initial_config(struct drm_fb_helper
*fb_helper, int bpp_sel)
}
EXPORT_SYMBOL(drm_fb_helper_initial_config);
+static int __drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper, bool force)
+{
+ int err = 0;
+
+ if (!drm_fbdev_emulation || !fb_helper)
+ return 0;
+
+ mutex_lock(&fb_helper->lock);
+ if (fb_helper->deferred_setup) {
+ err = __drm_fb_helper_initial_config_and_unlock(fb_helper,
+ fb_helper->preferred_bpp);
+ return err;
+ }
+ if (!force) {
+ if (!fb_helper->fb ||
!drm_master_internal_acquire(fb_helper->dev)) {
+ fb_helper->delayed_hotplug = true;
+ mutex_unlock(&fb_helper->lock);
+ return err;
+ }
+ drm_master_internal_release(fb_helper->dev);
+ }
+ drm_dbg_kms(fb_helper->dev, "\n");
+
+ drm_client_modeset_probe(&fb_helper->client, fb_helper->fb->width,
fb_helper->fb->height);
+ drm_setup_crtcs_fb(fb_helper);
+ mutex_unlock(&fb_helper->lock);
+
+ drm_fb_helper_set_par(fb_helper->fbdev);
+
+ return 0;
+}
+
/**
* drm_fb_helper_hotplug_event - respond to a hotplug notification by
* probing all the outputs attached to the fb
@@ -1953,35 +1987,7 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config);
*/
int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
{
- int err = 0;
-
- if (!drm_fbdev_emulation || !fb_helper)
- return 0;
-
- mutex_lock(&fb_helper->lock);
- if (fb_helper->deferred_setup) {
- err = __drm_fb_helper_initial_config_and_unlock(fb_helper,
- fb_helper->preferred_bpp);
- return err;
- }
-
- if (!fb_helper->fb || !drm_master_internal_acquire(fb_helper->dev)) {
- fb_helper->delayed_hotplug = true;
- mutex_unlock(&fb_helper->lock);
- return err;
- }
-
- drm_master_internal_release(fb_helper->dev);
-
- drm_dbg_kms(fb_helper->dev, "\n");
-
- drm_client_modeset_probe(&fb_helper->client, fb_helper->fb->width,
fb_helper->fb->height);
- drm_setup_crtcs_fb(fb_helper);
- mutex_unlock(&fb_helper->lock);
-
- drm_fb_helper_set_par(fb_helper->fbdev);
-
- return 0;
+ return __drm_fb_helper_hotplug_event(fb_helper, false);
}
EXPORT_SYMBOL(drm_fb_helper_hotplug_event);
Re: [PATCH] drm: fb_helper: improve CONFIG_FB dependency
On Fri, Oct 29, 2021 at 02:02:38PM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> My previous patch correctly addressed the possible link failure, but as
> Jani points out, the dependency is now stricter than it needs to be.
>
> Change it again, to allow DRM_FBDEV_EMULATION to be used when
> DRM_KMS_HELPER and FB are both loadable modules and DRM is linked into
> the kernel.
>
> As a side-effect, the option is now only visible when at least one DRM
> driver makes use of DRM_KMS_HELPER. This is better, because the option
> has no effect otherwise.
>
> Fixes: 606b102876e3 ("drm: fb_helper: fix CONFIG_FB dependency")
> Suggested-by: Acked-by: Jani Nikula
> Reviewed-by: Javier Martinez Canillas
> Signed-off-by: Arnd Bergmann
Queued up for the merge window, thanks for the patch.
-Daniel
> ---
> drivers/gpu/drm/Kconfig | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index c08860db2520..d2e6d8ce5000 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -117,9 +117,8 @@ config DRM_DEBUG_MODESET_LOCK
>
> config DRM_FBDEV_EMULATION
> bool "Enable legacy fbdev support for your modesetting driver"
> - depends on DRM
> - depends on FB=y || FB=DRM
> - select DRM_KMS_HELPER
> + depends on DRM_KMS_HELPER
> + depends on FB=y || FB=DRM_KMS_HELPER
> select FB_CFB_FILLRECT
> select FB_CFB_COPYAREA
> select FB_CFB_IMAGEBLIT
> --
> 2.29.2
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Re: [PATCH 2/2] drm: Remove CONFIG_DRM_KMS_CMA_HELPER option
On Mon, Nov 01, 2021 at 11:59:15PM +0800, kernel test robot wrote: > Hi Thomas, > > I love your patch! Yet something to improve: > > [auto build test ERROR on next-20211029] > [cannot apply to drm/drm-next shawnguo/for-next pinchartl-media/drm/du/next > v5.15 v5.15-rc7 v5.15-rc6 v5.15] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch] > > url: > https://github.com/0day-ci/linux/commits/Thomas-Zimmermann/drm-Small-CMA-cleanups/20211101-161911 > base:bdcc9f6a568275aed4cc32fd2312432d2ff1b704 > config: x86_64-randconfig-a004-20211101 (attached as .config) > compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project > 82ed106567063ea269c6d5669278b733e173a42f) > reproduce (this is a W=1 build): > wget > https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O > ~/bin/make.cross > chmod +x ~/bin/make.cross > # > https://github.com/0day-ci/linux/commit/c3c7ec5f9ccd90e78f0f2d3143505db4060bbf17 > git remote add linux-review https://github.com/0day-ci/linux > git fetch --no-tags linux-review > Thomas-Zimmermann/drm-Small-CMA-cleanups/20211101-161911 > git checkout c3c7ec5f9ccd90e78f0f2d3143505db4060bbf17 > # save the attached .config to linux build tree > mkdir build_dir > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 > O=build_dir ARCH=x86_64 SHELL=/bin/bash > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot > > All errors (new ones prefixed by >>, old ones prefixed by <<): > > >> ERROR: modpost: module drm_cma_helper uses symbol dma_buf_vmap from > >> namespace DMA_BUF, but does not import it. > >> ERROR: modpost: module drm_cma_helper uses symbol dma_buf_vunmap from > >> namespace DMA_BUF, but does not import it. I guess this is simply because kbuild tests on top of linux-next, where the namespacing is a bit funny. We might need a fixup when we backmerge. Either way this looks like a good simplification to me, on the series: Reviewed-by: Daniel Vetter > > --- > 0-DAY CI Kernel Test Service, Intel Corporation > https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH 02/13] drm/connector: Add helper to check if a mode requires scrambling
On Thu, Nov 04, 2021 at 05:37:21PM +0200, Ville Syrjälä wrote:
> On Tue, Nov 02, 2021 at 03:59:33PM +0100, Maxime Ripard wrote:
> > --- a/include/drm/drm_modes.h
> > +++ b/include/drm/drm_modes.h
> > @@ -424,6 +424,21 @@ static inline bool drm_mode_is_stereo(const struct
> > drm_display_mode *mode)
> > return mode->flags & DRM_MODE_FLAG_3D_MASK;
> > }
> >
> > +/**
> > + * drm_mode_hdmi_requires_scrambling - Checks if a mode requires HDMI
> > Scrambling
> > + * @mode: DRM display mode
> > + *
> > + * Checks if a given display mode requires the scrambling to be enabled.
> > + *
> > + * Returns:
> > + * A boolean stating whether it's required or not.
> > + */
> > +static inline bool
> > +drm_mode_hdmi_requires_scrambling(const struct drm_display_mode *mode)
> > +{
> > + return mode->clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ;
> > +}
>
> That's only correct for 8bpc. The actual limit is on the TMDS clock (or
> rather TMDS character rate as HDMI 2.0 calls it due to the 1/1 vs. 1/4
> magic for the actually transmitted TMDS clock).
Yeah we might need to add the bus format for the cable here too, to make
this complete.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Re: [PATCH] drm/virtio: Fix NULL dereference error in virtio_gpu_poll
On 04 November 2021 at 10:42 pm, Vivek Kasireddy wrote: > When virgl is not enabled, vfpriv pointer would not be allocated. > Therefore, check for a valid value before dereferencing. > > Reported-by: Christian Zigotzky > Cc: Gurchetan Singh > Cc: Gerd Hoffmann > Signed-off-by: Vivek Kasireddy > --- > drivers/gpu/drm/virtio/virtgpu_drv.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c > index 749db18dcfa2..d86e1ad4a972 100644 > --- a/drivers/gpu/drm/virtio/virtgpu_drv.c > +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c > @@ -163,10 +163,11 @@ static __poll_t virtio_gpu_poll(struct file *filp, > struct drm_file *drm_file = filp->private_data; > struct virtio_gpu_fpriv *vfpriv = drm_file->driver_priv; > struct drm_device *dev = drm_file->minor->dev; > + struct virtio_gpu_device *vgdev = dev->dev_private; > struct drm_pending_event *e = NULL; > __poll_t mask = 0; > > - if (!vfpriv->ring_idx_mask) > + if (!vgdev->has_virgl_3d || !vfpriv || !vfpriv->ring_idx_mask) > return drm_poll(filp, wait); > > poll_wait(filp, &drm_file->event_wait, wait); Tested-by: Christian Zigotzky [1] [1] https://i.ibb.co/N1vL5Kd/Kernel-5-16-alpha3-Power-PC.png
Re: [PATCH] Revert "drm/imx: Annotate dma-fence critical section in commit path"
On Wed, Nov 03, 2021 at 09:11:12PM -0300, Fabio Estevam wrote:
> This reverts commit f4b34faa08428d813fc3629f882c503487f94a12.
>
> Since commit f4b34faa0842 ("drm/imx: Annotate dma-fence critical section in
> commit path") the following possible circular dependency is detected:
>
> [5.001811] ==
> [5.001817] WARNING: possible circular locking dependency detected
> [5.001824] 5.14.9-01225-g45da36cc6fcc-dirty #1 Tainted: GW
> [5.001833] --
> [5.001838] kworker/u8:0/7 is trying to acquire lock:
> [5.001848] c1752080 (regulator_list_mutex){+.+.}-{3:3}, at:
> regulator_lock_dependent+0x40/0x294
> [5.001903]
> [5.001903] but task is already holding lock:
> [5.001909] c176df78 (dma_fence_map){}-{0:0}, at:
> imx_drm_atomic_commit_tail+0x10/0x160
> [5.001957]
> [5.001957] which lock already depends on the new lock.
> ...
>
> Revert it for now.
>
> Tested on a imx6q-sabresd.
>
> Fixes: f4b34faa0842 ("drm/imx: Annotate dma-fence critical section in commit
> path")
> Signed-off-by: Fabio Estevam
Yeah I have these on my todo list since a while, I need to properly
document the reasons why this doesn't work.
Queued up for the merge window, thanks for your patch.
-Daniel
> ---
> drivers/gpu/drm/imx/imx-drm-core.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/imx/imx-drm-core.c
> b/drivers/gpu/drm/imx/imx-drm-core.c
> index 9558e9e1b431..cb685fe2039b 100644
> --- a/drivers/gpu/drm/imx/imx-drm-core.c
> +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> @@ -81,7 +81,6 @@ static void imx_drm_atomic_commit_tail(struct
> drm_atomic_state *state)
> struct drm_plane_state *old_plane_state, *new_plane_state;
> bool plane_disabling = false;
> int i;
> - bool fence_cookie = dma_fence_begin_signalling();
>
> drm_atomic_helper_commit_modeset_disables(dev, state);
>
> @@ -112,7 +111,6 @@ static void imx_drm_atomic_commit_tail(struct
> drm_atomic_state *state)
> }
>
> drm_atomic_helper_commit_hw_done(state);
> - dma_fence_end_signalling(fence_cookie);
> }
>
> static const struct drm_mode_config_helper_funcs imx_drm_mode_config_helpers
> = {
> --
> 2.25.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Re: Questions about KMS flip
On Thu, Nov 04, 2021 at 12:44:34PM -0400, Harry Wentland wrote: > +Nick > > It looks to be the old drm_plane_state->fb holds that reference. See > dm_plane_helper_cleanup_fb() in amdgpu_dm.c. Yup plane state holds reference for its entire existing (well this holds in general as design principle for atomic state structs, just makes life easier). And the plane state is guaranteed to exist from when we first pin (prepare_fb plane hook) to when it's getting unpinned (cleanup_fb plane hook). Out of curiosity, what's blowing up? -Daniel > > Harry > > On 2021-11-04 08:51, Christian König wrote: > > Hi guys, > > > > adding the usual suspects which might know that of hand: When we do a KMS > > page flip, who keeps the reference to the BO while it is scanned out? > > > > We are running into warning backtraces from TTM which look more than odd. > > > > Thanks, > > Christian. > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH 02/13] drm/connector: Add helper to check if a mode requires scrambling
On Fri, Nov 05, 2021 at 07:02:30PM +0100, Daniel Vetter wrote:
> On Thu, Nov 04, 2021 at 05:37:21PM +0200, Ville Syrjälä wrote:
> > On Tue, Nov 02, 2021 at 03:59:33PM +0100, Maxime Ripard wrote:
> > > --- a/include/drm/drm_modes.h
> > > +++ b/include/drm/drm_modes.h
> > > @@ -424,6 +424,21 @@ static inline bool drm_mode_is_stereo(const struct
> > > drm_display_mode *mode)
> > > return mode->flags & DRM_MODE_FLAG_3D_MASK;
> > > }
> > >
> > > +/**
> > > + * drm_mode_hdmi_requires_scrambling - Checks if a mode requires HDMI
> > > Scrambling
> > > + * @mode: DRM display mode
> > > + *
> > > + * Checks if a given display mode requires the scrambling to be enabled.
> > > + *
> > > + * Returns:
> > > + * A boolean stating whether it's required or not.
> > > + */
> > > +static inline bool
> > > +drm_mode_hdmi_requires_scrambling(const struct drm_display_mode *mode)
> > > +{
> > > + return mode->clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ;
> > > +}
> >
> > That's only correct for 8bpc. The actual limit is on the TMDS clock (or
> > rather TMDS character rate as HDMI 2.0 calls it due to the 1/1 vs. 1/4
> > magic for the actually transmitted TMDS clock).
>
> Yeah we might need to add the bus format for the cable here too, to make
> this complete.
I don't think we have a usable thing for that on the drm level, so
would need to invent something. Oh, and the mode->clock vs.
mode->crtc_clock funny business also limits the usability of this
approach. So probably just easiest to pass in the the driver calculated
TMDS clock instead.
--
Ville Syrjälä
Intel
Re: [PATCH 2/3] drm/shmem-helper: Export dedicated wrappers for GEM object functions
On Fri, Nov 05, 2021 at 10:35:57AM +0100, Thomas Zimmermann wrote:
> Wrap GEM SHMEM functions for struct drm_gem_object_funcs and update
> all callers. This will allow for an update of the public interfaces
> of the GEM SHMEM helper library.
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/gpu/drm/drm_gem_shmem_helper.c | 45 -
> drivers/gpu/drm/lima/lima_gem.c | 8 +-
> drivers/gpu/drm/panfrost/panfrost_gem.c | 12 +--
> drivers/gpu/drm/v3d/v3d_bo.c| 14 +--
> drivers/gpu/drm/virtio/virtgpu_object.c | 15 ++-
> include/drm/drm_gem_shmem_helper.h | 120
> 6 files changed, 161 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
> b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index cd93e91b3487..72ac263f20be 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -30,14 +30,14 @@
Maybe add a few lines to the intro DOC: section about which functions
should be used where? Just so drivers don't make a mess out of this again
now that you cleaned it up.
It's ofc not going to be perfect, but better than nothing.
With that, on the series:
Acked-by: Daniel Vetter
But maybe wait for some more acks/reviews from driver folks.
-Daniel
> */
>
> static const struct drm_gem_object_funcs drm_gem_shmem_funcs = {
> - .free = drm_gem_shmem_free_object,
> - .print_info = drm_gem_shmem_print_info,
> - .pin = drm_gem_shmem_pin,
> - .unpin = drm_gem_shmem_unpin,
> - .get_sg_table = drm_gem_shmem_get_sg_table,
> - .vmap = drm_gem_shmem_vmap,
> - .vunmap = drm_gem_shmem_vunmap,
> - .mmap = drm_gem_shmem_mmap,
> + .free = drm_gem_shmem_object_free,
> + .print_info = drm_gem_shmem_object_print_info,
> + .pin = drm_gem_shmem_object_pin,
> + .unpin = drm_gem_shmem_object_unpin,
> + .get_sg_table = drm_gem_shmem_object_get_sg_table,
> + .vmap = drm_gem_shmem_object_vmap,
> + .vunmap = drm_gem_shmem_object_vunmap,
> + .mmap = drm_gem_shmem_object_mmap,
> };
>
> static struct drm_gem_shmem_object *
> @@ -121,8 +121,7 @@ EXPORT_SYMBOL_GPL(drm_gem_shmem_create);
> * @obj: GEM object to free
> *
> * This function cleans up the GEM object state and frees the memory used to
> - * store the object itself. It should be used to implement
> - * &drm_gem_object_funcs.free.
> + * store the object itself.
> */
> void drm_gem_shmem_free_object(struct drm_gem_object *obj)
> {
> @@ -248,8 +247,7 @@ EXPORT_SYMBOL(drm_gem_shmem_put_pages);
> * @obj: GEM object
> *
> * This function makes sure the backing pages are pinned in memory while the
> - * buffer is exported. It should only be used to implement
> - * &drm_gem_object_funcs.pin.
> + * buffer is exported.
> *
> * Returns:
> * 0 on success or a negative error code on failure.
> @@ -269,7 +267,7 @@ EXPORT_SYMBOL(drm_gem_shmem_pin);
> * @obj: GEM object
> *
> * This function removes the requirement that the backing pages are pinned in
> - * memory. It should only be used to implement &drm_gem_object_funcs.unpin.
> + * memory.
> */
> void drm_gem_shmem_unpin(struct drm_gem_object *obj)
> {
> @@ -340,11 +338,8 @@ static int drm_gem_shmem_vmap_locked(struct
> drm_gem_shmem_object *shmem, struct
> * store.
> *
> * This function makes sure that a contiguous kernel virtual address mapping
> - * exists for the buffer backing the shmem GEM object.
> - *
> - * This function can be used to implement &drm_gem_object_funcs.vmap. But it
> can
> - * also be called by drivers directly, in which case it will hide the
> - * differences between dma-buf imported and natively allocated objects.
> + * exists for the buffer backing the shmem GEM object. It hides the
> differences
> + * between dma-buf imported and natively allocated objects.
> *
> * Acquired mappings should be cleaned up by calling drm_gem_shmem_vunmap().
> *
> @@ -396,9 +391,8 @@ static void drm_gem_shmem_vunmap_locked(struct
> drm_gem_shmem_object *shmem,
> * drm_gem_shmem_vmap(). The mapping is only removed when the use count
> drops to
> * zero.
> *
> - * This function can be used to implement &drm_gem_object_funcs.vmap. But it
> can
> - * also be called by drivers directly, in which case it will hide the
> - * differences between dma-buf imported and natively allocated objects.
> + * This function hides the differences between dma-buf imported and natively
> + * allocated objects.
> */
> void drm_gem_shmem_vunmap(struct drm_gem_object *obj, struct dma_buf_map
> *map)
> {
> @@ -604,8 +598,7 @@ static const struct vm_operations_struct
> drm_gem_shmem_vm_ops = {
> * @vma: VMA for the area to be mapped
> *
> * This function implements an augmented version of the GEM DRM file mmap
> - * operation for shmem objects. Drivers which employ the shmem helpers should
> - * use this function as their &drm_gem_object_funcs.mmap handler.
> + * operation for shmem
[PATCH v5 0/5] drm/dp, drm/i915: Finish basic PWM support for VESA backlight helpers
When I originally moved all of the VESA backlight code in i915 into DRM helpers, one of the things I didn't have the hardware or time for testing was machines that used a combination of PWM and DPCD in order to control their backlights. This has since then caused some breakages and resulted in us disabling DPCD backlight support on such machines. This works fine, unless you have a machine that actually needs this functionality for backlight controls to work at all. Additionally, we will need to support PWM for when we start adding support for VESA's product (as in the product of multiplication) control mode for better brightness ranges. So - let's finally finish up implementing basic support for these types of backlights to solve these problems in our DP helpers, along with implementing support for this in i915. And since digging into this issue solved the last questions we really had about probing backlights in i915 for the most part, let's update some of the comments around that as well! Lyude Paul (5): drm/i915: Add support for panels with VESA backlights with PWM enable/disable drm/nouveau/kms/nv50-: Explicitly check DPCD backlights for aux enable/brightness drm/dp: Don't read back backlight mode in drm_edp_backlight_enable() drm/dp, drm/i915: Add support for VESA backlights using PWM for brightness control drm/i915: Clarify probing order in intel_dp_aux_init_backlight_funcs() drivers/gpu/drm/drm_dp_helper.c | 108 ++ .../drm/i915/display/intel_dp_aux_backlight.c | 81 ++--- drivers/gpu/drm/nouveau/nouveau_backlight.c | 5 +- include/drm/drm_dp_helper.h | 7 +- 4 files changed, 132 insertions(+), 69 deletions(-) -- 2.31.1
[PATCH v5 1/5] drm/i915: Add support for panels with VESA backlights with PWM enable/disable
This simply adds proper support for panel backlights that can be controlled
via VESA's backlight control protocol, but which also require that we
enable and disable the backlight via PWM instead of via the DPCD interface.
We also enable this by default, in order to fix some people's backlights
that were broken by not having this enabled.
For reference, backlights that require this and use VESA's backlight
interface tend to be laptops with hybrid GPUs, but this very well may
change in the future.
v4:
* Make sure that we call intel_backlight_level_to_pwm() in
intel_dp_aux_vesa_enable_backlight() - vsyrjala
Signed-off-by: Lyude Paul
Link: https://gitlab.freedesktop.org/drm/intel/-/issues/3680
Fixes: fe7d52bccab6 ("drm/i915/dp: Don't use DPCD backlights that need PWM
enable/disable")
Reviewed-by: Ville Syrjälä
Cc: # v5.12+
---
.../drm/i915/display/intel_dp_aux_backlight.c | 27 ++-
1 file changed, 21 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
index 569d17b4d00f..f05b71c01b8e 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
@@ -293,6 +293,13 @@ intel_dp_aux_vesa_enable_backlight(const struct
intel_crtc_state *crtc_state,
struct intel_panel *panel = &connector->panel;
struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
+ if (!panel->backlight.edp.vesa.info.aux_enable) {
+ u32 pwm_level = intel_backlight_invert_pwm_level(connector,
+
panel->backlight.pwm_level_max);
+
+ panel->backlight.pwm_funcs->enable(crtc_state, conn_state,
pwm_level);
+ }
+
drm_edp_backlight_enable(&intel_dp->aux,
&panel->backlight.edp.vesa.info, level);
}
@@ -304,6 +311,10 @@ static void intel_dp_aux_vesa_disable_backlight(const
struct drm_connector_state
struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
drm_edp_backlight_disable(&intel_dp->aux,
&panel->backlight.edp.vesa.info);
+
+ if (!panel->backlight.edp.vesa.info.aux_enable)
+ panel->backlight.pwm_funcs->disable(old_conn_state,
+
intel_backlight_invert_pwm_level(connector, 0));
}
static int intel_dp_aux_vesa_setup_backlight(struct intel_connector
*connector, enum pipe pipe)
@@ -321,6 +332,15 @@ static int intel_dp_aux_vesa_setup_backlight(struct
intel_connector *connector,
if (ret < 0)
return ret;
+ if (!panel->backlight.edp.vesa.info.aux_enable) {
+ ret = panel->backlight.pwm_funcs->setup(connector, pipe);
+ if (ret < 0) {
+ drm_err(&i915->drm,
+ "Failed to setup PWM backlight controls for eDP
backlight: %d\n",
+ ret);
+ return ret;
+ }
+ }
panel->backlight.max = panel->backlight.edp.vesa.info.max;
panel->backlight.min = 0;
if (current_mode == DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD) {
@@ -340,12 +360,7 @@ intel_dp_aux_supports_vesa_backlight(struct
intel_connector *connector)
struct intel_dp *intel_dp = intel_attached_dp(connector);
struct drm_i915_private *i915 = dp_to_i915(intel_dp);
- /* TODO: We currently only support AUX only backlight configurations,
not backlights which
-* require a mix of PWM and AUX controls to work. In the mean time,
these machines typically
-* work just fine using normal PWM controls anyway.
-*/
- if ((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
- drm_edp_backlight_supported(intel_dp->edp_dpcd)) {
+ if (drm_edp_backlight_supported(intel_dp->edp_dpcd)) {
drm_dbg_kms(&i915->drm, "AUX Backlight Control Supported!\n");
return true;
}
--
2.31.1
[PATCH v5 2/5] drm/nouveau/kms/nv50-: Explicitly check DPCD backlights for aux enable/brightness
Since we don't support hybrid AUX/PWM backlights in nouveau right now,
let's add some explicit checks so that we don't break nouveau once we
enable support for these backlights in other drivers.
Reviewed-by: Karol Herbst
Signed-off-by: Lyude Paul
---
drivers/gpu/drm/nouveau/nouveau_backlight.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_backlight.c
b/drivers/gpu/drm/nouveau/nouveau_backlight.c
index 1cbd71abc80a..ae2f2abc8f5a 100644
--- a/drivers/gpu/drm/nouveau/nouveau_backlight.c
+++ b/drivers/gpu/drm/nouveau/nouveau_backlight.c
@@ -308,7 +308,10 @@ nv50_backlight_init(struct nouveau_backlight *bl,
if (ret < 0)
return ret;
- if (drm_edp_backlight_supported(edp_dpcd)) {
+ /* TODO: Add support for hybrid PWM/DPCD panels */
+ if (drm_edp_backlight_supported(edp_dpcd) &&
+ (edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
+ (edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP)) {
NV_DEBUG(drm, "DPCD backlight controls supported on
%s\n",
nv_conn->base.name);
--
2.31.1
[PATCH v5 3/5] drm/dp: Don't read back backlight mode in drm_edp_backlight_enable()
As it turns out, apparently some machines will actually leave additional
backlight functionality like dynamic backlight control on before the OS
loads. Currently we don't take care to disable unsupported features when
writing back the backlight mode, which can lead to some rather strange
looking behavior when adjusting the backlight.
So, let's fix this by just not reading back the current backlight mode on
initial enable. I don't think there should really be any downsides to this,
and this will ensure we don't leave any unsupported functionality enabled.
This should fix at least one (but not all) of the issues seen with DPCD
backlight support on fi-bdw-samus
v5:
* Just avoid reading back DPCD register - Doug Anderson
Signed-off-by: Lyude Paul
Fixes: 867cf9cd73c3 ("drm/dp: Extract i915's eDP backlight code into DRM
helpers")
---
drivers/gpu/drm/drm_dp_helper.c | 40 ++---
1 file changed, 12 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index ada0a1ff262d..af2aad2f4725 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -3363,27 +3363,13 @@ int drm_edp_backlight_enable(struct drm_dp_aux *aux,
const struct drm_edp_backli
const u16 level)
{
int ret;
- u8 dpcd_buf, new_dpcd_buf;
+ u8 dpcd_buf = DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
- ret = drm_dp_dpcd_readb(aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
&dpcd_buf);
- if (ret != 1) {
- drm_dbg_kms(aux->drm_dev,
- "%s: Failed to read backlight mode: %d\n",
aux->name, ret);
- return ret < 0 ? ret : -EIO;
- }
-
- new_dpcd_buf = dpcd_buf;
-
- if ((dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) !=
DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD) {
- new_dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
- new_dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
-
- if (bl->pwmgen_bit_count) {
- ret = drm_dp_dpcd_writeb(aux, DP_EDP_PWMGEN_BIT_COUNT,
bl->pwmgen_bit_count);
- if (ret != 1)
- drm_dbg_kms(aux->drm_dev, "%s: Failed to write
aux pwmgen bit count: %d\n",
- aux->name, ret);
- }
+ if (bl->pwmgen_bit_count) {
+ ret = drm_dp_dpcd_writeb(aux, DP_EDP_PWMGEN_BIT_COUNT,
bl->pwmgen_bit_count);
+ if (ret != 1)
+ drm_dbg_kms(aux->drm_dev, "%s: Failed to write aux
pwmgen bit count: %d\n",
+ aux->name, ret);
}
if (bl->pwm_freq_pre_divider) {
@@ -3393,16 +3379,14 @@ int drm_edp_backlight_enable(struct drm_dp_aux *aux,
const struct drm_edp_backli
"%s: Failed to write aux backlight
frequency: %d\n",
aux->name, ret);
else
- new_dpcd_buf |= DP_EDP_BACKLIGHT_FREQ_AUX_SET_ENABLE;
+ dpcd_buf |= DP_EDP_BACKLIGHT_FREQ_AUX_SET_ENABLE;
}
- if (new_dpcd_buf != dpcd_buf) {
- ret = drm_dp_dpcd_writeb(aux,
DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf);
- if (ret != 1) {
- drm_dbg_kms(aux->drm_dev, "%s: Failed to write aux
backlight mode: %d\n",
- aux->name, ret);
- return ret < 0 ? ret : -EIO;
- }
+ ret = drm_dp_dpcd_writeb(aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
dpcd_buf);
+ if (ret != 1) {
+ drm_dbg_kms(aux->drm_dev, "%s: Failed to write aux backlight
mode: %d\n",
+ aux->name, ret);
+ return ret < 0 ? ret : -EIO;
}
ret = drm_edp_backlight_set_level(aux, bl, level);
--
2.31.1
[PATCH v5 4/5] drm/dp, drm/i915: Add support for VESA backlights using PWM for brightness control
Now that we've added support to i915 for controlling panel backlights that
need PWM to be enabled/disabled, let's finalize this and add support for
controlling brightness levels via PWM as well. This should hopefully put us
towards the path of supporting _ALL_ backlights via VESA's DPCD interface
which would allow us to finally start trusting the DPCD again.
Note however that we still don't enable using this by default on i915 when
it's not needed, primarily because I haven't yet had a chance to confirm if
it's safe to do this on the one machine in Intel's CI that had an issue
with this: samus-fi-bdw. I have done basic testing of this on other
machines though, by manually patching i915 to force it into PWM-only mode
on some of my laptops.
v2:
* Correct documentation (thanks Doug!)
* Get rid of backlight caps
Signed-off-by: Lyude Paul
Reviewed-by: Doug Anderson
Cc: Rajeev Nandan
Cc: Satadru Pramanik
---
drivers/gpu/drm/drm_dp_helper.c | 72 +--
.../drm/i915/display/intel_dp_aux_backlight.c | 44 +---
include/drm/drm_dp_helper.h | 7 +-
3 files changed, 89 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index af2aad2f4725..23f9073bc473 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -3290,6 +3290,10 @@ int drm_edp_backlight_set_level(struct drm_dp_aux *aux,
const struct drm_edp_bac
int ret;
u8 buf[2] = { 0 };
+ /* The panel uses the PWM for controlling brightness levels */
+ if (!bl->aux_set)
+ return 0;
+
if (bl->lsb_reg_used) {
buf[0] = (level & 0xff00) >> 8;
buf[1] = (level & 0x00ff);
@@ -3316,7 +3320,7 @@ drm_edp_backlight_set_enable(struct drm_dp_aux *aux,
const struct drm_edp_backli
int ret;
u8 buf;
- /* The panel uses something other then DPCD for enabling its backlight
*/
+ /* This panel uses the EDP_BL_PWR GPIO for enablement */
if (!bl->aux_enable)
return 0;
@@ -3351,11 +3355,11 @@ drm_edp_backlight_set_enable(struct drm_dp_aux *aux,
const struct drm_edp_backli
* restoring any important backlight state such as the given backlight level,
the brightness byte
* count, backlight frequency, etc.
*
- * Note that certain panels, while supporting brightness level controls over
DPCD, may not support
- * having their backlights enabled via the standard
%DP_EDP_DISPLAY_CONTROL_REGISTER. On such panels
- * &drm_edp_backlight_info.aux_enable will be set to %false, this function
will skip the step of
- * programming the %DP_EDP_DISPLAY_CONTROL_REGISTER, and the driver must
perform the required
- * implementation specific step for enabling the backlight after calling this
function.
+ * Note that certain panels do not support being enabled or disabled via DPCD,
but instead require
+ * that the driver handle enabling/disabling the panel through
implementation-specific means using
+ * the EDP_BL_PWR GPIO. For such panels, &drm_edp_backlight_info.aux_enable
will be set to %false,
+ * this function becomes a no-op, and the driver is expected to handle
powering the panel on using
+ * the EDP_BL_PWR GPIO.
*
* Returns: %0 on success, negative error code on failure.
*/
@@ -3363,7 +3367,12 @@ int drm_edp_backlight_enable(struct drm_dp_aux *aux,
const struct drm_edp_backli
const u16 level)
{
int ret;
- u8 dpcd_buf = DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
+ u8 dpcd_buf;
+
+ if (bl->aux_set)
+ dpcd_buf = DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
+ else
+ dpcd_buf = DP_EDP_BACKLIGHT_CONTROL_MODE_PWM;
if (bl->pwmgen_bit_count) {
ret = drm_dp_dpcd_writeb(aux, DP_EDP_PWMGEN_BIT_COUNT,
bl->pwmgen_bit_count);
@@ -3405,12 +3414,13 @@ EXPORT_SYMBOL(drm_edp_backlight_enable);
* @aux: The DP AUX channel to use
* @bl: Backlight capability info from drm_edp_backlight_init()
*
- * This function handles disabling DPCD backlight controls on a panel over
AUX. Note that some
- * panels have backlights that are enabled/disabled by other means, despite
having their brightness
- * values controlled through DPCD. On such panels
&drm_edp_backlight_info.aux_enable will be set to
- * %false, this function will become a no-op (and we will skip updating
- * %DP_EDP_DISPLAY_CONTROL_REGISTER), and the driver must take care to perform
it's own
- * implementation specific step for disabling the backlight.
+ * This function handles disabling DPCD backlight controls on a panel over AUX.
+ *
+ * Note that certain panels do not support being enabled or disabled via DPCD,
but instead require
+ * that the driver handle enabling/disabling the panel through
implementation-specific means using
+ * the EDP_BL_PWR GPIO. For such panels, &drm_edp_backlight_info.aux_enable
will be set to %false,
+ * this function becomes a
[PATCH v5 5/5] drm/i915: Clarify probing order in intel_dp_aux_init_backlight_funcs()
Hooray! We've managed to hit enough bugs upstream that I've been able to
come up with a pretty solid explanation for how backlight controls are
actually supposed to be detected and used these days. As well, having the
rest of the PWM bits in VESA's backlight interface implemented seems to
have fixed all of the problematic brightness controls laptop panels that
we've hit so far.
So, let's actually document this instead of just calling the laptop panels
liars. As well, I would like to formally apologize to all of the laptop
panels I called liars. I'm sorry laptop panels, hopefully you can all
forgive me and we can move past this~
Signed-off-by: Lyude Paul
Acked-by: Ville Syrjälä
---
.../drm/i915/display/intel_dp_aux_backlight.c| 16 +++-
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
index 96fe3eaba44a..8b9c925c4c16 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
@@ -456,11 +456,17 @@ int intel_dp_aux_init_backlight_funcs(struct
intel_connector *connector)
}
/*
-* A lot of eDP panels in the wild will report supporting both the
-* Intel proprietary backlight control interface, and the VESA
-* backlight control interface. Many of these panels are liars though,
-* and will only work with the Intel interface. So, always probe for
-* that first.
+* Since Intel has their own backlight control interface, the majority
of machines out there
+* using DPCD backlight controls with Intel GPUs will be using this
interface as opposed to
+* the VESA interface. However, other GPUs (such as Nvidia's) will
always use the VESA
+* interface. This means that there's quite a number of panels out
there that will advertise
+* support for both interfaces, primarily systems with Intel/Nvidia
hybrid GPU setups.
+*
+* There's a catch to this though: on many panels that advertise
support for both
+* interfaces, the VESA backlight interface will stop working once
we've programmed the
+* panel with Intel's OUI - which is also required for us to be able to
detect Intel's
+* backlight interface at all. This means that the only sensible way
for us to detect both
+* interfaces is to probe for Intel's first, and VESA's second.
*/
if (try_intel_interface &&
intel_dp_aux_supports_hdr_backlight(connector)) {
drm_dbg_kms(dev, "Using Intel proprietary eDP backlight
controls\n");
--
2.31.1
[PATCH v10 00/10] use DYNAMIC_DEBUG to implement DRM.debug & DRM.trace
Hi Jason, Greg, DRM-everyone,
This patchset has 3 separate but related parts:
1. DEFINE_DYNAMIC_DEBUG_BITGRPS [patch 1/10]
Declares DRM.debug style bitmap, bits control pr_debugs by matching formats
Adds callback to translate bits to $cmd > dynamic_debug/control
This could obsolete EXPORT(dynamic_debug_exec_queries) not included.
/* anticipated_usage */
static struct dyndbg_desc drm_categories_map[] = {
[0] = { DRM_DBG_CAT_CORE },
[1] = { DRM_DBG_CAT_DRIVER },
[2] = { DRM_DBG_CAT_KMS },
[3] = { DRM_DBG_CAT_PRIME }, ... };
DEFINE_DYNAMIC_DEBUG_BITGRPS(debug, __drm_debug,
" bits control drm.debug categories ",
drm_categories_map);
Please consider this patch for -next:
- new interface, new code, no users to break
- allows DRM folks to consider in earnest.
- api bikeshedding to do ?
struct dyndbg_desc isnt that great a name, others too probably.
2. use (1) to reimplement drm.debug [patches 3-7]:
1st in amdgpu & i915 to control existing pr_debugs by their formats
then in drm-print, for all drm.debug API users
POC for (1)
avoids drm_debug_enabled(), gives NOOP savings & new flexibility.
changes drm.debug categories from enum to format-prefix-string
alters in-log format to include the format-prefix-string
Daniel Vetter liked this at -v3
https://lore.kernel.org/lkml/YPbPvm%2FxcBlTK1wq@phenom.ffwll.local/
Im sure Ive missed stuff.
3. separately, Sean Paul proposed drm.trace to mirror drm.debug to tracefs
https://patchwork.freedesktop.org/series/78133/
He argues:
tracefs is fast/lightweight compared to syslog
independent selection (of drm categories) to tracefs
gives tailored traffic w.o flooding syslog
ISTM he's correct. So it follows that write-to-tracefs is also a good
feature for dyndbg, where its then available for all pr_debug users,
on a per-site basis, via echo +T >control. (iff CONFIG_TRACING).
So basically, I borg'd his:
[patch 14/14] drm/print: Add tracefs support to the drm logging helpers
Then I added a T flag, so it can be toggled from shell:
# turn on all drm's pr_debug --> tracefs
echo module drm +T > /proc/dynamic_debug/control
It appears to just work: (RFC)
The instance name is a placeholder, per-module subdirs kinda fits the
tracefs pattern, full mod/file-basename/function/line feels like
overkill, mod/basename-func.line would flatten it nicely. RFC.
[root@gandalf dyndbg-tracefs]# pwd
/sys/kernel/tracing/instances/dyndbg-tracefs
[root@gandalf dyndbg-tracefs]# echo 1 > /sys/module/drm/parameters/trace
[root@gandalf dyndbg-tracefs]# head -n16 trace | sed -e 's/^#//'
tracer: nop
entries-in-buffer/entries-written: 405/405 #P:24
_-=> irqs-off
/ _=> need-resched
| / _---=> hardirq/softirq
|| / _--=> preempt-depth
||| / _-=> migrate-disable
/ delay
TASK-PID CPU# | TIMESTAMP FUNCTION
| | | | | |
<...>-2254[000] . 7040.894352: __dynamic_pr_debug:
drm:core: comm="gnome-shel:cs0" pid=2254, dev=0xe200, auth=1, AMDGPU_CS
<...>-2207[015] . 7040.894654: __dynamic_pr_debug:
drm:core: comm="gnome-shell" pid=2207, dev=0xe200, auth=1, DRM_IOCTL_MODE_ADDFB2
<...>-2207[015] . 7040.995403: __dynamic_pr_debug:
drm:core: comm="gnome-shell" pid=2207, dev=0xe200, auth=1, DRM_IOCTL_MODE_RMFB
<...>-2207[015] . 7040.995413: __dynamic_pr_debug:
drm:core: OBJ ID: 121 (2)
This is the pr-debug doing most of that logging: (from dynamic_debug/control)
drivers/gpu/drm/drm_ioctl.c:866 [drm]drm_ioctl =T "drm:core: comm=\042%s\042
pid=%d, dev=0x%lx, auth=%d, %s\012"
Turning on decoration flags changes the trace:
echo module drm format drm:core: +mflt > /proc/dynamic_debug/control
TASK-PID CPU# | TIMESTAMP FUNCTION
| | | | | |
<...>-2254[003] . 15980.936660: __dynamic_pr_debug: [2254]
drm:drm_ioctl:866: drm:core: comm="gnome-shel:cs0" pid=2254, dev=0xe200,
auth=1, AMDGPU_CS
<...>-2207[015] . 15980.936966: __dynamic_pr_debug: [2207]
drm:drm_ioctl:866: drm:core: comm="gnome-shell" pid=2207, dev=0xe200, auth=1,
DRM_IOCTL_MODE_ADDFB2
<...>-2207[015] . 15981.037727: __dynamic_pr_debug: [2207]
drm:drm_ioctl:866: drm:core: comm="gnome-shell" pid=2207, dev=0xe200, auth=1,
DRM_IOCTL_MODE_RMFB
<...>-2207[015] . 15981.037739: __dynamic_pr_debug: [2207]
drm:drm_mode_object_put:195: drm:core: OBJ ID: 124 (2)
<...>-2207[015] . 15981.037742: __dynamic_pr_debug: [2207]
drm:drm_mode_object_put:195: drm:core: OBJ ID: 124 (1)
[PATCH v10 01/10] dyndbg: add DEFINE_DYNAMIC_DEBUG_BITGRPS macro and callbacks
DEFINE_DYNAMIC_DEBUG_BITGRPS(fsname, var, bitmap_desc, bitmap)
allows users to create a drm.debug style (bitmap) sysfs interface,
mapping each bit to a group of pr_debugs, matching on their formats.
This works well when the formats systematically include a prefix
string such as ERR|WARN|INFO, etc.
Such groups can (already) be manipulated like so:
echo "format $prefix +p" >control
This macro merely makes it easier to operate them as groups
/* standard usage */
static struct dyndbg_bitdesc my_bitmap[] = {
[0] = { "gvt:cmd:" },
[1] = { "gvt:core:" },
[2] = { "gvt:dpy:" },
[3] = { "gvt:el:" },
[4] = { "gvt:irq:" },
[5] = { "gvt:mm:" },
[6] = { "gvt:mmio:" },
[7] = { "gvt:render:" },
[8] = { "gvt:sched:" }
};
DEFINE_DYNAMIC_DEBUG_BITGRPS(debug_gvt, __gvt_debug,
"i915/gvt bitmap desc", my_bitmap);
In addition to the macro, patch adds:
- int param_set_dyndbg()
- int param_get_dyndbg()
- struct kernel_param_ops param_ops_dyndbg
Following the model of kernel/params.c STANDARD_PARAM_DEFS, these are
non-static and exported.
get/set use an augmented kernel_param; the arg refs a new struct
dyndbg_bitmap_param containing:
A- the map of "categories", an array of struct dyndbg_bitdescs,
indexed by bitpos, defining the match against pr_debug formats.
B- a pointer to the user module's ulong holding the bits/state.
By sharing state, we coordinate with code that still uses it
directly. This allows drm-debug api to be converted incrementally,
while still using __drm_debug & drm_debug_enabled() in other parts.
param_set_dyndbg() compares new vs old bits, and only updates prdbgs
on changes. This maximally preserves the underlying state, which may
have been customized via later `echo $cmd >control`. So if a user
really wants to know that all prdbgs are set precisely, they must
pre-clear then set.
dynamic_debug.h:
Add DEFINE_DYNAMIC_DEBUG_BITGRPS() described above, and a stub
throwing a BUILD_BUG (RFC) when used without DYNAMIC_DEBUG support.
Add structs dyndbg_bitdesc, dyndbg_bitmap_param to support the main
macro, and several helper macros wrapping the given categories with
^prefix and ' ' suffix. This way the callback can be more broadly
used, by using the right helper macro.
Also externs the struct kernel_param param_ops_dyndbg symbol, as is
done in moduleparams.h for all the STANDARD params.
USAGE NOTES:
Using dyndbg to query on "format $str" requires that $str must be
present in the compiled-in format string. Searching on "%s" does not
define a useful set of callsites.
Using DEFINE_DYNAMIC_DEBUG_CATEGORIES wo support gets a BUILD_BUG.
ISTM there is already action at a declarative distance, nobody needs
mystery as to why the /sysfs thingy didn't appear.
Dyndbg is agnostic wrt the categorization scheme used, in order to
play well with any prefix convention already in use in the codebase.
In fact, "prefix" is not strictly accurate without ^ anchor.
Ad-hoc categories and sub-categories are implicitly allowed, author
discipline and review is expected.
Hierarchical classes/categories are natural:
"^drm::" is used in a later commit
"^drm:::" is a natural extension.
"^drm:atomic:fail:" has been proposed, sounds directly useful
RFC: in a real sense we abandon enum strictures here, and lose some
compiler help, on spelling errs for example. Obviously "drm:" != "DRM:".
Some properties of a hierarchical category deserve explication:
Trailing spaces matter !
With 1..3-space ("drm: ", "drm:atomic: ", "drm:atomic:fail: "), the
":" doesn't terminate the search-space, the trailing space does. So a
"drm:" search spec will match all DRM categories & subcategories, and
will not be useful in an interface where all categories are already
controlled together. That said, "drm:atomic:" & "drm:atomic: " are
different, and both are useful in cases.
Ad-Hoc categories & sub-categories:
Ad-hoc categories are those format-prefixes already in use; both
amdgpu and i915 have numerous (120,~1800) pr_debugs, most of these use
a system, a small set (9,13) of prefixes, to categorize the output.
Dyndbg already works on these, this patch just allows adding a new
bitmap knob to control them.
Ad-hoc sub-categories are slightly trickier.
since drm_dbg_atomic("fail: ...") is a macro:
pr_debug("drm:atomic:" " " format,...) // cpp-paste in a trailing space
We get "drm:atomic: fail:", with that undesirable embedded space;
obviously not ideal wrt clear and simple prefixes.
a possible fix: drm_dbg_atomic_("fail: ..."); // trailing _ for ad-hoc subcat
Summarizing:
- "drm:kms: " & "drm:kms:" are different
- "drm:kms"also different - includes drm:kms2:
- "drm:kms:\t" also different - could be troublesome
- "drm:kms:*" doesn't work, no wildcard on format atm.
Order matters in DEFINE_DYNAMIC_DEBUG_CATEGORIES(... @bit_descs)
Since bits are/will-stay applied 0-N,
[PATCH v10 02/10] drm: fix doc grammar
allocates and initializes ... Signed-off-by: Jim Cromie --- include/drm/drm_drv.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h index 0cd95953cdf5..4b29261c4537 100644 --- a/include/drm/drm_drv.h +++ b/include/drm/drm_drv.h @@ -486,7 +486,7 @@ void *__devm_drm_dev_alloc(struct device *parent, * @type: the type of the struct which contains struct &drm_device * @member: the name of the &drm_device within @type. * - * This allocates and initialize a new DRM device. No device registration is done. + * This allocates and initializes a new DRM device. No device registration is done. * Call drm_dev_register() to advertice the device to user space and register it * with other core subsystems. This should be done last in the device * initialization sequence to make sure userspace can't access an inconsistent -- 2.31.1
[PATCH v10 03/10] amdgpu: use dyndbg.BITGRPS to control existing pr_debugs
logger_types.h defines many DC_LOG_*() categorized debug wrappers.
Most of these already use DRM debug API, so are controllable using
drm.debug, but others use a bare pr_debug("$prefix: .."), with 1 of 13
different class-prefixes matching [:uppercase:]
Use DEFINE_DYNAMIC_DEBUG_BITGRPS to create a sysfs location which maps
from bits to these 13 sets of categorized pr_debugs to en/disable.
Makefile adds -DDYNAMIC_DEBUG_MODULE for CONFIG_DYNAMIC_DEBUG_CORE,
otherwise BUILD_BUG_ON triggers (obvious errors on subtle misuse is
better than mysterious ones).
Signed-off-by: Jim Cromie
---
drivers/gpu/drm/amd/amdgpu/Makefile | 2 +
.../gpu/drm/amd/display/dc/core/dc_debug.c| 47 ++-
2 files changed, 48 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile
b/drivers/gpu/drm/amd/amdgpu/Makefile
index 653726588956..077342ca803f 100644
--- a/drivers/gpu/drm/amd/amdgpu/Makefile
+++ b/drivers/gpu/drm/amd/amdgpu/Makefile
@@ -38,6 +38,8 @@ ccflags-y := -I$(FULL_AMD_PATH)/include/asic_reg \
-I$(FULL_AMD_DISPLAY_PATH)/amdgpu_dm \
-I$(FULL_AMD_PATH)/amdkfd
+ccflags-$(CONFIG_DYNAMIC_DEBUG_CORE) += -DYNAMIC_DEBUG_MODULE
+
amdgpu-y := amdgpu_drv.o
# add KMS driver
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_debug.c
b/drivers/gpu/drm/amd/display/dc/core/dc_debug.c
index 21be2a684393..e49a755c6a69 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc_debug.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc_debug.c
@@ -36,8 +36,53 @@
#include "resource.h"
-#define DC_LOGGER_INIT(logger)
+#if defined(CONFIG_DRM_USE_DYNAMIC_DEBUG)
+#include
+
+unsigned long __debug_dc;
+EXPORT_SYMBOL(__debug_dc);
+
+#define help_(_N, _cat)"\t Bit-" #_N "\t" _cat "\n"
+
+#define DC_DYNDBG_BITMAP_DESC(name)\
+ "Control pr_debugs via /sys/module/amdgpu/parameters/" #name\
+ ", where each bit controls a debug category.\n" \
+ help_(0, "[SURFACE]:") \
+ help_(1, "[CURSOR]:") \
+ help_(2, "[PFLIP]:")\
+ help_(3, "[VBLANK]:") \
+ help_(4, "[HW_LINK_TRAINING]:") \
+ help_(5, "[HW_AUDIO]:") \
+ help_(6, "[SCALER]:") \
+ help_(7, "[BIOS]:") \
+ help_(8, "[BANDWIDTH_CALCS]:") \
+ help_(9, "[DML]:") \
+ help_(10, "[IF_TRACE]:")\
+ help_(11, "[GAMMA]:") \
+ help_(12, "[SMU_MSG]:")
+
+static struct dyndbg_bitdesc amdgpu_bitmap[] = {
+ [0] = { "[CURSOR]:" },
+ [1] = { "[PFLIP]:" },
+ [2] = { "[VBLANK]:" },
+ [3] = { "[HW_LINK_TRAINING]:" },
+ [4] = { "[HW_AUDIO]:" },
+ [5] = { "[SCALER]:" },
+ [6] = { "[BIOS]:" },
+ [7] = { "[BANDWIDTH_CALCS]:" },
+ [8] = { "[DML]:" },
+ [9] = { "[IF_TRACE]:" },
+ [10] = { "[GAMMA]:" },
+ [11] = { "[SMU_MSG]:" }
+};
+
+DEFINE_DYNAMIC_DEBUG_LOG_GROUPS(debug_dc, __debug_dc,
+ DC_DYNDBG_BITMAP_DESC(debug_dc),
+ amdgpu_bitmap);
+
+#endif
+#define DC_LOGGER_INIT(logger)
#define SURFACE_TRACE(...) do {\
if (dc->debug.surface_trace) \
--
2.31.1
[PATCH v10 05/10] i915/gvt: use dyndbg.BITGRPS for existing pr_debugs
The gvt component of this driver has ~120 pr_debugs with formats using
one of 9 fixed string prefixes, which are quite similar to those
enumerated in DRM debug categories. Following the interface model of
drm.debug, add a parameter to map bits to these format prefixes.
static struct dyndbg_bitdesc i915_bitmap[] = {
[0] = { "gvt:cmd:" },
[1] = { "gvt:core:" },
[2] = { "gvt:dpy:" },
[3] = { "gvt:el:" },
[4] = { "gvt:irq:" },
[5] = { "gvt:mm:" },
[6] = { "gvt:mmio:" },
[7] = { "gvt:render:" },
[8] = { "gvt:sched:" }
};
DEFINE_DYNAMIC_DEBUG_BITGRPS(debug_gvt, __gvt_debug,
"dyndbg bitmap desc",
If CONFIG_DYNAMIC_DEBUG_CORE=y, then gvt/Makefile adds
-DDYNAMIC_DEBUG_MODULE to cflags, which CONFIG_DYNAMIC_DEBUG=n
(CORE-only) builds need. This is redone more comprehensively soon.
Signed-off-by: Jim Cromie
---
drivers/gpu/drm/i915/Makefile| 2 ++
drivers/gpu/drm/i915/intel_gvt.c | 38
2 files changed, 40 insertions(+)
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 660bb03de6fc..0fa5f53312a8 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -317,6 +317,8 @@ i915-y += intel_gvt.o
include $(src)/gvt/Makefile
endif
+ccflags-$(CONFIG_DYNAMIC_DEBUG_CORE) += -DDYNAMIC_DEBUG_MODULE
+
obj-$(CONFIG_DRM_I915) += i915.o
obj-$(CONFIG_DRM_I915_GVT_KVMGT) += gvt/kvmgt.o
diff --git a/drivers/gpu/drm/i915/intel_gvt.c b/drivers/gpu/drm/i915/intel_gvt.c
index 4e70c1a9ef2e..efaac5777873 100644
--- a/drivers/gpu/drm/i915/intel_gvt.c
+++ b/drivers/gpu/drm/i915/intel_gvt.c
@@ -162,3 +162,41 @@ void intel_gvt_resume(struct drm_i915_private *dev_priv)
if (intel_gvt_active(dev_priv))
intel_gvt_pm_resume(dev_priv->gvt);
}
+
+#if defined(CONFIG_DRM_USE_DYNAMIC_DEBUG)
+
+unsigned long __gvt_debug;
+EXPORT_SYMBOL(__gvt_debug);
+
+static struct dyndbg_bitdesc i915_dyndbg_bitmap[] = {
+ [0] = { "gvt:cmd:" },
+ [1] = { "gvt:core:" },
+ [2] = { "gvt:dpy:" },
+ [3] = { "gvt:el:" },
+ [4] = { "gvt:irq:" },
+ [5] = { "gvt:mm:" },
+ [6] = { "gvt:mmio:" },
+ [7] = { "gvt:render:" },
+ [8] = { "gvt:sched:" }
+};
+
+#define help_(_N, _cat)"\t Bit-" #_N ":\t" _cat "\n"
+
+#define I915_GVT_CATEGORIES(name) \
+ " Enable debug output via /sys/module/i915/parameters/" #name \
+ ", where each bit enables a debug category.\n" \
+ help_(0, "gvt:cmd:")\
+ help_(1, "gvt:core:") \
+ help_(2, "gvt:dpy:")\
+ help_(3, "gvt:el:") \
+ help_(4, "gvt:irq:")\
+ help_(5, "gvt:mm:") \
+ help_(6, "gvt:mmio:") \
+ help_(7, "gvt:render:") \
+ help_(8, "gvt:sched:")
+
+DEFINE_DYNAMIC_DEBUG_BITGRPS(debug_gvt, __gvt_debug,
+I915_GVT_CATEGORIES(debug_gvt),
+i915_dyndbg_bitmap);
+
+#endif
--
2.31.1
[PATCH v10 04/10] i915/gvt: trim spaces from pr_debug "gvt: core:" prefixes
Taking embedded spaces out of existing prefixes makes them more easily
searchable; simplifying the extra quoting needed otherwise:
$> echo format "^gvt: core:" +p >control
Dropping the internal spaces means any trailing space in a query will
more clearly terminate the prefix being searched for.
Consider a generic drm-debug example:
# turn off ATOMIC reports
echo format "^drm:atomic: " -p > control
# turn off all ATOMIC:* reports, including any sub-categories
echo format "^drm:atomic:" -p > control
# turn on ATOMIC:FAIL: reports
echo format "^drm:atomic:fail: " +p > control
Removing embedded spaces in the format prefixes simplifies the
corresponding match string. This means that "quoted" match-prefixes
are only needed when the trailing space is desired, in order to
exclude explicitly sub-categorized pr-debugs; in this example,
"drm:atomic:fail:".
Signed-off-by: Jim Cromie
---
drivers/gpu/drm/i915/gvt/debug.h | 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/gvt/debug.h b/drivers/gpu/drm/i915/gvt/debug.h
index c6027125c1ec..bbecc279e077 100644
--- a/drivers/gpu/drm/i915/gvt/debug.h
+++ b/drivers/gpu/drm/i915/gvt/debug.h
@@ -36,30 +36,30 @@ do {
\
} while (0)
#define gvt_dbg_core(fmt, args...) \
- pr_debug("gvt: core: "fmt, ##args)
+ pr_debug("gvt:core: " fmt, ##args)
#define gvt_dbg_irq(fmt, args...) \
- pr_debug("gvt: irq: "fmt, ##args)
+ pr_debug("gvt:irq: " fmt, ##args)
#define gvt_dbg_mm(fmt, args...) \
- pr_debug("gvt: mm: "fmt, ##args)
+ pr_debug("gvt:mm: " fmt, ##args)
#define gvt_dbg_mmio(fmt, args...) \
- pr_debug("gvt: mmio: "fmt, ##args)
+ pr_debug("gvt:mmio: " fmt, ##args)
#define gvt_dbg_dpy(fmt, args...) \
- pr_debug("gvt: dpy: "fmt, ##args)
+ pr_debug("gvt:dpy: " fmt, ##args)
#define gvt_dbg_el(fmt, args...) \
- pr_debug("gvt: el: "fmt, ##args)
+ pr_debug("gvt:el: " fmt, ##args)
#define gvt_dbg_sched(fmt, args...) \
- pr_debug("gvt: sched: "fmt, ##args)
+ pr_debug("gvt:sched: " fmt, ##args)
#define gvt_dbg_render(fmt, args...) \
- pr_debug("gvt: render: "fmt, ##args)
+ pr_debug("gvt:render: " fmt, ##args)
#define gvt_dbg_cmd(fmt, args...) \
- pr_debug("gvt: cmd: "fmt, ##args)
+ pr_debug("gvt:cmd: " fmt, ##args)
#endif
--
2.31.1
[PATCH v10 07/10] drm_print: instrument drm_debug_enabled
Duplicate drm_debug_enabled() code into both "basic" and "dyndbg"
ifdef branches. Then add a pr_debug("todo: ...") into the "dyndbg"
branch.
Then convert the "dyndbg" branch's code to a macro, so that the
pr_debug() get its callsite info from the invoking function, instead
of from drm_debug_enabled() itself.
This gives us unique callsite info for the 8 remaining users of
drm_debug_enabled(), and lets us enable them individually to see how
much logging traffic they generate. The oft-visited callsites can
then be reviewed for runtime cost and possible optimizations.
Heres what we get:
bash-5.1# modprobe drm
dyndbg: 384 debug prints in module drm
bash-5.1# grep todo: /proc/dynamic_debug/control
drivers/gpu/drm/drm_edid.c:1843 [drm]connector_bad_edid =_ "todo: maybe avoid
via dyndbg\012"
drivers/gpu/drm/drm_print.c:309 [drm]___drm_dbg =p "todo: maybe avoid via
dyndbg\012"
drivers/gpu/drm/drm_print.c:286 [drm]__drm_dev_dbg =p "todo: maybe avoid via
dyndbg\012"
drivers/gpu/drm/drm_vblank.c:1491 [drm]drm_vblank_restore =_ "todo: maybe avoid
via dyndbg\012"
drivers/gpu/drm/drm_vblank.c:787
[drm]drm_crtc_vblank_helper_get_vblank_timestamp_internal =_ "todo: maybe avoid
via dyndbg\012"
drivers/gpu/drm/drm_vblank.c:410 [drm]drm_crtc_accurate_vblank_count =_ "todo:
maybe avoid via dyndbg\012"
drivers/gpu/drm/drm_atomic_uapi.c:1457 [drm]drm_mode_atomic_ioctl =_ "todo:
maybe avoid via dyndbg\012"
drivers/gpu/drm/drm_edid_load.c:178 [drm]edid_load =_ "todo: maybe avoid via
dyndbg\012"
At quick glance, edid won't qualify, drm_print might, drm_vblank is
strongest chance, maybe atomic-ioctl too.
Signed-off-by: Jim Cromie
---
include/drm/drm_print.h | 17 +++--
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index 392cff7cb95c..a902bd4d8c55 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -381,6 +381,11 @@ enum drm_debug_category {
#define DRM_DBG_CAT_DP DRM_UT_DP
#define DRM_DBG_CAT_DRMRES DRM_UT_DRMRES
+static inline bool drm_debug_enabled(enum drm_debug_category category)
+{
+ return unlikely(__drm_debug & category);
+}
+
#else /* CONFIG_DRM_USE_DYNAMIC_DEBUG */
/* join prefix + format in cpp so dyndbg can see it */
@@ -414,12 +419,13 @@ enum drm_debug_category {
#define DRM_DBG_CAT_DP "drm:dp: "
#define DRM_DBG_CAT_DRMRES "drm:res: "
-#endif /* CONFIG_DRM_USE_DYNAMIC_DEBUG */
+#define drm_debug_enabled(category)\
+ ({ \
+ pr_debug("todo: maybe avoid via dyndbg\n"); \
+ unlikely(__drm_debug & (category)); \
+ })
-static inline bool drm_debug_enabled(enum drm_debug_category category)
-{
- return unlikely(__drm_debug & category);
-}
+#endif /* CONFIG_DRM_USE_DYNAMIC_DEBUG */
/*
* struct device based logging
@@ -582,7 +588,6 @@ void __drm_dev_dbg(const struct device *dev, enum
drm_debug_category category,
#define drm_dbg_drmres(drm, fmt, ...) \
drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_DBG_CAT_DRMRES, fmt,
##__VA_ARGS__)
-
/*
* printk based logging
*
--
2.31.1
[PATCH v10 06/10] drm_print: add choice to use dynamic debug in drm-debug
drm's debug system writes 10 distinct categories of messages to syslog
using a small API[1]: drm_dbg*(10 names), DRM_DEV_DEBUG*(3 names),
DRM_DEBUG*(8 names). There are thousands of these callsites, each
categorized in this systematized way.
These callsites can be enabled at runtime by their category, each
controlled by a bit in drm.debug (/sys/modules/drm/parameter/debug).
In the current "basic" implementation, drm_debug_enabled() tests these
bits in __drm_debug each time an API[1] call is executed; while cheap
individually, the costs accumulate with uptime.
This patch uses dynamic-debug with (required) jump-label to patch
enabled callsites onto their respective NOOP slots, avoiding all
runtime bit-checks of __drm_debug by drm_debug_enabled().
Dynamic debug has no concept of category, but we can emulate one by
replacing enum categories with a set of prefix-strings; "drm:core:",
"drm:kms:" "drm:driver:" etc, and prepend them (at compile time) to
the given formats.
Then we can use:
# echo module drm format "^drm:core: " +p > control`
to enable the whole category with one query.
This conversion yields many new prdbg callsites:
dyndbg: 207 debug prints in module drm_kms_helper
dyndbg: 376 debug prints in module drm
dyndbg: 1811 debug prints in module i915
dyndbg: 3917 debug prints in module amdgpu
Each site costs 56 bytes of .data, which is a big increase for
drm modules, so CONFIG_DRM_USE_DYNAMIC_DEBUG makes it optional.
CONFIG_JUMP_LABEL is also required, to get the promised optimizations.
The "basic" -> "dyndbg" switchover is layered into the macro scheme
A. A "prefix" version of DRM_UT_ map, named DRM_DBG_CAT_
"basic": DRM_DBG_CAT_ <=== DRM_UT_. Identity map.
"dyndbg":
#define DRM_DBG_CAT_KMS"drm:kms: "
#define DRM_DBG_CAT_PRIME "drm:prime: "
#define DRM_DBG_CAT_ATOMIC "drm:atomic: "
DRM_UT_* are preserved, since theyre used elsewhere. Since the
callback maintains its state in __drm_debug, drm_debug_enabled() will
stay synchronized, and continue to work. We can address them
separately if they are called enough to be worth fixing.
B. drm_dev_dbg() & drm_debug() are interposed with macros
basic:forward to renamed fn, with args preserved
enabled: redirect to pr_debug, dev_dbg, with CATEGORY format catenated
This is where drm_debug_enabled() is avoided. The prefix is prepended
at compile-time, no category at runtime.
C. API[1] uses DRM_DBG_CAT_s
The API already uses B, now it uses A too, instead of DRM_UT_, to
get the correct token type for "basic" and "dyndbg" configs.
D. use DEFINE_DYNAMIC_DEBUG_CATEGORIES()
This defines the map using DRM_CAT_s, and creates the /sysfs
bitmap to control those categories.
CONFIG_DRM_USE_DYNAMIC_DEBUG is also used to adjust amdgpu, i915
makefiles to add -DDYNAMIC_DEBUG_MODULE; it includes the current
CONFIG_DYNAMIC_DEBUG_CORE and is enabled by the user.
LIMITATIONS:
dev_dbg(etal) effectively prepends twice, category then driver-name,
yielding format strings like so:
bash-5.1# grep amdgpu: /proc/dynamic_debug/control | grep drm: | cut -d= -f2-
_ "amdgpu: drm:core: fence driver on ring %s use gpu addr 0x%016llx\012"
_ "amdgpu: drm:kms: Cannot create framebuffer from imported dma_buf\012"
This means we cannot use anchored "^drm:kms: " to specify the
category, a small loss of precision.
Note that searching on "format ^amdgpu: " works, but this is less
valuable, because the same can be done with "module amdgpu".
NOTES:
Because the dyndbg callback is keeping state in __drm_debug, it
synchronizes with drm_debug_enabled() and its remaining users; the
switchover should be transparent.
Code Review is expected to catch the lack of correspondence between
bit=>prefix definitions (the selector) and the prefixes used in the
API[1] layer above pr_debug()
I've coded the categories with trailing spaces. This excludes any
sub-categories which might get added later. This convention protects
any "drm:atomic:fail:" callsites from getting stomped on by `echo 0 >
debug`. Other categories could differ, but we need some default.
Dyndbg requires that the prefix be in the compiled-in format string;
run-time prefixing evades callsite selection by category.
pr_debug("%s: ...", __func__, ...) // not ideal
Unfortunately __func__ is not a macro, and cannot be catenated at
preprocess/compile time.
If you want that, you might consider +mfl flags instead;)
Signed-off-by: Jim Cromie
---
v5:
. use DEFINE_DYNAMIC_DEBUG_CATEGORIES in drm_print.c
. s/DRM_DBG_CLASS_/DRM_DBG_CAT_/ - dont need another term
. default=y in Kconfig entry - per @DanVet
. move some commit-log prose to dyndbg commit
. add-prototyes to (param_get/set)_dyndbg
. more wrinkles found by
. relocate ratelimit chunk from elsewhere
v6:
. add kernel doc
. fix cpp paste, drop '#'
v7:
. change __drm_debug to long, to fit with DEFINE_DYNAMIC_DEBUG_CATEGORIES
. add -DDYNAMIC_DEBUG_MODULE to ccflags if DRM_USE_DYNAMIC_DEBUG
v8:
. adapt to altered ^ insertion
. add mem cos
[PATCH v10 10/10] drm: use DEFINE_DYNAMIC_DEBUG_TRACE_GROUPS in 3 places
add sysfs knobs to enable modules' pr_debug()s ---> tracefs
Signed-off-by: Jim Cromie
---
drivers/gpu/drm/amd/display/dc/core/dc_debug.c | 8
drivers/gpu/drm/drm_print.c| 13 ++---
drivers/gpu/drm/i915/intel_gvt.c | 15 ---
3 files changed, 30 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_debug.c
b/drivers/gpu/drm/amd/display/dc/core/dc_debug.c
index e49a755c6a69..58c56c1708e7 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc_debug.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc_debug.c
@@ -80,6 +80,14 @@ DEFINE_DYNAMIC_DEBUG_LOG_GROUPS(debug_dc, __debug_dc,
DC_DYNDBG_BITMAP_DESC(debug_dc),
amdgpu_bitmap);
+#if defined(CONFIG_TRACING)
+
+unsigned long __trace_dc;
+EXPORT_SYMBOL(__trace_dc);
+DEFINE_DYNAMIC_DEBUG_LOG_GROUPS(trace_dc, __trace_dc,
+ DC_DYNDBG_BITMAP_DESC(trace_dc),
+ amdgpu_bitmap);
+#endif
#endif
#define DC_LOGGER_INIT(logger)
diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
index d5e0ffad467b..ee20e9c14ce9 100644
--- a/drivers/gpu/drm/drm_print.c
+++ b/drivers/gpu/drm/drm_print.c
@@ -72,9 +72,16 @@ static struct dyndbg_bitdesc drm_dyndbg_bitmap[] = {
[8] = { DRM_DBG_CAT_DP },
[9] = { DRM_DBG_CAT_DRMRES }
};
-DEFINE_DYNAMIC_DEBUG_BITGRPS(debug, __drm_debug, DRM_DEBUG_DESC,
-drm_dyndbg_bitmap);
-
+DEFINE_DYNAMIC_DEBUG_LOG_GROUPS(debug, __drm_debug, DRM_DEBUG_DESC,
+ drm_dyndbg_bitmap);
+
+#ifdef CONFIG_TRACING
+struct trace_array *trace_arr;
+unsigned long __drm_trace;
+EXPORT_SYMBOL(__drm_trace);
+DEFINE_DYNAMIC_DEBUG_TRACE_GROUPS(trace, __drm_trace, DRM_DEBUG_DESC,
+ drm_dyndbg_bitmap);
+#endif
#endif
void __drm_puts_coredump(struct drm_printer *p, const char *str)
diff --git a/drivers/gpu/drm/i915/intel_gvt.c b/drivers/gpu/drm/i915/intel_gvt.c
index efaac5777873..84348d4aedf6 100644
--- a/drivers/gpu/drm/i915/intel_gvt.c
+++ b/drivers/gpu/drm/i915/intel_gvt.c
@@ -195,8 +195,17 @@ static struct dyndbg_bitdesc i915_dyndbg_bitmap[] = {
help_(7, "gvt:render:") \
help_(8, "gvt:sched:")
-DEFINE_DYNAMIC_DEBUG_BITGRPS(debug_gvt, __gvt_debug,
-I915_GVT_CATEGORIES(debug_gvt),
-i915_dyndbg_bitmap);
+DEFINE_DYNAMIC_DEBUG_LOG_GROUPS(debug_gvt, __gvt_debug,
+ I915_GVT_CATEGORIES(debug_gvt),
+ i915_dyndbg_bitmap);
+#if defined(CONFIG_TRACING)
+
+unsigned long __gvt_trace;
+EXPORT_SYMBOL(__gvt_trace);
+DEFINE_DYNAMIC_DEBUG_TRACE_GROUPS(trace_gvt, __gvt_trace,
+ I915_GVT_CATEGORIES(trace_gvt),
+ i915_dyndbg_bitmap);
+
+#endif
#endif
--
2.31.1
[PATCH v10 09/10] dyndbg: create DEFINE_DYNAMIC_DEBUG_LOG|TRACE_GROUPS
With the recent addition of pr_debug to tracefs via +T flag, we now
want to add drm.trace; like its model: drm.debug, it maps bits to
pr_debug categories, but this one enables/disables writing to tracefs
(iff CONFIG_TRACING).
Do this by:
1. add flags to dyndbg_bitmap_param, holds "p" or "T" to work for either.
add DEFINE_DYNAMIC_DEBUG_BITGRPS_FLAGS to init .flags
DEFINE_DYNAMIC_DEBUG_BITGRPS gets "p" for compat.
use it from...
2. DEFINE_DYNAMIC_DEBUG_LOG_GROUPS as (1) with "p" flags - print to syslog
DEFINE_DYNAMIC_DEBUG_TRACE_GROUPS as (1) with "T" flags - trace to tracefs
add kdoc to these
NOTES
The flags args (1) is a string, not just a 'p' or 'T'. This allows
use of decorator flags ("mflt") too, in case the module author wants
to insure those decorations are in the trace & log.
The LOG|TRACE (2) macros don't use any decorator flags, (and therefore
don't toggle them), allowing users to control those themselves.
Decorator flags are shared for both LOG and TRACE consumers,
coordination between users is expected. ATM, theres no declarative
way to preset decorator flags, but DEFINE_DYNAMIC_DEBUG_BITGRPS_FLAGS
can be used to explicitly toggle them.
Signed-off-by: Jim Cromie
---
include/linux/dynamic_debug.h | 44 ++-
lib/dynamic_debug.c | 4 ++--
2 files changed, 35 insertions(+), 13 deletions(-)
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 792bcff0297e..918ac1a92358 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -255,30 +255,52 @@ struct dyndbg_bitdesc {
struct dyndbg_bitmap_param {
unsigned long *bits;/* ref to shared state */
+ const char *flags;
unsigned int maplen;
struct dyndbg_bitdesc *map; /* indexed by bitpos */
};
#if defined(CONFIG_DYNAMIC_DEBUG) || \
(defined(CONFIG_DYNAMIC_DEBUG_CORE) && defined(DYNAMIC_DEBUG_MODULE))
+
+#define DEFINE_DYNAMIC_DEBUG_BITGRPS_FLAGS(fsname, _var, _flags, desc, data) \
+ MODULE_PARM_DESC(fsname, desc); \
+ static struct dyndbg_bitmap_param ddcats_##_var = \
+ { .bits = &(_var), .flags = (_flags), \
+ .map = data, .maplen = ARRAY_SIZE(data) };\
+ module_param_cb(fsname, ¶m_ops_dyndbg, &ddcats_##_var, 0644)
+
+#define DEFINE_DYNAMIC_DEBUG_BITGRPS(fsname, _var, desc, data) \
+ DEFINE_DYNAMIC_DEBUG_BITGRPS_FLAGS(fsname, _var, "p", desc, data)
+
/**
- * DEFINE_DYNAMIC_DEBUG_BITGRPS() - bitmap control of pr_debugs, by format
match
+ * DEFINE_DYNAMIC_DEBUG_LOG_GROUPS() - bitmap control of grouped pr_debugs -->
syslog
+ *
* @fsname: parameter basename under /sys
* @_var: C-identifier holding bitmap
* @desc: string summarizing the controls provided
* @bitmap: C array of struct dyndbg_bitdescs
*
- * Intended for modules with a systematic use of pr_debug prefixes in
- * the format strings, this allows modules calling pr_debugs to
- * control them in groups by matching against their formats, and map
- * them to bits 0-N of a sysfs control point.
+ * Intended for modules having pr_debugs with prefixed/categorized
+ * formats; this lets you group them by substring match, map groups to
+ * bits, and enable per group to write to syslog, via @fsname.
*/
-#define DEFINE_DYNAMIC_DEBUG_BITGRPS(fsname, _var, desc, data) \
- MODULE_PARM_DESC(fsname, desc); \
- static struct dyndbg_bitmap_param ddcats_##_var = \
- { .bits = &(_var), .map = data, \
- .maplen = ARRAY_SIZE(data) }; \
- module_param_cb(fsname, ¶m_ops_dyndbg, &ddcats_##_var, 0644)
+#define DEFINE_DYNAMIC_DEBUG_LOG_GROUPS(fsname, _var, desc, data) \
+ DEFINE_DYNAMIC_DEBUG_BITGRPS_FLAGS(fsname, _var, "p", desc, data)
+
+/**
+ * DEFINE_DYNAMIC_DEBUG_TRACE_GROUPS() - bitmap control of pr_debugs -->
tracefs
+ * @fsname: parameter basename under /sys
+ * @_var: C-identifier holding bitmap
+ * @desc: string summarizing the controls provided
+ * @bitmap: C array of struct dyndbg_bitdescs
+ *
+ * Intended for modules having pr_debugs with prefixed/categorized
+ * formats; this lets you group them by substring match, map groups to
+ * bits, and enable per group to write to tracebuf, via @fsname.
+ */
+#define DEFINE_DYNAMIC_DEBUG_TRACE_GROUPS(fsname, _var, desc, data)\
+ DEFINE_DYNAMIC_DEBUG_BITGRPS_FLAGS(fsname, _var, "T", desc, data)
extern const struct kernel_param_ops param_ops_dyndbg;
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index d6e9c833f5d4..f55861dd76b2 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -629,8 +629,8 @@ int param_set_dyndbg(const char *instr, const struct
kernel_param *kp)
for (i = 0; i < p->maplen && i < BITS_PER_LONG; map++, i++) {
if (test_bit(i, &inb
[PATCH v10 08/10] dyndbg: add print-to-tracefs, selftest with it - RFC
Sean Paul proposed, in: https://patchwork.freedesktop.org/series/78133/ drm/trace: Mirror DRM debug logs to tracefs His patchset's objective is to be able to independently steer some of the drm.debug stream to an alternate tracing destination, by splitting drm_debug_enabled() into syslog & trace flavors, and enabling them separately. 2 advantages were identified: 1- syslog is heavyweight, tracefs is much lighter 2- separate selection of enabled categories means less traffic Dynamic-Debug can do 2nd exceedingly well: A- all work is behind jump-label's NOOP, zero off cost. B- exact site selectivity, precisely the useful traffic. can tailor enabled set interactively, at shell. Since the tracefs interface is effective for drm (the threads suggest so), adding that interface to dynamic-debug has real potential for everyone including drm. if CONFIG_TRACING: Grab Sean's trace_init/cleanup code, use it to provide tracefs available by default to all pr_debugs. This will likely need some further per-module treatment; perhaps something reflecting hierarchy of module,file,function,line, maybe with a tuned flattening. endif CONFIG_TRACING Add a new +T flag to enable tracing, independent of +p, and add and use 3 macros: dyndbg_site_is_enabled/logging/tracing(), to encapsulate the flag checks. Existing code treats T like other flags. Add ddebug_validate_flags() as last step in ddebug_parse_flags(). Its only job is to fail on +T for non-CONFIG_TRACING builds. It only sees the new flags, and cannot validate specific state transitions. This is fine, since we have no need for that; such a test would have to be done in ddebug_change(), which actually updates the callsites. ddebug_change() adjusts the static-key-enable/disable condition to use _DPRINTK_ENABLED / abstraction macros. dynamic_emit_prefix() now gates on _DPRINTK_ENABLED too, as an optimization but mostly to allow decluttering of its users. __dynamic_pr_debug() etal get minor changes: - call dynamic_emit_prefix() 1st, _enabled() optimizes. - if (T) call trace_array_printk - if (!p) go around original printk code. done to minimize diff, goto-ectomy + reindent later/separately - share vaf across p|T WRT _dev, I skipped all the specific dev_emit_prefix additions for now. tracefs is a fast customer with different needs, its not clear that pretty device-ID-ish strings is useful tracefs content (on ingest), or that couldn't be done more efficiently while analysing or postprocesing the tracefs buffer. SELFTEST: test_dynamic_debug.ko: Uses the tracer facility to implement a kernel module selftest. TODO: Earlier core code had (tracerfn)() indirection, allowing a plugin side-effector we could test the results of. ATM all the tests which count +T'd callsite executions (and which expect >0) are failing. Now it needs a rethink to test from userspace, rather than the current test-once at module-load. It needs a parameters/testme button. So remainder of this is a bit stale - A custom tracer counts the number of calls (of T-enabled pr_debugs), - do_debugging(x) calls a set of categorized pr_debugs x times - test registers the tracer on the module then iteratively: manipulates dyndbg states via query-cmds, mostly format ^prefix runs do_debugging() counts enabled callsite executions reports mismatches - modprobe test_dynamic_debug use_bad_tracer=1 attaches a bad/recursive tracer Bad Things (did) Happen. has thrown me interesting panics. cannot replicate atm. RFC: (DONE) The "tracer" interface probably needs work and a new name. It is only 1/2 way towards a real tracefs interface; and the code I lifted from Sean Paul in the next patch could be implemented in dynamic_debug.c instead, and made available for all pr_debug users. This would also eliminate need for dynamic_debug_(un)register_tracer(), since dyndbg could just provide it when TRACING is on. NOTES: $> modprobe test_dynamic_debug dyndbg=+p it fails 3/29 tests. havent looked at why. $> modprobe test_dynamic_debug use_bad_tracer=1 Earlier in dev, bad_tracer() exploded in recursion, I havent been able to replicate that lately. Signed-off-by: Jim Cromie --- .../admin-guide/dynamic-debug-howto.rst | 7 +- MAINTAINERS | 1 + include/linux/dynamic_debug.h | 12 +- lib/Kconfig.debug | 11 + lib/Makefile | 1 + lib/dynamic_debug.c | 127 -- lib/test_dynamic_debug.c | 222 ++ 7 files changed, 355 insertions(+), 26 deletions(-) create mode 100644 lib/test_dynamic_debug.c diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst index b119b8277b3e..48d32782bb11 100644 --- a/Documentation/admin-guide/dynamic-debug-howto.rst +++ b/Documentation/admin-guide/dynamic-debug-howto.rst @@ -227,7 +227,8 @@ of the
[PATCH v2 0/6] Support module unload and hotunplug
v2: Introduces a TTM documentation change to clarify the discussion that happened after the first version of this series was sent. Also, removing pointless "unlikely" in the "Introduce a new placement for MOB page tables" commit that Thomas noticed. This is a largely trivial set that makes vmwgfx support module reload and PCI hot-unplug. It also makes IGT's core_hotunplug pass instead of kernel oops'ing. Cc: Christian König Cc: Thomas Hellström Zack Rusin (6): drm/vmwgfx: Remove the deprecated lower mem limit drm/vmwgfx: Release ttm memory if probe fails drm/vmwgfx: Fail to initialize on broken configs drm/vmwgfx: Introduce a new placement for MOB page tables drm/vmwgfx: Switch the internal BO's to ttm_bo_type_kernel drm/ttm: Clarify that the TTM_PL_SYSTEM buffers need to stay idle drivers/gpu/drm/vmwgfx/Makefile | 2 +- drivers/gpu/drm/vmwgfx/ttm_memory.c | 99 +-- drivers/gpu/drm/vmwgfx/ttm_memory.h | 6 +- drivers/gpu/drm/vmwgfx/vmwgfx_bo.c| 2 +- drivers/gpu/drm/vmwgfx/vmwgfx_cmd.c | 7 ++ drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 40 +--- drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 12 ++- .../gpu/drm/vmwgfx/vmwgfx_system_manager.c| 90 + drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c| 58 +-- include/drm/ttm/ttm_placement.h | 10 ++ 10 files changed, 174 insertions(+), 152 deletions(-) create mode 100644 drivers/gpu/drm/vmwgfx/vmwgfx_system_manager.c -- 2.32.0
[PATCH v2 2/6] drm/vmwgfx: Release ttm memory if probe fails
The ttm mem global state was leaking if the vmwgfx driver load failed.
In case of a driver load failure we have to make sure we also release
the ttm mem global state.
Signed-off-by: Zack Rusin
Reviewed-by: Martin Krastev
---
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 26 --
1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index ab9a1750e1df..8d0b083ba267 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -1617,34 +1617,40 @@ static int vmw_probe(struct pci_dev *pdev, const struct
pci_device_id *ent)
ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &driver);
if (ret)
- return ret;
+ goto out_error;
ret = pcim_enable_device(pdev);
if (ret)
- return ret;
+ goto out_error;
vmw = devm_drm_dev_alloc(&pdev->dev, &driver,
struct vmw_private, drm);
- if (IS_ERR(vmw))
- return PTR_ERR(vmw);
+ if (IS_ERR(vmw)) {
+ ret = PTR_ERR(vmw);
+ goto out_error;
+ }
pci_set_drvdata(pdev, &vmw->drm);
ret = ttm_mem_global_init(&ttm_mem_glob, &pdev->dev);
if (ret)
- return ret;
+ goto out_error;
ret = vmw_driver_load(vmw, ent->device);
if (ret)
- return ret;
+ goto out_release;
ret = drm_dev_register(&vmw->drm, 0);
- if (ret) {
- vmw_driver_unload(&vmw->drm);
- return ret;
- }
+ if (ret)
+ goto out_unload;
return 0;
+out_unload:
+ vmw_driver_unload(&vmw->drm);
+out_release:
+ ttm_mem_global_release(&ttm_mem_glob);
+out_error:
+ return ret;
}
static int __init vmwgfx_init(void)
--
2.32.0
[PATCH v2 1/6] drm/vmwgfx: Remove the deprecated lower mem limit
TTM during the transition to the new page allocator lost the ability
to constrain the allocations via the lower_mem_limit. The code has
been unused since the change:
256dd44bd897 ("drm/ttm: nuke old page allocator")
and there's no reason to keep it.
Fixes: 256dd44bd897 ("drm/ttm: nuke old page allocator")
Signed-off-by: Zack Rusin
Reviewed-by: Martin Krastev
---
drivers/gpu/drm/vmwgfx/ttm_memory.c | 99 +
drivers/gpu/drm/vmwgfx/ttm_memory.h | 6 +-
2 files changed, 2 insertions(+), 103 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/ttm_memory.c
b/drivers/gpu/drm/vmwgfx/ttm_memory.c
index edd17c30d5a5..2ced4c06ca45 100644
--- a/drivers/gpu/drm/vmwgfx/ttm_memory.c
+++ b/drivers/gpu/drm/vmwgfx/ttm_memory.c
@@ -34,7 +34,6 @@
#include
#include
#include
-#include
#include
#include
@@ -173,69 +172,7 @@ static struct kobj_type ttm_mem_zone_kobj_type = {
.sysfs_ops = &ttm_mem_zone_ops,
.default_attrs = ttm_mem_zone_attrs,
};
-
-static struct attribute ttm_mem_global_lower_mem_limit = {
- .name = "lower_mem_limit",
- .mode = S_IRUGO | S_IWUSR
-};
-
-static ssize_t ttm_mem_global_show(struct kobject *kobj,
-struct attribute *attr,
-char *buffer)
-{
- struct ttm_mem_global *glob =
- container_of(kobj, struct ttm_mem_global, kobj);
- uint64_t val = 0;
-
- spin_lock(&glob->lock);
- val = glob->lower_mem_limit;
- spin_unlock(&glob->lock);
- /* convert from number of pages to KB */
- val <<= (PAGE_SHIFT - 10);
- return snprintf(buffer, PAGE_SIZE, "%llu\n",
- (unsigned long long) val);
-}
-
-static ssize_t ttm_mem_global_store(struct kobject *kobj,
- struct attribute *attr,
- const char *buffer,
- size_t size)
-{
- int chars;
- uint64_t val64;
- unsigned long val;
- struct ttm_mem_global *glob =
- container_of(kobj, struct ttm_mem_global, kobj);
-
- chars = sscanf(buffer, "%lu", &val);
- if (chars == 0)
- return size;
-
- val64 = val;
- /* convert from KB to number of pages */
- val64 >>= (PAGE_SHIFT - 10);
-
- spin_lock(&glob->lock);
- glob->lower_mem_limit = val64;
- spin_unlock(&glob->lock);
-
- return size;
-}
-
-static struct attribute *ttm_mem_global_attrs[] = {
- &ttm_mem_global_lower_mem_limit,
- NULL
-};
-
-static const struct sysfs_ops ttm_mem_global_ops = {
- .show = &ttm_mem_global_show,
- .store = &ttm_mem_global_store,
-};
-
-static struct kobj_type ttm_mem_glob_kobj_type = {
- .sysfs_ops = &ttm_mem_global_ops,
- .default_attrs = ttm_mem_global_attrs,
-};
+static struct kobj_type ttm_mem_glob_kobj_type = {0};
static bool ttm_zones_above_swap_target(struct ttm_mem_global *glob,
bool from_wq, uint64_t extra)
@@ -435,11 +372,6 @@ int ttm_mem_global_init(struct ttm_mem_global *glob,
struct device *dev)
si_meminfo(&si);
- spin_lock(&glob->lock);
- /* set it as 0 by default to keep original behavior of OOM */
- glob->lower_mem_limit = 0;
- spin_unlock(&glob->lock);
-
ret = ttm_mem_init_kernel_zone(glob, &si);
if (unlikely(ret != 0))
goto out_no_zone;
@@ -527,35 +459,6 @@ void ttm_mem_global_free(struct ttm_mem_global *glob,
}
EXPORT_SYMBOL(ttm_mem_global_free);
-/*
- * check if the available mem is under lower memory limit
- *
- * a. if no swap disk at all or free swap space is under swap_mem_limit
- * but available system mem is bigger than sys_mem_limit, allow TTM
- * allocation;
- *
- * b. if the available system mem is less than sys_mem_limit but free
- * swap disk is bigger than swap_mem_limit, allow TTM allocation.
- */
-bool
-ttm_check_under_lowerlimit(struct ttm_mem_global *glob,
- uint64_t num_pages,
- struct ttm_operation_ctx *ctx)
-{
- int64_t available;
-
- /* We allow over commit during suspend */
- if (ctx->force_alloc)
- return false;
-
- available = get_nr_swap_pages() + si_mem_available();
- available -= num_pages;
- if (available < glob->lower_mem_limit)
- return true;
-
- return false;
-}
-
static int ttm_mem_global_reserve(struct ttm_mem_global *glob,
struct ttm_mem_zone *single_zone,
uint64_t amount, bool reserve)
diff --git a/drivers/gpu/drm/vmwgfx/ttm_memory.h
b/drivers/gpu/drm/vmwgfx/ttm_memory.h
index c50dba774485..7b0d617ebcb1 100644
--- a/drivers/gpu/drm/vmwgfx/ttm_memory.h
+++ b/drivers/gpu/drm/vmwgfx/ttm_memory.h
@@ -50,8 +50,6 @@
* @work: The workqueue callback for the shrink queue.
* @lock: Lock to protect the @shrink - and the me
[PATCH v2 3/6] drm/vmwgfx: Fail to initialize on broken configs
Some of our hosts have a bug where rescaning a pci bus results in stale
fifo memory being mapped on the host. This makes any fifo communication
impossible resulting in various kernel crashes.
Instead of unexpectedly crashing, predictably fail to load the driver
which will preserve the system.
Fixes: fb1d9738ca05 ("drm/vmwgfx: Add DRM driver for VMware Virtual GPU")
Signed-off-by: Zack Rusin
Reviewed-by: Martin Krastev
---
drivers/gpu/drm/vmwgfx/vmwgfx_cmd.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_cmd.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_cmd.c
index 67db472d3493..a3bfbb6c3e14 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_cmd.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_cmd.c
@@ -145,6 +145,13 @@ struct vmw_fifo_state *vmw_fifo_create(struct vmw_private
*dev_priv)
(unsigned int) max,
(unsigned int) min,
(unsigned int) fifo->capabilities);
+
+ if (unlikely(min >= max)) {
+ drm_warn(&dev_priv->drm,
+"FIFO memory is not usable. Driver failed to
initialize.");
+ return ERR_PTR(-ENXIO);
+ }
+
return fifo;
}
--
2.32.0
[PATCH v2 6/6] drm/ttm: Clarify that the TTM_PL_SYSTEM buffers need to stay idle
TTM was designed to allow TTM_PL_SYSTEM buffer to be fenced but over the years the code that was meant to handle it was broken and new changes can not deal with buffers which have been placed in TTM_PL_SYSTEM buf do not remain idle. CPU buffers which need to be fenced and shared with accelerators should be placed in driver specific placements that can explicitly handle CPU/accelerator buffer fencing. Currently, apart, from things silently failing nothing is enforcing that requirement which means that it's easy for drivers and new developers to get this wrong. To avoid the confusion we can document this requirement and clarify the solution. This came up during a discussion on dri-devel: https://lore.kernel.org/dri-devel/232f45e9-8748-1243-09bf-56763e666...@amd.com Signed-off-by: Zack Rusin Cc: Christian König Cc: Thomas Hellström --- include/drm/ttm/ttm_placement.h | 10 ++ 1 file changed, 10 insertions(+) diff --git a/include/drm/ttm/ttm_placement.h b/include/drm/ttm/ttm_placement.h index 76d1b9119a2b..89dfb58ff199 100644 --- a/include/drm/ttm/ttm_placement.h +++ b/include/drm/ttm/ttm_placement.h @@ -35,6 +35,16 @@ /* * Memory regions for data placement. + * + * Due to the fact that TTM_PL_SYSTEM BO's can be accessed by the hardware + * and are not directly evictable they're handled slightly differently + * from other placements. The most important and driver visible side-effect + * of that is that TTM_PL_SYSTEM BO's are not allowed to be fenced and have + * to remain idle. For BO's which reside in system memory but for which + * the accelerator requires direct access (i.e. their usage needs to be + * synchronized between the CPU and accelerator via fences) a new, driver + * private placement should be introduced that can handle such scenarios. + * */ #define TTM_PL_SYSTEM 0 -- 2.32.0
[PATCH v2 5/6] drm/vmwgfx: Switch the internal BO's to ttm_bo_type_kernel
There's never a need to access our internal kernel bo's from user-space. Those objects are used exclusively for internal support to guest backed surfaces (in otable setup and mob page tables) and there's no need to have them be of device type, i.e. mmappable from user-space. Signed-off-by: Zack Rusin Reviewed-by: Martin Krastev --- drivers/gpu/drm/vmwgfx/vmwgfx_bo.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c index fd007f1c1776..c97a3d5e90ce 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c @@ -494,7 +494,7 @@ int vmw_bo_create_kernel(struct vmw_private *dev_priv, unsigned long size, drm_vma_node_reset(&bo->base.vma_node); ret = ttm_bo_init_reserved(&dev_priv->bdev, bo, size, - ttm_bo_type_device, placement, 0, + ttm_bo_type_kernel, placement, 0, &ctx, NULL, NULL, NULL); if (unlikely(ret)) goto error_account; -- 2.32.0
[PATCH v2 4/6] drm/vmwgfx: Introduce a new placement for MOB page tables
For larger (bigger than a page) and noncontiguous mobs we have
to create page tables that allow the host to find the memory.
Those page tables just used regular system memory. Unfortunately
in TTM those BO's are not allowed to be busy thus can't be
fenced and we have to fence those bo's because we don't want
to destroy the page tables while the host is still executing
the command buffers which might be accessing them.
To solve it we introduce a new placement VMW_PL_SYSTEM which
is very similar to TTM_PL_SYSTEM except that it allows
fencing. This fixes kernel oops'es during unloading of the driver
(and pci hot remove/add) which were caused by busy BO's in
TTM_PL_SYSTEM being present in the delayed deletion list in
TTM (TTM_PL_SYSTEM manager is destroyed before the delayed
deletions are executed)
Signed-off-by: Zack Rusin
Reviewed-by: Martin Krastev
Cc: Christian König
Cc: Thomas Hellström
---
drivers/gpu/drm/vmwgfx/Makefile | 2 +-
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 14 ++-
drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 12 ++-
.../gpu/drm/vmwgfx/vmwgfx_system_manager.c| 90 +++
drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c| 58 ++--
5 files changed, 138 insertions(+), 38 deletions(-)
create mode 100644 drivers/gpu/drm/vmwgfx/vmwgfx_system_manager.c
diff --git a/drivers/gpu/drm/vmwgfx/Makefile b/drivers/gpu/drm/vmwgfx/Makefile
index bc323f7d4032..0188a312c38c 100644
--- a/drivers/gpu/drm/vmwgfx/Makefile
+++ b/drivers/gpu/drm/vmwgfx/Makefile
@@ -9,7 +9,7 @@ vmwgfx-y := vmwgfx_execbuf.o vmwgfx_gmr.o vmwgfx_kms.o
vmwgfx_drv.o \
vmwgfx_cotable.o vmwgfx_so.o vmwgfx_binding.o vmwgfx_msg.o \
vmwgfx_simple_resource.o vmwgfx_va.o vmwgfx_blit.o \
vmwgfx_validation.o vmwgfx_page_dirty.o vmwgfx_streamoutput.o \
-vmwgfx_devcaps.o ttm_object.o ttm_memory.o
+ vmwgfx_devcaps.o ttm_object.o ttm_memory.o vmwgfx_system_manager.o
vmwgfx-$(CONFIG_DRM_FBDEV_EMULATION) += vmwgfx_fb.o
vmwgfx-$(CONFIG_TRANSPARENT_HUGEPAGE) += vmwgfx_thp.o
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index 8d0b083ba267..daf65615308a 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -1071,6 +1071,12 @@ static int vmw_driver_load(struct vmw_private *dev_priv,
u32 pci_id)
"3D will be disabled.\n");
dev_priv->has_mob = false;
}
+ if (vmw_sys_man_init(dev_priv) != 0) {
+ drm_info(&dev_priv->drm,
+"No MOB page table memory available. "
+"3D will be disabled.\n");
+ dev_priv->has_mob = false;
+ }
}
if (dev_priv->has_mob && (dev_priv->capabilities & SVGA_CAP_DX)) {
@@ -1121,8 +1127,10 @@ static int vmw_driver_load(struct vmw_private *dev_priv,
u32 pci_id)
vmw_overlay_close(dev_priv);
vmw_kms_close(dev_priv);
out_no_kms:
- if (dev_priv->has_mob)
+ if (dev_priv->has_mob) {
vmw_gmrid_man_fini(dev_priv, VMW_PL_MOB);
+ vmw_sys_man_fini(dev_priv);
+ }
if (dev_priv->has_gmr)
vmw_gmrid_man_fini(dev_priv, VMW_PL_GMR);
vmw_devcaps_destroy(dev_priv);
@@ -1172,8 +1180,10 @@ static void vmw_driver_unload(struct drm_device *dev)
vmw_gmrid_man_fini(dev_priv, VMW_PL_GMR);
vmw_release_device_early(dev_priv);
- if (dev_priv->has_mob)
+ if (dev_priv->has_mob) {
vmw_gmrid_man_fini(dev_priv, VMW_PL_MOB);
+ vmw_sys_man_fini(dev_priv);
+ }
vmw_devcaps_destroy(dev_priv);
vmw_vram_manager_fini(dev_priv);
ttm_device_fini(&dev_priv->bdev);
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
index a833751099b5..df19dfb3ce18 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
@@ -82,8 +82,9 @@
VMWGFX_NUM_GB_SURFACE +\
VMWGFX_NUM_GB_SCREEN_TARGET)
-#define VMW_PL_GMR (TTM_PL_PRIV + 0)
-#define VMW_PL_MOB (TTM_PL_PRIV + 1)
+#define VMW_PL_GMR (TTM_PL_PRIV + 0)
+#define VMW_PL_MOB (TTM_PL_PRIV + 1)
+#define VMW_PL_SYSTEM (TTM_PL_PRIV + 2)
#define VMW_RES_CONTEXT ttm_driver_type0
#define VMW_RES_SURFACE ttm_driver_type1
@@ -1039,7 +1040,6 @@ extern struct ttm_placement vmw_vram_placement;
extern struct ttm_placement vmw_vram_sys_placement;
extern struct ttm_placement vmw_vram_gmr_placement;
extern struct ttm_placement vmw_sys_placement;
-extern struct ttm_placement vmw_evictable_placement;
extern struct ttm_placement vmw_srf_placement;
extern struct ttm_placement vmw_mob_placement;
extern struct ttm_placement vmw_nonfixed_placement;
@@ -1251,6 +1251,12 @@ int vmw_overlay_num_free_overlays(struct vmw_private
*dev_pr
[PATCH v2] drm/msm/devfreq: Fix OPP refcnt leak
From: Rob Clark
Reported-by: Douglas Anderson
Fixes: 9bc95570175a ("drm/msm: Devfreq tuning")
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/msm_gpu_devfreq.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/msm/msm_gpu_devfreq.c
b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
index d32b729b4616..07f1169df89b 100644
--- a/drivers/gpu/drm/msm/msm_gpu_devfreq.c
+++ b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
@@ -20,6 +20,10 @@ static int msm_devfreq_target(struct device *dev, unsigned
long *freq,
struct msm_gpu *gpu = dev_to_gpu(dev);
struct dev_pm_opp *opp;
+ /*
+* Note that devfreq_recommended_opp() can modify the freq
+* to something that actually is in the opp table:
+*/
opp = devfreq_recommended_opp(dev, freq, flags);
/*
@@ -28,6 +32,7 @@ static int msm_devfreq_target(struct device *dev, unsigned
long *freq,
*/
if (gpu->devfreq.idle_freq) {
gpu->devfreq.idle_freq = *freq;
+ dev_pm_opp_put(opp);
return 0;
}
--
2.31.1
[PATCH] staging: fbtft: Remove fb_watterott driver
This driver was made for a prototype and as far as I know it never went
into production because it was too slow. So let's remove it.
Signed-off-by: Noralf Trønnes
---
drivers/staging/fbtft/Kconfig| 6 -
drivers/staging/fbtft/Makefile | 1 -
drivers/staging/fbtft/fb_watterott.c | 302 ---
3 files changed, 309 deletions(-)
delete mode 100644 drivers/staging/fbtft/fb_watterott.c
diff --git a/drivers/staging/fbtft/Kconfig b/drivers/staging/fbtft/Kconfig
index dad1ddcd7b0c..4d29e8c1014e 100644
--- a/drivers/staging/fbtft/Kconfig
+++ b/drivers/staging/fbtft/Kconfig
@@ -200,9 +200,3 @@ config FB_TFT_UPD161704
depends on FB_TFT
help
Generic Framebuffer support for uPD161704
-
-config FB_TFT_WATTEROTT
- tristate "FB driver for the WATTEROTT LCD Controller"
- depends on FB_TFT
- help
- Generic Framebuffer support for WATTEROTT
diff --git a/drivers/staging/fbtft/Makefile b/drivers/staging/fbtft/Makefile
index e87193f7df14..e9cdf0f0a7da 100644
--- a/drivers/staging/fbtft/Makefile
+++ b/drivers/staging/fbtft/Makefile
@@ -36,4 +36,3 @@ obj-$(CONFIG_FB_TFT_TLS8204) += fb_tls8204.o
obj-$(CONFIG_FB_TFT_UC1611) += fb_uc1611.o
obj-$(CONFIG_FB_TFT_UC1701) += fb_uc1701.o
obj-$(CONFIG_FB_TFT_UPD161704) += fb_upd161704.o
-obj-$(CONFIG_FB_TFT_WATTEROTT) += fb_watterott.o
diff --git a/drivers/staging/fbtft/fb_watterott.c
b/drivers/staging/fbtft/fb_watterott.c
deleted file mode 100644
index a57e1f4feef3..
--- a/drivers/staging/fbtft/fb_watterott.c
+++ /dev/null
@@ -1,302 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0+
-/*
- * FB driver for the Watterott LCD Controller
- *
- * Copyright (C) 2013 Noralf Tronnes
- */
-
-#include
-#include
-#include
-#include
-
-#include "fbtft.h"
-
-#define DRVNAME"fb_watterott"
-#define WIDTH 320
-#define HEIGHT 240
-#define FPS5
-#define TXBUFLEN 1024
-#define DEFAULT_BRIGHTNESS 50
-
-#define CMD_VERSION0x01
-#define CMD_LCD_LED0x10
-#define CMD_LCD_RESET 0x11
-#define CMD_LCD_ORIENTATION0x20
-#define CMD_LCD_DRAWIMAGE 0x27
-#define COLOR_RGB323 8
-#define COLOR_RGB332 9
-#define COLOR_RGB233 10
-#define COLOR_RGB565 16
-
-static short mode = 565;
-module_param(mode, short, );
-MODULE_PARM_DESC(mode, "RGB color transfer mode: 332, 565 (default)");
-
-static void write_reg8_bus8(struct fbtft_par *par, int len, ...)
-{
- va_list args;
- int i, ret;
- u8 *buf = par->buf;
-
- va_start(args, len);
- for (i = 0; i < len; i++)
- *buf++ = (u8)va_arg(args, unsigned int);
- va_end(args);
-
- fbtft_par_dbg_hex(DEBUG_WRITE_REGISTER, par,
- par->info->device, u8, par->buf,
- len, "%s: ", __func__);
-
- ret = par->fbtftops.write(par, par->buf, len);
- if (ret < 0) {
- dev_err(par->info->device,
- "write() failed and returned %d\n", ret);
- return;
- }
-}
-
-static int write_vmem(struct fbtft_par *par, size_t offset, size_t len)
-{
- unsigned int start_line, end_line;
- u16 *vmem16 = (u16 *)(par->info->screen_buffer + offset);
- __be16 *pos = par->txbuf.buf + 1;
- __be16 *buf16 = par->txbuf.buf + 10;
- int i, j;
- int ret = 0;
-
- start_line = offset / par->info->fix.line_length;
- end_line = start_line + (len / par->info->fix.line_length) - 1;
-
- /* Set command header. pos: x, y, w, h */
- ((u8 *)par->txbuf.buf)[0] = CMD_LCD_DRAWIMAGE;
- pos[0] = 0;
- pos[2] = cpu_to_be16(par->info->var.xres);
- pos[3] = cpu_to_be16(1);
- ((u8 *)par->txbuf.buf)[9] = COLOR_RGB565;
-
- for (i = start_line; i <= end_line; i++) {
- pos[1] = cpu_to_be16(i);
- for (j = 0; j < par->info->var.xres; j++)
- buf16[j] = cpu_to_be16(*vmem16++);
- ret = par->fbtftops.write(par,
- par->txbuf.buf, 10 + par->info->fix.line_length);
- if (ret < 0)
- return ret;
- udelay(300);
- }
-
- return 0;
-}
-
-static inline int rgb565_to_rgb332(u16 c)
-{
- return ((c & 0xE000) >> 8) | ((c & 000700) >> 6) | ((c & 0x0018) >> 3);
-}
-
-static int write_vmem_8bit(struct fbtft_par *par, size_t offset, size_t len)
-{
- unsigned int start_line, end_line;
- u16 *vmem16 = (u16 *)(par->info->screen_buffer + offset);
- __be16 *pos = par->txbuf.buf + 1;
- u8 *buf8 = par->txbuf.buf + 10;
- int i, j;
- int ret = 0;
-
- start_line = offset / par->info->fix.line_length;
- end_line = start_line + (len / par->info->fix.line_length) - 1;
-
- /* Set command header. pos: x, y, w, h */
- ((u8 *)par->tx
Re: [PATCH v5 3/5] drm/dp: Don't read back backlight mode in drm_edp_backlight_enable()
Hi,
On Fri, Nov 5, 2021 at 11:34 AM Lyude Paul wrote:
>
> As it turns out, apparently some machines will actually leave additional
> backlight functionality like dynamic backlight control on before the OS
> loads. Currently we don't take care to disable unsupported features when
> writing back the backlight mode, which can lead to some rather strange
> looking behavior when adjusting the backlight.
>
> So, let's fix this by just not reading back the current backlight mode on
> initial enable. I don't think there should really be any downsides to this,
> and this will ensure we don't leave any unsupported functionality enabled.
>
> This should fix at least one (but not all) of the issues seen with DPCD
> backlight support on fi-bdw-samus
>
> v5:
> * Just avoid reading back DPCD register - Doug Anderson
>
> Signed-off-by: Lyude Paul
> Fixes: 867cf9cd73c3 ("drm/dp: Extract i915's eDP backlight code into DRM
> helpers")
> ---
> drivers/gpu/drm/drm_dp_helper.c | 40 ++---
> 1 file changed, 12 insertions(+), 28 deletions(-)
You forgot to CC me on this one! ;-)
This looks good to me now, so FWIW:
Reviewed-by: Douglas Anderson
Re: [PATCH v2] drm/msm/devfreq: Fix OPP refcnt leak
Hi,
On Fri, Nov 5, 2021 at 1:15 PM Rob Clark wrote:
>
> From: Rob Clark
>
> Reported-by: Douglas Anderson
> Fixes: 9bc95570175a ("drm/msm: Devfreq tuning")
> Signed-off-by: Rob Clark
> ---
> drivers/gpu/drm/msm/msm_gpu_devfreq.c | 5 +
> 1 file changed, 5 insertions(+)
Reviewed-by: Douglas Anderson
Re: [PATCH] staging/fbtft: Fix backlight
Den 30.10.2021 18.29, skrev Noralf Trønnes:
> Commit b4a1ed0cd18b ("fbdev: make FB_BACKLIGHT a tristate") forgot to
> update fbtft breaking its backlight support when FB_BACKLIGHT is a module.
>
> Fix this by using IS_ENABLED().
>
> Fixes: b4a1ed0cd18b ("fbdev: make FB_BACKLIGHT a tristate")
> Cc:
> Signed-off-by: Noralf Trønnes
> ---
I discovered that fb_ssd1351 also has this #ifdef, so need to fix that
as well. Will send a new version.
Noralf.
Re: [PATCH v2] drm/msm/devfreq: Fix OPP refcnt leak
On 11/5/21 3:20 PM, Rob Clark wrote:
From: Rob Clark
Reported-by: Douglas Anderson
Fixes: 9bc95570175a ("drm/msm: Devfreq tuning")
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/msm_gpu_devfreq.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/msm/msm_gpu_devfreq.c
b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
index d32b729b4616..07f1169df89b 100644
--- a/drivers/gpu/drm/msm/msm_gpu_devfreq.c
+++ b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
@@ -20,6 +20,10 @@ static int msm_devfreq_target(struct device *dev, unsigned
long *freq,
struct msm_gpu *gpu = dev_to_gpu(dev);
struct dev_pm_opp *opp;
+ /*
+* Note that devfreq_recommended_opp() can modify the freq
+* to something that actually is in the opp table:
+*/
opp = devfreq_recommended_opp(dev, freq, flags);
/*
@@ -28,6 +32,7 @@ static int msm_devfreq_target(struct device *dev, unsigned
long *freq,
*/
if (gpu->devfreq.idle_freq) {
gpu->devfreq.idle_freq = *freq;
+ dev_pm_opp_put(opp);
return 0;
}
Tested on the Lenovo Yoga C630 and don't see the message from v1 :D
Tested-By: Steev Klimaszewski
[PATCH v2] staging/fbtft: Fix backlight
Commit b4a1ed0cd18b ("fbdev: make FB_BACKLIGHT a tristate") forgot to
update fbtft breaking its backlight support when FB_BACKLIGHT is a module.
Since FB_TFT selects FB_BACKLIGHT there's no need for this conditional
so just remove it and we're good.
Fixes: b4a1ed0cd18b ("fbdev: make FB_BACKLIGHT a tristate")
Cc:
Signed-off-by: Noralf Trønnes
---
Changes since v1:
- No need for the #ifdef at all since FB_BACKLIGHT is always selected
- Fix fb_ssd1351 as well
fb_watterott has the same problem, but I've sent a removal patch for that one.
drivers/staging/fbtft/fb_ssd1351.c | 4
drivers/staging/fbtft/fbtft-core.c | 9 +
2 files changed, 1 insertion(+), 12 deletions(-)
diff --git a/drivers/staging/fbtft/fb_ssd1351.c
b/drivers/staging/fbtft/fb_ssd1351.c
index cf263a58a148..6fd549a424d5 100644
--- a/drivers/staging/fbtft/fb_ssd1351.c
+++ b/drivers/staging/fbtft/fb_ssd1351.c
@@ -187,7 +187,6 @@ static struct fbtft_display display = {
},
};
-#ifdef CONFIG_FB_BACKLIGHT
static int update_onboard_backlight(struct backlight_device *bd)
{
struct fbtft_par *par = bl_get_data(bd);
@@ -231,9 +230,6 @@ static void register_onboard_backlight(struct fbtft_par
*par)
if (!par->fbtftops.unregister_backlight)
par->fbtftops.unregister_backlight = fbtft_unregister_backlight;
}
-#else
-static void register_onboard_backlight(struct fbtft_par *par) { };
-#endif
FBTFT_REGISTER_DRIVER(DRVNAME, "solomon,ssd1351", &display);
diff --git a/drivers/staging/fbtft/fbtft-core.c
b/drivers/staging/fbtft/fbtft-core.c
index ed992ca605eb..1690358b8f01 100644
--- a/drivers/staging/fbtft/fbtft-core.c
+++ b/drivers/staging/fbtft/fbtft-core.c
@@ -128,7 +128,6 @@ static int fbtft_request_gpios(struct fbtft_par *par)
return 0;
}
-#ifdef CONFIG_FB_BACKLIGHT
static int fbtft_backlight_update_status(struct backlight_device *bd)
{
struct fbtft_par *par = bl_get_data(bd);
@@ -161,6 +160,7 @@ void fbtft_unregister_backlight(struct fbtft_par *par)
par->info->bl_dev = NULL;
}
}
+EXPORT_SYMBOL(fbtft_unregister_backlight);
static const struct backlight_ops fbtft_bl_ops = {
.get_brightness = fbtft_backlight_get_brightness,
@@ -198,12 +198,7 @@ void fbtft_register_backlight(struct fbtft_par *par)
if (!par->fbtftops.unregister_backlight)
par->fbtftops.unregister_backlight = fbtft_unregister_backlight;
}
-#else
-void fbtft_register_backlight(struct fbtft_par *par) { };
-void fbtft_unregister_backlight(struct fbtft_par *par) { };
-#endif
EXPORT_SYMBOL(fbtft_register_backlight);
-EXPORT_SYMBOL(fbtft_unregister_backlight);
static void fbtft_set_addr_win(struct fbtft_par *par, int xs, int ys, int xe,
int ye)
@@ -853,13 +848,11 @@ int fbtft_register_framebuffer(struct fb_info *fb_info)
fb_info->fix.smem_len >> 10, text1,
HZ / fb_info->fbdefio->delay, text2);
-#ifdef CONFIG_FB_BACKLIGHT
/* Turn on backlight if available */
if (fb_info->bl_dev) {
fb_info->bl_dev->props.power = FB_BLANK_UNBLANK;
fb_info->bl_dev->ops->update_status(fb_info->bl_dev);
}
-#endif
return 0;
--
2.33.0
Re: [PATCH 1/2] drm/atomic: document and enforce rules around "spurious" EBUSY
Hi Daniel,
Just got bitten by this warning when trying to do some refactoring in
amdgpu for trying to get rid of the DRM private object we use for our DC
state.
From a userspace perspective I understand that we want to avoid judder,
-EBUSY and other issues affecting the compositor from kernel having to
drag these CRTCs (or their planes) into the atomic state.
For bandwidth validation we need to understand the state of all CRTCs
and planes in use. Existing driver code maintains this as part of a
global state object in a DRM private atomic state. We have stalls in
atomic check (bad) to avoid freeing this state or modifying it at the
wrong times which avoid hitting this warning but essentially cause the
same judder issue.
While most hardware has independent pipes, I think almost all hardware
ends up having the memory interface/bandwidth as a global shared
resource that software state can't really abstract around.
There are cases where we know that there will be no (or minimal) impact
to the overall memory requirements for particular DRM updates. Our
validation already "over-allocates" for common display changes - page
flips, some format changes, cursor enable/disable. But for most cases
outside of that we do want to pull in _all_ the CRTCs and planes.
On our HW you won't get a blankout unless you're actually modifying a
stream timing itself so I think the ALLOW_MODESET flag is overkill here.
Rejecting the commit when the flag isn't set also ends up breaking
userspace in the process since it expects commits like pageflips between
different tiling modes to succeed with the legacy IOCTLs.
Any ideas about this? I missed the IRC discussion regarding this before
so I'm not sure if we have any alternatives that were dropped in favor
of this.
Regards,
Nicholas Kazlauskas
On 2020-09-25 4:46 a.m., Daniel Vetter wrote:
When doing an atomic modeset with ALLOW_MODESET drivers are allowed to
pull in arbitrary other resources, including CRTCs (e.g. when
reconfiguring global resources).
But in nonblocking mode userspace has then no idea this happened,
which can lead to spurious EBUSY calls, both:
- when that other CRTC is currently busy doing a page_flip the
ALLOW_MODESET commit can fail with an EBUSY
- on the other CRTC a normal atomic flip can fail with EBUSY because
of the additional commit inserted by the kernel without userspace's
knowledge
For blocking commits this isn't a problem, because everyone else will
just block until all the CRTC are reconfigured. Only thing userspace
can notice is the dropped frames without any reason for why frames got
dropped.
Consensus is that we need new uapi to handle this properly, but no one
has any idea what exactly the new uapi should look like. Since this
has been shipping for years already compositors need to deal no matter
what, so as a first step just try to enforce this across drivers
better with some checks.
v2: Add comments and a WARN_ON to enforce this only when allowed - we
don't want to silently convert page flips into blocking plane updates
just because the driver is buggy.
v3: Fix inverted WARN_ON (Pekka).
v4: Drop the uapi changes, only add a WARN_ON for now to enforce some
rules for drivers.
v5: Make the WARNING more informative (Daniel)
v6: Add unconditional debug output for compositor hackers to figure
out what's going on when they get an EBUSY (Daniel)
v7: Fix up old/new_crtc_state confusion for real (Pekka/Ville)
References:
https://lists.freedesktop.org/archives/dri-devel/2018-July/182281.html
Bugzilla: https://gitlab.freedesktop.org/wayland/weston/-/issues/24#note_9568
Cc: Daniel Stone
Cc: Pekka Paalanen
Cc: Simon Ser
Cc: Ville Syrjälä
Signed-off-by: Daniel Vetter
---
drivers/gpu/drm/drm_atomic.c | 29 +
1 file changed, 29 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 58527f151984..aac9122f1da2 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -281,6 +281,10 @@ EXPORT_SYMBOL(__drm_atomic_state_free);
* needed. It will also grab the relevant CRTC lock to make sure that the
state
* is consistent.
*
+ * WARNING: Drivers may only add new CRTC states to a @state if
+ * drm_atomic_state.allow_modeset is set, or if it's a driver-internal commit
+ * not created by userspace through an IOCTL call.
+ *
* Returns:
*
* Either the allocated state or the error code encoded into the pointer. When
@@ -1262,10 +1266,15 @@ int drm_atomic_check_only(struct drm_atomic_state
*state)
struct drm_crtc_state *new_crtc_state;
struct drm_connector *conn;
struct drm_connector_state *conn_state;
+ unsigned requested_crtc = 0;
+ unsigned affected_crtc = 0;
int i, ret = 0;
DRM_DEBUG_ATOMIC("checking %p\n", state);
+ for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
+ requested_crtc |= drm_crtc_mask(crtc);
+
for_each_oldne
Re: [PATCH v2 1/9] drm/format-helper: Export drm_fb_clip_offset()
Den 01.11.2021 15.15, skrev Thomas Zimmermann: > Provide a function that computes the offset into a blit destination > buffer. This will allow to move destination-buffer clipping into the > format-helper callers. > > v2: > * provide documentation (Sam) > * return 'unsigned int' (Sam, Noralf) > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Noralf Trønnes
Re: [PATCH 2/2] drm/msm/gpu: Respect PM QoS constraints
Hi,
On Wed, Nov 3, 2021 at 1:59 PM Rob Clark wrote:
>
> From: Rob Clark
>
> Signed-off-by: Rob Clark
> ---
> drivers/gpu/drm/msm/msm_gpu_devfreq.c | 31 +--
> 1 file changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> index b24e5475cafb..427c55002f4d 100644
> --- a/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> +++ b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> @@ -158,6 +158,33 @@ void msm_devfreq_suspend(struct msm_gpu *gpu)
> devfreq_suspend_device(gpu->devfreq.devfreq);
> }
>
> +static void set_target(struct msm_gpu *gpu, unsigned long freq)
> +{
> + struct msm_gpu_devfreq *df = &gpu->devfreq;
> + unsigned long min_freq, max_freq;
> + u32 flags = 0;
> +
> + /*
> +* When setting the target freq internally, we need to apply PM QoS
> +* constraints (such as cooling):
> +*/
> + min_freq = dev_pm_qos_read_value(df->devfreq->dev.parent,
> +DEV_PM_QOS_MIN_FREQUENCY);
Chatted with Rob offline about this, but to document on the lists for
those playing at home: the above function isn't exported to modules,
so this will fail with "allmodconfig".
In general this isn't the right approach here. I believe that the
right approach is to boost with freq_qos_update_request() and then
kick off a timer to stop boosting after a fixed period of time.
-Doug
Re: [PATCH v2] drm/msm/devfreq: Fix OPP refcnt leak
On 11/6/2021 1:50 AM, Rob Clark wrote:
From: Rob Clark
Reported-by: Douglas Anderson
Fixes: 9bc95570175a ("drm/msm: Devfreq tuning")
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/msm_gpu_devfreq.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/msm/msm_gpu_devfreq.c
b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
index d32b729b4616..07f1169df89b 100644
--- a/drivers/gpu/drm/msm/msm_gpu_devfreq.c
+++ b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
@@ -20,6 +20,10 @@ static int msm_devfreq_target(struct device *dev, unsigned
long *freq,
struct msm_gpu *gpu = dev_to_gpu(dev);
struct dev_pm_opp *opp;
+ /*
+* Note that devfreq_recommended_opp() can modify the freq
+* to something that actually is in the opp table:
+*/
opp = devfreq_recommended_opp(dev, freq, flags);
/*
@@ -28,6 +32,7 @@ static int msm_devfreq_target(struct device *dev, unsigned
long *freq,
*/
if (gpu->devfreq.idle_freq) {
gpu->devfreq.idle_freq = *freq;
+ dev_pm_opp_put(opp);
return 0;
}
Reviewed-by: Akhil P Oommen
-Akhil
Re: [PATCH] staging: fbtft: Remove fb_watterott driver
On Fri, Nov 05, 2021 at 09:24:48PM +0100, Noralf Trønnes wrote: > This driver was made for a prototype and as far as I know it never went > into production because it was too slow. So let's remove it. > > Signed-off-by: Noralf Trønnes Acked-by: Sam Ravnborg
Re: [PATCH v2] staging/fbtft: Fix backlight
On Fri, Nov 05, 2021 at 09:43:58PM +0100, Noralf Trønnes wrote:
> Commit b4a1ed0cd18b ("fbdev: make FB_BACKLIGHT a tristate") forgot to
> update fbtft breaking its backlight support when FB_BACKLIGHT is a module.
>
> Since FB_TFT selects FB_BACKLIGHT there's no need for this conditional
> so just remove it and we're good.
>
> Fixes: b4a1ed0cd18b ("fbdev: make FB_BACKLIGHT a tristate")
> Cc:
> Signed-off-by: Noralf Trønnes
Acked-by: Sam Ravnborg
Re: [PATCH] drm/msm/dp: do not initialize phy until plugin interrupt received
Quoting Bjorn Andersson (2021-11-05 10:48:37) > On Fri 05 Nov 10:28 PDT 2021, Kuogee Hsieh wrote: > > > From: Kuogee Hsieh > > > > Combo phy supports both USB and DP simultaneously. There may has a > > possible conflict during phy initialization phase between USB and > > DP driver which may cause USB phy timeout when USB tries to power > > up its phy. This patch has the DP driver not initialize its phy > > during DP driver initialization phase. Instead DP driver only enable > > required regulators and clocks so that it is able to receive HPD > > interrupts after completion of initialization phase. DP driver will > > initialize its phy when HPD plug-in interrupt received. > > Is this a hardware requirement, or is this a issue with the current > implementation of the QMP combo phy driver? We should not hack up the DP > driver to circumvent the latter. > > Also, I don't suppose there's anything here that prevents the HPD to > come before the USB PHY is powered up? Even though that seems less > likely in practice... > > > This patch also provides a positive side effects which balance regulator > > enable count since regulator only enabled at initialize phase and resume > > and disabled at followed suspend. > > > > Is this something that needs to be fixed separately, so that it can be > backported to stable kernels? Agreed. Please split the regulator balance problem from the DP initialization delay.
Re: [PATCH] drm/msm/dp: do not initialize phy until plugin interrupt received
Quoting Kuogee Hsieh (2021-11-05 10:28:11)
> From: Kuogee Hsieh
>
> Combo phy supports both USB and DP simultaneously. There may has a
> possible conflict during phy initialization phase between USB and
> DP driver which may cause USB phy timeout when USB tries to power
> up its phy. This patch has the DP driver not initialize its phy
> during DP driver initialization phase. Instead DP driver only enable
> required regulators and clocks so that it is able to receive HPD
> interrupts after completion of initialization phase. DP driver will
> initialize its phy when HPD plug-in interrupt received.
> This patch also provides a positive side effects which balance regulator
> enable count since regulator only enabled at initialize phase and resume
> and disabled at followed suspend.
>
Any Fixes: tag?
> Signed-off-by: Kuogee Hsieh
> ---
> drivers/gpu/drm/msm/dp/dp_ctrl.c| 97
> ++---
> drivers/gpu/drm/msm/dp/dp_ctrl.h| 9 ++--
> drivers/gpu/drm/msm/dp/dp_display.c | 71 ---
> 3 files changed, 119 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 7ec155d..e0e5dc9 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -1364,7 +1364,41 @@ static int dp_ctrl_enable_stream_clocks(struct
> dp_ctrl_private *ctrl)
> return ret;
> }
>
> -int dp_ctrl_host_init(struct dp_ctrl *dp_ctrl, bool flip, bool reset)
> +void dp_ctrl_irq_enable(struct dp_ctrl *dp_ctrl, bool flip)
> +{
> + struct dp_ctrl_private *ctrl;
> +
> + if (!dp_ctrl) {
> + DRM_ERROR("Invalid input data\n");
> + return;
> + }
> +
> + ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
> +
> + ctrl->dp_ctrl.orientation = flip;
> +
> + dp_catalog_ctrl_reset(ctrl->catalog);
> +
> + dp_catalog_ctrl_enable_irq(ctrl->catalog, true);
> +}
> +
> +void dp_ctrl_irq_disable(struct dp_ctrl *dp_ctrl)
> +{
> + struct dp_ctrl_private *ctrl;
> +
> + if (!dp_ctrl) {
Please lets stop adding NULL pointer checks. They should be pushed as
high as possible in the call chain so that we don't need them down deep
in the driver.
> + DRM_ERROR("Invalid input data\n");
> + return;
> + }
> +
> + ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
> +
> + dp_catalog_ctrl_reset(ctrl->catalog);
> +
> + dp_catalog_ctrl_enable_irq(ctrl->catalog, false);
> +}
> +
> +void dp_ctrl_phy_init(struct dp_ctrl *dp_ctrl)
> {
> struct dp_ctrl_private *ctrl;
> struct dp_io *dp_io;
> @@ -1372,34 +1406,24 @@ int dp_ctrl_host_init(struct dp_ctrl *dp_ctrl, bool
> flip, bool reset)
>
> if (!dp_ctrl) {
> DRM_ERROR("Invalid input data\n");
> - return -EINVAL;
> + return;
> }
>
> ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
> dp_io = &ctrl->parser->io;
> phy = dp_io->phy;
>
> - ctrl->dp_ctrl.orientation = flip;
> -
> - if (reset)
> - dp_catalog_ctrl_reset(ctrl->catalog);
> + DRM_DEBUG_DP("Before, phy=%x init_count=%d power_on=%d\n",
> + (u32)(uintptr_t)phy, phy->init_count, phy->power_count);
Use %p instead of casting to a u32.
>
> - DRM_DEBUG_DP("flip=%d\n", flip);
> dp_catalog_ctrl_phy_reset(ctrl->catalog);
> phy_init(phy);
> - dp_catalog_ctrl_enable_irq(ctrl->catalog, true);
>
> - return 0;
> + DRM_DEBUG_DP("After, phy=%x init_count=%d power_on=%d\n",
> + (u32)(uintptr_t)phy, phy->init_count, phy->power_count);
> }
>
> -/**
> - * dp_ctrl_host_deinit() - Uninitialize DP controller
> - * @dp_ctrl: Display Port Driver data
> - *
> - * Perform required steps to uninitialize DP controller
> - * and its resources.
> - */
> -void dp_ctrl_host_deinit(struct dp_ctrl *dp_ctrl)
> +void dp_ctrl_phy_exit(struct dp_ctrl *dp_ctrl)
> {
> struct dp_ctrl_private *ctrl;
> struct dp_io *dp_io;
> @@ -1414,10 +1438,14 @@ void dp_ctrl_host_deinit(struct dp_ctrl *dp_ctrl)
> dp_io = &ctrl->parser->io;
> phy = dp_io->phy;
>
> - dp_catalog_ctrl_enable_irq(ctrl->catalog, false);
> + DRM_DEBUG_DP("Before, phy=%x init_count=%d power_on=%d\n",
> + (u32)(uintptr_t)phy, phy->init_count, phy->power_count);
> +
> + dp_catalog_ctrl_phy_reset(ctrl->catalog);
> phy_exit(phy);
>
> - DRM_DEBUG_DP("Host deinitialized successfully\n");
> + DRM_DEBUG_DP("After, phy=%x init_count=%d power_on=%d\n",
> + (u32)(uintptr_t)phy, phy->init_count, phy->power_count);
> }
>
> static bool dp_ctrl_use_fixed_nvid(struct dp_ctrl_private *ctrl)
> @@ -1895,8 +1923,14 @@ int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl)
> return ret;
> }
>
> + DRM_DEBUG_DP("Before, phy=%x
Re: linux-next: build failure after merge of the drm-misc tree
Hi Jani,
On Fri, 05 Nov 2021 13:03:43 +0200 Jani Nikula wrote:
>
> I probably should have pushed c4f08d7246a5 ("drm/locking: fix
> __stack_depot_* name conflict") to drm-misc-next-fixes.
Please do so as builds will start failing otherwise :-(
--
Cheers,
Stephen Rothwell
pgpAxSxKLNrL7.pgp
Description: OpenPGP digital signature
