[PATCH v6 6/7] drm/vkms: Add a module param to enable/disable the default device
From: Jim Shargo
In many testing circumstances, we will want to just create a new device
and test against that. If we create a default device, it can be annoying
to have to manually select the new device instead of choosing the only
one that exists.
The param, enable_default, is defaulted to true to maintain backwards
compatibility.
Signed-off-by: Jim Shargo
Signed-off-by: Brandon Pollack
---
drivers/gpu/drm/vkms/vkms_drv.c | 45 ++---
1 file changed, 30 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index 6e7f20681890..293bebf8e8ce 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -42,17 +42,26 @@
#define DRIVER_MAJOR 1
#define DRIVER_MINOR 0
+static bool enable_default_device = true;
+module_param_named(enable_default_device, enable_default_device, bool, 0444);
+MODULE_PARM_DESC(enable_default_device,
+"Enable/Disable creating the default device");
+
static bool enable_cursor = true;
module_param_named(enable_cursor, enable_cursor, bool, 0444);
-MODULE_PARM_DESC(enable_cursor, "Enable/Disable cursor support");
+MODULE_PARM_DESC(enable_cursor,
+"Enable/Disable cursor support for the default device");
static bool enable_writeback = true;
module_param_named(enable_writeback, enable_writeback, bool, 0444);
-MODULE_PARM_DESC(enable_writeback, "Enable/Disable writeback connector
support");
+MODULE_PARM_DESC(
+ enable_writeback,
+ "Enable/Disable writeback connector support for the default device");
static bool enable_overlay;
module_param_named(enable_overlay, enable_overlay, bool, 0444);
-MODULE_PARM_DESC(enable_overlay, "Enable/Disable overlay support");
+MODULE_PARM_DESC(enable_overlay,
+"Enable/Disable overlay support for the default device");
DEFINE_DRM_GEM_FOPS(vkms_driver_fops);
@@ -99,6 +108,7 @@ static int vkms_config_show(struct seq_file *m, void *data)
struct drm_device *dev = entry->dev;
struct vkms_device *vkmsdev = drm_device_to_vkms_device(dev);
+ seq_printf(m, "default_device=%d\n", enable_default_device);
seq_printf(m, "writeback=%d\n", vkmsdev->config.writeback);
seq_printf(m, "cursor=%d\n", vkmsdev->config.cursor);
seq_printf(m, "overlay=%d\n", vkmsdev->config.overlay);
@@ -297,10 +307,7 @@ void vkms_remove_device(struct vkms_device *vkms_device)
static int __init vkms_init(void)
{
int ret;
- struct platform_device *pdev;
- struct vkms_device_setup vkms_device_setup = {
- .configfs = NULL,
- };
+ struct platform_device *default_pdev = NULL;
ret = platform_driver_register(_platform_driver);
if (ret) {
@@ -308,19 +315,27 @@ static int __init vkms_init(void)
return ret;
}
- pdev = platform_device_register_data(NULL, DRIVER_NAME, 0,
-_device_setup,
-sizeof(vkms_device_setup));
- if (IS_ERR(pdev)) {
- DRM_ERROR("Unable to register default vkms device\n");
- platform_driver_unregister(_platform_driver);
- return PTR_ERR(pdev);
+ if (enable_default_device) {
+ struct vkms_device_setup vkms_device_setup = {
+ .configfs = NULL,
+ };
+
+ default_pdev = platform_device_register_data(
+ NULL, DRIVER_NAME, 0, _device_setup,
+ sizeof(vkms_device_setup));
+ if (IS_ERR(default_pdev)) {
+ DRM_ERROR("Unable to register default vkms device\n");
+ platform_driver_unregister(_platform_driver);
+ return PTR_ERR(default_pdev);
+ }
}
ret = vkms_init_configfs();
if (ret) {
DRM_ERROR("Unable to initialize configfs\n");
- platform_device_unregister(pdev);
+ if (default_pdev)
+ platform_device_unregister(default_pdev);
+
platform_driver_unregister(_platform_driver);
}
--
2.42.0.rc2.253.gd59a3bf2b4-goog
[PATCH v6 7/7] drm/vkms Add hotplug support via configfs to VKMS.
This change adds the ability to read or write a "1" or a "0" to the
newly added "connected" attribute of a connector in the vkms entry in
configfs.
A write will trigger a call to drm_kms_helper_hotplug_event, causing a
hotplug uevent.
With this we can write virtualized multidisplay tests that involve
hotplugging displays (eg recompositing windows when a monitor is turned
off).
Signed-off-by: Brandon Pollack
---
Documentation/gpu/vkms.rst | 2 +-
drivers/gpu/drm/vkms/vkms_configfs.c | 68 ++--
drivers/gpu/drm/vkms/vkms_drv.h | 11 +
drivers/gpu/drm/vkms/vkms_output.c | 47 ++-
4 files changed, 123 insertions(+), 5 deletions(-)
diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst
index c3875bf66dba..7f715097539c 100644
--- a/Documentation/gpu/vkms.rst
+++ b/Documentation/gpu/vkms.rst
@@ -145,7 +145,7 @@ We want to be able to manipulate vkms instances without
having to reload the
module. Such configuration can be added as extensions to vkms's ConfigFS
support. Use-cases:
-- Hotplug/hotremove connectors on the fly (to be able to test DP MST handling
+- Hotremove connectors on the fly (to be able to test DP MST handling
of compositors).
- Change output configuration: Plug/unplug screens, change EDID, allow changing
diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c
b/drivers/gpu/drm/vkms/vkms_configfs.c
index bc35dcc47585..d231e28101ae 100644
--- a/drivers/gpu/drm/vkms/vkms_configfs.c
+++ b/drivers/gpu/drm/vkms/vkms_configfs.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0+
+#include "drm/drm_probe_helper.h"
#include
#include
#include
@@ -40,6 +41,7 @@
* `-- vkms
* `-- test
* |-- connectors
+ *`-- connected
* |-- crtcs
* |-- encoders
* |-- planes
@@ -89,6 +91,14 @@
*
* echo 1 > /config/vkms/test/enabled
*
+ * By default no display is "connected" so to connect a connector you'll also
+ * have to write 1 to a connectors "connected" attribute::
+ *
+ * echo 1 > /config/vkms/test/connectors/connector/connected
+ *
+ * One can verify that this is worked using the `modetest` utility or the
+ * equivalent for your platform.
+ *
* When you're done with the virtual device, you can clean up the device like
* so::
*
@@ -236,7 +246,58 @@ static void add_possible_encoders(struct config_group
*parent,
/* Connector item, e.g. /config/vkms/device/connectors/ID */
+static ssize_t connector_connected_show(struct config_item *item, char *buf)
+{
+ struct vkms_config_connector *connector =
+ item_to_config_connector(item);
+ struct vkms_configfs *configfs = connector_item_to_configfs(item);
+ bool connected = false;
+
+ mutex_lock(>lock);
+ connected = connector->connected;
+ mutex_unlock(>lock);
+
+ return sprintf(buf, "%d\n", connected);
+}
+
+static ssize_t connector_connected_store(struct config_item *item,
+const char *buf, size_t len)
+{
+ struct vkms_config_connector *connector =
+ item_to_config_connector(item);
+ struct vkms_configfs *configfs = connector_item_to_configfs(item);
+ int val, ret;
+
+ ret = kstrtouint(buf, 10, );
+ if (ret)
+ return ret;
+
+ if (val != 1 && val != 0)
+ return -EINVAL;
+
+ mutex_lock(>lock);
+ connector->connected = val;
+ if (!connector->connector) {
+ pr_info("VKMS Device %s is not yet enabled, connector will be
enabled on start",
+ configfs->device_group.cg_item.ci_name);
+ }
+ mutex_unlock(>lock);
+
+ if (connector->connector)
+ drm_kms_helper_hotplug_event(connector->connector->dev);
+
+ return len;
+}
+
+CONFIGFS_ATTR(connector_, connected);
+
+static struct configfs_attribute *connector_attrs[] = {
+ _attr_connected,
+ NULL,
+};
+
static struct config_item_type connector_type = {
+ .ct_attrs = connector_attrs,
.ct_owner = THIS_MODULE,
};
@@ -264,7 +325,7 @@ static ssize_t plane_type_show(struct config_item *item,
char *buf)
plane_type = plane->type;
mutex_unlock(>lock);
- return sprintf(buf, "%u", plane_type);
+ return sprintf(buf, "%u\n", plane_type);
}
static ssize_t plane_type_store(struct config_item *item, const char *buf,
@@ -319,6 +380,7 @@ static struct config_group *connectors_group_make(struct
config_group *group,
_type);
add_possible_encoders(>config_group,
>possible_encoders.group);
+ connector->connected = false;
return >config_group;
}
@@ -500,7 +562,7 @@ static ssize_t device_enabled_show(struct config_item
*item, char *buf)
is_enabled = configfs->vkms_device != NULL;
mutex_unlock(>lock);
- return sprintf(buf, "%d",
[PATCH v6 3/7] drm/vkms: Provide platform data when creating VKMS devices
From: Jim Shargo
This is a small refactor to make ConfigFS support easier. This should be
a no-op refactor.
Signed-off-by: Jim Shargo
Signed-off-by: Brandon Pollack
---
drivers/gpu/drm/vkms/vkms_drv.c| 14 --
drivers/gpu/drm/vkms/vkms_drv.h| 9 ++---
drivers/gpu/drm/vkms/vkms_output.c | 2 +-
3 files changed, 19 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index 65b1e2c52106..6c94c2b5d529 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -9,6 +9,7 @@
* the GPU in DRM API tests.
*/
+#include "asm-generic/errno-base.h"
#include
#include
#include
@@ -171,12 +172,14 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
dev->mode_config.preferred_depth = 0;
dev->mode_config.helper_private = _mode_config_helpers;
- return vkms_output_init(vkmsdev, 0);
+ return vkmsdev->is_default ? vkms_output_init_default(vkmsdev) :
+-EINVAL;
}
static int vkms_platform_probe(struct platform_device *pdev)
{
int ret;
+ struct vkms_device_setup *vkms_device_setup = pdev->dev.platform_data;
struct vkms_device *vkms_device;
void *grp;
@@ -195,6 +198,7 @@ static int vkms_platform_probe(struct platform_device *pdev)
vkms_device->config.cursor = enable_cursor;
vkms_device->config.writeback = enable_writeback;
vkms_device->config.overlay = enable_overlay;
+ vkms_device->is_default = vkms_device_setup->is_default;
ret = dma_coerce_mask_and_coherent(vkms_device->drm.dev,
DMA_BIT_MASK(64));
@@ -258,6 +262,9 @@ static int __init vkms_init(void)
{
int ret;
struct platform_device *pdev;
+ struct vkms_device_setup vkms_device_setup = {
+ .is_default = true,
+ };
ret = platform_driver_register(_platform_driver);
if (ret) {
@@ -265,8 +272,11 @@ static int __init vkms_init(void)
return ret;
}
- pdev = platform_device_register_simple(DRIVER_NAME, -1, NULL, 0);
+ pdev = platform_device_register_data(NULL, DRIVER_NAME, 0,
+_device_setup,
+sizeof(vkms_device_setup));
if (IS_ERR(pdev)) {
+ DRM_ERROR("Unable to register default vkms device\n");
platform_driver_unregister(_platform_driver);
return PTR_ERR(pdev);
}
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 761cd809617e..4262dcffd7e1 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -132,17 +132,20 @@ struct vkms_output {
struct vkms_plane planes[VKMS_MAX_PLANES];
};
-struct vkms_device;
-
struct vkms_config {
bool writeback;
bool cursor;
bool overlay;
};
+struct vkms_device_setup {
+ bool is_default;
+};
+
struct vkms_device {
struct drm_device drm;
struct platform_device *platform;
+ bool is_default;
struct vkms_output output;
struct vkms_config config;
};
@@ -166,7 +169,7 @@ struct vkms_crtc *vkms_crtc_init(struct vkms_device
*vkmsdev,
struct drm_plane *primary,
struct drm_plane *cursor);
-int vkms_output_init(struct vkms_device *vkmsdev, int index);
+int vkms_output_init_default(struct vkms_device *vkmsdev);
struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
enum drm_plane_type type);
diff --git a/drivers/gpu/drm/vkms/vkms_output.c
b/drivers/gpu/drm/vkms/vkms_output.c
index 86faf94f7408..bfc2e2362c6d 100644
--- a/drivers/gpu/drm/vkms/vkms_output.c
+++ b/drivers/gpu/drm/vkms/vkms_output.c
@@ -80,7 +80,7 @@ static struct drm_encoder *vkms_encoder_init(struct
vkms_device *vkms_device)
return encoder;
}
-int vkms_output_init(struct vkms_device *vkmsdev, int index)
+int vkms_output_init_default(struct vkms_device *vkmsdev)
{
struct vkms_output *output = >output;
struct drm_device *dev = >drm;
--
2.42.0.rc2.253.gd59a3bf2b4-goog
[PATCH v6 5/7] drm/vkms: Support enabling ConfigFS devices
From: Jim Shargo
VKMS now supports creating and using virtual devices!
In addition to the enabling logic, this commit also prevents users from
adding new objects once a card is registered.
Signed-off-by: Jim Shargo
Signed-off-by: Brandon Pollack
---
drivers/gpu/drm/vkms/vkms_configfs.c | 37 ++--
drivers/gpu/drm/vkms/vkms_crtc.c | 4 +-
drivers/gpu/drm/vkms/vkms_drv.c | 1 +
drivers/gpu/drm/vkms/vkms_drv.h | 4 +-
drivers/gpu/drm/vkms/vkms_output.c | 282 +++
drivers/gpu/drm/vkms/vkms_plane.c| 10 +-
6 files changed, 282 insertions(+), 56 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c
b/drivers/gpu/drm/vkms/vkms_configfs.c
index dae2e85d83a1..bc35dcc47585 100644
--- a/drivers/gpu/drm/vkms/vkms_configfs.c
+++ b/drivers/gpu/drm/vkms/vkms_configfs.c
@@ -508,29 +508,40 @@ static ssize_t device_enabled_store(struct config_item
*item, const char *buf,
{
struct vkms_configfs *configfs = item_to_configfs(item);
struct vkms_device *device;
- int value, ret;
+ int enabled, ret;
- ret = kstrtoint(buf, 0, );
+ ret = kstrtoint(buf, 0, );
if (ret)
return ret;
- if (value != 1)
- return -EINVAL;
-
- mutex_lock(>lock);
-
- if (configfs->vkms_device) {
+ if (enabled == 0) {
+ mutex_lock(>lock);
+ if (configfs->vkms_device) {
+ vkms_remove_device(configfs->vkms_device);
+ configfs->vkms_device = NULL;
+ }
mutex_unlock(>lock);
+
return len;
}
- device = vkms_add_device(configfs);
- mutex_unlock(>lock);
+ if (enabled == 1) {
+ mutex_lock(>lock);
+ if (!configfs->vkms_device) {
+ device = vkms_add_device(configfs);
+ if (IS_ERR(device)) {
+ mutex_unlock(>lock);
+ return -PTR_ERR(device);
+ }
+
+ configfs->vkms_device = device;
+ }
+ mutex_unlock(>lock);
- if (IS_ERR(device))
- return -PTR_ERR(device);
+ return len;
+ }
- return len;
+ return -EINVAL;
}
CONFIGFS_ATTR(device_, enabled);
diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index 74bbd675464b..2aa1c5246b7e 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -279,7 +279,7 @@ static const struct drm_crtc_helper_funcs
vkms_crtc_helper_funcs = {
struct vkms_crtc *vkms_crtc_init(struct vkms_device *vkmsdev,
struct drm_plane *primary,
-struct drm_plane *cursor)
+struct drm_plane *cursor, const char *name)
{
struct drm_device *dev = >drm;
struct vkms_crtc *vkms_crtc;
@@ -291,7 +291,7 @@ struct vkms_crtc *vkms_crtc_init(struct vkms_device
*vkmsdev,
vkms_crtc = >output.crtcs[vkmsdev->output.num_crtcs++];
ret = drmm_crtc_init_with_planes(dev, _crtc->base, primary, cursor,
-_crtc_funcs, NULL);
+_crtc_funcs, name);
if (ret) {
DRM_ERROR("Failed to init CRTC\n");
goto out_error;
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index 819e880a8cf7..6e7f20681890 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -275,6 +275,7 @@ struct vkms_device *vkms_add_device(struct vkms_configfs
*configfs)
dev, _platform_driver.driver))) {
pdev = to_platform_device(dev);
max_id = max(max_id, pdev->id);
+ put_device(dev);
}
pdev = platform_device_register_data(NULL, DRIVER_NAME, max_id + 1,
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 8cdd7949f661..2b9545ada9c2 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -250,13 +250,13 @@ void vkms_remove_device(struct vkms_device *vkms_device);
/* CRTC */
struct vkms_crtc *vkms_crtc_init(struct vkms_device *vkmsdev,
struct drm_plane *primary,
-struct drm_plane *cursor);
+struct drm_plane *cursor, const char *name);
int vkms_output_init(struct vkms_device *vkmsdev);
int vkms_output_init_default(struct vkms_device *vkmsdev);
struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
- enum drm_plane_type type);
+ enum drm_plane_type type, char* name, ...);
/* CRC Support */
const char *const *vkms_get_crc_sources(struct drm_crtc *crtc,
diff --git
[PATCH v6 4/7] drm/vkms: Add ConfigFS scaffolding to VKMS
From: Jim Shargo This change adds the basic scaffolding for ConfigFS, including setting up the default directories. It does not allow for the registration of configfs-backed devices, which is complex and provided in a follow-up commit. This CL includes docs about using ConfigFS with VKMS, but I'll summarize in brief here as well (assuming ConfigFS is mounted at /config/): To create a new device, you can do so via `mkdir /config/vkms/my-device`. This will create a number of directories and files automatically: /config `-- vkms `-- my-device |-- connectors |-- crtcs |-- encoders |-- planes `-- enabled You can then configure objects by mkdir'ing in each of the directories. When you're satisfied, you can `echo 1 > /config/vkms/my-device/enabled`. This will create a new device according to your configuration. For now, this will fail, but the next change will add support for it. Signed-off-by: Jim Shargo Signed-off-by: Brandon Pollack --- Documentation/gpu/vkms.rst | 18 +- drivers/gpu/drm/Kconfig | 1 + drivers/gpu/drm/vkms/Makefile| 1 + drivers/gpu/drm/vkms/vkms_configfs.c | 650 +++ drivers/gpu/drm/vkms/vkms_drv.c | 56 ++- drivers/gpu/drm/vkms/vkms_drv.h | 92 +++- drivers/gpu/drm/vkms/vkms_output.c | 5 + 7 files changed, 806 insertions(+), 17 deletions(-) create mode 100644 drivers/gpu/drm/vkms/vkms_configfs.c diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst index ba04ac7c2167..c3875bf66dba 100644 --- a/Documentation/gpu/vkms.rst +++ b/Documentation/gpu/vkms.rst @@ -51,6 +51,12 @@ To disable the driver, use :: sudo modprobe -r vkms +Configuration With ConfigFS +=== + +.. kernel-doc:: drivers/gpu/drm/vkms/vkms_configfs.c + :doc: ConfigFS Support for VKMS + Testing With IGT @@ -135,22 +141,16 @@ project. Runtime Configuration - -We want to be able to reconfigure vkms instance without having to reload the -module. Use/Test-cases: +We want to be able to manipulate vkms instances without having to reload the +module. Such configuration can be added as extensions to vkms's ConfigFS +support. Use-cases: - Hotplug/hotremove connectors on the fly (to be able to test DP MST handling of compositors). -- Configure planes/crtcs/connectors (we'd need some code to have more than 1 of - them first). - - Change output configuration: Plug/unplug screens, change EDID, allow changing the refresh rate. -The currently proposed solution is to expose vkms configuration through -configfs. All existing module options should be supported through configfs -too. - Writeback support - diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index ab9ef1c20349..e39ee0e8ca06 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -284,6 +284,7 @@ config DRM_VKMS depends on DRM && MMU select DRM_KMS_HELPER select DRM_GEM_SHMEM_HELPER + select CONFIGFS_FS select CRC32 default n help diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile index 1b28a6a32948..6b83907ad554 100644 --- a/drivers/gpu/drm/vkms/Makefile +++ b/drivers/gpu/drm/vkms/Makefile @@ -1,5 +1,6 @@ # SPDX-License-Identifier: GPL-2.0-only vkms-y := \ + vkms_configfs.o \ vkms_drv.o \ vkms_plane.o \ vkms_output.o \ diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c new file mode 100644 index ..dae2e85d83a1 --- /dev/null +++ b/drivers/gpu/drm/vkms/vkms_configfs.c @@ -0,0 +1,650 @@ +// SPDX-License-Identifier: GPL-2.0+ + +#include +#include +#include + +#include +#include + +#include "vkms_drv.h" + +/** + * DOC: ConfigFS Support for VKMS + * + * VKMS is instrumented with support for configuration via :doc:`ConfigFS + * <../filesystems/configfs>`. + * + * With VKMS installed, you can mount ConfigFS at ``/config/`` like so:: + * + * mkdir -p /config/ + * sudo mount -t configfs none /config + * + * This allows you to configure multiple virtual devices. Note + * that the default device which can be enabled in the module params with:: + * + * modprobe vkms default_device=1 + * + * is immutable because we cannot pre-populate ConfigFS directories with normal + * files. + * + * To set up a new device, create a new directory under the VKMS configfs + * directory:: + * + * mkdir /config/vkms/test + * + * With your device created you'll find an new directory ready to be + * configured:: + * + * /config + * `-- vkms + * `-- test + * |-- connectors + * |-- crtcs + * |-- encoders + * |-- planes + * `-- enabled + * + * Each directory you add within the connectors, crtcs, encoders, and planes + * directories will let you
[PATCH v6 2/7] drm/vkms: Support multiple DRM objects (crtcs, etc.) per VKMS device
From: Jim Shargo
This change supports multiple CRTCs, encoders, connectors instead of one
of each per device.
Since ConfigFS-based devices will support multiple crtcs, it's useful to
move all of the writeback/composition data from being per-"output" to
being per-CRTC.
Since there's still only ever one CRTC, this should be a no-op refactor.
Signed-off-by: Jim Shargo
Signed-off-by: Brandon Pollack
---
drivers/gpu/drm/vkms/vkms_composer.c | 30 +++
drivers/gpu/drm/vkms/vkms_crtc.c | 100 -
drivers/gpu/drm/vkms/vkms_drv.c | 12 +--
drivers/gpu/drm/vkms/vkms_drv.h | 70 +--
drivers/gpu/drm/vkms/vkms_output.c| 122 ++
drivers/gpu/drm/vkms/vkms_plane.c | 38 ++--
drivers/gpu/drm/vkms/vkms_writeback.c | 42 -
7 files changed, 261 insertions(+), 153 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c
b/drivers/gpu/drm/vkms/vkms_composer.c
index d5d4f642d367..a59eb75a21c4 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -300,13 +300,13 @@ void vkms_composer_worker(struct work_struct *work)
composer_work);
struct drm_crtc *crtc = crtc_state->base.crtc;
struct vkms_writeback_job *active_wb = crtc_state->active_writeback;
- struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
+ struct vkms_crtc *vkms_crtc = drm_crtc_to_vkms_crtc(crtc);
bool crc_pending, wb_pending;
u64 frame_start, frame_end;
u32 crc32 = 0;
int ret;
- spin_lock_irq(>composer_lock);
+ spin_lock_irq(_crtc->composer_lock);
frame_start = crtc_state->frame_start;
frame_end = crtc_state->frame_end;
crc_pending = crtc_state->crc_pending;
@@ -330,7 +330,7 @@ void vkms_composer_worker(struct work_struct *work)
crtc_state->gamma_lut.base = NULL;
}
- spin_unlock_irq(>composer_lock);
+ spin_unlock_irq(_crtc->composer_lock);
/*
* We raced with the vblank hrtimer and previous work already computed
@@ -348,10 +348,10 @@ void vkms_composer_worker(struct work_struct *work)
return;
if (wb_pending) {
- drm_writeback_signal_completion(>wb_connector, 0);
- spin_lock_irq(>composer_lock);
+ drm_writeback_signal_completion(_crtc->wb_connector, 0);
+ spin_lock_irq(_crtc->composer_lock);
crtc_state->wb_pending = false;
- spin_unlock_irq(>composer_lock);
+ spin_unlock_irq(_crtc->composer_lock);
}
/*
@@ -401,30 +401,30 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const
char *src_name,
return 0;
}
-void vkms_set_composer(struct vkms_output *out, bool enabled)
+void vkms_set_composer(struct vkms_crtc *vkms_crtc, bool enabled)
{
bool old_enabled;
if (enabled)
- drm_crtc_vblank_get(>crtc);
+ drm_crtc_vblank_get(_crtc->base);
- mutex_lock(>enabled_lock);
- old_enabled = out->composer_enabled;
- out->composer_enabled = enabled;
+ mutex_lock(_crtc->enabled_lock);
+ old_enabled = vkms_crtc->composer_enabled;
+ vkms_crtc->composer_enabled = enabled;
/* the composition wasn't enabled, so unlock the lock to make sure the
lock
* will be balanced even if we have a failed commit
*/
- if (!out->composer_enabled)
- mutex_unlock(>enabled_lock);
+ if (!vkms_crtc->composer_enabled)
+ mutex_unlock(_crtc->enabled_lock);
if (old_enabled)
- drm_crtc_vblank_put(>crtc);
+ drm_crtc_vblank_put(_crtc->base);
}
int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name)
{
- struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
+ struct vkms_crtc *out = drm_crtc_to_vkms_crtc(crtc);
bool enabled = false;
int ret = 0;
diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index 3c5ebf106b66..74bbd675464b 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0+
+#include "linux/mutex.h"
#include
#include
@@ -11,17 +12,16 @@
static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
{
- struct vkms_output *output = container_of(timer, struct vkms_output,
- vblank_hrtimer);
- struct drm_crtc *crtc = >crtc;
+ struct vkms_crtc *vkms_crtc = timer_to_vkms_crtc(timer);
+ struct drm_crtc *crtc = _crtc->base;
struct vkms_crtc_state *state;
u64 ret_overrun;
bool ret, fence_cookie, composer_enabled;
fence_cookie = dma_fence_begin_signalling();
- ret_overrun = hrtimer_forward_now(>vblank_hrtimer,
-
[PATCH v6 1/7] drm/vkms: Back VKMS with DRM memory management instead of static objects
From: Jim Shargo
This is a small refactor to make ConfigFS support easier. Once we
support ConfigFS, there can be multiple devices instantiated by the
driver, and so moving everything into managed memory makes things much
easier.
This should be a no-op refactor.
Signed-off-by: Jim Shargo
Signed-off-by: Brandon Pollack
---
drivers/gpu/drm/vkms/vkms_drv.c| 128 +++--
drivers/gpu/drm/vkms/vkms_drv.h| 4 +-
drivers/gpu/drm/vkms/vkms_output.c | 6 +-
3 files changed, 71 insertions(+), 67 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index dd0af086e7fa..387c832f5dc9 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -9,10 +9,12 @@
* the GPU in DRM API tests.
*/
+#include
#include
#include
#include
+#include
#include
#include
#include
@@ -37,8 +39,6 @@
#define DRIVER_MAJOR 1
#define DRIVER_MINOR 0
-static struct vkms_config *default_config;
-
static bool enable_cursor = true;
module_param_named(enable_cursor, enable_cursor, bool, 0444);
MODULE_PARM_DESC(enable_cursor, "Enable/Disable cursor support");
@@ -96,9 +96,9 @@ static int vkms_config_show(struct seq_file *m, void *data)
struct drm_device *dev = entry->dev;
struct vkms_device *vkmsdev = drm_device_to_vkms_device(dev);
- seq_printf(m, "writeback=%d\n", vkmsdev->config->writeback);
- seq_printf(m, "cursor=%d\n", vkmsdev->config->cursor);
- seq_printf(m, "overlay=%d\n", vkmsdev->config->overlay);
+ seq_printf(m, "writeback=%d\n", vkmsdev->config.writeback);
+ seq_printf(m, "cursor=%d\n", vkmsdev->config.cursor);
+ seq_printf(m, "overlay=%d\n", vkmsdev->config.overlay);
return 0;
}
@@ -166,121 +166,127 @@ static int vkms_modeset_init(struct vkms_device
*vkmsdev)
dev->mode_config.cursor_height = 512;
/* FIXME: There's a confusion between bpp and depth between this and
* fbdev helpers. We have to go with 0, meaning "pick the default",
-* which ix XRGB in all cases. */
+* which ix XRGB in all cases.
+*/
dev->mode_config.preferred_depth = 0;
dev->mode_config.helper_private = _mode_config_helpers;
return vkms_output_init(vkmsdev, 0);
}
-static int vkms_create(struct vkms_config *config)
+static int vkms_platform_probe(struct platform_device *pdev)
{
int ret;
- struct platform_device *pdev;
struct vkms_device *vkms_device;
+ void *grp;
- pdev = platform_device_register_simple(DRIVER_NAME, -1, NULL, 0);
- if (IS_ERR(pdev))
- return PTR_ERR(pdev);
-
- if (!devres_open_group(>dev, NULL, GFP_KERNEL)) {
- ret = -ENOMEM;
- goto out_unregister;
- }
+ grp = devres_open_group(>dev, NULL, GFP_KERNEL);
+ if (!grp)
+ return -ENOMEM;
vkms_device = devm_drm_dev_alloc(>dev, _driver,
struct vkms_device, drm);
if (IS_ERR(vkms_device)) {
ret = PTR_ERR(vkms_device);
- goto out_devres;
+ goto out_release_group;
}
+
vkms_device->platform = pdev;
- vkms_device->config = config;
- config->dev = vkms_device;
+ vkms_device->config.cursor = enable_cursor;
+ vkms_device->config.writeback = enable_writeback;
+ vkms_device->config.overlay = enable_overlay;
ret = dma_coerce_mask_and_coherent(vkms_device->drm.dev,
DMA_BIT_MASK(64));
-
if (ret) {
DRM_ERROR("Could not initialize DMA support\n");
- goto out_devres;
+ goto out_release_group;
}
ret = drm_vblank_init(_device->drm, 1);
if (ret) {
DRM_ERROR("Failed to vblank\n");
- goto out_devres;
+ goto out_release_group;
}
ret = vkms_modeset_init(vkms_device);
- if (ret)
- goto out_devres;
+ if (ret) {
+ DRM_ERROR("Unable to initialize modesetting\n");
+ goto out_release_group;
+ }
drm_debugfs_add_files(_device->drm, vkms_config_debugfs_list,
ARRAY_SIZE(vkms_config_debugfs_list));
ret = drm_dev_register(_device->drm, 0);
- if (ret)
- goto out_devres;
+ if (ret) {
+ DRM_ERROR("Unable to register device with id %d\n", pdev->id);
+ goto out_release_group;
+ }
drm_fbdev_generic_setup(_device->drm, 0);
+ platform_set_drvdata(pdev, vkms_device);
+ devres_close_group(>dev, grp);
return 0;
-out_devres:
- devres_release_group(>dev, NULL);
-out_unregister:
- platform_device_unregister(pdev);
+out_release_group:
+ devres_release_group(>dev, grp);
return ret;
}
-static int
[PATCH v6 0/7] Adds support for ConfigFS to VKMS!
Since Jim is busy with other work and I'm working on some things that rely on this, I've taken up the task of doing the iterations. I've addressed the comments as best I can (those replies are to each individual change) and here is the patch set to go with those. I added my own signoff to each commit, but I've left jshargo@ as the author of all the commits he wrote. I'm sure there is still more to address and the ICT tests that were writtein parallel to this may also need some additions, but I'm hoping we're in a good enough state to get this in and iterate from there soon. Since V6: rmdirs for documentation examples fix crtc mask for writebacks Since V5: Fixed some bad merge conflicts and locking behaviours as well as clarified some documentation, should be good to go now :) Since V4: Fixed up some documentation as suggested by Marius Fixed up some bad locking as suggested by Marius Small fixes here and there (most have email responses to previous chain emails) Since V3: I've added hotplug support in the latest patch. This has been reviewed some and the notes from that review are addressed here as well. Relevant/Utilizing work: === I've built a while test framework based on this as proof it functions (though I'm sure there may be lingering bugs!). You can check that out on crrev.com if you are interested and need to get started yourself (but be aware of any licensing that may differ from the kernel itself! Make sure you understand the license: https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform/tast-tests/LICENSE That said, you can see the changes in review on the crrev gerrit: https://chromium-review.googlesource.com/c/chromiumos/platform/tast-tests/+/469 Outro: = I really appreciate everyone's input and tolerance in getting these changes in. Jim's first patch series was this, and other than some small cleanups and documentation, taking over it is also mine. Thank you everyone :) Brandon Pollack (1): drm/vkms Add hotplug support via configfs to VKMS. Jim Shargo (6): drm/vkms: Back VKMS with DRM memory management instead of static objects drm/vkms: Support multiple DRM objects (crtcs, etc.) per VKMS device drm/vkms: Provide platform data when creating VKMS devices drm/vkms: Add ConfigFS scaffolding to VKMS drm/vkms: Support enabling ConfigFS devices drm/vkms: Add a module param to enable/disable the default device Documentation/gpu/vkms.rst| 20 +- drivers/gpu/drm/Kconfig | 1 + drivers/gpu/drm/vkms/Makefile | 1 + drivers/gpu/drm/vkms/vkms_composer.c | 30 +- drivers/gpu/drm/vkms/vkms_configfs.c | 723 ++ drivers/gpu/drm/vkms/vkms_crtc.c | 102 ++-- drivers/gpu/drm/vkms/vkms_drv.c | 206 +--- drivers/gpu/drm/vkms/vkms_drv.h | 182 +-- drivers/gpu/drm/vkms/vkms_output.c| 404 -- drivers/gpu/drm/vkms/vkms_plane.c | 44 +- drivers/gpu/drm/vkms/vkms_writeback.c | 42 +- 11 files changed, 1514 insertions(+), 241 deletions(-) create mode 100644 drivers/gpu/drm/vkms/vkms_configfs.c -- 2.42.0.rc2.253.gd59a3bf2b4-goog
Re: [PATCH v5 4/7] drm/vkms: Add ConfigFS scaffolding to VKMS
silly mistake, I should have been more cautious. Thank you for pickup up my slack On Mon, Aug 28, 2023 at 11:10 PM Marius Vlad wrote: > > Hi Brandon, > > See some minor missing rmdirs for connector_other and encoder_other. > > On Mon, Aug 28, 2023 at 08:17:06AM +, Brandon Pollack wrote: > > From: Jim Shargo > > > > This change adds the basic scaffolding for ConfigFS, including setting > > up the default directories. It does not allow for the registration of > > configfs-backed devices, which is complex and provided in a follow-up > > commit. > > > > This CL includes docs about using ConfigFS with VKMS, but I'll summarize > > in brief here as well (assuming ConfigFS is mounted at /config/): > > > > To create a new device, you can do so via `mkdir > > /config/vkms/my-device`. > > > > This will create a number of directories and files automatically: > > > > /config > > `-- vkms > > `-- my-device > > |-- connectors > > |-- crtcs > > |-- encoders > > |-- planes > > `-- enabled > > > > You can then configure objects by mkdir'ing in each of the directories. > > > > When you're satisfied, you can `echo 1 > /config/vkms/my-device/enabled`. > > This will create a new device according to your configuration. > > > > For now, this will fail, but the next change will add support for it. > > > > Signed-off-by: Jim Shargo > > Signed-off-by: Brandon Pollack > > --- > > Documentation/gpu/vkms.rst | 18 +- > > drivers/gpu/drm/Kconfig | 1 + > > drivers/gpu/drm/vkms/Makefile| 1 + > > drivers/gpu/drm/vkms/vkms_configfs.c | 648 +++ > > drivers/gpu/drm/vkms/vkms_drv.c | 56 ++- > > drivers/gpu/drm/vkms/vkms_drv.h | 92 +++- > > drivers/gpu/drm/vkms/vkms_output.c | 5 + > > 7 files changed, 804 insertions(+), 17 deletions(-) > > create mode 100644 drivers/gpu/drm/vkms/vkms_configfs.c > > > > diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst > > index ba04ac7c2167..c3875bf66dba 100644 > > --- a/Documentation/gpu/vkms.rst > > +++ b/Documentation/gpu/vkms.rst > > @@ -51,6 +51,12 @@ To disable the driver, use :: > > > >sudo modprobe -r vkms > > > > +Configuration With ConfigFS > > +=== > > + > > +.. kernel-doc:: drivers/gpu/drm/vkms/vkms_configfs.c > > + :doc: ConfigFS Support for VKMS > > + > > Testing With IGT > > > > > > @@ -135,22 +141,16 @@ project. > > Runtime Configuration > > - > > > > -We want to be able to reconfigure vkms instance without having to reload > > the > > -module. Use/Test-cases: > > +We want to be able to manipulate vkms instances without having to reload > > the > > +module. Such configuration can be added as extensions to vkms's ConfigFS > > +support. Use-cases: > > > > - Hotplug/hotremove connectors on the fly (to be able to test DP MST > > handling > >of compositors). > > > > -- Configure planes/crtcs/connectors (we'd need some code to have more than > > 1 of > > - them first). > > - > > - Change output configuration: Plug/unplug screens, change EDID, allow > > changing > >the refresh rate. > > > > -The currently proposed solution is to expose vkms configuration through > > -configfs. All existing module options should be supported through configfs > > -too. > > - > > Writeback support > > - > > > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > > index ab9ef1c20349..e39ee0e8ca06 100644 > > --- a/drivers/gpu/drm/Kconfig > > +++ b/drivers/gpu/drm/Kconfig > > @@ -284,6 +284,7 @@ config DRM_VKMS > > depends on DRM && MMU > > select DRM_KMS_HELPER > > select DRM_GEM_SHMEM_HELPER > > + select CONFIGFS_FS > > select CRC32 > > default n > > help > > diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile > > index 1b28a6a32948..6b83907ad554 100644 > > --- a/drivers/gpu/drm/vkms/Makefile > > +++ b/drivers/gpu/drm/vkms/Makefile > > @@ -1,5 +1,6 @@ > > # SPDX-License-Identifier: GPL-2.0-only > > vkms-y := \ > > + vkms_configfs.o \ > > vkms_drv.o \ > > vkms_plane.o \ > > vkms_output.o \ > > diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c > > b/drivers/gpu/drm/vkms/vkms_configfs.c > > new file mode 100644 > > index ..f2439629b37b > > --- /dev/null > > +++ b/drivers/gpu/drm/vkms/vkms_configfs.c > > @@ -0,0 +1,648 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > + > > +#include > > +#include > > +#include > > + > > +#include > > +#include > > + > > +#include "vkms_drv.h" > > + > > +/** > > + * DOC: ConfigFS Support for VKMS > > + * > > + * VKMS is instrumented with support for configuration via :doc:`ConfigFS > > + * <../filesystems/configfs>`. > > + * > > + * With VKMS installed, you can mount ConfigFS at ``/config/`` like so:: > > + * > > + * mkdir -p /config/ > > + * sudo mount -t configfs
[PATCH v3] fs: clean up usage of noop_dirty_folio
In folio_mark_dirty(), it can automatically fallback to
noop_dirty_folio() if a_ops->dirty_folio is not registered.
In anon_aops, dev_dax_aops and fb_deferred_io_aops, replacing .dirty_folio
with NULL makes them identical to default (empty_aops) and since we never
compare ->a_ops pointer with either of those, we can remove them
completely.
Acked-by: Al Viro
Reviewed-by: Jan Kara
Reviewed-by: Matthew Wilcox (Oracle)
Reviewed-by: Christoph Hellwig
Signed-off-by: Xueshi Hu
---
Changes in v3:
- correct the rationale of removal three empty aops
- v2: https://lore.kernel.org/linux-mm/20230828144242.GZ3390869@ZenIV/T/#t
Changes in v2:
- make noop_dirty_folio() inline as suggested by Matthew
- v1:
https://lore.kernel.org/linux-mm/zoxafrz9etovu...@infradead.org/T/#m073d45909b1df03ff09f382557dc4e84d0607c49
drivers/dax/device.c| 5 -
drivers/video/fbdev/core/fb_defio.c | 5 -
fs/aio.c| 1 -
fs/ext2/inode.c | 1 -
fs/ext4/inode.c | 1 -
fs/fuse/dax.c | 1 -
fs/hugetlbfs/inode.c| 1 -
fs/libfs.c | 5 -
fs/xfs/xfs_aops.c | 1 -
include/linux/pagemap.h | 1 -
mm/page-writeback.c | 18 +-
mm/secretmem.c | 1 -
mm/shmem.c | 1 -
mm/swap_state.c | 1 -
14 files changed, 5 insertions(+), 38 deletions(-)
diff --git a/drivers/dax/device.c b/drivers/dax/device.c
index 30665a3ff6ea..018aa9f88ec7 100644
--- a/drivers/dax/device.c
+++ b/drivers/dax/device.c
@@ -345,10 +345,6 @@ static unsigned long dax_get_unmapped_area(struct file
*filp,
return current->mm->get_unmapped_area(filp, addr, len, pgoff, flags);
}
-static const struct address_space_operations dev_dax_aops = {
- .dirty_folio= noop_dirty_folio,
-};
-
static int dax_open(struct inode *inode, struct file *filp)
{
struct dax_device *dax_dev = inode_dax(inode);
@@ -358,7 +354,6 @@ static int dax_open(struct inode *inode, struct file *filp)
dev_dbg(_dax->dev, "trace\n");
inode->i_mapping = __dax_inode->i_mapping;
inode->i_mapping->host = __dax_inode;
- inode->i_mapping->a_ops = _dax_aops;
filp->f_mapping = inode->i_mapping;
filp->f_wb_err = filemap_sample_wb_err(filp->f_mapping);
filp->f_sb_err = file_sample_sb_err(filp);
diff --git a/drivers/video/fbdev/core/fb_defio.c
b/drivers/video/fbdev/core/fb_defio.c
index 274f5d0fa247..08be3592281f 100644
--- a/drivers/video/fbdev/core/fb_defio.c
+++ b/drivers/video/fbdev/core/fb_defio.c
@@ -221,10 +221,6 @@ static const struct vm_operations_struct
fb_deferred_io_vm_ops = {
.page_mkwrite = fb_deferred_io_mkwrite,
};
-static const struct address_space_operations fb_deferred_io_aops = {
- .dirty_folio= noop_dirty_folio,
-};
-
int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma)
{
vma->vm_ops = _deferred_io_vm_ops;
@@ -307,7 +303,6 @@ void fb_deferred_io_open(struct fb_info *info,
{
struct fb_deferred_io *fbdefio = info->fbdefio;
- file->f_mapping->a_ops = _deferred_io_aops;
fbdefio->open_count++;
}
EXPORT_SYMBOL_GPL(fb_deferred_io_open);
diff --git a/fs/aio.c b/fs/aio.c
index 77e33619de40..4cf386f9cb1c 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -484,7 +484,6 @@ static int aio_migrate_folio(struct address_space *mapping,
struct folio *dst,
#endif
static const struct address_space_operations aio_ctx_aops = {
- .dirty_folio= noop_dirty_folio,
.migrate_folio = aio_migrate_folio,
};
diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
index 75983215c7a1..ce191bdf1c78 100644
--- a/fs/ext2/inode.c
+++ b/fs/ext2/inode.c
@@ -971,7 +971,6 @@ const struct address_space_operations ext2_aops = {
static const struct address_space_operations ext2_dax_aops = {
.writepages = ext2_dax_writepages,
.direct_IO = noop_direct_IO,
- .dirty_folio= noop_dirty_folio,
};
/*
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 43775a6ca505..67c1710c01b0 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3561,7 +3561,6 @@ static const struct address_space_operations ext4_da_aops
= {
static const struct address_space_operations ext4_dax_aops = {
.writepages = ext4_dax_writepages,
.direct_IO = noop_direct_IO,
- .dirty_folio= noop_dirty_folio,
.bmap = ext4_bmap,
.swap_activate = ext4_iomap_swap_activate,
};
diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
index 8e74f278a3f6..50ca767cbd5e 100644
--- a/fs/fuse/dax.c
+++ b/fs/fuse/dax.c
@@ -1326,7 +1326,6 @@ bool fuse_dax_inode_alloc(struct super_block *sb, struct
fuse_inode *fi)
static const struct address_space_operations fuse_dax_file_aops = {
Re: [PATCH v15 00/23] Add generic memory shrinker to VirtIO-GPU and Panfrost DRM drivers
On 8/28/23 18:24, Helen Mae Koike Fornazier wrote: > On Monday, August 28, 2023 11:37 -03, "Helen Mae Koike Fornazier" > wrote: > >> On Sunday, August 27, 2023 14:54 -03, Dmitry Osipenko >> wrote: >> >>> This series: >>> >>> 1. Adds common drm-shmem memory shrinker >>> 2. Enables shrinker for VirtIO-GPU driver >>> 3. Switches Panfrost driver to the common shrinker >> >> Hi Dmitry, >> >> Would you mind testing with drm-ci? We virt-io tests there and it would be >> really great to get your feedback of it. >> >> https://cgit.freedesktop.org/drm/drm/log/?h=topic/drm-ci > > sorry, I forgot that you also need this patchset: > https://lists.freedesktop.org/archives/dri-devel/2023-August/420063.html > to enable virtio_gpu test job. > > Thanks again. > Helen > >> >> You need to merge your changes with the above tree. >> To configure it, you just need to have a tree on gitlab.freedesktop.org, >> go to the settings and change the CI/CD configuration file from >> .gitlab-ci.yml >> to drivers/gpu/drm/ci/gitlab-ci.yml, and you can start a pipeline >> on your branch. >> >> at the time of this writting, gitlab.freedesktop.org is under maintenance, >> but it should be back soon. Thanks, Helen. I'll give it a try for the next version -- Best regards, Dmitry
Re: [PATCH v15 17/23] drm/shmem-helper: Add and use drm_gem_shmem_resv_assert_held() helper
On 8/28/23 13:12, Boris Brezillon wrote:
> On Sun, 27 Aug 2023 20:54:43 +0300
> Dmitry Osipenko wrote:
>
>> In a preparation of adding drm-shmem memory shrinker, move all reservation
>> locking lockdep checks to use new drm_gem_shmem_resv_assert_held() that
>> will resolve spurious lockdep warning about wrong locking order vs
>> fs_reclam code paths during freeing of shmem GEM, where lockdep isn't
>> aware that it's impossible to have locking contention with the fs_reclam
>> at this special time.
>>
>> Signed-off-by: Dmitry Osipenko
>> ---
>> drivers/gpu/drm/drm_gem_shmem_helper.c | 37 +-
>> 1 file changed, 25 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
>> b/drivers/gpu/drm/drm_gem_shmem_helper.c
>> index d96fee3d6166..ca5da976aafa 100644
>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
>> @@ -128,6 +128,23 @@ struct drm_gem_shmem_object
>> *drm_gem_shmem_create(struct drm_device *dev, size_t
>> }
>> EXPORT_SYMBOL_GPL(drm_gem_shmem_create);
>>
>> +static void drm_gem_shmem_resv_assert_held(struct drm_gem_shmem_object
>> *shmem)
>> +{
>> +/*
>> + * Destroying the object is a special case.. drm_gem_shmem_free()
>> + * calls many things that WARN_ON if the obj lock is not held. But
>> + * acquiring the obj lock in drm_gem_shmem_free() can cause a locking
>> + * order inversion between reservation_ww_class_mutex and fs_reclaim.
>> + *
>> + * This deadlock is not actually possible, because no one should
>> + * be already holding the lock when drm_gem_shmem_free() is called.
>> + * Unfortunately lockdep is not aware of this detail. So when the
>> + * refcount drops to zero, we pretend it is already locked.
>> + */
>> +if (kref_read(>base.refcount))
>> +drm_gem_shmem_resv_assert_held(shmem);
>> +}
>> +
>> /**
>> * drm_gem_shmem_free - Free resources associated with a shmem GEM object
>> * @shmem: shmem GEM object to free
>> @@ -142,8 +159,6 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object
>> *shmem)
>> if (obj->import_attach) {
>> drm_prime_gem_destroy(obj, shmem->sgt);
>> } else if (!shmem->imported_sgt) {
>> -dma_resv_lock(shmem->base.resv, NULL);
>> -
>> drm_WARN_ON(obj->dev, kref_read(>vmap_use_count));
>>
>> if (shmem->sgt) {
>> @@ -156,8 +171,6 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object
>> *shmem)
>> drm_gem_shmem_put_pages_locked(shmem);
>
> AFAICT, drm_gem_shmem_put_pages_locked() is the only function that's
> called in the free path and would complain about resv-lock not being
> held. I think I'd feel more comfortable if we were adding a
> drm_gem_shmem_free_pages() function that did everything
> drm_gem_shmem_put_pages_locked() does except for the lock_held() check
> and the refcount dec, and have it called here (and in
> drm_gem_shmem_put_pages_locked()). This way we can keep using
> dma_resv_assert_held() instead of having our own variant.
It's not only drm_gem_shmem_free_pages(), but any drm-shmem function
that drivers may use in the GEM's freeing callback.
For example, panfrost_gem_free_object() may unpin shmem BO and then do
drm_gem_shmem_free().
--
Best regards,
Dmitry
Re: [PATCH v15 12/23] drm/shmem-helper: Add and use pages_pin_count
On 8/28/23 14:46, Boris Brezillon wrote:
> On Sun, 27 Aug 2023 20:54:38 +0300
> Dmitry Osipenko wrote:
>
>> Add separate pages_pin_count for tracking of whether drm-shmem pages are
>> moveable or not. With the addition of memory shrinker support to drm-shmem,
>> the pages_use_count will no longer determine whether pages are hard-pinned
>> in memory, but whether pages exit and are soft-pinned (and could be swapped
>> out). The pages_pin_count > 1 will hard-pin pages in memory.
>>
>> Suggested-by: Boris Brezillon
>> Signed-off-by: Dmitry Osipenko
>> ---
>> drivers/gpu/drm/drm_gem_shmem_helper.c | 22 +-
>> include/drm/drm_gem_shmem_helper.h | 10 ++
>> 2 files changed, 27 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
>> b/drivers/gpu/drm/drm_gem_shmem_helper.c
>> index d545d3d227d7..1a7e5c332fd8 100644
>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
>> @@ -234,14 +234,22 @@ static int drm_gem_shmem_pin_locked(struct
>> drm_gem_shmem_object *shmem)
>>
>> dma_resv_assert_held(shmem->base.resv);
>>
>> +if (kref_get_unless_zero(>pages_pin_count))
>> +return 0;
>> +
>> ret = drm_gem_shmem_get_pages_locked(shmem);
>> +if (!ret)
>> +kref_init(>pages_pin_count);
>>
>> return ret;
>> }
>>
>> -static void drm_gem_shmem_unpin_locked(struct drm_gem_shmem_object *shmem)
>> +static void drm_gem_shmem_kref_unpin_pages(struct kref *kref)
>> {
>> -dma_resv_assert_held(shmem->base.resv);
>> +struct drm_gem_shmem_object *shmem;
>> +
>> +shmem = container_of(kref, struct drm_gem_shmem_object,
>> + pages_pin_count);
>>
>> drm_gem_shmem_put_pages_locked(shmem);
>> }
>> @@ -263,6 +271,9 @@ int drm_gem_shmem_pin(struct drm_gem_shmem_object *shmem)
>>
>> drm_WARN_ON(obj->dev, obj->import_attach);
>>
>> +if (kref_get_unless_zero(>pages_pin_count))
>> +return 0;
>> +
>> ret = dma_resv_lock_interruptible(shmem->base.resv, NULL);
>> if (ret)
>> return ret;
>> @@ -286,9 +297,10 @@ void drm_gem_shmem_unpin(struct drm_gem_shmem_object
>> *shmem)
>>
>> drm_WARN_ON(obj->dev, obj->import_attach);
>>
>> -dma_resv_lock(shmem->base.resv, NULL);
>> -drm_gem_shmem_unpin_locked(shmem);
>> -dma_resv_unlock(shmem->base.resv);
>> +if (kref_put_dma_resv(>pages_pin_count,
>> + drm_gem_shmem_kref_unpin_pages,
>> + obj->resv, NULL))
>> +dma_resv_unlock(obj->resv);
>> }
>> EXPORT_SYMBOL_GPL(drm_gem_shmem_unpin);
>>
>> diff --git a/include/drm/drm_gem_shmem_helper.h
>> b/include/drm/drm_gem_shmem_helper.h
>> index ec2d8b24e3cf..afb7cd671e2a 100644
>> --- a/include/drm/drm_gem_shmem_helper.h
>> +++ b/include/drm/drm_gem_shmem_helper.h
>> @@ -39,6 +39,16 @@ struct drm_gem_shmem_object {
>> */
>> unsigned int pages_use_count;
>>
>> +/**
>> + * @pages_pin_count:
>> + *
>> + * Reference count on the pinned pages table.
>> + * The pages allowed to be evicted and purged by memory
>> + * shrinker only when the count is zero, otherwise pages
>> + * are hard-pinned in memory.
>> + */
>> +struct kref pages_pin_count;
>
> I know it's tempting to use kref for the pages use/pin count, but I'm
> wondering if we wouldn't be better using a refcount_t, which provides
> overflow/underflow protection while still letting us control how we
> want to handle the locking for 0 <-> 1 transitions. By doing that, we
> avoid introducing core locking changes that might be more
> controversial/longer to get accepted. Besides, I suspect the resulting
> code (the one using a refcount_t) won't be more verbose/complicated (no
> release functions needed if you don't use kref_put(), which makes
> things closer to what we have right now).
Alright, let's try to use refcount_t since Christian also doesn't like kref
--
Best regards,
Dmitry
Re: [PATCH v15 10/23] locking/refcount, kref: Add kref_put_ww_mutex()
On 8/28/23 12:26, Boris Brezillon wrote:
> On Sun, 27 Aug 2023 20:54:36 +0300
> Dmitry Osipenko wrote:
>
>> Introduce kref_put_ww_mutex() helper that will handle the wait-wound
>> mutex auto-locking on kref_put(). This helper is wanted by DRM drivers
>> that extensively use dma-reservation locking which in turns uses ww-mutex.
>>
>> Signed-off-by: Dmitry Osipenko
>> ---
>> include/linux/kref.h | 12
>> include/linux/refcount.h | 5 +
>> lib/refcount.c | 34 ++
>> 3 files changed, 51 insertions(+)
>>
>> diff --git a/include/linux/kref.h b/include/linux/kref.h
>> index d32e21a2538c..b2d8dc6e9ae0 100644
>> --- a/include/linux/kref.h
>> +++ b/include/linux/kref.h
>> @@ -90,6 +90,18 @@ static inline int kref_put_lock(struct kref *kref,
>> return 0;
>> }
>>
>> +static inline int kref_put_ww_mutex(struct kref *kref,
>> +void (*release)(struct kref *kref),
>> +struct ww_mutex *lock,
>> +struct ww_acquire_ctx *ctx)
>> +{
>> +if (refcount_dec_and_ww_mutex_lock(>refcount, lock, ctx)) {
>> +release(kref);
>> +return 1;
>> +}
>> +return 0;
>> +}
>> +
>> /**
>> * kref_get_unless_zero - Increment refcount for object unless it is zero.
>> * @kref: object.
>> diff --git a/include/linux/refcount.h b/include/linux/refcount.h
>> index a62fcca97486..be9ad272bc77 100644
>> --- a/include/linux/refcount.h
>> +++ b/include/linux/refcount.h
>> @@ -99,6 +99,8 @@
>> #include
>>
>> struct mutex;
>> +struct ww_mutex;
>> +struct ww_acquire_ctx;
>>
>> /**
>> * typedef refcount_t - variant of atomic_t specialized for reference counts
>> @@ -366,4 +368,7 @@ extern __must_check bool
>> refcount_dec_and_lock(refcount_t *r, spinlock_t *lock)
>> extern __must_check bool refcount_dec_and_lock_irqsave(refcount_t *r,
>> spinlock_t *lock,
>> unsigned long *flags)
>> __cond_acquires(lock);
>> +extern __must_check bool refcount_dec_and_ww_mutex_lock(refcount_t *r,
>> +struct ww_mutex *lock,
>> +struct ww_acquire_ctx
>> *ctx) __cond_acquires(>base);
>> #endif /* _LINUX_REFCOUNT_H */
>> diff --git a/lib/refcount.c b/lib/refcount.c
>> index a207a8f22b3c..3f6fd0ceed02 100644
>> --- a/lib/refcount.c
>> +++ b/lib/refcount.c
>> @@ -6,6 +6,7 @@
>> #include
>> #include
>> #include
>> +#include
>> #include
>>
>> #define REFCOUNT_WARN(str) WARN_ONCE(1, "refcount_t: " str ".\n")
>> @@ -184,3 +185,36 @@ bool refcount_dec_and_lock_irqsave(refcount_t *r,
>> spinlock_t *lock,
>> return true;
>> }
>> EXPORT_SYMBOL(refcount_dec_and_lock_irqsave);
>> +
>> +/**
>> + * refcount_dec_and_ww_mutex_lock - return holding ww-mutex if able to
>> + * decrement refcount to 0
>> + * @r: the refcount
>> + * @lock: the ww-mutex to be locked
>> + * @ctx: wait-wound context
>> + *
>> + * Similar to atomic_dec_and_lock(), it will WARN on underflow and fail to
>> + * decrement when saturated at REFCOUNT_SATURATED.
>> + *
>> + * Provides release memory ordering, such that prior loads and stores are
>> done
>> + * before, and provides a control dependency such that free() must come
>> after.
>> + * See the comment on top.
>> + *
>> + * Return: true and hold ww-mutex lock if able to decrement refcount to 0,
>> + * false otherwise
>> + */
>> +bool refcount_dec_and_ww_mutex_lock(refcount_t *r, struct ww_mutex *lock,
>> +struct ww_acquire_ctx *ctx)
>> +{
>> +if (refcount_dec_not_one(r))
>> +return false;
>> +
>> +ww_mutex_lock(lock, ctx);
>
> Unless I'm wrong, ww_mutex_lock() can return -EDEADLK when ctx !=
> NULL, in which case, the lock is not held when it returns. Question is,
> do we really have a use case for ctx != NULL in that kref_put_ww_mutex()
> path. If we need to acquire other ww_locks, this lock, and the other
> locks should have been acquired beforehand, and we can simply call
> kref_put() when we want to release the ref on the resource.
Right, I completely forgot about the deadlocking
--
Best regards,
Dmitry
RE: [PATCH] drm/bridge: imx: fix potential NULL pointer dereference in imx8qxp_ldb_parse_dt_companion()
On Tuesday, August 29, 2023 2:38 AM Laurent Pinchart
wrote:
> On Tue, Aug 29, 2023 at 02:01:25AM +0800, Zhang Shurong wrote:
> > 在 2023年8月29日星期二 CST 上午1:28:22，Laurent Pinchart 写道：
> > > Hi Zhang,
> > >
> > > Thank you for the patch.
> > >
> > > On Tue, Aug 29, 2023 at 12:55:01AM +0800, Zhang Shurong wrote:
> > > > of_match_device() may fail and returns a NULL pointer.
> > >
> > > How can it return a NULL pointer here ?
> > >
> > > > Fix this by checking the return value of of_match_device().
> > > >
> > > > Fixes: 3818715f62b4 ("drm/bridge: imx: Add LDB support for i.MX8qxp")
> > > > Signed-off-by: Zhang Shurong
> > > > ---
> > > >
> > > > drivers/gpu/drm/bridge/imx/imx8qxp-ldb.c | 2 ++
> > > > 1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-ldb.c
> > > > b/drivers/gpu/drm/bridge/imx/imx8qxp-ldb.c index
> > > > 7984da9c0a35..d272f35c8eac 100644
> > > > --- a/drivers/gpu/drm/bridge/imx/imx8qxp-ldb.c
> > > > +++ b/drivers/gpu/drm/bridge/imx/imx8qxp-ldb.c
> > > > @@ -488,6 +488,8 @@ static int
> imx8qxp_ldb_parse_dt_companion(struct imx8qxp_ldb *imx8qxp_ldb)
> > > > * string.
> > > > */
> > > > match = of_match_device(dev->driver->of_match_table, dev);
> > > > + if (!match)
> > > > + return -ENODEV;
> > > > if (!of_device_is_compatible(companion, match->compatible)) {
> > > > DRM_DEV_ERROR(dev, "companion LDB is incompatible\n");
> > > > ret = -ENXIO;
> >
> > I think we can make it happen by designing the platform device in a way
> that
> > its name aligns with that of the driver. In such a scenario, when the driver
> > is probed, the of_match_device function will return null. You can verify
> > this
> > functionality by reviewing the following function:
> >
> > static int platform_match(struct device *dev, struct device_driver *drv)
>
> This particular device is found in OF-based systems only, and only
> instantiated through DT. You can create a platform_device manually that
> may match this driver, but that would be a made-up situation, not
> something that can happen in practice.
>
I agree with Laurent.
Regards,
Liu Ying
Re: [PATCH v2 4/9] drm/sched: Split free_job into own work item
On 8/28/23 20:41, Matthew Brost wrote: On Mon, Aug 28, 2023 at 08:04:31PM +0200, Danilo Krummrich wrote: On 8/11/23 04:31, Matthew Brost wrote: Rather than call free_job and run_job in same work item have a dedicated work item for each. This aligns with the design and intended use of work queues. Signed-off-by: Matthew Brost --- drivers/gpu/drm/scheduler/sched_main.c | 137 ++--- include/drm/gpu_scheduler.h| 8 +- 2 files changed, 106 insertions(+), 39 deletions(-) diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c index cede47afc800..b67469eac179 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -1275,7 +1338,8 @@ EXPORT_SYMBOL(drm_sched_submit_ready); void drm_sched_submit_stop(struct drm_gpu_scheduler *sched) I was wondering what the scheduler teardown sequence looks like for DRM_SCHED_POLICY_SINGLE_ENTITY and how XE does that. In Nouveau, userspace can ask the kernel to create a channel (or multiple), where each channel represents a ring feeding the firmware scheduler. Userspace can forcefully close channels via either a dedicated IOCTL or by just closing the FD which subsequently closes all channels opened through this FD. When this happens the scheduler needs to be teared down. Without keeping track of things in a driver specific way, the only thing I could really come up with is the following. /* Make sure no more jobs are fetched from the entity. */ drm_sched_submit_stop(); /* Wait for the channel to be idle, namely jobs in flight to complete. */ nouveau_channel_idle(); /* Stop the scheduler to free jobs from the pending_list. Ring must be idle at this * point, otherwise me might leak jobs. Feels more like a workaround to free * finished jobs. */ drm_sched_stop(); /* Free jobs from the entity queue. */ drm_sched_entity_fini(); /* Probably not even needed in this case. */ drm_sched_fini(); This doesn't look very straightforward though. I wonder if other drivers feeding firmware schedulers have similar cases. Maybe something like drm_sched_teardown(), which would stop job submission, wait for pending jobs to finish and subsequently free them up would makes sense? exec queue == gpu scheduler + entity in Xe We kinda invented our own flow with reference counting + use the TDR for cleanup. Thanks for the detailed explanation. In case of making it driver specific I thought about something similar, pretty much the same reference counting, but instead of the TDR, let jobs from the entity just return -ECANCELED from job_run() and also signal pending jobs with the same error code. On the other hand, I don't really want scheduler and job structures to potentially outlive the channel. Which is where I think I'd be nice to avoid consuming all the queued up jobs from the entity in the first place, stop the schdeduler with drm_sched_submit_stop(), signal all pending_jobs with -ECANCELED and call the free_job() callbacks right away. The latter I could probably do in Nouveau as well, however, it kinda feels wrong to do all that within the driver. Also, I was wondering how existing drivers using the GPU scheduler handle that. It seems like they just rely on the pending_list of the scheduler being empty once drm_sched_fini() is called. Admittedly, that's pretty likely (never to happen) since it's typically called on driver remove, but I don't see how that's actually ensured. Am I missing something? We have a creation ref for the exec queue plus each job takes a ref to the exec queue. On exec queue close [1][2] (whether that be IOCTL or FD close) we drop the creation reference and call a vfunc for killing thr exec queue. The firmware implementation is here [3]. If you read through it just sets the TDR to the minimum value [4], the TDR will kick any running jobs the off the hardware, signals the jobs fences, any jobs waiting on dependencies eventually flush out via run_job + TDR for cleanup without going on the hardware, the exec queue reference count goes to zero once all jobs are flushed out, we trigger the exec queue clean up flow and finally free all memory for the exec queue. Using the TDR in this way is how we teardown an exec queue for other reasons too (user page fault, user job times out, user job hang detected by firmware, device reset, etc...). This all works rather nicely and is a single code path for all of these cases. I'm no sure if this can be made any more generic nor do I really see the need too (at least I don't see Xe needing a generic solution). Hope this helps, Matt [1] https://gitlab.freedesktop.org/drm/xe/kernel/-/blob/drm-xe-next/drivers/gpu/drm/xe/xe_exec_queue.c#L911 [2] https://gitlab.freedesktop.org/drm/xe/kernel/-/blob/drm-xe-next/drivers/gpu/drm/xe/xe_device.c#L77 [3] https://gitlab.freedesktop.org/drm/xe/kernel/-/tree/drm-xe-next/drivers/gpu/drm/xe#L1184 [4]
Re: [PATCH RFC v5 02/10] drm: Introduce solid fill DRM plane property
On Tue, 29 Aug 2023 at 02:45, Jessica Zhang wrote:
>
>
>
> On 8/8/2023 3:57 PM, Jessica Zhang wrote:
> >
> >
> > On 8/7/2023 6:07 PM, Dmitry Baryshkov wrote:
> >>
> >>
> >> On 8 August 2023 00:41:07 GMT+03:00, Jessica Zhang
> >> wrote:
> >>>
> >>>
> >>> On 8/4/2023 6:27 AM, Dmitry Baryshkov wrote:
> On Fri, 28 Jul 2023 at 20:03, Jessica Zhang
> wrote:
> >
> > Document and add support for solid_fill property to drm_plane. In
> > addition, add support for setting and getting the values for
> > solid_fill.
> >
> > To enable solid fill planes, userspace must assign a property blob to
> > the "solid_fill" plane property containing the following information:
> >
> > struct drm_mode_solid_fill {
> > u32 version;
> > u32 r, g, b;
> > };
> >
> > Signed-off-by: Jessica Zhang
> > ---
> >drivers/gpu/drm/drm_atomic_state_helper.c | 9 +
> >drivers/gpu/drm/drm_atomic_uapi.c | 55
> > +++
> >drivers/gpu/drm/drm_blend.c | 30 +
> >include/drm/drm_blend.h | 1 +
> >include/drm/drm_plane.h | 35
> >include/uapi/drm/drm_mode.h | 24 ++
> >6 files changed, 154 insertions(+)
> >
>
> [skipped most of the patch]
>
> > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > index 43691058d28f..53c8efa5ad7f 100644
> > --- a/include/uapi/drm/drm_mode.h
> > +++ b/include/uapi/drm/drm_mode.h
> > @@ -259,6 +259,30 @@ struct drm_mode_modeinfo {
> > char name[DRM_DISPLAY_MODE_LEN];
> >};
> >
> > +/**
> > + * struct drm_mode_solid_fill - User info for solid fill planes
> > + *
> > + * This is the userspace API solid fill information structure.
> > + *
> > + * Userspace can enable solid fill planes by assigning the plane
> > "solid_fill"
> > + * property to a blob containing a single drm_mode_solid_fill
> > struct populated with an RGB323232
> > + * color and setting the pixel source to "SOLID_FILL".
> > + *
> > + * For information on the plane property, see
> > drm_plane_create_solid_fill_property()
> > + *
> > + * @version: Version of the blob. Currently, there is only support
> > for version == 1
> > + * @r: Red color value of single pixel
> > + * @g: Green color value of single pixel
> > + * @b: Blue color value of single pixel
> > + */
> > +struct drm_mode_solid_fill {
> > + __u32 version;
> > + __u32 r;
> > + __u32 g;
> > + __u32 b;
>
> Another thought about the drm_mode_solid_fill uABI. I still think we
> should add alpha here. The reason is the following:
>
> It is true that we have drm_plane_state::alpha and the plane's
> "alpha" property. However it is documented as "the plane-wide opacity
> [...] It can be combined with pixel alpha. The pixel values in the
> framebuffers are expected to not be pre-multiplied by the global alpha
> associated to the plane.".
>
> I can imagine a use case, when a user might want to enable plane-wide
> opacity, set "pixel blend mode" to "Coverage" and then switch between
> partially opaque framebuffer and partially opaque solid-fill without
> touching the plane's alpha value.
> >>>
> >>> Hi Dmitry,
> >>>
> >>> I don't really agree that adding a solid fill alpha would be a good
> >>> idea. Since the intent behind solid fill is to have a single color
> >>> for the entire plane, I think it makes more sense to have solid fill
> >>> rely on the global plane alpha.
> >>>
> >>> As stated in earlier discussions, I think having both a
> >>> solid_fill.alpha and a plane_state.alpha would be redundant and serve
> >>> to confuse the user as to which one to set.
> >>
> >> That depends on the blending mode: in Coverage mode one has
> >> independent plane and contents alpha values. And I consider alpha
> >> value to be a part of the colour in the rgba/bgra modes.
> >
> > Acked -- taking Sebastian's concern into consideration, I think I'll
> > have "PIXEL_SOURCE_SOLID_FILL_RGB" and add a separate
> > "PIXEL_SOURCE_SOLID_FILL_RGBA".
>
> Hi Dmitry,
>
> Since it looks like there's still some ongoing discussion with Pekka
> about whether to support an RGBA solid fill source, I'll just leave a
> note to add an RGBA source in the future.
Sure! Let's get the basic framework landed. After that we can extend
it with RGBA, if that seems sensible.
>
> Thanks,
>
> Jessica Zhang
>
> >
> > Thanks,
> >
> > Jessica Zhang
> >
> >>
> >>
> >>>
> >>> Thanks,
> >>>
> >>> Jessica Zhang
> >>>
>
> --
> With best wishes
> Dmitry
> >>
> >> --
> >> With best wishes
> >> Dmitry
--
With best wishes
Dmitry
[PATCH RFC v6 10/10] drm/msm/dpu: Add solid fill and pixel source properties
Add solid_fill and pixel_source properties to DPU plane
Reviewed-by: Dmitry Baryshkov
Signed-off-by: Jessica Zhang
---
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index 639ecbeeacf8..fda20c5e43b6 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -1453,6 +1453,8 @@ struct drm_plane *dpu_plane_init(struct drm_device *dev,
DPU_ERROR("failed to install zpos property, rc = %d\n", ret);
drm_plane_create_alpha_property(plane);
+ drm_plane_create_solid_fill_property(plane);
+ drm_plane_create_pixel_source_property(plane,
BIT(DRM_PLANE_PIXEL_SOURCE_SOLID_FILL));
drm_plane_create_blend_mode_property(plane,
BIT(DRM_MODE_BLEND_PIXEL_NONE) |
BIT(DRM_MODE_BLEND_PREMULTI) |
--
2.42.0
[PATCH RFC v6 08/10] drm/msm/dpu: Allow NULL FBs in atomic commit
Since solid fill planes allow for a NULL framebuffer in a valid commit,
add NULL framebuffer checks to atomic commit calls within DPU.
Reviewed-by: Dmitry Baryshkov
Signed-off-by: Jessica Zhang
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 9 ++-
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 41 ---
2 files changed, 34 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 8ce7586e2ddf..5e845510e8c1 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -451,6 +451,7 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc
*crtc,
struct drm_plane_state *state;
struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
struct dpu_plane_state *pstate = NULL;
+ const struct msm_format *fmt;
struct dpu_format *format;
struct dpu_hw_ctl *ctl = mixer->lm_ctl;
@@ -470,7 +471,13 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc
*crtc,
pstate = to_dpu_plane_state(state);
fb = state->fb;
- format = to_dpu_format(msm_framebuffer_format(pstate->base.fb));
+ if (drm_plane_solid_fill_enabled(state))
+ fmt = dpu_get_msm_format(&_dpu_crtc_get_kms(crtc)->base,
+ DRM_FORMAT_ABGR, 0);
+ else
+ fmt = msm_framebuffer_format(pstate->base.fb);
+
+ format = to_dpu_format(fmt);
if (pstate->stage == DPU_STAGE_BASE && format->alpha_enable)
bg_alpha_enable = true;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index c2aaaded07ed..114c803ff99b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -837,8 +837,13 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
pipe_cfg->dst_rect = new_plane_state->dst;
- fb_rect.x2 = new_plane_state->fb->width;
- fb_rect.y2 = new_plane_state->fb->height;
+ if (drm_plane_solid_fill_enabled(new_plane_state))
+ return 0;
+
+ if (new_plane_state->pixel_source == DRM_PLANE_PIXEL_SOURCE_FB &&
new_plane_state->fb) {
+ fb_rect.x2 = new_plane_state->fb->width;
+ fb_rect.y2 = new_plane_state->fb->height;
+ }
/* Ensure fb size is supported */
if (drm_rect_width(_rect) > MAX_IMG_WIDTH ||
@@ -1082,21 +1087,32 @@ static void dpu_plane_sspp_atomic_update(struct
drm_plane *plane)
struct drm_crtc *crtc = state->crtc;
struct drm_framebuffer *fb = state->fb;
bool is_rt_pipe;
- const struct dpu_format *fmt =
- to_dpu_format(msm_framebuffer_format(fb));
+ const struct dpu_format *fmt;
struct dpu_sw_pipe_cfg *pipe_cfg = >pipe_cfg;
struct dpu_sw_pipe_cfg *r_pipe_cfg = >r_pipe_cfg;
struct dpu_kms *kms = _dpu_plane_get_kms(>base);
struct msm_gem_address_space *aspace = kms->base.aspace;
struct dpu_hw_fmt_layout layout;
bool layout_valid = false;
- int ret;
- ret = dpu_format_populate_layout(aspace, fb, );
- if (ret)
- DPU_ERROR_PLANE(pdpu, "failed to get format layout, %d\n", ret);
- else
- layout_valid = true;
+ if (state->pixel_source == DRM_PLANE_PIXEL_SOURCE_FB && fb) {
+ int ret;
+
+ fmt = to_dpu_format(msm_framebuffer_format(fb));
+
+ ret = dpu_format_populate_layout(aspace, fb, );
+ if (ret)
+ DPU_ERROR_PLANE(pdpu, "failed to get format layout,
%d\n", ret);
+ else
+ layout_valid = true;
+
+ DPU_DEBUG_PLANE(pdpu, "FB[%u] " DRM_RECT_FP_FMT "->crtc%u "
DRM_RECT_FMT
+ ", %4.4s ubwc %d\n", fb->base.id,
DRM_RECT_FP_ARG(>src),
+ crtc->base.id, DRM_RECT_ARG(>dst),
+ (char *)>base.pixel_format,
DPU_FORMAT_IS_UBWC(fmt));
+ } else {
+ fmt = dpu_get_dpu_format(DRM_FORMAT_ABGR);
+ }
pstate->pending = true;
@@ -1104,11 +1120,6 @@ static void dpu_plane_sspp_atomic_update(struct
drm_plane *plane)
pstate->needs_qos_remap |= (is_rt_pipe != pdpu->is_rt_pipe);
pdpu->is_rt_pipe = is_rt_pipe;
- DPU_DEBUG_PLANE(pdpu, "FB[%u] " DRM_RECT_FP_FMT "->crtc%u " DRM_RECT_FMT
- ", %4.4s ubwc %d\n", fb->base.id,
DRM_RECT_FP_ARG(>src),
- crtc->base.id, DRM_RECT_ARG(>dst),
- (char *)>base.pixel_format,
DPU_FORMAT_IS_UBWC(fmt));
-
dpu_plane_sspp_update_pipe(plane, pipe, pipe_cfg, fmt,
drm_mode_vrefresh(>mode),
[PATCH RFC v6 05/10] drm/atomic: Add solid fill data to plane state dump
Add solid_fill property data to the atomic plane state dump.
Signed-off-by: Jessica Zhang
---
drivers/gpu/drm/drm_atomic.c | 4
drivers/gpu/drm/drm_plane.c | 8
include/drm/drm_plane.h | 3 +++
3 files changed, 15 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index bcecb64ccfad..3cb599b3304a 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -717,6 +717,10 @@ static void drm_atomic_plane_print_state(struct
drm_printer *p,
drm_printf(p, "\tfb=%u\n", state->fb ? state->fb->base.id : 0);
if (state->fb)
drm_framebuffer_print_info(p, 2, state->fb);
+ drm_printf(p, "\tsolid_fill=%u\n",
+ state->solid_fill_blob ?
state->solid_fill_blob->base.id : 0);
+ if (state->solid_fill_blob)
+ drm_plane_solid_fill_print_info(p, 2, state);
drm_printf(p, "\tcrtc-pos=" DRM_RECT_FMT "\n", DRM_RECT_ARG());
drm_printf(p, "\tsrc-pos=" DRM_RECT_FP_FMT "\n", DRM_RECT_FP_ARG());
drm_printf(p, "\trotation=%x\n", state->rotation);
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 559d101162ba..6244b622a21a 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -1495,6 +1495,14 @@ __drm_plane_get_damage_clips(const struct
drm_plane_state *state)
state->fb_damage_clips->data : NULL);
}
+void drm_plane_solid_fill_print_info(struct drm_printer *p, unsigned int
indent,
+const struct drm_plane_state *state)
+{
+ drm_printf_indent(p, indent, "r=0x%x\n", state->solid_fill.r);
+ drm_printf_indent(p, indent, "g=0x%x\n", state->solid_fill.g);
+ drm_printf_indent(p, indent, "b=0x%x\n", state->solid_fill.b);
+}
+
/**
* drm_plane_get_damage_clips - Returns damage clips.
* @state: Plane state.
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 49995c4be2ab..a58f84b6bd5e 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -1001,6 +1001,9 @@ drm_plane_get_damage_clips_count(const struct
drm_plane_state *state);
struct drm_mode_rect *
drm_plane_get_damage_clips(const struct drm_plane_state *state);
+void drm_plane_solid_fill_print_info(struct drm_printer *p, unsigned int
indent,
+const struct drm_plane_state *state);
+
int drm_plane_create_scaling_filter_property(struct drm_plane *plane,
unsigned int supported_filters);
--
2.42.0
[PATCH RFC v6 00/10] Support for Solid Fill Planes
Some drivers support hardware that have optimizations for solid fill planes. This series aims to expose these capabilities to userspace as some compositors have a solid fill flag (ex. SOLID_COLOR in the Android hardware composer HAL) that can be set by apps like the Android Gears app. In order to expose this capability to userspace, this series will: - Introduce solid_fill and pixel_source properties to allow userspace to toggle between FB and solid fill sources - Loosen NULL FB checks within the DRM atomic commit callstack to allow for NULL FB when solid fill is enabled. - Add NULL FB checks in methods where FB was previously assumed to be non-NULL - Have MSM DPU driver use drm_plane_state.solid_fill instead of dpu_plane_state.color_fill Note: The solid fill planes feature depends on both the solid_fill *and* pixel_source properties. To use this feature, userspace can set the solid_fill property to a blob containing the appropriate version number and solid fill color (in RGB323232 format) and and setting the pixel_source property to DRM_PLANE_PIXEL_SOURCE_COLOR. This will disable memory fetch and the resulting plane will display the color specified by the solid_fill blob. Currently, there's only one version of the solid_fill blob property. However if other drivers want to support a similar feature, but require more than just the solid fill color, they can extend this feature by creating additional versions of the drm_solid_fill struct. This 2 property approach was chosen because passing in a special 1x1 FB with the necessary color information would have unecessary overhead that does not reflect the behavior of the solid fill feature. In addition, assigning the solid fill blob to FB_ID would require loosening some core drm_property checks that might cause unwanted side effects elsewhere. --- Changes in v6: - Have _dpu_plane_color_fill() take in a single ABGR color instead of having separate alpha and BGR color parameters (Dmitry) - Drop plane->state->pixel_source != DRM_PLANE_PIXEL_SOURCE_FB check in SetPlane ioctl (Dmitry) - Add DRM_PLANE_PIXEL_SOURCE_NONE as a default pixel source (Sebastian) - Dropped versioning from solid fill property blob (Dmitry) - Use DRM_ENUM_NAME_FN (Dmitry) - Use drm_atomic_replace_property_blob_from_id() (Dmitry) - drm_atomic_check_fb -> drm_atomic_plane_check_fb (Dmitry) - Group redundant NULL FB checks (Dmitry) - Squashed drm_plane_needs_disable() implementation with DRM_PLANE_PIXEL_SOURCE_NONE declaration (Sebastian) - Add comment to support RGBA solid fill color in the future (Dmitry) - Link to v5: https://lore.kernel.org/r/20230728-solid-fill-v5-0-053dbefa9...@quicinc.com Changes in v5: - Added support for PIXEL_SOURCE_NONE (Sebastian) - Added WARN_ON() in drm_plane_has_visible_data() if pixel_source isn't set (Dmitry) - Added debugfs support for both properties (Dmitry) - Corrected u32 to u8 conversion (Pekka) - Moved drm_solid_fill_info struct and related documentation to include/uapi (Pekka) - Changed drm_solid_fill_info.version to __u32 for data alignment (Pekka) - Added more detailed UAPI and kernel documentation (Pekka) - Reordered patch series so that the pixel_source property is introduced before solid_fill (Dmitry) - Fixed inconsistent ABGR/RGBA format declaration (Pekka) - Reset pixel_source to FB in drm_mode_setplane() (Dmitry) - Rename supported_sources to extra_sources (Dmitry) - Only destroy old solid_fill blob state if new state is valid (Pekka) - Link to v4: https://lore.kernel.org/r/20230404-solid-fill-v4-0-f4ec0caa7...@quicinc.com Changes in v4: - Rebased onto latest kernel - Reworded cover letter for clarity (Dmitry) - Reworded commit messages for clarity - Split existing changes into smaller commits - Added pixel_source enum property (Dmitry, Pekka, Ville) - Updated drm-kms comment docs with pixel_source and solid_fill properties (Dmitry) - Inlined drm_atomic_convert_solid_fill_info() (Dmitry) - Passed in plane state alpha value to _dpu_plane_color_fill_pipe() - Link to v3: https://lore.kernel.org/r/20230104234036.636-1-quic_jessz...@quicinc.com Changes in v3: - Fixed some logic errors in atomic checks (Dmitry) - Introduced drm_plane_has_visible_data() and drm_atomic_check_fb() helper methods (Dmitry) - Fixed typo in drm_solid_fill struct documentation - Created drm_plane_has_visible_data() helper and corrected CRTC and FB NULL-check logic (Dmitry) - Merged `if (fb)` blocks in drm_atomic_plane_check() and abstracted them into helper method (Dmitry) - Inverted `if (solid_fill_enabled) else if (fb)` check order (Dmitry) - Fixed indentation (Dmitry) Changes in v2: - Dropped SOLID_FILL_FORMAT property (Simon) - Switched to implementing solid_fill property as a blob (Simon, Dmitry) - Added drm_solid_fill and drm_solid_fill_info structs (Simon) - Changed to checks for if solid_fill_blob is set (Dmitry) - Abstracted (plane_state && !solid_fill_blob) checks to helper method (Dmitry) - Removed
[PATCH RFC v6 01/10] drm: Introduce pixel_source DRM plane property
Add support for pixel_source property to drm_plane and related
documentation. In addition, force pixel_source to
DRM_PLANE_PIXEL_SOURCE_FB in DRM_IOCTL_MODE_SETPLANE as to not break
legacy userspace.
This enum property will allow user to specify a pixel source for the
plane. Possible pixel sources will be defined in the
drm_plane_pixel_source enum.
Currently, the only pixel sources are DRM_PLANE_PIXEL_SOURCE_FB (the
default value) and DRM_PLANE_PIXEL_SOURCE_NONE.
Signed-off-by: Jessica Zhang
---
drivers/gpu/drm/drm_atomic_state_helper.c | 1 +
drivers/gpu/drm/drm_atomic_uapi.c | 4 ++
drivers/gpu/drm/drm_blend.c | 90 +++
drivers/gpu/drm/drm_plane.c | 19 +--
include/drm/drm_blend.h | 2 +
include/drm/drm_plane.h | 21
6 files changed, 133 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c
b/drivers/gpu/drm/drm_atomic_state_helper.c
index 784e63d70a42..01638c51ce0a 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -252,6 +252,7 @@ void __drm_atomic_helper_plane_state_reset(struct
drm_plane_state *plane_state,
plane_state->alpha = DRM_BLEND_ALPHA_OPAQUE;
plane_state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
+ plane_state->pixel_source = DRM_PLANE_PIXEL_SOURCE_FB;
if (plane->color_encoding_property) {
if (!drm_object_property_get_default_value(>base,
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
b/drivers/gpu/drm/drm_atomic_uapi.c
index d867e7f9f2cd..454f980e16c9 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -544,6 +544,8 @@ static int drm_atomic_plane_set_property(struct drm_plane
*plane,
state->src_w = val;
} else if (property == config->prop_src_h) {
state->src_h = val;
+ } else if (property == plane->pixel_source_property) {
+ state->pixel_source = val;
} else if (property == plane->alpha_property) {
state->alpha = val;
} else if (property == plane->blend_mode_property) {
@@ -616,6 +618,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
*val = state->src_w;
} else if (property == config->prop_src_h) {
*val = state->src_h;
+ } else if (property == plane->pixel_source_property) {
+ *val = state->pixel_source;
} else if (property == plane->alpha_property) {
*val = state->alpha;
} else if (property == plane->blend_mode_property) {
diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
index 6e74de833466..c3c57bae06b7 100644
--- a/drivers/gpu/drm/drm_blend.c
+++ b/drivers/gpu/drm/drm_blend.c
@@ -185,6 +185,21 @@
* plane does not expose the "alpha" property, then this is
* assumed to be 1.0
*
+ * pixel_source:
+ * pixel_source is set up with drm_plane_create_pixel_source_property().
+ * It is used to toggle the active source of pixel data for the plane.
+ * The plane will only display data from the set pixel_source -- any
+ * data from other sources will be ignored.
+ *
+ * Possible values:
+ *
+ * "NONE":
+ * No active pixel source.
+ * Committing with a NONE pixel source will disable the plane.
+ *
+ * "FB":
+ * Framebuffer source set by the "FB_ID" property.
+ *
* Note that all the property extensions described here apply either to the
* plane or the CRTC (e.g. for the background color, which currently is not
* exposed and assumed to be black).
@@ -615,3 +630,78 @@ int drm_plane_create_blend_mode_property(struct drm_plane
*plane,
return 0;
}
EXPORT_SYMBOL(drm_plane_create_blend_mode_property);
+
+static const struct drm_prop_enum_list drm_pixel_source_enum_list[] = {
+ { DRM_PLANE_PIXEL_SOURCE_NONE, "NONE" },
+ { DRM_PLANE_PIXEL_SOURCE_FB, "FB" },
+};
+
+/**
+ * drm_plane_create_pixel_source_property - create a new pixel source property
+ * @plane: DRM plane
+ * @extra_sources: Bitmask of additional supported pixel_sources for the
driver.
+ *DRM_PLANE_PIXEL_SOURCE_FB and DRM_PLANE_PIXEL_SOURCE_NONE
will
+ *always be enabled as supported sources.
+ *
+ * This creates a new property describing the current source of pixel data for
the
+ * plane. The pixel_source will be initialized as DRM_PLANE_PIXEL_SOURCE_FB by
default.
+ *
+ * Drivers can set a custom default source by overriding the pixel_source
value in
+ * drm_plane_funcs.reset()
+ *
+ * The property is exposed to userspace as an enumeration property called
+ * "pixel_source" and has the following enumeration values:
+ *
+ * "NONE":
+ * No active pixel source
+ *
+ * "FB":
+ * Framebuffer pixel source
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
[PATCH RFC v6 09/10] drm/msm/dpu: Use DRM solid_fill property
Drop DPU_PLANE_COLOR_FILL_FLAG and check the DRM solid_fill property to
determine if the plane is solid fill. In addition drop the DPU plane
color_fill field as we can now use drm_plane_state.solid_fill instead,
and pass in drm_plane_state.alpha to _dpu_plane_color_fill_pipe() to
allow userspace to configure the alpha value for the solid fill color.
Reviewed-by: Dmitry Baryshkov
Signed-off-by: Jessica Zhang
---
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 37 +--
1 file changed, 25 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index 114c803ff99b..639ecbeeacf8 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -42,7 +42,6 @@
#define SHARP_SMOOTH_THR_DEFAULT 8
#define SHARP_NOISE_THR_DEFAULT2
-#define DPU_PLANE_COLOR_FILL_FLAG BIT(31)
#define DPU_ZPOS_MAX 255
/*
@@ -82,7 +81,6 @@ struct dpu_plane {
enum dpu_sspp pipe;
- uint32_t color_fill;
bool is_error;
bool is_rt_pipe;
const struct dpu_mdss_cfg *catalog;
@@ -606,19 +604,35 @@ static void _dpu_plane_color_fill_pipe(struct
dpu_plane_state *pstate,
_dpu_plane_setup_scaler(pipe, fmt, true, _cfg, pstate->rotation);
}
+static uint32_t _dpu_plane_get_abgr_fill_color(struct drm_plane_state *state)
+{
+ struct drm_solid_fill solid_fill = state->solid_fill;
+
+ uint32_t ret = 0;
+ uint8_t a = state->alpha & 0xFF;
+ uint8_t b = solid_fill.b >> 24;
+ uint8_t g = solid_fill.g >> 24;
+ uint8_t r = solid_fill.r >> 24;
+
+ ret |= a << 24;
+ ret |= b << 16;
+ ret |= g << 8;
+ ret |= r;
+
+ return ret;
+}
+
/**
* _dpu_plane_color_fill - enables color fill on plane
* @pdpu: Pointer to DPU plane object
* @color: RGB fill color value, [23..16] Blue, [15..8] Green, [7..0] Red
* @alpha: 8-bit fill alpha value, 255 selects 100% alpha
*/
-static void _dpu_plane_color_fill(struct dpu_plane *pdpu,
- uint32_t color, uint32_t alpha)
+static void _dpu_plane_color_fill(struct dpu_plane *pdpu, uint32_t color)
{
const struct dpu_format *fmt;
const struct drm_plane *plane = >base;
struct dpu_plane_state *pstate = to_dpu_plane_state(plane->state);
- u32 fill_color = (color & 0xFF) | ((alpha & 0xFF) << 24);
DPU_DEBUG_PLANE(pdpu, "\n");
@@ -633,11 +647,11 @@ static void _dpu_plane_color_fill(struct dpu_plane *pdpu,
/* update sspp */
_dpu_plane_color_fill_pipe(pstate, >pipe,
>pipe_cfg.dst_rect,
- fill_color, fmt);
+ color, fmt);
if (pstate->r_pipe.sspp)
_dpu_plane_color_fill_pipe(pstate, >r_pipe,
>r_pipe_cfg.dst_rect,
- fill_color, fmt);
+ color, fmt);
}
static int dpu_plane_prepare_fb(struct drm_plane *plane,
@@ -976,10 +990,9 @@ void dpu_plane_flush(struct drm_plane *plane)
*/
if (pdpu->is_error)
/* force white frame with 100% alpha pipe output on error */
- _dpu_plane_color_fill(pdpu, 0xFF, 0xFF);
- else if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG)
- /* force 100% alpha */
- _dpu_plane_color_fill(pdpu, pdpu->color_fill, 0xFF);
+ _dpu_plane_color_fill(pdpu, 0x);
+ else if (drm_plane_solid_fill_enabled(plane->state))
+ _dpu_plane_color_fill(pdpu,
_dpu_plane_get_abgr_fill_color(plane->state));
else {
dpu_plane_flush_csc(pdpu, >pipe);
dpu_plane_flush_csc(pdpu, >r_pipe);
@@ -1024,7 +1037,7 @@ static void dpu_plane_sspp_update_pipe(struct drm_plane
*plane,
}
/* override for color fill */
- if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG) {
+ if (drm_plane_solid_fill_enabled(plane->state)) {
_dpu_plane_set_qos_ctrl(plane, pipe, false);
/* skip remaining processing on color fill */
--
2.42.0
[PATCH RFC v6 03/10] drm: Add solid fill pixel source
Add "SOLID_FILL" as a valid pixel source. If the pixel_source property is
set to "SOLID_FILL", it will display data from the drm_plane "solid_fill"
blob property.
Reviewed-by: Dmitry Baryshkov
Signed-off-by: Jessica Zhang
---
drivers/gpu/drm/drm_blend.c | 10 +-
include/drm/drm_plane.h | 1 +
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
index 273021cc21c8..1016a206ca0c 100644
--- a/drivers/gpu/drm/drm_blend.c
+++ b/drivers/gpu/drm/drm_blend.c
@@ -200,6 +200,9 @@
* "FB":
* Framebuffer source set by the "FB_ID" property.
*
+ * "SOLID_FILL":
+ * Solid fill color source set by the "solid_fill" property.
+ *
* solid_fill:
* solid_fill is set up with drm_plane_create_solid_fill_property(). It
* contains pixel data that drivers can use to fill a plane.
@@ -638,6 +641,7 @@ EXPORT_SYMBOL(drm_plane_create_blend_mode_property);
static const struct drm_prop_enum_list drm_pixel_source_enum_list[] = {
{ DRM_PLANE_PIXEL_SOURCE_NONE, "NONE" },
{ DRM_PLANE_PIXEL_SOURCE_FB, "FB" },
+ { DRM_PLANE_PIXEL_SOURCE_SOLID_FILL, "SOLID_FILL" },
};
/**
@@ -662,6 +666,9 @@ static const struct drm_prop_enum_list
drm_pixel_source_enum_list[] = {
* "FB":
* Framebuffer pixel source
*
+ * "SOLID_FILL":
+ * Solid fill color pixel source
+ *
* Returns:
* Zero on success, negative errno on failure.
*/
@@ -671,7 +678,8 @@ int drm_plane_create_pixel_source_property(struct drm_plane
*plane,
struct drm_device *dev = plane->dev;
struct drm_property *prop;
static const unsigned int valid_source_mask =
BIT(DRM_PLANE_PIXEL_SOURCE_FB) |
-
BIT(DRM_PLANE_PIXEL_SOURCE_NONE);
+
BIT(DRM_PLANE_PIXEL_SOURCE_NONE) |
+
BIT(DRM_PLANE_PIXEL_SOURCE_SOLID_FILL);
int i;
/* FB is supported by default */
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index a38e18bfb43e..49995c4be2ab 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -43,6 +43,7 @@ enum drm_scaling_filter {
enum drm_plane_pixel_source {
DRM_PLANE_PIXEL_SOURCE_NONE,
DRM_PLANE_PIXEL_SOURCE_FB,
+ DRM_PLANE_PIXEL_SOURCE_SOLID_FILL,
DRM_PLANE_PIXEL_SOURCE_MAX
};
--
2.42.0
[PATCH RFC v6 04/10] drm/atomic: Add pixel source to plane state dump
Add pixel source to the atomic plane state dump
Signed-off-by: Jessica Zhang
---
drivers/gpu/drm/drm_atomic.c| 1 +
drivers/gpu/drm/drm_blend.c | 1 +
drivers/gpu/drm/drm_crtc_internal.h | 1 +
3 files changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index b4c6ffc438da..bcecb64ccfad 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -713,6 +713,7 @@ static void drm_atomic_plane_print_state(struct drm_printer
*p,
drm_printf(p, "plane[%u]: %s\n", plane->base.id, plane->name);
drm_printf(p, "\tcrtc=%s\n", state->crtc ? state->crtc->name :
"(null)");
+ drm_printf(p, "\tpixel-source=%s\n",
drm_get_pixel_source_name(state->pixel_source));
drm_printf(p, "\tfb=%u\n", state->fb ? state->fb->base.id : 0);
if (state->fb)
drm_framebuffer_print_info(p, 2, state->fb);
diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
index 1016a206ca0c..3d484aa1e029 100644
--- a/drivers/gpu/drm/drm_blend.c
+++ b/drivers/gpu/drm/drm_blend.c
@@ -643,6 +643,7 @@ static const struct drm_prop_enum_list
drm_pixel_source_enum_list[] = {
{ DRM_PLANE_PIXEL_SOURCE_FB, "FB" },
{ DRM_PLANE_PIXEL_SOURCE_SOLID_FILL, "SOLID_FILL" },
};
+DRM_ENUM_NAME_FN(drm_get_pixel_source_name, drm_pixel_source_enum_list);
/**
* drm_plane_create_pixel_source_property - create a new pixel source property
diff --git a/drivers/gpu/drm/drm_crtc_internal.h
b/drivers/gpu/drm/drm_crtc_internal.h
index 501a10edd0e1..7bc93ba449d5 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -267,6 +267,7 @@ int drm_plane_check_pixel_format(struct drm_plane *plane,
u32 format, u64 modifier);
struct drm_mode_rect *
__drm_plane_get_damage_clips(const struct drm_plane_state *state);
+const char *drm_get_pixel_source_name(int val);
/* drm_bridge.c */
void drm_bridge_detach(struct drm_bridge *bridge);
--
2.42.0
[PATCH RFC v6 06/10] drm/atomic: Move framebuffer checks to helper
Currently framebuffer checks happen directly in
drm_atomic_plane_check(). Move these checks into their own helper
method.
Reviewed-by: Dmitry Baryshkov
Signed-off-by: Jessica Zhang
---
drivers/gpu/drm/drm_atomic.c | 130 ---
1 file changed, 73 insertions(+), 57 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 3cb599b3304a..cc0e93d19e15 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -580,6 +580,76 @@ plane_switching_crtc(const struct drm_plane_state
*old_plane_state,
return true;
}
+static int drm_atomic_plane_check_fb(const struct drm_plane_state *state)
+{
+ struct drm_plane *plane = state->plane;
+ const struct drm_framebuffer *fb = state->fb;
+ struct drm_mode_rect *clips;
+
+ uint32_t num_clips;
+ unsigned int fb_width, fb_height;
+ int ret;
+
+ /* Check whether this plane supports the fb pixel format. */
+ ret = drm_plane_check_pixel_format(plane, fb->format->format,
+ fb->modifier);
+
+ if (ret) {
+ drm_dbg_atomic(plane->dev,
+ "[PLANE:%d:%s] invalid pixel format %p4cc,
modifier 0x%llx\n",
+ plane->base.id, plane->name,
+ >format->format, fb->modifier);
+ return ret;
+ }
+
+ fb_width = fb->width << 16;
+ fb_height = fb->height << 16;
+
+ /* Make sure source coordinates are inside the fb. */
+ if (state->src_w > fb_width ||
+ state->src_x > fb_width - state->src_w ||
+ state->src_h > fb_height ||
+ state->src_y > fb_height - state->src_h) {
+ drm_dbg_atomic(plane->dev,
+ "[PLANE:%d:%s] invalid source coordinates "
+ "%u.%06ux%u.%06u+%u.%06u+%u.%06u (fb %ux%u)\n",
+ plane->base.id, plane->name,
+ state->src_w >> 16,
+ ((state->src_w & 0x) * 15625) >> 10,
+ state->src_h >> 16,
+ ((state->src_h & 0x) * 15625) >> 10,
+ state->src_x >> 16,
+ ((state->src_x & 0x) * 15625) >> 10,
+ state->src_y >> 16,
+ ((state->src_y & 0x) * 15625) >> 10,
+ fb->width, fb->height);
+ return -ENOSPC;
+ }
+
+ clips = __drm_plane_get_damage_clips(state);
+ num_clips = drm_plane_get_damage_clips_count(state);
+
+ /* Make sure damage clips are valid and inside the fb. */
+ while (num_clips > 0) {
+ if (clips->x1 >= clips->x2 ||
+ clips->y1 >= clips->y2 ||
+ clips->x1 < 0 ||
+ clips->y1 < 0 ||
+ clips->x2 > fb_width ||
+ clips->y2 > fb_height) {
+ drm_dbg_atomic(plane->dev,
+ "[PLANE:%d:%s] invalid damage clip %d %d
%d %d\n",
+ plane->base.id, plane->name, clips->x1,
+ clips->y1, clips->x2, clips->y2);
+ return -EINVAL;
+ }
+ clips++;
+ num_clips--;
+ }
+
+ return 0;
+}
+
/**
* drm_atomic_plane_check - check plane state
* @old_plane_state: old plane state to check
@@ -596,9 +666,6 @@ static int drm_atomic_plane_check(const struct
drm_plane_state *old_plane_state,
struct drm_plane *plane = new_plane_state->plane;
struct drm_crtc *crtc = new_plane_state->crtc;
const struct drm_framebuffer *fb = new_plane_state->fb;
- unsigned int fb_width, fb_height;
- struct drm_mode_rect *clips;
- uint32_t num_clips;
int ret;
/* either *both* CRTC and FB must be set, or neither */
@@ -625,17 +692,6 @@ static int drm_atomic_plane_check(const struct
drm_plane_state *old_plane_state,
return -EINVAL;
}
- /* Check whether this plane supports the fb pixel format. */
- ret = drm_plane_check_pixel_format(plane, fb->format->format,
- fb->modifier);
- if (ret) {
- drm_dbg_atomic(plane->dev,
- "[PLANE:%d:%s] invalid pixel format %p4cc,
modifier 0x%llx\n",
- plane->base.id, plane->name,
- >format->format, fb->modifier);
- return ret;
- }
-
/* Give drivers some help against integer overflows */
if (new_plane_state->crtc_w > INT_MAX ||
new_plane_state->crtc_x > INT_MAX - (int32_t)
new_plane_state->crtc_w ||
@@ -649,50 +705,10 @@ static int
[PATCH RFC v6 07/10] drm/atomic: Loosen FB atomic checks
Loosen the requirements for atomic and legacy commit so that, in cases
where pixel_source != FB, the commit can still go through.
This includes adding framebuffer NULL checks in other areas to account for
FB being NULL when non-FB pixel sources are enabled.
To disable a plane, the pixel_source must be NONE or the FB must be NULL
if pixel_source == FB.
Signed-off-by: Jessica Zhang
---
drivers/gpu/drm/drm_atomic.c| 20 +++-
drivers/gpu/drm/drm_atomic_helper.c | 36
include/drm/drm_atomic_helper.h | 4 ++--
include/drm/drm_plane.h | 29 +
4 files changed, 62 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index cc0e93d19e15..cdc6cfedd433 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -668,14 +668,14 @@ static int drm_atomic_plane_check(const struct
drm_plane_state *old_plane_state,
const struct drm_framebuffer *fb = new_plane_state->fb;
int ret;
- /* either *both* CRTC and FB must be set, or neither */
- if (crtc && !fb) {
- drm_dbg_atomic(plane->dev, "[PLANE:%d:%s] CRTC set but no FB\n",
+ /* either *both* CRTC and pixel source must be set, or neither */
+ if (crtc && !drm_plane_has_visible_data(new_plane_state)) {
+ drm_dbg_atomic(plane->dev, "[PLANE:%d:%s] CRTC set but no
visible data\n",
plane->base.id, plane->name);
return -EINVAL;
- } else if (fb && !crtc) {
- drm_dbg_atomic(plane->dev, "[PLANE:%d:%s] FB set but no CRTC\n",
- plane->base.id, plane->name);
+ } else if (drm_plane_has_visible_data(new_plane_state) && !crtc) {
+ drm_dbg_atomic(plane->dev, "[PLANE:%d:%s] Source %d has visible
data but no CRTC\n",
+ plane->base.id, plane->name,
new_plane_state->pixel_source);
return -EINVAL;
}
@@ -706,9 +706,11 @@ static int drm_atomic_plane_check(const struct
drm_plane_state *old_plane_state,
}
- ret = drm_atomic_plane_check_fb(new_plane_state);
- if (ret)
- return ret;
+ if (new_plane_state->pixel_source == DRM_PLANE_PIXEL_SOURCE_FB && fb) {
+ ret = drm_atomic_plane_check_fb(new_plane_state);
+ if (ret)
+ return ret;
+ }
if (plane_switching_crtc(old_plane_state, new_plane_state)) {
drm_dbg_atomic(plane->dev,
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index 41b8066f61ff..a176064ee27e 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -864,7 +864,7 @@ int drm_atomic_helper_check_plane_state(struct
drm_plane_state *plane_state,
*src = drm_plane_state_src(plane_state);
*dst = drm_plane_state_dest(plane_state);
- if (!fb) {
+ if (!drm_plane_has_visible_data(plane_state)) {
plane_state->visible = false;
return 0;
}
@@ -881,25 +881,29 @@ int drm_atomic_helper_check_plane_state(struct
drm_plane_state *plane_state,
return -EINVAL;
}
- drm_rect_rotate(src, fb->width << 16, fb->height << 16, rotation);
+ if (plane_state->pixel_source == DRM_PLANE_PIXEL_SOURCE_FB && fb) {
+ drm_rect_rotate(src, fb->width << 16, fb->height << 16,
rotation);
- /* Check scaling */
- hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale);
- vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale);
- if (hscale < 0 || vscale < 0) {
- drm_dbg_kms(plane_state->plane->dev,
- "Invalid scaling of plane\n");
- drm_rect_debug_print("src: ", _state->src, true);
- drm_rect_debug_print("dst: ", _state->dst, false);
- return -ERANGE;
- }
+ /* Check scaling */
+ hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale);
+ vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale);
- if (crtc_state->enable)
- drm_mode_get_hv_timing(_state->mode, , );
+ if (hscale < 0 || vscale < 0) {
+ drm_dbg_kms(plane_state->plane->dev,
+ "Invalid scaling of plane\n");
+ drm_rect_debug_print("src: ", _state->src, true);
+ drm_rect_debug_print("dst: ", _state->dst, false);
+ return -ERANGE;
+ }
- plane_state->visible = drm_rect_clip_scaled(src, dst, );
+ if (crtc_state->enable)
+ drm_mode_get_hv_timing(_state->mode, ,
);
- drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16, rotation);
+
[PATCH RFC v6 02/10] drm: Introduce solid fill DRM plane property
Document and add support for solid_fill property to drm_plane. In
addition, add support for setting and getting the values for solid_fill.
To enable solid fill planes, userspace must assign a property blob to
the "solid_fill" plane property containing the following information:
struct drm_mode_solid_fill {
u32 r, g, b;
};
Signed-off-by: Jessica Zhang
---
drivers/gpu/drm/drm_atomic_state_helper.c | 9
drivers/gpu/drm/drm_atomic_uapi.c | 26 ++
drivers/gpu/drm/drm_blend.c | 30 ++
include/drm/drm_blend.h | 1 +
include/drm/drm_plane.h | 36 +++
include/uapi/drm/drm_mode.h | 24 +
6 files changed, 126 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c
b/drivers/gpu/drm/drm_atomic_state_helper.c
index 01638c51ce0a..86fb876efbe6 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -254,6 +254,11 @@ void __drm_atomic_helper_plane_state_reset(struct
drm_plane_state *plane_state,
plane_state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
plane_state->pixel_source = DRM_PLANE_PIXEL_SOURCE_FB;
+ if (plane_state->solid_fill_blob) {
+ drm_property_blob_put(plane_state->solid_fill_blob);
+ plane_state->solid_fill_blob = NULL;
+ }
+
if (plane->color_encoding_property) {
if (!drm_object_property_get_default_value(>base,
plane->color_encoding_property,
@@ -336,6 +341,9 @@ void __drm_atomic_helper_plane_duplicate_state(struct
drm_plane *plane,
if (state->fb)
drm_framebuffer_get(state->fb);
+ if (state->solid_fill_blob)
+ drm_property_blob_get(state->solid_fill_blob);
+
state->fence = NULL;
state->commit = NULL;
state->fb_damage_clips = NULL;
@@ -385,6 +393,7 @@ void __drm_atomic_helper_plane_destroy_state(struct
drm_plane_state *state)
drm_crtc_commit_put(state->commit);
drm_property_blob_put(state->fb_damage_clips);
+ drm_property_blob_put(state->solid_fill_blob);
}
EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
b/drivers/gpu/drm/drm_atomic_uapi.c
index 454f980e16c9..1cae596ab693 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -316,6 +316,20 @@ drm_atomic_set_crtc_for_connector(struct
drm_connector_state *conn_state,
}
EXPORT_SYMBOL(drm_atomic_set_crtc_for_connector);
+static void drm_atomic_set_solid_fill_prop(struct drm_plane_state *state)
+{
+ struct drm_mode_solid_fill *user_info;
+
+ if (!state->solid_fill_blob)
+ return;
+
+ user_info = (struct drm_mode_solid_fill *)state->solid_fill_blob->data;
+
+ state->solid_fill.r = user_info->r;
+ state->solid_fill.g = user_info->g;
+ state->solid_fill.b = user_info->b;
+}
+
static void set_out_fence_for_crtc(struct drm_atomic_state *state,
struct drm_crtc *crtc, s32 __user *fence_ptr)
{
@@ -546,6 +560,15 @@ static int drm_atomic_plane_set_property(struct drm_plane
*plane,
state->src_h = val;
} else if (property == plane->pixel_source_property) {
state->pixel_source = val;
+ } else if (property == plane->solid_fill_property) {
+ ret = drm_atomic_replace_property_blob_from_id(dev,
+ >solid_fill_blob,
+ val, sizeof(struct drm_mode_solid_fill),
+ -1, );
+ if (ret)
+ return ret;
+
+ drm_atomic_set_solid_fill_prop(state);
} else if (property == plane->alpha_property) {
state->alpha = val;
} else if (property == plane->blend_mode_property) {
@@ -620,6 +643,9 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
*val = state->src_h;
} else if (property == plane->pixel_source_property) {
*val = state->pixel_source;
+ } else if (property == plane->solid_fill_property) {
+ *val = state->solid_fill_blob ?
+ state->solid_fill_blob->base.id : 0;
} else if (property == plane->alpha_property) {
*val = state->alpha;
} else if (property == plane->blend_mode_property) {
diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
index c3c57bae06b7..273021cc21c8 100644
--- a/drivers/gpu/drm/drm_blend.c
+++ b/drivers/gpu/drm/drm_blend.c
@@ -200,6 +200,10 @@
* "FB":
* Framebuffer source set by the "FB_ID" property.
*
+ * solid_fill:
+ * solid_fill is set up with drm_plane_create_solid_fill_property(). It
+ * contains pixel
Re: [PATCH RFC v5 02/10] drm: Introduce solid fill DRM plane property
On 8/8/2023 3:57 PM, Jessica Zhang wrote:
On 8/7/2023 6:07 PM, Dmitry Baryshkov wrote:
On 8 August 2023 00:41:07 GMT+03:00, Jessica Zhang
wrote:
On 8/4/2023 6:27 AM, Dmitry Baryshkov wrote:
On Fri, 28 Jul 2023 at 20:03, Jessica Zhang
wrote:
Document and add support for solid_fill property to drm_plane. In
addition, add support for setting and getting the values for
solid_fill.
To enable solid fill planes, userspace must assign a property blob to
the "solid_fill" plane property containing the following information:
struct drm_mode_solid_fill {
u32 version;
u32 r, g, b;
};
Signed-off-by: Jessica Zhang
---
drivers/gpu/drm/drm_atomic_state_helper.c | 9 +
drivers/gpu/drm/drm_atomic_uapi.c | 55
+++
drivers/gpu/drm/drm_blend.c | 30 +
include/drm/drm_blend.h | 1 +
include/drm/drm_plane.h | 35
include/uapi/drm/drm_mode.h | 24 ++
6 files changed, 154 insertions(+)
[skipped most of the patch]
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 43691058d28f..53c8efa5ad7f 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -259,6 +259,30 @@ struct drm_mode_modeinfo {
char name[DRM_DISPLAY_MODE_LEN];
};
+/**
+ * struct drm_mode_solid_fill - User info for solid fill planes
+ *
+ * This is the userspace API solid fill information structure.
+ *
+ * Userspace can enable solid fill planes by assigning the plane
"solid_fill"
+ * property to a blob containing a single drm_mode_solid_fill
struct populated with an RGB323232
+ * color and setting the pixel source to "SOLID_FILL".
+ *
+ * For information on the plane property, see
drm_plane_create_solid_fill_property()
+ *
+ * @version: Version of the blob. Currently, there is only support
for version == 1
+ * @r: Red color value of single pixel
+ * @g: Green color value of single pixel
+ * @b: Blue color value of single pixel
+ */
+struct drm_mode_solid_fill {
+ __u32 version;
+ __u32 r;
+ __u32 g;
+ __u32 b;
Another thought about the drm_mode_solid_fill uABI. I still think we
should add alpha here. The reason is the following:
It is true that we have drm_plane_state::alpha and the plane's
"alpha" property. However it is documented as "the plane-wide opacity
[...] It can be combined with pixel alpha. The pixel values in the
framebuffers are expected to not be pre-multiplied by the global alpha
associated to the plane.".
I can imagine a use case, when a user might want to enable plane-wide
opacity, set "pixel blend mode" to "Coverage" and then switch between
partially opaque framebuffer and partially opaque solid-fill without
touching the plane's alpha value.
Hi Dmitry,
I don't really agree that adding a solid fill alpha would be a good
idea. Since the intent behind solid fill is to have a single color
for the entire plane, I think it makes more sense to have solid fill
rely on the global plane alpha.
As stated in earlier discussions, I think having both a
solid_fill.alpha and a plane_state.alpha would be redundant and serve
to confuse the user as to which one to set.
That depends on the blending mode: in Coverage mode one has
independent plane and contents alpha values. And I consider alpha
value to be a part of the colour in the rgba/bgra modes.
Acked -- taking Sebastian's concern into consideration, I think I'll
have "PIXEL_SOURCE_SOLID_FILL_RGB" and add a separate
"PIXEL_SOURCE_SOLID_FILL_RGBA".
Hi Dmitry,
Since it looks like there's still some ongoing discussion with Pekka
about whether to support an RGBA solid fill source, I'll just leave a
note to add an RGBA source in the future.
Thanks,
Jessica Zhang
Thanks,
Jessica Zhang
Thanks,
Jessica Zhang
--
With best wishes
Dmitry
--
With best wishes
Dmitry
Re: [PATCH -next] drm/exynos: fix a potential error pointer dereference
2023년 8월 12일 (토) 오후 4:17, Xiang Yang 님이 작성: > From: Xiang Yang > > Smatch reports the warning below: > drivers/gpu/drm/exynos/exynos_hdmi.c:1864 hdmi_bind() > error: 'crtc' dereferencing possible ERR_PTR() > > The return value of exynos_drm_crtc_get_by_type maybe ERR_PTR(-ENODEV), > which can not be used directly. Fix this by checking the return value > before using it. > Applied. Thanks, Inki Dae > > Signed-off-by: Xiang Yang > --- > drivers/gpu/drm/exynos/exynos_hdmi.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c > b/drivers/gpu/drm/exynos/exynos_hdmi.c > index f3aaa4ea3e68..dd9903eab563 100644 > --- a/drivers/gpu/drm/exynos/exynos_hdmi.c > +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c > @@ -1861,6 +1861,8 @@ static int hdmi_bind(struct device *dev, struct > device *master, void *data) > return ret; > > crtc = exynos_drm_crtc_get_by_type(drm_dev, > EXYNOS_DISPLAY_TYPE_HDMI); > + if (IS_ERR(crtc)) > + return PTR_ERR(crtc); > crtc->pipe_clk = >phy_clk; > > ret = hdmi_create_connector(encoder); > -- > 2.34.1 > >
Re: [PATCH 0/5] drm/bridge: samsung-dsim: fix various modes with ADV7535 bridge
On Mon, Aug 28, 2023 at 11:13 AM Adam Ford wrote: > > On Mon, Aug 28, 2023 at 10:59 AM Michael Tretter > wrote: > > > > I tested the i.MX8M Nano EVK with the NXP supplied MIPI-DSI adapter, > > which uses an ADV7535 MIPI-DSI to HDMI converter. I found that a few > > modes were working, but in many modes my monitor stayed dark. > > > > This series fixes the Samsung DSIM bridge driver to bring up a few more > > modes: > > > > The driver read the rate of the PLL ref clock only during probe. > > However, if the clock is re-parented to the VIDEO_PLL, changes to the > > pixel clock have an effect on the PLL ref clock. Therefore, the driver > > must read and potentially update the PLL ref clock on every modeset. > > > > I also found that the rounding mode of the porches and active area has > > an effect on the working modes. If the driver rounds up instead of > > rounding down and be calculates them in Hz instead of kHz, more modes > > start to work. > > > > The following table shows the modes that were working in my test without > > this patch set and the modes that are working now: > > > > |Mode | Before | Now | > > | 1920x1080-60.00 | X | X | > > | 1920x1080-59.94 || X | > > | 1920x1080-50.00 || X | > > | 1920x1080-30.00 || X | > > | 1920x1080-29.97 || X | > > | 1920x1080-25.00 || X | > > | 1920x1080-24.00 || | > > | 1920x1080-23.98 || | > > | 1680x1050-59.88 || X | > > | 1280x1024-75.03 | X | X | > > | 1280x1024-60.02 | X | X | > > | 1200x960-59.99 || X | > > | 1152x864-75.00 | X | X | > > | 1280x720-60.00 || | > > | 1280x720-59.94 || | > > | 1280x720-50.00 || X | > > | 1024x768-75.03 || X | > > | 1024x768-60.00 || X | > > | 800x600-75.00 | X | X | > > | 800x600-60.32 | X | X | > > | 720x576-50.00 | X | X | > > | 720x480-60.00 || | > > | 720x480-59.94 | X | | > > | 640x480-75.00 | X | X | > > | 640x480-60.00 || X | > > | 640x480-59.94 || X | > > | 720x400-70.08 || | > > > > Nice! > > > Interestingly, the 720x480-59.94 mode stopped working. However, I am > > able to bring up the 720x480 modes by manually hacking the active area > > (hsa) to 40 and carefully adjusting the clocks, but something still > > seems to be off. > > Checkout my > > branch: > https://github.com/aford173/linux/commit/183cf6d154afeb9b0300500b09d7b8ec53047a12 > > I found that certain resolutions don't properly round, and it seemed > to be related to the size of HFP. HFP of 110 when divded between 4 > lanes, required a round-up, but then I had to recalculate the rest of > the horizontal timings to compensate. > > I was going to push that as an RFC, but I will investigate your patch > series first. With my small rounding correction, I am able to get > 720x480 and 720p on my imx8mp, but not my mini/nano, so I am hoping > your patch series fixes that issue for me. > > > > > Unfortunately, a few more modes are still not working at all. The NXP > > downstream kernel has some quirks to handle some of the modes especially > > wrt. to the porches, but I cannot figure out, what the driver should > > actually do in these cases. Maybe there is still an error in the > > calculation of the porches and someone at NXP can chime in. > > Hopefully the comment in my above patch explains how the calculation > is corrected and adjusted to get some of the missing resolutions. I tested my above patch and it works to sync 720p-60 on my imx8mp, but it doesn't seem to sync on my imx8mm using the same monitor and HDMI cable. I don't have a DSI analyzer, so I might still send out a modified version of my patch as an RFC once this gets approved. adam > > > Michael > > > > Signed-off-by: Michael Tretter > > I'll try to reivew this week and respond with my feedback. > > adam > > > --- > > Marco Felsch (1): > > drm/bridge: samsung-dsim: add more mipi-dsi device debug information > > > > Michael Tretter (4): > > drm/bridge: samsung-dsim: reread ref clock before configuring PLL > > drm/bridge: samsung-dsim: update PLL reference clock > > drm/bridge: samsung-dsim: adjust porches by rounding up > > drm/bridge: samsung-dsim: calculate porches in Hz > > > > drivers/gpu/drm/bridge/samsung-dsim.c | 42 > > +-- > > include/drm/bridge/samsung-dsim.h | 1 + > > 2 files changed, 31 insertions(+), 12 deletions(-) > > --- > > base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c > > change-id: 20230818-samsung-dsim-42346444bce5 > > > > Best regards, > > -- > > Michael Tretter > >
Re: [Intel-gfx] [PATCH v5] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset
On 8/23/2023 10:37, John Harrison wrote:
On 8/23/2023 09:00, Daniel Vetter wrote:
On Tue, Aug 22, 2023 at 11:53:24AM -0700, John Harrison wrote:
On 8/11/2023 11:20, Zhanjun Dong wrote:
This attempts to avoid circular locking dependency between flush
delayed
work and intel_gt_reset.
When intel_gt_reset was called, task will hold a lock.
To cacel delayed work here, the _sync version will also acquire a
lock,
which might trigger the possible cirular locking dependency warning.
When intel_gt_reset called, reset_in_progress flag will be set, add
code
to check the flag, call async verion if reset is in progress.
Signed-off-by: Zhanjun Dong
Cc: John Harrison
Cc: Andi Shyti
Cc: Daniel Vetter
---
drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index a0e3ef1c65d2..600388c849f7 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -1359,7 +1359,16 @@ static void
guc_enable_busyness_worker(struct intel_guc *guc)
static void guc_cancel_busyness_worker(struct intel_guc *guc)
{
- cancel_delayed_work_sync(>timestamp.work);
+ /*
+ * When intel_gt_reset was called, task will hold a lock.
+ * To cacel delayed work here, the _sync version will also
acquire a lock, which might
+ * trigger the possible cirular locking dependency warning.
+ * Check the reset_in_progress flag, call async verion if
reset is in progress.
+ */
This needs to explain in much more detail what is going on and why
it is not
a problem. E.g.:
The busyness worker needs to be cancelled. In general that means
using the synchronous cancel version to ensure that an in-progress
worker will not keep executing beyond whatever is happening that
needs the cancel. E.g. suspend, driver unload, etc. However, in the
case of a reset, the synchronous version is not required and can
trigger a false deadlock detection warning.
The business worker takes the reset mutex to protect against resets
interfering with it. However, it does a trylock and bails out if
the
reset lock is already acquired. Thus there is no actual deadlock or
other concern with the worker running concurrently with a reset. So
an asynchronous cancel is safe in the case of a reset rather than a
driver unload or suspend type operation. On the other hand, if the
cancel_sync version is used when a reset is in progress then the
mutex deadlock detection sees the mutex being acquired through
multiple paths and complains.
So just don't bother. That keeps the detection code happy and is
safe because of the trylock code described above.
So why do we even need to cancel anything if it doesn't do anything
while
the reset is in progress?
It still needs to be cancelled. The worker only aborts if it is
actively executing concurrently with the reset. It might not start to
execute until after the reset has completed. And there is presumably a
reason why the cancel is being called, a reason not necessarily
related to resets at all. Leaving the worker to run arbitrarily after
the driver is expecting it to be stopped will lead to much worse
things than a fake lockdep splat, e.g. a use after free pointer deref.
John.
@Daniel Vetter - ping? Is this explanation sufficient? Are you okay with
this change now?
John.
Just remove the cancel from the reset path as uneeded instead, and
explain
why that's ok? Because that's defacto what the cancel_work with a
potential deadlock scenario for cancel_work_sync does, you either don't
need it at all, or the replacement creates a bug.
-Daniel
John.
+ if (guc_to_gt(guc)->uc.reset_in_progress)
+ cancel_delayed_work(>timestamp.work);
+ else
+ cancel_delayed_work_sync(>timestamp.work);
}
static void __reset_guc_busyness_stats(struct intel_guc *guc)
Re: [PATCH 5/5] drm/bridge: samsung-dsim: calculate porches in Hz
On Mon, Aug 28, 2023 at 10:59 AM Michael Tretter
wrote:
>
> Calculating the byte_clk in kHz is imprecise for a hs_clock of 55687500
> Hz, which may be used with a pixel clock of 74.25 MHz with mode
> 1920x1080-30.
>
> Fix the calculation by using HZ instead of kHZ.
>
> This requires to change the type to u64 to prevent overflows of the
> integer type.
>
I would argue this needs a fixes tag since the previous calculations
were not accurately generated for many resolutions and refresh rates.
Reviewed-by: Adam Ford #imx8mm-beacon
Tested-by: Adam Ford #imx8mm-beacon
> Signed-off-by: Michael Tretter
> ---
> drivers/gpu/drm/bridge/samsung-dsim.c | 10 ++
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 459be953be55..eb7aca2b9ab7 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -973,10 +973,12 @@ static void samsung_dsim_set_display_mode(struct
> samsung_dsim *dsi)
> u32 reg;
>
> if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO) {
> - int byte_clk_khz = dsi->hs_clock / 1000 / 8;
> - int hfp = DIV_ROUND_UP((m->hsync_start - m->hdisplay) *
> byte_clk_khz, m->clock);
> - int hbp = DIV_ROUND_UP((m->htotal - m->hsync_end) *
> byte_clk_khz, m->clock);
> - int hsa = DIV_ROUND_UP((m->hsync_end - m->hsync_start) *
> byte_clk_khz, m->clock);
> + u64 byte_clk = dsi->hs_clock / 8;
> + u64 pix_clk = m->clock * 1000;
> +
> + int hfp = DIV64_U64_ROUND_UP((m->hsync_start - m->hdisplay) *
> byte_clk, pix_clk);
> + int hbp = DIV64_U64_ROUND_UP((m->htotal - m->hsync_end) *
> byte_clk, pix_clk);
> + int hsa = DIV64_U64_ROUND_UP((m->hsync_end - m->hsync_start)
> * byte_clk, pix_clk);
>
> /* remove packet overhead when possible */
> hfp = max(hfp - 6, 0);
>
> --
> 2.39.2
>
Re: [PATCH 4/5] drm/bridge: samsung-dsim: adjust porches by rounding up
On Mon, Aug 28, 2023 at 1:26 PM Fabio Estevam wrote: > > Hi Michael, > > On Mon, Aug 28, 2023 at 12:59 PM Michael Tretter > wrote: > > > > The porches must be rounded up to make the samsung-dsim work. ...at some resolutions and refresh rates. > > The commit log could be improved here. > > The way it is written gives the impression that samsung-dsim does not > work currently. This is a big improvement in the number of resolutions and refresh rates. Reviewed-by: Adam Ford #imx8mm-beacon Tested-by: Adam Ford #imx8mm-beacon
Re: [PATCH 1/5] drm/bridge: samsung-dsim: add more mipi-dsi device debug information
On Mon, Aug 28, 2023 at 10:59 AM Michael Tretter wrote: > > From: Marco Felsch > > Since the MIPI configuration can be changed on demand it is very useful > to print more MIPI settings during the MIPI device attach step. > > Signed-off-by: Marco Felsch > Signed-off-by: Michael Tretter Reviewed-by: Adam Ford #imx8mm-beacon Tested-by: Adam Ford #imx8mm-beacon > --- > drivers/gpu/drm/bridge/samsung-dsim.c | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c > b/drivers/gpu/drm/bridge/samsung-dsim.c > index 73ec60757dbc..6778f1751faa 100644 > --- a/drivers/gpu/drm/bridge/samsung-dsim.c > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c > @@ -1711,7 +1711,10 @@ static int samsung_dsim_host_attach(struct > mipi_dsi_host *host, > return ret; > } > > - DRM_DEV_INFO(dev, "Attached %s device\n", device->name); > + DRM_DEV_INFO(dev, "Attached %s device (lanes:%d bpp:%d > mode-flags:0x%lx)\n", > +device->name, device->lanes, > +mipi_dsi_pixel_format_to_bpp(device->format), > +device->mode_flags); > > drm_bridge_add(>bridge); > > > -- > 2.39.2 >
Re: [PATCH 2/7] drm: adv7511: Add max_mode_clock variable to struct adv7511_chip_info
On Sun, Aug 13, 2023 at 1:05 PM Biju Das wrote:
>
> The ADV7533 supports a maximum pixel clock of 80MHz whereas it is 148.5MHz
> for ADV7535. Add max_mode_clock variable to struct adv7511_chip_info to
> handle this difference.
>
> Signed-off-by: Biju Das
For the series:
Reviewed-by: Adam Ford
Tested-by: Adam Ford #imx8mm-beacon
> ---
> drivers/gpu/drm/bridge/adv7511/adv7511.h | 1 +
> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 6 --
> drivers/gpu/drm/bridge/adv7511/adv7533.c | 2 +-
> 3 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> index 59e8ef10d72e..f8190442ffca 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> @@ -335,6 +335,7 @@ enum adv7511_type {
>
> struct adv7511_chip_info {
> enum adv7511_type type;
> + unsigned long max_mode_clock;
> };
>
> struct adv7511 {
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index 013d8d640ef4..193b2d5bc7e6 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -1371,11 +1371,13 @@ static const struct adv7511_chip_info
> adv7511_chip_info = {
> };
>
> static const struct adv7511_chip_info adv7533_chip_info = {
> - .type = ADV7533
> + .type = ADV7533,
> + .max_mode_clock = 8
> };
>
> static const struct adv7511_chip_info adv7535_chip_info = {
> - .type = ADV7535
> + .type = ADV7535,
> + .max_mode_clock = 148500
> };
>
> static const struct i2c_device_id adv7511_i2c_ids[] = {
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7533.c
> b/drivers/gpu/drm/bridge/adv7511/adv7533.c
> index c452c4dc1c3f..e92b27e0afd1 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7533.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7533.c
> @@ -108,7 +108,7 @@ enum drm_mode_status adv7533_mode_valid(struct adv7511
> *adv,
> u8 bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
>
> /* Check max clock for either 7533 or 7535 */
> - if (mode->clock > (adv->info->type == ADV7533 ? 8 : 148500))
> + if (mode->clock > adv->info->max_mode_clock)
> return MODE_CLOCK_HIGH;
>
> /* Check max clock for each lane */
> --
> 2.25.1
>
Re: [PATCH v4 0/3] drm: simplify support for transparent DRM bridges
On Tue, 22 Aug 2023 at 17:19, Laurent Pinchart wrote: > > On Tue, Aug 22, 2023 at 05:17:37PM +0300, Laurent Pinchart wrote: > > Hi Dmitry, > > > > Thank you for the patches. > > > > On Thu, Aug 17, 2023 at 05:55:13PM +0300, Dmitry Baryshkov wrote: > > > Supporting DP/USB-C can result in a chain of several transparent > > > bridges (PHY, redrivers, mux, etc). This results in drivers having > > > similar boilerplate code for such bridges. > > > > What do you mean by transparent bridge here ? Bridges are a DRM concept, > > and as far as I can tell, a PHY isn't a bridge. Why does it need to be > > handled as one, especially if it's completely transparent ? > > > > > Next, these drivers are susceptible to -EPROBE_DEFER loops: the next > > > bridge can either be probed from the bridge->attach callback, when it is > > > too late to return -EPROBE_DEFER, or from the probe() callback, when the > > > next bridge might not yet be available, because it depends on the > > > resources provided by the probing device. > > > > Can't device links help avoiding defer probing in those cases ? > > > > > Last, but not least, this results in the the internal knowledge of DRM > > > subsystem slowly diffusing into other subsystems, like PHY or USB/TYPEC. > > > > Why so ? The PHY subsystem should provide a PHY, without considering > > what subsystem it will be used by. This patch series seems to me to > > actually create this DRM dependency in other subsystems, > > I was wrong on this one, there are indeed existing drm_bridge instances > in drivers/usb/ and drivers/phy/. That's certainly not nice. Why do we > even need drm_bridge there, why can't the PHYs be acquired by their > consumers in DRM (and anywhere else) using the PHY API ? During the design of the DT bindings for DisplayPort on these platforms we have evaluated different approaches. First approach was to add a special 'displayport' property to the USB-C connector, pointing to DP. This approach was declined by DT maintainers. Then we tried different approaches towards building connection graphs between different parties. Finally it was determined that the easiest way is to describe all USB-C signal paths properly. SS lines start at the PHY, then they pass through different muxes and retimers and then end up at the usb-c-connector. SS lines are muxed by the USB+DP PHY and switched between USB-3 (SuperSpeed) and DP. Then comes the question of binding everything together from the DRM point of view. The usb-c-connector is the logical place for the last drm_bridge, unfortunately. We need to send HPD events from the TypeC port driver (either directly or from the altmode driver). Then either we have to point the connector->fwnode to the DP port or build the full drm_bridge chain. Second path was selected, as it fits better into the overall framework. > > > which I don't > > think is a very good idea. Resources should be registered in their own > > subsystem with the appropriate API, not in a way that is tied to a > > particular consumer. > > > > > To solve all these issues, define a separate DRM helper, which creates > > > separate aux device just for the bridge. During probe such aux device > > > doesn't result in the EPROBE_DEFER loops. Instead it allows the device > > > drivers to probe properly, according to the actual resource > > > dependencies. The bridge auxdevs are then probed when the next bridge > > > becomes available, sparing drivers from drm_bridge_attach() returning > > > -EPROBE_DEFER. > > > > I'm not thrilled :-( Let's discuss the questions above first. > > > > > Proposed merge strategy: immutable branch with the drm commit, which is > > > then merged into PHY and USB subsystems together with the corresponding > > > patch. > > > > > > Changes since v3: > > > - Moved bridge driver to gpu/drm/bridge (Neil Armstrong) > > > - Renamed it to aux-bridge (since there is already a simple_bridge > > > driver) > > > - Made CONFIG_OF mandatory for this driver (Neil Armstrong) > > > - Added missing kfree and ida_free (Dan Carpenter) > > > > > > Changes since v2: > > > - ifdef'ed bridge->of_node access (LKP) > > > > > > Changes since v1: > > > - Added EXPORT_SYMBOL_GPL / MODULE_LICENSE / etc. to drm_simple_bridge > > > > > > Dmitry Baryshkov (3): > > > drm/bridge: add transparent bridge helper > > > phy: qcom: qmp-combo: switch to DRM_AUX_BRIDGE > > > usb: typec: nb7vpq904m: switch to DRM_AUX_BRIDGE > > > > > > drivers/gpu/drm/bridge/Kconfig| 9 ++ > > > drivers/gpu/drm/bridge/Makefile | 1 + > > > drivers/gpu/drm/bridge/aux-bridge.c | 132 ++ > > > drivers/phy/qualcomm/Kconfig | 2 +- > > > drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 44 +--- > > > drivers/usb/typec/mux/Kconfig | 2 +- > > > drivers/usb/typec/mux/nb7vpq904m.c| 44 +--- > > > include/drm/bridge/aux-bridge.h | 19 > > > 8 files changed, 167 insertions(+), 86 deletions(-) > > >
[Bug 217664] Medion AMD Laptop doesn't wake up from s2idle with SATA disk
https://bugzilla.kernel.org/show_bug.cgi?id=217664 Mario Limonciello (AMD) (mario.limoncie...@amd.com) changed: What|Removed |Added Assignee|drivers_video-dri@kernel-bu |t...@kernel.org |gs.osdl.org | -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
[Bug 217664] Medion AMD Laptop doesn't wake up from s2idle with SATA disk
https://bugzilla.kernel.org/show_bug.cgi?id=217664 Mario Limonciello (AMD) (mario.limoncie...@amd.com) changed: What|Removed |Added Summary|Laptop doesnt wake up from |Medion AMD Laptop doesn't |suspend mode. |wake up from s2idle with ||SATA disk -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
[Bug 217664] Laptop doesnt wake up from suspend mode.
https://bugzilla.kernel.org/show_bug.cgi?id=217664 Mario Limonciello (AMD) (mario.limoncie...@amd.com) changed: What|Removed |Added Component|Power-Sleep-Wake|Serial ATA Product|ACPI|IO/Storage -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
[Bug 217664] Laptop doesnt wake up from suspend mode.
https://bugzilla.kernel.org/show_bug.cgi?id=217664 --- Comment #46 from Mario Limonciello (AMD) (mario.limoncie...@amd.com) --- (In reply to popus_czy_to_ty from comment #43) > Created attachment 304974 [details] > acpi dump It appears that your system doesn't have the StorageD3Enable _DSD on any of the AHCI ports, it's only for NVME. However we already have a force for this in the kernel for your system since kernel 6.3-rc1.: > 2023-08-27 07:32:12,410 INFO: ✅ AMD Ryzen 5 5600H with Radeon Graphics > (family 19 model 50) https://github.com/torvalds/linux/commit/e2a56364485e7789e7b8f342637c7f3a219f7ede > also i changed value from =3 to =0 on CONFIG_SATA_MOBILE_LPM_POLICY and i was > able to compile it smoothly and run, but still no change 3 is the right value (medium with DPM enabled) > Aug 26 17:57:15 Crawler-E25 kernel: ata1.00: Features: Trust Dev-Sleep > NCQ-sndrcv The disk supports the feature we need, DevSlp. > Aug 26 17:57:15 Crawler-E25 kernel: ahci :06:00.0: flags: 64bit ncq sntf > ilck pm led clo only pmp fbs pio slum part The controller does not support 'sds' or 'sadm'. 'sds' is DevSlp 'sadm' is aggressive DevSlp The 'sds' is the important one, this is what allows the disk to transition to DevSlp during suspend. Does your BIOS have any settings for this? > 2) sata is itself kubuntu 23.04, i will try to install that on main nvme > (after how to learn build from source not from distro Something else you can do if it makes your experimentation easier is to take a live USB key of Ubuntu and boot off that with your SATA disk disconnected. This should let you test whether this feature works without the SATA disk. -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
[Bug 217664] Laptop doesnt wake up from suspend mode.
https://bugzilla.kernel.org/show_bug.cgi?id=217664 --- Comment #45 from popus_czy_to_ty (pentelja...@o2.pl) --- also i changed value from =3 to =0 on CONFIG_SATA_MOBILE_LPM_POLICY and i was able to compile it smoothly and run, but still no change -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
[Bug 217664] Laptop doesnt wake up from suspend mode.
https://bugzilla.kernel.org/show_bug.cgi?id=217664 --- Comment #44 from popus_czy_to_ty (pentelja...@o2.pl) --- SAMSUNG MZVLB512HBJQ-0 512,1 GB (came in bundle (as laptop) Samsung SSD 860 EVO 250GB 250,0 GB -sata (added just for linux) i will reply after to you other questions later if i can. -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
[Bug 217664] Laptop doesnt wake up from suspend mode.
https://bugzilla.kernel.org/show_bug.cgi?id=217664 --- Comment #43 from popus_czy_to_ty (pentelja...@o2.pl) --- Created attachment 304974 --> https://bugzilla.kernel.org/attachment.cgi?id=304974=edit acpi dump -- 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 v6 1/4] drm: Use XArray instead of IDR for minors
On Fri, Aug 25, 2023 at 12:59:26PM -0400, James Zhu wrote:
>
> On 2023-07-24 17:14, Michał Winiarski wrote:
> > IDR is deprecated, and since XArray manages its own state with internal
> > locking, it simplifies the locking on DRM side.
> > Additionally, don't use the IRQ-safe variant, since operating on drm
> > minor is not done in IRQ context.
> >
> > Signed-off-by: Michał Winiarski
> > Suggested-by: Matthew Wilcox
> > ---
> > drivers/gpu/drm/drm_drv.c | 63 ---
> > 1 file changed, 25 insertions(+), 38 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > index 3eda026ffac6..3faecb01186f 100644
> > --- a/drivers/gpu/drm/drm_drv.c
> > +++ b/drivers/gpu/drm/drm_drv.c
> > @@ -34,6 +34,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > @@ -54,8 +55,7 @@ MODULE_AUTHOR("Gareth Hughes, Leif Delgass, José Fonseca,
> > Jon Smirl");
> > MODULE_DESCRIPTION("DRM shared core routines");
> > MODULE_LICENSE("GPL and additional rights");
> > -static DEFINE_SPINLOCK(drm_minor_lock);
> > -static struct idr drm_minors_idr;
> > +static DEFINE_XARRAY_ALLOC(drm_minors_xa);
> > /*
> >* If the drm core fails to init for whatever reason,
> > @@ -101,26 +101,23 @@ static struct drm_minor **drm_minor_get_slot(struct
> > drm_device *dev,
> > static void drm_minor_alloc_release(struct drm_device *dev, void *data)
> > {
> > struct drm_minor *minor = data;
> > - unsigned long flags;
> > WARN_ON(dev != minor->dev);
> > put_device(minor->kdev);
> > - if (minor->type == DRM_MINOR_ACCEL) {
> > + if (minor->type == DRM_MINOR_ACCEL)
> > accel_minor_remove(minor->index);
> > - } else {
> > - spin_lock_irqsave(_minor_lock, flags);
> > - idr_remove(_minors_idr, minor->index);
> > - spin_unlock_irqrestore(_minor_lock, flags);
> > - }
> > + else
> > + xa_erase(_minors_xa, minor->index);
> > }
> > +#define DRM_MINOR_LIMIT(t) ({ typeof(t) _t = (t); XA_LIMIT(64 * _t, 64 *
> > _t + 63); })
> > +
> > static int drm_minor_alloc(struct drm_device *dev, enum drm_minor_type
> > type)
> > {
> > struct drm_minor *minor;
> > - unsigned long flags;
> > - int r;
> > + int index, r;
> > minor = drmm_kzalloc(dev, sizeof(*minor), GFP_KERNEL);
> > if (!minor)
> > @@ -129,24 +126,17 @@ static int drm_minor_alloc(struct drm_device *dev,
> > enum drm_minor_type type)
> > minor->type = type;
> > minor->dev = dev;
> > - idr_preload(GFP_KERNEL);
> > if (type == DRM_MINOR_ACCEL) {
> > r = accel_minor_alloc();
> > + index = r;
> > } else {
> > - spin_lock_irqsave(_minor_lock, flags);
> > - r = idr_alloc(_minors_idr,
> > - NULL,
> > - 64 * type,
> > - 64 * (type + 1),
> > - GFP_NOWAIT);
> > - spin_unlock_irqrestore(_minor_lock, flags);
> > + r = xa_alloc(_minors_xa, , NULL,
> > DRM_MINOR_LIMIT(type), GFP_KERNEL);
> > }
> > - idr_preload_end();
> > if (r < 0)
> > return r;
> > - minor->index = r;
> > + minor->index = index;
> > r = drmm_add_action_or_reset(dev, drm_minor_alloc_release, minor);
> > if (r)
> > @@ -163,7 +153,7 @@ static int drm_minor_alloc(struct drm_device *dev, enum
> > drm_minor_type type)
> > static int drm_minor_register(struct drm_device *dev, enum drm_minor_type
> > type)
> > {
> > struct drm_minor *minor;
> > - unsigned long flags;
> > + void *entry;
> > int ret;
> > DRM_DEBUG("\n");
> > @@ -190,9 +180,12 @@ static int drm_minor_register(struct drm_device *dev,
> > enum drm_minor_type type)
> > if (minor->type == DRM_MINOR_ACCEL) {
> > accel_minor_replace(minor, minor->index);
> > } else {
> > - spin_lock_irqsave(_minor_lock, flags);
> > - idr_replace(_minors_idr, minor, minor->index);
> > - spin_unlock_irqrestore(_minor_lock, flags);
> > + entry = xa_store(_minors_xa, minor->index, minor,
> > GFP_KERNEL);
> > + if (xa_is_err(entry)) {
> > + ret = xa_err(entry);
> > + goto err_debugfs;
> > + }
> > + WARN_ON(entry);
> [JZ] would WARN_ON(entry != minor)be better?
We expect NULL here.
The same one that was previously stored here ⌄⌄⌄
r = xa_alloc(_minors_xa, >index, NULL, DRM_EXTENDED_MINOR_LIMIT,
GFP_KERNEL);
in drm_minor_alloc.
> > }
> > DRM_DEBUG("new minor registered %d\n", minor->index);
> > @@ -206,20 +199,16 @@ static int drm_minor_register(struct drm_device *dev,
> > enum drm_minor_type type)
> > static void drm_minor_unregister(struct drm_device *dev, enum
> > drm_minor_type type)
> > {
> > struct drm_minor *minor;
> > - unsigned long flags;
> > minor = *drm_minor_get_slot(dev, type);
> > if (!minor || !device_is_registered(minor->kdev))
Re: [PATCH v2 2/3] drm/i915/guc: Close deregister-context race against CT-loss
Additional update from the most recent testing. When relying solely on guc_lrc_desc_unpin getting a failure from deregister_context as a means for identifying that we are in the "deregister-context-vs-suspend-late" race, it is too late a location to handle this safely. This is because one of the first things destroyed_worker_func does it to take a gt pm wakeref - which triggers the gt_unpark function that does a whole lot bunch of other flows including triggering more workers and taking additional refs. That said, its best to not even call deregister_destroyed_contexts from the worker when !intel_guc_is_ready (ct-is-disabled). ...alan On Fri, 2023-08-25 at 11:54 -0700, Teres Alexis, Alan Previn wrote: > just a follow up note-to-self: > > On Tue, 2023-08-15 at 12:08 -0700, Teres Alexis, Alan Previn wrote: > > On Tue, 2023-08-15 at 09:56 -0400, Vivi, Rodrigo wrote: > > > On Mon, Aug 14, 2023 at 06:12:09PM -0700, Alan Previn wrote: > > > > > [snip] > > in guc_submission_send_busy_loop, we are incrementing the following > that needs to be decremented if the function fails. > > atomic_inc(>outstanding_submission_g2h); > > also, it seems that even with thie unroll design - we are still > leaking a wakeref elsewhere. this is despite a cleaner redesign of > flows in function "guc_lrc_desc_unpin" > (discussed earlier that wasnt very readible). > > will re-rev today but will probably need more follow ups > tracking that one more leaking gt-wakeref (one in thousands-cycles) > but at least now we are not hanging mid-suspend.. we bail from suspend > with useful kernel messages. > > > >
Re: [PATCH v2 9/9] accel/ivpu: Move MMU register definitions to ivpu_mmu.c
On 8/28/2023 3:47 AM, Stanislaw Gruszka wrote: From: Jacek Lawrynowicz MMU registers are not platform specific so they should be defined separate to platform regs. Signed-off-by: Jacek Lawrynowicz Reviewed-by: Stanislaw Gruszka Signed-off-by: Stanislaw Gruszka Reviewed-by: Jeffrey Hugo
Re: [PATCH v2 8/9] accel/ivpu/37xx: White space cleanup
On 8/28/2023 3:47 AM, Stanislaw Gruszka wrote: No functional change, adjust code formatting after previous changes. This feels incomplete. You are doing an adjustment, but to what, and why? Maybe - No functional change, adjust code formatting so that defines line up nicely to improve code readability. With that, or something similar Reviewed-by: Jeffrey Hugo
Re: [PATCH v2 7/9] accel/ivpu/37xx: Change register rename leftovers
On 8/28/2023 3:47 AM, Stanislaw Gruszka wrote: Change remaining MTL_VPU_ register names to generation based names. Reviewed-by: Karol Wachowski Signed-off-by: Stanislaw Gruszka Reviewed-by: Jeffrey Hugo
Re: [PATCH v2 6/9] accel/ivpu: Add ivpu_bo_vaddr() and ivpu_bo_size()
On 8/28/2023 3:47 AM, Stanislaw Gruszka wrote: From: Jacek Lawrynowicz Use: - ivpu_bo_vaddr(bo) instead of bo->kvaddr - ivpu_bo_size(bo) instead of bo->base.size This is a preparation for switch to a drm_gem_shmem_object as a base for ivpu_bo, where: - bo->kvaddr becomes bo->base.vaddr - bo->base.size becomes bo->base.base.size Using ivpu_bo_vaddr() and ivpu_bo_size() increases the readability of the code. Signed-off-by: Jacek Lawrynowicz Reviewed-by: Stanislaw Gruszka Signed-off-by: Stanislaw Gruszka Reviewed-by: Jeffrey Hugo
Re: [PATCH v2 5/9] accel/ivpu: Move ivpu_fw_load() to ivpu_fw_init()
On 8/28/2023 3:47 AM, Stanislaw Gruszka wrote:
From: Jacek Lawrynowicz
ivpu_fw_load() doesn't have to be called separately in ivpu_dev_init().
Signed-off-by: Jacek Lawrynowicz
Reviewed-by: Stanislaw Gruszka
Signed-off-by: Stanislaw Gruszka
---
drivers/accel/ivpu/ivpu_drv.c | 4
drivers/accel/ivpu/ivpu_fw.c | 6 +++---
drivers/accel/ivpu/ivpu_fw.h | 2 +-
3 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/accel/ivpu/ivpu_drv.c b/drivers/accel/ivpu/ivpu_drv.c
index 5310b54f036d..fa0680ba9340 100644
--- a/drivers/accel/ivpu/ivpu_drv.c
+++ b/drivers/accel/ivpu/ivpu_drv.c
@@ -561,10 +561,6 @@ static int ivpu_dev_init(struct ivpu_device *vdev)
if (ret)
goto err_ipc_fini;
- ret = ivpu_fw_load(vdev);
- if (ret)
- goto err_job_done_thread_fini;
-
ret = ivpu_boot(vdev);
if (ret)
goto err_job_done_thread_fini;
diff --git a/drivers/accel/ivpu/ivpu_fw.c b/drivers/accel/ivpu/ivpu_fw.c
index 9b6ecd3e9537..32a1ea322ca2 100644
--- a/drivers/accel/ivpu/ivpu_fw.c
+++ b/drivers/accel/ivpu/ivpu_fw.c
@@ -301,6 +301,8 @@ int ivpu_fw_init(struct ivpu_device *vdev)
if (ret)
goto err_fw_release;
+ ivpu_fw_load(vdev);
+
return 0;
err_fw_release:
@@ -314,7 +316,7 @@ void ivpu_fw_fini(struct ivpu_device *vdev)
ivpu_fw_release(vdev);
}
-int ivpu_fw_load(struct ivpu_device *vdev)
+void ivpu_fw_load(struct ivpu_device *vdev)
{
struct ivpu_fw_info *fw = vdev->fw;
u64 image_end_offset = fw->image_load_offset + fw->image_size;
@@ -331,8 +333,6 @@ int ivpu_fw_load(struct ivpu_device *vdev)
}
wmb(); /* Flush WC buffers after writing fw->mem */
-
- return 0;
}
static void ivpu_fw_boot_params_print(struct ivpu_device *vdev, struct vpu_boot_params *boot_params)
diff --git a/drivers/accel/ivpu/ivpu_fw.h b/drivers/accel/ivpu/ivpu_fw.h
index 8567fdf925fe..10ae2847f0ef 100644
--- a/drivers/accel/ivpu/ivpu_fw.h
+++ b/drivers/accel/ivpu/ivpu_fw.h
@@ -31,7 +31,7 @@ struct ivpu_fw_info {
int ivpu_fw_init(struct ivpu_device *vdev);
void ivpu_fw_fini(struct ivpu_device *vdev);
-int ivpu_fw_load(struct ivpu_device *vdev);
+void ivpu_fw_load(struct ivpu_device *vdev);
If ivpu_fw_load() is defined in ivpu_fw.c and only used in ivpu_fw.c, do
you need it in this header file anymore? Seems like you could remove
this, and also make the function static.
void ivpu_fw_boot_params_setup(struct ivpu_device *vdev, struct
vpu_boot_params *bp);
static inline bool ivpu_fw_is_cold_boot(struct ivpu_device *vdev)
Re: [PATCH v2 4/9] accel/ivpu: Initialize context with SSID = 1
On 8/28/2023 3:47 AM, Stanislaw Gruszka wrote: From: Karol Wachowski Context with SSID = 1 is reserved and accesses on that context happen only when context is uninitialized on the VPU side. Such access triggers MMU fault (0xa) "Invalid CD Fetch", which doesn't contain any useful information besides context ID. This commit will change that state, now (0x10) "Translation fault" will be triggered and accessed address will shown in the log. Signed-off-by: Karol Wachowski Reviewed-by: Stanislaw Gruszka Signed-off-by: Stanislaw Gruszka Reviewed-by: Jeffrey Hugo
Re: [PATCH v2 3/9] accel/ivpu: Print information about used workarounds
On 8/28/2023 3:47 AM, Stanislaw Gruszka wrote: Use ivpu_dbg(MISC) to print information about workarounds. Reviewed-by: Karol Wachowski Signed-off-by: Stanislaw Gruszka Reviewed-by: Jeffrey Hugo
Re: [PATCH v2 2/9] accel/ivpu: Remove duplicated error messages
On 8/28/2023 3:47 AM, Stanislaw Gruszka wrote:
From: Jacek Lawrynowicz
Reduce the number of error messages per single failure in
ivpu_dev_init(). Error messages are already printed by functions
called from ivpu_dev_init().
Signed-off-by: Jacek Lawrynowicz
Reviewed-by: Stanislaw Gruszka
Signed-off-by: Stanislaw Gruszka
---
drivers/accel/ivpu/ivpu_drv.c | 54 +++
drivers/accel/ivpu/ivpu_fw.c | 2 +-
drivers/accel/ivpu/ivpu_ipc.c | 13 +--
drivers/accel/ivpu/ivpu_mmu_context.c | 6 ++-
drivers/accel/ivpu/ivpu_pm.c | 4 +-
drivers/accel/ivpu/ivpu_pm.h | 2 +-
6 files changed, 29 insertions(+), 52 deletions(-)
diff --git a/drivers/accel/ivpu/ivpu_drv.c b/drivers/accel/ivpu/ivpu_drv.c
index ba79f397c9e8..b10b2909f05f 100644
--- a/drivers/accel/ivpu/ivpu_drv.c
+++ b/drivers/accel/ivpu/ivpu_drv.c
@@ -518,78 +518,52 @@ static int ivpu_dev_init(struct ivpu_device *vdev)
lockdep_set_class(>submitted_jobs_xa.xa_lock,
_jobs_xa_lock_class_key);
ret = ivpu_pci_init(vdev);
- if (ret) {
- ivpu_err(vdev, "Failed to initialize PCI device: %d\n", ret);
+ if (ret)
goto err_xa_destroy;
- }
ret = ivpu_irq_init(vdev);
- if (ret) {
- ivpu_err(vdev, "Failed to initialize IRQs: %d\n", ret);
+ if (ret)
goto err_xa_destroy;
- }
/* Init basic HW info based on buttress registers which are accessible before power up */
ret = ivpu_hw_info_init(vdev);
- if (ret) {
- ivpu_err(vdev, "Failed to initialize HW info: %d\n", ret);
+ if (ret)
goto err_xa_destroy;
- }
/* Power up early so the rest of init code can access VPU registers */
ret = ivpu_hw_power_up(vdev);
- if (ret) {
- ivpu_err(vdev, "Failed to power up HW: %d\n", ret);
+ if (ret)
goto err_xa_destroy;
- }
ret = ivpu_mmu_global_context_init(vdev);
- if (ret) {
- ivpu_err(vdev, "Failed to initialize global MMU context: %d\n",
ret);
+ if (ret)
goto err_power_down;
- }
ret = ivpu_mmu_init(vdev);
- if (ret) {
- ivpu_err(vdev, "Failed to initialize MMU device: %d\n", ret);
+ if (ret)
goto err_mmu_gctx_fini;
- }
ret = ivpu_fw_init(vdev);
- if (ret) {
- ivpu_err(vdev, "Failed to initialize firmware: %d\n", ret);
+ if (ret)
goto err_mmu_gctx_fini;
- }
ret = ivpu_ipc_init(vdev);
- if (ret) {
- ivpu_err(vdev, "Failed to initialize IPC: %d\n", ret);
+ if (ret)
goto err_fw_fini;
- }
- ret = ivpu_pm_init(vdev);
- if (ret) {
- ivpu_err(vdev, "Failed to initialize PM: %d\n", ret);
- goto err_ipc_fini;
- }
+ ivpu_pm_init(vdev);
ret = ivpu_job_done_thread_init(vdev);
- if (ret) {
- ivpu_err(vdev, "Failed to initialize job done thread: %d\n",
ret);
+ if (ret)
goto err_ipc_fini;
- }
ret = ivpu_fw_load(vdev);
- if (ret) {
- ivpu_err(vdev, "Failed to load firmware: %d\n", ret);
+ if (ret)
goto err_job_done_thread_fini;
- }
ret = ivpu_boot(vdev);
- if (ret) {
- ivpu_err(vdev, "Failed to boot: %d\n", ret);
+ if (ret)
goto err_job_done_thread_fini;
- }
ivpu_pm_enable(vdev);
@@ -651,10 +625,8 @@ static int ivpu_probe(struct pci_dev *pdev, const struct pci_device_id *id)
pci_set_drvdata(pdev, vdev);
ret = ivpu_dev_init(vdev);
- if (ret) {
- dev_err(>dev, "Failed to initialize VPU device: %d\n",
ret);
+ if (ret)
return ret;
- }
Commit text doesn't mention anything about this change.
ret = drm_dev_register(>drm, 0);
if (ret) {
diff --git a/drivers/accel/ivpu/ivpu_fw.c b/drivers/accel/ivpu/ivpu_fw.c
index 9827ea4d7b83..9b6ecd3e9537 100644
--- a/drivers/accel/ivpu/ivpu_fw.c
+++ b/drivers/accel/ivpu/ivpu_fw.c
@@ -78,7 +78,7 @@ static int ivpu_fw_request(struct ivpu_device *vdev)
}
}
- ivpu_err(vdev, "Failed to request firmware: %d\n", ret);
+ ivpu_err(vdev, "Failed to load firmware: %d\n", ret);
Commit text doesn't mention anything about this. Nor do I understand it
as this function doesn't load the firmware.
Everything below this point seems to be not related to $SUBJECT and
feels like should be a separate patch.
return ret;
}
diff --git a/drivers/accel/ivpu/ivpu_ipc.c b/drivers/accel/ivpu/ivpu_ipc.c
index fa0af59e39ab..6b2e9dbb284a 100644
--- a/drivers/accel/ivpu/ivpu_ipc.c
+++ b/drivers/accel/ivpu/ivpu_ipc.c
@@ -426,15 +426,20 @@ int ivpu_ipc_irq_handler(struct ivpu_device *vdev)
int
Re: [PATCH v2 1/9] accel/ivpu: Move set autosuspend delay to HW specific code
On 8/28/2023 3:47 AM, Stanislaw Gruszka wrote: From: Krystian Pradzynski Configure autosuspend values per HW generation and per platform. For non silicon platforms disable autosuspend for now, for silicon reduce it to 10 ms. Signed-off-by: Krystian Pradzynski Reviewed-by: Stanislaw Gruszka Signed-off-by: Stanislaw Gruszka Reviewed-by: Jeffrey Hugo
Re: [PATCH v3 0/7] GPU workload hints for better performance
On 28/08/2023 17:14, Yadav, Arvind wrote: On 8/28/2023 9:13 PM, Helen Mae Koike Fornazier wrote: On Monday, August 28, 2023 09:26 -03, Arvind Yadav wrote: AMDGPU SOCs supports dynamic workload based power profiles, which can provide fine-tuned performance for a particular type of workload. This patch series adds an interface to set/reset these power profiles based on the submitted job. The driver can dynamically switch the power profiles based on submitted job. This can optimize the power performance when the particular workload is on. Hi Arvind, Would you mind to test your patchset with drm-ci ? There is a amdgpu test there and I would love to get your feedback of the ci. You basically just need to apply the ci patch which is available on https://cgit.freedesktop.org/drm/drm/log/?h=topic/drm-ci There are instruction on the docs, but in short: to configure it, you push your branch to gitlab.freedesktop.org, go to the settings and change the CI/CD configuration file from .gitlab-ci.yml to drivers/gpu/drm/ci/gitlab-ci.yml, and you can trigger a pipeline on your branch to get tests running. (by the time of this writing, gitlab.fdo is under maintenance but should be up soonish). Hi Helen, I tried the steps as mentioned by you but looks like something is missing and build itself is failing. https://gitlab.freedesktop.org/ArvindYadav/drm-next/-/commits/smu_workload Thanks for your feedback! You need to apply this patch https://gitlab.freedesktop.org/ArvindYadav/drm-next/-/commit/cc6dcff192d07f9fe82645fbc4213c97e872156b This patch adds the file drivers/gpu/drm/ci/gitlab-ci.yml for you. And you can drop the patch where gitlab added the ci template. I replied here too https://gitlab.freedesktop.org/ArvindYadav/drm-next/-/commit/cc6dcff192d07f9fe82645fbc4213c97e872156b Could you try again with that patch? Thanks a lot! Helen Regards, ~Arvind Thank you! Helen v2: - Splitting workload_profile_set and workload_profile_put into two separate patches. - Addressed review comment. - Added new suspend function. - Added patch to switches the GPU workload mode for KFD. v3: - Addressed all review comment. - Changed the function name from *_set() to *_get(). - Now clearing all the profile in work handler. - Added *_clear_all function to clear all the power profile. Arvind Yadav (7): drm/amdgpu: Added init/fini functions for workload drm/amdgpu: Add new function to set GPU power profile drm/amdgpu: Add new function to put GPU power profile drm/amdgpu: Add suspend function to clear the GPU power profile. drm/amdgpu: Set/Reset GPU workload profile drm/amdgpu: switch workload context to/from compute Revert "drm/amd/amdgpu: switch on/off vcn power profile mode" drivers/gpu/drm/amd/amdgpu/Makefile | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu.h | 3 + drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c | 8 +- drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 6 + drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 5 + drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c | 14 +- drivers/gpu/drm/amd/amdgpu/amdgpu_workload.c | 226 ++ drivers/gpu/drm/amd/include/amdgpu_workload.h | 61 + 8 files changed, 309 insertions(+), 16 deletions(-) create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_workload.c create mode 100644 drivers/gpu/drm/amd/include/amdgpu_workload.h -- 2.34.1
Re: [PATCH v3 0/7] GPU workload hints for better performance
On 8/28/2023 9:13 PM, Helen Mae Koike Fornazier wrote: On Monday, August 28, 2023 09:26 -03, Arvind Yadav wrote: AMDGPU SOCs supports dynamic workload based power profiles, which can provide fine-tuned performance for a particular type of workload. This patch series adds an interface to set/reset these power profiles based on the submitted job. The driver can dynamically switch the power profiles based on submitted job. This can optimize the power performance when the particular workload is on. Hi Arvind, Would you mind to test your patchset with drm-ci ? There is a amdgpu test there and I would love to get your feedback of the ci. You basically just need to apply the ci patch which is available on https://cgit.freedesktop.org/drm/drm/log/?h=topic/drm-ci There are instruction on the docs, but in short: to configure it, you push your branch to gitlab.freedesktop.org, go to the settings and change the CI/CD configuration file from .gitlab-ci.yml to drivers/gpu/drm/ci/gitlab-ci.yml, and you can trigger a pipeline on your branch to get tests running. (by the time of this writing, gitlab.fdo is under maintenance but should be up soonish). Hi Helen, I tried the steps as mentioned by you but looks like something is missing and build itself is failing. https://gitlab.freedesktop.org/ArvindYadav/drm-next/-/commits/smu_workload Regards, ~Arvind Thank you! Helen v2: - Splitting workload_profile_set and workload_profile_put into two separate patches. - Addressed review comment. - Added new suspend function. - Added patch to switches the GPU workload mode for KFD. v3: - Addressed all review comment. - Changed the function name from *_set() to *_get(). - Now clearing all the profile in work handler. - Added *_clear_all function to clear all the power profile. Arvind Yadav (7): drm/amdgpu: Added init/fini functions for workload drm/amdgpu: Add new function to set GPU power profile drm/amdgpu: Add new function to put GPU power profile drm/amdgpu: Add suspend function to clear the GPU power profile. drm/amdgpu: Set/Reset GPU workload profile drm/amdgpu: switch workload context to/from compute Revert "drm/amd/amdgpu: switch on/off vcn power profile mode" drivers/gpu/drm/amd/amdgpu/Makefile | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu.h | 3 + drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c| 8 +- drivers/gpu/drm/amd/amdgpu/amdgpu_device.c| 6 + drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 5 + drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c | 14 +- drivers/gpu/drm/amd/amdgpu/amdgpu_workload.c | 226 ++ drivers/gpu/drm/amd/include/amdgpu_workload.h | 61 + 8 files changed, 309 insertions(+), 16 deletions(-) create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_workload.c create mode 100644 drivers/gpu/drm/amd/include/amdgpu_workload.h -- 2.34.1
Re: [PATCH v2] drm: Update file owner during use
On Wed, Jun 21, 2023 at 2:48 AM Tvrtko Ursulin
wrote:
>
> From: Tvrtko Ursulin
>
> With the typical model where the display server opens the file descriptor
> and then hands it over to the client(*), we were showing stale data in
> debugfs.
>
> Fix it by updating the drm_file->pid on ioctl access from a different
> process.
>
> The field is also made RCU protected to allow for lockless readers. Update
> side is protected with dev->filelist_mutex.
>
> Before:
>
> $ cat /sys/kernel/debug/dri/0/clients
> command pid dev master a uid magic
> Xorg 2344 0 yy 0 0
> Xorg 2344 0 ny 0 2
> Xorg 2344 0 ny 0 3
> Xorg 2344 0 ny 0 4
>
> After:
>
> $ cat /sys/kernel/debug/dri/0/clients
> command tgid dev master a uid magic
> Xorg 830 0 yy 0 0
>xfce4-session 880 0 ny 0 1
>xfwm4 943 0 ny 0 2
>neverball 1095 0 ny 0 3
>
> *)
> More detailed and historically accurate description of various handover
> implementation kindly provided by Emil Velikov:
>
> """
> The traditional model, the server was the orchestrator managing the
> primary device node. From the fd, to the master status and
> authentication. But looking at the fd alone, this has varied across
> the years.
>
> IIRC in the DRI1 days, Xorg (libdrm really) would have a list of open
> fd(s) and reuse those whenever needed, DRI2 the client was responsible
> for open() themselves and with DRI3 the fd was passed to the client.
>
> Around the inception of DRI3 and systemd-logind, the latter became
> another possible orchestrator. Whereby Xorg and Wayland compositors
> could ask it for the fd. For various reasons (hysterical and genuine
> ones) Xorg has a fallback path going the open(), whereas Wayland
> compositors are moving to solely relying on logind... some never had
> fallback even.
>
> Over the past few years, more projects have emerged which provide
> functionality similar (be that on API level, Dbus, or otherwise) to
> systemd-logind.
> """
>
> v2:
> * Fixed typo in commit text and added a fine historical explanation
>from Emil.
>
> Signed-off-by: Tvrtko Ursulin
> Cc: "Christian König"
> Cc: Daniel Vetter
> Acked-by: Christian König
> Reviewed-by: Emil Velikov
Reviewed-by: Rob Clark
Tested-by: Rob Clark
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 6 ++--
> drivers/gpu/drm/drm_auth.c | 3 +-
> drivers/gpu/drm/drm_debugfs.c | 10 ---
> drivers/gpu/drm/drm_file.c | 40 +++--
> drivers/gpu/drm/drm_ioctl.c | 3 ++
> drivers/gpu/drm/nouveau/nouveau_drm.c | 5 +++-
> drivers/gpu/drm/vmwgfx/vmwgfx_gem.c | 6 ++--
> include/drm/drm_file.h | 13 ++--
> 8 files changed, 71 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index 74055cba3dc9..849097dff02b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -963,6 +963,7 @@ static int amdgpu_debugfs_gem_info_show(struct seq_file
> *m, void *unused)
> list_for_each_entry(file, >filelist, lhead) {
> struct task_struct *task;
> struct drm_gem_object *gobj;
> + struct pid *pid;
> int id;
>
> /*
> @@ -972,8 +973,9 @@ static int amdgpu_debugfs_gem_info_show(struct seq_file
> *m, void *unused)
> * Therefore, we need to protect this ->comm access using RCU.
> */
> rcu_read_lock();
> - task = pid_task(file->pid, PIDTYPE_TGID);
> - seq_printf(m, "pid %8d command %s:\n", pid_nr(file->pid),
> + pid = rcu_dereference(file->pid);
> + task = pid_task(pid, PIDTYPE_TGID);
> + seq_printf(m, "pid %8d command %s:\n", pid_nr(pid),
>task ? task->comm : "");
> rcu_read_unlock();
>
> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
> index cf92a9ae8034..2ed2585ded37 100644
> --- a/drivers/gpu/drm/drm_auth.c
> +++ b/drivers/gpu/drm/drm_auth.c
> @@ -235,7 +235,8 @@ static int drm_new_set_master(struct drm_device *dev,
> struct drm_file *fpriv)
> static int
> drm_master_check_perm(struct drm_device *dev, struct drm_file *file_priv)
> {
> - if (file_priv->pid == task_pid(current) && file_priv->was_master)
> + if (file_priv->was_master &&
> + rcu_access_pointer(file_priv->pid) == task_pid(current))
> return 0;
>
> if (!capable(CAP_SYS_ADMIN))
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index
Re: [PATCH 01/20] drm/xlnx/zynqmp_disp: Use correct kerneldoc formatting in zynqmp_disp
Hi--
On 8/28/23 09:39, Laurent Pinchart wrote:
> Hi Lee,
>
> Thank you for the patch.
>
> On Thu, Aug 24, 2023 at 08:36:46AM +0100, Lee Jones wrote:
>> Fixes the following W=1 kernel build warning(s):
>>
>> drivers/gpu/drm/xlnx/zynqmp_disp.c:151: warning: Function parameter or
>> member 'blend' not described in 'zynqmp_disp'
>> drivers/gpu/drm/xlnx/zynqmp_disp.c:151: warning: Function parameter or
>> member 'avbuf' not described in 'zynqmp_disp'
>> drivers/gpu/drm/xlnx/zynqmp_disp.c:151: warning: Function parameter or
>> member 'audio' not described in 'zynqmp_disp'
>>
>> Signed-off-by: Lee Jones
>> ---
>> Cc: Hyun Kwon
>> Cc: Laurent Pinchart
>> Cc: David Airlie
>> Cc: Daniel Vetter
>> Cc: Michal Simek
>> Cc: dri-devel@lists.freedesktop.org
>> Cc: linux-arm-ker...@lists.infradead.org
>> ---
>> drivers/gpu/drm/xlnx/zynqmp_disp.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
>> b/drivers/gpu/drm/xlnx/zynqmp_disp.c
>> index 407bc07cec69a..7c64ab11fe2b0 100644
>> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
>> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
>> @@ -128,9 +128,9 @@ struct zynqmp_disp_layer {
>> * struct zynqmp_disp - Display controller
>> * @dev: Device structure
>> * @dpsub: Display subsystem
>> - * @blend.base: Register I/O base address for the blender
>> - * @avbuf.base: Register I/O base address for the audio/video buffer manager
>> - * @audio.base: Registers I/O base address for the audio mixer
>> + * @blend: .base: Register I/O base address for the blender
>> + * @avbuf: .base: Register I/O base address for the audio/video buffer
>> manager
>> + * @audio: .base: Registers I/O base address for the audio mixer
>
> This is really a hack to work around the warning, and not a clean fix.
> kerneldoc is still today unable, as far as I understand, to document
> nested structures. That's annoying, and the warning is a good way to
kernel-doc for nested structs/unions is documented in
Documentation/doc-guide/kernel-doc.rst.
This isn't pretty, but this fixes the warnings for me: (massage the
descriptions as you see fit)
---
drivers/gpu/drm/xlnx/zynqmp_disp.c |3 +++
1 file changed, 3 insertions(+)
diff -- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
b/drivers/gpu/drm/xlnx/zynqmp_disp.c
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
@@ -128,8 +128,11 @@ struct zynqmp_disp_layer {
* struct zynqmp_disp - Display controller
* @dev: Device structure
* @dpsub: Display subsystem
+ * @blend: blender struct
* @blend.base: Register I/O base address for the blender
+ * @avbuf: audio/video buffer manager struct
* @avbuf.base: Register I/O base address for the audio/video buffer manager
+ * @audio: audio mixer struct
* @audio.base: Registers I/O base address for the audio mixer
* @layers: Layers (planes)
*/
> remind us that it needs to be fixed. I'd be tempted to keep the warning
> for that reason.
>
>> * @layers: Layers (planes)
>> */
>> struct zynqmp_disp {
>
--
~Randy
Re: [PATCH v4 1/3] dt-bindings: display: panel: add common dual-link schema
Hi Krzysztof, Thank you for the patch. On Fri, Aug 25, 2023 at 02:11:40PM +0200, Krzysztof Kozlowski wrote: > Add schema with common properties shared among dual-link panel ICs. > > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Laurent Pinchart > --- > > Changes since v3: > 1. Re-phrase description of binding and ports (Laurent) > v3: > https://lore.kernel.org/all/20230823081500.84005-1-krzysztof.kozlow...@linaro.org/ > > Changes since v2: > 1. New Patch > v2: > https://lore.kernel.org/all/20230502120036.47165-1-krzysztof.kozlow...@linaro.org/ > v1: > https://lore.kernel.org/all/20230416153929.356330-1-krzysztof.kozlow...@linaro.org/ > --- > .../display/panel/panel-common-dual.yaml | 47 +++ > 1 file changed, 47 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/display/panel/panel-common-dual.yaml > > diff --git > a/Documentation/devicetree/bindings/display/panel/panel-common-dual.yaml > b/Documentation/devicetree/bindings/display/panel/panel-common-dual.yaml > new file mode 100644 > index ..cc7ea3c35c77 > --- /dev/null > +++ b/Documentation/devicetree/bindings/display/panel/panel-common-dual.yaml > @@ -0,0 +1,47 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/display/panel/panel-common-dual.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Common Properties for Dual-Link Display Panels > + > +maintainers: > + - Thierry Reding > + - Laurent Pinchart > + > +description: > + Properties common for Panel IC supporting dual link panels. Devices might > + support also single link. > + > +allOf: > + - $ref: panel-common.yaml# > + > +properties: > + ports: > +$ref: /schemas/graph.yaml#/properties/ports > +additionalProperties: false > + > +properties: > + port@0: > +$ref: /schemas/graph.yaml#/properties/port > +description: First link > + > + port@1: > +$ref: /schemas/graph.yaml#/properties/port > +description: Second link > + > + "#address-cells": true > + "#size-cells": true > + > +required: > + - port@0 > + > +# Single-panel setups are still allowed. > +oneOf: > + - required: > + - ports > + - required: > + - port > + > +additionalProperties: true -- Regards, Laurent Pinchart
[Bug 217664] Laptop doesnt wake up from suspend mode.
https://bugzilla.kernel.org/show_bug.cgi?id=217664 --- Comment #42 from Mario Limonciello (AMD) (mario.limoncie...@amd.com) --- s/NVME/SATA/ -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
[Bug 217664] Laptop doesnt wake up from suspend mode.
https://bugzilla.kernel.org/show_bug.cgi?id=217664
--- Comment #41 from Mario Limonciello (AMD) (mario.limoncie...@amd.com) ---
Was that NVME something you added to the system or it came with it?
I am suspecting that your platform or the NVME doesn't end up activating a
feature needed for s2idle to work properly called DevSlp at all or at the right
timing.
Can you please share to me an acpidump? I want to check if you have some _DSD
properties set up appropriately. You can see more about this in
7c5f641a5914 ("ata: libahci: Adjust behavior when StorageD3Enable _DSD is set")
If it's a missing _DSD but the system and disk both support DevSlp we can
probably find a way to work around that in the kernel.
--
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 v2 4/9] drm/sched: Split free_job into own work item
On Mon, Aug 28, 2023 at 08:04:31PM +0200, Danilo Krummrich wrote:
> On 8/11/23 04:31, Matthew Brost wrote:
> > Rather than call free_job and run_job in same work item have a dedicated
> > work item for each. This aligns with the design and intended use of work
> > queues.
> >
> > Signed-off-by: Matthew Brost
> > ---
> > drivers/gpu/drm/scheduler/sched_main.c | 137 ++---
> > include/drm/gpu_scheduler.h| 8 +-
> > 2 files changed, 106 insertions(+), 39 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> > b/drivers/gpu/drm/scheduler/sched_main.c
> > index cede47afc800..b67469eac179 100644
> > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > @@ -213,11 +213,12 @@ void drm_sched_rq_remove_entity(struct drm_sched_rq
> > *rq,
> >* drm_sched_rq_select_entity_rr - Select an entity which could provide a
> > job to run
> >*
> >* @rq: scheduler run queue to check.
> > + * @dequeue: dequeue selected entity
> >*
> >* Try to find a ready entity, returns NULL if none found.
> >*/
> > static struct drm_sched_entity *
> > -drm_sched_rq_select_entity_rr(struct drm_sched_rq *rq)
> > +drm_sched_rq_select_entity_rr(struct drm_sched_rq *rq, bool dequeue)
> > {
> > struct drm_sched_entity *entity;
> > @@ -227,8 +228,10 @@ drm_sched_rq_select_entity_rr(struct drm_sched_rq *rq)
> > if (entity) {
> > list_for_each_entry_continue(entity, >entities, list) {
> > if (drm_sched_entity_is_ready(entity)) {
> > - rq->current_entity = entity;
> > - reinit_completion(>entity_idle);
> > + if (dequeue) {
> > + rq->current_entity = entity;
> > + reinit_completion(>entity_idle);
> > + }
> > spin_unlock(>lock);
> > return entity;
> > }
> > @@ -238,8 +241,10 @@ drm_sched_rq_select_entity_rr(struct drm_sched_rq *rq)
> > list_for_each_entry(entity, >entities, list) {
> > if (drm_sched_entity_is_ready(entity)) {
> > - rq->current_entity = entity;
> > - reinit_completion(>entity_idle);
> > + if (dequeue) {
> > + rq->current_entity = entity;
> > + reinit_completion(>entity_idle);
> > + }
> > spin_unlock(>lock);
> > return entity;
> > }
> > @@ -257,11 +262,12 @@ drm_sched_rq_select_entity_rr(struct drm_sched_rq *rq)
> >* drm_sched_rq_select_entity_fifo - Select an entity which provides a
> > job to run
> >*
> >* @rq: scheduler run queue to check.
> > + * @dequeue: dequeue selected entity
> >*
> >* Find oldest waiting ready entity, returns NULL if none found.
> >*/
> > static struct drm_sched_entity *
> > -drm_sched_rq_select_entity_fifo(struct drm_sched_rq *rq)
> > +drm_sched_rq_select_entity_fifo(struct drm_sched_rq *rq, bool dequeue)
> > {
> > struct rb_node *rb;
> > @@ -271,8 +277,10 @@ drm_sched_rq_select_entity_fifo(struct drm_sched_rq
> > *rq)
> > entity = rb_entry(rb, struct drm_sched_entity, rb_tree_node);
> > if (drm_sched_entity_is_ready(entity)) {
> > - rq->current_entity = entity;
> > - reinit_completion(>entity_idle);
> > + if (dequeue) {
> > + rq->current_entity = entity;
> > + reinit_completion(>entity_idle);
> > + }
> > break;
> > }
> > }
> > @@ -282,13 +290,54 @@ drm_sched_rq_select_entity_fifo(struct drm_sched_rq
> > *rq)
> > }
> > /**
> > - * drm_sched_submit_queue - scheduler queue submission
> > + * drm_sched_run_job_queue - queue job submission
> >* @sched: scheduler instance
> >*/
> > -static void drm_sched_submit_queue(struct drm_gpu_scheduler *sched)
> > +static void drm_sched_run_job_queue(struct drm_gpu_scheduler *sched)
> > {
> > if (!READ_ONCE(sched->pause_submit))
> > - queue_work(sched->submit_wq, >work_submit);
> > + queue_work(sched->submit_wq, >work_run_job);
> > +}
> > +
> > +static struct drm_sched_entity *
> > +drm_sched_select_entity(struct drm_gpu_scheduler *sched, bool dequeue);
> > +
> > +/**
> > + * drm_sched_run_job_queue_if_ready - queue job submission if ready
> > + * @sched: scheduler instance
> > + */
> > +static void drm_sched_run_job_queue_if_ready(struct drm_gpu_scheduler
> > *sched)
> > +{
> > + if (drm_sched_select_entity(sched, false))
> > + drm_sched_run_job_queue(sched);
> > +}
> > +
> > +/**
> > + * drm_sched_free_job_queue - queue free job
> > + *
> > + * @sched: scheduler instance to queue free job
> > + */
> >
[Bug 217664] Laptop doesnt wake up from suspend mode.
https://bugzilla.kernel.org/show_bug.cgi?id=217664 --- Comment #40 from popus_czy_to_ty (pentelja...@o2.pl) --- 1) sorry for very long spam but im not into compiling own kernel too much, first https://cateee.net/lkddb/web-lkddb/SATA_MOBILE_LPM_POLICY.html i seen that page what you wrote then i stopped on https://davidaugustat.com/linux/how-to-compile-linux-kernel-on-ubuntu step `$ cp -v /boot/config-$(uname -r) .config` , which it gaves me CONFIG_SATA_MOBILE_LPM_POLICY=3 potentially in both cases (make localmodconfig ) aswell 2) sata is itself kubuntu 23.04, i will try to install that on main nvme (after how to learn build from source not from distro -- 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/bridge: imx: fix potential NULL pointer dereference in imx8qxp_ldb_parse_dt_companion()
On Tue, Aug 29, 2023 at 02:01:25AM +0800, Zhang Shurong wrote:
> 在 2023年8月29日星期二 CST 上午1:28:22，Laurent Pinchart 写道：
> > Hi Zhang,
> >
> > Thank you for the patch.
> >
> > On Tue, Aug 29, 2023 at 12:55:01AM +0800, Zhang Shurong wrote:
> > > of_match_device() may fail and returns a NULL pointer.
> >
> > How can it return a NULL pointer here ?
> >
> > > Fix this by checking the return value of of_match_device().
> > >
> > > Fixes: 3818715f62b4 ("drm/bridge: imx: Add LDB support for i.MX8qxp")
> > > Signed-off-by: Zhang Shurong
> > > ---
> > >
> > > drivers/gpu/drm/bridge/imx/imx8qxp-ldb.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-ldb.c
> > > b/drivers/gpu/drm/bridge/imx/imx8qxp-ldb.c index
> > > 7984da9c0a35..d272f35c8eac 100644
> > > --- a/drivers/gpu/drm/bridge/imx/imx8qxp-ldb.c
> > > +++ b/drivers/gpu/drm/bridge/imx/imx8qxp-ldb.c
> > > @@ -488,6 +488,8 @@ static int imx8qxp_ldb_parse_dt_companion(struct
> > > imx8qxp_ldb *imx8qxp_ldb)
> > >* string.
> > >*/
> > > match = of_match_device(dev->driver->of_match_table, dev);
> > > + if (!match)
> > > + return -ENODEV;
> > > if (!of_device_is_compatible(companion, match->compatible)) {
> > > DRM_DEV_ERROR(dev, "companion LDB is incompatible\n");
> > > ret = -ENXIO;
>
> I think we can make it happen by designing the platform device in a way that
> its name aligns with that of the driver. In such a scenario, when the driver
> is probed, the of_match_device function will return null. You can verify this
> functionality by reviewing the following function:
>
> static int platform_match(struct device *dev, struct device_driver *drv)
This particular device is found in OF-based systems only, and only
instantiated through DT. You can create a platform_device manually that
may match this driver, but that would be a made-up situation, not
something that can happen in practice.
--
Regards,
Laurent Pinchart
Re: [PATCH 4/5] drm/bridge: samsung-dsim: adjust porches by rounding up
Hi Michael, On Mon, Aug 28, 2023 at 12:59 PM Michael Tretter wrote: > > The porches must be rounded up to make the samsung-dsim work. The commit log could be improved here. The way it is written gives the impression that samsung-dsim does not work currently.
Re: [PATCH 3/5] drm/bridge: samsung-dsim: update PLL reference clock
On Mon, Aug 28, 2023 at 11:42 AM Marco Felsch wrote:
>
> On 23-08-28, Michael Tretter wrote:
> > The PLL requires a clock between 2 MHz and 30 MHz after the pre-divider.
> > The reference clock for the PLL may change due to changes to it's parent
> > clock. Thus, the frequency may be out of range or unsuited for
> > generating the high speed clock for MIPI DSI.
> >
> > Try to keep the pre-devider small, and set the reference clock close to
> > 30 MHz before recalculating the PLL configuration. Use a divider with a
> > power of two for the reference clock as this seems to work best in
> > my tests.
> >
> > Signed-off-by: Michael Tretter
> > ---
> > drivers/gpu/drm/bridge/samsung-dsim.c | 15 +--
> > 1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> > b/drivers/gpu/drm/bridge/samsung-dsim.c
> > index da90c2038042..4de6e4f116db 100644
> > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > @@ -611,10 +611,21 @@ static unsigned long samsung_dsim_set_pll(struct
> > samsung_dsim *dsi,
> > u16 m;
> > u32 reg;
> >
> > - if (dsi->pll_clk)
> > + if (dsi->pll_clk) {
> > + /*
> > + * Ensure that the reference clock is generated with a power
> > of
> > + * two divider from its parent, but close to the PLLs upper
> > + * limit of the valid range of 2 MHz to 30 MHz.
> > + */
> > + fin = clk_get_rate(clk_get_parent(dsi->pll_clk));
> > + while (fin > 30 * MHZ)
> > + fin = fin / 2;
>
> Really just a cosmetic nit: fin /= 2;
>
> Reviewed-by: Marco Felsch
>
> > + clk_set_rate(dsi->pll_clk, fin);
> > +
> > fin = clk_get_rate(dsi->pll_clk);
> > - else
> > + } else {
> > fin = dsi->pll_clk_rate;
> > + }
I don't have all the code style rules memorized. Did you run it
through checkpatch? I wonder if the braces here are appropriate for a
1-line after the else. If checkpatch is happy, I am fine with it.
adam
> > dev_dbg(dsi->dev, "PLL ref clock freq %lu\n", fin);
> >
> > fout = samsung_dsim_pll_find_pms(dsi, fin, freq, , , );
> >
> > --
> > 2.39.2
> >
> >
> >
Re: [PATCH] drm/bridge: imx: fix potential NULL pointer dereference in imx8qxp_ldb_parse_dt_companion()
在 2023年8月29日星期二 CST 上午1:28:22，Laurent Pinchart 写道：
> Hi Zhang,
>
> Thank you for the patch.
>
> On Tue, Aug 29, 2023 at 12:55:01AM +0800, Zhang Shurong wrote:
> > of_match_device() may fail and returns a NULL pointer.
>
> How can it return a NULL pointer here ?
>
> > Fix this by checking the return value of of_match_device().
> >
> > Fixes: 3818715f62b4 ("drm/bridge: imx: Add LDB support for i.MX8qxp")
> > Signed-off-by: Zhang Shurong
> > ---
> >
> > drivers/gpu/drm/bridge/imx/imx8qxp-ldb.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-ldb.c
> > b/drivers/gpu/drm/bridge/imx/imx8qxp-ldb.c index
> > 7984da9c0a35..d272f35c8eac 100644
> > --- a/drivers/gpu/drm/bridge/imx/imx8qxp-ldb.c
> > +++ b/drivers/gpu/drm/bridge/imx/imx8qxp-ldb.c
> > @@ -488,6 +488,8 @@ static int imx8qxp_ldb_parse_dt_companion(struct
> > imx8qxp_ldb *imx8qxp_ldb)>
> > * string.
> > */
> >
> > match = of_match_device(dev->driver->of_match_table, dev);
> >
> > + if (!match)
> > + return -ENODEV;
> >
> > if (!of_device_is_compatible(companion, match->compatible)) {
> >
> > DRM_DEV_ERROR(dev, "companion LDB is incompatible\n");
> > ret = -ENXIO;
I think we can make it happen by designing the platform device in a way that
its name aligns with that of the driver. In such a scenario, when the driver
is probed, the of_match_device function will return null. You can verify this
functionality by reviewing the following function:
static int platform_match(struct device *dev, struct device_driver *drv)
Re: [PATCH v2 4/9] drm/sched: Split free_job into own work item
On 8/11/23 04:31, Matthew Brost wrote:
Rather than call free_job and run_job in same work item have a dedicated
work item for each. This aligns with the design and intended use of work
queues.
Signed-off-by: Matthew Brost
---
drivers/gpu/drm/scheduler/sched_main.c | 137 ++---
include/drm/gpu_scheduler.h| 8 +-
2 files changed, 106 insertions(+), 39 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/sched_main.c
b/drivers/gpu/drm/scheduler/sched_main.c
index cede47afc800..b67469eac179 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -213,11 +213,12 @@ void drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
* drm_sched_rq_select_entity_rr - Select an entity which could provide a job
to run
*
* @rq: scheduler run queue to check.
+ * @dequeue: dequeue selected entity
*
* Try to find a ready entity, returns NULL if none found.
*/
static struct drm_sched_entity *
-drm_sched_rq_select_entity_rr(struct drm_sched_rq *rq)
+drm_sched_rq_select_entity_rr(struct drm_sched_rq *rq, bool dequeue)
{
struct drm_sched_entity *entity;
@@ -227,8 +228,10 @@ drm_sched_rq_select_entity_rr(struct drm_sched_rq *rq)
if (entity) {
list_for_each_entry_continue(entity, >entities, list) {
if (drm_sched_entity_is_ready(entity)) {
- rq->current_entity = entity;
- reinit_completion(>entity_idle);
+ if (dequeue) {
+ rq->current_entity = entity;
+ reinit_completion(>entity_idle);
+ }
spin_unlock(>lock);
return entity;
}
@@ -238,8 +241,10 @@ drm_sched_rq_select_entity_rr(struct drm_sched_rq *rq)
list_for_each_entry(entity, >entities, list) {
if (drm_sched_entity_is_ready(entity)) {
- rq->current_entity = entity;
- reinit_completion(>entity_idle);
+ if (dequeue) {
+ rq->current_entity = entity;
+ reinit_completion(>entity_idle);
+ }
spin_unlock(>lock);
return entity;
}
@@ -257,11 +262,12 @@ drm_sched_rq_select_entity_rr(struct drm_sched_rq *rq)
* drm_sched_rq_select_entity_fifo - Select an entity which provides a job to
run
*
* @rq: scheduler run queue to check.
+ * @dequeue: dequeue selected entity
*
* Find oldest waiting ready entity, returns NULL if none found.
*/
static struct drm_sched_entity *
-drm_sched_rq_select_entity_fifo(struct drm_sched_rq *rq)
+drm_sched_rq_select_entity_fifo(struct drm_sched_rq *rq, bool dequeue)
{
struct rb_node *rb;
@@ -271,8 +277,10 @@ drm_sched_rq_select_entity_fifo(struct drm_sched_rq *rq)
entity = rb_entry(rb, struct drm_sched_entity, rb_tree_node);
if (drm_sched_entity_is_ready(entity)) {
- rq->current_entity = entity;
- reinit_completion(>entity_idle);
+ if (dequeue) {
+ rq->current_entity = entity;
+ reinit_completion(>entity_idle);
+ }
break;
}
}
@@ -282,13 +290,54 @@ drm_sched_rq_select_entity_fifo(struct drm_sched_rq *rq)
}
/**
- * drm_sched_submit_queue - scheduler queue submission
+ * drm_sched_run_job_queue - queue job submission
* @sched: scheduler instance
*/
-static void drm_sched_submit_queue(struct drm_gpu_scheduler *sched)
+static void drm_sched_run_job_queue(struct drm_gpu_scheduler *sched)
{
if (!READ_ONCE(sched->pause_submit))
- queue_work(sched->submit_wq, >work_submit);
+ queue_work(sched->submit_wq, >work_run_job);
+}
+
+static struct drm_sched_entity *
+drm_sched_select_entity(struct drm_gpu_scheduler *sched, bool dequeue);
+
+/**
+ * drm_sched_run_job_queue_if_ready - queue job submission if ready
+ * @sched: scheduler instance
+ */
+static void drm_sched_run_job_queue_if_ready(struct drm_gpu_scheduler *sched)
+{
+ if (drm_sched_select_entity(sched, false))
+ drm_sched_run_job_queue(sched);
+}
+
+/**
+ * drm_sched_free_job_queue - queue free job
+ *
+ * @sched: scheduler instance to queue free job
+ */
+static void drm_sched_free_job_queue(struct drm_gpu_scheduler *sched)
+{
+ if (!READ_ONCE(sched->pause_submit))
+ queue_work(sched->submit_wq, >work_free_job);
+}
+
+/**
+ * drm_sched_free_job_queue_if_ready - queue free job if ready
+ *
+ * @sched: scheduler instance to queue free job
+ */
+static void drm_sched_free_job_queue_if_ready(struct drm_gpu_scheduler
Re: [PATCH] drm/bridge: imx: fix potential NULL pointer dereference in imx8qxp_ldb_parse_dt_companion()
Hi Zhang,
Thank you for the patch.
On Tue, Aug 29, 2023 at 12:55:01AM +0800, Zhang Shurong wrote:
> of_match_device() may fail and returns a NULL pointer.
How can it return a NULL pointer here ?
> Fix this by checking the return value of of_match_device().
>
> Fixes: 3818715f62b4 ("drm/bridge: imx: Add LDB support for i.MX8qxp")
> Signed-off-by: Zhang Shurong
> ---
> drivers/gpu/drm/bridge/imx/imx8qxp-ldb.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-ldb.c
> b/drivers/gpu/drm/bridge/imx/imx8qxp-ldb.c
> index 7984da9c0a35..d272f35c8eac 100644
> --- a/drivers/gpu/drm/bridge/imx/imx8qxp-ldb.c
> +++ b/drivers/gpu/drm/bridge/imx/imx8qxp-ldb.c
> @@ -488,6 +488,8 @@ static int imx8qxp_ldb_parse_dt_companion(struct
> imx8qxp_ldb *imx8qxp_ldb)
>* string.
>*/
> match = of_match_device(dev->driver->of_match_table, dev);
> + if (!match)
> + return -ENODEV;
> if (!of_device_is_compatible(companion, match->compatible)) {
> DRM_DEV_ERROR(dev, "companion LDB is incompatible\n");
> ret = -ENXIO;
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/panel: Add prepare_prev_first flag to Visionox VTDR6130
Hi Neil
Sorry I didnt respond earlier on this thread.
On 8/28/2023 1:49 AM, neil.armstr...@linaro.org wrote:
Hi Jessica,
On 25/08/2023 20:37, Jessica Zhang wrote:
On 8/21/2023 3:01 AM, neil.armstr...@linaro.org wrote:
Hi Maxime,
On 21/08/2023 10:17, Maxime Ripard wrote:
Hi,
On Fri, Aug 18, 2023 at 10:25:48AM +0200, neil.armstr...@linaro.org
wrote:
On 17/08/2023 20:35, Dmitry Baryshkov wrote:
On 16/08/2023 10:51, neil.armstr...@linaro.org wrote:
Sending HS commands will always work on any controller, it's all
about LP commands. The Samsung panels you listed only send HS
commands so they can use prepare_prev_first and work on any
controllers.
I think there is some misunderstanding there, supported by the
description of the flag.
If I remember correctly, some hosts (sunxi) can not send DCS
commands after enabling video stream and switching to HS mode, see
[1]. Thus, as you know, most of the drivers have all DSI panel setup
commands in drm_panel_funcs::prepare() /
drm_bridge_funcs::pre_enable() callbacks, not paying attention
whether these commands are to be sent in LP or in HS mode.
Previously DSI source drivers could power on the DSI link either in
mode_set() or in pre_enable() callbacks, with mode_set() being the
hack to make panel/bridge drivers to be able to send commands from
their prepare() / pre_enable() callbacks.
With the prev_first flags being introduced, we have established that
DSI link should be enabled in DSI host's pre_enable() callback and
switched to HS mode (be it command or video) in the enable()
callback.
So far so good.
It seems coherent, I would like first to have a state of all DSI host
drivers and make this would actually work first before adding the
prev_first flag to all the required panels.
This is definitely what we should do in an ideal world, but at least
for
sunxi there's no easy way for it at the moment. There's no
documentation
for it and the driver provided doesn't allow this to happen.
Note that I'm not trying to discourage you or something here, I'm
simply
pointing out that this will be something that we will have to take into
account. And it's possible that other drivers are in a similar
situation.
Unfortunately this change is not fully backwards-compatible. This
requires that all DSI panels sending commands from prepare() should
have the prepare_prev_first flag. In some sense, all such patches
might have Fixes: 5ea6b1702781 ("drm/panel: Add prepare_prev_first
flag to drm_panel").
This kind of migration should be done *before* any possible
regression, not the other way round.
If all panels sending commands from prepare() should have the
prepare_prev_first flag, then it should be first, check for
regressions then continue.
I understand, but this patch doesn't qualify as a fix for
9e15123eca79 and is too late to be merged in drm-misc-next for
v6.6, and since 9e15123eca79 actually breaks some support it
should be reverted (+ deps) since we are late in the rc cycles.
If we go this way, we can never reapply these patches. There will be
no guarantee that all panel drivers are completely converted. We
already have a story without an observable end -
DRM_BRIDGE_ATTACH_NO_CONNECTOR.
I don't understand this point, who would block re-applying the
patches ?
The migration to DRM_BRIDGE_ATTACH_NO_CONNECTOR was done over multiple
Linux version and went smoothly because we reverted regressing patches
and restarted when needed, I don't understand why we can't do this
here aswell.
I'd consider that the DSI driver is correct here and it is about the
panel drivers that require fixes patches. If you care about the
particular Fixes tag, I have provided one several lines above.
Unfortunately it should be done in the other way round, prepare for
migration, then migrate,
I mean if it's a required migration, then it should be done and I'll
support it from both bridge and panel PoV.
So, first this patch has the wrong Fixes tag, and I would like a
better explanation on the commit message in any case. Then I would
like to have an ack from some drm-misc maintainers before applying it
because it fixes a patch that was sent via the msm tree thus per the
drm-misc rules I cannot apply it via the drm-misc-next-fixes tree.
Sorry, it's not clear to me what you'd like our feedback on exactly?
So let me resume the situation:
- pre_enable_prev_first was introduced in [1]
- some panels made use of pre_enable_prev_first
- Visionox VTDR6130 was enabled on SM8550 systems and works on v6.5
kernels and before
- patch [2] was introduced on MSM DRM tree, breaking VTDR6130 on
SM8550 systems (and probably other Video mode panels on Qcom platforms)
- this fix was sent late, and is now too late to be merged via
drm-misc-next
Hi Neil and Maxime,
I agree with Neil that 9e15123eca79 was the commit that introduced the
issue (since it changed the MSM DSI host behavior).
However, I'm not too keen on simply reverting that patch because
1) it's not wrong to have
[PATCH] drm/bridge: imx: fix potential NULL pointer dereference in imx8qxp_ldb_parse_dt_companion()
of_match_device() may fail and returns a NULL pointer.
Fix this by checking the return value of of_match_device().
Fixes: 3818715f62b4 ("drm/bridge: imx: Add LDB support for i.MX8qxp")
Signed-off-by: Zhang Shurong
---
drivers/gpu/drm/bridge/imx/imx8qxp-ldb.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-ldb.c
b/drivers/gpu/drm/bridge/imx/imx8qxp-ldb.c
index 7984da9c0a35..d272f35c8eac 100644
--- a/drivers/gpu/drm/bridge/imx/imx8qxp-ldb.c
+++ b/drivers/gpu/drm/bridge/imx/imx8qxp-ldb.c
@@ -488,6 +488,8 @@ static int imx8qxp_ldb_parse_dt_companion(struct
imx8qxp_ldb *imx8qxp_ldb)
* string.
*/
match = of_match_device(dev->driver->of_match_table, dev);
+ if (!match)
+ return -ENODEV;
if (!of_device_is_compatible(companion, match->compatible)) {
DRM_DEV_ERROR(dev, "companion LDB is incompatible\n");
ret = -ENXIO;
--
2.30.2
Re: [PATCH 0/5] drm/bridge: samsung-dsim: fix various modes with ADV7535 bridge
On 23-08-28, Michael Tretter wrote: > I tested the i.MX8M Nano EVK with the NXP supplied MIPI-DSI adapter, > which uses an ADV7535 MIPI-DSI to HDMI converter. I found that a few > modes were working, but in many modes my monitor stayed dark. > > This series fixes the Samsung DSIM bridge driver to bring up a few more > modes: > > The driver read the rate of the PLL ref clock only during probe. > However, if the clock is re-parented to the VIDEO_PLL, changes to the > pixel clock have an effect on the PLL ref clock. Therefore, the driver > must read and potentially update the PLL ref clock on every modeset. > > I also found that the rounding mode of the porches and active area has > an effect on the working modes. If the driver rounds up instead of > rounding down and be calculates them in Hz instead of kHz, more modes > start to work. > > The following table shows the modes that were working in my test without > this patch set and the modes that are working now: > > |Mode | Before | Now | > | 1920x1080-60.00 | X | X | > | 1920x1080-59.94 || X | > | 1920x1080-50.00 || X | > | 1920x1080-30.00 || X | > | 1920x1080-29.97 || X | > | 1920x1080-25.00 || X | > | 1920x1080-24.00 || | > | 1920x1080-23.98 || | > | 1680x1050-59.88 || X | > | 1280x1024-75.03 | X | X | > | 1280x1024-60.02 | X | X | > | 1200x960-59.99 || X | > | 1152x864-75.00 | X | X | > | 1280x720-60.00 || | > | 1280x720-59.94 || | > | 1280x720-50.00 || X | > | 1024x768-75.03 || X | > | 1024x768-60.00 || X | > | 800x600-75.00 | X | X | > | 800x600-60.32 | X | X | > | 720x576-50.00 | X | X | > | 720x480-60.00 || | > | 720x480-59.94 | X | | > | 640x480-75.00 | X | X | > | 640x480-60.00 || X | > | 640x480-59.94 || X | > | 720x400-70.08 || | > > Interestingly, the 720x480-59.94 mode stopped working. However, I am > able to bring up the 720x480 modes by manually hacking the active area > (hsa) to 40 and carefully adjusting the clocks, but something still > seems to be off. > > Unfortunately, a few more modes are still not working at all. The NXP > downstream kernel has some quirks to handle some of the modes especially > wrt. to the porches, but I cannot figure out, what the driver should > actually do in these cases. Maybe there is still an error in the > calculation of the porches and someone at NXP can chime in. > > Michael > > Signed-off-by: Michael Tretter > --- > Marco Felsch (1): > drm/bridge: samsung-dsim: add more mipi-dsi device debug information > > Michael Tretter (4): > drm/bridge: samsung-dsim: reread ref clock before configuring PLL > drm/bridge: samsung-dsim: update PLL reference clock > drm/bridge: samsung-dsim: adjust porches by rounding up > drm/bridge: samsung-dsim: calculate porches in Hz Feel free to add my r-b for your patches. Reviewed-by: Marco Felsch
Re: [PATCH 3/5] drm/bridge: samsung-dsim: update PLL reference clock
On 23-08-28, Michael Tretter wrote:
> The PLL requires a clock between 2 MHz and 30 MHz after the pre-divider.
> The reference clock for the PLL may change due to changes to it's parent
> clock. Thus, the frequency may be out of range or unsuited for
> generating the high speed clock for MIPI DSI.
>
> Try to keep the pre-devider small, and set the reference clock close to
> 30 MHz before recalculating the PLL configuration. Use a divider with a
> power of two for the reference clock as this seems to work best in
> my tests.
>
> Signed-off-by: Michael Tretter
> ---
> drivers/gpu/drm/bridge/samsung-dsim.c | 15 +--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> b/drivers/gpu/drm/bridge/samsung-dsim.c
> index da90c2038042..4de6e4f116db 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -611,10 +611,21 @@ static unsigned long samsung_dsim_set_pll(struct
> samsung_dsim *dsi,
> u16 m;
> u32 reg;
>
> - if (dsi->pll_clk)
> + if (dsi->pll_clk) {
> + /*
> + * Ensure that the reference clock is generated with a power of
> + * two divider from its parent, but close to the PLLs upper
> + * limit of the valid range of 2 MHz to 30 MHz.
> + */
> + fin = clk_get_rate(clk_get_parent(dsi->pll_clk));
> + while (fin > 30 * MHZ)
> + fin = fin / 2;
Really just a cosmetic nit: fin /= 2;
Reviewed-by: Marco Felsch
> + clk_set_rate(dsi->pll_clk, fin);
> +
> fin = clk_get_rate(dsi->pll_clk);
> - else
> + } else {
> fin = dsi->pll_clk_rate;
> + }
> dev_dbg(dsi->dev, "PLL ref clock freq %lu\n", fin);
>
> fout = samsung_dsim_pll_find_pms(dsi, fin, freq, , , );
>
> --
> 2.39.2
>
>
>
Re: [PATCH 01/20] drm/xlnx/zynqmp_disp: Use correct kerneldoc formatting in zynqmp_disp
Hi Lee,
Thank you for the patch.
On Thu, Aug 24, 2023 at 08:36:46AM +0100, Lee Jones wrote:
> Fixes the following W=1 kernel build warning(s):
>
> drivers/gpu/drm/xlnx/zynqmp_disp.c:151: warning: Function parameter or
> member 'blend' not described in 'zynqmp_disp'
> drivers/gpu/drm/xlnx/zynqmp_disp.c:151: warning: Function parameter or
> member 'avbuf' not described in 'zynqmp_disp'
> drivers/gpu/drm/xlnx/zynqmp_disp.c:151: warning: Function parameter or
> member 'audio' not described in 'zynqmp_disp'
>
> Signed-off-by: Lee Jones
> ---
> Cc: Hyun Kwon
> Cc: Laurent Pinchart
> Cc: David Airlie
> Cc: Daniel Vetter
> Cc: Michal Simek
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-arm-ker...@lists.infradead.org
> ---
> drivers/gpu/drm/xlnx/zynqmp_disp.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> index 407bc07cec69a..7c64ab11fe2b0 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> @@ -128,9 +128,9 @@ struct zynqmp_disp_layer {
> * struct zynqmp_disp - Display controller
> * @dev: Device structure
> * @dpsub: Display subsystem
> - * @blend.base: Register I/O base address for the blender
> - * @avbuf.base: Register I/O base address for the audio/video buffer manager
> - * @audio.base: Registers I/O base address for the audio mixer
> + * @blend: .base: Register I/O base address for the blender
> + * @avbuf: .base: Register I/O base address for the audio/video buffer
> manager
> + * @audio: .base: Registers I/O base address for the audio mixer
This is really a hack to work around the warning, and not a clean fix.
kerneldoc is still today unable, as far as I understand, to document
nested structures. That's annoying, and the warning is a good way to
remind us that it needs to be fixed. I'd be tempted to keep the warning
for that reason.
> * @layers: Layers (planes)
> */
> struct zynqmp_disp {
--
Regards,
Laurent Pinchart
Re: [PATCH 0/5] drm/bridge: samsung-dsim: fix various modes with ADV7535 bridge
On Mon, Aug 28, 2023 at 10:59 AM Michael Tretter wrote: > > I tested the i.MX8M Nano EVK with the NXP supplied MIPI-DSI adapter, > which uses an ADV7535 MIPI-DSI to HDMI converter. I found that a few > modes were working, but in many modes my monitor stayed dark. > > This series fixes the Samsung DSIM bridge driver to bring up a few more > modes: > > The driver read the rate of the PLL ref clock only during probe. > However, if the clock is re-parented to the VIDEO_PLL, changes to the > pixel clock have an effect on the PLL ref clock. Therefore, the driver > must read and potentially update the PLL ref clock on every modeset. > > I also found that the rounding mode of the porches and active area has > an effect on the working modes. If the driver rounds up instead of > rounding down and be calculates them in Hz instead of kHz, more modes > start to work. > > The following table shows the modes that were working in my test without > this patch set and the modes that are working now: > > |Mode | Before | Now | > | 1920x1080-60.00 | X | X | > | 1920x1080-59.94 || X | > | 1920x1080-50.00 || X | > | 1920x1080-30.00 || X | > | 1920x1080-29.97 || X | > | 1920x1080-25.00 || X | > | 1920x1080-24.00 || | > | 1920x1080-23.98 || | > | 1680x1050-59.88 || X | > | 1280x1024-75.03 | X | X | > | 1280x1024-60.02 | X | X | > | 1200x960-59.99 || X | > | 1152x864-75.00 | X | X | > | 1280x720-60.00 || | > | 1280x720-59.94 || | > | 1280x720-50.00 || X | > | 1024x768-75.03 || X | > | 1024x768-60.00 || X | > | 800x600-75.00 | X | X | > | 800x600-60.32 | X | X | > | 720x576-50.00 | X | X | > | 720x480-60.00 || | > | 720x480-59.94 | X | | > | 640x480-75.00 | X | X | > | 640x480-60.00 || X | > | 640x480-59.94 || X | > | 720x400-70.08 || | > Nice! > Interestingly, the 720x480-59.94 mode stopped working. However, I am > able to bring up the 720x480 modes by manually hacking the active area > (hsa) to 40 and carefully adjusting the clocks, but something still > seems to be off. Checkout my branch: https://github.com/aford173/linux/commit/183cf6d154afeb9b0300500b09d7b8ec53047a12 I found that certain resolutions don't properly round, and it seemed to be related to the size of HFP. HFP of 110 when divded between 4 lanes, required a round-up, but then I had to recalculate the rest of the horizontal timings to compensate. I was going to push that as an RFC, but I will investigate your patch series first. With my small rounding correction, I am able to get 720x480 and 720p on my imx8mp, but not my mini/nano, so I am hoping your patch series fixes that issue for me. > > Unfortunately, a few more modes are still not working at all. The NXP > downstream kernel has some quirks to handle some of the modes especially > wrt. to the porches, but I cannot figure out, what the driver should > actually do in these cases. Maybe there is still an error in the > calculation of the porches and someone at NXP can chime in. Hopefully the comment in my above patch explains how the calculation is corrected and adjusted to get some of the missing resolutions. > Michael > > Signed-off-by: Michael Tretter I'll try to reivew this week and respond with my feedback. adam > --- > Marco Felsch (1): > drm/bridge: samsung-dsim: add more mipi-dsi device debug information > > Michael Tretter (4): > drm/bridge: samsung-dsim: reread ref clock before configuring PLL > drm/bridge: samsung-dsim: update PLL reference clock > drm/bridge: samsung-dsim: adjust porches by rounding up > drm/bridge: samsung-dsim: calculate porches in Hz > > drivers/gpu/drm/bridge/samsung-dsim.c | 42 > +-- > include/drm/bridge/samsung-dsim.h | 1 + > 2 files changed, 31 insertions(+), 12 deletions(-) > --- > base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c > change-id: 20230818-samsung-dsim-42346444bce5 > > Best regards, > -- > Michael Tretter >
Re: [PATCH (set 1) 00/20] Rid W=1 warnings from GPU
On 8/24/23 14:07, Lee Jones wrote: > On Thu, 24 Aug 2023, Jani Nikula wrote: >> On Thu, 24 Aug 2023, Lee Jones wrote: >>> This set is part of a larger effort attempting to clean-up W=1 >>> kernel builds, which are currently overwhelmingly riddled with >>> niggly little warnings. >> >> The next question is, how do we keep it W=1 clean going forward? > > My plan was to fix them all, then move each warning to W=0. > > Arnd recently submitted a set doing just that for a bunch of them. > > https://lore.kernel.org/all/20230811140327.3754597-1-a...@kernel.org/ > > I like to think a bunch of this is built on top of my previous efforts. > > GPU is a particularly tricky though - the warnings seem to come in faster > than I can squash them. Maybe the maintainers can find a way to test > new patches on merge? One approach for this which has proved effective in Mesa and other projects is to make warnings fatal in CI which must pass for any changes to be merged. There is ongoing work toward introducing this for the DRM subsystem, using gitlab.freedesktop.org CI. -- Earthling Michel Dänzer| https://redhat.com Libre software enthusiast | Mesa and Xwayland developer
Re: [RFC PATCH 04/10] drm/panel_helper: Introduce drm_panel_helper
Hi, On Mon, Aug 28, 2023 at 12:45 AM Maxime Ripard wrote: > > > For removal I'd be fine with just dropping the call and saying it's > > the responsibility of the driver to call drm_atomic_helper_shutdown(), > > as you suggest. I'd tend to believe that removal of DRM drivers is not > > used anywhere in "production" code (or at least not common) and I > > think it's super hard to get it right, to unregister and unbind all > > the DRM components in the right order. > > It depends on the kind of devices. USB devices are very likely to be > removed, platform devices very unlikely, and PCIe cards somewhere in the > middle :) Good point. Obviously those cases need to work. I guess I've just spent lots of time on the SoC case where all the pieces coming together are very intertwined with each other... > > Shutdown is called any time you reboot a device. That means that if a > > DRM driver is _not_ calling drm_atomic_helper_shutdown() on the > > panel's behalf at shutdown time then the panel won't be powered off > > properly. This feels to me like something that might actually matter. > > It does matter. What I disagree on is that you suggest working around > that brokenness in the core framework. What I'm saying is driver is > broken, we should keep the core framework sane and fix it in the driver. > > It should be fairly easy with a coccinelle script to figure out which > panels are affected, and to add that call in remove. I think I'm confused here. I've already figured out which panels are affected in my patch series, right? It's the set of panels that today try to power the panel off in their shutdown call, right? ...but I think we can't add the call you're suggesting, drm_atomic_helper_shutdown(), to the _panel_'s shutdown callback, can we? We need to add it to the shutdown callback of the top-level DRM driver, right? > > Panels tend to be one of those things that really care about their > > power sequencing and can even get damaged (or not turn on properly > > next time) if sequencing is not done properly, so just removing this > > code and putting the blame on the DRM driver seems scary to me. > > Sure, it's bad. But there's no difference compared to the approach you > suggest in that patch: you created a helper, yes, but every driver will > still have to call that helper and if they don't, the panel will still > be called and it's a bug. And we would have to convert everything to > that new helper. > > It's fundamentally the same discussion than what you were opposed to > above. I think the key difference here is that, if I understand correctly, drm_atomic_helper_shutdown() needs to be added to the top-level DRM driver, not to the panel itself. I guess I'm worried that I'll either miss a case or that simply adding a call to drm_atomic_helper_shutdown() in the top-level DRM driver will break something. Well, I suppose I can try it and see what happens... I'll try to cook up a v2 and we'll see what people say. I might keep the RFC tag for v2 just because I expect it to still be touching a lot of stuff. -Doug
Re: [PATCH 2/2] ARM: dts: imx: add support for the ATM0700D4 panel attached to sk-imx53
Hi Dmitry,
Thank you for the patch.
On Sun, Aug 27, 2023 at 12:54:29AM +0300, Dmitry Baryshkov wrote:
> The SK-ATM0700D4-Plug is an extension board (provided by the same
> manufacturer, [1]) which can be connected to the SK-IMX53 panel kit. The
> panel can be connected either using the RGB parallel bus or using the
> LVDS connector (recommended). Add DT files describing this "shield",
> both RGB and LVDS connections.
Shouldn't these be implemented as overlays ?
> [1] http://starterkit.ru/html/index.php?name=shop=view=64
>
> Signed-off-by: Dmitry Baryshkov
> ---
> arch/arm/boot/dts/nxp/imx/Makefile| 2 +
> .../nxp/imx/imx53-sk-imx53-atm0700d4-lvds.dts | 97 +++
> .../nxp/imx/imx53-sk-imx53-atm0700d4-rgb.dts | 112 ++
> .../dts/nxp/imx/imx53-sk-imx53-atm0700d4.dtsi | 45 +++
> 4 files changed, 256 insertions(+)
> create mode 100644
> arch/arm/boot/dts/nxp/imx/imx53-sk-imx53-atm0700d4-lvds.dts
> create mode 100644 arch/arm/boot/dts/nxp/imx/imx53-sk-imx53-atm0700d4-rgb.dts
> create mode 100644 arch/arm/boot/dts/nxp/imx/imx53-sk-imx53-atm0700d4.dtsi
>
> diff --git a/arch/arm/boot/dts/nxp/imx/Makefile
> b/arch/arm/boot/dts/nxp/imx/Makefile
> index 3629e343d322..7532ed6468a0 100644
> --- a/arch/arm/boot/dts/nxp/imx/Makefile
> +++ b/arch/arm/boot/dts/nxp/imx/Makefile
> @@ -47,6 +47,8 @@ dtb-$(CONFIG_SOC_IMX53) += \
> imx53-qsb.dtb \
> imx53-qsrb.dtb \
> imx53-sk-imx53.dtb \
> + imx53-sk-imx53-atm0700d4-lvds.dtb \
> + imx53-sk-imx53-atm0700d4-rgb.dtb \
> imx53-smd.dtb \
> imx53-tx53-x03x.dtb \
> imx53-tx53-x13x.dtb \
> diff --git a/arch/arm/boot/dts/nxp/imx/imx53-sk-imx53-atm0700d4-lvds.dts
> b/arch/arm/boot/dts/nxp/imx/imx53-sk-imx53-atm0700d4-lvds.dts
> new file mode 100644
> index ..b1c1e7c759b3
> --- /dev/null
> +++ b/arch/arm/boot/dts/nxp/imx/imx53-sk-imx53-atm0700d4-lvds.dts
> @@ -0,0 +1,97 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +//
> +// Copyright 2023 Linaro Ltd.
> +
> +/dts-v1/;
> +
> +#include
> +#include "imx53-sk-imx53-atm0700d4.dtsi"
> +
> +/ {
> + lvds-decoder {
> + compatible = "ti,sn65lvds94", "lvds-decoder";
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> +
> + lvds_decoder_in: endpoint {
> + remote-endpoint = <_out>;
> + };
> + };
> +
> + port@1 {
> + reg = <1>;
> +
> + lvds_decoder_out: endpoint {
> + remote-endpoint = <_rgb_in>;
> + };
> + };
> + };
> + };
> +};
> +
> + {
> + pinctrl_lvds0: lvds0grp {
> + /* LVDS pins only have pin mux configuration */
> + fsl,pins = <
> + MX53_PAD_LVDS0_CLK_P__LDB_LVDS0_CLK 0x8000
> + MX53_PAD_LVDS0_TX0_P__LDB_LVDS0_TX0 0x8000
> + MX53_PAD_LVDS0_TX1_P__LDB_LVDS0_TX1 0x8000
> + MX53_PAD_LVDS0_TX2_P__LDB_LVDS0_TX2 0x8000
> + MX53_PAD_LVDS0_TX3_P__LDB_LVDS0_TX3 0x8000
> + >;
> + };
> +
> + pinctrl_spi_gpio: spigrp {
> + fsl,pins = <
> + MX53_PAD_EIM_A22__GPIO2_16 0x1f4
> + MX53_PAD_EIM_A21__GPIO2_17 0x1f4
> + MX53_PAD_EIM_A16__GPIO2_22 0x1f4
> + MX53_PAD_EIM_A18__GPIO2_20 0x1f4
> + >;
> + };
> +};
> +
> + {
> + pinctrl-names = "default";
> + pinctrl-0 = <_lvds0>;
> + status = "okay";
> +
> + lvds0: lvds-channel@0 {
> + reg = <0>;
> + fsl,data-mapping = "spwg";
> + fsl,data-width = <24>;
> + status = "okay";
> +
> + port@2 {
> + reg = <2>;
> +
> + lvds0_out: endpoint {
> + remote-endpoint = <_decoder_in>;
> + };
> + };
> + };
> +};
> +
> +_rgb_in {
> + remote-endpoint = <_decoder_out>;
> +};
> +
> +_ts {
> + pinctrl-0 = <_spi_gpio>;
> + pinctrl-names = "default";
> +
> + sck-gpios = < 16 GPIO_ACTIVE_HIGH>;
> + miso-gpios = < 22 GPIO_ACTIVE_HIGH>;
> + mosi-gpios = < 17 GPIO_ACTIVE_HIGH>;
> + cs-gpios = < 20 GPIO_ACTIVE_HIGH>;
> +};
> +
> + {
> + interrupts-extended = < 22 IRQ_TYPE_EDGE_BOTH>;
> + pendown-gpio = < 22 GPIO_ACTIVE_LOW>;
> +};
> diff --git a/arch/arm/boot/dts/nxp/imx/imx53-sk-imx53-atm0700d4-rgb.dts
> b/arch/arm/boot/dts/nxp/imx/imx53-sk-imx53-atm0700d4-rgb.dts
> new file mode 100644
> index ..2559ada7e401
> ---
Re: [PATCH 1/2] dt-bindings: display/lvds-codec: add ti,sn65lvds94
Hi Dmitry, Thank you for the patch. On Sun, Aug 27, 2023 at 12:54:28AM +0300, Dmitry Baryshkov wrote: > Add compatible strings for TI sn65lvds94, LVDS serdes receiver. > > Signed-off-by: Dmitry Baryshkov Reviewed-by: Laurent Pinchart > --- > Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml > b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml > index 84aafcbf0919..6ceeed76e88e 100644 > --- a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml > +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml > @@ -41,6 +41,7 @@ properties: >- enum: >- ti,ds90cf364a # For the DS90CF364A FPD-Link LVDS Receiver >- ti,ds90cf384a # For the DS90CF384A FPD-Link LVDS Receiver > + - ti,sn65lvds94 # For the SN65DS94 LVDS serdes >- const: lvds-decoder # Generic LVDS decoders compatible fallback >- enum: >- thine,thc63lvdm83d # For the THC63LVDM83D LVDS serializer -- Regards, Laurent Pinchart
[PATCH 5/5] drm/bridge: samsung-dsim: calculate porches in Hz
Calculating the byte_clk in kHz is imprecise for a hs_clock of 55687500
Hz, which may be used with a pixel clock of 74.25 MHz with mode
1920x1080-30.
Fix the calculation by using HZ instead of kHZ.
This requires to change the type to u64 to prevent overflows of the
integer type.
Signed-off-by: Michael Tretter
---
drivers/gpu/drm/bridge/samsung-dsim.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
b/drivers/gpu/drm/bridge/samsung-dsim.c
index 459be953be55..eb7aca2b9ab7 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -973,10 +973,12 @@ static void samsung_dsim_set_display_mode(struct
samsung_dsim *dsi)
u32 reg;
if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO) {
- int byte_clk_khz = dsi->hs_clock / 1000 / 8;
- int hfp = DIV_ROUND_UP((m->hsync_start - m->hdisplay) *
byte_clk_khz, m->clock);
- int hbp = DIV_ROUND_UP((m->htotal - m->hsync_end) *
byte_clk_khz, m->clock);
- int hsa = DIV_ROUND_UP((m->hsync_end - m->hsync_start) *
byte_clk_khz, m->clock);
+ u64 byte_clk = dsi->hs_clock / 8;
+ u64 pix_clk = m->clock * 1000;
+
+ int hfp = DIV64_U64_ROUND_UP((m->hsync_start - m->hdisplay) *
byte_clk, pix_clk);
+ int hbp = DIV64_U64_ROUND_UP((m->htotal - m->hsync_end) *
byte_clk, pix_clk);
+ int hsa = DIV64_U64_ROUND_UP((m->hsync_end - m->hsync_start) *
byte_clk, pix_clk);
/* remove packet overhead when possible */
hfp = max(hfp - 6, 0);
--
2.39.2
[PATCH 3/5] drm/bridge: samsung-dsim: update PLL reference clock
The PLL requires a clock between 2 MHz and 30 MHz after the pre-divider.
The reference clock for the PLL may change due to changes to it's parent
clock. Thus, the frequency may be out of range or unsuited for
generating the high speed clock for MIPI DSI.
Try to keep the pre-devider small, and set the reference clock close to
30 MHz before recalculating the PLL configuration. Use a divider with a
power of two for the reference clock as this seems to work best in
my tests.
Signed-off-by: Michael Tretter
---
drivers/gpu/drm/bridge/samsung-dsim.c | 15 +--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
b/drivers/gpu/drm/bridge/samsung-dsim.c
index da90c2038042..4de6e4f116db 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -611,10 +611,21 @@ static unsigned long samsung_dsim_set_pll(struct
samsung_dsim *dsi,
u16 m;
u32 reg;
- if (dsi->pll_clk)
+ if (dsi->pll_clk) {
+ /*
+* Ensure that the reference clock is generated with a power of
+* two divider from its parent, but close to the PLLs upper
+* limit of the valid range of 2 MHz to 30 MHz.
+*/
+ fin = clk_get_rate(clk_get_parent(dsi->pll_clk));
+ while (fin > 30 * MHZ)
+ fin = fin / 2;
+ clk_set_rate(dsi->pll_clk, fin);
+
fin = clk_get_rate(dsi->pll_clk);
- else
+ } else {
fin = dsi->pll_clk_rate;
+ }
dev_dbg(dsi->dev, "PLL ref clock freq %lu\n", fin);
fout = samsung_dsim_pll_find_pms(dsi, fin, freq, , , );
--
2.39.2
[PATCH 4/5] drm/bridge: samsung-dsim: adjust porches by rounding up
The porches must be rounded up to make the samsung-dsim work.
Signed-off-by: Michael Tretter
---
drivers/gpu/drm/bridge/samsung-dsim.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
b/drivers/gpu/drm/bridge/samsung-dsim.c
index 4de6e4f116db..459be953be55 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -974,9 +974,9 @@ static void samsung_dsim_set_display_mode(struct
samsung_dsim *dsi)
if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO) {
int byte_clk_khz = dsi->hs_clock / 1000 / 8;
- int hfp = (m->hsync_start - m->hdisplay) * byte_clk_khz /
m->clock;
- int hbp = (m->htotal - m->hsync_end) * byte_clk_khz / m->clock;
- int hsa = (m->hsync_end - m->hsync_start) * byte_clk_khz /
m->clock;
+ int hfp = DIV_ROUND_UP((m->hsync_start - m->hdisplay) *
byte_clk_khz, m->clock);
+ int hbp = DIV_ROUND_UP((m->htotal - m->hsync_end) *
byte_clk_khz, m->clock);
+ int hsa = DIV_ROUND_UP((m->hsync_end - m->hsync_start) *
byte_clk_khz, m->clock);
/* remove packet overhead when possible */
hfp = max(hfp - 6, 0);
--
2.39.2
[PATCH 1/5] drm/bridge: samsung-dsim: add more mipi-dsi device debug information
From: Marco Felsch Since the MIPI configuration can be changed on demand it is very useful to print more MIPI settings during the MIPI device attach step. Signed-off-by: Marco Felsch Signed-off-by: Michael Tretter --- drivers/gpu/drm/bridge/samsung-dsim.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c index 73ec60757dbc..6778f1751faa 100644 --- a/drivers/gpu/drm/bridge/samsung-dsim.c +++ b/drivers/gpu/drm/bridge/samsung-dsim.c @@ -1711,7 +1711,10 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host, return ret; } - DRM_DEV_INFO(dev, "Attached %s device\n", device->name); + DRM_DEV_INFO(dev, "Attached %s device (lanes:%d bpp:%d mode-flags:0x%lx)\n", +device->name, device->lanes, +mipi_dsi_pixel_format_to_bpp(device->format), +device->mode_flags); drm_bridge_add(>bridge); -- 2.39.2
[PATCH 2/5] drm/bridge: samsung-dsim: reread ref clock before configuring PLL
The PLL reference clock may change at runtime. Thus, reading the clock
rate during probe is not sufficient to correctly configure the PLL for
the expected hs clock.
Read the actual rate of the reference clock before calculating the PLL
configuration parameters.
Signed-off-by: Michael Tretter
---
drivers/gpu/drm/bridge/samsung-dsim.c | 16 +---
include/drm/bridge/samsung-dsim.h | 1 +
2 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
b/drivers/gpu/drm/bridge/samsung-dsim.c
index 6778f1751faa..da90c2038042 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -611,7 +611,12 @@ static unsigned long samsung_dsim_set_pll(struct
samsung_dsim *dsi,
u16 m;
u32 reg;
- fin = dsi->pll_clk_rate;
+ if (dsi->pll_clk)
+ fin = clk_get_rate(dsi->pll_clk);
+ else
+ fin = dsi->pll_clk_rate;
+ dev_dbg(dsi->dev, "PLL ref clock freq %lu\n", fin);
+
fout = samsung_dsim_pll_find_pms(dsi, fin, freq, , , );
if (!fout) {
dev_err(dsi->dev,
@@ -1821,18 +1826,15 @@ static int samsung_dsim_parse_dt(struct samsung_dsim
*dsi)
u32 lane_polarities[5] = { 0 };
struct device_node *endpoint;
int i, nr_lanes, ret;
- struct clk *pll_clk;
ret = samsung_dsim_of_read_u32(node, "samsung,pll-clock-frequency",
>pll_clk_rate, 1);
/* If it doesn't exist, read it from the clock instead of failing */
if (ret < 0) {
dev_dbg(dev, "Using sclk_mipi for pll clock frequency\n");
- pll_clk = devm_clk_get(dev, "sclk_mipi");
- if (!IS_ERR(pll_clk))
- dsi->pll_clk_rate = clk_get_rate(pll_clk);
- else
- return PTR_ERR(pll_clk);
+ dsi->pll_clk = devm_clk_get(dev, "sclk_mipi");
+ if (IS_ERR(dsi->pll_clk))
+ return PTR_ERR(dsi->pll_clk);
}
/* If it doesn't exist, use pixel clock instead of failing */
diff --git a/include/drm/bridge/samsung-dsim.h
b/include/drm/bridge/samsung-dsim.h
index 05100e91ecb9..31ff88f152fb 100644
--- a/include/drm/bridge/samsung-dsim.h
+++ b/include/drm/bridge/samsung-dsim.h
@@ -87,6 +87,7 @@ struct samsung_dsim {
void __iomem *reg_base;
struct phy *phy;
struct clk **clks;
+ struct clk *pll_clk;
struct regulator_bulk_data supplies[2];
int irq;
struct gpio_desc *te_gpio;
--
2.39.2
[PATCH 0/5] drm/bridge: samsung-dsim: fix various modes with ADV7535 bridge
I tested the i.MX8M Nano EVK with the NXP supplied MIPI-DSI adapter, which uses an ADV7535 MIPI-DSI to HDMI converter. I found that a few modes were working, but in many modes my monitor stayed dark. This series fixes the Samsung DSIM bridge driver to bring up a few more modes: The driver read the rate of the PLL ref clock only during probe. However, if the clock is re-parented to the VIDEO_PLL, changes to the pixel clock have an effect on the PLL ref clock. Therefore, the driver must read and potentially update the PLL ref clock on every modeset. I also found that the rounding mode of the porches and active area has an effect on the working modes. If the driver rounds up instead of rounding down and be calculates them in Hz instead of kHz, more modes start to work. The following table shows the modes that were working in my test without this patch set and the modes that are working now: |Mode | Before | Now | | 1920x1080-60.00 | X | X | | 1920x1080-59.94 || X | | 1920x1080-50.00 || X | | 1920x1080-30.00 || X | | 1920x1080-29.97 || X | | 1920x1080-25.00 || X | | 1920x1080-24.00 || | | 1920x1080-23.98 || | | 1680x1050-59.88 || X | | 1280x1024-75.03 | X | X | | 1280x1024-60.02 | X | X | | 1200x960-59.99 || X | | 1152x864-75.00 | X | X | | 1280x720-60.00 || | | 1280x720-59.94 || | | 1280x720-50.00 || X | | 1024x768-75.03 || X | | 1024x768-60.00 || X | | 800x600-75.00 | X | X | | 800x600-60.32 | X | X | | 720x576-50.00 | X | X | | 720x480-60.00 || | | 720x480-59.94 | X | | | 640x480-75.00 | X | X | | 640x480-60.00 || X | | 640x480-59.94 || X | | 720x400-70.08 || | Interestingly, the 720x480-59.94 mode stopped working. However, I am able to bring up the 720x480 modes by manually hacking the active area (hsa) to 40 and carefully adjusting the clocks, but something still seems to be off. Unfortunately, a few more modes are still not working at all. The NXP downstream kernel has some quirks to handle some of the modes especially wrt. to the porches, but I cannot figure out, what the driver should actually do in these cases. Maybe there is still an error in the calculation of the porches and someone at NXP can chime in. Michael Signed-off-by: Michael Tretter --- Marco Felsch (1): drm/bridge: samsung-dsim: add more mipi-dsi device debug information Michael Tretter (4): drm/bridge: samsung-dsim: reread ref clock before configuring PLL drm/bridge: samsung-dsim: update PLL reference clock drm/bridge: samsung-dsim: adjust porches by rounding up drm/bridge: samsung-dsim: calculate porches in Hz drivers/gpu/drm/bridge/samsung-dsim.c | 42 +-- include/drm/bridge/samsung-dsim.h | 1 + 2 files changed, 31 insertions(+), 12 deletions(-) --- base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c change-id: 20230818-samsung-dsim-42346444bce5 Best regards, -- Michael Tretter
Re: [PATCH 1/2] dt-bindings: display/lvds-codec: add ti,sn65lvds94
On Sun, Aug 27, 2023 at 02:45:30PM +0200, Krzysztof Kozlowski wrote: > On 27/08/2023 14:19, Conor Dooley wrote: > > On Sun, Aug 27, 2023 at 12:54:28AM +0300, Dmitry Baryshkov wrote: > >> Add compatible strings for TI sn65lvds94, LVDS serdes receiver. > >> > >> Signed-off-by: Dmitry Baryshkov > > > > Acked-by: Conor Dooley > > For the record, patch looks good, but was not tested by automation. > Missing Cc-list. Ah crap, I saw it land in the wrong place the night prior but didn't remember while going back through my queue the following morning. Sorry about that! signature.asc Description: PGP signature
Re: [PATCH v3 0/7] GPU workload hints for better performance
On Monday, August 28, 2023 09:26 -03, Arvind Yadav wrote: > AMDGPU SOCs supports dynamic workload based power profiles, which can > provide fine-tuned performance for a particular type of workload. > This patch series adds an interface to set/reset these power profiles > based on the submitted job. The driver can dynamically switch > the power profiles based on submitted job. This can optimize the power > performance when the particular workload is on. Hi Arvind, Would you mind to test your patchset with drm-ci ? There is a amdgpu test there and I would love to get your feedback of the ci. You basically just need to apply the ci patch which is available on https://cgit.freedesktop.org/drm/drm/log/?h=topic/drm-ci There are instruction on the docs, but in short: to configure it, you push your branch to gitlab.freedesktop.org, go to the settings and change the CI/CD configuration file from .gitlab-ci.yml to drivers/gpu/drm/ci/gitlab-ci.yml, and you can trigger a pipeline on your branch to get tests running. (by the time of this writing, gitlab.fdo is under maintenance but should be up soonish). Thank you! Helen > > v2: > - Splitting workload_profile_set and workload_profile_put > into two separate patches. > - Addressed review comment. > - Added new suspend function. > - Added patch to switches the GPU workload mode for KFD. > > v3: > - Addressed all review comment. > - Changed the function name from *_set() to *_get(). > - Now clearing all the profile in work handler. > - Added *_clear_all function to clear all the power profile. > > > Arvind Yadav (7): > drm/amdgpu: Added init/fini functions for workload > drm/amdgpu: Add new function to set GPU power profile > drm/amdgpu: Add new function to put GPU power profile > drm/amdgpu: Add suspend function to clear the GPU power profile. > drm/amdgpu: Set/Reset GPU workload profile > drm/amdgpu: switch workload context to/from compute > Revert "drm/amd/amdgpu: switch on/off vcn power profile mode" > > drivers/gpu/drm/amd/amdgpu/Makefile | 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu.h | 3 + > drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c| 8 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c| 6 + > drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 5 + > drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c | 14 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_workload.c | 226 ++ > drivers/gpu/drm/amd/include/amdgpu_workload.h | 61 + > 8 files changed, 309 insertions(+), 16 deletions(-) > create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_workload.c > create mode 100644 drivers/gpu/drm/amd/include/amdgpu_workload.h > > -- > 2.34.1 >
Re: [PATCH] drm/prime: Support page array >= 4GB
On 2023-08-21 16:02, Philip Yang wrote:
Without unsigned long typecast, the size is passed in as zero if page
array size >= 4GB, nr_pages >= 0x10, then sg list converted will
have the first and the last chunk lost.
Signed-off-by: Philip Yang
The patch looks reasonable to me. I don't have authority to approve it.
But FWIW,
Acked-by: Felix Kuehling
Can anyone give a Reviewed-by?
Thanks,
Felix
---
drivers/gpu/drm/drm_prime.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index f924b8b4ab6b..2630ad2e504d 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -830,7 +830,7 @@ struct sg_table *drm_prime_pages_to_sg(struct drm_device
*dev,
if (max_segment == 0)
max_segment = UINT_MAX;
err = sg_alloc_table_from_pages_segment(sg, pages, nr_pages, 0,
- nr_pages << PAGE_SHIFT,
+ (unsigned long)nr_pages <<
PAGE_SHIFT,
max_segment, GFP_KERNEL);
if (err) {
kfree(sg);
Re: [PATCH] drm: bridge: it66121: Fix invalid connector dereference
On Friday, August 25, 2023 08:02 -03, Jai Luthra wrote:
> Fix the NULL pointer dereference when no monitor is connected, and the
> sound card is opened from userspace.
>
> Instead return an error as EDID information cannot be provided to
> the sound framework if there is no connector attached.
>
> Fixes: e0fd83dbe924 ("drm: bridge: it66121: Add audio support")
> Reported-by: Nishanth Menon
> Closes: https://lore.kernel.org/all/20230825105849.crhon42qndxqif4i@gondola/
> Signed-off-by: Jai Luthra
Reviewed-by: Helen Koike
> ---
> drivers/gpu/drm/bridge/ite-it66121.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c
> b/drivers/gpu/drm/bridge/ite-it66121.c
> index 466641c77fe9..d6fa00dea464 100644
> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> @@ -1446,6 +1446,11 @@ static int it66121_audio_get_eld(struct device *dev,
> void *data,
> {
> struct it66121_ctx *ctx = dev_get_drvdata(dev);
>
> + if (!ctx->connector) {
> + dev_dbg(dev, "No connector present, cannot provide EDID data");
> + return -EINVAL;
> + }
> +
> mutex_lock(>lock);
>
> memcpy(buf, ctx->connector->eld,
>
> ---
> base-commit: 6269320850097903b30be8f07a5c61d9f7592393
> change-id: 20230825-it66121_edid-6ee98517808b
>
> Best regards,
> --
> Jai Luthra
>
Re: [PATCH] spi: tegra: Fix missing IRQ check in tegra_slink_probe()
On Saturday, August 26, 2023 07:02 -03, Zhang Shurong
wrote:
> This func misses checking for platform_get_irq()'s call and may passes the
> negative error codes to request_irq(), which takes unsigned IRQ #,
> causing it to fail with -EINVAL, overriding an original error code.
>
> Fix this by stop calling request_irq() with invalid IRQ #s.
>
> Fixes: dc4dc3605639 ("spi: tegra: add spi driver for SLINK controller")
> Signed-off-by: Zhang Shurong
Reviewed-by: Helen Koike
> ---
> drivers/spi/spi-tegra20-slink.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/spi/spi-tegra20-slink.c b/drivers/spi/spi-tegra20-slink.c
> index 4d6db6182c5e..f5cd365c913a 100644
> --- a/drivers/spi/spi-tegra20-slink.c
> +++ b/drivers/spi/spi-tegra20-slink.c
> @@ -1086,6 +1086,8 @@ static int tegra_slink_probe(struct platform_device
> *pdev)
> reset_control_deassert(tspi->rst);
>
> spi_irq = platform_get_irq(pdev, 0);
> + if (spi_irq < 0)
> + return spi_irq;
> tspi->irq = spi_irq;
> ret = request_threaded_irq(tspi->irq, tegra_slink_isr,
> tegra_slink_isr_thread, IRQF_ONESHOT,
> --
> 2.30.2
>
Re: [PATCH v15 00/23] Add generic memory shrinker to VirtIO-GPU and Panfrost DRM drivers
On Monday, August 28, 2023 11:37 -03, "Helen Mae Koike Fornazier" wrote: > On Sunday, August 27, 2023 14:54 -03, Dmitry Osipenko > wrote: > > > This series: > > > > 1. Adds common drm-shmem memory shrinker > > 2. Enables shrinker for VirtIO-GPU driver > > 3. Switches Panfrost driver to the common shrinker > > Hi Dmitry, > > Would you mind testing with drm-ci? We virt-io tests there and it would be > really great to get your feedback of it. > > https://cgit.freedesktop.org/drm/drm/log/?h=topic/drm-ci sorry, I forgot that you also need this patchset: https://lists.freedesktop.org/archives/dri-devel/2023-August/420063.html to enable virtio_gpu test job. Thanks again. Helen > > You need to merge your changes with the above tree. > To configure it, you just need to have a tree on gitlab.freedesktop.org, > go to the settings and change the CI/CD configuration file from .gitlab-ci.yml > to drivers/gpu/drm/ci/gitlab-ci.yml, and you can start a pipeline > on your branch. > > at the time of this writting, gitlab.freedesktop.org is under maintenance, > but it should be back soon. > > Thank you! > Helen > > > > > Changelog: > > > > v15:- Moved drm-shmem reference counters to use kref that allows to > > optimize unlocked functions, like was suggested by Boris Brezillon. > > > > - Changed drm/gem/shmem function names to use _locked postfix and > > dropped the _unlocked, making the naming scheme consistent across > > DRM code, like was suggested by Boris Brezillon. > > > > - Added patch that fixes UAF in drm-shmem for drivers that import > > dma-buf and then release buffer in the import error code path. > > > > - Added patch that makes drm-shmem use new flag for SGT's get_pages() > > refcounting, preventing unbalanced refcounting when GEM is freed. > > > > - Fixed guest blob pinning in virtio-gpu driver that was missed > > previously in the shrinker patch. > > > > - Moved VC4 and virtio-gpu drivers to use drm_gem_put() in > > GEM-creation error code paths, which is now required by drm-shmem > > and was missed in a previous patch versions. > > > > - Virtio-GPU now attaches shmem pages to host on first use and not > > when BO is created. In older patch versions there was a potential > > race condition in the BO creation code path where both > > get_sgt()+object_attach() should've been made under same resv lock, > > otherwise pages could be evicted before attachment is invoked. > > > > - Virtio-GPU and drm-shmem shrinker patches are split into smaller > > ones. > > > > v14:- All the prerequisite reservation locking patches landed upstream, > > previously were a part of this series in v13 and older. > > > > > > https://lore.kernel.org/dri-devel/20230529223935.2672495-1-dmitry.osipe...@collabora.com/ > > > > - Added patches to improve locked/unlocked function names, like was > > suggested by Boris Brezillon for v13. > > > > - Made all exported drm-shmem symbols GPL, like was previously > > discussed with Thomas Zimmermann on this series. > > > > - Improved virtio-gpu shrinker patch. Now it won't detach purged BO > > when userspace closes GEM. Crosvm (and not qemu) checks res_id on > > CMD_CTX_DETACH_RESOURCE and prints noisy error message if ID is > > invalid, which wasn't noticed before. > > > > v13:- Updated virtio-gpu shrinker patch to use drm_gem_shmem_object_pin() > > directly instead of drm_gem_pin() and dropped patch that exported > > drm_gem_pin() functions, like was requested by Thomas Zimmermann in > > v12. > > > > v12:- Fixed the "no previous prototype for function" warning reported by > > kernel build bot for v11. > > > > - Fixed the missing reservation lock reported by Intel CI for VGEM > > driver. Other drivers using drm-shmem were affected similarly to > > VGEM. The problem was in the dma-buf attachment code path that led > > to drm-shmem pinning function which assumed the held reservation lock > > by drm_gem_pin(). In the past that code path was causing trouble for > > i915 driver and we've changed the locking scheme for the attachment > > code path in the dma-buf core to let exporters to handle the locking > > themselves. After a closer investigation, I realized that my > > assumption > > about testing of dma-buf export code path using Panfrost driver was > > incorrect. Now I created additional local test to exrecise the > > Panfrost > > export path. I also reproduced the issue reported by the Intel CI for > > v10. It's all fixed now by making the drm_gem_shmem_pin() to take the > > resv lock by itself. > > > > - Patches are based on top of drm-tip, CC'd intel-gfx CI for testing. > > > > v11:- Rebased on a recent linux-next. Added new patch as a result: > > > > drm/shmem-helper: Export
Re: [PATCH v4 1/3] dt-bindings: display: panel: add common dual-link schema
On Fri, 25 Aug 2023 14:11:40 +0200, Krzysztof Kozlowski wrote: > Add schema with common properties shared among dual-link panel ICs. > > Signed-off-by: Krzysztof Kozlowski > > --- > > Changes since v3: > 1. Re-phrase description of binding and ports (Laurent) > v3: > https://lore.kernel.org/all/20230823081500.84005-1-krzysztof.kozlow...@linaro.org/ > > Changes since v2: > 1. New Patch > v2: > https://lore.kernel.org/all/20230502120036.47165-1-krzysztof.kozlow...@linaro.org/ > v1: > https://lore.kernel.org/all/20230416153929.356330-1-krzysztof.kozlow...@linaro.org/ > --- > .../display/panel/panel-common-dual.yaml | 47 +++ > 1 file changed, 47 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/display/panel/panel-common-dual.yaml > Reviewed-by: Rob Herring
Re: [PATCH v3 0/7] GPU workload hints for better performance
[AMD Official Use Only - General] As mentioned with an older version of this series, this is an 'abuse' of power profile interface. This series is oversimplifying what PMFW algorithms are supposed to be doing. Whatever this series is doing, FW can do it better. To explain in simpler terms - it just tries to boost a profile based on ring type without even knowing how much of activity a job can trigger on a particular ring. A job scheduled to a GFX ring doesn't deserve a profile boost unless it can create a certain level of activity. In CPU terms, a job scheduled to a processor doesn't mean it deserves a frequency boost of that CPU. At minimum it depends on more details like whether that job is compute bound or memory bound or memory bound. While FW algorithms are designed to do that, this series tries to trivialise all such things. Unless you are able to show the tangible benefits in some terms like performance, power, or performance per watt, I don't think this should be the default behaviour where driver tries to override FW just based on job submissions to rings. Thanks, Lijo From: amd-gfx on behalf of Arvind Yadav Sent: Monday, August 28, 2023 5:56:07 PM To: Koenig, Christian ; Deucher, Alexander ; Sharma, Shashank ; Pan, Xinhui ; airl...@gmail.com ; dan...@ffwll.ch ; Kuehling, Felix ; amd-...@lists.freedesktop.org Cc: Yadav, Arvind ; linux-ker...@vger.kernel.org ; dri-devel@lists.freedesktop.org Subject: [PATCH v3 0/7] GPU workload hints for better performance AMDGPU SOCs supports dynamic workload based power profiles, which can provide fine-tuned performance for a particular type of workload. This patch series adds an interface to set/reset these power profiles based on the submitted job. The driver can dynamically switch the power profiles based on submitted job. This can optimize the power performance when the particular workload is on. v2: - Splitting workload_profile_set and workload_profile_put into two separate patches. - Addressed review comment. - Added new suspend function. - Added patch to switches the GPU workload mode for KFD. v3: - Addressed all review comment. - Changed the function name from *_set() to *_get(). - Now clearing all the profile in work handler. - Added *_clear_all function to clear all the power profile. Arvind Yadav (7): drm/amdgpu: Added init/fini functions for workload drm/amdgpu: Add new function to set GPU power profile drm/amdgpu: Add new function to put GPU power profile drm/amdgpu: Add suspend function to clear the GPU power profile. drm/amdgpu: Set/Reset GPU workload profile drm/amdgpu: switch workload context to/from compute Revert "drm/amd/amdgpu: switch on/off vcn power profile mode" drivers/gpu/drm/amd/amdgpu/Makefile | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu.h | 3 + drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c| 8 +- drivers/gpu/drm/amd/amdgpu/amdgpu_device.c| 6 + drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 5 + drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c | 14 +- drivers/gpu/drm/amd/amdgpu/amdgpu_workload.c | 226 ++ drivers/gpu/drm/amd/include/amdgpu_workload.h | 61 + 8 files changed, 309 insertions(+), 16 deletions(-) create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_workload.c create mode 100644 drivers/gpu/drm/amd/include/amdgpu_workload.h -- 2.34.1
Re: [PATCH v2 6/6] drm/drm-file: Allow size unit selection in drm_show_memory_stats
On Wed, Aug 23, 2023 at 6:36 PM Adrián Larumbe
wrote:
>
> The current implementation will try to pick the highest available
> unit. This is rather unflexible, and allowing drivers to display BO size
> statistics through fdinfo in units of their choice might be desirable.
>
> The new argument to drm_show_memory_stats is to be interpreted as the
> integer multiplier of a 10-power of 2, so 1 would give us size in Kib and 2
> in Mib. If we want drm-file functions to pick the highest unit, then 0
> should be passed.
>
> Signed-off-by: Adrián Larumbe
> ---
> drivers/gpu/drm/drm_file.c | 22 +-
> drivers/gpu/drm/msm/msm_drv.c | 2 +-
> drivers/gpu/drm/panfrost/panfrost_drv.c | 2 +-
> include/drm/drm_file.h | 5 +++--
> 4 files changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index 762965e3d503..517e1fb8072a 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -873,7 +873,7 @@ void drm_send_event(struct drm_device *dev, struct
> drm_pending_event *e)
> EXPORT_SYMBOL(drm_send_event);
>
> static void print_size(struct drm_printer *p, const char *stat,
> - const char *region, u64 sz)
> + const char *region, u64 sz, unsigned int unit)
> {
> const char *units[] = {"", " KiB", " MiB"};
> unsigned u;
> @@ -881,6 +881,8 @@ static void print_size(struct drm_printer *p, const char
> *stat,
> for (u = 0; u < ARRAY_SIZE(units) - 1; u++) {
> if (sz < SZ_1K)
> break;
> + if (unit > 0 && unit == u)
> + break;
> sz = div_u64(sz, SZ_1K);
> }
>
> @@ -898,17 +900,18 @@ static void print_size(struct drm_printer *p, const
> char *stat,
> void drm_print_memory_stats(struct drm_printer *p,
> const struct drm_memory_stats *stats,
> enum drm_gem_object_status supported_status,
> - const char *region)
> + const char *region,
> + unsigned int unit)
I'm not really adverse to changing what units we use.. or perhaps
changing the threshold to go to higher units to be 1x or 10x
of the previous unit. But I'm less excited about having different
drivers using different units.
BR,
-R
> {
> - print_size(p, "total", region, stats->private + stats->shared);
> - print_size(p, "shared", region, stats->shared);
> - print_size(p, "active", region, stats->active);
> + print_size(p, "total", region, stats->private + stats->shared, unit);
> + print_size(p, "shared", region, stats->shared, unit);
> + print_size(p, "active", region, stats->active, unit);
>
> if (supported_status & DRM_GEM_OBJECT_RESIDENT)
> - print_size(p, "resident", region, stats->resident);
> + print_size(p, "resident", region, stats->resident, unit);
>
> if (supported_status & DRM_GEM_OBJECT_PURGEABLE)
> - print_size(p, "purgeable", region, stats->purgeable);
> + print_size(p, "purgeable", region, stats->purgeable, unit);
> }
> EXPORT_SYMBOL(drm_print_memory_stats);
>
> @@ -916,11 +919,12 @@ EXPORT_SYMBOL(drm_print_memory_stats);
> * drm_show_memory_stats - Helper to collect and show standard fdinfo memory
> stats
> * @p: the printer to print output to
> * @file: the DRM file
> + * @unit: multipliyer of power of two exponent of desired unit
> *
> * Helper to iterate over GEM objects with a handle allocated in the
> specified
> * file.
> */
> -void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file)
> +void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file,
> unsigned int unit)
> {
> struct drm_gem_object *obj;
> struct drm_memory_stats status = {};
> @@ -967,7 +971,7 @@ void drm_show_memory_stats(struct drm_printer *p, struct
> drm_file *file)
> }
> spin_unlock(>table_lock);
>
> - drm_print_memory_stats(p, , supported_status, "memory");
> + drm_print_memory_stats(p, , supported_status, "memory", unit);
> }
> EXPORT_SYMBOL(drm_show_memory_stats);
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 2a0e3529598b..cd1198151744 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -1067,7 +1067,7 @@ static void msm_show_fdinfo(struct drm_printer *p,
> struct drm_file *file)
>
> msm_gpu_show_fdinfo(priv->gpu, file->driver_priv, p);
>
> - drm_show_memory_stats(p, file);
> + drm_show_memory_stats(p, file, 0);
> }
>
> static const struct file_operations fops = {
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c
> b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 93d5f5538c0b..79c08cee3e9d 100644
> ---
Re: [PATCH 8/8] staging/fbtft: Use fb_ops helpers for deferred I/O
On Mon, Aug 28, 2023 at 03:14:24PM +0200, Thomas Zimmermann wrote: > Generate callback functions for struct fb_ops with the fbdev macro > FB_GEN_DEFAULT_DEFERRED_SYSMEM_OPS(). Initialize struct fb_ops to > the generated functions with an fbdev initializer macro. > > Signed-off-by: Thomas Zimmermann Acked-by: Greg Kroah-Hartman
Re: [PATCH 7/8] staging/fbtft: Initialize fb_op struct as static const
On Mon, Aug 28, 2023 at 03:14:23PM +0200, Thomas Zimmermann wrote: > Replace dynamic allocation of the fb_ops instance with static > allocation. Initialize the fields at module-load time. The owner > field changes to THIS_MODULE, as in all other fbdev drivers. > > Signed-off-by: Thomas Zimmermann > --- > drivers/staging/fbtft/fbtft-core.c | 30 +- > 1 file changed, 13 insertions(+), 17 deletions(-) > Acked-by: Greg Kroah-Hartman
Re: [PATCH v2] fs: clean up usage of noop_dirty_folio
On Mon, Aug 28, 2023 at 03:54:49PM +0800, Xueshi Hu wrote: > In folio_mark_dirty(), it can automatically fallback to > noop_dirty_folio() if a_ops->dirty_folio is not registered. > > As anon_aops, dev_dax_aops and fb_deferred_io_aops becames empty, remove > them too. I'd put the last sentence as 'In dev_dax_aops and fb_deferred_io_aops replacing .dirty_folio with NULL makes them identical to default (empty_aops) and since we never compare ->a_ops pointer with either of those, we can remove them completely'. There could've been places like #define is_fb_deferred(mapping) (mapping)->a_ops == fb_deferred_io_aops and those would've been broken by that. The fact that there's nothing of that sort in the tree ought to be mentioned in commit message. Note that we *do* have places where method table comparisons are used in predicates like that, so it's not a pure theory; sure, missing that would've probably ended up with broken build, but that can easily be dependent upon the config (and that, alas, is also not a pure theory - BTDT). In this case the change is correct, fortunately... Other than that part of commit message - Acked-by: Al Viro
Re: [PATCH v15 00/23] Add generic memory shrinker to VirtIO-GPU and Panfrost DRM drivers
On Sunday, August 27, 2023 14:54 -03, Dmitry Osipenko wrote: > This series: > > 1. Adds common drm-shmem memory shrinker > 2. Enables shrinker for VirtIO-GPU driver > 3. Switches Panfrost driver to the common shrinker Hi Dmitry, Would you mind testing with drm-ci? We virt-io tests there and it would be really great to get your feedback of it. https://cgit.freedesktop.org/drm/drm/log/?h=topic/drm-ci You need to merge your changes with the above tree. To configure it, you just need to have a tree on gitlab.freedesktop.org, go to the settings and change the CI/CD configuration file from .gitlab-ci.yml to drivers/gpu/drm/ci/gitlab-ci.yml, and you can start a pipeline on your branch. at the time of this writting, gitlab.freedesktop.org is under maintenance, but it should be back soon. Thank you! Helen > > Changelog: > > v15:- Moved drm-shmem reference counters to use kref that allows to > optimize unlocked functions, like was suggested by Boris Brezillon. > > - Changed drm/gem/shmem function names to use _locked postfix and > dropped the _unlocked, making the naming scheme consistent across > DRM code, like was suggested by Boris Brezillon. > > - Added patch that fixes UAF in drm-shmem for drivers that import > dma-buf and then release buffer in the import error code path. > > - Added patch that makes drm-shmem use new flag for SGT's get_pages() > refcounting, preventing unbalanced refcounting when GEM is freed. > > - Fixed guest blob pinning in virtio-gpu driver that was missed > previously in the shrinker patch. > > - Moved VC4 and virtio-gpu drivers to use drm_gem_put() in > GEM-creation error code paths, which is now required by drm-shmem > and was missed in a previous patch versions. > > - Virtio-GPU now attaches shmem pages to host on first use and not > when BO is created. In older patch versions there was a potential > race condition in the BO creation code path where both > get_sgt()+object_attach() should've been made under same resv lock, > otherwise pages could be evicted before attachment is invoked. > > - Virtio-GPU and drm-shmem shrinker patches are split into smaller > ones. > > v14:- All the prerequisite reservation locking patches landed upstream, > previously were a part of this series in v13 and older. > > > https://lore.kernel.org/dri-devel/20230529223935.2672495-1-dmitry.osipe...@collabora.com/ > > - Added patches to improve locked/unlocked function names, like was > suggested by Boris Brezillon for v13. > > - Made all exported drm-shmem symbols GPL, like was previously > discussed with Thomas Zimmermann on this series. > > - Improved virtio-gpu shrinker patch. Now it won't detach purged BO > when userspace closes GEM. Crosvm (and not qemu) checks res_id on > CMD_CTX_DETACH_RESOURCE and prints noisy error message if ID is > invalid, which wasn't noticed before. > > v13:- Updated virtio-gpu shrinker patch to use drm_gem_shmem_object_pin() > directly instead of drm_gem_pin() and dropped patch that exported > drm_gem_pin() functions, like was requested by Thomas Zimmermann in > v12. > > v12:- Fixed the "no previous prototype for function" warning reported by > kernel build bot for v11. > > - Fixed the missing reservation lock reported by Intel CI for VGEM > driver. Other drivers using drm-shmem were affected similarly to > VGEM. The problem was in the dma-buf attachment code path that led > to drm-shmem pinning function which assumed the held reservation lock > by drm_gem_pin(). In the past that code path was causing trouble for > i915 driver and we've changed the locking scheme for the attachment > code path in the dma-buf core to let exporters to handle the locking > themselves. After a closer investigation, I realized that my assumption > about testing of dma-buf export code path using Panfrost driver was > incorrect. Now I created additional local test to exrecise the Panfrost > export path. I also reproduced the issue reported by the Intel CI for > v10. It's all fixed now by making the drm_gem_shmem_pin() to take the > resv lock by itself. > > - Patches are based on top of drm-tip, CC'd intel-gfx CI for testing. > > v11:- Rebased on a recent linux-next. Added new patch as a result: > > drm/shmem-helper: Export drm_gem_shmem_get_pages_sgt_locked() > > It's needed by the virtio-gpu driver to swap-in/unevict shmem > object, previously get_pages_sgt() didn't use locking. > > - Separated the "Add memory shrinker" patch into smaller parts to ease > the reviewing, as was requested by Thomas Zimmermann: > > drm/shmem-helper: Factor out pages alloc/release from > drm_gem_shmem_get/put_pages() > drm/shmem-helper: Add pages_pin_count
Re: [PATCH 0/3] Make Allwinner A64's pll-mipi keep its rate when parent rate changes
On 2023-08-28 at 10:04:51 +0200, Maxime Ripard wrote: > On Fri, Aug 25, 2023 at 05:07:58PM +0200, Frank Oltmanns wrote: >> Thank you for your feedback, Maxime! >> >> On 2023-08-25 at 10:13:53 +0200, Maxime Ripard wrote: >> > [[PGP Signed Part:Undecided]] >> > Hi, >> > >> > On Fri, Aug 25, 2023 at 07:36:36AM +0200, Frank Oltmanns wrote: >> >> I would like to make the Allwinner A64's pll-mipi to keep its rate when >> >> its parent's (pll-video0) rate changes. Keeping pll-mipi's rate is >> >> required, to let the A64 drive both an LCD and HDMI display at the same >> >> time, because both have pll-video0 as an ancestor. >> >> >> >> PATCH 1 adds this functionality as a feature into the clk framework (new >> >> flag: CLK_KEEP_RATE). >> >> >> >> Cores that use this flag, store a rate as req_rate when it or one of its >> >> descendants requests a new rate. >> >> >> >> That rate is then restored in the clk_change_rate recursion, which walks >> >> through the tree. It will reach the flagged core (e.g. pll-mipi) after >> >> the parent's rate (e.g. pll-video0) has already been set to the new >> >> rate. It will then call determine_rate (which requests the parent's >> >> current, i.e. new, rate) to determine a rate that is close to the >> >> flagged core's previous rate. Afterward it will re-calculate the rates >> >> for the flagged core's subtree. >> > >> > I don't think it's the right way forward. It makes the core logic more >> > complicated, for something that is redundant with the notifiers >> > mechanism that has been the go-to for that kind of things so far. >> >> Yeah, that was my initial idea as well. But I couldn't get it to work. >> See details below. >> >> Do you have an example of a clock that restores its previous rate after >> the parent rate has changed? I've looked left and right, but to me it >> seems that notifiers are mainly used for setting clocks into some kind >> of "safe mode" prior to the rate change. Examples: >> >> sunxi-ng: >> https://elixir.bootlin.com/linux/v6.4.11/source/drivers/clk/sunxi-ng/ccu_mux.c#L273 >> https://elixir.bootlin.com/linux/v6.4.11/source/drivers/clk/sunxi-ng/ccu_common.c#L60 >> >> but also others: >> https://elixir.bootlin.com/linux/v6.4.11/source/drivers/clk/at91/clk-master.c#L248 >> https://elixir.bootlin.com/linux/v6.4.11/source/drivers/clk/meson/meson8b.c#L3755 >> https://elixir.bootlin.com/linux/v6.4.11/source/drivers/clk/qcom/clk-cpu-8996.c#L546 > > There's examples for phases and parents, but not for rates afaics. We > shouldn't behave any differently though. > >> > It's not really obvious to me why the notifiers don't work there. >> > >> >> This work is inspired by an out-of-tree patchset [1] [2] [3]. >> >> Unfortunately, the patchset uses clk_set_rate() in a notifier callback, >> >> which the following comment on clk_notifier_register() forbids: "The >> >> callbacks associated with the notifier must not re-enter into the clk >> >> framework by calling any top-level clk APIs." [4] Furthermore, that >> >> out-of-tree patchset no longer works with the current linux-next, >> >> because setting pll-mipi is now also resetting pll-video0 [5]. >> > >> > Is it because of the "The callbacks associated with the notifier must >> > not re-enter into the clk framework by calling any top-level clk APIs." >> > comment? >> >> I don't think that's the reason. > > I'm not sure I follow you there. How can we find a solution to a problem > you don't know about or can't know for sure? I was hoping that the discussion here would give me some clues (and it does). You have already explained, that the issue is the locks. I'm still confused why Icenowy's patches work. They use clk_set_rate() in a notifier callback and despite that they work (up until kernel 6.5). The only thing that has changed here (that I'm aware of), is that pll-mipi now sets the parent rate in clk-next. >> I'm fairly certain that the problem is, that pll-mipi tries to set the >> parent rate. Maybe it should check if the parent is locked, before >> determining a rate that requires the parent rate to change. 樂 > > Why would the clock framework documentation mention an issue that only > arises with a single clock on a single SoC? No, sorry, that's not what I said or meant. I was wondering if ccu_nkm_determine_rate should check if the parent rate has no exclusive lock, before assuming it can change the parent rate, so that Icenowy's patches still work. > That comment in the clock framework you linked to clearly stated that > you can't use a top-level clock function in a notifier, and that's > because of the locking. Yes, it does. And that's why I thought that calling clk_set_rate() in the notifier callback was never the right choice. > If it's not what you're trying to fix, then I'd really like to know what > issue you're trying to fix *in the framework* (so, not on the pll-mipi > clock, or the A64). I'm not trying to "fix" anything in the framework in the sense that the framework has a bug. I propose to add
