Re: [PATCH v2 0/8] drm/msm: Convert fbdev to DRM client

[Thomas Zimmermann]

Am 05.04.23 um 03:35 schrieb Dmitry Baryshkov:


On Mon, 03 Apr 2023 14:45:30 +0200, Thomas Zimmermann wrote:

Convert msm' fbdev code to struct drm_client. Replaces the current
ad-hoc integration. The conversion includes a number of cleanups. As
with most other drivers' fbdev emulation, fbdev in msm is now just
another DRM client that runs after the DRM device has been registered.

Once all drivers' fbdev emulation has been converted to struct drm_client,
we can attempt to add additional in-kernel clients. A DRM-based dmesg
log or a bootsplash are commonly mentioned. DRM can then switch easily
among the existing clients if/when required.

[...]


Applied, thanks!


Great, thanks a lot!

Best regards
Thomas



[1/8] drm/msm: Include 
   https://gitlab.freedesktop.org/lumag/msm/-/commit/62c58ffe011d
[2/8] drm/msm: Clear aperture ownership outside of fbdev code
   https://gitlab.freedesktop.org/lumag/msm/-/commit/f4de16da5b40
[3/8] drm/msm: Remove fb from struct msm_fbdev
   https://gitlab.freedesktop.org/lumag/msm/-/commit/a5ddc0f1a7bc
[4/8] drm/msm: Remove struct msm_fbdev
   https://gitlab.freedesktop.org/lumag/msm/-/commit/09cbdbafbe9f
[5/8] drm/msm: Remove fbdev from struct msm_drm_private
   https://gitlab.freedesktop.org/lumag/msm/-/commit/37e8bad3ae5d
[6/8] drm/msm: Move module parameter 'fbdev' to fbdev code
   https://gitlab.freedesktop.org/lumag/msm/-/commit/2fa4748b5ad8
[7/8] drm/msm: Initialize fbdev DRM client
   https://gitlab.freedesktop.org/lumag/msm/-/commit/7e563538d210
[8/8] drm/msm: Implement fbdev emulation as in-kernel client
   https://gitlab.freedesktop.org/lumag/msm/-/commit/5ba5b96d3327

Best regards,


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


OpenPGP_signature
Description: OpenPGP digital signature

Re: [RFC PATCH 00/10] Xe DRM scheduler and long running workload plans

[Christian König]

Am 04.04.23 um 20:08 schrieb Matthew Brost:

On Tue, Apr 04, 2023 at 12:02:03PM -0600, Zeng, Oak wrote:

Hi Matt, Thomas,

Some very bold out of box thinking in this area:

1. so you want to use drm scheduler and dma-fence for long running workload. 
Why you want to do this in the first place? What is the benefit? Drm scheduler 
is pretty much a software scheduler. Modern gpu has scheduler built at fw/hw 
level, as you said below for intel this is Guc. Can xe driver just directly 
submit job to Guc, bypassing drm scheduler?


If we did that now we have 2 paths for dependency track, flow controling
the ring, resets / error handling / backend submission implementations.
We don't want this.


Well exactly that's the point: Why?

As far as I can see that are two completely distinct use cases, so you 
absolutely do want two completely distinct implementations for this.



2. using dma-fence for long run workload: I am well aware that page fault (and 
the consequent memory allocation/lock acquiring to fix the fault) can cause 
deadlock for a dma-fence wait. But I am not convinced that dma-fence can't be 
used purely because the nature of the workload that it runs very long 
(indefinite). I did a math: the dma_fence_wait_timeout function's third param 
is the timeout which is a signed long type. If HZ is 1000, this is about 23 
days. If 23 days is not long enough, can we just change the timeout parameter 
to signed 64 bits so it is much longer than our life time...

So I mainly argue we can't use dma-fence for long-run workload is not because 
the workload runs very long, rather because of the fact that we use page fault 
for long-run workload. If we enable page fault for short-run workload, we can't 
use dma-fence either. Page fault is the key thing here.

Now since we use page fault which is *fundamentally* controversial with 
dma-fence design, why now just introduce a independent concept such as 
user-fence instead of extending existing dma-fence?

I like unified design. If drm scheduler, dma-fence can be extended to work for 
everything, it is beautiful. But seems we have some fundamental problem here.


Thomas's patches turn a dma-fence into KMD sync point (e.g. we just use
the signal / CB infrastructure) and enforce we don't use use these
dma-fences from the scheduler in memory reclaim paths or export these to
user space or other drivers. Think of this mode as SW only fence.


Yeah and I truly think this is an really bad idea.

The signal/CB infrastructure in the dma_fence turned out to be the 
absolutely nightmare I initially predicted. Sorry to say that, but in 
this case the "I've told you so" is appropriate in my opinion.


If we need infrastructure for long running dependency tracking we should 
encapsulate that in a new framework and not try to mangle the existing 
code for something it was never intended for.


Christian.



Matt
  

Thanks,
Oak


-Original Message-
From: dri-devel  On Behalf Of
Matthew Brost
Sent: April 3, 2023 8:22 PM
To: dri-devel@lists.freedesktop.org; intel...@lists.freedesktop.org
Cc: robdcl...@chromium.org; thomas.hellst...@linux.intel.com; airl...@linux.ie;
l...@asahilina.net; boris.brezil...@collabora.com; Brost, Matthew
; christian.koe...@amd.com;
faith.ekstr...@collabora.com
Subject: [RFC PATCH 00/10] Xe DRM scheduler and long running workload plans

Hello,

As a prerequisite to merging the new Intel Xe DRM driver [1] [2], we
have been asked to merge our common DRM scheduler patches first as well
as develop a common solution for long running workloads with the DRM
scheduler. This RFC series is our first attempt at doing this. We
welcome any and all feedback.

This can we thought of as 4 parts detailed below.

- DRM scheduler changes for 1 to 1 relationship between scheduler and
entity (patches 1-3)

In Xe all of the scheduling of jobs is done by a firmware scheduler (the
GuC) which is a new paradigm WRT to the DRM scheduler and presents
severals problems as the DRM was originally designed to schedule jobs on
hardware queues. The main problem being that DRM scheduler expects the
submission order of jobs to be the completion order of jobs even across
multiple entities. This assumption falls apart with a firmware scheduler
as a firmware scheduler has no concept of jobs and jobs can complete out
of order. A novel solution for was originally thought of by Faith during
the initial prototype of Xe, create a 1 to 1 relationship between scheduler
and entity. I believe the AGX driver [3] is using this approach and
Boris may use approach as well for the Mali driver [4].

To support a 1 to 1 relationship we move the main execution function
from a kthread to a work queue and add a new scheduling mode which
bypasses code in the DRM which isn't needed in a 1 to 1 relationship.
The new scheduling mode should unify all drivers usage with a 1 to 1
relationship and can be thought of as using scheduler as a dependency /
infligt job tracker rather than a true scheduler.

- Generic mes

Re: [PATCH] drm: bridge: ldb: add support for using channel 1 only

[Luca Ceresoli]

Hi Marek,

thanks for the quick and detailed review!

On Wed, 5 Apr 2023 05:28:16 +0200
Marek Vasut  wrote:

> On 4/4/23 09:37, Luca Ceresoli wrote:
> 
> [...]
> 
> > @@ -177,28 +183,25 @@ static void fsl_ldb_atomic_enable(struct drm_bridge 
> > *bridge,
> > clk_prepare_enable(fsl_ldb->clk);
> >   
> > /* Program LDB_CTRL */
> > -   reg = LDB_CTRL_CH0_ENABLE;
> > -
> > -   if (fsl_ldb->lvds_dual_link)
> > -   reg |= LDB_CTRL_CH1_ENABLE | LDB_CTRL_SPLIT_MODE;
> > -
> > -   if (lvds_format_24bpp) {
> > -   reg |= LDB_CTRL_CH0_DATA_WIDTH;
> > -   if (fsl_ldb->lvds_dual_link)
> > -   reg |= LDB_CTRL_CH1_DATA_WIDTH;
> > -   }
> > -
> > -   if (lvds_format_jeida) {
> > -   reg |= LDB_CTRL_CH0_BIT_MAPPING;
> > -   if (fsl_ldb->lvds_dual_link)
> > -   reg |= LDB_CTRL_CH1_BIT_MAPPING;
> > -   }
> > -
> > -   if (mode->flags & DRM_MODE_FLAG_PVSYNC) {
> > -   reg |= LDB_CTRL_DI0_VSYNC_POLARITY;
> > -   if (fsl_ldb->lvds_dual_link)
> > -   reg |= LDB_CTRL_DI1_VSYNC_POLARITY;
> > -   }
> > +   reg =  
> 
> Cosmetic nit, do we need the newline here , can't we just move the first 
> '(fsl_ldb->ch0_enabled ? LDB_CTRL_CH0_ENABLE : 0) |' on the same line as 
> 'reg =' ? It might need a bit of indent with spaces, but that should be OK.

Sure. Maybe 'reg =(fsl...' would be even better, it checkpatch
allows.

> > @@ -311,10 +314,23 @@ static int fsl_ldb_probe(struct platform_device *pdev)
> > if (IS_ERR(fsl_ldb->regmap))
> > return PTR_ERR(fsl_ldb->regmap);
> >   
> > -   /* Locate the panel DT node. */
> > -   panel_node = of_graph_get_remote_node(dev->of_node, 1, 0);
> > -   if (!panel_node)
> > -   return -ENXIO;
> > +   /* Locate the remote ports and the panel node */
> > +   remote1 = of_graph_get_remote_node(dev->of_node, 1, 0);
> > +   remote2 = of_graph_get_remote_node(dev->of_node, 2, 0);
> > +   fsl_ldb->ch0_enabled = (remote1 != NULL);
> > +   fsl_ldb->ch1_enabled = (remote2 != NULL);
> > +   panel_node = of_node_get(remote1 ? remote1 : remote2);  
> 
> You can even do this without the middle 'remote1' I think:
> 
> panel_node = of_node_get(remote1 ? : remote2);

Apparently, but honestly with such short expressions clearly having no
side effects I think it's not helping readability.

> > +   of_node_put(remote1);
> > +   of_node_put(remote2);
> > +
> > +   if (!fsl_ldb->ch0_enabled && !fsl_ldb->ch1_enabled) {
> > +   of_node_put(panel_node);
> > +   return dev_err_probe(dev, -ENXIO, "No panel node found");
> > +   }
> > +
> > +   dev_dbg(dev, "Using %s\n",
> > +   fsl_ldb_is_dual(fsl_ldb) ? "dual mode" :  
> 
> I think this is called "dual-link mode" , maybe update the string .

I was using the terms from the NXP docs, but indeed in the kernel it
seems that "dual-link" is the common name. Updating that.

> > @@ -325,20 +341,26 @@ static int fsl_ldb_probe(struct platform_device *pdev)
> > if (IS_ERR(fsl_ldb->panel_bridge))
> > return PTR_ERR(fsl_ldb->panel_bridge);
> >   
> > -   /* Determine whether this is dual-link configuration */
> > -   port1 = of_graph_get_port_by_id(dev->of_node, 1);
> > -   port2 = of_graph_get_port_by_id(dev->of_node, 2);
> > -   dual_link = drm_of_lvds_get_dual_link_pixel_order(port1, port2);
> > -   of_node_put(port1);
> > -   of_node_put(port2);
> >   
> > -   if (dual_link == DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS) {
> > -   dev_err(dev, "LVDS channel pixel swap not supported.\n");
> > -   return -EINVAL;
> > -   }
> > +   if (fsl_ldb_is_dual(fsl_ldb)) {
> > +   struct device_node *port1, *port2;
> > +
> > +   port1 = of_graph_get_port_by_id(dev->of_node, 1);
> > +   port2 = of_graph_get_port_by_id(dev->of_node, 2);
> > +   dual_link = drm_of_lvds_get_dual_link_pixel_order(port1, port2);
> > +   of_node_put(port1);
> > +   of_node_put(port2);
> >   
> > -   if (dual_link == DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS)
> > -   fsl_ldb->lvds_dual_link = true;
> > +   if (dual_link < 0)
> > +   return dev_err_probe(dev, dual_link,
> > +"Error getting dual link 
> > configuration");  
> 
> Does this need a trailing '\n' in the formatting string or not ? I think 
> yes.

Oops, good catch.

> The rest looks good, with the few details fixed:
> 
> Reviewed-by: Marek Vasut 

Thanks!

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] drm/atomic-helper: Do not assume vblank is always present

[Javier Martinez Canillas]

Hello Zack,

Thanks for your patch.

Zack Rusin  writes:

> From: Zack Rusin 
>
> Many drivers (in particular all of the virtualized drivers) do not
> implement vblank handling. Assuming vblank is always present
> leads to crashes.
>
> Fix the crashes by making sure the device supports vblank before using
> it.
>

[...]

> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 299fa2a19a90..6438aeb1c65f 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -997,12 +997,16 @@ int drm_crtc_next_vblank_start(struct drm_crtc *crtc, 
> ktime_t *vblanktime)
>  {
>   unsigned int pipe = drm_crtc_index(crtc);
>   struct drm_vblank_crtc *vblank = &crtc->dev->vblank[pipe];
> - struct drm_display_mode *mode = &vblank->hwmode;
> + struct drm_display_mode *mode;
>   u64 vblank_start;
>  
> + if (!drm_dev_has_vblank(crtc->dev))
> + return -EINVAL;
> +
>   if (!vblank->framedur_ns || !vblank->linedur_ns)
>   return -EINVAL;
>  
> + mode = &vblank->hwmode;
>   if (!drm_crtc_get_last_vbltimestamp(crtc, vblanktime, false))
>   return -EINVAL;
>  

Rob already posted more or less the same fix:

https://lore.kernel.org/lkml/caf6aegsdt95-uakcv2+hdmlkd2xwfpen+fmudtrmc-ps7wb...@mail.gmail.com/T/

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [RFC PATCH 00/10] Xe DRM scheduler and long running workload plans

[Christian König]

Am 04.04.23 um 15:37 schrieb Matthew Brost:

On Tue, Apr 04, 2023 at 11:13:28AM +0200, Christian König wrote:

Hi,

Am 04.04.23 um 02:22 schrieb Matthew Brost:

Hello,

As a prerequisite to merging the new Intel Xe DRM driver [1] [2], we
have been asked to merge our common DRM scheduler patches first as well
as develop a common solution for long running workloads with the DRM
scheduler. This RFC series is our first attempt at doing this. We
welcome any and all feedback.

This can we thought of as 4 parts detailed below.

- DRM scheduler changes for 1 to 1 relationship between scheduler and
entity (patches 1-3)

In Xe all of the scheduling of jobs is done by a firmware scheduler (the
GuC) which is a new paradigm WRT to the DRM scheduler and presents
severals problems as the DRM was originally designed to schedule jobs on
hardware queues. The main problem being that DRM scheduler expects the
submission order of jobs to be the completion order of jobs even across
multiple entities. This assumption falls apart with a firmware scheduler
as a firmware scheduler has no concept of jobs and jobs can complete out
of order. A novel solution for was originally thought of by Faith during
the initial prototype of Xe, create a 1 to 1 relationship between scheduler
and entity. I believe the AGX driver [3] is using this approach and
Boris may use approach as well for the Mali driver [4].

To support a 1 to 1 relationship we move the main execution function
from a kthread to a work queue and add a new scheduling mode which
bypasses code in the DRM which isn't needed in a 1 to 1 relationship.
The new scheduling mode should unify all drivers usage with a 1 to 1
relationship and can be thought of as using scheduler as a dependency /
infligt job tracker rather than a true scheduler.

- Generic messaging interface for DRM scheduler

Idea is to be able to communicate to the submission backend with in band
(relative to main execution function) messages. Messages are backend
defined and flexable enough for any use case. In Xe we use these
messages to clean up entites, set properties for entites, and suspend /
resume execution of an entity [5]. I suspect other driver can leverage
this messaging concept too as it a convenient way to avoid races in the
backend.

Oh, please absolutely *don't* do this.

This is basically the design which makes a bunch of stuff so horrible broken
on Windows.

I can explain it in more detail if necessary, but I strongly recommend to
not go down this path.


I'm afraid we are going to have to discuss this further. Let me explain
my reasoning, basically the idea is to have a single main entry point to
backend - the work queue. This avoids the need for lock between run_job
and any message that changes an entites state, also it really helps
during the reset flows (either TDR or GT reset) as we can call
drm_sched_run_wq_stop can ensure that nothing else is in the backend
changing an entity state. It all works out really nicely actually, our
GuC backend is incredibly stable (hasn't really had a bug pop in about a
year) and way simpler than what we did in the i915. I think the simplity
to largely due to this design of limiting the entry points.

I personally don't see how this a poor design, limiting entry points
absolutely makes sense to me, if it didn't why not just call cleanup_job
bypassing the main execution thread (now worker), this is the exact same
concept.


Well then I strongly suggest to read a few analyses on the failure of 
the message processing loop on Windows.


Have you ever wondered why classic Win32 applications sometimes seems to 
be stuck and don't do anything? This design pattern combine with 
timeouts to solve deadlocks is the reason for that.


The major problem with this approach is that analyzing tools like 
lockdep have a hard time grasping the dependencies.


What you can do is to offload all your operations which are supposed to 
be run in the same thread as work items into a work queue. This is 
something lockdep understands and is able to scream out lout if someone 
messes up the deadlock dependencies.


Regards,
Christian.



FWIW Asahi liked the idea as well and think it could be useful for AGX.
Matt


Regards,
Christian.


- Support for using TDR for all error paths of a scheduler / entity

Fix a few races / bugs, add function to dynamically set the TDR timeout.

- Annotate dma-fences for long running workloads.

The idea here is to use dma-fences only as sync points within the
scheduler and never export them for long running workloads. By
annotating these fences as long running we ensure that these dma-fences
are never used in a way that breaks the dma-fence rules. A benefit of
thus approach is the scheduler can still safely flow control the
execution ring buffer via the job limit without breaking the dma-fence
rules.

Again this a first draft and looking forward to feedback.

Enjoy - Matt

[1] https://gitlab.freedesktop.org/drm/xe/kernel
[2] https://patchwork.freedesktop.org/

Re: [Intel-gfx] [PATCH 7/7] drm/i915: Allow user to set cache at BO creation

[Lionel Landwerlin]

On 04/04/2023 19:04, Yang, Fei wrote:

Subject: Re: [Intel-gfx] [PATCH 7/7] drm/i915: Allow user to set cache at BO 
creation

On 01/04/2023 09:38, fei.y...@intel.com wrote:

From: Fei Yang 

To comply with the design that buffer objects shall have immutable
cache setting through out its life cycle, {set, get}_caching ioctl's
are no longer supported from MTL onward. With that change caching
policy can only be set at object creation time. The current code
applies a default (platform dependent) cache setting for all objects.
However this is not optimal for performance tuning. The patch extends
the existing gem_create uAPI to let user set PAT index for the object
at creation time.
The new extension is platform independent, so UMD's can switch to
using this extension for older platforms as well, while {set,
get}_caching are still supported on these legacy paltforms for compatibility 
reason.

Cc: Chris Wilson 
Cc: Matt Roper 
Signed-off-by: Fei Yang 
Reviewed-by: Andi Shyti 


Just like the protected content uAPI, there is no way for userspace to tell
this feature is available other than trying using it.

Given the issues with protected content, is it not thing we could want to add?

Sorry I'm not aware of the issues with protected content, could you elaborate?
There was a long discussion on teams uAPI channel, could you comment there if
any concerns?

https://teams.microsoft.com/l/message/19:f1767bda6734476ba0a9c7d147b928d1@thread.skype/1675860924675?tenantId=46c98d88-e344-4ed4-8496-4ed7712e255d&groupId=379f3ae1-d138-4205-bb65-d4c7d38cb481&parentMessageId=1675860924675&teamName=GSE%20OSGC&channelName=i915%20uAPI%20changes&createdTime=1675860924675&allowXTenantAccess=false

Thanks,
-Fei



We wanted to have a getparam to detect protected support and were told 
to detect it by trying to create a context with it.


Now it appears trying to create a protected context can block for 
several seconds.


Since we have to report capabilities to the user even before it creates 
protected contexts, any app is at risk of blocking.



-Lionel





Thanks,

-Lionel



---
   drivers/gpu/drm/i915/gem/i915_gem_create.c | 33 
   include/uapi/drm/i915_drm.h| 36 ++
   tools/include/uapi/drm/i915_drm.h  | 36 ++
   3 files changed, 105 insertions(+)

Re: [PATCH 7/8] video/aperture: Only remove sysfb on the default vga pci device

[Aaron Plattner]

On 4/4/23 1:18 PM, Daniel Vetter wrote:

Instead of calling aperture_remove_conflicting_devices() to remove the
conflicting devices, just call to aperture_detach_devices() to detach
the device that matches the same PCI BAR / aperture range. Since the
former is just a wrapper of the latter plus a sysfb_disable() call,
and now that's done in this function but only for the primary devices.

This fixes a regression introduced by ee7a69aa38d8 ("fbdev: Disable
sysfb device registration when removing conflicting FBs"), where we
remove the sysfb when loading a driver for an unrelated pci device,
resulting in the user loosing their efifb console or similar.

Note that in practice this only is a problem with the nvidia blob,
because that's the only gpu driver people might install which does not
come with an fbdev driver of it's own. For everyone else the real gpu
driver will restore a working console.


It might be worth noting that this also affects devices that have no 
driver installed, or where the driver failed to initialize or was 
configured not to set a mode. E.g. I reproduced this problem on a laptop 
with i915.modeset=0 and an NVIDIA driver that calls 
drm_fbdev_generic_setup. It would also reproduce on a system that sets 
modeset=0 (or has a GPU that's too new for its corresponding kernel 
driver) and that passes an NVIDIA GPU through to a VM using vfio-pci 
since that also calls aperture_remove_conflicting_pci_devices.


I agree that in practice this will mostly affect people with our driver 
until I get my changes to add drm_fbdev_generic_setup checked in. But 
these other cases don't seem all that unlikely to me.


-- Aaron


Also note that in the referenced bug there's confusion that this same
bug also happens on amdgpu. But that was just another amdgpu specific
regression, which just happened to happen at roughly the same time and
with the same user-observable symptoms. That bug is fixed now, see
https://bugzilla.kernel.org/show_bug.cgi?id=216331#c15

Note that we should not have any such issues on non-pci multi-gpu
issues, because I could only find two such cases:
- SoC with some external panel over spi or similar. These panel
   drivers do not use drm_aperture_remove_conflicting_framebuffers(),
   so no problem.
- vga+mga, which is a direct console driver and entirely bypasses all
   this.

For the above reasons the cc: stable is just notionally, this patch
will need a backport and that's up to nvidia if they care enough.

v2:
- Explain a bit better why other multi-gpu that aren't pci shouldn't
   have any issues with making all this fully pci specific.

v3
- polish commit message (Javier)

Fixes: ee7a69aa38d8 ("fbdev: Disable sysfb device registration when removing 
conflicting FBs")
Tested-by: Aaron Plattner 
Reviewed-by: Javier Martinez Canillas 
References: https://bugzilla.kernel.org/show_bug.cgi?id=216303#c28
Signed-off-by: Daniel Vetter 
Cc: Aaron Plattner 
Cc: Javier Martinez Canillas 
Cc: Thomas Zimmermann 
Cc: Helge Deller 
Cc: Sam Ravnborg 
Cc: Alex Deucher 
Cc:  # v5.19+ (if someone else does the backport)
---
  drivers/video/aperture.c | 7 ---
  1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c
index 8f1437339e49..2394c2d310f8 100644
--- a/drivers/video/aperture.c
+++ b/drivers/video/aperture.c
@@ -321,15 +321,16 @@ int aperture_remove_conflicting_pci_devices(struct 
pci_dev *pdev, const char *na
  
  	primary = pdev == vga_default_device();
  
+	if (primary)

+   sysfb_disable();
+
for (bar = 0; bar < PCI_STD_NUM_BARS; ++bar) {
if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM))
continue;
  
  		base = pci_resource_start(pdev, bar);

size = pci_resource_len(pdev, bar);
-   ret = aperture_remove_conflicting_devices(base, size, name);
-   if (ret)
-   return ret;
+   aperture_detach_devices(base, size);
}
  
  	if (primary) {

Re: [PATCH 1/8] drm/gma500: Use drm_aperture_remove_conflicting_pci_framebuffers

[Thomas Zimmermann]

Hi

Am 04.04.23 um 22:18 schrieb Daniel Vetter:

This one nukes all framebuffers, which is a bit much. In reality
gma500 is igpu and never shipped with anything discrete, so there should
not be any difference.

v2: Unfortunately the framebuffer sits outside of the pci bars for
gma500, and so only using the pci helpers won't be enough. Otoh if we
only use non-pci helper, then we don't get the vga handling, and
subsequent refactoring to untangle these special cases won't work.

It's not pretty, but the simplest fix (since gma500 really is the only
quirky pci driver like this we have) is to just have both calls.

Signed-off-by: Daniel Vetter 
Cc: Patrik Jakobsson 
Cc: Thomas Zimmermann 
Cc: Javier Martinez Canillas 
---
  drivers/gpu/drm/gma500/psb_drv.c | 9 +++--
  1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
index 2ce96b1b9c74..f1e0eed8fea4 100644
--- a/drivers/gpu/drm/gma500/psb_drv.c
+++ b/drivers/gpu/drm/gma500/psb_drv.c
@@ -422,12 +422,17 @@ static int psb_pci_probe(struct pci_dev *pdev, const 
struct pci_device_id *ent)
  
  	/*

 * We cannot yet easily find the framebuffer's location in memory. So
-* remove all framebuffers here.
+* remove all framebuffers here. Note that we still want the pci special
+* handling to kick out vgacon.
 *
 * TODO: Refactor psb_driver_load() to map vdc_reg earlier. Then we
 *   might be able to read the framebuffer range from the device.
 */
-   ret = drm_aperture_remove_framebuffers(true, &driver);
+   ret = drm_aperture_remove_framebuffers(false, &driver);
+   if (ret)
+   return ret;
+
+   ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &driver);


This simply isn't it. If you have to work around your own API, it's time 
to rethink the API.


Best regards
Thomas


if (ret)
return ret;
  


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


OpenPGP_signature
Description: OpenPGP digital signature

[PATCH 1/1] drm/bridge: ti-sn65dsi83: use dev_err_probe if host attach failed

[Alexander Stein]

There might be cases where the host attach is deferred, use dev_err_probe
to add more detailed information to /sys/kernel/debug/devices_deferred.

Signed-off-by: Alexander Stein 
---
 drivers/gpu/drm/bridge/ti-sn65dsi83.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c 
b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index 91ecfbe45bf9..988e537ab884 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
@@ -698,8 +698,10 @@ static int sn65dsi83_probe(struct i2c_client *client)
drm_bridge_add(&ctx->bridge);
 
ret = sn65dsi83_host_attach(ctx);
-   if (ret)
+   if (ret) {
+   dev_err_probe(dev, ret, "failed to attach DSI host\n");
goto err_remove_bridge;
+   }
 
return 0;
 
-- 
2.34.1

Re: [PATCH v10 11/15] drm/atomic-helper: Set fence deadline for vblank

[Daniel Vetter]

On Wed, Apr 05, 2023 at 12:53:29AM +0300, Dmitry Baryshkov wrote:
> On 04/04/2023 22:16, Daniel Vetter wrote:
> > On Tue, Apr 04, 2023 at 08:22:05PM +0300, Dmitry Baryshkov wrote:
> > > On 08/03/2023 17:53, Rob Clark wrote:
> > > > From: Rob Clark 
> > > > 
> > > > For an atomic commit updating a single CRTC (ie. a pageflip) calculate
> > > > the next vblank time, and inform the fence(s) of that deadline.
> > > > 
> > > > v2: Comment typo fix (danvet)
> > > > v3: If there are multiple CRTCs, consider the time of the soonest vblank
> > > > 
> > > > Signed-off-by: Rob Clark 
> > > > Reviewed-by: Daniel Vetter 
> > > > Signed-off-by: Rob Clark 
> > > > ---
> > > >drivers/gpu/drm/drm_atomic_helper.c | 37 
> > > > +
> > > >1 file changed, 37 insertions(+)
> > > 
> > > As I started playing with hotplug on RB5 (sm8250, DSI-HDMI bridge), I 
> > > found
> > > that this patch introduces the following backtrace on HDMI hotplug. Is 
> > > there
> > > anything that I can do to debug/fix the issue? The warning seems harmless,
> > > but it would be probably be good to still fix it. With addresses decoded:
> > 
> > Bit a shot in the dark, but does the below help?
> 
> This indeed seems to fix the issue. I'm not sure about the possible side
> effects, but, if you were to send the patch:
> 
> Tested-by: Dmitry Baryshkov 

Thanks for the quick feedback, I already discussed this with Rob on irc
yesterday (and landed his more throughrough version of the drm_vblank.c
fix to drm-misc-next). I'll polish the drm_atomic_helper.c part asap and
will send it out. Would be great if you can then retest to make sure all
the pieces still work together for your case.
-Daniel

> 
> > 
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> > b/drivers/gpu/drm/drm_atomic_helper.c
> > index f21b5a74176c..6640d80d84f3 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -1528,6 +1528,9 @@ static void set_fence_deadline(struct drm_device *dev,
> > for_each_new_crtc_in_state (state, crtc, new_crtc_state, i) {
> > ktime_t v;
> > +   if (drm_atomic_crtc_needs_modeset(new_crtc_state))
> > +   continue;
> > +
> > if (drm_crtc_next_vblank_start(crtc, &v))
> > continue;
> > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> > index 78a8c51a4abf..7ae38e8e27e8 100644
> > --- a/drivers/gpu/drm/drm_vblank.c
> > +++ b/drivers/gpu/drm/drm_vblank.c
> > @@ -1001,6 +1001,9 @@ int drm_crtc_next_vblank_start(struct drm_crtc *crtc, 
> > ktime_t *vblanktime)
> > struct drm_display_mode *mode = &vblank->hwmode;
> > u64 vblank_start;
> > +   if (!drm_dev_has_vblank(crtc->dev))
> > +   return -EINVAL;
> > +
> > if (!vblank->framedur_ns || !vblank->linedur_ns)
> > return -EINVAL;
> > 
> > > 
> > > [   31.151348] [ cut here ]
> > > [   31.157043] msm_dpu ae01000.display-controller:
> > > drm_WARN_ON_ONCE(drm_drv_uses_atomic_modeset(dev))
> > > [   31.157177] WARNING: CPU: 0 PID: 13 at drivers/gpu/drm/drm_vblank.c:728
> > > drm_crtc_vblank_helper_get_vblank_timestamp_internal
> > > (drivers/gpu/drm/drm_vblank.c:728)
> > > [   31.180629] Modules linked in:
> > > [   31.184106] CPU: 0 PID: 13 Comm: kworker/0:1 Not tainted
> > > 6.3.0-rc2-8-gd39e48ca80c0 #542
> > > [   31.193358] Hardware name: Qualcomm Technologies, Inc. Robotics RB5 
> > > (DT)
> > > [   31.200796] Workqueue: events lt9611uxc_hpd_work
> > > [   31.205990] pstate: 6045 (nZCv daif +PAN -UAO -TCO -DIT -SSBS
> > > BTYPE=--)
> > > [   31.213722] pc : drm_crtc_vblank_helper_get_vblank_timestamp_internal
> > > (drivers/gpu/drm/drm_vblank.c:728)
> > > [   31.222032] lr : drm_crtc_vblank_helper_get_vblank_timestamp_internal
> > > (drivers/gpu/drm/drm_vblank.c:728)
> > > [   31.230341] sp : 880bb8d0
> > > [   31.234061] x29: 880bb900 x28: 0038 x27:
> > > 61a7956b8d60
> > > [   31.242051] x26:  x25:  x24:
> > > 880bb9c4
> > > [   31.250038] x23: 0001 x22: bf0033b94ef0 x21:
> > > 61a7957901d0
> > > [   31.258029] x20: 61a79571 x19: 61a78128b000 x18:
> > > fffec278
> > > [   31.266014] x17: 00400465 x16: 0020 x15:
> > > 0060
> > > [   31.274001] x14: 0001 x13: bf00354550e0 x12:
> > > 0825
> > > [   31.281989] x11: 02b7 x10: bf00354b1208 x9 :
> > > bf00354550e0
> > > [   31.289976] x8 : efff x7 : bf00354ad0e0 x6 :
> > > 02b7
> > > [   31.297963] x5 : 61a8feebbe48 x4 : 4000f2b7 x3 :
> > > a2a8c9f64000
> > > [   31.305947] x2 :  x1 :  x0 :
> > > 61a780283100
> > > [   31.313934] Call trace:
> > > [   31.316719] drm_crtc_vblank_helper_get_vblank_timestamp_internal
> > > (drivers/gpu/drm/drm_vblank.c:728)

Re: [PATCH] drm/atomic-helper: Do not assume vblank is always present

[Daniel Vetter]

On Wed, Apr 05, 2023 at 09:35:45AM +0200, Javier Martinez Canillas wrote:
> 
> Hello Zack,
> 
> Thanks for your patch.
> 
> Zack Rusin  writes:
> 
> > From: Zack Rusin 
> >
> > Many drivers (in particular all of the virtualized drivers) do not
> > implement vblank handling. Assuming vblank is always present
> > leads to crashes.
> >
> > Fix the crashes by making sure the device supports vblank before using
> > it.
> >
> 
> [...]
> 
> > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> > index 299fa2a19a90..6438aeb1c65f 100644
> > --- a/drivers/gpu/drm/drm_vblank.c
> > +++ b/drivers/gpu/drm/drm_vblank.c
> > @@ -997,12 +997,16 @@ int drm_crtc_next_vblank_start(struct drm_crtc *crtc, 
> > ktime_t *vblanktime)
> >  {
> > unsigned int pipe = drm_crtc_index(crtc);
> > struct drm_vblank_crtc *vblank = &crtc->dev->vblank[pipe];
> > -   struct drm_display_mode *mode = &vblank->hwmode;
> > +   struct drm_display_mode *mode;
> > u64 vblank_start;
> >  
> > +   if (!drm_dev_has_vblank(crtc->dev))
> > +   return -EINVAL;
> > +
> > if (!vblank->framedur_ns || !vblank->linedur_ns)
> > return -EINVAL;
> >  
> > +   mode = &vblank->hwmode;
> > if (!drm_crtc_get_last_vbltimestamp(crtc, vblanktime, false))
> > return -EINVAL;
> >  
> 
> Rob already posted more or less the same fix:
> 
> https://lore.kernel.org/lkml/caf6aegsdt95-uakcv2+hdmlkd2xwfpen+fmudtrmc-ps7wb...@mail.gmail.com/T/

Yeah I pushed it to drm-misc-next yesterday, please check that works. Also
note that we still (in some cases at least where) need another fix to
handle the crtc on/off state transitions (but only for drivers which do
have vblank), I'll send that out asap.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/1] drm/bridge: ti-sn65dsi83: use dev_err_probe if host attach failed

[Laurent Pinchart]

Hi Alexander,

Thank you for the patch.

On Wed, Apr 05, 2023 at 09:52:23AM +0200, Alexander Stein wrote:
> There might be cases where the host attach is deferred, use dev_err_probe
> to add more detailed information to /sys/kernel/debug/devices_deferred.
> 
> Signed-off-by: Alexander Stein 

Reviewed-by: Laurent Pinchart 

> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi83.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c 
> b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> index 91ecfbe45bf9..988e537ab884 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> @@ -698,8 +698,10 @@ static int sn65dsi83_probe(struct i2c_client *client)
>   drm_bridge_add(&ctx->bridge);
>  
>   ret = sn65dsi83_host_attach(ctx);
> - if (ret)
> + if (ret) {
> + dev_err_probe(dev, ret, "failed to attach DSI host\n");
>   goto err_remove_bridge;
> + }
>  
>   return 0;
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/8] drm/gma500: Use drm_aperture_remove_conflicting_pci_framebuffers

[Patrik Jakobsson]

On Wed, Apr 5, 2023 at 9:49 AM Thomas Zimmermann  wrote:
>
> Hi
>
> Am 04.04.23 um 22:18 schrieb Daniel Vetter:
> > This one nukes all framebuffers, which is a bit much. In reality
> > gma500 is igpu and never shipped with anything discrete, so there should
> > not be any difference.
> >
> > v2: Unfortunately the framebuffer sits outside of the pci bars for
> > gma500, and so only using the pci helpers won't be enough. Otoh if we
> > only use non-pci helper, then we don't get the vga handling, and
> > subsequent refactoring to untangle these special cases won't work.
> >
> > It's not pretty, but the simplest fix (since gma500 really is the only
> > quirky pci driver like this we have) is to just have both calls.
> >
> > Signed-off-by: Daniel Vetter 
> > Cc: Patrik Jakobsson 
> > Cc: Thomas Zimmermann 
> > Cc: Javier Martinez Canillas 
> > ---
> >   drivers/gpu/drm/gma500/psb_drv.c | 9 +++--
> >   1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/gma500/psb_drv.c 
> > b/drivers/gpu/drm/gma500/psb_drv.c
> > index 2ce96b1b9c74..f1e0eed8fea4 100644
> > --- a/drivers/gpu/drm/gma500/psb_drv.c
> > +++ b/drivers/gpu/drm/gma500/psb_drv.c
> > @@ -422,12 +422,17 @@ static int psb_pci_probe(struct pci_dev *pdev, const 
> > struct pci_device_id *ent)
> >
> >   /*
> >* We cannot yet easily find the framebuffer's location in memory. So
> > -  * remove all framebuffers here.
> > +  * remove all framebuffers here. Note that we still want the pci 
> > special
> > +  * handling to kick out vgacon.
> >*
> >* TODO: Refactor psb_driver_load() to map vdc_reg earlier. Then we
> >*   might be able to read the framebuffer range from the device.
> >*/
> > - ret = drm_aperture_remove_framebuffers(true, &driver);
> > + ret = drm_aperture_remove_framebuffers(false, &driver);
> > + if (ret)
> > + return ret;
> > +
> > + ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &driver);
>
> This simply isn't it. If you have to work around your own API, it's time
> to rethink the API.

Would it help if we figure out the stolen range here? It can
supposedly be found by reading pci config space, so no need to map vdc
regs first.

GBSM is the stolen base and TOLUD - GBSM = stolen size. Or read the
size out from GGC. Not sure which one is more reliable.

-Patrik

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

Re: [PATCH 1/8] drm/gma500: Use drm_aperture_remove_conflicting_pci_framebuffers

[Thomas Zimmermann]

Hi

Am 05.04.23 um 09:49 schrieb Thomas Zimmermann:

Hi

Am 04.04.23 um 22:18 schrieb Daniel Vetter:

This one nukes all framebuffers, which is a bit much. In reality
gma500 is igpu and never shipped with anything discrete, so there should
not be any difference.

v2: Unfortunately the framebuffer sits outside of the pci bars for
gma500, and so only using the pci helpers won't be enough. Otoh if we
only use non-pci helper, then we don't get the vga handling, and
subsequent refactoring to untangle these special cases won't work.

It's not pretty, but the simplest fix (since gma500 really is the only
quirky pci driver like this we have) is to just have both calls.

Signed-off-by: Daniel Vetter 
Cc: Patrik Jakobsson 
Cc: Thomas Zimmermann 
Cc: Javier Martinez Canillas 
---
  drivers/gpu/drm/gma500/psb_drv.c | 9 +++--
  1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/gma500/psb_drv.c 
b/drivers/gpu/drm/gma500/psb_drv.c

index 2ce96b1b9c74..f1e0eed8fea4 100644
--- a/drivers/gpu/drm/gma500/psb_drv.c
+++ b/drivers/gpu/drm/gma500/psb_drv.c
@@ -422,12 +422,17 @@ static int psb_pci_probe(struct pci_dev *pdev, 
const struct pci_device_id *ent)

  /*
   * We cannot yet easily find the framebuffer's location in 
memory. So

- * remove all framebuffers here.
+ * remove all framebuffers here. Note that we still want the pci 
special

+ * handling to kick out vgacon.
   *
   * TODO: Refactor psb_driver_load() to map vdc_reg earlier. Then we
   *   might be able to read the framebuffer range from the 
device.

   */
-    ret = drm_aperture_remove_framebuffers(true, &driver);
+    ret = drm_aperture_remove_framebuffers(false, &driver);
+    if (ret)
+    return ret;
+
+    ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, 
&driver);


This simply isn't it. If you have to work around your own API, it's time 
to rethink the API.


Here's a proposal:

 1) As you're changing aperture_remove_conflicting_devices() anyway, 
rename it to aperture_remove_conflicting_devices_at(), so it's clear 
that it takes a memory range.


 2) Introduce aperture_remove_conflicting_pci_devices_at(), which takes 
