date:20221107

This series provides an alternative container layer for VFIO implemented
using iommufd. This is optional, if CONFIG_IOMMUFD is not set then it will
not be compiled in.

At this point iommufd can be injected by passing in a iommfd FD to
VFIO_GROUP_SET_CONTAINER which will use the VFIO compat layer in iommufd
to obtain the compat IOAS and then connect up all the VFIO drivers as
appropriate.

This is temporary stopping point, a following series will provide a way to
directly open a VFIO device FD and directly connect it to IOMMUFD using
native ioctls that can expose the IOMMUFD features like hwpt, future
vPASID and dynamic attachment.

This series, in compat mode, has passed all the qemu tests we have
available, including the test suites for the Intel GVT mdev. Aside from
the temporary limitation with P2P memory this is belived to be fully
compatible with VFIO.

This is on github: https://github.com/jgunthorpe/linux/commits/vfio_iommufd

It requires the iommufd series:

https://lore.kernel.org/r/0-v4-0de2f6c78ed0+9d1-iommufd_...@nvidia.com

v2:
 - Rebase to v6.1-rc3, v4 iommufd series
 - Fixup comments and commit messages from list remarks
 - Fix leaking of the iommufd for mdevs
 - New patch to fix vfio modaliases when vfio container is disabled
 - Add a dmesg once when the iommufd provided /dev/vfio/vfio is opened
   to signal that iommufd is providing this
v1: https://lore.kernel.org/r/0-v1-4991695894d8+211-vfio_iommufd_...@nvidia.com

Jason Gunthorpe (11):
  vfio: Move vfio_device driver open/close code to a function
  vfio: Move vfio_device_assign_container() into
vfio_device_first_open()
  vfio: Rename vfio_device_assign/unassign_container()
  vfio: Move storage of allow_unsafe_interrupts to vfio_main.c
  vfio: Use IOMMU_CAP_ENFORCE_CACHE_COHERENCY for
vfio_file_enforced_coherent()
  vfio-iommufd: Allow iommufd to be used in place of a container fd
  vfio-iommufd: Support iommufd for physical VFIO devices
  vfio-iommufd: Support iommufd for emulated VFIO devices
  vfio: Move container related MODULE_ALIAS statements into container.c
  vfio: Make vfio_container optionally compiled
  iommufd: Allow iommufd to supply /dev/vfio/vfio

 drivers/gpu/drm/i915/gvt/kvmgt.c  |   3 +
 drivers/iommu/iommufd/Kconfig |  12 +
 drivers/iommu/iommufd/main.c  |  36 ++
 drivers/s390/cio/vfio_ccw_ops.c   |   3 +
 drivers/s390/crypto/vfio_ap_ops.c |   3 +
 drivers/vfio/Kconfig  |  36 +-
 drivers/vfio/Makefile |   5 +-
 drivers/vfio/container.c  | 141 ++--
 drivers/vfio/fsl-mc/vfio_fsl_mc.c |   3 +
 drivers/vfio/iommufd.c| 157 
 .../vfio/pci/hisilicon/hisi_acc_vfio_pci.c|   6 +
 drivers/vfio/pci/mlx5/main.c  |   3 +
 drivers/vfio/pci/vfio_pci.c   |   3 +
 drivers/vfio/platform/vfio_amba.c |   3 +
 drivers/vfio/platform/vfio_platform.c |   3 +
 drivers/vfio/vfio.h   | 100 +-
 drivers/vfio/vfio_iommu_type1.c   |   5 +-
 drivers/vfio/vfio_main.c  | 338 ++
 include/linux/vfio.h  |  39 ++
 19 files changed, 700 insertions(+), 199 deletions(-)
 create mode 100644 drivers/vfio/iommufd.c


base-commit: ca3067007d4f2aa7f3a5375bd256839e08a09453
-- 
2.38.1

[PATCH v2 01/11] vfio: Move vfio_device driver open/close code to a function

This error unwind is getting complicated. Move all the code into two
pair'd function. The functions should be called when the open_count == 1
after incrementing/before decrementing.

Reviewed-by: Kevin Tian 
Reviewed-by: Yi Liu 
Tested-by: Nicolin Chen 
Signed-off-by: Jason Gunthorpe 
---
 drivers/vfio/vfio_main.c | 95 ++--
 1 file changed, 53 insertions(+), 42 deletions(-)

diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index 2d168793d4e1ce..2e8346d13c16ca 100644
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -734,6 +734,51 @@ bool vfio_assert_device_open(struct vfio_device *device)
return !WARN_ON_ONCE(!READ_ONCE(device->open_count));
 }
 
+static int vfio_device_first_open(struct vfio_device *device)
+{
+   int ret;
+
+   lockdep_assert_held(&device->dev_set->lock);
+
+   if (!try_module_get(device->dev->driver->owner))
+   return -ENODEV;
+
+   /*
+* Here we pass the KVM pointer with the group under the lock.  If the
+* device driver will use it, it must obtain a reference and release it
+* during close_device.
+*/
+   mutex_lock(&device->group->group_lock);
+   device->kvm = device->group->kvm;
+   if (device->ops->open_device) {
+   ret = device->ops->open_device(device);
+   if (ret)
+   goto err_module_put;
+   }
+   vfio_device_container_register(device);
+   mutex_unlock(&device->group->group_lock);
+   return 0;
+
+err_module_put:
+   device->kvm = NULL;
+   mutex_unlock(&device->group->group_lock);
+   module_put(device->dev->driver->owner);
+   return ret;
+}
+
+static void vfio_device_last_close(struct vfio_device *device)
+{
+   lockdep_assert_held(&device->dev_set->lock);
+
+   mutex_lock(&device->group->group_lock);
+   vfio_device_container_unregister(device);
+   if (device->ops->close_device)
+   device->ops->close_device(device);
+   device->kvm = NULL;
+   mutex_unlock(&device->group->group_lock);
+   module_put(device->dev->driver->owner);
+}
+
 static struct file *vfio_device_open(struct vfio_device *device)
 {
struct file *filep;
@@ -745,29 +790,12 @@ static struct file *vfio_device_open(struct vfio_device 
*device)
if (ret)
return ERR_PTR(ret);
 
-   if (!try_module_get(device->dev->driver->owner)) {
-   ret = -ENODEV;
-   goto err_unassign_container;
-   }
-
mutex_lock(&device->dev_set->lock);
device->open_count++;
if (device->open_count == 1) {
-   /*
-* Here we pass the KVM pointer with the group under the read
-* lock.  If the device driver will use it, it must obtain a
-* reference and release it during close_device.
-*/
-   mutex_lock(&device->group->group_lock);
-   device->kvm = device->group->kvm;
-
-   if (device->ops->open_device) {
-   ret = device->ops->open_device(device);
-   if (ret)
-   goto err_undo_count;
-   }
-   vfio_device_container_register(device);
-   mutex_unlock(&device->group->group_lock);
+   ret = vfio_device_first_open(device);
+   if (ret)
+   goto err_unassign_container;
}
mutex_unlock(&device->dev_set->lock);
 
@@ -800,20 +828,11 @@ static struct file *vfio_device_open(struct vfio_device 
*device)
 
 err_close_device:
mutex_lock(&device->dev_set->lock);
-   mutex_lock(&device->group->group_lock);
-   if (device->open_count == 1 && device->ops->close_device) {
-   device->ops->close_device(device);
-
-   vfio_device_container_unregister(device);
-   }
-err_undo_count:
-   mutex_unlock(&device->group->group_lock);
+   if (device->open_count == 1)
+   vfio_device_last_close(device);
+err_unassign_container:
device->open_count--;
-   if (device->open_count == 0 && device->kvm)
-   device->kvm = NULL;
mutex_unlock(&device->dev_set->lock);
-   module_put(device->dev->driver->owner);
-err_unassign_container:
vfio_device_unassign_container(device);
return ERR_PTR(ret);
 }
@@ -1016,19 +1035,11 @@ static int vfio_device_fops_release(struct inode 
*inode, struct file *filep)
 
mutex_lock(&device->dev_set->lock);
vfio_assert_device_open(device);
-   mutex_lock(&device->group->group_lock);
-   if (device->open_count == 1 && device->ops->close_device)
-   device->ops->close_device(device);
-
-   vfio_device_container_unregister(device);
-   mutex_unlock(&device->group->group_lock);
+   if (device->open_count == 1)
+   vfio_device_last_close(device);
device->op

[PATCH v2 05/11] vfio: Use IOMMU_CAP_ENFORCE_CACHE_COHERENCY for vfio_file_enforced_coherent()

iommufd doesn't establish the iommu_domains until after the device FD is
opened, even if the container has been set. This design is part of moving
away from the group centric iommu APIs.

This is fine, except that the normal sequence of establishing the kvm
wbinvd won't work:

   group = open("/dev/vfio/XX")
   ioctl(group, VFIO_GROUP_SET_CONTAINER)
   ioctl(kvm, KVM_DEV_VFIO_GROUP_ADD)
   ioctl(group, VFIO_GROUP_GET_DEVICE_FD)

As the domains don't start existing until GET_DEVICE_FD. Further,
GET_DEVICE_FD requires that KVM_DEV_VFIO_GROUP_ADD already be done as that
is what sets the group->kvm and thus device->kvm for the driver to use
during open.

Now that we have device centric cap ops and the new
IOMMU_CAP_ENFORCE_CACHE_COHERENCY we know what the iommu_domain will be
capable of without having to create it. Use this to compute
vfio_file_enforced_coherent() and resolve the ordering problems.

VFIO always tries to upgrade domains to enforce cache coherency, it never
attaches a device that supports enforce cache coherency to a less capable
domain, so the cap test is a sufficient proxy for the ultimate
outcome. iommufd also ensures that devices that set the cap will be
connected to enforcing domains.

Tested-by: Nicolin Chen 
Signed-off-by: Jason Gunthorpe 
---
 drivers/vfio/container.c |  5 +++--
 drivers/vfio/vfio.h  |  2 --
 drivers/vfio/vfio_main.c | 29 -
 3 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/drivers/vfio/container.c b/drivers/vfio/container.c
index 499777930b08fa..d97747dfb05d02 100644
--- a/drivers/vfio/container.c
+++ b/drivers/vfio/container.c
@@ -188,8 +188,9 @@ void vfio_device_container_unregister(struct vfio_device 
*device)
device->group->container->iommu_data, device);
 }
 
-long vfio_container_ioctl_check_extension(struct vfio_container *container,
- unsigned long arg)
+static long
+vfio_container_ioctl_check_extension(struct vfio_container *container,
+unsigned long arg)
 {
struct vfio_iommu_driver *driver;
long ret = 0;
diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
index 54e5a8e0834ccb..247590334e14b0 100644
--- a/drivers/vfio/vfio.h
+++ b/drivers/vfio/vfio.h
@@ -119,8 +119,6 @@ int vfio_container_attach_group(struct vfio_container 
*container,
 void vfio_group_detach_container(struct vfio_group *group);
 void vfio_device_container_register(struct vfio_device *device);
 void vfio_device_container_unregister(struct vfio_device *device);
-long vfio_container_ioctl_check_extension(struct vfio_container *container,
- unsigned long arg);
 int __init vfio_container_init(void);
 void vfio_container_cleanup(void);
 
diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index e1fec1db6a3c93..5c0e810f8b4d08 100644
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -1625,24 +1625,27 @@ EXPORT_SYMBOL_GPL(vfio_file_is_group);
 bool vfio_file_enforced_coherent(struct file *file)
 {
struct vfio_group *group = file->private_data;
-   bool ret;
+   struct vfio_device *device;
+   bool ret = true;
 
if (!vfio_file_is_group(file))
return true;
 
-   mutex_lock(&group->group_lock);
-   if (group->container) {
-   ret = vfio_container_ioctl_check_extension(group->container,
-  VFIO_DMA_CC_IOMMU);
-   } else {
-   /*
-* Since the coherency state is determined only once a container
-* is attached the user must do so before they can prove they
-* have permission.
-*/
-   ret = true;
+   /*
+* If the device does not have IOMMU_CAP_ENFORCE_CACHE_COHERENCY then
+* any domain later attached to it will also not support it. If the cap
+* is set then the iommu_domain eventually attached to the device/group
+* must use a domain with enforce_cache_coherency().
+*/
+   mutex_lock(&group->device_lock);
+   list_for_each_entry(device, &group->device_list, group_next) {
+   if (!device_iommu_capable(device->dev,
+ IOMMU_CAP_ENFORCE_CACHE_COHERENCY)) {
+   ret = false;
+   break;
+   }
}
-   mutex_unlock(&group->group_lock);
+   mutex_unlock(&group->device_lock);
return ret;
 }
 EXPORT_SYMBOL_GPL(vfio_file_enforced_coherent);
-- 
2.38.1

[PATCH v2 07/11] vfio-iommufd: Support iommufd for physical VFIO devices

This creates the iommufd_device for the physical VFIO drivers. These are
all the drivers that are calling vfio_register_group_dev() and expect the
type1 code to setup a real iommu_domain against their parent struct
device.

The design gives the driver a choice in how it gets connected to iommufd
by providing bind_iommufd/unbind_iommufd/attach_ioas callbacks to
implement as required. The core code provides three default callbacks for
physical mode using a real iommu_domain. This is suitable for drivers
using vfio_register_group_dev()

Tested-by: Nicolin Chen 
Signed-off-by: Jason Gunthorpe 
---
 drivers/vfio/Makefile |  1 +
 drivers/vfio/fsl-mc/vfio_fsl_mc.c |  3 +
 drivers/vfio/iommufd.c| 99 +++
 .../vfio/pci/hisilicon/hisi_acc_vfio_pci.c|  6 ++
 drivers/vfio/pci/mlx5/main.c  |  3 +
 drivers/vfio/pci/vfio_pci.c   |  3 +
 drivers/vfio/platform/vfio_amba.c |  3 +
 drivers/vfio/platform/vfio_platform.c |  3 +
 drivers/vfio/vfio.h   | 15 +++
 drivers/vfio/vfio_main.c  | 13 ++-
 include/linux/vfio.h  | 25 +
 11 files changed, 172 insertions(+), 2 deletions(-)
 create mode 100644 drivers/vfio/iommufd.c

diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
index b693a1169286f8..3863922529ef20 100644
--- a/drivers/vfio/Makefile
+++ b/drivers/vfio/Makefile
@@ -6,6 +6,7 @@ obj-$(CONFIG_VFIO) += vfio.o
 vfio-y += vfio_main.o \
  iova_bitmap.o \
  container.o
+vfio-$(CONFIG_IOMMUFD) += iommufd.o
 
 obj-$(CONFIG_VFIO_VIRQFD) += vfio_virqfd.o
 obj-$(CONFIG_VFIO_IOMMU_TYPE1) += vfio_iommu_type1.o
diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc.c 
b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
index b16874e913e4f5..5cd4bb47644039 100644
--- a/drivers/vfio/fsl-mc/vfio_fsl_mc.c
+++ b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
@@ -592,6 +592,9 @@ static const struct vfio_device_ops vfio_fsl_mc_ops = {
.read   = vfio_fsl_mc_read,
.write  = vfio_fsl_mc_write,
.mmap   = vfio_fsl_mc_mmap,
+   .bind_iommufd   = vfio_iommufd_physical_bind,
+   .unbind_iommufd = vfio_iommufd_physical_unbind,
+   .attach_ioas= vfio_iommufd_physical_attach_ioas,
 };
 
 static struct fsl_mc_driver vfio_fsl_mc_driver = {
diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c
new file mode 100644
index 00..bf755d0f375c5d
--- /dev/null
+++ b/drivers/vfio/iommufd.c
@@ -0,0 +1,99 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2021-2022, NVIDIA CORPORATION & AFFILIATES
+ */
+#include 
+#include 
+
+#include "vfio.h"
+
+MODULE_IMPORT_NS(IOMMUFD);
+MODULE_IMPORT_NS(IOMMUFD_VFIO);
+
+int vfio_iommufd_bind(struct vfio_device *vdev, struct iommufd_ctx *ictx)
+{
+   u32 ioas_id;
+   u32 device_id;
+   int ret;
+
+   lockdep_assert_held(&vdev->dev_set->lock);
+
+   /*
+* If the driver doesn't provide this op then it means the device does
+* not do DMA at all. So nothing to do.
+*/
+   if (!vdev->ops->bind_iommufd)
+   return 0;
+
+   ret = vdev->ops->bind_iommufd(vdev, ictx, &device_id);
+   if (ret)
+   return ret;
+
+   ret = iommufd_vfio_compat_ioas_id(ictx, &ioas_id);
+   if (ret)
+   goto err_unbind;
+   ret = vdev->ops->attach_ioas(vdev, &ioas_id);
+   if (ret)
+   goto err_unbind;
+   vdev->iommufd_attached = true;
+
+   /*
+* The legacy path has no way to return the device id or the selected
+* pt_id
+*/
+   return 0;
+
+err_unbind:
+   if (vdev->ops->unbind_iommufd)
+   vdev->ops->unbind_iommufd(vdev);
+   return ret;
+}
+
+void vfio_iommufd_unbind(struct vfio_device *vdev)
+{
+   lockdep_assert_held(&vdev->dev_set->lock);
+
+   if (vdev->ops->unbind_iommufd)
+   vdev->ops->unbind_iommufd(vdev);
+}
+
+/*
+ * The physical standard ops mean that the iommufd_device is bound to the
+ * physical device vdev->dev that was provided to vfio_init_group_dev(). 
Drivers
+ * using this ops set should call vfio_register_group_dev()
+ */
+int vfio_iommufd_physical_bind(struct vfio_device *vdev,
+  struct iommufd_ctx *ictx, u32 *out_device_id)
+{
+   struct iommufd_device *idev;
+
+   idev = iommufd_device_bind(ictx, vdev->dev, out_device_id);
+   if (IS_ERR(idev))
+   return PTR_ERR(idev);
+   vdev->iommufd_device = idev;
+   return 0;
+}
+EXPORT_SYMBOL_GPL(vfio_iommufd_physical_bind);
+
+void vfio_iommufd_physical_unbind(struct vfio_device *vdev)
+{
+   lockdep_assert_held(&vdev->dev_set->lock);
+
+   if (vdev->iommufd_attached) {
+   iommufd_device_detach(vdev->iommufd_device);
+   vdev->iommufd_attached = false;
+   }
+   iommufd_device_unbind(vdev->iommufd_device);
+   vd

[PATCH v2 10/11] vfio: Make vfio_container optionally compiled

Add a kconfig CONFIG_VFIO_CONTAINER that controls compiling the container
code. If 'n' then only iommufd will provide the container service. All the
support for vfio iommu drivers, including type1, will not be built.

This allows a compilation check that no inappropriate dependencies between
the device/group and container have been created.

Tested-by: Nicolin Chen 
Signed-off-by: Jason Gunthorpe 
---
 drivers/vfio/Kconfig  | 35 +++
 drivers/vfio/Makefile |  4 +--
 drivers/vfio/vfio.h   | 65 +++
 3 files changed, 91 insertions(+), 13 deletions(-)

diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
index 1118d322eec97d..286c1663bd7564 100644
--- a/drivers/vfio/Kconfig
+++ b/drivers/vfio/Kconfig
@@ -3,8 +3,8 @@ menuconfig VFIO
tristate "VFIO Non-Privileged userspace driver framework"
select IOMMU_API
depends on IOMMUFD || !IOMMUFD
-   select VFIO_IOMMU_TYPE1 if MMU && (X86 || S390 || ARM || ARM64)
select INTERVAL_TREE
+   select VFIO_CONTAINER if IOMMUFD=n
help
  VFIO provides a framework for secure userspace device drivers.
  See Documentation/driver-api/vfio.rst for more details.
@@ -12,6 +12,18 @@ menuconfig VFIO
  If you don't know what to do here, say N.
 
 if VFIO
+config VFIO_CONTAINER
+   bool "Support for the VFIO container /dev/vfio/vfio"
+   select VFIO_IOMMU_TYPE1 if MMU && (X86 || S390 || ARM || ARM64)
+   default y
+   help
+ The VFIO container is the classic interface to VFIO for establishing
+ IOMMU mappings. If N is selected here then IOMMUFD must be used to
+ manage the mappings.
+
+ Unless testing IOMMUFD say Y here.
+
+if VFIO_CONTAINER
 config VFIO_IOMMU_TYPE1
tristate
default n
@@ -21,16 +33,6 @@ config VFIO_IOMMU_SPAPR_TCE
depends on SPAPR_TCE_IOMMU
default VFIO
 
-config VFIO_SPAPR_EEH
-   tristate
-   depends on EEH && VFIO_IOMMU_SPAPR_TCE
-   default VFIO
-
-config VFIO_VIRQFD
-   tristate
-   select EVENTFD
-   default n
-
 config VFIO_NOIOMMU
bool "VFIO No-IOMMU support"
help
@@ -44,6 +46,17 @@ config VFIO_NOIOMMU
  this mode since there is no IOMMU to provide DMA translation.
 
  If you don't know what to do here, say N.
+endif
+
+config VFIO_SPAPR_EEH
+   tristate
+   depends on EEH && VFIO_IOMMU_SPAPR_TCE
+   default VFIO
+
+config VFIO_VIRQFD
+   tristate
+   select EVENTFD
+   default n
 
 source "drivers/vfio/pci/Kconfig"
 source "drivers/vfio/platform/Kconfig"
diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
index 3863922529ef20..b953517dc70f99 100644
--- a/drivers/vfio/Makefile
+++ b/drivers/vfio/Makefile
@@ -4,9 +4,9 @@ vfio_virqfd-y := virqfd.o
 obj-$(CONFIG_VFIO) += vfio.o
 
 vfio-y += vfio_main.o \
- iova_bitmap.o \
- container.o
+ iova_bitmap.o
 vfio-$(CONFIG_IOMMUFD) += iommufd.o
+vfio-$(CONFIG_VFIO_CONTAINER) += container.o
 
 obj-$(CONFIG_VFIO_VIRQFD) += vfio_virqfd.o
 obj-$(CONFIG_VFIO_IOMMU_TYPE1) += vfio_iommu_type1.o
diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
index d57a08afb5cf5c..3378714a746274 100644
--- a/drivers/vfio/vfio.h
+++ b/drivers/vfio/vfio.h
@@ -55,7 +55,9 @@ struct vfio_group {
struct list_headdevice_list;
struct mutexdevice_lock;
struct list_headvfio_next;
+#if IS_ENABLED(CONFIG_VFIO_CONTAINER)
struct list_headcontainer_next;
+#endif
enum vfio_group_typetype;
struct mutexgroup_lock;
struct kvm  *kvm;
@@ -64,6 +66,7 @@ struct vfio_group {
struct iommufd_ctx  *iommufd;
 };
 
+#if IS_ENABLED(CONFIG_VFIO_CONTAINER)
 /* events for the backend driver notify callback */
 enum vfio_iommu_notify_type {
VFIO_IOMMU_CONTAINER_CLOSE = 0,
@@ -129,6 +132,68 @@ int vfio_container_dma_rw(struct vfio_container 
*container, dma_addr_t iova,
 
 int __init vfio_container_init(void);
 void vfio_container_cleanup(void);
+#else
+static inline struct vfio_container *
+vfio_container_from_file(struct file *filep)
+{
+   return NULL;
+}
+
+static inline int vfio_group_use_container(struct vfio_group *group)
+{
+   return -EOPNOTSUPP;
+}
+
+static inline void vfio_group_unuse_container(struct vfio_group *group)
+{
+}
+
+static inline int vfio_container_attach_group(struct vfio_container *container,
+ struct vfio_group *group)
+{
+   return -EOPNOTSUPP;
+}
+
+static inline void vfio_group_detach_container(struct vfio_group *group)
+{
+}
+
+static inline void vfio_device_container_register(struct vfio_device *device)
+{
+}
+
+static inline void vfio_device_container_unregister(struct vfio_device *device)
+{
+}
+
+static inline int vfio_container_pin_pages(struct vfio_container *contai

[PATCH v2 02/11] vfio: Move vfio_device_assign_container() into vfio_device_first_open()

The only thing this function does is assert the group has an assigned
container and incrs refcounts.

The overall model we have is that once a container_users refcount is
incremented it cannot be de-assigned from the group -
vfio_group_ioctl_unset_container() will fail and the group FD cannot be
closed.

Thus we do not need to check this on every device FD open, just the
first. Reorganize the code so that only the first open and last close
manages the container.

Reviewed-by: Kevin Tian 
Reviewed-by: Yi Liu 
Tested-by: Nicolin Chen 
Signed-off-by: Jason Gunthorpe 
---
 drivers/vfio/container.c |  4 ++--
 drivers/vfio/vfio_main.c | 24 +++-
 2 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/vfio/container.c b/drivers/vfio/container.c
index d74164abbf401d..dd79a66ec62cad 100644
--- a/drivers/vfio/container.c
+++ b/drivers/vfio/container.c
@@ -531,11 +531,11 @@ int vfio_device_assign_container(struct vfio_device 
*device)
 
 void vfio_device_unassign_container(struct vfio_device *device)
 {
-   mutex_lock(&device->group->group_lock);
+   lockdep_assert_held_write(&device->group->group_lock);
+
WARN_ON(device->group->container_users <= 1);
device->group->container_users--;
fput(device->group->opened_file);
-   mutex_unlock(&device->group->group_lock);
 }
 
 /*
diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index 2e8346d13c16ca..717c7f404feeea 100644
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -749,18 +749,24 @@ static int vfio_device_first_open(struct vfio_device 
*device)
 * during close_device.
 */
mutex_lock(&device->group->group_lock);
+   ret = vfio_device_assign_container(device);
+   if (ret)
+   goto err_module_put;
+
device->kvm = device->group->kvm;
if (device->ops->open_device) {
ret = device->ops->open_device(device);
if (ret)
-   goto err_module_put;
+   goto err_container;
}
vfio_device_container_register(device);
mutex_unlock(&device->group->group_lock);
return 0;
 
-err_module_put:
+err_container:
device->kvm = NULL;
+   vfio_device_unassign_container(device);
+err_module_put:
mutex_unlock(&device->group->group_lock);
module_put(device->dev->driver->owner);
return ret;
@@ -775,6 +781,7 @@ static void vfio_device_last_close(struct vfio_device 
*device)
if (device->ops->close_device)
device->ops->close_device(device);
device->kvm = NULL;
+   vfio_device_unassign_container(device);
mutex_unlock(&device->group->group_lock);
module_put(device->dev->driver->owner);
 }
@@ -784,18 +791,12 @@ static struct file *vfio_device_open(struct vfio_device 
*device)
struct file *filep;
int ret;
 
-   mutex_lock(&device->group->group_lock);
-   ret = vfio_device_assign_container(device);
-   mutex_unlock(&device->group->group_lock);
-   if (ret)
-   return ERR_PTR(ret);
-
mutex_lock(&device->dev_set->lock);
device->open_count++;
if (device->open_count == 1) {
ret = vfio_device_first_open(device);
if (ret)
-   goto err_unassign_container;
+   goto err_unlock;
}
mutex_unlock(&device->dev_set->lock);
 
@@ -830,10 +831,9 @@ static struct file *vfio_device_open(struct vfio_device 
*device)
mutex_lock(&device->dev_set->lock);
if (device->open_count == 1)
vfio_device_last_close(device);
-err_unassign_container:
+err_unlock:
device->open_count--;
mutex_unlock(&device->dev_set->lock);
-   vfio_device_unassign_container(device);
return ERR_PTR(ret);
 }
 
@@ -1040,8 +1040,6 @@ static int vfio_device_fops_release(struct inode *inode, 
struct file *filep)
device->open_count--;
mutex_unlock(&device->dev_set->lock);
 
-   vfio_device_unassign_container(device);
-
vfio_device_put_registration(device);
 
return 0;
-- 
2.38.1

[PATCH v2 08/11] vfio-iommufd: Support iommufd for emulated VFIO devices

Emulated VFIO devices are calling vfio_register_emulated_iommu_dev() and
consist of all the mdev drivers.

Like the physical drivers, support for iommufd is provided by the driver
supplying the correct standard ops. Provide ops from the core that
duplicate what vfio_register_emulated_iommu_dev() does.

Emulated drivers are where it is more likely to see variation in the
iommfd support ops. For instance IDXD will probably need to setup both a
iommfd_device context linked to a PASID and an iommufd_access context to
support all their mdev operations.

Tested-by: Nicolin Chen 
Signed-off-by: Jason Gunthorpe 
---
 drivers/gpu/drm/i915/gvt/kvmgt.c  |   3 +
 drivers/s390/cio/vfio_ccw_ops.c   |   3 +
 drivers/s390/crypto/vfio_ap_ops.c |   3 +
 drivers/vfio/container.c  | 110 +---
 drivers/vfio/iommufd.c|  58 +++
 drivers/vfio/vfio.h   |  10 ++-
 drivers/vfio/vfio_main.c  | 114 +-
 include/linux/vfio.h  |  14 
 8 files changed, 221 insertions(+), 94 deletions(-)

diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index 7a45e5360caf2d..579b230a0f58d9 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -1474,6 +1474,9 @@ static const struct vfio_device_ops intel_vgpu_dev_ops = {
.mmap   = intel_vgpu_mmap,
.ioctl  = intel_vgpu_ioctl,
.dma_unmap  = intel_vgpu_dma_unmap,
+   .bind_iommufd   = vfio_iommufd_emulated_bind,
+   .unbind_iommufd = vfio_iommufd_emulated_unbind,
+   .attach_ioas= vfio_iommufd_emulated_attach_ioas,
 };
 
 static int intel_vgpu_probe(struct mdev_device *mdev)
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 6ae4d012d80084..560453d99c24fc 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -588,6 +588,9 @@ static const struct vfio_device_ops vfio_ccw_dev_ops = {
.ioctl = vfio_ccw_mdev_ioctl,
.request = vfio_ccw_mdev_request,
.dma_unmap = vfio_ccw_dma_unmap,
+   .bind_iommufd = vfio_iommufd_emulated_bind,
+   .unbind_iommufd = vfio_iommufd_emulated_unbind,
+   .attach_ioas = vfio_iommufd_emulated_attach_ioas,
 };
 
 struct mdev_driver vfio_ccw_mdev_driver = {
diff --git a/drivers/s390/crypto/vfio_ap_ops.c 
b/drivers/s390/crypto/vfio_ap_ops.c
index 0b4cc8c597ae67..bb7776d207924f 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -1789,6 +1789,9 @@ static const struct vfio_device_ops 
vfio_ap_matrix_dev_ops = {
.close_device = vfio_ap_mdev_close_device,
.ioctl = vfio_ap_mdev_ioctl,
.dma_unmap = vfio_ap_mdev_dma_unmap,
+   .bind_iommufd = vfio_iommufd_emulated_bind,
+   .unbind_iommufd = vfio_iommufd_emulated_unbind,
+   .attach_ioas = vfio_iommufd_emulated_attach_ioas,
 };
 
 static struct mdev_driver vfio_ap_matrix_driver = {
diff --git a/drivers/vfio/container.c b/drivers/vfio/container.c
index 8772dad6808539..7f3961fd4b5aac 100644
--- a/drivers/vfio/container.c
+++ b/drivers/vfio/container.c
@@ -540,113 +540,41 @@ void vfio_group_unuse_container(struct vfio_group *group)
fput(group->opened_file);
 }
 
-/*
- * Pin contiguous user pages and return their associated host pages for local
- * domain only.
- * @device [in]  : device
- * @iova [in]: starting IOVA of user pages to be pinned.
- * @npage [in]   : count of pages to be pinned.  This count should not
- *be greater than VFIO_PIN_PAGES_MAX_ENTRIES.
- * @prot [in]: protection flags
- * @pages[out]   : array of host pages
- * Return error or number of pages pinned.
- *
- * A driver may only call this function if the vfio_device was created
- * by vfio_register_emulated_iommu_dev().
- */
-int vfio_pin_pages(struct vfio_device *device, dma_addr_t iova,
-  int npage, int prot, struct page **pages)
+int vfio_container_pin_pages(struct vfio_container *container,
+struct iommu_group *iommu_group, dma_addr_t iova,
+int npage, int prot, struct page **pages)
 {
-   struct vfio_container *container;
-   struct vfio_group *group = device->group;
-   struct vfio_iommu_driver *driver;
-   int ret;
-
-   if (!pages || !npage || !vfio_assert_device_open(device))
-   return -EINVAL;
+   struct vfio_iommu_driver *driver = container->iommu_driver;
 
if (npage > VFIO_PIN_PAGES_MAX_ENTRIES)
return -E2BIG;
 
-   /* group->container cannot change while a vfio device is open */
-   container = group->container;
-   driver = container->iommu_driver;
-   if (likely(driver && driver->ops->pin_pages))
-   ret = driver->ops->pin_pages(container->iommu_data,
-group->iommu_group, iova,
-

[PATCH v2 09/11] vfio: Move container related MODULE_ALIAS statements into container.c

The miscdev is in container.c, so should these related MODULE_ALIAS
statements. This is necessary for the next patch to be able to fully
disable /dev/vfio/vfio.

Fixes: cdc71fe4ecbf ("vfio: Move container code into drivers/vfio/container.c")
Reported-by: "Liu, Yi L" 
Signed-off-by: Jason Gunthorpe 
---
 drivers/vfio/container.c | 3 +++
 drivers/vfio/vfio_main.c | 2 --
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/vfio/container.c b/drivers/vfio/container.c
index 7f3961fd4b5aac..6b362d97d68220 100644
--- a/drivers/vfio/container.c
+++ b/drivers/vfio/container.c
@@ -608,3 +608,6 @@ void vfio_container_cleanup(void)
misc_deregister(&vfio_dev);
mutex_destroy(&vfio.iommu_drivers_lock);
 }
+
+MODULE_ALIAS_MISCDEV(VFIO_MINOR);
+MODULE_ALIAS("devname:vfio/vfio");
diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index cf49c5200a4c05..ee09ccf4a608e1 100644
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -2064,6 +2064,4 @@ MODULE_VERSION(DRIVER_VERSION);
 MODULE_LICENSE("GPL v2");
 MODULE_AUTHOR(DRIVER_AUTHOR);
 MODULE_DESCRIPTION(DRIVER_DESC);
-MODULE_ALIAS_MISCDEV(VFIO_MINOR);
-MODULE_ALIAS("devname:vfio/vfio");
 MODULE_SOFTDEP("post: vfio_iommu_type1 vfio_iommu_spapr_tce");
-- 
2.38.1

[PATCH v2 04/11] vfio: Move storage of allow_unsafe_interrupts to vfio_main.c

This legacy module knob has become uAPI, when set on the vfio_iommu_type1
it disables some security protections in the iommu drivers. Move the
storage for this knob to vfio_main.c so that iommufd can access it too.

The may need enhancing as we learn more about how necessary
allow_unsafe_interrupts is in the current state of the world. If vfio
container is disabled then this option will not be available to the user.

Tested-by: Nicolin Chen 
Signed-off-by: Jason Gunthorpe 
---
 drivers/vfio/vfio.h | 2 ++
 drivers/vfio/vfio_iommu_type1.c | 5 ++---
 drivers/vfio/vfio_main.c| 3 +++
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
index f95f4925b83bbd..54e5a8e0834ccb 100644
--- a/drivers/vfio/vfio.h
+++ b/drivers/vfio/vfio.h
@@ -130,4 +130,6 @@ extern bool vfio_noiommu __read_mostly;
 enum { vfio_noiommu = false };
 #endif
 
+extern bool vfio_allow_unsafe_interrupts;
+
 #endif
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 23c24fe98c00d4..186e33a006d314 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -44,9 +44,8 @@
 #define DRIVER_AUTHOR   "Alex Williamson "
 #define DRIVER_DESC "Type1 IOMMU driver for VFIO"
 
-static bool allow_unsafe_interrupts;
 module_param_named(allow_unsafe_interrupts,
-  allow_unsafe_interrupts, bool, S_IRUGO | S_IWUSR);
+  vfio_allow_unsafe_interrupts, bool, S_IRUGO | S_IWUSR);
 MODULE_PARM_DESC(allow_unsafe_interrupts,
 "Enable VFIO IOMMU support for on platforms without interrupt 
remapping support.");
 
@@ -2282,7 +2281,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
iommu_group_for_each_dev(iommu_group, (void 
*)IOMMU_CAP_INTR_REMAP,
 vfio_iommu_device_capable);
 
-   if (!allow_unsafe_interrupts && !msi_remap) {
+   if (!vfio_allow_unsafe_interrupts && !msi_remap) {
pr_warn("%s: No interrupt remapping support.  Use the module 
param \"allow_unsafe_interrupts\" to enable VFIO IOMMU support on this 
platform\n",
   __func__);
ret = -EPERM;
diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index 8c2dcb481ae10b..e1fec1db6a3c93 100644
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -51,6 +51,9 @@ static struct vfio {
struct ida  device_ida;
 } vfio;
 
+bool vfio_allow_unsafe_interrupts;
+EXPORT_SYMBOL_GPL(vfio_allow_unsafe_interrupts);
+
 static DEFINE_XARRAY(vfio_device_set_xa);
 static const struct file_operations vfio_group_fops;
 
-- 
2.38.1

[PATCH v2 03/11] vfio: Rename vfio_device_assign/unassign_container()

These functions don't really assign anything anymore, they just increment
some refcounts and do a sanity check. Call them
vfio_group_[un]use_container()

Reviewed-by: Kevin Tian 
Reviewed-by: Yi Liu 
Tested-by: Nicolin Chen 
Signed-off-by: Jason Gunthorpe 
---
 drivers/vfio/container.c | 14 ++
 drivers/vfio/vfio.h  |  4 ++--
 drivers/vfio/vfio_main.c |  6 +++---
 3 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/vfio/container.c b/drivers/vfio/container.c
index dd79a66ec62cad..499777930b08fa 100644
--- a/drivers/vfio/container.c
+++ b/drivers/vfio/container.c
@@ -511,10 +511,8 @@ void vfio_group_detach_container(struct vfio_group *group)
vfio_container_put(container);
 }
 
-int vfio_device_assign_container(struct vfio_device *device)
+int vfio_group_use_container(struct vfio_group *group)
 {
-   struct vfio_group *group = device->group;
-
lockdep_assert_held(&group->group_lock);
 
if (!group->container || !group->container->iommu_driver ||
@@ -529,13 +527,13 @@ int vfio_device_assign_container(struct vfio_device 
*device)
return 0;
 }
 
-void vfio_device_unassign_container(struct vfio_device *device)
+void vfio_group_unuse_container(struct vfio_group *group)
 {
-   lockdep_assert_held_write(&device->group->group_lock);
+   lockdep_assert_held(&group->group_lock);
 
-   WARN_ON(device->group->container_users <= 1);
-   device->group->container_users--;
-   fput(device->group->opened_file);
+   WARN_ON(group->container_users <= 1);
+   group->container_users--;
+   fput(group->opened_file);
 }
 
 /*
diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
index bcad54bbab08c4..f95f4925b83bbd 100644
--- a/drivers/vfio/vfio.h
+++ b/drivers/vfio/vfio.h
@@ -112,8 +112,8 @@ void vfio_unregister_iommu_driver(const struct 
vfio_iommu_driver_ops *ops);
 bool vfio_assert_device_open(struct vfio_device *device);
 
 struct vfio_container *vfio_container_from_file(struct file *filep);
-int vfio_device_assign_container(struct vfio_device *device);
-void vfio_device_unassign_container(struct vfio_device *device);
+int vfio_group_use_container(struct vfio_group *group);
+void vfio_group_unuse_container(struct vfio_group *group);
 int vfio_container_attach_group(struct vfio_container *container,
struct vfio_group *group);
 void vfio_group_detach_container(struct vfio_group *group);
diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index 717c7f404feeea..8c2dcb481ae10b 100644
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -749,7 +749,7 @@ static int vfio_device_first_open(struct vfio_device 
*device)
 * during close_device.
 */
mutex_lock(&device->group->group_lock);
-   ret = vfio_device_assign_container(device);
+   ret = vfio_group_use_container(device->group);
if (ret)
goto err_module_put;
 
@@ -765,7 +765,7 @@ static int vfio_device_first_open(struct vfio_device 
*device)
 
 err_container:
device->kvm = NULL;
-   vfio_device_unassign_container(device);
+   vfio_group_unuse_container(device->group);
 err_module_put:
mutex_unlock(&device->group->group_lock);
module_put(device->dev->driver->owner);
@@ -781,7 +781,7 @@ static void vfio_device_last_close(struct vfio_device 
*device)
if (device->ops->close_device)
device->ops->close_device(device);
device->kvm = NULL;
-   vfio_device_unassign_container(device);
+   vfio_group_unuse_container(device->group);
mutex_unlock(&device->group->group_lock);
module_put(device->dev->driver->owner);
 }
-- 
2.38.1

[PATCH v2 06/11] vfio-iommufd: Allow iommufd to be used in place of a container fd

This makes VFIO_GROUP_SET_CONTAINER accept both a vfio container FD and an
iommufd.

In iommufd mode an IOAS will exist after the SET_CONTAINER, but it will
not be attached to any groups.

For VFIO this means that the VFIO_GROUP_GET_STATUS and
VFIO_GROUP_FLAGS_VIABLE works subtly differently. With the container FD
the iommu_group_claim_dma_owner() is done during SET_CONTAINER but for
IOMMUFD this is done during VFIO_GROUP_GET_DEVICE_FD. Meaning that
VFIO_GROUP_FLAGS_VIABLE could be set but GET_DEVICE_FD will fail due to
viability.

As GET_DEVICE_FD can fail for many reasons already this is not expected to
be a meaningful difference.

Reorganize the tests for if the group has an assigned container or iommu
into a vfio_group_has_iommu() function and consolidate all the duplicated
WARN_ON's etc related to this.

Call container functions only if a container is actually present on the
group.

Tested-by: Nicolin Chen 
Signed-off-by: Jason Gunthorpe 
---
 drivers/vfio/Kconfig |  1 +
 drivers/vfio/container.c |  7 +++-
 drivers/vfio/vfio.h  |  2 +
 drivers/vfio/vfio_main.c | 86 +---
 4 files changed, 80 insertions(+), 16 deletions(-)

diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
index 86c381ceb9a1e9..1118d322eec97d 100644
--- a/drivers/vfio/Kconfig
+++ b/drivers/vfio/Kconfig
@@ -2,6 +2,7 @@
 menuconfig VFIO
tristate "VFIO Non-Privileged userspace driver framework"
select IOMMU_API
+   depends on IOMMUFD || !IOMMUFD
select VFIO_IOMMU_TYPE1 if MMU && (X86 || S390 || ARM || ARM64)
select INTERVAL_TREE
help
diff --git a/drivers/vfio/container.c b/drivers/vfio/container.c
index d97747dfb05d02..8772dad6808539 100644
--- a/drivers/vfio/container.c
+++ b/drivers/vfio/container.c
@@ -516,8 +516,11 @@ int vfio_group_use_container(struct vfio_group *group)
 {
lockdep_assert_held(&group->group_lock);
 
-   if (!group->container || !group->container->iommu_driver ||
-   WARN_ON(!group->container_users))
+   /*
+* The container fd has been assigned with VFIO_GROUP_SET_CONTAINER but
+* VFIO_SET_IOMMU hasn't been done yet.
+*/
+   if (!group->container->iommu_driver)
return -EINVAL;
 
if (group->type == VFIO_NO_IOMMU && !capable(CAP_SYS_RAWIO))
diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
index 247590334e14b0..985e13d52989ca 100644
--- a/drivers/vfio/vfio.h
+++ b/drivers/vfio/vfio.h
@@ -10,6 +10,7 @@
 #include 
 #include 
 
+struct iommufd_ctx;
 struct iommu_group;
 struct vfio_device;
 struct vfio_container;
@@ -60,6 +61,7 @@ struct vfio_group {
struct kvm  *kvm;
struct file *opened_file;
struct blocking_notifier_head   notifier;
+   struct iommufd_ctx  *iommufd;
 };
 
 /* events for the backend driver notify callback */
diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index 5c0e810f8b4d08..8c124290ce9f0d 100644
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -35,6 +35,7 @@
 #include 
 #include 
 #include 
+#include 
 #include "vfio.h"
 
 #define DRIVER_VERSION "0.3"
@@ -665,6 +666,16 @@ EXPORT_SYMBOL_GPL(vfio_unregister_group_dev);
 /*
  * VFIO Group fd, /dev/vfio/$GROUP
  */
+static bool vfio_group_has_iommu(struct vfio_group *group)
+{
+   lockdep_assert_held(&group->group_lock);
+   if (!group->container)
+   WARN_ON(group->container_users);
+   else
+   WARN_ON(!group->container_users);
+   return group->container || group->iommufd;
+}
+
 /*
  * VFIO_GROUP_UNSET_CONTAINER should fail if there are other users or
  * if there was no container to unset.  Since the ioctl is called on
@@ -676,15 +687,21 @@ static int vfio_group_ioctl_unset_container(struct 
vfio_group *group)
int ret = 0;
 
mutex_lock(&group->group_lock);
-   if (!group->container) {
+   if (!vfio_group_has_iommu(group)) {
ret = -EINVAL;
goto out_unlock;
}
-   if (group->container_users != 1) {
-   ret = -EBUSY;
-   goto out_unlock;
+   if (group->container) {
+   if (group->container_users != 1) {
+   ret = -EBUSY;
+   goto out_unlock;
+   }
+   vfio_group_detach_container(group);
+   }
+   if (group->iommufd) {
+   iommufd_ctx_put(group->iommufd);
+   group->iommufd = NULL;
}
-   vfio_group_detach_container(group);
 
 out_unlock:
mutex_unlock(&group->group_lock);
@@ -695,6 +712,7 @@ static int vfio_group_ioctl_set_container(struct vfio_group 
*group,
  int __user *arg)
 {
struct vfio_container *container;
+   struct iommufd_ctx *iommufd;
struct fd f;
int ret;
int fd;
@@ -707,7 +725,7 @@ static int vfio_group_ioctl_set_container(struc

[PATCH v2 11/11] iommufd: Allow iommufd to supply /dev/vfio/vfio

If the VFIO container is compiled out, give a kconfig option for iommufd
to provide the miscdev node with the same name and permissions as vfio
uses.

The compatibility node supports the same ioctls as VFIO and automatically
enables the VFIO compatible pinned page accounting mode.

Tested-by: Nicolin Chen 
Signed-off-by: Jason Gunthorpe 
---
 drivers/iommu/iommufd/Kconfig | 12 
 drivers/iommu/iommufd/main.c  | 36 +++
 2 files changed, 48 insertions(+)

diff --git a/drivers/iommu/iommufd/Kconfig b/drivers/iommu/iommufd/Kconfig
index 399a2edeaef6de..f387f803dc6f7f 100644
--- a/drivers/iommu/iommufd/Kconfig
+++ b/drivers/iommu/iommufd/Kconfig
@@ -12,6 +12,18 @@ config IOMMUFD
  If you don't know what to do here, say N.
 
 if IOMMUFD
+config IOMMUFD_VFIO_CONTAINER
+   bool "IOMMUFD provides the VFIO container /dev/vfio/vfio"
+   depends on VFIO && !VFIO_CONTAINER
+   default VFIO && !VFIO_CONTAINER
+   help
+ IOMMUFD will provide /dev/vfio/vfio instead of VFIO. This relies on
+ IOMMUFD providing compatibility emulation to give the same ioctls.
+ It provides an option to build a kernel with legacy VFIO components
+ removed.
+
+ Unless testing IOMMUFD say N here.
+
 config IOMMUFD_TEST
bool "IOMMU Userspace API Test support"
depends on RUNTIME_TESTING_MENU
diff --git a/drivers/iommu/iommufd/main.c b/drivers/iommu/iommufd/main.c
index ab3fa05f38505d..1eeb326f74f005 100644
--- a/drivers/iommu/iommufd/main.c
+++ b/drivers/iommu/iommufd/main.c
@@ -18,6 +18,7 @@
 #include 
 #include 
 
+#include "io_pagetable.h"
 #include "iommufd_private.h"
 #include "iommufd_test.h"
 
@@ -25,6 +26,7 @@ struct iommufd_object_ops {
void (*destroy)(struct iommufd_object *obj);
 };
 static const struct iommufd_object_ops iommufd_object_ops[];
+static struct miscdevice vfio_misc_dev;
 
 struct iommufd_object *_iommufd_object_alloc(struct iommufd_ctx *ictx,
 size_t size,
@@ -170,6 +172,16 @@ static int iommufd_fops_open(struct inode *inode, struct 
file *filp)
if (!ictx)
return -ENOMEM;
 
+   /*
+* For compatibility with VFIO when /dev/vfio/vfio is opened we default
+* to the same rlimit accounting as vfio uses.
+*/
+   if (IS_ENABLED(CONFIG_IOMMUFD_VFIO_CONTAINER) &&
+   filp->private_data == &vfio_misc_dev) {
+   ictx->account_mode = IOPT_PAGES_ACCOUNT_MM;
+   pr_info_once("IOMMUFD is providing /dev/vfio/vfio, not 
VFIO.\n");
+   }
+
xa_init_flags(&ictx->objects, XA_FLAGS_ALLOC1 | XA_FLAGS_ACCOUNT);
ictx->file = filp;
filp->private_data = ictx;
@@ -395,6 +407,15 @@ static struct miscdevice iommu_misc_dev = {
.mode = 0660,
 };
 
+
+static struct miscdevice vfio_misc_dev = {
+   .minor = VFIO_MINOR,
+   .name = "vfio",
+   .fops = &iommufd_fops,
+   .nodename = "vfio/vfio",
+   .mode = 0666,
+};
+
 static int __init iommufd_init(void)
 {
int ret;
@@ -402,18 +423,33 @@ static int __init iommufd_init(void)
ret = misc_register(&iommu_misc_dev);
if (ret)
return ret;
+
+   if (IS_ENABLED(CONFIG_IOMMUFD_VFIO_CONTAINER)) {
+   ret = misc_register(&vfio_misc_dev);
+   if (ret)
+   goto err_misc;
+   }
iommufd_test_init();
return 0;
+err_misc:
+   misc_deregister(&iommu_misc_dev);
+   return ret;
 }
 
 static void __exit iommufd_exit(void)
 {
iommufd_test_exit();
+   if (IS_ENABLED(CONFIG_IOMMUFD_VFIO_CONTAINER))
+   misc_deregister(&vfio_misc_dev);
misc_deregister(&iommu_misc_dev);
 }
 
 module_init(iommufd_init);
 module_exit(iommufd_exit);
 
+#if IS_ENABLED(CONFIG_IOMMUFD_VFIO_CONTAINER)
+MODULE_ALIAS_MISCDEV(VFIO_MINOR);
+MODULE_ALIAS("devname:vfio/vfio");
+#endif
 MODULE_DESCRIPTION("I/O Address Space Management for passthrough devices");
 MODULE_LICENSE("GPL");
-- 
2.38.1

[PATCH] drm/i915: Update workaround documentation

2022-11-07 Thread Lucas De Marchi

There were several updates in the driver on how the workarounds are
handled since its documentation was written. Update the documentation to
reflect the current reality.

Signed-off-by: Lucas De Marchi 
---
 drivers/gpu/drm/i915/gt/intel_workarounds.c | 87 +
 1 file changed, 56 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c 
b/drivers/gpu/drm/i915/gt/intel_workarounds.c
index 3cdf5c24dbc5..0db3713c1beb 100644
--- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
@@ -17,43 +17,68 @@
 /**
  * DOC: Hardware workarounds
  *
- * This file is intended as a central place to implement most [1]_ of the
- * required workarounds for hardware to work as originally intended. They fall
- * in five basic categories depending on how/when they are applied:
+ * This is intended as a central place to implement most [1]_ of the
+ * required workarounds for hardware to work as originally intended. Hardware
+ * workarounds are register programming documented to be executed in the driver
+ * that fall outside of the normal programming sequences for a platform. There
+ * are some basic categories of workarounds, depending on how/when they are
+ * applied:
  *
- * - Workarounds that touch registers that are saved/restored to/from the HW
- *   context image. The list is emitted (via Load Register Immediate commands)
- *   everytime a new context is created.
- * - GT workarounds. The list of these WAs is applied whenever these registers
- *   revert to default values (on GPU reset, suspend/resume [2]_, etc..).
- * - Display workarounds. The list is applied during display clock-gating
- *   initialization.
- * - Workarounds that whitelist a privileged register, so that UMDs can manage
- *   them directly. This is just a special case of a MMMIO workaround (as we
- *   write the list of these to/be-whitelisted registers to some special HW
- *   registers).
- * - Workaround batchbuffers, that get executed automatically by the hardware
- *   on every HW context restore.
+ * - Context workarounds: workarounds that touch registers that are
+ *   saved/restored to/from the HW context image. The list is emitted (via Load
+ *   Register Immediate commands) once when initializing the device and saved 
in
+ *   the default context. That default context is then used on every context
+ *   creation to have a "primed golden context", i.e. a context image that
+ *   already contains the changes needed to all the registers.
  *
- * .. [1] Please notice that there are other WAs that, due to their nature,
- *cannot be applied from a central place. Those are peppered around the 
rest
- *of the code, as needed.
+ * - Engine workarounds: the list of these WAs is applied whenever the specific
+ *   engine is reset. It's also possible that a set of engine classes share a
+ *   common power domain and they are reset together. This happens on some
+ *   platforms with render and compute engines. In this case (at least) one of
+ *   them need to keeep the workaround programming: the approach taken in the
+ *   driver is to tie those workarounds to the first compute/render engine that
+ *   is registered.  When executing with GuC submission, engine resets are
+ *   outside of kernel driver control, hence the list of registers involved in
+ *   written once, on engine initialization, and then passed to GuC, that
+ *   saves/restores their values before/after the reset takes place. See
+ *   ``drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c`` for reference.
  *
- * .. [2] Technically, some registers are powercontext saved & restored, so 
they
- *survive a suspend/resume. In practice, writing them again is not too
- *costly and simplifies things. We can revisit this in the future.
+ * - GT workarounds: the list of these WAs is applied whenever these registers
+ *   revert to their default values: on GPU reset, suspend/resume, etc.
  *
- * Layout
- * ~~
+ * - Register whitelist: some workarounds need to be implemented in userspace,
+ *   but need to touch privileged registers. The whitelist in the kernel
+ *   instructs the hardware to allow the access to happen. From the kernel 
side,
+ *   this is just a special case of a MMIO workaround (as we write the list of
+ *   these to/be-whitelisted registers to some special HW registers).
  *
- * Keep things in this file ordered by WA type, as per the above (context, GT,
- * display, register whitelist, batchbuffer). Then, inside each type, keep the
- * following order:
+ * - Workaround batchbuffers: buffers that get executed automatically by the
+ *   hardware on every HW context restore. These buffers are created and
+ *   programmed in the default context so the hardware always go through those
+ *   programming sequences when switching contexts. The support for workaround
+ *   batchbuffers is enabled these hardware mechanisms:
  *
- * - Infrastructure functions and macros
- * - WAs per platfor

Re: [Freedreno] [RFC PATCH 0/3] Support for Solid Fill Planes

2022-11-07 Thread Jessica Zhang

On 11/7/2022 2:09 PM, Rob Clark wrote:

On Mon, Nov 7, 2022 at 1:32 PM Jessica Zhang wrote:

On 11/7/2022 11:37 AM, Ville Syrjälä wrote:

On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote:

Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT
properties. When the color fill value is set, and the framebuffer is set
to NULL, memory fetch will be disabled.

Thinking a bit more universally I wonder if there should be
some kind of enum property:

enum plane_pixel_source {
FB,
COLOR,
LIVE_FOO,
LIVE_BAR,
...
}

Hi Ville,

Makes sense -- this way, we'd also lay some groundwork for cases where
drivers want to use other non-FB sources.

In addition, loosen the NULL FB checks within the atomic commit callstack
to allow a NULL FB when color_fill is nonzero and add FB checks in
methods where the FB was previously assumed to be non-NULL.

Finally, have the DPU driver use drm_plane_state.color_fill and
drm_plane_state.color_fill_format instead of dpu_plane_state.color_fill,
and add extra checks in the DPU atomic commit callstack to account for a
NULL FB in cases where color_fill is set.

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.

Userspace can set the color_fill value by setting COLOR_FILL_FORMAT to a
DRM format, setting COLOR_FILL to a color fill value, and setting the
framebuffer to NULL.

Is there some real reason for the format property? Ie. why not
just do what was the plan for the crttc background color and
specify the color in full 16bpc format and just pick as many
msbs from that as the hw can use?

The format property was added because we can't assume that all hardware
will support/use the same color format for solid fill planes. Even for
just MSM devices, the hardware supports different variations of RGB
formats [1].

Sure, but the driver can convert the format into whatever the hw
wants. A 1x1 color conversion is not going to be problematic ;-)

Hi Rob,

Hm... that's also a fair point. Just wondering, is there any advantage
of having the driver convert the format, other than not having to
implement an extra format property?

(In case we end up wrapping everything into a prop blob or something)

Thanks,

Jessica Zhang

BR,
-R

Thanks,

Jessica Zhang

[1]
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c#L697

Jessica Zhang (3):
drm: Introduce color fill properties for drm plane
drm: Adjust atomic checks for solid fill color
drm/msm/dpu: Use color_fill property for DPU planes

--
2.38.0

--
Ville Syrjälä
Intel

Re: bug report with amdgpu drm-next-6.2

2022-11-07 Thread Dennis Gilmore

appending amdgpu.runpm=0 to the bootargs, it was a suggestion as I
have to append pcie_aspm=off. I get further, and despite a lot of
noise X starts

[   19.501645] [drm] amdgpu kernel modesetting enabled.
[   19.515932] amdgpu: CRAT table disabled by module option
[   19.522205] amdgpu: IO link not available for non x86 platforms
[   19.528970] amdgpu: Virtual CRAT table created for CPU
[   19.535049] amdgpu: Topology: Add CPU node
[   19.540687] amdgpu 000d:03:00.0: Adding to iommu group 23
[   19.547913] [drm] initializing kernel modesetting (BEIGE_GOBY
0x1002:0x743F 0x1EAE:0x6401 0xC1).
[   19.557672] [drm] register mmio base: 0x5000
[   19.563139] [drm] register mmio size: 1048576
[   19.570124] [drm] add ip block number 0 
[   19.575836] [drm] add ip block number 1 
[   19.581510] [drm] add ip block number 2 
[   19.586184] ixgbe 0005:01:00.0 enP5p1s0f0: NIC Link is Up 10 Gbps,
Flow Control: RX/TX
[   19.586383] [drm] add ip block number 3 
[   19.595028] IPv6: ADDRCONF(NETDEV_CHANGE): enP5p1s0f0: link becomes ready
[   19.598635] [drm] add ip block number 4 
[   19.613779] [drm] add ip block number 5 
[   19.618939] [drm] add ip block number 6 
[   19.624622] [drm] add ip block number 7 
[   19.630292] [drm] add ip block number 8 
[   19.635887] amdgpu 000d:03:00.0: amdgpu: Fetched VBIOS from VFCT
[   19.642653] amdgpu: ATOM BIOS: 113-24X46SHB1-D02
[   19.648005] [drm] VCN(0) decode is enabled in VM mode
[   19.653811] amdgpu 000d:03:00.0: amdgpu: Trusted Memory Zone (TMZ)
feature disabled as experimental (default)
[   19.664520] amdgpu 000d:03:00.0: amdgpu: PCIE atomic ops is not supported
[   19.672121] [drm] vm size is 262144 GB, 4 levels, block size is
9-bit, fragment size is 9-bit
[   19.681487] amdgpu 000d:03:00.0: BAR 2: releasing [mem
0x34001000-0x3400101f 64bit pref]
[   19.691081] amdgpu 000d:03:00.0: BAR 0: releasing [mem
0x3400-0x34000fff 64bit pref]
[   19.700731] pcieport 000d:02:00.0: BAR 15: releasing [mem
0x3400-0x340017ff 64bit pref]
[   19.710586] pcieport 000d:01:00.0: BAR 15: releasing [mem
0x3400-0x340017ff 64bit pref]
[   19.720388] pcieport 000d:00:01.0: BAR 15: releasing [mem
0x3400-0x340017ff 64bit pref]
[   19.730217] pcieport 000d:00:01.0: bridge window [io
0x1000-0x0fff] to [bus 01-03] add_size 1000
[   19.739849] pcieport 000d:00:01.0: BAR 15: assigned [mem
0x3400-0x34017fff 64bit pref]
[   19.749572] pcieport 000d:00:01.0: BAR 13: no space for [io  size 0x1000]
[   19.757117] pcieport 000d:00:01.0: BAR 13: failed to assign [io  size 0x1000]
[   19.764984] pcieport 000d:00:01.0: BAR 13: no space for [io  size 0x1000]
[   19.772511] pcieport 000d:00:01.0: BAR 13: failed to assign [io  size 0x1000]
[   19.780396] pcieport 000d:01:00.0: BAR 15: assigned [mem
0x3400-0x34017fff 64bit pref]
[   19.790109] pcieport 000d:02:00.0: BAR 15: assigned [mem
0x3400-0x34017fff 64bit pref]
[   19.799826] amdgpu 000d:03:00.0: BAR 0: assigned [mem
0x3400-0x3400 64bit pref]
[   19.809289] amdgpu 000d:03:00.0: BAR 2: assigned [mem
0x3401-0x3401001f 64bit pref]
[   19.818742] pcieport 000d:00:01.0: PCI bridge to [bus 01-03]
[   19.825097] pcieport 000d:00:01.0:   bridge window [mem
0x5000-0x502f]
[   19.833055] pcieport 000d:00:01.0:   bridge window [mem
0x3400-0x34017fff 64bit pref]
[   19.842649] pcieport 000d:01:00.0: PCI bridge to [bus 02-03]
[   19.849028] pcieport 000d:01:00.0:   bridge window [mem
0x5000-0x501f]
[   19.856970] pcieport 000d:01:00.0:   bridge window [mem
0x3400-0x34017fff 64bit pref]
[   19.866567] pcieport 000d:02:00.0: PCI bridge to [bus 03]
[   19.872667] pcieport 000d:02:00.0:   bridge window [mem
0x5000-0x501f]
[   19.880613] pcieport 000d:02:00.0:   bridge window [mem
0x3400-0x34017fff 64bit pref]
[   19.890238] amdgpu 000d:03:00.0: amdgpu: VRAM: 4080M
0x0080 - 0x0080FEFF (4080M used)
[   19.900544] amdgpu 000d:03:00.0: amdgpu: GART: 512M
0x - 0x1FFF
[   19.909620] amdgpu 000d:03:00.0: amdgpu: AGP: 267894784M
0x0084 - 0x
[   19.919143] [drm] Detected VRAM RAM=4080M, BAR=4096M
[   19.924802] [drm] RAM width 64bits GDDR6
[   19.929508] [drm] amdgpu: 4080M of VRAM memory ready
[   19.935186] [drm] amdgpu: 31947M of GTT memory ready.
[   19.940974] [drm] GART: num cpu pages 131072, num gpu pages 131072
[   19.948004] [drm] PCIE GART of 512M enabled (table at 0x00800030).
[   19.965796] amdgpu 000d:03:00.0: amdgpu: PSP runtime database doesn't exist
[   19.973538] amdgpu 000d:03:00.0: amdgpu: PSP runtime database doesn't exist
[   21.665671] amdgpu 000d:03:00.0: amdgpu: STB initialized to 2048 entries
[   21.706368] [drm] Loading DMUB firmware via PSP: version=0x02020013
[   21.766108] [drm] use_doorbell being set to: [true]
[   21.836500] [drm] Found VCN firmware Version ENC: 1.24 DEC: 2 VEP:
0 Revision:

Re: [PATCH 4/8] pwm: atmel-hlcdc: fix struct clk pointer comparing

2022-11-07 Thread Cc Carnaghi

Mr

Re: [PATCH 2/2] drm: Convert users of drm_of_component_match_add to component_match_add_of

2022-11-07 Thread sarha

November 3, 2022 at 8:22 PM, "Sean Anderson" mailto:sean.ander...@seco.com?to=%22Sean%20Anderson%22%20%3Csean.anderson%40seco.com%3E
 > wrote:

> 
> Every user of this function either uses component_compare_of or
> something equivalent. Most of them immediately put the device node as
> well. Convert these users to component_match_add_of and remove
> drm_of_component_match_add.
> 
> Signed-off-by: Sean Anderson 

Acked-by: Jyri Sarha 

Also tested that Beaglebone-Black HDMI audio, that relies on componet system, 
still works. So for tilcdc:

Tested-by: Jyri Sarha 

> ---
> 
> .../gpu/drm/arm/display/komeda/komeda_drv.c | 6 ++--
> drivers/gpu/drm/arm/hdlcd_drv.c | 9 +-
> drivers/gpu/drm/arm/malidp_drv.c | 11 +--
> drivers/gpu/drm/armada/armada_drv.c | 10 ---
> drivers/gpu/drm/drm_of.c | 29 +++
> drivers/gpu/drm/etnaviv/etnaviv_drv.c | 4 +--
> .../gpu/drm/hisilicon/kirin/kirin_drm_drv.c | 3 +-
> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 3 +-
> drivers/gpu/drm/mediatek/mtk_drm_drv.c | 4 +--
> drivers/gpu/drm/msm/msm_drv.c | 14 -
> drivers/gpu/drm/sti/sti_drv.c | 3 +-
> drivers/gpu/drm/sun4i/sun4i_drv.c | 3 +-
> drivers/gpu/drm/tilcdc/tilcdc_external.c | 10 ++-
> include/drm/drm_of.h | 12 
> 14 files changed, 33 insertions(+), 88 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c 
> b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> index 9fce4239d4ad..e5bf439b799f 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> @@ -103,10 +103,8 @@ static void komeda_add_slave(struct device *master,
>  struct device_node *remote;
> 
>  remote = of_graph_get_remote_node(np, port, endpoint);
> - if (remote) {
> - drm_of_component_match_add(master, match, component_compare_of, remote);
> - of_node_put(remote);
> - }
> + if (remote)
> + component_match_add_of(master, match, remote);
> }
> 
> static int komeda_platform_probe(struct platform_device *pdev)
> diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
> index a032003c340c..18e58863a2f1 100644
> --- a/drivers/gpu/drm/arm/hdlcd_drv.c
> +++ b/drivers/gpu/drm/arm/hdlcd_drv.c
> @@ -352,11 +352,6 @@ static const struct component_master_ops 
> hdlcd_master_ops = {
>  .unbind = hdlcd_drm_unbind,
> };
> 
> -static int compare_dev(struct device *dev, void *data)
> -{
> - return dev->of_node == data;
> -}
> -
> static int hdlcd_probe(struct platform_device *pdev)
> {
>  struct device_node *port;
> @@ -367,9 +362,7 @@ static int hdlcd_probe(struct platform_device *pdev)
>  if (!port)
>  return -ENODEV;
> 
> - drm_of_component_match_add(&pdev->dev, &match, compare_dev, port);
> - of_node_put(port);
> -
> + component_match_add_of(&pdev->dev, &match, port);
>  return component_master_add_with_match(&pdev->dev, &hdlcd_master_ops,
>  match);
> }
> diff --git a/drivers/gpu/drm/arm/malidp_drv.c 
> b/drivers/gpu/drm/arm/malidp_drv.c
> index 1d0b0c54ccc7..aace8b87c6d3 100644
> --- a/drivers/gpu/drm/arm/malidp_drv.c
> +++ b/drivers/gpu/drm/arm/malidp_drv.c
> @@ -926,13 +926,6 @@ static const struct component_master_ops 
> malidp_master_ops = {
>  .unbind = malidp_unbind,
> };
> 
> -static int malidp_compare_dev(struct device *dev, void *data)
> -{
> - struct device_node *np = data;
> -
> - return dev->of_node == np;
> -}
> -
> static int malidp_platform_probe(struct platform_device *pdev)
> {
>  struct device_node *port;
> @@ -946,9 +939,7 @@ static int malidp_platform_probe(struct platform_device 
> *pdev)
>  if (!port)
>  return -ENODEV;
> 
> - drm_of_component_match_add(&pdev->dev, &match, malidp_compare_dev,
> - port);
> - of_node_put(port);
> + component_match_add_of(&pdev->dev, &match, port);
>  return component_master_add_with_match(&pdev->dev, &malidp_master_ops,
>  match);
> }
> diff --git a/drivers/gpu/drm/armada/armada_drv.c 
> b/drivers/gpu/drm/armada/armada_drv.c
> index 0643887800b4..c0211ad7a45d 100644
> --- a/drivers/gpu/drm/armada/armada_drv.c
> +++ b/drivers/gpu/drm/armada/armada_drv.c
> @@ -184,10 +184,12 @@ static void armada_add_endpoints(struct device *dev,
> 
>  for_each_endpoint_of_node(dev_node, ep) {
>  remote = of_graph_get_remote_port_parent(ep);
> - if (remote of_device_is_available(remote))
> - drm_of_component_match_add(dev, match, component_compare_of,
> - remote);
> - of_node_put(remote);
> + if (remote) {
> + if (of_device_is_available(remote))
> + component_match_add_of(dev, match, remote);
> + else
> + of_node_put(remote);
> + }
>  }
> }
> 
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index 7bbcb999bb75..0a474729ddf6 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -78,24 +78,6 @@ uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
> }
> EXPORT_SYMBOL(drm_of_find_possible_crtcs);
> 
> -/**
> - * drm_of_component_match_add - Add a component helper OF node match rule
> - * @master: master device
> - * @matchptr: component match

[PATCH] drm: Add orientation quirk for DynaBook K50

2022-11-07 Thread Allen Ballway

Panel is 800x1280 but mounted on a detachable form factor sidways.

Signed-off-by: Allen Ballway 
---

 .../gpu/drm/drm_panel_orientation_quirks.c| 20 ---
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_panel_orientation_quirks.c 
b/drivers/gpu/drm/drm_panel_orientation_quirks.c
index fc1728d46ac2a..8c4b007081730 100644
--- a/drivers/gpu/drm/drm_panel_orientation_quirks.c
+++ b/drivers/gpu/drm/drm_panel_orientation_quirks.c
@@ -30,12 +30,6 @@ struct drm_dmi_panel_orientation_data {
int orientation;
 };

-static const struct drm_dmi_panel_orientation_data asus_t100ha = {
-   .width = 800,
-   .height = 1280,
-   .orientation = DRM_MODE_PANEL_ORIENTATION_LEFT_UP,
-};
-
 static const struct drm_dmi_panel_orientation_data gpd_micropc = {
.width = 720,
.height = 1280,
@@ -115,6 +109,12 @@ static const struct drm_dmi_panel_orientation_data 
lcd1280x1920_rightside_up = {
.orientation = DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
 };

+static const struct drm_dmi_panel_orientation_data lcd800x1280_leftside_up = {
+   .width = 800,
+   .height = 1280,
+   .orientation = DRM_MODE_PANEL_ORIENTATION_LEFT_UP,
+};
+
 static const struct drm_dmi_panel_orientation_data lcd1600x2560_leftside_up = {
.width = 1600,
.height = 2560,
@@ -133,7 +133,7 @@ static const struct dmi_system_id orientation_data[] = {
  DMI_EXACT_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
  DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "T100HAN"),
},
-   .driver_data = (void *)&asus_t100ha,
+   .driver_data = (void *)&lcd800x1280_leftside_up,
}, {/* Asus T101HA */
.matches = {
  DMI_EXACT_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
@@ -172,6 +172,12 @@ static const struct dmi_system_id orientation_data[] = {
  DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Hi10 pro tablet"),
},
.driver_data = (void *)&lcd1200x1920_rightside_up,
+   }, {/* Dynabook K50 */
+   .matches = {
+ DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Dynabook Inc."),
+ DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "dynabook K50/FR"),
+   },
+   .driver_data = (void *)&lcd800x1280_leftside_up,
}, {/* GPD MicroPC (generic strings, also match on bios date) */
.matches = {
  DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Default string"),
--
2.38.1.431.g37b22c650d-goog

Re: [PATCH 21/26] drm: tilcdc: Remove #ifdef guards for PM related functions

2022-11-07 Thread sarha

November 7, 2022 at 7:52 PM, "Paul Cercueil" mailto:p...@crapouillou.net?to=%22Paul%20Cercueil%22%20%3Cpaul%40crapouillou.net%3E
 > wrote:

> 
> Use the DEFINE_SIMPLE_DEV_PM_OPS() and pm_sleep_ptr() macros to handle
> the .suspend/.resume callbacks.
> 
> These macros allow the suspend and resume functions to be automatically
> dropped by the compiler when CONFIG_SUSPEND is disabled, without having
> to use #ifdef guards.
> 
> This has the advantage of always compiling these functions in,
> independently of any Kconfig option. Thanks to that, bugs and other
> regressions are subsequently easier to catch.
> 
> Signed-off-by: Paul Cercueil 

Acked-by: Jyri Sarha 

> ---
> Cc: Jyri Sarha 
> Cc: Tomi Valkeinen 
> ---
> drivers/gpu/drm/tilcdc/tilcdc_drv.c | 9 +++--
> 1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c 
> b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> index f72755b8ea14..cd5bdc2f803a 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> @@ -496,7 +496,6 @@ static const struct drm_driver tilcdc_driver = {
> * Power management:
> */
> 
> -#ifdef CONFIG_PM_SLEEP
> static int tilcdc_pm_suspend(struct device *dev)
> {
>  struct drm_device *ddev = dev_get_drvdata(dev);
> @@ -518,11 +517,9 @@ static int tilcdc_pm_resume(struct device *dev)
>  pinctrl_pm_select_default_state(dev);
>  return drm_mode_config_helper_resume(ddev);
> }
> -#endif
> 
> -static const struct dev_pm_ops tilcdc_pm_ops = {
> - SET_SYSTEM_SLEEP_PM_OPS(tilcdc_pm_suspend, tilcdc_pm_resume)
> -};
> +static DEFINE_SIMPLE_DEV_PM_OPS(tilcdc_pm_ops,
> + tilcdc_pm_suspend, tilcdc_pm_resume);
> 
> /*
> * Platform driver:
> @@ -597,7 +594,7 @@ static struct platform_driver tilcdc_platform_driver = {
>  .remove = tilcdc_pdev_remove,
>  .driver = {
>  .name = "tilcdc",
> - .pm = &tilcdc_pm_ops,
> + .pm = pm_sleep_ptr(&tilcdc_pm_ops),
>  .of_match_table = tilcdc_of_match,
>  },
> };
> -- 
> 2.35.1
>

[PATCH v2 03/18] dt-bindings: msm: dsi-controller-main: Add vdd* descriptions back in

When converting from .txt to .yaml we didn't include descriptions for the
existing regulator supplies.

- vdd
- vdda
- vddio

Add those descriptions into the yaml now as they were prior to the
conversion. Mark the supplies as required as was previously the case in the
.txt implementation.

Warnings about missing regulators can be resolved by updating the relevant
dtsi files to point to fixed always-on regulators where appropriate.

Fixes: 4dbe55c97741 ("dt-bindings: msm: dsi: add yaml schemas for DSI bindings")
Cc: Rob Clark 
Cc: Abhinav Kumar 
Cc: Dmitry Baryshkov 
Cc: Sean Paul 
Cc: David Airlie 
Cc: Daniel Vetter 
Cc: Rob Herring 
Cc: Krzysztof Kozlowski 
Cc: linux-arm-...@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: freedr...@lists.freedesktop.org
Cc: devicet...@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
Signed-off-by: Bryan O'Donoghue 
---
 .../bindings/display/msm/dsi-controller-main.yaml | 15 +++
 1 file changed, 15 insertions(+)

diff --git 
a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml 
b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
index cf782c5f5bdb0..0f7747e55b9be 100644
--- a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
+++ b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
@@ -124,6 +124,18 @@ properties:
   - port@0
   - port@1
 
+  vdd-supply:
+description:
+  Phandle to vdd regulator device node
+
+  vddio-supply:
+description:
+  Phandle to vdd-io regulator device node
+
+  vdda-supply:
+description:
+  Phandle to vdda regulator device node
+
 required:
   - compatible
   - reg
@@ -135,6 +147,9 @@ required:
   - assigned-clocks
   - assigned-clock-parents
   - ports
+  - vdd-supply
+  - vddio-supply
+  - vdda-supply
 
 additionalProperties: false
 
-- 
2.38.1

[PATCH v2 06/18] dt-bindings: msm: dsi-controller-main: Alphanumerically sort compatible enum

Sort the order of the compatible strings alphanumerically.

Cc: Rob Clark 
Cc: Abhinav Kumar 
Cc: Dmitry Baryshkov 
Cc: Sean Paul 
Cc: David Airlie 
Cc: Daniel Vetter 
Cc: Rob Herring 
Cc: Krzysztof Kozlowski 
Cc: linux-arm-...@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: freedr...@lists.freedesktop.org
Cc: devicet...@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
Signed-off-by: Bryan O'Donoghue 
---
 .../devicetree/bindings/display/msm/dsi-controller-main.yaml| 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git 
a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml 
b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
index a607ccd4a905a..b35130a77b43e 100644
--- a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
+++ b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
@@ -15,8 +15,8 @@ allOf:
 properties:
   compatible:
 enum:
-  - qcom,mdss-dsi-ctrl
   - qcom,dsi-ctrl-6g-qcm2290
+  - qcom,mdss-dsi-ctrl
 
   reg:
 maxItems: 1
-- 
2.38.1

[PATCH v2 05/18] dt-bindings: msm: dsi-controller-main: Fix description of core clock

There's a typo in describing the core clock as an 'escape' clock. The
accurate description is 'core'.

Fixes: 4dbe55c97741 ("dt-bindings: msm: dsi: add yaml schemas for DSI bindings")
Cc: Rob Clark 
Cc: Abhinav Kumar 
Cc: Dmitry Baryshkov 
Cc: Sean Paul 
Cc: David Airlie 
Cc: Daniel Vetter 
Cc: Rob Herring 
Cc: Krzysztof Kozlowski 
Cc: linux-arm-...@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: freedr...@lists.freedesktop.org
Cc: devicet...@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
Signed-off-by: Bryan O'Donoghue 
---
 .../devicetree/bindings/display/msm/dsi-controller-main.yaml| 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git 
a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml 
b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
index cab38a20a54b0..a607ccd4a905a 100644
--- a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
+++ b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
@@ -32,7 +32,7 @@ properties:
   - description: Display byte clock
   - description: Display byte interface clock
   - description: Display pixel clock
-  - description: Display escape clock
+  - description: Display core clock
   - description: Display AHB clock
   - description: Display AXI clock
   - description: Core MultiMedia SubSystem clock
-- 
2.38.1

[PATCH v2 04/18] dt-bindings: msm: dsi-controller-main: Fix clock declarations

When converting from .txt to .yaml dt-binding descriptions we appear to
have missed some of the previous detail on the number and names of
permissible clocks.

Fixes: 4dbe55c97741 ("dt-bindings: msm: dsi: add yaml schemas for DSI bindings")
Reviewed-by: Dmitry Baryshkov 
Cc: Rob Clark 
Cc: Abhinav Kumar 
Cc: Dmitry Baryshkov 
Cc: Sean Paul 
Cc: David Airlie 
Cc: Daniel Vetter 
Cc: Rob Herring 
Cc: Krzysztof Kozlowski 
Cc: linux-arm-...@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: freedr...@lists.freedesktop.org
Cc: devicet...@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
Signed-off-by: Bryan O'Donoghue 
---
 .../bindings/display/msm/dsi-controller-main.yaml | 8 
 1 file changed, 8 insertions(+)

diff --git 
a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml 
b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
index 0f7747e55b9be..cab38a20a54b0 100644
--- a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
+++ b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
@@ -35,6 +35,10 @@ properties:
   - description: Display escape clock
   - description: Display AHB clock
   - description: Display AXI clock
+  - description: Core MultiMedia SubSystem clock
+  - description: MDP Core clock
+  - description: MNOC clock
+minItems: 6
 
   clock-names:
 items:
@@ -44,6 +48,10 @@ properties:
   - const: core
   - const: iface
   - const: bus
+  - const: core_mmss
+  - const: mdp_core
+  - const: mnoc
+minItems: 6
 
   phys:
 maxItems: 1
-- 
2.38.1

[PATCH v2 07/18] dt-bindings: msm: dsi-controller-main: Add compatible strings for every current SoC

Currently we do not differentiate between the various users of the
qcom,mdss-dsi-ctrl. The driver is flexible enough to operate from one
compatible string but, the hardware does have some significant differences
in the number of clocks.

To facilitate documenting the clocks add the following compatible strings

- qcom,mdss-dsi-ctrl-apq8064
- qcom,mdss-dsi-ctrl-msm8916
- qcom,mdss-dsi-ctrl-msm8974
- qcom,mdss-dsi-ctrl-msm8996
- qcom,mdss-dsi-ctrl-sc7180
- qcom,mdss-dsi-ctrl-sc7280
- qcom,mdss-dsi-ctrl-sdm630
- qcom,mdss-dsi-ctrl-sdm660
- qcom,mdss-dsi-ctrl-sdm845
- qcom,mdss-dsi-ctrl-sm8250

Cc: Rob Clark 
Cc: Abhinav Kumar 
Cc: Dmitry Baryshkov 
Cc: Sean Paul 
Cc: David Airlie 
Cc: Daniel Vetter 
Cc: Rob Herring 
Cc: Krzysztof Kozlowski 
Cc: linux-arm-...@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: freedr...@lists.freedesktop.org
Cc: devicet...@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
Signed-off-by: Bryan O'Donoghue 
---
 .../bindings/display/msm/dsi-controller-main.yaml  | 10 ++
 1 file changed, 10 insertions(+)

diff --git 
a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml 
b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
index b35130a77b43e..9db3e63acda3d 100644
--- a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
+++ b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
@@ -17,6 +17,16 @@ properties:
 enum:
   - qcom,dsi-ctrl-6g-qcm2290
   - qcom,mdss-dsi-ctrl
+  - qcom,mdss-dsi-ctrl-apq8064
+  - qcom,mdss-dsi-ctrl-msm8916
+  - qcom,mdss-dsi-ctrl-msm8974
+  - qcom,mdss-dsi-ctrl-msm8996
+  - qcom,mdss-dsi-ctrl-sc7180
+  - qcom,mdss-dsi-ctrl-sc7280
+  - qcom,mdss-dsi-ctrl-sdm630
+  - qcom,mdss-dsi-ctrl-sdm660
+  - qcom,mdss-dsi-ctrl-sdm845
+  - qcom,mdss-dsi-ctrl-sm8250
 
   reg:
 maxItems: 1
-- 
2.38.1

[PATCH v2 08/18] dt-bindings: msm: dsi-controller-main: Document clocks on a per compatible basis

Each compatible has a different set of clocks which are associated with it.
Add in the list of clocks for each compatible.

Cc: Rob Clark 
Cc: Abhinav Kumar 
Cc: Dmitry Baryshkov 
Cc: Sean Paul 
Cc: David Airlie 
Cc: Daniel Vetter 
Cc: Rob Herring 
Cc: Krzysztof Kozlowski 
Cc: linux-arm-...@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: freedr...@lists.freedesktop.org
Cc: devicet...@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
Signed-off-by: Bryan O'Donoghue 
---
 .../display/msm/dsi-controller-main.yaml  | 177 +++---
 1 file changed, 150 insertions(+), 27 deletions(-)

diff --git 
a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml 
b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
index 9db3e63acda3d..c975df0ca22fc 100644
--- a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
+++ b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
@@ -9,24 +9,22 @@ title: Qualcomm Display DSI controller
 maintainers:
   - Krishna Manikandan 
 
-allOf:
-  - $ref: "../dsi-controller.yaml#"
-
 properties:
   compatible:
-enum:
-  - qcom,dsi-ctrl-6g-qcm2290
-  - qcom,mdss-dsi-ctrl
-  - qcom,mdss-dsi-ctrl-apq8064
-  - qcom,mdss-dsi-ctrl-msm8916
-  - qcom,mdss-dsi-ctrl-msm8974
-  - qcom,mdss-dsi-ctrl-msm8996
-  - qcom,mdss-dsi-ctrl-sc7180
-  - qcom,mdss-dsi-ctrl-sc7280
-  - qcom,mdss-dsi-ctrl-sdm630
-  - qcom,mdss-dsi-ctrl-sdm660
-  - qcom,mdss-dsi-ctrl-sdm845
-  - qcom,mdss-dsi-ctrl-sm8250
+items:
+  - enum:
+  - qcom,dsi-ctrl-6g-qcm2290
+  - qcom,mdss-dsi-ctrl-apq8064
+  - qcom,mdss-dsi-ctrl-msm8916
+  - qcom,mdss-dsi-ctrl-msm8974
+  - qcom,mdss-dsi-ctrl-msm8996
+  - qcom,mdss-dsi-ctrl-sc7180
+  - qcom,mdss-dsi-ctrl-sc7280
+  - qcom,mdss-dsi-ctrl-sdm630
+  - qcom,mdss-dsi-ctrl-sdm660
+  - qcom,mdss-dsi-ctrl-sdm845
+  - qcom,mdss-dsi-ctrl-sm8250
+  - const: qcom,mdss-dsi-ctrl
 
   reg:
 maxItems: 1
@@ -51,17 +49,8 @@ properties:
 minItems: 6
 
   clock-names:
-items:
-  - const: byte
-  - const: byte_intf
-  - const: pixel
-  - const: core
-  - const: iface
-  - const: bus
-  - const: core_mmss
-  - const: mdp_core
-  - const: mnoc
 minItems: 6
+maxItems: 9
 
   phys:
 maxItems: 1
@@ -169,6 +158,140 @@ required:
   - vddio-supply
   - vdda-supply
 
+allOf:
+  - $ref: "../dsi-controller.yaml#"
+  - if:
+  properties:
+compatible:
+  contains:
+enum:
+  - qcom,mdss-dsi-ctrl-apq8064
+then:
+  properties:
+clocks:
+  maxItems: 7
+clock-names:
+  items:
+- const: iface
+- const: bus
+- const: core_mmss
+- const: src
+- const: byte
+- const: pixel
+- const: core
+  - if:
+  properties:
+compatible:
+  contains:
+enum:
+  - qcom,mdss-dsi-ctrl-msm8916
+then:
+  properties:
+clocks:
+  maxItems: 6
+clock-names:
+  items:
+- const: mdp_core
+- const: iface
+- const: bus
+- const: byte
+- const: pixel
+- const: core
+  - if:
+  properties:
+compatible:
+  contains:
+enum:
+  - qcom,mdss-dsi-ctrl-msm8974
+then:
+  properties:
+clocks:
+  maxItems: 3
+clock-names:
+  items:
+- const: iface
+- const: bus
+- const: vsync
+  - if:
+  properties:
+compatible:
+  contains:
+enum:
+  - qcom,mdss-dsi-ctrl-msm8996
+then:
+  properties:
+clocks:
+  maxItems: 7
+clock-names:
+  items:
+- const: mdp_core
+- const: byte
+- const: iface
+- const: bus
+- const: core_mmss
+- const: pixel
+- const: core
+  - if:
+  properties:
+compatible:
+  contains:
+enum:
+  - qcom,mdss-dsi-ctrl-sc7180
+  - qcom,mdss-dsi-ctrl-sc7280
+  - qcom,mdss-dsi-ctrl-sm8250
+then:
+  properties:
+clocks:
+  maxItems: 6
+clock-names:
+  items:
+- const: byte
+- const: byte_intf
+- const: pixel
+- const: core
+- const: iface
+- const: bus
+  - if:
+  properties:
+compatible:
+  contains:
+enum:
+  - qcom,mdss-dsi-ctrl-sdm630
+  - qcom,mdss-dsi-ctrl-sdm660
+then:
+  properties:
+clocks:
+  maxItems: 9
+clock-names:
+  items:
+- const: mdp_core
+- const: byte
+

[PATCH v2 02/18] dt-bindings: msm: dsi-controller-main: Fix power-domain constraint

power-domain is required for the sc7180 dispcc GDSC but not every qcom SoC
has a similar dependency for example the aqp8064.

Most Qcom SoC's using mdss-dsi-ctrl seem to have the ability to
power-collapse the MDP without collapsing DSI.

For example the qcom vendor kernel commit for apq8084, msm8226, msm8916, msm8974

https://review.carbonrom.org/plugins/gitiles/CarbonROM/android_kernel_oneplus_msm8994/+/7b5c011a770daa2811778937ed646237a28a8694

"ARM: dts: msm: add mdss gdsc supply to dsi controller device

 It is possible for the DSI controller to be active when MDP is
 power collapsed. DSI controller needs to have it's own vote for
 mdss gdsc to ensure that gdsc remains on in such cases."

This however doesn't appear to be the case for the apq8064 so we shouldn't
be marking power-domain as required in yaml checks.

Fixes: 4dbe55c97741 ("dt-bindings: msm: dsi: add yaml schemas for DSI bindings")
Reviewed-by: Dmitry Baryshkov 
Cc: Rob Clark 
Cc: Abhinav Kumar 
Cc: Dmitry Baryshkov 
Cc: Sean Paul 
Cc: David Airlie 
Cc: Daniel Vetter 
Cc: Rob Herring 
Cc: Krzysztof Kozlowski 
Cc: linux-arm-...@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: freedr...@lists.freedesktop.org
Cc: devicet...@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
Signed-off-by: Bryan O'Donoghue 
---
 .../devicetree/bindings/display/msm/dsi-controller-main.yaml | 1 -
 1 file changed, 1 deletion(-)

diff --git 
a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml 
b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
index 27ebfd5ffb22f..cf782c5f5bdb0 100644
--- a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
+++ b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
@@ -134,7 +134,6 @@ required:
   - phys
   - assigned-clocks
   - assigned-clock-parents
-  - power-domains
   - ports
 
 additionalProperties: false
-- 
2.38.1

[PATCH v2 01/18] dt-bindings: msm: dsi-controller-main: Fix operating-points-v2 constraint

The existing msm8916.dtsi does not depend on nor require operating points.

Fixes: 4dbe55c97741 ("dt-bindings: msm: dsi: add yaml schemas for DSI bindings")
Reviewed-by: Dmitry Baryshkov 
Acked-by: Krzysztof Kozlowski 
Cc: Rob Clark 
Cc: Abhinav Kumar 
Cc: Dmitry Baryshkov 
Cc: Sean Paul 
Cc: David Airlie 
Cc: Daniel Vetter 
Cc: Rob Herring 
Cc: Krzysztof Kozlowski 
Cc: linux-arm-...@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: freedr...@lists.freedesktop.org
Cc: devicet...@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
Signed-off-by: Bryan O'Donoghue 
---
 .../devicetree/bindings/display/msm/dsi-controller-main.yaml | 1 -
 1 file changed, 1 deletion(-)

diff --git 
a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml 
b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
index 7782bff89afc7..27ebfd5ffb22f 100644
--- a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
+++ b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
@@ -135,7 +135,6 @@ required:
   - assigned-clocks
   - assigned-clock-parents
   - power-domains
-  - operating-points-v2
   - ports
 
 additionalProperties: false
-- 
2.38.1

Re: [PATCH 06/10] vfio-iommufd: Allow iommufd to be used in place of a container fd

On Wed, Nov 02, 2022 at 03:28:20PM +0800, Yi Liu wrote:
> On 2022/10/26 02:50, Jason Gunthorpe wrote:
> > This makes VFIO_GROUP_SET_CONTAINER accept both a vfio container FD and an
> > iommufd.
> > 
> > In iommufd mode an IOAS will exist after the SET_CONTAINER, but it will
> > not be attached to any groups.
> 
> is there any special reason that we cannot attach the IOAS in the SET
> container phase or SET_IOMMU phase?

It is because iommufd has been deliberately made to work only on
struct device * not iommu_groups, and when we go to do the
SET_CONTAINER we have no idea what the device will be.

So defering the operation is the cleanest approach.

> >  From a VFIO perspective this means that the VFIO_GROUP_GET_STATUS and
> > VFIO_GROUP_FLAGS_VIABLE works subtly differently. With the container FD
> > the iommu_group_claim_dma_owner() is done during SET_CONTAINER but for
> > IOMMFD this is done during VFIO_GROUP_GET_DEVICE_FD. Meaning that
> 
> s/IOMMFD/IOMMUFD

Done

Jason

Re: [PATCH] drm/i915/mtl: Media GT and Render GT share common GGTT

2022-11-07 Thread Matt Roper

On Mon, Nov 07, 2022 at 07:43:59PM +0530, Iddamsetty, Aravind wrote:
> 
> 
> On 31-10-2022 23:16, Matt Roper wrote:
> > On Mon, Oct 31, 2022 at 06:01:11PM +0530, Aravind Iddamsetty wrote:
> >> On XE_LPM+ platforms the media engines are carved out into a separate
> >> GT but have a common GGTMMADR address range which essentially makes
> >> the GGTT address space to be shared between media and render GT.
> > 
> 
> 
> >>  
> >>  int intel_gt_init_mmio(struct intel_gt *gt)
> >> @@ -965,6 +973,9 @@ int intel_gt_tiles_init(struct drm_i915_private *i915)
> >>int ret;
> >>  
> >>for_each_gt(gt, i915, id) {
> >> +  if (GRAPHICS_VER(i915) >= 8)
> >> +  setup_private_pat(gt);
> >> +
> > 
> > Since the term "tile" is used for PVC-style remote tiles (which we have
> > some framework for, but haven't enabled yet), it seems confusing to have
> > the PAT setup for all GTs (including the standalone media GT) in a
> > function called intel_gt_tiles_init().  Maybe we should also have a prep
> > patch that renames this function if we're going to start doing non-tile
> > things in here too?
> 
> But isn't GT and Tile used interchangeably. Also, Could you please

The terminology has been used a bit inconsistently so far, but I think
we're trying to standardize on "tile" as referring to the PVC-style
combination of "GT + LMEM."  So I'd consider MTL's standalone media to
be a "GT," but not a "tile" because it isn't paired with its own lmem
unit.


Matt

> elaborate what do you mean by non tile related things here and as i
> understand PAT are per GT registers and in case of SA Media the
> gsi_offset get added.
> > 
> >>ret = intel_gt_probe_lmem(gt);
> >>if (ret)
> >>return ret;
> 
> 
> Thanks,
> Aravind.

-- 
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation

Re: KUnit issues - Was: [igt-dev] [PATCH RFC v2 8/8] drm/i915: check if current->mm is not NULL

2022-11-07 Thread Daniel Latypov

On Mon, Nov 7, 2022 at 10:38 AM Michał Winiarski
 wrote:
>
> On Thu, Nov 03, 2022 at 04:23:02PM +0100, Mauro Carvalho Chehab wrote:
> > Hi,
> >
> > I'm facing a couple of issues when testing KUnit with the i915 driver.
> >
> > The DRM subsystem and the i915 driver has, for a long time, his own
> > way to do unit tests, which seems to be added before KUnit.
> >
> > I'm now checking if it is worth start using KUnit at i915. So, I wrote
> > a RFC with some patches adding support for the tests we have to be
> > reported using Kernel TAP and KUnit.
> >
> > There are basically 3 groups of tests there:
> >
> > - mock tests - check i915 hardware-independent logic;
> > - live tests - run some hardware-specific tests;
> > - perf tests - check perf support - also hardware-dependent.
> >
> > As they depend on i915 driver, they run only on x86, with PCI
> > stack enabled, but the mock tests run nicely via qemu.
> >
> > The live and perf tests require a real hardware. As we run them
> > together with our CI, which, among other things, test module
> > unload/reload and test loading i915 driver with different
> > modprobe parameters, the KUnit tests should be able to run as
> > a module.
>
> Note that KUnit tests that are doing more of a functional/integration
> testing (on "live" hardware) rather than unit testing (where hardware
> interactions are mocked) are not very common.
> Do we have other KUnit tests like this merged?

I don't think we have other tests like this.

> Some of the "live tests" are not even that, being more of a pure
> hardware tests (e.g. live_workarounds, which is checking whether values
> in MMIO regs stick over various HW state transitions).
>
> I'm wondering, is KUnit the right tool for this job?

The main focus of KUnit is for hw-independent tests.
So in theory: no.

But I can imagine it could be easier to write the validation via
KUNIT_EXPECT_EQ and friends as opposed to writing your own kernel
module w/ its own set of macros, etc.

So my first thought is: "if it works, then you can try using it."
(Might want to take steps like make sure they don't get enabled by
CONFIG_KUNIT_ALL_TESTS=y).

Talking with David, he seems to have echoed my thoughts.
David also suggested that maybe the test could use a fake of the hw by
default, but have an option to run against real hw when available.
I think that sounds like a good chunk of work, so I don't know if you
need to worry about that.

Daniel

Re: [RFC PATCH 0/3] Support for Solid Fill Planes

2022-11-07 Thread Laurent Pinchart

On Mon, Nov 07, 2022 at 09:37:08PM +0200, Ville Syrjälä wrote:
> On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote:
> > Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT
> > properties. When the color fill value is set, and the framebuffer is set
> > to NULL, memory fetch will be disabled.
> 
> Thinking a bit more universally I wonder if there should be
> some kind of enum property:
> 
> enum plane_pixel_source {
>   FB,
>   COLOR,
>   LIVE_FOO,
>   LIVE_BAR,
>   ...
> }

That's something I could use on (older) Renesas hardware, where planes
can be fed with a live source. The issue there is that the live source
is a compositor with multiple inputs, handled through the V4L2 API. How
to synchronize the configuration of the compositor and the display
engine is an unsolved problem at the moment, but it could be solved
another day.

> > In addition, loosen the NULL FB checks within the atomic commit callstack
> > to allow a NULL FB when color_fill is nonzero and add FB checks in
> > methods where the FB was previously assumed to be non-NULL.
> > 
> > Finally, have the DPU driver use drm_plane_state.color_fill and
> > drm_plane_state.color_fill_format instead of dpu_plane_state.color_fill,
> > and add extra checks in the DPU atomic commit callstack to account for a
> > NULL FB in cases where color_fill is set.
> > 
> > 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.
> > 
> > Userspace can set the color_fill value by setting COLOR_FILL_FORMAT to a
> > DRM format, setting COLOR_FILL to a color fill value, and setting the
> > framebuffer to NULL.
> 
> Is there some real reason for the format property? Ie. why not 
> just do what was the plan for the crttc background color and
> specify the color in full 16bpc format and just pick as many
> msbs from that as the hw can use?
> 
> > Jessica Zhang (3):
> >   drm: Introduce color fill properties for drm plane
> >   drm: Adjust atomic checks for solid fill color
> >   drm/msm/dpu: Use color_fill property for DPU planes
> > 
> >  drivers/gpu/drm/drm_atomic.c  | 68 ---
> >  drivers/gpu/drm/drm_atomic_helper.c   | 34 +++-
> >  drivers/gpu/drm/drm_atomic_uapi.c |  8 +++
> >  drivers/gpu/drm/drm_blend.c   | 38 +
> >  drivers/gpu/drm/drm_plane.c   |  8 +--
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  |  7 ++-
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 66 ++
> >  include/drm/drm_atomic_helper.h   |  5 +-
> >  include/drm/drm_blend.h   |  2 +
> >  include/drm/drm_plane.h   | 28 ++
> >  10 files changed, 188 insertions(+), 76 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/5] drm/msm/dsi: add support for DSI-PHY on SM8350 and SM8450

2022-11-07 Thread Dmitry Baryshkov


On 07/11/2022 07:27, Abhinav Kumar wrote:



On 9/22/2022 4:30 AM, Dmitry Baryshkov wrote:

SM8350 and SM8450 use 5nm DSI PHYs, which share register definitions
with 7nm DSI PHYs. Rather than duplicating the driver, handle 5nm
variants inside the common 5+7nm driver.


I do realize that there is common code across PHYs but i am concerned 
about this type of unification of phy drivers.


This more or less follows downstream, which has unifier v4.0 driver.

If we have features which are PHY sequence dependent such as ULPS, this 
will just complicate things for us.


During development we initially tried to create a separate 5nm driver. 
However this resulted in huuuge code duplication. This would be prone to 
significant amount of errors if we change one of the drivers at some 
point and not another one.


Also some PHY registers might get added some might get removed across 
chipsets as this is the most frequently changed component.


Yes, I completely agree here. However beforehand we have successfully 
managed to have per-generation drivers, handling minor differences with 
quirks.




Even in this patch, I see this added to dsi_7nm_phy_disable()

 > +    /* Turn off REFGEN Vote */
 > +    dsi_phy_write(base + 
REG_DSI_7nm_PHY_CMN_GLBL_DIGTOP_SPARE10, 0x0);

 > +    wmb();
 > +    /* Delay to ensure HW removes vote before PHY shut down */
 > +    udelay(2);
 > +

What would be the impact of writing this for the existing 7nm PHY?


Let's probably guard this with the v4.3 check.



I am having some access issues to check the software interface so wanted 
to check.


I don't remember having any issues on RB5, but I did not run any special 
checks.






Co-developed-by: Robert Foss 
Signed-off-by: Dmitry Baryshkov 
---
  drivers/gpu/drm/msm/Kconfig   |   6 +-
  drivers/gpu/drm/msm/dsi/phy/dsi_phy.c |   4 +
  drivers/gpu/drm/msm/dsi/phy/dsi_phy.h |   2 +
  drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c | 132 --
  4 files changed, 131 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig
index 4e0cbd682725..e6c5dfbad009 100644
--- a/drivers/gpu/drm/msm/Kconfig
+++ b/drivers/gpu/drm/msm/Kconfig
@@ -140,12 +140,12 @@ config DRM_MSM_DSI_10NM_PHY
    Choose this option if DSI PHY on SDM845 is used on the platform.
  config DRM_MSM_DSI_7NM_PHY
-    bool "Enable DSI 7nm PHY driver in MSM DRM"
+    bool "Enable DSI 7nm/5nm PHY driver in MSM DRM"
  depends on DRM_MSM_DSI
  default y
  help
-  Choose this option if DSI PHY on SM8150/SM8250/SC7280 is used on
-  the platform.
+  Choose this option if DSI PHY on 
SM8150/SM8250/SM8350/SM8450/SC7280

+  is used on the platform.
  config DRM_MSM_HDMI
  bool "Enable HDMI support in MSM DRM driver"
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c 
b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c

index 7fc0975cb869..97cf6b8b34cc 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
@@ -567,6 +567,10 @@ static const struct of_device_id 
dsi_phy_dt_match[] = {

    .data = &dsi_phy_7nm_8150_cfgs },
  { .compatible = "qcom,sc7280-dsi-phy-7nm",
    .data = &dsi_phy_7nm_7280_cfgs },
+    { .compatible = "qcom,dsi-phy-5nm-8350",
+  .data = &dsi_phy_5nm_8350_cfgs },
+    { .compatible = "qcom,dsi-phy-5nm-8450",
+  .data = &dsi_phy_5nm_8450_cfgs },
  #endif
  {}
  };
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h 
b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h

index 60a99c6525b2..654cbfa14d6e 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
@@ -56,6 +56,8 @@ extern const struct msm_dsi_phy_cfg 
dsi_phy_10nm_8998_cfgs;

  extern const struct msm_dsi_phy_cfg dsi_phy_7nm_cfgs;
  extern const struct msm_dsi_phy_cfg dsi_phy_7nm_8150_cfgs;
  extern const struct msm_dsi_phy_cfg dsi_phy_7nm_7280_cfgs;
+extern const struct msm_dsi_phy_cfg dsi_phy_5nm_8350_cfgs;
+extern const struct msm_dsi_phy_cfg dsi_phy_5nm_8450_cfgs;
  struct msm_dsi_dphy_timing {
  u32 clk_zero;
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c 
b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c

index 9e7fa7d88ead..1696ff150b9e 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
@@ -39,8 +39,14 @@
  #define VCO_REF_CLK_RATE    1920
  #define FRAC_BITS 18
+/* Hardware is pre V4.1 */
+#define DSI_PHY_7NM_QUIRK_PRE_V4_1    BIT(0)
  /* Hardware is V4.1 */
-#define DSI_PHY_7NM_QUIRK_V4_1    BIT(0)
+#define DSI_PHY_7NM_QUIRK_V4_1    BIT(1)
+/* Hardware is V4.2 */
+#define DSI_PHY_7NM_QUIRK_V4_2    BIT(2)
+/* Hardware is V4.3 */
+#define DSI_PHY_7NM_QUIRK_V4_3    BIT(3)
  struct dsi_pll_config {
  bool enable_ssc;
@@ -116,7 +122,7 @@ static void dsi_pll_calc_dec_frac(struct 
dsi_pll_7nm *pll, struct dsi_pll_config

  dec_multiple = div_u64(pll_freq * multiplier, divider);
  dec = div_u64_rem(dec_multiple, m

[PATCH 2/2] drm/v3d: add missing mutex_destroy

2022-11-07 Thread Maíra Canal

v3d_perfmon_open_file() instantiates a mutex for a particular file
instance, but it never destroys it by calling mutex_destroy() in
v3d_perfmon_close_file().

Similarly, v3d_perfmon_create_ioctl() instantiates a mutex for a
particular perfmon, but it never destroys it by calling mutex_destroy()
in v3d_perfmon_destroy_ioctl().

So, add the missing mutex_destroy on both cases.

Signed-off-by: Maíra Canal 
---
 drivers/gpu/drm/v3d/v3d_perfmon.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/v3d/v3d_perfmon.c 
b/drivers/gpu/drm/v3d/v3d_perfmon.c
index 48972c49..292c73544255 100644
--- a/drivers/gpu/drm/v3d/v3d_perfmon.c
+++ b/drivers/gpu/drm/v3d/v3d_perfmon.c
@@ -113,6 +113,7 @@ void v3d_perfmon_close_file(struct v3d_file_priv *v3d_priv)
idr_for_each(&v3d_priv->perfmon.idr, v3d_perfmon_idr_del, NULL);
idr_destroy(&v3d_priv->perfmon.idr);
mutex_unlock(&v3d_priv->perfmon.lock);
+   mutex_destroy(&v3d_priv->perfmon.lock);
 }
 
 int v3d_perfmon_create_ioctl(struct drm_device *dev, void *data,
@@ -177,6 +178,7 @@ int v3d_perfmon_destroy_ioctl(struct drm_device *dev, void 
*data,
if (!perfmon)
return -EINVAL;
 
+   mutex_destroy(&perfmon->lock);
v3d_perfmon_put(perfmon);
 
return 0;
-- 
2.38.1

[PATCH 1/2] drm/v3d: switch to drmm_mutex_init

2022-11-07 Thread Maíra Canal

mutex_init() is supposed to be balanced by a call to mutex_destroy(), but
this is not currently happening on the v3d driver.

Considering the introduction of a DRM-managed mutex_init variant, switch
to drmm_mutex_init.

Signed-off-by: Maíra Canal 
---
 drivers/gpu/drm/v3d/v3d_gem.c | 17 +
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
index b8980440d137..96af1cb5202a 100644
--- a/drivers/gpu/drm/v3d/v3d_gem.c
+++ b/drivers/gpu/drm/v3d/v3d_gem.c
@@ -10,6 +10,7 @@
 #include 
 #include 
 
+#include 
 #include 
 #include 
 
@@ -1075,10 +1076,18 @@ v3d_gem_init(struct drm_device *dev)
 
spin_lock_init(&v3d->mm_lock);
spin_lock_init(&v3d->job_lock);
-   mutex_init(&v3d->bo_lock);
-   mutex_init(&v3d->reset_lock);
-   mutex_init(&v3d->sched_lock);
-   mutex_init(&v3d->cache_clean_lock);
+   ret = drmm_mutex_init(dev, &v3d->bo_lock);
+   if (ret)
+   return ret;
+   ret = drmm_mutex_init(dev, &v3d->reset_lock);
+   if (ret)
+   return ret;
+   ret = drmm_mutex_init(dev, &v3d->sched_lock);
+   if (ret)
+   return ret;
+   ret = drmm_mutex_init(dev, &v3d->cache_clean_lock);
+   if (ret)
+   return ret;
 
/* Note: We don't allocate address 0.  Various bits of HW
 * treat 0 as special, such as the occlusion query counters
-- 
2.38.1

[PATCH 0/2] Balance mutex_init and mutex_destroy calls

2022-11-07 Thread Maíra Canal

This series introduces some changes to assure the correct resource release on
the V3D driver, especially the mutex. Currently, the V3D has no mutex_destroy()
calls, which means that a mutex is being instantiated, but it is not being
released by the end of its use.

So, use the DRM-managed mutex_init variants when possible to manage the mutex
release and add mutex_destroy() calls when not possible.

Best Regards,
- Maíra Canal

Maíra Canal (2):
  drm/v3d: switch to drmm_mutex_init
  drm/v3d: add missing mutex_destroy

 drivers/gpu/drm/v3d/v3d_gem.c | 17 +
 drivers/gpu/drm/v3d/v3d_perfmon.c |  2 ++
 2 files changed, 15 insertions(+), 4 deletions(-)

-- 
2.38.1

Re: [PATCH] drm/ttm: optimize pool allocations a bit

2022-11-07 Thread Luben Tuikov

On 2022-11-07 14:58, Christian König wrote:
> If we got a page pool use it as much as possible.
> 
> If we can't get more pages from the pool allocate as much as possible.
> 
> Only if that still doesn't work reduce the order and try again.
> 
> Signed-off-by: Christian König 
> ---
>  drivers/gpu/drm/ttm/ttm_pool.c | 81 --
>  1 file changed, 57 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
> index 21b61631f73a..cf15874cf380 100644
> --- a/drivers/gpu/drm/ttm/ttm_pool.c
> +++ b/drivers/gpu/drm/ttm/ttm_pool.c
> @@ -344,6 +344,27 @@ static unsigned int ttm_pool_page_order(struct ttm_pool 
> *pool, struct page *p)
>   return p->private;
>  }
>  
> +/* Called when we got a page, either from a pool or newly allocated */
> +int ttm_pool_page_allocated(struct ttm_pool *pool, unsigned int order,
> + struct page *p, dma_addr_t **dma_addr,
> + unsigned long *num_pages, struct page ***pages)
> +{
> + unsigned int i;
> + int r;
> +
> + if (*dma_addr) {
> + r = ttm_pool_map(pool, order, p, dma_addr);
> + if (r)
> + return r;
> + }
> +
> + *num_pages -= 1 << order;
> + for (i = 1 << order; i; --i)
> + *((*pages)++) = p++;

Since we're using a for-loop here anyway, perhaps it's better to 
simplify-clarify
this as:
for (i = 1 << order; i; i--, (*pages)++, p++)
**pages = p;

Regards,
Luben

> +
> + return 0;
> +}
> +
>  /**
>   * ttm_pool_alloc - Fill a ttm_tt object
>   *
> @@ -385,45 +406,57 @@ int ttm_pool_alloc(struct ttm_pool *pool, struct ttm_tt 
> *tt,
>   for (order = min_t(unsigned int, MAX_ORDER - 1, __fls(num_pages));
>num_pages;
>order = min_t(unsigned int, order, __fls(num_pages))) {
> - bool apply_caching = false;
>   struct ttm_pool_type *pt;
>  
>   pt = ttm_pool_select_type(pool, tt->caching, order);
>   p = pt ? ttm_pool_type_take(pt) : NULL;
>   if (p) {
> - apply_caching = true;
> - } else {
> - p = ttm_pool_alloc_page(pool, gfp_flags, order);
> - if (p && PageHighMem(p))
> - apply_caching = true;
> - }
> -
> - if (!p) {
> - if (order) {
> - --order;
> - continue;
> - }
> - r = -ENOMEM;
> - goto error_free_all;
> - }
> -
> - if (apply_caching) {
>   r = ttm_pool_apply_caching(caching, pages,
>  tt->caching);
>   if (r)
>   goto error_free_page;
> - caching = pages + (1 << order);
> +
> + while (p) {
> + r = ttm_pool_page_allocated(pool, order, p,
> + &dma_addr,
> + &num_pages,
> + &pages);
> + if (r)
> + goto error_free_page;
> +
> + if (num_pages < (1 << order))
> + break;
> +
> + p = ttm_pool_type_take(pt);
> + }
> + caching = pages;
>   }
>  
> - if (dma_addr) {
> - r = ttm_pool_map(pool, order, p, &dma_addr);
> + while (num_pages >= (1 << order) &&
> +(p = ttm_pool_alloc_page(pool, gfp_flags, order))) {
> +
> + if (PageHighMem(p)) {
> + r = ttm_pool_apply_caching(caching, pages,
> +tt->caching);
> + if (r)
> + goto error_free_page;
> + }
> + r = ttm_pool_page_allocated(pool, order, p, &dma_addr,
> + &num_pages, &pages);
>   if (r)
>   goto error_free_page;
> + if (PageHighMem(p))
> + caching = pages;
>   }
>  
> - num_pages -= 1 << order;
> - for (i = 1 << order; i; --i)
> - *(pages++) = p++;
> + if (!p) {
> + if (order) {
> + --order;
> + continue;
> + }
> + r = -ENOMEM;
> + goto error_free_all;
> + }
>   }

Re: [Freedreno] [RFC PATCH 0/3] Support for Solid Fill Planes

2022-11-07 Thread Rob Clark

On Mon, Nov 7, 2022 at 1:32 PM Jessica Zhang  wrote:
>
>
>
> On 11/7/2022 11:37 AM, Ville Syrjälä wrote:
> > On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote:
> >> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT
> >> properties. When the color fill value is set, and the framebuffer is set
> >> to NULL, memory fetch will be disabled.
> >
> > Thinking a bit more universally I wonder if there should be
> > some kind of enum property:
> >
> > enum plane_pixel_source {
> >   FB,
> >   COLOR,
> >   LIVE_FOO,
> >   LIVE_BAR,
> >   ...
> > }
>
> Hi Ville,
>
> Makes sense -- this way, we'd also lay some groundwork for cases where
> drivers want to use other non-FB sources.
>
> >
> >> In addition, loosen the NULL FB checks within the atomic commit callstack
> >> to allow a NULL FB when color_fill is nonzero and add FB checks in
> >> methods where the FB was previously assumed to be non-NULL.
> >>
> >> Finally, have the DPU driver use drm_plane_state.color_fill and
> >> drm_plane_state.color_fill_format instead of dpu_plane_state.color_fill,
> >> and add extra checks in the DPU atomic commit callstack to account for a
> >> NULL FB in cases where color_fill is set.
> >>
> >> 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.
> >>
> >> Userspace can set the color_fill value by setting COLOR_FILL_FORMAT to a
> >> DRM format, setting COLOR_FILL to a color fill value, and setting the
> >> framebuffer to NULL.
> >
> > Is there some real reason for the format property? Ie. why not
> > just do what was the plan for the crttc background color and
> > specify the color in full 16bpc format and just pick as many
> > msbs from that as the hw can use?
>
> The format property was added because we can't assume that all hardware
> will support/use the same color format for solid fill planes. Even for
> just MSM devices, the hardware supports different variations of RGB
> formats [1].

Sure, but the driver can convert the format into whatever the hw
wants.  A 1x1 color conversion is not going to be problematic ;-)

BR,
-R

> Thanks,
>
> Jessica Zhang
>
> [1]
> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c#L697
>
> >
> >>
> >> Jessica Zhang (3):
> >>drm: Introduce color fill properties for drm plane
> >>drm: Adjust atomic checks for solid fill color
> >>drm/msm/dpu: Use color_fill property for DPU planes
> >>
> >>   drivers/gpu/drm/drm_atomic.c  | 68 ---
> >>   drivers/gpu/drm/drm_atomic_helper.c   | 34 +++-
> >>   drivers/gpu/drm/drm_atomic_uapi.c |  8 +++
> >>   drivers/gpu/drm/drm_blend.c   | 38 +
> >>   drivers/gpu/drm/drm_plane.c   |  8 +--
> >>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  |  7 ++-
> >>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 66 ++
> >>   include/drm/drm_atomic_helper.h   |  5 +-
> >>   include/drm/drm_blend.h   |  2 +
> >>   include/drm/drm_plane.h   | 28 ++
> >>   10 files changed, 188 insertions(+), 76 deletions(-)
> >>
> >> --
> >> 2.38.0
> >
> > --
> > Ville Syrjälä
> > Intel

Re: [RFC PATCH 0/3] Support for Solid Fill Planes

2022-11-07 Thread Jessica Zhang

On 11/7/2022 11:37 AM, Ville Syrjälä wrote:

On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote:

Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT
properties. When the color fill value is set, and the framebuffer is set
to NULL, memory fetch will be disabled.

Thinking a bit more universally I wonder if there should be
some kind of enum property:

enum plane_pixel_source {
FB,
COLOR,
LIVE_FOO,
LIVE_BAR,
...
}

Hi Ville,

Makes sense -- this way, we'd also lay some groundwork for cases where
drivers want to use other non-FB sources.

Userspace can set the color_fill value by setting COLOR_FILL_FORMAT to a
DRM format, setting COLOR_FILL to a color fill value, and setting the
framebuffer to NULL.

Thanks,

Jessica Zhang

[1]
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c#L697

Jessica Zhang (3):
drm: Introduce color fill properties for drm plane
drm: Adjust atomic checks for solid fill color
drm/msm/dpu: Use color_fill property for DPU planes

--
2.38.0

--
Ville Syrjälä
Intel

Re: [PATCH RFC 19/19] habanalabs: remove FOLL_FORCE usage

On Mon, Nov 7, 2022 at 6:19 PM David Hildenbrand  wrote:
>
> FOLL_FORCE is really only for debugger access. As we unpin the pinned pages
> using unpin_user_pages_dirty_lock(true), the assumption is that all these
> pages are writable.
>
> FOLL_FORCE in this case seems to be due to copy-and-past from other
> drivers. Let's just remove it.
>
> Cc: Oded Gabbay 
> Cc: Arnd Bergmann 
> Cc: Greg Kroah-Hartman 
> Signed-off-by: David Hildenbrand 
> ---
>  drivers/misc/habanalabs/common/memory.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/misc/habanalabs/common/memory.c 
> b/drivers/misc/habanalabs/common/memory.c
> index ef28f3b37b93..e35cca96bbef 100644
> --- a/drivers/misc/habanalabs/common/memory.c
> +++ b/drivers/misc/habanalabs/common/memory.c
> @@ -2312,8 +2312,7 @@ static int get_user_memory(struct hl_device *hdev, u64 
> addr, u64 size,
> if (!userptr->pages)
> return -ENOMEM;
>
> -   rc = pin_user_pages_fast(start, npages,
> -FOLL_FORCE | FOLL_WRITE | FOLL_LONGTERM,
> +   rc = pin_user_pages_fast(start, npages, FOLL_WRITE | FOLL_LONGTERM,
>  userptr->pages);
>
> if (rc != npages) {
> --
> 2.38.1
>
>
Acked-by: Oded Gabbay 
Thanks,
Oded

Re: [PATCH v2] gpu: host1x: Avoid trying to use GART on Tegra20

2022-11-07 Thread Jon Hunter


Thierry,

On 21/10/2022 08:41, Jon Hunter wrote:


On 20/10/2022 15:23, Robin Murphy wrote:

Since commit c7e3ca515e78 ("iommu/tegra: gart: Do not register with
bus") quite some time ago, the GART driver has effectively disabled
itself to avoid issues with the GPU driver expecting it to work in ways
that it doesn't. As of commit 57365a04c921 ("iommu: Move bus setup to
IOMMU device registration") that bodge no longer works, but really the
GPU driver should be responsible for its own behaviour anyway. Make the
workaround explicit.

Reported-by: Jon Hunter 
Suggested-by: Dmitry Osipenko 
Signed-off-by: Robin Murphy 
---

v2: Cover DRM instance too, move into *_wants_iommu() for consistency

  drivers/gpu/drm/tegra/drm.c | 4 
  drivers/gpu/host1x/dev.c    | 4 
  2 files changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index 6748ec1e0005..a1f909dac89a 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -1093,6 +1093,10 @@ static bool host1x_drm_wants_iommu(struct 
host1x_device *dev)

  struct host1x *host1x = dev_get_drvdata(dev->dev.parent);
  struct iommu_domain *domain;
+    /* Our IOMMU usage policy doesn't currently play well with GART */
+    if (of_machine_is_compatible("nvidia,tegra20"))
+    return false;
+
  /*
   * If the Tegra DRM clients are backed by an IOMMU, push buffers 
are

   * likely to be allocated beyond the 32-bit boundary if sufficient
diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
index 0cd3f97e7e49..f60ea24db0ec 100644
--- a/drivers/gpu/host1x/dev.c
+++ b/drivers/gpu/host1x/dev.c
@@ -292,6 +292,10 @@ static void 
host1x_setup_virtualization_tables(struct host1x *host)

  static bool host1x_wants_iommu(struct host1x *host1x)
  {
+    /* Our IOMMU usage policy doesn't currently play well with GART */
+    if (of_machine_is_compatible("nvidia,tegra20"))
+    return false;
+
  /*
   * If we support addressing a maximum of 32 bits of physical memory
   * and if the host1x firewall is enabled, there's no need to enable



Thanks! Works for me.

Tested-by: Jon Hunter 


We need to pick this fix up for v6.1.

Thanks
Jon

--
nvpublic

Re: [RFC PATCH v3 2/3] accel: add dedicated minor for accelerator devices

On Mon, Nov 7, 2022 at 6:20 PM Jeffrey Hugo  wrote:
>
> On 11/6/2022 2:02 PM, Oded Gabbay wrote:
> > --- a/drivers/accel/drm_accel.c
> > +++ b/drivers/accel/drm_accel.c
> > @@ -8,14 +8,25 @@
> >
> >   #include 
> >   #include 
> > +#include 
>
> If we are not using xarray at this time, do we still need this include?
>
> >
> >   #include 
> > +#include 
> > +#include 
> > +#include 
> >   #include 
> >   #include 
> >
> > +static DEFINE_SPINLOCK(accel_minor_lock);
> > +static struct idr accel_minors_idr;
>
> I beleive we should have an explicit include for the IDR header.
>
> > --- a/include/drm/drm_accel.h
> > +++ b/include/drm/drm_accel.h
> > @@ -8,12 +8,56 @@
> >   #ifndef DRM_ACCEL_H_
> >   #define DRM_ACCEL_H_
> >
> > -#define ACCEL_MAJOR 261
> > +#include 
> > +
> > +#define ACCEL_MAJOR  261
> > +#define ACCEL_MAX_MINORS 256
>
> This diff seems really weird.  The changes to the ACCEL_MAJOR define
> could get pushed to the previous patch, no?
>
> > @@ -23,9 +67,31 @@ static inline void accel_core_exit(void)
> >
> >   static inline int __init accel_core_init(void)
> >   {
> > + /* Return 0 to allow drm_core_init to complete successfully */
>
> Move to previous patch?
>
> > --- a/include/drm/drm_drv.h
> > +++ b/include/drm/drm_drv.h
> > @@ -94,6 +94,14 @@ enum drm_driver_feature {
> >* synchronization of command submission.
> >*/
> >   DRIVER_SYNCOBJ_TIMELINE = BIT(6),
> > + /**
> > +  * @DRIVER_COMPUTE_ACCEL:
> > +  *
> > +  * Driver supports compute acceleration devices. This flag is 
> > mutually exclusive with
> > +  * @DRIVER_RENDER and @DRIVER_MODESET. Devices that support both 
> > graphics and compute
> > +  * acceleration should be handled by two drivers that are connected 
> > using auxiliry bus.
>
> auxiliry -> auxiliary
>
All comments will be fixed.
Thanks,
Oded

Re: [RFC PATCH v3 1/3] drivers/accel: define kconfig and register a new major

On Mon, Nov 7, 2022 at 6:12 PM Jeffrey Hugo  wrote:
>
> On 11/6/2022 2:02 PM, Oded Gabbay wrote:
> > +int __init accel_core_init(void)
> > +{
> > + int ret;
> > +
> > + ret = accel_sysfs_init();
> > + if (ret < 0) {
> > + DRM_ERROR("Cannot create ACCEL class: %d\n", ret);
> > + goto error;
> > + }
> > +
> > + accel_debugfs_root = debugfs_create_dir("accel", NULL);
> > +
> > + ret = register_chrdev(ACCEL_MAJOR, "accel", &accel_stub_fops);
> > + if (ret < 0)
> > + goto error;
>
> We are not jumping over anything here.  Seems like this whole if block
> could just be removed.
correct, will be fixed.
>
> > +
> > +error:
> > + /* Any cleanup will be done in drm_core_exit() that will call
> > +  * to accel_core_exit()
> > +  */
>
> This doesn't look like the standard multi-line comment style.  Are we
> going to say that the accel subsystem follows net and differs from the
> kernel standard?
I'll change it to the kernel standard.
Thx,
Oded
>
> > + return ret;
> > +}

Re: [RFC PATCH v3 3/3] drm: initialize accel framework

On Mon, Nov 7, 2022 at 6:25 PM Jeffrey Hugo  wrote:
>
> On 11/6/2022 2:02 PM, Oded Gabbay wrote:
>
> > @@ -603,6 +626,14 @@ static int drm_dev_init(struct drm_device *dev,
> >   /* no per-device feature limits by default */
> >   dev->driver_features = ~0u;
> >
> > + if (drm_core_check_feature(dev, DRIVER_COMPUTE_ACCEL) &&
> > + (drm_core_check_feature(dev, DRIVER_RENDER) ||
> > + drm_core_check_feature(dev, DRIVER_MODESET))) 
> > {
>
> Shouldn't the indentation for the 2nd and 3rd line be such that the
> start of the lines is aligned with the "(" on the first line?
afaik there is no such rule. If there was, checkpatch should have reported that.
Oded

Re: [PATCH 12/26] drm: etnaviv: Remove #ifdef guards for PM related functions

2022-11-07 Thread Paul Cercueil


Hi Lucas,

Le lun. 7 nov. 2022 à 19:07:32 +0100, Lucas Stach 
 a écrit :

Am Montag, dem 07.11.2022 um 17:52 + schrieb Paul Cercueil:

 Use the RUNTIME_PM_OPS() and pm_ptr() macros to handle the
 .runtime_suspend/.runtime_resume callbacks.

 These macros allow the suspend and resume functions to be 
automatically

 dropped by the compiler when CONFIG_PM is disabled, without having
 to use #ifdef guards.

 This has the advantage of always compiling these functions in,
 independently of any Kconfig option. Thanks to that, bugs and other
 regressions are subsequently easier to catch.

 Some #ifdef CONFIG_PM guards were protecting simple statements, and 
were

 also converted to "if (IS_ENABLED(CONFIG_PM))".


Reasoning and the change itself look good.


That's an ack? :)


 Note that this driver should probably use the
 DEFINE_RUNTIME_DEV_PM_OPS() macro instead, which will provide
 .suspend/.resume callbacks, pointing to pm_runtime_force_suspend() 
and
 pm_runtime_force_resume() respectively; unless those callbacks 
really

 aren't needed.


This however isn't true, specifically this driver can _not_ use
pm_runtime_force_suspend, as the GPU can't be forced into suspend by
calling the rpm callback. A real suspend implementation would first
need to make sure the GPU finished working on the current queued jobs,
only then it would be able to power it down via the rpm suspend
callback.


Understood. I'll remove this paragraph if I have to V2.

Cheers,
-Paul



 Signed-off-by: Paul Cercueil 
 ---
 Cc: Lucas Stach 
 Cc: Russell King 
 Cc: Christian Gmeiner 
 Cc: etna...@lists.freedesktop.org
 ---
  drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 30 
+++

  1 file changed, 12 insertions(+), 18 deletions(-)

 diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c 
b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c

 index 37018bc55810..e9a5444ec1c7 100644
 --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
 +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
 @@ -1605,7 +1605,6 @@ static int etnaviv_gpu_hw_suspend(struct 
etnaviv_gpu *gpu)

return etnaviv_gpu_clk_disable(gpu);
  }

 -#ifdef CONFIG_PM
  static int etnaviv_gpu_hw_resume(struct etnaviv_gpu *gpu)
  {
int ret;
 @@ -1621,7 +1620,6 @@ static int etnaviv_gpu_hw_resume(struct 
etnaviv_gpu *gpu)


return 0;
  }
 -#endif

  static int
  etnaviv_gpu_cooling_get_max_state(struct thermal_cooling_device 
*cdev,
 @@ -1689,11 +1687,10 @@ static int etnaviv_gpu_bind(struct device 
*dev, struct device *master,

if (ret)
goto out_workqueue;

 -#ifdef CONFIG_PM
 -  ret = pm_runtime_get_sync(gpu->dev);
 -#else
 -  ret = etnaviv_gpu_clk_enable(gpu);
 -#endif
 +  if (IS_ENABLED(CONFIG_PM))
 +  ret = pm_runtime_get_sync(gpu->dev);
 +  else
 +  ret = etnaviv_gpu_clk_enable(gpu);
if (ret < 0)
goto out_sched;

 @@ -1737,12 +1734,12 @@ static void etnaviv_gpu_unbind(struct 
device *dev, struct device *master,


etnaviv_sched_fini(gpu);

 -#ifdef CONFIG_PM
 -  pm_runtime_get_sync(gpu->dev);
 -  pm_runtime_put_sync_suspend(gpu->dev);
 -#else
 -  etnaviv_gpu_hw_suspend(gpu);
 -#endif
 +  if (IS_ENABLED(CONFIG_PM)) {
 +  pm_runtime_get_sync(gpu->dev);
 +  pm_runtime_put_sync_suspend(gpu->dev);
 +  } else {
 +  etnaviv_gpu_hw_suspend(gpu);
 +  }

if (gpu->mmu_context)
etnaviv_iommu_context_put(gpu->mmu_context);
 @@ -1856,7 +1853,6 @@ static int etnaviv_gpu_platform_remove(struct 
platform_device *pdev)

return 0;
  }

 -#ifdef CONFIG_PM
  static int etnaviv_gpu_rpm_suspend(struct device *dev)
  {
struct etnaviv_gpu *gpu = dev_get_drvdata(dev);
 @@ -1899,18 +1895,16 @@ static int etnaviv_gpu_rpm_resume(struct 
device *dev)


return 0;
  }
 -#endif

  static const struct dev_pm_ops etnaviv_gpu_pm_ops = {
 -	SET_RUNTIME_PM_OPS(etnaviv_gpu_rpm_suspend, 
etnaviv_gpu_rpm_resume,

 - NULL)
 +	RUNTIME_PM_OPS(etnaviv_gpu_rpm_suspend, etnaviv_gpu_rpm_resume, 
NULL)

  };

  struct platform_driver etnaviv_gpu_driver = {
.driver = {
.name = "etnaviv-gpu",
.owner = THIS_MODULE,
 -  .pm = &etnaviv_gpu_pm_ops,
 +  .pm = pm_ptr(&etnaviv_gpu_pm_ops),
.of_match_table = etnaviv_gpu_match,
},
.probe = etnaviv_gpu_platform_probe,

Re: [PATCH 26/26] drm/i915/gt: Remove #ifdef guards for PM related functions

2022-11-07 Thread Paul Cercueil


Hi Rodrigo,

Le lun. 7 nov. 2022 à 15:31:49 -0500, Rodrigo Vivi 
 a écrit :

On Mon, Nov 07, 2022 at 05:55:10PM +, Paul Cercueil wrote:
 Instead of defining two versions of intel_sysfs_rc6_init(), one for 
each
 value of CONFIG_PM, add a check on !IS_ENABLED(CONFIG_PM) early in 
the
 function. This will allow the compiler to automatically drop the 
dead
 code when CONFIG_PM is disabled, without having to use #ifdef 
guards.


Making the RC6 to depend on the CONFIG_PM is probably an historical
mistake and probably the right solution is simply to remove that
dependency.


I don't know anything about i915, so the best I can do is update the 
code without changing the functionality.


Then someone with a better understanding can send a patch to remove the 
check on CONFIG_PM.


Cheers,
-Paul



 This has the advantage of always compiling these functions in,
 independently of any Kconfig option. Thanks to that, bugs and other
 regressions are subsequently easier to catch.

 Signed-off-by: Paul Cercueil 
 ---
 Cc: Jani Nikula 
 Cc: Joonas Lahtinen 
 Cc: Rodrigo Vivi 
 Cc: Tvrtko Ursulin 
 Cc: intel-...@lists.freedesktop.org
 ---
  drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c | 8 +---
  1 file changed, 1 insertion(+), 7 deletions(-)

 diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c 
b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c

 index 2b5f05b31187..decf892a4508 100644
 --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
 +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
 @@ -164,7 +164,6 @@ sysfs_gt_attribute_r_func(struct kobject *kobj, 
struct attribute *attr,

 NULL); 
\
INTEL_GT_ATTR_RO(_name)

 -#ifdef CONFIG_PM
  static u32 get_residency(struct intel_gt *gt, i915_reg_t reg)
  {
intel_wakeref_t wakeref;
 @@ -300,7 +299,7 @@ static void intel_sysfs_rc6_init(struct 
intel_gt *gt, struct kobject *kobj)

  {
int ret;

 -  if (!HAS_RC6(gt->i915))
 +  if (!IS_ENABLED(CONFIG_PM) || !HAS_RC6(gt->i915))
return;

ret = __intel_gt_sysfs_create_group(kobj, rc6_attr_group);
 @@ -329,11 +328,6 @@ static void intel_sysfs_rc6_init(struct 
intel_gt *gt, struct kobject *kobj)

 gt->info.id, ERR_PTR(ret));
}
  }
 -#else
 -static void intel_sysfs_rc6_init(struct intel_gt *gt, struct 
kobject *kobj)

 -{
 -}
 -#endif /* CONFIG_PM */

  static u32 __act_freq_mhz_show(struct intel_gt *gt)
  {
 --
 2.35.1

Re: [PATCH v2 56/65] clk: ingenic: cgu: Switch to determine_rate

2022-11-07 Thread Aidan MacDonald



Maxime Ripard  writes:

> Hi,
>
> On Fri, Nov 04, 2022 at 05:35:29PM +, Aidan MacDonald wrote:
>>
>> Maxime Ripard  writes:
>>
>> > Hi Paul,
>> >
>> > On Fri, Nov 04, 2022 at 02:31:20PM +, Paul Cercueil wrote:
>> >> Le ven. 4 nov. 2022 à 14:18:13 +0100, Maxime Ripard  a
>> >> écrit :
>> >> > The Ingenic CGU clocks implements a mux with a set_parent hook, but
>> >> > doesn't provide a determine_rate implementation.
>> >> >
>> >> > This is a bit odd, since set_parent() is there to, as its name implies,
>> >> > change the parent of a clock. However, the most likely candidate to
>> >> > trigger that parent change is a call to clk_set_rate(), with
>> >> > determine_rate() figuring out which parent is the best suited for a
>> >> > given rate.
>> >> >
>> >> > The other trigger would be a call to clk_set_parent(), but it's far less
>> >> > used, and it doesn't look like there's any obvious user for that clock.
>> >> >
>> >> > So, the set_parent hook is effectively unused, possibly because of an
>> >> > oversight. However, it could also be an explicit decision by the
>> >> > original author to avoid any reparenting but through an explicit call to
>> >> > clk_set_parent().
>> >> >
>> >> > The driver does implement round_rate() though, which means that we can
>> >> > change the rate of the clock, but we will never get to change the
>> >> > parent.
>> >> >
>> >> > However, It's hard to tell whether it's been done on purpose or not.
>> >> >
>> >> > Since we'll start mandating a determine_rate() implementation, let's
>> >> > convert the round_rate() implementation to a determine_rate(), which
>> >> > will also make the current behavior explicit. And if it was an
>> >> > oversight, the clock behaviour can be adjusted later on.
>> >>
>> >> So it's partly on purpose, partly because I didn't know about
>> >> .determine_rate.
>> >>
>> >> There's nothing odd about having a lonely .set_parent callback; in my case
>> >> the clocks are parented from the device tree.
>> >>
>> >> Having the clocks driver trigger a parent change when requesting a rate
>> >> change sounds very dangerous, IMHO. My MMC controller can be parented to 
>> >> the
>> >> external 48 MHz oscillator, and if the card requests 50 MHz, it could 
>> >> switch
>> >> to one of the PLLs. That works as long as the PLLs don't change rate, but 
>> >> if
>> >> one is configured as driving the CPU clock, it becomes messy.
>> >> The thing is, the clocks driver has no way to know whether or not it is
>> >> "safe" to use a designated parent.
>> >>
>> >> For that reason, in practice, I never actually want to have a clock
>> >> re-parented - it's almost always a bad idea vs. sticking to the parent 
>> >> clock
>> >> configured in the DTS.
>> >
>> > Yeah, and this is totally fine. But we need to be explicit about it. The
>> > determine_rate implementation I did in all the patches is an exact
>> > equivalent to the round_rate one if there was one. We will never ask to
>> > change the parent.
>> >
>> > Given what you just said, I would suggest to set the
>> > CLK_SET_RATE_NO_REPARENT flag as well.
>>
>> Ideally there should be a way for drivers and the device tree to
>> say, "clock X must be driven by clock Y", but the clock framework
>> would be allowed to re-parent clocks freely as long as it doesn't
>> violate any DT or driver constraints.
>
> I'm not really sure what you mean there, sorry. Isn't it what
> assigned-clock-parents/clk_set_parent() at probe, plus a determine_rate
> implementation that would affect best_parent_hw would already provide?

Assigning the parent clock in the DT works once, at boot, but going off
what you wrote in the commit message, if the clock driver has a
.determine_rate() implementation that *can* reparent clocks then it
probably *will* reparent them, and the DT assignment will be lost.

What I'm suggesting is a runtime constraint that the clock subsystem
would enforce, and actively prevent drivers from changing the parent.
Either explicitly with clk_set_parent() or due to .determine_rate().

That way you could write a .determine_rate() implementation that *can*
select a better parent, but if the DT applies a constraint to fix the
clock to a particular parent, the clock subsystem will force that parent
to be used so you can be sure the clock is never reparented by accident.

>> That way allowing reparenting doesn't need to be an all-or-nothing
>> thing, and it doesn't need to be decided at the clock driver level
>> with special flags.
>
> Like I said, the default implementation is already working to what you
> suggested if I understood properly. However, this has never been tested
> for any of the drivers in that series so I don't want to introduce (and
> debug ;)) regressions in all those drivers that were not setting any
> constraint but never actually tested their reparenting code.
>
> So that series is strictly equivalent to what you had before, it's just
> explicit now.
>
> If you find that some other decision make sense for your dr

Re: [PATCH 2/2] fbdev: Add support for the nomodeset kernel parameter

2022-11-07 Thread Helge Deller


On 11/7/22 16:30, Thomas Zimmermann wrote:

Hi

Am 07.11.22 um 14:57 schrieb Helge Deller:

On 11/7/22 11:49, Thomas Zimmermann wrote:

Support the kernel's nomodeset parameter for all PCI-based fbdev
drivers that use aperture helpers to remove other, hardware-agnostic
graphics drivers.

The parameter is a simple way of using the firmware-provided scanout
buffer if the hardware's native driver is broken.


Nah... it's probably not broken, but you want it disabled in order
to use the DRM driver instead?


No, it's really for broken native drivers or any kind of problematic
modesetting. Most DRM drivers already respect the nomodeset option
and won't load when given. All you'd get are the generic drivers,
such as simpledrm, efifb or simplefb.

There are better options of configuring video output on the kernel
command line.  But as graphics output is such a fundamental feature
to using a computer, we found that a simple and easy option to
workaround erroneous systems would benefit DRM users; hence the
nomodeset parameter.

As fbdev drivers also do modesetting, supporting the parameter simply
unifies the behavior.


Ok.


The same effect
could be achieved with per-driver options, but the importance of the
graphics output for many users makes a single, unified approach
worthwhile.

With nomodeset specified, the fbdev driver module will not load. This
unifies behavior with similar DRM drivers. In DRM helpers, modules
first check the nomodeset parameter before registering the PCI
driver. As fbdev has no such module helpers, we have to modify each
driver individually.


Ok.


The name 'nomodeset' is slightly misleading, but has been chosen for
historical reasons. Several drivers implemented it before it became a
general option for DRM. So keeping the existing name was preferred over
introducing a new one.



diff --git a/drivers/video/fbdev/aty/aty128fb.c 
b/drivers/video/fbdev/aty/aty128fb.c
index 57e398fe7a81c..1a26ac2865d65 100644
--- a/drivers/video/fbdev/aty/aty128fb.c
+++ b/drivers/video/fbdev/aty/aty128fb.c
@@ -2503,7 +2504,12 @@ static int aty128fb_init(void)
  {
  #ifndef MODULE
  char *option = NULL;
+#endif
+
+    if (video_firmware_drivers_only())
+    return -ENODEV;


I think it makes sense to give at least some info, why a specific
driver wasn't loaded, e.g. something like this kernel message:
aty128fb: Driver disabled due to "nomodeset" kernel parameter.

If you e.g. change the function video_firmware_drivers_only()
to become video_firmware_drivers_only(const char *drivername)
then you could print such a message in video_firmware_drivers_only()


Well, we do have such a message in disable_modeset() already. [1]
[1] 
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_nomodeset.c#L18


Yes, I saw it, but that message quite generic.
If for example my atyfb doesn't show up, I would grep dmesg for "aty" and
not "nomodeset"...



And I don't like very much the name of function video_firmware_drivers_only(),
but don't have any other better idea right now either...


It's part of the 'video' module, hence the prefix. The 'nomodeset'
option used to be implemented in several DRM drivers. It's an awful
name, but we didn't want to remove it or introduce a new one for the
same feature. So we kept nomodeset for all of DRM.  Then we started
bikeshedding the function name that returns the setting. And
firmware-drivers-only is the best description of what is happening
here. So that's how the name happend.


video_modesetting_disabled() ?
(Just an idea)

Helge

Re: [RFC PATCH v2 1/3] drivers/accel: define kconfig and register a new major

2022-11-07 Thread Dave Airlie

> > >
> > > "drm_minor" is not necessary anymore. Strictly managing minor numbers
> > > lost its value years ago when /dev/ was reorganized. Just use
> > > dynamic minors fully.
> > drm minor is not just about handling minor numbers. It contains the
> > entire code to manage devices that register with drm framework (e.g.
> > supply callbacks to file operations), manage their lifecycle,
> > resources (e.g. automatic free of resources on release), sysfs,
> > debugfs, etc.
>
> This is why you are having such troubles, this is already good library
> code. You don't need DRM to wrapper debugfs APIs, for instance. We
> have devm, though maybe it is not a good idea, etc
>
> Greg already pointed out the sysfs was not being done correctly
> anyhow.
>
> I don't think DRM is improving on these core kernel services. Just use
> the normal stuff directly.

At plumbers we decided a direction, I think the direction is good, if
there is refactoring to be done, I'd rather it was done in tree with a
clear direction.

Coming in now and saying we should go down a different path isn't
really helpful. We need to get rolling on this, we have drivers that
want to land somewhere now, which means we need to just get a
framework in place, leveraging drm code is the way to do it.

There is no need to an "accel" module, what does that even buy you,
the idea is to have an accel subsystem that allows drivers to use drm
features, not an accel subsystem that refactors drm features, that
would take years. There are already drivers for this subsystem wanting
to use GEM, and I don't think holding them up for a year to refactor
something that we don't have a clear reason or goal behind
refactoring.

If there is a problem with the drm subsystem interactions with the
kernel standard implementations then let's go fix that and accel will
also get fixed, but there's no reason to start going down that road at
the same time as introducing accel.

Also with the idr/xarray stuff, this isn't the patchset to be
introducing a bunch of new and divergent work, if this patchset
identifies deficiencies then let's document them and work on them in
parallel instead of blocking the initial landing in favour of some
future refactors with no in-tree users.

Dave.

Re: [PATCH 26/26] drm/i915/gt: Remove #ifdef guards for PM related functions

2022-11-07 Thread Rodrigo Vivi

On Mon, Nov 07, 2022 at 05:55:10PM +, Paul Cercueil wrote:
> Instead of defining two versions of intel_sysfs_rc6_init(), one for each
> value of CONFIG_PM, add a check on !IS_ENABLED(CONFIG_PM) early in the
> function. This will allow the compiler to automatically drop the dead
> code when CONFIG_PM is disabled, without having to use #ifdef guards.

Making the RC6 to depend on the CONFIG_PM is probably an historical
mistake and probably the right solution is simply to remove that
dependency.

> 
> This has the advantage of always compiling these functions in,
> independently of any Kconfig option. Thanks to that, bugs and other
> regressions are subsequently easier to catch.
> 
> Signed-off-by: Paul Cercueil 
> ---
> Cc: Jani Nikula 
> Cc: Joonas Lahtinen 
> Cc: Rodrigo Vivi 
> Cc: Tvrtko Ursulin 
> Cc: intel-...@lists.freedesktop.org
> ---
>  drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c | 8 +---
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c 
> b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
> index 2b5f05b31187..decf892a4508 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
> @@ -164,7 +164,6 @@ sysfs_gt_attribute_r_func(struct kobject *kobj, struct 
> attribute *attr,
>NULL); 
> \
>   INTEL_GT_ATTR_RO(_name)
>  
> -#ifdef CONFIG_PM
>  static u32 get_residency(struct intel_gt *gt, i915_reg_t reg)
>  {
>   intel_wakeref_t wakeref;
> @@ -300,7 +299,7 @@ static void intel_sysfs_rc6_init(struct intel_gt *gt, 
> struct kobject *kobj)
>  {
>   int ret;
>  
> - if (!HAS_RC6(gt->i915))
> + if (!IS_ENABLED(CONFIG_PM) || !HAS_RC6(gt->i915))
>   return;
>  
>   ret = __intel_gt_sysfs_create_group(kobj, rc6_attr_group);
> @@ -329,11 +328,6 @@ static void intel_sysfs_rc6_init(struct intel_gt *gt, 
> struct kobject *kobj)
>gt->info.id, ERR_PTR(ret));
>   }
>  }
> -#else
> -static void intel_sysfs_rc6_init(struct intel_gt *gt, struct kobject *kobj)
> -{
> -}
> -#endif /* CONFIG_PM */
>  
>  static u32 __act_freq_mhz_show(struct intel_gt *gt)
>  {
> -- 
> 2.35.1
>

Re: linux-next: build warning after merge of the drm tree

2022-11-07 Thread John Harrison


On 11/6/2022 19:29, Stephen Rothwell wrote:

Hi all,

After merging the drm tree, today's linux-next build (KCONFIG_NAME)
produced this warning:

drivers/gpu/drm/i915/i915_perf_types.h:319: warning: Function parameter or 
member 'lock' not described in 'i915_perf_stream'

Introduced by commit

   2db609c01495 ("drm/i915/perf: Replace gt->perf.lock with stream->lock for file 
ops")

Yes, a fix has been posted - 
https://patchwork.freedesktop.org/series/110633/


It is unclear how this escaped. It wasn't spotted in code review but 
moreover, the CI pre-merge doc build returned clean for that patch set 
(https://patchwork.freedesktop.org/series/107584/).


John.

Re: [RFC PATCH v2 1/3] drivers/accel: define kconfig and register a new major

2022-11-07 Thread Dave Airlie

On Mon, 7 Nov 2022 at 23:10, Jason Gunthorpe  wrote:
>
> On Mon, Nov 07, 2022 at 03:01:08PM +0200, Oded Gabbay wrote:
> > I don't agree with your statement that it should be "a layer over top of 
> > DRM".
> > Anything on top of DRM is a device driver.
> > Accel is not a device driver, it is a new type of drm minor / drm driver.
>
> Yeah, I still think this is not the right way, you are getting almost
> nothing from DRM and making everything more complicated in the
> process.

You are looking at the small picture that is these patches, there are
just infrastructure to start the process of merging drivers and
reusing other parts of the drm code.

We aren't going to ever get anywhere if we start splitting code out of
drm just in case, we get this stuff rolling in the tree and if we have
a pressing need to refactor it out into separate libraries later then
we can address that from a more educated place, instead of just
throwing huge refactors around before we have any code to even use
them.
>
> IMHO this is much better, because accel has very little need of DRM to
> manage a struct device/cdev in the first place.

Right now it doesn't, but when drivers start leveraging the other code
it will reuse a lot more code.

I'm not going to spend too much time entertaining this, devm vs drmm
memory etc are real problems drm has already identified if not
completely solved.

Dave.

Re: [v2 02/10] dts-bingings: display: bridge: Add MHDP HDMI bindings for i.MX8MQ

2022-11-07 Thread Rob Herring

On Fri, Nov 04, 2022 at 09:42:03AM -0400, Krzysztof Kozlowski wrote:
> On 04/11/2022 02:44, Sandor Yu wrote:
> > Add bindings for i.MX8MQ MHDP HDMI.
> 
> Typo in subject - bindings.

And 'dts'. It's 'dt-bindings: display: ...' Same for the rest of the 
series.

> 
> Also in the subject - drop redundant second word "bindings".
> 
> > 
> > Signed-off-by: Sandor Yu 
> > ---
> >  .../display/bridge/cdns,mhdp-imx8mq-hdmi.yaml | 67 +++
> >  1 file changed, 67 insertions(+)
> >  create mode 100644 
> > Documentation/devicetree/bindings/display/bridge/cdns,mhdp-imx8mq-hdmi.yaml

Re: [PATCH] docs/sphinx: More depth in the rtd sidebar toc

2022-11-07 Thread Daniel Vetter

On Fri, 28 Oct 2022 at 20:19, Jonathan Corbet  wrote:
>
> Daniel Vetter  writes:
>
> > We love to nest our documenation for good structure, but that means
> > the table of contents needs to keep up or you can't navigate them.
> >
> > Realized this trying to find the drm property documentation, which
> > with some shuffling around disappeared. Why I didn't realize we can do
> > this earlier, no idea.
> >
> > Since the relevant parts of the toc are only loaded if you're in the
> > right .html file there's no harm in going all the way to unlimited.
> >
> > Note that this has no impact on the classic theme (which doesn't have
> > the sidebar) nor on the various :toctree: rendered inline in the
> > output.
> >
> > Signed-off-by: Daniel Vetter 
> > Cc: Jonathan Corbet 
> > Cc: linux-...@vger.kernel.org
> > ---
> >  Documentation/conf.py | 4 
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/Documentation/conf.py b/Documentation/conf.py
> > index 934727e23e0e..5dc141c66726 100644
> > --- a/Documentation/conf.py
> > +++ b/Documentation/conf.py
> > @@ -240,6 +240,10 @@ if html_theme == 'sphinx_rtd_theme' or html_theme == 
> > 'sphinx_rtd_dark_mode':
> >  # Add color-specific RTD normal mode
> >  html_css_files.append('theme_rtd_colors.css')
> >
> > +html_theme_options = {
> > +'navigation_depth': -1,
> > +}
> > +
> >  except ImportError:
> >  html_theme = 'classic'
>
> So this patch isn't against docs-next, and applies to the RTD theme,
> which is no longer the default.  I have no objection to it, but have you
> looked at how your docs come out with the alabaster theme?

[sorry took a bit longer to get back to this]

Hm looks pretty, but more in a print style than using it dynamically,
you can't really click through the sidebar toc at all to quickly find
something, and if you're wrong, navigate up a few levels again. It's
just the toc for exactly the local document, nothing else at all. rtd
theme always gives you the full toc all the way up, and if you have
epic patience could actually give you the full toc on every document
(but that's probably not a good idea for the kernel). Do you need me
to send the rebased version or can you smash this one in?

btw on today's linux-next the sphinx.rst page isn't updated with the
new default theme choice of alabaster. That seems to have been
forgotten.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH] drm/ttm: optimize pool allocations a bit

2022-11-07 Thread Christian König

If we got a page pool use it as much as possible.

If we can't get more pages from the pool allocate as much as possible.

Only if that still doesn't work reduce the order and try again.

Signed-off-by: Christian König 
---
 drivers/gpu/drm/ttm/ttm_pool.c | 81 --
 1 file changed, 57 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
index 21b61631f73a..cf15874cf380 100644
--- a/drivers/gpu/drm/ttm/ttm_pool.c
+++ b/drivers/gpu/drm/ttm/ttm_pool.c
@@ -344,6 +344,27 @@ static unsigned int ttm_pool_page_order(struct ttm_pool 
*pool, struct page *p)
return p->private;
 }
 
+/* Called when we got a page, either from a pool or newly allocated */
+int ttm_pool_page_allocated(struct ttm_pool *pool, unsigned int order,
+   struct page *p, dma_addr_t **dma_addr,
+   unsigned long *num_pages, struct page ***pages)
+{
+   unsigned int i;
+   int r;
+
+   if (*dma_addr) {
+   r = ttm_pool_map(pool, order, p, dma_addr);
+   if (r)
+   return r;
+   }
+
+   *num_pages -= 1 << order;
+   for (i = 1 << order; i; --i)
+   *((*pages)++) = p++;
+
+   return 0;
+}
+
 /**
  * ttm_pool_alloc - Fill a ttm_tt object
  *
@@ -385,45 +406,57 @@ int ttm_pool_alloc(struct ttm_pool *pool, struct ttm_tt 
*tt,
for (order = min_t(unsigned int, MAX_ORDER - 1, __fls(num_pages));
 num_pages;
 order = min_t(unsigned int, order, __fls(num_pages))) {
-   bool apply_caching = false;
struct ttm_pool_type *pt;
 
pt = ttm_pool_select_type(pool, tt->caching, order);
p = pt ? ttm_pool_type_take(pt) : NULL;
if (p) {
-   apply_caching = true;
-   } else {
-   p = ttm_pool_alloc_page(pool, gfp_flags, order);
-   if (p && PageHighMem(p))
-   apply_caching = true;
-   }
-
-   if (!p) {
-   if (order) {
-   --order;
-   continue;
-   }
-   r = -ENOMEM;
-   goto error_free_all;
-   }
-
-   if (apply_caching) {
r = ttm_pool_apply_caching(caching, pages,
   tt->caching);
if (r)
goto error_free_page;
-   caching = pages + (1 << order);
+
+   while (p) {
+   r = ttm_pool_page_allocated(pool, order, p,
+   &dma_addr,
+   &num_pages,
+   &pages);
+   if (r)
+   goto error_free_page;
+
+   if (num_pages < (1 << order))
+   break;
+
+   p = ttm_pool_type_take(pt);
+   }
+   caching = pages;
}
 
-   if (dma_addr) {
-   r = ttm_pool_map(pool, order, p, &dma_addr);
+   while (num_pages >= (1 << order) &&
+  (p = ttm_pool_alloc_page(pool, gfp_flags, order))) {
+
+   if (PageHighMem(p)) {
+   r = ttm_pool_apply_caching(caching, pages,
+  tt->caching);
+   if (r)
+   goto error_free_page;
+   }
+   r = ttm_pool_page_allocated(pool, order, p, &dma_addr,
+   &num_pages, &pages);
if (r)
goto error_free_page;
+   if (PageHighMem(p))
+   caching = pages;
}
 
-   num_pages -= 1 << order;
-   for (i = 1 << order; i; --i)
-   *(pages++) = p++;
+   if (!p) {
+   if (order) {
+   --order;
+   continue;
+   }
+   r = -ENOMEM;
+   goto error_free_all;
+   }
}
 
r = ttm_pool_apply_caching(caching, pages, tt->caching);
-- 
2.34.1

Re: [PATCH RFC 05/19] mm: add early FAULT_FLAG_WRITE consistency checks

2022-11-07 Thread Nadav Amit

On Nov 7, 2022, at 11:27 AM, David Hildenbrand  wrote:

> !! External Email
> 
> On 07.11.22 20:03, Nadav Amit wrote:
>> On Nov 7, 2022, at 8:17 AM, David Hildenbrand  wrote:
>> 
>>> !! External Email
>>> 
>>> Let's catch abuse of FAULT_FLAG_WRITE early, such that we don't have to
>>> care in all other handlers and might get "surprises" if we forget to do
>>> so.
>>> 
>>> Write faults without VM_MAYWRITE don't make any sense, and our
>>> maybe_mkwrite() logic could have hidden such abuse for now.
>>> 
>>> Write faults without VM_WRITE on something that is not a COW mapping is
>>> similarly broken, and e.g., do_wp_page() could end up placing an
>>> anonymous page into a shared mapping, which would be bad.
>>> 
>>> This is a preparation for reliable R/O long-term pinning of pages in
>>> private mappings, whereby we want to make sure that we will never break
>>> COW in a read-only private mapping.
>>> 
>>> Signed-off-by: David Hildenbrand 
>>> ---
>>> mm/memory.c | 8 
>>> 1 file changed, 8 insertions(+)
>>> 
>>> diff --git a/mm/memory.c b/mm/memory.c
>>> index fe131273217a..826353da7b23 100644
>>> --- a/mm/memory.c
>>> +++ b/mm/memory.c
>>> @@ -5159,6 +5159,14 @@ static vm_fault_t sanitize_fault_flags(struct 
>>> vm_area_struct *vma,
>>> */
>>>if (!is_cow_mapping(vma->vm_flags))
>>>*flags &= ~FAULT_FLAG_UNSHARE;
>>> +   } else if (*flags & FAULT_FLAG_WRITE) {
>>> +   /* Write faults on read-only mappings are impossible ... */
>>> +   if (WARN_ON_ONCE(!(vma->vm_flags & VM_MAYWRITE)))
>>> +   return VM_FAULT_SIGSEGV;
>>> +   /* ... and FOLL_FORCE only applies to COW mappings. */
>>> +   if (WARN_ON_ONCE(!(vma->vm_flags & VM_WRITE) &&
>>> +!is_cow_mapping(vma->vm_flags)))
>>> +   return VM_FAULT_SIGSEGV;
>> 
>> Not sure about the WARN_*(). Seems as if it might trigger in benign even if
>> rare scenarios, e.g., mprotect() racing with page-fault.
> 
> We most certainly would want to catch any such broken/racy cases. There
> are no benign cases I could possibly think of.
> 
> Page faults need the mmap lock in read. mprotect() / VMA changes need
> the mmap lock in write. Whoever calls handle_mm_fault() is supposed to
> properly check VMA permissions.

My bad. I now see it. Thanks for explaining.

Re: [Intel-gfx] [PATCH] drm/i915: Don't wait forever in drop_caches

2022-11-07 Thread John Harrison


On 11/7/2022 06:09, Tvrtko Ursulin wrote:

On 04/11/2022 17:45, John Harrison wrote:

On 11/4/2022 03:01, Tvrtko Ursulin wrote:

On 03/11/2022 19:16, John Harrison wrote:

On 11/3/2022 02:38, Tvrtko Ursulin wrote:

On 03/11/2022 09:18, Tvrtko Ursulin wrote:

On 03/11/2022 01:33, John Harrison wrote:

On 11/2/2022 07:20, Tvrtko Ursulin wrote:

On 02/11/2022 12:12, Jani Nikula wrote:

On Tue, 01 Nov 2022, john.c.harri...@intel.com wrote:

From: John Harrison 

At the end of each test, IGT does a drop caches call via 
sysfs with


sysfs?
Sorry, that was meant to say debugfs. I've also been working on 
some sysfs IGT issues and evidently got my wires crossed!




special flags set. One of the possible paths waits for idle 
with an
infinite timeout. That causes problems for debugging issues 
when CI
catches a "can't go idle" test failure. Best case, the CI 
system times

out (after 90s), attempts a bunch of state dump actions and then
reboots the system to recover it. Worst case, the CI system 
can't do
anything at all and then times out (after 1000s) and simply 
reboots.
Sometimes a serial port log of dmesg might be available, 
sometimes not.


So rather than making life hard for ourselves, change the 
timeout to

be 10s rather than infinite. Also, trigger the standard
wedge/reset/recover sequence so that testing can continue with a
working system (if possible).

Signed-off-by: John Harrison 
---
  drivers/gpu/drm/i915/i915_debugfs.c | 7 ++-
  1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
b/drivers/gpu/drm/i915/i915_debugfs.c

index ae987e92251dd..9d916fbbfc27c 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -641,6 +641,9 @@ 
DEFINE_SIMPLE_ATTRIBUTE(i915_perf_noa_delay_fops,

    DROP_RESET_ACTIVE | \
    DROP_RESET_SEQNO | \
    DROP_RCU)
+
+#define DROP_IDLE_TIMEOUT    (HZ * 10)


I915_IDLE_ENGINES_TIMEOUT is defined in i915_drv.h. It's also 
only used

here.


So move here, dropping i915 prefix, next to the newly proposed 
one?

Sure, can do that.




I915_GEM_IDLE_TIMEOUT is defined in i915_gem.h. It's only used in
gt/intel_gt.c.


Move there and rename to GT_IDLE_TIMEOUT?

I915_GT_SUSPEND_IDLE_TIMEOUT is defined and used only in 
intel_gt_pm.c.


No action needed, maybe drop i915 prefix if wanted.

These two are totally unrelated and in code not being touched by 
this change. I would rather not conflate changing random other 
things with fixing this specific issue.



I915_IDLE_ENGINES_TIMEOUT is in ms, the rest are in jiffies.


Add _MS suffix if wanted.


My head spins.


I follow and raise that the newly proposed DROP_IDLE_TIMEOUT 
applies to DROP_ACTIVE and not only DROP_IDLE.
My original intention for the name was that is the 'drop caches 
timeout for intel_gt_wait_for_idle'. Which is quite the mouthful 
and hence abbreviated to DROP_IDLE_TIMEOUT. But yes, I realised 
later that name can be conflated with the DROP_IDLE flag. Will 
rename.





Things get refactored, code moves around, bits get left behind, 
who knows. No reason to get too worked up. :) As long as people 
are taking a wider view when touching the code base, and are 
not afraid to send cleanups, things should be good.
On the other hand, if every patch gets blocked in code review 
because someone points out some completely unrelated piece of 
code could be a bit better then nothing ever gets fixed. If you 
spot something that you think should be improved, isn't the 
general idea that you should post a patch yourself to improve it?


There's two maintainers per branch and an order of magnitude or 
two more developers so it'd be nice if cleanups would just be 
incoming on self-initiative basis. ;)


For the actual functional change at hand - it would be nice if 
code paths in question could handle SIGINT and then we could 
punt the decision on how long someone wants to wait purely to 
userspace. But it's probably hard and it's only debugfs so 
whatever.


The code paths in question will already abort on a signal won't 
they? Both intel_gt_wait_for_idle() and 
intel_guc_wait_for_pending_msg(), which is where the 
uc_wait_for_idle eventually ends up, have an 'if(signal_pending) 
return -EINTR;' check. Beyond that, it sounds like what you are 
asking for is a change in the IGT libraries and/or CI framework 
to start sending signals after some specific timeout. That seems 
like a significantly more complex change (in terms of the number 
of entities affected and number of groups involved) and 
unnecessary.


If you say so, I haven't looked at them all. But if the code path 
in question already aborts on signals then I am not sure what is 
the patch fixing? I assumed you are trying to avoid the write 
stuck in D forever, which then prevents driver unload and 
everything, requiring the test runner to eventually reboot. If 
you say SIGINT works then you can already recover from userspace, 
no?


Whether or not 10s is eno

Re: [PATCH 24/26] drm: gm12u320: Remove #ifdef guards for PM related functions

2022-11-07 Thread Hans de Goede

Hi,

On 11/7/22 18:55, Paul Cercueil wrote:
> Use the pm_ptr() macro to handle the .suspend / .resume / .reset_resume
> callbacks.
> 
> This macro allows the suspend and resume functions to be automatically
> dropped by the compiler when CONFIG_PM is disabled, without having
> to use #ifdef guards.
> 
> This has the advantage of always compiling these functions in,
> independently of any Kconfig option. Thanks to that, bugs and other
> regressions are subsequently easier to catch. It also allows to drop the
> __maybe_unused tags.
> 
> Signed-off-by: Paul Cercueil 
> ---
> Cc: Hans de Goede 

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede 

Regards,

Hans


> ---
>  drivers/gpu/drm/tiny/gm12u320.c | 15 +++
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tiny/gm12u320.c b/drivers/gpu/drm/tiny/gm12u320.c
> index 7441d992a5d7..0a901201142e 100644
> --- a/drivers/gpu/drm/tiny/gm12u320.c
> +++ b/drivers/gpu/drm/tiny/gm12u320.c
> @@ -4,6 +4,7 @@
>   */
>  
>  #include 
> +#include 
>  #include 
>  
>  #include 
> @@ -718,15 +719,15 @@ static void gm12u320_usb_disconnect(struct 
> usb_interface *interface)
>   drm_atomic_helper_shutdown(dev);
>  }
>  
> -static __maybe_unused int gm12u320_suspend(struct usb_interface *interface,
> -pm_message_t message)
> +static int gm12u320_suspend(struct usb_interface *interface,
> + pm_message_t message)
>  {
>   struct drm_device *dev = usb_get_intfdata(interface);
>  
>   return drm_mode_config_helper_suspend(dev);
>  }
>  
> -static __maybe_unused int gm12u320_resume(struct usb_interface *interface)
> +static int gm12u320_resume(struct usb_interface *interface)
>  {
>   struct drm_device *dev = usb_get_intfdata(interface);
>   struct gm12u320_device *gm12u320 = to_gm12u320(dev);
> @@ -747,11 +748,9 @@ static struct usb_driver gm12u320_usb_driver = {
>   .probe = gm12u320_usb_probe,
>   .disconnect = gm12u320_usb_disconnect,
>   .id_table = id_table,
> -#ifdef CONFIG_PM
> - .suspend = gm12u320_suspend,
> - .resume = gm12u320_resume,
> - .reset_resume = gm12u320_resume,
> -#endif
> + .suspend = pm_ptr(gm12u320_suspend),
> + .resume = pm_ptr(gm12u320_resume),
> + .reset_resume = pm_ptr(gm12u320_resume),
>  };
>  
>  module_usb_driver(gm12u320_usb_driver);

Re: [PATCH 22/26] drm: vboxvideo: Remove #ifdef guards for PM related functions

2022-11-07 Thread Hans de Goede

Hi,

On 11/7/22 18:52, Paul Cercueil wrote:
> Use the pm_sleep_ptr() macro to handle the .suspend / .resume callbacks.
> 
> This macro allows the suspend and resume functions to be automatically
> dropped by the compiler when CONFIG_SUSPEND is disabled, without having
> to use #ifdef guards.
> 
> This has the advantage of always compiling these functions in,
> independently of any Kconfig option. Thanks to that, bugs and other
> regressions are subsequently easier to catch.
> 
> Signed-off-by: Paul Cercueil 
> ---
> Cc: Hans de Goede 

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede 

Regards,

Hans


> ---
>  drivers/gpu/drm/vboxvideo/vbox_drv.c | 6 +-
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vboxvideo/vbox_drv.c 
> b/drivers/gpu/drm/vboxvideo/vbox_drv.c
> index f4f2bd79a7cb..79318441ed7e 100644
> --- a/drivers/gpu/drm/vboxvideo/vbox_drv.c
> +++ b/drivers/gpu/drm/vboxvideo/vbox_drv.c
> @@ -102,7 +102,6 @@ static void vbox_pci_remove(struct pci_dev *pdev)
>   vbox_hw_fini(vbox);
>  }
>  
> -#ifdef CONFIG_PM_SLEEP
>  static int vbox_pm_suspend(struct device *dev)
>  {
>   struct vbox_private *vbox = dev_get_drvdata(dev);
> @@ -160,16 +159,13 @@ static const struct dev_pm_ops vbox_pm_ops = {
>   .poweroff = vbox_pm_poweroff,
>   .restore = vbox_pm_resume,
>  };
> -#endif
>  
>  static struct pci_driver vbox_pci_driver = {
>   .name = DRIVER_NAME,
>   .id_table = pciidlist,
>   .probe = vbox_pci_probe,
>   .remove = vbox_pci_remove,
> -#ifdef CONFIG_PM_SLEEP
> - .driver.pm = &vbox_pm_ops,
> -#endif
> + .driver.pm = pm_sleep_ptr(&vbox_pm_ops),
>  };
>  
>  DEFINE_DRM_GEM_FOPS(vbox_fops);

Re: [RFC PATCH 0/3] Support for Solid Fill Planes

2022-11-07 Thread Ville Syrjälä

On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote:
> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT
> properties. When the color fill value is set, and the framebuffer is set
> to NULL, memory fetch will be disabled.

Thinking a bit more universally I wonder if there should be
some kind of enum property:

enum plane_pixel_source {
FB,
COLOR,
LIVE_FOO,
LIVE_BAR,
...
}

> In addition, loosen the NULL FB checks within the atomic commit callstack
> to allow a NULL FB when color_fill is nonzero and add FB checks in
> methods where the FB was previously assumed to be non-NULL.
> 
> Finally, have the DPU driver use drm_plane_state.color_fill and
> drm_plane_state.color_fill_format instead of dpu_plane_state.color_fill,
> and add extra checks in the DPU atomic commit callstack to account for a
> NULL FB in cases where color_fill is set.
> 
> 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.
> 
> Userspace can set the color_fill value by setting COLOR_FILL_FORMAT to a
> DRM format, setting COLOR_FILL to a color fill value, and setting the
> framebuffer to NULL.

Is there some real reason for the format property? Ie. why not 
just do what was the plan for the crttc background color and
specify the color in full 16bpc format and just pick as many
msbs from that as the hw can use?

> 
> Jessica Zhang (3):
>   drm: Introduce color fill properties for drm plane
>   drm: Adjust atomic checks for solid fill color
>   drm/msm/dpu: Use color_fill property for DPU planes
> 
>  drivers/gpu/drm/drm_atomic.c  | 68 ---
>  drivers/gpu/drm/drm_atomic_helper.c   | 34 +++-
>  drivers/gpu/drm/drm_atomic_uapi.c |  8 +++
>  drivers/gpu/drm/drm_blend.c   | 38 +
>  drivers/gpu/drm/drm_plane.c   |  8 +--
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  |  7 ++-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 66 ++
>  include/drm/drm_atomic_helper.h   |  5 +-
>  include/drm/drm_blend.h   |  2 +
>  include/drm/drm_plane.h   | 28 ++
>  10 files changed, 188 insertions(+), 76 deletions(-)
> 
> -- 
> 2.38.0

-- 
Ville Syrjälä
Intel

Re: [RFC PATCH v2 1/3] drivers/accel: define kconfig and register a new major

On Mon, Nov 7, 2022 at 6:31 PM Jason Gunthorpe  wrote:
>
> On Mon, Nov 07, 2022 at 05:53:55PM +0200, Oded Gabbay wrote:
> > On Mon, Nov 7, 2022 at 4:10 PM Jason Gunthorpe  wrote:
> > >
> > > On Mon, Nov 07, 2022 at 04:02:01PM +0200, Oded Gabbay wrote:
> > > > On Mon, Nov 7, 2022 at 3:10 PM Jason Gunthorpe  wrote:
> > > > >
> > > > > On Mon, Nov 07, 2022 at 03:01:08PM +0200, Oded Gabbay wrote:
> > > > > > I don't agree with your statement that it should be "a layer over 
> > > > > > top of DRM".
> > > > > > Anything on top of DRM is a device driver.
> > > > > > Accel is not a device driver, it is a new type of drm minor / drm 
> > > > > > driver.
> > > > >
> > > > > Yeah, I still think this is not the right way, you are getting almost
> > > > > nothing from DRM and making everything more complicated in the
> > > > > process.
> > > > >
> > > > > > The only alternative imo to that is to abandon the idea of reusing
> > > > > > drm, and just make an independant accel core code.
> > > > >
> > > > > Not quite really, layer it properly and librarize parts of DRM into
> > > > > things accel can re-use so they are not intimately tied to the DRM
> > > > > struct device notion.
> > > > >
> > > > > IMHO this is much better, because accel has very little need of DRM to
> > > > > manage a struct device/cdev in the first place.
> > > > >
> > > > > Jason
> > > > I'm not following. How can an accel device be a new type of drm_minor,
> > > > if it doesn't have access to all its functions and members ?
> > >
> > > "drm_minor" is not necessary anymore. Strictly managing minor numbers
> > > lost its value years ago when /dev/ was reorganized. Just use
> > > dynamic minors fully.
> > drm minor is not just about handling minor numbers. It contains the
> > entire code to manage devices that register with drm framework (e.g.
> > supply callbacks to file operations), manage their lifecycle,
> > resources (e.g. automatic free of resources on release), sysfs,
> > debugfs, etc.
>
> This is why you are having such troubles, this is already good library
> code. You don't need DRM to wrapper debugfs APIs, for instance. We
> have devm, though maybe it is not a good idea, etc
>
> Greg already pointed out the sysfs was not being done correctly
> anyhow.
>
> I don't think DRM is improving on these core kernel services. Just use
> the normal stuff directly.
I get what you are saying but if I do all that, then how is this
subsystem related to DRM and re-using its code ? (at least at this
stage)
btw, using the basic stuff directly was my original intention, if you
remember the original accel mail thread from July/August.
And then we all decided in LPC that we shouldn't do that and instead
accel should use the DRM code and just expose a new major+minor for
the new drivers.

So, something doesn't add up...
imo, we need to choose between doing accel either as a small new
feature in drm, or as an independent subsystem.
I just don't see how I do the former without calling drm code directly
and using all its wrappers.

>
> > > > How will accel device leverage, for example, the GEM code without
> > > > being a drm_minor ?
> > >
> > > Split GEM into a library so it doesn't require that.
> > I don't see the advantage of doing that over defining accel as a new
> > type of drm minor.
>
> Making things into smaller libraries is recognized as a far better
> kernel approach than trying to make a gigantic wide midlayer that stuffs
> itself into everything. LWN called this the "midlayer mistake" and
> wrote about the pitfalls a long time ago:
>
> https://lwn.net/Articles/336262/
>
> It is exactly what you are experiencing trying to stretch a
> midlayer even further out.
>
> Jason
I'm all for breaking it down to smaller libraries, I completely agree with you.
But as you wrote above, why do I even need to use the drm wrappers for
the basic stuff ? I'll just call the kernel api directly.
And if that's the case then I don't need to rip that code out of the
heart of drm and make it a separate module.

For GEM (as an example of something less basic) it might be a
different story, but we are not there yet.

Oded

Re: [PATCH RFC 05/19] mm: add early FAULT_FLAG_WRITE consistency checks

2022-11-07 Thread David Hildenbrand


On 07.11.22 20:03, Nadav Amit wrote:

On Nov 7, 2022, at 8:17 AM, David Hildenbrand  wrote:


!! External Email

Let's catch abuse of FAULT_FLAG_WRITE early, such that we don't have to
care in all other handlers and might get "surprises" if we forget to do
so.

Write faults without VM_MAYWRITE don't make any sense, and our
maybe_mkwrite() logic could have hidden such abuse for now.

Write faults without VM_WRITE on something that is not a COW mapping is
similarly broken, and e.g., do_wp_page() could end up placing an
anonymous page into a shared mapping, which would be bad.

This is a preparation for reliable R/O long-term pinning of pages in
private mappings, whereby we want to make sure that we will never break
COW in a read-only private mapping.

Signed-off-by: David Hildenbrand 
---
mm/memory.c | 8 
1 file changed, 8 insertions(+)

diff --git a/mm/memory.c b/mm/memory.c
index fe131273217a..826353da7b23 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -5159,6 +5159,14 @@ static vm_fault_t sanitize_fault_flags(struct 
vm_area_struct *vma,
 */
if (!is_cow_mapping(vma->vm_flags))
*flags &= ~FAULT_FLAG_UNSHARE;
+   } else if (*flags & FAULT_FLAG_WRITE) {
+   /* Write faults on read-only mappings are impossible ... */
+   if (WARN_ON_ONCE(!(vma->vm_flags & VM_MAYWRITE)))
+   return VM_FAULT_SIGSEGV;
+   /* ... and FOLL_FORCE only applies to COW mappings. */
+   if (WARN_ON_ONCE(!(vma->vm_flags & VM_WRITE) &&
+!is_cow_mapping(vma->vm_flags)))
+   return VM_FAULT_SIGSEGV;


Not sure about the WARN_*(). Seems as if it might trigger in benign even if
rare scenarios, e.g., mprotect() racing with page-fault.



We most certainly would want to catch any such broken/racy cases. There 
are no benign cases I could possibly think of.


Page faults need the mmap lock in read. mprotect() / VMA changes need 
the mmap lock in write. Whoever calls handle_mm_fault() is supposed to 
properly check VMA permissions.



--
Thanks,

David / dhildenb

[PATCH v2 5/7] drm/mtk: Use drm_mode_init() for on-stack modes

From: Ville Syrjälä 

Initialize on-stack modes with drm_mode_init() to guarantee
no stack garbage in the list head.

Based on the following cocci script, with manual fixups:
@decl@
identifier M;
expression E;
@@
- struct drm_display_mode M = E;
+ struct drm_display_mode M;

@@
identifier decl.M;
expression decl.E;
statement S, S1;
@@
struct drm_display_mode M;
... when != S
+ drm_mode_init(&M, &E);
+
S1

@@
expression decl.E;
@@
- &*E
+ E

Cc: Chun-Kuang Hu 
Cc: Philipp Zabel 
Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/mediatek/mtk_hdmi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c 
b/drivers/gpu/drm/mediatek/mtk_hdmi.c
index 4c80b6896dc3..12fa78f286ff 100644
--- a/drivers/gpu/drm/mediatek/mtk_hdmi.c
+++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c
@@ -1217,7 +1217,7 @@ static int mtk_hdmi_bridge_mode_valid(struct drm_bridge 
*bridge,
if (next_bridge) {
struct drm_display_mode adjusted_mode;
 
-   drm_mode_copy(&adjusted_mode, mode);
+   drm_mode_init(&adjusted_mode, mode);
if (!drm_bridge_chain_mode_fixup(next_bridge, mode,
 &adjusted_mode))
return MODE_BAD;
-- 
2.37.4

[PATCH v2 4/7] drm/msm: Use drm_mode_copy()

From: Ville Syrjälä 

struct drm_display_mode embeds a list head, so overwriting
the full struct with another one will corrupt the list
(if the destination mode is on a list). Use drm_mode_copy()
instead which explicitly preserves the list head of
the destination mode.

Even if we know the destination mode is not on any list
using drm_mode_copy() seems decent as it sets a good
example. Bad examples of not using it might eventually
get copied into code where preserving the list head
actually matters.

Obviously one case not covered here is when the mode
itself is embedded in a larger structure and the whole
structure is copied. But if we are careful when copying
into modes embedded in structures I think we can be a
little more reassured that bogus list heads haven't been
propagated in.

@is_mode_copy@
@@
drm_mode_copy(...)
{
...
}

@depends on !is_mode_copy@
struct drm_display_mode *mode;
expression E, S;
@@
(
- *mode = E
+ drm_mode_copy(mode, &E)
|
- memcpy(mode, E, S)
+ drm_mode_copy(mode, E)
)

@depends on !is_mode_copy@
struct drm_display_mode mode;
expression E;
@@
(
- mode = E
+ drm_mode_copy(&mode, &E)
|
- memcpy(&mode, E, S)
+ drm_mode_copy(&mode, E)
)

@@
struct drm_display_mode *mode;
@@
- &*mode
+ mode

Cc: Rob Clark 
Cc: Sean Paul 
Cc: Abhinav Kumar 
Cc: linux-arm-...@vger.kernel.org
Cc: freedr...@lists.freedesktop.org
Reviewed-by: Dmitry Baryshkov 
Reviewed-by: Abhinav Kumar 
Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/msm/dp/dp_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c
index a49f6dbbe888..c9d9b384ddd0 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -857,7 +857,7 @@ static int dp_display_set_mode(struct msm_dp *dp_display,
 
dp = container_of(dp_display, struct dp_display_private, dp_display);
 
-   dp->panel->dp_mode.drm_mode = mode->drm_mode;
+   drm_mode_copy(&dp->panel->dp_mode.drm_mode, &mode->drm_mode);
dp->panel->dp_mode.bpp = mode->bpp;
dp->panel->dp_mode.capabilities = mode->capabilities;
dp_panel_init_panel_info(dp->panel);
-- 
2.37.4

[PATCH v2 7/7] drm/sti: Use drm_mode_copy()

From: Ville Syrjälä 

struct drm_display_mode embeds a list head, so overwriting
the full struct with another one will corrupt the list
(if the destination mode is on a list). Use drm_mode_copy()
instead which explicitly preserves the list head of
the destination mode.

Even if we know the destination mode is not on any list
using drm_mode_copy() seems decent as it sets a good
example. Bad examples of not using it might eventually
get copied into code where preserving the list head
actually matters.

Obviously one case not covered here is when the mode
itself is embedded in a larger structure and the whole
structure is copied. But if we are careful when copying
into modes embedded in structures I think we can be a
little more reassured that bogus list heads haven't been
propagated in.

@is_mode_copy@
@@
drm_mode_copy(...)
{
...
}

@depends on !is_mode_copy@
struct drm_display_mode *mode;
expression E, S;
@@
(
- *mode = E
+ drm_mode_copy(mode, &E)
|
- memcpy(mode, E, S)
+ drm_mode_copy(mode, E)
)

@depends on !is_mode_copy@
struct drm_display_mode mode;
expression E;
@@
(
- mode = E
+ drm_mode_copy(&mode, &E)
|
- memcpy(&mode, E, S)
+ drm_mode_copy(&mode, E)
)

@@
struct drm_display_mode *mode;
@@
- &*mode
+ mode

Cc: Alain Volmat 
Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/sti/sti_dvo.c  | 2 +-
 drivers/gpu/drm/sti/sti_hda.c  | 2 +-
 drivers/gpu/drm/sti/sti_hdmi.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/sti/sti_dvo.c b/drivers/gpu/drm/sti/sti_dvo.c
index b6ee8a82e656..f3a5616b7daf 100644
--- a/drivers/gpu/drm/sti/sti_dvo.c
+++ b/drivers/gpu/drm/sti/sti_dvo.c
@@ -288,7 +288,7 @@ static void sti_dvo_set_mode(struct drm_bridge *bridge,
 
DRM_DEBUG_DRIVER("\n");
 
-   memcpy(&dvo->mode, mode, sizeof(struct drm_display_mode));
+   drm_mode_copy(&dvo->mode, mode);
 
/* According to the path used (main or aux), the dvo clocks should
 * have a different parent clock. */
diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c
index 03cc401ed593..ec6656b9ee7c 100644
--- a/drivers/gpu/drm/sti/sti_hda.c
+++ b/drivers/gpu/drm/sti/sti_hda.c
@@ -524,7 +524,7 @@ static void sti_hda_set_mode(struct drm_bridge *bridge,
 
DRM_DEBUG_DRIVER("\n");
 
-   memcpy(&hda->mode, mode, sizeof(struct drm_display_mode));
+   drm_mode_copy(&hda->mode, mode);
 
if (!hda_get_mode_idx(hda->mode, &mode_idx)) {
DRM_ERROR("Undefined mode\n");
diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
index cb82622877d2..fcc2194869d6 100644
--- a/drivers/gpu/drm/sti/sti_hdmi.c
+++ b/drivers/gpu/drm/sti/sti_hdmi.c
@@ -941,7 +941,7 @@ static void sti_hdmi_set_mode(struct drm_bridge *bridge,
DRM_DEBUG_DRIVER("\n");
 
/* Copy the drm display mode in the connector local structure */
-   memcpy(&hdmi->mode, mode, sizeof(struct drm_display_mode));
+   drm_mode_copy(&hdmi->mode, mode);
 
/* Update clock framerate according to the selected mode */
ret = clk_set_rate(hdmi->clk_pix, mode->clock * 1000);
-- 
2.37.4

[PATCH v2 6/7] drm/rockchip: Use drm_mode_copy()

From: Ville Syrjälä 

struct drm_display_mode embeds a list head, so overwriting
the full struct with another one will corrupt the list
(if the destination mode is on a list). Use drm_mode_copy()
instead which explicitly preserves the list head of
the destination mode.

Even if we know the destination mode is not on any list
using drm_mode_copy() seems decent as it sets a good
example. Bad examples of not using it might eventually
get copied into code where preserving the list head
actually matters.

Obviously one case not covered here is when the mode
itself is embedded in a larger structure and the whole
structure is copied. But if we are careful when copying
into modes embedded in structures I think we can be a
little more reassured that bogus list heads haven't been
propagated in.

@is_mode_copy@
@@
drm_mode_copy(...)
{
...
}

@depends on !is_mode_copy@
struct drm_display_mode *mode;
expression E, S;
@@
(
- *mode = E
+ drm_mode_copy(mode, &E)
|
- memcpy(mode, E, S)
+ drm_mode_copy(mode, E)
)

@depends on !is_mode_copy@
struct drm_display_mode mode;
expression E;
@@
(
- mode = E
+ drm_mode_copy(&mode, &E)
|
- memcpy(&mode, E, S)
+ drm_mode_copy(&mode, E)
)

@@
struct drm_display_mode *mode;
@@
- &*mode
+ mode

Signed-off-by: Ville Syrjälä 
Cc: Sandy Huang 
Cc: "Heiko Stübner" 
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-rockc...@lists.infradead.org
---
 drivers/gpu/drm/rockchip/cdn-dp-core.c | 2 +-
 drivers/gpu/drm/rockchip/inno_hdmi.c   | 2 +-
 drivers/gpu/drm/rockchip/rk3066_hdmi.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c 
b/drivers/gpu/drm/rockchip/cdn-dp-core.c
index 518ee13b1d6f..8526dda91931 100644
--- a/drivers/gpu/drm/rockchip/cdn-dp-core.c
+++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c
@@ -571,7 +571,7 @@ static void cdn_dp_encoder_mode_set(struct drm_encoder 
*encoder,
video->v_sync_polarity = !!(mode->flags & DRM_MODE_FLAG_NVSYNC);
video->h_sync_polarity = !!(mode->flags & DRM_MODE_FLAG_NHSYNC);
 
-   memcpy(&dp->mode, adjusted, sizeof(*mode));
+   drm_mode_copy(&dp->mode, adjusted);
 }
 
 static bool cdn_dp_check_link_status(struct cdn_dp_device *dp)
diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c 
b/drivers/gpu/drm/rockchip/inno_hdmi.c
index 87b2243ea23e..f51774866f41 100644
--- a/drivers/gpu/drm/rockchip/inno_hdmi.c
+++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
@@ -499,7 +499,7 @@ static void inno_hdmi_encoder_mode_set(struct drm_encoder 
*encoder,
inno_hdmi_setup(hdmi, adj_mode);
 
/* Store the display mode for plugin/DPMS poweron events */
-   memcpy(&hdmi->previous_mode, adj_mode, sizeof(hdmi->previous_mode));
+   drm_mode_copy(&hdmi->previous_mode, adj_mode);
 }
 
 static void inno_hdmi_encoder_enable(struct drm_encoder *encoder)
diff --git a/drivers/gpu/drm/rockchip/rk3066_hdmi.c 
b/drivers/gpu/drm/rockchip/rk3066_hdmi.c
index cf2cf51091a3..90145ad96984 100644
--- a/drivers/gpu/drm/rockchip/rk3066_hdmi.c
+++ b/drivers/gpu/drm/rockchip/rk3066_hdmi.c
@@ -395,7 +395,7 @@ rk3066_hdmi_encoder_mode_set(struct drm_encoder *encoder,
struct rk3066_hdmi *hdmi = encoder_to_rk3066_hdmi(encoder);
 
/* Store the display mode for plugin/DPMS poweron events. */
-   memcpy(&hdmi->previous_mode, adj_mode, sizeof(hdmi->previous_mode));
+   drm_mode_copy(&hdmi->previous_mode, adj_mode);
 }
 
 static void rk3066_hdmi_encoder_enable(struct drm_encoder *encoder)
-- 
2.37.4

[PATCH v2 3/7] drm/msm: Use drm_mode_init() for on-stack modes

From: Ville Syrjälä 

Initialize on-stack modes with drm_mode_init() to guarantee
no stack garbage in the list head, or that we aren't copying
over another mode's list head.

Based on the following cocci script, with manual fixups:
@decl@
identifier M;
expression E;
@@
- struct drm_display_mode M = E;
+ struct drm_display_mode M;

@@
identifier decl.M;
expression decl.E;
statement S, S1;
@@
struct drm_display_mode M;
... when != S
+ drm_mode_init(&M, &E);
+
S1

@@
expression decl.E;
@@
- &*E
+ E

Cc: Rob Clark 
Cc: Sean Paul 
Cc: Abhinav Kumar 
Cc: linux-arm-...@vger.kernel.org
Cc: freedr...@lists.freedesktop.org
Reviewed-by: Dmitry Baryshkov 
Reviewed-by: Abhinav Kumar 
Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
index 2c14646661b7..0f71e8fe7be7 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
@@ -237,12 +237,13 @@ static void dpu_encoder_phys_vid_setup_timing_engine(
unsigned long lock_flags;
struct dpu_hw_intf_cfg intf_cfg = { 0 };
 
+   drm_mode_init(&mode, &phys_enc->cached_mode);
+
if (!phys_enc->hw_ctl->ops.setup_intf_cfg) {
DPU_ERROR("invalid encoder %d\n", phys_enc != NULL);
return;
}
 
-   mode = phys_enc->cached_mode;
if (!phys_enc->hw_intf->ops.setup_timing_gen) {
DPU_ERROR("timing engine setup is not supported\n");
return;
@@ -634,7 +635,9 @@ static int dpu_encoder_phys_vid_get_frame_count(
 {
struct intf_status s = {0};
u32 fetch_start = 0;
-   struct drm_display_mode mode = phys_enc->cached_mode;
+   struct drm_display_mode mode;
+
+   drm_mode_init(&mode, &phys_enc->cached_mode);
 
if (!dpu_encoder_phys_vid_is_master(phys_enc))
return -EINVAL;
-- 
2.37.4

[PATCH v2 2/7] drm/hisilicon: Use drm_mode_init() for on-stack modes

From: Ville Syrjälä 

Initialize on-stack modes with drm_mode_init() to guarantee
no stack garbage in the list head.

Cc: Xinliang Liu 
Cc: Tian Tao 
Cc: John Stultz 
Cc: Xinwei Kong 
Cc: Chen Feng 
Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c 
b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c
index a0d5aa727d58..d9978b79828c 100644
--- a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c
+++ b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c
@@ -658,7 +658,7 @@ static enum drm_mode_status dsi_encoder_mode_valid(struct 
drm_encoder *encoder,
 * reset adj_mode to the mode value each time,
 * so we don't adjust the mode twice
 */
-   drm_mode_copy(&adj_mode, mode);
+   drm_mode_init(&adj_mode, mode);
 
crtc_funcs = crtc->helper_private;
if (crtc_funcs && crtc_funcs->mode_fixup)
-- 
2.37.4

[PATCH v2 1/7] drm/amdgpu: Use drm_mode_init() for on-stack modes

From: Ville Syrjälä 

Initialize on-stack modes with drm_mode_init() to guarantee
no stack garbage in the list head, or that we aren't copying
over another mode's list head.

Based on the following cocci script, with manual fixups:
@decl@
identifier M;
expression E;
@@
- struct drm_display_mode M = E;
+ struct drm_display_mode M;

@@
identifier decl.M;
expression decl.E;
statement S, S1;
@@
struct drm_display_mode M;
... when != S
+ drm_mode_init(&M, &E);
+
S1

@@
expression decl.E;
@@
- &*E
+ E

Cc: Harry Wentland 
Cc: Leo Li 
Cc: Rodrigo Siqueira 
Cc: Alex Deucher 
Cc: amd-...@lists.freedesktop.org
Reviewed-by: Harry Wentland 
Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index d9940a3c64dd..7fa4b61bc5bf 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5685,7 +5685,7 @@ create_stream_for_sink(struct amdgpu_dm_connector 
*aconnector,
const struct drm_connector_state *con_state =
dm_state ? &dm_state->base : NULL;
struct dc_stream_state *stream = NULL;
-   struct drm_display_mode mode = *drm_mode;
+   struct drm_display_mode mode;
struct drm_display_mode saved_mode;
struct drm_display_mode *freesync_mode = NULL;
bool native_mode_found = false;
@@ -5699,6 +5699,7 @@ create_stream_for_sink(struct amdgpu_dm_connector 
*aconnector,
 
struct dc_sink *sink = NULL;
 
+   drm_mode_init(&mode, drm_mode);
memset(&saved_mode, 0, sizeof(saved_mode));
 
if (aconnector == NULL) {
-- 
2.37.4

[PATCH v2 0/7] drm: Review of mode copies

From: Ville Syrjälä 

Repost of the stragglers from
https://patchwork.freedesktop.org/series/100393/

Note that I didn't rerun the cocci stuff, nor have I had
time to come up with something to inluce the cocci scripts
in the tree. So it's possible this is missing some new
things that have snuck in the meantime.

Ville Syrjälä (7):
  drm/amdgpu: Use drm_mode_init() for on-stack modes
  drm/hisilicon: Use drm_mode_init() for on-stack modes
  drm/msm: Use drm_mode_init() for on-stack modes
  drm/msm: Use drm_mode_copy()
  drm/mtk: Use drm_mode_init() for on-stack modes
  drm/rockchip: Use drm_mode_copy()
  drm/sti: Use drm_mode_copy()

 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c| 3 ++-
 drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c | 2 +-
 drivers/gpu/drm/mediatek/mtk_hdmi.c  | 2 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 7 +--
 drivers/gpu/drm/msm/dp/dp_display.c  | 2 +-
 drivers/gpu/drm/rockchip/cdn-dp-core.c   | 2 +-
 drivers/gpu/drm/rockchip/inno_hdmi.c | 2 +-
 drivers/gpu/drm/rockchip/rk3066_hdmi.c   | 2 +-
 drivers/gpu/drm/sti/sti_dvo.c| 2 +-
 drivers/gpu/drm/sti/sti_hda.c| 2 +-
 drivers/gpu/drm/sti/sti_hdmi.c   | 2 +-
 11 files changed, 16 insertions(+), 12 deletions(-)

-- 
2.37.4

Re: [PATCH v5 1/3] drm: Use XArray instead of IDR for minors

On Mon, Nov 7, 2022 at 6:20 PM Matthew Wilcox  wrote:
>
> On Sun, Nov 06, 2022 at 04:51:39PM +0200, Oded Gabbay wrote:
> > I tried executing the following code in a dummy driver I wrote:
>
> You don't need to write a dummy driver; you can write test-cases
> entirely in userspace.  Just add them to lib/test_xarray.c and then
> make -C tools/testing/radix-tree/
>
> > static DEFINE_XARRAY_ALLOC(xa_dummy);
> > void check_xa(void *pdev)
> > {
> >   void *entry;
> >   int ret, index;
> >
> >   ret = xa_alloc(&xa_dummy, &index, NULL, XA_LIMIT(0, 63), GFP_NOWAIT);
> >   if (ret < 0)
> >   return ret;
> >
> >   entry = xa_cmpxchg(&xa_dummy, index, NULL, pdev, GFP_KERNEL);
> >   if (xa_is_err(entry))
> >return;
> >
> >   xa_lock(&xa_dummy);
> >   xa_dev = xa_load(&xa_dummy, index);
> >   xa_unlock(&xa_dummy);
> > }
> >
> > And to my surprise xa_dev is always NULL, when it should be pdev.
> > I believe that because we first allocate the entry with NULL, it is
> > considered a "zero" entry in the XA.
> > And when we replace it, this attribute doesn't change so when we do
> > xa_load, the xa code thinks the entry is a "zero" entry and returns
> > NULL.
>
> There's no "attribute" to mark a zero entry.  It's just a zero entry.
> The problem is that you're cmpxchg'ing against NULL, and it's not NULL,
> it's the ZERO entry.  This is even documented in the test code:
>
> /* cmpxchg sees a reserved entry as ZERO */
> XA_BUG_ON(xa, xa_reserve(xa, 12345678, GFP_KERNEL) != 0);
> XA_BUG_ON(xa, xa_cmpxchg(xa, 12345678, XA_ZERO_ENTRY,
> xa_mk_value(12345678), GFP_NOWAIT) != NULL);
>
> I'm not quite sure why you're using xa_cmpxchg() here anyway; if you
> allocated it, it's yours and you can just xa_store() to it.
>
Hi Matthew,
Thanks for the explanation. I believe Michal's will fix his original
patch and I'll take that code.

Oded

Re: [Intel-gfx] [PATCH 1/2] drm/i915/gt: Add GT oriented dmesg output

2022-11-07 Thread John Harrison


On 11/7/2022 08:17, Tvrtko Ursulin wrote:

On 07/11/2022 09:33, Tvrtko Ursulin wrote:

On 05/11/2022 01:03, Ceraolo Spurio, Daniele wrote:

On 11/4/2022 10:25 AM, john.c.harri...@intel.com wrote:

From: John Harrison 

When trying to analyse bug reports from CI, customers, etc. it can be
difficult to work out exactly what is happening on which GT in a
multi-GT system. So add GT oriented debug/error message wrappers. If
used instead of the drm_ equivalents, you get the same output but with
a GT# prefix on it.

Signed-off-by: John Harrison 


The only downside to this is that we'll print "GT0: " even on 
single-GT devices. We could introduce a gt->info.name and print 
that, so we could have it different per-platform, but IMO it's not 
worth the effort.


Reviewed-by: Daniele Ceraolo Spurio 

I think it might be worth getting an ack from one of the maintainers 
to make sure we're all aligned on transitioning to these new logging 
macro for gt code.


Idea is I think a very good one. First I would suggest standardising 
to lowercase GT in logs because:


$ grep "GT%" i915/ -r
$ grep "gt%" i915/ -r
i915/gt/intel_gt_sysfs.c: gt->i915->sysfs_gt, "gt%d", gt->info.id))
i915/gt/intel_gt_sysfs.c:    "failed to initialize gt%d 
sysfs root\n", gt->info.id);
i915/gt/intel_gt_sysfs_pm.c: "failed to create 
gt%u RC6 sysfs files (%pe)\n",
i915/gt/intel_gt_sysfs_pm.c: "failed to 
create gt%u RC6p sysfs files (%pe)\n",
i915/gt/intel_gt_sysfs_pm.c: "failed to create 
gt%u RPS sysfs files (%pe)",
i915/gt/intel_gt_sysfs_pm.c: "failed to create 
gt%u punit_req_freq_mhz sysfs (%pe)",
i915/gt/intel_gt_sysfs_pm.c: "failed to 
create gt%u throttle sysfs files (%pe)",
i915/gt/intel_gt_sysfs_pm.c: "failed to 
create gt%u media_perf_power_attrs sysfs (%pe)\n",
i915/gt/intel_gt_sysfs_pm.c: "failed to add gt%u 
rps defaults (%pe)\n",
i915/i915_driver.c: drm_err(>->i915->drm, "gt%d: intel_pcode_init 
failed %d\n", id, ret);
i915/i915_hwmon.c:  snprintf(ddat_gt->name, 
sizeof(ddat_gt->name), "i915_gt%u", i);




Just because there are 11 existing instances of one form doesn't mean 
that the 275 instances that are waiting to be converted should be done 
incorrectly. GT is an acronym and should be capitalised.


Besides:
grep -r "GT " i915 | grep '"'
i915/vlv_suspend.c: drm_err(&i915->drm, "timeout disabling 
GT waking\n");
i915/vlv_suspend.c: "timeout waiting for GT wells to 
go %s\n",
i915/vlv_suspend.c: drm_dbg(&i915->drm, "GT register access while GT 
waking disabled\n");
i915/i915_gpu_error.c:  err_printf(m, "GT awake: %s\n", 
str_yes_no(gt->awake));

i915/i915_debugfs.c:    seq_printf(m, "GT awake? %s [%d], %llums\n",
i915/selftests/i915_gem_evict.c: pr_err("Failed to idle GT (on %s)", 
engine->name);

i915/intel_uncore.c:  "GT thread status wait timed out\n");
i915/gt/uc/selftest_guc_multi_lrc.c: drm_err(>->i915->drm, "GT failed 
to idle: %d\n", ret);
i915/gt/uc/selftest_guc.c: drm_err(>->i915->drm, "GT failed to idle: 
%d\n", ret);
i915/gt/uc/selftest_guc.c: drm_err(>->i915->drm, "GT failed to idle: 
%d\n", ret);
i915/gt/intel_gt_mcr.c: * Some GT registers are designed as "multicast" 
or "replicated" registers:
i915/gt/selftest_rps.c: pr_info("%s: rps counted %d C0 
cycles [%lldns] in %lldns [%d cycles], using GT clock frequency of %uKHz\n",
i915/gt/selftest_hangcheck.c:   pr_err("[%s] GT is 
wedged!\n", engine->name);

i915/gt/selftest_hangcheck.c:   pr_err("GT is wedged!\n");
i915/gt/intel_gt_clock_utils.c: "GT clock frequency 
changed, was %uHz, now %uHz!\n",
i915/gt/selftest_engine_pm.c:   pr_err("Unable to flush GT pm 
before test\n");
i915/gt/selftest_engine_pm.c:   pr_err("GT 
failed to idle\n");
i915/i915_sysfs.c:   "failed to register GT sysfs 
directory\n");
i915/intel_uncore.h: * of the basic non-engine GT registers 
(referred to as "GSI" on
i915/intel_uncore.h: * newer platforms, or "GT block" on older 
platforms)?  If so, we'll




Then there is a question of naming. Are we okay with GT_XXX or, do we 
want intel_gt_, or something completely different. I don't have a 
strong opinion at the moment so I'll add some more folks to Cc.


You mean GT_ERR("msg") vs intel_gt_err("msg")? Personally, I would 
prefer just gt_err("msg") to keep it as close to the official drm_* 
versions as possible. Print lines tend to be excessively long already. 
Taking a 'gt' parameter instead of a '>->i915->drm' parameter does 
help with that but it seems like calling the wrapper intel_gt_* is 
shooting ourselves in the foot on that one. And GT_ERR vs gt_err just 
comes down to the fact that it is a macro wrapper and therefore is 
required to be in upper case.


There was a maintainer level mini

Re: Must-Pass Test Suite for KMS drivers

2022-11-07 Thread Rob Clark

On Mon, Nov 7, 2022 at 1:29 AM Maxime Ripard  wrote:
>
> On Thu, Oct 27, 2022 at 08:08:28AM -0700, Rob Clark wrote:
> > On Wed, Oct 26, 2022 at 1:17 AM  wrote:
> > >
> > > Hi Rob,
> > >
> > > On Mon, Oct 24, 2022 at 08:48:15AM -0700, Rob Clark wrote:
> > > > On Mon, Oct 24, 2022 at 5:43 AM  wrote:
> > > > > I've discussing the idea for the past year to add an IGT test suite 
> > > > > that
> > > > > all well-behaved KMS drivers must pass.
> > > > >
> > > > > The main idea behind it comes from v4l2-compliance and cec-compliance,
> > > > > that are being used to validate that the drivers are sane.
> > > > >
> > > > > We should probably start building up the test list, and eventually
> > > > > mandate that all tests pass for all the new KMS drivers we would merge
> > > > > in the kernel, and be run by KCi or similar.
> > > >
> > > > Let's get https://patchwork.freedesktop.org/patch/502641/ merged
> > > > first, that already gives us a mechanism similar to what we use in
> > > > mesa to track pass/fail/flake
> > >
> > > I'm not sure it's a dependency per-se, and I believe both can (and
> > > should) happen separately.
> >
> > Basically my reasoning is that getting IGT green is a process that so
> > far is consisting of equal parts IGT test fixes, to clear out
> > lingering i915'isms, etc, and driver fixes.  Yes, you could do this
> > manually but the drm/ci approach seems like it would make it easier to
> > track, so it is easier to see what tests are being run on which hw,
> > and what the pass/fail/flake status is.  And the expectation files can
> > also be updated as we uprev the igt version being used in CI.
> >
> > I could be biased by how CI has been deployed (IMHO, successfully) in
> > mesa.. my experience there doesn't make me see any value in a
> > "mustpass" list.  But does make me see value in automating and
> > tracking status.  Obviously we want all the tests to pass, but getting
> > there is going to be a process.  Tracking that progress is the thing
> > that is useful now.
>
> Yeah, I understand where you're coming from, and for CI I agree that
> your approach looks like the best one.
>
> It's not what I'm trying to address though.
>
> My issue is that, even though I have a bunch of KMS experience by now,
> every time I need to use IGT, I have exactly zero idea what test I
> need to run to check that a given driver behaves decently.
>
> I have no idea which tests I should run, which tests are supposed to be
> working but can't really because of some intel-specific behavior, which
> tests are skipped but shouldn't, which tests are broken but should be,
> etc.

yeah, I feel your pain.. I think the best suggestion I can make atm is
to compare to the xfails from the other drivers, and if in doubt ask
on #igt

BR,
-R

> I don't want to have a nice table with everything green because there
> was no regression, I want to see which bugs I haven't found out are
> still lingering in my driver. I've been chasing bugs too many times
> where it turned out that there was a test for that in IGT somewhere,
> hidden in a 70k tests haystack with zero documentation.
>
> So, yeah, I get what you're saying, it makes sense, and please go
> forward with drm/ci. I still think we need to find a beginning of a
> solution for the issue I'm talking about.
>
> Maxime

[PATCH] drm/fb-helper: Fix missing kerneldoc include

2022-11-07 Thread Daniel Vetter

This was lost in the code movement done in 8ab59da26bc0
("drm/fb-helper: Move generic fbdev emulation into separate source
file").

Fixes: 8ab59da26bc0 ("drm/fb-helper: Move generic fbdev emulation into separate 
source file")
Cc: Thomas Zimmermann 
Cc: Javier Martinez Canillas 
Signed-off-by: Daniel Vetter 
---
 Documentation/gpu/drm-kms-helpers.rst | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/gpu/drm-kms-helpers.rst 
b/Documentation/gpu/drm-kms-helpers.rst
index dbc85fd7a971..a4860ffd6e86 100644
--- a/Documentation/gpu/drm-kms-helpers.rst
+++ b/Documentation/gpu/drm-kms-helpers.rst
@@ -116,6 +116,9 @@ fbdev Helper Functions Reference
 .. kernel-doc:: drivers/gpu/drm/drm_fb_helper.c
:export:
 
+.. kernel-doc:: drivers/gpu/drm/drm_fbdev_generic.c
+   :export:
+
 format Helper Functions Reference
 =
 
-- 
2.37.2

Re: [PATCH RFC 05/19] mm: add early FAULT_FLAG_WRITE consistency checks

2022-11-07 Thread Nadav Amit

On Nov 7, 2022, at 8:17 AM, David Hildenbrand  wrote:

> !! External Email
> 
> Let's catch abuse of FAULT_FLAG_WRITE early, such that we don't have to
> care in all other handlers and might get "surprises" if we forget to do
> so.
> 
> Write faults without VM_MAYWRITE don't make any sense, and our
> maybe_mkwrite() logic could have hidden such abuse for now.
> 
> Write faults without VM_WRITE on something that is not a COW mapping is
> similarly broken, and e.g., do_wp_page() could end up placing an
> anonymous page into a shared mapping, which would be bad.
> 
> This is a preparation for reliable R/O long-term pinning of pages in
> private mappings, whereby we want to make sure that we will never break
> COW in a read-only private mapping.
> 
> Signed-off-by: David Hildenbrand 
> ---
> mm/memory.c | 8 
> 1 file changed, 8 insertions(+)
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index fe131273217a..826353da7b23 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -5159,6 +5159,14 @@ static vm_fault_t sanitize_fault_flags(struct 
> vm_area_struct *vma,
> */
>if (!is_cow_mapping(vma->vm_flags))
>*flags &= ~FAULT_FLAG_UNSHARE;
> +   } else if (*flags & FAULT_FLAG_WRITE) {
> +   /* Write faults on read-only mappings are impossible ... */
> +   if (WARN_ON_ONCE(!(vma->vm_flags & VM_MAYWRITE)))
> +   return VM_FAULT_SIGSEGV;
> +   /* ... and FOLL_FORCE only applies to COW mappings. */
> +   if (WARN_ON_ONCE(!(vma->vm_flags & VM_WRITE) &&
> +!is_cow_mapping(vma->vm_flags)))
> +   return VM_FAULT_SIGSEGV;

Not sure about the WARN_*(). Seems as if it might trigger in benign even if
rare scenarios, e.g., mprotect() racing with page-fault.

Re: [PATCH 04/10] vfio: Move storage of allow_unsafe_interrupts to vfio_main.c