Re: simpledrm, running display servers, and drivers replacing simpledrm while the display server is running
On Fri, May 10, 2024 at 02:45:48PM +0200, Thomas Zimmermann wrote: > Hi > > > (This was discussed on #dri-devel, but I'll reiterate here as well). > > > > There are two problems at hand; one is the race condition during boot > > when the login screen (or whatever display server appears first) is > > launched with simpledrm, only some moments later having the real GPU > > driver appear. > > > > The other is general purpose GPU hotplugging, including the unplugging > > the GPU decided by the compositor to be the primary one. > > The situation of booting with simpledrm (problem 2) is a special case of > problem 1. From the kernel's perspective, unloading simpledrm is the same as > what you call general purpose GPU hotplugging. Even through there is not a > full GPU, but a trivial scanout buffer. In userspace, you see the same > sequence of events as in the general case. Sure, in a way it is, but the consequence and frequency of occurence is quite different, so I think it makes sense to think of them as different problems, since they need different solutions. One is about fixing userspace components support for arbitrary hotplugging, the other for mitigating the race condition that caused this discussion to begin with. > > > > > The latter is something that should be handled in userspace, by > > compositors, etc, I agree. > > > > The former, however, is not properly solved by userspace learning how to > > deal with primary GPU unplugging and switching to using a real GPU > > driver, as it'd break the booting and login experience. > > > > When it works, i.e. the race condition is not hit, is this: > > > > * System boots > > * Plymouth shows a "splash" screen > > * The login screen display server is launched with the real GPU driver > > * The login screen interface is smoothly animating using hardware > > accelerating, presenting "advanced" graphical content depending on > > hardware capabilities (e.g. high color bit depth, HDR, and so on) > > > > If the race condition is hit, with a compositor supporting primary GPU > > hotplugging, it'll work like this: > > > > * System boots > > * Plymouth shows a "splash" screen > > * The login screen display server is launched with simpledrm > > * Due to using simpldrm, the login screen interface is not animated and > > just plops up, and no "advanced" graphical content is enabled due to > > apparent missing hardware capabilities > > * The real GPU driver appears, the login screen now starts to become > > animated, and may suddenly change appearance due to capabilties > > having changed > > > > Thus, by just supporting hotplugging the primary GPU in userspace, we'll > > still end up with a glitchy boot experience, and it forces userspace to > > add things like sleep(10) to work around this. > > > > In other words, fixing userspace is *not* a correct solution to the > > problem, it's a work around (albeit a behaivor we want for other > > reasons) for the race condition. > > To really fix the flickering, you need to read the old DRM device's atomic > state and apply it to the new device. Then tell the desktop and applications > to re-init their rendering stack. > > Depending on the DRM driver and its hardware, it might be possible to do > this without flickering. The key is to not loose the original scanout > buffer, while not probing the new device driver. But that needs work in each > individual DRM driver. This doesn't sound like it'll fix any flickering as I describe them. First, the loss of initial animation when the login interface appears is not something one can "fix", since it has already happened. Avoiding flickering when switching to the new driver is only possible if one limits oneself to what simpledrm was capable of doing, i.e. no HDR signaling etc. > > > > > Arguably, the only place a more educated guess about whether to wait or > > not, and if so how long, is the kernel. > > As I said before, driver modules come and go and hardware devices come and > go. > > To detect if there might be a native driver waiting to be loaded, you can > test for > > - 'nomodeset' on the command line -> no native driver Makes sense to not wait here, and just assume simpledrm forever. > - 'systemd-load-modules' not started -> maybe wait > - look for drivers under /lib/modules//kernel/drivers/gpu/drm/ -> > maybe wait I suspect this is not useful for general purpose distributions. I have 43 kernel GPU modules there, on a F40 installation. > - maybe udev can tell you more > - it might for detection help that recently simpledrm devices refer to their > parent PCI device > - maybe systemd tracks the probed devices If the kernel already plumbs enough state so userspace components can make a decent decision, instead of just sleeping for an arbitrary amount of time, then great. This is to some degree what https://github.com/systemd/systemd/issues/32509 is about. Jonas > > Best regards > Thomas > > > > > > >
Re: simpledrm, running display servers, and drivers replacing simpledrm while the display server is running
On Fri, May 10, 2024 at 09:32:02AM +0200, Thomas Zimmermann wrote: > Hi > > Am 09.05.24 um 15:06 schrieb nerdopolis: > > > > Hi > > > > > > So I have been made aware of an apparent race condition of some drivers > > taking a bit longer to load, which could lead to a possible race > > condition of display servers/greeters using the simpledrm device, and > > then experiencing problems once the real driver loads, the simpledrm > > device that the display servers are using as their primary GPU goes > > away. > > > > > > For example Weston crashes, Xorg crashes, wlroots seems to stay running, > > but doesn't draw anything on the screen, kwin aborts, > > > > This is if you boot on a QEMU machine with the virtio card, with > > modprobe.blacklist=virtio_gpu, and then, when the display server is > > running, run sudo modprobe virtio-gpu > > > > > > Namely, it's been recently reported here: > > https://github.com/sddm/sddm/issues/1917 and here > > https://github.com/systemd/systemd/issues/32509 > > > > > > My thinking: Instead of simpledrm's /dev/dri/card0 device going away > > when the real driver loads, is it possible for simpledrm to instead > > simulate an unplug of the fake display/CRTC? > > > > To my knowledge, there's no hotplugging for CRTCs. > > > That way in theory, the simpledrm device will now be useless for drawing > > for drawing to the screen at that point, since the real driver is now > > taken over, but this way here, at least the display server doesn't lose > > its handles to the /dev/dri/card0 device, (and then maybe only remove > > itself once the final handle to it closes?) > > > > > > Is something like this possible to do with the way simpledrm works with > > the low level video memory? Or is this not possible? > > > > Userspace needs to be prepared that graphics devices can do hotplugging. The > correct solution is to make compositors work without graphics devices. (This was discussed on #dri-devel, but I'll reiterate here as well). There are two problems at hand; one is the race condition during boot when the login screen (or whatever display server appears first) is launched with simpledrm, only some moments later having the real GPU driver appear. The other is general purpose GPU hotplugging, including the unplugging the GPU decided by the compositor to be the primary one. The latter is something that should be handled in userspace, by compositors, etc, I agree. The former, however, is not properly solved by userspace learning how to deal with primary GPU unplugging and switching to using a real GPU driver, as it'd break the booting and login experience. When it works, i.e. the race condition is not hit, is this: * System boots * Plymouth shows a "splash" screen * The login screen display server is launched with the real GPU driver * The login screen interface is smoothly animating using hardware accelerating, presenting "advanced" graphical content depending on hardware capabilities (e.g. high color bit depth, HDR, and so on) If the race condition is hit, with a compositor supporting primary GPU hotplugging, it'll work like this: * System boots * Plymouth shows a "splash" screen * The login screen display server is launched with simpledrm * Due to using simpldrm, the login screen interface is not animated and just plops up, and no "advanced" graphical content is enabled due to apparent missing hardware capabilities * The real GPU driver appears, the login screen now starts to become animated, and may suddenly change appearance due to capabilties having changed Thus, by just supporting hotplugging the primary GPU in userspace, we'll still end up with a glitchy boot experience, and it forces userspace to add things like sleep(10) to work around this. In other words, fixing userspace is *not* a correct solution to the problem, it's a work around (albeit a behaivor we want for other reasons) for the race condition. Arguably, the only place a more educated guess about whether to wait or not, and if so how long, is the kernel. Jonas > > The next best solution is to keep the final DRM device open until a new one > shows up. All DRM graphics drivers with hotplugging support are required to > accept commands after their hardware has been unplugged. They simply won't > display anything. > > Best regards > Thomas > > > > > > Thanks > > > > -- > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Frankenstrasse 146, 90461 Nuernberg, Germany > GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman > HRB 36809 (AG Nuernberg) >
Re: [RFC] Plane color pipeline KMS uAPI
On Thu, May 11, 2023 at 04:56:47PM +, Joshua Ashton wrote: > When we are talking about being 'prescriptive' in the API, are we > outright saying we don't want to support arbitrary 3D LUTs, or are we > just offering certain algorithms to be 'executed' for a plane/crtc/etc > in the atomic API? I am confused... The 'prescriptive' idea that the RFC of this thread proposes *is* a way to support arbitrary 3D LUTs (and other mathematical operations), arbitrarily, in a somewhat vendored way, only that it will not be vendor prefixed hard coded properties with specific positions in the pipeline, but instead more or less an introspectable pipeline, describing what kind of LUT's, Matrix multiplication (and in what order) etc a hardware can do. The theoretical userspace library would be the one turning descriptive "please turn this into that" requests into the "prescriptive" color pipeline operations. It would target general purpose compositors, but it wouldn't be mandatory. Doing vendor specific implemantions in gamescope would be possible; it wouldn't look like the verion that exist somewhere now that uses a bunch of AMD_* properties, it'd look more like the example Simon had in the initial RFC. Jonas > > There is so much stuff to do with color, that I don't think a > prescriptive API in the kernel could ever keep up with the things that > we want to be pushing from Gamescope/SteamOS. For example, we have so > many things going on, night mode, SDR gamut widening, HDR/SDR gain, > the ability to apply 'looks' for eg. invert luma or for retro looks, > enhanced contrast, tonemapping, inverse tonemapping... We also are > going to be doing a bunch of stuff with EETFs for handling out of > range HDR content for scanout. > > Some of what we do is kinda standard, regular "there is a paper on > this" algorithms, and others are not. > While yes, it might be very possible to do simple things, once you > start wanting to do something 'different', that's kinda lock-in. > > Whether this co-exists with arbitrary LUTs (that we definitely want > for SteamOS) or not: > I think putting a bunch of math-y stuff like this into the kernel is > probably the complete wrong approach. Everything would need to be > fixed point and it would be a huge pain in the butt to deal with on > that side. > > Maybe this is a "hot take", but IMO, DRM atomic is already waaay too > much being done in the kernel space. I think making it go even further > and having it be a prescriptive color API is a complete step in the > wrong direction. > > There is also the problem of... if there is a bug in the math here or > we want to add a new feature, if it's kernel side, you are locked in > to having that bug until the next release on your distro and probably > years if it's a new feature! > Updating kernels is much harder for 'enterprise' distros if it is not > mission critical. Having all of this in userspace is completely fine > however... > > If you want to make some userspace prescriptive -> descriptive color > library I am all for that for general case compositors, but I don't > think I would use something like that in Gamescope. > That's not to be rude, we are just picky and want freedom to do what > we want and iterate on it easily. > > I guess this all comes back to my initial point... having some > userspace to handle stuff that is either kinda or entirely vendor > specific is the right way of solving this problem :-P > > - Joshie ✨ > > On Thu, 11 May 2023 at 09:51, Karol Herbst wrote: > > > > On Wed, May 10, 2023 at 9:59 AM Jonas Ådahl wrote: > > > > > > On Tue, May 09, 2023 at 08:22:30PM +, Simon Ser wrote: > > > > On Tuesday, May 9th, 2023 at 21:53, Dave Airlie > > > > wrote: > > > > > > > > > There are also other vendor side effects to having this in userspace. > > > > > > > > > > Will the library have a loader? > > > > > Will it allow proprietary plugins? > > > > > Will it allow proprietary reimplementations? > > > > > What will happen when a vendor wants distros to ship their > > > > > proprietary fork of said library? > > > > > > > > > > How would NVIDIA integrate this with their proprietary stack? > > > > > > > > Since all color operations exposed by KMS are standard, the library > > > > would just be a simple one: no loader, no plugin, no proprietary pieces, > > > > etc. > > > > > > > > > > There might be pipelines/color-ops only exposed by proprietary out of > > > tree drivers; the operation types and semantics should ideally be &
Re: [RFC] Plane color pipeline KMS uAPI
On Tue, May 09, 2023 at 08:22:30PM +, Simon Ser wrote: > On Tuesday, May 9th, 2023 at 21:53, Dave Airlie wrote: > > > There are also other vendor side effects to having this in userspace. > > > > Will the library have a loader? > > Will it allow proprietary plugins? > > Will it allow proprietary reimplementations? > > What will happen when a vendor wants distros to ship their > > proprietary fork of said library? > > > > How would NVIDIA integrate this with their proprietary stack? > > Since all color operations exposed by KMS are standard, the library > would just be a simple one: no loader, no plugin, no proprietary pieces, > etc. > There might be pipelines/color-ops only exposed by proprietary out of tree drivers; the operation types and semantics should ideally be defined upstream, but the code paths would in practice be vendor specific, potentially without any upstream driver using them. It should be clear whether an implementation that makes such a pipeline work is in scope for the upstream library. The same applies to the kernel; it must be clear whether pipeline elements that potentially will only be exposed by out of tree drivers will be acceptable upstream, at least as documented operations. Jonas
Re: [RFC] Plane color pipeline KMS uAPI
On Mon, May 08, 2023 at 09:14:18AM +1000, Dave Airlie wrote: > On Sat, 6 May 2023 at 08:21, Sebastian Wick wrote: > > > > On Fri, May 5, 2023 at 10:40 PM Dave Airlie wrote: > > > > > > On Fri, 5 May 2023 at 01:23, Simon Ser wrote: > > > > > > > > Hi all, > > > > > > > > The goal of this RFC is to expose a generic KMS uAPI to configure the > > > > color > > > > pipeline before blending, ie. after a pixel is tapped from a plane's > > > > framebuffer and before it's blended with other planes. With this new > > > > uAPI we > > > > aim to reduce the battery life impact of color management and HDR on > > > > mobile > > > > devices, to improve performance and to decrease latency by skipping > > > > composition on the 3D engine. This proposal is the result of > > > > discussions at > > > > the Red Hat HDR hackfest [1] which took place a few days ago. Engineers > > > > familiar with the AMD, Intel and NVIDIA hardware have participated in > > > > the > > > > discussion. > > > > > > > > This proposal takes a prescriptive approach instead of a descriptive > > > > approach. > > > > Drivers describe the available hardware blocks in terms of low-level > > > > mathematical operations, then user-space configures each block. We > > > > decided > > > > against a descriptive approach where user-space would provide a > > > > high-level > > > > description of the colorspace and other parameters: we want to give more > > > > control and flexibility to user-space, e.g. to be able to replicate > > > > exactly the > > > > color pipeline with shaders and switch between shaders and KMS pipelines > > > > seamlessly, and to avoid forcing user-space into a particular color > > > > management > > > > policy. > > > > > > I'm not 100% sold on the prescriptive here, let's see if someone can > > > get me over the line with some questions later. > > > > > > My feeling is color pipeline hw is not a done deal, and that hw > > > vendors will be revising/evolving/churning the hw blocks for a while > > > longer, as there is no real standards in the area to aim for, all the > > > vendors are mostly just doing whatever gets Windows over the line and > > > keeps hw engineers happy. So I have some concerns here around forwards > > > compatibility and hence the API design. > > > > > > I guess my main concern is if you expose a bunch of hw blocks and > > > someone comes up with a novel new thing, will all existing userspace > > > work, without falling back to shaders? > > > Do we have minimum guarantees on what hardware blocks have to be > > > exposed to build a useable pipeline? > > > If a hardware block goes away in a new silicon revision, do I have to > > > rewrite my compositor? or will it be expected that the kernel will > > > emulate the old pipelines on top of whatever new fancy thing exists. > > > > I think there are two answers to those questions. > > These aren't selling me much better :-) > > > > The first one is that right now KMS already doesn't guarantee that > > every property is supported on all hardware. The guarantee we have is > > that properties that are supported on a piece of hardware on a > > specific kernel will be supported on the same hardware on later > > kernels. The color pipeline is no different here. For a specific piece > > of hardware a newer kernel might only change the pipelines in a > > backwards compatible way and add new pipelines. > > > > So to answer your question: if some hardware with a novel pipeline > > will show up it might not be supported and that's fine. We already > > have cases where some hardware does not support the gamma lut property > > but only the CSC property and that breaks night light because we never > > bothered to write a shader fallback. KMS provides ways to offload work > > but a generic user space always has to provide a fallback and this > > doesn't change. Hardware specific user space on the other hand will > > keep working with the forward compatibility guarantees we want to > > provide. > > In my mind we've screwed up already, isn't a case to be made for > continue down the same path. > > The kernel is meant to be a hardware abstraction layer, not just a > hardware exposure layer. The kernel shouldn't set policy and there are > cases where it can't act as an abstraction layer (like where you need > a compiler), but I'm not sold that this case is one of those yet. I'm > open to being educated here on why it would be. It would still be an abstraction of the hardware, just that the level of abstraction is a bit "lower" than your intuition currently tells you we should have. IMO it's not too different from the kernel providing low level input events describing what what the hardware can do and does, with a rather massive user space library (libinput) turning all of that low level nonsense to actual useful abstractions. In this case it's the other way around, the kernel provides vendor independent knobs that describe what the output hardware can do, and exactly how it
Re: [PATCH v2 1/8] drm: Disable the cursor plane on atomic contexts with virtualized drivers
On Thu, May 04, 2023 at 01:39:04PM +0300, Pekka Paalanen wrote: > On Thu, 4 May 2023 01:50:25 + > Zack Rusin wrote: > > > On Wed, 2023-05-03 at 09:48 +0200, Javier Martinez Canillas wrote: > > > Zack Rusin writes: > > > > > > > On Tue, 2023-05-02 at 11:32 +0200, Javier Martinez Canillas wrote: > > > > > > AFAICT this is the only remaining thing to be addressed for this > > > > > series ? > > > > > > > > No, there was more. tbh I haven't had the time to think about whether > > > > the above > > > > makes sense to me, e.g. I'm not sure if having virtualized drivers > > > > expose > > > > "support > > > > universal planes" and adding another plane which is not universal (the > > > > only > > > > "universal" plane on them being the default one) makes more sense than > > > > a flag > > > > that > > > > says "this driver requires a cursor in the cursor plane". There's > > > > certainly a > > > > huge > > > > difference in how userspace would be required to handle it and it's way > > > > uglier > > > > with > > > > two different cursor planes. i.e. there's a lot of ways in which this > > > > could be > > > > cleaner in the kernel but they all require significant changes to > > > > userspace, > > > > that go > > > > way beyond "attach hotspot info to this plane". I'd like to avoid > > > > approaches > > > > that > > > > mean running with atomic kms requires completely separate paths for > > > > virtualized > > > > drivers because no one will ever support and maintain it. > > > > > > > > It's not a trivial thing because it's fundamentally hard to untangle > > > > the fact > > > > the > > > > virtualized drivers have been advertising universal plane support > > > > without ever > > > > supporting universal planes. Especially because most new userspace in > > > > general > > > > checks > > > > for "universal planes" to expose atomic kms paths. > > > > > > > > > > After some discussion on the #dri-devel, your approach makes sense and the > > > only contention point is the name of the driver feature flag name. The one > > > you are using (DRIVER_VIRTUAL) seems to be too broad and generic (the fact > > > that vkms won't set and is a virtual driver as well, is a good example). > > > > > > Maybe something like DRIVER_CURSOR_HOTSPOT or DRIVER_CURSOR_COMMANDEERING > > > would be more accurate and self explanatory ? > > > > Sure, or even more verbose DRIVER_NEEDS_CURSOR_PLANE_HOTSPOT, but it sounds > > like > > Pekka doesn't agree with this approach. As I mentioned in my response to > > him, I'd be > > happy with any approach that gets paravirtualized drivers working with > > atomic kms, > > but atm I don't have enough time to be creating a new kernel subsystem or a > > new set > > of uapi's for paravirtualized drivers and then porting mutter/kwin to those. > > It seems I have not been clear enough, apologies. Once more, in short: > > Zack, I'm worried about this statement from you (copied from above): > > > > > I'd like to avoid approaches that mean running with atomic kms > > > > requires completely separate paths for virtualized drivers > > > > because no one will ever support and maintain it. > > It feels like you are intentionally limiting your own design options > for the fear of "no one will ever support it". I'm worried that over > the coming years, that will lead to a hard to use, hard to maintain > patchwork of vague or undocumented or just too many little UAPI details. > > Please, don't limit your designs. There are good reasons why nested KMS > drivers behave fundamentally differently to most KMS hardware drivers. > Userspace that does not or cannot take that into account is unavoidably > crippled. >From a compositor side, there is a valid reason to minimize the uAPI difference between "nested virtual machine" code paths and "running on actual hardware" code paths, which is to let virtual machines with a viewer connected to KMS act as a testing environment, rather than a production environment. Running a production environment in a virtual machine doesn't really need to use KMS at all. When using virtual machines for testing, I want to minimize the amount of differentation between running on hardware and running in the VM because otherwise the parts that are tested are not the same. I realize that hotpspots and the cursor moving viewer side contradicts that to some degree, but still, from a graphical testing witha VM point of view, one has to compromise, as testing isn't just for the KMS layer, but for the DE and distribution as a whole. Jonas > > > Thanks, > pq
Re: [PATCH v10 01/15] dma-buf/dma-fence: Add deadline awareness
On Fri, Mar 17, 2023 at 08:59:48AM -0700, Rob Clark wrote: > On Fri, Mar 17, 2023 at 3:23 AM Jonas Ådahl wrote: > > > > On Thu, Mar 16, 2023 at 09:28:55AM -0700, Rob Clark wrote: > > > On Thu, Mar 16, 2023 at 2:26 AM Jonas Ådahl wrote: > > > > > > > > On Wed, Mar 15, 2023 at 09:19:49AM -0700, Rob Clark wrote: > > > > > On Wed, Mar 15, 2023 at 6:53 AM Jonas Ådahl wrote: > > > > > > > > > > > > On Fri, Mar 10, 2023 at 09:38:18AM -0800, Rob Clark wrote: > > > > > > > On Fri, Mar 10, 2023 at 7:45 AM Jonas Ådahl > > > > > > > wrote: > > > > > > > > > > > > > > > > On Wed, Mar 08, 2023 at 07:52:52AM -0800, Rob Clark wrote: > > > > > > > > > From: Rob Clark > > > > > > > > > > > > > > > > > > Add a way to hint to the fence signaler of an upcoming > > > > > > > > > deadline, such as > > > > > > > > > vblank, which the fence waiter would prefer not to miss. > > > > > > > > > This is to aid > > > > > > > > > the fence signaler in making power management decisions, like > > > > > > > > > boosting > > > > > > > > > frequency as the deadline approaches and awareness of missing > > > > > > > > > deadlines > > > > > > > > > so that can be factored in to the frequency scaling. > > > > > > > > > > > > > > > > > > v2: Drop dma_fence::deadline and related logic to filter > > > > > > > > > duplicate > > > > > > > > > deadlines, to avoid increasing dma_fence size. The > > > > > > > > > fence-context > > > > > > > > > implementation will need similar logic to track deadlines > > > > > > > > > of all > > > > > > > > > the fences on the same timeline. [ckoenig] > > > > > > > > > v3: Clarify locking wrt. set_deadline callback > > > > > > > > > v4: Clarify in docs comment that this is a hint > > > > > > > > > v5: Drop DMA_FENCE_FLAG_HAS_DEADLINE_BIT. > > > > > > > > > v6: More docs > > > > > > > > > v7: Fix typo, clarify past deadlines > > > > > > > > > > > > > > > > > > Signed-off-by: Rob Clark > > > > > > > > > Reviewed-by: Christian König > > > > > > > > > Acked-by: Pekka Paalanen > > > > > > > > > Reviewed-by: Bagas Sanjaya > > > > > > > > > --- > > > > > > > > > > > > > > > > Hi Rob! > > > > > > > > > > > > > > > > > Documentation/driver-api/dma-buf.rst | 6 +++ > > > > > > > > > drivers/dma-buf/dma-fence.c | 59 > > > > > > > > > > > > > > > > > > include/linux/dma-fence.h| 22 +++ > > > > > > > > > 3 files changed, 87 insertions(+) > > > > > > > > > > > > > > > > > > diff --git a/Documentation/driver-api/dma-buf.rst > > > > > > > > > b/Documentation/driver-api/dma-buf.rst > > > > > > > > > index 622b8156d212..183e480d8cea 100644 > > > > > > > > > --- a/Documentation/driver-api/dma-buf.rst > > > > > > > > > +++ b/Documentation/driver-api/dma-buf.rst > > > > > > > > > @@ -164,6 +164,12 @@ DMA Fence Signalling Annotations > > > > > > > > > .. kernel-doc:: drivers/dma-buf/dma-fence.c > > > > > > > > > :doc: fence signalling annotation > > > > > > > > > > > > > > > > > > +DMA Fence Deadline Hints > > > > > > > > > + > > > > > > > > > + > > > > > > > > > +.. kernel-doc:: drivers/dma-buf/dma-fence.c > > > > > > > > > + :doc: deadline hints > > > > > > > > > + > > > > > > > > > DMA Fences Functions Reference > > > > > > > > >
Re: [PATCH v3 1/2] drm: Introduce plane SIZE_HINTS property
On Fri, Mar 17, 2023 at 03:15:56PM +0200, Ville Syrjälä wrote: > On Fri, Mar 17, 2023 at 01:29:13PM +0100, Jonas Ådahl wrote: > > On Fri, Mar 17, 2023 at 01:21:43PM +0100, Jonas Ådahl wrote: > > > On Fri, Mar 17, 2023 at 01:33:25PM +0200, Ville Syrjälä wrote: > > > > On Fri, Mar 17, 2023 at 11:34:16AM +0100, Jonas Ådahl wrote: > > > > > On Mon, Mar 13, 2023 at 06:33:11PM +0200, Ville Syrjala wrote: > > > > > > From: Ville Syrjälä > > > > > > > > > > > > Add a new immutable plane property by which a plane can advertise > > > > > > a handful of recommended plane sizes. This would be mostly exposed > > > > > > by cursor planes as a slightly more capable replacement for > > > > > > the DRM_CAP_CURSOR_WIDTH/HEIGHT caps, which can only declare > > > > > > a one size fits all limit for the whole device. > > > > > > > > > > > > Currently eg. amdgpu/i915/nouveau just advertize the max cursor > > > > > > size via the cursor size caps. But always using the max sized > > > > > > cursor can waste a surprising amount of power, so a better > > > > > > stragey is desirable. > > > > > > > > > > > > Most other drivers don't specify any cursor size at all, in > > > > > > which case the ioctl code just claims that 64x64 is a great > > > > > > choice. Whether that is actually true is debatable. > > > > > > > > > > > > A poll of various compositor developers informs us that > > > > > > blindly probing with setcursor/atomic ioctl to determine > > > > > > suitable cursor sizes is not acceptable, thus the > > > > > > introduction of the new property to supplant the cursor > > > > > > size caps. The compositor will now be free to select a > > > > > > more optimal cursor size from the short list of options. > > > > > > > > > > > > Note that the reported sizes (either via the property or the > > > > > > caps) make no claims about things such as plane scaling. So > > > > > > these things should only really be consulted for simple > > > > > > "cursor like" use cases. > > > > > > > > > > > > v2: Try to add some docs > > > > > > v3: Specify that value 0 is reserved for future use (basic idea > > > > > > from Jonas) > > > > > > Drop the note about typical hardware (Pekka) > > > > > > > > > > > > Cc: Simon Ser > > > > > > Cc: Jonas Ådahl > > > > > > Cc: Daniel Stone > > > > > > Cc: Pekka Paalanen > > > > > > Acked-by: Harry Wentland > > > > > > Acked-by: Pekka Paalanen > > > > > > Signed-off-by: Ville Syrjälä > > > > > > --- > > > > > > drivers/gpu/drm/drm_mode_config.c | 7 > > > > > > drivers/gpu/drm/drm_plane.c | 53 > > > > > > +++ > > > > > > include/drm/drm_mode_config.h | 5 +++ > > > > > > include/drm/drm_plane.h | 4 +++ > > > > > > include/uapi/drm/drm_mode.h | 11 +++ > > > > > > 5 files changed, 80 insertions(+) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_mode_config.c > > > > > > b/drivers/gpu/drm/drm_mode_config.c > > > > > > index 87eb591fe9b5..21860f94a18c 100644 > > > > > > --- a/drivers/gpu/drm/drm_mode_config.c > > > > > > +++ b/drivers/gpu/drm/drm_mode_config.c > > > > > > @@ -374,6 +374,13 @@ static int > > > > > > drm_mode_create_standard_properties(struct drm_device *dev) > > > > > > return -ENOMEM; > > > > > > dev->mode_config.modifiers_property = prop; > > > > > > > > > > > > + prop = drm_property_create(dev, > > > > > > + DRM_MODE_PROP_IMMUTABLE | > > > > > > DRM_MODE_PROP_BLOB, > > > > > > + "SIZE_HINTS", 0); > > > > > > + if (!prop) > > > > > > + return -ENOMEM; > > > > > > + dev->mode_config.size_hints_property = prop; > > > > > > + > >
Re: [PATCH v3 1/2] drm: Introduce plane SIZE_HINTS property
On Fri, Mar 17, 2023 at 01:21:43PM +0100, Jonas Ådahl wrote: > On Fri, Mar 17, 2023 at 01:33:25PM +0200, Ville Syrjälä wrote: > > On Fri, Mar 17, 2023 at 11:34:16AM +0100, Jonas Ådahl wrote: > > > On Mon, Mar 13, 2023 at 06:33:11PM +0200, Ville Syrjala wrote: > > > > From: Ville Syrjälä > > > > > > > > Add a new immutable plane property by which a plane can advertise > > > > a handful of recommended plane sizes. This would be mostly exposed > > > > by cursor planes as a slightly more capable replacement for > > > > the DRM_CAP_CURSOR_WIDTH/HEIGHT caps, which can only declare > > > > a one size fits all limit for the whole device. > > > > > > > > Currently eg. amdgpu/i915/nouveau just advertize the max cursor > > > > size via the cursor size caps. But always using the max sized > > > > cursor can waste a surprising amount of power, so a better > > > > stragey is desirable. > > > > > > > > Most other drivers don't specify any cursor size at all, in > > > > which case the ioctl code just claims that 64x64 is a great > > > > choice. Whether that is actually true is debatable. > > > > > > > > A poll of various compositor developers informs us that > > > > blindly probing with setcursor/atomic ioctl to determine > > > > suitable cursor sizes is not acceptable, thus the > > > > introduction of the new property to supplant the cursor > > > > size caps. The compositor will now be free to select a > > > > more optimal cursor size from the short list of options. > > > > > > > > Note that the reported sizes (either via the property or the > > > > caps) make no claims about things such as plane scaling. So > > > > these things should only really be consulted for simple > > > > "cursor like" use cases. > > > > > > > > v2: Try to add some docs > > > > v3: Specify that value 0 is reserved for future use (basic idea from > > > > Jonas) > > > > Drop the note about typical hardware (Pekka) > > > > > > > > Cc: Simon Ser > > > > Cc: Jonas Ådahl > > > > Cc: Daniel Stone > > > > Cc: Pekka Paalanen > > > > Acked-by: Harry Wentland > > > > Acked-by: Pekka Paalanen > > > > Signed-off-by: Ville Syrjälä > > > > --- > > > > drivers/gpu/drm/drm_mode_config.c | 7 > > > > drivers/gpu/drm/drm_plane.c | 53 +++ > > > > include/drm/drm_mode_config.h | 5 +++ > > > > include/drm/drm_plane.h | 4 +++ > > > > include/uapi/drm/drm_mode.h | 11 +++ > > > > 5 files changed, 80 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/drm_mode_config.c > > > > b/drivers/gpu/drm/drm_mode_config.c > > > > index 87eb591fe9b5..21860f94a18c 100644 > > > > --- a/drivers/gpu/drm/drm_mode_config.c > > > > +++ b/drivers/gpu/drm/drm_mode_config.c > > > > @@ -374,6 +374,13 @@ static int > > > > drm_mode_create_standard_properties(struct drm_device *dev) > > > > return -ENOMEM; > > > > dev->mode_config.modifiers_property = prop; > > > > > > > > + prop = drm_property_create(dev, > > > > + DRM_MODE_PROP_IMMUTABLE | > > > > DRM_MODE_PROP_BLOB, > > > > + "SIZE_HINTS", 0); > > > > + if (!prop) > > > > + return -ENOMEM; > > > > + dev->mode_config.size_hints_property = prop; > > > > + > > > > return 0; > > > > } > > > > > > > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c > > > > index 24e7998d1731..d2a6fdff1a57 100644 > > > > --- a/drivers/gpu/drm/drm_plane.c > > > > +++ b/drivers/gpu/drm/drm_plane.c > > > > @@ -140,6 +140,26 @@ > > > > * DRM_FORMAT_MOD_LINEAR. Before linux kernel release v5.1 there > > > > have been > > > > * various bugs in this area with inconsistencies between the > > > > capability > > > > * flag and per-plane properties. > > > > + * > > > > + * SIZE_HINTS: > > > > + * Blob property which contains the set of recommended plane size > > > &
Re: [PATCH v3 1/2] drm: Introduce plane SIZE_HINTS property
On Fri, Mar 17, 2023 at 01:33:25PM +0200, Ville Syrjälä wrote: > On Fri, Mar 17, 2023 at 11:34:16AM +0100, Jonas Ådahl wrote: > > On Mon, Mar 13, 2023 at 06:33:11PM +0200, Ville Syrjala wrote: > > > From: Ville Syrjälä > > > > > > Add a new immutable plane property by which a plane can advertise > > > a handful of recommended plane sizes. This would be mostly exposed > > > by cursor planes as a slightly more capable replacement for > > > the DRM_CAP_CURSOR_WIDTH/HEIGHT caps, which can only declare > > > a one size fits all limit for the whole device. > > > > > > Currently eg. amdgpu/i915/nouveau just advertize the max cursor > > > size via the cursor size caps. But always using the max sized > > > cursor can waste a surprising amount of power, so a better > > > stragey is desirable. > > > > > > Most other drivers don't specify any cursor size at all, in > > > which case the ioctl code just claims that 64x64 is a great > > > choice. Whether that is actually true is debatable. > > > > > > A poll of various compositor developers informs us that > > > blindly probing with setcursor/atomic ioctl to determine > > > suitable cursor sizes is not acceptable, thus the > > > introduction of the new property to supplant the cursor > > > size caps. The compositor will now be free to select a > > > more optimal cursor size from the short list of options. > > > > > > Note that the reported sizes (either via the property or the > > > caps) make no claims about things such as plane scaling. So > > > these things should only really be consulted for simple > > > "cursor like" use cases. > > > > > > v2: Try to add some docs > > > v3: Specify that value 0 is reserved for future use (basic idea from > > > Jonas) > > > Drop the note about typical hardware (Pekka) > > > > > > Cc: Simon Ser > > > Cc: Jonas Ådahl > > > Cc: Daniel Stone > > > Cc: Pekka Paalanen > > > Acked-by: Harry Wentland > > > Acked-by: Pekka Paalanen > > > Signed-off-by: Ville Syrjälä > > > --- > > > drivers/gpu/drm/drm_mode_config.c | 7 > > > drivers/gpu/drm/drm_plane.c | 53 +++ > > > include/drm/drm_mode_config.h | 5 +++ > > > include/drm/drm_plane.h | 4 +++ > > > include/uapi/drm/drm_mode.h | 11 +++ > > > 5 files changed, 80 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_mode_config.c > > > b/drivers/gpu/drm/drm_mode_config.c > > > index 87eb591fe9b5..21860f94a18c 100644 > > > --- a/drivers/gpu/drm/drm_mode_config.c > > > +++ b/drivers/gpu/drm/drm_mode_config.c > > > @@ -374,6 +374,13 @@ static int > > > drm_mode_create_standard_properties(struct drm_device *dev) > > > return -ENOMEM; > > > dev->mode_config.modifiers_property = prop; > > > > > > + prop = drm_property_create(dev, > > > +DRM_MODE_PROP_IMMUTABLE | DRM_MODE_PROP_BLOB, > > > +"SIZE_HINTS", 0); > > > + if (!prop) > > > + return -ENOMEM; > > > + dev->mode_config.size_hints_property = prop; > > > + > > > return 0; > > > } > > > > > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c > > > index 24e7998d1731..d2a6fdff1a57 100644 > > > --- a/drivers/gpu/drm/drm_plane.c > > > +++ b/drivers/gpu/drm/drm_plane.c > > > @@ -140,6 +140,26 @@ > > > * DRM_FORMAT_MOD_LINEAR. Before linux kernel release v5.1 there > > > have been > > > * various bugs in this area with inconsistencies between the > > > capability > > > * flag and per-plane properties. > > > + * > > > + * SIZE_HINTS: > > > + * Blob property which contains the set of recommended plane size > > > + * which can used for simple "cursor like" use cases (eg. no > > > scaling). > > > + * Using these hints frees userspace from extensive probing of > > > + * supported plane sizes through atomic/setcursor ioctls. > > > + * > > > + * For optimal usage userspace should pick the smallest size > > > + * that satisfies its own requirements. > > > + * > > > + * The blob contains an array of struct drm_plane_size_hint. > > > + * > > > + *
Re: [PATCH v3 1/2] drm: Introduce plane SIZE_HINTS property
On Mon, Mar 13, 2023 at 06:33:11PM +0200, Ville Syrjala wrote: > From: Ville Syrjälä > > Add a new immutable plane property by which a plane can advertise > a handful of recommended plane sizes. This would be mostly exposed > by cursor planes as a slightly more capable replacement for > the DRM_CAP_CURSOR_WIDTH/HEIGHT caps, which can only declare > a one size fits all limit for the whole device. > > Currently eg. amdgpu/i915/nouveau just advertize the max cursor > size via the cursor size caps. But always using the max sized > cursor can waste a surprising amount of power, so a better > stragey is desirable. > > Most other drivers don't specify any cursor size at all, in > which case the ioctl code just claims that 64x64 is a great > choice. Whether that is actually true is debatable. > > A poll of various compositor developers informs us that > blindly probing with setcursor/atomic ioctl to determine > suitable cursor sizes is not acceptable, thus the > introduction of the new property to supplant the cursor > size caps. The compositor will now be free to select a > more optimal cursor size from the short list of options. > > Note that the reported sizes (either via the property or the > caps) make no claims about things such as plane scaling. So > these things should only really be consulted for simple > "cursor like" use cases. > > v2: Try to add some docs > v3: Specify that value 0 is reserved for future use (basic idea from Jonas) > Drop the note about typical hardware (Pekka) > > Cc: Simon Ser > Cc: Jonas Ådahl > Cc: Daniel Stone > Cc: Pekka Paalanen > Acked-by: Harry Wentland > Acked-by: Pekka Paalanen > Signed-off-by: Ville Syrjälä > --- > drivers/gpu/drm/drm_mode_config.c | 7 > drivers/gpu/drm/drm_plane.c | 53 +++ > include/drm/drm_mode_config.h | 5 +++ > include/drm/drm_plane.h | 4 +++ > include/uapi/drm/drm_mode.h | 11 +++ > 5 files changed, 80 insertions(+) > > diff --git a/drivers/gpu/drm/drm_mode_config.c > b/drivers/gpu/drm/drm_mode_config.c > index 87eb591fe9b5..21860f94a18c 100644 > --- a/drivers/gpu/drm/drm_mode_config.c > +++ b/drivers/gpu/drm/drm_mode_config.c > @@ -374,6 +374,13 @@ static int drm_mode_create_standard_properties(struct > drm_device *dev) > return -ENOMEM; > dev->mode_config.modifiers_property = prop; > > + prop = drm_property_create(dev, > +DRM_MODE_PROP_IMMUTABLE | DRM_MODE_PROP_BLOB, > +"SIZE_HINTS", 0); > + if (!prop) > + return -ENOMEM; > + dev->mode_config.size_hints_property = prop; > + > return 0; > } > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c > index 24e7998d1731..d2a6fdff1a57 100644 > --- a/drivers/gpu/drm/drm_plane.c > +++ b/drivers/gpu/drm/drm_plane.c > @@ -140,6 +140,26 @@ > * DRM_FORMAT_MOD_LINEAR. Before linux kernel release v5.1 there have > been > * various bugs in this area with inconsistencies between the capability > * flag and per-plane properties. > + * > + * SIZE_HINTS: > + * Blob property which contains the set of recommended plane size > + * which can used for simple "cursor like" use cases (eg. no scaling). > + * Using these hints frees userspace from extensive probing of > + * supported plane sizes through atomic/setcursor ioctls. > + * > + * For optimal usage userspace should pick the smallest size > + * that satisfies its own requirements. > + * > + * The blob contains an array of struct drm_plane_size_hint. > + * > + * Drivers should only attach this property to planes that > + * support a very limited set of sizes. > + * > + * Note that property value 0 (ie. no blob) is reserved for potential > + * future use. Current userspace is expected to ignore the property > + * if the value is 0, and fall back to some other means (eg. > + * _CAP_CURSOR_WIDTH and _CAP_CURSOR_HEIGHT) to determine > + * the appropriate plane size to use. Does this intend to mean userspace has the kernel's blessing on choosing an arbitrary size as long as it's smaller than _CAP_CURSOR_WIDTH x _CAP_CURSOR_HEIGHT? It's a bit to vague for me to make a confident interpretation whether I can, or whether I should pretend I didn't see SIZE_HINTS and apply the old logic, meaning only using the exact cap size. Jonas > */ > > static unsigned int drm_num_planes(struct drm_device *dev) > @@ -1582,3 +1602,36 @@ int drm_plane_create_scaling_filter_property(struct > drm_plane *plane, > return 0; > } > EXP
Re: [PATCH v10 01/15] dma-buf/dma-fence: Add deadline awareness
On Thu, Mar 16, 2023 at 09:28:55AM -0700, Rob Clark wrote: > On Thu, Mar 16, 2023 at 2:26 AM Jonas Ådahl wrote: > > > > On Wed, Mar 15, 2023 at 09:19:49AM -0700, Rob Clark wrote: > > > On Wed, Mar 15, 2023 at 6:53 AM Jonas Ådahl wrote: > > > > > > > > On Fri, Mar 10, 2023 at 09:38:18AM -0800, Rob Clark wrote: > > > > > On Fri, Mar 10, 2023 at 7:45 AM Jonas Ådahl wrote: > > > > > > > > > > > > On Wed, Mar 08, 2023 at 07:52:52AM -0800, Rob Clark wrote: > > > > > > > From: Rob Clark > > > > > > > > > > > > > > Add a way to hint to the fence signaler of an upcoming deadline, > > > > > > > such as > > > > > > > vblank, which the fence waiter would prefer not to miss. This is > > > > > > > to aid > > > > > > > the fence signaler in making power management decisions, like > > > > > > > boosting > > > > > > > frequency as the deadline approaches and awareness of missing > > > > > > > deadlines > > > > > > > so that can be factored in to the frequency scaling. > > > > > > > > > > > > > > v2: Drop dma_fence::deadline and related logic to filter duplicate > > > > > > > deadlines, to avoid increasing dma_fence size. The > > > > > > > fence-context > > > > > > > implementation will need similar logic to track deadlines of > > > > > > > all > > > > > > > the fences on the same timeline. [ckoenig] > > > > > > > v3: Clarify locking wrt. set_deadline callback > > > > > > > v4: Clarify in docs comment that this is a hint > > > > > > > v5: Drop DMA_FENCE_FLAG_HAS_DEADLINE_BIT. > > > > > > > v6: More docs > > > > > > > v7: Fix typo, clarify past deadlines > > > > > > > > > > > > > > Signed-off-by: Rob Clark > > > > > > > Reviewed-by: Christian König > > > > > > > Acked-by: Pekka Paalanen > > > > > > > Reviewed-by: Bagas Sanjaya > > > > > > > --- > > > > > > > > > > > > Hi Rob! > > > > > > > > > > > > > Documentation/driver-api/dma-buf.rst | 6 +++ > > > > > > > drivers/dma-buf/dma-fence.c | 59 > > > > > > > > > > > > > > include/linux/dma-fence.h| 22 +++ > > > > > > > 3 files changed, 87 insertions(+) > > > > > > > > > > > > > > diff --git a/Documentation/driver-api/dma-buf.rst > > > > > > > b/Documentation/driver-api/dma-buf.rst > > > > > > > index 622b8156d212..183e480d8cea 100644 > > > > > > > --- a/Documentation/driver-api/dma-buf.rst > > > > > > > +++ b/Documentation/driver-api/dma-buf.rst > > > > > > > @@ -164,6 +164,12 @@ DMA Fence Signalling Annotations > > > > > > > .. kernel-doc:: drivers/dma-buf/dma-fence.c > > > > > > > :doc: fence signalling annotation > > > > > > > > > > > > > > +DMA Fence Deadline Hints > > > > > > > + > > > > > > > + > > > > > > > +.. kernel-doc:: drivers/dma-buf/dma-fence.c > > > > > > > + :doc: deadline hints > > > > > > > + > > > > > > > DMA Fences Functions Reference > > > > > > > ~~ > > > > > > > > > > > > > > diff --git a/drivers/dma-buf/dma-fence.c > > > > > > > b/drivers/dma-buf/dma-fence.c > > > > > > > index 0de0482cd36e..f177c56269bb 100644 > > > > > > > --- a/drivers/dma-buf/dma-fence.c > > > > > > > +++ b/drivers/dma-buf/dma-fence.c > > > > > > > @@ -912,6 +912,65 @@ dma_fence_wait_any_timeout(struct dma_fence > > > > > > > **fences, uint32_t count, > > > > > > > } > > > > > > > EXPORT_SYMBOL(dma_fence_wait_any_timeout); > > > > > > > > > > > > > > +/** > > > > > > > + * DOC: deadline hints > > > > > > > + * > &
Re: [PATCH v10 01/15] dma-buf/dma-fence: Add deadline awareness
On Wed, Mar 15, 2023 at 09:19:49AM -0700, Rob Clark wrote: > On Wed, Mar 15, 2023 at 6:53 AM Jonas Ådahl wrote: > > > > On Fri, Mar 10, 2023 at 09:38:18AM -0800, Rob Clark wrote: > > > On Fri, Mar 10, 2023 at 7:45 AM Jonas Ådahl wrote: > > > > > > > > On Wed, Mar 08, 2023 at 07:52:52AM -0800, Rob Clark wrote: > > > > > From: Rob Clark > > > > > > > > > > Add a way to hint to the fence signaler of an upcoming deadline, such > > > > > as > > > > > vblank, which the fence waiter would prefer not to miss. This is to > > > > > aid > > > > > the fence signaler in making power management decisions, like boosting > > > > > frequency as the deadline approaches and awareness of missing > > > > > deadlines > > > > > so that can be factored in to the frequency scaling. > > > > > > > > > > v2: Drop dma_fence::deadline and related logic to filter duplicate > > > > > deadlines, to avoid increasing dma_fence size. The fence-context > > > > > implementation will need similar logic to track deadlines of all > > > > > the fences on the same timeline. [ckoenig] > > > > > v3: Clarify locking wrt. set_deadline callback > > > > > v4: Clarify in docs comment that this is a hint > > > > > v5: Drop DMA_FENCE_FLAG_HAS_DEADLINE_BIT. > > > > > v6: More docs > > > > > v7: Fix typo, clarify past deadlines > > > > > > > > > > Signed-off-by: Rob Clark > > > > > Reviewed-by: Christian König > > > > > Acked-by: Pekka Paalanen > > > > > Reviewed-by: Bagas Sanjaya > > > > > --- > > > > > > > > Hi Rob! > > > > > > > > > Documentation/driver-api/dma-buf.rst | 6 +++ > > > > > drivers/dma-buf/dma-fence.c | 59 > > > > > > > > > > include/linux/dma-fence.h| 22 +++ > > > > > 3 files changed, 87 insertions(+) > > > > > > > > > > diff --git a/Documentation/driver-api/dma-buf.rst > > > > > b/Documentation/driver-api/dma-buf.rst > > > > > index 622b8156d212..183e480d8cea 100644 > > > > > --- a/Documentation/driver-api/dma-buf.rst > > > > > +++ b/Documentation/driver-api/dma-buf.rst > > > > > @@ -164,6 +164,12 @@ DMA Fence Signalling Annotations > > > > > .. kernel-doc:: drivers/dma-buf/dma-fence.c > > > > > :doc: fence signalling annotation > > > > > > > > > > +DMA Fence Deadline Hints > > > > > + > > > > > + > > > > > +.. kernel-doc:: drivers/dma-buf/dma-fence.c > > > > > + :doc: deadline hints > > > > > + > > > > > DMA Fences Functions Reference > > > > > ~~ > > > > > > > > > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > > > > > index 0de0482cd36e..f177c56269bb 100644 > > > > > --- a/drivers/dma-buf/dma-fence.c > > > > > +++ b/drivers/dma-buf/dma-fence.c > > > > > @@ -912,6 +912,65 @@ dma_fence_wait_any_timeout(struct dma_fence > > > > > **fences, uint32_t count, > > > > > } > > > > > EXPORT_SYMBOL(dma_fence_wait_any_timeout); > > > > > > > > > > +/** > > > > > + * DOC: deadline hints > > > > > + * > > > > > + * In an ideal world, it would be possible to pipeline a workload > > > > > sufficiently > > > > > + * that a utilization based device frequency governor could arrive > > > > > at a minimum > > > > > + * frequency that meets the requirements of the use-case, in order > > > > > to minimize > > > > > + * power consumption. But in the real world there are many > > > > > workloads which > > > > > + * defy this ideal. For example, but not limited to: > > > > > + * > > > > > + * * Workloads that ping-pong between device and CPU, with > > > > > alternating periods > > > > > + * of CPU waiting for device, and device waiting on CPU. This can > > > > > result in > > > > > + * devfreq and cpufreq seeing idle time in their respective >
Re: [PATCH v10 01/15] dma-buf/dma-fence: Add deadline awareness
On Fri, Mar 10, 2023 at 09:38:18AM -0800, Rob Clark wrote: > On Fri, Mar 10, 2023 at 7:45 AM Jonas Ådahl wrote: > > > > On Wed, Mar 08, 2023 at 07:52:52AM -0800, Rob Clark wrote: > > > From: Rob Clark > > > > > > Add a way to hint to the fence signaler of an upcoming deadline, such as > > > vblank, which the fence waiter would prefer not to miss. This is to aid > > > the fence signaler in making power management decisions, like boosting > > > frequency as the deadline approaches and awareness of missing deadlines > > > so that can be factored in to the frequency scaling. > > > > > > v2: Drop dma_fence::deadline and related logic to filter duplicate > > > deadlines, to avoid increasing dma_fence size. The fence-context > > > implementation will need similar logic to track deadlines of all > > > the fences on the same timeline. [ckoenig] > > > v3: Clarify locking wrt. set_deadline callback > > > v4: Clarify in docs comment that this is a hint > > > v5: Drop DMA_FENCE_FLAG_HAS_DEADLINE_BIT. > > > v6: More docs > > > v7: Fix typo, clarify past deadlines > > > > > > Signed-off-by: Rob Clark > > > Reviewed-by: Christian König > > > Acked-by: Pekka Paalanen > > > Reviewed-by: Bagas Sanjaya > > > --- > > > > Hi Rob! > > > > > Documentation/driver-api/dma-buf.rst | 6 +++ > > > drivers/dma-buf/dma-fence.c | 59 > > > include/linux/dma-fence.h| 22 +++ > > > 3 files changed, 87 insertions(+) > > > > > > diff --git a/Documentation/driver-api/dma-buf.rst > > > b/Documentation/driver-api/dma-buf.rst > > > index 622b8156d212..183e480d8cea 100644 > > > --- a/Documentation/driver-api/dma-buf.rst > > > +++ b/Documentation/driver-api/dma-buf.rst > > > @@ -164,6 +164,12 @@ DMA Fence Signalling Annotations > > > .. kernel-doc:: drivers/dma-buf/dma-fence.c > > > :doc: fence signalling annotation > > > > > > +DMA Fence Deadline Hints > > > + > > > + > > > +.. kernel-doc:: drivers/dma-buf/dma-fence.c > > > + :doc: deadline hints > > > + > > > DMA Fences Functions Reference > > > ~~ > > > > > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > > > index 0de0482cd36e..f177c56269bb 100644 > > > --- a/drivers/dma-buf/dma-fence.c > > > +++ b/drivers/dma-buf/dma-fence.c > > > @@ -912,6 +912,65 @@ dma_fence_wait_any_timeout(struct dma_fence > > > **fences, uint32_t count, > > > } > > > EXPORT_SYMBOL(dma_fence_wait_any_timeout); > > > > > > +/** > > > + * DOC: deadline hints > > > + * > > > + * In an ideal world, it would be possible to pipeline a workload > > > sufficiently > > > + * that a utilization based device frequency governor could arrive at a > > > minimum > > > + * frequency that meets the requirements of the use-case, in order to > > > minimize > > > + * power consumption. But in the real world there are many workloads > > > which > > > + * defy this ideal. For example, but not limited to: > > > + * > > > + * * Workloads that ping-pong between device and CPU, with alternating > > > periods > > > + * of CPU waiting for device, and device waiting on CPU. This can > > > result in > > > + * devfreq and cpufreq seeing idle time in their respective domains > > > and in > > > + * result reduce frequency. > > > + * > > > + * * Workloads that interact with a periodic time based deadline, such > > > as double > > > + * buffered GPU rendering vs vblank sync'd page flipping. In this > > > scenario, > > > + * missing a vblank deadline results in an *increase* in idle time on > > > the GPU > > > + * (since it has to wait an additional vblank period), sending a > > > signal to > > > + * the GPU's devfreq to reduce frequency, when in fact the opposite is > > > what is > > > + * needed. > > > > This is the use case I'd like to get some better understanding about how > > this series intends to work, as the problematic scheduling behavior > > triggered by missed deadlines has plagued compositing display servers > > for a long time. > > > > I apologize, I'm not a GPU driver developer, nor an OpenGL driv
Re: [PATCH v10 01/15] dma-buf/dma-fence: Add deadline awareness
On Wed, Mar 08, 2023 at 07:52:52AM -0800, Rob Clark wrote: > From: Rob Clark > > Add a way to hint to the fence signaler of an upcoming deadline, such as > vblank, which the fence waiter would prefer not to miss. This is to aid > the fence signaler in making power management decisions, like boosting > frequency as the deadline approaches and awareness of missing deadlines > so that can be factored in to the frequency scaling. > > v2: Drop dma_fence::deadline and related logic to filter duplicate > deadlines, to avoid increasing dma_fence size. The fence-context > implementation will need similar logic to track deadlines of all > the fences on the same timeline. [ckoenig] > v3: Clarify locking wrt. set_deadline callback > v4: Clarify in docs comment that this is a hint > v5: Drop DMA_FENCE_FLAG_HAS_DEADLINE_BIT. > v6: More docs > v7: Fix typo, clarify past deadlines > > Signed-off-by: Rob Clark > Reviewed-by: Christian König > Acked-by: Pekka Paalanen > Reviewed-by: Bagas Sanjaya > --- Hi Rob! > Documentation/driver-api/dma-buf.rst | 6 +++ > drivers/dma-buf/dma-fence.c | 59 > include/linux/dma-fence.h| 22 +++ > 3 files changed, 87 insertions(+) > > diff --git a/Documentation/driver-api/dma-buf.rst > b/Documentation/driver-api/dma-buf.rst > index 622b8156d212..183e480d8cea 100644 > --- a/Documentation/driver-api/dma-buf.rst > +++ b/Documentation/driver-api/dma-buf.rst > @@ -164,6 +164,12 @@ DMA Fence Signalling Annotations > .. kernel-doc:: drivers/dma-buf/dma-fence.c > :doc: fence signalling annotation > > +DMA Fence Deadline Hints > + > + > +.. kernel-doc:: drivers/dma-buf/dma-fence.c > + :doc: deadline hints > + > DMA Fences Functions Reference > ~~ > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > index 0de0482cd36e..f177c56269bb 100644 > --- a/drivers/dma-buf/dma-fence.c > +++ b/drivers/dma-buf/dma-fence.c > @@ -912,6 +912,65 @@ dma_fence_wait_any_timeout(struct dma_fence **fences, > uint32_t count, > } > EXPORT_SYMBOL(dma_fence_wait_any_timeout); > > +/** > + * DOC: deadline hints > + * > + * In an ideal world, it would be possible to pipeline a workload > sufficiently > + * that a utilization based device frequency governor could arrive at a > minimum > + * frequency that meets the requirements of the use-case, in order to > minimize > + * power consumption. But in the real world there are many workloads which > + * defy this ideal. For example, but not limited to: > + * > + * * Workloads that ping-pong between device and CPU, with alternating > periods > + * of CPU waiting for device, and device waiting on CPU. This can result > in > + * devfreq and cpufreq seeing idle time in their respective domains and in > + * result reduce frequency. > + * > + * * Workloads that interact with a periodic time based deadline, such as > double > + * buffered GPU rendering vs vblank sync'd page flipping. In this > scenario, > + * missing a vblank deadline results in an *increase* in idle time on the > GPU > + * (since it has to wait an additional vblank period), sending a signal to > + * the GPU's devfreq to reduce frequency, when in fact the opposite is > what is > + * needed. This is the use case I'd like to get some better understanding about how this series intends to work, as the problematic scheduling behavior triggered by missed deadlines has plagued compositing display servers for a long time. I apologize, I'm not a GPU driver developer, nor an OpenGL driver developer, so I will need some hand holding when it comes to understanding exactly what piece of software is responsible for communicating what piece of information. > + * > + * To this end, deadline hint(s) can be set on a _fence via > _fence_set_deadline. > + * The deadline hint provides a way for the waiting driver, or userspace, to > + * convey an appropriate sense of urgency to the signaling driver. > + * > + * A deadline hint is given in absolute ktime (CLOCK_MONOTONIC for userspace > + * facing APIs). The time could either be some point in the future (such as > + * the vblank based deadline for page-flipping, or the start of a > compositor's > + * composition cycle), or the current time to indicate an immediate deadline > + * hint (Ie. forward progress cannot be made until this fence is signaled). Is it guaranteed that a GPU driver will use the actual start of the vblank as the effective deadline? I have some memories of seing something about vblank evasion browsing driver code, which I might have misunderstood, but I have yet to find whether this is something userspace can actually expect to be something it can rely on. Can userspace set a deadline that targets the next vblank deadline before GPU work has been flushed e.g. at the start of a paint cycle, and still be sure that the kernel has the information it needs to know it
Re: [PATCH v2 1/2] drm: Introduce plane SIZE_HINTS property
On Tue, Feb 14, 2023 at 12:28:44PM +0200, Ville Syrjälä wrote: > On Tue, Feb 14, 2023 at 10:54:27AM +0100, Jonas Ådahl wrote: > > On Tue, Feb 14, 2023 at 11:25:56AM +0200, Ville Syrjälä wrote: > > > On Thu, Feb 09, 2023 at 03:16:23PM +0100, Jonas Ådahl wrote: > > > > On Wed, Feb 08, 2023 at 11:10:16PM +0200, Ville Syrjala wrote: > > > > > From: Ville Syrjälä > > > > > > > > > > Add a new immutable plane property by which a plane can advertise > > > > > a handful of recommended plane sizes. This would be mostly exposed > > > > > by cursor planes as a slightly more capable replacement for > > > > > the DRM_CAP_CURSOR_WIDTH/HEIGHT caps, which can only declare > > > > > a one size fits all limit for the whole device. > > > > > > > > > > Currently eg. amdgpu/i915/nouveau just advertize the max cursor > > > > > size via the cursor size caps. But always using the max sized > > > > > cursor can waste a surprising amount of power, so a better > > > > > stragey is desirable. > > > > > > > > > > Most other drivers don't specify any cursor size at all, in > > > > > which case the ioctl code just claims that 64x64 is a great > > > > > choice. Whether that is actually true is debatable. > > > > > > > > > > A poll of various compositor developers informs us that > > > > > blindly probing with setcursor/atomic ioctl to determine > > > > > suitable cursor sizes is not acceptable, thus the > > > > > introduction of the new property to supplant the cursor > > > > > size caps. The compositor will now be free to select a > > > > > more optimal cursor size from the short list of options. > > > > > > > > > > Note that the reported sizes (either via the property or the > > > > > caps) make no claims about things such as plane scaling. So > > > > > these things should only really be consulted for simple > > > > > "cursor like" use cases. > > > > > > > > > > v2: Try to add some docs > > > > > > > > > > Cc: Simon Ser > > > > > Cc: Jonas Ådahl > > > > > Cc: Daniel Stone > > > > > Cc: Pekka Paalanen > > > > > Acked-by: Harry Wentland > > > > > Signed-off-by: Ville Syrjälä > > > > > --- > > > > > drivers/gpu/drm/drm_mode_config.c | 7 + > > > > > drivers/gpu/drm/drm_plane.c | 48 > > > > > +++ > > > > > include/drm/drm_mode_config.h | 5 > > > > > include/drm/drm_plane.h | 4 +++ > > > > > include/uapi/drm/drm_mode.h | 11 +++ > > > > > 5 files changed, 75 insertions(+) > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_mode_config.c > > > > > b/drivers/gpu/drm/drm_mode_config.c > > > > > index 87eb591fe9b5..21860f94a18c 100644 > > > > > --- a/drivers/gpu/drm/drm_mode_config.c > > > > > +++ b/drivers/gpu/drm/drm_mode_config.c > > > > > @@ -374,6 +374,13 @@ static int > > > > > drm_mode_create_standard_properties(struct drm_device *dev) > > > > > return -ENOMEM; > > > > > dev->mode_config.modifiers_property = prop; > > > > > > > > > > + prop = drm_property_create(dev, > > > > > +DRM_MODE_PROP_IMMUTABLE | > > > > > DRM_MODE_PROP_BLOB, > > > > > +"SIZE_HINTS", 0); > > > > > + if (!prop) > > > > > + return -ENOMEM; > > > > > + dev->mode_config.size_hints_property = prop; > > > > > + > > > > > return 0; > > > > > } > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c > > > > > index 24e7998d1731..ae51b1f83755 100644 > > > > > --- a/drivers/gpu/drm/drm_plane.c > > > > > +++ b/drivers/gpu/drm/drm_plane.c > > > > > @@ -140,6 +140,21 @@ > > > > > * DRM_FORMAT_MOD_LINEAR. Before linux kernel release v5.1 there > > > > > have been > > > > > * various bugs in this area with inconsistencies between the > > > > >
Re: [PATCH v2 1/2] drm: Introduce plane SIZE_HINTS property
On Tue, Feb 14, 2023 at 11:25:56AM +0200, Ville Syrjälä wrote: > On Thu, Feb 09, 2023 at 03:16:23PM +0100, Jonas Ådahl wrote: > > On Wed, Feb 08, 2023 at 11:10:16PM +0200, Ville Syrjala wrote: > > > From: Ville Syrjälä > > > > > > Add a new immutable plane property by which a plane can advertise > > > a handful of recommended plane sizes. This would be mostly exposed > > > by cursor planes as a slightly more capable replacement for > > > the DRM_CAP_CURSOR_WIDTH/HEIGHT caps, which can only declare > > > a one size fits all limit for the whole device. > > > > > > Currently eg. amdgpu/i915/nouveau just advertize the max cursor > > > size via the cursor size caps. But always using the max sized > > > cursor can waste a surprising amount of power, so a better > > > stragey is desirable. > > > > > > Most other drivers don't specify any cursor size at all, in > > > which case the ioctl code just claims that 64x64 is a great > > > choice. Whether that is actually true is debatable. > > > > > > A poll of various compositor developers informs us that > > > blindly probing with setcursor/atomic ioctl to determine > > > suitable cursor sizes is not acceptable, thus the > > > introduction of the new property to supplant the cursor > > > size caps. The compositor will now be free to select a > > > more optimal cursor size from the short list of options. > > > > > > Note that the reported sizes (either via the property or the > > > caps) make no claims about things such as plane scaling. So > > > these things should only really be consulted for simple > > > "cursor like" use cases. > > > > > > v2: Try to add some docs > > > > > > Cc: Simon Ser > > > Cc: Jonas Ådahl > > > Cc: Daniel Stone > > > Cc: Pekka Paalanen > > > Acked-by: Harry Wentland > > > Signed-off-by: Ville Syrjälä > > > --- > > > drivers/gpu/drm/drm_mode_config.c | 7 + > > > drivers/gpu/drm/drm_plane.c | 48 +++ > > > include/drm/drm_mode_config.h | 5 > > > include/drm/drm_plane.h | 4 +++ > > > include/uapi/drm/drm_mode.h | 11 +++ > > > 5 files changed, 75 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_mode_config.c > > > b/drivers/gpu/drm/drm_mode_config.c > > > index 87eb591fe9b5..21860f94a18c 100644 > > > --- a/drivers/gpu/drm/drm_mode_config.c > > > +++ b/drivers/gpu/drm/drm_mode_config.c > > > @@ -374,6 +374,13 @@ static int > > > drm_mode_create_standard_properties(struct drm_device *dev) > > > return -ENOMEM; > > > dev->mode_config.modifiers_property = prop; > > > > > > + prop = drm_property_create(dev, > > > +DRM_MODE_PROP_IMMUTABLE | DRM_MODE_PROP_BLOB, > > > +"SIZE_HINTS", 0); > > > + if (!prop) > > > + return -ENOMEM; > > > + dev->mode_config.size_hints_property = prop; > > > + > > > return 0; > > > } > > > > > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c > > > index 24e7998d1731..ae51b1f83755 100644 > > > --- a/drivers/gpu/drm/drm_plane.c > > > +++ b/drivers/gpu/drm/drm_plane.c > > > @@ -140,6 +140,21 @@ > > > * DRM_FORMAT_MOD_LINEAR. Before linux kernel release v5.1 there > > > have been > > > * various bugs in this area with inconsistencies between the > > > capability > > > * flag and per-plane properties. > > > + * > > > + * SIZE_HINTS: > > > + * Blob property which contains the set of recommended plane size > > > + * which can used for simple "cursor like" use cases (eg. no > > > scaling). > > > + * Using these hints frees userspace from extensive probing of > > > + * supported plane sizes through atomic/setcursor ioctls. > > > + * > > > + * For optimal usage userspace should pick the smallest size > > > + * that satisfies its own requirements. > > > + * > > > + * The blob contains an array of struct drm_plane_size_hint. > > > + * > > > + * Drivers should only attach this property to planes that > > > + * support a very limited set of sizes (eg. cursor planes > > > + * on typical hardware). > > > > This is a
Re: [PATCH v2 1/2] drm: Introduce plane SIZE_HINTS property
On Wed, Feb 08, 2023 at 11:10:16PM +0200, Ville Syrjala wrote: > From: Ville Syrjälä > > Add a new immutable plane property by which a plane can advertise > a handful of recommended plane sizes. This would be mostly exposed > by cursor planes as a slightly more capable replacement for > the DRM_CAP_CURSOR_WIDTH/HEIGHT caps, which can only declare > a one size fits all limit for the whole device. > > Currently eg. amdgpu/i915/nouveau just advertize the max cursor > size via the cursor size caps. But always using the max sized > cursor can waste a surprising amount of power, so a better > stragey is desirable. > > Most other drivers don't specify any cursor size at all, in > which case the ioctl code just claims that 64x64 is a great > choice. Whether that is actually true is debatable. > > A poll of various compositor developers informs us that > blindly probing with setcursor/atomic ioctl to determine > suitable cursor sizes is not acceptable, thus the > introduction of the new property to supplant the cursor > size caps. The compositor will now be free to select a > more optimal cursor size from the short list of options. > > Note that the reported sizes (either via the property or the > caps) make no claims about things such as plane scaling. So > these things should only really be consulted for simple > "cursor like" use cases. > > v2: Try to add some docs > > Cc: Simon Ser > Cc: Jonas Ådahl > Cc: Daniel Stone > Cc: Pekka Paalanen > Acked-by: Harry Wentland > Signed-off-by: Ville Syrjälä > --- > drivers/gpu/drm/drm_mode_config.c | 7 + > drivers/gpu/drm/drm_plane.c | 48 +++ > include/drm/drm_mode_config.h | 5 > include/drm/drm_plane.h | 4 +++ > include/uapi/drm/drm_mode.h | 11 +++ > 5 files changed, 75 insertions(+) > > diff --git a/drivers/gpu/drm/drm_mode_config.c > b/drivers/gpu/drm/drm_mode_config.c > index 87eb591fe9b5..21860f94a18c 100644 > --- a/drivers/gpu/drm/drm_mode_config.c > +++ b/drivers/gpu/drm/drm_mode_config.c > @@ -374,6 +374,13 @@ static int drm_mode_create_standard_properties(struct > drm_device *dev) > return -ENOMEM; > dev->mode_config.modifiers_property = prop; > > + prop = drm_property_create(dev, > +DRM_MODE_PROP_IMMUTABLE | DRM_MODE_PROP_BLOB, > +"SIZE_HINTS", 0); > + if (!prop) > + return -ENOMEM; > + dev->mode_config.size_hints_property = prop; > + > return 0; > } > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c > index 24e7998d1731..ae51b1f83755 100644 > --- a/drivers/gpu/drm/drm_plane.c > +++ b/drivers/gpu/drm/drm_plane.c > @@ -140,6 +140,21 @@ > * DRM_FORMAT_MOD_LINEAR. Before linux kernel release v5.1 there have > been > * various bugs in this area with inconsistencies between the capability > * flag and per-plane properties. > + * > + * SIZE_HINTS: > + * Blob property which contains the set of recommended plane size > + * which can used for simple "cursor like" use cases (eg. no scaling). > + * Using these hints frees userspace from extensive probing of > + * supported plane sizes through atomic/setcursor ioctls. > + * > + * For optimal usage userspace should pick the smallest size > + * that satisfies its own requirements. > + * > + * The blob contains an array of struct drm_plane_size_hint. > + * > + * Drivers should only attach this property to planes that > + * support a very limited set of sizes (eg. cursor planes > + * on typical hardware). This is a bit awkward since problematic drivers would only expose this property if they are new enough. A way to avoid this is for all new drivers expose this property, but special case the size 0x0 as "everything below the max limit goes". Then userspace can do (ignoring the missing cap fallback). if (has(SIZE_HINTS)) size = figure_out_size(SIZE_HINTS, DRM_CAP_CURSOR_WIDTH, DRM_CAP_CURSOR_HEIGHT, preferred_size); else size = DRM_CAP_CURSOR_WIDTH x DRM_CAP_CURSOR_WIDTH; With `figure_out_size()` knowing how to deal with 0x0 in the size hints to use `preferred_size` directly. Jonas > */ > > static unsigned int drm_num_planes(struct drm_device *dev) > @@ -1582,3 +1597,36 @@ int drm_plane_create_scaling_filter_property(struct > drm_plane *plane, > return 0; > } > EXPORT_SYMBOL(drm_plane_create_scaling_filter_property); > + > +/** &
Re: [PATCH 1/2] Revert "drm: hide unregistered connectors from GETCONNECTOR IOCTL"
On Tue, Nov 15, 2022 at 08:55:12AM +, Simon Ser wrote: > I've already pushed both patches to drm-misc-next. It's not in any 6.1 rc yet, and apparently it needs to be pushed to drm-misc-fixes to get into 6.1.
Re: [PATCH 1/2] Revert "drm: hide unregistered connectors from GETCONNECTOR IOCTL"
Can you update the commit message so at least the first patch can land
for 6.1 so we can avoid regressions? E.g. something like
It caused logically active but disconnected MST display port connectors to
disappear from the drmModeGetResources() list, meaning userspace is made to
believe the connector is already disabled. This conflicts with the intended
behavior of userspace, which is to detect the connector got disconnected
and then disabling it.
When userspace later attempts post a new mode set commit, if that commit
uses the same CRTC used to previously drive the disconnected connector,
it will fail as that CRTC is logically still tied to the disconnected
connector.
This was discovered by a bisecting docking station hot plugging
regression using mutter.
```
(feel free to edit in any way you want).
Jonas
On Mon, Oct 17, 2022 at 03:31:57PM +, Simon Ser wrote:
> This reverts commit 981f09295687f856d5345e19c7084aca481c1395.
>
> It turns out this breaks Mutter.
>
> Signed-off-by: Simon Ser
> Cc: Daniel Vetter
> Cc: Lyude Paul
> Cc: Jonas Ådahl
> ---
> drivers/gpu/drm/drm_mode_config.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_mode_config.c
> b/drivers/gpu/drm/drm_mode_config.c
> index 939d621c9ad4..688c8afe0bf1 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -151,9 +151,6 @@ int drm_mode_getresources(struct drm_device *dev, void
> *data,
> count = 0;
> connector_id = u64_to_user_ptr(card_res->connector_id_ptr);
> drm_for_each_connector_iter(connector, _iter) {
> - if (connector->registration_state != DRM_CONNECTOR_REGISTERED)
> - continue;
> -
> /* only expose writeback connectors if userspace understands
> them */
> if (!file_priv->writeback_connectors &&
> (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK))
> --
> 2.38.0
>
>
Re: [PATCH 1/2] Revert "drm: hide unregistered connectors from GETCONNECTOR IOCTL"
On Tue, Oct 18, 2022 at 12:58:53PM +0300, Ville Syrjälä wrote:
> On Tue, Oct 18, 2022 at 11:27:19AM +0200, Jonas Ådahl wrote:
> > On Tue, Oct 18, 2022 at 12:14:09PM +0300, Ville Syrjälä wrote:
> > > On Mon, Oct 17, 2022 at 03:31:57PM +, Simon Ser wrote:
> > > > This reverts commit 981f09295687f856d5345e19c7084aca481c1395.
> > > >
> > > > It turns out this breaks Mutter.
> > >
> > > A bit more detail would be a good to help future archaeologists.
> >
> > Perhaps a better explanation is
> >
> > It turns out this causes logically active but disconnected MST display
> > port connectors to disappear from the drmModeGetResources() list,
>
> That was the whole point was it not? So I'd drop the
> "it turns out" part.
>
> > meaning userspace is made to believe the connector is already disabled.
>
> That wording to me implies its a generic issue affecting all
> userspace when so far it looks like only mutter is affected.
Maybe other userspace was? I only found out by testing drm-next, and
only tried using mutter when bisecting.
> So apparently mutter (for some reason) assumes that the
> connector has somehow magically been disabled by someone
> else if it disappears from the list of resources?
Mutter makes the assumption that connectors it can interact with are the
ones that drmModeGetResources() return - nothing magic about that.
Jonas
>
> >
> > When userspace then attempts post a new mode set commit, if that commit
> > uses the same CRTC used to previously drive the disconnected connector,
> > it will fail as that CRTC is logically still tied to the disconnected
> > connector.
> >
> > This was discovered by a bisecting docking station hot plugging
> > regression using mutter.
> >
> >
> > Jonas
> >
> > >
> > > >
> > > > Signed-off-by: Simon Ser
> > > > Cc: Daniel Vetter
> > > > Cc: Lyude Paul
> > > > Cc: Jonas Ådahl
> > > > ---
> > > > drivers/gpu/drm/drm_mode_config.c | 3 ---
> > > > 1 file changed, 3 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_mode_config.c
> > > > b/drivers/gpu/drm/drm_mode_config.c
> > > > index 939d621c9ad4..688c8afe0bf1 100644
> > > > --- a/drivers/gpu/drm/drm_mode_config.c
> > > > +++ b/drivers/gpu/drm/drm_mode_config.c
> > > > @@ -151,9 +151,6 @@ int drm_mode_getresources(struct drm_device *dev,
> > > > void *data,
> > > > count = 0;
> > > > connector_id = u64_to_user_ptr(card_res->connector_id_ptr);
> > > > drm_for_each_connector_iter(connector, _iter) {
> > > > - if (connector->registration_state !=
> > > > DRM_CONNECTOR_REGISTERED)
> > > > - continue;
> > > > -
> > > > /* only expose writeback connectors if userspace
> > > > understands them */
> > > > if (!file_priv->writeback_connectors &&
> > > > (connector->connector_type ==
> > > > DRM_MODE_CONNECTOR_WRITEBACK))
> > > > --
> > > > 2.38.0
> > > >
> > >
> > > --
> > > Ville Syrjälä
> > > Intel
> > >
>
> --
> Ville Syrjälä
> Intel
>
Re: [PATCH 1/2] Revert "drm: hide unregistered connectors from GETCONNECTOR IOCTL"
On Tue, Oct 18, 2022 at 12:14:09PM +0300, Ville Syrjälä wrote:
> On Mon, Oct 17, 2022 at 03:31:57PM +, Simon Ser wrote:
> > This reverts commit 981f09295687f856d5345e19c7084aca481c1395.
> >
> > It turns out this breaks Mutter.
>
> A bit more detail would be a good to help future archaeologists.
Perhaps a better explanation is
It turns out this causes logically active but disconnected MST display
port connectors to disappear from the drmModeGetResources() list,
meaning userspace is made to believe the connector is already disabled.
When userspace then attempts post a new mode set commit, if that commit
uses the same CRTC used to previously drive the disconnected connector,
it will fail as that CRTC is logically still tied to the disconnected
connector.
This was discovered by a bisecting docking station hot plugging
regression using mutter.
Jonas
>
> >
> > Signed-off-by: Simon Ser
> > Cc: Daniel Vetter
> > Cc: Lyude Paul
> > Cc: Jonas Ådahl
> > ---
> > drivers/gpu/drm/drm_mode_config.c | 3 ---
> > 1 file changed, 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_mode_config.c
> > b/drivers/gpu/drm/drm_mode_config.c
> > index 939d621c9ad4..688c8afe0bf1 100644
> > --- a/drivers/gpu/drm/drm_mode_config.c
> > +++ b/drivers/gpu/drm/drm_mode_config.c
> > @@ -151,9 +151,6 @@ int drm_mode_getresources(struct drm_device *dev, void
> > *data,
> > count = 0;
> > connector_id = u64_to_user_ptr(card_res->connector_id_ptr);
> > drm_for_each_connector_iter(connector, _iter) {
> > - if (connector->registration_state != DRM_CONNECTOR_REGISTERED)
> > - continue;
> > -
> > /* only expose writeback connectors if userspace understands
> > them */
> > if (!file_priv->writeback_connectors &&
> > (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK))
> > --
> > 2.38.0
> >
>
> --
> Ville Syrjälä
> Intel
>
Re: [PATCH 2/2] drm/connector: send hotplug uevent on connector cleanup
On Mon, Oct 17, 2022 at 03:32:01PM +, Simon Ser wrote: > A typical DP-MST unplug removes a KMS connector. However care must > be taken to properly synchronize with user-space. The expected > sequence of events is the following: > > 1. The kernel notices that the DP-MST port is gone. > 2. The kernel marks the connector as disconnected, then sends a >uevent to make user-space re-scan the connector list. > 3. User-space notices the connector goes from connected to disconnected, >disables it. > 4. Kernel handles the the IOCTL disabling the connector. On success, >the very last reference to the struct drm_connector is dropped and >drm_connector_cleanup() is called. > 5. The connector is removed from the list, and a uevent is sent to tell >user-space that the connector disappeared. > > The very last step was missing. As a result, user-space thought the > connector still existed and could try to disable it again. Since the > kernel no longer knows about the connector, that would end up with > EINVAL and confused user-space. > > Fix this by sending a hotplug uevent from drm_connector_cleanup(). > > Signed-off-by: Simon Ser > Cc: sta...@vger.kernel.org > Cc: Daniel Vetter > Cc: Lyude Paul > Cc: Jonas Ådahl Tested-by: Jonas Ådahl Jonas
Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS
On Fri, Jun 10, 2022 at 10:49:31AM +0300, Pekka Paalanen wrote: > On Thu, 9 Jun 2022 19:39:39 + > Zack Rusin wrote: > > > On Wed, 2022-06-08 at 10:53 +0300, Pekka Paalanen wrote: > > > On Tue, 7 Jun 2022 17:50:24 + > > > Zack Rusin wrote: > > > > > > > On Tue, 2022-06-07 at 11:07 +0300, Pekka Paalanen wrote: > > > > > On Fri, 03 Jun 2022 14:14:59 + > > > > > Simon Ser wrote: > > > > > > > > > > > Hi, > > > > > > > > > > > > Please, read this thread: > > > > > > https://lists.freedesktop.org/archives/dri-devel/2020-March/thread.html#259615 > > > > > > > > > > > > It has a lot of information about the pitfalls of cursor hotspot and > > > > > > other things done by VM software. > > > > > > > > > > > > In particular: since the driver will ignore the KMS cursor plane > > > > > > position set by user-space, I don't think it's okay to just expose > > > > > > without opt-in from user-space (e.g. with a DRM_CLIENT_CAP). > > > > > > > > > > > > cc wayland-devel and Pekka for user-space feedback. > > > > > > > > > > > > On Thursday, June 2nd, 2022 at 17:42, Zack Rusin > > > > > > wrote: > > > > > > > > > > > > > - all userspace code needs to hardcore a list of drivers which > > > > > > > require > > > > > > > hotspots because there's no way to query from drm "does this > > > > > > > driver > > > > > > > require hotspot" > > > > > > > > > > > > Can you elaborate? I'm not sure I understand what you mean here. > > > > > > > > > > > > > > > > Hi Zack and everyone, > > > > > > > > > > I would like to try to reboot this discussion and explain where I come > > > > > from. Maybe I have misunderstood something. > > > > > > > > First of all thanks for restarting the discussions. I think Gerd > > > > did a good > > > > job responding to individual points, so let me take a step back and > > > > explain the big > > > > picture here so we can reboot. > > > > > > > > > Which root problems do you want to solve exactly? > > > > > > > > The problem that this patch set is solving is the relatively trivial > > > > problem of not > > > > having a way of setting the hotspot in the atomic kms interface. When we > > > > (virtualized drivers) are using the native cursor we need to know not > > > > only the image > > > > > > Could you clarify what is "native cursor" here? > > > I guess it is the host window system pointer's cursor? > > > > Right, exactly. I'm a little hesitant to call it "host" because it gets > > tricky in > > remote scenarios, where the host is some ESXi server but the local machine > > is the > > one that's actually interacting with the guest. So it's the cursor of the > > machine > > where the guest screen is displayed. > > > > > > > > Now, where the disagreements come from is from the fact that all > > > > virtualized drivers > > > > do not implement the atomic KMS cursor plane contract exactly as > > > > specified. In > > > > atomic kms with universal planes the "cursor" plane can be anything so > > > > asking for > > > > hotspot's for something that's not a cursor is a bit silly (but > > > > arguably so is > > > > calling it a cursor plane and then complaining that people expect > > > > cursor in it). > > > > > > > > So the argument is that we can't put hotspot data into atomic kms > > > > without first > > > > rewriting all of the virtualized drivers cursor code to fix the > > > > underlying contract > > > > violation that they all commit. That would likely be a lot easier sell > > > > if not that > > > > gnome/kde don't put none cursor stuff in the cursor plane, so all this > > > > work is to > > > > fix breakages that seem to affect 0 of our users (and I completely > > > > understand that > > > > we'd still like all the drivers to be correct and unified in terms of > > > > their > > > > behaviour, I'm just saying it's a hard sell without something that we > > > > can point to > > > > and say "this fixes/improves things for our customers") > > > > > > What's the cost of making paravirtualized drivers honour the UAPI > > > contract? > > > Can't you do that on the side of implementing these new hotspot > > > properties, with the little addition to allowing guest userspace to be > > > explicit about whether it supports commandeering or not? > > > > I'm reluctant here because "fixing" here seems to be a bit ephemeral as we > > move from > > one solution to the next. I'm happy to write a patch that's adding a > > DRM_CLIENT_CAP_VIRTUAL_CURSOR_AWARE flag and hiding the cursor plane in > > virtualized > > drivers for clients that advertise DRM_CLIENT_CAP_ATOMIC but not > > DRM_CLIENT_CAP_VIRTUAL_CURSOR_AWARE, but that doesn't solve Weston on > > virtualized > > drivers. > > Mind, I have not talked about hiding cursor planes. I have talked > *only* about stopping commandeering cursor planes if guest userspace > does not indicate it is prepared for commandeering. > > I don't understand how it does not "solve on Weston
Re: Call for an EDID parsing library
On Wed, Apr 07, 2021 at 10:59:18AM +, Simon Ser wrote: > FWIW, with my Sway/wlroots hat on I think this is a great idea and I'd > definitely be interested in using such as library. A C API with no > dependencies is pretty important from my point-of-view. > > I'd prefer if C++ was not used at all (and could almost be baited into > doing the work if that were the case), but it seems that ship has > sailed already. The same for Mutter / GNOME, not having to maintain a EDID parser would be great. Though personally I don't care if it's implemented in C++, C or whatever, as long as there is a C API to use. Jonas > > Simon > ___ > wayland-devel mailing list > wayland-de...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/wayland-devel ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm: document that blobs are ref'counted
On Thu, Nov 05, 2020 at 10:56:53AM +0200, Pekka Paalanen wrote:
> On Wed, 04 Nov 2020 17:01:40 +
> Simon Ser wrote:
>
> > User-space doesn't need to keep track of blobs that might be in use by
> > the kernel. User-space can just destroy blobs as soon as they don't need
> > them anymore.
> >
> > Signed-off-by: Simon Ser
> > Signed-off-by: Daniel Stone
> > Reviewed-by: Jonas Ådahl
> > Cc: Pekka Paalanen
> > Cc: Daniel Vetter
> > ---
> > include/uapi/drm/drm_mode.h | 6 ++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > index 863eda048265..5ad10ab2a577 100644
> > --- a/include/uapi/drm/drm_mode.h
> > +++ b/include/uapi/drm/drm_mode.h
> > @@ -924,6 +924,12 @@ struct drm_mode_create_blob {
> > * struct drm_mode_destroy_blob - Destroy user blob
> > * @blob_id: blob_id to destroy
> > * Destroy a user-created blob property.
> > + *
> > + * User-space can release blobs as soon as they do not need to refer to
> > them by
> > + * their blob object ID. For instance, if you are using a MODE_ID blob in
> > an
> > + * atomic commit and you will not make another commit re-using the same
> > ID, you
> > + * can destroy the blob as soon as the commit has been issued, without
> > waiting
> > + * for it to complete.
> > */
> > struct drm_mode_destroy_blob {
> > __u32 blob_id;
>
> Reviewed-by: Pekka Paalanen
This version is as clear to me as the previous one so both are
Reviewed-by: Jonas Ådahl
Jonas
>
>
> Thanks,
> pq
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: document that blobs are ref'counted
Thanks for this!
I can't review the correctness, but the description looks clear to me
so,
Reviewed-by: Jonas Ådahl
Jonas
On Thu, Oct 22, 2020 at 09:38:05AM +, Simon Ser wrote:
> User-space doesn't need to keep track of blobs that might be in use by
> the kernel. User-space can just destroy blobs as soon as they don't need
> them anymore.
>
> Signed-off-by: Simon Ser
> Cc: Pekka Paalanen
> Cc: Daniel Vetter
> Cc: Jonas Ådahl
> ---
> include/uapi/drm/drm_mode.h | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 863eda048265..f7c41aa4b5eb 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -924,6 +924,10 @@ struct drm_mode_create_blob {
> * struct drm_mode_destroy_blob - Destroy user blob
> * @blob_id: blob_id to destroy
> * Destroy a user-created blob property.
> + *
> + * Blobs are reference-counted by the kernel, so user-space can destroy them
> as
> + * soon as they're done with them. For instance user-space can destroy a
> blob
> + * used in an atomic commit right after performing the atomic commit ioctl.
> */
> struct drm_mode_destroy_blob {
> __u32 blob_id;
> --
> 2.28.0
>
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Plumbing explicit synchronization through the Linux ecosystem
On Mon, Mar 16, 2020 at 10:37:04PM -0500, Jason Ekstrand wrote: > On Mon, Mar 16, 2020 at 6:39 PM Roman Gilg wrote: > > > > On Wed, Mar 11, 2020 at 8:21 PM Jason Ekstrand wrote: > > > > > > On Wed, Mar 11, 2020 at 12:31 PM Jason Ekstrand > > > wrote: > > > > > > > > All, > > > > > > > > Sorry for casting such a broad net with this one. I'm sure most people > > > > who reply will get at least one mailing list rejection. However, this > > > > is an issue that affects a LOT of components and that's why it's > > > > thorny to begin with. Please pardon the length of this e-mail as > > > > well; I promise there's a concrete point/proposal at the end. > > > > > > > > > > > > Explicit synchronization is the future of graphics and media. At > > > > least, that seems to be the consensus among all the graphics people > > > > I've talked to. I had a chat with one of the lead Android graphics > > > > engineers recently who told me that doing explicit sync from the start > > > > was one of the best engineering decisions Android ever made. It's > > > > also the direction being taken by more modern APIs such as Vulkan. > > > > > > > > > > > > ## What are implicit and explicit synchronization? > > > > > > > > For those that aren't familiar with this space, GPUs, media encoders, > > > > etc. are massively parallel and synchronization of some form is > > > > required to ensure that everything happens in the right order and > > > > avoid data races. Implicit synchronization is when bits of work (3D, > > > > compute, video encode, etc.) are implicitly based on the absolute > > > > CPU-time order in which API calls occur. Explicit synchronization is > > > > when the client (whatever that means in any given context) provides > > > > the dependency graph explicitly via some sort of synchronization > > > > primitives. If you're still confused, consider the following > > > > examples: > > > > > > > > With OpenGL and EGL, almost everything is implicit sync. Say you have > > > > two OpenGL contexts sharing an image where one writes to it and the > > > > other textures from it. The way the OpenGL spec works, the client has > > > > to make the API calls to render to the image before (in CPU time) it > > > > makes the API calls which texture from the image. As long as it does > > > > this (and maybe inserts a glFlush?), the driver will ensure that the > > > > rendering completes before the texturing happens and you get correct > > > > contents. > > > > > > > > Implicit synchronization can also happen across processes. Wayland, > > > > for instance, is currently built on implicit sync where the client > > > > does their rendering and then does a hand-off (via wl_surface::commit) > > > > to tell the compositor it's done at which point the compositor can now > > > > texture from the surface. The hand-off ensures that the client's > > > > OpenGL API calls happen before the server's OpenGL API calls. > > > > > > > > A good example of explicit synchronization is the Vulkan API. There, > > > > a client (or multiple clients) can simultaneously build command > > > > buffers in different threads where one of those command buffers > > > > renders to an image and the other textures from it and then submit > > > > both of them at the same time with instructions to the driver for > > > > which order to execute them in. The execution order is described via > > > > the VkSemaphore primitive. With the new VK_KHR_timeline_semaphore > > > > extension, you can even submit the work which does the texturing > > > > BEFORE the work which does the rendering and the driver will sort it > > > > out. > > > > > > > > The #1 problem with implicit synchronization (which explicit solves) > > > > is that it leads to a lot of over-synchronization both in client space > > > > and in driver/device space. The client has to synchronize a lot more > > > > because it has to ensure that the API calls happen in a particular > > > > order. The driver/device have to synchronize a lot more because they > > > > never know what is going to end up being a synchronization point as an > > > > API call on another thread/process may occur at any time. As we move > > > > to more and more multi-threaded programming this synchronization (on > > > > the client-side especially) becomes more and more painful. > > > > > > > > > > > > ## Current status in Linux > > > > > > > > Implicit synchronization in Linux works via a the kernel's internal > > > > dma_buf and dma_fence data structures. A dma_fence is a tiny object > > > > which represents the "done" status for some bit of work. Typically, > > > > dma_fences are created as a by-product of someone submitting some bit > > > > of work (say, 3D rendering) to the kernel. The dma_buf object has a > > > > set of dma_fences on it representing shared (read) and exclusive > > > > (write) access to the object. When work is submitted which, for > > > > instance renders to the dma_buf, it's queued waiting on all the fences > >