a PCI device and a memory range. It should do the is_primary and vgacon 
stuff, but kick out the range.


 3) Call aperture_remove_conflicting_pci_devices_at() from gma500 with 
the full range. Even if we can restructure gma500 to detect the firmware 
framebuffer from its registers (there's this TODO item), we'd still need 
aperture_remove_conflicting_pci_devices_at() to do something useful with it.


 4) With that, aperture_remove_conflicting_devices_at() can drop the 
primary argument.


Of course, the DRM-related interface should be adapted as well. There's 
a bit of overlap in the implementation of both PCI aperture helpers, but 
the overall interface is clear.


Best regards
Thomas



Best regards
Thomas


  if (ret)
  return ret;




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


OpenPGP_signature
Description: OpenPGP digital signature

Re: [PATCH] drm/vblank: Simplify drm_dev_has_vblank()

[Daniel Vetter]

On Wed, Apr 05, 2023 at 12:00:23AM +0300, Ville Syrjälä wrote:
> On Tue, Apr 04, 2023 at 10:46:00PM +0200, Daniel Vetter wrote:
> > On Mon, Apr 03, 2023 at 09:07:35AM -0700, Rob Clark wrote:
> > > From: Rob Clark 
> > > 
> > > What does vblank have to do with num_crtcs?  Well, this was technically
> > > correct, but you'd have to go look at where num_crtcs is initialized to
> > > understand why.  Lets just replace it with the simpler and more obvious
> > > check.
> > 
> > If you want to fix this, then I think the right fix is to rename num_crtcs
> > to be something like num_vblank_crtcs. It's a historical accident back
> > when vblanks without kms was a thing.
> > 
> > Plan B is someone gets really busy and fixes up the entire vblank mess and
> > moves it into drm_crtc struct. Now that the dri1 drivers are gone we could
> > indeed do that.
> 
> And easy first step could to simply wrap all the naked
> &dev->vblank[drm_crtc_index()] things into a function
> call with some cocci/etc. That way most of the vblank
> code doesn't need to care where that thing actually lives.

Yeah I think that might work out. Roughly:
- Wrap all the drm_vblank_crtc lookups
- Emebed it into drm_crtc, delete the drm_device->vblank array
- rename drm_device->num_crtc to something more meaningful maybe and move
  into drm_modeset_config

The big holdup always was step 2 because we still had to care about legacy
drivers without drm_crtc, which meant you'd have to have two paths, which
was kinda really annoying.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH v2 1/2] drm: bridge: ldb: add missing \n in dev_warn() string

[Luca Ceresoli]

dev_warn() and similar require a training \n.

Signed-off-by: Luca Ceresoli 
---
 drivers/gpu/drm/bridge/fsl-ldb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c
