Re: [PATCH 2/2] fb: amifb: Convert to platform remove callback returning void

[Geert Uytterhoeven]

On Thu, Nov 9, 2023 at 11:02 PM Uwe Kleine-König
 wrote:
> The .remove() callback for a platform driver returns an int which makes
> many driver authors wrongly assume it's possible to do error handling by
> returning an error code. However the value returned is ignored (apart
> from emitting a warning) and this typically results in resource leaks.
>
> To improve here there is a quest to make the remove callback return
> void. In the first step of this quest all drivers are converted to
> .remove_new(), which already returns void. Eventually after all drivers
> are converted, .remove_new() will be renamed to .remove().
>
> Trivially convert this driver from always returning zero in the remove
> callback to the void returning variant.
>
> Signed-off-by: Uwe Kleine-König 

Reviewed-by: Geert Uytterhoeven 

Gr{oetje,eeting}s,

Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH 1/2] fb: amifb: Mark driver struct with __refdata to prevent section mismatch warning

[Geert Uytterhoeven]

Hi Uwe,

On Thu, Nov 9, 2023 at 11:02 PM Uwe Kleine-König
 wrote:
> As described in the added code comment, a reference to .exit.text is ok
> for drivers registered via module_platform_driver_probe(). Make this
> explicit to prevent a section mismatch warning.
>
> Signed-off-by: Uwe Kleine-König 

Thanks for your patch!
Why am I not seeing the actual section mismatch warning, not even
with W=1?

Reviewed-by: Geert Uytterhoeven 

Gr{oetje,eeting}s,

Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH 02/17] dt-bindings: i2c: exynos5: add specific compatibles for existing SoC

[Krzysztof Kozlowski]

On 09/11/2023 19:05, Alim Akhtar wrote:

(...)

Please trim unrelated parts of response/quote before and after your message.

>> @@ -25,7 +25,15 @@ properties:
>>- samsung,exynos5250-hsi2c# Exynos5250 and Exynos5420
>>- samsung,exynos5260-hsi2c# Exynos5260
>>- samsung,exynos7-hsi2c   # Exynos7
>> -  - samsung,exynosautov9-hsi2c  # ExynosAutoV9 and Exynos850
>> +  - samsung,exynosautov9-hsi2c
>> +  - items:
>> +  - enum:
>> +  - samsung,exynos5433-hsi2c
>> +  - const: samsung,exynos7-hsi2c
>> +  - items:
>> +  - enum:
>> +  - samsung,exynos850-hsi2c
> Does this need an entry in allOf:? to indicate exynos850 also has 2 clocks?
> 

No, autov9 is there already.

>> +  - const: samsung,exynosautov9-hsi2c


Best regards,
Krzysztof

Re: [PATCH i-g-t 2/2] lib/kunit: Execute test cases synchronously

[Mauro Carvalho Chehab]

On Mon,  6 Nov 2023 10:59:38 +0100
Janusz Krzysztofik  wrote:

> Up to now we've been loading a KUnit test module in background and parsing
> in parallel its KTAP output with results from all the module's test cases.
> However, recent KUnit implementation is capable of executing only those
> test cases that match a user filter specified on test module load.
> 
> Stop loading the KUnit test module in background once per the whole
> subtest.  Instead, load the module on each dynamic sub-subtest execution
> with a filter that selects a specific test case and wait for its
> completion.  If successful and no kernel taint then parse the whole KTAP
> report it has produced and translate it to IGT result of the sub-subtest.
> 
> With that in place, we no longer need to skip the whole subtest on a
> failure from module loading or KTAP reading or parsing.  Since such event
> is now local to execution of an individual test case, only fail its
> corresponding dynamic sub-subtests and continue with subsequent ones.
> However, still omit execution of subsequent test cases once the kernel
> gets tainted.

The main reason why loading the module in background was done in the
first place was because if there is a BUG() or PANIC() or gpu hang
while executing KUnit, the dmesg KUnit parser was not producing anything,
and were blocking normal dmesg parsing by IGT runner. While I didn't
test such changes, we need to check what will be there at the JSON file
when IGT resume run is enabled, and those kind of bugs are triggered.

-

Yet, let's take one step back, as IMO this may not solve the issues.
See, when you say:  

> only fail its
> corresponding dynamic sub-subtests and continue with subsequent ones.

When a normal failure happens on KUnit, other tests will be executed. This
is the normal KUnit behavior. So, except if we're doing something wrong on
Xe or KMS tests, the other subtests after the failure will be tested.

However, if such failure is calling BUG_ON(), then the driver will be
tainted, and will prevent module unload. On such case, it doesn't matter
if tests are executed in batch or not: IGT will try to unload the
KUnit module but it will fail due to BUG_ON() behavior.

Also, doing module unload/reload every time will bring some performance penalty,
as loading/unloading KUnit for every individual test case will increase the
time to execute such tests.

Ok, if are there filtering rules on IGT limiting what subtests will be
executed, then be it.  But for normal execution, I can't see why we
would be adding a performance penalty.

Regards,
Mauro

