[PATCH v7 9/9] drm_print: add _ddebug descriptor to drm_*dbg prototypes

[Jim Cromie]

upgrade the callchain to drm_dbg() and drm_dev_dbg(); add a struct
_ddebug ptr parameter to them, and supply that additional param by
replacing the '_no_desc' flavor of dyndbg Factory macro currently used
with the flavor that supplies the descriptor.

NOTES:

The descriptor gives these fns access to the decorator flags, but they
do none of the dynamic-prefixing done by dynamic_emit_prefix(), which
is currently static.

DRM already has conventions for logging/messaging; just tossing
optional decorations on top probably wouldn't help.  Instead, existing
flags (or new ones, perhaps 'sd' ala lspci) can be used to make
current message conventions optional.  This suggests a new
drmdbg_prefix_emit() to handle prefixing locally.

For CONFIG_DRM_USE_DYNAMIC_DEBUG=N, just pass null descriptor.

desc->class_id is redundant with category parameter, but its
availability is dependent on desc.

Signed-off-by: Jim Cromie 
---
 drivers/gpu/drm/drm_print.c |  8 +---
 include/drm/drm_print.h | 23 ---
 2 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
index ec477c44a784..5b93c11895bb 100644
--- a/drivers/gpu/drm/drm_print.c
+++ b/drivers/gpu/drm/drm_print.c
@@ -29,6 +29,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -278,8 +279,8 @@ void drm_dev_printk(const struct device *dev, const char 
*level,
 }
 EXPORT_SYMBOL(drm_dev_printk);
 