index 6bac160b395b..bb13a9143edd 100644
--- a/drivers/gpu/drm/bridge/fsl-ldb.c
+++ b/drivers/gpu/drm/bridge/fsl-ldb.c
@@ -170,7 +170,7 @@ static void fsl_ldb_atomic_enable(struct drm_bridge *bridge,
 
configured_link_freq = clk_get_rate(fsl_ldb->clk);
if (configured_link_freq != requested_link_freq)
-   dev_warn(fsl_ldb->dev, "Configured LDB clock (%lu Hz) does not 
match requested LVDS clock: %lu Hz",
+   dev_warn(fsl_ldb->dev, "Configured LDB clock (%lu Hz) does not 
match requested LVDS clock: %lu Hz\n",
 configured_link_freq,
 requested_link_freq);
 
-- 
2.34.1

[PATCH v2 2/2] drm: bridge: ldb: add support for using channel 1 only

[Luca Ceresoli]

The LDB driver currently checks whether dual mode is used, otherwise it
assumes only channel 0 is in use. Add support for using only channel 1. In
device tree terms, this means linking port 2 only.

Doing this cleanly requires changing the logic of the probe functions from
this:

 1. use of_graph_get_remote_node() on port 1 to find the panel
 2. use drm_of_lvds_get_dual_link_pixel_order() to detect dual mode

to this:

 1. use of_graph_get_remote_node() twice to find remote ports
 2. reuse the result of the above to know whether each channel is enabled
and to find the panel
 3. if (both channels as enabled)
use drm_of_lvds_get_dual_link_pixel_order() to detect dual mode

Also add a dev_dbg() to log the detected mode and log an error in case no
panel was found (no channel enabled).

Signed-off-by: Luca Ceresoli 
Reviewed-by: Marek Vasut 

---

Changes in v2 (suggested by Marek):
 - cosmetic: avoid pointless newline after 'reg =' and 'reg |='
 - use "dual-link" naming as elsewhere in the kernel
 - add missing trailing \n on dev_err_probe()
 - Add Marek's review tag
---
 drivers/gpu/drm/bridge/fsl-ldb.c | 101 ++-
 1 file changed, 59 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c
index bb13a9143edd..c20dba247735 100644
--- a/drivers/gpu/drm/bridge/fsl-ldb.c
+++ b/drivers/gpu/drm/bridge/fsl-ldb.c
@@ -84,10 +84,16 @@ struct fsl_ldb {
struct drm_bridge *panel_bridge;
struct clk *clk;
struct regmap *regmap;
-   bool lvds_dual_link;
const struct fsl_ldb_devdata *devdata;
+   bool ch0_enabled;
+   bool ch1_enabled;
 };
 
+static bool fsl_ldb_is_dual(const struct fsl_ldb *fsl_ldb)
+{
+   return (fsl_ldb->ch0_enabled && fsl_ldb->ch1_enabled);
+}
+
 static inline struct fsl_ldb *to_fsl_ldb(struct drm_bridge *bridge)
 {
return container_of(bridge, struct fsl_ldb, bridge);
@@ -95,7 +101,7 @@ static inline struct fsl_ldb *to_fsl_ldb(struct drm_bridge 
*bridge)
 
 static unsigned long fsl_ldb_link_frequency(struct fsl_ldb *fsl_ldb, int clock)
 {
-   if (fsl_ldb->lvds_dual_link)
+   if (fsl_ldb_is_dual(fsl_ldb))
return clock * 3500;
else
return clock * 7000;
@@ -177,28 +183,21 @@ static void fsl_ldb_atomic_enable(struct drm_bridge 
*bridge,
clk_prepare_enable(fsl_ldb->clk);
 
/* Program LDB_CTRL */
-   reg = LDB_CTRL_CH0_ENABLE;
+   reg =   (fsl_ldb->ch0_enabled ? LDB_CTRL_CH0_ENABLE : 0) |
+   (fsl_ldb->ch1_enabled ? LDB_CTRL_CH1_ENABLE : 0) |
+   (fsl_ldb_is_dual(fsl_ldb) ? LDB_CTRL_SPLIT_MODE : 0);
 
-   if (fsl_ldb->lvds_dual_link)
-   reg |= LDB_CTRL_CH1_ENABLE | LDB_CTRL_SPLIT_MODE;
+   if (lvds_format_24bpp)
+   reg |=  (fsl_ldb->ch0_enabled ? LDB_CTRL_CH0_DATA_WIDTH : 0) |
+   (fsl_ldb->ch1_enabled ? LDB_CTRL_CH1_DATA_WIDTH : 0);
 
-   if (lvds_format_24bpp) {
-   reg |= LDB_CTRL_CH0_DATA_WIDTH;
-   if (fsl_ldb->lvds_dual_link)
-   reg |= LDB_CTRL_CH1_DATA_WIDTH;
-   }
+   if (lvds_format_jeida)
+   reg |=  (fsl_ldb->ch0_enabled ? LDB_CTRL_CH0_BIT_MAPPING : 0) |
+   (fsl_ldb->ch1_enabled ? LDB_CTRL_CH1_BIT_MAPPING : 0);
 
-   if (lvds_format_jeida) {
-   reg |= LDB_CTRL_CH0_BIT_MAPPING;
-   if (fsl_ldb->lvds_dual_link)
-   reg |= LDB_CTRL_CH1_BIT_MAPPING;
-   }
-
-   if (mode->flags & DRM_MODE_FLAG_PVSYNC) {
-   reg |= LDB_CTRL_DI0_VSYNC_POLARITY;
-   if (fsl_ldb->lvds_dual_link)
-   reg |= LDB_CTRL_DI1_VSYNC_POLARITY;
-   }
+   if (mode->flags & DRM_MODE_FLAG_PVSYNC)
+   reg |=  (fsl_ldb->ch0_enabled ? LDB_CTRL_DI0_VSYNC_POLARITY : 
0) |
+   (fsl_ldb->ch1_enabled ? LDB_CTRL_DI1_VSYNC_POLARITY : 
0);
 
regmap_write(fsl_ldb->regmap, fsl_ldb->devdata->ldb_ctrl, reg);
 
@@ -210,9 +209,8 @@ static void fsl_ldb_atomic_enable(struct drm_bridge *bridge,
/* Wait for VBG to stabilize. */
usleep_range(15, 20);
 
-   reg |= LVDS_CTRL_CH0_EN;
-   if (fsl_ldb->lvds_dual_link)
-   reg |= LVDS_CTRL_CH1_EN;
+   reg |=  (fsl_ldb->ch0_enabled ? LVDS_CTRL_CH0_EN : 0) |
+   (fsl_ldb->ch1_enabled ? LVDS_CTRL_CH1_EN : 0);
 
regmap_write(fsl_ldb->regmap, fsl_ldb->devdata->lvds_ctrl, reg);
 }
@@ -265,7 +263,7 @@ fsl_ldb_mode_valid(struct drm_bridge *bridge,
 {
struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
 
-   if (mode->clock > (fsl_ldb->lvds_dual_link ? 16 : 8))
+   if (mode->clock > (fsl_ldb_is_dual(fsl_ldb) ? 16 : 8))
return MODE_CLOCK_HIGH;
 
return MODE_OK;
@@ -286,7 +284,7 @@ static int fsl_ldb_probe(struct platform_device *pdev)
 {
struct device *dev = &pdev->dev;
st

Re: [PATCH 1/8] drm/gma500: Use drm_aperture_remove_conflicting_pci_framebuffers

[Thomas Zimmermann]

Hi Patrik

Am 05.04.23 um 10:05 schrieb Patrik Jakobsson:

On Wed, Apr 5, 2023 at 9:49 AM Thomas Zimmermann  wrote:


Hi

Am 04.04.23 um 22:18 schrieb Daniel Vetter:

This one nukes all framebuffers, which is a bit much. In reality
gma500 is igpu and never shipped with anything discrete, so there should
not be any difference.

v2: Unfortunately the framebuffer sits outside of the pci bars for
gma500, and so only using the pci helpers won't be enough. Otoh if we
only use non-pci helper, then we don't get the vga handling, and
subsequent refactoring to untangle these special cases won't work.

It's not pretty, but the simplest fix (since gma500 really is the only
quirky pci driver like this we have) is to just have both calls.

Signed-off-by: Daniel Vetter 
Cc: Patrik Jakobsson 
Cc: Thomas Zimmermann 
Cc: Javier Martinez Canillas 
---
   drivers/gpu/drm/gma500/psb_drv.c | 9 +++--
   1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
index 2ce96b1b9c74..f1e0eed8fea4 100644
--- a/drivers/gpu/drm/gma500/psb_drv.c
+++ b/drivers/gpu/drm/gma500/psb_drv.c
@@ -422,12 +422,17 @@ static int psb_pci_probe(struct pci_dev *pdev, const 
struct pci_device_id *ent)

   /*
* We cannot yet easily find the framebuffer's location in memory. So
-  * remove all framebuffers here.
+  * remove all framebuffers here. Note that we still want the pci special
+  * handling to kick out vgacon.
*
* TODO: Refactor psb_driver_load() to map vdc_reg earlier. Then we
*   might be able to read the framebuffer range from the device.
*/
- ret = drm_aperture_remove_framebuffers(true, &driver);
+ ret = drm_aperture_remove_framebuffers(false, &driver);
+ if (ret)
+ return ret;
+
+ ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &driver);


This simply isn't it. If you have to work around your own API, it's time
to rethink the API.


Would it help if we figure out the stolen range here? It can
supposedly be found by reading pci config space, so no need to map vdc
regs first.

GBSM is the stolen base and TOLUD - GBSM = stolen size. Or read the
size out from GGC. Not sure which one is more reliable.


See my other email here. We'd still need a nice interface for the 
aperture helpers.


Best regards
Thomas



-Patrik



Best regards
Thomas


   if (ret)
   return ret;



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


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


OpenPGP_signature
Description: OpenPGP digital signature

[PATCH] drm/atomic-helper: Don't set deadline for modesets

[Daniel Vetter]

If the crtc is being switched on or off then the semantics of
computing the timestampe of the next vblank is somewhat ill-defined.
And indeed, the code splats with a warning in the timestamp
computation code. Specifically it hits the check to make sure that
atomic drivers have full set up the timing constants in the drm_vblank
structure, and that's just not the case before the crtc is actually
on.

For robustness it seems best to just not set deadlines for modesets.

Link: 
https://lore.kernel.org/dri-devel/dfc21f18-7e1e-48f0-c05a-d659b9c90...@linaro.org/
Fixes: d39e48ca80c0 ("drm/atomic-helper: Set fence deadline for vblank")
Cc: Rob Clark 
Cc: Daniel Vetter 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Thomas Zimmermann 
Reported-by: Dmitry Baryshkov 
Tested-by: Dmitry Baryshkov  # test patch only
Cc: Dmitry Baryshkov 
Signed-off-by: Daniel Vetter 
---
 drivers/gpu/drm/drm_atomic_helper.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
b/drivers/gpu/drm/drm_atomic_helper.c
index f21b5a74176c..6640d80d84f3 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1528,6 +1528,9 @@ static void set_fence_deadline(struct drm_device *dev,
for_each_new_crtc_in_state (state, crtc, new_crtc_state, i) {
ktime_t v;
 
+   if (drm_atomic_crtc_needs_modeset(new_crtc_state))
+   continue;
+
if (drm_crtc_next_vblank_start(crtc, &v))
continue;
 
-- 
2.40.0

Re: [PATCH 1/8] drm/gma500: Use drm_aperture_remove_conflicting_pci_framebuffers

[Thomas Zimmermann]

Hi

Am 04.04.23 um 22:18 schrieb Daniel Vetter:

This one nukes all framebuffers, which is a bit much. In reality
gma500 is igpu and never shipped with anything discrete, so there should
not be any difference.


I do own an Intel DN2800MT board with gma500 hardware. [1] It has a PCIe 
x1 slot. I never tried, but in principle, there could be another 
graphics card in the system. The linked spec say 'Discrete: None'. I 
don't know what that means exactly.


Best regards
Thomas

[1] 
https://ark.intel.com/content/www/us/en/ark/products/56455/intel-desktop-board-dn2800mt.html




v2: Unfortunately the framebuffer sits outside of the pci bars for
gma500, and so only using the pci helpers won't be enough. Otoh if we
only use non-pci helper, then we don't get the vga handling, and
subsequent refactoring to untangle these special cases won't work.

It's not pretty, but the simplest fix (since gma500 really is the only
quirky pci driver like this we have) is to just have both calls.

Signed-off-by: Daniel Vetter 
Cc: Patrik Jakobsson 
Cc: Thomas Zimmermann 
Cc: Javier Martinez Canillas 
---
  drivers/gpu/drm/gma500/psb_drv.c | 9 +++--
  1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
index 2ce96b1b9c74..f1e0eed8fea4 100644
--- a/drivers/gpu/drm/gma500/psb_drv.c
+++ b/drivers/gpu/drm/gma500/psb_drv.c
@@ -422,12 +422,17 @@ static int psb_pci_probe(struct pci_dev *pdev, const 
struct pci_device_id *ent)
  
  	/*

 * We cannot yet easily find the framebuffer's location in memory. So
-* remove all framebuffers here.
+* remove all framebuffers here. Note that we still want the pci special
+* handling to kick out vgacon.
 *
 * TODO: Refactor psb_driver_load() to map vdc_reg earlier. Then we
 *   might be able to read the framebuffer range from the device.
 */
-   ret = drm_aperture_remove_framebuffers(true, &driver);
+   ret = drm_aperture_remove_framebuffers(false, &driver);
+   if (ret)
+   return ret;
+
+   ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &driver);
if (ret)
return ret;
  


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


OpenPGP_signature
Description: OpenPGP digital signature

Re: [RFC PATCH 0/4] uapi, drm: Add and implement RLIMIT_GPUPRIO

[Daniel Vetter]

On Tue, 4 Apr 2023 at 12:45, Tvrtko Ursulin
 wrote:
>
>
> Hi,
>
> On 03/04/2023 20:40, Joshua Ashton wrote:
> > Hello all!
> >
> > I would like to propose a new API for allowing processes to control
> > the priority of GPU queues similar to RLIMIT_NICE/RLIMIT_RTPRIO.
> >
> > The main reason for this is for compositors such as Gamescope and
> > SteamVR vrcompositor to be able to create realtime async compute
> > queues on AMD without the need of CAP_SYS_NICE.
> >
> > The current situation is bad for a few reasons, one being that in order
> > to setcap the executable, typically one must run as root which involves
> > a pretty high privelage escalation in order to achieve one
> > small feat, a realtime async compute queue queue for VR or a compositor.
> > The executable cannot be setcap'ed inside a
> > container nor can the setcap'ed executable be run in a container with
> > NO_NEW_PRIVS.
> >
> > I go into more detail in the description in
> > `uapi: Add RLIMIT_GPUPRIO`.
> >
> > My initial proposal here is to add a new RLIMIT, `RLIMIT_GPUPRIO`,
> > which seems to make most initial sense to me to solve the problem.
> >
> > I am definitely not set that this is the best formulation however
> > or if this should be linked to DRM (in terms of it's scheduler
> > priority enum/definitions) in any way and and would really like other
> > people's opinions across the stack on this.
> >
> > Once initial concern is that potentially this RLIMIT could out-live
> > the lifespan of DRM. It sounds crazy saying it right now, something
> > that definitely popped into my mind when touching `resource.h`. :-)
> >
> > Anyway, please let me know what you think!
> > Definitely open to any feedback and advice you may have. :D
>
> Interesting! I tried to solved the similar problem two times in the past 
> already.
>
> First time I was proposing to tie nice to DRM scheduling priority [1] - if 
> the latter has been left at default - drawing the analogy with the 
> nice+ionice handling. That was rejected and I was nudged towards the cgroups 
> route.
>
> So with that second attempt I implemented a hierarchical opaque drm.priority 
> cgroup controller [2]. I think it would allow you to solve your use case too 
> by placing your compositor in a cgroup with an elevated priority level.
>
> Implementation wise in my proposal it was left to individual drivers to 
> "meld" the opaque cgroup drm.priority with the driver specific priority 
> concept.
>
> That too wasn't too popular with the feedback (AFAIR) that the priority is a 
> too subsystem specific concept.
>
> Finally I was left with a weight based drm cgroup controller, exactly 
> following the controls of the CPU and IO ones, but with much looser runtime 
> guarantees. [3]
>
> I don't think this last one works for your use case, at least not at the 
> current state for drm scheduling capability, where the implementation is a 
> "bit" too reactive for realtime.
>
> Depending on how the discussion around your rlimit proposal goes, perhaps one 
> alternative could be to go the cgroup route and add an attribute like 
> drm.realtime. That perhaps sounds abstract and generic enough to be passable. 
> Built as a simplification of [2] it wouldn't be too complicated.
>
> On the actual proposal of RLIMIT_GPUPRIO...
>
> The name would be problematic since we have generic hw accelerators (not just 
> GPUs) under the DRM subsystem. Perhaps RLIMIT_DRMPRIO would be better but I 
> think you will need to copy some more mailing lists and people on that one. 
> Because I can imagine one or two more fundamental questions this opens up, as 
> you have eluded in your cover letter as well.

So I don't want to get into the bikeshed, I think Tvrtko summarized
pretty well that this is a hard problem with lots of attempts (I think
some more from amd too). I think what we need are two pieces here
really:
- A solid summary of all the previous attempts from everyone in this
space of trying to manage gpu compute resources (all the various
cgroup attempts, sched priority), listening the pros/cons. There's
also the fdinfo stuff just for reporting gpu usage which blew up kinda
badly and didn't have much discussion among all the stakeholders.
- Everyone on cc who's doing new drivers using drm/sched (which I
think is everyone really, or using that currently. So that's like
etnaviv, lima, amd, intel with the new xe, probably new nouveau driver
too, amd ofc, panfrost, asahi. Please cc everyone.

Unless we do have some actual rough consens in this space across all
stakeholders I think all we'll achieve is just yet another rfc that
goes nowhere. Or maybe something like the minimal fdinfo stuff
(minimal I guess to avoid wider discussion) which then blew up because
it wasn't thought out well enough.

Adding at least some of the people who probably should be cc'ed on
this. Please add more.

Cheers, Daniel


>
> Regards,
>
> Tvrtko
>
> [1] 
> https://lore.kernel.org/dri-devel/20220407152806.3387898-1-tvrtko.ursu...@linux.intel.c

Re: [RESEND PATCH v4 03/21] staging: media: tegra-video: fix .vidioc_enum_fmt_vid_cap to return all formats

[Luca Ceresoli]

Hi Laurent,

On Wed, 5 Apr 2023 05:30:48 +0300
Laurent Pinchart  wrote:

> Hi Luca,
> 
> On Tue, Apr 04, 2023 at 04:12:51PM +0200, Luca Ceresoli wrote:
> > On Wed, 29 Mar 2023 13:16:22 +0200 Hans Verkuil wrote:
> >   
> > > Hi Luca,
> > > 
> > > I finally found the time to test this series. It looks OK, except for 
> > > this patch.  
> > 
> > Thank you very much for taking the time!
> >   
> > > The list of supported formats really has to be the intersection of what 
> > > the tegra
> > > supports and what the sensor supports.
> > > 
> > > Otherwise you would advertise pixelformats that cannot be used, and the 
> > > application
> > > would have no way of knowing that.  
> > 
> > As far as I understand, I think we should rather make this driver fully
> > behave as an MC-centric device. It is already using MC quite
> > successfully after all.
> > 
> > Do you think this is correct?  
> 
> Given the use cases for this driver, I agree.

Ok, thanks for the feedback. I will send a v5 with this change.

Best regards,
Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [RFC PATCH 00/10] Xe DRM scheduler and long running workload plans

[Daniel Vetter]

On Wed, Apr 05, 2023 at 09:41:23AM +0200, Christian König wrote:
> Am 04.04.23 um 15:37 schrieb Matthew Brost:
> > On Tue, Apr 04, 2023 at 11:13:28AM +0200, Christian König wrote:
> > > Hi,
> > > 
> > > Am 04.04.23 um 02:22 schrieb Matthew Brost:
> > > > Hello,
> > > > 
> > > > As a prerequisite to merging the new Intel Xe DRM driver [1] [2], we
> > > > have been asked to merge our common DRM scheduler patches first as well
> > > > as develop a common solution for long running workloads with the DRM
> > > > scheduler. This RFC series is our first attempt at doing this. We
> > > > welcome any and all feedback.
> > > > 
> > > > This can we thought of as 4 parts detailed below.
> > > > 
> > > > - DRM scheduler changes for 1 to 1 relationship between scheduler and
> > > > entity (patches 1-3)
> > > > 
> > > > In Xe all of the scheduling of jobs is done by a firmware scheduler (the
> > > > GuC) which is a new paradigm WRT to the DRM scheduler and presents
> > > > severals problems as the DRM was originally designed to schedule jobs on
> > > > hardware queues. The main problem being that DRM scheduler expects the
> > > > submission order of jobs to be the completion order of jobs even across
> > > > multiple entities. This assumption falls apart with a firmware scheduler
> > > > as a firmware scheduler has no concept of jobs and jobs can complete out
> > > > of order. A novel solution for was originally thought of by Faith during
> > > > the initial prototype of Xe, create a 1 to 1 relationship between 
> > > > scheduler
> > > > and entity. I believe the AGX driver [3] is using this approach and
> > > > Boris may use approach as well for the Mali driver [4].
> > > > 
> > > > To support a 1 to 1 relationship we move the main execution function
> > > > from a kthread to a work queue and add a new scheduling mode which
> > > > bypasses code in the DRM which isn't needed in a 1 to 1 relationship.
> > > > The new scheduling mode should unify all drivers usage with a 1 to 1
> > > > relationship and can be thought of as using scheduler as a dependency /
> > > > infligt job tracker rather than a true scheduler.
> > > > 
> > > > - Generic messaging interface for DRM scheduler
> > > > 
> > > > Idea is to be able to communicate to the submission backend with in band
> > > > (relative to main execution function) messages. Messages are backend
> > > > defined and flexable enough for any use case. In Xe we use these
> > > > messages to clean up entites, set properties for entites, and suspend /
> > > > resume execution of an entity [5]. I suspect other driver can leverage
> > > > this messaging concept too as it a convenient way to avoid races in the
> > > > backend.
> > > Oh, please absolutely *don't* do this.
> > > 
> > > This is basically the design which makes a bunch of stuff so horrible 
> > > broken
> > > on Windows.
> > > 
> > > I can explain it in more detail if necessary, but I strongly recommend to
> > > not go down this path.
> > > 
> > I'm afraid we are going to have to discuss this further. Let me explain
> > my reasoning, basically the idea is to have a single main entry point to
> > backend - the work queue. This avoids the need for lock between run_job
> > and any message that changes an entites state, also it really helps
> > during the reset flows (either TDR or GT reset) as we can call
> > drm_sched_run_wq_stop can ensure that nothing else is in the backend
> > changing an entity state. It all works out really nicely actually, our
> > GuC backend is incredibly stable (hasn't really had a bug pop in about a
> > year) and way simpler than what we did in the i915. I think the simplity
> > to largely due to this design of limiting the entry points.
> > 
> > I personally don't see how this a poor design, limiting entry points
> > absolutely makes sense to me, if it didn't why not just call cleanup_job
> > bypassing the main execution thread (now worker), this is the exact same
> > concept.
> 
> Well then I strongly suggest to read a few analyses on the failure of the
> message processing loop on Windows.
> 
> Have you ever wondered why classic Win32 applications sometimes seems to be
> stuck and don't do anything? This design pattern combine with timeouts to
> solve deadlocks is the reason for that.
> 
> The major problem with this approach is that analyzing tools like lockdep
> have a hard time grasping the dependencies.

wq is fully annotated and actually splats. Plain kthread doesn't, without
adding something like the dma_fence_signalling stuff.

But yeah if you block badly in the work items and stall the entire queue,
then things go sideways real bad. There's not really any tools we have in
the kernel to enforce this, since we still want to allow mutex and
sleeping and stuff like that.

> What you can do is to offload all your operations which are supposed to be
> run in the same thread as work items into a work queue. This is something
> lockdep understands and is able to scream out lout if someone messes 

Re: [RFC PATCH 00/10] Xe DRM scheduler and long running workload plans

[Daniel Vetter]

On Wed, Apr 05, 2023 at 09:30:11AM +0200, Christian König wrote:
> Am 04.04.23 um 20:08 schrieb Matthew Brost:
> > On Tue, Apr 04, 2023 at 12:02:03PM -0600, Zeng, Oak wrote:
> > > Hi Matt, Thomas,
> > > 
> > > Some very bold out of box thinking in this area:
> > > 
> > > 1. so you want to use drm scheduler and dma-fence for long running 
> > > workload. Why you want to do this in the first place? What is the 
> > > benefit? Drm scheduler is pretty much a software scheduler. Modern gpu 
> > > has scheduler built at fw/hw level, as you said below for intel this is 
> > > Guc. Can xe driver just directly submit job to Guc, bypassing drm 
> > > scheduler?
> > > 
> > If we did that now we have 2 paths for dependency track, flow controling
> > the ring, resets / error handling / backend submission implementations.
> > We don't want this.
> 
> Well exactly that's the point: Why?
> 
> As far as I can see that are two completely distinct use cases, so you
> absolutely do want two completely distinct implementations for this.
> 
> > > 2. using dma-fence for long run workload: I am well aware that page fault 
> > > (and the consequent memory allocation/lock acquiring to fix the fault) 
> > > can cause deadlock for a dma-fence wait. But I am not convinced that 
> > > dma-fence can't be used purely because the nature of the workload that it 
> > > runs very long (indefinite). I did a math: the dma_fence_wait_timeout 
> > > function's third param is the timeout which is a signed long type. If HZ 
> > > is 1000, this is about 23 days. If 23 days is not long enough, can we 
> > > just change the timeout parameter to signed 64 bits so it is much longer 
> > > than our life time...
> > > 
> > > So I mainly argue we can't use dma-fence for long-run workload is not 
> > > because the workload runs very long, rather because of the fact that we 
> > > use page fault for long-run workload. If we enable page fault for 
> > > short-run workload, we can't use dma-fence either. Page fault is the key 
> > > thing here.
> > > 
> > > Now since we use page fault which is *fundamentally* controversial with 
> > > dma-fence design, why now just introduce a independent concept such as 
> > > user-fence instead of extending existing dma-fence?
> > > 
> > > I like unified design. If drm scheduler, dma-fence can be extended to 
> > > work for everything, it is beautiful. But seems we have some fundamental 
> > > problem here.
> > > 
> > Thomas's patches turn a dma-fence into KMD sync point (e.g. we just use
> > the signal / CB infrastructure) and enforce we don't use use these
> > dma-fences from the scheduler in memory reclaim paths or export these to
> > user space or other drivers. Think of this mode as SW only fence.
> 
> Yeah and I truly think this is an really bad idea.
> 
> The signal/CB infrastructure in the dma_fence turned out to be the
> absolutely nightmare I initially predicted. Sorry to say that, but in this
> case the "I've told you so" is appropriate in my opinion.
> 
> If we need infrastructure for long running dependency tracking we should
> encapsulate that in a new framework and not try to mangle the existing code
> for something it was never intended for.

Concurring hard (already typed that up somewhere else). I'd go one step
further and ask whether the kernel really has to handle dependencies for
these long-running compute jobs. The entire design with userspace memory
fences assumes that this is userspace's job.

Also for drm_syncobj we've also pushed a lot of the dependency handling to
userspace, with submit threads in mesa. So if there is any blocking to be
done (running out of ring space), why can't we sort that out the same way?
Meaning:
1. superfast direct-to-hw submit path (using doorbells or whatever)
2. submit ioctl which only succeds if it doesn't have to do any userspace
memory fence waits, otherwise you get EWOULDBLOCK
3. userspace sorts out the mess in a submit thread if it gets an
EWOULDBLOCK, because fundamentally the kernel cannot guarantee a
bottomless queue. If userspace wants bottomless, they need to handle the
allocating and delaying imo

You can even make 3 entirely as-needed, which means for the usual
fast-path you'll never see the userspace thread created unless you do hit
an EWOULDBLOCK.

If we insist that the kernel handles the long-running dependencies fully
then all we end up doing is implementing step 3, but entirely in the
kernel instead of userspace. And in the kernel every bug gets you halfway
to a CVE, and I just don't think that makes much sense for something which
is the fallback of the fallback - once you run out of ring space you're
not going to have a great day not matter what.

I'd go as far and say if we want step 3 in the kernel someone needs to
supply the real-world (i.e. real application running real workloads, not
some microbenchmark) benchmark to proof it's actually worth the pain.
Otherwise on-demand userspace submit thread.
-Daniel

> 
> Christian.
> 
> > 
> > Matt
> > > Than

Re: [PATCH 0/3] Revert lima fdinfo patchset

[Daniel Vetter]

On Wed, Apr 05, 2023 at 08:50:06AM +0800, Qiang Yu wrote:
> Applied to drm-misc-next, sorry for the trouble.

No worries, I already complained to Lucas/etnaviv people that the sched
revert should have been at least posted/discussed on dri-devel. Imo this
isn't on you.
-Daniel

> 
> Regards,
> Qiang
> 
> On Wed, Apr 5, 2023 at 3:28 AM Daniel Vetter  wrote:
> >
> > On Tue, Apr 04, 2023 at 04:17:33PM +0100, Emil Velikov wrote:
> > > On Tue, 4 Apr 2023 at 08:13,  wrote:
> > > >
> > > > From: Qiang Yu 
> > > >
> > > > Upstream has reverted the dependant commit
> > > > df622729ddbf ("drm/scheduler: track GPU active time per entity""),
> > > > but this patchset has been pushed to drm-misc-next which still
> > > > has that commit. To avoid other branch build fail after merge
> > > > drm-misc-next, just revert the lima patchset on drm-misc-next too.
> > > >
> > >
> > > The bug/revert is unfortunate, although we better keep the build clean
> > > or Linus will go bananas ^_^
> > >
> > > Fwiw for the series:
> > > Acked-by: Emil Velikov 
> >
> > Can you (eitehr of you really) please push asap and make sure this doesn't
> > miss the last drm-misc-next pull (-rc6 is this w/e)? Otherwise we'll have
> > a bit a mess.
> > -Daniel
> >
> > >
> > > HTH
> > > -Emil
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 7/8] video/aperture: Only remove sysfb on the default vga pci device

[Daniel Vetter]

On Tue, Apr 04, 2023 at 01:59:33PM -0700, Aaron Plattner wrote:
> On 4/4/23 1:18 PM, Daniel Vetter wrote:
> > Instead of calling aperture_remove_conflicting_devices() to remove the
> > conflicting devices, just call to aperture_detach_devices() to detach
> > the device that matches the same PCI BAR / aperture range. Since the
> > former is just a wrapper of the latter plus a sysfb_disable() call,
> > and now that's done in this function but only for the primary devices.
> > 
> > This fixes a regression introduced by ee7a69aa38d8 ("fbdev: Disable
> > sysfb device registration when removing conflicting FBs"), where we
> > remove the sysfb when loading a driver for an unrelated pci device,
> > resulting in the user loosing their efifb console or similar.
> > 
> > Note that in practice this only is a problem with the nvidia blob,
> > because that's the only gpu driver people might install which does not
> > come with an fbdev driver of it's own. For everyone else the real gpu
> > driver will restore a working console.
> 
> It might be worth noting that this also affects devices that have no driver
> installed, or where the driver failed to initialize or was configured not to
> set a mode. E.g. I reproduced this problem on a laptop with i915.modeset=0
> and an NVIDIA driver that calls drm_fbdev_generic_setup. It would also
> reproduce on a system that sets modeset=0 (or has a GPU that's too new for
> its corresponding kernel driver) and that passes an NVIDIA GPU through to a
> VM using vfio-pci since that also calls
> aperture_remove_conflicting_pci_devices.
> 
> I agree that in practice this will mostly affect people with our driver
> until I get my changes to add drm_fbdev_generic_setup checked in. But these
> other cases don't seem all that unlikely to me.

Thomas Z. refactored the entire modeset=0 handling to be more consistent
across drivers, so I think in practice it'll again only happen with the
nvidia blob driver (unless you patch in the call to
drm_firmware_drivers_only()). Or if you dont use nomodeset or similar and
instead use a driver-specific module option, which isn't what howtos in
distros recommend.

I can add this to the commit message if you want?
-Daniel

> 
> -- Aaron
> 
> > Also note that in the referenced bug there's confusion that this same
> > bug also happens on amdgpu. But that was just another amdgpu specific
> > regression, which just happened to happen at roughly the same time and
> > with the same user-observable symptoms. That bug is fixed now, see
> > https://bugzilla.kernel.org/show_bug.cgi?id=216331#c15
> > 
> > Note that we should not have any such issues on non-pci multi-gpu
> > issues, because I could only find two such cases:
> > - SoC with some external panel over spi or similar. These panel
> >drivers do not use drm_aperture_remove_conflicting_framebuffers(),
> >so no problem.
> > - vga+mga, which is a direct console driver and entirely bypasses all
> >this.
> > 
> > For the above reasons the cc: stable is just notionally, this patch
> > will need a backport and that's up to nvidia if they care enough.
> > 
> > v2:
> > - Explain a bit better why other multi-gpu that aren't pci shouldn't
> >have any issues with making all this fully pci specific.
> > 
> > v3
> > - polish commit message (Javier)
> > 
> > Fixes: ee7a69aa38d8 ("fbdev: Disable sysfb device registration when 
> > removing conflicting FBs")
> > Tested-by: Aaron Plattner 
> > Reviewed-by: Javier Martinez Canillas 
> > References: https://bugzilla.kernel.org/show_bug.cgi?id=216303#c28
> > Signed-off-by: Daniel Vetter 
> > Cc: Aaron Plattner 
> > Cc: Javier Martinez Canillas 
> > Cc: Thomas Zimmermann 
> > Cc: Helge Deller 
> > Cc: Sam Ravnborg 
> > Cc: Alex Deucher 
> > Cc:  # v5.19+ (if someone else does the backport)
> > ---
> >   drivers/video/aperture.c | 7 ---
> >   1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c
> > index 8f1437339e49..2394c2d310f8 100644
> > --- a/drivers/video/aperture.c
> > +++ b/drivers/video/aperture.c
> > @@ -321,15 +321,16 @@ int aperture_remove_conflicting_pci_devices(struct 
> > pci_dev *pdev, const char *na
> > primary = pdev == vga_default_device();
> > +   if (primary)
> > +   sysfb_disable();
> > +
> > for (bar = 0; bar < PCI_STD_NUM_BARS; ++bar) {
> > if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM))
> > continue;
> > base = pci_resource_start(pdev, bar);
> > size = pci_resource_len(pdev, bar);
> > -   ret = aperture_remove_conflicting_devices(base, size, name);
> > -   if (ret)
> > -   return ret;
> > +   aperture_detach_devices(base, size);
> > }
> > if (primary) {

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] fbdev: Don't spam dmesg on bad userspace ioctl input

[Michel Dänzer]

On 4/4/23 14:36, Daniel Vetter wrote:
> There's a few reasons the kernel should not spam dmesg on bad
> userspace ioctl input:
> - at warning level it results in CI false positives
> - it allows userspace to drown dmesg output, potentially hiding real
>   issues.
> 
> None of the other generic EINVAL checks report in the
> FBIOPUT_VSCREENINFO ioctl do this, so it's also inconsistent.
> 
> I guess the intent of the patch which introduced this warning was that
> the drivers ->fb_check_var routine should fail in that case. Reality
> is that there's too many fbdev drivers and not enough people
> maintaining them by far, and so over the past few years we've simply
> handled all these validation gaps by tighning the checks in the core,
> because that's realistically really all that will ever happen.
> 
> Reported-by: syzbot+20dcf81733d43ddff...@syzkaller.appspotmail.com
> Link: 
> https://syzkaller.appspot.com/bug?id=c5faf983bfa4a607de530cd3bb00bf06cefc
> Fixes: 6c11df58fd1a ("fbmem: Check virtual screen sizes in fb_set_var()")
> Cc: Helge Deller 
> Cc: Geert Uytterhoeven 
> Cc: sta...@vger.kernel.org # v5.4+
> Cc: Daniel Vetter 
> Cc: Javier Martinez Canillas 
> Cc: Thomas Zimmermann 
> Signed-off-by: Daniel Vetter 
> ---
>  drivers/video/fbdev/core/fbmem.c | 4 
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/video/fbdev/core/fbmem.c 
> b/drivers/video/fbdev/core/fbmem.c
> index 875541ff185b..9757f4bcdf57 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -1021,10 +1021,6 @@ fb_set_var(struct fb_info *info, struct 
> fb_var_screeninfo *var)
>   /* verify that virtual resolution >= physical resolution */
>   if (var->xres_virtual < var->xres ||
>   var->yres_virtual < var->yres) {
> - pr_warn("WARNING: fbcon: Driver '%s' missed to adjust virtual 
> screen size (%ux%u vs. %ux%u)\n",
> - info->fix.id,
> - var->xres_virtual, var->yres_virtual,
> - var->xres, var->yres);
>   return -EINVAL;
>   }
>  

Make it pr_warn_once? 99.9...% of the benefit, without spam.


-- 
Earthling Michel Dänzer|  https://redhat.com
Libre software enthusiast  | Mesa and Xwayland developer

Re: [RESEND PATCH v4 03/21] staging: media: tegra-video: fix .vidioc_enum_fmt_vid_cap to return all formats

[Hans Verkuil]

On 05/04/2023 10:31, Luca Ceresoli wrote:
> Hi Laurent,
> 
> On Wed, 5 Apr 2023 05:30:48 +0300
> Laurent Pinchart  wrote:
> 
>> Hi Luca,
>>
>> On Tue, Apr 04, 2023 at 04:12:51PM +0200, Luca Ceresoli wrote:
>>> On Wed, 29 Mar 2023 13:16:22 +0200 Hans Verkuil wrote:
>>>   
 Hi Luca,

 I finally found the time to test this series. It looks OK, except for this 
 patch.  
>>>
>>> Thank you very much for taking the time!
>>>   
 The list of supported formats really has to be the intersection of what 
 the tegra
 supports and what the sensor supports.

 Otherwise you would advertise pixelformats that cannot be used, and the 
 application
 would have no way of knowing that.  
>>>
>>> As far as I understand, I think we should rather make this driver fully
>>> behave as an MC-centric device. It is already using MC quite
>>> successfully after all.
>>>
>>> Do you think this is correct?  
>>
>> Given the use cases for this driver, I agree.

I disagree.

This driver doesn't use the media controller for anything at the moment. The
/dev/mediaX device just shows the internal topology (i.e. connected sensors),
but otherwise it does nothing.

While it would be great if we could unlock the ISP on the Tegra, the reality
is that it is entirely closed source and can't be used in a linux driver, and
that's not going to change, sadly.

That leaves us with just a basic CSI capture driver. Rather than trying to
change this driver to a full MC device with no benefits, just drop this change
and get your code in.

Note that this driver will stay in staging since it still fails when I try to
capture from two sensors at the same time: syncpoint errors start appearing
in that case. I think there are locking issues. I think I have someone to take
a look at that, but first I want your series to get merged.

In the very unlikely event that the ISP can be implemented in a linux driver,
it will probably become a new driver.

Regards,

Hans

> 
> Ok, thanks for the feedback. I will send a v5 with this change.
> 
> Best regards,
> Luca
> 

Re: [RFC PATCH 00/10] Xe DRM scheduler and long running workload plans

[Christian König]

Am 05.04.23 um 10:34 schrieb Daniel Vetter:

On Wed, Apr 05, 2023 at 09:41:23AM +0200, Christian König wrote:

Am 04.04.23 um 15:37 schrieb Matthew Brost:

On Tue, Apr 04, 2023 at 11:13:28AM +0200, Christian König wrote:

Hi,

Am 04.04.23 um 02:22 schrieb Matthew Brost:

Hello,

As a prerequisite to merging the new Intel Xe DRM driver [1] [2], we
have been asked to merge our common DRM scheduler patches first as well
as develop a common solution for long running workloads with the DRM
scheduler. This RFC series is our first attempt at doing this. We
welcome any and all feedback.

This can we thought of as 4 parts detailed below.

- DRM scheduler changes for 1 to 1 relationship between scheduler and
entity (patches 1-3)

In Xe all of the scheduling of jobs is done by a firmware scheduler (the
GuC) which is a new paradigm WRT to the DRM scheduler and presents
severals problems as the DRM was originally designed to schedule jobs on
hardware queues. The main problem being that DRM scheduler expects the
submission order of jobs to be the completion order of jobs even across
multiple entities. This assumption falls apart with a firmware scheduler
as a firmware scheduler has no concept of jobs and jobs can complete out
of order. A novel solution for was originally thought of by Faith during
the initial prototype of Xe, create a 1 to 1 relationship between scheduler
and entity. I believe the AGX driver [3] is using this approach and
Boris may use approach as well for the Mali driver [4].

To support a 1 to 1 relationship we move the main execution function
from a kthread to a work queue and add a new scheduling mode which
bypasses code in the DRM which isn't needed in a 1 to 1 relationship.
The new scheduling mode should unify all drivers usage with a 1 to 1
relationship and can be thought of as using scheduler as a dependency /
infligt job tracker rather than a true scheduler.

- Generic messaging interface for DRM scheduler

Idea is to be able to communicate to the submission backend with in band
(relative to main execution function) messages. Messages are backend
defined and flexable enough for any use case. In Xe we use these
messages to clean up entites, set properties for entites, and suspend /
resume execution of an entity [5]. I suspect other driver can leverage
this messaging concept too as it a convenient way to avoid races in the
backend.

Oh, please absolutely *don't* do this.

This is basically the design which makes a bunch of stuff so horrible broken
on Windows.

I can explain it in more detail if necessary, but I strongly recommend to
not go down this path.


I'm afraid we are going to have to discuss this further. Let me explain
my reasoning, basically the idea is to have a single main entry point to
backend - the work queue. This avoids the need for lock between run_job
and any message that changes an entites state, also it really helps
during the reset flows (either TDR or GT reset) as we can call
drm_sched_run_wq_stop can ensure that nothing else is in the backend
changing an entity state. It all works out really nicely actually, our
GuC backend is incredibly stable (hasn't really had a bug pop in about a
year) and way simpler than what we did in the i915. I think the simplity
to largely due to this design of limiting the entry points.

I personally don't see how this a poor design, limiting entry points
absolutely makes sense to me, if it didn't why not just call cleanup_job
bypassing the main execution thread (now worker), this is the exact same
concept.

Well then I strongly suggest to read a few analyses on the failure of the
message processing loop on Windows.

Have you ever wondered why classic Win32 applications sometimes seems to be
stuck and don't do anything? This design pattern combine with timeouts to
solve deadlocks is the reason for that.

The major problem with this approach is that analyzing tools like lockdep
have a hard time grasping the dependencies.

wq is fully annotated and actually splats. Plain kthread doesn't, without
adding something like the dma_fence_signalling stuff.

But yeah if you block badly in the work items and stall the entire queue,
then things go sideways real bad. There's not really any tools we have in
the kernel to enforce this, since we still want to allow mutex and
sleeping and stuff like that.


What you can do is to offload all your operations which are supposed to be
run in the same thread as work items into a work queue. This is something
lockdep understands and is able to scream out lout if someone messes up the
deadlock dependencies.

I thought that's the plan here?


At least from my impression that didn't looked like what was implemented 
here.



  Or at least what I thought the plan was,
and why I really think we need a per engine worqqueue to make it work well
(and also why I suggested the refactoring to split up drm_scheduler into
the driver api struct, which stays per-engine, and the internal backend
which would be per drm_sched_entity fo

Re: [PATCH 1/1] drm/bridge: ti-sn65dsi83: use dev_err_probe if host attach failed

[rfoss]

From: Robert Foss 

On Wed, 5 Apr 2023 09:52:23 +0200, Alexander Stein wrote:
> There might be cases where the host attach is deferred, use dev_err_probe
> to add more detailed information to /sys/kernel/debug/devices_deferred.
> 
> 

Applied, thanks!

[1/1] drm/bridge: ti-sn65dsi83: use dev_err_probe if host attach failed
  https://cgit.freedesktop.org/drm/drm-misc/commit/?id=ce7498acaa88



Rob

Re: [PATCH 1/8] drm/gma500: Use drm_aperture_remove_conflicting_pci_framebuffers

[Daniel Vetter]

On Wed, Apr 05, 2023 at 10:07:54AM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 05.04.23 um 09:49 schrieb Thomas Zimmermann:
> > Hi
> > 
> > Am 04.04.23 um 22:18 schrieb Daniel Vetter:
> > > This one nukes all framebuffers, which is a bit much. In reality
> > > gma500 is igpu and never shipped with anything discrete, so there should
> > > not be any difference.
> > > 
> > > v2: Unfortunately the framebuffer sits outside of the pci bars for
> > > gma500, and so only using the pci helpers won't be enough. Otoh if we
> > > only use non-pci helper, then we don't get the vga handling, and
> > > subsequent refactoring to untangle these special cases won't work.
> > > 
> > > It's not pretty, but the simplest fix (since gma500 really is the only
> > > quirky pci driver like this we have) is to just have both calls.
> > > 
> > > Signed-off-by: Daniel Vetter 
> > > Cc: Patrik Jakobsson 
> > > Cc: Thomas Zimmermann 
> > > Cc: Javier Martinez Canillas 
> > > ---
> > >   drivers/gpu/drm/gma500/psb_drv.c | 9 +++--
> > >   1 file changed, 7 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/gma500/psb_drv.c
> > > b/drivers/gpu/drm/gma500/psb_drv.c
> > > index 2ce96b1b9c74..f1e0eed8fea4 100644
> > > --- a/drivers/gpu/drm/gma500/psb_drv.c
> > > +++ b/drivers/gpu/drm/gma500/psb_drv.c
> > > @@ -422,12 +422,17 @@ static int psb_pci_probe(struct pci_dev *pdev,
> > > const struct pci_device_id *ent)
> > >   /*
> > >    * We cannot yet easily find the framebuffer's location in
> > > memory. So
> > > - * remove all framebuffers here.
> > > + * remove all framebuffers here. Note that we still want the
> > > pci special
> > > + * handling to kick out vgacon.
> > >    *
> > >    * TODO: Refactor psb_driver_load() to map vdc_reg earlier. Then we
> > >    *   might be able to read the framebuffer range from the
> > > device.
> > >    */
> > > -    ret = drm_aperture_remove_framebuffers(true, &driver);
> > > +    ret = drm_aperture_remove_framebuffers(false, &driver);
> > > +    if (ret)
> > > +    return ret;
> > > +
> > > +    ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev,
> > > &driver);
> > 
> > This simply isn't it. If you have to work around your own API, it's time
> > to rethink the API.
> 
> Here's a proposal:
> 
>  1) As you're changing aperture_remove_conflicting_devices() anyway, rename
> it to aperture_remove_conflicting_devices_at(), so it's clear that it takes
> a memory range.
> 
>  2) Introduce aperture_remove_conflicting_pci_devices_at(), which takes a
> PCI device and a memory range. It should do the is_primary and vgacon stuff,
> but kick out the range.
> 
>  3) Call aperture_remove_conflicting_pci_devices_at() from gma500 with the
> full range. Even if we can restructure gma500 to detect the firmware
> framebuffer from its registers (there's this TODO item), we'd still need
> aperture_remove_conflicting_pci_devices_at() to do something useful with it.
> 
>  4) With that, aperture_remove_conflicting_devices_at() can drop the primary
> argument.
> 
> Of course, the DRM-related interface should be adapted as well. There's a
> bit of overlap in the implementation of both PCI aperture helpers, but the
> overall interface is clear.

This essentially just gives us a helper which does the above two
open-coded steps but all wrapped up. For gma500 only. Also I really don't
think I'm working around the api here, it's gma500 which is special:

- Normal pci devices all have their fw framebuffer within the memory bars,
  never outside. So for those the pci version is the right interface.

- If the framebuffer is outside of the pci bar then it's just not really a
  pci vga device anymore, but looks a lot more like a SoC design.

gma500 is somehow both at the same time, so it gets two calls. And having
both calls explicitly I think is better, because it highlights the dual
nature of gma500 of being both a pci vga devices and a SoC embedded
device. Trying to make a wrapper to hide this dual nature just so we have
a single call still seems worse to me. Aside from it's just boilerplate
for no gain.

Frankly at that point I think it would be clearer to call the gma500
function remove_conflicting_gma500 or something like that. Or at least
remove_conflicting_pci_and_separate_range_at.

This is imo similar to the hypothetical case of a SoC chip which also
happens to decode legacy vga, without being a pci device. We could add a
new interface function which just nukes the vga stuff (without the pci
device tie-in, maybe with some code sharing internally in aperture.c), and
then that driver does 2 calls: 1. nuke aperture range 2. nuke vga stuff.
And sure if you have a lot of those maybe you could make a helper to safe
a few lines of code, but semantically it's still two different things
your're removing.

Or another case: A pci device with 2 subfunctions, each a gpu device. This
happened with dual-head gpus 20 years ago because windows 2000 insisted
that each c

[PULL] drm-intel-fixes

[Jani Nikula]

Hi Dave & Daniel -

drm-intel-fixes-2023-04-05:
drm/i915 fixes for v6.3-rc6:
- Fix DP MST DSC M/N calculation to use compressed bpp
- Fix racy use-after-free in perf ioctl
- Fix context runtime accounting
- Fix handling of GT reset during HuC loading
- Fix use of unsigned vm_fault_t for error values

BR,
Jani.

The following changes since commit 7e364e56293bb98cae1b55fd835f5991c4e96e7d:

  Linux 6.3-rc5 (2023-04-02 14:29:29 -0700)

are available in the Git repository at:

  git://anongit.freedesktop.org/drm/drm-intel tags/drm-intel-fixes-2023-04-05

for you to fetch changes up to dc3421560a67361442f33ec962fc6dd48895a0df:

  drm/i915: Fix context runtime accounting (2023-04-03 11:37:00 +0300)


drm/i915 fixes for v6.3-rc6:
- Fix DP MST DSC M/N calculation to use compressed bpp
- Fix racy use-after-free in perf ioctl
- Fix context runtime accounting
- Fix handling of GT reset during HuC loading
- Fix use of unsigned vm_fault_t for error values


Daniele Ceraolo Spurio (1):
  drm/i915/huc: Cancel HuC delayed load timer on reset.

Matthew Auld (1):
  drm/i915/ttm: fix sparse warning

Min Li (1):
  drm/i915: fix race condition UAF in i915_perf_add_config_ioctl

Stanislav Lisovskiy (1):
  drm/i915: Use compressed bpp when calculating m/n value for DP MST DSC

Tvrtko Ursulin (1):
  drm/i915: Fix context runtime accounting

 drivers/gpu/drm/i915/display/intel_dp_mst.c  |  2 +-
 drivers/gpu/drm/i915/gem/i915_gem_ttm.c  |  5 +++--
 drivers/gpu/drm/i915/gt/intel_execlists_submission.c | 12 ++--
 drivers/gpu/drm/i915/gt/uc/intel_huc.c   |  7 +++
 drivers/gpu/drm/i915/gt/uc/intel_huc.h   |  7 +--
 drivers/gpu/drm/i915/i915_perf.c |  6 +++---
 6 files changed, 25 insertions(+), 14 deletions(-)

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH v2 1/2] drm: bridge: ldb: add missing \n in dev_warn() string

[rfoss]

From: Robert Foss 

On Wed, 5 Apr 2023 10:10:56 +0200, Luca Ceresoli wrote:
> dev_warn() and similar require a training \n.
> 
> 

Applied, thanks!

[1/2] drm: bridge: ldb: add missing \n in dev_warn() string
  https://cgit.freedesktop.org/drm/drm-misc/commit/?id=8cc0b604f234
[2/2] drm: bridge: ldb: add support for using channel 1 only
  https://cgit.freedesktop.org/drm/drm-misc/commit/?id=e09220f42b5c



Rob

Re: [RFC PATCH 00/10] Xe DRM scheduler and long running workload plans

[Daniel Vetter]

On Wed, Apr 05, 2023 at 10:53:26AM +0200, Christian König wrote:
> Am 05.04.23 um 10:34 schrieb Daniel Vetter:
> > On Wed, Apr 05, 2023 at 09:41:23AM +0200, Christian König wrote:
> > > Am 04.04.23 um 15:37 schrieb Matthew Brost:
> > > > On Tue, Apr 04, 2023 at 11:13:28AM +0200, Christian König wrote:
> > > > > Hi,
> > > > > 
> > > > > Am 04.04.23 um 02:22 schrieb Matthew Brost:
> > > > > > Hello,
> > > > > > 
> > > > > > As a prerequisite to merging the new Intel Xe DRM driver [1] [2], we
> > > > > > have been asked to merge our common DRM scheduler patches first as 
> > > > > > well
> > > > > > as develop a common solution for long running workloads with the DRM
> > > > > > scheduler. This RFC series is our first attempt at doing this. We
> > > > > > welcome any and all feedback.
> > > > > > 
> > > > > > This can we thought of as 4 parts detailed below.
> > > > > > 
> > > > > > - DRM scheduler changes for 1 to 1 relationship between scheduler 
> > > > > > and
> > > > > > entity (patches 1-3)
> > > > > > 
> > > > > > In Xe all of the scheduling of jobs is done by a firmware scheduler 
> > > > > > (the
> > > > > > GuC) which is a new paradigm WRT to the DRM scheduler and presents
> > > > > > severals problems as the DRM was originally designed to schedule 
> > > > > > jobs on
> > > > > > hardware queues. The main problem being that DRM scheduler expects 
> > > > > > the
> > > > > > submission order of jobs to be the completion order of jobs even 
> > > > > > across
> > > > > > multiple entities. This assumption falls apart with a firmware 
> > > > > > scheduler
> > > > > > as a firmware scheduler has no concept of jobs and jobs can 
> > > > > > complete out
> > > > > > of order. A novel solution for was originally thought of by Faith 
> > > > > > during
> > > > > > the initial prototype of Xe, create a 1 to 1 relationship between 
> > > > > > scheduler
> > > > > > and entity. I believe the AGX driver [3] is using this approach and
> > > > > > Boris may use approach as well for the Mali driver [4].
> > > > > > 
> > > > > > To support a 1 to 1 relationship we move the main execution function
> > > > > > from a kthread to a work queue and add a new scheduling mode which
> > > > > > bypasses code in the DRM which isn't needed in a 1 to 1 
> > > > > > relationship.
> > > > > > The new scheduling mode should unify all drivers usage with a 1 to 1
> > > > > > relationship and can be thought of as using scheduler as a 
> > > > > > dependency /
> > > > > > infligt job tracker rather than a true scheduler.
> > > > > > 
> > > > > > - Generic messaging interface for DRM scheduler
> > > > > > 
> > > > > > Idea is to be able to communicate to the submission backend with in 
> > > > > > band
> > > > > > (relative to main execution function) messages. Messages are backend
> > > > > > defined and flexable enough for any use case. In Xe we use these
> > > > > > messages to clean up entites, set properties for entites, and 
> > > > > > suspend /
> > > > > > resume execution of an entity [5]. I suspect other driver can 
> > > > > > leverage
> > > > > > this messaging concept too as it a convenient way to avoid races in 
> > > > > > the
> > > > > > backend.
> > > > > Oh, please absolutely *don't* do this.
> > > > > 
> > > > > This is basically the design which makes a bunch of stuff so horrible 
> > > > > broken
> > > > > on Windows.
> > > > > 
> > > > > I can explain it in more detail if necessary, but I strongly 
> > > > > recommend to
> > > > > not go down this path.
> > > > > 
> > > > I'm afraid we are going to have to discuss this further. Let me explain
> > > > my reasoning, basically the idea is to have a single main entry point to
> > > > backend - the work queue. This avoids the need for lock between run_job
> > > > and any message that changes an entites state, also it really helps
> > > > during the reset flows (either TDR or GT reset) as we can call
> > > > drm_sched_run_wq_stop can ensure that nothing else is in the backend
> > > > changing an entity state. It all works out really nicely actually, our
> > > > GuC backend is incredibly stable (hasn't really had a bug pop in about a
> > > > year) and way simpler than what we did in the i915. I think the simplity
> > > > to largely due to this design of limiting the entry points.
> > > > 
> > > > I personally don't see how this a poor design, limiting entry points
> > > > absolutely makes sense to me, if it didn't why not just call cleanup_job
> > > > bypassing the main execution thread (now worker), this is the exact same
> > > > concept.
> > > Well then I strongly suggest to read a few analyses on the failure of the
> > > message processing loop on Windows.
> > > 
> > > Have you ever wondered why classic Win32 applications sometimes seems to 
> > > be
> > > stuck and don't do anything? This design pattern combine with timeouts to
> > > solve deadlocks is the reason for that.
> > > 
> > > The major problem with this approach is that analyzing tools like lockdep
> > >

Re: [PATCH 1/8] drm/gma500: Use drm_aperture_remove_conflicting_pci_framebuffers

[Daniel Vetter]

On Wed, Apr 05, 2023 at 10:19:55AM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 04.04.23 um 22:18 schrieb Daniel Vetter:
> > This one nukes all framebuffers, which is a bit much. In reality
> > gma500 is igpu and never shipped with anything discrete, so there should
> > not be any difference.
> 
> I do own an Intel DN2800MT board with gma500 hardware. [1] It has a PCIe x1
> slot. I never tried, but in principle, there could be another graphics card
> in the system. The linked spec say 'Discrete: None'. I don't know what that
> means exactly.

Well even if that's the case, I'm not making the situation worse. Because
the old code also nuked everything. The new one at least only nukes the
vga if gma500 is decoding that, and not the the discrete card. In practice
it won't help, because I don't think you'll boot this in legacy vga mode
with vga16fb :-)
-Daniel