> 
> Signed-off-by: Janusz Krzysztofik 
> ---
>  lib/igt_kmod.c | 135 -
>  1 file changed, 31 insertions(+), 104 deletions(-)
> 
> diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> index 37591b7389..5bd8f56088 100644
> --- a/lib/igt_kmod.c
> +++ b/lib/igt_kmod.c
> @@ -960,6 +960,7 @@ static int kunit_kmsg_get_results(struct igt_list_head 
> *results,
>  {
>   char *suite_name = NULL, *case_name = NULL;
>   struct igt_ktap_results *ktap;
> + unsigned long taints;
>   int flags, err;
>  
>   if (igt_debug_on(tst->kmsg < 0))
> @@ -998,6 +999,11 @@ static int kunit_kmsg_get_results(struct igt_list_head 
> *results,
>   if (err)
>   goto out_kmod_unload;
>  
> + if (igt_debug_on(igt_kernel_tainted())) {
> + err = -ENOTRECOVERABLE;
> + goto out_remove_module;
> + }
> +
>   ktap = igt_ktap_alloc(results);
>   if (igt_debug_on(!ktap)) {
>   err = -ENOMEM;
> @@ -1203,84 +1209,43 @@ static void __igt_kunit(struct igt_ktest *tst,
>   const char *opts,
>   struct igt_list_head *tests)
>  {
> - struct modprobe_data modprobe = { tst->kmod, opts, 0, pthread_self(), };
> - char *suite_name = NULL, *case_name = NULL;
> - struct igt_ktap_result *t, *r = NULL;
> - struct igt_ktap_results *ktap;
> - pthread_mutexattr_t attr;
> - IGT_LIST_HEAD(results);
> - int ret = -EINPROGRESS;
> - unsigned long taints;
> -
> - igt_skip_on(lseek(tst->kmsg, 0, SEEK_END) < 0);
> -
> - ktap = igt_ktap_alloc();
> - igt_require(ktap);
> + struct igt_ktap_result *t;
>  
>   igt_list_for_each_entry(t, tests, link) {
> + char *suite_name = NULL, *case_name = NULL;
> + IGT_LIST_HEAD(results);
> + unsigned long taints;
> +
>   igt_dynamic_f("%s%s%s",
> strcmp(t->suite_name, name) ?  t->suite_name : "",
> strcmp(t->suite_name, name) ? "-" : "",
> t->case_name) {
> + struct igt_ktap_result *r = NULL;
> + char filter[1024];
> + int expect = 2;
>  
> - if (!modprobe.thread) {
> - igt_assert_eq(pthread_mutexattr_init(), 0);
> - 

Re: [PATCH v8 04/12] dt-bindings: phy: amlogic, g12a-mipi-dphy-analog: drop unneeded reg property and example

[Neil Armstrong]

On 09/11/2023 19:04, Conor Dooley wrote:

On Thu, Nov 09, 2023 at 10:00:05AM +0100, Neil Armstrong wrote:

Now this bindings is referred from amlogic,meson-gx-hhi-sysctrl.yaml and is
documented as a subnode of a simple-mfd, drop the invalid reg property.


I'd expect a note here tbh about how removing reg & relying on being a
subnode of the simple-mfd is safe to do. It looks like your driver
was added at the same time as this binding & it was always documented as
being a child of the simple-mfd system controller, so I'd kinda expect
to see a Fixes tag on this patch..

Am I missing something?


No you're totally right, I'll reword the commit message and add a Fixes tags.

Thanks,
Neil





Also drop the unnecessary example, the top level bindings example should
be enough.

Signed-off-by: Neil Armstrong 
---
  .../bindings/phy/amlogic,g12a-mipi-dphy-analog.yaml  | 12 
  1 file changed, 12 deletions(-)

diff --git 
a/Documentation/devicetree/bindings/phy/amlogic,g12a-mipi-dphy-analog.yaml 
b/Documentation/devicetree/bindings/phy/amlogic,g12a-mipi-dphy-analog.yaml
index c8c83acfb871..81c2654b7e57 100644
--- a/Documentation/devicetree/bindings/phy/amlogic,g12a-mipi-dphy-analog.yaml
+++ b/Documentation/devicetree/bindings/phy/amlogic,g12a-mipi-dphy-analog.yaml
@@ -16,20 +16,8 @@ properties:
"#phy-cells":
  const: 0
  
-  reg:

-maxItems: 1
-
  required:
- compatible
-  - reg
- "#phy-cells"
  
  additionalProperties: false

-
-examples:
-  - |
-phy@0 {
-  compatible = "amlogic,g12a-mipi-dphy-analog";
-  reg = <0x0 0xc>;
-  #phy-cells = <0>;
-};

--
2.34.1

Re: [PATCH v8 02/12] dt-bindings: soc: amlogic, meson-gx-hhi-sysctrl: add example covering meson-axg-hhi-sysctrl

[Neil Armstrong]

On 09/11/2023 18:34, Conor Dooley wrote:

On Thu, Nov 09, 2023 at 10:00:03AM +0100, Neil Armstrong wrote:

Add a thirst example covering the meson-axg-hhi-sysctrl variant and more


What on earth is a thirst example? Some sort of "hysterical raisins"
type of thing?

My confusion about that word aside,
Acked-by: Conor Dooley 


Indeed, I'll fix this bad typo :-)

Thanks,
Neil



Cheers,
Conor.


importantly the phy subnode.

Signed-off-by: Neil Armstrong 
---
  .../soc/amlogic/amlogic,meson-gx-hhi-sysctrl.yaml  | 41 ++
  1 file changed, 41 insertions(+)

diff --git 
a/Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-gx-hhi-sysctrl.yaml
 
b/Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-gx-hhi-sysctrl.yaml
index 16977e4e4357..2edf4ccea845 100644
--- 
a/Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-gx-hhi-sysctrl.yaml
+++ 
b/Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-gx-hhi-sysctrl.yaml
@@ -158,3 +158,44 @@ examples:
  };
  };
  };
+
+bus@ff63c000 {
+compatible = "simple-bus";
+reg = <0xff63c000 0x1c00>;
+#address-cells = <1>;
+#size-cells = <1>;
+ranges = <0x0 0xff63c000 0x1c00>;
+
+system-controller@0 {
+compatible = "amlogic,meson-axg-hhi-sysctrl", "simple-mfd", 
"syscon";
+reg = <0 0x400>;
+
+clock-controller {
+compatible = "amlogic,axg-clkc";
+#clock-cells = <1>;
+clocks = <>;
+clock-names = "xtal";
+};
+
+power-controller {
+compatible = "amlogic,meson-axg-pwrc";
+#power-domain-cells = <1>;
+amlogic,ao-sysctrl = <_AO>;
+
+resets = <_viu>,
+ <_venc>,
+ <_vcbus>,
+ <_vencl>,
+ <_vid_lock>;
+reset-names = "viu", "venc", "vcbus", "vencl", "vid_lock";
+clocks = <_vpu>, <_vapb>;
+clock-names = "vpu", "vapb";
+};
+
+phy {
+compatible = "amlogic,axg-mipi-pcie-analog-phy";
+#phy-cells = <0>;
+status = "disabled";
+};
+};
+};

--
2.34.1

[PATCH] drm/amd/display: clean up some inconsistent indenting

[Jiapeng Chong]

No functional modification involved.

drivers/gpu/drm/amd/amdgpu/../display/dc/dml2/display_mode_util.c:118 
dml_floor() warn: if statement not indented.

Reported-by: Abaci Robot 
Closes: https://bugzilla.openanolis.cn/show_bug.cgi?id=7224
Signed-off-by: Jiapeng Chong 
---
 drivers/gpu/drm/amd/display/dc/dml2/display_mode_util.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/dc/dml2/display_mode_util.c 
b/drivers/gpu/drm/amd/display/dc/dml2/display_mode_util.c
index c247aee89caf..16f4c506a334 100644
--- a/drivers/gpu/drm/amd/display/dc/dml2/display_mode_util.c
+++ b/drivers/gpu/drm/amd/display/dc/dml2/display_mode_util.c
@@ -116,7 +116,7 @@ dml_float_t dml_ceil(dml_float_t x, dml_float_t granularity)
 dml_float_t dml_floor(dml_float_t x, dml_float_t granularity)
 {
if (granularity == 0)
-   return 0;
+   return 0;
//return (dml_float_t) (floor(x / granularity) * granularity);
return (dml_float_t)dcn_bw_floor2(x, granularity);
 }
-- 
2.20.1.7.g153144c

Re: [RFC PATCH v3 04/12] netdev: support binding dma-buf to netdevice

[Yunsheng Lin]

On 2023/11/10 10:59, Mina Almasry wrote:
> On Thu, Nov 9, 2023 at 12:30 AM Paolo Abeni  wrote:
>>
>> I'm trying to wrap my head around the whole infra... the above line is
>> confusing. Why do you increment dma_addr? it will be re-initialized in
>> the next iteration.
>>
> 
> That is just a mistake, sorry. Will remove this increment.

You seems to be combining comments in different thread and replying in
one thread, I am not sure that is a good practice and I almost missed the
reply below as I don't seem to be cc'ed.

> 
> On Thu, Nov 9, 2023 at 1:29 AM Yunsheng Lin  wrote:> 
> >>>
> gen_pool_destroy BUG_ON() if it's not empty at the time of destroying.
> Technically that should never happen, because
> __netdev_devmem_binding_free() should only be called when the refcount
> hits 0, so all the chunks have been freed back to the gen_pool. But,
> just in case, I don't want to crash the server just because I'm
> leaking a chunk... this is a bit of defensive programming that is
> typically frowned upon, but the behavior of gen_pool is so severe I
> think the WARN() + check is warranted here.

 It seems it is pretty normal for the above to happen nowadays because of
 retransmits timeouts, NAPI defer schemes mentioned below:

 https://lkml.kernel.org/netdev/168269854650.2191653.8465259808498269815.stgit@firesoul/

 And currently page pool core handles that by using a workqueue.
>>>
>>> Forgive me but I'm not understanding the concern here.
>>>
>>> __netdev_devmem_binding_free() is called when binding->ref hits 0.
>>>
>>> binding->ref is incremented when an iov slice of the dma-buf is
>>> allocated, and decremented when an iov is freed. So,
>>> __netdev_devmem_binding_free() can't really be called unless all the
>>> iovs have been freed, and gen_pool_size() == gen_pool_avail(),
>>> regardless of what's happening on the page_pool side of things, right?
>>
>> I seems to misunderstand it. In that case, it seems to be about
>> defensive programming like other checking.
>>
>> By looking at it more closely, it seems napi_frag_unref() call
>> page_pool_page_put_many() directly， which means devmem seems to
>> be bypassing the napi_safe optimization.
>>
>> Can napi_frag_unref() reuse napi_pp_put_page() in order to reuse
>> the napi_safe optimization?
>>
> 
> I think it already does. page_pool_page_put_many() is only called if
> !recycle or !napi_pp_put_page(). In that case
> page_pool_page_put_many() is just a replacement for put_page(),
> because this 'page' may be an iov.

Is there a reason why not calling napi_pp_put_page() for devmem too
instead of calling page_pool_page_put_many()? mem provider has a
'release_page' ops, calling page_pool_page_put_many() directly here
seems to be bypassing the 'release_page' ops, which means devmem is
bypassing most of the main features of page pool.

As far as I can tell, the main features of page pool:
1. Allow lockless allocation and freeing in pool->alloc cache by
   utilizing NAPI non-concurrent context.
2. Allow concurrent allocation and freeing in pool->ring cache by
   utilizing ptr_ring.
3. Allow dma map/unmap and cache sync optimization.
4. Allow detailed stats logging and tracing.
5. Allow some bulk allocation and freeing.
6. support both skb packet and xdp frame.

I am wondering what is the main features that devmem is utilizing
by intergrating into page pool?

It seems the driver can just call netdev_alloc_devmem() and
napi_frag_unref() can call netdev_free_devmem() directly without
intergrating into page pool and it should just works too?

Maybe we should consider creating a new thin layer, in order to
demux to page pool, devmem or other mem type if my suggestion does
not work out too?

> 

[PATCH 2/2] drm/virtio: Modify RESOURCE_GET_LAYOUT ioctl

[Julia Zhang]

Modify RESOURCE_GET_LAYOUT ioctl to handle the use case that query
correct stride for guest linear resource before it is created.

Signed-off-by: Julia Zhang 
---
 drivers/gpu/drm/virtio/virtgpu_drv.h   | 26 --
 drivers/gpu/drm/virtio/virtgpu_ioctl.c | 47 --
 drivers/gpu/drm/virtio/virtgpu_vq.c| 35 +++
 include/uapi/drm/virtgpu_drm.h |  6 ++--
 include/uapi/linux/virtio_gpu.h|  8 ++---
 5 files changed, 66 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h 
b/drivers/gpu/drm/virtio/virtgpu_drv.h
index d6fc0d4ecb7d..82dffb3e4c6b 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -93,15 +93,6 @@ struct virtio_gpu_object {
bool host3d_blob, guest_blob;
uint32_t blob_mem, blob_flags;
 
-   atomic_t layout_state;
-   struct {
-   uint64_t offset;
-   uint64_t size;
-   uint32_t stride;
-   } planes[VIRTIO_GPU_RES_MAX_PLANES];
-   uint64_t modifier;
-   uint32_t num_planes;
-
int uuid_state;
uuid_t uuid;
 
@@ -225,6 +216,16 @@ struct virtio_gpu_drv_cap_cache {
atomic_t is_valid;
 };
 
+struct virtio_gpu_query_info {
+   uint32_t num_planes;
+   uint64_t modifier;
+   struct {
+   uint64_t offset;
+   uint32_t stride;
+   } planes [VIRTIO_GPU_MAX_RESOURCE_PLANES];
+   atomic_t is_valid;
+};
+
 struct virtio_gpu_device {
struct drm_device *ddev;
 
@@ -448,7 +449,12 @@ void virtio_gpu_cmd_host_wait(struct virtio_gpu_device 
*vgdev,
 
 int
 virtio_gpu_cmd_get_resource_layout(struct virtio_gpu_device *vgdev,
-  struct virtio_gpu_object *bo);
+  struct virtio_gpu_query_info *bo_info,
+  uint32_t width,
+  uint32_t height,
+  uint32_t format,
+  uint32_t bind,
+  uint32_t hw_res_handle);
 
 /* virtgpu_display.c */
 int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev);
diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c 
b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index 51d04460d0d8..034a7c0927a5 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -685,9 +685,9 @@ static int virtio_gpu_resource_query_layout_ioctl(struct 
drm_device *dev,
 {
struct drm_virtgpu_resource_query_layout *args = data;
struct virtio_gpu_device *vgdev = dev->dev_private;
-   struct drm_gem_object *obj;
-   struct virtio_gpu_object *bo;
-   int layout_state;
+   struct drm_gem_object *obj = NULL;
+   struct virtio_gpu_object *bo = NULL;
+   struct virtio_gpu_query_info bo_info = {0};
int ret = 0;
int i;
 
@@ -696,50 +696,45 @@ static int virtio_gpu_resource_query_layout_ioctl(struct 
drm_device *dev,
return -EINVAL;
}
 
-   obj = drm_gem_object_lookup(file, args->handle);
-   if (obj == NULL) {
-   DRM_ERROR("invalid handle 0x%x\n", args->handle);
-   return -ENOENT;
-   }
-   bo = gem_to_virtio_gpu_obj(obj);
-
-   layout_state = atomic_read(>layout_state);
-   if (layout_state == STATE_ERR) {
-   ret = -EINVAL;
-   goto out;
-   } else if (layout_state == STATE_OK) {
-   goto valid;
+   if (args->handle > 0) {
+   obj = drm_gem_object_lookup(file, args->handle);
+   if (obj == NULL) {
+   DRM_ERROR("invalid handle 0x%x\n", args->handle);
+   return -ENOENT;
+   }
+   bo = gem_to_virtio_gpu_obj(obj);
}
 
-   ret = virtio_gpu_cmd_get_resource_layout(vgdev, bo);
+   ret = virtio_gpu_cmd_get_resource_layout(vgdev, _info, args->width,
+args->height, args->format,
+args->bind, bo ? 
bo->hw_res_handle : 0);
if (ret)
goto out;
 
ret = wait_event_timeout(vgdev->resp_wq,
-atomic_read(>layout_state) == STATE_OK,
+atomic_read(_info.is_valid),
 5 * HZ);
if (!ret)
goto out;
 
 valid:
smp_rmb();
-   WARN_ON(atomic_read(>layout_state) != STATE_OK);
-   args->num_planes = bo->num_planes;
-   args->modifier = bo->modifier;
+   WARN_ON(atomic_read(_info.is_valid));
+   args->num_planes = bo_info.num_planes;
+   args->modifier = bo_info.modifier;
for (i = 0; i < args->num_planes; i++) {
-   args->planes[i].offset = bo->planes[i].offset;
-   args->planes[i].size = bo->planes[i].size;
-   args->planes[i].stride = 

[PATCH 1/2] drm/virtio: Implement RESOURCE_GET_LAYOUT ioctl

[Julia Zhang]

From: Daniel Stone 

This ioctl allows the guest to discover how the guest actually allocated
the underlying buffer, which allows buffers to be used for GL<->Vulkan
interop and through standard window systems. It's also a step towards
properly supporting modifiers in the guest.
---
 drivers/gpu/drm/virtio/virtgpu_drv.c   |  1 +
 drivers/gpu/drm/virtio/virtgpu_drv.h   | 16 +-
 drivers/gpu/drm/virtio/virtgpu_ioctl.c | 71 ++
 drivers/gpu/drm/virtio/virtgpu_kms.c   |  8 ++-
 drivers/gpu/drm/virtio/virtgpu_vq.c| 56 
 include/uapi/drm/virtgpu_drm.h | 19 +++
 include/uapi/linux/virtio_gpu.h| 30 +++
 7 files changed, 198 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c 
b/drivers/gpu/drm/virtio/virtgpu_drv.c
index 4f7140e27614..1ee09974d4b7 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
@@ -190,6 +190,7 @@ static unsigned int features[] = {
VIRTIO_GPU_F_RESOURCE_BLOB,
VIRTIO_GPU_F_CONTEXT_INIT,
VIRTIO_GPU_F_CONTEXT_FENCE_WAIT,
+   VIRTIO_GPU_F_RESOURCE_QUERY_LAYOUT,
 };
 static struct virtio_driver virtio_gpu_driver = {
.feature_table = features,
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h 
b/drivers/gpu/drm/virtio/virtgpu_drv.h
index 7ef4b3df0ada..d6fc0d4ecb7d 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -93,6 +93,15 @@ struct virtio_gpu_object {
bool host3d_blob, guest_blob;
uint32_t blob_mem, blob_flags;
 
+   atomic_t layout_state;
+   struct {
+   uint64_t offset;
+   uint64_t size;
+   uint32_t stride;
+   } planes[VIRTIO_GPU_RES_MAX_PLANES];
+   uint64_t modifier;
+   uint32_t num_planes;
+
int uuid_state;
uuid_t uuid;
 
@@ -249,6 +258,7 @@ struct virtio_gpu_device {
bool has_host_visible;
bool has_context_init;
bool has_host_fence_wait;
+   bool has_resource_query_layout;
struct virtio_shm_region host_visible_region;
struct drm_mm host_visible_mm;
 
@@ -281,7 +291,7 @@ struct virtio_gpu_fpriv {
 };
 
 /* virtgpu_ioctl.c */
-#define DRM_VIRTIO_NUM_IOCTLS 12
+#define DRM_VIRTIO_NUM_IOCTLS 13
 extern struct drm_ioctl_desc virtio_gpu_ioctls[DRM_VIRTIO_NUM_IOCTLS];
 void virtio_gpu_create_context(struct drm_device *dev, struct drm_file *file);
 
@@ -436,6 +446,10 @@ int virtio_gpu_cmd_status_freezing(struct 
virtio_gpu_device *vgdev,
 void virtio_gpu_cmd_host_wait(struct virtio_gpu_device *vgdev,
  uint32_t ctx_id, uint64_t fence_id);
 
+int
+virtio_gpu_cmd_get_resource_layout(struct virtio_gpu_device *vgdev,
+  struct virtio_gpu_object *bo);
+
 /* virtgpu_display.c */
 int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev);
 void virtio_gpu_modeset_fini(struct virtio_gpu_device *vgdev);
diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c 
b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index b6079d2bff69..51d04460d0d8 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -107,6 +107,9 @@ static int virtio_gpu_getparam_ioctl(struct drm_device 
*dev, void *data,
case VIRTGPU_PARAM_SUPPORTED_CAPSET_IDs:
value = vgdev->capset_id_mask;
break;
+   case VIRTGPU_PARAM_RESOURCE_QUERY_LAYOUT:
+   value = vgdev->has_resource_query_layout ? 1 : 0;
+   break;
default:
return -EINVAL;
}
@@ -676,6 +679,70 @@ static int virtio_gpu_context_init_ioctl(struct drm_device 
*dev,
return ret;
 }
 
+static int virtio_gpu_resource_query_layout_ioctl(struct drm_device *dev,
+ void *data,
+ struct drm_file *file)
+{
+   struct drm_virtgpu_resource_query_layout *args = data;
+   struct virtio_gpu_device *vgdev = dev->dev_private;
+   struct drm_gem_object *obj;
+   struct virtio_gpu_object *bo;
+   int layout_state;
+   int ret = 0;
+   int i;
+
+   if (!vgdev->has_resource_query_layout) {
+   DRM_ERROR("failing: no RQL on host\n");
+   return -EINVAL;
+   }
+
+   obj = drm_gem_object_lookup(file, args->handle);
+   if (obj == NULL) {
+   DRM_ERROR("invalid handle 0x%x\n", args->handle);
+   return -ENOENT;
+   }
+   bo = gem_to_virtio_gpu_obj(obj);
+
+   layout_state = atomic_read(>layout_state);
+   if (layout_state == STATE_ERR) {
+   ret = -EINVAL;
+   goto out;
+   } else if (layout_state == STATE_OK) {
+   goto valid;
+   }
+
+   ret = virtio_gpu_cmd_get_resource_layout(vgdev, bo);
+   if (ret)
+   goto out;
+
+   ret = wait_event_timeout(vgdev->resp_wq,
+ 

[PATCH 0/2] Add RESOURCE_GET_LAYOUT ioctl

[Julia Zhang]

This is to add a new ioctl RESOURCE_GET_LAYOUT to virtio-gpu to get the
information about how the host has actually allocated the buffer. It is
implemented to query the stride of linear buffer for dGPU prime on guest VM,
related mesa mr: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/23896

Daniel Stone (1):
  drm/virtio: Implement RESOURCE_GET_LAYOUT ioctl

Julia Zhang (1):
  drm/virtio: Modify RESOURCE_GET_LAYOUT ioctl

 drivers/gpu/drm/virtio/virtgpu_drv.c   |  1 +
 drivers/gpu/drm/virtio/virtgpu_drv.h   | 22 -
 drivers/gpu/drm/virtio/virtgpu_ioctl.c | 66 ++
 drivers/gpu/drm/virtio/virtgpu_kms.c   |  8 +++-
 drivers/gpu/drm/virtio/virtgpu_vq.c| 63 
 include/uapi/drm/virtgpu_drm.h | 21 
 include/uapi/linux/virtio_gpu.h| 30 
 7 files changed, 208 insertions(+), 3 deletions(-)

-- 
2.34.1

Re: [PATCH 0/2] fb: amifb: Convert to platform remove callback returning void

[Helge Deller]

On 11/9/23 23:01, Uwe Kleine-König wrote:

in the series 
https://lore.kernel.org/20231107091740.3924258-1-u.kleine-koe...@pengutronix.de
I suggested to replace module_platform_driver_probe() in the amifb
driver. Geert Uytterhoeven objected because the amiga platform has very
little RAM. This series implements the alternative: Mark the driver
struct with __refdata to suppress the section warning. Patch #2 is just
a rebase from the above series.


Thanks for the follow-up!
I've applied both patches.

Helge

Re: [REGRESSION]: acpi/nouveau: Hardware unavailable upon resume or suspend fails

[Kai-Heng Feng]

Hi Owen,

On Fri, Nov 10, 2023 at 5:55 AM Owen T. Heisler  wrote:
>
> #regzbot introduced: 89c290ea758911e660878e26270e084d862c03b0
> #regzbot link: https://gitlab.freedesktop.org/drm/nouveau/-/issues/273
> #regzbot link: https://bugzilla.kernel.org/show_bug.cgi?id=218124

Thanks for the bug report. Do you prefer to continue the discussion
here, on gitlab or on bugzilla?

>
> ## Reproducing
>
> 1. Boot system to framebuffer console.
> 2. Run `systemctl suspend`. If undocked without secondary display,
> suspend fails. If docked with secondary display, suspend succeeds.
> 3. Resume from suspend if applicable.
> 4. System is now in a broken state.

So I guess we need to put those devices to ACPI D3 for suspend. Let's
discuss this on your preferred platform.

Kai-Heng

>
> ## Testing
>
> - culprit commit is 89c290ea758911e660878e26270e084d862c03b0
> - v6.6 fails
> - v6.6 with culprit commit reverted does not fail
> - Compiled with
> 
>
> ## Hardware
>
> - ThinkPad W530 2438-52U
> - Dock with Nvidia-connected DVI ports
> - Secondary display connected via DVI
> - Nvidia Optimus GPU switching system
>
> ```console
> $ lspci | grep -i vga
> 00:02.0 VGA compatible controller: Intel Corporation 3rd Gen Core
> processor Graphics Controller (rev 09)
> 01:00.0 VGA compatible controller: NVIDIA Corporation GK107GLM [Quadro
> K2000M] (rev a1)
> ```
>
> ## Decoded logs from v6.6
>
> - System is not docked and fails to suspend:
> 
> - System is docked and fails after resume:
> 

[PATCH] drm/qxl: remove unused declaration

[heminhong]

Some functions are never used by the driver,
removing the functions declaration, it can be reducing program size,
and improving code readability and maintainability.

Signed-off-by: heminhong 
---
 drivers/gpu/drm/qxl/qxl_drv.h | 7 ---
 1 file changed, 7 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index 307a890fde13..32069acd93f8 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -119,7 +119,6 @@ struct qxl_output {
 
 #define to_qxl_crtc(x) container_of(x, struct qxl_crtc, base)
 #define drm_connector_to_qxl_output(x) container_of(x, struct qxl_output, base)
-#define drm_encoder_to_qxl_output(x) container_of(x, struct qxl_output, enc)
 
 struct qxl_mman {
struct ttm_device   bdev;
@@ -256,8 +255,6 @@ struct qxl_device {
 
 #define to_qxl(dev) container_of(dev, struct qxl_device, ddev)
 
-int qxl_debugfs_fence_init(struct qxl_device *rdev);
-
 int qxl_device_init(struct qxl_device *qdev, struct pci_dev *pdev);
 void qxl_device_fini(struct qxl_device *qdev);
 
@@ -344,8 +341,6 @@ qxl_image_alloc_objects(struct qxl_device *qdev,
int height, int stride);
 void qxl_image_free_objects(struct qxl_device *qdev, struct qxl_drm_image 
*dimage);
 
-void qxl_update_screen(struct qxl_device *qxl);
-
 /* qxl io operations (qxl_cmd.c) */
 
 void qxl_io_create_primary(struct qxl_device *qdev,
@@ -445,8 +440,6 @@ int qxl_hw_surface_dealloc(struct qxl_device *qdev,
 
 int qxl_bo_check_id(struct qxl_device *qdev, struct qxl_bo *bo);
 
-struct qxl_drv_surface *
-qxl_surface_lookup(struct drm_device *dev, int surface_id);
 void qxl_surface_evict(struct qxl_device *qdev, struct qxl_bo *surf, bool 
freeing);
 
 /* qxl_ioctl.c */
-- 
2.25.1

Re: [PATCH 4/5] accel/ivpu: Use dedicated work for job timeout detection

[Jeffrey Hugo]

On 11/7/2023 5:35 AM, Jacek Lawrynowicz wrote:

From: Stanislaw Gruszka 

Change to use work for timeout detection. Needed for thread_irq
conversion.

Signed-off-by: Stanislaw Gruszka 


Missing SOB.

  
  void ivpu_pm_cancel_recovery(struct ivpu_device *vdev)

  {
+   drm_WARN_ON(>drm, 
delayed_work_pending(>pm->job_timeout_work));


This seems odd.  Looks like this function is only called from the 
dev_fini.  It's a non-fatal error to tear down the device (hotplug? 
fatal error?) if a job is pending?


-Jeff

Re: [PATCH 3/5] accel/ivpu: Do not use cons->aborted for job_done_thread

[Jeffrey Hugo]

On 11/7/2023 5:35 AM, Jacek Lawrynowicz wrote:

From: Stanislaw Gruszka 

This allow to simplify ivpu_ipc_receive() as now we do not have
to process all messages in aborted state - they will be freed in
ivpu_ipc_consumer_del().

Signed-off-by: Stanislaw Gruszka 


Missing SOB.

-Jeff

Re: [RFC PATCH v3 08/12] net: support non paged skb frags

[Mina Almasry]

On Thu, Nov 9, 2023 at 1:15 AM Paolo Abeni  wrote:
>
> On Sun, 2023-11-05 at 18:44 -0800, Mina Almasry wrote:
> [...]
> > @@ -3421,7 +3446,7 @@ static inline struct page *skb_frag_page(const 
> > skb_frag_t *frag)
> >   */
> >  static inline void __skb_frag_ref(skb_frag_t *frag)
> >  {
> > - get_page(skb_frag_page(frag));
> > + page_pool_page_get_many(frag->bv_page, 1);
>
> I guess the above needs #ifdef CONFIG_PAGE_POOL guards and explicit
> skb_frag_is_page_pool_iov() check ?
>

It doesn't actually. page_pool_page_* helpers are compiled in
regardless of CONFIG_PAGE_POOL, and handle both page_pool_iov* & page*
just fine (the checking happens inside the function).

You may yell at me that it's too confusing... I somewhat agree, but
I'm unsure of what is a better name or location for the helpers. The
helpers handle (page_pool_iov* || page*) gracefully, so they seem to
belong in the page pool for me, but it is indeed surprising/confusing
that these helpers are available even if !CONFIG_PAGE_POOL.

>
> Cheers,
>
> Paolo
>
>


-- 
Thanks,
Mina

Re: [PATCH 2/5] accel/ivpu: Do not use irqsave in ivpu_ipc_dispatch

[Jeffrey Hugo]

On 11/7/2023 5:35 AM, Jacek Lawrynowicz wrote:

From: Stanislaw Gruszka 

ivpu_ipc_dispatch is always called with irqs disabled. Add lockdep
assertion and remove unneeded _irqsave/_irqrestore.

Signed-off-by: Stanislaw Gruszka 


Missing SOB.

-Jeff

Re: [PATCH 1/5] accel/ivpu: Rename cons->rx_msg_lock

[Jeffrey Hugo]

On 11/7/2023 5:35 AM, Jacek Lawrynowicz wrote:

From: Stanislaw Gruszka 

Now the cons->rx_msg_lock protects also 'abort' field so rename the lock.

Signed-off-by: Stanislaw Gruszka 


Jacek, this is missing your SOB.

-Jeff

Re: [RFC PATCH v3 04/12] netdev: support binding dma-buf to netdevice

[Mina Almasry]

On Thu, Nov 9, 2023 at 12:30 AM Paolo Abeni  wrote:
>
> I'm trying to wrap my head around the whole infra... the above line is
> confusing. Why do you increment dma_addr? it will be re-initialized in
> the next iteration.
>

That is just a mistake, sorry. Will remove this increment.

On Thu, Nov 9, 2023 at 1:29 AM Yunsheng Lin  wrote:> >>>
> >>> gen_pool_destroy BUG_ON() if it's not empty at the time of destroying.
> >>> Technically that should never happen, because
> >>> __netdev_devmem_binding_free() should only be called when the refcount
> >>> hits 0, so all the chunks have been freed back to the gen_pool. But,
> >>> just in case, I don't want to crash the server just because I'm
> >>> leaking a chunk... this is a bit of defensive programming that is
> >>> typically frowned upon, but the behavior of gen_pool is so severe I
> >>> think the WARN() + check is warranted here.
> >>
> >> It seems it is pretty normal for the above to happen nowadays because of
> >> retransmits timeouts, NAPI defer schemes mentioned below:
> >>
> >> https://lkml.kernel.org/netdev/168269854650.2191653.8465259808498269815.stgit@firesoul/
> >>
> >> And currently page pool core handles that by using a workqueue.
> >
> > Forgive me but I'm not understanding the concern here.
> >
> > __netdev_devmem_binding_free() is called when binding->ref hits 0.
> >
> > binding->ref is incremented when an iov slice of the dma-buf is
> > allocated, and decremented when an iov is freed. So,
> > __netdev_devmem_binding_free() can't really be called unless all the
> > iovs have been freed, and gen_pool_size() == gen_pool_avail(),
> > regardless of what's happening on the page_pool side of things, right?
>
> I seems to misunderstand it. In that case, it seems to be about
> defensive programming like other checking.
>
> By looking at it more closely, it seems napi_frag_unref() call
> page_pool_page_put_many() directly， which means devmem seems to
> be bypassing the napi_safe optimization.
>
> Can napi_frag_unref() reuse napi_pp_put_page() in order to reuse
> the napi_safe optimization?
>

I think it already does. page_pool_page_put_many() is only called if
!recycle or !napi_pp_put_page(). In that case
page_pool_page_put_many() is just a replacement for put_page(),
because this 'page' may be an iov.

-- 
Thanks,
Mina

Re: [Intel-gfx] [PATCH 2/2] drm/i915/guc: Add a selftest for FAST_REQUEST errors

[John Harrison]

On 11/9/2023 12:33, Daniele Ceraolo Spurio wrote:

On 11/6/2023 3:59 PM, john.c.harri...@intel.com wrote:

From: John Harrison 

There is a mechanism for reporting errors from fire and forget H2G
messages. This is the only way to find out about almost any error in
the GuC backend submission path. So it would be useful to know that it
is working.

Signed-off-by: John Harrison 
---
  drivers/gpu/drm/i915/gt/uc/intel_guc.h    |   4 +
  drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c |   9 ++
  drivers/gpu/drm/i915/gt/uc/selftest_guc.c | 122 ++
  3 files changed, 135 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h 
b/drivers/gpu/drm/i915/gt/uc/intel_guc.h

index 2b6dfe62c8f2a..e22c12ce245ad 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
@@ -297,6 +297,10 @@ struct intel_guc {
   * @number_guc_id_stolen: The number of guc_ids that have been 
stolen

   */
  int number_guc_id_stolen;
+    /**
+ * @fast_response_selftest: Backdoor to CT handler for fast 
response selftest

+ */
+    u32 fast_response_selftest;
  #endif
  };
  diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c 
b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c

index 89e314b3756bb..9d958afb78b7f 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
@@ -1076,6 +1076,15 @@ static int ct_handle_response(struct 
intel_guc_ct *ct, struct ct_incoming_msg *r

  found = true;
  break;
  }
+
+#ifdef CONFIG_DRM_I915_SELFTEST
+    if (!found && ct_to_guc(ct)->fast_response_selftest) {
+    CT_DEBUG(ct, "Assuming unsolicited response due to 
FAST_REQUEST selftest\n");

+    ct_to_guc(ct)->fast_response_selftest++;
+    found = 1;


found = true ? it's the same thing, but it's cleaner to assign boolean 
values to bool variables

Doh.




+    }
+#endif
+
  if (!found) {
  CT_ERROR(ct, "Unsolicited response message: len %u, data 
%#x (fence %u, last %u)\n",

   len, hxg[0], fence, ct->requests.last_fence);
diff --git a/drivers/gpu/drm/i915/gt/uc/selftest_guc.c 
b/drivers/gpu/drm/i915/gt/uc/selftest_guc.c

index bfb72143566f6..97fbbb396336c 100644
--- a/drivers/gpu/drm/i915/gt/uc/selftest_guc.c
+++ b/drivers/gpu/drm/i915/gt/uc/selftest_guc.c
@@ -286,11 +286,133 @@ static int intel_guc_steal_guc_ids(void *arg)
  return ret;
  }
  +/*
+ * Send a context schedule H2G message with an invalid context id.
+ * This should generate a GUC_RESULT_INVALID_CONTEXT response.
+ */
+static int bad_h2g(struct intel_guc *guc)
+{
+    u32 action[3], len = 0;


AFAICS This is a 2 DW command, so you can use action[2].

Yup. Copy and paste bug.




+
+    action[len++] = INTEL_GUC_ACTION_SCHED_CONTEXT;
+    action[len++] = 0x12345678;
+
+    return intel_guc_send_nb(guc, action, len, 0);
+}
+
+/*
+ * Set a spinner running to make sure the system is alive and active,
+ * then send a bad but asynchronous H2G command and wait to see if an
+ * error response is returned. If no response is received or if the
+ * spinner dies then the test will fail.
+ */
+#define FAST_RESPONSE_TIMEOUT_MS    1000
+static int intel_guc_fast_request(void *arg)
+{
+    struct intel_gt *gt = arg;
+    struct intel_context *ce;
+    struct igt_spinner spin;
+    struct i915_request *rq;
+    intel_wakeref_t wakeref;
+    struct intel_engine_cs *engine = 
intel_selftest_find_any_engine(gt);

+    ktime_t before, now, delta;
+    bool spinning = false;
+    u64 delta_ms;
+    int ret = 0;
+
+    if (!engine)
+    return 0;
+
+    wakeref = intel_runtime_pm_get(gt->uncore->rpm);
+
+    ce = intel_context_create(engine);
+    if (IS_ERR(ce)) {
+    ret = PTR_ERR(ce);
+    gt_err(gt, "Failed to create spinner request: %pe\n", ce);
+    goto err_pm;
+    }
+
+    ret = igt_spinner_init(, engine->gt);
+    if (ret) {
+    gt_err(gt, "Failed to create spinner: %pe\n", ERR_PTR(ret));
+    goto err_pm;
+    }
+    spinning = true;
+
+    rq = igt_spinner_create_request(, ce, MI_ARB_CHECK);
+    intel_context_put(ce);
+    if (IS_ERR(rq)) {
+    ret = PTR_ERR(rq);
+    gt_err(gt, "Failed to create spinner request: %pe\n", rq);
+    goto err_spin;
+    }
+
+    ret = request_add_spin(rq, );
+    if (ret) {
+    gt_err(gt, "Failed to add Spinner request: %pe\n", 
ERR_PTR(ret));

+    goto err_rq;
+    }
+
+    gt->uc.guc.fast_response_selftest = 1;
+
+    ret = bad_h2g(>uc.guc);
+    if (ret) {
+    gt_err(gt, "Failed to send H2G: %pe\n", ERR_PTR(ret));
+    goto err_rq;
+    }
+
+    before = ktime_get();
+    while (gt->uc.guc.fast_response_selftest == 1) {
+    ret = i915_request_wait(rq, 0, 1);
+    if (ret != -ETIME) {
+    gt_err(gt, "Request wait failed: %pe\n", ERR_PTR(ret));
+    goto err_rq;
+    }
+    now = ktime_get();
+    delta = ktime_sub(now, before);
+    delta_ms = ktime_to_ms(delta);
+
+    if 

Re: [PATCH 1/1] drm/mediatek: Fix access violation in mtk_drm_crtc_dma_dev_get

[胡俊光]

[PATCH v2 RESEND] drm/panel: starry-2081101qfh032011-53g: Fine tune the panel power sequence

[xiazhengqiao]

For the "starry, 2081101qfh032011-53g" panel, it is stipulated in the
panel spec that MIPI needs to keep the LP11 state before the
lcm_reset pin is pulled high.

Fixes: 6069b66cd962 ("drm/panel: support for STARRY 2081101QFH032011-53G 
MIPI-DSI panel")
Signed-off-by: xiazhengqiao 
Reviewed-by: Jessica Zhang 
---
 drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c 
b/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c
index 4f370bc6dca8..4ed8c2e28d37 100644
--- a/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c
+++ b/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c
@@ -1765,6 +1765,7 @@ static const struct panel_desc starry_qfh032011_53g_desc 
= {
.mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
  MIPI_DSI_MODE_LPM,
.init_cmds = starry_qfh032011_53g_init_cmd,
+   .lp11_before_reset = true,
 };
 
 static const struct drm_display_mode starry_himax83102_j02_default_mode = {
-- 
2.17.1

Re: [PATCH drm-misc-next v6] drm/sched: implement dynamic job-flow control

[Luben Tuikov]

On 2023-11-09 19:57, Luben Tuikov wrote:
> On 2023-11-09 19:16, Danilo Krummrich wrote:
[snip]
>> @@ -667,6 +771,8 @@ EXPORT_SYMBOL(drm_sched_resubmit_jobs);
>>   * drm_sched_job_init - init a scheduler job
>>   * @job: scheduler job to init
>>   * @entity: scheduler entity to use
>> + * @credits: the number of credits this job contributes to the schedulers
>> + * credit limit
>>   * @owner: job owner for debugging
>>   *
>>   * Refer to drm_sched_entity_push_job() documentation
>> @@ -684,7 +790,7 @@ EXPORT_SYMBOL(drm_sched_resubmit_jobs);
>>   */
>>  int drm_sched_job_init(struct drm_sched_job *job,
>> struct drm_sched_entity *entity,
>> -   void *owner)
>> +   u32 credits, void *owner)
>>  {
>>  if (!entity->rq) {
>>  /* This will most likely be followed by missing frames
>> @@ -695,7 +801,11 @@ int drm_sched_job_init(struct drm_sched_job *job,
>>  return -ENOENT;
>>  }
>>  
>> +if (unlikely(!credits))
>> +return -EINVAL;
>> +
> 
> This will most likely result in bad user experience (read: blank screen),
> and debugging this would be really hard without something to go by
> in the kernel log.
> 
> (This was exactly the case with amdgpu when 56e449603f0ac5
> ("drm/sched: Convert the GPU scheduler to variable number of run-queues") 
> was being worked on and merged. Without the drm_err() on missing rq in
> the lines immediately before the hunk above returning -ENOENT, there
> was no indication why setting up an fb was failing very early on (blank 
> screen).)
> 
> So it is best to print a "[drm] *ERROR* "-equivalent string in the logs,
> so that we can make a note of this, without relying on drivers, old and new, 
> logging
> that drm_sched_job_init() failed.

If you add _exactly_ this,

if (unlikely(!credits)) {
pr_err("*ERROR* %s: credits cannot be 0!\n", __func__)
return -EINVAL;
}

You can add my,

Reviewed-by: Luben Tuikov 

and push it.
-- 
Regards,
Luben


OpenPGP_0x4C15479431A334AF.asc
Description: OpenPGP public key


OpenPGP_signature.asc
Description: OpenPGP digital signature

Re: [Intel-gfx] [PATCH] drm/i915: Read a shadowed mmio register for ggtt flush

[Belgaumkar, Vinay]

On 11/9/2023 12:35 PM, Ville Syrjälä wrote:

On Thu, Nov 09, 2023 at 12:01:26PM -0800, Belgaumkar, Vinay wrote:

On 11/9/2023 11:30 AM, Ville Syrjälä wrote:

On Thu, Nov 09, 2023 at 11:21:48AM -0800, Vinay Belgaumkar wrote:

We read RENDER_HEAD as a part of the flush. If GT is in
deeper sleep states, this could lead to read errors since we are
not using a forcewake. Safer to read a shadowed register instead.

IIRC shadowing is only thing for writes, not reads.

Sure, but reading from a shadowed register does return the cached value

Does it? I suppose that would make some sense, but I don't recall that
ever being stated anywhere. At least before the shadow registers
existed reads would just give you zeroes when not awake.


(even though we don't care about the vakue here). When GT is in deeper
sleep states, it is better to read a shadowed (cached) value instead of
trying to attempt an mmio register read without a force wake anyways.

So you're saying reads from non-shadowed registers fails somehow
when not awake? How exactly do they fail? And when reading from
a shadowed register that failure never happens?


We could hit problems like the one being addressed here - 
https://patchwork.freedesktop.org/series/125356/.  Reading from a 
shadowed register will avoid any needless references(without a wake) to 
the MMIO space. Shouldn't hurt to make this change for all gens IMO.


Thanks,

Vinay.




Thanks,

Vinay.


Cc: John Harrison 
Cc: Daniele Ceraolo Spurio 
Signed-off-by: Vinay Belgaumkar 
---
   drivers/gpu/drm/i915/gt/intel_gt.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c 
b/drivers/gpu/drm/i915/gt/intel_gt.c
index ed32bf5b1546..ea814ea5f700 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -451,7 +451,7 @@ void intel_gt_flush_ggtt_writes(struct intel_gt *gt)
   
   		spin_lock_irqsave(>lock, flags);

intel_uncore_posting_read_fw(uncore,
-RING_HEAD(RENDER_RING_BASE));
+RING_TAIL(RENDER_RING_BASE));
spin_unlock_irqrestore(>lock, flags);
}
   }
--
2.38.1

Re: [PATCH] drm/sched: Define pr_fmt() for DRM using pr_*()

[Danilo Krummrich]

On 11/10/23 01:27, Luben Tuikov wrote:

Define pr_fmt() as "[drm] " for DRM code using pr_*() facilities, especially
when no devices are available. This makes it easier to browse kernel logs.

Signed-off-by: Luben Tuikov 
---
  include/drm/drm_print.h | 14 ++
  1 file changed, 14 insertions(+)

diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index a93a387f8a1a15..e8fe60d0eb8783 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -26,6 +26,20 @@
  #ifndef DRM_PRINT_H_
  #define DRM_PRINT_H_
  
+/* Define this before including linux/printk.h, so that the format

+ * string in pr_*() macros is correctly set for DRM. If a file wants
+ * to define this to something else, it should do so before including
+ * this header file.


There are a couple occurrences of pr_fmt in drm code, but it seems like
all of those are correctly defined before any includes.

Acked-by: Danilo Krummrich 


+ *
+ * It is encouraged code using pr_err() to prefix their format with
+ * the string "*ERROR* ", to make it easier to scan kernel logs. For
+ * instance,
+ *   pr_err("*ERROR* ", args).
+ */
+#ifndef pr_fmt
+#define pr_fmt(fmt) "[drm] " fmt
+#endif
+
  #include 
  #include 
  #include 

base-commit: f3123c2590005c5ff631653d31428e40cd10c618

Re: [PATCH drm-misc-next v6] drm/sched: implement dynamic job-flow control

[Luben Tuikov]

On 2023-11-09 19:16, Danilo Krummrich wrote:
> Currently, job flow control is implemented simply by limiting the number
> of jobs in flight. Therefore, a scheduler is initialized with a credit
> limit that corresponds to the number of jobs which can be sent to the
> hardware.
> 
> This implies that for each job, drivers need to account for the maximum
> job size possible in order to not overflow the ring buffer.
> 
> However, there are drivers, such as Nouveau, where the job size has a
> rather large range. For such drivers it can easily happen that job
> submissions not even filling the ring by 1% can block subsequent
> submissions, which, in the worst case, can lead to the ring run dry.
> 
> In order to overcome this issue, allow for tracking the actual job size
> instead of the number of jobs. Therefore, add a field to track a job's
> credit count, which represents the number of credits a job contributes
> to the scheduler's credit limit.
> 
> Signed-off-by: Danilo Krummrich 
> ---
> Changes in V2:
> ==
>   - fixed up influence on scheduling fairness due to consideration of a job's
> size
> - If we reach a ready entity in drm_sched_select_entity() but can't 
> actually
>   queue a job from it due to size limitations, just give up and go to 
> sleep
>   until woken up due to a pending job finishing, rather than continue to 
> try
>   other entities.
>   - added a callback to dynamically update a job's credits (Boris)
>   - renamed 'units' to 'credits'
>   - fixed commit message and comments as requested by Luben
> 
> Changes in V3:
> ==
>   - rebased onto V7 of the "DRM scheduler changes for Xe" series by Matt
>   - move up drm_sched_can_queue() instead of adding a forward declaration
> (Boris)
>   - add a drm_sched_available_credits() helper (Boris)
>   - adjust control flow in drm_sched_rq_select_entity_fifo() to Luben's 
> proposal
>   - re-phrase a few comments and fix a typo (Luben)
>   - change naming of all structures credit fields and function parameters to 
> the
> following scheme
> - drm_sched_job::credits
> - drm_gpu_scheduler::credit_limit
> - drm_gpu_scheduler::credit_count
> - drm_sched_init(..., u32 credit_limit, ...)
> - drm_sched_job_init(..., u32 credits, ...)
>   - add proper documentation for the scheduler's job-flow control mechanism
> 
> Changes in V4:
> ==
>   - address Lubens comments regarding documentation
>   - switch to drm_WARN() variants
>   - WARN_ON() drivers passing in zero job credits for both 
> drm_sched_job_init()
> and the update_job_credits() callback
>   - don't retry with another runq if job doesn't fit on the ring to prevent
> priority inversion
>   - rebase onto drm-misc-next (will probably land before Matt's series)
> 
> Changes in V5:
> ==
>   - fix panfrost, lima and etnaviv build
>   - add proposed comments regarding how the code avoids runq priority 
> inversion
>   - address Lubens feedback regarding wording
>   - rebase onto latest drm-misc-next (XE scheduler patches)
> 
> Changes in V6:
> ==
>   - rebase due to conflicts introduced meanwhile
>   - drm_sched_job_init(): fail with EINVAL, rather than WARN() if job->credits
> is zero
>   - drm_sched_can_queue: truncate job->credits if they exceed the scheduler's
> credit limit to guarantee forward progress
> 
> Patch also available in [1].
> 
> [1] https://gitlab.freedesktop.org/nouvelles/kernel/-/commits/sched-credits/
> ---
>  Documentation/gpu/drm-mm.rst  |   6 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_job.c   |   2 +-
>  drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c  |   2 +-
>  drivers/gpu/drm/etnaviv/etnaviv_gpu.c |   2 +-
>  drivers/gpu/drm/lima/lima_device.c|   2 +-
>  drivers/gpu/drm/lima/lima_sched.c |   2 +-
>  drivers/gpu/drm/msm/msm_gem_submit.c  |   2 +-
>  drivers/gpu/drm/nouveau/nouveau_sched.c   |   2 +-
>  drivers/gpu/drm/panfrost/panfrost_drv.c   |   2 +-
>  drivers/gpu/drm/panfrost/panfrost_job.c   |   2 +-
>  .../gpu/drm/scheduler/gpu_scheduler_trace.h   |   2 +-
>  drivers/gpu/drm/scheduler/sched_main.c| 168 ++
>  drivers/gpu/drm/v3d/v3d_gem.c |   2 +-
>  include/drm/gpu_scheduler.h   |  28 ++-
>  14 files changed, 173 insertions(+), 51 deletions(-)
> 
> diff --git a/Documentation/gpu/drm-mm.rst b/Documentation/gpu/drm-mm.rst
> index 602010cb6894..acc5901ac840 100644
> --- a/Documentation/gpu/drm-mm.rst
> +++ b/Documentation/gpu/drm-mm.rst
> @@ -552,6 +552,12 @@ Overview
>  .. kernel-doc:: drivers/gpu/drm/scheduler/sched_main.c
> :doc: Overview
>  
> +Flow Control
> +
> +
> +.. kernel-doc:: drivers/gpu/drm/scheduler/sched_main.c
> +   :doc: Flow Control
> +
>  Scheduler Function References
>  -
>  
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c

[PATCH 1/2] drm/i915/guc: Don't double enable a context

[John . C . Harrison]

From: John Harrison 

If a context is blocked, unblocked and subitted repeatedly in rapid
succession, the driver can end up trying to enable the context while
the previous enable request is still in flight. This can lead to much
confusion in the state tracking.

Prevent that by checking the pending enable flag before trying to
enable a context.

Signed-off-by: John Harrison 
---
 drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c 
b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index d37698bd6b91a..d399e4d238c10 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -720,7 +720,7 @@ static int __guc_add_request(struct intel_guc *guc, struct 
i915_request *rq)
if (unlikely(context_blocked(ce) && !intel_context_is_parent(ce)))
goto out;
 
-   enabled = context_enabled(ce) || context_blocked(ce);
+   enabled = context_enabled(ce) || context_blocked(ce) || 
context_pending_enable(ce);
 
if (!enabled) {
action[len++] = INTEL_GUC_ACTION_SCHED_CONTEXT_MODE_SET;
-- 
2.41.0

[PATCH 0/2] Don't send double context enable/disable requests

[John . C . Harrison]

From: John Harrison 

The driver could sometimes send context enable/disable requests when a
previous request was still pending. This is not allowed. So stop doing
it.

Signed-off-by: John Harrison 


John Harrison (2):
  drm/i915/guc: Don't double enable a context
  drm/i915/guc: Don't disable a context whose enable is still pending

 .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 33 +++
 1 file changed, 26 insertions(+), 7 deletions(-)

-- 
2.41.0

[PATCH 2/2] drm/i915/guc: Don't disable a context whose enable is still pending

[John . C . Harrison]

From: John Harrison 

Various processes involve requesting GuC to disable a given context.
However context enable/disable is an asynchronous process in the GuC.
Thus, it is possible the previous enable request is still being
processed when the disable request is triggered. Having both enable
and disable in flight concurrently is illegal - GuC will return an
error and fail the second operation. The KMD side handler for the
completion message also can't cope with having both pending flags set.
So delay the disable request until it is safe to send.

Signed-off-by: John Harrison 
---
 .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 31 +++
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c 
b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index d399e4d238c10..8c34b0a5abf9a 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -3150,7 +3150,8 @@ guc_context_revoke(struct intel_context *ce, struct 
i915_request *rq,
guc_cancel_context_requests(ce);
intel_engine_signal_breadcrumbs(ce->engine);
} else if (!context_pending_disable(ce)) {
-   u16 guc_id;
+   u16 guc_id = ~0;
+   bool pending_enable = context_pending_enable(ce);
 
/*
 * We add +2 here as the schedule disable complete CTB handler
@@ -3158,7 +3159,11 @@ guc_context_revoke(struct intel_context *ce, struct 
i915_request *rq,
 */
atomic_add(2, >pin_count);
 
-   guc_id = prep_context_pending_disable(ce);
+   if (pending_enable)
+   guc_id = ce->guc_id.id;
+   else
+   guc_id = prep_context_pending_disable(ce);
+
spin_unlock_irqrestore(>guc_state.lock, flags);
 
/*
@@ -3169,7 +3174,15 @@ guc_context_revoke(struct intel_context *ce, struct 
i915_request *rq,
with_intel_runtime_pm(runtime_pm, wakeref) {
__guc_context_set_preemption_timeout(guc, guc_id,
 
preempt_timeout_ms);
-   __guc_context_sched_disable(guc, ce, guc_id);
+   if (!pending_enable)
+   __guc_context_sched_disable(guc, ce, guc_id);
+   }
+
+   if (pending_enable) {
+   /* Can't have both in flight concurrently, so try again 
later... */
+   mod_delayed_work(system_unbound_wq,
+
>guc_state.sched_disable_delay_work,
+msecs_to_jiffies(1));
}
} else {
if (!context_guc_id_invalid(ce))
@@ -3222,7 +3235,13 @@ static void __delay_sched_disable(struct work_struct 
*wrk)
 
spin_lock_irqsave(>guc_state.lock, flags);
 
-   if (bypass_sched_disable(guc, ce)) {
+   if (context_pending_enable(ce)) {
+   spin_unlock_irqrestore(>guc_state.lock, flags);
+   /* Can't have both in flight concurrently, so try again 
later... */
+   mod_delayed_work(system_unbound_wq,
+>guc_state.sched_disable_delay_work,
+msecs_to_jiffies(1));
+   } else if (bypass_sched_disable(guc, ce)) {
spin_unlock_irqrestore(>guc_state.lock, flags);
intel_context_sched_disable_unpin(ce);
} else {
@@ -3257,8 +3276,8 @@ static void guc_context_sched_disable(struct 
intel_context *ce)
if (bypass_sched_disable(guc, ce)) {
spin_unlock_irqrestore(>guc_state.lock, flags);
intel_context_sched_disable_unpin(ce);
-   } else if (!intel_context_is_closed(ce) && !guc_id_pressure(guc, ce) &&
-  delay) {
+   } else if ((!intel_context_is_closed(ce) && !guc_id_pressure(guc, ce) &&
+   delay) || context_pending_enable(ce)) {
spin_unlock_irqrestore(>guc_state.lock, flags);
mod_delayed_work(system_unbound_wq,
 >guc_state.sched_disable_delay_work,
-- 
2.41.0

Re: BUG in drm_kms_helper_poll_enable() fixed by reverting "drm/ast: report connection status on Display Port."

[Kim Phillips]

+LKML

On 11/9/23 7:47 AM, Jocelyn Falempe wrote:

On 09/11/2023 01:37, Kim Phillips wrote:

Hi, current linux kernel commit 90450a06162e
("Merge tag 'rcu-fixes-v6.7' of 
git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks")
and the attached config cause the following BUG when booting on
a reference AMD Zen4 development server:

[   59.995717] input: OpenBMC virtual_input as 
/devices/pci:00/:00:07.1/:02:00.4/usb3/3-2/3-2.6/3-2.6:1.0/0003:1D6B:0104.0002/input/input4
[   60.033135] ast :c2:00.0: vgaarb: deactivate vga console
[   60.066230] ast :c2:00.0: [drm] Using default configuration
[   60.070342] hid-generic 0003:1D6B:0104.0002: input,hidraw0: USB HID v1.01 
Keyboard [OpenBMC virtual_input] on usb-:02:00.4-2.6/input0
[   60.072843] ast :c2:00.0: [drm] AST 2600 detected
[   60.072851] ast :c2:00.0: [drm] Using ASPEED DisplayPort transmitter
[   60.099891] ast :c2:00.0: [drm] dram MCLK=396 Mhz type=1 bus_width=16
[   60.115780] [drm] Initialized ast 0.1.0 20120228 for :c2:00.0 on minor 0
[   60.135643] fbcon: astdrmfb (fb0) is primary device
[   60.135649] fbcon: Deferring console take-over
[   60.146162] ast :c2:00.0: [drm] fb0: astdrmfb frame buffer device
[   60.331802] input: OpenBMC virtual_input as 
/devices/pci:00/:00:07.1/:02:00.4/usb3/3-2/3-2.6/3-2.6:1.0/0003:1D6B:0104.0002/input/input5
[   60.405807] hid-generic 0003:1D6B:0104.0002: input,hidraw0: USB HID v1.01 
Keyboard [OpenBMC virtual_input] on usb-:02:00.4-2.6/input0
[   60.423774] input: OpenBMC virtual_input as 
/devices/pci:00/:00:07.1/:02:00.4/usb3/3-2/3-2.6/3-2.6:1.1/0003:1D6B:0104.0004/input/input6
[   60.443170] hid-generic 0003:1D6B:0104.0004: input,hidraw1: USB HID v1.01 
Mouse [OpenBMC virtual_input] on usb-:02:00.4-2.6/input1
[   60.460675] ast :c2:00.0: vgaarb: deactivate vga console
[   60.479996] ast :c2:00.0: [drm] Using default configuration
[   60.486603] ast :c2:00.0: [drm] AST 2600 detected
[   60.492249] ast :c2:00.0: [drm] Using ASPEED DisplayPort transmitter
[   60.499732] ast :c2:00.0: [drm] dram MCLK=396 Mhz type=1 bus_width=16
[   60.508955] BUG: unable to handle page fault for address: 8881e98109f0
[   60.516623] #PF: supervisor write access in kernel mode
[   60.522449] #PF: error_code(0x0002) - not-present page
[   60.528168] PGD 8dbc01067 P4D 8dbc01067 PUD 104c984067 PMD 104c837067 PTE 
800e167ef060
[   60.537394] Oops: 0002 [#1] PREEMPT SMP DEBUG_PAGEALLOC NOPTI
[   60.543805] CPU: 0 PID: 9 Comm: kworker/0:1 Tainted: G W  6.6.0+ #3
[   60.552251] Hardware name: AMD Corporation ONYX/ONYX, BIOS ROX100AB 
09/14/2023
[   60.560309] Workqueue: events work_for_cpu_fn
[   60.565173] RIP: 0010:enqueue_timer 
(/home/amd/git/linux/./include/linux/list.h:1034 
/home/amd/git/linux/kernel/time/timer.c:605)
[ 60.570129] Code: 44 00 00 55 48 89 e5 41 55 49 89 cd 41 54 49 89 fc 53 48 89 f3 89 
d6 48 8d 84 f7 b0 00 00 00 48 8b 08 48 89 0b 48 85 c9 74 04 <48> 89 59 08 48 89 
18 48 89 43 08 49 8d 44 24 68 48 0f ab 30 8b 4b
All code

    0:   44 00 00    add    %r8b,(%rax)
    3:   55  push   %rbp
    4:   48 89 e5    mov    %rsp,%rbp
    7:   41 55   push   %r13
    9:   49 89 cd    mov    %rcx,%r13
    c:   41 54   push   %r12
    e:   49 89 fc    mov    %rdi,%r12
   11:   53  push   %rbx
   12:   48 89 f3    mov    %rsi,%rbx
   15:   89 d6   mov    %edx,%esi
   17:   48 8d 84 f7 b0 00 00    lea    0xb0(%rdi,%rsi,8),%rax
   1e:   00
   1f:   48 8b 08    mov    (%rax),%rcx
   22:   48 89 0b    mov    %rcx,(%rbx)
   25:   48 85 c9    test   %rcx,%rcx
   28:   74 04   je 0x2e
   2a:*  48 89 59 08 mov    %rbx,0x8(%rcx)   <-- trapping 
instruction
   2e:   48 8
   31:   48 89 43 08 mov    %rax,0x8(%rbx)
   35:   49 8d 44 24 68  lea    0x68(%r12),%rax
   3a:   48 0f ab 30 bts    %rsi,(%rax)
   3e:   8b  .byte 0x8b
   3f:   4b  rex.WXB

Code starting with the faulting instruction
===
    0:   48 89 59 08 mov    %rbx,0x8(%rcx)
    4:   48 89 18    mov    %rbx,(%rax)
    7:   48 89 43 08 mov    %rax,0x8(%rbx)
    b:   49 8d 44 24 68  lea    0x68(%r12),%rax
   10:   48 0f ab 30 bts    %rsi,(%rax)
   14:   8b  .byte 0x8b
   15:   4b  rex.WXB
[   60.591081] RSP: 0018:c90dbbe0 EFLAGS: 00010086
[   60.596908] RAX: 888fd59e31b8 RBX: 8881ec87c9e8 RCX: 8881e98109e8
[   60.604866] RDX: 0099 RSI: 0099 RDI: 888fd59e2c40
[   60.612826] RBP: c90dbbf8 R08: 0001 R09: 888fd59e2c40
[   60.620787] R10: 550d 

[PATCH] drm/sched: Define pr_fmt() for DRM using pr_*()

[Luben Tuikov]

Define pr_fmt() as "[drm] " for DRM code using pr_*() facilities, especially
when no devices are available. This makes it easier to browse kernel logs.

Signed-off-by: Luben Tuikov 
---
 include/drm/drm_print.h | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index a93a387f8a1a15..e8fe60d0eb8783 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -26,6 +26,20 @@
 #ifndef DRM_PRINT_H_
 #define DRM_PRINT_H_
 
+/* Define this before including linux/printk.h, so that the format
+ * string in pr_*() macros is correctly set for DRM. If a file wants
+ * to define this to something else, it should do so before including
+ * this header file.
+ *
+ * It is encouraged code using pr_err() to prefix their format with
+ * the string "*ERROR* ", to make it easier to scan kernel logs. For
+ * instance,
+ *   pr_err("*ERROR* ", args).
+ */
+#ifndef pr_fmt
+#define pr_fmt(fmt) "[drm] " fmt
+#endif
+
 #include 
 #include 
 #include 

base-commit: f3123c2590005c5ff631653d31428e40cd10c618
-- 
2.42.1

Re: [PATCH v6 4/5] drm/panel-edp: Add override_edid_mode quirk for generic edp

[Doug Anderson]

Hi,

On Wed, Nov 8, 2023 at 9:04 AM Doug Anderson  wrote:
>
> Hi,
>
> On Wed, Nov 8, 2023 at 7:45 AM Maxime Ripard  wrote:
> >
> > > @@ -575,9 +599,18 @@ static int panel_edp_get_modes(struct drm_panel 
> > > *panel,
> > >
> > >   if (!p->edid)
> > >   p->edid = drm_get_edid(connector, p->ddc);
> > > -
> > > - if (p->edid)
> > > - num += drm_add_edid_modes(connector, p->edid);
> > > + if (p->edid) {
> > > + if (has_override_edid_mode) {
> >
> > It's not clear to me why the override mechanism is only there when
> > there's a ddc bus?
>
> I think you're confusing the two different (but related) issues
> addressed by this series. One is when you're using the generic
> "edp-panel" compatible string. In that case the mode comes from the
> EDID and only the EDID since there's no hardcoded mode. We need a mode
> override there since some EDIDs shipped with a bad mode. That's the
> subject of ${SUBJECT} patch.
>
> The second issue is what to do with a hardcoded mode. That's the
> subject of the next patch in the series (patch #5). Previously we
> merged the hardcoded and EDID modes. Now (in the next patch) we use
> only the hardcoded mode. There's no need for a fixup because the mode
> is hardcoded in the kernel.
>
>
> > You mentioned before that you were following panel-simple,
>
> As of the newest version of the patch, it's no longer following
> panel-simple in response to your feedback on previous versions.
>
> > but that's a
> > clear deviation from what I can see. If there's a reason for that
> > deviation, that's fine by me, but it should at least be documented in
> > the commit log.
>
> I think the commit log is OK. I suspect the confusion is only because
> you've reviewed previous versions of the series. Please shout if
> things still look confusing.
>
>
> > > @@ -950,6 +983,19 @@ static const struct panel_desc auo_b101ean01 = {
> > >   },
> > >  };
> > >
> > > +static const struct drm_display_mode auo_b116xa3_mode = {
> > > + .clock = 70589,
> > > + .hdisplay = 1366,
> > > + .hsync_start = 1366 + 40,
> > > + .hsync_end = 1366 + 40 + 40,
> > > + .htotal = 1366 + 40 + 40 + 32,
> > > + .vdisplay = 768,
> > > + .vsync_start = 768 + 10,
> > > + .vsync_end = 768 + 10 + 12,
> > > + .vtotal = 768 + 10 + 12 + 6,
> > > + .flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC,
> > > +};
> >
> > That should be a separate patch
>
> That's fair. I didn't think it was a huge deal, but I agree that it's
> slightly cleaner.

I've pushed the first 3 patches but not this patch nor the next one.
It wouldn't hurt to split patch #4 as Maxime requests and then send
the split patch #4 plus patch #5 as a v7. It's probably worth holding
off until either some time passes or Maxime responds and says that his
other concerns are addressed.


-Doug

Re: [PATCH v6 3/5] drm/panel-edp: drm/panel-edp: Add several generic edp panels

[Doug Anderson]

Hi,

On Tue, Nov 7, 2023 at 12:46 PM Hsin-Yi Wang  wrote:
>
> Add a few generic edp panels used by mt8186 chromebooks.
>
> Signed-off-by: Hsin-Yi Wang 
> Reviewed-by: Douglas Anderson 
> ---
> no change.
> ---
>  drivers/gpu/drm/panel/panel-edp.c | 51 +++
>  1 file changed, 51 insertions(+)

Pushed to drm-misc-next:

4d53cf814795 drm/panel-edp: drm/panel-edp: Add several generic edp panels

Re: [PATCH v6 2/5] drm/panel-edp: drm/panel-edp: Fix AUO B116XTN02 name

[Doug Anderson]

Hi,

On Tue, Nov 7, 2023 at 12:46 PM Hsin-Yi Wang  wrote:
>
> Rename AUO 0x235c B116XTN02 to B116XTN02.3 according to decoding edid.
>
> Fixes: 3db2420422a5 ("drm/panel-edp: Add AUO B116XTN02, BOE 
> NT116WHM-N21,836X2, NV116WHM-N49 V8.0")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Hsin-Yi Wang 
> Reviewed-by: Douglas Anderson 
> ---
> v5->v6: split to 2 patches.
> ---
>  drivers/gpu/drm/panel/panel-edp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Even though this is a fix, it's not super urgent and since it'll cause
conflicts with other changes, so pushed to drm-misc-next rather than
drm-misc-fixes.

962845c090c4 drm/panel-edp: drm/panel-edp: Fix AUO B116XTN02 name

Re: [PATCH v6 1/5] drm/panel-edp: drm/panel-edp: Fix AUO B116XAK01 name and timing

[Doug Anderson]

Hi,

On Tue, Nov 7, 2023 at 12:46 PM Hsin-Yi Wang  wrote:
>
> Rename AUO 0x405c B116XAK01 to B116XAK01.0 and adjust the timing of
> auo_b116xak01: T3=200, T12=500, T7_max = 50 according to decoding edid
> and datasheet.
>
> Fixes: da458286a5e2 ("drm/panel: Add support for AUO B116XAK01 panel")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Hsin-Yi Wang 
> Reviewed-by: Douglas Anderson 
> ---
> v5->v6: split to 2 patches.
> ---
>  drivers/gpu/drm/panel/panel-edp.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Even though this is a fix, it's not super urgent since it'll cause
conflicts with other changes, so pushed to drm-misc-next rather than
drm-misc-fixes.

fc6e76792965 drm/panel-edp: drm/panel-edp: Fix AUO B116XAK01 name and timing

[PATCH drm-misc-next v6] drm/sched: implement dynamic job-flow control

[Danilo Krummrich]

Currently, job flow control is implemented simply by limiting the number
of jobs in flight. Therefore, a scheduler is initialized with a credit
limit that corresponds to the number of jobs which can be sent to the
hardware.

This implies that for each job, drivers need to account for the maximum
job size possible in order to not overflow the ring buffer.

However, there are drivers, such as Nouveau, where the job size has a
rather large range. For such drivers it can easily happen that job
submissions not even filling the ring by 1% can block subsequent
submissions, which, in the worst case, can lead to the ring run dry.

In order to overcome this issue, allow for tracking the actual job size
instead of the number of jobs. Therefore, add a field to track a job's
credit count, which represents the number of credits a job contributes
to the scheduler's credit limit.

Signed-off-by: Danilo Krummrich 
---
Changes in V2:
==
  - fixed up influence on scheduling fairness due to consideration of a job's
size
- If we reach a ready entity in drm_sched_select_entity() but can't actually
  queue a job from it due to size limitations, just give up and go to sleep
  until woken up due to a pending job finishing, rather than continue to try
  other entities.
  - added a callback to dynamically update a job's credits (Boris)
  - renamed 'units' to 'credits'
  - fixed commit message and comments as requested by Luben

Changes in V3:
==
  - rebased onto V7 of the "DRM scheduler changes for Xe" series by Matt
  - move up drm_sched_can_queue() instead of adding a forward declaration
(Boris)
  - add a drm_sched_available_credits() helper (Boris)
  - adjust control flow in drm_sched_rq_select_entity_fifo() to Luben's proposal
  - re-phrase a few comments and fix a typo (Luben)
  - change naming of all structures credit fields and function parameters to the
following scheme
- drm_sched_job::credits
- drm_gpu_scheduler::credit_limit
- drm_gpu_scheduler::credit_count
- drm_sched_init(..., u32 credit_limit, ...)
- drm_sched_job_init(..., u32 credits, ...)
  - add proper documentation for the scheduler's job-flow control mechanism

Changes in V4:
==
  - address Lubens comments regarding documentation
  - switch to drm_WARN() variants
  - WARN_ON() drivers passing in zero job credits for both drm_sched_job_init()
and the update_job_credits() callback
  - don't retry with another runq if job doesn't fit on the ring to prevent
priority inversion
  - rebase onto drm-misc-next (will probably land before Matt's series)

Changes in V5:
==
  - fix panfrost, lima and etnaviv build
  - add proposed comments regarding how the code avoids runq priority inversion
  - address Lubens feedback regarding wording
  - rebase onto latest drm-misc-next (XE scheduler patches)

Changes in V6:
==
  - rebase due to conflicts introduced meanwhile
  - drm_sched_job_init(): fail with EINVAL, rather than WARN() if job->credits
is zero
  - drm_sched_can_queue: truncate job->credits if they exceed the scheduler's
credit limit to guarantee forward progress

Patch also available in [1].

[1] https://gitlab.freedesktop.org/nouvelles/kernel/-/commits/sched-credits/
---
 Documentation/gpu/drm-mm.rst  |   6 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.c   |   2 +-
 drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c  |   2 +-
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c |   2 +-
 drivers/gpu/drm/lima/lima_device.c|   2 +-
 drivers/gpu/drm/lima/lima_sched.c |   2 +-
 drivers/gpu/drm/msm/msm_gem_submit.c  |   2 +-
 drivers/gpu/drm/nouveau/nouveau_sched.c   |   2 +-
 drivers/gpu/drm/panfrost/panfrost_drv.c   |   2 +-
 drivers/gpu/drm/panfrost/panfrost_job.c   |   2 +-
 .../gpu/drm/scheduler/gpu_scheduler_trace.h   |   2 +-
 drivers/gpu/drm/scheduler/sched_main.c| 168 ++
 drivers/gpu/drm/v3d/v3d_gem.c |   2 +-
 include/drm/gpu_scheduler.h   |  28 ++-
 14 files changed, 173 insertions(+), 51 deletions(-)

diff --git a/Documentation/gpu/drm-mm.rst b/Documentation/gpu/drm-mm.rst
index 602010cb6894..acc5901ac840 100644
--- a/Documentation/gpu/drm-mm.rst
+++ b/Documentation/gpu/drm-mm.rst
@@ -552,6 +552,12 @@ Overview
 .. kernel-doc:: drivers/gpu/drm/scheduler/sched_main.c
:doc: Overview
 
+Flow Control
+
+
+.. kernel-doc:: drivers/gpu/drm/scheduler/sched_main.c
+   :doc: Flow Control
+
 Scheduler Function References
 -
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
index 1f357198533f..62bb7fc7448a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
@@ -115,7 +115,7 @@ int amdgpu_job_alloc(struct amdgpu_device *adev, struct 
amdgpu_vm *vm,
if (!entity)
return 0;
 
-   return 

Re: [PATCH] drm/sched: Qualify drm_sched_wakeup() by drm_sched_entity_is_ready()

[Danilo Krummrich]

On 11/10/23 01:01, Luben Tuikov wrote:

Don't "wake up" the GPU scheduler unless the entity is ready, as well as we
can queue to the scheduler, i.e. there is no point in waking up the scheduler
for the entity unless the entity is ready.

Signed-off-by: Luben Tuikov 
Fixes: bc8d6a9df99038 ("drm/sched: Don't disturb the entity when in RR-mode 
scheduling")


Reviewed-by: Danilo Krummrich 



---
  drivers/gpu/drm/scheduler/sched_entity.c | 4 ++--
  drivers/gpu/drm/scheduler/sched_main.c   | 8 +---
  include/drm/gpu_scheduler.h  | 2 +-
  3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c 
b/drivers/gpu/drm/scheduler/sched_entity.c
index f1db63cc819812..4d42b1e4daa67f 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -370,7 +370,7 @@ static void drm_sched_entity_wakeup(struct dma_fence *f,
container_of(cb, struct drm_sched_entity, cb);
  
  	drm_sched_entity_clear_dep(f, cb);

-   drm_sched_wakeup(entity->rq->sched);
+   drm_sched_wakeup(entity->rq->sched, entity);
  }
  
  /**

@@ -602,7 +602,7 @@ void drm_sched_entity_push_job(struct drm_sched_job 
*sched_job)
if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
drm_sched_rq_update_fifo(entity, submit_ts);
  
-		drm_sched_wakeup(entity->rq->sched);

+   drm_sched_wakeup(entity->rq->sched, entity);
}
  }
  EXPORT_SYMBOL(drm_sched_entity_push_job);
diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
b/drivers/gpu/drm/scheduler/sched_main.c
index cd0dc3f81d05f0..8f5e466bd58239 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -925,10 +925,12 @@ static bool drm_sched_can_queue(struct drm_gpu_scheduler 
*sched)
   *
   * Wake up the scheduler if we can queue jobs.
   */
-void drm_sched_wakeup(struct drm_gpu_scheduler *sched)
+void drm_sched_wakeup(struct drm_gpu_scheduler *sched,
+ struct drm_sched_entity *entity)
  {
-   if (drm_sched_can_queue(sched))
-   drm_sched_run_job_queue(sched);
+   if (drm_sched_entity_is_ready(entity))
+   if (drm_sched_can_queue(sched))
+   drm_sched_run_job_queue(sched);
  }
  
  /**

diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 754fd2217334e5..09916c84703f59 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -559,7 +559,7 @@ void drm_sched_entity_modify_sched(struct drm_sched_entity 
*entity,
  
  void drm_sched_tdr_queue_imm(struct drm_gpu_scheduler *sched);

  void drm_sched_job_cleanup(struct drm_sched_job *job);
-void drm_sched_wakeup(struct drm_gpu_scheduler *sched);
+void drm_sched_wakeup(struct drm_gpu_scheduler *sched, struct drm_sched_entity 
*entity);
  bool drm_sched_wqueue_ready(struct drm_gpu_scheduler *sched);
  void drm_sched_wqueue_stop(struct drm_gpu_scheduler *sched);
  void drm_sched_wqueue_start(struct drm_gpu_scheduler *sched);

base-commit: f415a6078f640ab15bae34d3c6a1d8e6071363de

[PATCH v2] drm/msm/dsi: use the correct VREG_CTRL_1 value for 4nm cphy

[Jonathan Marek]

Use the same value as the downstream driver. This change is needed for CPHY
mode to work correctly.

Fixes: 8b034e6771113 ("drm/msm/dsi: add support for DSI-PHY on SM8550")
Signed-off-by: Jonathan Marek 
---
v2: fixed the Fixes: line

 drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c 
b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
index 3b1ed02f644d..89a6344bc865 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
@@ -918,7 +918,7 @@ static int dsi_7nm_phy_enable(struct msm_dsi_phy *phy,
if ((phy->cfg->quirks & DSI_PHY_7NM_QUIRK_V5_2)) {
if (phy->cphy_mode) {
vreg_ctrl_0 = 0x45;
-   vreg_ctrl_1 = 0x45;
+   vreg_ctrl_1 = 0x41;
glbl_rescode_top_ctrl = 0x00;
glbl_rescode_bot_ctrl = 0x00;
} else {
-- 
2.26.1

[PATCH] drm/sched: Qualify drm_sched_wakeup() by drm_sched_entity_is_ready()

[Luben Tuikov]

Don't "wake up" the GPU scheduler unless the entity is ready, as well as we
can queue to the scheduler, i.e. there is no point in waking up the scheduler
for the entity unless the entity is ready.

Signed-off-by: Luben Tuikov 
Fixes: bc8d6a9df99038 ("drm/sched: Don't disturb the entity when in RR-mode 
scheduling")
---
 drivers/gpu/drm/scheduler/sched_entity.c | 4 ++--
 drivers/gpu/drm/scheduler/sched_main.c   | 8 +---
 include/drm/gpu_scheduler.h  | 2 +-
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c 
b/drivers/gpu/drm/scheduler/sched_entity.c
index f1db63cc819812..4d42b1e4daa67f 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -370,7 +370,7 @@ static void drm_sched_entity_wakeup(struct dma_fence *f,
container_of(cb, struct drm_sched_entity, cb);
 
drm_sched_entity_clear_dep(f, cb);
-   drm_sched_wakeup(entity->rq->sched);
+   drm_sched_wakeup(entity->rq->sched, entity);
 }
 
 /**
@@ -602,7 +602,7 @@ void drm_sched_entity_push_job(struct drm_sched_job 
*sched_job)
if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
drm_sched_rq_update_fifo(entity, submit_ts);
 
-   drm_sched_wakeup(entity->rq->sched);
+   drm_sched_wakeup(entity->rq->sched, entity);
}
 }
 EXPORT_SYMBOL(drm_sched_entity_push_job);
diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
b/drivers/gpu/drm/scheduler/sched_main.c
index cd0dc3f81d05f0..8f5e466bd58239 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -925,10 +925,12 @@ static bool drm_sched_can_queue(struct drm_gpu_scheduler 
*sched)
  *
  * Wake up the scheduler if we can queue jobs.
  */
-void drm_sched_wakeup(struct drm_gpu_scheduler *sched)
+void drm_sched_wakeup(struct drm_gpu_scheduler *sched,
+ struct drm_sched_entity *entity)
 {
-   if (drm_sched_can_queue(sched))
-   drm_sched_run_job_queue(sched);
+   if (drm_sched_entity_is_ready(entity))
+   if (drm_sched_can_queue(sched))
+   drm_sched_run_job_queue(sched);
 }
 
 /**
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 754fd2217334e5..09916c84703f59 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -559,7 +559,7 @@ void drm_sched_entity_modify_sched(struct drm_sched_entity 
*entity,
 
 void drm_sched_tdr_queue_imm(struct drm_gpu_scheduler *sched);
 void drm_sched_job_cleanup(struct drm_sched_job *job);
-void drm_sched_wakeup(struct drm_gpu_scheduler *sched);
+void drm_sched_wakeup(struct drm_gpu_scheduler *sched, struct drm_sched_entity 
*entity);
 bool drm_sched_wqueue_ready(struct drm_gpu_scheduler *sched);
 void drm_sched_wqueue_stop(struct drm_gpu_scheduler *sched);
 void drm_sched_wqueue_start(struct drm_gpu_scheduler *sched);

base-commit: f415a6078f640ab15bae34d3c6a1d8e6071363de
-- 
2.42.1

[PATCH] drm/msm/dsi: use the correct VREG_CTRL_1 value for 4nm cphy

[Jonathan Marek]

Use the same value as the downstream driver. This change is needed for CPHY
mode to work correctly.

Fixes: 93f0ca6fd61c ("drm/msm/dsi: fix VREG_CTRL_1 value for 4nm cphy")
Signed-off-by: Jonathan Marek 
---
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c 
b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
index 3b1ed02f644d..89a6344bc865 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
@@ -918,7 +918,7 @@ static int dsi_7nm_phy_enable(struct msm_dsi_phy *phy,
if ((phy->cfg->quirks & DSI_PHY_7NM_QUIRK_V5_2)) {
if (phy->cphy_mode) {
vreg_ctrl_0 = 0x45;
-   vreg_ctrl_1 = 0x45;
+   vreg_ctrl_1 = 0x41;
glbl_rescode_top_ctrl = 0x00;
glbl_rescode_bot_ctrl = 0x00;
} else {
-- 
2.26.1

Re: [PATCH] drm/sched: Don't disturb the entity when in RR-mode scheduling

[Luben Tuikov]

On 2023-11-09 18:41, Danilo Krummrich wrote:
> On 11/9/23 20:24, Danilo Krummrich wrote:
>> On 11/9/23 07:52, Luben Tuikov wrote:
>>> Hi,
>>>
>>> On 2023-11-07 19:41, Danilo Krummrich wrote:
 On 11/7/23 05:10, Luben Tuikov wrote:
> Don't call drm_sched_select_entity() in drm_sched_run_job_queue().  In 
> fact,
> rename __drm_sched_run_job_queue() to just drm_sched_run_job_queue(), and 
> let
> it do just that, schedule the work item for execution.
>
> The problem is that drm_sched_run_job_queue() calls 
> drm_sched_select_entity()
> to determine if the scheduler has an entity ready in one of its 
> run-queues,
> and in the case of the Round-Robin (RR) scheduling, the function
> drm_sched_rq_select_entity_rr() does just that, selects the _next_ entity
> which is ready, sets up the run-queue and completion and returns that
> entity. The FIFO scheduling algorithm is unaffected.
>
> Now, since drm_sched_run_job_work() also calls drm_sched_select_entity(), 
> then
> in the case of RR scheduling, that would result in 
> drm_sched_select_entity()
> having been called twice, which may result in skipping a ready entity if 
> more
> than one entity is ready. This commit fixes this by eliminating the call 
> to
> drm_sched_select_entity() from drm_sched_run_job_queue(), and leaves it 
> only
> in drm_sched_run_job_work().
>
> v2: Rebased on top of Tvrtko's renames series of patches. (Luben)
>   Add fixes-tag. (Tvrtko)
>
> Signed-off-by: Luben Tuikov 
> Fixes: f7fe64ad0f22ff ("drm/sched: Split free_job into own work item")
> ---
>    drivers/gpu/drm/scheduler/sched_main.c | 16 +++-
>    1 file changed, 3 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
> b/drivers/gpu/drm/scheduler/sched_main.c
> index 27843e37d9b769..cd0dc3f81d05f0 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -256,10 +256,10 @@ drm_sched_rq_select_entity_fifo(struct drm_sched_rq 
> *rq)
>    }
>    /**
> - * __drm_sched_run_job_queue - enqueue run-job work
> + * drm_sched_run_job_queue - enqueue run-job work
>     * @sched: scheduler instance
>     */
> -static void __drm_sched_run_job_queue(struct drm_gpu_scheduler *sched)
> +static void drm_sched_run_job_queue(struct drm_gpu_scheduler *sched)
>    {
>    if (!READ_ONCE(sched->pause_submit))
>    queue_work(sched->submit_wq, >work_run_job);
> @@ -928,7 +928,7 @@ static bool drm_sched_can_queue(struct 
> drm_gpu_scheduler *sched)
>    void drm_sched_wakeup(struct drm_gpu_scheduler *sched)
>    {
>    if (drm_sched_can_queue(sched))
> -    __drm_sched_run_job_queue(sched);
> +    drm_sched_run_job_queue(sched);
>    }
>    /**
> @@ -1040,16 +1040,6 @@ drm_sched_pick_best(struct drm_gpu_scheduler 
> **sched_list,
>    }
>    EXPORT_SYMBOL(drm_sched_pick_best);
> -/**
> - * drm_sched_run_job_queue - enqueue run-job work if there are ready 
> entities
> - * @sched: scheduler instance
> - */
> -static void drm_sched_run_job_queue(struct drm_gpu_scheduler *sched)
> -{
> -    if (drm_sched_select_entity(sched))

 Hm, now that I rebase my patch to implement dynamic job-flow control I 
 recognize that
 we probably need the peek semantics here. If we do not select an entity 
 here, we also
 do not check whether the corresponding job fits on the ring.

 Alternatively, we simply can't do this check in drm_sched_wakeup(). The 
 consequence would
 be that we don't detect that we need to wait for credits to free up before 
 the run work is
 already executing and the run work selects an entity.
>>>
>>> So I rebased v5 on top of the latest drm-misc-next, and looked around and 
>>> found out that
>>> drm_sched_wakeup() is missing drm_sched_entity_is_ready(). It should look 
>>> like the following,
>>
>> Yeah, but that's just the consequence of re-basing it onto Tvrtko's patch.
>>
>> My point is that by removing drm_sched_select_entity() from 
>> drm_sched_run_job_queue() we do not
>> only loose the check whether the selected entity is ready, but also whether 
>> we have enough
>> credits to actually run a new job. This can lead to queuing up work that 
>> does nothing but calling
>> drm_sched_select_entity() and return.
> 
> Ok, I see it now.  We don't need to peek, we know the entity at 
> drm_sched_wakeup().
> 
> However, the missing drm_sched_entity_is_ready() check should have been added 
> already when
> drm_sched_select_entity() was removed. Gonna send a fix for that as well.

Let me do that, since I added it to your patch.
Then you can rebase your credits patch onto mine.

-- 
Regards,
Luben


OpenPGP_0x4C15479431A334AF.asc

Re: [PATCH] drm/sched: Don't disturb the entity when in RR-mode scheduling

[Danilo Krummrich]

On 11/9/23 20:24, Danilo Krummrich wrote:

On 11/9/23 07:52, Luben Tuikov wrote:

Hi,

On 2023-11-07 19:41, Danilo Krummrich wrote:

On 11/7/23 05:10, Luben Tuikov wrote:

Don't call drm_sched_select_entity() in drm_sched_run_job_queue().  In fact,
rename __drm_sched_run_job_queue() to just drm_sched_run_job_queue(), and let
it do just that, schedule the work item for execution.

The problem is that drm_sched_run_job_queue() calls drm_sched_select_entity()
to determine if the scheduler has an entity ready in one of its run-queues,
and in the case of the Round-Robin (RR) scheduling, the function
drm_sched_rq_select_entity_rr() does just that, selects the _next_ entity
which is ready, sets up the run-queue and completion and returns that
entity. The FIFO scheduling algorithm is unaffected.

Now, since drm_sched_run_job_work() also calls drm_sched_select_entity(), then
in the case of RR scheduling, that would result in drm_sched_select_entity()
having been called twice, which may result in skipping a ready entity if more
than one entity is ready. This commit fixes this by eliminating the call to
drm_sched_select_entity() from drm_sched_run_job_queue(), and leaves it only
in drm_sched_run_job_work().

v2: Rebased on top of Tvrtko's renames series of patches. (Luben)
  Add fixes-tag. (Tvrtko)

Signed-off-by: Luben Tuikov 
Fixes: f7fe64ad0f22ff ("drm/sched: Split free_job into own work item")
---
   drivers/gpu/drm/scheduler/sched_main.c | 16 +++-
   1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
b/drivers/gpu/drm/scheduler/sched_main.c
index 27843e37d9b769..cd0dc3f81d05f0 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -256,10 +256,10 @@ drm_sched_rq_select_entity_fifo(struct drm_sched_rq *rq)
   }
   /**
- * __drm_sched_run_job_queue - enqueue run-job work
+ * drm_sched_run_job_queue - enqueue run-job work
    * @sched: scheduler instance
    */
-static void __drm_sched_run_job_queue(struct drm_gpu_scheduler *sched)
+static void drm_sched_run_job_queue(struct drm_gpu_scheduler *sched)
   {
   if (!READ_ONCE(sched->pause_submit))
   queue_work(sched->submit_wq, >work_run_job);
@@ -928,7 +928,7 @@ static bool drm_sched_can_queue(struct drm_gpu_scheduler 
*sched)
   void drm_sched_wakeup(struct drm_gpu_scheduler *sched)
   {
   if (drm_sched_can_queue(sched))
-    __drm_sched_run_job_queue(sched);
+    drm_sched_run_job_queue(sched);
   }
   /**
@@ -1040,16 +1040,6 @@ drm_sched_pick_best(struct drm_gpu_scheduler 
**sched_list,
   }
   EXPORT_SYMBOL(drm_sched_pick_best);
-/**
- * drm_sched_run_job_queue - enqueue run-job work if there are ready entities
- * @sched: scheduler instance
- */
-static void drm_sched_run_job_queue(struct drm_gpu_scheduler *sched)
-{
-    if (drm_sched_select_entity(sched))


Hm, now that I rebase my patch to implement dynamic job-flow control I 
recognize that
we probably need the peek semantics here. If we do not select an entity here, 
we also
do not check whether the corresponding job fits on the ring.

Alternatively, we simply can't do this check in drm_sched_wakeup(). The 
consequence would
be that we don't detect that we need to wait for credits to free up before the 
run work is
already executing and the run work selects an entity.


So I rebased v5 on top of the latest drm-misc-next, and looked around and found 
out that
drm_sched_wakeup() is missing drm_sched_entity_is_ready(). It should look like 
the following,


Yeah, but that's just the consequence of re-basing it onto Tvrtko's patch.

My point is that by removing drm_sched_select_entity() from 
drm_sched_run_job_queue() we do not
only loose the check whether the selected entity is ready, but also whether we 
have enough
credits to actually run a new job. This can lead to queuing up work that does 
nothing but calling
drm_sched_select_entity() and return.


Ok, I see it now.  We don't need to peek, we know the entity at 
drm_sched_wakeup().

However, the missing drm_sched_entity_is_ready() check should have been added 
already when
drm_sched_select_entity() was removed. Gonna send a fix for that as well.

- Danilo



By peeking the entity we could know this *before* scheduling work and hence 
avoid some CPU scheduler
overhead.

However, since this patch already landed and we can fail the same way if the 
selected entity isn't
ready I don't consider this to be a blocker for the credit patch, hence I will 
send out a v6.



void drm_sched_wakeup(struct drm_gpu_scheduler *sched,
  struct drm_sched_entity *entity)
{
if (drm_sched_entity_is_ready(entity))
    if (drm_sched_can_queue(sched, entity))
    drm_sched_run_job_queue(sched);
}

See the attached patch. (Currently running with base-commit and the attached 
patch.)

Re: [RFC PATCH v3 04/23] drm/vkms: Add kunit tests for VKMS LUT handling

[Arthur Grillo]

On 08/11/23 13:36, Harry Wentland wrote:
> Debugging LUT math is much easier when we can unit test
> it. Add kunit functionality to VKMS and add tests for
>  - get_lut_index
>  - lerp_u16
> 
> v3:
>  - Use include way of testing static functions (Arthur)
> 
> Signed-off-by: Harry Wentland 
> Cc: Arthur Grillo 
> ---
>  drivers/gpu/drm/vkms/Kconfig  |  5 ++
>  drivers/gpu/drm/vkms/tests/.kunitconfig   |  4 ++
>  drivers/gpu/drm/vkms/tests/vkms_color_tests.c | 62 +++
>  drivers/gpu/drm/vkms/vkms_composer.c  |  8 ++-
>  4 files changed, 77 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/gpu/drm/vkms/tests/.kunitconfig
>  create mode 100644 drivers/gpu/drm/vkms/tests/vkms_color_tests.c
> 
> diff --git a/drivers/gpu/drm/vkms/Kconfig b/drivers/gpu/drm/vkms/Kconfig
> index b9ecdebecb0b..c1f8b343ff0e 100644
> --- a/drivers/gpu/drm/vkms/Kconfig
> +++ b/drivers/gpu/drm/vkms/Kconfig
> @@ -13,3 +13,8 @@ config DRM_VKMS
> a VKMS.
>  
> If M is selected the module will be called vkms.
> +
> +config DRM_VKMS_KUNIT_TESTS
> + tristate "Tests for VKMS" if !KUNIT_ALL_TESTS
> + depends on DRM_VKMS && KUNIT
> + default KUNIT_ALL_TESTS
> diff --git a/drivers/gpu/drm/vkms/tests/.kunitconfig 
> b/drivers/gpu/drm/vkms/tests/.kunitconfig
> new file mode 100644
> index ..70e378228cbd
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/tests/.kunitconfig
> @@ -0,0 +1,4 @@
> +CONFIG_KUNIT=y
> +CONFIG_DRM=y
> +CONFIG_DRM_VKMS=y
> +CONFIG_DRM_VKMS_KUNIT_TESTS=y
> diff --git a/drivers/gpu/drm/vkms/tests/vkms_color_tests.c 
> b/drivers/gpu/drm/vkms/tests/vkms_color_tests.c
> new file mode 100644
> index ..b995114cf6b8
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/tests/vkms_color_tests.c
> @@ -0,0 +1,62 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +
> +#include 
> +
> +#include 
> +
> +#define TEST_LUT_SIZE 16
> +
> +static struct drm_color_lut test_linear_array[TEST_LUT_SIZE] = {
> + { 0x0, 0x0, 0x0, 0 },
> + { 0x, 0x, 0x, 0 },
> + { 0x, 0x, 0x, 0 },
> + { 0x, 0x, 0x, 0 },
> + { 0x, 0x, 0x, 0 },
> + { 0x, 0x, 0x, 0 },
> + { 0x, 0x, 0x, 0 },
> + { 0x, 0x, 0x, 0 },
> + { 0x, 0x, 0x, 0 },
> + { 0x, 0x, 0x, 0 },
> + { 0x, 0x, 0x, 0 },
> + { 0x, 0x, 0x, 0 },
> + { 0x, 0x, 0x, 0 },
> + { 0x, 0x, 0x, 0 },
> + { 0x, 0x, 0x, 0 },
> + { 0x, 0x, 0x, 0 },
> +};
> +
> +const struct vkms_color_lut test_linear_lut = {
> + .base = test_linear_array,
> + .lut_length = TEST_LUT_SIZE,
> + .channel_value2index_ratio = 0xf000fll
> +};
> +
> +
> +static void vkms_color_test_get_lut_index(struct kunit *test)
> +{
> + int i;
> +
> + KUNIT_EXPECT_EQ(test, drm_fixp2int(get_lut_index(_linear_lut, 
> test_linear_array[0].red)), 0);
> +
> + for (i = 0; i < TEST_LUT_SIZE; i++)
> + KUNIT_EXPECT_EQ(test, 
> drm_fixp2int_ceil(get_lut_index(_linear_lut, test_linear_array[i].red)), 
> i);
> +}
> +
> +static void vkms_color_test_lerp(struct kunit *test)
> +{
> + KUNIT_EXPECT_EQ(test, lerp_u16(0x0, 0x10, 0x8000), 0x8);
> +}
> +
> +static struct kunit_case vkms_color_test_cases[] = {
> + KUNIT_CASE(vkms_color_test_get_lut_index),
> + KUNIT_CASE(vkms_color_test_lerp),
> + {}
> +};
> +
> +static struct kunit_suite vkms_color_test_suite = {
> + .name = "vkms-color",
> + .test_cases = vkms_color_test_cases,
> +};
> +kunit_test_suite(vkms_color_test_suite);
> +
> +MODULE_LICENSE("GPL");
> \ No newline at end of file
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c 
> b/drivers/gpu/drm/vkms/vkms_composer.c
> index 3c99fb8b54e2..6f942896036e 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -91,7 +91,7 @@ static void fill_background(const struct pixel_argb_u16 
> *background_color,
>  }
>  
>  // lerp(a, b, t) = a + (b - a) * t
> -static u16 lerp_u16(u16 a, u16 b, s64 t)
> +u16 lerp_u16(u16 a, u16 b, s64 t)

Now you don't need to remove the static keyword.

>  {
>   s64 a_fp = drm_int2fixp(a);
>   s64 b_fp = drm_int2fixp(b);
> @@ -101,7 +101,7 @@ static u16 lerp_u16(u16 a, u16 b, s64 t)
>   return drm_fixp2int(a_fp + delta);
>  }
>  
> -static s64 get_lut_index(const struct vkms_color_lut *lut, u16 channel_value)
> +s64 get_lut_index(const struct vkms_color_lut *lut, u16 channel_value)

DITTO

Best Regards,
~Arthur Grillo

>  {
>   s64 color_channel_fp = drm_int2fixp(channel_value);
>  
> @@ -429,3 +429,7 @@ int vkms_set_crc_source(struct drm_crtc *crtc, const char 
> *src_name)
>  
>   return ret;
>  }
> +
> +#ifdef CONFIG_DRM_VKMS_KUNIT_TESTS
> +#include "tests/vkms_color_tests.c"
> +#endif

[PATCH 0/2] fb: amifb: Convert to platform remove callback returning void

[Uwe Kleine-König]

Hello,

in the series 
https://lore.kernel.org/20231107091740.3924258-1-u.kleine-koe...@pengutronix.de
I suggested to replace module_platform_driver_probe() in the amifb
driver. Geert Uytterhoeven objected because the amiga platform has very
little RAM. This series implements the alternative: Mark the driver
struct with __refdata to suppress the section warning. Patch #2 is just
a rebase from the above series.

Best regards
Uwe

Uwe Kleine-König (2):
  fb: amifb: Mark driver struct with __refdata to prevent section
mismatch warning
  fb: amifb: Convert to platform remove callback returning void

 drivers/video/fbdev/amifb.c | 13 +
 1 file changed, 9 insertions(+), 4 deletions(-)


base-commit: aba6ab57a910ad4b940c2024d15f2cdbf5b7f76b
-- 
2.42.0

[PATCH 2/2] fb: amifb: Convert to platform remove callback returning void

[Uwe Kleine-König]

The .remove() callback for a platform driver returns an int which makes
many driver authors wrongly assume it's possible to do error handling by
returning an error code. However the value returned is ignored (apart
from emitting a warning) and this typically results in resource leaks.

To improve here there is a quest to make the remove callback return
void. In the first step of this quest all drivers are converted to
.remove_new(), which already returns void. Eventually after all drivers
are converted, .remove_new() will be renamed to .remove().

Trivially convert this driver from always returning zero in the remove
callback to the void returning variant.

Signed-off-by: Uwe Kleine-König 
---
 drivers/video/fbdev/amifb.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/video/fbdev/amifb.c b/drivers/video/fbdev/amifb.c
index 4a1bc693cebd..305f396c764c 100644
--- a/drivers/video/fbdev/amifb.c
+++ b/drivers/video/fbdev/amifb.c
@@ -3752,7 +3752,7 @@ static int __init amifb_probe(struct platform_device 
*pdev)
 }
 
 
-static int __exit amifb_remove(struct platform_device *pdev)
+static void __exit amifb_remove(struct platform_device *pdev)
 {
struct fb_info *info = platform_get_drvdata(pdev);
 
@@ -3765,7 +3765,6 @@ static int __exit amifb_remove(struct platform_device 
*pdev)
chipfree();
framebuffer_release(info);
amifb_video_off();
-   return 0;
 }
 
 /*
@@ -3775,7 +3774,7 @@ static int __exit amifb_remove(struct platform_device 
*pdev)
  * triggers a section mismatch warning.
  */
 static struct platform_driver amifb_driver __refdata = {
-   .remove = __exit_p(amifb_remove),
+   .remove_new = __exit_p(amifb_remove),
.driver   = {
.name   = "amiga-video",
},
-- 
2.42.0

[PATCH 1/2] fb: amifb: Mark driver struct with __refdata to prevent section mismatch warning

[Uwe Kleine-König]

As described in the added code comment, a reference to .exit.text is ok
for drivers registered via module_platform_driver_probe(). Make this
explicit to prevent a section mismatch warning.

Signed-off-by: Uwe Kleine-König 
---
 drivers/video/fbdev/amifb.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/amifb.c b/drivers/video/fbdev/amifb.c
index b18c6b4f129a..4a1bc693cebd 100644
--- a/drivers/video/fbdev/amifb.c
+++ b/drivers/video/fbdev/amifb.c
@@ -3768,7 +3768,13 @@ static int __exit amifb_remove(struct platform_device 
*pdev)
return 0;
 }
 
-static struct platform_driver amifb_driver = {
+/*
+ * amifb_remove() lives in .exit.text. For drivers registered via
+ * module_platform_driver_probe() this ok because they cannot get unboud at
+ * runtime. The driver needs to be marked with __refdata, otherwise modpost
+ * triggers a section mismatch warning.
+ */
+static struct platform_driver amifb_driver __refdata = {
.remove = __exit_p(amifb_remove),
.driver   = {
.name   = "amiga-video",
-- 
2.42.0

[PATCH V2 2/4] nv3051d: Add Powkiddy RK2023 Panel Support

[Chris Morgan]

From: Chris Morgan 

Refactor the driver to add support for the powkiddy,rk2023-panel
panel. This panel is extremely similar to the rg353p-panel but
requires a smaller vertical back porch and isn't as tolerant of
higher speeds. Note that while all of these panels are identical in
size (70x57) it is possible future panels may not be.

Tested on my RG351V, RG353P, RG353V, and RK2023.

Signed-off-by: Chris Morgan 
---
 .../gpu/drm/panel/panel-newvision-nv3051d.c   | 57 +++
 1 file changed, 46 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-newvision-nv3051d.c 
b/drivers/gpu/drm/panel/panel-newvision-nv3051d.c
index 79de6c886292..94d89ffd596b 100644
--- a/drivers/gpu/drm/panel/panel-newvision-nv3051d.c
+++ b/drivers/gpu/drm/panel/panel-newvision-nv3051d.c
@@ -28,6 +28,7 @@ struct nv3051d_panel_info {
unsigned int num_modes;
u16 width_mm, height_mm;
u32 bus_flags;
+   u32 mode_flags;
 };
 
 struct panel_nv3051d {
@@ -261,6 +262,8 @@ static int panel_nv3051d_unprepare(struct drm_panel *panel)
 
usleep_range(1, 15000);
 
+   gpiod_set_value_cansleep(ctx->reset_gpio, 1);
+
regulator_disable(ctx->vdd);
 
return 0;
@@ -385,15 +388,7 @@ static int panel_nv3051d_probe(struct mipi_dsi_device *dsi)
 
dsi->lanes = 4;
dsi->format = MIPI_DSI_FMT_RGB888;
-   dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
- MIPI_DSI_MODE_LPM | MIPI_DSI_MODE_NO_EOT_PACKET;
-
-   /*
-* The panel in the RG351V is identical to the 353P, except it
-* requires MIPI_DSI_CLOCK_NON_CONTINUOUS to operate correctly.
-*/
-   if (of_device_is_compatible(dev->of_node, "anbernic,rg351v-panel"))
-   dsi->mode_flags |= MIPI_DSI_CLOCK_NON_CONTINUOUS;
+   dsi->mode_flags = ctx->panel_info->mode_flags;
 
drm_panel_init(>panel, >dev, _nv3051d_funcs,
   DRM_MODE_CONNECTOR_DSI);
@@ -481,16 +476,56 @@ static const struct drm_display_mode 
nv3051d_rgxx3_modes[] = {
},
 };
 
-static const struct nv3051d_panel_info nv3051d_rgxx3_info = {
+static const struct drm_display_mode nv3051d_rk2023_modes[] = {
+   {
+   .hdisplay   = 640,
+   .hsync_start= 640 + 40,
+   .hsync_end  = 640 + 40 + 2,
+   .htotal = 640 + 40 + 2 + 80,
+   .vdisplay   = 480,
+   .vsync_start= 480 + 18,
+   .vsync_end  = 480 + 18 + 2,
+   .vtotal = 480 + 18 + 2 + 4,
+   .clock  = 24150,
+   .flags  = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
+   },
+};
+
+static const struct nv3051d_panel_info nv3051d_rg351v_info = {
.display_modes = nv3051d_rgxx3_modes,
.num_modes = ARRAY_SIZE(nv3051d_rgxx3_modes),
.width_mm = 70,
.height_mm = 57,
.bus_flags = DRM_BUS_FLAG_DE_LOW | DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE,
+   .mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
+ MIPI_DSI_MODE_LPM | MIPI_DSI_MODE_NO_EOT_PACKET |
+ MIPI_DSI_CLOCK_NON_CONTINUOUS,
+};
+
+static const struct nv3051d_panel_info nv3051d_rg353p_info = {
+   .display_modes = nv3051d_rgxx3_modes,
+   .num_modes = ARRAY_SIZE(nv3051d_rgxx3_modes),
+   .width_mm = 70,
+   .height_mm = 57,
+   .bus_flags = DRM_BUS_FLAG_DE_LOW | DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE,
+   .mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
+ MIPI_DSI_MODE_LPM | MIPI_DSI_MODE_NO_EOT_PACKET,
+};
+
+static const struct nv3051d_panel_info nv3051d_rk2023_info = {
+   .display_modes = nv3051d_rk2023_modes,
+   .num_modes = ARRAY_SIZE(nv3051d_rk2023_modes),
+   .width_mm = 70,
+   .height_mm = 57,
+   .bus_flags = DRM_BUS_FLAG_DE_LOW | DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE,
+   .mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
+ MIPI_DSI_MODE_LPM | MIPI_DSI_MODE_NO_EOT_PACKET,
 };
 
 static const struct of_device_id newvision_nv3051d_of_match[] = {
-   { .compatible = "newvision,nv3051d", .data = _rgxx3_info },
+   { .compatible = "anbernic,rg351v-panel", .data = _rg351v_info },
+   { .compatible = "anbernic,rg353p-panel", .data = _rg353p_info },
+   { .compatible = "powkiddy,rk2023-panel", .data = _rk2023_info },
{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, newvision_nv3051d_of_match);
-- 
2.34.1

[PATCH V2 4/4] arm64: dts: rockchip: add Powkiddy RK2023

[Chris Morgan]

From: Chris Morgan 

Add support for the Powkiddy RK2023. The Powkiddy RK2023 is a handheld
gaming device with a 3.5 inch screen powered by the Rockchip RK3566
SoC. The device is almost identical to the Anbernic RG353P except it
lacks eMMC, a function button, a touch screen, no UART headers on the
board, and the panel has slightly different timings.

Signed-off-by: Chris Morgan 
---
 arch/arm64/boot/dts/rockchip/Makefile |   1 +
 .../dts/rockchip/rk3566-powkiddy-rk2023.dts   | 161 ++
 2 files changed, 162 insertions(+)
 create mode 100644 arch/arm64/boot/dts/rockchip/rk3566-powkiddy-rk2023.dts

diff --git a/arch/arm64/boot/dts/rockchip/Makefile 
b/arch/arm64/boot/dts/rockchip/Makefile
index a18f33bf0c0e..f969618da352 100644
--- a/arch/arm64/boot/dts/rockchip/Makefile
+++ b/arch/arm64/boot/dts/rockchip/Makefile
@@ -78,6 +78,7 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3566-anbernic-rg503.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3566-pinenote-v1.1.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3566-pinenote-v1.2.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3566-powkiddy-rgb30.dtb
+dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3566-powkiddy-rk2023.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3566-quartz64-a.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3566-quartz64-b.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3566-radxa-cm3-io.dtb
diff --git a/arch/arm64/boot/dts/rockchip/rk3566-powkiddy-rk2023.dts 
b/arch/arm64/boot/dts/rockchip/rk3566-powkiddy-rk2023.dts
new file mode 100644
index ..5740412f6b2b
--- /dev/null
+++ b/arch/arm64/boot/dts/rockchip/rk3566-powkiddy-rk2023.dts
@@ -0,0 +1,161 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+
+/dts-v1/;
+
+#include 
+#include 
+#include 
+#include "rk3566-anbernic-rg353x.dtsi"
+
+/ {
+   model = "RK2023";
+   compatible = "powkiddy,rk2023", "rockchip,rk3566";
+
+   aliases {
+   mmc1 = 
+   mmc2 = 
+   mmc3 = 
+   };
+
+   battery: battery {
+   compatible = "simple-battery";
+   charge-full-design-microamp-hours = <3151000>;
+   charge-term-current-microamp = <30>;
+   constant-charge-current-max-microamp = <200>;
+   constant-charge-voltage-max-microvolt = <425>;
+   factory-internal-resistance-micro-ohms = <117000>;
+   voltage-max-design-microvolt = <4172000>;
+   voltage-min-design-microvolt = <340>;
+
+   ocv-capacity-celsius = <20>;
+   ocv-capacity-table-0 =  <4172000 100>, <4092000 95>, <4035000 
90>, <399 85>,
+   <3939000 80>, <3895000 75>, <3852000 
70>, <3807000 65>,
+   <3762000 60>, <3713000 55>, <3672000 
50>, <3647000 45>,
+   <3629000 40>, <3613000 35>, <3598000 
30>, <3578000 25>,
+   <355 20>, <3519000 15>, <3479000 
10>, <3438000 5>,
+   <340 0>;
+   };
+
+   /* Channels reversed for headphones. */
+   sound {
+   compatible = "simple-audio-card";
+   simple-audio-card,name = "rk817_int";
+   simple-audio-card,format = "i2s";
+   simple-audio-card,hp-det-gpio = < RK_PC6 
GPIO_ACTIVE_HIGH>;
+   simple-audio-card,mclk-fs = <256>;
+   simple-audio-card,widgets =
+   "Microphone", "Mic Jack",
+   "Headphone", "Headphones",
+   "Speaker", "Internal Speakers";
+   simple-audio-card,routing =
+   "MICL", "Mic Jack",
+   "Headphones", "HPOL",
+   "Headphones", "HPOR",
+   "Internal Speakers", "SPKO";
+
+   simple-audio-card,codec {
+   sound-dai = <>;
+   };
+
+   simple-audio-card,cpu {
+   sound-dai = <_8ch>;
+   };
+   };
+
+};
+
+/delete-node/ _keys;
+
+ {
+   /delete-property/ stdout-path;
+};
+
+ {
+   assigned-clocks = < CLK_RTC_32K>, < PLL_GPLL>,
+ < PLL_PPLL>, < PLL_VPLL>;
+   assigned-clock-rates = <32768>, <12>,
+ <2>, <11520>;
+};
+
+_keys_control {
+   button-r1 {
+   gpios = < RK_PB3 GPIO_ACTIVE_LOW>;
+   label = "TR";
+   linux,code = ;
+   };
+
+   button-r2 {
+   gpios = < RK_PB4 GPIO_ACTIVE_LOW>;
+   label = "TR2";
+   linux,code = ;
+   };
+};
+
+/delete-node/ &{/i2c@fdd4/regulator@40};
+
+ {
+   vdd_cpu: regulator@1c {
+   compatible = "tcs,tcs4525";
+   reg = <0x1c>;
+   fcs,suspend-voltage-selector = <1>;
+   regulator-always-on;
+   regulator-boot-on;
+   regulator-min-microvolt = <712500>;
+   

[PATCH V2 3/4] dt-bindings: arm: rockchip: Add Powkiddy RK2023

[Chris Morgan]

From: Chris Morgan 

The Powkiddy RK2023 is a handheld gaming device made by Powkiddy and
powered by the Rockchip RK3566 SoC. Group the Powkiddy RK3566 based
devices together as they are both extremely similar.

Signed-off-by: Chris Morgan 
---
 Documentation/devicetree/bindings/arm/rockchip.yaml | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/rockchip.yaml 
b/Documentation/devicetree/bindings/arm/rockchip.yaml
index 5f7c6c4aad8f..5b015c4ed775 100644
--- a/Documentation/devicetree/bindings/arm/rockchip.yaml
+++ b/Documentation/devicetree/bindings/arm/rockchip.yaml
@@ -674,9 +674,11 @@ properties:
   - const: pine64,soquartz
   - const: rockchip,rk3566
 
-  - description: Powkiddy RGB30
+  - description: Powkiddy RK3566 Handheld Gaming Console
 items:
-  - const: powkiddy,rgb30
+  - enum:
+  - powkiddy,rgb30
+  - powkiddy,rk2023
   - const: rockchip,rk3566
 
   - description: Radxa Compute Module 3(CM3)
-- 
2.34.1

[PATCH V2 0/4] rockchip: Add Powkiddy RK2023

[Chris Morgan]

From: Chris Morgan 

Add support for the Powkiddy RK2023, which is extremely similar to
existing devices from Anbernic.

Changes since V1:
 - Necessary clock changes have been accepted to mainline, so removed
   from this series.
   
https://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git/commit/?id=f1db0865b4628d5e2e85347350c077a71f0629d2
 - Combined Powkiddy RK3566 devices in devicetree documentation.
   Dropped ack from binding as this change is vastly different than
   the previous update.
 - Updated panel driver to hold panel in reset status after unprepare.

Chris Morgan (4):
  dt-bindings: display: panel: Update NewVision NV3051D compatibles
  nv3051d: Add Powkiddy RK2023 Panel Support
  dt-bindings: arm: rockchip: Add Powkiddy RK2023
  arm64: dts: rockchip: add Powkiddy RK2023

 .../devicetree/bindings/arm/rockchip.yaml |   6 +-
 .../display/panel/newvision,nv3051d.yaml  |   2 +-
 arch/arm64/boot/dts/rockchip/Makefile |   1 +
 .../dts/rockchip/rk3566-powkiddy-rk2023.dts   | 161 ++
 .../gpu/drm/panel/panel-newvision-nv3051d.c   |  57 +--
 5 files changed, 213 insertions(+), 14 deletions(-)
 create mode 100644 arch/arm64/boot/dts/rockchip/rk3566-powkiddy-rk2023.dts

-- 
2.34.1

[PATCH V2 1/4] dt-bindings: display: panel: Update NewVision NV3051D compatibles

[Chris Morgan]

From: Chris Morgan 

Update the NewVision NV3051D compatible strings by adding a new panel,
the powkiddy,rk2023-panel, and removing another entry, the
anbernic,rg353v-panel. The rg353v-panel is exactly identical to the
rg353p-panel and is not currently in use by any existing device tree.
The rk2023-panel is similar to the rg353p-panel but has slightly
different timings.

I originally wrote the driver checking for the newvision,nv3051d
compatible string which worked fine when there was only 1 panel type.
When I added support for the 351v-panel I *should* have changed how the
compatible string was handled, but instead I simply added a check in the
probe function to look for the secondary string of
"anbernic,rg351v-panel". Now that I am adding the 3rd panel type of
"powkiddy,rk2023-panel" I am correcting the driver to do it the right
way by checking for the specific compatibles.

Signed-off-by: Chris Morgan 
---
 .../devicetree/bindings/display/panel/newvision,nv3051d.yaml| 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git 
a/Documentation/devicetree/bindings/display/panel/newvision,nv3051d.yaml 
b/Documentation/devicetree/bindings/display/panel/newvision,nv3051d.yaml
index cce775a87f87..7a634fbc465e 100644
--- a/Documentation/devicetree/bindings/display/panel/newvision,nv3051d.yaml
+++ b/Documentation/devicetree/bindings/display/panel/newvision,nv3051d.yaml
@@ -21,7 +21,7 @@ properties:
   - enum:
   - anbernic,rg351v-panel
   - anbernic,rg353p-panel
-  - anbernic,rg353v-panel
+  - powkiddy,rk2023-panel
   - const: newvision,nv3051d
 
   reg: true
-- 
2.34.1

Re: [PATCH] drm/sched: fix potential page fault in drm_sched_job_init()

[Luben Tuikov]

On 2023-11-09 14:55, Danilo Krummrich wrote:
> On 11/9/23 01:09, Danilo Krummrich wrote:
>> On 11/8/23 06:46, Luben Tuikov wrote:
>>> Hi,
>>>
>>> Could you please use my gmail address, the one one I'm responding from--I 
>>> don't want
>>> to miss any DRM scheduler patches. BTW, the luben.tui...@amd.com email 
>>> should bounce
>>> as undeliverable.
>>>
>>> On 2023-11-07 21:26, Danilo Krummrich wrote:
 Commit 56e449603f0a ("drm/sched: Convert the GPU scheduler to variable
 number of run-queues") introduces drm_err() in drm_sched_job_init(), in
 order to indicate that the given entity has no runq, however at this
 time job->sched is not yet set, likely to be NULL initialized, and hence
 shouldn't be used.

 Replace the corresponding drm_err() call with pr_err() to avoid a
 potential page fault.

 While at it, extend the documentation of drm_sched_job_init() to
 indicate that job->sched is not a valid pointer until
 drm_sched_job_arm() has been called.

 Fixes: 56e449603f0a ("drm/sched: Convert the GPU scheduler to variable 
 number of run-queues")
 Signed-off-by: Danilo Krummrich 
 ---
   drivers/gpu/drm/scheduler/sched_main.c | 5 -
   1 file changed, 4 insertions(+), 1 deletion(-)

 diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
 b/drivers/gpu/drm/scheduler/sched_main.c
 index 27843e37d9b7..dd28389f0ddd 100644
 --- a/drivers/gpu/drm/scheduler/sched_main.c
 +++ b/drivers/gpu/drm/scheduler/sched_main.c
 @@ -680,6 +680,9 @@ EXPORT_SYMBOL(drm_sched_resubmit_jobs);
    * This function returns -ENOENT in this case (which probably should be 
 -EIO as
    * a more meanigful return value).
    *
 + * Note that job->sched is not a valid pointer until drm_sched_job_arm() 
 has
 + * been called.
 + *
>>>
>>> Good catch!
>>>
>>> Did you actually get this to page-fault and have a kernel log?
>>
>> No, I just found it because I was about to make the same mistake.
>>
>>>
>>> I'm asking because we see it correctly set in this kernel log coming from 
>>> AMD,
>>
>> I think that's because amdgpu just sets job->sched to *some* scheduler 
>> instance after
>> job allocation [1].
>>
>> [1] 
>> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c#L108
>>
>>>
>>> [   11.886024] amdgpu :0a:00.0: [drm] *ERROR* drm_sched_job_init: 
>>> entity has no rq!
>>>
>>> in this email,
>>> https://lore.kernel.org/r/CADnq5_PS64jYS_Y3kGW27m-kuWP+FQFiaVcOaZiB=jlsgpn...@mail.gmail.com
>>>
    * Returns 0 for success, negative error code otherwise.
    */
   int drm_sched_job_init(struct drm_sched_job *job,
 @@ -691,7 +694,7 @@ int drm_sched_job_init(struct drm_sched_job *job,
    * or worse--a blank screen--leave a trail in the
    * logs, so this can be debugged easier.
    */
 -    drm_err(job->sched, "%s: entity has no rq!\n", __func__);
 +    pr_err("%s: entity has no rq!\n", __func__);
>>>
>>> Is it feasible to do something like the following?
>>>
>>>     dev_err(job->sched ? job->sched->dev : NULL, "%s: entity has no 
>>> rq!\n", __func__);
>>
>> I don't think that's a good idea. Although I'd assume that every driver 
>> zero-initializes its job
>> structures, I can't see a rule enforcing that. Hence, job->sched can be a 
>> random value until
>> drm_sched_job_arm() is called.
>>
>> However, I notice there are quite a view more fields of struct drm_sched_job 
>> that are never
>> initialized, hence there are either a couple more potential bugs or missing 
>> documentation that
>> drivers *must* ensure that a job is zero-initialized.
> 
> Any opinions on that? Otherwise I'd probably go ahead and send a fix for the 
> other bugs too.

Send the patches.

Will those patches also add pr_fmt() for DRM?

I'm asking because you said you'll add pr_fmt() in a "separate" patch, and I 
thought it was
okay being self-contained in your patch as per the version I sent.
-- 
Regards,
Luben


OpenPGP_0x4C15479431A334AF.asc
Description: OpenPGP public key


OpenPGP_signature.asc
Description: OpenPGP digital signature

Re: [PATCH v7 0/7] incorporate pm runtime framework and eDP clean up

[Dmitry Baryshkov]

Hello Kuogee,


On Thu, 9 Nov 2023 at 19:51, Kuogee Hsieh  wrote:
>
>
> On 11/8/2023 10:27 AM, Abhinav Kumar wrote:
> >
> >
> > On 11/8/2023 10:10 AM, Kuogee Hsieh wrote:
> >>
> >> On 11/7/2023 1:23 PM, Dmitry Baryshkov wrote:
> >>> On Tue, 7 Nov 2023 at 23:01, Kuogee Hsieh 
> >>> wrote:
> 
>  On 11/6/2023 5:55 PM, Dmitry Baryshkov wrote:
> > On Sat, 7 Oct 2023 at 01:55, Kuogee Hsieh
> >  wrote:
> >> The purpose of this patch series is to incorporate pm runtime
> >> framework
> >> into MSM eDP/DP driver so that eDP panel can be detected by DRM
> >> eDP panel
> >> driver during system probe time. During incorporating procedure,
> >> original
> >> customized pm realted fucntions, such as dp_pm_prepare(),
> >> dp_pm_suspend(),
> >> dp_pm_resume() and dp_pm_prepare(), are removed and replaced with
> >> functions
> >> provided by pm runtiem framework such as
> >> pm_runtime_force_suspend() and
> >> pm_runtime_force_resume(). In addition, both eDP aux-bus and irq
> >> handler
> >> are bound at system probe time too.
> > With this patchset in place I can crash the board using the following
> > sequence (SM8350-HDK):
> >
> > - plug the USBC DP dongle
> > - run modetest at any mode, don't press Enter yet
> > - unplug the dongle
> > - press Enter to stop modetest
> >
> > => the board resets to Sahara.
> >
> > Please ping me if you need any additional information from my side.
>  questiosn,
> 
>  1) which dongle are you used?
> >>> I have used several Dell and Hama USB-C dongles.
> >>>
>  2) what code branch shoud I used to duplicate this problem.
> >>> I have pushed my kernel tree to
> >>> git.codelinaro.org/dmitry.baryshkov/linux.git, branch test-dp-rpm
> >>> I had several UCSI patches on top, but they should not be relevant.
> >> git.codelinaro.org/dmitry.baryshkov/linux.git, branch test-dp-rpm <==
> >> I synced out his branch and it is still work at my chromebook Kodiak
> >> DUT.
> >>>
> >
> > Perhaps the gap in test results with the same tree is due to internal
> > hpd vs hpd pin. We need to try this on a device which does not use
> > internal hpd.
>
>
> Hi Dmitry,

First of all, I was able to reproduce the issue without this patch
series. Kuogee, I must ask your pardon, it is not a regression and it
is not caused by this series.
So, we have a bug, but not a regression.

Second, a stable reproducer:

When you unplug and re-plug the dongle, switch the orientation of the dongle.
This way the system crashes in 100% of cases.

Here are the last messages that I see on my console:

trying to open device 'tilcdc'...failed
trying to open device 'msm'...done
setting mode 3840x2160-30.00Hz on connectors 34, crtc 84
failed to set gamma: Function not implemented
[   25.504828] [drm:dpu_encoder_phys_vid_wait_for_commit_done:487]
[dpu error]vblank timeout
[   25.515024] [drm:dpu_kms_wait_for_commit_done:494] [dpu error]wait
for commit done returned -110
[   25.621146] [drm:dpu_encoder_frame_done_timeout:2342] [dpu
error]enc33 frame done timeout
Format: Log Type - Time(microsec) - Message - Optional Info
Log Type: B - Since Boot(Power On Reset),  D - Delta,  S - Statistic
S - QC_IMAGE_VERSION_STRING=BOOT.MXF.1.0-00848-LAHAINA-1


>
> I have two more questions,
>
> 1) are you see test pattern shows at external DP when you run modetest?

Yes, I see the pattern

> 2) is *.kcrash file created under /var/spool/crash/ when system crashed.
> If it is, can you share it?

There is no kcrash, as there is no kernel crash. The system reboots to
the download mode.

>
> Thanks,
>
> >
>  I can not duplicate  system crash problem at my setup kodiak (SM7325)
>  chrome book with my PM runtime patch series.
> 
>  my code base is Linux 6.6-rc2 + pm runtime patch series (7 patches)
> 
>  I did:
> 
>  1) plugin either apple dongle (DP-to-HDMI) + 1080p display or DP typeC
>  cable directly to 1080p display
> 
>  2)  stop ui
> 
>  3) /usr/bin/modetest -M msm -s 34:1920x1080 (see test pattern show at
>  display)
> 
>  4) unplug apple dongle or DP typeC cable
> 
>  5) hit enter key
> 
>  6) start ui
> 
>  7) display back to login page of chrome book
> 
> 
> >>>



-- 
With best wishes
Dmitry

Re: [Intel-gfx] [PATCH] drm/i915: Read a shadowed mmio register for ggtt flush

[Ville Syrjälä]

On Thu, Nov 09, 2023 at 12:01:26PM -0800, Belgaumkar, Vinay wrote:
> 
> On 11/9/2023 11:30 AM, Ville Syrjälä wrote:
> > On Thu, Nov 09, 2023 at 11:21:48AM -0800, Vinay Belgaumkar wrote:
> >> We read RENDER_HEAD as a part of the flush. If GT is in
> >> deeper sleep states, this could lead to read errors since we are
> >> not using a forcewake. Safer to read a shadowed register instead.
> > IIRC shadowing is only thing for writes, not reads.
> 
> Sure, but reading from a shadowed register does return the cached value

Does it? I suppose that would make some sense, but I don't recall that
ever being stated anywhere. At least before the shadow registers
existed reads would just give you zeroes when not awake.

> (even though we don't care about the vakue here). When GT is in deeper 
> sleep states, it is better to read a shadowed (cached) value instead of 
> trying to attempt an mmio register read without a force wake anyways.

So you're saying reads from non-shadowed registers fails somehow
when not awake? How exactly do they fail? And when reading from
a shadowed register that failure never happens?

> 
> Thanks,
> 
> Vinay.
> 
> >
> >> Cc: John Harrison 
> >> Cc: Daniele Ceraolo Spurio 
> >> Signed-off-by: Vinay Belgaumkar 
> >> ---
> >>   drivers/gpu/drm/i915/gt/intel_gt.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c 
> >> b/drivers/gpu/drm/i915/gt/intel_gt.c
> >> index ed32bf5b1546..ea814ea5f700 100644
> >> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> >> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> >> @@ -451,7 +451,7 @@ void intel_gt_flush_ggtt_writes(struct intel_gt *gt)
> >>   
> >>spin_lock_irqsave(>lock, flags);
> >>intel_uncore_posting_read_fw(uncore,
> >> -   RING_HEAD(RENDER_RING_BASE));
> >> +   RING_TAIL(RENDER_RING_BASE));
> >>spin_unlock_irqrestore(>lock, flags);
> >>}
> >>   }
> >> -- 
> >> 2.38.1

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Fix for potential false positives in GuC hang selftest

[Daniele Ceraolo Spurio]

On 11/6/2023 3:59 PM, john.c.harri...@intel.com wrote:

From: John Harrison 

Noticed that the hangcheck selftest is submitting a non-preemptoble
spinner. That means that even if the GuC does not die, the heartbeat
will still kick in and trigger a reset. Which is rather defeating the
purpose of the test - to verify that the heartbeat will kick in if the
GuC itself has died. The test is deliberately killing the GuC, so it
should never hit the case of a non-dead GuC. But it is not impossible
that the kill might fail at some future point due to other driver
re-work.

So, make the spinner pre-emptible. That way the heartbeat can get
through if the GuC is alive and context switching. Thus a reset only
happens if the GuC dies. Thus, if the kill should stop working the
test will now fail rather than claim to pass.

Signed-off-by: John Harrison 


Reviewed-by: Daniele Ceraolo Spurio 

Daniele


---
  drivers/gpu/drm/i915/gt/uc/selftest_guc_hangcheck.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/selftest_guc_hangcheck.c 
b/drivers/gpu/drm/i915/gt/uc/selftest_guc_hangcheck.c
index 34b5d952e2bcb..26fdc392fce6c 100644
--- a/drivers/gpu/drm/i915/gt/uc/selftest_guc_hangcheck.c
+++ b/drivers/gpu/drm/i915/gt/uc/selftest_guc_hangcheck.c
@@ -74,7 +74,7 @@ static int intel_hang_guc(void *arg)
goto err;
}
  
-	rq = igt_spinner_create_request(, ce, MI_NOOP);

+   rq = igt_spinner_create_request(, ce, MI_ARB_CHECK);
intel_context_put(ce);
if (IS_ERR(rq)) {
ret = PTR_ERR(rq);

Re: [Intel-gfx] [PATCH 2/2] drm/i915/guc: Add a selftest for FAST_REQUEST errors

[Daniele Ceraolo Spurio]

On 11/6/2023 3:59 PM, john.c.harri...@intel.com wrote:

From: John Harrison 

There is a mechanism for reporting errors from fire and forget H2G
messages. This is the only way to find out about almost any error in
the GuC backend submission path. So it would be useful to know that it
is working.

Signed-off-by: John Harrison 
---
  drivers/gpu/drm/i915/gt/uc/intel_guc.h|   4 +
  drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c |   9 ++
  drivers/gpu/drm/i915/gt/uc/selftest_guc.c | 122 ++
  3 files changed, 135 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h 
b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
index 2b6dfe62c8f2a..e22c12ce245ad 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
@@ -297,6 +297,10 @@ struct intel_guc {
 * @number_guc_id_stolen: The number of guc_ids that have been stolen
 */
int number_guc_id_stolen;
+   /**
+* @fast_response_selftest: Backdoor to CT handler for fast response 
selftest
+*/
+   u32 fast_response_selftest;
  #endif
  };
  
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c

index 89e314b3756bb..9d958afb78b7f 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
@@ -1076,6 +1076,15 @@ static int ct_handle_response(struct intel_guc_ct *ct, 
struct ct_incoming_msg *r
found = true;
break;
}
+
+#ifdef CONFIG_DRM_I915_SELFTEST
+   if (!found && ct_to_guc(ct)->fast_response_selftest) {
+   CT_DEBUG(ct, "Assuming unsolicited response due to FAST_REQUEST 
selftest\n");
+   ct_to_guc(ct)->fast_response_selftest++;
+   found = 1;


found = true ? it's the same thing, but it's cleaner to assign boolean 
values to bool variables



+   }
+#endif
+
if (!found) {
CT_ERROR(ct, "Unsolicited response message: len %u, data %#x (fence 
%u, last %u)\n",
 len, hxg[0], fence, ct->requests.last_fence);
diff --git a/drivers/gpu/drm/i915/gt/uc/selftest_guc.c 
b/drivers/gpu/drm/i915/gt/uc/selftest_guc.c
index bfb72143566f6..97fbbb396336c 100644
--- a/drivers/gpu/drm/i915/gt/uc/selftest_guc.c
+++ b/drivers/gpu/drm/i915/gt/uc/selftest_guc.c
@@ -286,11 +286,133 @@ static int intel_guc_steal_guc_ids(void *arg)
return ret;
  }
  
+/*

+ * Send a context schedule H2G message with an invalid context id.
+ * This should generate a GUC_RESULT_INVALID_CONTEXT response.
+ */
+static int bad_h2g(struct intel_guc *guc)
+{
+   u32 action[3], len = 0;


AFAICS This is a 2 DW command, so you can use action[2].


+
+   action[len++] = INTEL_GUC_ACTION_SCHED_CONTEXT;
+   action[len++] = 0x12345678;
+
+   return intel_guc_send_nb(guc, action, len, 0);
+}
+
+/*
+ * Set a spinner running to make sure the system is alive and active,
+ * then send a bad but asynchronous H2G command and wait to see if an
+ * error response is returned. If no response is received or if the
+ * spinner dies then the test will fail.
+ */
+#define FAST_RESPONSE_TIMEOUT_MS   1000
+static int intel_guc_fast_request(void *arg)
+{
+   struct intel_gt *gt = arg;
+   struct intel_context *ce;
+   struct igt_spinner spin;
+   struct i915_request *rq;
+   intel_wakeref_t wakeref;
+   struct intel_engine_cs *engine = intel_selftest_find_any_engine(gt);
+   ktime_t before, now, delta;
+   bool spinning = false;
+   u64 delta_ms;
+   int ret = 0;
+
+   if (!engine)
+   return 0;
+
+   wakeref = intel_runtime_pm_get(gt->uncore->rpm);
+
+   ce = intel_context_create(engine);
+   if (IS_ERR(ce)) {
+   ret = PTR_ERR(ce);
+   gt_err(gt, "Failed to create spinner request: %pe\n", ce);
+   goto err_pm;
+   }
+
+   ret = igt_spinner_init(, engine->gt);
+   if (ret) {
+   gt_err(gt, "Failed to create spinner: %pe\n", ERR_PTR(ret));
+   goto err_pm;
+   }
+   spinning = true;
+
+   rq = igt_spinner_create_request(, ce, MI_ARB_CHECK);
+   intel_context_put(ce);
+   if (IS_ERR(rq)) {
+   ret = PTR_ERR(rq);
+   gt_err(gt, "Failed to create spinner request: %pe\n", rq);
+   goto err_spin;
+   }
+
+   ret = request_add_spin(rq, );
+   if (ret) {
+   gt_err(gt, "Failed to add Spinner request: %pe\n", 
ERR_PTR(ret));
+   goto err_rq;
+   }
+
+   gt->uc.guc.fast_response_selftest = 1;
+
+   ret = bad_h2g(>uc.guc);
+   if (ret) {
+   gt_err(gt, "Failed to send H2G: %pe\n", ERR_PTR(ret));
+   goto err_rq;
+   }
+
+   before = ktime_get();
+   while (gt->uc.guc.fast_response_selftest == 1) {
+   ret = i915_request_wait(rq, 0, 1);
+   if (ret != -ETIME) {
+   

Re: [PATCH 01/22] fb: amifb: Stop using platform_driver_probe()

[Helge Deller]

On 11/8/23 22:34, Geert Uytterhoeven wrote:

Hi Helge,

On Wed, Nov 8, 2023 at 10:32 PM Helge Deller  wrote:

On 11/8/23 22:06, Geert Uytterhoeven wrote:

On Tue, Nov 7, 2023 at 10:20 AM Uwe Kleine-König
 wrote:

On today's platforms the benefit of platform_driver_probe() isn't that
relevant any more. It allows to drop some code after booting (or module
loading) for .probe() and discard the .remove() function completely if
the driver is built-in. This typically saves a few 100k.


Which is a lot on platforms with only a few MiBs of RAM...


True.
Given the warnings below, what is your suggestion?
Better to drop the amifb patch ?


I think so. There is a reason these drivers use platform_driver_probe().


Ok, I've dropped both amifb patches.

Helge

Re: [PATCH 03/17] dt-bindings: i2c: samsung, s3c2410-i2c: add specific compatibles for existing SoC

[Linus Walleij]

On Wed, Nov 8, 2023 at 11:44 AM Krzysztof Kozlowski
 wrote:

> Samsung Exynos SoC reuses several devices from older designs, thus
> historically we kept the old (block's) compatible only.  This works fine
> and there is no bug here, however guidelines expressed in
> Documentation/devicetree/bindings/writing-bindings.rst state that:
> 1. Compatibles should be specific.
> 2. We should add new compatibles in case of bugs or features.
>
> Add compatibles specific to each SoC in front of all old-SoC-like
> compatibles.
>
> Signed-off-by: Krzysztof Kozlowski 

Makes perfect sense to me:
Reviewed-by: Linus Walleij 

Yours,
Linus Walleij

Re: [Intel-gfx] [PATCH] drm/i915: Read a shadowed mmio register for ggtt flush

[Belgaumkar, Vinay]

On 11/9/2023 11:30 AM, Ville Syrjälä wrote:

On Thu, Nov 09, 2023 at 11:21:48AM -0800, Vinay Belgaumkar wrote:

We read RENDER_HEAD as a part of the flush. If GT is in
deeper sleep states, this could lead to read errors since we are
not using a forcewake. Safer to read a shadowed register instead.

IIRC shadowing is only thing for writes, not reads.


Sure, but reading from a shadowed register does return the cached value 
(even though we don't care about the vakue here). When GT is in deeper 
sleep states, it is better to read a shadowed (cached) value instead of 
trying to attempt an mmio register read without a force wake anyways.


Thanks,

Vinay.




Cc: John Harrison 
Cc: Daniele Ceraolo Spurio 
Signed-off-by: Vinay Belgaumkar 
---
  drivers/gpu/drm/i915/gt/intel_gt.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c 
b/drivers/gpu/drm/i915/gt/intel_gt.c
index ed32bf5b1546..ea814ea5f700 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -451,7 +451,7 @@ void intel_gt_flush_ggtt_writes(struct intel_gt *gt)
  
  		spin_lock_irqsave(>lock, flags);

intel_uncore_posting_read_fw(uncore,
-RING_HEAD(RENDER_RING_BASE));
+RING_TAIL(RENDER_RING_BASE));
spin_unlock_irqrestore(>lock, flags);
}
  }
--
2.38.1

Re: [PATCH] drm/sched: fix potential page fault in drm_sched_job_init()

[Danilo Krummrich]

On 11/9/23 01:09, Danilo Krummrich wrote:

On 11/8/23 06:46, Luben Tuikov wrote:

Hi,

Could you please use my gmail address, the one one I'm responding from--I don't 
want
to miss any DRM scheduler patches. BTW, the luben.tui...@amd.com email should 
bounce
as undeliverable.

On 2023-11-07 21:26, Danilo Krummrich wrote:

Commit 56e449603f0a ("drm/sched: Convert the GPU scheduler to variable
number of run-queues") introduces drm_err() in drm_sched_job_init(), in
order to indicate that the given entity has no runq, however at this
time job->sched is not yet set, likely to be NULL initialized, and hence
shouldn't be used.

Replace the corresponding drm_err() call with pr_err() to avoid a
potential page fault.

While at it, extend the documentation of drm_sched_job_init() to
indicate that job->sched is not a valid pointer until
drm_sched_job_arm() has been called.

Fixes: 56e449603f0a ("drm/sched: Convert the GPU scheduler to variable number of 
run-queues")
Signed-off-by: Danilo Krummrich 
---
  drivers/gpu/drm/scheduler/sched_main.c | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
b/drivers/gpu/drm/scheduler/sched_main.c
index 27843e37d9b7..dd28389f0ddd 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -680,6 +680,9 @@ EXPORT_SYMBOL(drm_sched_resubmit_jobs);
   * This function returns -ENOENT in this case (which probably should be -EIO 
as
   * a more meanigful return value).
   *
+ * Note that job->sched is not a valid pointer until drm_sched_job_arm() has
+ * been called.
+ *


Good catch!

Did you actually get this to page-fault and have a kernel log?


No, I just found it because I was about to make the same mistake.



I'm asking because we see it correctly set in this kernel log coming from AMD,


I think that's because amdgpu just sets job->sched to *some* scheduler instance 
after
job allocation [1].

[1] 
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c#L108



[   11.886024] amdgpu :0a:00.0: [drm] *ERROR* drm_sched_job_init: entity 
has no rq!

in this email,
https://lore.kernel.org/r/CADnq5_PS64jYS_Y3kGW27m-kuWP+FQFiaVcOaZiB=jlsgpn...@mail.gmail.com


   * Returns 0 for success, negative error code otherwise.
   */
  int drm_sched_job_init(struct drm_sched_job *job,
@@ -691,7 +694,7 @@ int drm_sched_job_init(struct drm_sched_job *job,
   * or worse--a blank screen--leave a trail in the
   * logs, so this can be debugged easier.
   */
-    drm_err(job->sched, "%s: entity has no rq!\n", __func__);
+    pr_err("%s: entity has no rq!\n", __func__);


Is it feasible to do something like the following?

    dev_err(job->sched ? job->sched->dev : NULL, "%s: entity has no rq!\n", 
__func__);


I don't think that's a good idea. Although I'd assume that every driver 
zero-initializes its job
structures, I can't see a rule enforcing that. Hence, job->sched can be a 
random value until
drm_sched_job_arm() is called.

However, I notice there are quite a view more fields of struct drm_sched_job 
that are never
initialized, hence there are either a couple more potential bugs or missing 
documentation that
drivers *must* ensure that a job is zero-initialized.


Any opinions on that? Otherwise I'd probably go ahead and send a fix for the 
other bugs too.



Not quite sure if we really want to rely on the latter for core 
infrastructure...




  return -ENOENT;
  }

base-commit: c015fb6d01adb616fb54824feb55ce5ab18e8ca1

Re: [Intel-gfx] [PATCH] drm/i915: Read a shadowed mmio register for ggtt flush

[Ville Syrjälä]

On Thu, Nov 09, 2023 at 11:21:48AM -0800, Vinay Belgaumkar wrote:
> We read RENDER_HEAD as a part of the flush. If GT is in
> deeper sleep states, this could lead to read errors since we are
> not using a forcewake. Safer to read a shadowed register instead.

IIRC shadowing is only thing for writes, not reads.

> 
> Cc: John Harrison 
> Cc: Daniele Ceraolo Spurio 
> Signed-off-by: Vinay Belgaumkar 
> ---
>  drivers/gpu/drm/i915/gt/intel_gt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c 
> b/drivers/gpu/drm/i915/gt/intel_gt.c
> index ed32bf5b1546..ea814ea5f700 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -451,7 +451,7 @@ void intel_gt_flush_ggtt_writes(struct intel_gt *gt)
>  
>   spin_lock_irqsave(>lock, flags);
>   intel_uncore_posting_read_fw(uncore,
> -  RING_HEAD(RENDER_RING_BASE));
> +  RING_TAIL(RENDER_RING_BASE));
>   spin_unlock_irqrestore(>lock, flags);
>   }
>  }
> -- 
> 2.38.1

-- 
Ville Syrjälä
Intel

Re: [PATCH] drm/sched: Don't disturb the entity when in RR-mode scheduling

[Danilo Krummrich]

On 11/9/23 07:52, Luben Tuikov wrote:

Hi,

On 2023-11-07 19:41, Danilo Krummrich wrote:

On 11/7/23 05:10, Luben Tuikov wrote:

Don't call drm_sched_select_entity() in drm_sched_run_job_queue().  In fact,
rename __drm_sched_run_job_queue() to just drm_sched_run_job_queue(), and let
it do just that, schedule the work item for execution.

The problem is that drm_sched_run_job_queue() calls drm_sched_select_entity()
to determine if the scheduler has an entity ready in one of its run-queues,
and in the case of the Round-Robin (RR) scheduling, the function
drm_sched_rq_select_entity_rr() does just that, selects the _next_ entity
which is ready, sets up the run-queue and completion and returns that
entity. The FIFO scheduling algorithm is unaffected.

Now, since drm_sched_run_job_work() also calls drm_sched_select_entity(), then
in the case of RR scheduling, that would result in drm_sched_select_entity()
having been called twice, which may result in skipping a ready entity if more
than one entity is ready. This commit fixes this by eliminating the call to
drm_sched_select_entity() from drm_sched_run_job_queue(), and leaves it only
in drm_sched_run_job_work().

v2: Rebased on top of Tvrtko's renames series of patches. (Luben)
  Add fixes-tag. (Tvrtko)

Signed-off-by: Luben Tuikov 
Fixes: f7fe64ad0f22ff ("drm/sched: Split free_job into own work item")
---
   drivers/gpu/drm/scheduler/sched_main.c | 16 +++-
   1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
b/drivers/gpu/drm/scheduler/sched_main.c
index 27843e37d9b769..cd0dc3f81d05f0 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -256,10 +256,10 @@ drm_sched_rq_select_entity_fifo(struct drm_sched_rq *rq)
   }
   
   /**

- * __drm_sched_run_job_queue - enqueue run-job work
+ * drm_sched_run_job_queue - enqueue run-job work
* @sched: scheduler instance
*/
-static void __drm_sched_run_job_queue(struct drm_gpu_scheduler *sched)
+static void drm_sched_run_job_queue(struct drm_gpu_scheduler *sched)
   {
if (!READ_ONCE(sched->pause_submit))
queue_work(sched->submit_wq, >work_run_job);
@@ -928,7 +928,7 @@ static bool drm_sched_can_queue(struct drm_gpu_scheduler 
*sched)
   void drm_sched_wakeup(struct drm_gpu_scheduler *sched)
   {
if (drm_sched_can_queue(sched))
-   __drm_sched_run_job_queue(sched);
+   drm_sched_run_job_queue(sched);
   }
   
   /**

@@ -1040,16 +1040,6 @@ drm_sched_pick_best(struct drm_gpu_scheduler 
**sched_list,
   }
   EXPORT_SYMBOL(drm_sched_pick_best);
   
-/**

- * drm_sched_run_job_queue - enqueue run-job work if there are ready entities
- * @sched: scheduler instance
- */
-static void drm_sched_run_job_queue(struct drm_gpu_scheduler *sched)
-{
-   if (drm_sched_select_entity(sched))


Hm, now that I rebase my patch to implement dynamic job-flow control I 
recognize that
we probably need the peek semantics here. If we do not select an entity here, 
we also
do not check whether the corresponding job fits on the ring.

Alternatively, we simply can't do this check in drm_sched_wakeup(). The 
consequence would
be that we don't detect that we need to wait for credits to free up before the 
run work is
already executing and the run work selects an entity.


So I rebased v5 on top of the latest drm-misc-next, and looked around and found 
out that
drm_sched_wakeup() is missing drm_sched_entity_is_ready(). It should look like 
the following,


Yeah, but that's just the consequence of re-basing it onto Tvrtko's patch.

My point is that by removing drm_sched_select_entity() from 
drm_sched_run_job_queue() we do not
only loose the check whether the selected entity is ready, but also whether we 
have enough
credits to actually run a new job. This can lead to queuing up work that does 
nothing but calling
drm_sched_select_entity() and return.

By peeking the entity we could know this *before* scheduling work and hence 
avoid some CPU scheduler
overhead.

However, since this patch already landed and we can fail the same way if the 
selected entity isn't
ready I don't consider this to be a blocker for the credit patch, hence I will 
send out a v6.



void drm_sched_wakeup(struct drm_gpu_scheduler *sched,
  struct drm_sched_entity *entity)
{
if (drm_sched_entity_is_ready(entity))
if (drm_sched_can_queue(sched, entity))
drm_sched_run_job_queue(sched);
}

See the attached patch. (Currently running with base-commit and the attached 
patch.)

Re: [PATCH 03/17] dt-bindings: i2c: samsung,s3c2410-i2c: add specific compatibles for existing SoC

[Wolfram Sang]

On Wed, Nov 08, 2023 at 11:43:29AM +0100, Krzysztof Kozlowski wrote:
> Samsung Exynos SoC reuses several devices from older designs, thus
> historically we kept the old (block's) compatible only.  This works fine
> and there is no bug here, however guidelines expressed in
> Documentation/devicetree/bindings/writing-bindings.rst state that:
> 1. Compatibles should be specific.
> 2. We should add new compatibles in case of bugs or features.
> 
> Add compatibles specific to each SoC in front of all old-SoC-like
> compatibles.
> 
> Signed-off-by: Krzysztof Kozlowski 
> 
> ---
> 
> I propose to take the patch through Samsung SoC (me). See cover letter
> for explanation.

I am fine that you take it once all review comments are addressed. Given
that:

Acked-by: Wolfram Sang 



signature.asc
Description: PGP signature

[PATCH] drm/i915: Read a shadowed mmio register for ggtt flush

[Vinay Belgaumkar]

We read RENDER_HEAD as a part of the flush. If GT is in
deeper sleep states, this could lead to read errors since we are
not using a forcewake. Safer to read a shadowed register instead.

Cc: John Harrison 
Cc: Daniele Ceraolo Spurio 
Signed-off-by: Vinay Belgaumkar 
---
 drivers/gpu/drm/i915/gt/intel_gt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c 
b/drivers/gpu/drm/i915/gt/intel_gt.c
index ed32bf5b1546..ea814ea5f700 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -451,7 +451,7 @@ void intel_gt_flush_ggtt_writes(struct intel_gt *gt)
 
spin_lock_irqsave(>lock, flags);
intel_uncore_posting_read_fw(uncore,
-RING_HEAD(RENDER_RING_BASE));
+RING_TAIL(RENDER_RING_BASE));
spin_unlock_irqrestore(>lock, flags);
}
 }
-- 
2.38.1

Re: [PATCH 02/17] dt-bindings: i2c: exynos5: add specific compatibles for existing SoC

[Wolfram Sang]

On Wed, Nov 08, 2023 at 11:43:28AM +0100, Krzysztof Kozlowski wrote:
> Samsung Exynos SoC reuses several devices from older designs, thus
> historically we kept the old (block's) compatible only.  This works fine
> and there is no bug here, however guidelines expressed in
> Documentation/devicetree/bindings/writing-bindings.rst state that:
> 1. Compatibles should be specific.
> 2. We should add new compatibles in case of bugs or features.
> 
> Add compatibles specific to each SoC in front of all old-SoC-like
> compatibles.
> 
> Signed-off-by: Krzysztof Kozlowski 
> 
> ---
> 
> I propose to take the patch through Samsung SoC (me). See cover letter
> for explanation.

I am fine that you take it once all review comments are addressed. Given
that:

Acked-by: Wolfram Sang 



signature.asc
Description: PGP signature

Re: [RFC PATCH 2/2] vc4: introduce DMA-BUF heap

[Maxime Ripard]

On Thu, Nov 09, 2023 at 03:42:38PM +, Dave Stevenson wrote:
> Hi Simon and Maxime
> 
> On Thu, 9 Nov 2023 at 09:12, Maxime Ripard  wrote:
> >
> > Hi Simon,
> >
> > On Thu, Nov 09, 2023 at 07:45:58AM +, Simon Ser wrote:
> > > User-space sometimes needs to allocate scanout-capable memory for
> > > GPU rendering purposes. On a vc4/v3d split render/display SoC, this
> > > is achieved via DRM dumb buffers: the v3d user-space driver opens
> > > the primary vc4 node, allocates a DRM dumb buffer there, exports it
> > > as a DMA-BUF, imports it into the v3d render node, and renders to it.
> > >
> > > However, DRM dumb buffers are only meant for CPU rendering, they are
> > > not intended to be used for GPU rendering. Primary nodes should only
> > > be used for mode-setting purposes, other programs should not attempt
> > > to open it. Moreover, opening the primary node is already broken on
> > > some setups: systemd grants permission to open primary nodes to
> > > physically logged in users, but this breaks when the user is not
> > > physically logged in (e.g. headless setup) and when the distribution
> > > is using a different init (e.g. Alpine Linux uses openrc).
> > >
> > > We need an alternate way for v3d to allocate scanout-capable memory.
> > > Leverage DMA heaps for this purpose: expose a CMA heap to user-space.
> > > Preliminary testing has been done with wlroots [1].
> > >
> > > This is still an RFC. Open questions:
> > >
> > > - Does this approach make sense to y'all in general?
> >
> > Makes sense to me :)
> >
> > > - What would be a good name for the heap? "vc4" is maybe a bit naive and
> > >   not precise enough. Something with "cma"? Do we need to plan a naming
> > >   scheme to accomodate for multiple vc4 devices?
> >
> > That's a general issue though that happens with pretty much all devices
> > with a separate node for modesetting and rendering, so I don't think
> > addressing it only for vc4 make sense, we should make it generic.
> >
> > So maybe something like "scanout"?
> >
> > One thing we need to consider too is that the Pi5 will have multiple
> > display nodes (4(?) iirc) with different capabilities, vc4 being only
> > one of them. This will impact that solution too.
> 
> It does need to scale.
> 
> Pi5 adds 4 additional DRM devices (2xDSI, 1xDPI, and 1xComposite
> Video), and just this last week I've been running Wayfire with TinyDRM
> drivers for SPI displays and UDL (DisplayLink) outputs as well.
> Presumably all of those drivers need to have appropriate hooks added
> so they each expose a dma-heap to enable scanout buffers to be
> allocated.
> 
> Can we add another function pointer to the struct drm_driver (or
> similar) to do the allocation, and move the actual dmabuf handling
> code into the core?

Yeah, I agree here, it just seems easier to provide a global hook and a
somewhat sane default to cover all drivers in one go (potentially with
additional restrictions, like only for modeset-only drivers or
something).

Maxime


signature.asc
Description: PGP signature

Re: [PATCH] drm/sched: fix potential page fault in drm_sched_job_init()

[Danilo Krummrich]

On 11/9/23 05:23, Luben Tuikov wrote:

On 2023-11-08 19:09, Danilo Krummrich wrote:

On 11/8/23 06:46, Luben Tuikov wrote:

Hi,

Could you please use my gmail address, the one one I'm responding from--I don't 
want
to miss any DRM scheduler patches. BTW, the luben.tui...@amd.com email should 
bounce
as undeliverable.

On 2023-11-07 21:26, Danilo Krummrich wrote:

Commit 56e449603f0a ("drm/sched: Convert the GPU scheduler to variable
number of run-queues") introduces drm_err() in drm_sched_job_init(), in
order to indicate that the given entity has no runq, however at this
time job->sched is not yet set, likely to be NULL initialized, and hence
shouldn't be used.

Replace the corresponding drm_err() call with pr_err() to avoid a
potential page fault.

While at it, extend the documentation of drm_sched_job_init() to
indicate that job->sched is not a valid pointer until
drm_sched_job_arm() has been called.

Fixes: 56e449603f0a ("drm/sched: Convert the GPU scheduler to variable number of 
run-queues")
Signed-off-by: Danilo Krummrich 
---
   drivers/gpu/drm/scheduler/sched_main.c | 5 -
   1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
b/drivers/gpu/drm/scheduler/sched_main.c
index 27843e37d9b7..dd28389f0ddd 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -680,6 +680,9 @@ EXPORT_SYMBOL(drm_sched_resubmit_jobs);
* This function returns -ENOENT in this case (which probably should be -EIO 
as
* a more meanigful return value).
*
+ * Note that job->sched is not a valid pointer until drm_sched_job_arm() has
+ * been called.
+ *


Good catch!

Did you actually get this to page-fault and have a kernel log?


No, I just found it because I was about to make the same mistake.



I'm asking because we see it correctly set in this kernel log coming from AMD,


I think that's because amdgpu just sets job->sched to *some* scheduler instance 
after
job allocation [1].

[1] 
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c#L108



[   11.886024] amdgpu :0a:00.0: [drm] *ERROR* drm_sched_job_init: entity 
has no rq!

in this email,
https://lore.kernel.org/r/CADnq5_PS64jYS_Y3kGW27m-kuWP+FQFiaVcOaZiB=jlsgpn...@mail.gmail.com


* Returns 0 for success, negative error code otherwise.
*/
   int drm_sched_job_init(struct drm_sched_job *job,
@@ -691,7 +694,7 @@ int drm_sched_job_init(struct drm_sched_job *job,
 * or worse--a blank screen--leave a trail in the
 * logs, so this can be debugged easier.
 */
-   drm_err(job->sched, "%s: entity has no rq!\n", __func__);
+   pr_err("%s: entity has no rq!\n", __func__);


Is it feasible to do something like the following?

dev_err(job->sched ? job->sched->dev : NULL, "%s: entity has no 
rq!\n", __func__);


I don't think that's a good idea. Although I'd assume that every driver 
zero-initializes its job
structures, I can't see a rule enforcing that. Hence, job->sched can be a 
random value until
drm_sched_job_arm() is called.


Okay. However, when using pr_err() we're losing "[drm] *ERROR* " prefix and we 
scan for that
in the logs to quickly find the cause of the error.

Perhaps we can define pr_fmt() and also include "*ERROR*" so that we can get 
the desired result
as the attached patch shows?


Sure, I'd add the pr_fmt() in a separate patch though.

Re: [RFC PATCH 2/2] vc4: introduce DMA-BUF heap

[Maxime Ripard]

On Thu, Nov 09, 2023 at 03:31:44PM +, Simon Ser wrote:
> > > - What would be a good name for the heap? "vc4" is maybe a bit naive and
> > > not precise enough. Something with "cma"? Do we need to plan a naming
> > > scheme to accomodate for multiple vc4 devices?
> > 
> > That's a general issue though that happens with pretty much all devices
> > with a separate node for modesetting and rendering, so I don't think
> > addressing it only for vc4 make sense, we should make it generic.
> > 
> > So maybe something like "scanout"?
> > 
> > One thing we need to consider too is that the Pi5 will have multiple
> > display nodes (4(?) iirc) with different capabilities, vc4 being only
> > one of them. This will impact that solution too.
> 
> I'm not sure trying to find a unique generic name for all split render/display
> SoC is a good idea:
> 
> - For the purposes of replacing DRM dumb buffers usage from v3d, we don't
>   actually need a generic name, it's perfectly fine to hardcode a name here
>   since vc4 is pretty much hardcoded there already.

Right. I'm wondering how that will work with drivers like panfrost or
powervr that will need to interact with different display drivers. We
will have the same issue for those, but we won't be able to hardcode the
driver name.

Similarly, if you have multiple display drivers, what "scanout-capable"
will mean might differ from one device to the other. Some have
constraints on the memory they can access for example, so you cannot
just assume that a scanout-capable buffer allocated by vc4 might not be
scanout-capable for one of the RP1 DSI device.

> - As you said, "scanout" may be ill-defined depending on the system. What if
>   a PCIe or USB device is plugged in? What if vkms is loaded? What if there 
> are
>   multiple platform display devices? What does "scanout" mean then?
> - A generic solution to advertise what DMA heaps a DRM display device is
>   compatible with is probably better addressed by populating sysfs with new
>   files.

That would be a good idea indeed

>   We've talked with Sima at XDC about adding a symlink pointing to the
>   DMA heap and extra metadata files describing priorities and such.
>   However we don't actually need that part for the purposes of v3d --
>   I think I'd rather defer that until more DMA heaps are plumbed
>   across the DRM drivers.

Honestly, I don't think we can afford to only consider vc4/v3d here. The
issue you described seem to affect any SoC with a split scanout/GPU,
which is pretty much all of them? And if they are all affected, we
should design something that fixes it once and for all.

Maxime


signature.asc
Description: PGP signature

Re: [RFC PATCH 2/2] vc4: introduce DMA-BUF heap

[Dave Stevenson]

Hi Simon

On Thu, 9 Nov 2023 at 17:46, Simon Ser  wrote:
>
> On Thursday, November 9th, 2023 at 16:42, Dave Stevenson 
>  wrote:
>
> > > > - What would be a good name for the heap? "vc4" is maybe a bit naive and
> > > >   not precise enough. Something with "cma"? Do we need to plan a naming
> > > >   scheme to accomodate for multiple vc4 devices?
> > >
> > > That's a general issue though that happens with pretty much all devices
> > > with a separate node for modesetting and rendering, so I don't think
> > > addressing it only for vc4 make sense, we should make it generic.
> > >
> > > So maybe something like "scanout"?
> > >
> > > One thing we need to consider too is that the Pi5 will have multiple
> > > display nodes (4(?) iirc) with different capabilities, vc4 being only
> > > one of them. This will impact that solution too.
> >
> > It does need to scale.
> >
> > Pi5 adds 4 additional DRM devices (2xDSI, 1xDPI, and 1xComposite
> > Video), and just this last week I've been running Wayfire with TinyDRM
> > drivers for SPI displays and UDL (DisplayLink) outputs as well.
> > Presumably all of those drivers need to have appropriate hooks added
> > so they each expose a dma-heap to enable scanout buffers to be
> > allocated.
>
> I'm not sure this makes sense necessarily for all of these devices. For vc4 
> and
> the 4 additional RPi 5 DRM devices, probably. For the rest, e.g. UDL, I'm not
> sure it makes sense to expose scanout memory allocation via DMA heaps: AFAIK
> UDL needs CPU access to the buffers, it can't "scanout", and thus directly
> rendering via v3d to a "scanout-capable" buffer doesn't make sense. There will
> be a copy (CPU download) either way, and allocating via v3d wouldn't make a
> difference.

You as a developer may know that UDL is going to need CPU access, but
how does a generic userspace app know? Is it a case of falling back to
allocating via the renderer if there is no suitably named heap?

> > Can we add another function pointer to the struct drm_driver (or
> > similar) to do the allocation, and move the actual dmabuf handling
> > code into the core?
>
> Do you mean that the new logic introduced in this patch should be in DRM core
> to allow other drivers to more easily re-use it? Or do you mean something 
> else?

Yes, make it easy to reuse between drivers.

> Indeed, there is nothing vc4-specific in this patch, the only requirement is
> that the driver uses drm_gem_dma_helper. So this code could be moved into (or
> alongside) that helper in DRM core. However, maybe it would be best to wait to
> have a second user for this infrastructure before we move into core.

Upstreaming of the DSI / DPI / composite drivers for Pi5 should only
be a few months away, and they can all directly scanout.

I expect the Rockchip platforms to also fall into the same category as
the Pi, with Mali as the 3D IP, and the VOP block as the scanout
engine. Have I missed some detail that means that they aren't a second
user for this?

> > > > - Need to add !vc5 support.
> > >
> > > If by !vc5 you mean RPi0-3, then it's probably not needed there at all
> > > since it has a single node for both modesetting and rendering?
> >
> > That is true, but potentially vc4 could be rendering for scanout via UDL or 
> > SPI.
> > Is it easier to always have the dma-heap allocator for every DRM card
> > rather than making userspace mix and match depending on whether it is
> > all in one vs split?
>
> I don't think it's realistic to try to always make DMA heaps available for 
> each
> and every driver which might need it from day one. It's too big of a task. And
> it's an even bigger task to try to design a fully generic heap compatibility
> uAPI from day one. I'd much rather add the heaps one by one, if and when we
> figure that it makes sense, and incrementally work our way through.

Is it really that massive a task? We have the dma heap UAPI for
handling the allocations, so what new UAPI is required?

If it's a new function pointer in struct drm_driver, then the heap is
only created by the core if that function is defined using the driver
name. The function returns a struct dma_buf *.
Any driver using drm_gem_dma_helper can use the new helper function
that is basically your vc4_dma_heap_allocate. The "if
(WARN_ON_ONCE(!vc4->is_vc5))" could be handled by not setting the
function pointer on those platforms.

Sorry, I feel I must be missing some critical piece of information here.

  Dave

Re: [PATCH drm-misc-next v8 09/12] drm/gpuvm: reference count drm_gpuvm structures

[Danilo Krummrich]

On 11/9/23 17:03, Christian König wrote:

Am 09.11.23 um 16:50 schrieb Thomas Hellström:

[SNIP]



Did we get any resolution on this?

FWIW, my take on this is that it would be possible to get GPUVM to work both 
with and without internal refcounting; If with, the driver needs a vm close to 
resolve cyclic references, if without that's not necessary. If GPUVM is allowed 
to refcount in mappings and vm_bos, that comes with a slight performance drop 
but as Danilo pointed out, the VM lifetime problem iterating over a vm_bo's 
mapping becomes much easier and the code thus becomes easier to maintain moving 
forward. That convinced me it's a good thing.


I strongly believe you guys stumbled over one of the core problems with the VM 
here and I think that reference counting is the right answer to solving this.

The big question is that what is reference counted and in which direction does 
the dependency points, e.g. we have here VM, BO, BO_VM and Mapping objects.

Those patches here suggest a counted Mapping -> VM reference and I'm pretty sure that 
this isn't a good idea. What we should rather really have is a BO -> VM or BO_VM 
->VM reference. In other words that each BO which is part of the VM keeps a reference 
to the VM.


We have both. Please see the subsequent patch introducing VM_BO structures for 
that.

As I explained, mappings (struct drm_gpuva) keep a pointer to their VM they're 
mapped
in and besides that it doesn't make sense to free a VM that still contains 
mappings,
the reference count ensures that. This simply ensures memory safety.



BTW: At least in amdgpu we can have BOs which (temporary) doesn't have any 
mappings, but are still considered part of the VM.


That should be possible.





Another issue Christian brought up is that something intended to be embeddable 
(a base class) shouldn't really have its own refcount. I think that's a valid 
point. If you at some point need to derive from multiple such structs each 
having its own refcount, things will start to get weird. One way to resolve 
that would be to have the driver's subclass provide get() and put() ops, and 
export a destructor for the base-class, rather than to have the base-class 
provide the refcount and a destructor  ops.


GPUVM simply follows the same pattern we have with drm_gem_objects. And I think 
it makes
sense. Why would we want to embed two struct drm_gpuvm in a single driver 
structure?



Well, I have never seen stuff like that in the kernel. Might be that this 
works, but I would rather not try if avoidable.



That would also make it possible for the driver to decide the context for the 
put() call: If the driver needs to be able to call put() from irq / atomic 
context but the base-class'es destructor doesn't allow atomic context, the 
driver can push freeing out to a work item if needed.

Finally, the refcount overflow Christian pointed out. Limiting the number of 
mapping sounds like a reasonable remedy to me.


Well that depends, I would rather avoid having a dependency for mappings.

Taking the CPU VM handling as example as far as I know vm_area_structs doesn't 
grab a reference to their mm_struct either. Instead they get automatically 
destroyed when the mm_struct is destroyed.


Certainly, that would be possible. However, thinking about it, this might call 
for
huge trouble.

First of all, we'd still need to reference count a GPUVM and take a reference 
for each
VM_BO, as we do already. Now instead of simply increasing the reference count 
for each
mapping as well, we'd need a *mandatory* driver callback that is called when 
the GPUVM
reference count drops to zero. Maybe something like vm_destroy().

The reason is that GPUVM can't just remove all mappings from the tree nor can 
it free them
by itself, since drivers might use them for tracking their allocated page 
tables and/or
other stuff.

Now, let's think about the scope this callback might be called from. When a 
VM_BO is destroyed
the driver might hold a couple of locks (for Xe it would be the VM's shared 
dma-resv lock and
potentially the corresponding object's dma-resv lock if they're not the same 
already). If
destroying this VM_BO leads to the VM being destroyed, the drivers vm_destroy() 
callback would
be called with those locks being held as well.

I feel like doing this finally opens the doors of the locking hell entirely. I 
think we should
really avoid that.



Which makes sense in that case because when the mm_struct is gone the 
vm_area_struct doesn't make sense any more either.

What we clearly need is a reference to prevent the VM or at least the shared 
resv to go away to early.


Yeah, that was a good hint and we've covered that.



Regards,
Christian.



But I think all of this is fixable as follow-ups if needed, unless I'm missing 
something crucial.


Fully agree, I think at this point we should go ahead and land this series.



Just my 2 cents.

/Thomas

RE: [PATCH 02/17] dt-bindings: i2c: exynos5: add specific compatibles for existing SoC

[Alim Akhtar]

> -Original Message-
> From: Krzysztof Kozlowski 
> Sent: Wednesday, November 8, 2023 4:13 PM
> To: David Airlie ; Daniel Vetter ;
> Maarten Lankhorst ; Maxime Ripard
> ; Thomas Zimmermann ;
> Rob Herring ; Krzysztof Kozlowski
> ; Conor Dooley
> ; Alim Akhtar ; Andi
> Shyti ; Jonathan Cameron ; Lars-
> Peter Clausen ; Lee Jones ; Ulf
> Hansson ; Tomasz Figa ;
> Sylwester Nawrocki ; Linus Walleij
> ; Thierry Reding ; Uwe
> Kleine-König ; Alessandro Zummo
> ; Alexandre Belloni
> ; Greg Kroah-Hartman
> ; Jiri Slaby ; Liam
> Girdwood ; Mark Brown ;
> Jaehoon Chung ; Sam Protsenko
> ; dri-devel@lists.freedesktop.org;
> devicet...@vger.kernel.org; linux-ker...@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; linux-samsung-...@vger.kernel.org; linux-
> i...@vger.kernel.org; linux-...@vger.kernel.org; linux-...@vger.kernel.org;
> linux-g...@vger.kernel.org; linux-...@vger.kernel.org; linux-
> r...@vger.kernel.org; linux-ser...@vger.kernel.org; alsa-devel@alsa-
> project.org; linux-so...@vger.kernel.org
> Cc: Krzysztof Kozlowski 
> Subject: [PATCH 02/17] dt-bindings: i2c: exynos5: add specific compatibles for
> existing SoC
> 
> Samsung Exynos SoC reuses several devices from older designs, thus
> historically we kept the old (block's) compatible only.  This works fine and
> there is no bug here, however guidelines expressed in
> Documentation/devicetree/bindings/writing-bindings.rst state that:
> 1. Compatibles should be specific.
> 2. We should add new compatibles in case of bugs or features.
> 
> Add compatibles specific to each SoC in front of all old-SoC-like compatibles.
> 
> Signed-off-by: Krzysztof Kozlowski 
> 
> ---
> 
> I propose to take the patch through Samsung SoC (me). See cover letter for
> explanation.
> ---
>  Documentation/devicetree/bindings/i2c/i2c-exynos5.yaml | 10 +-
>  .../devicetree/bindings/soc/samsung/exynos-usi.yaml|  2 +-
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-exynos5.yaml
> b/Documentation/devicetree/bindings/i2c/i2c-exynos5.yaml
> index 3e52a0db6c41..c1f5d2cb7709 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-exynos5.yaml
> +++ b/Documentation/devicetree/bindings/i2c/i2c-exynos5.yaml
> @@ -25,7 +25,15 @@ properties:
>- samsung,exynos5250-hsi2c# Exynos5250 and Exynos5420
>- samsung,exynos5260-hsi2c# Exynos5260
>- samsung,exynos7-hsi2c   # Exynos7
> -  - samsung,exynosautov9-hsi2c  # ExynosAutoV9 and Exynos850
> +  - samsung,exynosautov9-hsi2c
> +  - items:
> +  - enum:
> +  - samsung,exynos5433-hsi2c
> +  - const: samsung,exynos7-hsi2c
> +  - items:
> +  - enum:
> +  - samsung,exynos850-hsi2c
Does this need an entry in allOf:? to indicate exynos850 also has 2 clocks?

> +  - const: samsung,exynosautov9-hsi2c
>- const: samsung,exynos5-hsi2c# Exynos5250 and Exynos5420
>  deprecated: true
> 
> diff --git a/Documentation/devicetree/bindings/soc/samsung/exynos-
> usi.yaml b/Documentation/devicetree/bindings/soc/samsung/exynos-
> usi.yaml
> index a6836904a4f8..5b7ab69546c4 100644
> --- a/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml
> +++ b/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml
> @@ -155,7 +155,7 @@ examples:
>  };
> 
>  hsi2c_0: i2c@1382 {
> -compatible = "samsung,exynosautov9-hsi2c";
> +compatible = "samsung,exynos850-hsi2c",
> + "samsung,exynosautov9-hsi2c";
>  reg = <0x1382 0xc0>;
>  interrupts = ;
>  #address-cells = <1>;
> --
> 2.34.1

Re: [Intel-gfx] [PATCH] drm/i915/quirk: Add quirk for devices with incorrect PWM frequency

[Dmitry Torokhov]

Hi Allen,

On Tue, Oct 17, 2023 at 06:01:39PM +, Allen Ballway wrote:
> Cyernet T10C has a bad default PWM frequency causing the display to
> strobe when the brightness is less than 100%. Create a new quirk to use
> the value from the BIOS rather than the default register value.
> 
> Signed-off-by: Allen Ballway 
> 
> ---
> 
>  .../gpu/drm/i915/display/intel_backlight.c|  3 ++-
>  drivers/gpu/drm/i915/display/intel_quirks.c   | 26 +++
>  drivers/gpu/drm/i915/display/intel_quirks.h   |  1 +
>  3 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c 
> b/drivers/gpu/drm/i915/display/intel_backlight.c
> index 2e8f17c045222..c4dcfece9deca 100644
> --- a/drivers/gpu/drm/i915/display/intel_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_backlight.c
> @@ -1388,7 +1388,8 @@ static int vlv_setup_backlight(struct intel_connector 
> *connector, enum pipe pipe
>   ctl = intel_de_read(i915, VLV_BLC_PWM_CTL(pipe));
>   panel->backlight.pwm_level_max = ctl >> 16;
> 
> - if (!panel->backlight.pwm_level_max)
> + if (!panel->backlight.pwm_level_max ||
> + intel_has_quirk(i915, QUIRK_IGNORE_DEFAULT_PWM_FREQUENCY))
>   panel->backlight.pwm_level_max = 
> get_backlight_max_vbt(connector);

I think it would be better if we did:

if (!intel_has_quirk(i915, QUIRK_IGNORE_DEFAULT_PWM_FREQUENCY)) {
ctl = intel_de_read(i915, VLV_BLC_PWM_CTL(pipe));
panel->backlight.pwm_level_max = ctl >> 16;
} else {
panel->backlight.pwm_level_max = 0;
}

if (!panel->backlight.pwm_level_max)
panel->backlight.pwm_level_max = 
get_backlight_max_vbt(connector);

The "else" branch can potentially be omitted if we know that backlight
member is initialized to 0 (I suspect it is).

> 
>   if (!panel->backlight.pwm_level_max)
> diff --git a/drivers/gpu/drm/i915/display/intel_quirks.c 
> b/drivers/gpu/drm/i915/display/intel_quirks.c
> index a280448df771a..ff6cb499428ce 100644
> --- a/drivers/gpu/drm/i915/display/intel_quirks.c
> +++ b/drivers/gpu/drm/i915/display/intel_quirks.c
> @@ -65,6 +65,12 @@ static void quirk_no_pps_backlight_power_hook(struct 
> drm_i915_private *i915)
>   drm_info(>drm, "Applying no pps backlight power quirk\n");
>  }
> 
> +static void quirk_ignore_default_pwm_frequency(struct drm_i915_private *i915)
> +{
> + intel_set_quirk(i915, QUIRK_IGNORE_DEFAULT_PWM_FREQUENCY);
> + drm_info(>drm, "Applying ignore default pwm frequency quirk");
> +}
> +
>  struct intel_quirk {
>   int device;
>   int subsystem_vendor;
> @@ -90,6 +96,12 @@ static int intel_dmi_no_pps_backlight(const struct 
> dmi_system_id *id)
>   return 1;
>  }
> 
> +static int intel_dmi_ignore_default_pwm_frequency(const struct dmi_system_id 
> *id)
> +{
> + DRM_INFO("Default PWM frequency is incorrect and is overridden on 
> %s\n", id->ident);
> + return 1;
> +}
> +
>  static const struct intel_dmi_quirk intel_dmi_quirks[] = {
>   {
>   .dmi_id_list = &(const struct dmi_system_id[]) {
> @@ -136,6 +148,20 @@ static const struct intel_dmi_quirk intel_dmi_quirks[] = 
> {
>   },
>   .hook = quirk_no_pps_backlight_power_hook,
>   },
> + {
> + .dmi_id_list = &(const struct dmi_system_id[]) {
> + {
> + .callback = 
> intel_dmi_ignore_default_pwm_frequency,
> + .ident = "Cybernet T10C Tablet",
> + .matches = {DMI_EXACT_MATCH(DMI_BOARD_VENDOR,
> + "Cybernet 
> Manufacturing Inc."),
> + DMI_EXACT_MATCH(DMI_BOARD_NAME, 
> "T10C Tablet"),
> + },
> + },
> + { }
> + },
> + .hook = quirk_ignore_default_pwm_frequency,
> + },
>  };
> 
>  static struct intel_quirk intel_quirks[] = {
> diff --git a/drivers/gpu/drm/i915/display/intel_quirks.h 
> b/drivers/gpu/drm/i915/display/intel_quirks.h
> index 10a4d163149fd..70589505e5a0e 100644
> --- a/drivers/gpu/drm/i915/display/intel_quirks.h
> +++ b/drivers/gpu/drm/i915/display/intel_quirks.h
> @@ -17,6 +17,7 @@ enum intel_quirk_id {
>   QUIRK_INVERT_BRIGHTNESS,
>   QUIRK_LVDS_SSC_DISABLE,
>   QUIRK_NO_PPS_BACKLIGHT_POWER_HOOK,
> + QUIRK_IGNORE_DEFAULT_PWM_FREQUENCY

You want a trailing comma here.

>  };
> 
>  void intel_init_quirks(struct drm_i915_private *i915);
> --
> 2.42.0.655.g421f12c284-goog
> 

Thanks.

-- 
Dmitry

Re: [PATCH v8 04/12] dt-bindings: phy: amlogic,g12a-mipi-dphy-analog: drop unneeded reg property and example

[Conor Dooley]

On Thu, Nov 09, 2023 at 10:00:05AM +0100, Neil Armstrong wrote:
> Now this bindings is referred from amlogic,meson-gx-hhi-sysctrl.yaml and is
> documented as a subnode of a simple-mfd, drop the invalid reg property.

I'd expect a note here tbh about how removing reg & relying on being a
subnode of the simple-mfd is safe to do. It looks like your driver
was added at the same time as this binding & it was always documented as
being a child of the simple-mfd system controller, so I'd kinda expect
to see a Fixes tag on this patch..

Am I missing something?

> 
> Also drop the unnecessary example, the top level bindings example should
> be enough.
> 
> Signed-off-by: Neil Armstrong 
> ---
>  .../bindings/phy/amlogic,g12a-mipi-dphy-analog.yaml  | 12 
> 
>  1 file changed, 12 deletions(-)
> 
> diff --git 
> a/Documentation/devicetree/bindings/phy/amlogic,g12a-mipi-dphy-analog.yaml 
> b/Documentation/devicetree/bindings/phy/amlogic,g12a-mipi-dphy-analog.yaml
> index c8c83acfb871..81c2654b7e57 100644
> --- a/Documentation/devicetree/bindings/phy/amlogic,g12a-mipi-dphy-analog.yaml
> +++ b/Documentation/devicetree/bindings/phy/amlogic,g12a-mipi-dphy-analog.yaml
> @@ -16,20 +16,8 @@ properties:
>"#phy-cells":
>  const: 0
>  
> -  reg:
> -maxItems: 1
> -
>  required:
>- compatible
> -  - reg
>- "#phy-cells"
>  
>  additionalProperties: false
> -
> -examples:
> -  - |
> -phy@0 {
> -  compatible = "amlogic,g12a-mipi-dphy-analog";
> -  reg = <0x0 0xc>;
> -  #phy-cells = <0>;
> -};
> 
> -- 
> 2.34.1
> 


signature.asc
Description: PGP signature

RE: [PATCH 01/17] dt-bindings: hwinfo: samsung,exynos-chipid: add specific compatibles for existing SoC

[Alim Akhtar]

> -Original Message-
> From: Krzysztof Kozlowski 
> Sent: Wednesday, November 8, 2023 4:13 PM
> To: David Airlie ; Daniel Vetter ;
> Maarten Lankhorst ; Maxime Ripard
> ; Thomas Zimmermann ;
> Rob Herring ; Krzysztof Kozlowski
> ; Conor Dooley
> ; Alim Akhtar ; Andi
> Shyti ; Jonathan Cameron ; Lars-
> Peter Clausen ; Lee Jones ; Ulf
> Hansson ; Tomasz Figa ;
> Sylwester Nawrocki ; Linus Walleij
> ; Thierry Reding ; Uwe
> Kleine-König ; Alessandro Zummo
> ; Alexandre Belloni
> ; Greg Kroah-Hartman
> ; Jiri Slaby ; Liam
> Girdwood ; Mark Brown ;
> Jaehoon Chung ; Sam Protsenko
> ; dri-devel@lists.freedesktop.org;
> devicet...@vger.kernel.org; linux-ker...@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; linux-samsung-...@vger.kernel.org; linux-
> i...@vger.kernel.org; linux-...@vger.kernel.org; linux-...@vger.kernel.org;
> linux-g...@vger.kernel.org; linux-...@vger.kernel.org; linux-
> r...@vger.kernel.org; linux-ser...@vger.kernel.org; alsa-devel@alsa-
> project.org; linux-so...@vger.kernel.org
> Cc: Krzysztof Kozlowski 
> Subject: [PATCH 01/17] dt-bindings: hwinfo: samsung,exynos-chipid: add
> specific compatibles for existing SoC
> 
> Samsung Exynos SoC reuses several devices from older designs, thus
> historically we kept the old (block's) compatible only.  This works fine and
> there is no bug here, however guidelines expressed in
> Documentation/devicetree/bindings/writing-bindings.rst state that:
> 1. Compatibles should be specific.
> 2. We should add new compatibles in case of bugs or features.
> 
> Add compatibles specific to each SoC in front of all old-SoC-like compatibles.
> 
> Signed-off-by: Krzysztof Kozlowski 
> 
Reviewed-by: Alim Akhtar 

> ---
> 
> I propose to take the patch through Samsung SoC (me). See cover letter for
> explanation.
> ---
>  .../bindings/hwinfo/samsung,exynos-chipid.yaml  | 17 ++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/hwinfo/samsung,exynos-
> chipid.yaml
> b/Documentation/devicetree/bindings/hwinfo/samsung,exynos-chipid.yaml
> index 95cbdcb56efe..45f3d468db7c 100644
> --- a/Documentation/devicetree/bindings/hwinfo/samsung,exynos-
> chipid.yaml
> +++ b/Documentation/devicetree/bindings/hwinfo/samsung,exynos-
> chipid.yam
> +++ l
> @@ -11,9 +11,20 @@ maintainers:
> 
>  properties:
>compatible:
> -enum:
> -  - samsung,exynos4210-chipid
> -  - samsung,exynos850-chipid
> +oneOf:
> +  - enum:
> +  - samsung,exynos4210-chipid
> +  - samsung,exynos850-chipid
> +  - items:
> +  - enum:
> +  - samsung,exynos5433-chipid
> +  - samsung,exynos7-chipid
> +  - const: samsung,exynos4210-chipid
> +  - items:
> +  - enum:
> +  - samsung,exynos7885-chipid
> +  - samsung,exynosautov9-chipid
> +  - const: samsung,exynos850-chipid
> 
>reg:
>  maxItems: 1
> --
> 2.34.1

Re: [PATCH v7 0/7] incorporate pm runtime framework and eDP clean up

[Kuogee Hsieh]

On 11/8/2023 10:27 AM, Abhinav Kumar wrote:



On 11/8/2023 10:10 AM, Kuogee Hsieh wrote:


On 11/7/2023 1:23 PM, Dmitry Baryshkov wrote:
On Tue, 7 Nov 2023 at 23:01, Kuogee Hsieh  
wrote:


On 11/6/2023 5:55 PM, Dmitry Baryshkov wrote:
On Sat, 7 Oct 2023 at 01:55, Kuogee Hsieh 
 wrote:
The purpose of this patch series is to incorporate pm runtime 
framework
into MSM eDP/DP driver so that eDP panel can be detected by DRM 
eDP panel
driver during system probe time. During incorporating procedure, 
original
customized pm realted fucntions, such as dp_pm_prepare(), 
dp_pm_suspend(),
dp_pm_resume() and dp_pm_prepare(), are removed and replaced with 
functions
provided by pm runtiem framework such as 
pm_runtime_force_suspend() and
pm_runtime_force_resume(). In addition, both eDP aux-bus and irq 
handler

are bound at system probe time too.

With this patchset in place I can crash the board using the following
sequence (SM8350-HDK):

- plug the USBC DP dongle
- run modetest at any mode, don't press Enter yet
- unplug the dongle
- press Enter to stop modetest

=> the board resets to Sahara.

Please ping me if you need any additional information from my side.

questiosn,

1) which dongle are you used?

I have used several Dell and Hama USB-C dongles.


2) what code branch shoud I used to duplicate this problem.

I have pushed my kernel tree to
git.codelinaro.org/dmitry.baryshkov/linux.git, branch test-dp-rpm
I had several UCSI patches on top, but they should not be relevant.
git.codelinaro.org/dmitry.baryshkov/linux.git, branch test-dp-rpm <== 
I synced out his branch and it is still work at my chromebook Kodiak 
DUT.




Perhaps the gap in test results with the same tree is due to internal 
hpd vs hpd pin. We need to try this on a device which does not use 
internal hpd.



Hi Dmitry,

I have two more questions,

1) are you see test pattern shows at external DP when you run modetest?
2) is *.kcrash file created under /var/spool/crash/ when system crashed. 
If it is, can you share it?


Thanks,




I can not duplicate  system crash problem at my setup kodiak (SM7325)
chrome book with my PM runtime patch series.

my code base is Linux 6.6-rc2 + pm runtime patch series (7 patches)

I did:

1) plugin either apple dongle (DP-to-HDMI) + 1080p display or DP typeC
cable directly to 1080p display

2)  stop ui

3) /usr/bin/modetest -M msm -s 34:1920x1080 (see test pattern show at
display)

4) unplug apple dongle or DP typeC cable

5) hit enter key

6) start ui

7) display back to login page of chrome book

Re: [PATCH v8 03/12] dt-bindings: phy: amlogic,meson-axg-mipi-pcie-analog: drop text about parent syscon and drop example

[Conor Dooley]

On Thu, Nov 09, 2023 at 10:00:04AM +0100, Neil Armstrong wrote:
> Since this bindings is referred from amlogic,meson-gx-hhi-sysctrl.yaml, drop 
> the now
> useless description about the parent node and also drop the unnecessary 
> example.
> 
> Signed-off-by: Neil Armstrong 

Acked-by: Conor Dooley 

Cheers,
COnor.

> ---
>  .../phy/amlogic,meson-axg-mipi-pcie-analog.yaml | 17 
> -
>  1 file changed, 17 deletions(-)
> 
> diff --git 
> a/Documentation/devicetree/bindings/phy/amlogic,meson-axg-mipi-pcie-analog.yaml
>  
> b/Documentation/devicetree/bindings/phy/amlogic,meson-axg-mipi-pcie-analog.yaml
> index 009a39808318..70def36e5688 100644
> --- 
> a/Documentation/devicetree/bindings/phy/amlogic,meson-axg-mipi-pcie-analog.yaml
> +++ 
> b/Documentation/devicetree/bindings/phy/amlogic,meson-axg-mipi-pcie-analog.yaml
> @@ -9,16 +9,6 @@ title: Amlogic AXG shared MIPI/PCIE analog PHY
>  maintainers:
>- Remi Pommarel 
>  
> -description: |+
> -  The Everything-Else Power Domains node should be the child of a syscon
> -  node with the required property:
> -
> -  - compatible: Should be the following:
> -"amlogic,meson-gx-hhi-sysctrl", "simple-mfd", "syscon"
> -
> -  Refer to the bindings described in
> -  Documentation/devicetree/bindings/mfd/syscon.yaml
> -
>  properties:
>compatible:
>  const: amlogic,axg-mipi-pcie-analog-phy
> @@ -31,10 +21,3 @@ required:
>- "#phy-cells"
>  
>  additionalProperties: false
> -
> -examples:
> -  - |
> -mpphy: phy {
> -  compatible = "amlogic,axg-mipi-pcie-analog-phy";
> -  #phy-cells = <0>;
> -};
> 
> -- 
> 2.34.1
> 


signature.asc
Description: PGP signature

Re: [RFC PATCH 2/2] vc4: introduce DMA-BUF heap

[Simon Ser]

On Thursday, November 9th, 2023 at 16:42, Dave Stevenson 
 wrote:

> > > - What would be a good name for the heap? "vc4" is maybe a bit naive and
> > >   not precise enough. Something with "cma"? Do we need to plan a naming
> > >   scheme to accomodate for multiple vc4 devices?
> >
> > That's a general issue though that happens with pretty much all devices
> > with a separate node for modesetting and rendering, so I don't think
> > addressing it only for vc4 make sense, we should make it generic.
> >
> > So maybe something like "scanout"?
> >
> > One thing we need to consider too is that the Pi5 will have multiple
> > display nodes (4(?) iirc) with different capabilities, vc4 being only
> > one of them. This will impact that solution too.
> 
> It does need to scale.
> 
> Pi5 adds 4 additional DRM devices (2xDSI, 1xDPI, and 1xComposite
> Video), and just this last week I've been running Wayfire with TinyDRM
> drivers for SPI displays and UDL (DisplayLink) outputs as well.
> Presumably all of those drivers need to have appropriate hooks added
> so they each expose a dma-heap to enable scanout buffers to be
> allocated.

I'm not sure this makes sense necessarily for all of these devices. For vc4 and
the 4 additional RPi 5 DRM devices, probably. For the rest, e.g. UDL, I'm not
sure it makes sense to expose scanout memory allocation via DMA heaps: AFAIK
UDL needs CPU access to the buffers, it can't "scanout", and thus directly
rendering via v3d to a "scanout-capable" buffer doesn't make sense. There will
be a copy (CPU download) either way, and allocating via v3d wouldn't make a
difference.

> Can we add another function pointer to the struct drm_driver (or
> similar) to do the allocation, and move the actual dmabuf handling
> code into the core?

Do you mean that the new logic introduced in this patch should be in DRM core
to allow other drivers to more easily re-use it? Or do you mean something else?

Indeed, there is nothing vc4-specific in this patch, the only requirement is
that the driver uses drm_gem_dma_helper. So this code could be moved into (or
alongside) that helper in DRM core. However, maybe it would be best to wait to
have a second user for this infrastructure before we move into core.

> > > - Need to add !vc5 support.
> >
> > If by !vc5 you mean RPi0-3, then it's probably not needed there at all
> > since it has a single node for both modesetting and rendering?
> 
> That is true, but potentially vc4 could be rendering for scanout via UDL or 
> SPI.
> Is it easier to always have the dma-heap allocator for every DRM card
> rather than making userspace mix and match depending on whether it is
> all in one vs split?

I don't think it's realistic to try to always make DMA heaps available for each
and every driver which might need it from day one. It's too big of a task. And
it's an even bigger task to try to design a fully generic heap compatibility
uAPI from day one. I'd much rather add the heaps one by one, if and when we
figure that it makes sense, and incrementally work our way through.

Re: [PATCH v8 02/12] dt-bindings: soc: amlogic, meson-gx-hhi-sysctrl: add example covering meson-axg-hhi-sysctrl

[Conor Dooley]

On Thu, Nov 09, 2023 at 10:00:03AM +0100, Neil Armstrong wrote:
> Add a thirst example covering the meson-axg-hhi-sysctrl variant and more

What on earth is a thirst example? Some sort of "hysterical raisins"
type of thing?

My confusion about that word aside,
Acked-by: Conor Dooley 

Cheers,
Conor.

> importantly the phy subnode.
> 
> Signed-off-by: Neil Armstrong 
> ---
>  .../soc/amlogic/amlogic,meson-gx-hhi-sysctrl.yaml  | 41 
> ++
>  1 file changed, 41 insertions(+)
> 
> diff --git 
> a/Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-gx-hhi-sysctrl.yaml
>  
> b/Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-gx-hhi-sysctrl.yaml
> index 16977e4e4357..2edf4ccea845 100644
> --- 
> a/Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-gx-hhi-sysctrl.yaml
> +++ 
> b/Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-gx-hhi-sysctrl.yaml
> @@ -158,3 +158,44 @@ examples:
>  };
>  };
>  };
> +
> +bus@ff63c000 {
> +compatible = "simple-bus";
> +reg = <0xff63c000 0x1c00>;
> +#address-cells = <1>;
> +#size-cells = <1>;
> +ranges = <0x0 0xff63c000 0x1c00>;
> +
> +system-controller@0 {
> +compatible = "amlogic,meson-axg-hhi-sysctrl", "simple-mfd", 
> "syscon";
> +reg = <0 0x400>;
> +
> +clock-controller {
> +compatible = "amlogic,axg-clkc";
> +#clock-cells = <1>;
> +clocks = <>;
> +clock-names = "xtal";
> +};
> +
> +power-controller {
> +compatible = "amlogic,meson-axg-pwrc";
> +#power-domain-cells = <1>;
> +amlogic,ao-sysctrl = <_AO>;
> +
> +resets = <_viu>,
> + <_venc>,
> + <_vcbus>,
> + <_vencl>,
> + <_vid_lock>;
> +reset-names = "viu", "venc", "vcbus", "vencl", "vid_lock";
> +clocks = <_vpu>, <_vapb>;
> +clock-names = "vpu", "vapb";
> +};
> +
> +phy {
> +compatible = "amlogic,axg-mipi-pcie-analog-phy";
> +#phy-cells = <0>;
> +status = "disabled";
> +};
> +};
> +};
> 
> -- 
> 2.34.1
> 


signature.asc
Description: PGP signature

[PATCH 6/6] drm/todo: Add entry about implementing buffer age for damage tracking

[Javier Martinez Canillas]

Currently, only damage tracking for frame damage is supported. If a driver
needs to do buffer damage (e.g: the framebuffer attached to plane's state
has changed since the last page-flip), the damage helpers just fallback to
a full plane update.

Add en entry in the TODO about implementing buffer age or any other damage
accumulation algorithm for buffer damage handling.

Suggested-by: Simon Ser 
Signed-off-by: Javier Martinez Canillas 
---

 Documentation/gpu/todo.rst | 20 
 1 file changed, 20 insertions(+)

diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index 03fe5d1247be..adaa154210a0 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -765,6 +765,26 @@ Contact: Hans de Goede
 
 Level: Advanced
 
+Buffer age or other damage accumulation algorithm for buffer damage handling
+
+
+Drivers that do per-buffer uploads, need a buffer damage handling (rather than
+frame damage like drivers that do per-plane or per-CRTC uploads), but there is
+no support to get the buffer age or any other damage accumulation algorithm.
+
+For this reason, the damage helpers just fallback to a full plane update if the
+framebuffer attached to a plane has changed since the last page-flip.
+
+This should be improved to get damage tracking properly working on drivers that
+do per-buffer uploads.
+
+More information about damage tracking and references to learning materials in
+`Damage Tracking Properties 
`_
+
+Contact: Javier Martinez Canillas 
+
+Level: Advanced
+
 Outside DRM
 ===
 
-- 
2.41.0

[PATCH 5/6] drm/plane: Extend damage tracking kernel-doc

[Javier Martinez Canillas]

The "Damage Tracking Properties" section in the documentation doesn't have
info about the two type of damage handling: frame damage vs buffer damage.

Add that to the section, mention the different helpers that should be used
by drivers depending on the damage handling type used and refer to sites
that have more content about damage types and damage tracking in general.

Suggested-by: Simon Ser 
Signed-off-by: Javier Martinez Canillas 
---

 drivers/gpu/drm/drm_damage_helper.c | 10 ++
 drivers/gpu/drm/drm_plane.c | 22 +++---
 2 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_damage_helper.c 
b/drivers/gpu/drm/drm_damage_helper.c
index b72062c9d31c..ac9986da7d7c 100644
--- a/drivers/gpu/drm/drm_damage_helper.c
+++ b/drivers/gpu/drm/drm_damage_helper.c
@@ -367,8 +367,9 @@ static bool __drm_atomic_helper_damage_merged(const struct 
drm_plane_state *old_
  * This function merges any valid plane damage clips into one rectangle and
  * returns it in @rect.
  *
- * For details see: drm_atomic_helper_damage_iter_init() and
- * drm_atomic_helper_damage_iter_next().
+ * For details see: drm_atomic_helper_damage_iter_init(),
+ * drm_atomic_helper_damage_iter_next() and
+ * `Damage Tracking Properties`_.
  *
  * Note that this helper is for drivers that do per-plane uploads and expect
  * to handle frame damages. Drivers that do per-buffer uploads instead should
@@ -400,8 +401,9 @@ EXPORT_SYMBOL(drm_atomic_helper_damage_merged);
  * full plane update should happen. It also ensure helper iterator will return
  * _plane_state.src as damage.
  *
- * For details see: drm_atomic_helper_buffer_damage_iter_init() and
- * drm_atomic_helper_damage_iter_next().
+ * For details see: drm_atomic_helper_buffer_damage_iter_init(),
+ * drm_atomic_helper_damage_iter_next() and
+ * `Damage Tracking Properties`_.
  *
  * Returns:
  * True if there is valid buffer damage otherwise false.
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 24e7998d1731..f137a99b3435 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -1439,9 +1439,25 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
  *
  * Drivers that are interested in damage interface for plane should enable
  * FB_DAMAGE_CLIPS property by calling drm_plane_enable_fb_damage_clips().
- * Drivers implementing damage can use drm_atomic_helper_damage_iter_init() and
- * drm_atomic_helper_damage_iter_next() helper iterator function to get damage
- * rectangles clipped to _plane_state.src.
+ *
+ * Note that there are two types of damage handling: frame damage and buffer
+ * damage. The drivers implementing a per-plane or per-CRTC upload target and
+ * need to handle frame damage can use drm_atomic_helper_damage_iter_init(),
+ * but drivers implementing a per-buffer upload target and need to handle 
buffer
+ * damage should use drm_atomic_helper_buffer_damage_iter_init() helper 
instead.
+ *
+ * Once the iterator has been initialized by the damage helpers mentioned 
above,
+ * the drm_atomic_helper_damage_iter_next() helper iterator function can be 
used
+ * to get damage rectangles clipped to _plane_state.src.
+ *
+ * The type of damage handling implemented depends on the driver's upload 
target
+ * but notice that when using swap buffers, the returned damage rectangle is 
the
+ * _plane_state.src, since a full plane update should happen. There is no
+ * buffer age support or similar damage accumulation algorithm implemented yet.
+ *
+ * For more information about the two type of damage, see:
+ * 
https://registry.khronos.org/EGL/extensions/KHR/EGL_KHR_swap_buffers_with_damage.txt
+ * https://emersion.fr/blog/2019/intro-to-damage-tracking/
  */
 
 /**
-- 
2.41.0

[PATCH 2/6] drm: Add drm_atomic_helper_buffer_damage_{iter_init, merged}() helpers

[Javier Martinez Canillas]

To be used by drivers that do per-buffer (e.g: virtio-gpu) uploads (rather
than per-plane uploads), since these type of drivers need to handle buffer
damages instead of frame damages.

The drm_atomic_helper_buffer_damage_iter_init() has the same logic than
drm_atomic_helper_damage_iter_init() but it also takes into account if the
framebuffer attached to plane's state has changed since the last update.

And the drm_atomic_helper_buffer_damage_merged() is just a version of the
drm_atomic_helper_damage_merged() helper, but it uses the iter_init helper
that is mentioned above.

Fixes: 01f05940a9a7 ("drm/virtio: Enable fb damage clips property for the 
primary plane")
Cc:  # v6.4+
Reported-by: nerdopolis 
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218115
Suggested-by: Sima Vetter 
Signed-off-by: Javier Martinez Canillas 
---

 drivers/gpu/drm/drm_damage_helper.c | 79 ++---
 include/drm/drm_damage_helper.h |  7 +++
 2 files changed, 80 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_damage_helper.c 
b/drivers/gpu/drm/drm_damage_helper.c
index aa2325567918..b72062c9d31c 100644
--- a/drivers/gpu/drm/drm_damage_helper.c
+++ b/drivers/gpu/drm/drm_damage_helper.c
@@ -204,7 +204,8 @@ EXPORT_SYMBOL(drm_atomic_helper_dirtyfb);
 static void
 __drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter 
*iter,
 const struct drm_plane_state *old_state,
-const struct drm_plane_state *state)
+const struct drm_plane_state *state,
+bool buffer_damage)
 {
struct drm_rect src;
memset(iter, 0, sizeof(*iter));
@@ -223,7 +224,8 @@ __drm_atomic_helper_damage_iter_init(struct 
drm_atomic_helper_damage_iter *iter,
iter->plane_src.x2 = (src.x2 >> 16) + !!(src.x2 & 0x);
iter->plane_src.y2 = (src.y2 >> 16) + !!(src.y2 & 0x);
 
-   if (!iter->clips || !drm_rect_equals(>src, _state->src)) {
+   if (!iter->clips || !drm_rect_equals(>src, _state->src) ||
+   (buffer_damage && old_state->fb != state->fb)) {
iter->clips = NULL;
iter->num_clips = 0;
iter->full_update = true;
@@ -243,6 +245,10 @@ __drm_atomic_helper_damage_iter_init(struct 
drm_atomic_helper_damage_iter *iter,
  * update). Currently this iterator returns full plane src in case plane src
  * changed but that can be changed in future to return damage.
  *
+ * Note that this helper is for drivers that do per-plane uploads and expect
+ * to handle frame damages. Drivers that do per-buffer uploads instead should
+ * use @drm_atomic_helper_buffer_damage_iter_init() that handles buffer 
damages.
+ *
  * For the case when plane is not visible or plane update should not happen the
  * first call to iter_next will return false. Note that this helper use clipped
  * _plane_state.src, so driver calling this helper should have called
@@ -253,10 +259,37 @@ drm_atomic_helper_damage_iter_init(struct 
drm_atomic_helper_damage_iter *iter,
   const struct drm_plane_state *old_state,
   const struct drm_plane_state *state)
 {
-   __drm_atomic_helper_damage_iter_init(iter, old_state, state);
+   __drm_atomic_helper_damage_iter_init(iter, old_state, state, false);
 }
 EXPORT_SYMBOL(drm_atomic_helper_damage_iter_init);
 
+/**
+ * drm_atomic_helper_buffer_damage_iter_init - Initialize the buffer damage 
iterator.
+ * @iter: The iterator to initialize.
+ * @old_state: Old plane state for validation.
+ * @state: Plane state from which to iterate the damage clips.
+ *
+ * Initialize an iterator, which clips buffer damage
+ * _plane_state.fb_damage_clips to plane _plane_state.src. This 
iterator
+ * returns full plane src in case buffer damage is not present because 
user-space
+ * didn't sent, driver discarded it (it want to do full plane update) or the 
plane
+ * @state has an attached framebuffer that is different than the one in @state 
(it
+ * has changed since the last plane update).
+ *
+ * For the case when plane is not visible or plane update should not happen the
+ * first call to iter_next will return false. Note that this helper use clipped
+ * _plane_state.src, so driver calling this helper should have called
+ * drm_atomic_helper_check_plane_state() earlier.
+ */
+void
+drm_atomic_helper_buffer_damage_iter_init(struct drm_atomic_helper_damage_iter 
*iter,
+ const struct drm_plane_state 
*old_state,
+ const struct drm_plane_state *state)
+{
+   __drm_atomic_helper_damage_iter_init(iter, old_state, state, true);
+}
+EXPORT_SYMBOL(drm_atomic_helper_buffer_damage_iter_init);
+
 /**
  * drm_atomic_helper_damage_iter_next - Advance the damage iterator.
  * @iter: The iterator to advance.
@@ -301,7 +334,8 @@ 

[PATCH 4/6] drm/vmwgfx: Use drm_atomic_helper_buffer_damage_iter_init() for buffer damage

[Javier Martinez Canillas]

The driver does per-buffer uploads. It needs to use the damage helper that
handles buffer damages, rather than the helper that handles frame damages.

Suggested-by: Sima Vetter 
Signed-off-by: Javier Martinez Canillas 
---

 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c 
b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index 1489ad73c103..91cda125784e 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -2912,7 +2912,7 @@ int vmw_du_helper_plane_update(struct vmw_du_update_plane 
*update)
 * Iterate in advance to check if really need plane update and find the
 * number of clips that actually are in plane src for fifo allocation.
 */
-   drm_atomic_helper_damage_iter_init(, old_state, state);
+   drm_atomic_helper_buffer_damage_iter_init(, old_state, state);
drm_atomic_for_each_plane_damage(, )
num_hits++;
 
-- 
2.41.0

[PATCH 1/6] drm: Move drm_atomic_helper_damage_{iter_init, merged}() to helpers

[Javier Martinez Canillas]

We need a similar drm_atomic_helper_buffer_damage_merged() helper function
that takes into account if a framebuffer attached to the plane has changed
since the last plane update (page-flip).

Since both damage helpers will share most of the current logic, move it to
an internal helper. The drm_atomic_helper_buffer_damage_merged() will have
to use a different drm_atomic_helper_buffer_damage_iter_init() function so
move that logic also to an internal helper.

Fixes: 01f05940a9a7 ("drm/virtio: Enable fb damage clips property for the 
primary plane")
Cc:  # v6.4+
Reported-by: nerdopolis 
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218115
Suggested-by: Sima Vetter 
Signed-off-by: Javier Martinez Canillas 
---

 drivers/gpu/drm/drm_damage_helper.c | 95 +
 1 file changed, 55 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/drm_damage_helper.c 
b/drivers/gpu/drm/drm_damage_helper.c
index d8b2955e88fd..aa2325567918 100644
--- a/drivers/gpu/drm/drm_damage_helper.c
+++ b/drivers/gpu/drm/drm_damage_helper.c
@@ -201,28 +201,10 @@ int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb,
 }
 EXPORT_SYMBOL(drm_atomic_helper_dirtyfb);
 
-/**
- * drm_atomic_helper_damage_iter_init - Initialize the damage iterator.
- * @iter: The iterator to initialize.
- * @old_state: Old plane state for validation.
- * @state: Plane state from which to iterate the damage clips.
- *
- * Initialize an iterator, which clips plane damage
- * _plane_state.fb_damage_clips to plane _plane_state.src. This 
iterator
- * returns full plane src in case damage is not present because either
- * user-space didn't sent or driver discarded it (it want to do full plane
- * update). Currently this iterator returns full plane src in case plane src
- * changed but that can be changed in future to return damage.
- *
- * For the case when plane is not visible or plane update should not happen the
- * first call to iter_next will return false. Note that this helper use clipped
- * _plane_state.src, so driver calling this helper should have called
- * drm_atomic_helper_check_plane_state() earlier.
- */
-void
-drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
-  const struct drm_plane_state *old_state,
-  const struct drm_plane_state *state)
+static void
+__drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter 
*iter,
+const struct drm_plane_state *old_state,
+const struct drm_plane_state *state)
 {
struct drm_rect src;
memset(iter, 0, sizeof(*iter));
@@ -247,6 +229,32 @@ drm_atomic_helper_damage_iter_init(struct 
drm_atomic_helper_damage_iter *iter,
iter->full_update = true;
}
 }
+
+/**
+ * drm_atomic_helper_damage_iter_init - Initialize the damage iterator.
+ * @iter: The iterator to initialize.
+ * @old_state: Old plane state for validation.
+ * @state: Plane state from which to iterate the damage clips.
+ *
+ * Initialize an iterator, which clips plane damage
+ * _plane_state.fb_damage_clips to plane _plane_state.src. This 
iterator
+ * returns full plane src in case damage is not present because either
+ * user-space didn't sent or driver discarded it (it want to do full plane
+ * update). Currently this iterator returns full plane src in case plane src
+ * changed but that can be changed in future to return damage.
+ *
+ * For the case when plane is not visible or plane update should not happen the
+ * first call to iter_next will return false. Note that this helper use clipped
+ * _plane_state.src, so driver calling this helper should have called
+ * drm_atomic_helper_check_plane_state() earlier.
+ */
+void
+drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
+  const struct drm_plane_state *old_state,
+  const struct drm_plane_state *state)
+{
+   __drm_atomic_helper_damage_iter_init(iter, old_state, state);
+}
 EXPORT_SYMBOL(drm_atomic_helper_damage_iter_init);
 
 /**
@@ -291,24 +299,9 @@ drm_atomic_helper_damage_iter_next(struct 
drm_atomic_helper_damage_iter *iter,
 }
 EXPORT_SYMBOL(drm_atomic_helper_damage_iter_next);
 
-/**
- * drm_atomic_helper_damage_merged - Merged plane damage
- * @old_state: Old plane state for validation.
- * @state: Plane state from which to iterate the damage clips.
- * @rect: Returns the merged damage rectangle
- *
- * This function merges any valid plane damage clips into one rectangle and
- * returns it in @rect.
- *
- * For details see: drm_atomic_helper_damage_iter_init() and
- * drm_atomic_helper_damage_iter_next().
- *
- * Returns:
- * True if there is valid plane damage otherwise false.
- */
-bool drm_atomic_helper_damage_merged(const struct drm_plane_state *old_state,
-struct drm_plane_state *state,
-  

[PATCH 3/6] drm/virtio: Use drm_atomic_helper_buffer_damage_merged() for buffer damage

[Javier Martinez Canillas]

The driver does per-buffer uploads. It needs to use the damage helper that
handles buffer damages, rather than the helper that handles frame damages.

Fixes: 01f05940a9a7 ("drm/virtio: Enable fb damage clips property for the 
primary plane")
Cc:  # v6.4+
Reported-by: nerdopolis 
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218115
Suggested-by: Sima Vetter 
Signed-off-by: Javier Martinez Canillas 
---

 drivers/gpu/drm/virtio/virtgpu_plane.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c 
b/drivers/gpu/drm/virtio/virtgpu_plane.c
index a2e045f3a000..1adfd9813cde 100644
--- a/drivers/gpu/drm/virtio/virtgpu_plane.c
+++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
@@ -183,7 +183,7 @@ static void virtio_gpu_primary_plane_update(struct 
drm_plane *plane,
return;
}
 
-   if (!drm_atomic_helper_damage_merged(old_state, plane->state, ))
+   if (!drm_atomic_helper_buffer_damage_merged(old_state, plane->state, 
))
return;
 
bo = gem_to_virtio_gpu_obj(plane->state->fb->obj[0]);
-- 
2.41.0

[PATCH 0/6] drm: Allow the damage helpers to handle buffer damage

[Javier Martinez Canillas]

Hello,

This series is to fix an issue that surfaced after damage clipping was
enabled for the virtio-gpu by commit 01f05940a9a7 ("drm/virtio: Enable
fb damage clips property for the primary plane").

After that change, flickering artifacts was reported to be present with
both weston and wlroots wayland compositors when running in a virtual
machine. The cause was identified by Sima Vetter, who pointed out that
virtio-gpu does per-buffer uploads and for this reason it needs to do
a buffer damage handling, instead of frame damage handling.

Their suggestion was to extend the damage helpers to cover that case
and given that there's isn't a buffer damage accumulation algorithm
(e.g: buffer age), just do a full plane update if the framebuffer that
is attached to a plane changed since the last plane update (page-flip).

Patch #1 is just a refactoring to allow the logic of the frame damage
helpers to be shared by the buffer damage helpers.

Patch #2 adds the helpers that are needed for buffer damage handling.

Patch #3 fixes the virtio-gpu damage handling logic by using the
helper that is required by drivers that need to handle buffer damage.

Patch #4 fixes the vmwgfx similarly, since that driver also needs to
handle buffer damage and should have the same issue (although I have
not tested it due not having a VMWare setup).

Patch #5 adds to the KMS damage tracking kernel-doc some paragraphs
about damage tracking types and references to links that explain
frame damage vs buffer damage.

Finally patch #6 adds an item to the DRM/KMS todo, about the need to
implement some buffer damage accumulation algorithm instead of just
doing a full plane update in this case.

Because commit 01f05940a9a7 landed in v6.4, the first three patches
are marked as Fixes and Cc stable.

I've tested this on a VM with weston, was able to reproduce the issue
reported and the patches did fix the problem.

Please let me know what you think. Specially on the wording since could
made mistakes due just learning about these concepts yesterday thanks to
Sima, Simon and Pekka.

Best regards,
Javier


Javier Martinez Canillas (6):
  drm: Move drm_atomic_helper_damage_{iter_init,merged}() to helpers
  drm: Add drm_atomic_helper_buffer_damage_{iter_init,merged}() helpers
  drm/virtio: Use drm_atomic_helper_buffer_damage_merged() for buffer
damage
  drm/vmwgfx: Use drm_atomic_helper_buffer_damage_iter_init() for buffer
damage
  drm/plane: Extend damage tracking kernel-doc
  drm/todo: Add entry about implementing buffer age for damage tracking

 Documentation/gpu/todo.rst |  20 +++
 drivers/gpu/drm/drm_damage_helper.c| 166 +++--
 drivers/gpu/drm/drm_plane.c|  22 +++-
 drivers/gpu/drm/virtio/virtgpu_plane.c |   2 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c|   2 +-
 include/drm/drm_damage_helper.h|   7 ++
 6 files changed, 173 insertions(+), 46 deletions(-)

-- 
2.41.0

[PATCH] drm/panel: auo, b101uan08.3: Fine tune the panel power sequence

[Xuxin Xiong]

For "auo,b101uan08.3" this panel, it is stipulated in the panel spec that
MIPI needs to keep the LP11 state before the lcm_reset pin is pulled high.

BUG=b:309908277
TEST=emerge-kukui chromeos-kernel-5_10

Signed-off-by: Xuxin Xiong 
---
 drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c 
b/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c
index 9323e7b9e384..a287be1aaf70 100644
--- a/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c
+++ b/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c
@@ -1709,6 +1709,7 @@ static const struct panel_desc auo_b101uan08_3_desc = {
.mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
  MIPI_DSI_MODE_LPM,
.init_cmds = auo_b101uan08_3_init_cmd,
+   .lp11_before_reset = true,
 };
 
 static const struct drm_display_mode boe_tv105wum_nw0_default_mode = {
-- 
2.39.2

[PATCH] drm/panel: auo, b101uan08.3: Fine tune the panel power sequence

[Xuxin Xiong]

For "auo,b101uan08.3" this panel, it is stipulated in the panel spec that
MIPI needs to keep the LP11 state before the lcm_reset pin is pulled high.

BUG=b:309908277
TEST=emerge-kukui chromeos-kernel-5_10

Signed-off-by: Xuxin Xiong 
---
 drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c 
b/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c
index 9323e7b9e384..a287be1aaf70 100644
--- a/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c
+++ b/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c
@@ -1709,6 +1709,7 @@ static const struct panel_desc auo_b101uan08_3_desc = {
.mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
  MIPI_DSI_MODE_LPM,
.init_cmds = auo_b101uan08_3_init_cmd,
+   .lp11_before_reset = true,
 };
 
 static const struct drm_display_mode boe_tv105wum_nw0_default_mode = {
-- 
2.39.2

Re: [RFC PATCH v3 10/12] tcp: RX path for devmem TCP

[Edward Cree]

On 09/11/2023 02:39, Mina Almasry wrote:
> On Wed, Nov 8, 2023 at 7:36 AM Edward Cree  wrote:
>>  If not then surely the way to return a memory area
>>  in an io_uring idiom is just to post a new read sqe ('RX descriptor')
>>  pointing into it, rather than explicitly returning it with setsockopt.
> 
> We're interested in using this with regular TCP sockets, not
> necessarily io_uring.
Fair.  I just wanted to push against the suggestion upthread that "oh,
 since io_uring supports setsockopt() we can just ignore it and it'll
 all magically work later" (paraphrased).
If you can keep the "allocate buffers out of a devmem region" and "post
 RX descriptors built on those buffers" APIs separate (inside the
 kernel; obviously both triggered by a single call to the setsockopt()
 uAPI) that'll likely make things simpler for the io_uring interface I
 describe, which will only want the latter.

-ed

PS: Here's a crazy idea that I haven't thought through at all: what if
 you allow device memory to be mmap()ed into process address space
 (obviously with none of r/w/x because it's unreachable), so that your
 various uAPIs can just operate on pointers (e.g. the setsockopt
 becomes the madvise it's named after; recvmsg just uses or populates
 the iovec rather than needing a cmsg).  Then if future devices have
 their memory CXL accessible that can potentially be enabled with no
 change to the uAPI (userland just starts being able to access the
 region without faulting).
And you can maybe add a semantic flag to recvmsg saying "if you don't
 use all the buffers in my iovec, keep hold of the rest of them for
 future incoming traffic, and if I post new buffers with my next
 recvmsg, add those to the tail of the RXQ rather than replacing the
 ones you've got".  That way you can still have the "userland
 directly fills the RX ring" behaviour even with TCP sockets.

Re: [PATCH drm-misc-next v8 09/12] drm/gpuvm: reference count drm_gpuvm structures

[Christian König]

Am 09.11.23 um 16:50 schrieb Thomas Hellström:

[SNIP]



Did we get any resolution on this?

FWIW, my take on this is that it would be possible to get GPUVM to 
work both with and without internal refcounting; If with, the driver 
needs a vm close to resolve cyclic references, if without that's not 
necessary. If GPUVM is allowed to refcount in mappings and vm_bos, 
that comes with a slight performance drop but as Danilo pointed out, 
the VM lifetime problem iterating over a vm_bo's mapping becomes much 
easier and the code thus becomes easier to maintain moving forward. 
That convinced me it's a good thing.


I strongly believe you guys stumbled over one of the core problems with 
the VM here and I think that reference counting is the right answer to 
solving this.


The big question is that what is reference counted and in which 
direction does the dependency points, e.g. we have here VM, BO, BO_VM 
and Mapping objects.


Those patches here suggest a counted Mapping -> VM reference and I'm 
pretty sure that this isn't a good idea. What we should rather really 
have is a BO -> VM or BO_VM ->VM reference. In other words that each BO 
which is part of the VM keeps a reference to the VM.


BTW: At least in amdgpu we can have BOs which (temporary) doesn't have 
any mappings, but are still considered part of the VM.




Another issue Christian brought up is that something intended to be 
embeddable (a base class) shouldn't really have its own refcount. I 
think that's a valid point. If you at some point need to derive from 
multiple such structs each having its own refcount, things will start 
to get weird. One way to resolve that would be to have the driver's 
subclass provide get() and put() ops, and export a destructor for the 
base-class, rather than to have the base-class provide the refcount 
and a destructor  ops.


Well, I have never seen stuff like that in the kernel. Might be that 
this works, but I would rather not try if avoidable.




That would also make it possible for the driver to decide the context 
for the put() call: If the driver needs to be able to call put() from 
irq / atomic context but the base-class'es destructor doesn't allow 
atomic context, the driver can push freeing out to a work item if needed.


Finally, the refcount overflow Christian pointed out. Limiting the 
number of mapping sounds like a reasonable remedy to me.


Well that depends, I would rather avoid having a dependency for mappings.

Taking the CPU VM handling as example as far as I know vm_area_structs 
doesn't grab a reference to their mm_struct either. Instead they get 
automatically destroyed when the mm_struct is destroyed.


Which makes sense in that case because when the mm_struct is gone the 
vm_area_struct doesn't make sense any more either.


What we clearly need is a reference to prevent the VM or at least the 
shared resv to go away to early.


Regards,
Christian.



But I think all of this is fixable as follow-ups if needed, unless I'm 
missing something crucial.


Just my 2 cents.

/Thomas

Re: [PATCH drm-misc-next v8 09/12] drm/gpuvm: reference count drm_gpuvm structures

[Thomas Hellström]

Danilo, Christian

On 11/6/23 17:42, Danilo Krummrich wrote:

On Mon, Nov 06, 2023 at 04:10:50PM +0100, Christian König wrote:

Am 06.11.23 um 15:11 schrieb Danilo Krummrich:

On Mon, Nov 06, 2023 at 02:05:13PM +0100, Christian König wrote:

Am 06.11.23 um 13:16 schrieb Danilo Krummrich:

[SNIP]
This reference count just prevents that the VM is freed as long as other
ressources are attached to it that carry a VM pointer, such as mappings and
VM_BOs. The motivation for that are VM_BOs. For mappings it's indeed a bit
paranoid, but it doesn't hurt either and keeps it consistant.

Ah! Yeah, we have similar semantics in amdgpu as well.

But we keep the reference to the root GEM object and not the VM.

Ok, that makes much more sense then keeping one reference for each mapping.


Because of this the mapping should *never* have a reference to the VM, but
rather the VM destroys all mapping when it is destroyed itself.


Hence, If the VM is still alive at a point where you don't expect it to
be, then it's
simply a driver bug.

Driver bugs is just what I try to prevent here. When individual mappings
keep the VM structure alive then drivers are responsible to clean them up,
if the VM cleans up after itself then we don't need to worry about it in the
driver.

Drivers are *always* responsible for that. This has nothing to do with whether
the VM is reference counted or not. GPUVM can't clean up mappings after itself.

Why not?

I feel like we're talking past each other here, at least to some extend.
However, I can't yet see where exactly the misunderstanding resides.

+1


At least in amdgpu we have it exactly like that. E.g. the higher level can
cleanup the BO_VM structure at any time possible, even when there are
mappings.

What do you mean with "cleanup the VM_BO structue" exactly?

The VM_BO structure keeps track of all the mappings mapped in the VM_BO's VM
being backed by the VM_BO's GEM object. And the GEM objects keeps a list of
the corresponding VM_BOs.

Hence, as long as there are mappings that this VM_BO keeps track of, this VM_BO
should stay alive.

No, exactly the other way around. When the VM_BO structure is destroyed the
mappings are destroyed with them.

This seems to be the same misunderstanding as with the VM reference count.

It seems to me that you want to say that for amdgpu it seems to be a use-case
to get rid of all mappings backed by a given BO and mapped in a given VM, hence
a VM_BO. You can do that. Thers's even a helper for that in GPUVM.

But also in this case you first need to get rid of all mappings before you
*free* the VM_BO - GPUVM ensures that.


Otherwise you would need to destroy each individual mapping separately
before teardown which is quite inefficient.

Not sure what you mean, but I don't see a difference between walking all VM_BOs
and removing their mappings and walking the VM's tree of mappings and removing
each of them. Comes down to the same effort in the end. But surely can go both
ways if you know all the existing VM_BOs.


The VM then keeps track which areas still need to be invalidated
in the physical representation of the page tables.

And the VM does that through its tree of mappings (struct drm_gpuva). Hence, if
the VM would just remove those structures on cleanup by itself, you'd loose the
ability of cleaning up the page tables. Unless, you track this separately, which
would make the whole tracking of GPUVM itself kinda pointless.

But how do you then keep track of areas which are freed and needs to be
updated so that nobody can access the underlying memory any more?

"areas which are freed", what do refer to? What do yo mean with that?

Do you mean areas of the VA space not containing mappings? Why would I need to
track them explicitly? When the mapping is removed the corresponding page tables
/ page table entries are gone as well, hence no subsequent access to the
underlaying memory would be possible.


I would expect that the generalized GPU VM handling would need something
similar. If we leave that to the driver then each driver would have to
implement that stuff on it's own again.

Similar to what? What exactly do you think can be generalized here?

Similar to how amdgpu works.

I don't think it's quite fair to just throw the "look at what amdgpu does"
argument at me. What am I supposed to do? Read and understand *every* detail of
*every* driver?

Did you read through the GPUVM code? That's a honest question and I'm asking it
because I feel like you're picking up some details from commit messages and
start questioning them (and that's perfectly fine and absolutely welcome).

But if the answers don't satisfy you or do not lead to a better understanding it
just seems you ask others to check out amdgpu rather than taking the time to go
though the proposed code yourself making suggestions to improve it or explicitly
point out the changes you require.


 From what I can see you are basically re-inventing everything we already
have in there and asking the same 

Re: [RFC PATCH 2/2] vc4: introduce DMA-BUF heap

[Dave Stevenson]

Hi Simon and Maxime

On Thu, 9 Nov 2023 at 09:12, Maxime Ripard  wrote:
>
> Hi Simon,
>
> On Thu, Nov 09, 2023 at 07:45:58AM +, Simon Ser wrote:
> > User-space sometimes needs to allocate scanout-capable memory for
> > GPU rendering purposes. On a vc4/v3d split render/display SoC, this
> > is achieved via DRM dumb buffers: the v3d user-space driver opens
> > the primary vc4 node, allocates a DRM dumb buffer there, exports it
> > as a DMA-BUF, imports it into the v3d render node, and renders to it.
> >
> > However, DRM dumb buffers are only meant for CPU rendering, they are
> > not intended to be used for GPU rendering. Primary nodes should only
> > be used for mode-setting purposes, other programs should not attempt
> > to open it. Moreover, opening the primary node is already broken on
> > some setups: systemd grants permission to open primary nodes to
> > physically logged in users, but this breaks when the user is not
> > physically logged in (e.g. headless setup) and when the distribution
> > is using a different init (e.g. Alpine Linux uses openrc).
> >
> > We need an alternate way for v3d to allocate scanout-capable memory.
> > Leverage DMA heaps for this purpose: expose a CMA heap to user-space.
> > Preliminary testing has been done with wlroots [1].
> >
> > This is still an RFC. Open questions:
> >
> > - Does this approach make sense to y'all in general?
>
> Makes sense to me :)
>
> > - What would be a good name for the heap? "vc4" is maybe a bit naive and
> >   not precise enough. Something with "cma"? Do we need to plan a naming
> >   scheme to accomodate for multiple vc4 devices?
>
> That's a general issue though that happens with pretty much all devices
> with a separate node for modesetting and rendering, so I don't think
> addressing it only for vc4 make sense, we should make it generic.
>
> So maybe something like "scanout"?
>
> One thing we need to consider too is that the Pi5 will have multiple
> display nodes (4(?) iirc) with different capabilities, vc4 being only
> one of them. This will impact that solution too.

It does need to scale.

Pi5 adds 4 additional DRM devices (2xDSI, 1xDPI, and 1xComposite
Video), and just this last week I've been running Wayfire with TinyDRM
drivers for SPI displays and UDL (DisplayLink) outputs as well.
Presumably all of those drivers need to have appropriate hooks added
so they each expose a dma-heap to enable scanout buffers to be
allocated.

Can we add another function pointer to the struct drm_driver (or
similar) to do the allocation, and move the actual dmabuf handling
code into the core?

> > - Right now root privileges are necessary to open the heap. Should we
> >   allow everybody to open the heap by default (after all, user-space can
> >   already allocate arbitrary amounts of GPU memory)? Should we leave it
> >   up to user-space to solve this issue (via logind/seatd or a Wayland
> >   protocol or something else)?
>
> I would have expected a udev rule to handle that?
>
> > TODO:
> >
> > - Need to add !vc5 support.
>
> If by !vc5 you mean RPi0-3, then it's probably not needed there at all
> since it has a single node for both modesetting and rendering?

That is true, but potentially vc4 could be rendering for scanout via UDL or SPI.
Is it easier to always have the dma-heap allocator for every DRM card
rather than making userspace mix and match depending on whether it is
all in one vs split?

  Dave

Re: [RFC PATCH 2/2] vc4: introduce DMA-BUF heap

[Simon Ser]

On Thursday, November 9th, 2023 at 16:14, T.J. Mercier  
wrote:

> > + exp_info.priv = vc4; / TODO: unregister when unloading */
> > +
> 
> So unregistering a heap isn't currently possible, but we're trying to
> enable that here:
> https://lore.kernel.org/all/20231106120423.23364-7-yunfei.d...@mediatek.com/

Great!

Note, I wouldn't actually need any ref'counting, a simpler function to
always unregister would be enough for my purposes I think. But if you
have an actual use-case for the ref'counting, that's completely fine for
me as well.

Re: [PATCH] drm/amd/pm: replace 1-element arrays with flexible-array members

[Alex Deucher]

On Thu, Nov 9, 2023 at 7:14 AM José Pekkarinen
 wrote:
>
> On 2023-11-09 11:06, Greg KH wrote:
> > On Thu, Nov 09, 2023 at 10:43:50AM +0200, José Pekkarinen wrote:
> >> On 2023-11-08 09:29, Greg KH wrote:
> >> > On Wed, Nov 08, 2023 at 08:54:35AM +0200, José Pekkarinen wrote:
> >> > > The following case seems to be safe to be replaced with a flexible
> >> > > array
> >> > > to clean up the added coccinelle warning. This patch will just do it.
> >> > >
> >> > > drivers/gpu/drm/amd/pm/powerplay/smumgr/smu8_smumgr.h:76:38-63:
> >> > > WARNING use flexible-array member instead 
> >> > > (https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays)
> >> > >
> >> > > Signed-off-by: José Pekkarinen 
> >> > > ---
> >> > >  drivers/gpu/drm/amd/pm/powerplay/smumgr/smu8_smumgr.h | 2 +-
> >> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> > >
> >> > > diff --git a/drivers/gpu/drm/amd/pm/powerplay/smumgr/smu8_smumgr.h
> >> > > b/drivers/gpu/drm/amd/pm/powerplay/smumgr/smu8_smumgr.h
> >> > > index c7b61222d258..1ce4087005f0 100644
> >> > > --- a/drivers/gpu/drm/amd/pm/powerplay/smumgr/smu8_smumgr.h
> >> > > +++ b/drivers/gpu/drm/amd/pm/powerplay/smumgr/smu8_smumgr.h
> >> > > @@ -73,7 +73,7 @@ struct smu8_register_index_data_pair {
> >> > >
> >> > >  struct smu8_ih_meta_data {
> >> > >  uint32_t command;
> >> > > -struct smu8_register_index_data_pair 
> >> > > register_index_value_pair[1];
> >> > > +struct smu8_register_index_data_pair 
> >> > > register_index_value_pair[];
> >> >
> >> > Did you just change this structure size without any need to change any
> >> > code as well?  How was this tested?
> >>
> >> I didn't find any use of that struct member, if I missed
> >> something here, please let me know and I'll happily address any
> >> needed further work.
> >
> > I don't think this is even a variable array.  It's just a one element
> > one, which is fine, don't be confused by the coccinelle "warning" here,
> > it's fired many false-positives and you need to verify this properly
> > with the driver authors first before changing anything.
>
>   My apologies to you, and anybody that feels the same, it is not my
> intention to bother with mistaken patches, I just assume that this patch
> or any other from me, will go to review process, where it should be fine
> if the patch is right, wrong, need further work, or further testing
> either
> from my side or anybody else, and at the end of the day I need to do
> patches if I want to find my mentorship patches, and graduate.
>
> > In short, you just changed the size of this structure, are you _sure_
> > you can do that?  And yes, it doesn't look like this field is used, but
> > the structure is, so be careful.
>
>  I don't know, let check it out together and see where this goes.

I think it may have been used with the SMU firmware.  I'll need to
check with that team to determine if this was meant to be a variable
sized array or not.

Alex

Re: [RFC PATCH 2/2] vc4: introduce DMA-BUF heap

[Simon Ser]

On Thursday, November 9th, 2023 at 13:14, Maira Canal  wrote:

> On 11/9/23 04:45, Simon Ser wrote:
> > User-space sometimes needs to allocate scanout-capable memory for
> > GPU rendering purposes. On a vc4/v3d split render/display SoC, this
> > is achieved via DRM dumb buffers: the v3d user-space driver opens
> > the primary vc4 node, allocates a DRM dumb buffer there, exports it
> > as a DMA-BUF, imports it into the v3d render node, and renders to it.
> >
> > However, DRM dumb buffers are only meant for CPU rendering, they are
> > not intended to be used for GPU rendering. Primary nodes should only
> > be used for mode-setting purposes, other programs should not attempt
> > to open it. Moreover, opening the primary node is already broken on
> > some setups: systemd grants permission to open primary nodes to
> > physically logged in users, but this breaks when the user is not
> > physically logged in (e.g. headless setup) and when the distribution
> > is using a different init (e.g. Alpine Linux uses openrc).
> >
> > We need an alternate way for v3d to allocate scanout-capable memory.
> 
> For me, it is a bit unclear how we import the vc4 DMA-BUF heap into v3d.
> Should we create an IOCTL for it on v3d?

Hm, but v3d is the render driver, so it shouldn't have any knowledge of what
vc4 -- the display driver -- actually needs to be able to scanout a piece of
memory. It's true that other x86 drivers like amdgpu and i915 just have their
own IOCTL to allocate scanout-capable memory, however the split render/display
SoC situation makes the situation a bit more hairy for vc4/v3d.

You can think of the vc4 DMA-BUF heaps as a vc4 alloc IOCTL, except it's using
a more standard framework instead of a custom IOCTL.

Does this make sense?

> Also, if you need some help testing with the RPi 5, you can ping on IRC
> and I can try to help by testing.

Thank you, appreciated!

Re: [Intel-gfx] [PATCH v2 0/6] drm/edid: split out drm_eld.[ch], add some SAD helpers

[Jani Nikula]

On Thu, 02 Nov 2023, Jani Nikula  wrote:
> On Tue, 31 Oct 2023, Jani Nikula  wrote:
>> v2 of https://patchwork.freedesktop.org/series/123384/
>>
>> Jani Nikula (6):
>>   drm/edid: split out drm_eld.h from drm_edid.h
>>   drm/eld: replace uint8_t with u8
>>   drm/edid: include drm_eld.h only where required
>>   drm/edid: use a temp variable for sads to drop one level of
>> dereferences
>>   drm/edid: add helpers to get/set struct cea_sad from/to 3-byte sad
>>   drm/eld: add helpers to modify the SADs of an ELD
>
> Maxime, Maarten, Thomas -
>
> I'm moving a bunch of code around here, and would like to get your acks
> before merging. I'm planning on merging this via drm-misc-next, it's
> just that it only has Intel reviews, and don't want to feel like I'm
> sneaking this in.

Merged with Maxime's IRC ack, which I forgot to add to the commit
messages. *facepalm*. Sorry about that.

Thanks Mitul for reviews!

BR,
Jani.

>
> Thanks,
> Jani.
>
>>
>>  Documentation/gpu/drm-kms-helpers.rst |   6 +
>>  drivers/gpu/drm/Makefile  |   1 +
>>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |   1 +
>>  drivers/gpu/drm/drm_edid.c|  43 +++--
>>  drivers/gpu/drm/drm_eld.c |  55 ++
>>  drivers/gpu/drm/drm_internal.h|   6 +
>>  drivers/gpu/drm/i915/display/intel_audio.c|   1 +
>>  .../drm/i915/display/intel_crtc_state_dump.c  |   1 +
>>  drivers/gpu/drm/i915/display/intel_sdvo.c |   1 +
>>  drivers/gpu/drm/nouveau/dispnv50/disp.c   |   1 +
>>  drivers/gpu/drm/radeon/radeon_audio.c |   1 +
>>  drivers/gpu/drm/tegra/hdmi.c  |   1 +
>>  drivers/gpu/drm/tegra/sor.c   |   1 +
>>  include/drm/drm_edid.h| 148 
>>  include/drm/drm_eld.h | 164 ++
>>  sound/core/pcm_drm_eld.c  |   1 +
>>  sound/soc/codecs/hdac_hdmi.c  |   1 +
>>  sound/soc/codecs/hdmi-codec.c |   1 +
>>  sound/x86/intel_hdmi_audio.c  |   1 +
>>  19 files changed, 275 insertions(+), 160 deletions(-)
>>  create mode 100644 drivers/gpu/drm/drm_eld.c
>>  create mode 100644 include/drm/drm_eld.h

-- 
Jani Nikula, Intel

Re: [RFC PATCH 2/2] vc4: introduce DMA-BUF heap

[Simon Ser]

Thanks for the feedback Iago! I've replied to Maxime and I believe that
covers your questions as well.

Re: [RFC PATCH 2/2] vc4: introduce DMA-BUF heap

[Simon Ser]

On Thursday, November 9th, 2023 at 10:11, Maxime Ripard  
wrote:

> > - Does this approach make sense to y'all in general?
> 
> Makes sense to me :)

Great to hear!

> > - What would be a good name for the heap? "vc4" is maybe a bit naive and
> > not precise enough. Something with "cma"? Do we need to plan a naming
> > scheme to accomodate for multiple vc4 devices?
> 
> That's a general issue though that happens with pretty much all devices
> with a separate node for modesetting and rendering, so I don't think
> addressing it only for vc4 make sense, we should make it generic.
> 
> So maybe something like "scanout"?
> 
> One thing we need to consider too is that the Pi5 will have multiple
> display nodes (4(?) iirc) with different capabilities, vc4 being only
> one of them. This will impact that solution too.

I'm not sure trying to find a unique generic name for all split render/display
SoC is a good idea:

- For the purposes of replacing DRM dumb buffers usage from v3d, we don't
  actually need a generic name, it's perfectly fine to hardcode a name here
  since vc4 is pretty much hardcoded there already.
- As you said, "scanout" may be ill-defined depending on the system. What if
  a PCIe or USB device is plugged in? What if vkms is loaded? What if there are
  multiple platform display devices? What does "scanout" mean then?
- A generic solution to advertise what DMA heaps a DRM display device is
  compatible with is probably better addressed by populating sysfs with new
  files. We've talked with Sima at XDC about adding a symlink pointing to the
  DMA heap and extra metadata files describing priorities and such. However we
  don't actually need that part for the purposes of v3d -- I think I'd rather
  defer that until more DMA heaps are plumbed across the DRM drivers.

So I'd be in favor of using something descriptive, platform-specific for the
heap name, something that user-space can't generically try to interpret or
parse.

> > - Right now root privileges are necessary to open the heap. Should we
> > allow everybody to open the heap by default (after all, user-space can
> > already allocate arbitrary amounts of GPU memory)? Should we leave it
> > up to user-space to solve this issue (via logind/seatd or a Wayland
> > protocol or something else)?
> 
> I would have expected a udev rule to handle that?

Right. That sounds fine to me. (It's not the end of the world if v3d can't
allocate scanout-capable memory if the heap is not accessible -- just means we
will go through an extra copy in the compositor. And we can always keep the DRM
legacy primary node stuff as a fallback if there really are concerns.)

> > TODO:
> > 
> > - Need to add !vc5 support.
> 
> If by !vc5 you mean RPi0-3, then it's probably not needed there at all
> since it has a single node for both modesetting and rendering?

Ah that's a good point. So we can just not expose any heap for !vc5.

Re: [RFC PATCH 2/2] vc4: introduce DMA-BUF heap

[T.J. Mercier]

On Wed, Nov 8, 2023 at 11:46 PM Simon Ser  wrote:
>
> +int vc4_dma_heap_create(struct vc4_dev *vc4)
> +{
> +   struct dma_heap_export_info exp_info;
> +   struct dma_heap *heap;
> +
> +   exp_info.name = "vc4"; /* TODO: allow multiple? */
> +   exp_info.ops = _dma_heap_ops;
> +   exp_info.priv = vc4; /* TODO: unregister when unloading */
> +

So unregistering a heap isn't currently possible, but we're trying to
enable that here:
https://lore.kernel.org/all/20231106120423.23364-7-yunfei.d...@mediatek.com/

[PATCH 11/20] drm/sched: Convert the GPU scheduler to variable number of run-queues

[Rodrigo Vivi]

From: Luben Tuikov 

The GPU scheduler has now a variable number of run-queues, which are set up at
drm_sched_init() time. This way, each driver announces how many run-queues it
requires (supports) per each GPU scheduler it creates. Note, that run-queues
correspond to scheduler "priorities", thus if the number of run-queues is set
to 1 at drm_sched_init(), then that scheduler supports a single run-queue,
i.e. single "priority". If a driver further sets a single entity per
run-queue, then this creates a 1-to-1 correspondence between a scheduler and
a scheduled entity.

Cc: Lucas Stach 
Cc: Russell King 
Cc: Qiang Yu 
Cc: Rob Clark 
Cc: Abhinav Kumar 
Cc: Dmitry Baryshkov 
Cc: Danilo Krummrich 
Cc: Matthew Brost 
Cc: Boris Brezillon 
Cc: Alex Deucher 
Cc: Christian König 
Cc: Emma Anholt 
Cc: etna...@lists.freedesktop.org
Cc: l...@lists.freedesktop.org
Cc: linux-arm-...@vger.kernel.org
Cc: freedr...@lists.freedesktop.org
Cc: nouv...@lists.freedesktop.org
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Luben Tuikov 
Acked-by: Christian König 
Link: https://lore.kernel.org/r/20231023032251.164775-1-luben.tui...@amd.com
(cherry picked from commit 56e449603f0ac580700621a356d35d5716a62ce5)
Signed-off-by: Rodrigo Vivi 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.c|  4 +-
 drivers/gpu/drm/etnaviv/etnaviv_sched.c|  1 +
 drivers/gpu/drm/lima/lima_sched.c  |  4 +-
 drivers/gpu/drm/msm/msm_ringbuffer.c   |  5 +-
 drivers/gpu/drm/nouveau/nouveau_sched.c|  1 +
 drivers/gpu/drm/panfrost/panfrost_job.c|  1 +
 drivers/gpu/drm/scheduler/sched_entity.c   | 18 +-
 drivers/gpu/drm/scheduler/sched_main.c | 74 ++
 drivers/gpu/drm/v3d/v3d_sched.c|  5 ++
 include/drm/gpu_scheduler.h|  9 ++-
 11 files changed, 98 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 30c4f5cca02c..15074232fbbd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -2280,6 +2280,7 @@ static int amdgpu_device_init_schedulers(struct 
amdgpu_device *adev)
}
 
r = drm_sched_init(>sched, _sched_ops,
+  DRM_SCHED_PRIORITY_COUNT,
   ring->num_hw_submission, 0,
   timeout, adev->reset_domain->wq,
   ring->sched_score, ring->name,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
index 78476bc75b4e..1f357198533f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
@@ -325,8 +325,8 @@ void amdgpu_job_stop_all_jobs_on_sched(struct 
drm_gpu_scheduler *sched)
int i;
 
/* Signal all jobs not yet scheduled */
-   for (i = DRM_SCHED_PRIORITY_COUNT - 1; i >= DRM_SCHED_PRIORITY_MIN; 
i--) {
-   struct drm_sched_rq *rq = >sched_rq[i];
+   for (i = sched->num_rqs - 1; i >= DRM_SCHED_PRIORITY_MIN; i--) {
+   struct drm_sched_rq *rq = sched->sched_rq[i];
spin_lock(>lock);
list_for_each_entry(s_entity, >entities, list) {
while ((s_job = 
to_drm_sched_job(spsc_queue_pop(_entity->job_queue {
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.c 
b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
index 345fec6cb1a4..9b79f218e21a 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_sched.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
@@ -135,6 +135,7 @@ int etnaviv_sched_init(struct etnaviv_gpu *gpu)
int ret;
 
ret = drm_sched_init(>sched, _sched_ops,
+DRM_SCHED_PRIORITY_COUNT,
 etnaviv_hw_jobs_limit, etnaviv_job_hang_limit,
 msecs_to_jiffies(500), NULL, NULL,
 dev_name(gpu->dev), gpu->dev);
diff --git a/drivers/gpu/drm/lima/lima_sched.c 
b/drivers/gpu/drm/lima/lima_sched.c
index ffd91a5ee299..295f0353a02e 100644
--- a/drivers/gpu/drm/lima/lima_sched.c
+++ b/drivers/gpu/drm/lima/lima_sched.c
@@ -488,7 +488,9 @@ int lima_sched_pipe_init(struct lima_sched_pipe *pipe, 
const char *name)
 
INIT_WORK(>recover_work, lima_sched_recover_work);
 
-   return drm_sched_init(>base, _sched_ops, 1,
+   return drm_sched_init(>base, _sched_ops,
+ DRM_SCHED_PRIORITY_COUNT,
+ 1,
  lima_job_hang_limit,
  msecs_to_jiffies(timeout), NULL,
  NULL, name, pipe->ldev->dev);
diff --git a/drivers/gpu/drm/msm/msm_ringbuffer.c 
b/drivers/gpu/drm/msm/msm_ringbuffer.c
index 40c0bc35a44c..95257ab0185d 100644
--- a/drivers/gpu/drm/msm/msm_ringbuffer.c
+++ b/drivers/gpu/drm/msm/msm_ringbuffer.c
@@ -95,8 +95,9 @@ struct