-void __drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
-  const char *format, ...)
+void __drm_dev_dbg(struct _ddebug *desc, const struct device *dev,
+  enum drm_debug_category category, const char *format, ...)
 {
struct va_format vaf;
va_list args;
@@ -287,6 +288,7 @@ void __drm_dev_dbg(const struct device *dev, enum 
drm_debug_category category,
if (!__drm_debug_enabled(category))
return;
 
+   /* we know we are printing for either syslog, tracefs, or both */
va_start(args, format);
vaf.fmt = format;
vaf.va = 
@@ -302,7 +304,7 @@ void __drm_dev_dbg(const struct device *dev, enum 
drm_debug_category category,
 }
 EXPORT_SYMBOL(__drm_dev_dbg);
 
-void ___drm_dbg(enum drm_debug_category category, const char *format, ...)
+void ___drm_dbg(struct _ddebug *desc, enum drm_debug_category category, const 
char *format, ...)
 {
struct va_format vaf;
va_list args;
diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index 9af57d3df259..a44fb7ef257f 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -354,9 +354,10 @@ static inline bool drm_debug_enabled_raw(enum 
drm_debug_category category)
 __printf(3, 4)
 void drm_dev_printk(const struct device *dev, const char *level,
const char *format, ...);
-__printf(3, 4)
-void __drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
-const char *format, ...);
+struct _ddebug;
+__printf(4, 5)
+void __drm_dev_dbg(struct _ddebug *desc, const struct device *dev,
+  enum drm_debug_category category, const char *format, ...);
 
 /**
  * DRM_DEV_ERROR() - Error output.
@@ -406,11 +407,11 @@ void __drm_dev_dbg(const struct device *dev, enum 
drm_debug_category category,
 
 #if !defined(CONFIG_DRM_USE_DYNAMIC_DEBUG)
 #define drm_dev_dbg(dev, cat, fmt, ...)\
-   __drm_dev_dbg(dev, cat, fmt, ##__VA_ARGS__)
+   __drm_dev_dbg(NULL, dev, cat, fmt, ##__VA_ARGS__)
 #else
 #define drm_dev_dbg(dev, cat, fmt, ...)\
-   _dynamic_func_call_no_desc(fmt, __drm_dev_dbg,  \
-  dev, cat, fmt, ##__VA_ARGS__)
+   _dynamic_func_call_cls(cat, fmt, __drm_dev_dbg, \
+  dev, cat, fmt, ##__VA_ARGS__)
 #endif
 
 /**
@@ -514,17 +515,17 @@ void __drm_dev_dbg(const struct device *dev, enum 
drm_debug_category category,
  * Prefer drm_device based logging over device or prink based logging.
  */
 
-__printf(2, 3)
-void ___drm_dbg(enum drm_debug_category category, const char *format, ...);
+__printf(3, 4)
+void ___drm_dbg(struct _ddebug *desc, enum drm_debug_category category, const 
char *format, ...);
 __printf(1, 2)
 void __drm_err(const char *format, ...);
 
 #if !defined(CONFIG_DRM_USE_DYNAMIC_DEBUG)
-#define __drm_dbg(fmt, ...)___drm_dbg(fmt, ##__VA_ARGS__)
+#define __drm_dbg(fmt, ...)___drm_dbg(NULL, fmt, ##__VA_ARGS__)
 #else
 #define __drm_dbg(cat, fmt, ...)   \
-   _dynamic_func_call_no_desc(fmt, ___drm_dbg, \
-  cat, fmt, ##__VA_ARGS__)
+   _dynamic_func_call_cls(cat, fmt, ___drm_dbg,\
+  cat, fmt, ##__VA_ARGS__)
 #endif
 
 /* Macros to make printk easier */
-- 
2.37.3

[PATCH v7 4/9] drm_print: wrap drm_*_dbg in dyndbg descriptor factory macro

[Jim Cromie]

For CONFIG_DRM_USE_DYNAMIC_DEBUG=y, wrap __drm_dbg() & __drm_dev_dbg()
in one of dyndbg's Factory macros: _dynamic_func_call_no_desc().

This adds the callsite descriptor into the code, and an entry for each
into /proc/dynamic_debug/control.

  #> echo class DRM_UT_ATOMIC +p > /proc/dynamic_debug/control

CONFIG_DRM_USE_DYNAMIC_DEBUG=y/n is configurable because of the .data
footprint cost of per-callsite control; 56 bytes/site * ~2k for i915,
~4k callsites for amdgpu.  This is large enough that a kernel builder
might not want it.

Signed-off-by: Jim Cromie 
---
 drivers/gpu/drm/Kconfig  | 12 
 drivers/gpu/drm/Makefile |  2 ++
 include/drm/drm_print.h  | 12 
 3 files changed, 26 insertions(+)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 6c2256e8474b..2438e0dccfa1 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -50,6 +50,18 @@ config DRM_DEBUG_MM
 
  If in doubt, say "N".
 
+config DRM_USE_DYNAMIC_DEBUG
+   bool "use dynamic debug to implement drm.debug"
+   default y
+   depends on DRM
+   depends on DYNAMIC_DEBUG || DYNAMIC_DEBUG_CORE
+   depends on JUMP_LABEL
+   help
+ Use dynamic-debug to avoid drm_debug_enabled() runtime overheads.
+ Due to callsite counts in DRM drivers (~4k in amdgpu) and 56
+ bytes per callsite, the .data costs can be substantial, and
+ are therefore configurable.
+
 config DRM_DEBUG_SELFTEST
tristate "kselftests for DRM"
depends on DRM
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index e7af358e6dda..6828197967a6 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -3,6 +3,8 @@
 # Makefile for the drm device driver.  This driver provides support for the
 # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
 
+CFLAGS-$(CONFIG_DRM_USE_DYNAMIC_DEBUG) += -DDYNAMIC_DEBUG_MODULE
+
 drm-y   := drm_aperture.o drm_auth.o drm_cache.o \
drm_file.o drm_gem.o drm_ioctl.o \
drm_drv.o \
diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index c429c258c957..2d2cef76b5c1 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -384,8 +384,14 @@ void __drm_dev_dbg(const struct device *dev, enum 
drm_debug_category category,
}   \
 })
 
+#if !defined(CONFIG_DRM_USE_DYNAMIC_DEBUG)
 #define drm_dev_dbg(dev, cat, fmt, ...)\
__drm_dev_dbg(dev, cat, fmt, ##__VA_ARGS__)
+#else
+#define drm_dev_dbg(dev, cat, fmt, ...)\
+   _dynamic_func_call_no_desc(fmt, __drm_dev_dbg,  \
+  dev, cat, fmt, ##__VA_ARGS__)
+#endif
 
 /**
  * DRM_DEV_DEBUG() - Debug output for generic drm code
@@ -492,7 +498,13 @@ void ___drm_dbg(enum drm_debug_category category, const 
char *format, ...);
 __printf(1, 2)
 void __drm_err(const char *format, ...);
 
+#if !defined(CONFIG_DRM_USE_DYNAMIC_DEBUG)
 #define __drm_dbg(fmt, ...)___drm_dbg(fmt, ##__VA_ARGS__)
+#else
+#define __drm_dbg(cat, fmt, ...)   \
+   _dynamic_func_call_no_desc(fmt, ___drm_dbg, \
+  cat, fmt, ##__VA_ARGS__)
+#endif
 
 /* Macros to make printk easier */
 
-- 
2.37.3

[PATCH v7 8/9] drm_print: prefer bare printk KERN_DEBUG on generic fn

[Jim Cromie]

drm_print.c calls pr_debug() just once, from __drm_printfn_debug(),
which is a generic/service fn.  The callsite is compile-time enabled
by DEBUG in both DYNAMIC_DEBUG=y/n builds.

For dyndbg builds, reverting this callsite back to bare printk is
correcting a few anti-features:

1- callsite is generic, serves multiple drm users.
   it is soft-wired on currently by #define DEBUG
   could accidentally: #> echo -p > /proc/dynamic_debug/control

2- optional "decorations" by dyndbg are unhelpful/misleading here,
   they describe only the generic site, not end users

IOW, 1,2 are unhelpful at best, and possibly confusing.

reverting yields a nominal data and text shrink:

   textdata bss dec hex filename
 462583   36604   54592 553779   87333 /kernel/drivers/gpu/drm/drm.ko
 462515   36532   54592 553639   872a7 -dirty/kernel/drivers/gpu/drm/drm.ko

Signed-off-by: Jim Cromie 
---
 drivers/gpu/drm/drm_print.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
index cb203d63b286..ec477c44a784 100644
--- a/drivers/gpu/drm/drm_print.c
+++ b/drivers/gpu/drm/drm_print.c
@@ -23,8 +23,6 @@
  * Rob Clark 
  */
 
-#define DEBUG /* for pr_debug() */
-
 #include 
 
 #include 
@@ -185,7 +183,8 @@ EXPORT_SYMBOL(__drm_printfn_info);
 
 void __drm_printfn_debug(struct drm_printer *p, struct va_format *vaf)
 {
-   pr_debug("%s %pV", p->prefix, vaf);
+   /* pr_debug callsite decorations are unhelpful here */
+   printk(KERN_DEBUG "%s %pV", p->prefix, vaf);
 }
 EXPORT_SYMBOL(__drm_printfn_debug);
 
-- 
2.37.3

[PATCH v7 5/9] drm-print.h: include dyndbg header

[Jim Cromie]

lkp robot told me:

  >> drivers/gpu/drm/drm_ioc32.c:989:2:
  error: call to undeclared function '_dynamic_func_call_cls';
  ISO C99 and later do not support implicit function declarations
  [-Wimplicit-function-declaration]

   DRM_DEBUG("comm=\"%s\", pid=%d, dev=0x%lx, auth=%d, %s\n",

Since that macro is defined in drm_print.h, and under DRM_USE_DYN*=y
configs, invokes dyndbg-factory macros, include dynamic_debug.h from
there too, so that those configs have the definitions of all the
macros in the callchain.

This is done as a separate patch mostly to see how lkp sorts it.

Signed-off-by: Jim Cromie 
---
 include/drm/drm_print.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index 2d2cef76b5c1..f8bb3e7158c6 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -31,6 +31,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 
-- 
2.37.3

[PATCH v7 6/9] drm-print: add drm_dbg_driver to improve namespace symmetry

[Jim Cromie]

drm_print defines all of these:
drm_dbg_{core,kms,prime,atomic,vbl,lease,_dp,_drmres}

but not drm_dbg_driver itself, since it was the original drm_dbg.

To improve namespace symmetry, change the drm_dbg defn to
drm_dbg_driver, and redef grandfathered name to symmetric one.

This will help with nouveau, which uses its own stack of macros to
construct calls to dev_info, dev_dbg, etc, for which adaptation means
drm_dbg_##driver constructs.

Signed-off-by: Jim Cromie 
---
 include/drm/drm_print.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index f8bb3e7158c6..dfdd81c3287c 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -468,7 +468,7 @@ void __drm_dev_dbg(const struct device *dev, enum 
drm_debug_category category,
 
 #define drm_dbg_core(drm, fmt, ...)\
drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_CORE, fmt, ##__VA_ARGS__)
-#define drm_dbg(drm, fmt, ...) \
+#define drm_dbg_driver(drm, fmt, ...)  
\
drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_DRIVER, fmt, 
##__VA_ARGS__)
 #define drm_dbg_kms(drm, fmt, ...) \
drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_KMS, fmt, ##__VA_ARGS__)
@@ -487,6 +487,7 @@ void __drm_dev_dbg(const struct device *dev, enum 
drm_debug_category category,
 #define drm_dbg_drmres(drm, fmt, ...)  \
drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_DRMRES, fmt, 
##__VA_ARGS__)
 
+#define drm_dbg(drm, fmt, ...) drm_dbg_driver(drm, fmt, ##__VA_ARGS__)
 
 /*
  * printk based logging
-- 
2.37.3

[PATCH v7 7/9] drm_print: optimize drm_debug_enabled for jump-label

[Jim Cromie]

When CONFIG_DRM_USE_DYNAMIC_DEBUG=y, the drm.debug API (a macro stack,
calling _+drm_*dbg() eventually) invokes a dyndbg Factory macro to
create a descriptor for each callsite, thus making them individually
>control-able.

In this case, the calls to _drm_*dbg are unreachable unless the
callsite is enabled.  So those calls can short-circuit their early
do-nothing returns.  Provide and use __drm_debug_enabled(), to do this
when config'd, or the _raw flags-check otherwize.

And since dyndbg is in use, lets also instrument the remaining users
of drm_debug_enabled, by wrapping the _raw in a macro with a:

  pr_debug("todo: is this frequent enough to optimize ?\n");

For CONFIG_DRM_USE_DYNAMIC_DEBUG=n, do no site instrumenting at all,
since JUMP_LABEL might be off, and we don't want to make work.

With drm, amdgpu, i915, nouveau loaded, heres remaining uses of
drm_debug_enabled(), which costs ~1.5kb data to control the
pr_debug("todo:..")s.

Some of those uses might be ok to use __drm_debug_enabled() by
inspection, others might warrant conversion to use dyndbg Factory
macros, and that would want callrate data to estimate the savings
possible.  TBH, any remaining savings are probably small; drm.debug
covers the vast bulk of the uses.  Maybe "vblank" is the exception.

:#> grep todo /proc/dynamic_debug/control | wc
 21 1682357
:#> grep todo /proc/dynamic_debug/control
drivers/gpu/drm/drm_edid_load.c:178 [drm]edid_load =_ "todo: maybe avoid via 
dyndbg\n"
drivers/gpu/drm/drm_vblank.c:410 [drm]drm_crtc_accurate_vblank_count =_ "todo: 
maybe avoid via dyndbg\n"
drivers/gpu/drm/drm_vblank.c:787 
[drm]drm_crtc_vblank_helper_get_vblank_timestamp_internal =_ "todo: maybe avoid 
via dyndbg\n"
drivers/gpu/drm/drm_vblank.c:1491 [drm]drm_vblank_restore =_ "todo: maybe avoid 
via dyndbg\n"
drivers/gpu/drm/drm_vblank.c:1433 [drm]drm_vblank_enable =_ "todo: maybe avoid 
via dyndbg\n"
drivers/gpu/drm/drm_plane.c:2168 [drm]drm_mode_setplane =_ "todo: maybe avoid 
via dyndbg\n"
drivers/gpu/drm/display/drm_dp_mst_topology.c:1359 
[drm_display_helper]drm_dp_mst_wait_tx_reply =_ "todo: maybe avoid via dyndbg\n"
drivers/gpu/drm/display/drm_dp_mst_topology.c:2864 
[drm_display_helper]process_single_tx_qlock =_ "todo: maybe avoid via dyndbg\n"
drivers/gpu/drm/display/drm_dp_mst_topology.c:2909 
[drm_display_helper]drm_dp_queue_down_tx =_ "todo: maybe avoid via dyndbg\n"
drivers/gpu/drm/display/drm_dp_mst_topology.c:1686 
[drm_display_helper]drm_dp_mst_update_slots =_ "todo: maybe avoid via dyndbg\n"
drivers/gpu/drm/i915/display/intel_dp.c: [i915]intel_dp_print_rates =_ 
"todo: maybe avoid via dyndbg\n"
drivers/gpu/drm/i915/display/intel_backlight.c:5434 [i915]cnp_enable_backlight 
=_ "todo: maybe avoid via dyndbg\n"
drivers/gpu/drm/i915/display/intel_backlight.c:5459 
[i915]intel_backlight_device_register =_ "todo: maybe avoid via dyndbg\n"
drivers/gpu/drm/i915/display/intel_opregion.c:43 
[i915]intel_opregion_notify_encoder =_ "todo: maybe avoid via dyndbg\n"
drivers/gpu/drm/i915/display/intel_opregion.c:53 [i915]asle_set_backlight =_ 
"todo: maybe avoid via dyndbg\n"
drivers/gpu/drm/i915/display/intel_bios.c:1088 [i915]intel_bios_is_dsi_present 
=_ "todo: maybe avoid via dyndbg\n"
drivers/gpu/drm/i915/display/intel_display_debugfs.c:6153 
[i915]i915_drrs_ctl_set =_ "todo: maybe avoid via dyndbg\n"
drivers/gpu/drm/i915/intel_pcode.c:26 [i915]snb_pcode_read =_ "todo: maybe 
avoid via dyndbg\n"
drivers/gpu/drm/i915/i915_getparam.c:785 [i915]i915_getparam_ioctl =_ "todo: 
maybe avoid via dyndbg\n"
drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c:282 [amdgpu]vcn_v2_5_process_interrupt =_ 
"todo: maybe avoid via dyndbg\n"
drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c:433 [amdgpu]vcn_v2_0_process_interrupt =_ 
"todo: maybe avoid via dyndbg\n"
:#>

Signed-off-by: Jim Cromie 
---
- simplify drm-debug-enabled choices, @DanVet
---
 drivers/gpu/drm/drm_print.c |  4 ++--
 include/drm/drm_print.h | 21 -
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
index 29a29949ad0b..cb203d63b286 100644
--- a/drivers/gpu/drm/drm_print.c
+++ b/drivers/gpu/drm/drm_print.c
@@ -285,7 +285,7 @@ void __drm_dev_dbg(const struct device *dev, enum 
drm_debug_category category,
struct va_format vaf;
va_list args;
 
-   if (!drm_debug_enabled(category))
+   if (!__drm_debug_enabled(category))
return;
 
va_start(args, format);
@@ -308,7 +308,7 @@ void ___drm_dbg(enum drm_debug_category category, const 
char *format, ...)
struct va_format vaf;
va_list args;
 
-   if (!drm_debug_enabled(category))
+   if (!__drm_debug_enabled(category))
return;
 
va_start(args, format);
diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index dfdd81c3287c..9af57d3df259 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -321,11 +321,30 @@ enum drm_debug_category {

[PATCH v7 3/9] drm_print: interpose drm_*dbg with forwarding macros

[Jim Cromie]

change drm_dev_dbg & drm_dbg to macros, which forward to the renamed
functions (with __ prefix added).

Those functions sit below the categorized layer of macros implementing
the DRM debug.category API, and implement most of it.  These are good
places to insert dynamic-debug jump-label mechanics, which will allow
DRM to avoid the runtime cost of drm_debug_enabled().

no functional changes.

memory cost baseline: (unchanged)
bash-5.1# drms_load
[9.220389] dyndbg:   1 debug prints in module drm
[9.224426] ACPI: bus type drm_connector registered
[9.302192] dyndbg:   2 debug prints in module ttm
[9.305033] dyndbg:   8 debug prints in module video
[9.627563] dyndbg: 127 debug prints in module i915
[9.721505] AMD-Vi: AMD IOMMUv2 functionality not available on this system - 
This is not a bug.
[   10.091345] dyndbg: 2196 debug prints in module amdgpu
[   10.106589] [drm] amdgpu kernel modesetting enabled.
[   10.107270] amdgpu: CRAT table not found
[   10.107926] amdgpu: Virtual CRAT table created for CPU
[   10.108398] amdgpu: Topology: Add CPU node
[   10.168507] dyndbg:   3 debug prints in module wmi
[   10.329587] dyndbg:   3 debug prints in module nouveau

Signed-off-by: Jim Cromie 
---
 drivers/gpu/drm/drm_print.c | 10 +-
 include/drm/drm_print.h |  9 +++--
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
index ec32df35a3e3..29a29949ad0b 100644
--- a/drivers/gpu/drm/drm_print.c
+++ b/drivers/gpu/drm/drm_print.c
@@ -279,8 +279,8 @@ void drm_dev_printk(const struct device *dev, const char 
*level,
 }
 EXPORT_SYMBOL(drm_dev_printk);
 
-void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
-const char *format, ...)
+void __drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
+  const char *format, ...)
 {
struct va_format vaf;
va_list args;
@@ -301,9 +301,9 @@ void drm_dev_dbg(const struct device *dev, enum 
drm_debug_category category,
 
va_end(args);
 }
-EXPORT_SYMBOL(drm_dev_dbg);
+EXPORT_SYMBOL(__drm_dev_dbg);
 
-void __drm_dbg(enum drm_debug_category category, const char *format, ...)
+void ___drm_dbg(enum drm_debug_category category, const char *format, ...)
 {
struct va_format vaf;
va_list args;
@@ -320,7 +320,7 @@ void __drm_dbg(enum drm_debug_category category, const char 
*format, ...)
 
va_end(args);
 }
-EXPORT_SYMBOL(__drm_dbg);
+EXPORT_SYMBOL(___drm_dbg);
 
 void __drm_err(const char *format, ...)
 {
diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index 668273e36c2c..c429c258c957 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -335,7 +335,7 @@ __printf(3, 4)
 void drm_dev_printk(const struct device *dev, const char *level,
const char *format, ...);
 __printf(3, 4)
-void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
+void __drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
 const char *format, ...);
 
 /**
@@ -384,6 +384,9 @@ void drm_dev_dbg(const struct device *dev, enum 
drm_debug_category category,
}   \
 })
 
+#define drm_dev_dbg(dev, cat, fmt, ...)\
+   __drm_dev_dbg(dev, cat, fmt, ##__VA_ARGS__)
+
 /**
  * DRM_DEV_DEBUG() - Debug output for generic drm code
  *
@@ -485,10 +488,12 @@ void drm_dev_dbg(const struct device *dev, enum 
drm_debug_category category,
  */
 
 __printf(2, 3)
-void __drm_dbg(enum drm_debug_category category, const char *format, ...);
+void ___drm_dbg(enum drm_debug_category category, const char *format, ...);
 __printf(1, 2)
 void __drm_err(const char *format, ...);
 
+#define __drm_dbg(fmt, ...)___drm_dbg(fmt, ##__VA_ARGS__)
+
 /* Macros to make printk easier */
 
 #define _DRM_PRINTK(once, level, fmt, ...) \
-- 
2.37.3

[PATCH v7 2/9] drm: POC drm on dyndbg - use in core, 2 helpers, 3 drivers.

[Jim Cromie]

Use DECLARE_DYNDBG_CLASSMAP across DRM:

 - in .c files, since macro defines/initializes a record

 - in drivers, $mod_{drv,drm,param}.c
   ie where param setup is done, since a classmap is param related

 - in drm/drm_print.c
   since existing __drm_debug param is defined there,
   and we ifdef it, and provide an elaborated alternative.

 - in drm_*_helper modules:
   dp/drm_dp - 1st item in makefile target
   drivers/gpu/drm/drm_crtc_helper.c - random pick iirc.

Since these modules all use identical CLASSMAP declarations (ie: names
and .class_id's) they will all respond together to "class DRM_UT_*"
query-commands:

  :#> echo class DRM_UT_KMS +p > /proc/dynamic_debug/control

NOTES:

This changes __drm_debug from int to ulong, so BIT() is usable on it.

DRM's enum drm_debug_category values need to sync with the index of
their respective class-names here.  Then .class_id == category, and
dyndbg's class FOO mechanisms will enable drm_dbg(DRM_UT_KMS, ...).

Though DRM needs consistent categories across all modules, thats not
generally needed; modules X and Y could define FOO differently (ie a
different NAME => class_id mapping), changes are made according to
each module's private class-map.

No callsites are actually selected by this patch, since none are
class'd yet.

Signed-off-by: Jim Cromie 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 14 +
 drivers/gpu/drm/display/drm_dp_helper.c | 13 
 drivers/gpu/drm/drm_crtc_helper.c   | 13 
 drivers/gpu/drm/drm_print.c | 27 +++--
 drivers/gpu/drm/i915/i915_params.c  | 12 +++
 drivers/gpu/drm/nouveau/nouveau_drm.c   | 13 
 include/drm/drm_print.h |  3 ++-
 7 files changed, 92 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 429fcdf28836..5f091cb52de2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -38,6 +38,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 
 #include "amdgpu.h"
 #include "amdgpu_irq.h"
@@ -185,6 +187,18 @@ int amdgpu_vcnfw_log;
 
 static void amdgpu_drv_delayed_reset_work_handler(struct work_struct *work);
 
+DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0,
+   "DRM_UT_CORE",
+   "DRM_UT_DRIVER",
+   "DRM_UT_KMS",
+   "DRM_UT_PRIME",
+   "DRM_UT_ATOMIC",
+   "DRM_UT_VBL",
+   "DRM_UT_STATE",
+   "DRM_UT_LEASE",
+   "DRM_UT_DP",
+   "DRM_UT_DRMRES");
+
 struct amdgpu_mgpu_info mgpu_info = {
.mutex = __MUTEX_INITIALIZER(mgpu_info.mutex),
.delayed_reset_work = __DELAYED_WORK_INITIALIZER(
diff --git a/drivers/gpu/drm/display/drm_dp_helper.c 
b/drivers/gpu/drm/display/drm_dp_helper.c
index e5bab236b3ae..196dfb1e8d87 100644
--- a/drivers/gpu/drm/display/drm_dp_helper.c
+++ b/drivers/gpu/drm/display/drm_dp_helper.c
@@ -30,6 +30,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -40,6 +41,18 @@
 
 #include "drm_dp_helper_internal.h"
 
+DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0,
+   "DRM_UT_CORE",
+   "DRM_UT_DRIVER",
+   "DRM_UT_KMS",
+   "DRM_UT_PRIME",
+   "DRM_UT_ATOMIC",
+   "DRM_UT_VBL",
+   "DRM_UT_STATE",
+   "DRM_UT_LEASE",
+   "DRM_UT_DP",
+   "DRM_UT_DRMRES");
+
 struct dp_aux_backlight {
struct backlight_device *base;
struct drm_dp_aux *aux;
diff --git a/drivers/gpu/drm/drm_crtc_helper.c 
b/drivers/gpu/drm/drm_crtc_helper.c
index 8a6d54515f92..a8cee6694cf6 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -32,6 +32,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -51,6 +52,18 @@
 
 #include "drm_crtc_helper_internal.h"
 
+DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0,
+   "DRM_UT_CORE",
+   "DRM_UT_DRIVER",
+   "DRM_UT_KMS",
+   "DRM_UT_PRIME",
+   "DRM_UT_ATOMIC",
+   "DRM_UT_VBL",
+   "DRM_UT_STATE",
+   "DRM_UT_LEASE",
+   "DRM_UT_DP",
+   "DRM_UT_DRMRES");
+
 /**
  * DOC: overview
  *
diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
index f783d4963d4b..ec32df35a3e3 100644
--- a/drivers/gpu/drm/drm_print.c
+++ b/drivers/gpu/drm/drm_print.c
@@ -40,7 +40,7 @@
  * __drm_debug: Enable debug output.
  * Bitmask of DRM_UT_x. See include/drm/drm_print.h for details.
  */

[PATCH v7 1/9] drm_print: condense enum drm_debug_category

[Jim Cromie]

enum drm_debug_category has 10 categories, but is initialized with
bitmasks which require 10 bits of underlying storage.  By using
natural enumeration, and moving the BIT(cat) into drm_debug_enabled(),
the enum fits in 4 bits, allowing the category to be represented
directly in pr_debug callsites, via the ddebug.class_id field.

While this slightly pessimizes the bit-test in drm_debug_enabled(),
using dyndbg with JUMP_LABEL will avoid the function entirely.

NOTE: this change forecloses the possibility of doing:

  drm_dbg(DRM_UT_CORE|DRM_UT_KMS, "weird 2-cat experiment")

but thats already strongly implied by the use of the enum itself; its
not a normal enum if it can be 2 values simultaneously.

Signed-off-by: Jim Cromie 
---
 include/drm/drm_print.h | 22 +++---
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index 22fabdeed297..b3b470440e46 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -279,49 +279,49 @@ enum drm_debug_category {
 * @DRM_UT_CORE: Used in the generic drm code: drm_ioctl.c, drm_mm.c,
 * drm_memory.c, ...
 */
-   DRM_UT_CORE = 0x01,
+   DRM_UT_CORE,
/**
 * @DRM_UT_DRIVER: Used in the vendor specific part of the driver: i915,
 * radeon, ... macro.
 */
-   DRM_UT_DRIVER   = 0x02,
+   DRM_UT_DRIVER,
/**
 * @DRM_UT_KMS: Used in the modesetting code.
 */
-   DRM_UT_KMS  = 0x04,
+   DRM_UT_KMS,
/**
 * @DRM_UT_PRIME: Used in the prime code.
 */
-   DRM_UT_PRIME= 0x08,
+   DRM_UT_PRIME,
/**
 * @DRM_UT_ATOMIC: Used in the atomic code.
 */
-   DRM_UT_ATOMIC   = 0x10,
+   DRM_UT_ATOMIC,
/**
 * @DRM_UT_VBL: Used for verbose debug message in the vblank code.
 */
-   DRM_UT_VBL  = 0x20,
+   DRM_UT_VBL,
/**
 * @DRM_UT_STATE: Used for verbose atomic state debugging.
 */
-   DRM_UT_STATE= 0x40,
+   DRM_UT_STATE,
/**
 * @DRM_UT_LEASE: Used in the lease code.
 */
-   DRM_UT_LEASE= 0x80,
+   DRM_UT_LEASE,
/**
 * @DRM_UT_DP: Used in the DP code.
 */
-   DRM_UT_DP   = 0x100,
+   DRM_UT_DP,
/**
 * @DRM_UT_DRMRES: Used in the drm managed resources code.
 */
-   DRM_UT_DRMRES   = 0x200,
+   DRM_UT_DRMRES
 };
 
 static inline bool drm_debug_enabled(enum drm_debug_category category)
 {
-   return unlikely(__drm_debug & category);
+   return unlikely(__drm_debug & BIT(category));
 }
 
 /*
-- 
2.37.3

[PATCH v7 0/9] dyndbg: drm.debug adaptation

[Jim Cromie]

hi Greg, Dan, Jason, DRM-folk,

heres follow-up to V6:
  rebased on driver-core/driver-core-next for -v6 applied bits (thanks)
  rework drm_debug_enabled{_raw,_instrumented,} per Dan.

It excludes:
  nouveau parts (immature)
  tracefs parts (I missed --to=Steve on v6)
  split _ddebug_site and de-duplicate experiment (way unready)

IOW, its the remaining commits of V6 on which Dan gave his Reviewed-by.

If these are good to apply, I'll rebase and repost the rest separately.

These are also available at:
https://github.com/jimc/linux/releases/tag/dyndbg%2Fdd-drm-class-911

RFC:

DECLARE_DYNDBG_CLASSMAP's interface can be improved: class-names are
currently strings, like "DRM_UT_CORE", which must have corresponding
ENUM symbols defined.  It would be better to just pass DRM_UT_CORE,
etc, and stringify the va-args inside the macro while initializing
classnames member.  But how ?


Jim Cromie (9):
  drm_print: condense enum drm_debug_category
  drm: POC drm on dyndbg - use in core, 2 helpers, 3 drivers.
  drm_print: interpose drm_*dbg with forwarding macros
  drm_print: wrap drm_*_dbg in dyndbg descriptor factory macro
  drm-print.h: include dyndbg header
  drm-print: add drm_dbg_driver to improve namespace symmetry
  drm_print: optimize drm_debug_enabled for jump-label
  drm_print: prefer bare printk KERN_DEBUG on generic fn
  drm_print: add _ddebug descriptor to drm_*dbg prototypes

 drivers/gpu/drm/Kconfig | 12 
 drivers/gpu/drm/Makefile|  2 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 14 +
 drivers/gpu/drm/display/drm_dp_helper.c | 13 +
 drivers/gpu/drm/drm_crtc_helper.c   | 13 +
 drivers/gpu/drm/drm_print.c | 48 +++
 drivers/gpu/drm/i915/i915_params.c  | 12 
 drivers/gpu/drm/nouveau/nouveau_drm.c   | 13 +
 include/drm/drm_print.h | 78 +++--
 9 files changed, 174 insertions(+), 31 deletions(-)

-- 
2.37.3

[PATCH linux-next] drm/amd/display/amdgpu_dm: remove duplicate included header files

[cgel . zte]

From: Xu Panda 

soc15_common.h is included more than once.

Reported-by: Zeal Robot 
Signed-off-by: Xu Panda 
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 7a93162633ae..0e42bf496a73 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -98,8 +98,6 @@
 #include "soc15_common.h"
 #include "vega10_ip_offset.h"

-#include "soc15_common.h"
-
 #include "gc/gc_11_0_0_offset.h"
 #include "gc/gc_11_0_0_sh_mask.h"

-- 
2.15.2

linux-next: manual merge of the drm-intel tree with the drm tree

[Stephen Rothwell]

Hi all,

Today's linux-next merge of the drm-intel tree got a conflict in:

  drivers/gpu/drm/i915/intel_pm.c

between commit:

  254e5e8829a9 ("drm: Remove unnecessary include statements of 
drm_plane_helper.h")

from the drm tree and commit:

  42a0d256496f ("drm/i915: Extract skl_watermark.c")

from the drm-intel tree.

I fixed it up (they both removed the same include) and can carry the fix
as necessary. This is now fixed as far as linux-next is concerned, but
any non trivial conflicts should be mentioned to your upstream maintainer
when your tree is submitted for merging.  You may also want to consider
cooperating with the maintainer of the conflicting tree to minimise any
particularly complex conflicts.

-- 
Cheers,
Stephen Rothwell


pgpH0axBMOn2c.pgp
Description: OpenPGP digital signature

Re: [PATCH v3 35/37] drm/i915: add descriptions for some RPM macros at intel_gt_pm.h

[Andi Shyti]

Hi Mauro,

[...]

> +/**
> + * intel_gt_pm_is_awake: Query whether the runtime PM is awake held
> + *
> + * @gt: pointer to the graphics engine

...

> +/**
> + * intel_gt_pm_get: grab a runtime PM reference ensuring that GT is powered 
> up
> + * @gt: pointer to the graphics engine

...

> +/**
> + * __intel_gt_pm_get: Acquire the runtime PM reference again
> + * @gt: pointer to the graphics engine which contains the wakeref

...

> +/**
> + * intel_gt_pm_get_if_awake: Acquire the runtime PM reference if active
> + * @gt: pointer to the graphics engine which contains the PM reference

...

> +/**
> + * intel_gt_pm_put: Release the runtime PM reference
> + * @gt: pointer to the graphics engine which contains the PM reference

...

> +/**
> + * with_intel_gt_pm - get a GT reference ensuring that GT is powered up,
> + *   run some code and then put the reference away.
> + *
> + * @gt: pointer to the gt

as you can see sometimes you put that extra blank line and
sometimes not... can we please stick to one style?

Thanks,
Andi

Re: [PATCH v3 33/37] drm/i915 i915_gem_object_types.h: document struct i915_lut_handle

[Andi Shyti]

Hi Mauro,

On Fri, Sep 09, 2022 at 09:34:40AM +0200, Mauro Carvalho Chehab wrote:
> commit d1b48c1e7184 ("drm/i915: Replace execbuf vma ht with an idr")
> added a rbtree list to allow searching for obj/ctx.
> 
> Document it.
> 
> Reviewed-by: Rodrigo Vivi 
> Signed-off-by: Mauro Carvalho Chehab 
> ---
> 
> To avoid mailbombing on a large number of people, only mailing lists were C/C 
> on the cover.
> See [PATCH v3 00/37] at: 
> https://lore.kernel.org/all/cover.1662708705.git.mche...@kernel.org/
> 
>  drivers/gpu/drm/i915/gem/i915_gem_object_types.h | 12 +---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h 
> b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> index 9f6b14ec189a..35746cf268ea 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> @@ -21,9 +21,15 @@ struct drm_i915_gem_object;
>  struct intel_fronbuffer;
>  struct intel_memory_region;
>  
> -/*
> - * struct i915_lut_handle tracks the fast lookups from handle to vma used
> - * for execbuf. Although we use a radixtree for that mapping, in order to
> +/**
> + * struct i915_lut_handle - tracks the fast lookups from handle to vma used
> + * for execbuf.
> + *

just to be picky: do we or don't we want this extra space here? I
think that besides our personal taste it's important to have a
coherent style.

I would r-b it anyway if I didn't look the next patch.

Andi

> + * @obj_link: link to the object associated with the @handle.
> + * @ctx: context associated with the @handle.
> + * @handle: a rbtree handle to lookup context for specific obj/vma.
> + *
> + * Although we use a radixtree for that mapping, in order to
>   * remove them as the object or context is closed, we need a secondary list
>   * and a translation entry (i915_lut_handle).
>   */
> -- 
> 2.37.3

Re: [PATCH v3 19/37] drm/i915: stop using kernel-doc markups for something else

[Andi Shyti]

Hi Mauro,

On Fri, Sep 09, 2022 at 09:34:26AM +0200, Mauro Carvalho Chehab wrote:
> There are some occurrences of "/**" that aren't actually part of
> a kernel-doc markup. Replace them by "/*", in order to make easier
> to identify what i915 files contain kernel-doc markups.
> 
> Reviewed-by: Rodrigo Vivi 
> Signed-off-by: Mauro Carvalho Chehab 

nice cleanup!

Reviewed-by: Andi Shyti 

Thanks,
Andi

> ---
> 
> To avoid mailbombing on a large number of people, only mailing lists were C/C 
> on the cover.
> See [PATCH v3 00/37] at: 
> https://lore.kernel.org/all/cover.1662708705.git.mche...@kernel.org/
> 
>  drivers/gpu/drm/i915/display/dvo_ch7017.c | 26 +++
>  drivers/gpu/drm/i915/display/dvo_ch7xxx.c |  6 +-
>  .../drm/i915/display/intel_display_types.h|  2 +-
>  drivers/gpu/drm/i915/display/intel_dvo_dev.h  |  6 +-
>  drivers/gpu/drm/i915/display/intel_sdvo.c |  4 +-
>  drivers/gpu/drm/i915/display/intel_tv.c   |  2 +-
>  drivers/gpu/drm/i915/gt/intel_context_types.h | 69 +--
>  drivers/gpu/drm/i915/gt/intel_ggtt_fencing.h  |  2 +-
>  drivers/gpu/drm/i915/gt/intel_gt_types.h  | 12 ++--
>  drivers/gpu/drm/i915/gt/intel_reset_types.h   |  4 +-
>  .../gpu/drm/i915/gt/intel_timeline_types.h|  6 +-
>  .../drm/i915/gt/shaders/clear_kernel/hsw.asm  |  4 +-
>  .../drm/i915/gt/shaders/clear_kernel/ivb.asm  |  4 +-
>  drivers/gpu/drm/i915/gt/uc/guc_capture_fwif.h | 10 +--
>  drivers/gpu/drm/i915/i915_drm_client.h|  2 +-
>  drivers/gpu/drm/i915/i915_drv.h   | 24 +++
>  drivers/gpu/drm/i915/i915_file_private.h  |  8 +--
>  drivers/gpu/drm/i915/i915_gpu_error.h |  4 +-
>  drivers/gpu/drm/i915/i915_pmu.h   | 32 -
>  drivers/gpu/drm/i915/intel_uncore.h   |  4 +-
>  20 files changed, 115 insertions(+), 116 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/dvo_ch7017.c 
> b/drivers/gpu/drm/i915/display/dvo_ch7017.c
> index 0589994dde11..581e29ab77e4 100644
> --- a/drivers/gpu/drm/i915/display/dvo_ch7017.c
> +++ b/drivers/gpu/drm/i915/display/dvo_ch7017.c
> @@ -55,13 +55,13 @@
>  #define CH7017_TEST_PATTERN  0x48
>  
>  #define CH7017_POWER_MANAGEMENT  0x49
> -/** Enables the TV output path. */
> +/* Enables the TV output path. */
>  #define CH7017_TV_EN (1 << 0)
>  #define CH7017_DAC0_POWER_DOWN   (1 << 1)
>  #define CH7017_DAC1_POWER_DOWN   (1 << 2)
>  #define CH7017_DAC2_POWER_DOWN   (1 << 3)
>  #define CH7017_DAC3_POWER_DOWN   (1 << 4)
> -/** Powers down the TV out block, and DAC0-3 */
> +/* Powers down the TV out block, and DAC0-3 */
>  #define CH7017_TV_POWER_DOWN_EN  (1 << 5)
>  
>  #define CH7017_VERSION_ID0x4a
> @@ -84,26 +84,26 @@
>  #define CH7017_UP_SCALER_HORIZONTAL_INC_10x5e
>  
>  #define CH7017_HORIZONTAL_ACTIVE_PIXEL_INPUT 0x5f
> -/**< Low bits of horizontal active pixel input */
> +/* Low bits of horizontal active pixel input */
>  
>  #define CH7017_ACTIVE_INPUT_LINE_OUTPUT  0x60
> -/** High bits of horizontal active pixel input */
> +/* High bits of horizontal active pixel input */
>  #define CH7017_LVDS_HAP_INPUT_MASK   (0x7 << 0)
> -/** High bits of vertical active line output */
> +/* High bits of vertical active line output */
>  #define CH7017_LVDS_VAL_HIGH_MASK(0x7 << 3)
>  
>  #define CH7017_VERTICAL_ACTIVE_LINE_OUTPUT   0x61
> -/**< Low bits of vertical active line output */
> +/* Low bits of vertical active line output */
>  
>  #define CH7017_HORIZONTAL_ACTIVE_PIXEL_OUTPUT0x62
> -/**< Low bits of horizontal active pixel output */
> +/* Low bits of horizontal active pixel output */
>  
>  #define CH7017_LVDS_POWER_DOWN   0x63
> -/** High bits of horizontal active pixel output */
> +/* High bits of horizontal active pixel output */
>  #define CH7017_LVDS_HAP_HIGH_MASK(0x7 << 0)
> -/** Enables the LVDS power down state transition */
> +/* Enables the LVDS power down state transition */
>  #define CH7017_LVDS_POWER_DOWN_EN(1 << 6)
> -/** Enables the LVDS upscaler */
> +/* Enables the LVDS upscaler */
>  #define CH7017_LVDS_UPSCALER_EN  (1 << 7)
>  #define CH7017_LVDS_POWER_DOWN_DEFAULT_RESERVED 0x08
>  
> @@ -116,9 +116,9 @@
>  #define CH7017_LVDS_ENCODING_2   0x65
>  
>  #define CH7017_LVDS_PLL_CONTROL  0x66
> -/** Enables the LVDS panel output path */
> +/* Enables the LVDS panel output path */
>  #define CH7017_LVDS_PANEN(1 << 0)
> -/** Enables the LVDS panel backlight */
> +/* Enables the LVDS panel backlight */
>  #define CH7017_LVDS_BKLEN(1 << 3)
>  
>  #define CH7017_POWER_SEQUENCING_T1   0x67
> @@ -197,7 +197,7 @@ static bool ch7017_write(struct intel_dvo_device *dvo, u8 
> addr, u8 val)
>   return i2c_transfer(dvo->i2c_bus, , 1) == 1;
>  }
>  
> -/** Probes for a CH7017 on the given bus and slave address. */
> +/* Probes for a CH7017 on the given bus and 

Re: [PATCH v3 17/37] drm/i915: i915_gem_wait.c: fix a kernel-doc markup

[Andi Shyti]

Hi Mauro,

On Fri, Sep 09, 2022 at 09:34:24AM +0200, Mauro Carvalho Chehab wrote:
> The return codes for i915_gem_wait_ioctl() have identation issues,
> and will be displayed on a very confusing way. Use lists to improve
> its output.
> 
> Reviewed-by: Rodrigo Vivi 
> Signed-off-by: Mauro Carvalho Chehab 

Reviewed-by: Andi Shyti 

Andi

> ---
> 
> To avoid mailbombing on a large number of people, only mailing lists were C/C 
> on the cover.
> See [PATCH v3 00/37] at: 
> https://lore.kernel.org/all/cover.1662708705.git.mche...@kernel.org/
> 
>  drivers/gpu/drm/i915/gem/i915_gem_wait.c | 24 +---
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c 
> b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
> index 4a33ad2d122b..1fd5cff552ed 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
> @@ -210,23 +210,25 @@ static unsigned long to_wait_timeout(s64 timeout_ns)
>   * @data: ioctl data blob
>   * @file: drm file pointer
>   *
> - * Returns 0 if successful, else an error is returned with the remaining 
> time in
> - * the timeout parameter.
> - *  -ETIME: object is still busy after timeout
> - *  -ERESTARTSYS: signal interrupted the wait
> - *  -ENONENT: object doesn't exist
> - * Also possible, but rare:
> - *  -EAGAIN: incomplete, restart syscall
> - *  -ENOMEM: damn
> - *  -ENODEV: Internal IRQ fail
> - *  -E?: The add request failed
> - *
>   * The wait ioctl with a timeout of 0 reimplements the busy ioctl. With any
>   * non-zero timeout parameter the wait ioctl will wait for the given number 
> of
>   * nanoseconds on an object becoming unbusy. Since the wait itself does so
>   * without holding struct_mutex the object may become re-busied before this
>   * function completes. A similar but shorter * race condition exists in the 
> busy
>   * ioctl
> + *
> + * Returns:
> + * 0 if successful, else an error is returned with the remaining time in
> + * the timeout parameter.
> + * * -ETIME: object is still busy after timeout
> + * * -ERESTARTSYS: signal interrupted the wait
> + * * -ENONENT: object doesn't exist
> + *
> + * Also possible, but rare:
> + * * -EAGAIN: incomplete, restart syscall
> + * * -ENOMEM: damn
> + * * -ENODEV: Internal IRQ fail
> + * * -E?: The add request failed
>   */
>  int
>  i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file 
> *file)
> -- 
> 2.37.3

linux-next: Fixes tag needs some work in the drm tree

[Stephen Rothwell]

Hi all,

In commit

  89b03aeaef16 ("drm/vkms: fix 32bit compilation error by replacing macros")

Fixes tag

  Fixes: a19c2ac9858 ("drm: vkms: Add support to the RGB565 format")

has these problem(s):

  - Target SHA1 does not exist

Maybe you meant

Fixes: 396369d67549 ("drm: vkms: Add support to the RGB565 format")

-- 
Cheers,
Stephen Rothwell


pgpwfyAVi2oDt.pgp
Description: OpenPGP digital signature

Re: [v4] drm/msm/disp/dpu1: add support for dspp sub block flush in sc7280

[Stephen Boyd]

Quoting Kalyan Thota (2022-09-08 00:26:28)
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c 
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> index a35ecb6..bbda09a 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> @@ -307,6 +309,31 @@ static void dpu_hw_ctl_update_pending_flush_dspp(struct 
> dpu_hw_ctl *ctx,
> }
>  }
>
> +static void dpu_hw_ctl_update_pending_flush_dspp_subblocks(
> +   struct dpu_hw_ctl *ctx, enum dpu_dspp dspp, u32 dspp_sub_blk)
> +{
> +   uint32_t flushbits = 0, active;

Please use u32 in the kernel. It's shorter.

[PATCH v5 3/3] drm: Introduce skip_legacy_minors modparam

[Michał Winiarski]

While there is support for >64 DRM devices on kernel side, existing
userspace may still have some hardcoded assumptions and it's possible
that it will require changes to be able to use more than 64 devices.
Add a modparam to simplify testing and development of >64 devices
support on userspace side by allocating minors from the >=192 range
(without the need of having >64 physical devices connected).

Signed-off-by: Michał Winiarski 
---
 drivers/gpu/drm/drm_drv.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 3718bd6bbef6..368408997fed 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -56,6 +56,11 @@ MODULE_LICENSE("GPL and additional rights");
 
 static DEFINE_XARRAY_ALLOC(drm_minors_xa);
 
+static bool skip_legacy_minors;
+module_param_unsafe(skip_legacy_minors, bool, 0400);
+MODULE_PARM_DESC(skip_legacy_minors,
+"Don't allocate minors in 0-192 range. This can be used for 
testing userspace support for >64 drm devices (default: false)");
+
 /*
  * If the drm core fails to init for whatever reason,
  * we should prevent any drivers from registering with it.
@@ -112,7 +117,7 @@ static void drm_minor_alloc_release(struct drm_device *dev, 
void *data)
 static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
 {
struct drm_minor *minor;
-   int r;
+   int r = -EBUSY;
 
minor = drmm_kzalloc(dev, sizeof(*minor), GFP_KERNEL);
if (!minor)
@@ -127,7 +132,8 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned 
int type)
 * and 128-191 are render nodes.
 * After reaching the limit, we're allocating minors dynamically - 
first-come, first-serve.
 */
-   r = xa_alloc(_minors_xa, >index, NULL, 
DRM_LEGACY_MINOR_LIMIT(type), GFP_KERNEL);
+   if (!skip_legacy_minors)
+   r = xa_alloc(_minors_xa, >index, NULL, 
DRM_LEGACY_MINOR_LIMIT(type), GFP_KERNEL);
if (r == -EBUSY)
r = xa_alloc(_minors_xa, >index, NULL, 
DRM_MINOR_LIMIT, GFP_KERNEL);
if (r < 0)
-- 
2.37.3

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

[Michał Winiarski]

IDR is deprecated, and since XArray manages its own state with internal
locking, it simplifies the locking on DRM side.
Additionally, don't use the IRQ-safe variant, since operating on drm
minor is not done in IRQ context.

Signed-off-by: Michał Winiarski 
Suggested-by: Matthew Wilcox 
---
 drivers/gpu/drm/drm_drv.c | 51 ++-
 1 file changed, 18 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 8214a0b1ab7f..61d24cdcd0f8 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -34,6 +34,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -53,8 +54,7 @@ MODULE_AUTHOR("Gareth Hughes, Leif Delgass, José Fonseca, Jon 
Smirl");
 MODULE_DESCRIPTION("DRM shared core routines");
 MODULE_LICENSE("GPL and additional rights");
 
-static DEFINE_SPINLOCK(drm_minor_lock);
-static struct idr drm_minors_idr;
+static DEFINE_XARRAY_ALLOC(drm_minors_xa);
 
 /*
  * If the drm core fails to init for whatever reason,
@@ -98,21 +98,19 @@ static struct drm_minor **drm_minor_get_slot(struct 
drm_device *dev,
 static void drm_minor_alloc_release(struct drm_device *dev, void *data)
 {
struct drm_minor *minor = data;
-   unsigned long flags;
 
WARN_ON(dev != minor->dev);
 
put_device(minor->kdev);
 
-   spin_lock_irqsave(_minor_lock, flags);
-   idr_remove(_minors_idr, minor->index);
-   spin_unlock_irqrestore(_minor_lock, flags);
+   xa_erase(_minors_xa, minor->index);
 }
 
+#define DRM_MINOR_LIMIT(t) ({ typeof(t) _t = (t); XA_LIMIT(64 * _t, 64 * _t + 
63); })
+
 static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
 {
struct drm_minor *minor;
-   unsigned long flags;
int r;
 
minor = drmm_kzalloc(dev, sizeof(*minor), GFP_KERNEL);
@@ -122,21 +120,10 @@ static int drm_minor_alloc(struct drm_device *dev, 
unsigned int type)
minor->type = type;
minor->dev = dev;
 
-   idr_preload(GFP_KERNEL);
-   spin_lock_irqsave(_minor_lock, flags);
-   r = idr_alloc(_minors_idr,
- NULL,
- 64 * type,
- 64 * (type + 1),
- GFP_NOWAIT);
-   spin_unlock_irqrestore(_minor_lock, flags);
-   idr_preload_end();
-
+   r = xa_alloc(_minors_xa, >index, NULL, 
DRM_MINOR_LIMIT(type), GFP_KERNEL);
if (r < 0)
return r;
 
-   minor->index = r;
-
r = drmm_add_action_or_reset(dev, drm_minor_alloc_release, minor);
if (r)
return r;
@@ -152,7 +139,7 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned 
int type)
 static int drm_minor_register(struct drm_device *dev, unsigned int type)
 {
struct drm_minor *minor;
-   unsigned long flags;
+   void *entry;
int ret;
 
DRM_DEBUG("\n");
@@ -172,9 +159,12 @@ static int drm_minor_register(struct drm_device *dev, 
unsigned int type)
goto err_debugfs;
 
/* replace NULL with @minor so lookups will succeed from now on */
-   spin_lock_irqsave(_minor_lock, flags);
-   idr_replace(_minors_idr, minor, minor->index);
-   spin_unlock_irqrestore(_minor_lock, flags);
+   entry = xa_cmpxchg(_minors_xa, minor->index, NULL, , 
GFP_KERNEL);
+   if (xa_is_err(entry)) {
+   ret = xa_err(entry);
+   goto err_debugfs;
+   }
+   WARN_ON(entry);
 
DRM_DEBUG("new minor registered %d\n", minor->index);
return 0;
@@ -187,16 +177,13 @@ static int drm_minor_register(struct drm_device *dev, 
unsigned int type)
 static void drm_minor_unregister(struct drm_device *dev, unsigned int type)
 {
struct drm_minor *minor;
-   unsigned long flags;
 
minor = *drm_minor_get_slot(dev, type);
if (!minor || !device_is_registered(minor->kdev))
return;
 
/* replace @minor with NULL so lookups will fail from now on */
-   spin_lock_irqsave(_minor_lock, flags);
-   idr_replace(_minors_idr, NULL, minor->index);
-   spin_unlock_irqrestore(_minor_lock, flags);
+   xa_store(_minors_xa, minor->index, NULL, GFP_KERNEL);
 
device_del(minor->kdev);
dev_set_drvdata(minor->kdev, NULL); /* safety belt */
@@ -215,13 +202,12 @@ static void drm_minor_unregister(struct drm_device *dev, 
unsigned int type)
 struct drm_minor *drm_minor_acquire(unsigned int minor_id)
 {
struct drm_minor *minor;
-   unsigned long flags;
 
-   spin_lock_irqsave(_minor_lock, flags);
-   minor = idr_find(_minors_idr, minor_id);
+   xa_lock(_minors_xa);
+   minor = xa_load(_minors_xa, minor_id);
if (minor)
drm_dev_get(minor->dev);
-   spin_unlock_irqrestore(_minor_lock, flags);
+   xa_unlock(_minors_xa);
 
if (!minor) {
return ERR_PTR(-ENODEV);
@@ -1037,7 +1023,7 @@ static void drm_core_exit(void)

[PATCH v5 2/3] drm: Expand max DRM device number to full MINORBITS

[Michał Winiarski]

Having a limit of 64 DRM devices is not good enough for modern world
where we have multi-GPU servers, SR-IOV virtual functions and virtual
devices used for testing.
Let's utilize full minor range for DRM devices.
To avoid regressing the existing userspace, we're still maintaining the
numbering scheme where 0-63 is used for primary, 64-127 is reserved
(formerly for control) and 128-191 is used for render.
For minors >= 192, we're allocating minors dynamically on a first-come,
first-served basis.

Signed-off-by: Michał Winiarski 
---
 drivers/gpu/drm/drm_drv.c | 13 +++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 61d24cdcd0f8..3718bd6bbef6 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -106,7 +106,8 @@ static void drm_minor_alloc_release(struct drm_device *dev, 
void *data)
xa_erase(_minors_xa, minor->index);
 }
 
-#define DRM_MINOR_LIMIT(t) ({ typeof(t) _t = (t); XA_LIMIT(64 * _t, 64 * _t + 
63); })
+#define DRM_LEGACY_MINOR_LIMIT(t) ({ typeof(t) _t = (t); XA_LIMIT(64 * _t, 64 
* _t + 63); })
+#define DRM_MINOR_LIMIT XA_LIMIT(192, (1 << MINORBITS) - 1)
 
 static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
 {
@@ -120,7 +121,15 @@ static int drm_minor_alloc(struct drm_device *dev, 
unsigned int type)
minor->type = type;
minor->dev = dev;
 
-   r = xa_alloc(_minors_xa, >index, NULL, 
DRM_MINOR_LIMIT(type), GFP_KERNEL);
+   /*
+* DRM used to support 64 devices, for backwards compatibility we need 
to maintain the
+* minor allocation scheme where minors 0-63 are primary nodes, 64-127 
are control nodes,
+* and 128-191 are render nodes.
+* After reaching the limit, we're allocating minors dynamically - 
first-come, first-serve.
+*/
+   r = xa_alloc(_minors_xa, >index, NULL, 
DRM_LEGACY_MINOR_LIMIT(type), GFP_KERNEL);
+   if (r == -EBUSY)
+   r = xa_alloc(_minors_xa, >index, NULL, 
DRM_MINOR_LIMIT, GFP_KERNEL);
if (r < 0)
return r;
 
-- 
2.37.3

[PATCH v5 0/3] drm: Use full allocated minor range for DRM

[Michał Winiarski]

64 DRM device nodes is not enough for everyone.
Upgrade it to ~512K (which definitely is more than enough).

To allow testing userspace support for >64 devices, add additional DRM
modparam (skip_legacy_minors) which causes DRM to skip allocating minors
in 0-192 range.
Additionally - convert minors to use XArray instead of IDR to simplify the
locking.

v1 -> v2:
Don't touch DRM_MINOR_CONTROL and its range (Simon Ser)

v2 -> v3:
Don't use legacy scheme for >=192 minor range (Dave Airlie)
Add modparam for testing (Dave Airlie)
Add lockdep annotation for IDR (Daniel Vetter)

v3 -> v4:
Convert from IDR to XArray (Matthew Wilcox)

v4 -> v5:
Fixup IDR to XArray conversion (Matthew Wilcox)

Michał Winiarski (3):
  drm: Use XArray instead of IDR for minors
  drm: Expand max DRM device number to full MINORBITS
  drm: Introduce skip_legacy_minors modparam

 drivers/gpu/drm/drm_drv.c | 68 +++
 1 file changed, 34 insertions(+), 34 deletions(-)

-- 
2.37.3

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

[Michał Winiarski]

On Tue, Sep 06, 2022 at 10:02:24PM +0100, Matthew Wilcox wrote:
> On Tue, Sep 06, 2022 at 10:16:27PM +0200, Michał Winiarski wrote:
> > IDR is deprecated, and since XArray manages its own state with internal
> > locking, it simplifies the locking on DRM side.
> > Additionally, don't use the IRQ-safe variant, since operating on drm
> > minor is not done in IRQ context.
> > 
> > Signed-off-by: Michał Winiarski 
> > Suggested-by: Matthew Wilcox 
> 
> I have a few questions, but I like where you're going.
> 
> > @@ -98,21 +98,18 @@ static struct drm_minor **drm_minor_get_slot(struct 
> > drm_device *dev,
> >  static void drm_minor_alloc_release(struct drm_device *dev, void *data)
> >  {
> > struct drm_minor *minor = data;
> > -   unsigned long flags;
> >  
> > WARN_ON(dev != minor->dev);
> >  
> > put_device(minor->kdev);
> >  
> > -   spin_lock_irqsave(_minor_lock, flags);
> > -   idr_remove(_minors_idr, minor->index);
> > -   spin_unlock_irqrestore(_minor_lock, flags);
> > +   xa_release(_minors_xa, minor->index);
> 
> Has it definitely been unused at this point?  I would think that
> xa_erase() (an unconditional store) would be the correct function to
> call.

Yes, unless there's a programmers error somewhere - I'll replace it though.

> 
> > @@ -122,20 +119,12 @@ static int drm_minor_alloc(struct drm_device *dev, 
> > unsigned int type)
> > minor->type = type;
> > minor->dev = dev;
> >  
> > -   idr_preload(GFP_KERNEL);
> > -   spin_lock_irqsave(_minor_lock, flags);
> > -   r = idr_alloc(_minors_idr,
> > - NULL,
> > - 64 * type,
> > - 64 * (type + 1),
> > - GFP_NOWAIT);
> > -   spin_unlock_irqrestore(_minor_lock, flags);
> > -   idr_preload_end();
> > -
> > +   r = xa_alloc(_minors_xa, , NULL,
> > +XA_LIMIT(64 * type, 64 * (type + 1) - 1), GFP_KERNEL);
> > if (r < 0)
> > return r;
> >  
> > -   minor->index = r;
> > +   minor->index = id;
> 
> Wouldn't it be better to call:
> 
>   r = xa_alloc(_minors_xa, >index, NULL,
>   XA_LIMIT(64 * type, 64 * (type + 1) - 1), GFP_KERNEL);
> 
> I might also prefer a little syntactic sugar like:
> 
> #define DRM_MINOR_LIMIT(type) XA_LIMIT(64 * (type), 64 * (type) + 63)
> 
> but that's definitely a matter of taste.

Sure.

> 
> > @@ -172,9 +161,12 @@ static int drm_minor_register(struct drm_device *dev, 
> > unsigned int type)
> > goto err_debugfs;
> >  
> > /* replace NULL with @minor so lookups will succeed from now on */
> > -   spin_lock_irqsave(_minor_lock, flags);
> > -   idr_replace(_minors_idr, minor, minor->index);
> > -   spin_unlock_irqrestore(_minor_lock, flags);
> > +   entry = xa_store(_minors_xa, minor->index, , GFP_KERNEL);
> > +   if (xa_is_err(entry)) {
> > +   ret = xa_err(entry);
> > +   goto err_debugfs;
> > +   }
> > +   WARN_ON(entry);
> 
> Might be better as an xa_cmpxchg()?

Ack.

> 
> > @@ -187,16 +179,13 @@ static int drm_minor_register(struct drm_device *dev, 
> > unsigned int type)
> >  static void drm_minor_unregister(struct drm_device *dev, unsigned int type)
> >  {
> > struct drm_minor *minor;
> > -   unsigned long flags;
> >  
> > minor = *drm_minor_get_slot(dev, type);
> > if (!minor || !device_is_registered(minor->kdev))
> > return;
> >  
> > /* replace @minor with NULL so lookups will fail from now on */
> > -   spin_lock_irqsave(_minor_lock, flags);
> > -   idr_replace(_minors_idr, NULL, minor->index);
> > -   spin_unlock_irqrestore(_minor_lock, flags);
> > +   xa_erase(_minors_xa, minor->index);
> 
> This isn't an exact replacement, but I'm not sure whether that makes a
> difference.  xa_erase() allows allocation of this ID again while
> idr_replace() means that lookups return NULL, but the ID remains in
> use.  The equivalent of idr_replace() is:
>   xa_store(_minors_xa, minor->index, NULL, GFP_KERNEL);

It does makes a difference, I'll change it to the equivalent.

> 
> > @@ -215,13 +204,10 @@ static void drm_minor_unregister(struct drm_device 
> > *dev, unsigned int type)
> >  struct drm_minor *drm_minor_acquire(unsigned int minor_id)
> >  {
> > struct drm_minor *minor;
> > -   unsigned long flags;
> >  
> > -   spin_lock_irqsave(_minor_lock, flags);
> > -   minor = idr_find(_minors_idr, minor_id);
> > +   minor = xa_load(_minors_xa, minor_id);
> > if (minor)
> > drm_dev_get(minor->dev);
> > -   spin_unlock_irqrestore(_minor_lock, flags);
> 
> This is also not an exact equivalent as the dev_drm_get() is now outside
> the lock.  Does that matter in this case?  I don't know the code well
> enough to say.  If you want it to be equivalent, then:
> 
>   xa_lock(_minors_xa);
>   minor = xa_load(_minors_xa, minor_id);
>   if (minor)
>   drm_dev_get(minor->dev);
>   xa_unlock(_minors_xa);
> 
> would be the code to use.

Again, it does matter, I'll change it.

> 
> > @@ -1037,7 +1023,7 @@ 

Re: [PATCH v3 4/9] drm/mediatek: Add gamma support different lut_size for other SoC

[kernel test robot]

Hi Jason-JH.Lin",

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on next-20220909]
[also build test WARNING on v6.0-rc4]
[cannot apply to drm-misc/drm-misc-next robh/for-next drm/drm-next linus/master 
v6.0-rc4 v6.0-rc3 v6.0-rc2]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:
https://github.com/intel-lab-lkp/linux/commits/Jason-JH-Lin/Add-gamma-lut-support-for-mt8195/20220911-233949
base:9a82ccda91ed2b40619cb3c10d446ae1f97bab6e
config: arm-allyesconfig 
(https://download.01.org/0day-ci/archive/20220912/202209120412.a3tfzuo9-...@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# 
https://github.com/intel-lab-lkp/linux/commit/1d5b909c49fd62f1e6055fd3d077b9c5fae53e00
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review 
Jason-JH-Lin/Add-gamma-lut-support-for-mt8195/20220911-233949
git checkout 1d5b909c49fd62f1e6055fd3d077b9c5fae53e00
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 
O=build_dir ARCH=arm SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot 

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/mediatek/mtk_disp_gamma.c:59:14: warning: no previous 
>> prototype for 'mtk_gamma_get_size' [-Wmissing-prototypes]
  59 | unsigned int mtk_gamma_get_size(struct device *dev)
 |  ^~


vim +/mtk_gamma_get_size +59 drivers/gpu/drm/mediatek/mtk_disp_gamma.c

58  
  > 59  unsigned int mtk_gamma_get_size(struct device *dev)
60  {
61  struct mtk_disp_gamma *gamma = dev_get_drvdata(dev);
62  unsigned int lut_size = LUT_SIZE_DEFAULT;
63  
64  if (gamma && gamma->data)
65  lut_size = gamma->data->lut_size;
66  
67  return lut_size;
68  }
69  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 0/5] drm/amd/display: Reduce stack usage for clang

[Maíra Canal]

Hi Nathan,

I have built-tested the whole series with clang 14.0.5 (Fedora
14.0.5-1.fc36), using:

$ make -kj"$(nproc)" ARCH=x86_64 LLVM=1 mrproper allmodconfig
drivers/gpu/drm/amd/amdgpu/

Great to see this patchset coming for DML!

To the whole series:

Tested-by: Maíra Canal 

Best Regards,
- Maíra Canal

On 8/30/22 17:34, Nathan Chancellor wrote:
> Hi all,
> 
> This series aims to address the following warnings, which are visible
> when building x86_64 allmodconfig with clang after commit 3876a8b5e241
> ("drm/amd/display: Enable building new display engine with KCOV
> enabled").
> 
> 
> drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn30/display_mode_vba_30.c:3542:6:
>  error: stack frame size (2200) exceeds limit (2048) in 
> 'dml30_ModeSupportAndSystemConfigurationFull' [-Werror,-Wframe-larger-than]
> void dml30_ModeSupportAndSystemConfigurationFull(struct display_mode_lib 
> *mode_lib)
> ^
> 1 error generated.
> 
> 
> drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn31/display_mode_vba_31.c:3908:6:
>  error: stack frame size (2216) exceeds limit (2048) in 
> 'dml31_ModeSupportAndSystemConfigurationFull' [-Werror,-Wframe-larger-than]
> void dml31_ModeSupportAndSystemConfigurationFull(struct display_mode_lib 
> *mode_lib)
> ^
> 1 error generated.
> 
> 
> drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn32/display_mode_vba_32.c:1721:6:
>  error: stack frame size (2152) exceeds limit (2048) in 
> 'dml32_ModeSupportAndSystemConfigurationFull' [-Werror,-Wframe-larger-than]
> void dml32_ModeSupportAndSystemConfigurationFull(struct display_mode_lib 
> *mode_lib)
> ^
> 1 error generated.
> 
> This series is based on commit b3235e8635e1 ("drm/amd/display: clean up
> some inconsistent indentings"). These warnings are fatal for
> allmodconfig due to CONFIG_WERROR so ideally, I would like to see these
> patches cherry-picked to a branch targeting mainline to allow our builds
> to go back to green. However, since this series is not exactly trivial
> in size, I can understand not wanting to apply these to mainline during
> the -rc cycle. If they cannot be cherry-picked to mainline, I can add a
> patch raising the value of -Wframe-larger-than for these files that can
> be cherry-picked to 6.0/mainline then add a revert of that change as the
> last patch in the stack so everything goes back to normal for -next/6.1.
> I am open to other options though!
> 
> I have built this series against clang 16.0.0 (ToT) and GCC 12.2.0 for
> x86_64. It has seen no runtime testing, as my only test system with AMD
> graphics is a Renoir one, which as far as I understand it uses DCN 2.1.
> 
> Nathan Chancellor (5):
>   drm/amd/display: Reduce number of arguments of
> dml32_CalculateWatermarksMALLUseAndDRAMSpeedChangeSupport()
>   drm/amd/display: Reduce number of arguments of
> dml32_CalculatePrefetchSchedule()
>   drm/amd/display: Reduce number of arguments of dml31's
> CalculateWatermarksAndDRAMSpeedChangeSupport()
>   drm/amd/display: Reduce number of arguments of dml31's
> CalculateFlipSchedule()
>   drm/amd/display: Mark dml30's UseMinimumDCFCLK() as noinline for stack
> usage
> 
>  .../dc/dml/dcn30/display_mode_vba_30.c|   2 +-
>  .../dc/dml/dcn31/display_mode_vba_31.c| 420 +-
>  .../dc/dml/dcn32/display_mode_vba_32.c| 236 +++---
>  .../dc/dml/dcn32/display_mode_vba_util_32.c   | 323 ++
>  .../dc/dml/dcn32/display_mode_vba_util_32.h   |  51 +--
>  5 files changed, 318 insertions(+), 714 deletions(-)
> 
> 
> base-commit: b3235e8635e1dd7ac1a27a73330e9880dfe05154

Re: [PATCH v6 01/12] dt-bindings: display/msm: split qcom,mdss bindings

[Krzysztof Kozlowski]

On 11/09/2022 20:36, Krzysztof Kozlowski wrote:

>> If your child schema fails, the referencing schema fails as well...
> 
> 
> Although now with DSI-PHY I cannot reproduce it and I am pretty sure I
> reproduced it with DPU controllers after modifying the DTS to lack a
> property... Hmmm

https://github.com/devicetree-org/dt-schema/pull/82


Best regards,
Krzysztof

[PATCH v5 2/2] drm/tests: Change "igt_" prefix to "drm_test_"

[Maíra Canal]

With the introduction of KUnit, IGT is no longer the only option to run
the DRM unit tests, as the tests can be run through kunit-tool or on
real hardware with CONFIG_KUNIT.

Therefore, remove the "igt_" prefix from the tests and replace it with
the "drm_test_" prefix, making the tests' names independent from the tool
used.

Signed-off-by: Maíra Canal 
Acked-by: David Gow 
Acked-by: Maxime Ripard 
---
v1 -> v2: 
https://lore.kernel.org/dri-devel/20220830211603.191734-1-mairaca...@riseup.net/
- Change "drm_" prefix to "test_drm_", as "drm_" can be a bit confusing (Jani 
Nikula).

v2 -> v3: 
https://lore.kernel.org/dri-devel/20220901124210.591994-1-mairaca...@riseup.net/
- Change "drm_" prefix to "drm_test_" (Jani Nikula and Maxime Ripard).
- Add David's Acked-by tag.

v3 -> v4: 
https://lore.kernel.org/dri-devel/20220907200247.89679-1-mairaca...@riseup.net/
- Rebase on top of drm-misc-next.
- Add Maxime's Acked-by tag.

v4 -> v5: 
https://lore.kernel.org/dri-devel/202209110709.zo1yjeye-...@intel.com/T/#mac65eb9447b57f1ebbe7cc516f38c11c7a076295
- No changes.
---
 drivers/gpu/drm/tests/drm_buddy_test.c|  84 +-
 .../gpu/drm/tests/drm_cmdline_parser_test.c   | 156 +-
 .../gpu/drm/tests/drm_damage_helper_test.c|  84 +-
 .../gpu/drm/tests/drm_dp_mst_helper_test.c|   8 +-
 .../gpu/drm/tests/drm_format_helper_test.c|   8 +-
 drivers/gpu/drm/tests/drm_format_test.c   |  72 
 drivers/gpu/drm/tests/drm_mm_test.c   | 155 -
 drivers/gpu/drm/tests/drm_plane_helper_test.c |   4 +-
 drivers/gpu/drm/tests/drm_rect_test.c |  16 +-
 9 files changed, 294 insertions(+), 293 deletions(-)

diff --git a/drivers/gpu/drm/tests/drm_buddy_test.c 
b/drivers/gpu/drm/tests/drm_buddy_test.c
index d76f83833e75..7a2b2d6bc3fe 100644
--- a/drivers/gpu/drm/tests/drm_buddy_test.c
+++ b/drivers/gpu/drm/tests/drm_buddy_test.c
@@ -13,7 +13,7 @@
 
 #include "../lib/drm_random.h"
 
-#define IGT_TIMEOUT(name__)
\
+#define TIMEOUT(name__)
\
unsigned long name__ = jiffies + MAX_SCHEDULE_TIMEOUT
 
 static unsigned int random_seed;
@@ -24,7 +24,7 @@ static inline u64 get_size(int order, u64 chunk_size)
 }
 
 __printf(2, 3)
-static bool __igt_timeout(unsigned long timeout, const char *fmt, ...)
+static bool __timeout(unsigned long timeout, const char *fmt, ...)
 {
va_list va;
 
@@ -43,8 +43,8 @@ static bool __igt_timeout(unsigned long timeout, const char 
*fmt, ...)
return true;
 }
 
-static void __igt_dump_block(struct kunit *test, struct drm_buddy *mm,
-struct drm_buddy_block *block, bool buddy)
+static void __dump_block(struct kunit *test, struct drm_buddy *mm,
+struct drm_buddy_block *block, bool buddy)
 {
kunit_err(test, "block info: header=%llx, state=%u, order=%d, 
offset=%llx size=%llx root=%d buddy=%d\n",
  block->header, drm_buddy_block_state(block),
@@ -52,20 +52,20 @@ static void __igt_dump_block(struct kunit *test, struct 
drm_buddy *mm,
  drm_buddy_block_size(mm, block), !block->parent, 
buddy);
 }
 
-static void igt_dump_block(struct kunit *test, struct drm_buddy *mm,
-  struct drm_buddy_block *block)
+static void dump_block(struct kunit *test, struct drm_buddy *mm,
+  struct drm_buddy_block *block)
 {
struct drm_buddy_block *buddy;
 
-   __igt_dump_block(test, mm, block, false);
+   __dump_block(test, mm, block, false);
 
buddy = drm_get_buddy(block);
if (buddy)
-   __igt_dump_block(test, mm, buddy, true);
+   __dump_block(test, mm, buddy, true);
 }
 
-static int igt_check_block(struct kunit *test, struct drm_buddy *mm,
-  struct drm_buddy_block *block)
+static int check_block(struct kunit *test, struct drm_buddy *mm,
+  struct drm_buddy_block *block)
 {
struct drm_buddy_block *buddy;
unsigned int block_state;
@@ -137,8 +137,8 @@ static int igt_check_block(struct kunit *test, struct 
drm_buddy *mm,
return err;
 }
 
-static int igt_check_blocks(struct kunit *test, struct drm_buddy *mm,
-   struct list_head *blocks, u64 expected_size, bool 
is_contiguous)
+static int check_blocks(struct kunit *test, struct drm_buddy *mm,
+   struct list_head *blocks, u64 expected_size, bool 
is_contiguous)
 {
struct drm_buddy_block *block;
struct drm_buddy_block *prev;
@@ -150,7 +150,7 @@ static int igt_check_blocks(struct kunit *test, struct 
drm_buddy *mm,
total = 0;
 
list_for_each_entry(block, blocks, link) {
-   err = igt_check_block(test, mm, block);
+   err = check_block(test, mm, block);
 
if (!drm_buddy_block_is_allocated(block)) {
 

[PATCH v5 1/2] drm/tests: Split drm_framebuffer_create_test into parameterized tests

[Maíra Canal]

The igt_check_drm_framebuffer_create is based on a loop that executes
tests for all createbuffer_tests test cases. This could be better
represented by parameterized tests, provided by KUnit.

So, convert the igt_check_drm_framebuffer_create into parameterized tests.

Signed-off-by: Maíra Canal 
Reviewed-by: Michał Winiarski 
Reviewed-by: David Gow 
---
v1 -> v2: 
https://lore.kernel.org/dri-devel/20220830211603.191734-1-mairaca...@riseup.net/
- Use .init for mock_drm_device instead of a global variable. (Michał Winiarski)
- Add Michał's Reviewed-by tag.

v2 -> v3: 
https://lore.kernel.org/dri-devel/20220901124210.591994-1-mairaca...@riseup.net/
- Add David's Reviewed-by tag.

v3 -> v4: 
https://lore.kernel.org/dri-devel/20220907200247.89679-1-mairaca...@riseup.net/
- No changes.

v4 -> v5: 
https://lore.kernel.org/dri-devel/202209110709.zo1yjeye-...@intel.com/T/#mac65eb9447b57f1ebbe7cc516f38c11c7a076295
- Reduce stack usage on drm_framebuffer_test_init (kernel test bot).
---
 drivers/gpu/drm/tests/drm_framebuffer_test.c | 53 +++-
 1 file changed, 30 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/tests/drm_framebuffer_test.c 
b/drivers/gpu/drm/tests/drm_framebuffer_test.c
index ec7a08ba4056..df235b7fdaa5 100644
--- a/drivers/gpu/drm/tests/drm_framebuffer_test.c
+++ b/drivers/gpu/drm/tests/drm_framebuffer_test.c
@@ -25,7 +25,7 @@ struct drm_framebuffer_test {
const char *name;
 };
 
-static struct drm_framebuffer_test createbuffer_tests[] = {
+static const struct drm_framebuffer_test drm_framebuffer_create_cases[] = {
 { .buffer_created = 1, .name = "ABGR normal sizes",
.cmd = { .width = 600, .height = 600, .pixel_format = 
DRM_FORMAT_ABGR,
 .handles = { 1, 0, 0 }, .pitches = { 4 * 600, 0, 0 },
@@ -330,43 +330,50 @@ static struct drm_mode_config_funcs mock_config_funcs = {
.fb_create = fb_create_mock,
 };
 
-static struct drm_device mock_drm_device = {
-   .mode_config = {
-   .min_width = MIN_WIDTH,
-   .max_width = MAX_WIDTH,
-   .min_height = MIN_HEIGHT,
-   .max_height = MAX_HEIGHT,
-   .funcs = _config_funcs,
-   },
-};
+static int drm_framebuffer_test_init(struct kunit *test)
+{
+   struct drm_device *mock;
+
+   mock = kunit_kzalloc(test, sizeof(*mock), GFP_KERNEL);
+   KUNIT_ASSERT_NOT_ERR_OR_NULL(test, mock);
 
-static int execute_drm_mode_fb_cmd2(struct drm_mode_fb_cmd2 *r)
+   mock->mode_config.min_width = MIN_WIDTH;
+   mock->mode_config.max_width = MAX_WIDTH;
+   mock->mode_config.min_height = MIN_HEIGHT;
+   mock->mode_config.max_height = MAX_HEIGHT;
+   mock->mode_config.funcs = _config_funcs;
+
+   test->priv = mock;
+   return 0;
+}
+
+static void drm_test_framebuffer_create(struct kunit *test)
 {
+   const struct drm_framebuffer_test *params = test->param_value;
+   struct drm_device *mock = test->priv;
int buffer_created = 0;
 
-   mock_drm_device.dev_private = _created;
-   drm_internal_framebuffer_create(_drm_device, r, NULL);
-   return buffer_created;
+   mock->dev_private = _created;
+   drm_internal_framebuffer_create(mock, >cmd, NULL);
+   KUNIT_EXPECT_EQ(test, params->buffer_created, buffer_created);
 }
 
-static void igt_check_drm_framebuffer_create(struct kunit *test)
+static void drm_framebuffer_test_to_desc(const struct drm_framebuffer_test *t, 
char *desc)
 {
-   int i = 0;
-
-   for (i = 0; i < ARRAY_SIZE(createbuffer_tests); i++) {
-   KUNIT_EXPECT_EQ_MSG(test, createbuffer_tests[i].buffer_created,
-   
execute_drm_mode_fb_cmd2(_tests[i].cmd),
-"Test %d: \"%s\" failed\n", i, createbuffer_tests[i].name);
-   }
+   strcpy(desc, t->name);
 }
 
+KUNIT_ARRAY_PARAM(drm_framebuffer_create, drm_framebuffer_create_cases,
+ drm_framebuffer_test_to_desc);
+
 static struct kunit_case drm_framebuffer_tests[] = {
-   KUNIT_CASE(igt_check_drm_framebuffer_create),
+   KUNIT_CASE_PARAM(drm_test_framebuffer_create, 
drm_framebuffer_create_gen_params),
{ }
 };
 
 static struct kunit_suite drm_framebuffer_test_suite = {
.name = "drm_framebuffer",
+   .init = drm_framebuffer_test_init,
.test_cases = drm_framebuffer_tests,
 };
 
-- 
2.37.3

NULL pointer dereference in i915 in i915_gem_do_execbuffer, eb_lookup_vmas

[Maxim Mikityanskiy]

Hi all,

I experience periodic crashes on HP Elite Dragonfly G2 (i7-1165G7)
which, I believe, are related to a bug in the i915 driver. I collected
a stacktrace and a kdump on 5.19.8 compiled with KASAN. This time the
screen was frozen, but I could SSH into the machine. Sometimes a
kernel panic happens.

I will appreciate it if this can be fixed. Please find the decoded
stack trace below:

[ +13,386280] general protection fault, probably for non-canonical
address 0xdcec:  [#1] PREEMPT SMP DEBUG_PAGEALLOC
KASAN NOPTI
[  +0,24] KASAN: null-ptr-deref in range
[0x0760-0x0767]
[  +0,17] Hardware name: HP HP Elite Dragonfly G2 Notebook
PC/8716, BIOS T90 Ver. 01.01.04 01/03/2021
[   +0,06] RIP: 0010:i915_gem_do_execbuffer
(./drivers/gpu/drm/i915/gem/i915_gem_object.h:632
drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:921
drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:3391) i915
[ +0,000207] Code: b8 00 00 00 48 89 f8 48 c1 e8 03 80 3c 28 00 0f 85
5d 3f 00 00 4d 8b af b8 00 00 00 49 8d bd 60 07 00 00 48 89 f8 48 c1
e8 03 <80> 3c 28 00 0f 85 34 3f 00 00 49 83 bd 60 07 00 00 00 74 6c 4c
89
All code

   0:b8 00 00 00 48   mov$0x4800,%eax
   5:89 f8mov%edi,%eax
   7:48 c1 e8 03  shr$0x3,%rax
   b:80 3c 28 00  cmpb   $0x0,(%rax,%rbp,1)
   f:0f 85 5d 3f 00 00jne0x3f72
  15:4d 8b af b8 00 00 00 mov0xb8(%r15),%r13
  1c:49 8d bd 60 07 00 00 lea0x760(%r13),%rdi
  23:48 89 f8 mov%rdi,%rax
  26:48 c1 e8 03  shr$0x3,%rax
  2a:*80 3c 28 00  cmpb   $0x0,(%rax,%rbp,1)
<-- trapping instruction
  2e:0f 85 34 3f 00 00jne0x3f68
  34:49 83 bd 60 07 00 00 cmpq   $0x0,0x760(%r13)
  3b:00
  3c:74 6cje 0xaa
  3e:4c   rex.WR
  3f:89   .byte 0x89

Code starting with the faulting instruction
===
   0:80 3c 28 00  cmpb   $0x0,(%rax,%rbp,1)
   4:0f 85 34 3f 00 00jne0x3f3e
   a:49 83 bd 60 07 00 00 cmpq   $0x0,0x760(%r13)
  11:00
  12:74 6cje 0x80
  14:4c   rex.WR
  15:89   .byte 0x89
[  +0,08] RSP: 0018:c9000a74f7e8 EFLAGS: 00010202
[  +0,09] RAX: 00ec RBX: 0008 RCX: 
[  +0,06] RDX: 0001 RSI: c9000a74f880 RDI: 0760
[  +0,05] RBP: dc00 R08: 888135ae8000 R09: 
[  +0,05] R10: fbfff5b2f5a4 R11: c16145cb R12: 0008
[  +0,05] R13:  R14: 001c R15: 88813513d640
[  +0,05] FS:  7f1329fced00() GS:1000()
knlGS:
[  +0,07] CS:  0010 DS:  ES:  CR0: 80050033
[  +0,05] CR2: 7f5c6ff8f80c CR3: 00011493e005 CR4: 00f70ef0
[  +0,06] PKRU: 5554
[  +0,04] Call Trace:
[  +0,05]  
[   +0,10] ? parse_timeline_fences
(drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:3292) i915
[   +0,000193] ? kernel_text_address (kernel/extable.c:125 kernel/extable.c:94)
[   +0,39] ? reacquire_held_locks (kernel/locking/lockdep.c:5674)
[   +0,09] ? __schedule_bug (kernel/sched/core.c:9820)
[   +0,14] i915_gem_execbuffer2_ioctl
(drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:3543) i915
[   +0,000184] ? i915_gem_do_execbuffer
(drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:3503) i915
[   +0,000177] drm_ioctl_kernel (drivers/gpu/drm/drm_ioctl.c:782)
[   +0,09] ? drm_version (drivers/gpu/drm/drm_ioctl.c:767)
[   +0,11] drm_ioctl (drivers/gpu/drm/drm_ioctl.c:886)
[   +0,08] ? i915_gem_do_execbuffer
(drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:3503) i915
[   +0,000174] ? drm_ioctl_kernel (drivers/gpu/drm/drm_ioctl.c:807)
[   +0,06] ? lock_release (./include/trace/events/lock.h:69
kernel/locking/lockdep.c:5677)
[   +0,08] ? lock_downgrade (kernel/locking/lockdep.c:5634)
[   +0,07] ? ktime_get_coarse_real_ts64
(./include/linux/seqlock.h:274 kernel/time/timekeeping.c:2261)
[   +0,19] __x64_sys_ioctl (fs/ioctl.c:51 fs/ioctl.c:870
fs/ioctl.c:856 fs/ioctl.c:856)
[   +0,09] do_syscall_64 (arch/x86/entry/common.c:50
arch/x86/entry/common.c:80)
[   +0,08] ? do_syscall_64 (arch/x86/entry/common.c:87)
[   +0,07] entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:120)
[  +0,08] RIP: 0033:0x7f132a9579ef
[ +0,06] Code: 00 48 89 44 24 18 31 c0 48 8d 44 24 60 c7 04 24 10
00 00 00 48 89 44 24 08 48 8d 44 24 20 48 89 44 24 10 b8 10 00 00 00
0f 05 <89> c2 3d 00 f0 ff ff 77 18 48 8b 44 24 18 64 48 2b 04 25 28 00
00
All code

   0:00 48 89 add%cl,-0x77(%rax)
   3:44 24 18 rex.R and $0x18,%al
   6:31 c0   

Re: [PATCH v6 01/12] dt-bindings: display/msm: split qcom,mdss bindings

[Krzysztof Kozlowski]

On 11/09/2022 20:36, Krzysztof Kozlowski wrote:

>> /home/krzk/dev/linux/linux/out/arch/arm64/boot/dts/qcom/sda660-inforce-ifc6560.dtb:
>> dsi@c994000: 'vdda-supply' does not match any of the regexes:
>> 'pinctrl-[0-9]+'
>>
>>
>>
>> If your child schema fails, the referencing schema fails as well...
> 
> 
> Although now with DSI-PHY I cannot reproduce it and I am pretty sure I
> reproduced it with DPU controllers after modifying the DTS to lack a
> property... Hmmm
> 
I think I have a fix for this in DT schema.

Best regards,
Krzysztof

Re: [PATCH v6 01/12] dt-bindings: display/msm: split qcom,mdss bindings

[Krzysztof Kozlowski]

On 11/09/2022 20:32, Krzysztof Kozlowski wrote:
> On 11/09/2022 19:45, Dmitry Baryshkov wrote:
>> On Sun, 11 Sept 2022 at 16:57, Krzysztof Kozlowski
>>  wrote:
>>>
>>> On 11/09/2022 15:45, Dmitry Baryshkov wrote:
 On Sun, 11 Sept 2022 at 14:27, Krzysztof Kozlowski
  wrote:
>
> On 10/09/2022 14:54, Dmitry Baryshkov wrote:
>>>
>>> However I think there is no such problem, as Dmitry said, that ref
>>> changes anything. There will be always failure - either from parent
>>> schema (using $ref) or from device child schema (the one which actually
>>> misses the property).
>>
>> Initially I stumbled upon this issue with the dsi and dsi_phy nodes
>> for msm8996 devices. If I have $ref here, dsi1/dsi1_phy nodes will
>> emit warnings regarding the missing -supply properties despite nodes
>> being disabled. If I use `compatible' here, the schema checks pass.
>> Thus I'd prefer to leave `compatible' here. Not to mention that it
>> also allows specifying a tighter binding than just using the $ref.
>
> I don't think we understood each other. I claim that error will be there
> anyway, just from different schema. So your change fixes nothing in
> total schema check...

 If the node is disabled, there will be no different schema check.
>>>
>>> As I wrote before, there was.
>>
>> The following results were captured with the following command, with
>> most of the DSI and MDSS schema files fixed, using the following
>> command:
>> $ PATH=~/.local/bin/:$PATH make -C ../build-64/ ARCH=arm64
>> qcom/sda660-inforce-ifc6560.dtb  CHECK_DTBS=y
>> DT_SCHEMA_FILES=display/msm
>>
>> As you can see from the example below, when using 'compatible' I'm
>> getting warnings just for the gpu@500 node, while using $ref I
>> also got warnings for the dsi-phy@c996400 node (disabled in the DT
>> file).
>> For your reference the tree in question is uploaded to the:
>> https://git.linaro.org/people/dmitry.baryshkov/kernel.git msm-mdss-yaml
> 
> I did not say anything about msm-mdss. I said you will get errors from
> child schema anyway.
> 
>   From schema:
> /home/krzk/dev/linux/linux/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
> 
> /home/krzk/dev/linux/linux/out/arch/arm64/boot/dts/qcom/sda660-inforce-ifc6560.dtb:
> dsi@c994000: clock-names: ['mdp_core', 'byte', 'byte_intf', 'mnoc',
> 'iface', 'bus', 'core_mmss', 'pixel', 'core'] is too long
> 
>   From schema:
> /home/krzk/dev/linux/linux/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
> 
> /home/krzk/dev/linux/linux/out/arch/arm64/boot/dts/qcom/sda660-inforce-ifc6560.dtb:
> dsi@c994000: 'vdda-supply' does not match any of the regexes:
> 'pinctrl-[0-9]+'
> 
> 
> 
> If your child schema fails, the referencing schema fails as well...


Although now with DSI-PHY I cannot reproduce it and I am pretty sure I
reproduced it with DPU controllers after modifying the DTS to lack a
property... Hmmm


Best regards,
Krzysztof

Re: [PATCH v6 01/12] dt-bindings: display/msm: split qcom,mdss bindings

[Krzysztof Kozlowski]

On 11/09/2022 19:45, Dmitry Baryshkov wrote:
> On Sun, 11 Sept 2022 at 16:57, Krzysztof Kozlowski
>  wrote:
>>
>> On 11/09/2022 15:45, Dmitry Baryshkov wrote:
>>> On Sun, 11 Sept 2022 at 14:27, Krzysztof Kozlowski
>>>  wrote:

 On 10/09/2022 14:54, Dmitry Baryshkov wrote:
>>
>> However I think there is no such problem, as Dmitry said, that ref
>> changes anything. There will be always failure - either from parent
>> schema (using $ref) or from device child schema (the one which actually
>> misses the property).
>
> Initially I stumbled upon this issue with the dsi and dsi_phy nodes
> for msm8996 devices. If I have $ref here, dsi1/dsi1_phy nodes will
> emit warnings regarding the missing -supply properties despite nodes
> being disabled. If I use `compatible' here, the schema checks pass.
> Thus I'd prefer to leave `compatible' here. Not to mention that it
> also allows specifying a tighter binding than just using the $ref.

 I don't think we understood each other. I claim that error will be there
 anyway, just from different schema. So your change fixes nothing in
 total schema check...
>>>
>>> If the node is disabled, there will be no different schema check.
>>
>> As I wrote before, there was.
> 
> The following results were captured with the following command, with
> most of the DSI and MDSS schema files fixed, using the following
> command:
> $ PATH=~/.local/bin/:$PATH make -C ../build-64/ ARCH=arm64
> qcom/sda660-inforce-ifc6560.dtb  CHECK_DTBS=y
> DT_SCHEMA_FILES=display/msm
> 
> As you can see from the example below, when using 'compatible' I'm
> getting warnings just for the gpu@500 node, while using $ref I
> also got warnings for the dsi-phy@c996400 node (disabled in the DT
> file).
> For your reference the tree in question is uploaded to the:
> https://git.linaro.org/people/dmitry.baryshkov/kernel.git msm-mdss-yaml

I did not say anything about msm-mdss. I said you will get errors from
child schema anyway.

From schema:
/home/krzk/dev/linux/linux/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml

/home/krzk/dev/linux/linux/out/arch/arm64/boot/dts/qcom/sda660-inforce-ifc6560.dtb:
dsi@c994000: clock-names: ['mdp_core', 'byte', 'byte_intf', 'mnoc',
'iface', 'bus', 'core_mmss', 'pixel', 'core'] is too long

From schema:
/home/krzk/dev/linux/linux/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml

/home/krzk/dev/linux/linux/out/arch/arm64/boot/dts/qcom/sda660-inforce-ifc6560.dtb:
dsi@c994000: 'vdda-supply' does not match any of the regexes:
'pinctrl-[0-9]+'



If your child schema fails, the referencing schema fails as well...


Best regards,
Krzysztof

Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs

[Rob Clark]

On Wed, Sep 7, 2022 at 3:25 AM Dmitry Osipenko
 wrote:
>
> On 8/23/22 19:47, Rob Clark wrote:
> > On Tue, Aug 23, 2022 at 3:01 AM Christian König
> >  wrote:
> >>
> >> Am 22.08.22 um 19:26 schrieb Dmitry Osipenko:
> >>> On 8/16/22 22:55, Dmitry Osipenko wrote:
>  On 8/16/22 15:03, Christian König wrote:
> > Am 16.08.22 um 13:44 schrieb Dmitry Osipenko:
> >> [SNIP]
> >>> The other complication I noticed is that we don't seem to keep around
> >>> the fd after importing to a GEM handle.  And I could imagine that
> >>> doing so could cause issues with too many fd's.  So I guess the best
> >>> thing is to keep the status quo and let drivers that cannot mmap
> >>> imported buffers just fail mmap?
> >> That actually should be all the drivers excluding those that use
> >> DRM-SHMEM because only DRM-SHMEM uses dma_buf_mmap(), that's why it
> >> works for Panfrost. I'm pretty sure mmaping of imported GEMs doesn't
> >> work for the MSM driver, isn't it?
> >>
> >> Intel and AMD drivers don't allow to map the imported dma-bufs. Both
> >> refuse to do the mapping.
> >>
> >> Although, AMDGPU "succeeds" to do the mapping using
> >> AMDGPU_GEM_DOMAIN_GTT, but then touching the mapping causes bus fault,
> >> hence mapping actually fails. I think it might be the AMDGPU
> >> driver/libdrm bug, haven't checked yet.
> > That's then certainly broken somehow. Amdgpu should nerve ever have
> > allowed to mmap() imported DMA-bufs and the last time I check it didn't.
>  I'll take a closer look. So far I can only tell that it's a kernel
>  driver issue because once I re-applied this "Don't map imported GEMs"
>  patch, AMDGPU began to refuse mapping AMDGPU_GEM_DOMAIN_GTT.
> 
> >> So we're back to the point that neither of DRM drivers need to map
> >> imported dma-bufs and this was never tested. In this case this patch is
> >> valid, IMO.
>  Actually, I'm now looking at Etnaviv and Nouveau and seems they should
>  map imported dma-buf properly. I know that people ran Android on
>  Etnaviv. So maybe devices with a separated GPU/display need to map
>  imported display BO for Android support. Wish somebody who ran Android
>  on one of these devices using upstream drivers could give a definitive
>  answer. I may try to test Nouveau later on.
> 
> >>> Nouveau+Intel combo doesn't work because of [1] that says:
> >>>
> >>> "Refuse to fault imported pages. This should be handled (if at all) by
> >>> redirecting mmap to the exporter."
> >>>
> >>> [1]
> >>> https://elixir.bootlin.com/linux/v5.19/source/drivers/gpu/drm/ttm/ttm_bo_vm.c#L154
> >>>
> >>> Interestingly, I noticed that there are IGT tests which check prime
> >>> mmaping of Nouveau+Intel [2] (added 9 years ago), but they fail as well,
> >>> as expected. The fact that IGT has such tests is interesting because it
> >>> suggests that the mapping worked in the past. It's also surprising that
> >>> nobody cared to fix the failing tests. For the reference, I checked
> >>> v5.18 and today's linux-next.
> >>>
> >>> [2]
> >>> https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/prime_nv_test.c#L132
> >>>
> >>> Starting subtest: nv_write_i915_cpu_mmap_read
> >>> Received signal SIGBUS.
> >>> Stack trace:
> >>>   #0 [fatal_sig_handler+0x163]
> >>>   #1 [__sigaction+0x50]
> >>>   #2 [__igt_uniquereal_main354+0x406]
> >>>   #3 [main+0x23]
> >>>   #4 [__libc_start_call_main+0x80]
> >>>   #5 [__libc_start_main+0x89]
> >>>   #6 [_start+0x25]
> >>> Subtest nv_write_i915_cpu_mmap_read: CRASH (0,005s)
> >>>
> >>> Starting subtest: nv_write_i915_gtt_mmap_read
> >>> Received signal SIGBUS.
> >>> Stack trace:
> >>>   #0 [fatal_sig_handler+0x163]
> >>>   #1 [__sigaction+0x50]
> >>>   #2 [__igt_uniquereal_main354+0x33d]
> >>>   #3 [main+0x23]
> >>>   #4 [__libc_start_call_main+0x80]
> >>>   #5 [__libc_start_main+0x89]
> >>>   #6 [_start+0x25]
> >>> Subtest nv_write_i915_gtt_mmap_read: CRASH (0,004s)
> >>>
> >>> I'm curious about the Etnaviv driver because it uses own shmem
> >>> implementation and maybe it has a working mmaping of imported GEMs since
> >>> it imports the dma-buf pages into Entaviv BO. Although, it should be
> >>> risking to map pages using a different caching attributes (WC) from the
> >>> exporter, which is prohibited on ARM ad then one may try to map imported
> >>> udmabuf.
>
> I see now that Etnaviv uses dma_buf_mmap(), so it should be okay.
>
> >>> Apparently, the Intel DG TTM driver should be able to map imported
> >>> dma-buf because it sets TTM_TT_FLAG_EXTERNAL_MAPPABLE.
> >>
> >> Even with that flag set it is illegal to map the pages directly by an
> >> importer.
> >>
> >> If that ever worked then the only real solution is to redirect mmap()
> >> calls on importer BOs to dma_buf_mmap().
> >
> > Yeah, I think this is the best option.  Forcing userspace to hang on
> > to the fd just in case someone calls readpix would be 

Re: [PATCH v6 01/12] dt-bindings: display/msm: split qcom, mdss bindings

[Dmitry Baryshkov]

On Sun, 11 Sept 2022 at 16:57, Krzysztof Kozlowski
 wrote:
>
> On 11/09/2022 15:45, Dmitry Baryshkov wrote:
> > On Sun, 11 Sept 2022 at 14:27, Krzysztof Kozlowski
> >  wrote:
> >>
> >> On 10/09/2022 14:54, Dmitry Baryshkov wrote:
> 
>  However I think there is no such problem, as Dmitry said, that ref
>  changes anything. There will be always failure - either from parent
>  schema (using $ref) or from device child schema (the one which actually
>  misses the property).
> >>>
> >>> Initially I stumbled upon this issue with the dsi and dsi_phy nodes
> >>> for msm8996 devices. If I have $ref here, dsi1/dsi1_phy nodes will
> >>> emit warnings regarding the missing -supply properties despite nodes
> >>> being disabled. If I use `compatible' here, the schema checks pass.
> >>> Thus I'd prefer to leave `compatible' here. Not to mention that it
> >>> also allows specifying a tighter binding than just using the $ref.
> >>
> >> I don't think we understood each other. I claim that error will be there
> >> anyway, just from different schema. So your change fixes nothing in
> >> total schema check...
> >
> > If the node is disabled, there will be no different schema check.
>
> As I wrote before, there was.

The following results were captured with the following command, with
most of the DSI and MDSS schema files fixed, using the following
command:
$ PATH=~/.local/bin/:$PATH make -C ../build-64/ ARCH=arm64
qcom/sda660-inforce-ifc6560.dtb  CHECK_DTBS=y
DT_SCHEMA_FILES=display/msm

As you can see from the example below, when using 'compatible' I'm
getting warnings just for the gpu@500 node, while using $ref I
also got warnings for the dsi-phy@c996400 node (disabled in the DT
file).
For your reference the tree in question is uploaded to the:
https://git.linaro.org/people/dmitry.baryshkov/kernel.git msm-mdss-yaml

Logs:

#1 mdss.yaml using compatible enum for dsi-phy:
  "^dsi-phy@[1-9a-f][0-9a-f]*$":
type: object
properties:
  compatible:
enum:
  - qcom,dsi-phy-14nm
  - qcom,dsi-phy-14nm-660
  - qcom,dsi-phy-20nm
  - qcom,dsi-phy-28nm-hpm
  - qcom,dsi-phy-28nm-lp

make: Entering directory '/home/lumag/Projects/Qcomm/build-64'
/home/lumag/Projects/Qcomm/kernel/arch/arm64/Makefile:36: Detected
assembler with broken .inst; disassembly will be unreliable
  UPD include/config/kernel.release
  LINTDocumentation/devicetree/bindings
  CHKDT   Documentation/devicetree/bindings/processed-schema.json
  SCHEMA  Documentation/devicetree/bindings/processed-schema.json
/home/lumag/Projects/Qcomm/kernel/Documentation/devicetree/bindings/regulator/qcom,spmi-regulator.yaml:
ignoring, error in schema: patternProperties:
^(5vs[1-2]|(l|s)[1-9][0-9]?|lvs[1-3])$: properties
  DTC/CHECK   arch/arm64/boot/dts/qcom/sda660-inforce-ifc6560.dtb
/home/lumag/Projects/Qcomm/build-64/arch/arm64/boot/dts/qcom/sda660-inforce-ifc6560.dtb:
gpu@500: clock-names:4: 'anyOf' conditional failed, one must be
fixed:
'core' was expected
'iface' was expected
'mem' was expected
'mem_iface' was expected
'alt_mem_iface' was expected
'gfx3d' was expected
'rbbmtimer' was expected
>From schema: 
>/home/lumag/Projects/Qcomm/kernel/Documentation/devicetree/bindings/display/msm/gpu.yaml
make: Leaving directory '/home/lumag/Projects/Qcomm/build-64'


#2 mdss.yaml having dsi-phy rewritten to $ref:
  "^dsi-phy@[1-9a-f][0-9a-f]*$":
type: object
oneOf:
  - $ref: dsi-phy-14nm.yaml
  - $ref: dsi-phy-20nm.yaml
  - $ref: dsi-phy-28nm.yaml

make: Entering directory '/home/lumag/Projects/Qcomm/build-64'
/home/lumag/Projects/Qcomm/kernel/arch/arm64/Makefile:36: Detected
assembler with broken .inst; disassembly will be unreliable
  LINTDocumentation/devicetree/bindings
  CHKDT   Documentation/devicetree/bindings/processed-schema.json
  SCHEMA  Documentation/devicetree/bindings/processed-schema.json
/home/lumag/Projects/Qcomm/kernel/Documentation/devicetree/bindings/regulator/qcom,spmi-regulator.yaml:
ignoring, error in schema: patternProperties:
^(5vs[1-2]|(l|s)[1-9][0-9]?|lvs[1-3])$: properties
  DTC/CHECK   arch/arm64/boot/dts/qcom/sda660-inforce-ifc6560.dtb
/home/lumag/Projects/Qcomm/build-64/arch/arm64/boot/dts/qcom/sda660-inforce-ifc6560.dtb:
gpu@500: clock-names:4: 'anyOf' conditional failed, one must be
fixed:
'core' was expected
'iface' was expected
'mem' was expected
'mem_iface' was expected
'alt_mem_iface' was expected
'gfx3d' was expected
'rbbmtimer' was expected
>From schema: 
>/home/lumag/Projects/Qcomm/kernel/Documentation/devicetree/bindings/display/msm/gpu.yaml
/home/lumag/Projects/Qcomm/build-64/arch/arm64/boot/dts/qcom/sda660-inforce-ifc6560.dtb:
mdss@c90: dsi-phy@c996400: 'oneOf' conditional failed, one must be
fixed:
'vcca-supply' is a required property
'vddio-supply' is a required property
Unevaluated properties are not allowed ('compatible', 'reg-names' were
unexpected)
'qcom,dsi-phy-20nm' was expected
'qcom,dsi-phy-14nm-660' is not one 

Re: [PATCH 1/4] drm/gma500: Refactor backlight support

[Sam Ravnborg]

Hi Hans,

> >> +static int gma_backlight_update_status(struct backlight_device *bd)
> >> +{
> >> +  struct drm_device *dev = bl_get_data(bd);
> >> +  int level = bd->props.brightness;
> > 
> > Here backlight_get_brightness(bd) should be used.
> 
> Ack, but the old set methods all 3 used:
> 
>   int level = bd->props.brightness;
> 
> So that would be a small functional / behavior change.
> 
> As such I would prefer to split using backlight_get_brightness(bd)
> out into a separate patch for version 2 of the series.
> Like how I also made the change from type = BACKLIGHT_PLATFORM ->
> type = BACKLIGHT_RAW a separate change.
> 
> Would that be ok with you ?
That would be perfect!

> > 
> > 
> >>  int gma_backlight_init(struct drm_device *dev)
> >>  {
> >> -#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
> >>struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> >> +  struct backlight_properties props = {};
> >> +  int ret;
> >> +
> >>dev_priv->backlight_enabled = true;
> >> -  return dev_priv->ops->backlight_init(dev);
> >> -#else
> >> -  return 0;
> >> +  dev_priv->backlight_level = 100;
> >> +
> >> +  ret = dev_priv->ops->backlight_init(dev);
> >> +  if (ret)
> >> +  return ret;
> >> +
> >> +#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
> >> +  props.brightness = dev_priv->backlight_level;
> >> +  props.max_brightness = PSB_MAX_BRIGHTNESS;
> >> +  props.type = BACKLIGHT_PLATFORM;
> >> +
> >> +  dev_priv->backlight_device =
> >> +  backlight_device_register(dev_priv->ops->backlight_name,
> >> +dev->dev, dev,
> >> +_backlight_ops, );
> > 
> > Consider using the devm_backlight_device_register() variant.
> > Then you can drop gma_backlight_exit() - I think..
> 
> The problem is the rest of the driver does not use devm_foo functions,
> so then psb_driver_unload() which runs before the devm cleanup functions
> will already release various iommap-s before the backlight is unregistered.
> 
> This leaves a race where the backlight device could be poked and then try
> to use no longer valid pointers in the main driver struct to write to the hw.

Thanks for the explanation. When someone update the driver to devn_ then
they surely will include backlight too.
(I do not try to persuade you to do it, your time is better spent on the
bigger backlight picture).

Sam

Re: [PATCH] drm/hyperv: Don't rely on screen_info.lfb_base for Gen1 VMs

[Saurabh Singh Sengar]

On Sat, Sep 10, 2022 at 08:11:24PM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 09.09.22 um 16:43 schrieb Saurabh Sengar:
> >hyperv_setup_vram tries to remove conflicting framebuffer based on
> >'screen_info'. As observed in past due to some bug or wrong setting
> >in grub, the 'screen_info' fields may not be set for Gen1, and in such
> >cases drm_aperture_remove_conflicting_framebuffers will not do anything
> >useful.
> >For Gen1 VMs, it should always be possible to get framebuffer
> >conflict removed using PCI device instead.
> >
> >Fixes: a0ab5abced55 ("drm/hyperv : Removing the restruction of VRAM 
> >allocation with PCI bar size")
> >Signed-off-by: Saurabh Sengar 
> >---
> >  drivers/gpu/drm/hyperv/hyperv_drm_drv.c | 24 
> >  1 file changed, 20 insertions(+), 4 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c 
> >b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
> >index 6d11e7938c83..b0cc974efa45 100644
> >--- a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
> >+++ b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
> >@@ -73,12 +73,28 @@ static int hyperv_setup_vram(struct hyperv_drm_device 
> >*hv,
> >  struct hv_device *hdev)
> >  {
> > struct drm_device *dev = >dev;
> >+struct pci_dev *pdev;
> > int ret;
> >-drm_aperture_remove_conflicting_framebuffers(screen_info.lfb_base,
> >- screen_info.lfb_size,
> >- false,
> >- _driver);
> >+if (efi_enabled(EFI_BOOT)) {
> >+
> >drm_aperture_remove_conflicting_framebuffers(screen_info.lfb_base,
> >+ 
> >screen_info.lfb_size,
> 
> Using screen_info here seems wrong in any case. You want to remove
> the framebuffer devices that conflict with your driver, which might
> be unrelated to screen_info. AFAICT the correct solution would
> always retrieve the PCI device for removal (i.e., always do the else
> branch).

In a Gen2 VM, the Hyper-V frame buffer device is presented only as a VMbus 
device.
It's not presented as a PCI device like it is in a Gen1 VM. This would have 
worked
if we had the frame buffer device available as PCI device in Gen2 but 
unfortunately
thats not the case here.

Regards,
Saurabh

> 
> Best regard
> Thomas
> 
> >+ false,
> >+ _driver);
> >+} else {
> >+pdev = pci_get_device(PCI_VENDOR_ID_MICROSOFT, 
> >PCI_DEVICE_ID_HYPERV_VIDEO, NULL);
> >+if (!pdev) {
> >+drm_err(dev, "Unable to find PCI Hyper-V video\n");
> >+return -ENODEV;
> >+}
> >+
> >+ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, 
> >_driver);
> >+pci_dev_put(pdev);
> >+if (ret) {
> >+drm_err(dev, "Not able to remove boot fb\n");
> >+return ret;
> >+}
> >+}
> > hv->fb_size = (unsigned long)hv->mmio_megabytes * 1024 * 1024;
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Ivo Totev

Re: [PATCH] drm/hyperv: Add ratelimit on error message

[Saurabh Singh Sengar]

On Sat, Sep 10, 2022 at 08:06:05PM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 09.09.22 um 17:09 schrieb Saurabh Sengar:
> >Due to a full ring buffer, the driver may be unable to send updates to
> >the Hyper-V host.  But outputing the error message can make the problem
> >worse because console output is also typically written to the frame
> >buffer.
> >Rate limiting the error message, also output the error code for additional
> >diagnosability.
> >
> >Signed-off-by: Saurabh Sengar 
> >---
> >  drivers/gpu/drm/hyperv/hyperv_drm_proto.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c 
> >b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
> >index 76a182a..013a782 100644
> >--- a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
> >+++ b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
> >@@ -208,7 +208,7 @@ static inline int hyperv_sendpacket(struct hv_device 
> >*hdev, struct synthvid_msg
> >VM_PKT_DATA_INBAND, 0);
> > if (ret)
> >-drm_err(>dev, "Unable to send packet via vmbus\n");
> >+drm_err_ratelimited(>dev, "Unable to send packet via vmbus; 
> >error %d\n", ret);
> 
> I better option would probably be drm_err_once(). Then you'd get the
> first error message and skip all others.

Thanks for your comment however the intention here is to limit the printk 
messages and break the chain
rather then printing only once. There can be cases where at different point of 
time we again get a
ring buffer full condition and we don't want to miss that. We should get the 
message for each of these
errror which are widely-separated in time not just the first time.

> 
> Best regards
> Thomas
> 
> > return ret;
> >  }
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Ivo Totev

Re: [PATCH v2] drm/vkms: fix 32bit compilation error by replacing macros

[Igor Matheus Andrade Torrente]

Reviewed-by: Igor Torrente 

On 9/10/22 16:03, Melissa Wen wrote:

Replace vkms_formats macro for fixed-point operations with functions
from drm/drm_fixed.h to do the same job and fix 32-bit compilation
errors.

v2:
- don't cast results to s32 (Igor)
- add missing drm_fixp2int conversion (Igor)

Fixes: a19c2ac9858 ("drm: vkms: Add support to the RGB565 format")
Tested-by: Sudip Mukherjee  (v1)
Reviewed-by: Alex Deucher  (v1)
Reported-by: Sudip Mukherjee 
Reported-by: kernel test robot 
Signed-off-by: Melissa Wen 
---
  drivers/gpu/drm/vkms/vkms_formats.c | 53 +++--
  1 file changed, 19 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_formats.c 
b/drivers/gpu/drm/vkms/vkms_formats.c
index 300abb4d1dfe..d4950688b3f1 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.c
+++ b/drivers/gpu/drm/vkms/vkms_formats.c
@@ -1,27 +1,12 @@
  // SPDX-License-Identifier: GPL-2.0+
  
-#include 

+#include 
  #include 
+#include 
+#include 
  
  #include "vkms_formats.h"
  
-/* The following macros help doing fixed point arithmetic. */

-/*
- * With Fixed-Point scale 15 we have 17 and 15 bits of integer and fractional
- * parts respectively.
- *  |     0.000    |
- * 31  0
- */
-#define SHIFT 15
-
-#define INT_TO_FIXED(a) ((a) << SHIFT)
-#define FIXED_MUL(a, b) ((s32)(((s64)(a) * (b)) >> SHIFT))
-#define FIXED_DIV(a, b) ((s32)(((s64)(a) << SHIFT) / (b)))
-/* This macro converts a fixed point number to int, and round half up it */
-#define FIXED_TO_INT_ROUND(a) (((a) + (1 << (SHIFT - 1))) >> SHIFT)
-#define INT_TO_FIXED_DIV(a, b) (FIXED_DIV(INT_TO_FIXED(a), INT_TO_FIXED(b)))
-#define INT_TO_FIXED_DIV(a, b) (FIXED_DIV(INT_TO_FIXED(a), INT_TO_FIXED(b)))
-
  static size_t pixel_offset(const struct vkms_frame_info *frame_info, int x, 
int y)
  {
return frame_info->offset + (y * frame_info->pitch)
@@ -137,19 +122,19 @@ static void RGB565_to_argb_u16(struct line_buffer 
*stage_buffer,
int x_limit = min_t(size_t, drm_rect_width(_info->dst),
   stage_buffer->n_pixels);
  
-	s32 fp_rb_ratio = INT_TO_FIXED_DIV(65535, 31);

-   s32 fp_g_ratio = INT_TO_FIXED_DIV(65535, 63);
+   s64 fp_rb_ratio = drm_fixp_div(drm_int2fixp(65535), drm_int2fixp(31));
+   s64 fp_g_ratio = drm_fixp_div(drm_int2fixp(65535), drm_int2fixp(63));
  
  	for (size_t x = 0; x < x_limit; x++, src_pixels++) {

u16 rgb_565 = le16_to_cpu(*src_pixels);
-   s32 fp_r = INT_TO_FIXED((rgb_565 >> 11) & 0x1f);
-   s32 fp_g = INT_TO_FIXED((rgb_565 >> 5) & 0x3f);
-   s32 fp_b = INT_TO_FIXED(rgb_565 & 0x1f);
+   s64 fp_r = drm_int2fixp((rgb_565 >> 11) & 0x1f);
+   s64 fp_g = drm_int2fixp((rgb_565 >> 5) & 0x3f);
+   s64 fp_b = drm_int2fixp(rgb_565 & 0x1f);
  
  		out_pixels[x].a = (u16)0x;

-   out_pixels[x].r = FIXED_TO_INT_ROUND(FIXED_MUL(fp_r, 
fp_rb_ratio));
-   out_pixels[x].g = FIXED_TO_INT_ROUND(FIXED_MUL(fp_g, 
fp_g_ratio));
-   out_pixels[x].b = FIXED_TO_INT_ROUND(FIXED_MUL(fp_b, 
fp_rb_ratio));
+   out_pixels[x].r = drm_fixp2int(drm_fixp_mul(fp_r, fp_rb_ratio));
+   out_pixels[x].g = drm_fixp2int(drm_fixp_mul(fp_g, fp_g_ratio));
+   out_pixels[x].b = drm_fixp2int(drm_fixp_mul(fp_b, fp_rb_ratio));
}
  }
  
@@ -248,17 +233,17 @@ static void argb_u16_to_RGB565(struct vkms_frame_info *frame_info,

int x_limit = min_t(size_t, drm_rect_width(_info->dst),
src_buffer->n_pixels);
  
-	s32 fp_rb_ratio = INT_TO_FIXED_DIV(65535, 31);

-   s32 fp_g_ratio = INT_TO_FIXED_DIV(65535, 63);
+   s64 fp_rb_ratio = drm_fixp_div(drm_int2fixp(65535), drm_int2fixp(31));
+   s64 fp_g_ratio = drm_fixp_div(drm_int2fixp(65535), drm_int2fixp(63));
  
  	for (size_t x = 0; x < x_limit; x++, dst_pixels++) {

-   s32 fp_r = INT_TO_FIXED(in_pixels[x].r);
-   s32 fp_g = INT_TO_FIXED(in_pixels[x].g);
-   s32 fp_b = INT_TO_FIXED(in_pixels[x].b);
+   s64 fp_r = drm_int2fixp(in_pixels[x].r);
+   s64 fp_g = drm_int2fixp(in_pixels[x].g);
+   s64 fp_b = drm_int2fixp(in_pixels[x].b);
  
-		u16 r = FIXED_TO_INT_ROUND(FIXED_DIV(fp_r, fp_rb_ratio));

-   u16 g = FIXED_TO_INT_ROUND(FIXED_DIV(fp_g, fp_g_ratio));
-   u16 b = FIXED_TO_INT_ROUND(FIXED_DIV(fp_b, fp_rb_ratio));
+   u16 r = drm_fixp2int(drm_fixp_div(fp_r, fp_rb_ratio));
+   u16 g = drm_fixp2int(drm_fixp_div(fp_g, fp_g_ratio));
+   u16 b = drm_fixp2int(drm_fixp_div(fp_b, fp_rb_ratio));
  
  		*dst_pixels = cpu_to_le16(r << 11 | g << 5 | b);

}

Re: [PATCH] drm/vkms: fix 32bit compilation error by replacing macros

[Igor Matheus Andrade Torrente]

On 9/10/22 16:10, Melissa Wen wrote:

On 09/09, Igor Matheus Andrade Torrente wrote:

Hi Mellisa,

Thanks for the patch fixing my mistakes.

On 9/9/22 08:41, Melissa Wen wrote:

Replace vkms_formats macros for fixed-point operations with functions
from drm/drm_fixed.h to do the same job and fix 32-bit compilation
errors.

Fixes: a19c2ac9858 ("drm: vkms: Add support to the RGB565 format")
Tested-by: Sudip Mukherjee 
Reported-by: Sudip Mukherjee 
Reported-by: kernel test robot 
Signed-off-by: Melissa Wen 
---
   drivers/gpu/drm/vkms/vkms_formats.c | 53 +++--
   1 file changed, 19 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_formats.c 
b/drivers/gpu/drm/vkms/vkms_formats.c
index 300abb4d1dfe..ddcd3cfeeaac 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.c
+++ b/drivers/gpu/drm/vkms/vkms_formats.c
@@ -1,27 +1,12 @@
   // SPDX-License-Identifier: GPL-2.0+
-#include 
+#include 
   #include 
+#include 
+#include 
   #include "vkms_formats.h"
-/* The following macros help doing fixed point arithmetic. */
-/*
- * With Fixed-Point scale 15 we have 17 and 15 bits of integer and fractional
- * parts respectively.
- *  |     0.000    |
- * 31  0
- */
-#define SHIFT 15
-
-#define INT_TO_FIXED(a) ((a) << SHIFT)
-#define FIXED_MUL(a, b) ((s32)(((s64)(a) * (b)) >> SHIFT))
-#define FIXED_DIV(a, b) ((s32)(((s64)(a) << SHIFT) / (b)))
-/* This macro converts a fixed point number to int, and round half up it */
-#define FIXED_TO_INT_ROUND(a) (((a) + (1 << (SHIFT - 1))) >> SHIFT)
-#define INT_TO_FIXED_DIV(a, b) (FIXED_DIV(INT_TO_FIXED(a), INT_TO_FIXED(b)))
-#define INT_TO_FIXED_DIV(a, b) (FIXED_DIV(INT_TO_FIXED(a), INT_TO_FIXED(b)))
-
   static size_t pixel_offset(const struct vkms_frame_info *frame_info, int x, 
int y)
   {
return frame_info->offset + (y * frame_info->pitch)
@@ -137,19 +122,19 @@ static void RGB565_to_argb_u16(struct line_buffer 
*stage_buffer,
int x_limit = min_t(size_t, drm_rect_width(_info->dst),
   stage_buffer->n_pixels);
-   s32 fp_rb_ratio = INT_TO_FIXED_DIV(65535, 31);
-   s32 fp_g_ratio = INT_TO_FIXED_DIV(65535, 63);
+   s32 fp_rb_ratio = drm_fixp_div(drm_int2fixp(65535), 31);
+   s32 fp_g_ratio = drm_fixp_div(drm_int2fixp(65535), 63);


I think you need to add `drm_int2fixp` to 31 and 63.


for (size_t x = 0; x < x_limit; x++, src_pixels++) {
u16 rgb_565 = le16_to_cpu(*src_pixels);
-   s32 fp_r = INT_TO_FIXED((rgb_565 >> 11) & 0x1f);
-   s32 fp_g = INT_TO_FIXED((rgb_565 >> 5) & 0x3f);
-   s32 fp_b = INT_TO_FIXED(rgb_565 & 0x1f);
+   s32 fp_r = drm_int2fixp((rgb_565 >> 11) & 0x1f);
+   s32 fp_g = drm_int2fixp((rgb_565 >> 5) & 0x3f);
+   s32 fp_b = drm_int2fixp(rgb_565 & 0x1f);


And we are cast implicitly from 64 bits int to 32 bits which is
implementation-defined AFAIK. So, probably we should be using `s64` for all
of these variables.

I tested the patch. And I'm seeing some differences in the intermediate
results. From my testing, these changes solve those differences.


Hi Igor,

Thanks for checking the calc results and all inputs provided.  I just
sent a second version, can you take a look? I replicated your
suggestions for RGB565_to_argb_u16() in argb_u16_to_RGB565() and
double-checked for i386 and arm. Let me know what yexiuou think.


I tested it here and everything seem to be working :).





Another thing that may have an impact on the final output is the lack of
rounding in drm_fixed.h. This can potentially produce the wrong result.


Yeah, I see... I can include a comment about the rounding issue for
further improvements, or do you plan to work on it?


A comment would be good. Maybe add to the VKMS TODO list?

I'm busy with other stuffs, and can't work on this any time soon. But 
It's pretty simple thing to implement.




Thanks,

Melissa


Thanks,
---
Igor Torrente


out_pixels[x].a = (u16)0x;
-   out_pixels[x].r = FIXED_TO_INT_ROUND(FIXED_MUL(fp_r, 
fp_rb_ratio));
-   out_pixels[x].g = FIXED_TO_INT_ROUND(FIXED_MUL(fp_g, 
fp_g_ratio));
-   out_pixels[x].b = FIXED_TO_INT_ROUND(FIXED_MUL(fp_b, 
fp_rb_ratio));
+   out_pixels[x].r = drm_fixp2int(drm_fixp_mul(fp_r, fp_rb_ratio));
+   out_pixels[x].g = drm_fixp2int(drm_fixp_mul(fp_g, fp_g_ratio));
+   out_pixels[x].b = drm_fixp2int(drm_fixp_mul(fp_b, fp_rb_ratio));
}
   }
@@ -248,17 +233,17 @@ static void argb_u16_to_RGB565(struct vkms_frame_info 
*frame_info,
int x_limit = min_t(size_t, drm_rect_width(_info->dst),
src_buffer->n_pixels);
-   s32 fp_rb_ratio = INT_TO_FIXED_DIV(65535, 31);
-   s32 fp_g_ratio = INT_TO_FIXED_DIV(65535, 63);
+   s32 fp_rb_ratio = drm_fixp_div(drm_int2fixp(65535), 31);
+   s32 fp_g_ratio = 

Re: [PATCH v6 01/12] dt-bindings: display/msm: split qcom,mdss bindings

[Krzysztof Kozlowski]

On 11/09/2022 15:45, Dmitry Baryshkov wrote:
> On Sun, 11 Sept 2022 at 14:27, Krzysztof Kozlowski
>  wrote:
>>
>> On 10/09/2022 14:54, Dmitry Baryshkov wrote:

 However I think there is no such problem, as Dmitry said, that ref
 changes anything. There will be always failure - either from parent
 schema (using $ref) or from device child schema (the one which actually
 misses the property).
>>>
>>> Initially I stumbled upon this issue with the dsi and dsi_phy nodes
>>> for msm8996 devices. If I have $ref here, dsi1/dsi1_phy nodes will
>>> emit warnings regarding the missing -supply properties despite nodes
>>> being disabled. If I use `compatible' here, the schema checks pass.
>>> Thus I'd prefer to leave `compatible' here. Not to mention that it
>>> also allows specifying a tighter binding than just using the $ref.
>>
>> I don't think we understood each other. I claim that error will be there
>> anyway, just from different schema. So your change fixes nothing in
>> total schema check...
> 
> If the node is disabled, there will be no different schema check.

As I wrote before, there was.

Best regards,
Krzysztof

Re: [PATCH v6 01/12] dt-bindings: display/msm: split qcom, mdss bindings

[Dmitry Baryshkov]

On Sun, 11 Sept 2022 at 14:27, Krzysztof Kozlowski
 wrote:
>
> On 10/09/2022 14:54, Dmitry Baryshkov wrote:
> >>
> >> However I think there is no such problem, as Dmitry said, that ref
> >> changes anything. There will be always failure - either from parent
> >> schema (using $ref) or from device child schema (the one which actually
> >> misses the property).
> >
> > Initially I stumbled upon this issue with the dsi and dsi_phy nodes
> > for msm8996 devices. If I have $ref here, dsi1/dsi1_phy nodes will
> > emit warnings regarding the missing -supply properties despite nodes
> > being disabled. If I use `compatible' here, the schema checks pass.
> > Thus I'd prefer to leave `compatible' here. Not to mention that it
> > also allows specifying a tighter binding than just using the $ref.
>
> I don't think we understood each other. I claim that error will be there
> anyway, just from different schema. So your change fixes nothing in
> total schema check...

If the node is disabled, there will be no different schema check.

-- 
With best wishes
Dmitry

Re: [PATCH 1/4] drm/gma500: Refactor backlight support

[Hans de Goede]

Hi Sam,

On 9/11/22 13:48, Sam Ravnborg wrote:
> Hi Hans,
> 
> just a few minor things. See comments.
> I like the diff - removes much more than it adds.

I'm glad you like it and thank you for the review.

> On Sat, Sep 10, 2022 at 10:50:58PM +0200, Hans de Goede wrote:
>> Refactor backlight support so that the gma_backlight_enable() /
>> gma_backlight_disable() / gma_backlight_set() functions used by
>> the Opregion handle will also work if no backlight_device gets
>> registered.
>>
>> This is a preparation patch for not registering the gma500's own backlight
>> device when acpi_video should be used, since registering 2 backlight
>> devices for a single display really is undesirable.
>>
>> Since the acpi-video interface often uses the OpRegion we need to keep
>> the OpRegion functional even when dev_priv->backlight_device is NULL.
>>
>> As a result of this refactor the actual backlight_device_register()
>> call is moved to the shared backlight.c code and all #ifdefs related to
>> CONFIG_BACKLIGHT_CLASS_DEVICE are now also limited to backlight.c .
>>
>> No functional changes intended.
>>
>> This has been tested on a Packard Bell Dot SC (Intel Atom N2600, cedarview)
>> and a Sony Vaio vpc-x11s1e (Intel N540, poulsbo) laptop.
>>
>> Signed-off-by: Hans de Goede 
>> ---
> 
>> +static int gma_backlight_update_status(struct backlight_device *bd)
>> +{
>> +struct drm_device *dev = bl_get_data(bd);
>> +int level = bd->props.brightness;
> 
> Here backlight_get_brightness(bd) should be used.

Ack, but the old set methods all 3 used:

int level = bd->props.brightness;

So that would be a small functional / behavior change.

As such I would prefer to split using backlight_get_brightness(bd)
out into a separate patch for version 2 of the series.
Like how I also made the change from type = BACKLIGHT_PLATFORM ->
type = BACKLIGHT_RAW a separate change.

Would that be ok with you ?

> 
> 
>>  int gma_backlight_init(struct drm_device *dev)
>>  {
>> -#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
>>  struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
>> +struct backlight_properties props = {};
>> +int ret;
>> +
>>  dev_priv->backlight_enabled = true;
>> -return dev_priv->ops->backlight_init(dev);
>> -#else
>> -return 0;
>> +dev_priv->backlight_level = 100;
>> +
>> +ret = dev_priv->ops->backlight_init(dev);
>> +if (ret)
>> +return ret;
>> +
>> +#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
>> +props.brightness = dev_priv->backlight_level;
>> +props.max_brightness = PSB_MAX_BRIGHTNESS;
>> +props.type = BACKLIGHT_PLATFORM;
>> +
>> +dev_priv->backlight_device =
>> +backlight_device_register(dev_priv->ops->backlight_name,
>> +  dev->dev, dev,
>> +  _backlight_ops, );
> 
> Consider using the devm_backlight_device_register() variant.
> Then you can drop gma_backlight_exit() - I think..

The problem is the rest of the driver does not use devm_foo functions,
so then psb_driver_unload() which runs before the devm cleanup functions
will already release various iommap-s before the backlight is unregistered.

This leaves a race where the backlight device could be poked and then try
to use no longer valid pointers in the main driver struct to write to the hw.

Regards,

Hans





> 
>> +if (IS_ERR(dev_priv->backlight_device))
>> +return PTR_ERR(dev_priv->backlight_device);
>>  #endif
>> +
>> +return 0;
>>  }
>>  
>>  void gma_backlight_exit(struct drm_device *dev)
>>  {
>>  #ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
>>  struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
>> -if (dev_priv->backlight_device) {
>> -dev_priv->backlight_device->props.brightness = 0;
>> -backlight_update_status(dev_priv->backlight_device);
>> +
>> +if (dev_priv->backlight_device)
>>  backlight_device_unregister(dev_priv->backlight_device);
>> -}
>>  #endif
>>  }
> 

Re: [PATCH v2] drm/vkms: fix 32bit compilation error by replacing macros

[Dave Airlie]

On Sun, 11 Sept 2022 at 05:03, Melissa Wen  wrote:
>
> Replace vkms_formats macro for fixed-point operations with functions
> from drm/drm_fixed.h to do the same job and fix 32-bit compilation
> errors.
>
> v2:
> - don't cast results to s32 (Igor)
> - add missing drm_fixp2int conversion (Igor)

btw I've applied this in drm-next, as otherwise I can't build my
32-bit arm build.

Dave.

Re: [PATCH 1/4] drm/gma500: Refactor backlight support

[Sam Ravnborg]

Hi Hans,

just a few minor things. See comments.
I like the diff - removes much more than it adds.

Sam

On Sat, Sep 10, 2022 at 10:50:58PM +0200, Hans de Goede wrote:
> Refactor backlight support so that the gma_backlight_enable() /
> gma_backlight_disable() / gma_backlight_set() functions used by
> the Opregion handle will also work if no backlight_device gets
> registered.
> 
> This is a preparation patch for not registering the gma500's own backlight
> device when acpi_video should be used, since registering 2 backlight
> devices for a single display really is undesirable.
> 
> Since the acpi-video interface often uses the OpRegion we need to keep
> the OpRegion functional even when dev_priv->backlight_device is NULL.
> 
> As a result of this refactor the actual backlight_device_register()
> call is moved to the shared backlight.c code and all #ifdefs related to
> CONFIG_BACKLIGHT_CLASS_DEVICE are now also limited to backlight.c .
> 
> No functional changes intended.
> 
> This has been tested on a Packard Bell Dot SC (Intel Atom N2600, cedarview)
> and a Sony Vaio vpc-x11s1e (Intel N540, poulsbo) laptop.
> 
> Signed-off-by: Hans de Goede 
> ---

> +static int gma_backlight_update_status(struct backlight_device *bd)
> +{
> + struct drm_device *dev = bl_get_data(bd);
> + int level = bd->props.brightness;

Here backlight_get_brightness(bd) should be used.


>  int gma_backlight_init(struct drm_device *dev)
>  {
> -#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
>   struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> + struct backlight_properties props = {};
> + int ret;
> +
>   dev_priv->backlight_enabled = true;
> - return dev_priv->ops->backlight_init(dev);
> -#else
> - return 0;
> + dev_priv->backlight_level = 100;
> +
> + ret = dev_priv->ops->backlight_init(dev);
> + if (ret)
> + return ret;
> +
> +#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
> + props.brightness = dev_priv->backlight_level;
> + props.max_brightness = PSB_MAX_BRIGHTNESS;
> + props.type = BACKLIGHT_PLATFORM;
> +
> + dev_priv->backlight_device =
> + backlight_device_register(dev_priv->ops->backlight_name,
> +   dev->dev, dev,
> +   _backlight_ops, );

Consider using the devm_backlight_device_register() variant.
Then you can drop gma_backlight_exit() - I think..

> + if (IS_ERR(dev_priv->backlight_device))
> + return PTR_ERR(dev_priv->backlight_device);
>  #endif
> +
> + return 0;
>  }
>  
>  void gma_backlight_exit(struct drm_device *dev)
>  {
>  #ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
>   struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> - if (dev_priv->backlight_device) {
> - dev_priv->backlight_device->props.brightness = 0;
> - backlight_update_status(dev_priv->backlight_device);
> +
> + if (dev_priv->backlight_device)
>   backlight_device_unregister(dev_priv->backlight_device);
> - }
>  #endif
>  }

Re: [PATCH v6 01/12] dt-bindings: display/msm: split qcom,mdss bindings

[Krzysztof Kozlowski]

On 10/09/2022 14:54, Dmitry Baryshkov wrote:
>>
>> However I think there is no such problem, as Dmitry said, that ref
>> changes anything. There will be always failure - either from parent
>> schema (using $ref) or from device child schema (the one which actually
>> misses the property).
> 
> Initially I stumbled upon this issue with the dsi and dsi_phy nodes
> for msm8996 devices. If I have $ref here, dsi1/dsi1_phy nodes will
> emit warnings regarding the missing -supply properties despite nodes
> being disabled. If I use `compatible' here, the schema checks pass.
> Thus I'd prefer to leave `compatible' here. Not to mention that it
> also allows specifying a tighter binding than just using the $ref.

I don't think we understood each other. I claim that error will be there
anyway, just from different schema. So your change fixes nothing in
total schema check...


Best regards,
Krzysztof

Re: [PATCH v10 3/9] compiler_types.h: Add assert_type to catch type mis-match while compiling

[Andi Shyti]

Hi Gwan-gyeong,

On Fri, Sep 09, 2022 at 07:59:07PM +0900, Gwan-gyeong Mun wrote:
> It adds assert_type and assert_typable macros to catch type mis-match while

/Add/It adds/, please use the imperative form.

> compiling. The existing typecheck() macro outputs build warnings, but the
> newly added assert_type() macro uses the _Static_assert() keyword (which is
> introduced in C11) to generate a build break when the types are different
> and can be used to detect explicit build errors.
> Unlike the assert_type() macro, assert_typable() macro allows a constant
> value as the second argument.
> 
> Suggested-by: Kees Cook 
> Signed-off-by: Gwan-gyeong Mun 
> Cc: Thomas Hellström 
> Cc: Matthew Auld 
> Cc: Nirmoy Das 
> Cc: Jani Nikula 
> Cc: Andi Shyti 
> Cc: Mauro Carvalho Chehab 
> Cc: Andrzej Hajda 
> Cc: Kees Cook 
> ---
>  include/linux/compiler_types.h | 39 ++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index 4f2a819fd60a..19cc125918bb 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> @@ -294,6 +294,45 @@ struct ftrace_likely_data {
>  /* Are two types/vars the same type (ignoring qualifiers)? */
>  #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
>  
> +/**
> + * assert_type - break compile if the first argument's data type and the 
> second
> + *   argument's data type are not the same

I would use /aborts compilation/break compile/

> + *

nowhere is written that this extra blank line is not needed, but
just checking the style in compiler_types.h it is not used.

I personally like the blank line, but standing to the general
taste, it should be removed also for keeping a coherent style.

> + * @t1: data type or variable
> + * @t2: data type or variable
> + *
> + * The first and second arguments can be data types or variables or mixed 
> (the
> + * first argument is the data type and the second argument is variable or 
> vice
> + * versa). It determines whether the first argument's data type and the 
> second
> + * argument's data type are the same while compiling, and it breaks compile 
> if
> + * the two types are not the same.
> + * See also assert_typable().
> + */
> +#define assert_type(t1, t2) _Static_assert(__same_type(t1, t2))

In C11 _Static_assert is defined as:

  _Static_assert ( constant-expression , string-literal ) ;

While

  _Static_assert ( constant-expression ) ;

is defined in C17 along with the previous. I think you should add
the error message as a 'string-literal'.

Andi

> +/**
> + * assert_typable - break compile if the first argument's data type and the
> + *  second argument's data type are not the same
> + *
> + * @t: data type or variable
> + * @n: data type or variable or constant value
> + *
> + * The first and second arguments can be data types or variables or mixed 
> (the
> + * first argument is the data type and the second argument is variable or 
> vice
> + * versa). Unlike the assert_type() macro, this macro allows a constant value
> + * as the second argument. And if the second argument is a constant value, it
> + * always passes. And it doesn't mean that the types are explicitly the same.
> + * When a constant value is used as the second argument, if you need an
> + * overflow check when assigning a constant value to a variable of the type 
> of
> + * the first argument, you can use the overflows_type() macro. When a 
> constant
> + * value is not used as a second argument, it determines whether the first
> + * argument's data type and the second argument's data type are the same 
> while
> + * compiling, and it breaks compile if the two types are not the same.
> + * See also assert_type() and overflows_type().
> + */
> +#define assert_typable(t, n) _Static_assert(__builtin_constant_p(n) ||   
> \
> + __same_type(t, typeof(n)))
> +
>  /*
>   * __unqual_scalar_typeof(x) - Declare an unqualified scalar type, leaving
>   *  non-scalar types unchanged.
> -- 
> 2.37.1

Re: [PATCH v10 1/9] overflow: Allow mixed type arguments

[Andi Shyti]

Hi Kees,

On Fri, Sep 09, 2022 at 07:59:05PM +0900, Gwan-gyeong Mun wrote:
> From: Kees Cook 
> 
> When the check_[op]_overflow() helpers were introduced, all arguments were
> required to be the same type to make the fallback macros simpler. However,
> now that the fallback macros have been removed[1], it is fine to allow
> mixed types, which makes using the helpers much more useful, as they
> can be used to test for type-based overflows (e.g. adding two large ints
> but storing into a u8), as would be handy in the drm core[2].
> 
> Remove the restriction, and add additional self-tests that exercise some
> of the mixed-type overflow cases, and double-check for accidental macro
> side-effects.

I would split in two different patches the check_[op]_overflow()
helpers with the tests.

Overall they look good though.

> [1] https://git.kernel.org/linus/4eb6bd55cfb22ffc20652732340c4962f3ac9a91
> [2] 
> https://lore.kernel.org/lkml/20220824084514.2261614-2-gwan-gyeong@intel.com
> 
> Cc: Rasmus Villemoes 
> Cc: Gwan-gyeong Mun 
> Cc: Andrzej Hajda 
> Cc: "Gustavo A. R. Silva" 
> Cc: Nick Desaulniers 
> Cc: linux-harden...@vger.kernel.org
> Signed-off-by: Kees Cook 

Gwan-gyeong, can you please add your SoB here? And you don't need
to 'Cc:' yourself.

Andi

Re: Meeting (BOF) at Plumbers Dublin to discuss backlight brightness as connector object property RFC?

[Hans de Goede]

Hi All,

On 9/9/22 12:23, Hans de Goede wrote:
> Hi All,
> 
> I will be at Plumbers Dublin next week and I was wondering if
> anyone interested in this wants to get together for a quick
> discussion / birds of a feather session about this?
> 
> I have just posted version 2 of the RFC:
> https://lore.kernel.org/dri-devel/b61d3eeb-6213-afac-2e70-7b9791c86...@redhat.com/T/#u
> 
> If you are interested in meeting up please send me
> an email off-list (no need to spam the list further with this)
> also please let me know if there are any times which do not
> work for you, and/or times which have your preference.
> 
> I don't have a specific room/time for this yet, but if people
> are interested I will try to get a room from the organization
> and if that does not work out I'm sure we will figure something
> out.

I have a date, time and location for this now. The BOF session
is scheduled on Monday September 12th in the "Meeting 9" room:

https://lpc.events/event/16/contributions/1390/

Regards,

Hans

Re: [Intel-gfx] Developing a new backlight driver for specific OLED screen

[Aurélien]

Hi, 

> + dri-devel mailing list that looks more appropriated.
> + Hans and Lyude who were recently working to standardize some of the
> backlight stuff.

Thank you for these contacts. I'll try there if I need.

> I don't believe you want to use the i915 API, but move the common functions
> to the drm subsystem itself and then reuse as a drm device.

If there is enough generic code I'll do everything with the DRM API. 
Unfortunately I can't spend too much time in order to generalize the i915 
common functions.

> Aurélien, are you aware of drivers/gpu/drm/display/drm_dp_helper.c and
> all the functions around struct dp_aux_backlight and struct
> drm_edp_backlight_info?

Not yet. Since I'm not familiar with GPU/display drivers I didn't know what 
could be a good starting point. 
Indeed I already checked the intel_dp_aux_backlight.c code. That's why I told 
about using the "i915 API code" at first. But since this display is independent 
from the GPU i didn't want to link both code. 
Then that's a really good point if there is already an independant API. I'll 
have a look this evening.

> Does the display use some proprietary, non-VESA DPCD registers? There's
> already some of that in i915 for Intel proprietary interfaces.

For sure. It's an OLED display. Thus there is no backlight. It uses specific 
registers to control the brightness of the screen.
Unfortunately I guess the mechanism is not shared with many OLED displays...

Thank you for your help.

Aurélien

Re: [PATCH v3 01/15] vfio: Add helpers for unifying vfio_device life cycle

[Ethan Zhao]

Hi, Kevin,

在 2022/9/9 18:22, Kevin Tian 写道:

The idea is to let vfio core manage the vfio_device life cycle instead
of duplicating the logic cross drivers. This is also a preparatory
step for adding struct device into vfio_device.

New pair of helpers together with a kref in vfio_device:

  - vfio_alloc_device()
  - vfio_put_device()


To be honest, this pair of functions make me confusing to understand their

behaviour from wording point of view:

- vfio_alloc_device(),  Okay, it allocates the vfio device, no reference
 count thing. but,
- vfio_put_device()
 seems it will decrease reference count and then if it is zero, free it.
 so they are not of one *pair* about wording.

How about
 
- vfio_alloc_device() / - vfio_free_device()

or
- vfio_get_device() / - vfio_put_device(), perhaps not match their behviour
in following code.

 


Thanks,
Ethan
 



Drivers can register @init/@release callbacks to manage any private
state wrapping the vfio_device.

However vfio-ccw doesn't fit this model due to a life cycle mess
that its private structure mixes both parent and mdev info hence must
be allocated/freed outside of the life cycle of vfio device.

Per prior discussions this won't be fixed in short term by IBM folks.

Instead of waiting for those modifications introduce another helper
vfio_init_device() so ccw can call it to initialize a pre-allocated
vfio_device.

Further implication of the ccw trick is that vfio_device cannot be
freed uniformly in vfio core. Instead, require *EVERY* driver to
implement @release and free vfio_device inside. Then ccw can choose
to delay the free at its own discretion.

Another trick down the road is that kvzalloc() is used to accommodate
the need of gvt which uses vzalloc() while all others use kzalloc().
So drivers should call a helper vfio_free_device() to free the
vfio_device instead of assuming that kfree() or vfree() is appliable.

Later once the ccw mess is fixed we can remove those tricks and
fully handle structure alloc/free in vfio core.

Existing vfio_{un}init_group_dev() will be deprecated after all
existing usages are converted to the new model.

Suggested-by: Jason Gunthorpe 
Co-developed-by: Yi Liu 
Signed-off-by: Yi Liu 
Signed-off-by: Kevin Tian 
Reviewed-by: Tony Krowiak 
Reviewed-by: Jason Gunthorpe 
Reviewed-by: Eric Auger 
---
  drivers/vfio/vfio_main.c | 92 
  include/linux/vfio.h | 25 ++-
  2 files changed, 116 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index 27d9186f35d5..adc1b697bb78 100644
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -498,6 +498,98 @@ void vfio_uninit_group_dev(struct vfio_device *device)
  }
  EXPORT_SYMBOL_GPL(vfio_uninit_group_dev);
  
+/* Release helper called by vfio_put_device() */

+void vfio_device_release(struct kref *kref)
+{
+   struct vfio_device *device =
+   container_of(kref, struct vfio_device, kref);
+
+   vfio_uninit_group_dev(device);
+
+   /*
+* kvfree() cannot be done here due to a life cycle mess in
+* vfio-ccw. Before the ccw part is fixed all drivers are
+* required to support @release and call vfio_free_device()
+* from there.
+*/
+   device->ops->release(device);
+}
+EXPORT_SYMBOL_GPL(vfio_device_release);
+
+/*
+ * Alloc and initialize vfio_device so it can be registered to vfio
+ * core.
+ *
+ * Drivers should use the wrapper vfio_alloc_device() for allocation.
+ * @size is the size of the structure to be allocated, including any
+ * private data used by the driver.
+ *
+ * Driver may provide an @init callback to cover device private data.
+ *
+ * Use vfio_put_device() to release the structure after success return.
+ */
+struct vfio_device *_vfio_alloc_device(size_t size, struct device *dev,
+  const struct vfio_device_ops *ops)
+{
+   struct vfio_device *device;
+   int ret;
+
+   if (WARN_ON(size < sizeof(struct vfio_device)))
+   return ERR_PTR(-EINVAL);
+
+   device = kvzalloc(size, GFP_KERNEL);
+   if (!device)
+   return ERR_PTR(-ENOMEM);
+
+   ret = vfio_init_device(device, dev, ops);
+   if (ret)
+   goto out_free;
+   return device;
+
+out_free:
+   kvfree(device);
+   return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(_vfio_alloc_device);
+
+/*
+ * Initialize a vfio_device so it can be registered to vfio core.
+ *
+ * Only vfio-ccw driver should call this interface.
+ */
+int vfio_init_device(struct vfio_device *device, struct device *dev,
+const struct vfio_device_ops *ops)
+{
+   int ret;
+
+   vfio_init_group_dev(device, dev, ops);
+
+   if (ops->init) {
+   ret = ops->init(device);
+   if (ret)
+   goto out_uninit;
+   }
+
+   kref_init(>kref);
+   return 0;
+
+out_uninit:
+