> 
> Best regards
> Thomas
> 
> [1] 
> https://ark.intel.com/content/www/us/en/ark/products/56455/intel-desktop-board-dn2800mt.html
> 
> > 
> > v2: Unfortunately the framebuffer sits outside of the pci bars for
> > gma500, and so only using the pci helpers won't be enough. Otoh if we
> > only use non-pci helper, then we don't get the vga handling, and
> > subsequent refactoring to untangle these special cases won't work.
> > 
> > It's not pretty, but the simplest fix (since gma500 really is the only
> > quirky pci driver like this we have) is to just have both calls.
> > 
> > Signed-off-by: Daniel Vetter 
> > Cc: Patrik Jakobsson 
> > Cc: Thomas Zimmermann 
> > Cc: Javier Martinez Canillas 
> > ---
> >   drivers/gpu/drm/gma500/psb_drv.c | 9 +++--
> >   1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/gma500/psb_drv.c 
> > b/drivers/gpu/drm/gma500/psb_drv.c
> > index 2ce96b1b9c74..f1e0eed8fea4 100644
> > --- a/drivers/gpu/drm/gma500/psb_drv.c
> > +++ b/drivers/gpu/drm/gma500/psb_drv.c
> > @@ -422,12 +422,17 @@ static int psb_pci_probe(struct pci_dev *pdev, const 
> > struct pci_device_id *ent)
> > /*
> >  * We cannot yet easily find the framebuffer's location in memory. So
> > -* remove all framebuffers here.
> > +* remove all framebuffers here. Note that we still want the pci special
> > +* handling to kick out vgacon.
> >  *
> >  * TODO: Refactor psb_driver_load() to map vdc_reg earlier. Then we
> >  *   might be able to read the framebuffer range from the device.
> >  */
> > -   ret = drm_aperture_remove_framebuffers(true, &driver);
> > +   ret = drm_aperture_remove_framebuffers(false, &driver);
> > +   if (ret)
> > +   return ret;
> > +
> > +   ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &driver);
> > if (ret)
> > return ret;
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Ivo Totev




-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

fdinfo blew up? (Was: [RFC PATCH 0/4] uapi, drm: Add and implement RLIMIT_GPUPRIO)

[Tvrtko Ursulin]

On 05/04/2023 09:28, Daniel Vetter wrote:

On Tue, 4 Apr 2023 at 12:45, Tvrtko Ursulin
 wrote:



Hi,

On 03/04/2023 20:40, Joshua Ashton wrote:

Hello all!

I would like to propose a new API for allowing processes to control
the priority of GPU queues similar to RLIMIT_NICE/RLIMIT_RTPRIO.

The main reason for this is for compositors such as Gamescope and
SteamVR vrcompositor to be able to create realtime async compute
queues on AMD without the need of CAP_SYS_NICE.

The current situation is bad for a few reasons, one being that in order
to setcap the executable, typically one must run as root which involves
a pretty high privelage escalation in order to achieve one
small feat, a realtime async compute queue queue for VR or a compositor.
The executable cannot be setcap'ed inside a
container nor can the setcap'ed executable be run in a container with
NO_NEW_PRIVS.

I go into more detail in the description in
`uapi: Add RLIMIT_GPUPRIO`.

My initial proposal here is to add a new RLIMIT, `RLIMIT_GPUPRIO`,
which seems to make most initial sense to me to solve the problem.

I am definitely not set that this is the best formulation however
or if this should be linked to DRM (in terms of it's scheduler
priority enum/definitions) in any way and and would really like other
people's opinions across the stack on this.

Once initial concern is that potentially this RLIMIT could out-live
the lifespan of DRM. It sounds crazy saying it right now, something
that definitely popped into my mind when touching `resource.h`. :-)

Anyway, please let me know what you think!
Definitely open to any feedback and advice you may have. :D


Interesting! I tried to solved the similar problem two times in the past 
already.

First time I was proposing to tie nice to DRM scheduling priority [1] - if the 
latter has been left at default - drawing the analogy with the nice+ionice 
handling. That was rejected and I was nudged towards the cgroups route.

So with that second attempt I implemented a hierarchical opaque drm.priority 
cgroup controller [2]. I think it would allow you to solve your use case too by 
placing your compositor in a cgroup with an elevated priority level.

Implementation wise in my proposal it was left to individual drivers to "meld" 
the opaque cgroup drm.priority with the driver specific priority concept.

That too wasn't too popular with the feedback (AFAIR) that the priority is a 
too subsystem specific concept.

Finally I was left with a weight based drm cgroup controller, exactly following 
the controls of the CPU and IO ones, but with much looser runtime guarantees. 
[3]

I don't think this last one works for your use case, at least not at the current state 
for drm scheduling capability, where the implementation is a "bit" too reactive 
for realtime.

Depending on how the discussion around your rlimit proposal goes, perhaps one 
alternative could be to go the cgroup route and add an attribute like 
drm.realtime. That perhaps sounds abstract and generic enough to be passable. 
Built as a simplification of [2] it wouldn't be too complicated.

On the actual proposal of RLIMIT_GPUPRIO...

The name would be problematic since we have generic hw accelerators (not just 
GPUs) under the DRM subsystem. Perhaps RLIMIT_DRMPRIO would be better but I 
think you will need to copy some more mailing lists and people on that one. 
Because I can imagine one or two more fundamental questions this opens up, as 
you have eluded in your cover letter as well.


So I don't want to get into the bikeshed, I think Tvrtko summarized
pretty well that this is a hard problem with lots of attempts (I think
some more from amd too). I think what we need are two pieces here
really:
- A solid summary of all the previous attempts from everyone in this
space of trying to manage gpu compute resources (all the various
cgroup attempts, sched priority), listening the pros/cons. There's
also the fdinfo stuff just for reporting gpu usage which blew up kinda
badly and didn't have much discussion among all the stakeholders.
- Everyone on cc who's doing new drivers using drm/sched (which I
think is everyone really, or using that currently. So that's like
etnaviv, lima, amd, intel with the new xe, probably new nouveau driver
too, amd ofc, panfrost, asahi. Please cc everyone.

Unless we do have some actual rough consens in this space across all
stakeholders I think all we'll achieve is just yet another rfc that
goes nowhere. Or maybe something like the minimal fdinfo stuff
(minimal I guess to avoid wider discussion) which then blew up because
it wasn't thought out well enough.


On the particular point how fdinfo allegedly blew up - are you referring 
to client usage stats? If so this would be the first time I hear about 
any problems in that space. Which would be "a bit" surprising given it's 
the thing I drove standardisation of. All I heard were positive 
comments. Both "works for us" from driver implementors and positives 
from the users.


Regards,

Tvr

Re: fdinfo blew up? (Was: [RFC PATCH 0/4] uapi, drm: Add and implement RLIMIT_GPUPRIO)

[Daniel Vetter]

On Wed, 5 Apr 2023 at 11:11, Tvrtko Ursulin
 wrote:
>
>
> On 05/04/2023 09:28, Daniel Vetter wrote:
> > On Tue, 4 Apr 2023 at 12:45, Tvrtko Ursulin
> >  wrote:
> >>
> >>
> >> Hi,
> >>
> >> On 03/04/2023 20:40, Joshua Ashton wrote:
> >>> Hello all!
> >>>
> >>> I would like to propose a new API for allowing processes to control
> >>> the priority of GPU queues similar to RLIMIT_NICE/RLIMIT_RTPRIO.
> >>>
> >>> The main reason for this is for compositors such as Gamescope and
> >>> SteamVR vrcompositor to be able to create realtime async compute
> >>> queues on AMD without the need of CAP_SYS_NICE.
> >>>
> >>> The current situation is bad for a few reasons, one being that in order
> >>> to setcap the executable, typically one must run as root which involves
> >>> a pretty high privelage escalation in order to achieve one
> >>> small feat, a realtime async compute queue queue for VR or a compositor.
> >>> The executable cannot be setcap'ed inside a
> >>> container nor can the setcap'ed executable be run in a container with
> >>> NO_NEW_PRIVS.
> >>>
> >>> I go into more detail in the description in
> >>> `uapi: Add RLIMIT_GPUPRIO`.
> >>>
> >>> My initial proposal here is to add a new RLIMIT, `RLIMIT_GPUPRIO`,
> >>> which seems to make most initial sense to me to solve the problem.
> >>>
> >>> I am definitely not set that this is the best formulation however
> >>> or if this should be linked to DRM (in terms of it's scheduler
> >>> priority enum/definitions) in any way and and would really like other
> >>> people's opinions across the stack on this.
> >>>
> >>> Once initial concern is that potentially this RLIMIT could out-live
> >>> the lifespan of DRM. It sounds crazy saying it right now, something
> >>> that definitely popped into my mind when touching `resource.h`. :-)
> >>>
> >>> Anyway, please let me know what you think!
> >>> Definitely open to any feedback and advice you may have. :D
> >>
> >> Interesting! I tried to solved the similar problem two times in the past 
> >> already.
> >>
> >> First time I was proposing to tie nice to DRM scheduling priority [1] - if 
> >> the latter has been left at default - drawing the analogy with the 
> >> nice+ionice handling. That was rejected and I was nudged towards the 
> >> cgroups route.
> >>
> >> So with that second attempt I implemented a hierarchical opaque 
> >> drm.priority cgroup controller [2]. I think it would allow you to solve 
> >> your use case too by placing your compositor in a cgroup with an elevated 
> >> priority level.
> >>
> >> Implementation wise in my proposal it was left to individual drivers to 
> >> "meld" the opaque cgroup drm.priority with the driver specific priority 
> >> concept.
> >>
> >> That too wasn't too popular with the feedback (AFAIR) that the priority is 
> >> a too subsystem specific concept.
> >>
> >> Finally I was left with a weight based drm cgroup controller, exactly 
> >> following the controls of the CPU and IO ones, but with much looser 
> >> runtime guarantees. [3]
> >>
> >> I don't think this last one works for your use case, at least not at the 
> >> current state for drm scheduling capability, where the implementation is a 
> >> "bit" too reactive for realtime.
> >>
> >> Depending on how the discussion around your rlimit proposal goes, perhaps 
> >> one alternative could be to go the cgroup route and add an attribute like 
> >> drm.realtime. That perhaps sounds abstract and generic enough to be 
> >> passable. Built as a simplification of [2] it wouldn't be too complicated.
> >>
> >> On the actual proposal of RLIMIT_GPUPRIO...
> >>
> >> The name would be problematic since we have generic hw accelerators (not 
> >> just GPUs) under the DRM subsystem. Perhaps RLIMIT_DRMPRIO would be better 
> >> but I think you will need to copy some more mailing lists and people on 
> >> that one. Because I can imagine one or two more fundamental questions this 
> >> opens up, as you have eluded in your cover letter as well.
> >
> > So I don't want to get into the bikeshed, I think Tvrtko summarized
> > pretty well that this is a hard problem with lots of attempts (I think
> > some more from amd too). I think what we need are two pieces here
> > really:
> > - A solid summary of all the previous attempts from everyone in this
> > space of trying to manage gpu compute resources (all the various
> > cgroup attempts, sched priority), listening the pros/cons. There's
> > also the fdinfo stuff just for reporting gpu usage which blew up kinda
> > badly and didn't have much discussion among all the stakeholders.
> > - Everyone on cc who's doing new drivers using drm/sched (which I
> > think is everyone really, or using that currently. So that's like
> > etnaviv, lima, amd, intel with the new xe, probably new nouveau driver
> > too, amd ofc, panfrost, asahi. Please cc everyone.
> >
> > Unless we do have some actual rough consens in this space across all
> > stakeholders I think all we'll achieve is just yet another rfc that
> > goes nowhere.

Re: [PATCH 3/8] drm/aperture: Remove primary argument

[Thierry Reding]

On Tue, Apr 04, 2023 at 10:18:37PM +0200, Daniel Vetter wrote:
> Only really pci devices have a business setting this - it's for
> figuring out whether the legacy vga stuff should be nuked too. And
> with the preceeding two patches those are all using the pci version of
> this.
> 
> Which means for all other callers primary == false and we can remove
> it now.
> 
> v2:
> - Reorder to avoid compile fail (Thomas)
> - Include gma500, which retained it's called to the non-pci version.
> 
> Signed-off-by: Daniel Vetter 
> Cc: Thomas Zimmermann 
> Cc: Javier Martinez Canillas 
> Cc: Maarten Lankhorst 
> Cc: Maxime Ripard 
> Cc: Deepak Rawat 
> Cc: Neil Armstrong 
> Cc: Kevin Hilman 
> Cc: Jerome Brunet 
> Cc: Martin Blumenstingl 
> Cc: Thierry Reding 
> Cc: Jonathan Hunter 
> Cc: Emma Anholt 
> Cc: Helge Deller 
> Cc: David Airlie 
> Cc: Daniel Vetter 
> Cc: linux-hyp...@vger.kernel.org
> Cc: linux-amlo...@lists.infradead.org
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: linux-te...@vger.kernel.org
> Cc: linux-fb...@vger.kernel.org
> ---
>  drivers/gpu/drm/arm/hdlcd_drv.c |  2 +-
>  drivers/gpu/drm/armada/armada_drv.c |  2 +-
>  drivers/gpu/drm/drm_aperture.c  | 11 +++
>  drivers/gpu/drm/gma500/psb_drv.c|  2 +-
>  drivers/gpu/drm/hyperv/hyperv_drm_drv.c |  1 -
>  drivers/gpu/drm/meson/meson_drv.c   |  2 +-
>  drivers/gpu/drm/msm/msm_fbdev.c |  2 +-
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c |  2 +-
>  drivers/gpu/drm/stm/drv.c   |  2 +-
>  drivers/gpu/drm/sun4i/sun4i_drv.c   |  2 +-
>  drivers/gpu/drm/tegra/drm.c |  2 +-
>  drivers/gpu/drm/vc4/vc4_drv.c   |  2 +-
>  include/drm/drm_aperture.h  |  7 +++
>  13 files changed, 16 insertions(+), 23 deletions(-)

Acked-by: Thierry Reding 


signature.asc
Description: PGP signature

Re: [PATCH 1/8] drm/gma500: Use drm_aperture_remove_conflicting_pci_framebuffers

[Thomas Zimmermann]

Hi

Am 05.04.23 um 10:59 schrieb Daniel Vetter:

On Wed, Apr 05, 2023 at 10:07:54AM +0200, Thomas Zimmermann wrote:

Hi

Am 05.04.23 um 09:49 schrieb Thomas Zimmermann:

Hi

Am 04.04.23 um 22:18 schrieb Daniel Vetter:

This one nukes all framebuffers, which is a bit much. In reality
gma500 is igpu and never shipped with anything discrete, so there should
not be any difference.

v2: Unfortunately the framebuffer sits outside of the pci bars for
gma500, and so only using the pci helpers won't be enough. Otoh if we
only use non-pci helper, then we don't get the vga handling, and
subsequent refactoring to untangle these special cases won't work.

It's not pretty, but the simplest fix (since gma500 really is the only
quirky pci driver like this we have) is to just have both calls.

Signed-off-by: Daniel Vetter 
Cc: Patrik Jakobsson 
Cc: Thomas Zimmermann 
Cc: Javier Martinez Canillas 
---
   drivers/gpu/drm/gma500/psb_drv.c | 9 +++--
   1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/gma500/psb_drv.c
b/drivers/gpu/drm/gma500/psb_drv.c
index 2ce96b1b9c74..f1e0eed8fea4 100644
--- a/drivers/gpu/drm/gma500/psb_drv.c
+++ b/drivers/gpu/drm/gma500/psb_drv.c
@@ -422,12 +422,17 @@ static int psb_pci_probe(struct pci_dev *pdev,
const struct pci_device_id *ent)
   /*
    * We cannot yet easily find the framebuffer's location in
memory. So
- * remove all framebuffers here.
+ * remove all framebuffers here. Note that we still want the
pci special
+ * handling to kick out vgacon.
    *
    * TODO: Refactor psb_driver_load() to map vdc_reg earlier. Then we
    *   might be able to read the framebuffer range from the
device.
    */
-    ret = drm_aperture_remove_framebuffers(true, &driver);
+    ret = drm_aperture_remove_framebuffers(false, &driver);
+    if (ret)
+    return ret;
+
+    ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev,
&driver);


This simply isn't it. If you have to work around your own API, it's time
to rethink the API.


Here's a proposal:

  1) As you're changing aperture_remove_conflicting_devices() anyway, rename
it to aperture_remove_conflicting_devices_at(), so it's clear that it takes
a memory range.

  2) Introduce aperture_remove_conflicting_pci_devices_at(), which takes a
PCI device and a memory range. It should do the is_primary and vgacon stuff,
but kick out the range.

  3) Call aperture_remove_conflicting_pci_devices_at() from gma500 with the
full range. Even if we can restructure gma500 to detect the firmware
framebuffer from its registers (there's this TODO item), we'd still need
aperture_remove_conflicting_pci_devices_at() to do something useful with it.

  4) With that, aperture_remove_conflicting_devices_at() can drop the primary
argument.

Of course, the DRM-related interface should be adapted as well. There's a
bit of overlap in the implementation of both PCI aperture helpers, but the
overall interface is clear.


This essentially just gives us a helper which does the above two
open-coded steps but all wrapped up. For gma500 only. Also I really don't
think I'm working around the api here, it's gma500 which is special:

- Normal pci devices all have their fw framebuffer within the memory bars,
   never outside. So for those the pci version is the right interface.

- If the framebuffer is outside of the pci bar then it's just not really a
   pci vga device anymore, but looks a lot more like a SoC design.

gma500 is somehow both at the same time, so it gets two calls. And having


It's not "both at the same time." It like an SoC that can act as VGA. 
But it's not really a PCI graphics card on its own. Maybe that's just 
nitpicking, though.



both calls explicitly I think is better, because it highlights the dual
nature of gma500 of being both a pci vga devices and a SoC embedded
device. Trying to make a wrapper to hide this dual nature just so we have
a single call still seems worse to me. Aside from it's just boilerplate
for no gain.

Frankly at that point I think it would be clearer to call the gma500
function remove_conflicting_gma500 or something like that. Or at least
remove_conflicting_pci_and_separate_range_at.


Yes. If you don't want a new _pci_devices_at() aperture helper, please 
duplicate the _pci_devices() helper within gma500 (with its sysfb and 
vgacon handling). Then let it take the gma500 memory range where the 
generic _pci() helper iterates over PCI BARs.


This would mark gma500 as special, while clearly communicating what's 
going on.




This is imo similar to the hypothetical case of a SoC chip which also
happens to decode legacy vga, without being a pci device. We could add a
new interface function which just nukes the vga stuff (without the pci
device tie-in, maybe with some code sharing internally in aperture.c), and
then that driver does 2 calls: 1. nuke aperture range 2. nuke vga stuff.
And sure if you have a lot of those maybe you could make a helper to safe
a few lines o

Re: [PATCH 2/2] phy: mtk-mipi-csi: add driver for CSI phy

[Julien Stephan]

On Mon, Apr 03, 2023 at 11:51:50AM +0200, Krzysztof Kozlowski wrote:
> On 03/04/2023 09:19, Julien Stephan wrote:
> > From: Phi-bang Nguyen 
> >
> > This is a new driver that supports the MIPI CSI CD-PHY for mediatek
> > mt8365 soc
> >
> > Signed-off-by: Louis Kuo 
> > Signed-off-by: Phi-bang Nguyen 
> > [Julien Stephan: use regmap]
> > [Julien Stephan: use GENMASK]
> > Co-developed-by: Julien Stephan 
> > Signed-off-by: Julien Stephan 
> > ---
> >  .../bindings/phy/mediatek,csi-phy.yaml|   9 +-
> >  MAINTAINERS   |   1 +
> >  drivers/phy/mediatek/Kconfig  |   8 +
> >  drivers/phy/mediatek/Makefile |   2 +
> >  .../phy/mediatek/phy-mtk-mipi-csi-rx-reg.h| 435 ++
> >  drivers/phy/mediatek/phy-mtk-mipi-csi.c   | 392 
> >  6 files changed, 845 insertions(+), 2 deletions(-)
> >  create mode 100644 drivers/phy/mediatek/phy-mtk-mipi-csi-rx-reg.h
> >  create mode 100644 drivers/phy/mediatek/phy-mtk-mipi-csi.c
> >
> > diff --git a/Documentation/devicetree/bindings/phy/mediatek,csi-phy.yaml 
> > b/Documentation/devicetree/bindings/phy/mediatek,csi-phy.yaml
> > index c026e43f35fd..ad4ba1d93a68 100644
> > --- a/Documentation/devicetree/bindings/phy/mediatek,csi-phy.yaml
> > +++ b/Documentation/devicetree/bindings/phy/mediatek,csi-phy.yaml
>
> NAK, bindings are always separate patches. It also does not make any
> sense - you just added it.
>
:( I messed up my rebase -i. This need to be moved and squashed with the
previous patch. I will fix it in v2. Thank you for reporting it

> > @@ -33,9 +33,14 @@ additionalProperties: false
> >
> >  examples:
> >- |
> > -phy@10011800 {
> > +soc {
> > +  #address-cells = <2>;
> > +  #size-cells = <2>;
> > +
> > +  phy@11c1 {
> >  compatible = "mediatek,mt8365-mipi-csi";
> > -reg = <0 0x10011800 0 0x60>;
> > +reg = <0 0x11c1 0 0x4000>;
> >  #phy-cells = <1>;
> > +  };
> >  };
>
>
>
> k_mipi_dphy_of_match[] = {
> > +   {.compatible = "mediatek,mt8365-mipi-csi"},
> > +   {},
> > +};
> > +MODULE_DEVICE_TABLE(of, mtk_mipi_dphy_of_match);
> > +
> > +static struct platform_driver mipi_dphy_pdrv = {
> > +   .probe = mtk_mipi_dphy_probe,
> > +   .remove = mtk_mipi_dphy_remove,
> > +   .driver = {
> > +   .name   = "mtk-mipi-csi",
> > +   .of_match_table = of_match_ptr(mtk_mipi_dphy_of_match),
>
> Drop of_match_ptr(). You should see W=1 warnings when compile testing.
>
I do not not see any warnings when trying to compile with W=1. Am I
missing something? I will drop it in v2 anyway

Best
Julien
>
> Best regards,
> Krzysztof
>

Re: [RESEND PATCH v4 03/21] staging: media: tegra-video: fix .vidioc_enum_fmt_vid_cap to return all formats

[Thierry Reding]

On Wed, Apr 05, 2023 at 10:50:37AM +0200, Hans Verkuil wrote:
[...]
> Note that this driver will stay in staging since it still fails when I try to
> capture from two sensors at the same time: syncpoint errors start appearing
> in that case. I think there are locking issues. I think I have someone to take
> a look at that, but first I want your series to get merged.

Mikko (added) is familiar with syncpoints, so he may be able to help
with. Can you provide steps to reproduce these issues? That may make
it easier for us to help figure this out.

Unfortunately I don't have any device with an actual sensor on it, so
I can only test with the test pattern generator, but syncpoint errors
sound like they would happen with either setup.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH 1/8] drm/gma500: Use drm_aperture_remove_conflicting_pci_framebuffers

[Daniel Vetter]

On Wed, Apr 05, 2023 at 11:26:51AM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 05.04.23 um 10:59 schrieb Daniel Vetter:
> > On Wed, Apr 05, 2023 at 10:07:54AM +0200, Thomas Zimmermann wrote:
> > > Hi
> > > 
> > > Am 05.04.23 um 09:49 schrieb Thomas Zimmermann:
> > > > Hi
> > > > 
> > > > Am 04.04.23 um 22:18 schrieb Daniel Vetter:
> > > > > This one nukes all framebuffers, which is a bit much. In reality
> > > > > gma500 is igpu and never shipped with anything discrete, so there 
> > > > > should
> > > > > not be any difference.
> > > > > 
> > > > > v2: Unfortunately the framebuffer sits outside of the pci bars for
> > > > > gma500, and so only using the pci helpers won't be enough. Otoh if we
> > > > > only use non-pci helper, then we don't get the vga handling, and
> > > > > subsequent refactoring to untangle these special cases won't work.
> > > > > 
> > > > > It's not pretty, but the simplest fix (since gma500 really is the only
> > > > > quirky pci driver like this we have) is to just have both calls.
> > > > > 
> > > > > Signed-off-by: Daniel Vetter 
> > > > > Cc: Patrik Jakobsson 
> > > > > Cc: Thomas Zimmermann 
> > > > > Cc: Javier Martinez Canillas 
> > > > > ---
> > > > >    drivers/gpu/drm/gma500/psb_drv.c | 9 +++--
> > > > >    1 file changed, 7 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/gma500/psb_drv.c
> > > > > b/drivers/gpu/drm/gma500/psb_drv.c
> > > > > index 2ce96b1b9c74..f1e0eed8fea4 100644
> > > > > --- a/drivers/gpu/drm/gma500/psb_drv.c
> > > > > +++ b/drivers/gpu/drm/gma500/psb_drv.c
> > > > > @@ -422,12 +422,17 @@ static int psb_pci_probe(struct pci_dev *pdev,
> > > > > const struct pci_device_id *ent)
> > > > >    /*
> > > > >     * We cannot yet easily find the framebuffer's location in
> > > > > memory. So
> > > > > - * remove all framebuffers here.
> > > > > + * remove all framebuffers here. Note that we still want the
> > > > > pci special
> > > > > + * handling to kick out vgacon.
> > > > >     *
> > > > >     * TODO: Refactor psb_driver_load() to map vdc_reg earlier. 
> > > > > Then we
> > > > >     *   might be able to read the framebuffer range from the
> > > > > device.
> > > > >     */
> > > > > -    ret = drm_aperture_remove_framebuffers(true, &driver);
> > > > > +    ret = drm_aperture_remove_framebuffers(false, &driver);
> > > > > +    if (ret)
> > > > > +    return ret;
> > > > > +
> > > > > +    ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev,
> > > > > &driver);
> > > > 
> > > > This simply isn't it. If you have to work around your own API, it's time
> > > > to rethink the API.
> > > 
> > > Here's a proposal:
> > > 
> > >   1) As you're changing aperture_remove_conflicting_devices() anyway, 
> > > rename
> > > it to aperture_remove_conflicting_devices_at(), so it's clear that it 
> > > takes
> > > a memory range.
> > > 
> > >   2) Introduce aperture_remove_conflicting_pci_devices_at(), which takes a
> > > PCI device and a memory range. It should do the is_primary and vgacon 
> > > stuff,
> > > but kick out the range.
> > > 
> > >   3) Call aperture_remove_conflicting_pci_devices_at() from gma500 with 
> > > the
> > > full range. Even if we can restructure gma500 to detect the firmware
> > > framebuffer from its registers (there's this TODO item), we'd still need
> > > aperture_remove_conflicting_pci_devices_at() to do something useful with 
> > > it.
> > > 
> > >   4) With that, aperture_remove_conflicting_devices_at() can drop the 
> > > primary
> > > argument.
> > > 
> > > Of course, the DRM-related interface should be adapted as well. There's a
> > > bit of overlap in the implementation of both PCI aperture helpers, but the
> > > overall interface is clear.
> > 
> > This essentially just gives us a helper which does the above two
> > open-coded steps but all wrapped up. For gma500 only. Also I really don't
> > think I'm working around the api here, it's gma500 which is special:
> > 
> > - Normal pci devices all have their fw framebuffer within the memory bars,
> >never outside. So for those the pci version is the right interface.
> > 
> > - If the framebuffer is outside of the pci bar then it's just not really a
> >pci vga device anymore, but looks a lot more like a SoC design.
> > 
> > gma500 is somehow both at the same time, so it gets two calls. And having
> 
> It's not "both at the same time." It like an SoC that can act as VGA. But
> it's not really a PCI graphics card on its own. Maybe that's just
> nitpicking, though.

I don't see why it can't be a pci vga card. There is no requirement that a
pci vga card must be also a non-vga card with real non-vga framebuffer. We
don't have a hole lot of them really.

> > both calls explicitly I think is better, because it highlights the dual
> > nature of gma500 of being both a pci vga devices and a SoC embedded
> > device. Trying to make a wrapper to hide this dual nature just so we have
> > a single call still se

Re: [PULL] drm-intel-fixes

[Daniel Vetter]

On Wed, Apr 05, 2023 at 12:04:04PM +0300, Jani Nikula wrote:
> 
> Hi Dave & Daniel -
> 
> drm-intel-fixes-2023-04-05:
> drm/i915 fixes for v6.3-rc6:
> - Fix DP MST DSC M/N calculation to use compressed bpp
> - Fix racy use-after-free in perf ioctl
> - Fix context runtime accounting
> - Fix handling of GT reset during HuC loading
> - Fix use of unsigned vm_fault_t for error values
> 
> BR,
> Jani.
> 
> The following changes since commit 7e364e56293bb98cae1b55fd835f5991c4e96e7d:
> 
>   Linux 6.3-rc5 (2023-04-02 14:29:29 -0700)
> 
> are available in the Git repository at:
> 
>   git://anongit.freedesktop.org/drm/drm-intel tags/drm-intel-fixes-2023-04-05
> 
> for you to fetch changes up to dc3421560a67361442f33ec962fc6dd48895a0df:
> 
>   drm/i915: Fix context runtime accounting (2023-04-03 11:37:00 +0300)

Pulled, thanks

> 
> 
> drm/i915 fixes for v6.3-rc6:
> - Fix DP MST DSC M/N calculation to use compressed bpp
> - Fix racy use-after-free in perf ioctl
> - Fix context runtime accounting
> - Fix handling of GT reset during HuC loading
> - Fix use of unsigned vm_fault_t for error values
> 
> 
> Daniele Ceraolo Spurio (1):
>   drm/i915/huc: Cancel HuC delayed load timer on reset.
> 
> Matthew Auld (1):
>   drm/i915/ttm: fix sparse warning
> 
> Min Li (1):
>   drm/i915: fix race condition UAF in i915_perf_add_config_ioctl
> 
> Stanislav Lisovskiy (1):
>   drm/i915: Use compressed bpp when calculating m/n value for DP MST DSC
> 
> Tvrtko Ursulin (1):
>   drm/i915: Fix context runtime accounting
> 
>  drivers/gpu/drm/i915/display/intel_dp_mst.c  |  2 +-
>  drivers/gpu/drm/i915/gem/i915_gem_ttm.c  |  5 +++--
>  drivers/gpu/drm/i915/gt/intel_execlists_submission.c | 12 ++--
>  drivers/gpu/drm/i915/gt/uc/intel_huc.c   |  7 +++
>  drivers/gpu/drm/i915/gt/uc/intel_huc.h   |  7 +--
>  drivers/gpu/drm/i915/i915_perf.c |  6 +++---
>  6 files changed, 25 insertions(+), 14 deletions(-)
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] video/aperture: fix typos

[Sui Jingfeng]

Hi,

thanks you for the time and effort  for reviewing.

On 2023/4/4 19:03, Javier Martinez Canillas wrote:

Javier Martinez Canillas  writes:

[...]


/*
 * Remove the device from the device hierarchy. This is the right thing
-* to do for firmware-based DRM drivers, such as EFI, VESA or VGA. After
+* to do for firmware-based fb drivers, such as EFI, VESA or VGA. After

That sentences is not well phrased. Maybe say 'This is required for
firmware-provided graphics, such as EFI, VESA or VGA.'


Graphic drivers or display drivers would indeed be more accurate here. But
I think that "fb drivers" is still well pharsed since the are other places
where either fbdev or DRM drivers for firmware-provided framebuffers are
named like that.


Sui,

Maybe you could post a follow-up patch to improve the comment as suggested
by Thomas?


Yes, certainly.


This is the right thing to do for conflicting drivers takes over the 
hardware resource required.



But the comments is actually nearly perfect in overall, it has some 
difficulty to improve


the perfection.  Below is my personal understanding toward the above 
sentence.



efifb and simplefb belong to the class of firmware based framebuffer driver.

They are generic and platform agnostic, yet they have to relay on the 
firmware


to passing fb format, fb size, fb base address, fb resolution and fb 
stride etc to the kernel.


Linux kernel using those information to fill the global screen_info 
structure.


sysfb_init() then using the global screen_info to  create a platform device,

the device will be claimed by efifb or simplefb driver finally. This is 
a hand over solution.


It relay on the firmware setup such a framebuffer and hand over the 
state(this is


actually a kind of modeset state) to kernel.


efifb only own the potential hardware resource for a very short time if a

conflicting drm driver probe successfully.


For the platform/graphics without  a drm driver, developers may choose to

use efifb driver as a replacement.  So, there are no conflicting happen on

such a case. The `nomodeset` kernel cmd options can also be used for

debugging and testing purpose if the more intelligent drm driver is broken

due to bugs.

Re: [RFC PATCH 00/10] Xe DRM scheduler and long running workload plans

[Christian König]

Am 05.04.23 um 11:07 schrieb Daniel Vetter:

[SNIP]

I would approach it from the complete other side. This component here is a
tool to decide what job should run next.

How that is then signaled and run should not be part of the scheduler, but
another more higher level component.

This way you also don't have a problem with not using DMA-fences as
dependencies as well as constrains for running more jobs.

I think we're talking about two things here and mixing them up.

For the dependencies I agree with you, and imo that higher level tool
should probably just be an on-demand submit thread in userspace for the
rare case where the kernel would need to sort out a dependency otherwise
(due to running out of ringspace in the per-ctx ringbuffer).

The other thing is the message passing stuff, and this is what I was
talking about above. This has nothing to do with handling dependencies,
but with talking to the gpu fw. Here the intel design issue is that the fw
only provides a single queue, and it's in-order. Which means it
fundamentally has the stalling issue you describe as a point against a
message passing design. And fundamentally we need to be able to talk to
the fw in the scheduler ->run_job callback.

The proposal here for the message passing part is that since it has the
stalling issue already anyway, and the scheduler needs to be involved
anyway, it makes sense to integrated this (as an optional thing, only for
drivers which have this kind of fw interface) into the scheduler.
Otherwise you just end up with two layers for no reason and more ping-pong
delay because the ->run_job needs to kick off the subordinate driver layer
first. Note that for this case the optional message passing support in the
drm/scheduler actually makes things better, because it allows you to cut
out one layer.

Of course if a driver with better fw interface uses this message passing
support, then that's bad. Hence the big warning in the kerneldoc.


Well what I wanted to say is that if you design the dependency handling 
/ scheduler properly you don't need the message passing through it.


For example if the GPU scheduler component uses a work item to do it's 
handling instead of a kthread you could also let the driver specify the 
work queue where this work item is executed on.


When you design it like this the driver specifies the thread context of 
execution for it's job. In other words it can specify a single threaded 
firmware work queue as well.


When you then have other messages which needs to be passed to the 
firmware you can also use the same single threaded workqueue for this.


Drivers which have a different firmware interface would just use one of 
the system work queues instead.


This approach basically decouples the GPU scheduler component from the 
message passing functionality.


Regards,
Christian.



-Daniel

[PATCH] drm/panel-edp: Add AUO NE135FBM-N41 v8.1 panel entry

[AngeloGioacchino Del Regno]

Add a panel entry with delay_200_500_e50 for the AUO NE135FBM-N41
version 8.1, found on a number of ACER laptops, including the
Swift 3 (SF313-52, SF313-53), Chromebook Spin 513 (CP513-2H) and
others.

Signed-off-by: AngeloGioacchino Del Regno 

---
 drivers/gpu/drm/panel/panel-edp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/panel/panel-edp.c 
b/drivers/gpu/drm/panel/panel-edp.c
index 926906ca2304..e23ddab2126e 100644
--- a/drivers/gpu/drm/panel/panel-edp.c
+++ b/drivers/gpu/drm/panel/panel-edp.c
@@ -1879,6 +1879,7 @@ static const struct edp_panel_entry edp_panels[] = {
EDP_PANEL_ENTRY('B', 'O', 'E', 0x07d1, &boe_nv133fhm_n61.delay, 
"NV133FHM-N61"),
EDP_PANEL_ENTRY('B', 'O', 'E', 0x082d, &boe_nv133fhm_n61.delay, 
"NV133FHM-N62"),
EDP_PANEL_ENTRY('B', 'O', 'E', 0x094b, &delay_200_500_e50, 
"NT116WHM-N21"),
+   EDP_PANEL_ENTRY('B', 'O', 'E', 0x095f, &delay_200_500_e50, 
"NE135FBM-N41 v8.1"),
EDP_PANEL_ENTRY('B', 'O', 'E', 0x098d, &boe_nv110wtm_n61.delay, 
"NV110WTM-N61"),
EDP_PANEL_ENTRY('B', 'O', 'E', 0x09dd, &delay_200_500_e50, 
"NT116WHM-N21"),
EDP_PANEL_ENTRY('B', 'O', 'E', 0x0a5d, &delay_200_500_e50, 
"NV116WHM-N45"),
-- 
2.40.0

Re: [RFC PATCH 00/10] Xe DRM scheduler and long running workload plans

[Daniel Vetter]

On Wed, 5 Apr 2023 at 11:57, Christian König  wrote:
>
> Am 05.04.23 um 11:07 schrieb Daniel Vetter:
> > [SNIP]
> >> I would approach it from the complete other side. This component here is a
> >> tool to decide what job should run next.
> >>
> >> How that is then signaled and run should not be part of the scheduler, but
> >> another more higher level component.
> >>
> >> This way you also don't have a problem with not using DMA-fences as
> >> dependencies as well as constrains for running more jobs.
> > I think we're talking about two things here and mixing them up.
> >
> > For the dependencies I agree with you, and imo that higher level tool
> > should probably just be an on-demand submit thread in userspace for the
> > rare case where the kernel would need to sort out a dependency otherwise
> > (due to running out of ringspace in the per-ctx ringbuffer).
> >
> > The other thing is the message passing stuff, and this is what I was
> > talking about above. This has nothing to do with handling dependencies,
> > but with talking to the gpu fw. Here the intel design issue is that the fw
> > only provides a single queue, and it's in-order. Which means it
> > fundamentally has the stalling issue you describe as a point against a
> > message passing design. And fundamentally we need to be able to talk to
> > the fw in the scheduler ->run_job callback.
> >
> > The proposal here for the message passing part is that since it has the
> > stalling issue already anyway, and the scheduler needs to be involved
> > anyway, it makes sense to integrated this (as an optional thing, only for
> > drivers which have this kind of fw interface) into the scheduler.
> > Otherwise you just end up with two layers for no reason and more ping-pong
> > delay because the ->run_job needs to kick off the subordinate driver layer
> > first. Note that for this case the optional message passing support in the
> > drm/scheduler actually makes things better, because it allows you to cut
> > out one layer.
> >
> > Of course if a driver with better fw interface uses this message passing
> > support, then that's bad. Hence the big warning in the kerneldoc.
>
> Well what I wanted to say is that if you design the dependency handling
> / scheduler properly you don't need the message passing through it.
>
> For example if the GPU scheduler component uses a work item to do it's
> handling instead of a kthread you could also let the driver specify the
> work queue where this work item is executed on.
>
> When you design it like this the driver specifies the thread context of
> execution for it's job. In other words it can specify a single threaded
> firmware work queue as well.
>
> When you then have other messages which needs to be passed to the
> firmware you can also use the same single threaded workqueue for this.
>
> Drivers which have a different firmware interface would just use one of
> the system work queues instead.
>
> This approach basically decouples the GPU scheduler component from the
> message passing functionality.

Hm I guess we've been talking past each another big time, because
that's really what I thought was under discussions? Essentially the
current rfc, but implementing with some polish.

iow I agree with you (I think at least).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/3] drm/fb-helper: set x/yres_virtual in drm_fb_helper_check_var

[Javier Martinez Canillas]

Daniel Vetter  writes:

> Drivers are supposed to fix this up if needed if they don't outright
> reject it. Uncovered by 6c11df58fd1a ("fbmem: Check virtual screen
> sizes in fb_set_var()").
>

Should have a Fixes: tag ? I understand what was uncovered by that commit
but it help distros to figure out if something has to be cherry-picked by
them. So I believe that would be useful to have it.

The patch looks good to me.

Reviewed-by: Javier Martinez Canillas 

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 2/3] drm/fb-helper: drop redundant pixclock check from drm_fb_helper_set_par()

[Javier Martinez Canillas]

Daniel Vetter  writes:

> The fb_check_var hook is supposed to validate all this stuff. Any
> errors from fb_set_par are considered driver/hw issues and resulting
> in dmesg warnings.
>
> Luckily we do fix up the pixclock already, so this is all fine.
>
> Signed-off-by: Daniel Vetter 
> Cc: Maarten Lankhorst 
> Cc: Maxime Ripard 
> Cc: Thomas Zimmermann 
> ---

Makes sense to drop this.

Reviewed-by: Javier Martinez Canillas 

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH] video/aperture: fix typos

[Sui Jingfeng]

On 2023/4/5 17:55, Sui Jingfeng wrote:

Hi,

thanks you for the time and effort  for reviewing.

On 2023/4/4 19:03, Javier Martinez Canillas wrote:

Javier Martinez Canillas  writes:

[...]


   /*
    * Remove the device from the device hierarchy. This is the 
right thing
- * to do for firmware-based DRM drivers, such as EFI, VESA or 
VGA. After
+ * to do for firmware-based fb drivers, such as EFI, VESA or 
VGA. After

That sentences is not well phrased. Maybe say 'This is required for
firmware-provided graphics, such as EFI, VESA or VGA.'

Graphic drivers or display drivers would indeed be more accurate 
here. But
I think that "fb drivers" is still well pharsed since the are other 
places
where either fbdev or DRM drivers for firmware-provided framebuffers 
are

named like that.


Sui,

Maybe you could post a follow-up patch to improve the comment as 
suggested

by Thomas?


Yes, certainly.


This is the right thing to do for conflicting drivers takes over the 
hardware resource required.




While the `drivers` include both drm driver and the real device 
dependent framebuffer driver,


They are typically build as kernel module as oppose to the efifb and 
simplefb which is built


into kernel kernel.  Firmware framebuffer is conflict with the drm 
driver is because the memory


region they use overlaps.  If there no overlap, then no `taken over`  
will be happen.



By the way,  I'm asked to made efifb and simplefb works on LoongArch and 
Mips platform in the past.


We are using downstream kernel(linux-4.19)  at that time. efifb is ony 
works for platform with uefi


firmware support. By ensure that framebuffer locate at the address range 
of the on-board video ram(VRAM)


and passing screen_info parameters to kernel correctly, 
drm_aperture_remove_conflicting_framebuffers


will success. This required the uefi firmware engineer understand this, 
for loongson bridge chip, this need


a small trick.  Simplefb is only tested on loongson SoC platform by 
providing fb parameters in DT which


match the PMON firmware's setting. As the framebuffer may located at 
anywhere in the physical address


space, there no aperture for SoC anymore.  Should call 
aperture_remove_conflicting_devices(0, ~0, false, "drmfb")


to remove them all.




But the comments is actually nearly perfect in overall, it has some 
difficulty to improve


the perfection.  Below is my personal understanding toward the above 
sentence.



efifb and simplefb belong to the class of firmware based framebuffer 
driver.


They are generic and platform agnostic, yet they have to relay on the 
firmware


to passing fb format, fb size, fb base address, fb resolution and fb 
stride etc to the kernel.


Linux kernel using those information to fill the global screen_info 
structure.


sysfb_init() then using the global screen_info to  create a platform 
device,


the device will be claimed by efifb or simplefb driver finally. This 
is a hand over solution.


It relay on the firmware setup such a framebuffer and hand over the 
state(this is


actually a kind of modeset state) to kernel.


efifb only own the potential hardware resource for a very short time if a

conflicting drm driver probe successfully.


For the platform/graphics without  a drm driver, developers may choose to

use efifb driver as a replacement.  So, there are no conflicting 
happen on


such a case. The `nomodeset` kernel cmd options can also be used for

debugging and testing purpose if the more intelligent drm driver is 
broken


due to bugs.

Re: [PATCH 1/5] drm/i915/ttm: Add I915_BO_PREALLOC

[Das, Nirmoy]

On 4/4/2023 5:30 PM, Andrzej Hajda wrote:



On 04.04.2023 16:30, Nirmoy Das wrote:

Add a mechanism to keep existing data when creating
a ttm object with I915_BO_ALLOC_USER flag.

Cc: Matthew Auld 
Cc: Andi Shyti 
Cc: Andrzej Hajda 
Cc: Ville Syrjälä 
Cc: Jani Nikula 
Cc: Imre Deak 
Signed-off-by: Nirmoy Das 
---
  drivers/gpu/drm/i915/gem/i915_gem_object_types.h | 15 +++
  drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c |  5 +++--
  2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h 
b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h

index 5dcbbef31d44..830c11431ee8 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
@@ -328,6 +328,12 @@ struct drm_i915_gem_object {
   */
  #define I915_BO_ALLOC_GPU_ONLY  BIT(6)
  #define I915_BO_ALLOC_CCS_AUX  BIT(7)
+/*
+ * Object is allowed to retain its initial data and will not be 
cleared on first

+ * access if used along with I915_BO_ALLOC_USER. This is mainly to keep
+ * preallocated framebuffer data intact while transitioning it to 
i915drmfb.

+ */
+#define I915_BO_PREALLOC  BIT(8)
  #define I915_BO_ALLOC_FLAGS (I915_BO_ALLOC_CONTIGUOUS | \
   I915_BO_ALLOC_VOLATILE | \
   I915_BO_ALLOC_CPU_CLEAR | \
@@ -335,10 +341,11 @@ struct drm_i915_gem_object {
   I915_BO_ALLOC_PM_VOLATILE | \
   I915_BO_ALLOC_PM_EARLY | \
   I915_BO_ALLOC_GPU_ONLY | \
- I915_BO_ALLOC_CCS_AUX)
-#define I915_BO_READONLY  BIT(8)
-#define I915_TILING_QUIRK_BIT 9 /* unknown swizzling; do not 
release! */

-#define I915_BO_PROTECTED BIT(10)
+ I915_BO_ALLOC_CCS_AUX | \
+ I915_BO_PREALLOC)
+#define I915_BO_READONLY  BIT(9)
+#define I915_TILING_QUIRK_BIT 10 /* unknown swizzling; do not 
release! */

+#define I915_BO_PROTECTED BIT(11)
  /**
   * @mem_flags - Mutable placement-related flags
   *
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c 
b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c

index dd188dfcc423..69eb20ed4d47 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
@@ -576,7 +576,7 @@ int i915_ttm_move(struct ttm_buffer_object *bo, 
bool evict,

  struct dma_fence *migration_fence = NULL;
  struct ttm_tt *ttm = bo->ttm;
  struct i915_refct_sgt *dst_rsgt;
-    bool clear;
+    bool clear, prealloc_bo;
  int ret;
    if (GEM_WARN_ON(i915_ttm_is_ghost_object(bo))) {
@@ -632,7 +632,8 @@ int i915_ttm_move(struct ttm_buffer_object *bo, 
bool evict,

  return PTR_ERR(dst_rsgt);
    clear = !i915_ttm_cpu_maps_iomem(bo->resource) && (!ttm || 
!ttm_tt_is_populated(ttm));
-    if (!(clear && ttm && !(ttm->page_flags & 
TTM_TT_FLAG_ZERO_ALLOC))) {

+    prealloc_bo = obj->flags & I915_BO_PREALLOC;
+    if (!(clear && ttm && !((ttm->page_flags & 
TTM_TT_FLAG_ZERO_ALLOC) && !prealloc_bo))) {


This looks like school exercise for complicated usage of logical 
operators, and I have problem with understanding this :)

Couldn't this be somehow simplified?


(I thought I sent this email yesterday but was stuck in oAuth pop up 
sign-in)


Yes, this can be improved I think, took me while too.



Anyway as the patch just reuses existing code:
Reviewed-by: Andrzej Hajda 



Thanks Andrzej,

Nirmoy



Regards
Andrzej



  struct i915_deps deps;
    i915_deps_init(&deps, GFP_KERNEL | __GFP_NORETRY | 
__GFP_NOWARN);

Re: [PATCH 3/3] drm/fb-helper: fix input validation gaps in check_var

[Javier Martinez Canillas]

Daniel Vetter  writes:

> Apparently drivers need to check all this stuff themselves, which for
> most things makes sense I guess. And for everything else we luck out,
> because modern distros stopped supporting any other fbdev drivers than
> drm ones and I really don't want to argue anymore about who needs to
> check stuff. Therefore fixing all this just for drm fbdev emulation is
> good enough.
>

Agreed.

> Note that var->active is not set or validated. This is just control
> flow for fbmem.c and needs to be validated in there as needed.
>
> Signed-off-by: Daniel Vetter 
> Cc: Maarten Lankhorst 
> Cc: Maxime Ripard 
> Cc: Thomas Zimmermann 
> ---

[...]

>  
> +static void __fill_var(struct fb_var_screeninfo *var,
> +struct drm_framebuffer *fb)
> +{
> + int i;
> +
> + var->xres_virtual = fb->width;
> + var->yres_virtual = fb->height;
> + var->accel_flags = FB_ACCELF_TEXT;
> + var->bits_per_pixel = drm_format_info_bpp(fb->format, 0);
> +
> + var->height = var->width = 0;
> + var->left_margin = var->right_margin = 0;
> + var->upper_margin = var->lower_margin = 0;
> + var->hsync_len = var->vsync_len = 0;
> + var->sync = var->vmode = 0;
> + var->rotate = 0;
> + var->colorspace = 0;
> + for (i = 0; i < 4; i++)
> + var->reserved[i] = 0;
> +}
> +
>  /**
>   * drm_fb_helper_check_var - implementation for &fb_ops.fb_check_var
>   * @var: screeninfo to check
> @@ -1595,8 +1616,22 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo 
> *var,
>   return -EINVAL;
>   }
>  
> - var->xres_virtual = fb->width;
> - var->yres_virtual = fb->height;
> + __fill_var(var, fb);
> +

[...]

There is the following here (in latest drm-misc/drm-misc-next at least):

/*
 * Changes struct fb_var_screeninfo are currently not pushed back
 * to KMS, hence fail if different settings are requested.
 */
bpp = drm_format_info_bpp(format, 0);
if (var->bits_per_pixel > bpp ||
var->xres > fb->width || var->yres > fb->height ||
var->xres_virtual > fb->width || var->yres_virtual > fb->height) {
drm_dbg_kms(dev, "fb requested width/height/bpp can't fit in 
current fb "
  "request %dx%d-%d (virtual %dx%d) > %dx%d-%d\n",
  var->xres, var->yres, var->bits_per_pixel,
  var->xres_virtual, var->yres_virtual,
  fb->width, fb->height, bpp);
return -EINVAL;
}

but only the 'var->xres > fb->width || var->yres > fb->height' from the
conditions checked could be false after your __fill_var() call above.

You should drop the 'var->bits_per_pixel > bpp', 'var->xres_virtual >
fb->width' and 'var->yres_virtual > fb->height' checks I believe since
those will always be true.

> + /*
> +  * fb_pan_display() validates this, but fb_set_par() doesn't and just
> +  * falls over. Note that __fill_var above adjusts y/res_virtual.
> +  */
> + if (var->yoffset > var->yres_virtual - var->yres ||
> + var->xoffset > var->xres_virtual - var->xres)
> + return -EINVAL;
> +
> + /* We neither support grayscale nor FOURCC (also stored in here). */
> + if (var->grayscale > 0)
> + return -EINVAL;
> +
> + if (var->nonstd)
> + return -EINVAL;
>  
>   /*
>* Workaround for SDL 1.2, which is known to be setting all pixel format
> @@ -1612,11 +1647,6 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo 
> *var,
>   drm_fb_helper_fill_pixel_fmt(var, format);
>   }
>  

Other than what I mentioned, the patch makes sense to me.

Reviewed-by: Javier Martinez Canillas 

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 1/5] drm/i915/ttm: Add I915_BO_PREALLOC

[Das, Nirmoy]

On 4/4/2023 6:23 PM, Andi Shyti wrote:

Hi Nirmoy,

On Tue, Apr 04, 2023 at 04:30:56PM +0200, Nirmoy Das wrote:

Add a mechanism to keep existing data when creating
a ttm object with I915_BO_ALLOC_USER flag.

why do we need this mechanism? What was the logic behind? These
are all questions people might have when checking this commit.
Please be a bit more explicative.



Agree, the commit message is bit short. I will add more content in next 
revision.





Cc: Matthew Auld 
Cc: Andi Shyti 
Cc: Andrzej Hajda 
Cc: Ville Syrjälä 
Cc: Jani Nikula 
Cc: Imre Deak 
Signed-off-by: Nirmoy Das 

Reviewed-by: Andi Shyti 



Thanks,

Nirmoy



Thanks,
Andi

Re: [PATCH 1/8] drm/gma500: Use drm_aperture_remove_conflicting_pci_framebuffers

[Thomas Zimmermann]

Hi

Am 05.04.23 um 11:38 schrieb Daniel Vetter:

On Wed, Apr 05, 2023 at 11:26:51AM +0200, Thomas Zimmermann wrote:

Hi

Am 05.04.23 um 10:59 schrieb Daniel Vetter:

On Wed, Apr 05, 2023 at 10:07:54AM +0200, Thomas Zimmermann wrote:

Hi

Am 05.04.23 um 09:49 schrieb Thomas Zimmermann:

Hi

Am 04.04.23 um 22:18 schrieb Daniel Vetter:

This one nukes all framebuffers, which is a bit much. In reality
gma500 is igpu and never shipped with anything discrete, so there should
not be any difference.

v2: Unfortunately the framebuffer sits outside of the pci bars for
gma500, and so only using the pci helpers won't be enough. Otoh if we
only use non-pci helper, then we don't get the vga handling, and
subsequent refactoring to untangle these special cases won't work.

It's not pretty, but the simplest fix (since gma500 really is the only
quirky pci driver like this we have) is to just have both calls.

Signed-off-by: Daniel Vetter 
Cc: Patrik Jakobsson 
Cc: Thomas Zimmermann 
Cc: Javier Martinez Canillas 
---
    drivers/gpu/drm/gma500/psb_drv.c | 9 +++--
    1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/gma500/psb_drv.c
b/drivers/gpu/drm/gma500/psb_drv.c
index 2ce96b1b9c74..f1e0eed8fea4 100644
--- a/drivers/gpu/drm/gma500/psb_drv.c
+++ b/drivers/gpu/drm/gma500/psb_drv.c
@@ -422,12 +422,17 @@ static int psb_pci_probe(struct pci_dev *pdev,
const struct pci_device_id *ent)
    /*
     * We cannot yet easily find the framebuffer's location in
memory. So
- * remove all framebuffers here.
+ * remove all framebuffers here. Note that we still want the
pci special
+ * handling to kick out vgacon.
     *
     * TODO: Refactor psb_driver_load() to map vdc_reg earlier. Then we
     *   might be able to read the framebuffer range from the
device.
     */
-    ret = drm_aperture_remove_framebuffers(true, &driver);
+    ret = drm_aperture_remove_framebuffers(false, &driver);
+    if (ret)
+    return ret;
+
+    ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev,
&driver);


This simply isn't it. If you have to work around your own API, it's time
to rethink the API.


Here's a proposal:

   1) As you're changing aperture_remove_conflicting_devices() anyway, rename
it to aperture_remove_conflicting_devices_at(), so it's clear that it takes
a memory range.

   2) Introduce aperture_remove_conflicting_pci_devices_at(), which takes a
PCI device and a memory range. It should do the is_primary and vgacon stuff,
but kick out the range.

   3) Call aperture_remove_conflicting_pci_devices_at() from gma500 with the
full range. Even if we can restructure gma500 to detect the firmware
framebuffer from its registers (there's this TODO item), we'd still need
aperture_remove_conflicting_pci_devices_at() to do something useful with it.

   4) With that, aperture_remove_conflicting_devices_at() can drop the primary
argument.

Of course, the DRM-related interface should be adapted as well. There's a
bit of overlap in the implementation of both PCI aperture helpers, but the
overall interface is clear.


This essentially just gives us a helper which does the above two
open-coded steps but all wrapped up. For gma500 only. Also I really don't
think I'm working around the api here, it's gma500 which is special:

- Normal pci devices all have their fw framebuffer within the memory bars,
never outside. So for those the pci version is the right interface.

- If the framebuffer is outside of the pci bar then it's just not really a
pci vga device anymore, but looks a lot more like a SoC design.

gma500 is somehow both at the same time, so it gets two calls. And having


It's not "both at the same time." It like an SoC that can act as VGA. But
it's not really a PCI graphics card on its own. Maybe that's just
nitpicking, though.


I don't see why it can't be a pci vga card. There is no requirement that a
pci vga card must be also a non-vga card with real non-vga framebuffer. We
don't have a hole lot of them really.


both calls explicitly I think is better, because it highlights the dual
nature of gma500 of being both a pci vga devices and a SoC embedded
device. Trying to make a wrapper to hide this dual nature just so we have
a single call still seems worse to me. Aside from it's just boilerplate
for no gain.

Frankly at that point I think it would be clearer to call the gma500
function remove_conflicting_gma500 or something like that. Or at least
remove_conflicting_pci_and_separate_range_at.


Yes. If you don't want a new _pci_devices_at() aperture helper, please
duplicate the _pci_devices() helper within gma500 (with its sysfb and vgacon
handling). Then let it take the gma500 memory range where the generic _pci()
helper iterates over PCI BARs.

This would mark gma500 as special, while clearly communicating what's going
on.


The thing is, pci is self-describing. We don't need to open code every
variant in every driver, the pci code can figure out in a generic w

[PATCH] drm/nouveau/fb: add missing sysmen flush callbacks

[Karol Herbst]

Closes: https://gitlab.freedesktop.org/drm/nouveau/-/issues/203
Fixes: 5728d064190e1 ("drm/nouveau/fb: handle sysmem flush page from common 
code")
Signed-off-by: Karol Herbst 
---
 drivers/gpu/drm/nouveau/nvkm/subdev/fb/gf108.c | 1 +
 drivers/gpu/drm/nouveau/nvkm/subdev/fb/gk104.c | 1 +
 drivers/gpu/drm/nouveau/nvkm/subdev/fb/gk110.c | 1 +
 drivers/gpu/drm/nouveau/nvkm/subdev/fb/gm107.c | 1 +
 4 files changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/fb/gf108.c 
b/drivers/gpu/drm/nouveau/nvkm/subdev/fb/gf108.c
index 76678dd60f93f..c4c6f67af7ccc 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/fb/gf108.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/fb/gf108.c
@@ -31,6 +31,7 @@ gf108_fb = {
.init = gf100_fb_init,
.init_page = gf100_fb_init_page,
.intr = gf100_fb_intr,
+   .sysmem.flush_page_init = gf100_fb_sysmem_flush_page_init,
.ram_new = gf108_ram_new,
.default_bigpage = 17,
 };
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/fb/gk104.c 
b/drivers/gpu/drm/nouveau/nvkm/subdev/fb/gk104.c
index f73442ccb424b..433fa966ba231 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/fb/gk104.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/fb/gk104.c
@@ -77,6 +77,7 @@ gk104_fb = {
.init = gf100_fb_init,
.init_page = gf100_fb_init_page,
.intr = gf100_fb_intr,
+   .sysmem.flush_page_init = gf100_fb_sysmem_flush_page_init,
.ram_new = gk104_ram_new,
.default_bigpage = 17,
.clkgate_pack = gk104_fb_clkgate_pack,
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/fb/gk110.c 
b/drivers/gpu/drm/nouveau/nvkm/subdev/fb/gk110.c
index 45d6cdffafeed..4dc283dedf8b5 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/fb/gk110.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/fb/gk110.c
@@ -59,6 +59,7 @@ gk110_fb = {
.init = gf100_fb_init,
.init_page = gf100_fb_init_page,
.intr = gf100_fb_intr,
+   .sysmem.flush_page_init = gf100_fb_sysmem_flush_page_init,
.ram_new = gk104_ram_new,
.default_bigpage = 17,
.clkgate_pack = gk110_fb_clkgate_pack,
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/fb/gm107.c 
b/drivers/gpu/drm/nouveau/nvkm/subdev/fb/gm107.c
index de52462a92bf0..90bfff616d35b 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/fb/gm107.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/fb/gm107.c
@@ -31,6 +31,7 @@ gm107_fb = {
.init = gf100_fb_init,
.init_page = gf100_fb_init_page,
.intr = gf100_fb_intr,
+   .sysmem.flush_page_init = gf100_fb_sysmem_flush_page_init,
.ram_new = gm107_ram_new,
.default_bigpage = 17,
 };
-- 
2.39.2

Re: [PATCH RFC 04/18] rust: drm: gem: Add GEM object abstraction

[Daniel Vetter]

Meta: I'm trying to unblock myself by limiting each reply to a narrow-ish
topic. Otherwise it's just too much. Here's the first.

On Tue, Mar 07, 2023 at 11:25:29PM +0900, Asahi Lina wrote:
> The DRM GEM subsystem is the DRM memory management subsystem used by
> most modern drivers. Add a Rust abstraction to allow Rust DRM driver
> implementations to use it.
> 
> Signed-off-by: Asahi Lina 
> ---
>  rust/bindings/bindings_helper.h |   1 +
>  rust/helpers.c  |  23 +++
>  rust/kernel/drm/drv.rs  |   4 +-
>  rust/kernel/drm/gem/mod.rs  | 374 
> 
>  rust/kernel/drm/mod.rs  |   1 +
>  5 files changed, 401 insertions(+), 2 deletions(-)
> 
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 7d7828faf89c..7183dfe6473f 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -9,6 +9,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> diff --git a/rust/helpers.c b/rust/helpers.c
> index 73b2ce607f27..78ec4162b03b 100644
> --- a/rust/helpers.c
> +++ b/rust/helpers.c
> @@ -18,6 +18,7 @@
>   * accidentally exposed.
>   */
>  
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -374,6 +375,28 @@ void rust_helper_init_completion(struct completion *c)
>  }
>  EXPORT_SYMBOL_GPL(rust_helper_init_completion);
>  
> +#ifdef CONFIG_DRM
> +
> +void rust_helper_drm_gem_object_get(struct drm_gem_object *obj)
> +{
> + drm_gem_object_get(obj);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_drm_gem_object_get);
> +
> +void rust_helper_drm_gem_object_put(struct drm_gem_object *obj)
> +{
> + drm_gem_object_put(obj);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_drm_gem_object_put);
> +
> +__u64 rust_helper_drm_vma_node_offset_addr(struct drm_vma_offset_node *node)
> +{
> + return drm_vma_node_offset_addr(node);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_drm_vma_node_offset_addr);

Uh all the rust helper wrappers for all the kernel in a single file does
not sound good. Can we not split these up into each subsystem, and then
maybe instead of sprinkling #ifdef all over a .c file Make the compilation
of that file conditional on rust support (plus whatever other Kconfig gate
the other c files has already)?

Otherwise if rust adoption picks up there's going to be endless amounts of
cross-subsystem conflicts.

Also similarly, can we perhaps split up the bindings_helper.h file in a
per-subsystem way?


> +
> +#endif
> +
>  /*
>   * We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type
>   * as the Rust `usize` type, so we can use it in contexts where Rust
> diff --git a/rust/kernel/drm/drv.rs b/rust/kernel/drm/drv.rs
> index 1dcb651e1417..c138352cb489 100644
> --- a/rust/kernel/drm/drv.rs
> +++ b/rust/kernel/drm/drv.rs
> @@ -126,7 +126,7 @@ pub struct AllocOps {

Similary I guess this needs to be all under rust for rust reasons. I'm
assuming that the plan is that rust patches in here get acked/reviewed by
rust people, but then merged through the drm subsystem? At least long term
I think that's the least painful way.

Meaning we need a MAINTAINERS entry for rust/kernel/drm which adds
dri-devel for review and the usual git repos somewhere earlier in the
series.
-Daniel

>  }
>  
>  /// Trait for memory manager implementations. Implemented internally.
> -pub trait AllocImpl: Sealed {
> +pub trait AllocImpl: Sealed + drm::gem::IntoGEMObject {
>  /// The C callback operations for this memory manager.
>  const ALLOC_OPS: AllocOps;
>  }
> @@ -263,7 +263,7 @@ impl Registration {
>  drm,
>  registered: false,
>  vtable,
> -fops: Default::default(), // TODO: GEM abstraction
> +fops: drm::gem::create_fops(),
>  _pin: PhantomPinned,
>  _p: PhantomData,
>  })
> diff --git a/rust/kernel/drm/gem/mod.rs b/rust/kernel/drm/gem/mod.rs
> new file mode 100644
> index ..8a7d99613718
> --- /dev/null
> +++ b/rust/kernel/drm/gem/mod.rs
> @@ -0,0 +1,374 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +
> +//! DRM GEM API
> +//!
> +//! C header: 
> [`include/linux/drm/drm_gem.h`](../../../../include/linux/drm/drm_gem.h)
> +
> +use alloc::boxed::Box;
> +
> +use crate::{
> +bindings,
> +drm::{device, drv, file},
> +error::{to_result, Result},
> +prelude::*,
> +};
> +use core::{mem, mem::ManuallyDrop, ops::Deref, ops::DerefMut};
> +
> +/// GEM object functions, which must be implemented by drivers.
> +pub trait BaseDriverObject: Sync + Send + Sized {
> +/// Create a new driver data object for a GEM object of a given size.
> +fn new(dev: &device::Device, size: usize) -> Result;
> +
> +/// Open a new handle to an existing object, associated with a File.
> +fn open(
> +_obj: &<::Driver as drv::Driver>::Object,
> +_file: &file::File<<::Driver as 
> drv::Driver>::File>,
> +) -> Result {
> +Ok(())
> + 

Re: [PATCH] drm/bridge: ti-sn65dsi83: Do not generate HFP/HBP/HSA and EOT packet

[rfoss]

From: Robert Foss 

On Mon, 3 Apr 2023 21:02:42 +0200, Marek Vasut wrote:
> Do not generate the HS front and back porch gaps, the HSA gap and
> EOT packet, as per "SN65DSI83 datasheet SLLSEC1I - SEPTEMBER 2012
> - REVISED OCTOBER 2020", page 22, these packets are not required.
> This makes the TI SN65DSI83 bridge work with Samsung DSIM on i.MX8MN.
> 
> 

Applied, thanks!

[1/1] drm/bridge: ti-sn65dsi83: Do not generate HFP/HBP/HSA and EOT packet
  https://cgit.freedesktop.org/drm/drm-misc/commit/?id=ca161b259cc8



Rob

Re: [PATCH RFC 08/18] rust: dma_fence: Add DMA Fence abstraction

[Daniel Vetter]

On Tue, Mar 07, 2023 at 11:25:33PM +0900, Asahi Lina wrote:
> DMA fences are the internal synchronization primitive used for DMA
> operations like GPU rendering, video en/decoding, etc. Add an
> abstraction to allow Rust drivers to interact with this subsystem.
> 
> Note: This uses a raw spinlock living next to the fence, since we do
> not interact with it other than for initialization.
> TODO: Expose this to the user at some point with a safe abstraction.
> 
> Signed-off-by: Asahi Lina 
> ---
>  rust/bindings/bindings_helper.h |   2 +
>  rust/helpers.c  |  53 
>  rust/kernel/dma_fence.rs| 532 
> 

This should probably be in the dma-buf namespace like on the C side?
There's a pile of tightly coupled concepts that I expect we'll all need
sooner or later (dma-fence/buf/resv at least).

Also I guess same questions about separate files and MAINTAINER entries as
for the drm stuff.
-Daniel

>  rust/kernel/lib.rs  |   2 +
>  4 files changed, 589 insertions(+)
> 
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 9f152d373df8..705af292a5b4 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -14,6 +14,8 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
>  #include 
>  #include 
>  #include 
> diff --git a/rust/helpers.c b/rust/helpers.c
> index 388ff1100ea5..8e906a7a7d8a 100644
> --- a/rust/helpers.c
> +++ b/rust/helpers.c
> @@ -23,6 +23,8 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -30,6 +32,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -388,6 +391,56 @@ int rust_helper_sg_dma_len(const struct scatterlist *sg)
>  }
>  EXPORT_SYMBOL_GPL(rust_helper_sg_dma_len);
>  
> +void rust_helper___spin_lock_init(spinlock_t *lock, const char *name,
> +   struct lock_class_key *key)
> +{
> +#ifdef CONFIG_DEBUG_SPINLOCK
> +# ifndef CONFIG_PREEMPT_RT
> + __raw_spin_lock_init(spinlock_check(lock), name, key, LD_WAIT_CONFIG);
> +# else
> + rt_mutex_base_init(&lock->lock);
> + __rt_spin_lock_init(lock, name, key, false);
> +# endif
> +#else
> + spin_lock_init(lock);
> +#endif
> +}
> +EXPORT_SYMBOL_GPL(rust_helper___spin_lock_init);
> +
> +#ifdef CONFIG_DMA_SHARED_BUFFER
> +
> +void rust_helper_dma_fence_get(struct dma_fence *fence)
> +{
> + dma_fence_get(fence);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_dma_fence_get);
> +
> +void rust_helper_dma_fence_put(struct dma_fence *fence)
> +{
> + dma_fence_put(fence);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_dma_fence_put);
> +
> +struct dma_fence_chain *rust_helper_dma_fence_chain_alloc(void)
> +{
> + return dma_fence_chain_alloc();
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_dma_fence_chain_alloc);
> +
> +void rust_helper_dma_fence_chain_free(struct dma_fence_chain *chain)
> +{
> + dma_fence_chain_free(chain);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_dma_fence_chain_free);
> +
> +void rust_helper_dma_fence_set_error(struct dma_fence *fence, int error)
> +{
> + dma_fence_set_error(fence, error);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_dma_fence_set_error);
> +
> +#endif
> +
>  #ifdef CONFIG_DRM
>  
>  void rust_helper_drm_gem_object_get(struct drm_gem_object *obj)
> diff --git a/rust/kernel/dma_fence.rs b/rust/kernel/dma_fence.rs
> new file mode 100644
> index ..ca93380d9da2
> --- /dev/null
> +++ b/rust/kernel/dma_fence.rs
> @@ -0,0 +1,532 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! DMA fence abstraction.
> +//!
> +//! C header: [`include/linux/dma_fence.h`](../../include/linux/dma_fence.h)
> +
> +use crate::{
> +bindings,
> +error::{to_result, Result},
> +prelude::*,
> +sync::LockClassKey,
> +types::Opaque,
> +};
> +use core::fmt::Write;
> +use core::ops::{Deref, DerefMut};
> +use core::ptr::addr_of_mut;
> +use core::sync::atomic::{AtomicU64, Ordering};
> +
> +/// Any kind of DMA Fence Object
> +///
> +/// # Invariants
> +/// raw() returns a valid pointer to a dma_fence and we own a reference to 
> it.
> +pub trait RawDmaFence: crate::private::Sealed {
> +/// Returns the raw `struct dma_fence` pointer.
> +fn raw(&self) -> *mut bindings::dma_fence;
> +
> +/// Returns the raw `struct dma_fence` pointer and consumes the object.
> +///
> +/// The caller is responsible for dropping the reference.
> +fn into_raw(self) -> *mut bindings::dma_fence
> +where
> +Self: Sized,
> +{
> +let ptr = self.raw();
> +core::mem::forget(self);
> +ptr
> +}
> +
> +/// Advances this fence to the chain node which will signal this 
> sequence number.
> +/// If no sequence number is provided, this returns `self` again.
> +fn chain_find_seqno(self, seqno: u64) -> Result
> +where
> +Self: Sized,
> +{
> +let mut ptr = self.into_raw();
> +

Re: [PATCH 1/8] drm/gma500: Use drm_aperture_remove_conflicting_pci_framebuffers

[Javier Martinez Canillas]

Thomas Zimmermann  writes:

[...]

>
> Your comment says that it calls a PCI function to clean up to vgacon. 
> That comment explains what is happening, not why. And how the PCI and 
> vgacon code work together is non-obvious.
>
> Again, here's my proposal for gma500:
>
> // call this from psb_pci_probe()
> int gma_remove_conflicting_framebuffers(struct pci_dev *pdev, const
>   struct drm_driver *req_driver)
> {
>   resource_size_t base = 0;
>   resource_size_t size = (resource_size_t)-1;
>   const char *name = req_driver->name;
>   int ret;
>
>   /*
>* We cannot yet easily find the framebuffer's location in
>* memory. So remove all framebuffers here.
>*
>* TODO: Refactor psb_driver_load() to map vdc_reg earlier. Then
>*   we might be able to read the framebuffer range from the
>*   device.
>*/
>   ret = aperture_remove_conflicting_devices(base, size, name);
>   if (ret)
>   return ret;
>
>   /*
>* WARNING: Apparently we must kick fbdev drivers before vgacon,
>* otherwise the vga fbdev driver falls over.
>*/
>   ret = vga_remove_vgacon(pdev);
>   if (ret)
>   return ret;
>
>   return 0;
> }
>

If this is enough I agree that is much more easier code to understand.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH RFC 04/18] rust: drm: gem: Add GEM object abstraction

[Miguel Ojeda]

On Wed, Apr 5, 2023 at 1:08 PM Daniel Vetter  wrote:
>
> Uh all the rust helper wrappers for all the kernel in a single file does
> not sound good. Can we not split these up into each subsystem, and then
> maybe instead of sprinkling #ifdef all over a .c file Make the compilation
> of that file conditional on rust support (plus whatever other Kconfig gate
> the other c files has already)?

Indeed, the plan is splitting the `kernel` crate and giving each
subsystem its own crate, bindings, helpers, etc.

Cheers,
Miguel

Re: [PATCH 1/2] drm/bridge: lt9211: Do not generate HFP/HBP/HSA and EOT packet

[Robert Foss]

On Tue, Apr 4, 2023 at 12:13 AM Marek Vasut  wrote:
>
> Do not generate the HS front and back porch gaps, the HSA gap and
> EOT packet, as these packets are not required. This makes the bridge
> work with Samsung DSIM on i.MX8MM and i.MX8MP.
>
> Signed-off-by: Marek Vasut 
> ---
> Cc: Andrzej Hajda 
> Cc: Daniel Vetter 
> Cc: David Airlie 
> Cc: Jagan Teki 
> Cc: Jernej Skrabec 
> Cc: Jonas Karlman 
> Cc: Laurent Pinchart 
> Cc: Michael Walle 
> Cc: Neil Armstrong 
> Cc: Robert Foss 
> Cc: dri-devel@lists.freedesktop.org
> ---
>  drivers/gpu/drm/bridge/lontium-lt9211.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/lontium-lt9211.c 
> b/drivers/gpu/drm/bridge/lontium-lt9211.c
> index 3e19fff6547a2..00db681512385 100644
> --- a/drivers/gpu/drm/bridge/lontium-lt9211.c
> +++ b/drivers/gpu/drm/bridge/lontium-lt9211.c
> @@ -709,7 +709,9 @@ static int lt9211_host_attach(struct lt9211 *ctx)
> dsi->lanes = dsi_lanes;
> dsi->format = MIPI_DSI_FMT_RGB888;
> dsi->mode_flags = MIPI_DSI_MODE_VIDEO | 
> MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
> - MIPI_DSI_MODE_VIDEO_HSE;
> + MIPI_DSI_MODE_VIDEO_HSE | 
> MIPI_DSI_MODE_VIDEO_NO_HSA |
> + MIPI_DSI_MODE_VIDEO_NO_HFP | 
> MIPI_DSI_MODE_VIDEO_NO_HBP |
> + MIPI_DSI_MODE_NO_EOT_PACKET;
>
> ret = devm_mipi_dsi_attach(dev, dsi);
> if (ret < 0) {
> --
> 2.39.2
>


Reviewed-by: Robert Foss 

Snoozing this patch for a week before applying.

Re: [PATCH 2/2] drm/bridge: lt9611: Do not generate HFP/HBP/HSA and EOT packet

[Robert Foss]

On Tue, Apr 4, 2023 at 12:13 AM Marek Vasut  wrote:
>
> Do not generate the HS front and back porch gaps, the HSA gap and
> EOT packet, as these packets are not required. This makes the bridge
> work with Samsung DSIM on i.MX8MM and i.MX8MP.
>
> Signed-off-by: Marek Vasut 
> ---
> Cc: Andrzej Hajda 
> Cc: Daniel Vetter 
> Cc: David Airlie 
> Cc: Jagan Teki 
> Cc: Jernej Skrabec 
> Cc: Jonas Karlman 
> Cc: Laurent Pinchart 
> Cc: Michael Walle 
> Cc: Neil Armstrong 
> Cc: Robert Foss 
> Cc: dri-devel@lists.freedesktop.org
> ---
>  drivers/gpu/drm/bridge/lontium-lt9611.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/lontium-lt9611.c 
> b/drivers/gpu/drm/bridge/lontium-lt9611.c
> index a25d21a7d5c19..151efe92711c4 100644
> --- a/drivers/gpu/drm/bridge/lontium-lt9611.c
> +++ b/drivers/gpu/drm/bridge/lontium-lt9611.c
> @@ -774,7 +774,9 @@ static struct mipi_dsi_device *lt9611_attach_dsi(struct 
> lt9611 *lt9611,
> dsi->lanes = 4;
> dsi->format = MIPI_DSI_FMT_RGB888;
> dsi->mode_flags = MIPI_DSI_MODE_VIDEO | 
> MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
> - MIPI_DSI_MODE_VIDEO_HSE;
> + MIPI_DSI_MODE_VIDEO_HSE | 
> MIPI_DSI_MODE_VIDEO_NO_HSA |
> + MIPI_DSI_MODE_VIDEO_NO_HFP | 
> MIPI_DSI_MODE_VIDEO_NO_HBP |
> + MIPI_DSI_MODE_NO_EOT_PACKET;
>
> ret = devm_mipi_dsi_attach(dev, dsi);
> if (ret < 0) {
> --
> 2.39.2
>

Reviewed-by: Robert Foss 

Snoozing this patch for a week before applying.

Re: [PATCH RFC 04/18] rust: drm: gem: Add GEM object abstraction

[Daniel Vetter]

On Wed, Apr 05, 2023 at 01:19:47PM +0200, Miguel Ojeda wrote:
> On Wed, Apr 5, 2023 at 1:08 PM Daniel Vetter  wrote:
> >
> > Uh all the rust helper wrappers for all the kernel in a single file does
> > not sound good. Can we not split these up into each subsystem, and then
> > maybe instead of sprinkling #ifdef all over a .c file Make the compilation
> > of that file conditional on rust support (plus whatever other Kconfig gate
> > the other c files has already)?
> 
> Indeed, the plan is splitting the `kernel` crate and giving each
> subsystem its own crate, bindings, helpers, etc.

Ok if this is just interim I think it's fine. Would still be good to have
the MAINTAINERS entry though even just to cover the interim state. Least
because I'm assuming that when things are split up you'd still want to
keep the rust list on cc for the rust parts, even when they move into
subsystems?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH RFC 03/18] rust: drm: file: Add File abstraction

[Daniel Vetter]

On Mon, Mar 13, 2023 at 07:07:09PM -0700, Boqun Feng wrote:
> On Mon, Mar 13, 2023 at 12:49:57PM -0500, Faith Ekstrand wrote:
> > On Fri, 2023-03-10 at 07:16 +0900, Asahi Lina wrote:
> > > On 10/03/2023 06.16, Faith Ekstrand wrote:
> > > > On Tue, 2023-03-07 at 23:25 +0900, Asahi Lina wrote:
> > > > > A DRM File is the DRM counterpart to a kernel file structure,
> > > > > representing an open DRM file descriptor. Add a Rust abstraction
> > > > > to
> > > > > allow drivers to implement their own File types that implement
> > > > > the
> > > > > DriverFile trait.
> > > > > 
> > > > > Signed-off-by: Asahi Lina 
> > > > > ---
> > > > >  rust/bindings/bindings_helper.h |   1 +
> > > > >  rust/kernel/drm/drv.rs  |   7 ++-
> > > > >  rust/kernel/drm/file.rs | 113
> > > > > 
> > > > >  rust/kernel/drm/mod.rs  |   1 +
> > > > >  4 files changed, 120 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/rust/bindings/bindings_helper.h
> > > > > b/rust/bindings/bindings_helper.h
> > > > > index 2a999138c4ae..7d7828faf89c 100644
> > > > > --- a/rust/bindings/bindings_helper.h
> > > > > +++ b/rust/bindings/bindings_helper.h
> > > > > @@ -8,6 +8,7 @@
> > > > >  
> > > > >  #include 
> > > > >  #include 
> > > > > +#include 
> > > > >  #include 
> > > > >  #include 
> > > > >  #include 
> > > > > diff --git a/rust/kernel/drm/drv.rs b/rust/kernel/drm/drv.rs
> > > > > index 29a465515dc9..1dcb651e1417 100644
> > > > > --- a/rust/kernel/drm/drv.rs
> > > > > +++ b/rust/kernel/drm/drv.rs
> > > > > @@ -144,6 +144,9 @@ pub trait Driver {
> > > > >  /// Should be either `drm::gem::Object` or
> > > > > `drm::gem::shmem::Object`.
> > > > >  type Object: AllocImpl;
> > > > >  
> > > > > +    /// The type used to represent a DRM File (client)
> > > > > +    type File: drm::file::DriverFile;
> > > > > +
> > > > >  /// Driver metadata
> > > > >  const INFO: DriverInfo;
> > > > >  
> > > > > @@ -213,8 +216,8 @@ macro_rules! drm_device_register {
> > > > >  impl Registration {
> > > > >  const VTABLE: bindings::drm_driver = drm_legacy_fields! {
> > > > >  load: None,
> > > > > -    open: None, // TODO: File abstraction
> > > > > -    postclose: None, // TODO: File abstraction
> > > > > +    open: Some(drm::file::open_callback::),
> > > > > +    postclose:
> > > > > Some(drm::file::postclose_callback::),
> > > > >  lastclose: None,
> > > > >  unload: None,
> > > > >  release: None,
> > > > > diff --git a/rust/kernel/drm/file.rs b/rust/kernel/drm/file.rs
> > > > > new file mode 100644
> > > > > index ..48751e93c38a
> > > > > --- /dev/null
> > > > > +++ b/rust/kernel/drm/file.rs
> > > > > @@ -0,0 +1,113 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0 OR MIT
> > > > > +
> > > > > +//! DRM File objects.
> > > > > +//!
> > > > > +//! C header:
> > > > > [`include/linux/drm/drm_file.h`](../../../../include/linux/drm/dr
> > > > > m_fi
> > > > > le.h)
> > > > > +
> > > > > +use crate::{bindings, drm, error::Result};
> > > > > +use alloc::boxed::Box;
> > > > > +use core::marker::PhantomData;
> > > > > +use core::ops::Deref;
> > > > > +
> > > > > +/// Trait that must be implemented by DRM drivers to represent a
> > > > > DRM
> > > > > File (a client instance).
> > > > > +pub trait DriverFile {
> > > > > +    /// The parent `Driver` implementation for this
> > > > > `DriverFile`.
> > > > > +    type Driver: drm::drv::Driver;
> > > > > +
> > > > > +    /// Open a new file (called when a client opens the DRM
> > > > > device).
> > > > > +    fn open(device: &drm::device::Device) ->
> > > > > Result>;
> > > > > +}
> > > > > +
> > > > > +/// An open DRM File.
> > > > > +///
> > > > > +/// # Invariants
> > > > > +/// `raw` is a valid pointer to a `drm_file` struct.
> > > > > +#[repr(transparent)]
> > > > > +pub struct File {
> > > > > +    raw: *mut bindings::drm_file,
> > > > > +    _p: PhantomData,
> > > > > +}
> > > > > +
> > > > > +pub(super) unsafe extern "C" fn open_callback(
> > > > > +    raw_dev: *mut bindings::drm_device,
> > > > > +    raw_file: *mut bindings::drm_file,
> > > > > +) -> core::ffi::c_int {
> > > > > +    let drm = core::mem::ManuallyDrop::new(unsafe {
> > > > > drm::device::Device::from_raw(raw_dev) });
> > > > 
> > > > Maybe you can help educate me a bit here... This feels like a
> > > > really
> > > > sketchy pattern.  We're creating a Device from a pointer, an
> > > > operation
> > > > which inherently consumes a reference but then marking it
> > > > ManuallyDrop
> > > > so drm_device_put() never gets called.  It took me a while but I
> > > > think
> > > > I figured out what you're trying to do: Make it so all the Rust
> > > > stuff
> > > > works with Device, not drm_device but it still feels really wrong. 
> > > > It
> > > > works, it just feels like there's a lot of unsafe abstraction
> > > > juggling
> > > > happening here and I expect this operation is going to be pret

Re: [PATCH v5 0/8] QAIC accel driver

[Jacek Lawrynowicz]

Hi,

On 03.04.2023 19:22, Jeffrey Hugo wrote:
> On 3/27/2023 9:54 AM, Jeffrey Hugo wrote:
>> This series introduces a driver under the accel subsystem (QAIC -
>> Qualcomm AIC) for the Qualcomm Cloud AI 100 product (AIC100).  AIC100 is
>> a PCIe adapter card that hosts a dedicated machine learning inference
>> accelerator.
>>
>> The previous version (v4) can be found at:
>> https://lore.kernel.org/all/1679325074-5494-1-git-send-email-quic_jh...@quicinc.com/
> 
> Looks like things have been silent on this revision and we have a number of 
> review tags already.  Seems like this series is ready for merge.
> 
> I'd like to see this queued for 6.4 if possible.  Given that we are at 
> 6.3-rc5, it seems like this would need to be merged now(ish) to make 6.4.
> 
> Jacek, since you have commit permissions in drm-misc and are an active Accel 
> maintainer, I wonder if it would be appropriate for you to merge this series 
> to drm-misc.  Thoughts?

I'm would be happy to merge it but I think it needs to be acked by Oded first.

Regards,
Jacek

Re: [PATCH 2/8] video/aperture: use generic code to figure out the vga default device

[Javier Martinez Canillas]

Daniel Vetter  writes:

> Since vgaarb has been promoted to be a core piece of the pci subsystem
> we don't have to open code random guesses anymore, we actually know
> this in a platform agnostic way, and there's no need for an x86
> specific hack. See also 1d38fe6ee6a8 ("PCI/VGA: Move vgaarb to
> drivers/pci")
>
> This should not result in any functional change, and the non-x86
> multi-gpu pci systems are probably rare enough to not matter (I don't
> know of any tbh). But it's a nice cleanup, so let's do it.
>
> There's been a few questions on previous iterations on dri-devel and
> irc:
>
> - fb_is_primary_device() seems to be yet another implementation of
>   this theme, and at least on x86 it checks for both
>   vga_default_device OR rom shadowing. There shouldn't ever be a case
>   where rom shadowing gives any additional hints about the boot vga
>   device, but if there is then the default vga selection in vgaarb
>   should probably be fixed. And not special-case checks replicated all
>   over.
>

Agreed and if there are regressions reported then could be added there.

> - Thomas also brought up that on most !x86 systems
>   fb_is_primary_device() returns 0, except on sparc/parisc. But these
>   2 special cases are about platform specific devices and not pci, so
>   shouldn't have any interactions.
>
> - Furthermore fb_is_primary_device() is a bit a red herring since it's
>   only used to select the right fbdev driver for fbcon, and not for
>   the fw handover dance which the aperture helpers handle. At least
>   for x86 we might want to look into unifying them, but that's a
>   separate thing.
>
> v2: Extend commit message trying to summarize various discussions.
>
> Signed-off-by: Daniel Vetter 
> Cc: Thomas Zimmermann 
> Cc: Javier Martinez Canillas 
> Cc: Helge Deller 
> Cc: linux-fb...@vger.kernel.org
> Cc: Bjorn Helgaas 
> Cc: linux-...@vger.kernel.org
> ---
>  drivers/video/aperture.c | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c
> index b009468ffdff..8835d3bc39bf 100644
> --- a/drivers/video/aperture.c
> +++ b/drivers/video/aperture.c
> @@ -324,13 +324,11 @@ EXPORT_SYMBOL(aperture_remove_conflicting_devices);
>   */
>  int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev, const char 
> *name)
>  {
> - bool primary = false;
> + bool primary;
>   resource_size_t base, size;
>   int bar, ret;
>  
> -#ifdef CONFIG_X86
> - primary = pdev->resource[PCI_ROM_RESOURCE].flags & 
> IORESOURCE_ROM_SHADOW;
> -#endif
> + primary = pdev == vga_default_device();
>

Maybe enclose the check in parenthesis to make it easier to read ?

Reviewed-by: Javier Martinez Canillas 

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 3/8] drm/aperture: Remove primary argument

[Javier Martinez Canillas]

Daniel Vetter  writes:

> Only really pci devices have a business setting this - it's for
> figuring out whether the legacy vga stuff should be nuked too. And
> with the preceeding two patches those are all using the pci version of
> this.
>
> Which means for all other callers primary == false and we can remove
> it now.
>
> v2:
> - Reorder to avoid compile fail (Thomas)
> - Include gma500, which retained it's called to the non-pci version.
>
> Signed-off-by: Daniel Vetter 

Reviewed-by: Javier Martinez Canillas 

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 4/8] video/aperture: Only kick vgacon when the pdev is decoding vga

[Javier Martinez Canillas]

Daniel Vetter  writes:

> Otherwise it's bit silly, and we might throw out the driver for the
> screen the user is actually looking at. I haven't found a bug report
> for this case yet, but we did get bug reports for the analog case
> where we're throwing out the efifb driver.
>
> v2: Flip the check around to make it clear it's a special case for
> kicking out the vgacon driver only (Thomas)
>
> References: https://bugzilla.kernel.org/show_bug.cgi?id=216303
> Signed-off-by: Daniel Vetter 
> Cc: Thomas Zimmermann 
> Cc: Javier Martinez Canillas 
> Cc: Helge Deller 
> Cc: linux-fb...@vger.kernel.org
> ---
>  drivers/video/aperture.c | 16 +---
>  1 file changed, 9 insertions(+), 7 deletions(-)

Reviewed-by: Javier Martinez Canillas 

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 5/8] video/aperture: Move vga handling to pci function

[Javier Martinez Canillas]

Daniel Vetter  writes:

> A few reasons for this:
>
> - It's really the only one where this matters. I tried looking around,
>   and I didn't find any non-pci vga-compatible controllers for x86
>   (since that's the only platform where we had this until a few
>   patches ago), where a driver participating in the aperture claim
>   dance would interfere.
>
> - I also don't expect that any future bus anytime soon will
>   not just look like pci towards the OS, that's been the case for like
>   25+ years by now for practically everything (even non non-x86).
>
> - Also it's a bit funny if we have one part of the vga removal in the
>   pci function, and the other in the generic one.
>
> v2: Rebase.
>
> Signed-off-by: Daniel Vetter 
> Cc: Thomas Zimmermann 
> Cc: Javier Martinez Canillas 
> Cc: Helge Deller 
> Cc: linux-fb...@vger.kernel.org
> ---
>  drivers/video/aperture.c | 15 +++
>  1 file changed, 7 insertions(+), 8 deletions(-)

Reviewed-by: Javier Martinez Canillas 

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 6/8] video/aperture: Drop primary argument

[Javier Martinez Canillas]

Daniel Vetter  writes:

> With the preceeding patches it's become defunct. Also I'm about to add
> a different boolean argument, so it's better to keep the confusion
> down to the absolute minimum.
>
> v2: Since the hypervfb patch got droppped (it's only a pci device for
> gen1 vm, not for gen2) there is one leftover user in an actual driver
> left to touch.
>
> Signed-off-by: Daniel Vetter 
> Cc: Thomas Zimmermann 
> Cc: Javier Martinez Canillas 
> Cc: Helge Deller 
> Cc: linux-fb...@vger.kernel.org
> Cc: Maarten Lankhorst 
> Cc: Maxime Ripard 
> Cc: "K. Y. Srinivasan" 
> Cc: Haiyang Zhang 
> Cc: Wei Liu 
> Cc: Dexuan Cui 
> Cc: linux-hyp...@vger.kernel.org
> ---
>  drivers/gpu/drm/drm_aperture.c  | 2 +-
>  drivers/video/aperture.c| 7 +++
>  drivers/video/fbdev/hyperv_fb.c | 2 +-
>  include/linux/aperture.h| 9 -
>  4 files changed, 9 insertions(+), 11 deletions(-)
>

Reviewed-by: Javier Martinez Canillas 

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 7/8] video/aperture: Only remove sysfb on the default vga pci device

[Javier Martinez Canillas]

Daniel Vetter  writes:

> Instead of calling aperture_remove_conflicting_devices() to remove the
> conflicting devices, just call to aperture_detach_devices() to detach
> the device that matches the same PCI BAR / aperture range. Since the
> former is just a wrapper of the latter plus a sysfb_disable() call,
> and now that's done in this function but only for the primary devices.
>
> This fixes a regression introduced by ee7a69aa38d8 ("fbdev: Disable
> sysfb device registration when removing conflicting FBs"), where we
> remove the sysfb when loading a driver for an unrelated pci device,
> resulting in the user loosing their efifb console or similar.
>
> Note that in practice this only is a problem with the nvidia blob,
> because that's the only gpu driver people might install which does not
> come with an fbdev driver of it's own. For everyone else the real gpu
> driver will restore a working console.
>
> Also note that in the referenced bug there's confusion that this same
> bug also happens on amdgpu. But that was just another amdgpu specific
> regression, which just happened to happen at roughly the same time and
> with the same user-observable symptoms. That bug is fixed now, see
> https://bugzilla.kernel.org/show_bug.cgi?id=216331#c15
>
> Note that we should not have any such issues on non-pci multi-gpu
> issues, because I could only find two such cases:
> - SoC with some external panel over spi or similar. These panel
>   drivers do not use drm_aperture_remove_conflicting_framebuffers(),
>   so no problem.
> - vga+mga, which is a direct console driver and entirely bypasses all
>   this.
>
> For the above reasons the cc: stable is just notionally, this patch
> will need a backport and that's up to nvidia if they care enough.
>
> v2:
> - Explain a bit better why other multi-gpu that aren't pci shouldn't
>   have any issues with making all this fully pci specific.
>
> v3
> - polish commit message (Javier)
>
> Fixes: ee7a69aa38d8 ("fbdev: Disable sysfb device registration when removing 
> conflicting FBs")
> Tested-by: Aaron Plattner 
> Reviewed-by: Javier Martinez Canillas 
> References: https://bugzilla.kernel.org/show_bug.cgi?id=216303#c28
> Signed-off-by: Daniel Vetter 
> Cc: Aaron Plattner 
> Cc: Javier Martinez Canillas 
> Cc: Thomas Zimmermann 
> Cc: Helge Deller 
> Cc: Sam Ravnborg 
> Cc: Alex Deucher 
> Cc:  # v5.19+ (if someone else does the backport)
> ---
>  drivers/video/aperture.c | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)

Reviewed-by: Javier Martinez Canillas 

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v1 0/3] Fixes for PSR

[Dmitry Baryshkov]

On Fri, 31 Mar 2023 19:28:31 +0530, Vinod Polimera wrote:
> while in virtual terminal with PSR enabled, there will be
> no atomic commits triggered resulting in no screen update.
> Update the dirtyfb flag into plane state during atomic check
> to flush the pixel data explicitly.
> 
> Avoid scheduling PSR commits from different work queues while
> running in PSR mode already.
> 
> [...]

Applied, thanks!

[1/3] drm/msm/dpu: set dirty_fb flag while in self refresh mode
  https://gitlab.freedesktop.org/lumag/msm/-/commit/ddf5387d1fb7
[2/3] msm/disp/dpu: allow atomic_check in PSR usecase
  https://gitlab.freedesktop.org/lumag/msm/-/commit/86c56ba51aec

Note, patch 3, which solves a different issue (and which requires additional 
changes) was not applied.

Best regards,
-- 
Dmitry Baryshkov 

Re: [PATCH 8/8] fbdev: Simplify fb_is_primary_device for x86

[Javier Martinez Canillas]

Daniel Vetter  writes:

> vga_default_device really is supposed to cover all corners, at least
> for x86. Additionally checking for rom shadowing should be redundant,
> because the bios/fw only does that for the boot vga device.
>
> If this turns out to be wrong, then most likely that's a special case
> which should be added to the vgaarb code, not replicated all over.
>
> Patch motived by changes to the aperture helpers, which also have this
> open code in a bunch of places, and which are all removed in a
> clean-up series. This function here is just for selecting the default
> fbdev device for fbcon, but I figured for consistency it might be good
> to throw this patch in on top.
>
> Note that the shadow rom check predates vgaarb, which was only wired
> up in 88674088d10c ("x86: Use vga_default_device() when determining
> whether an fb is primary"). That patch doesn't explain why we still
> fall back to the shadow rom check.
>
> Signed-off-by: Daniel Vetter 
> Cc: Daniel Vetter 
> Cc: Helge Deller 
> Cc: Daniel Vetter 
> Cc: Javier Martinez Canillas 
> Cc: Thomas Zimmermann 
> Cc: Thomas Gleixner 
> Cc: Ingo Molnar 
> Cc: Borislav Petkov 
> Cc: Dave Hansen 
> Cc: x...@kernel.org
> Cc: "H. Peter Anvin" 
> ---
>  arch/x86/video/fbdev.c | 13 +

[...]

> + if (pci_dev == vga_default_device())
>   return 1;
>   return 0;

or just return pci_dev == vga_default_device() ;

Reviewed-by: Javier Martinez Canillas 

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 1/5] drm/i915/ttm: Add I915_BO_PREALLOC

[Andi Shyti]

Hi Nirmoy,

> > > Add a mechanism to keep existing data when creating
> > > a ttm object with I915_BO_ALLOC_USER flag.
> > why do we need this mechanism? What was the logic behind? These
> > are all questions people might have when checking this commit.
> > Please be a bit more explicative.
> 
> 
> Agree, the commit message is bit short. I will add more content in next
> revision.

you don't need to send a new version just for this commit log.

You could just propose a new commit log in the reply and if it's
OK, add it before pushing it.

As you wish.

Andi

> > 
> > > Cc: Matthew Auld 
> > > Cc: Andi Shyti 
> > > Cc: Andrzej Hajda 
> > > Cc: Ville Syrjälä 
> > > Cc: Jani Nikula 
> > > Cc: Imre Deak 
> > > Signed-off-by: Nirmoy Das 
> > Reviewed-by: Andi Shyti 
> 
> 
> Thanks,
> 
> Nirmoy
> 
> > 
> > Thanks,
> > Andi

Re: [PATCH] drm/atomic-helper: Don't set deadline for modesets

[Ville Syrjälä]

On Wed, Apr 05, 2023 at 10:16:50AM +0200, Daniel Vetter wrote:
> If the crtc is being switched on or off then the semantics of
> computing the timestampe of the next vblank is somewhat ill-defined.
> And indeed, the code splats with a warning in the timestamp
> computation code. Specifically it hits the check to make sure that
> atomic drivers have full set up the timing constants in the drm_vblank
> structure, and that's just not the case before the crtc is actually
> on.
> 
> For robustness it seems best to just not set deadlines for modesets.
> 
> Link: 
> https://lore.kernel.org/dri-devel/dfc21f18-7e1e-48f0-c05a-d659b9c90...@linaro.org/
> Fixes: d39e48ca80c0 ("drm/atomic-helper: Set fence deadline for vblank")
> Cc: Rob Clark 
> Cc: Daniel Vetter 
> Cc: Maarten Lankhorst 
> Cc: Maxime Ripard 
> Cc: Thomas Zimmermann 
> Reported-by: Dmitry Baryshkov 
> Tested-by: Dmitry Baryshkov  # test patch only
> Cc: Dmitry Baryshkov 
> Signed-off-by: Daniel Vetter 
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> b/drivers/gpu/drm/drm_atomic_helper.c
> index f21b5a74176c..6640d80d84f3 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1528,6 +1528,9 @@ static void set_fence_deadline(struct drm_device *dev,
>   for_each_new_crtc_in_state (state, crtc, new_crtc_state, i) {
>   ktime_t v;
>  
> + if (drm_atomic_crtc_needs_modeset(new_crtc_state))
> + continue;

Should this stuff also be skipped when !new_crtc_state->active?
I didn't actually check what drm_crtc_next_vblank_start() ends
up doing in that case.

> +
>   if (drm_crtc_next_vblank_start(crtc, &v))
>   continue;
>  
> -- 
> 2.40.0

-- 
Ville Syrjälä
Intel

[PATCH v3 63/65] ASoC: tlv320aic32x4: pll: Switch to determine_rate

[Maxime Ripard]

The tlv320aic32x4 PLL clocks implements a mux with a set_parent hook, but
doesn't provide a determine_rate implementation.

This is a bit odd, since set_parent() is there to, as its name implies,
change the parent of a clock. However, the most likely candidate to
trigger that parent change is a call to clk_set_rate(), with
determine_rate() figuring out which parent is the best suited for a
given rate.

The other trigger would be a call to clk_set_parent(), but it's far less
used, and it doesn't look like there's any obvious user for that clock.

So, the set_parent hook is effectively unused, possibly because of an
oversight. However, it could also be an explicit decision by the
original author to avoid any reparenting but through an explicit call to
clk_set_parent().

The driver does implement round_rate() though, which means that we can
change the rate of the clock, but we will never get to change the
parent.

However, It's hard to tell whether it's been done on purpose or not.

Since we'll start mandating a determine_rate() implementation, let's
convert the round_rate() implementation to a determine_rate(), which
will also make the current behavior explicit. And if it was an
oversight, the clock behaviour can be adjusted later on.

Signed-off-by: Maxime Ripard 
---
 sound/soc/codecs/tlv320aic32x4-clk.c | 19 ---
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/sound/soc/codecs/tlv320aic32x4-clk.c 
b/sound/soc/codecs/tlv320aic32x4-clk.c
index 65b72373cb95..d8b8ea3eaa12 100644
--- a/sound/soc/codecs/tlv320aic32x4-clk.c
+++ b/sound/soc/codecs/tlv320aic32x4-clk.c
@@ -205,18 +205,23 @@ static unsigned long clk_aic32x4_pll_recalc_rate(struct 
clk_hw *hw,
return clk_aic32x4_pll_calc_rate(&settings, parent_rate);
 }
 
-static long clk_aic32x4_pll_round_rate(struct clk_hw *hw,
-   unsigned long rate,
-   unsigned long *parent_rate)
+static int clk_aic32x4_pll_determine_rate(struct clk_hw *hw,
+ struct clk_rate_request *req)
 {
struct clk_aic32x4_pll_muldiv settings;
+   unsigned long rate;
int ret;
 
-   ret = clk_aic32x4_pll_calc_muldiv(&settings, rate, *parent_rate);
+   ret = clk_aic32x4_pll_calc_muldiv(&settings, req->rate, 
req->best_parent_rate);
if (ret < 0)
-   return 0;
+   return -EINVAL;
 
-   return clk_aic32x4_pll_calc_rate(&settings, *parent_rate);
+   rate = clk_aic32x4_pll_calc_rate(&settings, req->best_parent_rate);
+   if (rate < 0)
+   return rate;
+
+   req->rate = rate;
+   return 0;
 }
 
 static int clk_aic32x4_pll_set_rate(struct clk_hw *hw,
@@ -267,7 +272,7 @@ static const struct clk_ops aic32x4_pll_ops = {
.unprepare = clk_aic32x4_pll_unprepare,
.is_prepared = clk_aic32x4_pll_is_prepared,
.recalc_rate = clk_aic32x4_pll_recalc_rate,
-   .round_rate = clk_aic32x4_pll_round_rate,
+   .determine_rate = clk_aic32x4_pll_determine_rate,
.set_rate = clk_aic32x4_pll_set_rate,
.set_parent = clk_aic32x4_pll_set_parent,
.get_parent = clk_aic32x4_pll_get_parent,

-- 
2.39.2

[PATCH v3 64/65] ASoC: tlv320aic32x4: div: Switch to determine_rate

[Maxime Ripard]

The tlv320aic32x4 divider clocks implements a mux with a set_parent
hook, but doesn't provide a determine_rate implementation.

This is a bit odd, since set_parent() is there to, as its name implies,
change the parent of a clock. However, the most likely candidate to
trigger that parent change is a call to clk_set_rate(), with
determine_rate() figuring out which parent is the best suited for a
given rate.

The other trigger would be a call to clk_set_parent(), but it's far less
used, and it doesn't look like there's any obvious user for that clock.

So, the set_parent hook is effectively unused, possibly because of an
oversight. However, it could also be an explicit decision by the
original author to avoid any reparenting but through an explicit call to
clk_set_parent().

The driver does implement round_rate() though, which means that we can
change the rate of the clock, but we will never get to change the
parent.

However, It's hard to tell whether it's been done on purpose or not.

Since we'll start mandating a determine_rate() implementation, let's
convert the round_rate() implementation to a determine_rate(), which
will also make the current behavior explicit. And if it was an
oversight, the clock behaviour can be adjusted later on.

Signed-off-by: Maxime Ripard 
---
 sound/soc/codecs/tlv320aic32x4-clk.c | 13 +++--
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/sound/soc/codecs/tlv320aic32x4-clk.c 
b/sound/soc/codecs/tlv320aic32x4-clk.c
index d8b8ea3eaa12..707c9951fac0 100644
--- a/sound/soc/codecs/tlv320aic32x4-clk.c
+++ b/sound/soc/codecs/tlv320aic32x4-clk.c
@@ -333,16 +333,17 @@ static int clk_aic32x4_div_set_rate(struct clk_hw *hw, 
unsigned long rate,
AIC32X4_DIV_MASK, divisor);
 }
 
-static long clk_aic32x4_div_round_rate(struct clk_hw *hw, unsigned long rate,
-   unsigned long *parent_rate)
+static int clk_aic32x4_div_determine_rate(struct clk_hw *hw,
+ struct clk_rate_request *req)
 {
unsigned long divisor;
 
-   divisor = DIV_ROUND_UP(*parent_rate, rate);
+   divisor = DIV_ROUND_UP(req->best_parent_rate, req->rate);
if (divisor > 128)
return -EINVAL;
 
-   return DIV_ROUND_UP(*parent_rate, divisor);
+   req->rate = DIV_ROUND_UP(req->best_parent_rate, divisor);
+   return 0;
 }
 
 static unsigned long clk_aic32x4_div_recalc_rate(struct clk_hw *hw,
@@ -361,7 +362,7 @@ static const struct clk_ops aic32x4_div_ops = {
.prepare = clk_aic32x4_div_prepare,
.unprepare = clk_aic32x4_div_unprepare,
.set_rate = clk_aic32x4_div_set_rate,
-   .round_rate = clk_aic32x4_div_round_rate,
+   .determine_rate = clk_aic32x4_div_determine_rate,
.recalc_rate = clk_aic32x4_div_recalc_rate,
 };
 
@@ -389,7 +390,7 @@ static const struct clk_ops aic32x4_bdiv_ops = {
.set_parent = clk_aic32x4_bdiv_set_parent,
.get_parent = clk_aic32x4_bdiv_get_parent,
.set_rate = clk_aic32x4_div_set_rate,
-   .round_rate = clk_aic32x4_div_round_rate,
+   .determine_rate = clk_aic32x4_div_determine_rate,
.recalc_rate = clk_aic32x4_div_recalc_rate,
 };
 

-- 
2.39.2

[PATCH v3 65/65] clk: Forbid to register a mux without determine_rate

[Maxime Ripard]

The determine_rate hook allows to select the proper parent and its rate
for a given clock configuration. On another hand, set_parent is there to
change the parent of a mux.

Some clocks provide a set_parent hook but don't implement
determine_rate. In such a case, set_parent is pretty much useless since
the clock framework will always assume the current parent is to be used,
and we will thus never change it.

This situation can be solved in two ways:
  - either we don't need to change the parent, and we thus shouldn't
implement set_parent;
  - or we don't want to change the parent, in this case we should set
CLK_SET_RATE_NO_REPARENT;
  - or we're missing a determine_rate implementation.

The latter is probably just an oversight from the driver's author, and
we should thus raise their awareness about the fact that the current
state of the driver is confusing.

All the drivers in-tree have been converted by now, so let's prevent any
clock with set_parent but without determine_rate to register so that it
can't sneak in again in the future.

Reviewed-by: AngeloGioacchino Del Regno 

Signed-off-by: Maxime Ripard 
---
 drivers/clk/clk.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index f9fc8730ed17..4c1f28fb8c1f 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3746,6 +3746,13 @@ static int __clk_core_init(struct clk_core *core)
goto out;
}
 
+   if (core->ops->set_parent && !core->ops->determine_rate) {
+   pr_err("%s: %s must implement .set_parent & .determine_rate\n",
+   __func__, core->name);
+   ret = -EINVAL;
+   goto out;
+   }
+
if (core->num_parents > 1 && !core->ops->get_parent) {
pr_err("%s: %s must implement .get_parent as it has multi 
parents\n",
   __func__, core->name);

-- 
2.39.2

Re: [PATCH] drm: bridge: ldb: add support for using channel 1 only

[Marek Vasut]

On 4/5/23 09:30, Luca Ceresoli wrote:

[...]


@@ -311,10 +314,23 @@ static int fsl_ldb_probe(struct platform_device *pdev)
if (IS_ERR(fsl_ldb->regmap))
return PTR_ERR(fsl_ldb->regmap);
   
-	/* Locate the panel DT node. */

-   panel_node = of_graph_get_remote_node(dev->of_node, 1, 0);
-   if (!panel_node)
-   return -ENXIO;
+   /* Locate the remote ports and the panel node */
+   remote1 = of_graph_get_remote_node(dev->of_node, 1, 0);
+   remote2 = of_graph_get_remote_node(dev->of_node, 2, 0);
+   fsl_ldb->ch0_enabled = (remote1 != NULL);
+   fsl_ldb->ch1_enabled = (remote2 != NULL);
+   panel_node = of_node_get(remote1 ? remote1 : remote2);


You can even do this without the middle 'remote1' I think:

panel_node = of_node_get(remote1 ? : remote2);


Apparently, but honestly with such short expressions clearly having no
side effects I think it's not helping readability.


I think even the ternary operator itself isn't helpful much, but that's 
a matter of taste, and I don't have a better suggestion which would 
improve the readability either (I tried to expand it into if()... but 
that looks bad too).


No need to change anything.

[...]

Thanks for the patch.

Re: [PATCH RESEND v3 0/3] drm/ttm: Small fixes / cleanups in prep for shrinking

[Christian König]

Am 04.04.23 um 22:06 schrieb Thomas Hellström:

I collected the, from my POW, uncontroversial patches from V1 of the TTM
shrinker series, some corrected after the initial patch submission, one
patch added from the Xe RFC ("drm/ttm: Don't print error message if
eviction was interrupted"). It would be nice to have these reviewed and
merged while reworking the rest.

v2:
- Simplify __ttm_pool_free().
- Fix the TTM_TT_FLAG bit numbers.
- Keep all allocation orders for TTM pages at or below PMD order

v3:
- Rename __tm_pool_free() to ttm_pool_free_range(). Document.
- Compile-fix.


Reviewed-by: Christian König  for the series.



Thomas Hellström (3):
   drm/ttm/pool: Fix ttm_pool_alloc error path
   drm/ttm: Reduce the number of used allocation orders for TTM pages
   drm/ttm: Make the call to ttm_tt_populate() interruptible when
 faulting

  drivers/gpu/drm/ttm/ttm_bo_vm.c |  13 +++-
  drivers/gpu/drm/ttm/ttm_pool.c  | 111 
  2 files changed, 80 insertions(+), 44 deletions(-)

Re: [PATCH RFC 04/18] rust: drm: gem: Add GEM object abstraction

[Miguel Ojeda]

On Wed, Apr 5, 2023 at 1:23 PM Daniel Vetter  wrote:
>
> Ok if this is just interim I think it's fine. Would still be good to have
> the MAINTAINERS entry though even just to cover the interim state. Least
> because I'm assuming that when things are split up you'd still want to
> keep the rust list on cc for the rust parts, even when they move into
> subsystems?

Sorry, I missed to reply the second part of your email -- replying here.

Currently, the subsystem's code is under `rust/` (though modules can
go already into other folders). One of the reasons was technical
simplicity, and a nice side effect is that we could bootstrap things
while getting C maintainers involved over time.

To accomplish that, the guidelines for contributing Rust code are that
the respective maintainers need to be at least Cc'd, even if the files
do not hit the `F:` fields for the time being -- see [1]. But, for us,
ideally, the maintainers will take the changes through their tree,
instead of going through the Rust one, since that is the end goal.

And, of course, if you already want to have `F:` fields for the Rust
code, that is even better! (Whether those should be in the same entry
or in a new one, it is up to you, of course, and whether it is a
different set of people / level of support / etc.)

Then, when the `kernel` crate split happens, we can move the code
directly under whatever folders it should be naturally, when their
maintainers are ready. For some subsystems, that may mean they do not
need any `F:` fields since they are already covered (e.g. if they did
not create a new entry for Rust code only). And for cases like yours,
where you already had `F:` fields, it means the move of the files can
be done right away as soon as the split happens.

In short, we would definitely welcome if you add `F:` fields already
(whether in existing or new entries) -- it would mean you are ahead of
the curve! :)

As for the mailing list, yes, for the time being, I ask that all
changes to please be sent to the Rust list, so that everybody that
wants to follow the Rust progress has everything in a single place, so
that we try to remain consistent in the beginning on e.g. coding
guidelines, so that Rust reviewers can help spot mistakes, and so on
and so forth.

But, as Rust grows in the kernel, as systems become non-experimental,
and as maintainers take ownership of the code, that should eventually
go away and let things be as usual with C code. Then the Rust
subsystem (and its list) will become smaller, and it will be the
subsystem (and the discussion place) for anything not covered by other
subsystems, such as core Rust abstractions and types, Rust
infrastructure and so on.

How does that sound?

[1] https://rust-for-linux.com/contributing#the-rust-subsystem (I may
reorganize this to be Rust's `P:` field, by the way)

Cheers,
Miguel

Re: [PATCH RFC 09/18] rust: drm: syncobj: Add DRM Sync Object abstraction

[Daniel Vetter]

On Tue, Mar 07, 2023 at 11:25:34PM +0900, Asahi Lina wrote:
> DRM Sync Objects are a container for a DMA fence, and can be waited on
> signaled, exported, and imported from userspace. Add a Rust abstraction
> so Rust DRM drivers can support this functionality.
> 
> Signed-off-by: Asahi Lina 
> ---
>  rust/bindings/bindings_helper.h |  1 +
>  rust/helpers.c  | 19 ++
>  rust/kernel/drm/mod.rs  |  1 +
>  rust/kernel/drm/syncobj.rs  | 77 
> +
>  4 files changed, 98 insertions(+)
> 
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 705af292a5b4..b6696011f3a4 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -12,6 +12,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> diff --git a/rust/helpers.c b/rust/helpers.c
> index 8e906a7a7d8a..11965b1e2f4e 100644
> --- a/rust/helpers.c
> +++ b/rust/helpers.c
> @@ -20,6 +20,7 @@
>  
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -461,6 +462,24 @@ __u64 rust_helper_drm_vma_node_offset_addr(struct 
> drm_vma_offset_node *node)
>  }
>  EXPORT_SYMBOL_GPL(rust_helper_drm_vma_node_offset_addr);
>  
> +void rust_helper_drm_syncobj_get(struct drm_syncobj *obj)
> +{
> + drm_syncobj_get(obj);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_drm_syncobj_get);
> +
> +void rust_helper_drm_syncobj_put(struct drm_syncobj *obj)
> +{
> + drm_syncobj_put(obj);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_drm_syncobj_put);
> +
> +struct dma_fence *rust_helper_drm_syncobj_fence_get(struct drm_syncobj 
> *syncobj)
> +{
> + return drm_syncobj_fence_get(syncobj);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_drm_syncobj_fence_get);
> +
>  #ifdef CONFIG_DRM_GEM_SHMEM_HELPER
>  
>  void rust_helper_drm_gem_shmem_object_free(struct drm_gem_object *obj)
> diff --git a/rust/kernel/drm/mod.rs b/rust/kernel/drm/mod.rs
> index 73fab2dee3af..dae98826edfd 100644
> --- a/rust/kernel/drm/mod.rs
> +++ b/rust/kernel/drm/mod.rs
> @@ -8,3 +8,4 @@ pub mod file;
>  pub mod gem;
>  pub mod ioctl;
>  pub mod mm;
> +pub mod syncobj;
> diff --git a/rust/kernel/drm/syncobj.rs b/rust/kernel/drm/syncobj.rs
> new file mode 100644
> index ..10eed05eb27a
> --- /dev/null
> +++ b/rust/kernel/drm/syncobj.rs
> @@ -0,0 +1,77 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +
> +//! DRM Sync Objects
> +//!
> +//! C header: 
> [`include/linux/drm/drm_syncobj.h`](../../../../include/linux/drm/drm_syncobj.h)
> +
> +use crate::{bindings, dma_fence::*, drm, error::Result, prelude::*};
> +
> +/// A DRM Sync Object
> +///
> +/// # Invariants
> +/// ptr is a valid pointer to a drm_syncobj and we own a reference to it.
> +pub struct SyncObj {
> +ptr: *mut bindings::drm_syncobj,
> +}
> +
> +impl SyncObj {
> +/// Looks up a sync object by its handle for a given `File`.
> +pub fn lookup_handle(file: &impl drm::file::GenericFile, handle: u32) -> 
> Result {
> +// SAFETY: The arguments are all valid per the type invariants.
> +let ptr = unsafe { bindings::drm_syncobj_find(file.raw() as *mut _, 
> handle) };

Just an aside, but the semantics of this are nasty: You're not allowed to
hold any locks while calling this. We have runtime checks for that (if you
enable lockdep), but I don't see any way to encode that on the rust side
and check it at compile time :-/

> +
> +if ptr.is_null() {
> +Err(ENOENT)
> +} else {
> +Ok(SyncObj { ptr })
> +}
> +}
> +
> +/// Returns the DMA fence associated with this sync object, if any.
> +pub fn fence_get(&self) -> Option {
> +let fence = unsafe { bindings::drm_syncobj_fence_get(self.ptr) };
> +if fence.is_null() {
> +None
> +} else {
> +// SAFETY: The pointer is non-NULL and drm_syncobj_fence_get 
> acquired an
> +// additional reference.
> +Some(unsafe { Fence::from_raw(fence) })
> +}
> +}
> +
> +/// Replaces the DMA fence with a new one, or removes it if fence is 
> None.
> +pub fn replace_fence(&self, fence: Option<&Fence>) {
> +unsafe {
> +bindings::drm_syncobj_replace_fence(
> +self.ptr,
> +fence.map_or(core::ptr::null_mut(), |a| a.raw()),
> +)
> +};
> +}
> +
> +/// Adds a new timeline point to the syncobj.
> +pub fn add_point(&self, chain: FenceChain, fence: &Fence, point: u64) {
> +// SAFETY: All arguments should be valid per the respective type 
> invariants.
> +// This takes over the FenceChain ownership.
> +unsafe { bindings::drm_syncobj_add_point(self.ptr, chain.into_raw(), 
> fence.raw(), point) };
> +}
> +}
> +
> +impl Drop for SyncObj {
> +fn drop(&mut self) {
> +// SAFETY: We own a reference to this syncobj.
> +unsafe { bindings::drm_syncobj_put(self.ptr

Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG

[Roman Beranek]

Hello Maxime,

On Wed Mar 29, 2023 at 9:58 PM CEST, Maxime Ripard wrote:
> > In order to preserve semantic correctness however, I propose to preface
> > the change with a patch that renames sun4i_dotclock and tcon-pixel-clock
> > such that dot/pixel is replaced with d/data. What do you think?
>
> I don't think it's exposed to the userspace in any way so it makes sense to me
>

Here's a new series that includes those renames:


It turns out however that the new dclk rates can't be set exactly as
requested without touching pll-video0*, tcon0 now therefore gets
reparented from pll-mipi to pll-video0-2x which, as it further turns
out, breaks DSI. While simply forbidding the video0-2x mux option seems
to me as the right way to go because there's not much use for it with
non-DSI interfaces either besides the opportunity to power pll-mipi
down, I'd like to run by you first.

Kind regards,
Roman

* As pll-mipi doesn't have CLK_SET_RATE_PARENT flag set, pll-video0
  retains its boot-time rate of 294 MHz set by sunxi-dw-hdmi driver
  in u-boot. Why 294 MHz (as opposed to the default rate of 297 MHz)?
  The driver actually asks for 297 MHz, clock_set_pll3 rounds it to
  294 MHz though because it limits itself to 6 MHz steps.

Re: [PATCH v5 0/8] QAIC accel driver

[Oded Gabbay]

On Wed, Apr 5, 2023 at 2:26 PM Jacek Lawrynowicz
 wrote:
>
> Hi,
>
> On 03.04.2023 19:22, Jeffrey Hugo wrote:
> > On 3/27/2023 9:54 AM, Jeffrey Hugo wrote:
> >> This series introduces a driver under the accel subsystem (QAIC -
> >> Qualcomm AIC) for the Qualcomm Cloud AI 100 product (AIC100).  AIC100 is
> >> a PCIe adapter card that hosts a dedicated machine learning inference
> >> accelerator.
> >>
> >> The previous version (v4) can be found at:
> >> https://lore.kernel.org/all/1679325074-5494-1-git-send-email-quic_jh...@quicinc.com/
> >
> > Looks like things have been silent on this revision and we have a number of 
> > review tags already.  Seems like this series is ready for merge.
> >
> > I'd like to see this queued for 6.4 if possible.  Given that we are at 
> > 6.3-rc5, it seems like this would need to be merged now(ish) to make 6.4.
> >
> > Jacek, since you have commit permissions in drm-misc and are an active 
> > Accel maintainer, I wonder if it would be appropriate for you to merge this 
> > series to drm-misc.  Thoughts?
>
> I'm would be happy to merge it but I think it needs to be acked by Oded first.
>
> Regards,
> Jacek

Hi,
Entire patch-set is:
Acked-by: Oded Gabbay 

Thanks,
Oded

Re: [RFC PATCH 08/10] dma-buf/dma-fence: Introduce long-running completion fences

[Thomas Hellström]

Hi,

On 4/4/23 21:25, Daniel Vetter wrote:

On Tue, Apr 04, 2023 at 07:02:23PM +, Matthew Brost wrote:

On Tue, Apr 04, 2023 at 08:14:01PM +0200, Thomas Hellström (Intel) wrote:

On 4/4/23 15:10, Christian König wrote:

Am 04.04.23 um 14:54 schrieb Thomas Hellström:

Hi, Christian,

On 4/4/23 11:09, Christian König wrote:

Am 04.04.23 um 02:22 schrieb Matthew Brost:

From: Thomas Hellström 

For long-running workloads, drivers either need to open-code
completion
waits, invent their own synchronization primitives or internally use
dma-fences that do not obey the cross-driver dma-fence protocol, but
without any lockdep annotation all these approaches are error prone.

So since for example the drm scheduler uses dma-fences it is
desirable for
a driver to be able to use it for throttling and error
handling also with
internal dma-fences tha do not obey the cros-driver
dma-fence protocol.

Introduce long-running completion fences in form of
dma-fences, and add
lockdep annotation for them. In particular:

* Do not allow waiting under any memory management locks.
* Do not allow to attach them to a dma-resv object.
* Introduce a new interface for adding callbacks making the
helper adding
    a callback sign off on that it is aware that the dma-fence may not
    complete anytime soon. Typically this will be the
scheduler chaining
    a new long-running fence on another one.

Well that's pretty much what I tried before:
https://lwn.net/Articles/893704/


I don't think this quite the same, this explictly enforces that we don't
break the dma-fence rules (in path of memory allocations, exported in
any way), essentially this just SW sync point reusing dma-fence the
infrastructure for signaling / callbacks. I believe your series tried to
export these fences to user space (admittedly I haven't fully read your
series).

In this use case we essentially just want to flow control the ring via
the dma-scheduler + maintain a list of pending jobs so the TDR can be
used for cleanup if LR entity encounters an error. To me this seems
perfectly reasonable but I know dma-femce rules are akin to a holy war.

If we return NULL in run_job, now we have to be able to sink all jobs
in the backend regardless on ring space, maintain a list of jobs pending
for cleanup after errors, and write a different cleanup path as now the
TDR doesn't work. Seems very, very silly to duplicate all of this code
when the DRM scheduler provides all of this for us. Also if we go this
route, now all drivers are going to invent ways to handle LR jobs /w the
DRM scheduler.

This solution is pretty clear, mark the scheduler as LR, and don't
export any fences from the scheduler. If you try to export these fences
a blow up happens.

The problem is if you mix things up. Like for resets you need all the
schedulers on an engine/set-of-engines to quiescent or things get
potentially hilarious. If you now have a scheduler in forever limbo, the
dma_fence guarantees are right out the window.

But the issue you're having is fairly specific if it's just about
ringspace. I think the dumbest fix is to just block in submit if you run
out of per-ctx ringspace, and call it a day. This notion that somehow the
kernel is supposed to provide a bottomless queue of anything userspace
submits simply doesn't hold up in reality (as much as userspace standards
committees would like it to), and as long as it doesn't have a real-world
perf impact it doesn't really matter why we end up blocking in the submit
ioctl. It might also be a simple memory allocation that hits a snag in
page reclaim.


So it seems the discussion around the long-running synchronization 
diverged a bit between threads and this thread was hijacked for 
preempt-fences and userptr.


Do I understand it correctly that the recommendation from both Daniel 
and Christian is to *not* use the drm scheduler for long-running compute 
jobs, but track any internal dma-fence dependencies (pipelined clearing 
or whatever) in a separate mechanism and handle unresolved dependencies 
on other long-running jobs using -EWOULDBLOCK?


Thanks,
Thomas






And the reasons why it was rejected haven't changed.

Regards,
Christian.


Yes, TBH this was mostly to get discussion going how we'd best
tackle this problem while being able to reuse the scheduler for
long-running workloads.

I couldn't see any clear decision on your series, though, but one
main difference I see is that this is intended for driver-internal
use only. (I'm counting using the drm_scheduler as a helper for
driver-private use). This is by no means a way to try tackle the
indefinite fence problem.

Well this was just my latest try to tackle this, but essentially the
problems are the same as with your approach: When we express such
operations as dma_fence there is always the change that we leak that
somewhere.

My approach of adding a flag noting that this operation is dangerous and
can't be synced with something memory management depends on tried to
contain this as much as 

Re: [PATCH 1/5] drm/i915/ttm: Add I915_BO_PREALLOC

[Das, Nirmoy]

Hi Andi,

On 4/5/2023 1:53 PM, Andi Shyti wrote:

Hi Nirmoy,


Add a mechanism to keep existing data when creating
a ttm object with I915_BO_ALLOC_USER flag.

why do we need this mechanism? What was the logic behind? These
are all questions people might have when checking this commit.
Please be a bit more explicative.


Agree, the commit message is bit short. I will add more content in next
revision.

you don't need to send a new version just for this commit log.

You could just propose a new commit log in the reply and if it's
OK, add it before pushing it.


Let me know what do you think about:

Add a mechanism to preserve existing data when creating a TTM

object with the I915_BO_ALLOC_USER flag. This will be used in the subsequent

patch where the I915_BO_ALLOC_USER flag will be applied to the framebuffer

object. For a pre-allocated framebuffer without the I915_BO_PREALLOC flag,

TTM would clear the content, which is not desirable.

Thanks,

Nirmoy



As you wish.

Andi


Cc: Matthew Auld
Cc: Andi Shyti
Cc: Andrzej Hajda
Cc: Ville Syrjälä
Cc: Jani Nikula
Cc: Imre Deak
Signed-off-by: Nirmoy Das

Reviewed-by: Andi Shyti


Thanks,

Nirmoy


Thanks,
Andi

Re: [PATCH RESEND v3 0/3] drm/ttm: Small fixes / cleanups in prep for shrinking

[Thomas Hellström]

On 4/5/23 14:32, Christian König wrote:

Am 04.04.23 um 22:06 schrieb Thomas Hellström:

I collected the, from my POW, uncontroversial patches from V1 of the TTM
shrinker series, some corrected after the initial patch submission, one
patch added from the Xe RFC ("drm/ttm: Don't print error message if
eviction was interrupted"). It would be nice to have these reviewed and
merged while reworking the rest.

v2:
- Simplify __ttm_pool_free().
- Fix the TTM_TT_FLAG bit numbers.
- Keep all allocation orders for TTM pages at or below PMD order

v3:
- Rename __tm_pool_free() to ttm_pool_free_range(). Document.
- Compile-fix.


Reviewed-by: Christian König  for the series.


Thanks, Christian.

/Thomas






Thomas Hellström (3):
   drm/ttm/pool: Fix ttm_pool_alloc error path
   drm/ttm: Reduce the number of used allocation orders for TTM pages
   drm/ttm: Make the call to ttm_tt_populate() interruptible when
 faulting

  drivers/gpu/drm/ttm/ttm_bo_vm.c |  13 +++-
  drivers/gpu/drm/ttm/ttm_pool.c  | 111 
  2 files changed, 80 insertions(+), 44 deletions(-)

Re: [PATCH RFC 04/18] rust: drm: gem: Add GEM object abstraction

[Daniel Vetter]

On Wed, Apr 05, 2023 at 02:32:12PM +0200, Miguel Ojeda wrote:
> On Wed, Apr 5, 2023 at 1:23 PM Daniel Vetter  wrote:
> >
> > Ok if this is just interim I think it's fine. Would still be good to have
> > the MAINTAINERS entry though even just to cover the interim state. Least
> > because I'm assuming that when things are split up you'd still want to
> > keep the rust list on cc for the rust parts, even when they move into
> > subsystems?
> 
> Sorry, I missed to reply the second part of your email -- replying here.
> 
> Currently, the subsystem's code is under `rust/` (though modules can
> go already into other folders). One of the reasons was technical
> simplicity, and a nice side effect is that we could bootstrap things
> while getting C maintainers involved over time.
> 
> To accomplish that, the guidelines for contributing Rust code are that
> the respective maintainers need to be at least Cc'd, even if the files
> do not hit the `F:` fields for the time being -- see [1]. But, for us,
> ideally, the maintainers will take the changes through their tree,
> instead of going through the Rust one, since that is the end goal.
> 
> And, of course, if you already want to have `F:` fields for the Rust
> code, that is even better! (Whether those should be in the same entry
> or in a new one, it is up to you, of course, and whether it is a
> different set of people / level of support / etc.)
> 
> Then, when the `kernel` crate split happens, we can move the code
> directly under whatever folders it should be naturally, when their
> maintainers are ready. For some subsystems, that may mean they do not
> need any `F:` fields since they are already covered (e.g. if they did
> not create a new entry for Rust code only). And for cases like yours,
> where you already had `F:` fields, it means the move of the files can
> be done right away as soon as the split happens.
> 
> In short, we would definitely welcome if you add `F:` fields already
> (whether in existing or new entries) -- it would mean you are ahead of
> the curve! :)
> 
> As for the mailing list, yes, for the time being, I ask that all
> changes to please be sent to the Rust list, so that everybody that
> wants to follow the Rust progress has everything in a single place, so
> that we try to remain consistent in the beginning on e.g. coding
> guidelines, so that Rust reviewers can help spot mistakes, and so on
> and so forth.
> 
> But, as Rust grows in the kernel, as systems become non-experimental,
> and as maintainers take ownership of the code, that should eventually
> go away and let things be as usual with C code. Then the Rust
> subsystem (and its list) will become smaller, and it will be the
> subsystem (and the discussion place) for anything not covered by other
> subsystems, such as core Rust abstractions and types, Rust
> infrastructure and so on.
> 
> How does that sound?

Yeah sounds all great!

I think interim at least a separate rust drm entry
would be good, to make sure we always cc both rust and dri-devel. Once
it's too much for you and you generally trust the dri-devel folks to not
design stupid interfaces, we can then drop that and only ping rust folks
when needed. I do expect that's some years out though.
-Daniel

> 
> [1] https://rust-for-linux.com/contributing#the-rust-subsystem (I may
> reorganize this to be Rust's `P:` field, by the way)
> 
> Cheers,
> Miguel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [RFC PATCH 08/10] dma-buf/dma-fence: Introduce long-running completion fences

[Christian König]

Am 05.04.23 um 14:35 schrieb Thomas Hellström:

Hi,

On 4/4/23 21:25, Daniel Vetter wrote:

On Tue, Apr 04, 2023 at 07:02:23PM +, Matthew Brost wrote:
On Tue, Apr 04, 2023 at 08:14:01PM +0200, Thomas Hellström (Intel) 
wrote:

On 4/4/23 15:10, Christian König wrote:

Am 04.04.23 um 14:54 schrieb Thomas Hellström:

Hi, Christian,

On 4/4/23 11:09, Christian König wrote:

Am 04.04.23 um 02:22 schrieb Matthew Brost:

From: Thomas Hellström 

For long-running workloads, drivers either need to open-code
completion
waits, invent their own synchronization primitives or 
internally use
dma-fences that do not obey the cross-driver dma-fence 
protocol, but
without any lockdep annotation all these approaches are error 
prone.


So since for example the drm scheduler uses dma-fences it is
desirable for
a driver to be able to use it for throttling and error
handling also with
internal dma-fences tha do not obey the cros-driver
dma-fence protocol.

Introduce long-running completion fences in form of
dma-fences, and add
lockdep annotation for them. In particular:

* Do not allow waiting under any memory management locks.
* Do not allow to attach them to a dma-resv object.
* Introduce a new interface for adding callbacks making the
helper adding
    a callback sign off on that it is aware that the dma-fence 
may not

    complete anytime soon. Typically this will be the
scheduler chaining
    a new long-running fence on another one.

Well that's pretty much what I tried before:
https://lwn.net/Articles/893704/

I don't think this quite the same, this explictly enforces that we 
don't

break the dma-fence rules (in path of memory allocations, exported in
any way), essentially this just SW sync point reusing dma-fence the
infrastructure for signaling / callbacks. I believe your series 
tried to

export these fences to user space (admittedly I haven't fully read your
series).

In this use case we essentially just want to flow control the ring via
the dma-scheduler + maintain a list of pending jobs so the TDR can be
used for cleanup if LR entity encounters an error. To me this seems
perfectly reasonable but I know dma-femce rules are akin to a holy war.

If we return NULL in run_job, now we have to be able to sink all jobs
in the backend regardless on ring space, maintain a list of jobs 
pending

for cleanup after errors, and write a different cleanup path as now the
TDR doesn't work. Seems very, very silly to duplicate all of this code
when the DRM scheduler provides all of this for us. Also if we go this
route, now all drivers are going to invent ways to handle LR jobs /w 
the

DRM scheduler.

This solution is pretty clear, mark the scheduler as LR, and don't
export any fences from the scheduler. If you try to export these fences
a blow up happens.

The problem is if you mix things up. Like for resets you need all the
schedulers on an engine/set-of-engines to quiescent or things get
potentially hilarious. If you now have a scheduler in forever limbo, the
dma_fence guarantees are right out the window.

But the issue you're having is fairly specific if it's just about
ringspace. I think the dumbest fix is to just block in submit if you run
out of per-ctx ringspace, and call it a day. This notion that somehow 
the

kernel is supposed to provide a bottomless queue of anything userspace
submits simply doesn't hold up in reality (as much as userspace 
standards
committees would like it to), and as long as it doesn't have a 
real-world
perf impact it doesn't really matter why we end up blocking in the 
submit

ioctl. It might also be a simple memory allocation that hits a snag in
page reclaim.


So it seems the discussion around the long-running synchronization 
diverged a bit between threads and this thread was hijacked for 
preempt-fences and userptr.


Do I understand it correctly that the recommendation from both Daniel 
and Christian is to *not* use the drm scheduler for long-running 
compute jobs, but track any internal dma-fence dependencies (pipelined 
clearing or whatever) in a separate mechanism and handle unresolved 
dependencies on other long-running jobs using -EWOULDBLOCK?


Yeah, I think that's a good summary.

If needed we could extract some scheduler functionality into separate 
components, but the fundamental problem is that to the GPU scheduler 
provides a dma_fence interface to the outside to signal job completion 
and Daniel and I seem to agree that you really don't want that.


Regards,
Christian.



Thanks,
Thomas






And the reasons why it was rejected haven't changed.

Regards,
Christian.


Yes, TBH this was mostly to get discussion going how we'd best
tackle this problem while being able to reuse the scheduler for
long-running workloads.

I couldn't see any clear decision on your series, though, but one
main difference I see is that this is intended for driver-internal
use only. (I'm counting using the drm_scheduler as a helper for
driver-private use). This is by no means a way to try tackle the
in

Re: [PATCH] drm/bridge: ps8640: Use constant sleep time for polling hpd

[rfoss]

From: Robert Foss 

On Fri, 31 Mar 2023 11:02:04 +0800, Pin-yen Lin wrote:
> The default hpd_wait_us in panel_edp.c is 2 seconds. This makes the
> sleep time in the polling of _ps8640_wait_hpd_asserted become 200ms.
> Change it to a constant 20ms to speed up the function.
> 
> 

Applied, thanks!

[1/1] drm/bridge: ps8640: Use constant sleep time for polling hpd
  https://cgit.freedesktop.org/drm/drm-misc/commit/?id=4224011374d1



Rob

Re: [RFC PATCH 08/10] dma-buf/dma-fence: Introduce long-running completion fences

[Daniel Vetter]

On Wed, Apr 05, 2023 at 02:39:35PM +0200, Christian König wrote:
> Am 05.04.23 um 14:35 schrieb Thomas Hellström:
> > Hi,
> > 
> > On 4/4/23 21:25, Daniel Vetter wrote:
> > > On Tue, Apr 04, 2023 at 07:02:23PM +, Matthew Brost wrote:
> > > > On Tue, Apr 04, 2023 at 08:14:01PM +0200, Thomas Hellström
> > > > (Intel) wrote:
> > > > > On 4/4/23 15:10, Christian König wrote:
> > > > > > Am 04.04.23 um 14:54 schrieb Thomas Hellström:
> > > > > > > Hi, Christian,
> > > > > > > 
> > > > > > > On 4/4/23 11:09, Christian König wrote:
> > > > > > > > Am 04.04.23 um 02:22 schrieb Matthew Brost:
> > > > > > > > > From: Thomas Hellström 
> > > > > > > > > 
> > > > > > > > > For long-running workloads, drivers either need to open-code
> > > > > > > > > completion
> > > > > > > > > waits, invent their own synchronization
> > > > > > > > > primitives or internally use
> > > > > > > > > dma-fences that do not obey the cross-driver
> > > > > > > > > dma-fence protocol, but
> > > > > > > > > without any lockdep annotation all these
> > > > > > > > > approaches are error prone.
> > > > > > > > > 
> > > > > > > > > So since for example the drm scheduler uses dma-fences it is
> > > > > > > > > desirable for
> > > > > > > > > a driver to be able to use it for throttling and error
> > > > > > > > > handling also with
> > > > > > > > > internal dma-fences tha do not obey the cros-driver
> > > > > > > > > dma-fence protocol.
> > > > > > > > > 
> > > > > > > > > Introduce long-running completion fences in form of
> > > > > > > > > dma-fences, and add
> > > > > > > > > lockdep annotation for them. In particular:
> > > > > > > > > 
> > > > > > > > > * Do not allow waiting under any memory management locks.
> > > > > > > > > * Do not allow to attach them to a dma-resv object.
> > > > > > > > > * Introduce a new interface for adding callbacks making the
> > > > > > > > > helper adding
> > > > > > > > >     a callback sign off on that it is aware
> > > > > > > > > that the dma-fence may not
> > > > > > > > >     complete anytime soon. Typically this will be the
> > > > > > > > > scheduler chaining
> > > > > > > > >     a new long-running fence on another one.
> > > > > > > > Well that's pretty much what I tried before:
> > > > > > > > https://lwn.net/Articles/893704/
> > > > > > > > 
> > > > I don't think this quite the same, this explictly enforces that
> > > > we don't
> > > > break the dma-fence rules (in path of memory allocations, exported in
> > > > any way), essentially this just SW sync point reusing dma-fence the
> > > > infrastructure for signaling / callbacks. I believe your series
> > > > tried to
> > > > export these fences to user space (admittedly I haven't fully read your
> > > > series).
> > > > 
> > > > In this use case we essentially just want to flow control the ring via
> > > > the dma-scheduler + maintain a list of pending jobs so the TDR can be
> > > > used for cleanup if LR entity encounters an error. To me this seems
> > > > perfectly reasonable but I know dma-femce rules are akin to a holy war.
> > > > 
> > > > If we return NULL in run_job, now we have to be able to sink all jobs
> > > > in the backend regardless on ring space, maintain a list of jobs
> > > > pending
> > > > for cleanup after errors, and write a different cleanup path as now the
> > > > TDR doesn't work. Seems very, very silly to duplicate all of this code
> > > > when the DRM scheduler provides all of this for us. Also if we go this
> > > > route, now all drivers are going to invent ways to handle LR
> > > > jobs /w the
> > > > DRM scheduler.
> > > > 
> > > > This solution is pretty clear, mark the scheduler as LR, and don't
> > > > export any fences from the scheduler. If you try to export these fences
> > > > a blow up happens.
> > > The problem is if you mix things up. Like for resets you need all the
> > > schedulers on an engine/set-of-engines to quiescent or things get
> > > potentially hilarious. If you now have a scheduler in forever limbo, the
> > > dma_fence guarantees are right out the window.
> > > 
> > > But the issue you're having is fairly specific if it's just about
> > > ringspace. I think the dumbest fix is to just block in submit if you run
> > > out of per-ctx ringspace, and call it a day. This notion that
> > > somehow the
> > > kernel is supposed to provide a bottomless queue of anything userspace
> > > submits simply doesn't hold up in reality (as much as userspace
> > > standards
> > > committees would like it to), and as long as it doesn't have a
> > > real-world
> > > perf impact it doesn't really matter why we end up blocking in the
> > > submit
> > > ioctl. It might also be a simple memory allocation that hits a snag in
> > > page reclaim.
> > 
> > So it seems the discussion around the long-running synchronization
> > diverged a bit between threads and this thread was hijacked for
> > preempt-fences and userptr.
> > 
> > Do I understand it correctly that the recommendation from both Daniel
> > and Christian is to

Re: [PATCH] drm/atomic-helper: Do not assume vblank is always present

[Martin Krastev (VMware)]

From: Martin Krastev 


LGTM!

Reviewed-by: Martin Krastev 


Regards,

Martin


On 5.04.23 г. 7:56 ч., Zack Rusin wrote:

From: Zack Rusin 

Many drivers (in particular all of the virtualized drivers) do not
implement vblank handling. Assuming vblank is always present
leads to crashes.

Fix the crashes by making sure the device supports vblank before using
it.

Fixes crashes on boot, as in:
Oops:  [#1] PREEMPT SMP PTI
CPU: 0 PID: 377 Comm: systemd-modules Not tainted 6.3.0-rc4-vmwgfx #1
Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference 
Platform, BIOS 6.00 11/12/2020
RIP: 0010:drm_crtc_next_vblank_start+0x2c/0x80 [drm]
Code: 1e fa 0f 1f 44 00 00 8b 87 90 00 00 00 48 8b 17 55 48 8d 0c c0 48 89 e5 41 54 53 
48 8d 1c 48 48 c1 e3 04 48 03 9a 40 01 00 00 <8b> 53 74 85 d2 74 3f 8b 4>
RSP: 0018:b825004073c8 EFLAGS: 00010246
RAX:  RBX:  RCX: 
RDX: 9b18cf8d RSI: b825004073e8 RDI: 9b18d0f4
RBP: b825004073d8 R08: 9b18cf8d R09: 
R10: 9b18c57ef6e8 R11: 9b18c2d59e00 R12: 
R13: 9b18cfa99280 R14:  R15: 9b18cf8d
FS:  7f2b82538900() GS:9b19f7c0() knlGS:
CS:  0010 DS:  ES:  CR0: 80050033
CR2: 0074 CR3: 0001060a6003 CR4: 003706f0
Call Trace:
  
  drm_atomic_helper_wait_for_fences+0x87/0x1e0 [drm_kms_helper]
  drm_atomic_helper_commit+0xa1/0x160 [drm_kms_helper]
  drm_atomic_commit+0x9a/0xd0 [drm]
  ? __pfx___drm_printfn_info+0x10/0x10 [drm]
  drm_client_modeset_commit_atomic+0x1e9/0x230 [drm]
  drm_client_modeset_commit_locked+0x5f/0x160 [drm]
  ? mutex_lock+0x17/0x50
  drm_client_modeset_commit+0x2b/0x50 [drm]
  __drm_fb_helper_restore_fbdev_mode_unlocked+0xca/0x100 [drm_kms_helper]
  drm_fb_helper_set_par+0x40/0x80 [drm_kms_helper]
  fbcon_init+0x2c5/0x690
  visual_init+0xee/0x190
  do_bind_con_driver.isra.0+0x1ce/0x4b0
  do_take_over_console+0x136/0x220
  ? vprintk_default+0x21/0x30
  do_fbcon_takeover+0x78/0x130
  do_fb_registered+0x139/0x270
  fbcon_fb_registered+0x3b/0x90
  ? fb_add_videomode+0x81/0xf0
  register_framebuffer+0x22f/0x330
  __drm_fb_helper_initial_config_and_unlock+0x349/0x520 [drm_kms_helper]
  ? kmalloc_large+0x25/0xc0
  drm_fb_helper_initial_config+0x3f/0x50 [drm_kms_helper]
  drm_fbdev_generic_client_hotplug+0x73/0xc0 [drm_kms_helper]
  drm_fbdev_generic_setup+0x99/0x170 [drm_kms_helper]

Signed-off-by: Zack Rusin 
Fixes: d39e48ca80c0 ("drm/atomic-helper: Set fence deadline for vblank")
Cc: Rob Clark 
Cc: Daniel Vetter 
---
  drivers/gpu/drm/drm_vblank.c | 6 +-
  1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 299fa2a19a90..6438aeb1c65f 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -997,12 +997,16 @@ int drm_crtc_next_vblank_start(struct drm_crtc *crtc, 
ktime_t *vblanktime)
  {
unsigned int pipe = drm_crtc_index(crtc);
struct drm_vblank_crtc *vblank = &crtc->dev->vblank[pipe];
-   struct drm_display_mode *mode = &vblank->hwmode;
+   struct drm_display_mode *mode;
u64 vblank_start;
  
+	if (!drm_dev_has_vblank(crtc->dev))

+   return -EINVAL;
+
if (!vblank->framedur_ns || !vblank->linedur_ns)
return -EINVAL;
  
+	mode = &vblank->hwmode;

if (!drm_crtc_get_last_vbltimestamp(crtc, vblanktime, false))
return -EINVAL;
  

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

[Paul Cercueil]

Hi Maxime,

Le lundi 27 mars 2023 à 21:24 +0200, Maxime Ripard a écrit :
> On Fri, Mar 24, 2023 at 08:58:48PM +, Aidan MacDonald wrote:
> > > > My suggestion: add a per-clock bitmap to keep track of which
> > > > parents
> > > > are allowed. Any operation that would select a parent clock not
> > > > on the
> > > > whitelist should fail. Automatic reparenting should only select
> > > > from
> > > > clocks on the whitelist. And we need new DT bindings for
> > > > controlling
> > > > the whitelist, for example:
> > > > 
> > > >     clock-parents-0 = <&clk1>, <&pll_c>;
> > > >     clock-parents-1 = <&clk2>, <&pll_a>, <&pll_b>;
> > > > 
> > > > This means that clk1 can only have pll_c as a parent, while
> > > > clk2 can
> > > > have pll_a or pll_b as parents. By default every clock will be
> > > > able
> > > > to use any parent, so a list is only needed if the machine
> > > > needs a
> > > > more restrictive policy.
> > > > 
> > > > assigned-clock-parents should disable automatic reparenting,
> > > > but allow
> > > > explicit clk_set_parent(). This will allow clock drivers to
> > > > start doing
> > > > reparenting without breaking old DTs.
> > > 
> > > I'm generally not a fan of putting all these policies in the
> > > device
> > > tree. Do you have an example where it wouldn't be possible to do
> > > exactly
> > > this from the driver itself?
> > 
> > I'm confused. What's implicit in the example is clk1 and clk2 might
> > have *other* possible choices of parent clock and the device tree
> > is
> > limiting what the OS is allowed to choose.
> > 
> > Why would you put such arbitrary limitations into the driver?
> 
> Why would we put such arbitrary limitations in the firmware? As this
> entire thread can attest, people are already using the device tree to
> work around the limitations of the Linux driver, or reduce the
> features of Linux because they can rely on the device tree. Either
> way, it's linked to the state of the Linux driver, and any other OS
> or
> Linux version could very well implement something more dynamic.

Probably because if we have to choose between setting policy in the
kernel or in the firmware, it is arguably better to set it in the
firmware.

Especially when talking about clocks, as the firmware is already the
one programming the boot clocks.

Cheers,
-Paul

> > They would be different from machine to machine, unless the clock
> > tree is so simple there is only *one* meaningful way to configure
> > it.
> 
> If we look at the device trees we have in-tree, most of the users of
> assigned-clocks are the same from one board to another.
> 
> > Most SoCs are complicated enough that there will be tradeoffs
> > depending on what peripherals you are using (typically a single
> > machine will not use *every* peripheral device provided by the
> > SoC).
> 
> We already have APIs to lock parents or rates on a given clock from
> the consumer. It's far superior (feature-wise) than what the device
> tree will ever offer because it's code, and it depends on the usage
> already since an unused driver won't probe.
> 
> Maxime

Re: [RFC PATCH v8] media: mediatek: vcodec: support stateless AV1 decoder

[AngeloGioacchino Del Regno]

Il 30/01/23 13:38, Xiaoyong Lu ha scritto:

Add mediatek av1 decoder linux driver which use the stateless API in
MT8195.

Signed-off-by: Xiaoyong Lu


Reviewed-by: AngeloGioacchino Del Regno 


On MT8195 Tomato
Tested-by: AngeloGioacchino Del Regno 

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

[Paul Cercueil]

Hi Maxime,

Le mardi 04 avril 2023 à 12:11 +0200, Maxime Ripard a écrit :
> The Ingenic CGU clocks implements a mux with a set_parent hook, but
> doesn't provide a determine_rate implementation.
> 
> This is a bit odd, since set_parent() is there to, as its name
> implies,
> change the parent of a clock. However, the most likely candidate to
> trigger that parent change is a call to clk_set_rate(), with
> determine_rate() figuring out which parent is the best suited for a
> given rate.
> 
> The other trigger would be a call to clk_set_parent(), but it's far
> less
> used, and it doesn't look like there's any obvious user for that
> clock.
> 
> So, the set_parent hook is effectively unused, possibly because of an
> oversight. However, it could also be an explicit decision by the
> original author to avoid any reparenting but through an explicit call
> to
> clk_set_parent().

As I said in the v2 (IIRC), clk_set_parent() is used when re-parenting
from the device tree.

> 
> The driver does implement round_rate() though, which means that we
> can
> change the rate of the clock, but we will never get to change the
> parent.
> 
> However, It's hard to tell whether it's been done on purpose or not.
> 
> Since we'll start mandating a determine_rate() implementation, let's
> convert the round_rate() implementation to a determine_rate(), which
> will also make the current behavior explicit. And if it was an
> oversight, the clock behaviour can be adjusted later on.

So just to be sure, this patch won't make clk_set_rate() automatically
switch parents, right?

Allowing automatic re-parenting sounds like a huge can of worms...

Cheers,
-Paul

> 
> Signed-off-by: Maxime Ripard 
> ---
>  drivers/clk/ingenic/cgu.c | 15 ---
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/clk/ingenic/cgu.c b/drivers/clk/ingenic/cgu.c
> index 1f7ba30f5a1b..0c9c8344ad11 100644
> --- a/drivers/clk/ingenic/cgu.c
> +++ b/drivers/clk/ingenic/cgu.c
> @@ -491,22 +491,23 @@ ingenic_clk_calc_div(struct clk_hw *hw,
> return div;
>  }
>  
> -static long
> -ingenic_clk_round_rate(struct clk_hw *hw, unsigned long req_rate,
> -  unsigned long *parent_rate)
> +static int ingenic_clk_determine_rate(struct clk_hw *hw,
> + struct clk_rate_request *req)
>  {
> struct ingenic_clk *ingenic_clk = to_ingenic_clk(hw);
> const struct ingenic_cgu_clk_info *clk_info =
> to_clk_info(ingenic_clk);
> unsigned int div = 1;
>  
> if (clk_info->type & CGU_CLK_DIV)
> -   div = ingenic_clk_calc_div(hw, clk_info,
> *parent_rate, req_rate);
> +   div = ingenic_clk_calc_div(hw, clk_info, req-
> >best_parent_rate,
> +  req->rate);
> else if (clk_info->type & CGU_CLK_FIXDIV)
> div = clk_info->fixdiv.div;
> else if (clk_hw_can_set_rate_parent(hw))
> -   *parent_rate = req_rate;
> +   req->best_parent_rate = req->rate;
>  
> -   return DIV_ROUND_UP(*parent_rate, div);
> +   req->rate = DIV_ROUND_UP(req->best_parent_rate, div);
> +   return 0;
>  }
>  
>  static inline int ingenic_clk_check_stable(struct ingenic_cgu *cgu,
> @@ -626,7 +627,7 @@ static const struct clk_ops ingenic_clk_ops = {
> .set_parent = ingenic_clk_set_parent,
>  
> .recalc_rate = ingenic_clk_recalc_rate,
> -   .round_rate = ingenic_clk_round_rate,
> +   .determine_rate = ingenic_clk_determine_rate,
> .set_rate = ingenic_clk_set_rate,
>  
> .enable = ingenic_clk_enable,
> 

Re: [RFC PATCH 08/10] dma-buf/dma-fence: Introduce long-running completion fences

[Daniel Vetter]

On Tue, Apr 04, 2023 at 07:48:27PM +, Matthew Brost wrote:
> On Tue, Apr 04, 2023 at 09:25:52PM +0200, Daniel Vetter wrote:
> > On Tue, Apr 04, 2023 at 07:02:23PM +, Matthew Brost wrote:
> > > On Tue, Apr 04, 2023 at 08:14:01PM +0200, Thomas Hellström (Intel) wrote:
> > > > 
> > > > On 4/4/23 15:10, Christian König wrote:
> > > > > Am 04.04.23 um 14:54 schrieb Thomas Hellström:
> > > > > > Hi, Christian,
> > > > > > 
> > > > > > On 4/4/23 11:09, Christian König wrote:
> > > > > > > Am 04.04.23 um 02:22 schrieb Matthew Brost:
> > > > > > > > From: Thomas Hellström 
> > > > > > > > 
> > > > > > > > For long-running workloads, drivers either need to open-code
> > > > > > > > completion
> > > > > > > > waits, invent their own synchronization primitives or 
> > > > > > > > internally use
> > > > > > > > dma-fences that do not obey the cross-driver dma-fence 
> > > > > > > > protocol, but
> > > > > > > > without any lockdep annotation all these approaches are error 
> > > > > > > > prone.
> > > > > > > > 
> > > > > > > > So since for example the drm scheduler uses dma-fences it is
> > > > > > > > desirable for
> > > > > > > > a driver to be able to use it for throttling and error
> > > > > > > > handling also with
> > > > > > > > internal dma-fences tha do not obey the cros-driver
> > > > > > > > dma-fence protocol.
> > > > > > > > 
> > > > > > > > Introduce long-running completion fences in form of
> > > > > > > > dma-fences, and add
> > > > > > > > lockdep annotation for them. In particular:
> > > > > > > > 
> > > > > > > > * Do not allow waiting under any memory management locks.
> > > > > > > > * Do not allow to attach them to a dma-resv object.
> > > > > > > > * Introduce a new interface for adding callbacks making the
> > > > > > > > helper adding
> > > > > > > >    a callback sign off on that it is aware that the dma-fence 
> > > > > > > > may not
> > > > > > > >    complete anytime soon. Typically this will be the
> > > > > > > > scheduler chaining
> > > > > > > >    a new long-running fence on another one.
> > > > > > > 
> > > > > > > Well that's pretty much what I tried before:
> > > > > > > https://lwn.net/Articles/893704/
> > > > > > > 
> > > 
> > > I don't think this quite the same, this explictly enforces that we don't
> > > break the dma-fence rules (in path of memory allocations, exported in
> > > any way), essentially this just SW sync point reusing dma-fence the
> > > infrastructure for signaling / callbacks. I believe your series tried to
> > > export these fences to user space (admittedly I haven't fully read your
> > > series).
> > > 
> > > In this use case we essentially just want to flow control the ring via
> > > the dma-scheduler + maintain a list of pending jobs so the TDR can be
> > > used for cleanup if LR entity encounters an error. To me this seems
> > > perfectly reasonable but I know dma-femce rules are akin to a holy war.
> > > 
> > > If we return NULL in run_job, now we have to be able to sink all jobs
> > > in the backend regardless on ring space, maintain a list of jobs pending
> > > for cleanup after errors, and write a different cleanup path as now the
> > > TDR doesn't work. Seems very, very silly to duplicate all of this code
> > > when the DRM scheduler provides all of this for us. Also if we go this
> > > route, now all drivers are going to invent ways to handle LR jobs /w the
> > > DRM scheduler.
> > > 
> > > This solution is pretty clear, mark the scheduler as LR, and don't
> > > export any fences from the scheduler. If you try to export these fences
> > > a blow up happens.
> > 
> > The problem is if you mix things up. Like for resets you need all the
> > schedulers on an engine/set-of-engines to quiescent or things get
> > potentially hilarious. If you now have a scheduler in forever limbo, the
> > dma_fence guarantees are right out the window.
> > 
> 
> Right, a GT reset on Xe is:
> 
> Stop all schedulers
> Do a reset
> Ban any schedulers which we think caused the GT reset
> Resubmit all schedulers which we think were good
> Restart all schedulers
> 
> None of this flow depends on LR dma-fences, all of this uses the DRM
> sched infrastructure and work very well compared to the i915. Rewriting
> all this with a driver specific implementation is what we are trying to
> avoid.
> 
> Similarly if LR entity hangs on its own (not a GT reset, rather the
> firmware does the reset for us) we use all the DRM scheduler
> infrastructure to handle this. Again this works rather well...

Yeah this is why I don't think duplicating everything that long-running
jobs need makes any sense. iow I agree with you.

> > But the issue you're having is fairly specific if it's just about
> > ringspace. I think the dumbest fix is to just block in submit if you run
> > out of per-ctx ringspace, and call it a day. This notion that somehow the
> 
> How does that not break the dma-fence rules? A job can publish its
> finished fence after ARM, if the finished fence fence waits on ring
> spac

Re: [PATCH 1/8] drm/gma500: Use drm_aperture_remove_conflicting_pci_framebuffers

[Daniel Vetter]

On Wed, Apr 05, 2023 at 01:16:27PM +0200, Javier Martinez Canillas wrote:
> Thomas Zimmermann  writes:
> 
> [...]
> 
> >
> > Your comment says that it calls a PCI function to clean up to vgacon. 
> > That comment explains what is happening, not why. And how the PCI and 
> > vgacon code work together is non-obvious.

Would a better comment help then:

/*
 * gma500 is a strange hybrid device, which both acts as a pci
 * device (for legacy vga functionality) but also more like an
 * integrated display on a SoC where the framebuffer simply
 * resides in main memory and not in a special pci bar (that
 * internally redirects to a stolen range of main memory) like all
 * other integrated pci display devices have.
 *
 * To catch all cases we need to both remove conflicting fw
 * drivers for the pci device and main memory.
 */
> >
> > Again, here's my proposal for gma500:
> >
> > // call this from psb_pci_probe()
> > int gma_remove_conflicting_framebuffers(struct pci_dev *pdev, const
> > struct drm_driver *req_driver)
> > {
> > resource_size_t base = 0;
> > resource_size_t size = (resource_size_t)-1;
> > const char *name = req_driver->name;
> > int ret;
> >
> > /*
> >  * We cannot yet easily find the framebuffer's location in
> >  * memory. So remove all framebuffers here.
> >  *
> >  * TODO: Refactor psb_driver_load() to map vdc_reg earlier. Then
> >  *   we might be able to read the framebuffer range from the
> >  *   device.
> >  */
> > ret = aperture_remove_conflicting_devices(base, size, name);

Why can't this be a call to drm_aperture_remove_framebuffers? At least as
long as we don't implement the "read out actual fb base and size" code,
which also none of the other soc drivers bother with?

> > if (ret)
> > return ret;
> >
> > /*
> >  * WARNING: Apparently we must kick fbdev drivers before vgacon,
> >  * otherwise the vga fbdev driver falls over.
> >  */
> > ret = vga_remove_vgacon(pdev);

This isn't enough, we also nuke stuff that's mapping the vga fb range.
Which is really the reason I don't want to open code random stuff, pci is
self-describing, if it's decoding legacy vga it can figure this out and we
only have to implement the "how do I nuke legacy vga fw drivers from a pci
driver" once.

Not twice like this would result in, with the gma500 version being only
half the thing.

If it absolutely has to be a separate function for the gma500 pci legacy
vga (I still don't get why, it's just a pci vga device, there's absolutely
nothing special about that part at all) then I think it needs to be at
least a common "nuke a legacy vga device for me pls" function, which
shares the implementation with the pci one.

But not open-coding just half of it only.

> > if (ret)
> > return ret;
> >
> > return 0;
> > }
> >
> 
> If this is enough I agree that is much more easier code to understand.

It's still two calls and more code with more bugs? I'm not seeing the
point.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 3/3] drm/fb-helper: fix input validation gaps in check_var

[Daniel Vetter]

On Wed, Apr 05, 2023 at 12:52:12PM +0200, Javier Martinez Canillas wrote:
> Daniel Vetter  writes:
> 
> > Apparently drivers need to check all this stuff themselves, which for
> > most things makes sense I guess. And for everything else we luck out,
> > because modern distros stopped supporting any other fbdev drivers than
> > drm ones and I really don't want to argue anymore about who needs to
> > check stuff. Therefore fixing all this just for drm fbdev emulation is
> > good enough.
> >
> 
> Agreed.
> 
> > Note that var->active is not set or validated. This is just control
> > flow for fbmem.c and needs to be validated in there as needed.
> >
> > Signed-off-by: Daniel Vetter 
> > Cc: Maarten Lankhorst 
> > Cc: Maxime Ripard 
> > Cc: Thomas Zimmermann 
> > ---
> 
> [...]
> 
> >  
> > +static void __fill_var(struct fb_var_screeninfo *var,
> > +  struct drm_framebuffer *fb)
> > +{
> > +   int i;
> > +
> > +   var->xres_virtual = fb->width;
> > +   var->yres_virtual = fb->height;
> > +   var->accel_flags = FB_ACCELF_TEXT;
> > +   var->bits_per_pixel = drm_format_info_bpp(fb->format, 0);
> > +
> > +   var->height = var->width = 0;
> > +   var->left_margin = var->right_margin = 0;
> > +   var->upper_margin = var->lower_margin = 0;
> > +   var->hsync_len = var->vsync_len = 0;
> > +   var->sync = var->vmode = 0;
> > +   var->rotate = 0;
> > +   var->colorspace = 0;
> > +   for (i = 0; i < 4; i++)
> > +   var->reserved[i] = 0;
> > +}
> > +
> >  /**
> >   * drm_fb_helper_check_var - implementation for &fb_ops.fb_check_var
> >   * @var: screeninfo to check
> > @@ -1595,8 +1616,22 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo 
> > *var,
> > return -EINVAL;
> > }
> >  
> > -   var->xres_virtual = fb->width;
> > -   var->yres_virtual = fb->height;
> > +   __fill_var(var, fb);
> > +
> 
> [...]
> 
> There is the following here (in latest drm-misc/drm-misc-next at least):
> 
>   /*
>* Changes struct fb_var_screeninfo are currently not pushed back
>* to KMS, hence fail if different settings are requested.
>*/
>   bpp = drm_format_info_bpp(format, 0);
>   if (var->bits_per_pixel > bpp ||
>   var->xres > fb->width || var->yres > fb->height ||
>   var->xres_virtual > fb->width || var->yres_virtual > fb->height) {
>   drm_dbg_kms(dev, "fb requested width/height/bpp can't fit in 
> current fb "
> "request %dx%d-%d (virtual %dx%d) > %dx%d-%d\n",
> var->xres, var->yres, var->bits_per_pixel,
> var->xres_virtual, var->yres_virtual,
> fb->width, fb->height, bpp);
>   return -EINVAL;
>   }
> 
> but only the 'var->xres > fb->width || var->yres > fb->height' from the
> conditions checked could be false after your __fill_var() call above.
> 
> You should drop the 'var->bits_per_pixel > bpp', 'var->xres_virtual >
> fb->width' and 'var->yres_virtual > fb->height' checks I believe since
> those will always be true.

The __fill_var is after this. I'm honestly not sure what the exact
semantics are supposed to be, but essentially if userspace asks for too
big virtual size, we reject it. And for anything else we then tell it
(with __fill_var) how big the actually available space is.

What I'm wondering now is whether too small x/yres won't lead to problems
of some sorts ... For multi-screen we set the virtual size to be big
enough for all crtc, and then just set x/yres to be the smallest output.
That way fbcon knows to only draw as much as is visible on all screens.
But if you then pan that too much, the bigger screens might not have a big
enough buffer anymore and things fail (but shouldn't).

Not sure how to fix that tbh.
-Daniel

> 
> > +   /*
> > +* fb_pan_display() validates this, but fb_set_par() doesn't and just
> > +* falls over. Note that __fill_var above adjusts y/res_virtual.
> > +*/
> > +   if (var->yoffset > var->yres_virtual - var->yres ||
> > +   var->xoffset > var->xres_virtual - var->xres)
> > +   return -EINVAL;
> > +
> > +   /* We neither support grayscale nor FOURCC (also stored in here). */
> > +   if (var->grayscale > 0)
> > +   return -EINVAL;
> > +
> > +   if (var->nonstd)
> > +   return -EINVAL;
> >  
> > /*
> >  * Workaround for SDL 1.2, which is known to be setting all pixel format
> > @@ -1612,11 +1647,6 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo 
> > *var,
> > drm_fb_helper_fill_pixel_fmt(var, format);
> > }
> >  
> 
> Other than what I mentioned, the patch makes sense to me.
> 
> Reviewed-by: Javier Martinez Canillas 
> 
> -- 
> Best regards,
> 
> Javier Martinez Canillas
> Core Platforms
> Red Hat
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/3] drm/fb-helper: set x/yres_virtual in drm_fb_helper_check_var

[Daniel Vetter]

On Wed, Apr 05, 2023 at 12:21:11PM +0200, Javier Martinez Canillas wrote:
> Daniel Vetter  writes:
> 
> > Drivers are supposed to fix this up if needed if they don't outright
> > reject it. Uncovered by 6c11df58fd1a ("fbmem: Check virtual screen
> > sizes in fb_set_var()").
> >
> 
> Should have a Fixes: tag ? I understand what was uncovered by that commit
> but it help distros to figure out if something has to be cherry-picked by
> them. So I believe that would be useful to have it.
> 
> The patch looks good to me.

The cc: stable should go far enough back for that. Or that was at least my
idea ... I can add the Fixes: back since I had it but dropped it
intentionally because it's not really a bug in the fbmem patch.
-Daniel

> Reviewed-by: Javier Martinez Canillas 
> 
> -- 
> Best regards,
> 
> Javier Martinez Canillas
> Core Platforms
> Red Hat
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v5 0/8] QAIC accel driver

[Daniel Vetter]

On Wed, Apr 05, 2023 at 03:35:19PM +0300, Oded Gabbay wrote:
> On Wed, Apr 5, 2023 at 2:26 PM Jacek Lawrynowicz
>  wrote:
> >
> > Hi,
> >
> > On 03.04.2023 19:22, Jeffrey Hugo wrote:
> > > On 3/27/2023 9:54 AM, Jeffrey Hugo wrote:
> > >> This series introduces a driver under the accel subsystem (QAIC -
> > >> Qualcomm AIC) for the Qualcomm Cloud AI 100 product (AIC100).  AIC100 is
> > >> a PCIe adapter card that hosts a dedicated machine learning inference
> > >> accelerator.
> > >>
> > >> The previous version (v4) can be found at:
> > >> https://lore.kernel.org/all/1679325074-5494-1-git-send-email-quic_jh...@quicinc.com/
> > >
> > > Looks like things have been silent on this revision and we have a number 
> > > of review tags already.  Seems like this series is ready for merge.
> > >
> > > I'd like to see this queued for 6.4 if possible.  Given that we are at 
> > > 6.3-rc5, it seems like this would need to be merged now(ish) to make 6.4.
> > >
> > > Jacek, since you have commit permissions in drm-misc and are an active 
> > > Accel maintainer, I wonder if it would be appropriate for you to merge 
> > > this series to drm-misc.  Thoughts?
> >
> > I'm would be happy to merge it but I think it needs to be acked by Oded 
> > first.
> >
> > Regards,
> > Jacek
> 
> Hi,
> Entire patch-set is:
> Acked-by: Oded Gabbay 

Once Jacke has pushed this I htink it would also be good to get Jeffrey
commit rights for drm-misc, so that in the future bugfixes for the qaic
driver can be pushed directly by the qaic team. Still with acks/r-b
requirements as per usual, and I guess for anything bigger/new uapi an ack
from oded is needed.

https://drm.pages.freedesktop.org/maintainer-tools/commit-access.html#drm-misc

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch