Re: [PATCH v2] drm/panthor: Display FW version information

[Carsten Haitzler]

+1

I like the interface ver. :)

On 9/6/24 10:40 AM, Steven Price wrote:

The version number output when loading the firmware is actually the
interface version not the version of the firmware itself. Update the
message to make this clearer.

However, the firmware binary has a git SHA embedded into it which can be
used to identify which firmware binary is being loaded. So output this
as a drm_info() so that it's obvious from a dmesg log which firmware
binary is being used.

Reviewed-by: Boris Brezillon 
Reviewed-by: Liviu Dudau 
Signed-off-by: Steven Price 
---
v2:
  * Fix indentation
  * Also update the FW interface message to include "using interface" to
make it clear it's not the FW version
  * Add Reviewed-bys

  drivers/gpu/drm/panthor/panthor_fw.c | 57 +++-
  1 file changed, 56 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panthor/panthor_fw.c 
b/drivers/gpu/drm/panthor/panthor_fw.c
index 857f3f11258a..aea5dd9a4969 100644
--- a/drivers/gpu/drm/panthor/panthor_fw.c
+++ b/drivers/gpu/drm/panthor/panthor_fw.c
@@ -78,6 +78,12 @@ enum panthor_fw_binary_entry_type {
  
  	/** @CSF_FW_BINARY_ENTRY_TYPE_TIMELINE_METADATA: Timeline metadata interface. */

CSF_FW_BINARY_ENTRY_TYPE_TIMELINE_METADATA = 4,
+
+   /**
+* @CSF_FW_BINARY_ENTRY_TYPE_BUILD_INFO_METADATA: Metadata about how
+* the FW binary was built.
+*/
+   CSF_FW_BINARY_ENTRY_TYPE_BUILD_INFO_METADATA = 6
  };
  
  #define CSF_FW_BINARY_ENTRY_TYPE(ehdr)	((ehdr) & 0xff)

@@ -132,6 +138,13 @@ struct panthor_fw_binary_section_entry_hdr {
} data;
  };
  
+struct panthor_fw_build_info_hdr {

+   /** @meta_start: Offset of the build info data in the FW binary */
+   u32 meta_start;
+   /** @meta_size: Size of the build info data in the FW binary */
+   u32 meta_size;
+};
+
  /**
   * struct panthor_fw_binary_iter - Firmware binary iterator
   *
@@ -628,6 +641,46 @@ static int panthor_fw_load_section_entry(struct 
panthor_device *ptdev,
return 0;
  }
  
+static int panthor_fw_read_build_info(struct panthor_device *ptdev,

+ const struct firmware *fw,
+ struct panthor_fw_binary_iter *iter,
+ u32 ehdr)
+{
+   struct panthor_fw_build_info_hdr hdr;
+   char header[9];
+   const char git_sha_header[sizeof(header)] = "git_sha: ";
+   int ret;
+
+   ret = panthor_fw_binary_iter_read(ptdev, iter, &hdr, sizeof(hdr));
+   if (ret)
+   return ret;
+
+   if (hdr.meta_start > fw->size ||
+   hdr.meta_start + hdr.meta_size > fw->size) {
+   drm_err(&ptdev->base, "Firmware build info corrupt\n");
+   /* We don't need the build info, so continue */
+   return 0;
+   }
+
+   if (memcmp(git_sha_header, fw->data + hdr.meta_start,
+  sizeof(git_sha_header))) {
+   /* Not the expected header, this isn't metadata we understand */
+   return 0;
+   }
+
+   /* Check that the git SHA is NULL terminated as expected */
+   if (fw->data[hdr.meta_start + hdr.meta_size - 1] != '\0') {
+   drm_warn(&ptdev->base, "Firmware's git sha is not NULL 
terminated\n");
+   /* Don't treat as fatal */
+   return 0;
+   }
+
+   drm_info(&ptdev->base, "Firmware git sha: %s\n",
+fw->data + hdr.meta_start + sizeof(git_sha_header));
+
+   return 0;
+}
+
  static void
  panthor_reload_fw_sections(struct panthor_device *ptdev, bool full_reload)
  {
@@ -672,6 +725,8 @@ static int panthor_fw_load_entry(struct panthor_device 
*ptdev,
switch (CSF_FW_BINARY_ENTRY_TYPE(ehdr)) {
case CSF_FW_BINARY_ENTRY_TYPE_IFACE:
return panthor_fw_load_section_entry(ptdev, fw, &eiter, ehdr);
+   case CSF_FW_BINARY_ENTRY_TYPE_BUILD_INFO_METADATA:
+   return panthor_fw_read_build_info(ptdev, fw, &eiter, ehdr);
  
  	/* FIXME: handle those entry types? */

case CSF_FW_BINARY_ENTRY_TYPE_CONFIG:
@@ -921,7 +976,7 @@ static int panthor_fw_init_ifaces(struct panthor_device 
*ptdev)
return ret;
}
  
-	drm_info(&ptdev->base, "CSF FW v%d.%d.%d, Features %#x Instrumentation features %#x",

+   drm_info(&ptdev->base, "CSF FW using interface v%d.%d.%d, Features %#x 
Instrumentation features %#x",
 CSF_IFACE_VERSION_MAJOR(glb_iface->control->version),
 CSF_IFACE_VERSION_MINOR(glb_iface->control->version),
 CSF_IFACE_VERSION_PATCH(glb_iface->control->version),

Re: [PATCH] drm/panthor: Display FW version information

[Carsten Haitzler]

On 9/5/24 10:08 PM, Liviu Dudau wrote:

On Thu, Sep 05, 2024 at 06:45:28PM +0200, Boris Brezillon wrote:

On Thu,  5 Sep 2024 16:51:44 +0100
Steven Price  wrote:


The firmware binary has a git SHA embedded into it which can be used to
identify which firmware binary is being loaded. Output this as a
drm_info() so that it's obvious from a dmesg log which firmware binary
is being used.

Signed-off-by: Steven Price 


Just one formatting issue mentioned below, looks good otherwise.

Reviewed-by: Boris Brezillon 


Reviewed-by: Liviu Dudau 




---
  drivers/gpu/drm/panthor/panthor_fw.c | 55 
  1 file changed, 55 insertions(+)

diff --git a/drivers/gpu/drm/panthor/panthor_fw.c 
b/drivers/gpu/drm/panthor/panthor_fw.c
index 857f3f11258a..ef007287575c 100644
--- a/drivers/gpu/drm/panthor/panthor_fw.c
+++ b/drivers/gpu/drm/panthor/panthor_fw.c
@@ -78,6 +78,12 @@ enum panthor_fw_binary_entry_type {
  
  	/** @CSF_FW_BINARY_ENTRY_TYPE_TIMELINE_METADATA: Timeline metadata interface. */

CSF_FW_BINARY_ENTRY_TYPE_TIMELINE_METADATA = 4,
+
+   /**
+* @CSF_FW_BINARY_ENTRY_TYPE_BUILD_INFO_METADATA: Metadata about how
+* the FW binary was built.
+*/
+   CSF_FW_BINARY_ENTRY_TYPE_BUILD_INFO_METADATA = 6
  };
  
  #define CSF_FW_BINARY_ENTRY_TYPE(ehdr)	((ehdr) & 0xff)

@@ -132,6 +138,13 @@ struct panthor_fw_binary_section_entry_hdr {
} data;
  };
  
+struct panthor_fw_build_info_hdr {

+   /** @meta_start: Offset of the build info data in the FW binary */
+   u32 meta_start;
+   /** @meta_size: Size of the build info data in the FW binary */
+   u32 meta_size;
+};
+
  /**
   * struct panthor_fw_binary_iter - Firmware binary iterator
   *
@@ -628,6 +641,46 @@ static int panthor_fw_load_section_entry(struct 
panthor_device *ptdev,
return 0;
  }
  
+static int panthor_fw_read_build_info(struct panthor_device *ptdev,

+ const struct firmware *fw,
+ struct panthor_fw_binary_iter *iter,
+ u32 ehdr)
+{
+   struct panthor_fw_build_info_hdr hdr;
+   char header[9];
+   const char git_sha_header[sizeof(header)] = "git_sha: ";
+   int ret;
+
+   ret = panthor_fw_binary_iter_read(ptdev, iter, &hdr, sizeof(hdr));
+   if (ret)
+   return ret;
+
+   if (hdr.meta_start > fw->size ||
+   hdr.meta_start + hdr.meta_size > fw->size) {
+   drm_err(&ptdev->base, "Firmware build info corrupt\n");
+   /* We don't need the build info, so continue */
+   return 0;
+   }
+
+   if (memcmp(git_sha_header, fw->data + hdr.meta_start,
+   sizeof(git_sha_header))) {


Indentation seems broken here:

if (memcmp(git_sha_header, fw->data + hdr.meta_start,
   sizeof(git_sha_header))) {


+   /* Not the expected header, this isn't metadata we understand */
+   return 0;
+   }
+
+   /* Check that the git SHA is NULL terminated as expected */
+   if (fw->data[hdr.meta_start + hdr.meta_size - 1] != '\0') {
+   drm_warn(&ptdev->base, "Firmware's git sha is not NULL 
terminated\n");
+   /* Don't treat as fatal */
+   return 0;
+   }
+
+   drm_info(&ptdev->base, "Firmware git sha: %s\n",
+fw->data + hdr.meta_start + sizeof(git_sha_header));


Maybe we should also change the "FW vX.Y.Z" message into "FW interface
vX.Y.Z" to clarify things.


Or something like "FW using interface vX.Y.Z"


+1. I like the idea. It's a bit confusing otherwise.


Best regards,
Liviu




+
+   return 0;
+}
+
  static void
  panthor_reload_fw_sections(struct panthor_device *ptdev, bool full_reload)
  {
@@ -672,6 +725,8 @@ static int panthor_fw_load_entry(struct panthor_device 
*ptdev,
switch (CSF_FW_BINARY_ENTRY_TYPE(ehdr)) {
case CSF_FW_BINARY_ENTRY_TYPE_IFACE:
return panthor_fw_load_section_entry(ptdev, fw, &eiter, ehdr);
+   case CSF_FW_BINARY_ENTRY_TYPE_BUILD_INFO_METADATA:
+   return panthor_fw_read_build_info(ptdev, fw, &eiter, ehdr);
  
  	/* FIXME: handle those entry types? */

case CSF_FW_BINARY_ENTRY_TYPE_CONFIG:

Re: [RFC PATCH] drm: panthor: add dev_coredumpv support

[Carsten Haitzler]

On 7/23/24 5:06 PM, Boris Brezillon wrote:

Hi Steve,

On Mon, 15 Jul 2024 10:12:16 +0100
Steven Price  wrote:


I note it also shows that the "panthor_regs.rs" would ideally be shared.
For arm64 we have been moving to generating system register descriptions
from a text source (see arch/arm64/tools/sysreg) - I'm wondering whether
something similar is needed for Panthor to generate both C and Rust
headers? Although perhaps that's overkill, sysregs are certainly
somewhat more complex.


Just had a long discussion with Daniel regarding this panthor_regs.rs
auto-generation, and, while I agree this is something we'd rather do if
we intend to maintain the C and rust code base forever, I'm not
entirely convinced this is super useful here because:

1. the C code base is meant to be entirely replaced by a rust driver.
Of course, that's not going to happen overnight, so maybe it'd be worth
having this autogen script but...

2. the set of register and register fields seems to be pretty stable.
We might have a few things to update to support v11, v12, etc, but it
doesn't look like the layout will suddenly become completely different.

3. the number of registers and fields is somewhat reasonable, which
means we should be able to catch mistakes during review. And in case
one slip through, it's not the end of the world either because this
stays internal to the kernel driver. We'll either figure it out when
rust-ifying panthor components, or that simply means the register is
not used and the mistake is harmless until the register starts being
used

4. we're still unclear on how GPU registers should be exposed in rust,
so any script we develop is likely to require heavy changes every time
we change our mind


You have a good point. A script sounds nice, but given the restricted 
domain size, it maybe better to be manually maintained. Given that I 
also think the right way to access registers is to do it as safely as 
possible.


So a gpu_write() or gpu_read() are "unsafe" in that you can write 
invalid values to a just about anything in C. If we're trying to harden 
drivers like panthor and make it "impossible" to do the wrong thing, 
then IMHO for example MCU_CONTROL should be abstracted so I can ONLY 
write MCU_CONTROL_* values that are for that register and nothing else 
in Rust. This should fail at compile time if I ever write something 
invalid to a register, and I can't write to anything but a known/exposed 
register.


Interestingly the C code could also abstract the same way and at least 
produce warnings too and become safer. It may be useful to mimic the 
design pattern there to keep panthor.rs and panthor.c in sync more easily?


So my opinion would be to try get the maximum value from Rust and have 
things like proper register abstractions that are definitely safe.



For all these reasons, I think I'd prefer to have Daniel focus on a
proper rust abstraction to expose GPU registers and fields the rust-way,
rather than have him spend days/weeks on a script that is likely to be
used a couple times (if not less) before the driver is entirely
rewritten in rust. I guess the only interesting aspect remaining after
the conversion is done is conciseness of register definitions if we
were using some sort of descriptive format that gets converted to rust
code, but it comes at the cost of maintaining this script. I'd probably
have a completely different opinion if the Mali register layout was a
moving target, but it doesn't seem to be the case.

FYI, Daniel has a python script parsing panthor_regs.h and generating
panthor_regs.rs out of it which he can share if you're interested.

Regards,

Boris

Re: [RFC PATCH] drm: panthor: add dev_coredumpv support

[Carsten Haitzler]

We did discuss this, but I've come to the conclusion that's the wrong
approach. Converting is going to need to track kernel closely as there
are lots of dependencies with the various rust abstractions needed. If
we just copy over the C driver, that's an invitation to diverge and
accumulate technical debt. The advice to upstreaming things is never
go work on a fork for a couple of years and come back with a huge pile
of code to upstream. I don't think this situation is any different. If
there's a path to do it in small pieces, we should take it.


I'd be quite keen for the "fork" to live in the upstream kernel. My
preference is for the two drivers to sit side-by-side. I'm not sure
whether that's a common view though.


I agree that a panthor.rs should to exist side by side with the C for 
some time. I guess it's going to be in the order of a year or so (or 
maybe more) and not a few weeks, so keeping the C and Rust in sync will 
be important.


My take is that such drivers probably belong in non-mainline dev trees 
until they settle a bit, are at least fully functional and we're down to 
arguing finer details. Especially since the other Rust infra they depend 
on not mainline yet either.


Given that, my opinion is this patch probably needs to be originally in 
C then with a rust idiomatic port in the in-progress rust rewrite, or 
there needs to be a lot more effort to building the right panthor layers 
such as better register abstractions for example as part of this which 
certainly will raise the workload to get this in.

[PATCH v2] drm: Fix alignment of temporary stack ioctl buffers

[carsten . haitzler]

From: Carsten Haitzler 

In a few places (core drm + AMD kfd driver), the ioctl handling uses a
temporary 128 byte buffer on the stack to copy to/from user. ioctl data
can have structs with types of much larger sizes than a byte and a
system may require alignment of types in these. At the same time the
compiler may align a char buf to something else as it has no idea that
this buffer is used for storing structs with such alignment
requirements. At a minimum putting in alignment information as an
attribute should be a warning in future if an architecture that needs
more alignment appears.

This was discovered while implementing capability ABI support in Linux
on ARM's Morello CPU (128 bit capability "pointers" in userspace, with
a 64bit non-capability kernel (hybrid) setup). In this, userspace
ioctl structs now had to transport capabilities that needed 16 byte
alignment, but the kernel was not putting these data buffers on that
alignment boundary.

Currently the largest type that is needed is a u64 so the alignment
only asks for that.

Signed-off-by: Carsten Haitzler 
---
 drivers/dma-buf/dma-heap.c   | 2 +-
 drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 2 +-
 drivers/gpu/drm/drm_ioctl.c  | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
index 84ae708fafe7..8fa68b8a9b60 100644
--- a/drivers/dma-buf/dma-heap.c
+++ b/drivers/dma-buf/dma-heap.c
@@ -126,7 +126,7 @@ static unsigned int dma_heap_ioctl_cmds[] = {
 static long dma_heap_ioctl(struct file *file, unsigned int ucmd,
   unsigned long arg)
 {
-   char stack_kdata[128];
+   _Alignas(u64) char stack_kdata[128];
char *kdata = stack_kdata;
unsigned int kcmd;
unsigned int in_size, out_size, drv_size, ksize;
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
index fdf171ad4a3c..201a5c0227ec 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
@@ -3236,7 +3236,7 @@ static long kfd_ioctl(struct file *filep, unsigned int 
cmd, unsigned long arg)
amdkfd_ioctl_t *func;
const struct amdkfd_ioctl_desc *ioctl = NULL;
unsigned int nr = _IOC_NR(cmd);
-   char stack_kdata[128];
+   _Alignas(u64) char stack_kdata[128];
char *kdata = NULL;
unsigned int usize, asize;
int retcode = -EINVAL;
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index e368fc084c77..77a88b597c0b 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -767,7 +767,7 @@ long drm_ioctl(struct file *filp,
drm_ioctl_t *func;
unsigned int nr = DRM_IOCTL_NR(cmd);
int retcode = -EINVAL;
-   char stack_kdata[128];
+   _Alignas(u64) char stack_kdata[128];
char *kdata = NULL;
unsigned int in_size, out_size, drv_size, ksize;
bool is_driver_ioctl;
-- 
2.25.1

Re: [PATCH] drm: Fix alignment of temporary stack ioctl buffers

[Carsten Haitzler]

On 6/11/24 5:17 PM, T.J. Mercier wrote:

On Tue, Jun 11, 2024 at 2:35 AM  wrote:


From: Carsten Haitzler 

In a few places (core drm + AMD kfd driver), the ioctl handling uses a
temporary 128 byte buffer on the stack to copy to/from user. ioctl data
can have structs with types of much larger sizes than a byte and a
system may require alignment of types in these. At the same time the
compiler may align a char buf to something else as it has no idea that
this buffer is used for storing structs with such alignment
requirements. At a minimum putting in alignment information as an
attribute should be a warning in future if an architecture that needs
more alignment appears.

This was discovered while implementing capability ABI support in Linux
on ARM's Morello CPU (128 bit capability "pointers" in userspace, with
a 64bit non-capability kernel (hybrid) setup). In this, userspace
ioctl structs now had to transport capabilities that needed 16 byte
alignment, but the kernel was not putting these data buffers on that
alignment boundary.

Currently the largest type that is needed is a u64 so the alignment
only asks for that.


Makes sense to me.

Now that the kernel depends on C11, I think:
__attribute__((aligned(__alignof__(u64


Aaaah yes. I'm living in the past as usual. :)


can be simply reduced to:
_Alignas(u64)

and put first instead of last in the declaration:
_Alignas(u64) char stack_kdata[128];


Yup. Will do. Expect a V2 to come in.



Signed-off-by: Carsten Haitzler 
---
  drivers/dma-buf/dma-heap.c   | 2 +-
  drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 2 +-
  drivers/gpu/drm/drm_ioctl.c  | 2 +-
  3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
index 84ae708fafe7..a49e20440edf 100644
--- a/drivers/dma-buf/dma-heap.c
+++ b/drivers/dma-buf/dma-heap.c
@@ -126,7 +126,7 @@ static unsigned int dma_heap_ioctl_cmds[] = {
  static long dma_heap_ioctl(struct file *file, unsigned int ucmd,
unsigned long arg)
  {
-   char stack_kdata[128];
+   char stack_kdata[128] __attribute__((aligned(__alignof__(u64;
 char *kdata = stack_kdata;
 unsigned int kcmd;
 unsigned int in_size, out_size, drv_size, ksize;
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
index fdf171ad4a3c..69a0aae0f016 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
@@ -3236,7 +3236,7 @@ static long kfd_ioctl(struct file *filep, unsigned int 
cmd, unsigned long arg)
 amdkfd_ioctl_t *func;
 const struct amdkfd_ioctl_desc *ioctl = NULL;
 unsigned int nr = _IOC_NR(cmd);
-   char stack_kdata[128];
+   char stack_kdata[128] __attribute__((aligned(__alignof__(u64;
 char *kdata = NULL;
 unsigned int usize, asize;
 int retcode = -EINVAL;
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index e368fc084c77..aac3d5a539a6 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -767,7 +767,7 @@ long drm_ioctl(struct file *filp,
 drm_ioctl_t *func;
 unsigned int nr = DRM_IOCTL_NR(cmd);
 int retcode = -EINVAL;
-   char stack_kdata[128];
+   char stack_kdata[128] __attribute__((aligned(__alignof__(u64;
 char *kdata = NULL;
 unsigned int in_size, out_size, drv_size, ksize;
 bool is_driver_ioctl;
--
2.25.1

[PATCH] drm: Fix alignment of temporary stack ioctl buffers

[carsten . haitzler]

From: Carsten Haitzler 

In a few places (core drm + AMD kfd driver), the ioctl handling uses a
temporary 128 byte buffer on the stack to copy to/from user. ioctl data
can have structs with types of much larger sizes than a byte and a
system may require alignment of types in these. At the same time the
compiler may align a char buf to something else as it has no idea that
this buffer is used for storing structs with such alignment
requirements. At a minimum putting in alignment information as an
attribute should be a warning in future if an architecture that needs
more alignment appears.

This was discovered while implementing capability ABI support in Linux
on ARM's Morello CPU (128 bit capability "pointers" in userspace, with
a 64bit non-capability kernel (hybrid) setup). In this, userspace
ioctl structs now had to transport capabilities that needed 16 byte
alignment, but the kernel was not putting these data buffers on that
alignment boundary.

Currently the largest type that is needed is a u64 so the alignment
only asks for that.

Signed-off-by: Carsten Haitzler 
---
 drivers/dma-buf/dma-heap.c   | 2 +-
 drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 2 +-
 drivers/gpu/drm/drm_ioctl.c  | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
index 84ae708fafe7..a49e20440edf 100644
--- a/drivers/dma-buf/dma-heap.c
+++ b/drivers/dma-buf/dma-heap.c
@@ -126,7 +126,7 @@ static unsigned int dma_heap_ioctl_cmds[] = {
 static long dma_heap_ioctl(struct file *file, unsigned int ucmd,
   unsigned long arg)
 {
-   char stack_kdata[128];
+   char stack_kdata[128] __attribute__((aligned(__alignof__(u64;
char *kdata = stack_kdata;
unsigned int kcmd;
unsigned int in_size, out_size, drv_size, ksize;
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
index fdf171ad4a3c..69a0aae0f016 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
@@ -3236,7 +3236,7 @@ static long kfd_ioctl(struct file *filep, unsigned int 
cmd, unsigned long arg)
amdkfd_ioctl_t *func;
const struct amdkfd_ioctl_desc *ioctl = NULL;
unsigned int nr = _IOC_NR(cmd);
-   char stack_kdata[128];
+   char stack_kdata[128] __attribute__((aligned(__alignof__(u64;
char *kdata = NULL;
unsigned int usize, asize;
int retcode = -EINVAL;
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index e368fc084c77..aac3d5a539a6 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -767,7 +767,7 @@ long drm_ioctl(struct file *filp,
drm_ioctl_t *func;
unsigned int nr = DRM_IOCTL_NR(cmd);
int retcode = -EINVAL;
-   char stack_kdata[128];
+   char stack_kdata[128] __attribute__((aligned(__alignof__(u64;
char *kdata = NULL;
unsigned int in_size, out_size, drv_size, ksize;
bool is_driver_ioctl;
-- 
2.25.1

Re: [PATCH] drm/komeda: Fix handling of atomic commits in the atomic_commit_tail hook

[Carsten Haitzler]

Looks good to me - as per log.

On 7/22/22 13:21, Liviu Dudau wrote:

Komeda driver relies on the generic DRM atomic helper functions to handle
commits. It only implements an atomic_commit_tail hook for the
mode_config_helper_funcs and even that one is pretty close to the generic
implementation with the exception of additional dma_fence signalling.

What the generic helper framework doesn't do is waiting for the actual
hardware to signal that the commit parameters have been written into the
appropriate registers. As we signal CRTC events only on the irq handlers,
we need to flush the configuration and wait for the hardware to respond.

Add the Komeda specific implementation for atomic_commit_hw_done() that
flushes and waits for flip done before calling 
drm_atomic_helper_commit_hw_done().

The fix was prompted by a patch from Carsten Haitzler where he was trying to
solve the same issue but in a different way that I think can lead to wrong
event signaling to userspace.

Reported-by: Carsten Haitzler 
Tested-by: Carsten Haitzler 
Signed-off-by: Liviu Dudau 
---
  .../gpu/drm/arm/display/komeda/komeda_crtc.c  |  4 ++--
  .../gpu/drm/arm/display/komeda/komeda_kms.c   | 21 ++-
  .../gpu/drm/arm/display/komeda/komeda_kms.h   |  2 ++
  3 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c 
b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
index 59172acb9738..292f533d8cf0 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
@@ -235,7 +235,7 @@ void komeda_crtc_handle_event(struct komeda_crtc   *kcrtc,
crtc->state->event = NULL;
drm_crtc_send_vblank_event(crtc, event);
} else {
-   DRM_WARN("CRTC[%d]: FLIP happen but no pending 
commit.\n",
+   DRM_WARN("CRTC[%d]: FLIP happened but no pending 
commit.\n",
 drm_crtc_index(&kcrtc->base));
}
spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
@@ -286,7 +286,7 @@ komeda_crtc_atomic_enable(struct drm_crtc *crtc,
komeda_crtc_do_flush(crtc, old);
  }
  
-static void

+void
  komeda_crtc_flush_and_wait_for_flip_done(struct komeda_crtc *kcrtc,
 struct completion *input_flip_done)
  {
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c 
b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
index 93b7f09b96ca..327051bba5b6 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
@@ -69,6 +69,25 @@ static const struct drm_driver komeda_kms_driver = {
.minor = 1,
  };
  
+static void komeda_kms_atomic_commit_hw_done(struct drm_atomic_state *state)

+{
+   struct drm_device *dev = state->dev;
+   struct komeda_kms_dev *kms = to_kdev(dev);
+   int i;
+
+   for (i = 0; i < kms->n_crtcs; i++) {
+   struct komeda_crtc *kcrtc = &kms->crtcs[i];
+
+   if (kcrtc->base.state->active) {
+   struct completion *flip_done = NULL;
+   if (kcrtc->base.state->event)
+   flip_done = 
kcrtc->base.state->event->base.completion;
+   komeda_crtc_flush_and_wait_for_flip_done(kcrtc, 
flip_done);
+   }
+   }
+   drm_atomic_helper_commit_hw_done(state);
+}
+
  static void komeda_kms_commit_tail(struct drm_atomic_state *old_state)
  {
struct drm_device *dev = old_state->dev;
@@ -81,7 +100,7 @@ static void komeda_kms_commit_tail(struct drm_atomic_state 
*old_state)
  
  	drm_atomic_helper_commit_modeset_enables(dev, old_state);
  
-	drm_atomic_helper_commit_hw_done(old_state);

+   komeda_kms_atomic_commit_hw_done(old_state);
  
  	drm_atomic_helper_wait_for_flip_done(dev, old_state);
  
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h

index 7889e380ab23..7339339ef6b8 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
@@ -183,6 +183,8 @@ void komeda_kms_cleanup_private_objs(struct komeda_kms_dev 
*kms);
  
  void komeda_crtc_handle_event(struct komeda_crtc   *kcrtc,

  struct komeda_events *evts);
+void komeda_crtc_flush_and_wait_for_flip_done(struct komeda_crtc *kcrtc,
+ struct completion 
*input_flip_done);
  
  struct komeda_kms_dev *komeda_kms_attach(struct komeda_dev *mdev);

  void komeda_kms_detach(struct komeda_kms_dev *kms);

Re: [PATCH 2/3] drm/komeda - At init write GCU control block to handle already on DPU

[Carsten Haitzler]

On 7/8/22 17:07, Liviu Dudau wrote:

On Mon, Jun 06, 2022 at 12:47:13PM +0100, carsten.haitz...@foss.arm.com wrote:

From: Carsten Haitzler 

If something has already set up the DPU before the komeda driver comes
up, it will fail to init because it was just writing to the SRST bit in
the GCU control register and ignoring others. This resulted in TBU
bringup stalling and init failing. By writing completely we also  set the
mode back to 0 (inactive) too and thus TBU bringup works.


This is a rather large hammer, tbh. I would like to see if there is a better 
way of
handling the handover from EFIFB that this patch is trying to fix, but I lack an
usable plaform for that. It will generate a flicker at module load time, but if 
users
of Morello are happy with that, then


Just FYI - it'll flicker anyway as the PHY is external and gets 
re-initted etc. anyway... This also happens to handle the situation 
where something goes wrong and you have an already initted komeda sue to 
a previous module load (and it's still alive and working due to an 
unclean shutdown). It'll allow you to load the module again :) So it's 
multi-useful.



Acked-by: Liviu Dudau 

Best regards,
Liviu



Signed-off-by: Carsten Haitzler 
---
  drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c 
b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
index 00fa56c29b3e..39618c1a4c81 100644
--- a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
+++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
@@ -309,8 +309,7 @@ static int d71_reset(struct d71_dev *d71)
u32 __iomem *gcu = d71->gcu_addr;
int ret;
  
-	malidp_write32_mask(gcu, BLK_CONTROL,

-   GCU_CONTROL_SRST, GCU_CONTROL_SRST);
+   malidp_write32(gcu, BLK_CONTROL, GCU_CONTROL_SRST);
  
  	ret = dp_wait_cond(!(malidp_read32(gcu, BLK_CONTROL) & GCU_CONTROL_SRST),

   100, 1000, 1);
--
2.32.0

Re: [PATCH 3/3] drm/komeda - Fix handling of pending crtc state commit to avoid lock-up

[Carsten Haitzler]

On 7/11/22 11:13, Liviu Dudau wrote:

On Fri, Jul 08, 2022 at 07:03:37PM +0100, Carsten Haitzler wrote:



On 7/8/22 17:02, Liviu Dudau wrote:

On Mon, Jun 06, 2022 at 12:47:14PM +0100, carsten.haitz...@foss.arm.com wrote:

From: Carsten Haitzler 


Hi Carsten,



Sometimes there is an extra dcm crtc state in the pipeline whose
penting vblank event has not been handled yet. We would previously
throw this out and the vblank event never triggers leading to hard
lockups higher up the stack where an expected vlank event never comes
back (screen freezes).

This fixes that by tracking a pending crtc state that needs handling
and handle it producing a vlank event next vblank if it had not
laready been handled before. This fixes the hangs and ensures our
display keeps updating seamlessly and is certainly a much better state
to be in than after some time ending up with a mysteriously frozen
screen and a lot of kernle messages complaining about this too.


Sorry it took me so long to review and reply to this patch, but I had this 
nagging


No worries. :)


feeling that the patch is not actually correct so I've tried to track the actual
issue. It turns out that the problem is easy to reproduce in a different setup 
with
Mali D71 and it comes from the fact that Komeda doesn't properly wait for the
hardware to signal acceptance of config valid on setting new commits. I have 
created
a new patch that I would be happy if you can test to try to fix the actual 
issue.


This works (tested).


Thank you very much for testing this! Can I add your Tested-by?


Indeed. Go for it.


  One errant "flip without commit":

[9.402559] fbcon: Taking over console
[9.525373] [drm:komeda_print_events [komeda]] *ERROR* err detect: gcu:
MERR, pipes[0]: FLIP, pipes[1]: None
[9.525455] [drm] CRTC[0]: FLIP happened but no pending commit.
[9.542215] Console: switching to colour frame buffer device 240x67



Is this with your 2/3 patch applied and coming out from the firmware having 
already
initialised the hardware? If so, the handover probably doesn't quiescence the
interrupts correctly so an interrupt is pending when the kernel driver is
initialised. That's another area worth looking at for the handover purposes.


Yeah. the firmware is not doing a very clean handover - it's leaving the 
hardware in whatever state it has. it probably could shut down 
interrupts on handover, but not something to worry about in the kernel 
driver at this point.



But nothing worrying. It does work, though doesn't compile due to:

drivers/gpu/drm/arm/display/komeda/komeda_kms.c: In function
‘komeda_kms_atomic_commit_hw_done’:
drivers/gpu/drm/arm/display/komeda/komeda_kms.c:77:9: error: ‘for’ loop
initial declarations are only allowed in C99 or C11 mode
77 | for (int i = 0; i < kms->n_crtcs; i++) {
   | ^~~
drivers/gpu/drm/arm/display/komeda/komeda_kms.c:77:9: note: use option
‘-std=c9
’, ‘-std=gnu99’, ‘-std=c11’ or ‘-std=gnu11’ to compile your code

but that was a trivial fixup.


Interesting that I'm not seeing that, probably due to using GCC12? Anyway, I'll 
fix
that and send a proper patch.


I tend to use newer gcc's indeed, but I am using gcc11 in this case.


Your commit handler does sit and wait. I guess I avoided that and had it
still deferred and handled next time the vblank interrupt fires. Yours is a
bit shorter and less complex so it wins. :)


Yes, that's the essence of my issue with your patch. Delaying the handling of 
the
event until the next vblank means older software that doesn't use the 
timestamping
from the vblank event will get wrong framerates (playing video might be 
affected).


But this is a rare situation. I certainly was very happy going from "my 
entire display locks up and only a reboot really fixes it" to "look ma - 
it works and I didn't even see a stuttered frame!" :) But your fix is 
better in that regard.



Waiting here when we're also calling drm_atomic_helper_wait_for_flip_done() 
after
drm_atomic_helper_commit_hw_done() feels wrong, but then the later is checking 
if we
have consumed the event so we have to. Maybe the introduction of the
drm_atomic_helper_fake_vblank() is needed in komeda as well like the generic
commit_tail helper function does? I need to investigate more the next time I 
get some
spare cycles on komeda.

I will send a new email with the updated patch and your Tested-by if that's OK.


All happy and fine with that.


Best regards,
Liviu




--8<---

  From 76f9e5fed216a815e9eb56152f85454521079f10 Mon Sep 17 00:00:00 2001
From: Liviu Dudau 
Date: Fri, 8 Jul 2022 16:39:21 +0100
Subject: [PATCH] drm/komeda: Fix handling of atomic commits in the
   atomic_commit_tail hook

Komeda driver relies on the generic DRM atomic helper functions to handle
commits. It only implements

Re: [PATCH 3/3] drm/komeda - Fix handling of pending crtc state commit to avoid lock-up

[Carsten Haitzler]

On 7/14/22 13:20, Robin Murphy wrote:

On 2022-07-11 11:13, Liviu Dudau wrote:
[...]

But nothing worrying. It does work, though doesn't compile due to:

drivers/gpu/drm/arm/display/komeda/komeda_kms.c: In function
‘komeda_kms_atomic_commit_hw_done’:
drivers/gpu/drm/arm/display/komeda/komeda_kms.c:77:9: error: ‘for’ loop
initial declarations are only allowed in C99 or C11 mode
    77 | for (int i = 0; i < kms->n_crtcs; i++) {
   | ^~~
drivers/gpu/drm/arm/display/komeda/komeda_kms.c:77:9: note: use option
‘-std=c9
’, ‘-std=gnu99’, ‘-std=c11’ or ‘-std=gnu11’ to compile your code

but that was a trivial fixup.


Interesting that I'm not seeing that, probably due to using GCC12? 
Anyway, I'll fix

that and send a proper patch.


FWIW we do use -std=gnu11 since 5.18 (see e8c07082a810), but I'm not 
entirely sure what the status quo is for using the new features in fixes 
which might also warrant backporting to stable. I believe Carsten's 
stuck on an older kernel thanks to constraints of the rest of that 
project ;)


Not that old - my last sync was like end of April, but i was basing my 
commits off a stable kernel release tree (5.17.4), I have multiple 
kernels for different purposes and for this stuck to something released 
vaguely recently (i synced my tree to latest release before sending off 
the patch set). I'm not sure on the kernel policy for the above for (int 
i = 0;...) etc. usage. I tend to still be more conservative and keep my 
vars at top of the block anyway out of decades of habit.

Re: [PATCH 3/3] drm/komeda - Fix handling of pending crtc state commit to avoid lock-up

[Carsten Haitzler]

On 7/8/22 17:02, Liviu Dudau wrote:

On Mon, Jun 06, 2022 at 12:47:14PM +0100, carsten.haitz...@foss.arm.com wrote:

From: Carsten Haitzler 


Hi Carsten,



Sometimes there is an extra dcm crtc state in the pipeline whose
penting vblank event has not been handled yet. We would previously
throw this out and the vblank event never triggers leading to hard
lockups higher up the stack where an expected vlank event never comes
back (screen freezes).

This fixes that by tracking a pending crtc state that needs handling
and handle it producing a vlank event next vblank if it had not
laready been handled before. This fixes the hangs and ensures our
display keeps updating seamlessly and is certainly a much better state
to be in than after some time ending up with a mysteriously frozen
screen and a lot of kernle messages complaining about this too.


Sorry it took me so long to review and reply to this patch, but I had this 
nagging


No worries. :)


feeling that the patch is not actually correct so I've tried to track the actual
issue. It turns out that the problem is easy to reproduce in a different setup 
with
Mali D71 and it comes from the fact that Komeda doesn't properly wait for the
hardware to signal acceptance of config valid on setting new commits. I have 
created
a new patch that I would be happy if you can test to try to fix the actual 
issue.


This works (tested). One errant "flip without commit":

[9.402559] fbcon: Taking over console
[9.525373] [drm:komeda_print_events [komeda]] *ERROR* err detect: 
gcu: MERR, pipes[0]: FLIP, pipes[1]: None

[9.525455] [drm] CRTC[0]: FLIP happened but no pending commit.
[9.542215] Console: switching to colour frame buffer device 240x67

But nothing worrying. It does work, though doesn't compile due to:

drivers/gpu/drm/arm/display/komeda/komeda_kms.c: In function 
‘komeda_kms_atomic_commit_hw_done’:
drivers/gpu/drm/arm/display/komeda/komeda_kms.c:77:9: error: ‘for’ loop 
initial declarations are only allowed in C99 or C11 mode

   77 | for (int i = 0; i < kms->n_crtcs; i++) {
  | ^~~
drivers/gpu/drm/arm/display/komeda/komeda_kms.c:77:9: note: use option 
‘-std=c9

’, ‘-std=gnu99’, ‘-std=c11’ or ‘-std=gnu11’ to compile your code

but that was a trivial fixup.

Your commit handler does sit and wait. I guess I avoided that and had it 
still deferred and handled next time the vblank interrupt fires. Yours 
is a bit shorter and less complex so it wins. :)



--8<---

 From 76f9e5fed216a815e9eb56152f85454521079f10 Mon Sep 17 00:00:00 2001
From: Liviu Dudau 
Date: Fri, 8 Jul 2022 16:39:21 +0100
Subject: [PATCH] drm/komeda: Fix handling of atomic commits in the
  atomic_commit_tail hook

Komeda driver relies on the generic DRM atomic helper functions to handle
commits. It only implements an atomic_commit_tail hook for the
mode_config_helper_funcs and even that one is pretty close to the generic
implementation with the exception of additional dma_fence signalling.

What the generic helper framework doesn't do is waiting for the actual
hardware to signal that the commit parameters have been written into the
appropriate registers. As we signal CRTC events only on the irq handlers,
we need to flush the configuration and wait for the hardware to respond.

Add the Komeda specific implementation for atomic_commit_hw_done() that
flushes and waits for flip done before calling 
drm_atomic_helper_commit_hw_done().

The fix was prompted by a patch from Carsten Haitzler where he was trying to
solve the same issue but in a different way that I think can lead to wrong
event signaling to userspace.

Reported-by: Carsten Haitzler 
Signed-off-by: Liviu Dudau 
---
  .../gpu/drm/arm/display/komeda/komeda_crtc.c  |  4 ++--
  .../gpu/drm/arm/display/komeda/komeda_kms.c   | 20 ++-
  .../gpu/drm/arm/display/komeda/komeda_kms.h   |  2 ++
  3 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c 
b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
index 59172acb973803d..292f533d8cf0de6 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
@@ -235,7 +235,7 @@ void komeda_crtc_handle_event(struct komeda_crtc   *kcrtc,
crtc->state->event = NULL;
drm_crtc_send_vblank_event(crtc, event);
} else {
-   DRM_WARN("CRTC[%d]: FLIP happen but no pending 
commit.\n",
+   DRM_WARN("CRTC[%d]: FLIP happened but no pending 
commit.\n",
 drm_crtc_index(&kcrtc->base));
}
spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
@@ -286,7 +286,7 @@ komeda_crtc_atomic_enable(struct drm_crtc *crtc,
komeda_crtc_do_flush(crtc, 

Re: [PATCH 2/3] drm/komeda - At init write GCU control block to handle already on DPU

[Carsten Haitzler]

On 7/8/22 17:07, Liviu Dudau wrote:

On Mon, Jun 06, 2022 at 12:47:13PM +0100, carsten.haitz...@foss.arm.com wrote:

From: Carsten Haitzler 

If something has already set up the DPU before the komeda driver comes
up, it will fail to init because it was just writing to the SRST bit in
the GCU control register and ignoring others. This resulted in TBU
bringup stalling and init failing. By writing completely we also  set the
mode back to 0 (inactive) too and thus TBU bringup works.


This is a rather large hammer, tbh. I would like to see if there is a better 
way of
handling the handover from EFIFB that this patch is trying to fix, but I lack an
usable plaform for that. It will generate a flicker at module load time, but if 
users
of Morello are happy with that, then


We're pretty happy with that setup right now. Certainly better than 
Komeda failing to init.



Acked-by: Liviu Dudau 

Best regards,
Liviu



Signed-off-by: Carsten Haitzler 
---
  drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c 
b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
index 00fa56c29b3e..39618c1a4c81 100644
--- a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
+++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
@@ -309,8 +309,7 @@ static int d71_reset(struct d71_dev *d71)
u32 __iomem *gcu = d71->gcu_addr;
int ret;
  
-	malidp_write32_mask(gcu, BLK_CONTROL,

-   GCU_CONTROL_SRST, GCU_CONTROL_SRST);
+   malidp_write32(gcu, BLK_CONTROL, GCU_CONTROL_SRST);
  
  	ret = dp_wait_cond(!(malidp_read32(gcu, BLK_CONTROL) & GCU_CONTROL_SRST),

   100, 1000, 1);
--
2.32.0

[PATCH 3/3] drm/komeda - Fix handling of pending crtc state commit to avoid lock-up

[carsten . haitzler]

From: Carsten Haitzler 

Sometimes there is an extra dcm crtc state in the pipeline whose
penting vblank event has not been handled yet. We would previously
throw this out and the vblank event never triggers leading to hard
lockups higher up the stack where an expected vlank event never comes
back (screen freezes).

This fixes that by tracking a pending crtc state that needs handling
and handle it producing a vlank event next vblank if it had not
laready been handled before. This fixes the hangs and ensures our
display keeps updating seamlessly and is certainly a much better state
to be in than after some time ending up with a mysteriously frozen
screen and a lot of kernle messages complaining about this too.

Signed-off-by: Carsten Haitzler 
---
 .../gpu/drm/arm/display/komeda/komeda_crtc.c  | 10 ++
 .../gpu/drm/arm/display/komeda/komeda_kms.c   | 19 ++-
 .../gpu/drm/arm/display/komeda/komeda_kms.h   |  3 +++
 3 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c 
b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
index 59172acb9738..b7f0a5f97222 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
@@ -227,6 +227,16 @@ void komeda_crtc_handle_event(struct komeda_crtc   *kcrtc,
complete_all(kcrtc->disable_done);
kcrtc->disable_done = NULL;
} else if (crtc->state->event) {
+   if (kcrtc->state_needs_handling) {
+   event = kcrtc->state_needs_handling->event;
+   if (event) {
+   kcrtc->state_needs_handling->event = 
NULL;
+   kcrtc->state_needs_handling = NULL;
+   drm_crtc_send_vblank_event(crtc, event);
+   } else {
+   kcrtc->state_needs_handling = NULL;
+   }
+   }
event = crtc->state->event;
/*
 * Consume event before notifying drm core that flip
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c 
b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
index 93b7f09b96ca..bbc051a1896a 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
@@ -226,10 +226,27 @@ static int komeda_kms_check(struct drm_device *dev,
return 0;
 }
 
+static int komeda_kms_commit(struct drm_device *drm,
+ struct drm_atomic_state *state,
+ bool nonblock)
+{
+   int i;
+   struct drm_crtc *crtc;
+   struct drm_crtc_state *old_crtc_state, *new_crtc_state;
+   struct komeda_crtc *kcrtc;
+
+   for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
+ new_crtc_state, i) {
+   kcrtc = to_kcrtc(crtc);
+   kcrtc->state_needs_handling = crtc->state;
+   }
+   return drm_atomic_helper_commit(drm, state, nonblock);
+}
+
 static const struct drm_mode_config_funcs komeda_mode_config_funcs = {
.fb_create  = komeda_fb_create,
.atomic_check   = komeda_kms_check,
-   .atomic_commit  = drm_atomic_helper_commit,
+   .atomic_commit  = komeda_kms_commit,
 };
 
 static void komeda_kms_mode_config_init(struct komeda_kms_dev *kms,
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h 
b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
index 456f3c435719..8ff3ad04dfe4 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
@@ -84,6 +84,9 @@ struct komeda_crtc {
 
/** @disable_done: this flip_done is for tracing the disable */
struct completion *disable_done;
+
+   /** @state_needs_handling: Has not had it's vblank event handled yet */
+   struct drm_crtc_state *state_needs_handling;
 };
 
 /**
-- 
2.32.0

[PATCH 2/3] drm/komeda - At init write GCU control block to handle already on DPU

[carsten . haitzler]

From: Carsten Haitzler 

If something has already set up the DPU before the komeda driver comes
up, it will fail to init because it was just writing to the SRST bit in
the GCU control register and ignoring others. This resulted in TBU
bringup stalling and init failing. By writing completely we also  set the
mode back to 0 (inactive) too and thus TBU bringup works.

Signed-off-by: Carsten Haitzler 
---
 drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c 
b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
index 00fa56c29b3e..39618c1a4c81 100644
--- a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
+++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
@@ -309,8 +309,7 @@ static int d71_reset(struct d71_dev *d71)
u32 __iomem *gcu = d71->gcu_addr;
int ret;
 
-   malidp_write32_mask(gcu, BLK_CONTROL,
-   GCU_CONTROL_SRST, GCU_CONTROL_SRST);
+   malidp_write32(gcu, BLK_CONTROL, GCU_CONTROL_SRST);
 
ret = dp_wait_cond(!(malidp_read32(gcu, BLK_CONTROL) & 
GCU_CONTROL_SRST),
   100, 1000, 1);
-- 
2.32.0

[PATCH 1/3] drm/komeda - Add legacy FB support so VT's work as expected

[carsten . haitzler]

From: Carsten Haitzler 

The komeda driver doesn't come up with a visible text (FB) mode VT by
default as it was missing legacy FB support. It's useful to have a
working text VT on a system for debug and general usability, so enable
it. You can always toggle CONFIG_FRAMEBUFFER_CONSOLE.

Signed-off-by: Carsten Haitzler 
---
 drivers/gpu/drm/arm/display/komeda/komeda_drv.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c 
b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
index e7933930a657..c0c7933a9631 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
@@ -9,6 +9,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include "komeda_dev.h"
 #include "komeda_kms.h"
@@ -71,6 +72,7 @@ static int komeda_bind(struct device *dev)
}
 
dev_set_drvdata(dev, mdrv);
+   drm_fbdev_generic_setup(&mdrv->kms->base, 32);
 
return 0;
 
-- 
2.32.0

Re: [PATCH v2 11/11] dt-bindings: display: convert Arm Komeda to DT schema

[Carsten Haitzler]

That seems sensible to me. It matches the kind of DT content I know 
works. It's certainly more detailed now.


On 5/6/22 15:05, Andre Przywara wrote:

The Arm Komeda (aka Mali-D71) is a display controller that scans out a
framebuffer and hands a signal to a digital encoder to generate a DVI
or HDMI signal. It supports up to two pipelines, each frame can be
composed of up to four layers.

Convert the existing DT binding to DT schema.

Signed-off-by: Andre Przywara 
---
  .../bindings/display/arm,komeda.txt   |  78 ---
  .../bindings/display/arm,komeda.yaml  | 130 ++
  2 files changed, 130 insertions(+), 78 deletions(-)
  delete mode 100644 Documentation/devicetree/bindings/display/arm,komeda.txt
  create mode 100644 Documentation/devicetree/bindings/display/arm,komeda.yaml

diff --git a/Documentation/devicetree/bindings/display/arm,komeda.txt 
b/Documentation/devicetree/bindings/display/arm,komeda.txt
deleted file mode 100644
index 8513695ee47fe..0
--- a/Documentation/devicetree/bindings/display/arm,komeda.txt
+++ /dev/null
@@ -1,78 +0,0 @@
-Device Tree bindings for Arm Komeda display driver
-
-Required properties:
-- compatible: Should be "arm,mali-d71"
-- reg: Physical base address and length of the registers in the system
-- interrupts: the interrupt line number of the device in the system
-- clocks: A list of phandle + clock-specifier pairs, one for each entry
-in 'clock-names'
-- clock-names: A list of clock names. It should contain:
-  - "aclk": for the main processor clock
-- #address-cells: Must be 1
-- #size-cells: Must be 0
-- iommus: configure the stream id to IOMMU, Must be configured if want to
-enable iommu in display. for how to configure this node please reference
-devicetree/bindings/iommu/arm,smmu-v3.txt,
-devicetree/bindings/iommu/iommu.txt
-
-Required properties for sub-node: pipeline@nq
-Each device contains one or two pipeline sub-nodes (at least one), each
-pipeline node should provide properties:
-- reg: Zero-indexed identifier for the pipeline
-- clocks: A list of phandle + clock-specifier pairs, one for each entry
-in 'clock-names'
-- clock-names: should contain:
-  - "pxclk": pixel clock
-
-- port: each pipeline connect to an encoder input port. The connection is
-modeled using the OF graph bindings specified in
-Documentation/devicetree/bindings/graph.txt
-
-Optional properties:
-  - memory-region: phandle to a node describing memory (see
-Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt)
-to be used for the framebuffer; if not present, the framebuffer may
-be located anywhere in memory.
-
-Example:
-/ {
-   ...
-
-   dp0: display@c0 {
-   #address-cells = <1>;
-   #size-cells = <0>;
-   compatible = "arm,mali-d71";
-   reg = <0xc0 0x2>;
-   interrupts = <0 168 4>;
-   clocks = <&dpu_aclk>;
-   clock-names = "aclk";
-   iommus = <&smmu 0>, <&smmu 1>, <&smmu 2>, <&smmu 3>,
-   <&smmu 4>, <&smmu 5>, <&smmu 6>, <&smmu 7>,
-   <&smmu 8>, <&smmu 9>;
-
-   dp0_pipe0: pipeline@0 {
-   clocks = <&fpgaosc2>;
-   clock-names = "pxclk";
-   reg = <0>;
-
-   port {
-   dp0_pipe0_out: endpoint {
-   remote-endpoint = <&db_dvi0_in>;
-   };
-   };
-   };
-
-   dp0_pipe1: pipeline@1 {
-   clocks = <&fpgaosc2>;
-   clock-names = "pxclk";
-   reg = <1>;
-
-   port {
-   dp0_pipe1_out: endpoint {
-   remote-endpoint = <&db_dvi1_in>;
-   };
-   };
-   };
-   };
-   ...
-};
diff --git a/Documentation/devicetree/bindings/display/arm,komeda.yaml 
b/Documentation/devicetree/bindings/display/arm,komeda.yaml
new file mode 100644
index 0..9f4aade97f10a
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/arm,komeda.yaml
@@ -0,0 +1,130 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/arm,komeda.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Arm Komeda display processor
+
+maintainers:
+  - Liviu Dudau 
+  - Andre Przywara 
+
+description:
+  The Arm Mali D71 display processor supports up to two displays with up
+  to a 4K resolution each. Each pipeline can be composed of up to four
+  layers. It is typically connected to a digital display connector like HDMI.
+
+properties:
+  compatible:
+oneOf:
+  - items:
+  - const: arm,mali-d32
+  - const: arm,mali-d

Re: [RFC] drm/kms: control display brightness through drm_connector properties

[Carsten Haitzler]

On Mon, 11 Apr 2022 12:27:37 +0200 Hans de Goede  said:

> Hi,
> 
> On 4/7/22 20:58, Carsten Haitzler wrote:
> > On Thu, 7 Apr 2022 17:38:59 +0200 Hans de Goede  said:
> > 
> > Below you covered our usual /sys/class/backlight device friends... what
> > about DDC monitor controls? These function similarly but just remotely
> > control a screen via I2C and also suffer from the same problems of "need
> > root" and "have to do some fun in mapping them to a given screen".
> 
> Right, supporting this definitely is part of the plan, this is why my original
> email had the following footnote:

Yay!

> 1) The need to be able to map the backlight device to a specific display
> has become clear once more with the recent proposal to add DDCDI support:
> https://lore.kernel.org/lkml/20220403230850.2986-1-yusisameri...@gmail.com/

Oh gee - I missed that. my bad!

> :)
>  
> > Otherwise I welcome this de-uglification of the backlight device and
> > putting it together with the drm device that controls that monitor.
> 
> Thx.

Having to deal with the backlight device madness is a big pain (have already
done it - DDC included) and properly exposing these things attached to the
proper KMS device is absolutely the right thing. Admittedly this punts the job
of matching a backlight device to the right video output in KMS to the kernel
so at least it gets solved in one place rather than it being re-invented again
and again per wm/desktop/compositor.

> > Just to make life more fun ... DDC does much more than backlight controls.
> > It can control essentially anything that is in the OSD for your monitor
> > (contrast, brightness, backlight, sharpness, color temperatures, input to
> > display (DP vs HDMI vs DVI etc.), an for extra fun points can even tel you
> > the current rotation state of your monitor. All of these do make sense to
> > live as drm connector properties too. Perhaps not a first iteration but
> > something to consider in this design.
> 
> One thing which I do want to take into account is to make sure that userspace
> can still do DDC/CI for all the other features. I know there is demand for
> adding brightness control over DDC/CI. I'm not aware of any concrete use-cases
> for the other DDC/CI settings. Also DDC/CI can include some pretty crazy
> stuff like setting up picture in picture using 2 different inputs of the
> monitor, which is very vendor specific. So all in all I think that we should
> just punt most of the DDC/CI stuff to userspace.

Having spent some time with DDC you're right - it can have some interesting
properties, but a wide number seem to be highly common between monitors and
make total sense to regularly use if available. Backlight/brightness is just
the immediate focus here.

> With that said I agree that it would be good to think about possibly other
> some of the other settings in case some use-case does show up for those.
> 
> The biggest problem with doing this is coming up with some prefix to
> namespace things. I've gone with bl_brightness to avoid a conflict
> with the existing TV specific properties which have plain "brightness"
> put if we want to e.g. also add DDC/CI contrast as a property later
> then it might be good to come up with another more generic prefix
> which can be shared between laptop-panel-brightness, DDC/CI-brightness
> and DDC/CI-contrast ... ?
> 
> So any suggestions for a better prefix?

Well here is my take. Have DDC properties separate from a build-in backlight
device. Userspace code will have to essentially do something like:

if (built_in_backlight_exists(output)) // built in backlight device
  set_backlight_brightness(output, val);
else if (ddc_prop_exists(output, 0x10)) // 0x10 is ddc brightness/backlight
  set_ddc_int_val(output, 0x10, val);
else // fallback for ye olde setuid tooling
  { ...
  }

DDC properties are quite simple in essence so just exposing the set so you can
read/write them (and check if they exist at all) would do the right thing - tie
DDC to the output visa KMS, (you still could use I2C directly if you like and
go behind KMS's back) but it'd then punt the policy decision of which
properties are common/sane to userspace without adding a possibly "endless" set
of "let's now adopt/abstract this DDC property" discussions. Wayland
compositors can adopts the properties they see as most useful for their uses.
Xorg could expose them via XRR output properties. So my take at least is to give
DDC it's own property namespace/set that allows an arbitrary set of numbered
properties and leave it pretty raw.

> Regards,
> 
> Hans
> 
> 
> 
> 
> 
> > 
> >> As discussed already several times in the past:
> >>  https://www.x.org/wiki/Even

Re: [RFC] drm/kms: control display brightness through drm_connector properties

[Carsten Haitzler]

 provided a minimum (non 0) duty-cycle below which the driver will
> never go.
> 
> bl_brightness_control_method: ro, enum, possible values:
> none: The GPU driver expects brightness control to be provided by another
>   driver and that driver has not loaded yet.
> unknown:  The underlying control mechanism is unknown.
> pwm:  The brightness property directly controls the duty-cycle of a PWM
>   output.
> firmware: The brightness is controlled through firmware calls.
> DDC/CI:   The brightness is controlled through the DDC/CI protocol.
> gmux: The brightness is controlled by the GMUX.
> Note this enum may be extended in the future, so other values may
> be read, these should be treated as "unknown".
> 
> When brightness control becomes available after being reported
> as not available before (bl_brightness_control_method=="none")
> a uevent with CONNECTOR= and
> PROPERTY= will be generated
> at this point all the properties must be re-read.
> 
> When/once brightness control is available then all the read-only
> properties are fixed and will never change.
> 
> Besides the "none" value for no driver having loaded yet,
> the different bl_brightness_control_method values are intended for
> (userspace) heuristics for such things as the brightness setting
> linearly controlling electrical power or setting perceived brightness.
> 
> Regards,
> 
> Hans
> 
> 
> 1) The need to be able to map the backlight device to a specific display
> has become clear once more with the recent proposal to add DDCDI support:
> https://lore.kernel.org/lkml/20220403230850.2986-1-yusisameri...@gmail.com/
> 
> 2)
> https://lore.kernel.org/all/4b17ba08-39f3-57dd-5aad-d37d844b0...@linux.intel.com/
> Note this proposal included a method for userspace to be able to tell the
> kernel if the native/acpi_video/vendor backlight device should be used, but
> this has been solved in the kernel for years now:
> https://www.spinics.net/lists/linux-acpi/msg50526.html An initial
> implementation of this proposal is available here:
> https://cgit.freedesktop.org/~mperes/linux/log/?h=backlight
> 


-- 
- Codito, ergo sum - "I code, therefore I am" --
Carsten Haitzler - ras...@rasterman.com

Re: [PATCH 2/3] drm/arm/komeda : change driver to use drm_writeback_connector.base pointer

[Carsten Haitzler]

This makes sense given the other patches in your series, but it seems as 
yet no one has anything to say about this. I don't have anything 
specific to comment on other than it seems to make the correct changes 
to komeda given the rest.


Reviewed-by: Carsten Haitzler 

On 1/11/22 10:18, Kandpal, Suraj wrote:

Making changes to komeda driver because we had to change
drm_writeback_connector.base into a pointer the reason for which is
expained in the Patch (drm: add writeback pointers to drm_connector).

Signed-off-by: Kandpal, Suraj 
---
  drivers/gpu/drm/arm/display/komeda/komeda_crtc.c | 2 +-
  drivers/gpu/drm/arm/display/komeda/komeda_kms.h  | 3 ++-
  drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c | 9 +
  3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c 
b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
index 59172acb9738..eb37f41c1790 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
@@ -265,7 +265,7 @@ komeda_crtc_do_flush(struct drm_crtc *crtc,
if (slave && has_bit(slave->id, kcrtc_st->affected_pipes))
komeda_pipeline_update(slave, old->state);
  
-	conn_st = wb_conn ? wb_conn->base.base.state : NULL;

+   conn_st = wb_conn ? wb_conn->base.base->state : NULL;
if (conn_st && conn_st->writeback_job)
drm_writeback_queue_job(&wb_conn->base, conn_st);
  
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h

index 456f3c435719..8d83883a1d99 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
@@ -53,6 +53,7 @@ struct komeda_plane_state {
   * struct komeda_wb_connector
   */
  struct komeda_wb_connector {
+   struct drm_connector conn;
/** @base: &drm_writeback_connector */
struct drm_writeback_connector base;
  
@@ -136,7 +137,7 @@ struct komeda_kms_dev {

  static inline bool is_writeback_only(struct drm_crtc_state *st)
  {
struct komeda_wb_connector *wb_conn = to_kcrtc(st->crtc)->wb_conn;
-   struct drm_connector *conn = wb_conn ? &wb_conn->base.base : NULL;
+   struct drm_connector *conn = wb_conn ? wb_conn->base.base : NULL;
  
  	return conn && (st->connector_mask == BIT(drm_connector_index(conn)));

  }
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c 
b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
index e465cc4879c9..0caaf483276d 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
@@ -51,7 +51,7 @@ komeda_wb_encoder_atomic_check(struct drm_encoder *encoder,
return -EINVAL;
}
  
-	wb_layer = to_kconn(to_wb_conn(conn_st->connector))->wb_layer;

+   wb_layer = 
to_kconn(drm_connector_to_writeback(conn_st->connector))->wb_layer;
  
  	/*

 * No need for a full modested when the only connector changed is the
@@ -123,7 +123,7 @@ komeda_wb_connector_fill_modes(struct drm_connector 
*connector,
  static void komeda_wb_connector_destroy(struct drm_connector *connector)
  {
drm_connector_cleanup(connector);
-   kfree(to_kconn(to_wb_conn(connector)));
+   kfree(to_kconn(drm_connector_to_writeback(connector)));
  }
  
  static const struct drm_connector_funcs komeda_wb_connector_funcs = {

@@ -155,6 +155,7 @@ static int komeda_wb_connector_add(struct komeda_kms_dev 
*kms,
kwb_conn->wb_layer = kcrtc->master->wb_layer;
  
  	wb_conn = &kwb_conn->base;

+   wb_conn->base = &kwb_conn->conn;
wb_conn->encoder.possible_crtcs = BIT(drm_crtc_index(&kcrtc->base));
  
  	formats = komeda_get_layer_fourcc_list(&mdev->fmt_tbl,

@@ -171,9 +172,9 @@ static int komeda_wb_connector_add(struct komeda_kms_dev 
*kms,
return err;
}
  
-	drm_connector_helper_add(&wb_conn->base, &komeda_wb_conn_helper_funcs);

+   drm_connector_helper_add(wb_conn->base, &komeda_wb_conn_helper_funcs);
  
-	info = &kwb_conn->base.base.display_info;

+   info = &kwb_conn->base.base->display_info;
info->bpc = __fls(kcrtc->master->improc->supported_color_depths);
info->color_formats = kcrtc->master->improc->supported_color_formats;
  

Re: [PATCH 02/60] drm/arm/hdlcd: Add support for the nomodeset kernel parameter

[Carsten Haitzler]

Seems fine.

Reviewed-by: Carsten Haitzler 

On 12/15/21 00:59, Javier Martinez Canillas wrote:

According to disable Documentation/admin-guide/kernel-parameters.txt, this
parameter can be used to disable kernel modesetting.

DRM drivers will not perform display-mode changes or accelerated rendering
and only the systewm system framebuffer will be available if it was set-up.

But only a few DRM drivers currently check for nomodeset, make this driver
to also support the command line parameter.

Signed-off-by: Javier Martinez Canillas 
---

  drivers/gpu/drm/arm/hdlcd_drv.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
index 479c2422a2e0..0939a64a9bd2 100644
--- a/drivers/gpu/drm/arm/hdlcd_drv.c
+++ b/drivers/gpu/drm/arm/hdlcd_drv.c
@@ -382,6 +382,9 @@ static int hdlcd_probe(struct platform_device *pdev)
struct device_node *port;
struct component_match *match = NULL;
  
+	if (drm_firmware_drivers_only())

+   return -ENODEV;
+
/* there is only one output port inside each device, find it */
port = of_graph_get_remote_node(pdev->dev.of_node, 0, 0);
if (!port)

Re: [PATCH 01/60] drm/komeda: Add support for the nomodeset kernel parameter

[Carsten Haitzler]

Seems fine.

Reviewed-by: Carsten Haitzler 

On 12/15/21 00:59, Javier Martinez Canillas wrote:

According to disable Documentation/admin-guide/kernel-parameters.txt, this
parameter can be used to disable kernel modesetting.

DRM drivers will not perform display-mode changes or accelerated rendering
and only the systewm system framebuffer will be available if it was set-up.

But only a few DRM drivers currently check for nomodeset, make this driver
to also support the command line parameter.

Signed-off-by: Javier Martinez Canillas 
---

  drivers/gpu/drm/arm/display/komeda/komeda_drv.c | 4 
  1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c 
b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
index e7933930a657..4f6d5c2103ec 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
@@ -9,6 +9,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include "komeda_dev.h"
  #include "komeda_kms.h"
@@ -117,6 +118,9 @@ static int komeda_platform_probe(struct platform_device 
*pdev)
struct component_match *match = NULL;
struct device_node *child;
  
+	if (drm_firmware_drivers_only())

+   return -EINVAL;
+
if (!dev->of_node)
return -ENODEV;
  

Re: [PATCH 03/60] drm/malidp: Add support for the nomodeset kernel parameter

[Carsten Haitzler]

Seems fine to me.

Reviewed-by: Carsten Haitzler 

On 12/15/21 00:59, Javier Martinez Canillas wrote:

According to disable Documentation/admin-guide/kernel-parameters.txt, this
parameter can be used to disable kernel modesetting.

DRM drivers will not perform display-mode changes or accelerated rendering
and only the systewm system framebuffer will be available if it was set-up.

But only a few DRM drivers currently check for nomodeset, make this driver
to also support the command line parameter.

Signed-off-by: Javier Martinez Canillas 
---

  drivers/gpu/drm/arm/malidp_drv.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index 78d15b04b105..5da4168eb76d 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -938,6 +938,9 @@ static int malidp_platform_probe(struct platform_device 
*pdev)
struct device_node *port;
struct component_match *match = NULL;
  
+	if (drm_firmware_drivers_only())

+   return -ENODEV;
+
if (!pdev->dev.of_node)
return -ENODEV;
  

Re: [PATCH] drm/arm: arm hdlcd select DRM_GEM_CMA_HELPER

[Carsten Haitzler]

I sent an updated patch with the Fixes:

On 1/24/22 16:13, Steven Price wrote:

On 24/01/2022 15:13, carsten.haitz...@foss.arm.com wrote:

From: Carsten Haitzler 

Without DRM_GEM_CMA_HELPER HDLCD won't build. This needs to be there too.

Signed-off-by: Carsten Haitzler 


To add the context, DRM_HDLCD used to select DRM_KMS_CMA_HELPER but that
was removed in commit 09717af7d13d ("drm: Remove
CONFIG_DRM_KMS_CMA_HELPER option"). DRM_KMS_CMA_HELPER would select
DRM_GEM_CMA_HELPER but that transitive dependency was lost and
apparently the fixup was missed in that commit.

So we need a:

Fixes: 09717af7d13d ("drm: Remove CONFIG_DRM_KMS_CMA_HELPER option")

and with that...

Reviewed-by: Steven Price 

Steve


---
  drivers/gpu/drm/arm/Kconfig | 1 +
  1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/arm/Kconfig b/drivers/gpu/drm/arm/Kconfig
index 58a242871b28..6e3f1d600541 100644
--- a/drivers/gpu/drm/arm/Kconfig
+++ b/drivers/gpu/drm/arm/Kconfig
@@ -6,6 +6,7 @@ config DRM_HDLCD
depends on DRM && OF && (ARM || ARM64 || COMPILE_TEST)
depends on COMMON_CLK
select DRM_KMS_HELPER
+   select DRM_GEM_CMA_HELPER
help
  Choose this option if you have an ARM High Definition Colour LCD
  controller.

[PATCH] drm/arm: arm hdlcd select DRM_GEM_CMA_HELPER

[carsten . haitzler]

From: Carsten Haitzler 

Without DRM_GEM_CMA_HELPER HDLCD won't build. This needs to be there too.

Fixes: 09717af7d13d ("drm: Remove CONFIG_DRM_KMS_CMA_HELPER option")

Signed-off-by: Carsten Haitzler 
---
 drivers/gpu/drm/arm/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/arm/Kconfig b/drivers/gpu/drm/arm/Kconfig
index 58a242871b28..6e3f1d600541 100644
--- a/drivers/gpu/drm/arm/Kconfig
+++ b/drivers/gpu/drm/arm/Kconfig
@@ -6,6 +6,7 @@ config DRM_HDLCD
depends on DRM && OF && (ARM || ARM64 || COMPILE_TEST)
depends on COMMON_CLK
select DRM_KMS_HELPER
+   select DRM_GEM_CMA_HELPER
help
  Choose this option if you have an ARM High Definition Colour LCD
  controller.
-- 
2.30.1

Re: [PATCH] drm/arm: Fix hdlcd selects to add DRM_GEM_CMA_HELPER for build

[Carsten Haitzler]

Sorry - I meant disregard THIS one - following patch is right.

On 1/24/22 14:58, carsten.haitz...@foss.arm.com wrote:

From: Carsten Haitzler 

Without DRM_GEM_CMA_HELPER HDLCD won't build. This needs to be there too.

Signed-off-by: Carsten Haitzler 
---
  drivers/gpu/drm/arm/Kconfig | 1 +
  1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/arm/Kconfig b/drivers/gpu/drm/arm/Kconfig
index 58a242871b28..9eaceb981d92 100644
--- a/drivers/gpu/drm/arm/Kconfig
+++ b/drivers/gpu/drm/arm/Kconfig
@@ -6,6 +6,7 @@ config DRM_HDLCD
depends on DRM && OF && (ARM || ARM64 || COMPILE_TEST)
depends on COMMON_CLK
select DRM_KMS_HELPER
+select DRM_GEM_CMA_HELPER
help
  Choose this option if you have an ARM High Definition Colour LCD
  controller.

Re: [PATCH] drm/arm: arm hdlcd select DRM_GEM_CMA_HELPER

[Carsten Haitzler]

Sorry - noise. Ignore this one. Following one is fixed.

On 1/24/22 15:13, carsten.haitz...@foss.arm.com wrote:

From: Carsten Haitzler 

Without DRM_GEM_CMA_HELPER HDLCD won't build. This needs to be there too.

Signed-off-by: Carsten Haitzler 
---
  drivers/gpu/drm/arm/Kconfig | 1 +
  1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/arm/Kconfig b/drivers/gpu/drm/arm/Kconfig
index 58a242871b28..6e3f1d600541 100644
--- a/drivers/gpu/drm/arm/Kconfig
+++ b/drivers/gpu/drm/arm/Kconfig
@@ -6,6 +6,7 @@ config DRM_HDLCD
depends on DRM && OF && (ARM || ARM64 || COMPILE_TEST)
depends on COMMON_CLK
select DRM_KMS_HELPER
+   select DRM_GEM_CMA_HELPER
help
  Choose this option if you have an ARM High Definition Colour LCD
  controller.

[PATCH] drm/arm: arm hdlcd select DRM_GEM_CMA_HELPER

[carsten . haitzler]

From: Carsten Haitzler 

Without DRM_GEM_CMA_HELPER HDLCD won't build. This needs to be there too.

Signed-off-by: Carsten Haitzler 
---
 drivers/gpu/drm/arm/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/arm/Kconfig b/drivers/gpu/drm/arm/Kconfig
index 58a242871b28..6e3f1d600541 100644
--- a/drivers/gpu/drm/arm/Kconfig
+++ b/drivers/gpu/drm/arm/Kconfig
@@ -6,6 +6,7 @@ config DRM_HDLCD
depends on DRM && OF && (ARM || ARM64 || COMPILE_TEST)
depends on COMMON_CLK
select DRM_KMS_HELPER
+   select DRM_GEM_CMA_HELPER
help
  Choose this option if you have an ARM High Definition Colour LCD
  controller.
-- 
2.30.1

[PATCH] drm/arm: Fix hdlcd selects to add DRM_GEM_CMA_HELPER for build

[carsten . haitzler]

From: Carsten Haitzler 

Without DRM_GEM_CMA_HELPER HDLCD won't build. This needs to be there too.

Signed-off-by: Carsten Haitzler 
---
 drivers/gpu/drm/arm/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/arm/Kconfig b/drivers/gpu/drm/arm/Kconfig
index 58a242871b28..9eaceb981d92 100644
--- a/drivers/gpu/drm/arm/Kconfig
+++ b/drivers/gpu/drm/arm/Kconfig
@@ -6,6 +6,7 @@ config DRM_HDLCD
depends on DRM && OF && (ARM || ARM64 || COMPILE_TEST)
depends on COMMON_CLK
select DRM_KMS_HELPER
+select DRM_GEM_CMA_HELPER
help
  Choose this option if you have an ARM High Definition Colour LCD
  controller.
-- 
2.30.1

Re: [PATCH] drm/plane: Move range check for format_count earlier

[Carsten Haitzler]

On 12/3/21 13:08, Liviu Dudau wrote:

On Fri, Dec 03, 2021 at 10:28:15AM +, Steven Price wrote:

While the check for format_count > 64 in __drm_universal_plane_init()
shouldn't be hit (it's a WARN_ON), in its current position it will then
leak the plane->format_types array and fail to call
drm_mode_object_unregister() leaking the modeset identifier. Move it to
the start of the function to avoid allocating those resources in the
first place.

Signed-off-by: Steven Price 


Well spotted!

Reviewed-by: Liviu Dudau 

I'm going to wait to see if anyone else has any comments before I'll merge this 
into
drm-misc-fixes (or should it be drm-misc-next-fixes?)


This definitely looks to fix an ugly I spotted too while looking at your 
prior patch thinking it might be wrong so a +1 from me.



Best regards,
Liviu


---
  drivers/gpu/drm/drm_plane.c | 14 +++---
  1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 82afb854141b..fd0bf90fb4c2 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -249,6 +249,13 @@ static int __drm_universal_plane_init(struct drm_device 
*dev,
if (WARN_ON(config->num_total_plane >= 32))
return -EINVAL;
  
+	/*

+* First driver to need more than 64 formats needs to fix this. Each
+* format is encoded as a bit and the current code only supports a u64.
+*/
+   if (WARN_ON(format_count > 64))
+   return -EINVAL;
+
WARN_ON(drm_drv_uses_atomic_modeset(dev) &&
(!funcs->atomic_destroy_state ||
 !funcs->atomic_duplicate_state));
@@ -270,13 +277,6 @@ static int __drm_universal_plane_init(struct drm_device 
*dev,
return -ENOMEM;
}
  
-	/*

-* First driver to need more than 64 formats needs to fix this. Each
-* format is encoded as a bit and the current code only supports a u64.
-*/
-   if (WARN_ON(format_count > 64))
-   return -EINVAL;
-
if (format_modifiers) {
const uint64_t *temp_modifiers = format_modifiers;
  
--

2.25.1

Re: [PATCH] drm/komeda: return early if drm_universal_plane_init() fails.

[Carsten Haitzler]

On 12/3/21 14:15, Carsten Haitzler wrote:

On 12/3/21 10:09, Liviu Dudau wrote:
If drm_universal_plane_init() fails early we jump to the common 
cleanup code
that calls komeda_plane_destroy() which in turn could access the 
uninitalised
drm_plane and crash. Return early if an error is detected without 
going through

the common code.

Reported-by: Steven Price 
Signed-off-by: Liviu Dudau 
---
  drivers/gpu/drm/arm/display/komeda/komeda_plane.c | 6 --
  1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c 
b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c

index aa193c58f4bf6d9..517b94c3bcaf966 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
@@ -279,8 +279,10 @@ static int komeda_plane_add(struct komeda_kms_dev 
*kms,

  komeda_put_fourcc_list(formats);
-    if (err)
-    goto cleanup;
+    if (err) {
+    kfree(kplane);
+    return err;
+    }
  drm_plane_helper_add(plane, &komeda_plane_helper_funcs);



Ummm... can I disagree here? this goto cleanup I think is just fine 
because plane has been set before drm_universal_plane_init() is called 
which is before the if (err) here. komeda_plane_destroy() in cleanup: 
does all the right things, so this patch isn't needed. I think it's less 
clean as it adds a new "handle error" path special-case instance where a 
special case is not needed. The fix to Zhou's original patch was needed 
for exactly the reason Liviu said - but not this one... ?


Let me take that back - it seems an init fail shouldn't call cleanup but 
the init fail doesn't quite cleanup properly. Steven found this and 
already sent a patch.

Re: [PATCH] drm/komeda: return early if drm_universal_plane_init() fails.

[Carsten Haitzler]

On 12/3/21 10:09, Liviu Dudau wrote:

If drm_universal_plane_init() fails early we jump to the common cleanup code
that calls komeda_plane_destroy() which in turn could access the uninitalised
drm_plane and crash. Return early if an error is detected without going through
the common code.

Reported-by: Steven Price 
Signed-off-by: Liviu Dudau 
---
  drivers/gpu/drm/arm/display/komeda/komeda_plane.c | 6 --
  1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c 
b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
index aa193c58f4bf6d9..517b94c3bcaf966 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
@@ -279,8 +279,10 @@ static int komeda_plane_add(struct komeda_kms_dev *kms,
  
  	komeda_put_fourcc_list(formats);
  
-	if (err)

-   goto cleanup;
+   if (err) {
+   kfree(kplane);
+   return err;
+   }
  
  	drm_plane_helper_add(plane, &komeda_plane_helper_funcs);
  



Ummm... can I disagree here? this goto cleanup I think is just fine 
because plane has been set before drm_universal_plane_init() is called 
which is before the if (err) here. komeda_plane_destroy() in cleanup: 
does all the right things, so this patch isn't needed. I think it's less 
clean as it adds a new "handle error" path special-case instance where a 
special case is not needed. The fix to Zhou's original patch was needed 
for exactly the reason Liviu said - but not this one... ?

Re: Call for an EDID parsing library

[Carsten Haitzler]

On Wed, 7 Apr 2021 11:44:04 +0300 Pekka Paalanen  said:

> Hi all,
> 
> with display servers proliferating thanks to Wayland, and the Linux
> kernel exposing only a very limited set of information based on EDID
> (rightfully so!), the need to interpret EDID blobs is spreading even
> more. I would like to start the discussion about starting a project to
> develop a shared library for parsing EDID blobs. This is not the first
> time either, other people have suggested it years and years ago already,
> but apparently it didn't quite materialise as far as I know.
> 
> Right now, it seems that more or less every display server and other
> KMS application is hand-rolling its own EDID parsing code, even for the
> most trivial information (monitor make, model, and serial number). With
> HDR and color management support coming to Wayland, the need to parse
> more things out of EDID will only increase. These things are not
> exposed by the kernel, and most of these things have no use for the
> kernel either.
> 
> My personal motivation for this is that I don't want to be developing
> or reviewing yet another partial EDID parser implementation in Weston.
> 
> I recall ponderings about sharing the same EDID parsing code between
> the kernel and userspace, but I got the feeling that it would be a
> hindrance in process more than a benefit from sharing code. It would
> need to live in the kernel tree, to be managed with the kernel
> development process, use the kernel "standard libraries", and adhere to
> kernel programming style - all which are good and fine, but maybe also
> more restricting than useful in this case. Therefore I would suggest a
> userspace-only library.
> 
> Everyone hand-rolling their own parsing code has the obvious
> disadvantages. In the opposite, I would expect a new shared EDID
> parsing library and project to:
> - be hosted under gitlab.freedesktop.org
> - be MIT licensed
> - offer at least a C ABI
> - employ mandatory Gitlab CI to ensure with sample EDID blobs that it
>   cannot regress
> 
> Prior art can be found in various places. I believe Xorg xserver has
> its battle-tested EDID parsing code. Ajax once played with the idea in
> https://cgit.freedesktop.org/~ajax/libminitru/ . Then we have
> https://git.linuxtv.org/edid-decode.git too which has code and test
> data but not a C ABI (right?).
> 
> It does not necessarily need to be a new project. Would edid-decode
> project be open to adding a C library ABI?
> 
> edid-decode is already MIT licensed and seems to have a lot of code,
> too, but that's all I know for now.
> 
> Would there be anyone interested to take lead or work on a project like
> this?
> 
> Personally I don't think I'd be working on it, but I would be really
> happy to use it in Weston.
> 
> Should it be a new project, or grow inside edid-decode or something
> else?
> 
> I believe MIT license is important to have wide adoption of it. C ABI
> similarly. Also that it would be a "small" library without heavy
> dependencies.

I'd say it needs nothing more than libc - I can't see the justification for
more than that. If this is the case along with the above you have given, then I
see no reason for it to not be used by everyone other than the usual user
complaint of "too many dependencies (of a compositor)". :)

I'd definitely consider using it.

> What do you think? Could anyone spare their time for this?
> 
> Who would be interested in using it if this library appeared?
> 
> 
> Thanks,
> pq


-- 
- Codito, ergo sum - "I code, therefore I am" --
Carsten Haitzler - ras...@rasterman.com

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/komeda: Convert sysfs sprintf/snprintf family to sysfs_emit

[Carsten Haitzler]

On 3/30/21 2:25 AM, Tian Tao wrote:

Fix the following coccicheck warning:
drivers/gpu/drm/arm/display/komeda/komeda_dev.c:97:8-16: WARNING:
use scnprintf or sprintf
drivers/gpu/drm/arm/display/komeda/komeda_dev.c:88:8-16: WARNING:
use scnprintf or sprintf
drivers/gpu/drm/arm/display/komeda/komeda_dev.c:65:8-16: WARNING:
use scnprintf or sprintf

Signed-off-by: Tian Tao 
---
  drivers/gpu/drm/arm/display/komeda/komeda_dev.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_dev.c 
b/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
index ca891ae..cc7664c 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
@@ -62,7 +62,7 @@ core_id_show(struct device *dev, struct device_attribute 
*attr, char *buf)
  {
struct komeda_dev *mdev = dev_to_mdev(dev);
  
-	return snprintf(buf, PAGE_SIZE, "0x%08x\n", mdev->chip.core_id);

+   return sysfs_emit(buf, "0x%08x\n", mdev->chip.core_id);
  }
  static DEVICE_ATTR_RO(core_id);
  
@@ -85,7 +85,7 @@ config_id_show(struct device *dev, struct device_attribute *attr, char *buf)

if (pipe->layers[i]->layer_type == KOMEDA_FMT_RICH_LAYER)
config_id.n_richs++;
}
-   return snprintf(buf, PAGE_SIZE, "0x%08x\n", config_id.value);
+   return sysfs_emit(buf, "0x%08x\n", config_id.value);
  }
  static DEVICE_ATTR_RO(config_id);
  
@@ -94,7 +94,7 @@ aclk_hz_show(struct device *dev, struct device_attribute *attr, char *buf)

  {
struct komeda_dev *mdev = dev_to_mdev(dev);
  
-	return snprintf(buf, PAGE_SIZE, "%lu\n", clk_get_rate(mdev->aclk));

+   return sysfs_emit(buf, "%lu\n", clk_get_rate(mdev->aclk));
  }
  static DEVICE_ATTR_RO(aclk_hz);
  



Looks OK to me.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/komeda: Fix off-by-1 when with readback conn due to rounding

[Carsten Haitzler]

On 3/12/21 10:55 AM, Brian Starkey wrote:

(Adding back James again - did you use get_maintainer.pl?)

On Thu, Mar 11, 2021 at 12:08:46PM +, carsten.haitz...@foss.arm.com wrote:

From: Carsten Haitzler 

When setting up a readback connector that writes data back to memory
rather than to an actual output device (HDMI etc.), rounding was set
to round. As the DPU uses a higher internal number of bits when generating
a color value, this round-down back to 8bit ended up with everything
being off-by one. e.g. #fefefe became #ff. This sets


Perhaps overly pedantic, but now we've tracked down what was actually
happening I think we can be more precise here. Not _everything_ is
off-by-one, it's just rounding in the standard sense - if the most


Well a very large number of pixels were off-by-1 ... I guess it's an 
exaggeration but a "vast number of pixels were off by 1". I guess I was 
just using common terms like "everything is expensive here" doesn't 
actually mean absolutely everything but a very vast number of things. 
You know what I mean. :) The comment as a whole describing rounding 
policies should provide more details. I just write the log as a "when 
spelunking through history, this log will give me some broader insight 
into what this change is without being war and peace and If I want to 
see more and this commit is interesting to my spelunking efforts, I'll 
git log -U to read that".



significant bit-to-be-discarded is set, the value is rounded up to
minimise the absolute error introduced by bit-depth reduction.


rounding to "round-down" so things end up correct by turning on the LW_TRC
round down flag.


Can we call it "truncate" rather than round down? I think it makes
"TRC" a bit more understandable.


That's the official name from the docs though (TRC)... makes it easier 
to match to them... So I think you can argue this both ways. The comment 
where it's used though does make it clear...




Signed-off-by: Carsten Haitzler 
---
  drivers/gpu/drm/arm/display/komeda/d71/d71_component.c | 7 ++-
  drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h  | 1 +
  2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c 
b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
index 8a02ade369db..e97acc5519d1 100644
--- a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
+++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
@@ -468,7 +468,12 @@ static void d71_wb_layer_update(struct komeda_component *c,
struct komeda_layer_state *st = to_layer_st(state);
struct drm_connector_state *conn_st = state->wb_conn->state;
struct komeda_fb *kfb = to_kfb(conn_st->writeback_job->fb);
-   u32 ctrl = L_EN | LW_OFM, mask = L_EN | LW_OFM | LW_TBU_EN;
+   /* LW_TRC sets rounding to truncate not round which is needed for
+* the output of writeback to match the input in the most common
+* use cases like RGB888 -> RGB888, so set this bit by default
+*/


Hm, not sure why this file uses "net/" style comments, but as you
said, this is in-keeping with the rest of the file, so meh :-)


Yup. Just stick to "follow the style there" unless there is seemingly a 
good reason that what is there is horribly "broken" and needs fixing up.



+   u32 ctrl = LW_TRC | L_EN | LW_OFM;
+   u32 mask = LW_TRC | L_EN | LW_OFM | LW_TBU_EN;


If you were aiming for matching register order, this should be:

 L_EN | LW_TRC | LW_OFM | LW_TBU_EN


I think it'd be nice to have the exact behaviour in the commit
message, but either way this seems OK as a pragmatic fix so:


git log -U ? :)


Reviewed-by: Brian Starkey 

Thanks,
-Brian


u32 __iomem *reg = c->reg;
  
  	d71_layer_update_fb(c, kfb, st->addr);

diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h 
b/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h
index e80172a0b320..a8036689d721 100644
--- a/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h
+++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h
@@ -321,6 +321,7 @@
  #define LAYER_WR_FORMAT   0x0D8
  
  /* Layer_WR control bits */

+#define LW_TRC BIT(1)
  #define LW_OFMBIT(4)
  #define LW_LALPHA(x)  (((x) & 0xFF) << 8)
  #define LW_A_WCACHE(x)(((x) & 0xF) << 28)
--
2.30.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel



___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/komeda: Fix off-by-1 when with readback conn due to rounding

[carsten . haitzler]

From: Carsten Haitzler 

When setting up a readback connector that writes data back to memory
rather than to an actual output device (HDMI etc.), rounding was set
to round. As the DPU uses a higher internal number of bits when generating
a color value, this round-down back to 8bit ended up with everything
being off-by one. e.g. #fefefe became #ff. This sets
rounding to "round-down" so things end up correct by turning on the LW_TRC
round down flag.

Signed-off-by: Carsten Haitzler 
---
 drivers/gpu/drm/arm/display/komeda/d71/d71_component.c | 7 ++-
 drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h  | 1 +
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c 
b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
index 8a02ade369db..e97acc5519d1 100644
--- a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
+++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
@@ -468,7 +468,12 @@ static void d71_wb_layer_update(struct komeda_component *c,
struct komeda_layer_state *st = to_layer_st(state);
struct drm_connector_state *conn_st = state->wb_conn->state;
struct komeda_fb *kfb = to_kfb(conn_st->writeback_job->fb);
-   u32 ctrl = L_EN | LW_OFM, mask = L_EN | LW_OFM | LW_TBU_EN;
+   /* LW_TRC sets rounding to truncate not round which is needed for
+* the output of writeback to match the input in the most common
+* use cases like RGB888 -> RGB888, so set this bit by default
+*/
+   u32 ctrl = LW_TRC | L_EN | LW_OFM;
+   u32 mask = LW_TRC | L_EN | LW_OFM | LW_TBU_EN;
u32 __iomem *reg = c->reg;
 
d71_layer_update_fb(c, kfb, st->addr);
diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h 
b/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h
index e80172a0b320..a8036689d721 100644
--- a/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h
+++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h
@@ -321,6 +321,7 @@
 #define LAYER_WR_FORMAT0x0D8
 
 /* Layer_WR control bits */
+#define LW_TRC BIT(1)
 #define LW_OFM BIT(4)
 #define LW_LALPHA(x)   (((x) & 0xFF) << 8)
 #define LW_A_WCACHE(x) (((x) & 0xF) << 28)
-- 
2.30.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/komeda: Fix off-by-1 when with readback conn due to rounding

[Carsten Haitzler]

On 3/9/21 11:36 AM, Brian Starkey wrote:

Hi Carsten, (+James for komeda)

Thanks for typing this up.

On Fri, Mar 05, 2021 at 04:38:53PM +, carsten.haitz...@foss.arm.com wrote:

From: Carsten Haitzler 

When setting up a readback conenctor that writes data back to memory


s/readback conenctor/writeback connector/ (similar in the subject)


rather than to an actual output device (HDMI etc.), rounding was ses


s/ses/set/


I swear I re-read the log text... I must be auto-correcting in my head 
as I read. :)



to round-down. As the DPU uses a higher internal number of bits when


"round-down" isn't really accurate - the rounding mode "rounds" based
on the most-significant discarded bit - so can round-up too.

Come to think of it, I can't explain 0xff becoming 0xfe, but still,
truncation is likely fine.


Actually it was the other way - I mixed up the src/dest, but TRC does 
fix it which is the important bit.



generating a color value, this round-down back to 8bit ended up with
everything being off-by one. e.g. #ff became #fefefe. This sets
rounding to "round" so things end up correct by turning on the round
flag (LW_TRC).


LW_TRC is the truncation flag. 0: Round, 1: Truncate



Signed-off-by: Carsten Haitzler 
---
  drivers/gpu/drm/arm/display/komeda/d71/d71_component.c | 6 +-
  drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h  | 1 +
  2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c 
b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
index 8a02ade369db..d551e79fa0f1 100644
--- a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
+++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
@@ -468,7 +468,11 @@ static void d71_wb_layer_update(struct komeda_component *c,
struct komeda_layer_state *st = to_layer_st(state);
struct drm_connector_state *conn_st = state->wb_conn->state;
struct komeda_fb *kfb = to_kfb(conn_st->writeback_job->fb);
-   u32 ctrl = L_EN | LW_OFM, mask = L_EN | LW_OFM | LW_TBU_EN;
+   /* LW_TRC sets rounding to round not truncate which is needed for
+ * the output of writeback to match the input in the most common
+ * use cases like RGB888 -> RGB888, so set this bit by default */


/*
  * Comment style should be like this
  */

Same as above though - your description is inverted. By setting the
LW_TRC bit, you're forcing the hardware to truncate instead of round.


Yeah - inverted. But the source does have mixed comment styles with some
/*
 * 
 */

and some

/*  */

and some

/* 
 */

with the last 2 most common.


+   u32 ctrl = L_EN | LW_OFM | LW_TRC;
+   u32 mask = L_EN | LW_OFM | LW_TBU_EN | LW_TRC;


Really nitpicking, but I think it'd be good to keep these in the same
order as the bits in the register: L_EN | LW_TRC | LW_OFM | LW_TBU_EN


I can do that. I'll send another with the above.


Cheers,
-Brian


u32 __iomem *reg = c->reg;
  
  	d71_layer_update_fb(c, kfb, st->addr);

diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h 
b/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h
index e80172a0b320..a8036689d721 100644
--- a/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h
+++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h
@@ -321,6 +321,7 @@
  #define LAYER_WR_FORMAT   0x0D8
  
  /* Layer_WR control bits */

+#define LW_TRC BIT(1)
  #define LW_OFMBIT(4)
  #define LW_LALPHA(x)  (((x) & 0xFF) << 8)
  #define LW_A_WCACHE(x)(((x) & 0xF) << 28)
--
2.30.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/komeda: Fix off-by-1 when with readback conn due to rounding

[carsten . haitzler]

From: Carsten Haitzler 

When setting up a readback conenctor that writes data back to memory
rather than to an actual output device (HDMI etc.), rounding was ses
to round-down. As the DPU uses a higher internal number of bits when
generating a color value, this round-down back to 8bit ended up with
everything being off-by one. e.g. #ff became #fefefe. This sets
rounding to "round" so things end up correct by turning on the round
flag (LW_TRC).

Signed-off-by: Carsten Haitzler 
---
 drivers/gpu/drm/arm/display/komeda/d71/d71_component.c | 6 +-
 drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h  | 1 +
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c 
b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
index 8a02ade369db..d551e79fa0f1 100644
--- a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
+++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
@@ -468,7 +468,11 @@ static void d71_wb_layer_update(struct komeda_component *c,
struct komeda_layer_state *st = to_layer_st(state);
struct drm_connector_state *conn_st = state->wb_conn->state;
struct komeda_fb *kfb = to_kfb(conn_st->writeback_job->fb);
-   u32 ctrl = L_EN | LW_OFM, mask = L_EN | LW_OFM | LW_TBU_EN;
+   /* LW_TRC sets rounding to round not truncate which is needed for
+ * the output of writeback to match the input in the most common
+ * use cases like RGB888 -> RGB888, so set this bit by default */
+   u32 ctrl = L_EN | LW_OFM | LW_TRC;
+   u32 mask = L_EN | LW_OFM | LW_TBU_EN | LW_TRC;
u32 __iomem *reg = c->reg;
 
d71_layer_update_fb(c, kfb, st->addr);
diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h 
b/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h
index e80172a0b320..a8036689d721 100644
--- a/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h
+++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h
@@ -321,6 +321,7 @@
 #define LAYER_WR_FORMAT0x0D8
 
 /* Layer_WR control bits */
+#define LW_TRC BIT(1)
 #define LW_OFM BIT(4)
 #define LW_LALPHA(x)   (((x) & 0xFF) << 8)
 #define LW_A_WCACHE(x) (((x) & 0xF) << 28)
-- 
2.30.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/komeda: Fix bit check to import to value of proper type

[carsten . haitzler]

From: Carsten Haitzler 

Another issue found by KASAN. The bit finding is buried inside the
dp_for_each_set_bit() macro (that passes on to for_each_set_bit() that
calls the bit stuff. These bit functions want an unsigned long pointer
as input and just dumbly casting leads to out-of-bounds accesses.
This fixes that.

Signed-off-by: Carsten Haitzler 
---
 .../drm/arm/display/include/malidp_utils.h|  3 ---
 .../drm/arm/display/komeda/komeda_pipeline.c  | 16 +++-
 .../display/komeda/komeda_pipeline_state.c| 19 +++
 3 files changed, 22 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/arm/display/include/malidp_utils.h 
b/drivers/gpu/drm/arm/display/include/malidp_utils.h
index 3bc383d5bf73..49a1d7f3539c 100644
--- a/drivers/gpu/drm/arm/display/include/malidp_utils.h
+++ b/drivers/gpu/drm/arm/display/include/malidp_utils.h
@@ -13,9 +13,6 @@
 #define has_bit(nr, mask)  (BIT(nr) & (mask))
 #define has_bits(bits, mask)   (((bits) & (mask)) == (bits))
 
-#define dp_for_each_set_bit(bit, mask) \
-   for_each_set_bit((bit), ((unsigned long *)&(mask)), sizeof(mask) * 8)
-
 #define dp_wait_cond(__cond, __tries, __min_range, __max_range)\
 ({ \
int num_tries = __tries;\
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c 
b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
index 719a79728e24..06c595378dda 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
@@ -46,8 +46,9 @@ void komeda_pipeline_destroy(struct komeda_dev *mdev,
 {
struct komeda_component *c;
int i;
+   unsigned long avail_comps = pipe->avail_comps;
 
-   dp_for_each_set_bit(i, pipe->avail_comps) {
+   for_each_set_bit(i, &avail_comps, 32) {
c = komeda_pipeline_get_component(pipe, i);
komeda_component_destroy(mdev, c);
}
@@ -247,6 +248,7 @@ static void komeda_pipeline_dump(struct komeda_pipeline 
*pipe)
 {
struct komeda_component *c;
int id;
+   unsigned long avail_comps = pipe->avail_comps;
 
DRM_INFO("Pipeline-%d: n_layers: %d, n_scalers: %d, output: %s.\n",
 pipe->id, pipe->n_layers, pipe->n_scalers,
@@ -258,7 +260,7 @@ static void komeda_pipeline_dump(struct komeda_pipeline 
*pipe)
 pipe->of_output_links[1] ?
 pipe->of_output_links[1]->full_name : "none");
 
-   dp_for_each_set_bit(id, pipe->avail_comps) {
+   for_each_set_bit(id, &avail_comps, 32) {
c = komeda_pipeline_get_component(pipe, id);
 
komeda_component_dump(c);
@@ -270,8 +272,9 @@ static void komeda_component_verify_inputs(struct 
komeda_component *c)
struct komeda_pipeline *pipe = c->pipeline;
struct komeda_component *input;
int id;
+   unsigned long supported_inputs = c->supported_inputs;
 
-   dp_for_each_set_bit(id, c->supported_inputs) {
+   for_each_set_bit(id, &supported_inputs, 32) {
input = komeda_pipeline_get_component(pipe, id);
if (!input) {
c->supported_inputs &= ~(BIT(id));
@@ -302,8 +305,9 @@ static void komeda_pipeline_assemble(struct komeda_pipeline 
*pipe)
struct komeda_component *c;
struct komeda_layer *layer;
int i, id;
+   unsigned long avail_comps = pipe->avail_comps;
 
-   dp_for_each_set_bit(id, pipe->avail_comps) {
+   for_each_set_bit(id, &avail_comps, 32) {
c = komeda_pipeline_get_component(pipe, id);
komeda_component_verify_inputs(c);
}
@@ -355,13 +359,15 @@ void komeda_pipeline_dump_register(struct komeda_pipeline 
*pipe,
 {
struct komeda_component *c;
u32 id;
+   unsigned long avail_comps;
 
seq_printf(sf, "\n Pipeline-%d ==\n", pipe->id);
 
if (pipe->funcs && pipe->funcs->dump_register)
pipe->funcs->dump_register(pipe, sf);
 
-   dp_for_each_set_bit(id, pipe->avail_comps) {
+   avail_comps = pipe->avail_comps;
+   for_each_set_bit(id, &avail_comps, 32) {
c = komeda_pipeline_get_component(pipe, id);
 
seq_printf(sf, "\n--%s--\n", c->name);
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c 
b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
index e8b1e15312d8..176cdf411f9f 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
@@ -1232,14 +1232,15 @@ komeda_pipeline_unbound_components(struct 
komeda_pipeline *pipe,
struct komeda_pipeline_state *old = priv_to_pipe_st(pipe->o

Re: [PATCH] drm/komeda: Fix bit check to import to value of proper type

[Carsten Haitzler]

On 1/20/21 3:44 PM, Steven Price wrote:

Sent a new patch to list with updates as discussed.


On 18/01/2021 14:20, carsten.haitz...@foss.arm.com wrote:

From: Carsten Haitzler 

Another issue found by KASAN. The bit finding is bueried inside the


NIT: s/bueried/buried/


dp_for_each_set_bit() macro (that passes on to for_each_set_bit() that
calls the bit stuff. These bit functions want an unsigned long pointer
as input and just dumbly casting leads to out-of-bounds accesses.
This fixes that.


This seems like an underlying bug/lack of clear documentation for the
underlying find_*_bit() functions. dp_for_each_set_bit() tries to do the
right thing:

   #define dp_for_each_set_bit(bit, mask) \
   for_each_set_bit((bit), ((unsigned long *)&(mask)), sizeof(mask) 
* 8)


i.e. passing the actual size of type. However because of the case to
unsigned long (and subsequent accesses using that type) the compiler is
free to make accesses beyond the size of the variable (and indeed this
is completely broken on big-endian). The implementation actually
requires that the bitmap is really an array of unsigned long - no other
type will work.

So I think the fix should also include fixing the dp_for_each_set_bit()
macro - the cast is bogus as it stands.

Doing that I also get warnings on komeda_pipeline::avail_comps and
komeda_pipeline::supported_inputs - although I note there are other
bitmasks mentioned.

The other option is to fix dp_for_each_set_bit() itself using a little 
hack:


-   for_each_set_bit((bit), ((unsigned long *)&(mask)), sizeof(mask) 
* 8)
+   for_each_set_bit((bit), (&((unsigned long){mask})), sizeof(mask) 
* 8)


With that I don't think you need any other change as the mask is actually
in a new unsigned long on the stack.

Steve



Signed-off-by: Carsten Haitzler 
---
  .../drm/arm/display/komeda/komeda_pipeline_state.c | 14 --
  1 file changed, 8 insertions(+), 6 deletions(-)

diff --git 
a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c 
b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c

index e8b1e15312d8..f7dddb9f015d 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
@@ -1232,7 +1232,8 @@ komeda_pipeline_unbound_components(struct 
komeda_pipeline *pipe,
  struct komeda_pipeline_state *old = 
priv_to_pipe_st(pipe->obj.state);

  struct komeda_component_state *c_st;
  struct komeda_component *c;
-    u32 disabling_comps, id;
+    u32 id;
+    unsigned long disabling_comps;
  WARN_ON(!old);
@@ -1262,7 +1263,6 @@ int komeda_release_unclaimed_resources(struct 
komeda_pipeline *pipe,

  st = komeda_pipeline_get_new_state(pipe, drm_st);
  else
  st = komeda_pipeline_get_state_and_set_crtc(pipe, drm_st, 
NULL);

-


NIT: Random white space change


  if (WARN_ON(IS_ERR_OR_NULL(st)))
  return -EINVAL;
@@ -1287,7 +1287,8 @@ bool komeda_pipeline_disable(struct 
komeda_pipeline *pipe,

  struct komeda_pipeline_state *old;
  struct komeda_component *c;
  struct komeda_component_state *c_st;
-    u32 id, disabling_comps = 0;
+    u32 id;
+    unsigned long disabling_comps;
  old = komeda_pipeline_get_old_state(pipe, old_state);
@@ -1297,7 +1298,7 @@ bool komeda_pipeline_disable(struct 
komeda_pipeline *pipe,

  disabling_comps = old->active_comps &
    pipe->standalone_disabled_comps;
-    DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, disabling_comps: 
0x%x.\n",
+    DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, disabling_comps: 
0x%lx.\n",

   pipe->id, old->active_comps, disabling_comps);
  dp_for_each_set_bit(id, disabling_comps) {
@@ -1331,13 +1332,14 @@ void komeda_pipeline_update(struct 
komeda_pipeline *pipe,
  struct komeda_pipeline_state *new = 
priv_to_pipe_st(pipe->obj.state);

  struct komeda_pipeline_state *old;
  struct komeda_component *c;
-    u32 id, changed_comps = 0;
+    u32 id;
+    unsigned long changed_comps;
  old = komeda_pipeline_get_old_state(pipe, old_state);
  changed_comps = new->active_comps | old->active_comps;
-    DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, changed: 0x%x.\n",
+    DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, changed: 0x%lx.\n",
   pipe->id, new->active_comps, changed_comps);
  dp_for_each_set_bit(id, changed_comps) {





___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/komeda: Fix bit check to import to value of proper type

[carsten . haitzler]

From: Carsten Haitzler 

Another issue found by KASAN. The bit finding is buried inside the
dp_for_each_set_bit() macro (that passes on to for_each_set_bit() that
calls the bit stuff. These bit functions want an unsigned long pointer
as input and just dumbly casting leads to out-of-bounds accesses.
This fixes that.

Signed-off-by: Carsten Haitzler 
---
 .../gpu/drm/arm/display/include/malidp_utils.h   | 10 --
 .../gpu/drm/arm/display/komeda/komeda_pipeline.c | 16 +++-
 .../arm/display/komeda/komeda_pipeline_state.c   | 13 -
 3 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/arm/display/include/malidp_utils.h 
b/drivers/gpu/drm/arm/display/include/malidp_utils.h
index 3bc383d5bf73..8d289cd0b5b8 100644
--- a/drivers/gpu/drm/arm/display/include/malidp_utils.h
+++ b/drivers/gpu/drm/arm/display/include/malidp_utils.h
@@ -12,9 +12,15 @@
 
 #define has_bit(nr, mask)  (BIT(nr) & (mask))
 #define has_bits(bits, mask)   (((bits) & (mask)) == (bits))
-
+/*
+#define dp_for_each_set_bit(bit, mask) \
+   for_each_set_bit((bit), (&((unsigned long)(mask))), sizeof(mask) * 8)
+#define dp_for_each_set_bit(bit, mask) \
+   unsigned long __local_mask = mask; \
+   for_each_set_bit((bit), (&__local_mask), sizeof(mask) * 8)
+*/
 #define dp_for_each_set_bit(bit, mask) \
-   for_each_set_bit((bit), ((unsigned long *)&(mask)), sizeof(mask) * 8)
+   for_each_set_bit((bit), &(mask), sizeof(mask) * 8)
 
 #define dp_wait_cond(__cond, __tries, __min_range, __max_range)\
 ({ \
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c 
b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
index 719a79728e24..a85c8a806334 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
@@ -46,8 +46,9 @@ void komeda_pipeline_destroy(struct komeda_dev *mdev,
 {
struct komeda_component *c;
int i;
+   unsigned long avail_comps = pipe->avail_comps;
 
-   dp_for_each_set_bit(i, pipe->avail_comps) {
+   dp_for_each_set_bit(i, avail_comps) {
c = komeda_pipeline_get_component(pipe, i);
komeda_component_destroy(mdev, c);
}
@@ -247,6 +248,7 @@ static void komeda_pipeline_dump(struct komeda_pipeline 
*pipe)
 {
struct komeda_component *c;
int id;
+   unsigned long avail_comps = pipe->avail_comps;
 
DRM_INFO("Pipeline-%d: n_layers: %d, n_scalers: %d, output: %s.\n",
 pipe->id, pipe->n_layers, pipe->n_scalers,
@@ -258,7 +260,7 @@ static void komeda_pipeline_dump(struct komeda_pipeline 
*pipe)
 pipe->of_output_links[1] ?
 pipe->of_output_links[1]->full_name : "none");
 
-   dp_for_each_set_bit(id, pipe->avail_comps) {
+   dp_for_each_set_bit(id, avail_comps) {
c = komeda_pipeline_get_component(pipe, id);
 
komeda_component_dump(c);
@@ -270,8 +272,9 @@ static void komeda_component_verify_inputs(struct 
komeda_component *c)
struct komeda_pipeline *pipe = c->pipeline;
struct komeda_component *input;
int id;
+   unsigned long supported_inputs = c->supported_inputs;
 
-   dp_for_each_set_bit(id, c->supported_inputs) {
+   dp_for_each_set_bit(id, supported_inputs) {
input = komeda_pipeline_get_component(pipe, id);
if (!input) {
c->supported_inputs &= ~(BIT(id));
@@ -302,8 +305,9 @@ static void komeda_pipeline_assemble(struct komeda_pipeline 
*pipe)
struct komeda_component *c;
struct komeda_layer *layer;
int i, id;
+   unsigned long avail_comps = pipe->avail_comps;
 
-   dp_for_each_set_bit(id, pipe->avail_comps) {
+   dp_for_each_set_bit(id, avail_comps) {
c = komeda_pipeline_get_component(pipe, id);
komeda_component_verify_inputs(c);
}
@@ -355,13 +359,15 @@ void komeda_pipeline_dump_register(struct komeda_pipeline 
*pipe,
 {
struct komeda_component *c;
u32 id;
+   unsigned long avail_comps;
 
seq_printf(sf, "\n Pipeline-%d ==\n", pipe->id);
 
if (pipe->funcs && pipe->funcs->dump_register)
pipe->funcs->dump_register(pipe, sf);
 
-   dp_for_each_set_bit(id, pipe->avail_comps) {
+   avail_comps = pipe->avail_comps;
+   dp_for_each_set_bit(id, avail_comps) {
c = komeda_pipeline_get_component(pipe, id);
 
seq_printf(sf, "\n--%s--\n", c->name);
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c 
b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
index e8b1e15312d8..7640dae7f4bf 1006

Re: [PATCH] drm/komeda: Fix bit check to import to value of proper type

[Carsten Haitzler]

On 1/21/21 4:40 PM, Steven Price wrote:

On 21/01/2021 12:22, Carsten Haitzler wrote:

On 1/20/21 3:44 PM, Steven Price wrote:

On 18/01/2021 14:20, carsten.haitz...@foss.arm.com wrote:

From: Carsten Haitzler 

Another issue found by KASAN. The bit finding is bueried inside the


NIT: s/bueried/buried/


Yup. typo not spotted by me. Thanks. Also - i spotted an accidental 
whitespace change along the way so can fix both in a new patch.



dp_for_each_set_bit() macro (that passes on to for_each_set_bit() that
calls the bit stuff. These bit functions want an unsigned long pointer
as input and just dumbly casting leads to out-of-bounds accesses.
This fixes that.


This seems like an underlying bug/lack of clear documentation for the
underlying find_*_bit() functions. dp_for_each_set_bit() tries to do the
right thing:


Correct. This was a general problem I spotted - the bit funcs were 
written to want a unsigned long but were being used on u32's by just 
casting the ptr to the type and this did indeed have technical issues.



   #define dp_for_each_set_bit(bit, mask) \
   for_each_set_bit((bit), ((unsigned long *)&(mask)), 
sizeof(mask) * 8)


i.e. passing the actual size of type. However because of the case to
unsigned long (and subsequent accesses using that type) the compiler is
free to make accesses beyond the size of the variable (and indeed this
is completely broken on big-endian). The implementation actually
requires that the bitmap is really an array of unsigned long - no other
type will work.


Precisely. So a bit loose. The bit funcs are used widely enough, so 
just fixing this code here to pass in the expected type is probably 
the least disruptive fix.



So I think the fix should also include fixing the dp_for_each_set_bit()
macro - the cast is bogus as it stands.


Yup. Removing the cast does indeed find more badness that needs 
fixing. I'll do an updated patch with this.



Doing that I also get warnings on komeda_pipeline::avail_comps and
komeda_pipeline::supported_inputs - although I note there are other
bitmasks mentioned.

The other option is to fix dp_for_each_set_bit() itself using a 
little hack:


-   for_each_set_bit((bit), ((unsigned long *)&(mask)), 
sizeof(mask) * 8)
+   for_each_set_bit((bit), (&((unsigned long){mask})), 
sizeof(mask) * 8)


With that I don't think you need any other change as the mask is 
actually

in a new unsigned long on the stack.


That would be wonderful if it worked :). Unfortunately your above 
proposal leads to:


./drivers/gpu/drm/arm/display/komeda/../include/malidp_utils.h:17:27: 
error: lvalue required as unary ‘&’ operand
    17 |  for_each_set_bit((bit), (&((unsigned long)(mask))), 
sizeof(mask) * 8)

   |   ^


Looks like you didn't notice the subtle change above. My change uses 
braces ('{}') around 'mask' - I believe it's a GCC extension ("compound 
literals") and it creates an lvalue so you can then take the address of 
it...


Oh... ewww. I did indeed skip the {}'s and just looked at them as ()'s 
:) I'm not so hot on using such extensions if it can be avoided. :) I 
just removed the cast and fixed up all the usages. Patch will come with 
this.


I'm not sure if it's a good approach to the problem or not. The 
alternative is to fix up various places to use unsigned longs so you can 
use the unwrapped for_each_set_bit().


Steve

./include/linux/bitops.h:34:30: note: in definition of macro 
‘for_each_set_bit’

    34 |   (bit) = find_next_bit((addr), (size), (bit) + 1))
   |  ^~~~
drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c:1243:2: 
note: in expansion of macro ‘dp_for_each_set_bit’

  1243 |  dp_for_each_set_bit(id, disabling_comps) {
   |  ^~~

Basically can't take address of an "unnamed local var". :| That is with:

#define dp_for_each_set_bit(bit, mask) \
 for_each_set_bit((bit), (&((unsigned long)(mask))), 
sizeof(mask) * 8)


I could try have the dp_for_each macro create new local vars on its 
own a bit like:


#define dp_for_each_set_bit(bit, mask) \
 unsigned long __local_mask = mask; \
 for_each_set_bit((bit), (&__local_mask), sizeof(mask) * 8)

But we all know where this leads... (multiple bad places with adding 
warnings and potential and pitfalls that then lead with ever more 
invasive changes to address like if code in future might do if (x) 
dp_for_each...). I'd prefer to be able to write code more loosely 
(pass in any time and it just does the right thing), but trying to 
balance this with least disruption and ugliness.



Steve



Signed-off-by: Carsten Haitzler 
---
  .../drm/arm/display/komeda/komeda_pipeline_state.c | 14 
--

  1 file changed, 8 insertions(+), 6 deletions(-)

diff --git 
a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c 

Re: [PATCH] drm/komeda: Fix bit check to import to value of proper type

[Carsten Haitzler]

On 1/20/21 3:44 PM, Steven Price wrote:

On 18/01/2021 14:20, carsten.haitz...@foss.arm.com wrote:

From: Carsten Haitzler 

Another issue found by KASAN. The bit finding is bueried inside the


NIT: s/bueried/buried/


Yup. typo not spotted by me. Thanks. Also - i spotted an accidental 
whitespace change along the way so can fix both in a new patch.



dp_for_each_set_bit() macro (that passes on to for_each_set_bit() that
calls the bit stuff. These bit functions want an unsigned long pointer
as input and just dumbly casting leads to out-of-bounds accesses.
This fixes that.


This seems like an underlying bug/lack of clear documentation for the
underlying find_*_bit() functions. dp_for_each_set_bit() tries to do the
right thing:


Correct. This was a general problem I spotted - the bit funcs were 
written to want a unsigned long but were being used on u32's by just 
casting the ptr to the type and this did indeed have technical issues.



   #define dp_for_each_set_bit(bit, mask) \
   for_each_set_bit((bit), ((unsigned long *)&(mask)), sizeof(mask) 
* 8)


i.e. passing the actual size of type. However because of the case to
unsigned long (and subsequent accesses using that type) the compiler is
free to make accesses beyond the size of the variable (and indeed this
is completely broken on big-endian). The implementation actually
requires that the bitmap is really an array of unsigned long - no other
type will work.


Precisely. So a bit loose. The bit funcs are used widely enough, so just 
fixing this code here to pass in the expected type is probably the least 
disruptive fix.



So I think the fix should also include fixing the dp_for_each_set_bit()
macro - the cast is bogus as it stands.


Yup. Removing the cast does indeed find more badness that needs fixing. 
I'll do an updated patch with this.



Doing that I also get warnings on komeda_pipeline::avail_comps and
komeda_pipeline::supported_inputs - although I note there are other
bitmasks mentioned.

The other option is to fix dp_for_each_set_bit() itself using a little 
hack:


-   for_each_set_bit((bit), ((unsigned long *)&(mask)), sizeof(mask) 
* 8)
+   for_each_set_bit((bit), (&((unsigned long){mask})), sizeof(mask) 
* 8)


With that I don't think you need any other change as the mask is actually
in a new unsigned long on the stack.


That would be wonderful if it worked :). Unfortunately your above 
proposal leads to:


./drivers/gpu/drm/arm/display/komeda/../include/malidp_utils.h:17:27: 
error: lvalue required as unary ‘&’ operand
   17 |  for_each_set_bit((bit), (&((unsigned long)(mask))), 
sizeof(mask) * 8)

  |   ^
./include/linux/bitops.h:34:30: note: in definition of macro 
‘for_each_set_bit’

   34 |   (bit) = find_next_bit((addr), (size), (bit) + 1))
  |  ^~~~
drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c:1243:2: note: 
in expansion of macro ‘dp_for_each_set_bit’

 1243 |  dp_for_each_set_bit(id, disabling_comps) {
  |  ^~~

Basically can't take address of an "unnamed local var". :| That is with:

#define dp_for_each_set_bit(bit, mask) \
for_each_set_bit((bit), (&((unsigned long)(mask))), 
sizeof(mask) * 8)


I could try have the dp_for_each macro create new local vars on its own 
a bit like:


#define dp_for_each_set_bit(bit, mask) \
unsigned long __local_mask = mask; \
for_each_set_bit((bit), (&__local_mask), sizeof(mask) * 8)

But we all know where this leads... (multiple bad places with adding 
warnings and potential and pitfalls that then lead with ever more 
invasive changes to address like if code in future might do if (x) 
dp_for_each...). I'd prefer to be able to write code more loosely (pass 
in any time and it just does the right thing), but trying to balance 
this with least disruption and ugliness.



Steve



Signed-off-by: Carsten Haitzler 
---
  .../drm/arm/display/komeda/komeda_pipeline_state.c | 14 --
  1 file changed, 8 insertions(+), 6 deletions(-)

diff --git 
a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c 
b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c

index e8b1e15312d8..f7dddb9f015d 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
@@ -1232,7 +1232,8 @@ komeda_pipeline_unbound_components(struct 
komeda_pipeline *pipe,
  struct komeda_pipeline_state *old = 
priv_to_pipe_st(pipe->obj.state);

  struct komeda_component_state *c_st;
  struct komeda_component *c;
-    u32 disabling_comps, id;
+    u32 id;
+    unsigned long disabling_comps;
  WARN_ON(!old);
@@ -1262,7 +1263,6 @@ int komeda_release_unclaimed_resources(struct 
komeda_pipeline *pipe,

  st = komeda_pipeline_get_new_state(pipe, drm_st);
  else
  st = komeda_pipeline_get_state_and_set_crtc(pi

[PATCH] drm/komeda: Fix bit check to import to value of proper type

[carsten . haitzler]

From: Carsten Haitzler 

Another issue found by KASAN. The bit finding is bueried inside the
dp_for_each_set_bit() macro (that passes on to for_each_set_bit() that
calls the bit stuff. These bit functions want an unsigned long pointer
as input and just dumbly casting leads to out-of-bounds accesses.
This fixes that.

Signed-off-by: Carsten Haitzler 
---
 .../drm/arm/display/komeda/komeda_pipeline_state.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c 
b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
index e8b1e15312d8..f7dddb9f015d 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
@@ -1232,7 +1232,8 @@ komeda_pipeline_unbound_components(struct komeda_pipeline 
*pipe,
struct komeda_pipeline_state *old = priv_to_pipe_st(pipe->obj.state);
struct komeda_component_state *c_st;
struct komeda_component *c;
-   u32 disabling_comps, id;
+   u32 id;
+   unsigned long disabling_comps;
 
WARN_ON(!old);
 
@@ -1262,7 +1263,6 @@ int komeda_release_unclaimed_resources(struct 
komeda_pipeline *pipe,
st = komeda_pipeline_get_new_state(pipe, drm_st);
else
st = komeda_pipeline_get_state_and_set_crtc(pipe, drm_st, NULL);
-
if (WARN_ON(IS_ERR_OR_NULL(st)))
return -EINVAL;
 
@@ -1287,7 +1287,8 @@ bool komeda_pipeline_disable(struct komeda_pipeline *pipe,
struct komeda_pipeline_state *old;
struct komeda_component *c;
struct komeda_component_state *c_st;
-   u32 id, disabling_comps = 0;
+   u32 id;
+   unsigned long disabling_comps;
 
old = komeda_pipeline_get_old_state(pipe, old_state);
 
@@ -1297,7 +1298,7 @@ bool komeda_pipeline_disable(struct komeda_pipeline *pipe,
disabling_comps = old->active_comps &
  pipe->standalone_disabled_comps;
 
-   DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, disabling_comps: 0x%x.\n",
+   DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, disabling_comps: 
0x%lx.\n",
 pipe->id, old->active_comps, disabling_comps);
 
dp_for_each_set_bit(id, disabling_comps) {
@@ -1331,13 +1332,14 @@ void komeda_pipeline_update(struct komeda_pipeline 
*pipe,
struct komeda_pipeline_state *new = priv_to_pipe_st(pipe->obj.state);
struct komeda_pipeline_state *old;
struct komeda_component *c;
-   u32 id, changed_comps = 0;
+   u32 id;
+   unsigned long changed_comps;
 
old = komeda_pipeline_get_old_state(pipe, old_state);
 
changed_comps = new->active_comps | old->active_comps;
 
-   DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, changed: 0x%x.\n",
+   DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, changed: 0x%lx.\n",
 pipe->id, new->active_comps, changed_comps);
 
dp_for_each_set_bit(id, changed_comps) {
-- 
2.29.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/komeda: Fix bit check to import to value of proper type

[carsten . haitzler]

From: Carsten Haitzler 

KASAN found this problem. find_first_bit() expects to look at a
pointer pointing to a long, but we look at a u32 - this is going to be
an issue with endianess but, KSAN already flags this as out-of-bounds
stack reads. This fixes it by just importing inot a local long.

Signed-off-by: Carsten Haitzler 
---
 drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c 
b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
index 452e505a1fd3..719a79728e24 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
@@ -137,9 +137,10 @@ komeda_pipeline_get_first_component(struct komeda_pipeline 
*pipe,
u32 comp_mask)
 {
struct komeda_component *c = NULL;
+   unsigned long comp_mask_local = (unsigned long)comp_mask;
int id;
 
-   id = find_first_bit((unsigned long *)&comp_mask, 32);
+   id = find_first_bit(&comp_mask_local, 32);
if (id < 32)
c = komeda_pipeline_get_component(pipe, id);
 
-- 
2.29.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/komeda: Handle NULL pointer access code path in error case

[carsten . haitzler]

From: Carsten Haitzler 

komeda_component_get_old_state() technically can return a NULL
pointer. komeda_compiz_set_input() even warns when this happens, but
then proceeeds to use that NULL pointer tocompare memory content there
agains the new sate to see if it changed. In this case, it's better to
assume that the input changed as there is no old state to compare
against and thus assume the changes happen anyway.

Signed-off-by: Carsten Haitzler 
---
 drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c 
b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
index 8f32ae7c25d0..e8b1e15312d8 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
@@ -707,7 +707,8 @@ komeda_compiz_set_input(struct komeda_compiz *compiz,
WARN_ON(!old_st);
 
/* compare with old to check if this input has been changed */
-   if (memcmp(&(to_compiz_st(old_st)->cins[idx]), cin, sizeof(*cin)))
+   if (!old_st ||
+   memcmp(&(to_compiz_st(old_st)->cins[idx]), cin, sizeof(*cin)))
c_st->changed_active_inputs |= BIT(idx);
 
komeda_component_add_input(c_st, &dflow->input, idx);
-- 
2.29.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/komeda: Remove useless variable assignment

[carsten . haitzler]

From: Carsten Haitzler 

ret is not actually read after this (only written in one case then
returned), so this assign line is useless. This removes that assignment.

Signed-off-by: Carsten Haitzler 
---
 drivers/gpu/drm/arm/display/komeda/komeda_dev.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_dev.c 
b/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
index 1d767473ba8a..eea76f51f662 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
@@ -163,7 +163,6 @@ static int komeda_parse_dt(struct device *dev, struct 
komeda_dev *mdev)
ret = of_reserved_mem_device_init(dev);
if (ret && ret != -ENODEV)
return ret;
-   ret = 0;
 
for_each_available_child_of_node(np, child) {
if (of_node_name_eq(child, "pipeline")) {
-- 
2.29.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel