Re: [PATCH v1 1/3] drm/ci: refactor software-driver stage jobs
Hi Vignesh, On Fri, 4 Oct 2024 at 09:31, Vignesh Raman wrote: > +.software-driver: > + stage: software-driver > + extends: > +- .test-gl > +- .test-rules > + timeout: "1h30m" > + tags: > +- kvm > + script: > +- ln -sf $CI_PROJECT_DIR/install /install > +- mv install/bzImage /lava-files/bzImage > +- mkdir -p /lib/modules > +- mkdir -p $CI_PROJECT_DIR/results > +- ln -sf $CI_PROJECT_DIR/results /results > +- install/crosvm-runner.sh install/igt_runner.sh Instead of inlining this here, can we please move towards reusing more of .gitlab-ci/common/init-stage[12].sh? If those files need to be modified then that's totally fine, but I'd rather have something more predictable, and fewer random pieces of shell in each job section.
Re: [PATCH v1] drm/ci: uprev IGT and deqp-runner
Hi Vignesh, On Thu, 5 Sept 2024 at 10:41, Vignesh Raman wrote: > Uprev IGT to the latest version and deqp-runner > to v0.20.0. Also update expectation files. Thanks! This is: Reviewed-by: Daniel Stone
Re: [PATCH v9 0/6] drm/ci: Add support for GPU and display testing
Hi Vignesh, On Tue, 30 Jul 2024 at 03:16, Vignesh Raman wrote: > Some ARM SOCs have a separate display controller and GPU, each with > different drivers. For mediatek mt8173, the GPU driver is powervr, > and the display driver is mediatek. In the case of mediatek mt8183, > the GPU driver is panfrost, and the display driver is mediatek. > With rockchip rk3288/rk3399, the GPU driver is panfrost, while the > display driver is rockchip. For amlogic meson G12B (A311D) SOC, the > GPU driver is panfrost, and the display driver is meson. > > IGT tests run various tests with different xfails and can test both > GPU devices and KMS/display devices. Currently, in drm-ci for MediaTek, > Rockchip, and Amlogic Meson platforms, only the GPU driver is tested. > This leads to incomplete coverage since the display is never tested on > these platforms. This commit series adds support in drm-ci to run tests > for both GPU and display drivers for MediaTek mt8173/mt8183, Rockchip > rk3288/rk3399, and Amlogic Meson G12B (A311D) platforms. > > Update the expectations file, and skip driver-specific tests and > tools_test on non-intel platforms. Thanks, series looks sensible and is: Reviewed-by: Daniel Stone Cheers, Daniel
Re: [PATCH v3 2/9] drm: Export drm_plane_has_format()
On Wed, 19 Jun 2024 at 12:31, Ville Syrjala wrote: > Export drm_plane_has_format() so that drivers can use it. Acked-by: Daniel Stone
Re: [PATCH v2 1/2] drm: Introduce plane SIZE_HINTS property
Hi, On Tue, 27 Feb 2024 at 19:35, Ville Syrjala wrote: > 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. Acked-by: Daniel Stone Cheers, Daniel
Re: [PULL] drm-xe-next
Hi, On Mon, 26 Feb 2024 at 03:21, Lucas De Marchi wrote: > All of this should be fixed by now: dim is used for applying and pushing > patches, which has additional checks so that doesn't happen again. Still > pending confirmation from Daniel Stone if the git server hooks are ready > in gitlab so we properly forbid pushes without dim, like we do with the > git.fd.o infra. Yeah, I did that last week. Cheers, Daniel
Re: [PATCH 2/7] drm/uAPI: Add "active color format" drm property as feedback for userspace
Hi, On Wed, 10 Jan 2024 at 10:44, Daniel Vetter wrote: > On Tue, Jan 09, 2024 at 11:12:11PM +, Andri Yngvason wrote: > > ţri., 9. jan. 2024 kl. 22:32 skrifađi Daniel Stone : > > > How does userspace determine what's happened without polling? Will it > > > only change after an `ALLOW_MODESET` commit, and be guaranteed to be > > > updated after the commit has completed and the event being sent? > > > Should it send a HOTPLUG event? Other? > > > > > > > Userspace does not determine what's happened without polling. The purpose > > of this property is not for programmatic verification that the preferred > > property was applied. It is my understanding that it's mostly intended for > > debugging purposes. It should only change as a consequence of modesetting, > > although I didn't actually look into what happens if you set the "preferred > > color format" outside of a modeset. > > This feels a bit irky to me, since we don't have any synchronization and > it kinda breaks how userspace gets to know about stuff. > > For context the current immutable properties are all stuff that's derived > from the sink (like edid, or things like that). Userspace is guaranteed to > get a hotplug event (minus driver bugs as usual) if any of these change, > and we've added infrastructure so that the hotplug event even contains the > specific property so that userspace can avoid re-read (which can cause > some costly re-probing) them all. Right. > [...] > > This thing here works entirely differently, and I think we need somewhat > new semantics for this: > > - I agree it should be read-only for userspace, so immutable sounds right. > > - But I also agree with Daniel Stone that this should be tied more > directly to the modeset state. > > So I think the better approach would be to put the output type into > drm_connector_state, require that drivers compute it in their > ->atomic_check code (which in the future would allow us to report it out > for TEST_ONLY commits too), and so guarantee that the value is updated > right after the kms ioctl returns (and not somewhen later for non-blocking > commits). That's exactly the point. Whether userspace gets an explicit notification or it has to 'know' when to read isn't much of an issue - just as long as it's well defined. I think the suggestion of 'do it in atomic_check and then it's guaranteed to be readable when the commit completes' is a good one. I do still have some reservations - for instance, why do we have the fallback to auto when userspace has explicitly requested a certain type? - but they may have been covered previously. Cheers, Daniel
Re: [PATCH 2/7] drm/uAPI: Add "active color format" drm property as feedback for userspace
Hi, On Tue, 9 Jan 2024 at 18:12, Andri Yngvason wrote: > + * active color format: > + * This read-only property tells userspace the color format actually used > + * by the hardware display engine "on the cable" on a connector. The > chosen > + * value depends on hardware capabilities, both display engine and > + * connected monitor. Drivers shall use > + * drm_connector_attach_active_color_format_property() to install this > + * property. Possible values are "not applicable", "rgb", "ycbcr444", > + * "ycbcr422", and "ycbcr420". How does userspace determine what's happened without polling? Will it only change after an `ALLOW_MODESET` commit, and be guaranteed to be updated after the commit has completed and the event being sent? Should it send a HOTPLUG event? Other? Cheers, Daniel
Re: [Intel-gfx] Commit messages (was: [PATCH v11] drm/amdgpu: add drm buddy support to amdgpu)
On Wed, 23 Mar 2022 at 15:14, Alex Deucher wrote: > On Wed, Mar 23, 2022 at 11:04 AM Daniel Stone wrote: > > That's not what anyone's saying here ... > > > > No-one's demanding AMD publish RTL, or internal design docs, or > > hardware specs, or URLs to JIRA tickets no-one can access. > > > > This is a large and invasive commit with pretty big ramifications; > > containing exactly two lines of commit message, one of which just > > duplicates the subject. > > > > It cannot be the case that it's completely impossible to provide any > > justification, background, or details, about this commit being made. > > Unless, of course, it's to fix a non-public security issue, that is > > reasonable justification for eliding some of the details. But then > > again, 'huge change which is very deliberately opaque' is a really > > good way to draw a lot of attention to the commit, and it would be > > better to provide more detail about the change to help it slip under > > the radar. > > > > If dri-devel@ isn't allowed to inquire about patches which are posted, > > then CCing the list is just a façade; might as well just do it all > > internally and periodically dump out pull requests. > > I think we are in agreement. I think the withheld information > Christian was referring to was on another thread with Christian and > Paul discussing a workaround for a hardware bug: > https://www.spinics.net/lists/amd-gfx/msg75908.html Right, that definitely seems like some crossed wires. I don't see anything wrong with that commit at all: the commit message and a comment notes that there is a hardware issue preventing Raven from being able to do TMZ+GTT, and the code does the very straightforward and obvious thing to ensure that on VCN 1.0, any TMZ buffer must be VRAM-placed. This one, on the other hand, is much less clear ... Cheers, Daniel
Re: [Intel-gfx] Commit messages (was: [PATCH v11] drm/amdgpu: add drm buddy support to amdgpu)
Hi Alex, On Wed, 23 Mar 2022 at 14:42, Alex Deucher wrote: > On Wed, Mar 23, 2022 at 10:00 AM Daniel Stone wrote: > > On Wed, 23 Mar 2022 at 08:19, Christian König > > wrote: > > > Well the key point is it's not about you to judge that. > > > > > > If you want to complain about the commit message then come to me with > > > that and don't request information which isn't supposed to be publicly > > > available. > > > > > > So to make it clear: The information is intentionally hold back and not > > > made public. > > > > In that case, the code isn't suitable to be merged into upstream > > trees; it can be resubmitted when it can be explained. > > So you are saying we need to publish the problematic RTL to be able to > fix a HW bug in the kernel? That seems a little unreasonable. Also, > links to internal documents or bug trackers don't provide much value > to the community since they can't access them. In general, adding > internal documents to commit messages is frowned on. That's not what anyone's saying here ... No-one's demanding AMD publish RTL, or internal design docs, or hardware specs, or URLs to JIRA tickets no-one can access. This is a large and invasive commit with pretty big ramifications; containing exactly two lines of commit message, one of which just duplicates the subject. It cannot be the case that it's completely impossible to provide any justification, background, or details, about this commit being made. Unless, of course, it's to fix a non-public security issue, that is reasonable justification for eliding some of the details. But then again, 'huge change which is very deliberately opaque' is a really good way to draw a lot of attention to the commit, and it would be better to provide more detail about the change to help it slip under the radar. If dri-devel@ isn't allowed to inquire about patches which are posted, then CCing the list is just a façade; might as well just do it all internally and periodically dump out pull requests. Cheers, Daniel
Re: [Intel-gfx] Commit messages (was: [PATCH v11] drm/amdgpu: add drm buddy support to amdgpu)
On Wed, 23 Mar 2022 at 08:19, Christian König wrote: > Am 23.03.22 um 09:10 schrieb Paul Menzel: > > Sorry, I disagree. The motivation needs to be part of the commit > > message. For example see recent discussion on the LWN article > > *Donenfeld: Random number generator enhancements for Linux 5.17 and > > 5.18* [1]. > > > > How much the commit message should be extended, I do not know, but the > > current state is insufficient (too terse). > > Well the key point is it's not about you to judge that. > > If you want to complain about the commit message then come to me with > that and don't request information which isn't supposed to be publicly > available. > > So to make it clear: The information is intentionally hold back and not > made public. In that case, the code isn't suitable to be merged into upstream trees; it can be resubmitted when it can be explained. Cheers, Daniel
Re: [Intel-gfx] [PATCH 01/21] MAINTAINERS: Add entry for fbdev core
On Mon, 31 Jan 2022 at 21:06, Daniel Vetter wrote: > Ever since Tomi extracted the core code in 2014 it's been defacto me > maintaining this, with help from others from dri-devel and sometimes > Linus (but those are mostly merge conflicts): > > $ git shortlog -ns drivers/video/fbdev/core/ | head -n5 > 35 Daniel Vetter > 23 Linus Torvalds > 10 Hans de Goede > 9 Dave Airlie > 6 Peter Rosin > > I think ideally we'd also record that the various firmware fb drivers > (efifb, vesafb, ...) are also maintained in drm-misc because for the > past few years the patches have either been to fix handover issues > with drm drivers, or caused handover issues with drm drivers. So any > other tree just doesn't make sense. But also, there's plenty of > outdated MAINTAINER entries for these with people and git trees that > haven't been active in years, so maybe let's just leave them alone. > And furthermore distros are now adopting simpledrm as the firmware fb > driver, so hopefully the need to care about the fbdev firmware drivers > will go down going forward. > > Note that drm-misc is group maintained, I expect that to continue like > we've done before, so no new expectations that patches all go through > my hands. That would be silly. This also means I'm happy to put any > other volunteer's name in the M: line, but otherwise git log says I'm > the one who's stuck with this. Acked-by: Daniel Stone
Re: [Intel-gfx] [PATCH v3 0/8] DG2 accelerated migration/clearing support
Hi Matthew, On Mon, 6 Dec 2021 at 13:32, Matthew Auld wrote: > Enable accelerated moves and clearing on DG2. On such HW we have minimum page > size restrictions when accessing LMEM from the GTT, where we now have to use > 64K > GTT pages or larger. With the ppGTT the page-table also has a slightly > different > layout from past generations when using the 64K GTT mode(which is still > enabled > on via some PDE bit), where it is now compacted down to 32 qword entries. Note > that on discrete the paging structures must also be placed in LMEM, and we > need > to able to modify them via the GTT itself(see patch 7), which is one of the > complications here. > > The series needs to be applied on top of the DG2 enabling branch: > https://cgit.freedesktop.org/~ramaling/linux/log/?h=dg2_enabling_ww49.3 What are the changes to the v1/v2? Cheers, Daniel
Re: [Intel-gfx] [RFC 6/8] drm: Document fdinfo format specification
Hi Tvrtko, Thanks for typing this up! On Thu, 15 Jul 2021 at 10:18, Tvrtko Ursulin wrote: > +Mandatory fully standardised keys > +- > + > +- drm-driver: > + > +String shall contain a fixed string uniquely identified the driver handling > +the device in question. For example name of the respective kernel module. I think let's be more prescriptive and just say that it is the module name. > +Optional fully standardised keys > + > + > +- drm-pdev: > + > +For PCI devices this should contain the PCI slot address of the device in > +question. How about just major:minor of the DRM render node device it's attached to? > +- drm-client-id: > + > +Unique value relating to the open DRM file descriptor used to distinguish > +duplicated and shared file descriptors. Conceptually the value should map 1:1 > +to the in kernel representation of `struct drm_file` instances. > + > +Uniqueness of the value shall be either globally unique, or unique within the > +scope of each device, in which case `drm-pdev` shall be present as well. > + > +Userspace should make sure to not double account any usage statistics by > using > +the above described criteria in order to associate data to individual > clients. > + > +- drm-engine-: ns > + > +GPUs usually contain multiple execution engines. Each shall be given a stable > +and unique name (str), with possible values documented in the driver specific > +documentation. > + > +Value shall be in specified time units which the respective GPU engine spent > +busy executing workloads belonging to this client. > + > +Values are not required to be constantly monotonic if it makes the driver > +implementation easier, but are required to catch up with the previously > reported > +larger value within a reasonable period. Upon observing a value lower than > what > +was previously read, userspace is expected to stay with that larger previous > +value until a monotonic update is seen. Yeah, that would work well for Mali/Panfrost. We can queue multiple jobs in the hardware, which can either be striped across multiple cores with an affinity mask (e.g. 3 cores for your client and 1 for your compositor), or picked according to priority, or ... The fine-grained performance counters (e.g. time spent waiting for sampler) are only GPU-global. So if you have two jobs running simultaneously, you have no idea who's responsible for what. But it does give us coarse-grained counters which are accounted per-job-slot, including exactly this metric: amount of 'GPU time' (whatever that means) occupied by that job slot during the sampling period. So we could support that nicely if we fenced job-slot updates with register reads/writes. Something I'm missing though is how we enable this information. Seems like it would be best to either only do it whilst fdinfo is open (and re-read it whenever you need an update), or on a per-driver sysfs toggle, or ... ? > +- drm-memory-: [KiB|MiB] > + > +Each possible memory type which can be used to store buffer objects by the > +GPU in question shall be given a stable and unique name to be returned as the > +string here. > + > +Value shall reflect the amount of storage currently consumed by the buffer > +object belong to this client, in the respective memory region. > + > +Default unit shall be bytes with optional unit specifiers of 'KiB' or 'MiB' > +indicating kibi- or mebi-bytes. I'm a bit wary of the accounting here. Is it buffer allocations originating from the client, in which case it conceptually clashes with gralloc? Is it the client which last wrote to the buffer? The client with the oldest open handle to the buffer? Other? Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [Mesa-dev] [PATCH] dma-buf: Document dma-buf implicit fencing/resv fencing rules
Hi, On Wed, 23 Jun 2021 at 17:20, Daniel Vetter wrote: > +* > +* IMPLICIT SYNCHRONIZATION RULES: > +* > +* Drivers which support implicit synchronization of buffer access as > +* e.g. exposed in `Implicit Fence Poll Support`_ should follow the > +* below rules. 'Should' ... ? Must. > +* - Drivers should add a shared fence through > +* dma_resv_add_shared_fence() for anything the userspace API > +* considers a read access. This highly depends upon the API and > +* window system: E.g. OpenGL is generally implicitly synchronized > on > +* Linux, but explicitly synchronized on Android. Whereas Vulkan is > +* generally explicitly synchronized for everything, and window > system > +* buffers have explicit API calls (which then need to make sure the > +* implicit fences store here in @resv are updated correctly). > +* > +* - [...] Mmm, I think this is all right, but it could be worded much more clearly. Right now it's a bunch of points all smashed into one, and there's a lot of room for misinterpretation. Here's a strawman, starting with most basic and restrictive, working through to when you're allowed to wriggle your way out: Rule 1: Drivers must add a shared fence through dma_resv_add_shared_fence() for any read accesses against that buffer. This appends a fence to the shared array, ensuring that any future non-read access will be synchronised against this operation to only begin after it has completed. Rule 2: Drivers must add an exclusive fence through dma_resv_add_excl_fence() for any write accesses against that buffer. This replaces the exclusive fence with the new operation, ensuring that all future access will be synchronised against this operation to only begin after it has completed. Rule 3: Drivers must synchronise all accesses to buffers against existing implicit fences. Read accesses must synchronise against the exclusive fence (read-after-write), and write accesses must synchronise against both the exclusive (write-after-write) and shared (write-after-read) fences. Note 1: Users like OpenGL and window systems on non-Android userspace are generally implicitly synchronised. An implicitly-synchronised userspace is unaware of fences from prior operations, so the kernel mediates scheduling to create the illusion that GPU work is FIFO. For example, an application will flush and schedule GPU write work to render its image, then immediately tell the window system to display that image; the window system may immediately flush and schedule GPU read work to display that image, with neither waiting for the write to have completed. The kernel provides coherence by synchronising the read access against the write fence in the exclusive slot, so that the image displayed is correct. Note 2: Users like Vulkan and Android window system are generally explicitly synchronised. An explicitly-synchronised userspace is responsible for tracking its own read and write access and providing the kernel with synchronisation barriers. For instance, a Vulkan application rendering to a buffer and subsequently using it as a read texture, must annotate the read operation with a read-after-write synchronisation barrier. Note 3: Implicit and explicit userspace can coexist. For instance, an explicitly-synchronised Vulkan application may be running as a client of an implicitly-synchronised window system which uses OpenGL for composition; an implicitly-synchronised OpenGL application may be running as a client of a window system which uses Vulkan for composition. Note 4: Some subsystems, for example V4L2, do not pipeline operations, and instead only return to userspace when the scheduled work against a buffer has fully retired. Exemption 1: Fully self-coherent userspace may skip implicit synchronisation barriers. For instance, accesses between two Vulkan-internal buffers allocated by a single application do not need to synchronise against each other's implicit fences, as the client is responsible for explicitly providing barriers for access. A self-contained OpenGL userspace also has no need to implicitly synchronise its access if the driver instead tracks all access and inserts the appropriate synchronisation barriers. Exemption 2: When implicit and explicit userspace coexist, the explicit side may skip intermediate synchronisation, and only place synchronisation barriers at transition points. For example, a Vulkan compositor displaying a buffer from an OpenGL application would need to synchronise its first access against the fence placed in the exclusive implicit-synchronisation slot. Once this read has fully retired, the compositor has no need to participate in implicit synchronisation until it is ready to return the buffer to the application, at which point it must insert all its non-retired accesses into the shared slot, which the application will then synchronise future write
Re: [Intel-gfx] [PATCH 5/7] dma-buf: Add an API for exporting sync files (v11)
Hi, On Wed, 26 May 2021 at 13:46, Christian König wrote: > Am 26.05.21 um 13:31 schrieb Daniel Stone: > > How would we insert a syncobj+val into a resv though? Like, if we pass > > an unmaterialised syncobj+val here to insert into the resv, then an > > implicit-only media user (or KMS) goes to sync against the resv, what > > happens? > > Well this is for exporting, not importing. So we don't need to worry > about that. > > It's just my thinking because the drm_syncobj is the backing object on > VkSemaphore implementations these days, isn't it? Yeah, I can see that to an extent. But then binary vs. timeline syncobjs are very different in use (which is unfortunate tbh), and then we have an asymmetry between syncobj export & sync_file import. You're right that we do want a syncobj though. This is probably not practical due to smashing uAPI to bits, but if we could wind the clock back a couple of years, I suspect the interface we want is that export can either export a sync_file or a binary syncobj, and further that binary syncobjs could transparently act as timeline semaphores by mapping any value (either wait or signal) to the binary signal. In hindsight, we should probably just never have had binary syncobj. Oh well. Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 5/7] dma-buf: Add an API for exporting sync files (v11)
Hi Christian, On Wed, 26 May 2021 at 12:02, Christian König wrote: > Am 25.05.21 um 23:17 schrieb Jason Ekstrand: > > This new IOCTL solves this problem by allowing us to get a snapshot of > > the implicit synchronization state of a given dma-buf in the form of a > > sync file. It's effectively the same as a poll() or I915_GEM_WAIT only, > > instead of CPU waiting directly, it encapsulates the wait operation, at > > the current moment in time, in a sync_file so we can check/wait on it > > later. As long as the Vulkan driver does the sync_file export from the > > dma-buf before we re-introduce it for rendering, it will only contain > > fences from the compositor or display. This allows to accurately turn > > it into a VkFence or VkSemaphore without any over- synchronization. > > Regarding that, why do we actually use a syncfile and not a drm_syncobj > here? > > The later should be much closer to a Vulkan timeline semaphore. How would we insert a syncobj+val into a resv though? Like, if we pass an unmaterialised syncobj+val here to insert into the resv, then an implicit-only media user (or KMS) goes to sync against the resv, what happens? Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [Linaro-mm-sig] [PATCH 04/11] drm/panfrost: Fix implicit sync
On Fri, 21 May 2021 at 14:09, Christian König wrote: > Am 21.05.21 um 14:54 schrieb Daniel Stone: > > If you're curious, the interface definitions are in the csf/ directory > > in the 'Bifrost kernel driver' r30p0 download you can get from the Arm > > developer site. Unfortunately the exact semantics aren't completely > > clear. > > Well it is actually relatively simple. Take a look at the timeline > semaphores from Vulkan, everybody is basically implementing the same > semantics now. > > When you queued up a bunch of commands on your hardware, the first one > will write value 1 to a 64bit memory location, the second one will write > value 2, the third value 3 and so on. After writing the value the > hardware raises and interrupt signal to everybody interested. > > In other words pretty standard memory fence behavior. > > When you now have a second queue which depends on work of the first one > you look at the memory location and do a compare. If you depend on the > third submission you just wait for the value to be >3 and are done. Right, it is clearly defined to the timeline semaphore semantics, I just meant that it's not clear how it works at a lower level wrt the synchronisation and signaling. The simplest possible interpretation is that wait_addrval blocks infinitely before kick-cmdbuf, but that seems painful with only 32 queues. And the same for fences, which are a binary signal. I guess we'll find out. My tooth hurts. Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [Linaro-mm-sig] [PATCH 04/11] drm/panfrost: Fix implicit sync
On Fri, 21 May 2021 at 13:28, Christian König wrote: > Am 21.05.21 um 14:22 schrieb Daniel Stone: > > Yeah, the 'second-generation Valhall' GPUs coming later this year / > > early next year are starting to get pretty weird. Firmware-mediated > > job scheduling out of multiple queues, userspace having direct access > > to the queues and can do inter-queue synchronisation (at least I think > > so), etc. For bonus points, synchronisation is based on $addr = $val > > to signal and $addr == $val to wait, with a separate fence primitive > > as well. > > Well that sounds familiar :) I laughed when I first saw it, because it was better than crying I guess. If you're curious, the interface definitions are in the csf/ directory in the 'Bifrost kernel driver' r30p0 download you can get from the Arm developer site. Unfortunately the exact semantics aren't completely clear. > > Obviously Arm should be part of this conversation here, but I guess > > we'll have to wait for a while yet to see how everything's shaken out > > with this new gen, and hope that whatever's been designed upstream in > > the meantime is actually vaguely compatible ... > > Yeah, going to keep you in CC when we start to code and review user fences. Awesome, thanks Christian. Appreciate it. :) Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 04/11] drm/panfrost: Fix implicit sync
Hi, On Fri, 21 May 2021 at 10:10, Daniel Vetter wrote: > Currently this has no practial relevance I think because there's not > many who can pull off a setup with panfrost and another gpu in the > same system. But the rules are that if you're setting an exclusive > fence, indicating a gpu write access in the implicit fencing system, > then you need to wait for all fences, not just the previous exclusive > fence. > > panfrost against itself has no problem, because it always sets the > exclusive fence (but that's probably something that will need to be > fixed for vulkan and/or multi-engine gpus, or you'll suffer badly). > Also no problem with that against display. Yeah, the 'second-generation Valhall' GPUs coming later this year / early next year are starting to get pretty weird. Firmware-mediated job scheduling out of multiple queues, userspace having direct access to the queues and can do inter-queue synchronisation (at least I think so), etc. For bonus points, synchronisation is based on $addr = $val to signal and $addr == $val to wait, with a separate fence primitive as well. Obviously Arm should be part of this conversation here, but I guess we'll have to wait for a while yet to see how everything's shaken out with this new gen, and hope that whatever's been designed upstream in the meantime is actually vaguely compatible ... Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/7] Per client engine busyness
Hi, On Tue, 18 May 2021 at 10:09, Tvrtko Ursulin wrote: > I was just wondering if stat(2) and a chrdev major check would be a > solid criteria to more efficiently (compared to parsing the text > content) detect drm files while walking procfs. Maybe I'm missing something, but is the per-PID walk actually a measurable performance issue rather than just a bit unpleasant? Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC PATCH 1/5] drm/doc/rfc: i915 GuC submission / DRM scheduler integration plan
Hi, On Tue, 11 May 2021 at 15:34, Daniel Vetter wrote: > On Thu, May 06, 2021 at 10:30:45AM -0700, Matthew Brost wrote: > > +No major changes are required to the uAPI for basic GuC submission. The > > only > > +change is a new scheduler attribute: > > I915_SCHEDULER_CAP_STATIC_PRIORITY_MAP. > > +This attribute indicates the 2k i915 user priority levels are statically > > mapped > > +into 3 levels as follows: > > + > > +* -1k to -1 Low priority > > +* 0 Medium priority > > +* 1 to 1k High priority > > + > > +This is needed because the GuC only has 4 priority bands. The highest > > priority > > +band is reserved with the kernel. This aligns with the DRM scheduler > > priority > > +levels too. > > Please Cc: mesa and get an ack from Jason Ekstrand or Ken Graunke on this, > just to be sure. A reference to the actual specs this targets would help. I don't have oneAPI to hand if it's relevant, but the two in graphics world are https://www.khronos.org/registry/EGL/extensions/IMG/EGL_IMG_context_priority.txt and https://www.khronos.org/registry/vulkan/specs/1.2-extensions/html/chap5.html#devsandqueues-priority - both of them pretty much say that the implementation may do anything or nothing at all, so this isn't a problem for spec conformance, only a matter of user priority (sorry). Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/atomic: Add the crtc to affected crtc only if uapi.enable = true
On Tue, 16 Mar 2021 at 21:35, Daniel Vetter wrote: > On Tue, Mar 9, 2021 at 10:14 AM Pekka Paalanen > wrote: > > On Mon, 8 Mar 2021 16:52:58 -0800 > > "Navare, Manasi" wrote: > > > Hmm well after the actual real commit, since the second crtc is stolen > > > even though it is not being used for the display output, it is > > > used for joiner so the uapi.enable will be true after the real commit. > > > > > > so actually the assertion would fail in this case. > > > > > > @Ville @Danvet any suggestions here in that case? > > That is very bad. We can't frob uapi state like that. I think that > calls for even more checks to make sure kms drivers who try to play > clever games don't get it wrong, so we probably need to check uapi > enable and active state in another mask before/after ->atomic_check > too. Or something like that. > Yeah. We can _never_ generate externally-visible completion events. We can later fail to enable the stolen CRTC - because trying to enable new things can fail for any reason whatsoever - but we can't generate spurious completion events, as doing so falls into the uncanny valley. If the kernel is doing clever things behind userspace's back - such as stealing planes or CRTCs - then userspace can never know about it, apart from failing to enable those resources later. The kernel can either never do anything clever (and make userspace bind them both together), or be extremely clever (by hiding the entire details from userspace), but it cannot choose the halfway house of doing clever things behind userspace's back (such as stealing new CRTCs) whilst also exposing all those details to userspace (such as delivering spurious completion events for resources userspace never requested to be programmed). Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] Revert "drm/atomic: document and enforce rules around "spurious" EBUSY"
On Wed, 10 Feb 2021 at 15:07, Ville Syrjälä wrote: > On Wed, Feb 10, 2021 at 01:38:45PM +, Simon Ser wrote: > > The WARN_ON only happens if allow_modeset == false. If allow_modeset == > > true, > > then the driver is allowed to steal an unrelated pipe. > > > > Maybe i915 is stealing a pipe without allow_modeset? > > No. All page flips etc. will have to get split up internally > between multiple crtcs. I think this is the salient point. > So I think there's basically three options: > a) massive rewrite of i915 to bypass even more of drm_atomic stuff > b) allow i915 to silence that warning, which opens up the question >whether the warn is doing any good if it can just be bypassed > c) nuke the warning entirely > > a) is not going to happen, and it would any way allow i915 to > do things any which way it wants without tripping the warn, > rendering the warn entirely toothless. > > Hmm. Maybe there is a d) which would be to ignore all crtcs > that are not logically enabled in the warn? Not sure if that > could allow something to slit through that people want it to > catch? So from what I understand, if I enable CRTC 44 and the driver magically decides to split it up as a 'big-joiner' output, it will also steal CRTC 50 to work as the other half of the output. Then if I attach plane 47 to CRTC 44, posting a FB to plane 47 will result in me getting atomic completion events for both CRTC 44 and CRTC 50? That's not OK from a userspace perspective. If you want to do magic to create the illusion of a single CRTC, then that magic needs to be consistent. At the moment it's the worst kind of magic: it does implicit things under the hood for you, but then leaks all of the behind-the-scenes detail into userspace. Continuing with that would force us all to just ignore whatever events we see, because we can't reason about what they may be or why they're generated. Which doesn't allow for any kind of best practice in userspace. Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/ttm: don't set page->mapping
Hi, On Wed, 25 Nov 2020 at 18:06, Jason Gunthorpe wrote: > On Wed, Nov 25, 2020 at 05:28:32PM +0100, Daniel Vetter wrote: > > Apologies again, this shouldn't have been included. But at least I > > have an idea now why this patch somehow was included in the git > > send-email. Lovely interface :-/ > > I wrote a bit of a script around this because git send-email just too > hard to use > > The key workflow change I made was to have it prepare all the emails > to send and open them in an editor for review - exactly as they would > be sent to the lists. > > It uses a empty 'cover-letter' commit and automatically transforms it > into exactly the right stuff. Keeps track of everything you send in > git, and there is a little tool to auto-run git range-diff to help > build change logs.. This sounds a fair bit like patman, which does something similar and also lets you annotate commit messages with changelogs. But of course, suggesting different methods of carving patches into stone tablets to someone who's written their own, is even more of a windmill tilt than rDMA. ;) Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/atomic: debug output for EBUSY
Hi, On Fri, 25 Sep 2020 at 09:46, Daniel Vetter wrote: > Hopefully we'll have the drm crash recorder RSN, but meanwhile > compositors would like to know a bit better why they get an EBUSY. Thanks a lot, this is super helpful! Both patches are: Reviewed-by: Daniel Stone Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: document and enforce rules around "spurious" EBUSY from atomic_commit
Hi,
On Tue, 22 Sep 2020 at 19:18, Daniel Vetter wrote:
> + for_each_new_crtc_in_state(state, crtc, old_crtc_state, i)
> + affected_crtc |= drm_crtc_mask(crtc);
> +
> + /*
> +* For commits that allow modesets drivers can add other CRTCs to the
> +* atomic commit, e.g. when they need to reallocate global resources.
> +* This can cause spurious EBUSY, which robs compositors of a very
> +* effective sanity check for their drawing loop. Therefor only allow
> +* this for modeset commits.
> +*
> +* FIXME: Should add affected_crtc mask to the ATOMIC IOCTL as an
> output
> +* so compositors know what's going on.
> +*/
> + if (affected_crtc != requested_crtc) {
> + /* adding other CRTC is only allowed for modeset commits */
> + WARN_ON(!state->allow_modeset);
> + }
> +
Can we please add a DRM_DEBUG() here, regardless of the WARN_ON(), to
let people know what's happening?
With that, R-b me.
Cheers,
Daniel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: avoid spurious EBUSY due to nonblocking atomic modesets
Hi, On Tue, 22 Sep 2020 at 17:02, Daniel Vetter wrote: > On Tue, Sep 22, 2020 at 4:14 PM Daniel Stone wrote: > > I think we need a guarantee that this never happens if ALLOW_MODESET > > is always used in blocking mode, plus in future a cap we can use to > > detect that we won't be getting spurious EBUSY events. > > > > I really don't want to ever paper over this, because it's one of the > > clearest indications that userspace has its timing/signalling wrong. > > Ok so the hang-up last time around iirc was that I broke igt by making > a few things more synchronous. Let's hope I'm not also breaking stuff > with the WARN_ON ... > > New plan: > - make this patch here only document existing behaviour and enforce it > with the WARN_ON > - new uapi would be behind a flag or something, with userspace and > everything hanging off it. > > Thoughts? What do you mean by 'new uapi'? The proposal that the kernel communicates back which object IDs have been added to the state behind your back? That it's been made automatically blocking? Something else? Cheers, Daniel (the other one) ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: avoid spurious EBUSY due to nonblocking atomic modesets
On Tue, 22 Sep 2020 at 15:04, Daniel Vetter wrote: > On Tue, Sep 22, 2020 at 3:36 PM Marius Vlad wrote: > > Gentle ping. I've tried out Linus's master tree and, and like Pekka, > > I've noticed this isn't integrated/added. > > Defacto the uapi we have now is that userspace needs to ignore "spurious" > EBUSY. This really, really, really, bites. I think we need a guarantee that this never happens if ALLOW_MODESET is always used in blocking mode, plus in future a cap we can use to detect that we won't be getting spurious EBUSY events. I really don't want to ever paper over this, because it's one of the clearest indications that userspace has its timing/signalling wrong. Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] sw_sync deadlock avoidance, take 3
Hi all, On Wed, 15 Jul 2020 at 12:57, Daniel Vetter wrote: > On Wed, Jul 15, 2020 at 1:47 PM Daniel Stone wrote: > > On Wed, 15 Jul 2020 at 12:05, Bas Nieuwenhuizen > > wrote: > > > Yes, this is used as part of the Android stack on Chrome OS (need to > > > see if ChromeOS specific, but > > > https://source.android.com/devices/graphics/sync#sync_timeline > > > suggests not) > > > > Android used to mandate it for their earlier iteration of release > > fences, which was an empty/future fence having no guarantee of > > eventual forward progress until someone committed work later on. For > > example, when you committed a buffer to SF, it would give you an empty > > 'release fence' for that buffer which would only be tied to work to > > signal it when you committed your _next_ buffer, which might never > > happen. They removed that because a) future fences were a bad idea, > > and b) it was only ever useful if you assumed strictly > > FIFO/round-robin return order which wasn't always true. > > > > So now it's been watered down to 'use this if you don't have a > > hardware timeline', but why don't we work with Android people to get > > that removed entirely? > > I think there's some testcases still using these, but most real fence > testcases use vgem nowadays. So from an upstream pov there's indeed > not much if anything holding us back from just deleting this all. And > would probably be a good idea. It looks like this is just a docs hangover which can be fixed; sw_sync is no longer part of the unified Android kernel image, so it can no longer be relied on post-Treble. So let's just continue on the assumption that sw_sync is not anything anyone can rely on. Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] sw_sync deadlock avoidance, take 3
Hi, On Wed, 15 Jul 2020 at 12:05, Bas Nieuwenhuizen wrote: > On Wed, Jul 15, 2020 at 12:34 PM Chris Wilson > wrote: > > Maybe now is the time to ask: are you using sw_sync outside of > > validation? > > Yes, this is used as part of the Android stack on Chrome OS (need to > see if ChromeOS specific, but > https://source.android.com/devices/graphics/sync#sync_timeline > suggests not) Android used to mandate it for their earlier iteration of release fences, which was an empty/future fence having no guarantee of eventual forward progress until someone committed work later on. For example, when you committed a buffer to SF, it would give you an empty 'release fence' for that buffer which would only be tied to work to signal it when you committed your _next_ buffer, which might never happen. They removed that because a) future fences were a bad idea, and b) it was only ever useful if you assumed strictly FIFO/round-robin return order which wasn't always true. So now it's been watered down to 'use this if you don't have a hardware timeline', but why don't we work with Android people to get that removed entirely? Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] sw_sync deadlock avoidance, take 3
Hi, On Wed, 15 Jul 2020 at 11:23, Bas Nieuwenhuizen wrote: > My concern with going in this direction was that we potentially allow > an application to allocate a lot of kernel memory but not a lot of fds > by creating lots of fences and then closing the fds but never > signaling them. Is that not an issue? sw_sync is a userspace DoS mechanism by design - if someone wants to enable and use it, they have bigger problems than unbounded memory allocations. Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 03/25] dma-buf.rst: Document why idenfinite fences are a bad idea
On Thu, 9 Jul 2020 at 09:05, Daniel Vetter wrote: > On Thu, Jul 09, 2020 at 08:36:43AM +0100, Daniel Stone wrote: > > On Tue, 7 Jul 2020 at 21:13, Daniel Vetter wrote: > > > Comes up every few years, gets somewhat tedious to discuss, let's > > > write this down once and for all. > > > > Thanks for writing this up! I wonder if any of the notes from my reply > > to the previous-version thread would be helpful to more explicitly > > encode the carrot of dma-fence's positive guarantees, rather than just > > the stick of 'don't do this'. ;) Either way, this is: > > I think the carrot should go into the intro section for dma-fence, this > section here is very much just the "don't do this" part. The previous > patches have an attempt at encoding this a bit, maybe see whether there's > a place for your reply (or parts of it) to fit? Sounds good to me. > > Acked-by: Daniel Stone > > > > > What I'm not sure about is whether the text should be more explicit in > > > flat out mandating the amdkfd eviction fences for long running compute > > > workloads or workloads where userspace fencing is allowed. > > > > ... or whether we just say that you can never use dma-fence in > > conjunction with userptr. > > Uh userptr is entirely different thing. That one is ok. It's userpsace > fences or gpu futexes or future fences or whatever we want to call them. > Or is there some other confusion here?. I mean generating a dma_fence from a batch which will try to page in userptr. Given that userptr could be backed by absolutely anything at all, it doesn't seem smart to allow fences to rely on a pointer to an mmap'ed NFS file. So it seems like batches should be mutually exclusive between arbitrary SVM userptr and generating a dma-fence? Speaking of entirely different things ... the virtio-gpu bit really doesn't belong in this patch. Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 03/25] dma-buf.rst: Document why idenfinite fences are a bad idea
Hi, On Tue, 7 Jul 2020 at 21:13, Daniel Vetter wrote: > Comes up every few years, gets somewhat tedious to discuss, let's > write this down once and for all. Thanks for writing this up! I wonder if any of the notes from my reply to the previous-version thread would be helpful to more explicitly encode the carrot of dma-fence's positive guarantees, rather than just the stick of 'don't do this'. ;) Either way, this is: Acked-by: Daniel Stone > What I'm not sure about is whether the text should be more explicit in > flat out mandating the amdkfd eviction fences for long running compute > workloads or workloads where userspace fencing is allowed. ... or whether we just say that you can never use dma-fence in conjunction with userptr. Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 01/25] dma-fence: basic lockdep annotations
Hi, On Wed, 8 Jul 2020 at 16:13, Daniel Vetter wrote: > On Wed, Jul 8, 2020 at 4:57 PM Christian König > wrote: > > Could we merge this controlled by a separate config option? > > > > This way we could have the checks upstream without having to fix all the > > stuff before we do this? > > Since it's fully opt-in annotations nothing blows up if we don't merge > any annotations. So we could start merging the first 3 patches. After > that the fun starts ... > > My rough idea was that first I'd try to tackle display, thus far > there's 2 actual issues in drivers: > - amdgpu has some dma_resv_lock in commit_tail, plus a kmalloc. I > think those should be fairly easy to fix (I'd try a stab at them even) > - vmwgfx has a full on locking inversion with dma_resv_lock in > commit_tail, and that one is functional. Not just reading something > which we can safely assume to be invariant anyway (like the tmz flag > for amdgpu, or whatever it was). > > I've done a pile more annotations patches for other atomic drivers > now, so hopefully that flushes out any remaining offenders here. Since > some of the annotations are in helper code worst case we might need a > dev->mode_config.broken_atomic_commit flag to disable them. At least > for now I have 0 plans to merge any of these while there's known > unsolved issues. Maybe if some drivers take forever to get fixed we > can then apply some duct-tape for the atomic helper annotation patch. > Instead of a flag we can also copypasta the atomic_commit_tail hook, > leaving the annotations out and adding a huge warning about that. How about an opt-in drm_driver DRIVER_DEADLOCK_HAPPY flag? At first this could just disable the annotations and nothing else, but as we see the annotations gaining real-world testing and maturity, we could eventually make it taint the kernel. Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 03/18] dma-fence: basic lockdep annotations
Hi, Jumping in after a couple of weeks where I've paged most everything out of my brain ... On Fri, 19 Jun 2020 at 10:43, Daniel Vetter wrote: > On Fri, Jun 19, 2020 at 10:13:35AM +0100, Chris Wilson wrote: > > > The proposed patches might very well encode the wrong contract, that's > > > all up for discussion. But fundamentally questioning that we need one > > > is missing what upstream is all about. > > > > Then I have not clearly communicated, as my opinion is not that > > validation is worthless, but that the implementation is enshrining a > > global property on a low level primitive that prevents it from being > > used elsewhere. And I want to replace completion [chains] with fences, and > > bio with fences, and closures with fences, and what other equivalencies > > there are in the kernel. The fence is as central a locking construct as > > struct completion and deserves to be a foundational primitive provided > > by kernel/ used throughout all drivers for discrete problem domains. > > > > This is narrowing dma_fence whereby adding > > struct lockdep_map *dma_fence::wait_map > > and annotating linkage, allows you to continue to specify that all > > dma_fence used for a particular purpose must follow common rules, > > without restricting the primitive for uses outside of this scope. > > Somewhere else in this thread I had discussions with Jason Gunthorpe about > this topic. It might maybe change somewhat depending upon exact rules, but > his take is very much "I don't want dma_fence in rdma". Or pretty close to > that at least. > > Similar discussions with habanalabs, they're using dma_fence internally > without any of the uapi. Discussion there has also now concluded that it's > best if they remove them, and simply switch over to a wait_queue or > completion like every other driver does. > > The next round of the patches already have a paragraph to at least > somewhat limit how non-gpu drivers use dma_fence. And I guess actual > consensus might be pointing even more strongly at dma_fence being solely > something for gpus and closely related subsystem (maybe media) for syncing > dma-buf access. > > So dma_fence as general replacement for completion chains I think just > wont happen. > > What might make sense is if e.g. the lockdep annotations could be reused, > at least in design, for wait_queue or completion or anything else > really. I do think that has a fair chance compared to the automagic > cross-release annotations approach, which relied way too heavily on > guessing where barriers are. My experience from just a bit of playing > around with these patches here and discussing them with other driver > maintainers is that accurately deciding where critical sections start and > end is a job for humans only. And if you get it wrong, you will have a > false positive. > > And you're indeed correct that if we'd do annotations for completions and > wait queues, then that would need to have a class per semantically > equivalent user, like we have lockdep classes for mutexes, not just one > overall. > > But dma_fence otoh is something very specific, which comes with very > specific rules attached - it's not a generic wait_queue at all. Originally > it did start out as one even, but it is a very specialized wait_queue. > > So there's imo two cases: > > - Your completion is entirely orthogonal of dma_fences, and can never ever > block a dma_fence. Don't use dma_fence for this, and no problem. It's > just another wait_queue somewhere. > > - Your completion can eventually, maybe through lots of convolutions and > depdencies, block a dma_fence. In that case full dma_fence rules apply, > and the only thing you can do with a custom annotation is make the rules > even stricter. E.g. if a sub-timeline in the scheduler isn't allowed to > take certain scheduler locks. But the userspace visible/published fence > do take them, maybe as part of command submission or retirement. > Entirely hypotethical, no idea any driver actually needs this. I don't claim to understand the implementation of i915's scheduler and GEM handling, and it seems like there's some public context missing here. But to me, the above is a good statement of what I (and a lot of other userspace) have been relying on - that dma-fence is a very tightly scoped thing which is very predictable but in extremis. It would be great to have something like this enshrined in dma-fence documentation, visible to both kernel and external users. The properties we've so far been assuming for the graphics pipeline - covering production & execution of vertex/fragment workloads on the GPU, framebuffer display, and to the extent this is necessary involving compute - are something like this: A single dma-fence with no dependencies represents (the tail of) a unit of work, which has been all but committed to the hardware. Once committed to the hardware, this work will complete (successfully or in error) in bounded time. The unit of work referred to
Re: [Intel-gfx] [PATCH 03/18] dma-fence: basic lockdep annotations
Hi, On Thu, 11 Jun 2020 at 09:44, Dave Airlie wrote: > On Thu, 11 Jun 2020 at 18:01, Chris Wilson wrote: > > Introducing a global lockmap that cannot capture the rules correctly, > > Can you document the rules all drivers should be following then, > because from here it looks to get refactored every version of i915, > and it would be nice if we could all aim for the same set of things > roughly. We've already had enough problems with amdgpu vs i915 vs > everyone else with fences, if this stops that in the future then I'd > rather we have that than just some unwritten rules per driver and > untestable. As someone who has sunk a bunch of work into explicit-fencing awareness in my compositor so I can never be blocked, I'd be disappointed if the infrastructure was ultimately pointless because the documented fencing rules were \_o_/ or thereabouts. Lockdep definitely isn't my area of expertise so I can't comment on the patch per se, but having something to ensure we don't hit deadlocks sure seems a lot better than nothing. Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: avoid spurious EBUSY due to nonblocking atomic modesets
On Thu, 14 May 2020 at 08:25, Daniel Vetter wrote: > On Thu, May 14, 2020 at 9:18 AM Daniel Stone wrote: > > On Thu, 14 May 2020 at 08:08, Daniel Vetter wrote: > > I'd be very much in favour of putting the blocking down in the kernel > > at least until the kernel can give us a clear indication to tell us > > what's going on, and ideally which other resources need to be dragged > > in, in a way which is distinguishable from your compositor having > > broken synchronisation. > > We know, the patch already computes that ... So would be a matter of > exporting that to userspace. We have a mask of all additional crtc > that will get an event and will -EBUSY until that's done. Yep, but unless and until that happens, could we please get this in? Given it would require uAPI changes, we'd need to modify all the compositors to work with the old path (random EBUSY) and the new path (predictable and obvious), so at least preserving the promise that per-CRTC updates are really independent would be good. Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: avoid spurious EBUSY due to nonblocking atomic modesets
Hi, On Thu, 14 May 2020 at 08:08, Daniel Vetter wrote: > > Did anything happen with this? > > Nope. There's an igt now that fails with this, and I'm not sure > whether changing the igt is the right idea or not. > > I'm kinda now thinking about changing this to instead document under > which exact situations you can get a spurious EBUSY, and enforcing > that in the code with some checks. Essentially only possible if you do > a ALLOW_MODESET | NONBLOCKING on the other crtc. And then tell > userspace you get to eat that. We've been shipping with this for so > long by now that's defacto the uapi anyway :-/ > > Thoughts? Too horrible? I've been trying to avoid that, to be honest. Taking a random delay because the kernel needs to do global things is fine. But making userspace either do an expensive/complicated cross-CRTC synchronisation is less easy; for some compositors, that means reaching across threads to make sure all CRTCs are quiescent. Either that, or deferring your ALLOW_MODESET to somewhere else, like an idle handler, far away from where you were originally trying to do it, which wouldn't be pleasant. The other option is that we teach people to ignore EBUSY as random noise which can just sometimes happen and to try again (when? how often? and you still have cross-CRTC synchronisation issues), which doesn't scream compositor best practice to me. I'd be very much in favour of putting the blocking down in the kernel at least until the kernel can give us a clear indication to tell us what's going on, and ideally which other resources need to be dragged in, in a way which is distinguishable from your compositor having broken synchronisation. Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: avoid spurious EBUSY due to nonblocking atomic modesets
On Wed, 8 Apr 2020 at 17:24, Daniel Vetter wrote: > Resending because last attempt failed CI and meanwhile the results are > lost :-/ Did anything happen with this? Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] gitlab.fd.o financial situation and impact on services
Hi Jan, On Fri, 28 Feb 2020 at 10:09, Jan Engelhardt wrote: > On Friday 2020-02-28 08:59, Daniel Stone wrote: > >I believe that in January, we had $2082 of network cost (almost > >entirely egress; ingress is basically free) and $1750 of > >cloud-storage cost (almost all of which was download). That's based > >on 16TB of cloud-storage (CI artifacts, container images, file > >uploads, Git LFS) egress and 17.9TB of other egress (the web service > >itself, repo activity). Projecting that out [×12 for a year] gives > >us roughly $45k of network activity alone, > > I had come to a similar conclusion a few years back: It is not very > economic to run ephemereal buildroots (and anything like it) between > two (or more) "significant locations" of which one end is located in > a Large Cloud datacenter like EC2/AWS/etc. > > As for such usecases, me and my surrounding peers have used (other) > offerings where there is 50 TB free network/month, and yes that may > have entailed doing more adminning than elsewhere - but an admin > appreciates $2000 a lot more than a corporation, too. Yes, absolutely. For context, our storage & network costs have increased >10x in the past 12 months (~$320 Jan 2019), >3x in the past 6 months (~$1350 July 2019), and ~2x in the past 3 months (~$2000 Oct 2019). I do now (personally) think that it's crossed the point at which it would be worthwhile paying an admin to solve the problems that cloud services currently solve for us - which wasn't true before. Such an admin could also deal with things like our SMTP delivery failure rate, which in the past year has spiked over 50% (see previous email), demand for new services such as Discourse which will enable user support without either a) users having to subscribe to a mailing list, or b) bug trackers being cluttered up with user requests and other non-bugs, etc. Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [Mesa-dev] gitlab.fd.o financial situation and impact on services
On Fri, 28 Feb 2020 at 10:06, Erik Faye-Lund wrote: > On Fri, 2020-02-28 at 11:40 +0200, Lionel Landwerlin wrote: > > Yeah, changes on vulkan drivers or backend compilers should be > > fairly > > sandboxed. > > > > We also have tools that only work for intel stuff, that should never > > trigger anything on other people's HW. > > > > Could something be worked out using the tags? > > I think so! We have the pre-defined environment variable > CI_MERGE_REQUEST_LABELS, and we can do variable conditions: > > https://docs.gitlab.com/ee/ci/yaml/#onlyvariablesexceptvariables > > That sounds like a pretty neat middle-ground to me. I just hope that > new pipelines are triggered if new labels are added, because not > everyone is allowed to set labels, and sometimes people forget... There's also this which is somewhat more robust: https://gitlab.freedesktop.org/mesa/mesa/merge_requests/2569 Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] gitlab.fd.o financial situation and impact on services
On Fri, 28 Feb 2020 at 08:48, Dave Airlie wrote: > On Fri, 28 Feb 2020 at 18:18, Daniel Stone wrote: > > The last I looked, Google GCP / Amazon AWS / Azure were all pretty > > comparable in terms of what you get and what you pay for them. > > Obviously providers like Packet and Digital Ocean who offer bare-metal > > services are cheaper, but then you need to find someone who is going > > to properly administer the various machines, install decent > > monitoring, make sure that more storage is provisioned when we need > > more storage (which is basically all the time), make sure that the > > hardware is maintained in decent shape (pretty sure one of the fd.o > > machines has had a drive in imminent-failure state for the last few > > months), etc. > > > > Given the size of our service, that's a much better plan (IMO) than > > relying on someone who a) isn't an admin by trade, b) has a million > > other things to do, and c) hasn't wanted to do it for the past several > > years. But as long as that's the resources we have, then we're paying > > the cloud tradeoff, where we pay more money in exchange for fewer > > problems. > > Admin for gitlab and CI is a full time role anyways. The system is > definitely not self sustaining without time being put in by you and > anholt still. If we have $75k to burn on credits, and it was diverted > to just pay an admin to admin the real hw + gitlab/CI would that not > be a better use of the money? I didn't know if we can afford $75k for > an admin, but suddenly we can afford it for gitlab credits? s/gitlab credits/GCP credits/ I took a quick look at HPE, which we previously used for bare metal, and it looks like we'd be spending $25-50k (depending on how much storage you want to provision, how much room you want to leave to provision more storage later, how much you care about backups) to run a similar level of service so that'd put a bit of a dint in your year-one budget. The bare-metal hosting providers also add up to more expensive than you might think, again especially if you want either redundancy or just backups. > > Yes, we could federate everything back out so everyone runs their own > > builds and executes those. Tinderbox did something really similar to > > that IIRC; not sure if Buildbot does as well. Probably rules out > > pre-merge testing, mind. > > Why? does gitlab not support the model? having builds done in parallel > on runners closer to the test runners seems like it should be a thing. > I guess artifact transfer would cost less then as a result. It does support the model but if every single build executor is also compiling Mesa from scratch locally, how long do you think that's going to take? > > Again, if you want everything to be centrally > > designed/approved/monitored/controlled, that's a fine enough idea, and > > I'd be happy to support whoever it was who was doing that for all of > > fd.o. > > I don't think we have any choice but to have someone centrally > controlling it, You can't have a system in place that lets CI users > burn largs sums of money without authorisation, and that is what we > have now. OK, not sure who it is who's going to be approving every update to every .gitlab-ci.yml in the repository, or maybe we just have zero shared runners and anyone who wants to do builds can BYO. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] gitlab.fd.o financial situation and impact on services
On Fri, 28 Feb 2020 at 03:38, Dave Airlie wrote: > b) we probably need to take a large step back here. > > Look at this from a sponsor POV, why would I give X.org/fd.o > sponsorship money that they are just giving straight to google to pay > for hosting credits? Google are profiting in some minor way from these > hosting credits being bought by us, and I assume we aren't getting any > sort of discounts here. Having google sponsor the credits costs google > substantially less than having any other company give us money to do > it. The last I looked, Google GCP / Amazon AWS / Azure were all pretty comparable in terms of what you get and what you pay for them. Obviously providers like Packet and Digital Ocean who offer bare-metal services are cheaper, but then you need to find someone who is going to properly administer the various machines, install decent monitoring, make sure that more storage is provisioned when we need more storage (which is basically all the time), make sure that the hardware is maintained in decent shape (pretty sure one of the fd.o machines has had a drive in imminent-failure state for the last few months), etc. Given the size of our service, that's a much better plan (IMO) than relying on someone who a) isn't an admin by trade, b) has a million other things to do, and c) hasn't wanted to do it for the past several years. But as long as that's the resources we have, then we're paying the cloud tradeoff, where we pay more money in exchange for fewer problems. > If our current CI architecture is going to burn this amount of money a > year and we hadn't worked this out in advance of deploying it then I > suggest the system should be taken offline until we work out what a > sustainable system would look like within the budget we have, whether > that be never transferring containers and build artifacts from the > google network, just having local runner/build combos etc. Yes, we could federate everything back out so everyone runs their own builds and executes those. Tinderbox did something really similar to that IIRC; not sure if Buildbot does as well. Probably rules out pre-merge testing, mind. The reason we hadn't worked everything out in advance of deploying is because Mesa has had 3993 MRs in the not long over a year since moving, and a similar number in GStreamer, just taking the two biggest users. At the start it was 'maybe let's use MRs if you want to but make sure everything still goes through the list', and now it's something different. Similarly the CI architecture hasn't been 'designed', so much as that people want to run dEQP and Piglit on their hardware pre-merge in an open fashion that's actually accessible to people, and have just done it. Again, if you want everything to be centrally designed/approved/monitored/controlled, that's a fine enough idea, and I'd be happy to support whoever it was who was doing that for all of fd.o. Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] gitlab.fd.o financial situation and impact on services
Hi Matt, On Thu, 27 Feb 2020 at 23:45, Matt Turner wrote: > We're paying 75K USD for the bandwidth to transfer data from the > GitLab cloud instance. i.e., for viewing the https site, for > cloning/updating git repos, and for downloading CI artifacts/images to > the testing machines (AFAIU). I believe that in January, we had $2082 of network cost (almost entirely egress; ingress is basically free) and $1750 of cloud-storage cost (almost all of which was download). That's based on 16TB of cloud-storage (CI artifacts, container images, file uploads, Git LFS) egress and 17.9TB of other egress (the web service itself, repo activity). Projecting that out gives us roughly $45k of network activity alone, so it looks like this figure is based on a projected increase of ~50%. The actual compute capacity is closer to $1150/month. > I was not aware that we were being charged for anything wrt GitLab > hosting yet (and neither was anyone on my team at Intel that I've > asked). This... kind of needs to be communicated. > > A consistent concern put forth when we were discussing switching to > GitLab and building CI was... how do we pay for it. It felt like that > concern was always handwaved away. I heard many times that if we > needed more runners that we could just ask Google to spin up a few > more. If we needed testing machines they'd be donated. No one > mentioned that all the while we were paying for bandwidth... Perhaps > people building the CI would make different decisions about its > structure if they knew it was going to wipe out the bank account. The original answer is that GitLab themselves offered to sponsor enough credit on Google Cloud to get us started. They used GCP themselves so they could assist us (me) in getting bootstrapped, which was invaluable. After that, Google's open-source program office offered to sponsor us for $30k/year, which was I believe last April. Since then the service usage has increased roughly by a factor of 10, so our 12-month sponsorship is no longer enough to cover 12 months. > What percentage of the bandwidth is consumed by transferring CI > images, etc? Wouldn't 75K USD would be enough to buy all the testing > machines we need and host them within Google or wherever so we don't > need to pay for huge amounts of bandwidth? Unless the Google Cloud Platform starts offering DragonBoards, it wouldn't reduce our bandwidth usage as the corporate network is treated separately for egress. Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC][PATCH 5/5] drm/i915/display: Add Nearest-neighbor based integer scaling support
Hi,
On Tue, 25 Feb 2020 at 07:17, Pankaj Bharadiya
wrote:
> @@ -415,18 +415,26 @@ skl_program_scaler(struct intel_plane *plane,
> u16 y_vphase, uv_rgb_vphase;
> int hscale, vscale;
> const struct drm_plane_state *state = &plane_state->uapi;
> + u32 src_w = drm_rect_width(&plane_state->uapi.src) >> 16;
> + u32 src_h = drm_rect_height(&plane_state->uapi.src) >> 16;
> u32 scaling_filter = PS_FILTER_MEDIUM;
> + struct drm_rect dst;
>
> if (state->scaling_filter == DRM_SCALING_FILTER_NEAREST_NEIGHBOR) {
> scaling_filter = PS_FILTER_PROGRAMMED;
> + skl_setup_nearest_neighbor_filter(dev_priv, pipe, scaler_id);
> +
> + /* Make the scaling window size to integer multiple of source
> +* TODO: Should userspace take desision to round scaling
> window
> +* to integer multiple?
> +*/
> + crtc_w = rounddown(crtc_w, src_w);
> + crtc_h = rounddown(crtc_h, src_h);
The kernel should absolutely not be changing the co-ordinates that
userspace requested.
Cheers,
Daniel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: avoid spurious EBUSY due to nonblocking atomic modesets
On Thu, 5 Jul 2018 at 11:21, Daniel Vetter wrote: > When doing an atomic modeset with ALLOW_MODESET drivers are allowed to > pull in arbitrary other resources, including CRTCs (e.g. when > reconfiguring global resources). > > But in nonblocking mode userspace has then no idea this happened, > which can lead to spurious EBUSY calls, both: > - when that other CRTC is currently busy doing a page_flip the > ALLOW_MODESET commit can fail with an EBUSY > - on the other CRTC a normal atomic flip can fail with EBUSY because > of the additional commit inserted by the kernel without userspace's > knowledge > > For blocking commits this isn't a problem, because everyone else will > just block until all the CRTC are reconfigured. Only thing userspace > can notice is the dropped frames without any reason for why frames got > dropped. > > Consensus is that we need new uapi to handle this properly, but no one > has any idea what exactly the new uapi should look like. As a stop-gap > plug this problem by demoting nonblocking commits which might cause > issues by including CRTCs not in the original request to blocking > commits. Thanks for writing this up Daniel, and for reminding me about it some time later as well ... Reviewed-by: Daniel Stone Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/fbdev: Fallback to non tiled mode if all tiles not present
Hi, On Wed, 6 Nov 2019 at 02:47, Dave Airlie wrote: > Otherwise I think this seems fine, though it does beg the question in > my mind of what happens if I get 2 8K monitors, and plug the first > tile of one in, and the second tile of the other in. Honestly in that case I think 'you get to literally keep both pieces' is a reasonable answer. Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: Add support for integrated privacy screens
Hi Thierry, On Fri, 25 Oct 2019 at 12:36, Thierry Reding wrote: > On Thu, Oct 24, 2019 at 01:45:16PM -0700, Rajat Jain wrote: > > I did think about having a state variable in software to get and set > > this. However, I think it is not very far fetched that some platforms > > may have "hardware kill" switches that allow hardware to switch > > privacy-screen on and off directly, in addition to the software > > control that we are implementing. Privacy is a touchy subject in > > enterprise, and anything that reduces the possibility of having any > > inconsistency between software state and hardware state is desirable. > > So in this case, I chose to not have a state in software about this - > > we just report the hardware state everytime we are asked for it. > > So this doesn't really work with atomic KMS, then. The main idea behind > atomic KMS is that you apply a configuration either completely or not at > all. So at least for setting this property you'd have to go through the > state object. > > Now, for reading out the property you might be able to get away with the > above. I'm not sure if that's enough to keep the state up-to-date, > though. Is there some way for a kill switch to trigger an interrupt or > other event of some sort so that the state could be kept up-to-date? > > Daniel (or anyone else), do you know of any precedent for state that > might get modified behind the atomic helpers' back? Seems to me like we > need to find some point where we can actually read back the current > "hardware value" of this privacy screen property and store that back > into the state. Well, apart from connector state, though that isn't really a property as such, there's the link_state property, which is explicitly designed to do just that. That has been quite carefully designed for the back-and-forth though. Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PULL] drm-misc-next
Hi, On Tue, 6 Aug 2019 at 10:58, Daniel Vetter wrote: > On Tue, Aug 6, 2019 at 11:55 AM Emil Velikov wrote: > > On Tue, 6 Aug 2019 at 10:49, Daniel Vetter wrote: > > > The thing is, dim push shouldn't allow you to do that. And the patches > > > have clearly been applied with dim apply (or at least you added the > > > Link), unlike Rob who seems to just have pushed the revert. > > > > > Thanks, did not know about dim push. Will make sure I use it. > > So the intro doc isn't good enough, and we need to enforce it. I think > Daniel's idea was to have a pre-merge hook which checks for a git > variable using --push-option. Can you pls look into this? I guess we'd > need the dim patch, and example premerge hook to be installed > server-side. Should have a nice error message too ofc. Yeah, the docs are already quite clear that you cannot push to the DRM trees with normal git, and that you have to use dim. Not only does it check and enforce all the rules in the documentation, but it also rebuilds drm-tip and keeps other trees in sync, which isn't done with a regular git push. The idea I had a few weeks ago was to have dim use 'git push --push-option fdo.pushedWithDim=this-was-pushed-with-dim-and-not-manually', then have the hooks on the server side check for that option and refuse any direct pushes. (Or at least, if people are pushing directly, they have to _really_ try to be doing it, and can't do it by accident.) If someone types up the dim patch to do that and gets it committed, after a couple of days' grace period for everyone to update I can roll out the server-side hooks which refuse non-dim pushes. Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] Fwd: PSA: Mailman changes, From addresses no longer accurate
Hi all, Unfortunately, freedesktop.org's job of sending mail to a huge number of people whilst pretending to be other people, has just got even harder than it was. fd.o can no longer (at least for the moment) send mail with the From addresses of DMARC/DKIM/SPF-protected sender domains. When we try to do so, large providers reject the mail, despite DMARC records explicitly specifying that the mail should be accepted. Worse than that, not only does the immediate delivery attempt fail, but it punishes our sender reputation enough that _other_ mail delivery fails: after we hit a DMARC-related failure, large providers often refuse delivery attempts for mail from non-DMARC-protected domains. As a result, if the sender domain has a DMARC record, we rewrite the From address to be the list's address, preserving the original author in Reply-To. I'm chasing this up through a few channels, but in the meantime, please be aware that the From address on mails may no longer be accurate. If you are attempting to apply patches with git-am, please check that the commit author is not 'Person Name via listname-devel '. If you are replying privately to a list mail, please be _very_ careful that you are replying to the original sender (in Reply-To) and not the list itself. Cheers, Daniel -- Forwarded message ----- From: Daniel Stone Date: Mon, 11 Feb 2019 at 23:38 Subject: PSA: Mailman changes, From addresses no longer accurate To: , Hi all, We have hit another step change in aggressive anti-spam techniques from major mail providers. Over the past few days, we saw a huge spike in the number of mails we were failing to deliver to GMail and outlook.com in particular. It looks like it is now no longer acceptable for us to break DMARC/DKIM/SPF. These are DNS-based extensions to SMTP, which allow domains to publish policies as to who should be allowed to send email on their behalf. SPF provides source filtering, so e.g. freedesktop.org could specify that no-one should accept mail with a From: *@freedesktop.org unless it came from gabe.freedesktop.org. Mailman completely breaks this: if I specified a filter only allowing Google to send mail for @fooishbar.org, then anyone enforcing SPF would reject receipt of this mail, as it would arrive from fd.o with my From address. DKIM allows domains to publish a public key in DNS, inserting a header into mails sent from their domain cryptographically signing the value of named headers. Mailman breaks this too: changing the Sender header (such that bounces get processed by Mailman, rather than sending a deluge of out-of-office and mailbox-over-quota messages to whoever posts to the list) can break most DKIM signatures. Mailman adding the unsubscribe footer also breaks this; we could make it not add the footer, but in order to do so we'd have to convince ourselves that we were not decreasing our GDPR compliance. DMARC ties the two together, allowing domains to specify whether or not DKIM/SPF should be mandatory, and if they fail, what action should be taken. Despite some domains specifying a fail action of 'none' (receiving MTA to send an advisory report to a named email address, but still allow the email), it seems that popular services still interpret 'none' as 'reject'. As a result, Google in particular is dropping some number of our mails on the floor. This does _not_ just apply to mails which fail DMARC/DKIM/SPF: every mail we send that fails these filters (which is a lot - e.g. Intel and NVIDIA both have SPF) decreases our sender reputation with GMail and causes it to reject legitimate mails. I've reached out to Google through a couple of channels to see what we can do to increase our delivery rate to them. In the meantime, if your mail is hosted by Google, or Outlook, and you think you're missing mails - you probably are. Mailman has also now been reconfigured such that if it spots a potential DMARC violation, it rewrites the From address to be @lists.freedesktop.org, keeping the original author in Reply-To. It also strips DKIM headers. This seems to be about the best we can do, unless and until the major mail service providers offer us some alternate way to send mail. If you are replying privately to someone, you should check very carefully that you are replying to them and not to the list. Unfortunately we don't have a good answer in the long run. The latest published RFC at https://tools.ietf.org/html/rfc6377 suggests that there are no good solutions. If anyone is blessed with an abundance of time and familiar with the current email landscape, I would love to talk to you and get your help to work on this. Unfortunately we don't have the manpower to actually properly monitor email; it can often take a collapse in successful-delivery rates for us to notice. Ultimately, I suspect the best solution is to move most of our discussions to dedicated fora like GitLab issues, or some
Re: [Intel-gfx] [v7 1/2] drm: Add colorspace connector property
Hi, On Tue, 29 Jan 2019 at 15:24, Brian Starkey wrote: > On Tue, Jan 29, 2019 at 01:30:43PM +, Shankar, Uma wrote: > > >> +#define DP_COLORIMETRY_SCRGB 15 > > >> +#define DP_COLORIMETRY_DCI_P3 16 > > >> +#define DP_COLORIMETRY_CUSTOM_COLOR_PROFILE 17 > > Sorry, I somehow missed your reply earlier in the month - but this > wasn't what I meant. > > The expectation with enum properties is that the numeric values are > determined at runtime - userspace shouldn't depend on them being known > at compile-time. So, in include/uapi there shouldn't be these numeric > values. > > The strings themselves effectively form the UABI, so I was wondering > if they should be defined in include/uapi, but you would be the first > to do that. > > Daniel Vetter and/or Daniel Stone might have opinions on this. That's correct. The DPMS definitions are a historical aberration, and we should not be adding any more numeric definitions of enum properties to uABI. They can be fixed in the kernel, but userspace must only rely on the strings. Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [igt-dev] RFC: Migration to Gitlab
Hi Rodrigo, On Wed, 22 Aug 2018 at 17:06, Rodrigo Vivi wrote: > On Wed, Aug 22, 2018 at 10:19:19AM -0400, Adam Jackson wrote: > > On Wed, 2018-08-22 at 16:13 +0300, Jani Nikula wrote: > > > - Sticking to fdo bugzilla and disabling gitlab issues for at least > > > drm-intel for the time being. Doing that migration in the same go is a > > > bit much I think. Reassignment across bugzilla and gitlab will be an > > > issue. > > > > Can you elaborate a bit on the issues here? The actual move-the-bugs > > process has been pretty painless for the parts of xorg we've done so > > far. > > I guess there is nothing against moving the bugs there. The concern is only on > doing everything at once. > > I'm in favor of moving gits for now and after we are confident that > everything is there and working we move the bugs. As Daniel alluded to, the only issue I really have is moving _all_ the kernel repos at once. At the end of the year we'll have easy automatic scaling thanks to the independent services being separated. As it is, all the GitLab services (apart from CI runners) run on a single machine, so we have limited options if it becomes overwhelmed with load. Do you have a particular concern about the repos? e.g. what would you check for to make sure things are 'there and working'? > One question about the bugzilla: > > Will all the referrences on all commit messages get outdated after > bugzilla is dead? > Or bugzilla will stay up for referrence but closed for interaction? > or all old closed stuff are always moved and bugzilla.fd.o as well and > bugzilla.fd.o will be mirroring gitlab? When bugs are migrated from Bugzilla to GitLab, only open bugs are migrated. Closed ones are left in place, as is; open ones have a comment at the end saying that the bug has moved to GitLab, a URL linking to the new GitLab issue, and telling them to please chase it up there. Even when we move everyone completely off Bugzilla, we will keep it as a read-only mirror forever. Even with Phabricator, which very few people ever used, has had all its bugs and code review captured and archived, so we can continue to preserve all the old content and links, without having to run the actual service. Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [igt-dev] RFC: Migration to Gitlab
Hi, On Wed, 22 Aug 2018 at 15:44, Daniel Vetter wrote: > On Wed, Aug 22, 2018 at 3:13 PM, Jani Nikula > wrote: > > Just a couple of concerns from drm/i915 perspective for starters: > > > > - Patchwork integration. I think we'll want to keep patchwork for at > > least intel-gfx etc. for the time being. IIUC the one thing we need is > > some server side hook to update patchwork on git push. > > > > - Sticking to fdo bugzilla and disabling gitlab issues for at least > > drm-intel for the time being. Doing that migration in the same go is a > > bit much I think. Reassignment across bugzilla and gitlab will be an > > issue. > > Good points, forgot about both. Patchwork reading the mailing list > should keep working as-is, but the update hook needs looking into. All the hooks are retained. gitlab.fd.o pushes to git.fd.o, and git.fd.o still executes all the old hooks. This includes Patchwork, readthedocs, AppVeyor, and whatever else. > For merge requests I think best approach is to enable them very > selectively at first for testing out, and then making a per-subproject > decision whether they make sense. E.g. I think for maintainer-tools > integrating make check and the doc building into gitlab CI would be > sweet, and worth looking into gitlab merge requests just to automate > that. Again best left out of scope for the initial migration. You don't need MRs to do that. You can build a .gitlab-ci.yml file which will run make check or build the docs or whatever, and have that run on pushes. Anyone can then fork the repo, push their changes to that fork, and see the CI results from there. It's like Travis: the CI configuration is a (mutable) part of the codebase, not a separate 'thing' hanging off a specific repo. So if you write the CI pipeline first, you can have people running CI on push completely independently of switching the workflow to use MRs. Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] RFC: Migration to Gitlab
Hi, On Wed, 22 Aug 2018 at 16:02, Emil Velikov wrote: > On 22 August 2018 at 12:44, Daniel Vetter wrote: > > I think it's time to brainstorm a bit about the gitlab migration. Basic > > reasons: > > > > - fd.o admins want to deprecate shell accounts and hand-rolled > > infrastructure, because it's a pain to keep secure&updated. > > > > - gitlab will allow us to add committers on our own, greatly > > simplifying that process (and offloading that task from fd.o admins). > > Random thought - I really wish the admins spoke early and louder about issues. > From infra to manpower and adhoc tool maintenance. I thought I mostly had it covered, but fair enough. What knowledge are you missing and how should it best be delivered? One first-order issue is that as documented at https://www.freedesktop.org/wiki/AccountRequests/ creating accounts requires manual admin intervention. You can also search the 'freedesktop.org' product on Bugzilla to see the amount of time we spend shuffling around GPG & SSH keys, which is about the worst possible use of my time. Many other people have had access to drive the ancient shell-script frontend to LDAP before, but for some reason they mostly aren't very enthusiastic about doing it all the time. In the mesa-dev@ thread about Mesa's migration, which is linked from my blog post, I went into quite a lot of detail about why Jenkins was not suitable to roll out across fd.o globally. That knowledge was gained from trial & error, which was a lot of time burnt. The end result is that we don't have any CI, except if people hang Travis/AppVeyor off GitHub mirrors. You've personally seen what's involved in setting up Git repository hooks so we can build docs. We can't give access to let people work on those, without giving them direct access to the literal Git repository itself on disk. The hooks mostly involve bespoke sets of rsync jobs and hand-managed SSH credentials and external services to build docs and so on and so forth. None of this is accessible to a newcomer who wants to make a non-code contribution: you have to find someone with access to the repo to go bash some shell scripts directly and hope you didn't screw it up. None of this is auditable, so if the repo mysteriously gets wiped, then hopefully there are some backups somewhere. But there definitely aren't any logs. Luckily we prevent most people from having access to most repositories via a mandatory 'shell' which only allows people to push Git; this was written by hand by us 12 years ago. What else? Our fork of Patchwork was until recently based on an ancient fork of Django, in a bespoke unreproducible production environment. Bugzilla is patched for spam and templates (making upgrades complex), yet we still have a surprising amount of spam pass through. There's no way to delete spam, but you have to manually move every bug to the 'spam' group, then go through and delete the user which involves copying & pasting the email and a few clicks per user. Mailman is patched to support Recaptcha, bringing more upgrade fun. ikiwiki breaks all the time because it's designed to have the public-access web server on the same host as the writeable Git repositories. Our servers are several years old and approaching life expiry, and we have no more spare disk. You can see in #freedesktop IRC the constant network connectivity issues people have to PSU almost every day. Our attempts to root-cause and solve those have got nowhere. I could go on, but the 'moved elsewhere' list in https://gitlab.freedesktop.org/freedesktop/freedesktop/issues/2 indicates that we do have problems to solve, and that changing peoples' SSH keys for them isn't the best way for us to be spending the extremely limited time that we do have. > > For the full in-depth writeup of everything, see > > > > https://www.fooishbar.org/blog/gitlab-fdo-introduction/ If you haven't seen this, or the post it was linked from, they would be a good start: https://lists.freedesktop.org/archives/freedesktop/2018-July/000370.html There's also the long thread on mesa-dev a long time back which covers a lot of this ground already. > > - Figuring out the actual migration - we've been adding a pile of > > committers since fd.o LDAP was converted to gitlab once back in > > spring. We need to at least figure out how to move the new > > accounts/committers. > > As a observer, allow me to put some ideas. You've mostly covered them > all, my emphasis is to seriously stick with _one_ thing at a time. > Attempting to do multiple things in parallel will end up with > sub-optimal results. > > - (at random point) cleanup the committers list - people who have not > contributed in the last 1 year? libdrm was previously covered under the Mesa ACL. Here are the other committer lists, which you can see via 'getent group' on annarchy or another machine: amdkfd:x:922:fxkuehl,agd5f,deathsimple,danvet,jazhou,jbridgman,hwentland,tstdenis,gitlab-mirror,rui drm-meson:x:936:narmstrong drm:x:940:airlie
Re: [Intel-gfx] Shared atomic state causing Weston repaint failure
Hey Jakob, On Thu, 5 Jul 2018 at 14:32, Jakob Bornecrantz wrote: > So from a VR compositor getting blocked like this is a no-go as the > user would quickly throw EPUKE. The situation is compounded by the > fact that the VR compositor has no idea what the display compositor is > doing with regards to setting modes so can not do any mitigating on > its side (like displaying a black screen). Yeah, definitely. > Some solutions that springs to mind (some I admit are probably not possible). > > - Make sure we don't get into this situation by locking the resources > of the VR crtc group or allocating enough bandwidth for the display > compositor crtcs up front. > > - Add priority and preemption to atomic so that VR compositor can > never be blocked. > > - Add X/Wayland protocol for the compositor to tell the VR compositor > that a modeset might effect it, so it can display a black-screen > during that time. > > - Make it possible for the VR compositor to tell the kernel what it > should do this case, like show black if I happen to get block before I > can queue a new pageflip. The specific issue on Intel is that they have a shared FIFO setup, and that enabling/reconfiguring a new CRTC requires FIFO reconfiguration, which in turn requires a global stall, even if you're not touching any leased resources directly. So there's no way to avoid the resources, or get around it with priorities. Ultimately I think we're going to need a general protocol to deal with this though. I can think of other situations - say, DDR reclocking, where the reclocking isn't voluntary but forced by platform thermals - where you might need to sit out a frame or two due to external factors. Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] Shared atomic state causing Weston repaint failure
Hi, The atomic API being super-explicit about how userspace sequences its calls is great and all, but having shared global state implicitly dragged in is kind of ruining my day. Currently on Intel, Weston sometimes fails on hotplug, because a commit which only enables CRTC B (not touching CRTC A or any other CRTC!), causes all commits to CRTC A to fail until CRTC B's modeset commit has fully retired: https://gitlab.freedesktop.org/wayland/weston/issues/24 The reason is that committing CRTC B resizes the DDB allocation for CRTC A as well, pulling CRTC A's CRTC state into the commit. This makes sense, but on the other hand it's totally opaque to userspace, and impossible for us to reason about when making commits. I suggested some options in that GitLab commit, none of which I like: * if any other CRTCs are pulled into a commit state, always execute a blocking commit in the kernel * if we're passing ALLOW_MODESET in userspace, only ever do blocking commits * whenever we get -EBUSY in userspace, assume we've been screwed by the kernel and defer until other outputs have completed * whenever we want to reconfigure any output in userspace, wait until all outputs are completely quiescent and do a single atomic commit covering all outputs The first one seems completely non-obvious from the kernel, but on the other hand the current -EBUSY failing behaviour is also non-obvious. The second is maybe the most reasonable, but on the other hand just working around a painful leaky abstraction: we also can't know upfront from userspace if this is actually going to be required, or if we're just killing responsiveness blocking for no reason. The third is the thing I least want to do, because it might well paper over legitimate bugs in userspace, and complicates our state tracking for no reason. The fourth is probably the most legitimate, but, well ... someone has to type up all the code to make our output-configuration API completely asynchronous. I suspect we're the first ones to be hitting this, because Weston has a truly independent per-CRTC repaint loop, we're one of the few atomic users, and also because Pekka did some seriously brutal hotplug testing whilst reworking Weston's output configuration API. Also because our approach to failed output repaints is to just freeze the output until it next cycles off and on, which is much more apparent than just silently dropping a frame here and there. ;) Any bright ideas on what could practically be done here? Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v3 2/2] drm/i915: Move GEM BO inside drm_framebuffer
Since drm_framebuffer can now store GEM objects directly, place them
there rather than in our own subclass.
v2: Only hold a single reference per framebuffer, not per plane. (Ville)
v3: Drop NULL check in intel_fb_obj. (Ville)
Signed-off-by: Daniel Stone
Reviewed-by: Ville Syrjälä
Cc: Jani Nikula
Cc: Joonas Lahtinen
Cc: Rodrigo Vivi
Cc: intel-gfx@lists.freedesktop.org
---
drivers/gpu/drm/i915/intel_display.c | 4 ++--
drivers/gpu/drm/i915/intel_drv.h | 3 +--
2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 1b2cf631305e..12226a2c8d39 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14370,9 +14370,9 @@ static int intel_framebuffer_init(struct
intel_framebuffer *intel_fb,
i, fb->pitches[i], stride_alignment);
goto err;
}
- }
- intel_fb->obj = obj;
+ fb->obj[i] = &obj->base;
+ }
ret = intel_fill_fb_info(dev_priv, fb);
if (ret)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 12002fc77235..9cb1dc219af8 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -194,7 +194,6 @@ enum intel_output_type {
struct intel_framebuffer {
struct drm_framebuffer base;
- struct drm_i915_gem_object *obj;
struct intel_rotation_info rot_info;
/* for each plane in the normal GTT view */
@@ -1005,7 +1004,7 @@ struct cxsr_latency {
#define to_intel_framebuffer(x) container_of(x, struct intel_framebuffer, base)
#define to_intel_plane(x) container_of(x, struct intel_plane, base)
#define to_intel_plane_state(x) container_of(x, struct intel_plane_state, base)
-#define intel_fb_obj(x) (x ? to_intel_framebuffer(x)->obj : NULL)
+#define intel_fb_obj(x) ((x) ? to_intel_bo((x)->obj[0]) : NULL)
struct intel_hdmi {
i915_reg_t hdmi_reg;
--
2.17.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v3 1/2] drm/i915: Use intel_fb_obj() everywhere
We already have a macro to pull the GEM object from a FB, so use it
everywhere. We'll make use of this later to move the object storage.
Signed-off-by: Daniel Stone
Reviewed-by: Ville Syrjälä
Cc: Jani Nikula
Cc: Joonas Lahtinen
Cc: Rodrigo Vivi
Cc: intel-gfx@lists.freedesktop.org
---
drivers/gpu/drm/i915/i915_debugfs.c | 4 ++--
drivers/gpu/drm/i915/intel_display.c | 19 ++-
drivers/gpu/drm/i915/intel_fbdev.c | 9 +
3 files changed, 17 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index 52515445ac40..e9b1b8df6ef5 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1908,7 +1908,7 @@ static int i915_gem_framebuffer_info(struct seq_file *m,
void *data)
fbdev_fb->base.format->cpp[0] * 8,
fbdev_fb->base.modifier,
drm_framebuffer_read_refcount(&fbdev_fb->base));
- describe_obj(m, fbdev_fb->obj);
+ describe_obj(m, intel_fb_obj(&fbdev_fb->base));
seq_putc(m, '\n');
}
#endif
@@ -1926,7 +1926,7 @@ static int i915_gem_framebuffer_info(struct seq_file *m,
void *data)
fb->base.format->cpp[0] * 8,
fb->base.modifier,
drm_framebuffer_read_refcount(&fb->base));
- describe_obj(m, fb->obj);
+ describe_obj(m, intel_fb_obj(&fb->base));
seq_putc(m, '\n');
}
mutex_unlock(&dev->mode_config.fb_lock);
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index c9ec88acad9c..1b2cf631305e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2474,6 +2474,7 @@ intel_fill_fb_info(struct drm_i915_private *dev_priv,
{
struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
struct intel_rotation_info *rot_info = &intel_fb->rot_info;
+ struct drm_i915_gem_object *obj = intel_fb_obj(fb);
u32 gtt_offset_rotated = 0;
unsigned int max_size = 0;
int i, num_planes = fb->format->num_planes;
@@ -2538,7 +2539,7 @@ intel_fill_fb_info(struct drm_i915_private *dev_priv,
* fb layout agrees with the fence layout. We already check
that the
* fb stride matches the fence stride elsewhere.
*/
- if (i == 0 && i915_gem_object_is_tiled(intel_fb->obj) &&
+ if (i == 0 && i915_gem_object_is_tiled(obj) &&
(x + width) * cpp > fb->pitches[i]) {
DRM_DEBUG_KMS("bad fb plane %d offset: 0x%x\n",
i, fb->offsets[i]);
@@ -2623,9 +2624,9 @@ intel_fill_fb_info(struct drm_i915_private *dev_priv,
max_size = max(max_size, offset + size);
}
- if (max_size * tile_size > intel_fb->obj->base.size) {
+ if (max_size * tile_size > obj->base.size) {
DRM_DEBUG_KMS("fb too big for bo (need %u bytes, have %zu
bytes)\n",
- max_size * tile_size, intel_fb->obj->base.size);
+ max_size * tile_size, obj->base.size);
return -EINVAL;
}
@@ -14082,14 +14083,15 @@ static void intel_setup_outputs(struct
drm_i915_private *dev_priv)
static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
{
struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
+ struct drm_i915_gem_object *obj = intel_fb_obj(fb);
drm_framebuffer_cleanup(fb);
- i915_gem_object_lock(intel_fb->obj);
- WARN_ON(!intel_fb->obj->framebuffer_references--);
- i915_gem_object_unlock(intel_fb->obj);
+ i915_gem_object_lock(obj);
+ WARN_ON(!obj->framebuffer_references--);
+ i915_gem_object_unlock(obj);
- i915_gem_object_put(intel_fb->obj);
+ i915_gem_object_put(obj);
kfree(intel_fb);
}
@@ -14098,8 +14100,7 @@ static int intel_user_framebuffer_create_handle(struct
drm_framebuffer *fb,
struct drm_file *file,
unsigned int *handle)
{
- struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
- struct drm_i915_gem_object *obj = intel_fb->obj;
+ struct drm_i915_gem_object *obj = intel_fb_obj(fb);
if (obj->userptr.mm) {
DRM_DEBUG("attempting to use a userptr for a framebuffer,
denied\n");
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c
b/drivers/gpu/drm/i915/intel_fbdev.c
index e9e02b58b7be..fb2f9fce34cd 100644
--- a/drivers/gpu/drm/i
[Intel-gfx] [PATCH v2 1/2] drm/i915: Use intel_fb_obj() everywhere
We already have a macro to pull the GEM object from a FB, so use it
everywhere. We'll make use of this later to move the object storage.
Signed-off-by: Daniel Stone
Cc: Jani Nikula
Cc: Joonas Lahtinen
Cc: Rodrigo Vivi
Cc: Ville Syrjälä
Cc: intel-gfx@lists.freedesktop.org
---
drivers/gpu/drm/i915/i915_debugfs.c | 4 ++--
drivers/gpu/drm/i915/intel_display.c | 19 ++-
drivers/gpu/drm/i915/intel_fbdev.c | 9 +
3 files changed, 17 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index 52515445ac40..e9b1b8df6ef5 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1908,7 +1908,7 @@ static int i915_gem_framebuffer_info(struct seq_file *m,
void *data)
fbdev_fb->base.format->cpp[0] * 8,
fbdev_fb->base.modifier,
drm_framebuffer_read_refcount(&fbdev_fb->base));
- describe_obj(m, fbdev_fb->obj);
+ describe_obj(m, intel_fb_obj(&fbdev_fb->base));
seq_putc(m, '\n');
}
#endif
@@ -1926,7 +1926,7 @@ static int i915_gem_framebuffer_info(struct seq_file *m,
void *data)
fb->base.format->cpp[0] * 8,
fb->base.modifier,
drm_framebuffer_read_refcount(&fb->base));
- describe_obj(m, fb->obj);
+ describe_obj(m, intel_fb_obj(&fb->base));
seq_putc(m, '\n');
}
mutex_unlock(&dev->mode_config.fb_lock);
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index c9ec88acad9c..1b2cf631305e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2474,6 +2474,7 @@ intel_fill_fb_info(struct drm_i915_private *dev_priv,
{
struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
struct intel_rotation_info *rot_info = &intel_fb->rot_info;
+ struct drm_i915_gem_object *obj = intel_fb_obj(fb);
u32 gtt_offset_rotated = 0;
unsigned int max_size = 0;
int i, num_planes = fb->format->num_planes;
@@ -2538,7 +2539,7 @@ intel_fill_fb_info(struct drm_i915_private *dev_priv,
* fb layout agrees with the fence layout. We already check
that the
* fb stride matches the fence stride elsewhere.
*/
- if (i == 0 && i915_gem_object_is_tiled(intel_fb->obj) &&
+ if (i == 0 && i915_gem_object_is_tiled(obj) &&
(x + width) * cpp > fb->pitches[i]) {
DRM_DEBUG_KMS("bad fb plane %d offset: 0x%x\n",
i, fb->offsets[i]);
@@ -2623,9 +2624,9 @@ intel_fill_fb_info(struct drm_i915_private *dev_priv,
max_size = max(max_size, offset + size);
}
- if (max_size * tile_size > intel_fb->obj->base.size) {
+ if (max_size * tile_size > obj->base.size) {
DRM_DEBUG_KMS("fb too big for bo (need %u bytes, have %zu
bytes)\n",
- max_size * tile_size, intel_fb->obj->base.size);
+ max_size * tile_size, obj->base.size);
return -EINVAL;
}
@@ -14082,14 +14083,15 @@ static void intel_setup_outputs(struct
drm_i915_private *dev_priv)
static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
{
struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
+ struct drm_i915_gem_object *obj = intel_fb_obj(fb);
drm_framebuffer_cleanup(fb);
- i915_gem_object_lock(intel_fb->obj);
- WARN_ON(!intel_fb->obj->framebuffer_references--);
- i915_gem_object_unlock(intel_fb->obj);
+ i915_gem_object_lock(obj);
+ WARN_ON(!obj->framebuffer_references--);
+ i915_gem_object_unlock(obj);
- i915_gem_object_put(intel_fb->obj);
+ i915_gem_object_put(obj);
kfree(intel_fb);
}
@@ -14098,8 +14100,7 @@ static int intel_user_framebuffer_create_handle(struct
drm_framebuffer *fb,
struct drm_file *file,
unsigned int *handle)
{
- struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
- struct drm_i915_gem_object *obj = intel_fb->obj;
+ struct drm_i915_gem_object *obj = intel_fb_obj(fb);
if (obj->userptr.mm) {
DRM_DEBUG("attempting to use a userptr for a framebuffer,
denied\n");
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c
b/drivers/gpu/drm/i915/intel_fbdev.c
index e9e02b58b7be..fb2f9fce34cd 100644
--- a/drivers/gpu/drm/i
[Intel-gfx] [PATCH v2 2/2] drm/i915: Move GEM BO inside drm_framebuffer
Since drm_framebuffer can now store GEM objects directly, place them
there rather than in our own subclass.
v2: Only hold a single reference per framebuffer, not per plane. (Ville)
Signed-off-by: Daniel Stone
Cc: Ville Syrjälä
Cc: Jani Nikula
Cc: Joonas Lahtinen
Cc: Rodrigo Vivi
Cc: intel-gfx@lists.freedesktop.org
---
drivers/gpu/drm/i915/intel_display.c | 4 ++--
drivers/gpu/drm/i915/intel_drv.h | 3 +--
2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 1b2cf631305e..12226a2c8d39 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14370,9 +14370,9 @@ static int intel_framebuffer_init(struct
intel_framebuffer *intel_fb,
i, fb->pitches[i], stride_alignment);
goto err;
}
- }
- intel_fb->obj = obj;
+ fb->obj[i] = &obj->base;
+ }
ret = intel_fill_fb_info(dev_priv, fb);
if (ret)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 12002fc77235..03e1d1d7fb58 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -194,7 +194,6 @@ enum intel_output_type {
struct intel_framebuffer {
struct drm_framebuffer base;
- struct drm_i915_gem_object *obj;
struct intel_rotation_info rot_info;
/* for each plane in the normal GTT view */
@@ -1005,7 +1004,7 @@ struct cxsr_latency {
#define to_intel_framebuffer(x) container_of(x, struct intel_framebuffer, base)
#define to_intel_plane(x) container_of(x, struct intel_plane, base)
#define to_intel_plane_state(x) container_of(x, struct intel_plane_state, base)
-#define intel_fb_obj(x) (x ? to_intel_framebuffer(x)->obj : NULL)
+#define intel_fb_obj(x) (((x) && (x)->obj[0]) ? to_intel_bo((x)->obj[0]) :
NULL)
struct intel_hdmi {
i915_reg_t hdmi_reg;
--
2.17.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/4] drm/i915: Move GEM BO inside drm_framebuffer
Hi Ville, On 23 March 2018 at 14:49, Daniel Stone wrote: > On 23 March 2018 at 14:42, Ville Syrjälä > wrote: >> Hmm. I'm thinking we can stick to the single reference per fb. >> IIRC this counter is there just to prevent changes of the obj >> tiling mode as long as any fb exists that uses the object. So >> doesn't actually matter how many planes the fb has. >> >> Naturally the story would be slightly difference if we supported >> fbs using multiple different BOs, as each BO would need to get its >> framebuffer_references adjusted. > > Yeah, fair enough. It looks a little bit weird (perhaps deserving of a > comment) there. The reason to do that was just the general principle > of having one reference per object pointer, especially when other > drivers (ones which can have separate BOs in a single logical image) > will and do refcount them separately. Having different refcounting > semantics in shared structures depending on which driver is in use > makes me itchy. Absent any other comment, I've dropped this change and will keep a single framebuffer_reference[s] for v2. Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 00/24] drm_framebuffer boilerplate removal
Hi, I've been working on a getfb2[0] ioctl, which amongst other things supports multi-planar framebuffers as well as modifiers. getfb currently calls the framebuffer's handle_create hook, which doesn't support multiple planes. Thanks to Noralf's recent work, drivers can just store GEM objects directly in drm_framebuffer. I use this directly in getfb2: we need direct access to the GEM objects and not a vfunc in order to not hand out duplicate GEM names for the same object. This series converts all drivers except for nouveau, which was a little too non-trivial for my comfort, to storing GEM objects directly in drm_framebuffer. For those drivers whose driver_framebuffer struct was nothing but drm_framebuffer + BO, it deletes the driver-specific struct. It also makes use of Noralf's generic framebuffer helpers for create_handle and destroy where possible. I don't have the hardware for most of these drivers, so have had to settle for just staring really hard at the diff. I intend to remove create_handle when all drivers are converted over to placing BOs directly inside drm_framebuffer. For most drivers there's a relatively easy conversion to using the helpers for basically all framebuffer handling and fbdev emulation as well, though that's a bit further than I was willing to go without hardware to test on ... Cheers, Daniel [0]: https://lists.freedesktop.org/archives/dri-devel/2018-March/170512.html ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/4] drm/i915: Move GEM BO inside drm_framebuffer
Hi Ville, On 23 March 2018 at 14:42, Ville Syrjälä wrote: > On Fri, Mar 23, 2018 at 01:45:50PM +0000, Daniel Stone wrote: >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -13916,7 +13916,8 @@ static void intel_user_framebuffer_destroy(struct >> drm_framebuffer *fb) >> drm_framebuffer_cleanup(fb); >> >> i915_gem_object_lock(obj); >> - WARN_ON(!obj->framebuffer_references--); >> + WARN_ON(obj->framebuffer_references < fb->format->num_planes); >> + obj->framebuffer_references -= fb->format->num_planes; > > Hmm. I'm thinking we can stick to the single reference per fb. > IIRC this counter is there just to prevent changes of the obj > tiling mode as long as any fb exists that uses the object. So > doesn't actually matter how many planes the fb has. > > Naturally the story would be slightly difference if we supported > fbs using multiple different BOs, as each BO would need to get its > framebuffer_references adjusted. Yeah, fair enough. It looks a little bit weird (perhaps deserving of a comment) there. The reason to do that was just the general principle of having one reference per object pointer, especially when other drivers (ones which can have separate BOs in a single logical image) will and do refcount them separately. Having different refcounting semantics in shared structures depending on which driver is in use makes me itchy. Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/4] drm/i915: Use intel_fb_obj() everywhere
We already have a macro to pull the GEM object from a FB, so use it
everywhere. We'll make use of this later to move the object storage.
Signed-off-by: Daniel Stone
Cc: Jani Nikula
Cc: Joonas Lahtinen
Cc: Rodrigo Vivi
Cc: intel-gfx@lists.freedesktop.org
---
drivers/gpu/drm/i915/i915_debugfs.c | 4 ++--
drivers/gpu/drm/i915/intel_display.c | 19 ++-
drivers/gpu/drm/i915/intel_fbdev.c | 9 +
3 files changed, 17 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index 7816cd53100a..68541df6f601 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1893,7 +1893,7 @@ static int i915_gem_framebuffer_info(struct seq_file *m,
void *data)
fbdev_fb->base.format->cpp[0] * 8,
fbdev_fb->base.modifier,
drm_framebuffer_read_refcount(&fbdev_fb->base));
- describe_obj(m, fbdev_fb->obj);
+ describe_obj(m, intel_fb_obj(&fbdev_fb->base));
seq_putc(m, '\n');
}
#endif
@@ -1911,7 +1911,7 @@ static int i915_gem_framebuffer_info(struct seq_file *m,
void *data)
fb->base.format->cpp[0] * 8,
fb->base.modifier,
drm_framebuffer_read_refcount(&fb->base));
- describe_obj(m, fb->obj);
+ describe_obj(m, intel_fb_obj(&fb->base));
seq_putc(m, '\n');
}
mutex_unlock(&dev->mode_config.fb_lock);
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 1b16b534871a..49b0772e9abc 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2478,6 +2478,7 @@ intel_fill_fb_info(struct drm_i915_private *dev_priv,
{
struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
struct intel_rotation_info *rot_info = &intel_fb->rot_info;
+ struct drm_i915_gem_object *obj = intel_fb_obj(fb);
u32 gtt_offset_rotated = 0;
unsigned int max_size = 0;
int i, num_planes = fb->format->num_planes;
@@ -2542,7 +2543,7 @@ intel_fill_fb_info(struct drm_i915_private *dev_priv,
* fb layout agrees with the fence layout. We already check
that the
* fb stride matches the fence stride elsewhere.
*/
- if (i == 0 && i915_gem_object_is_tiled(intel_fb->obj) &&
+ if (i == 0 && i915_gem_object_is_tiled(obj) &&
(x + width) * cpp > fb->pitches[i]) {
DRM_DEBUG_KMS("bad fb plane %d offset: 0x%x\n",
i, fb->offsets[i]);
@@ -2627,9 +2628,9 @@ intel_fill_fb_info(struct drm_i915_private *dev_priv,
max_size = max(max_size, offset + size);
}
- if (max_size * tile_size > intel_fb->obj->base.size) {
+ if (max_size * tile_size > obj->base.size) {
DRM_DEBUG_KMS("fb too big for bo (need %u bytes, have %zu
bytes)\n",
- max_size * tile_size, intel_fb->obj->base.size);
+ max_size * tile_size, obj->base.size);
return -EINVAL;
}
@@ -13910,14 +13911,15 @@ static void intel_setup_outputs(struct
drm_i915_private *dev_priv)
static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
{
struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
+ struct drm_i915_gem_object *obj = intel_fb_obj(fb);
drm_framebuffer_cleanup(fb);
- i915_gem_object_lock(intel_fb->obj);
- WARN_ON(!intel_fb->obj->framebuffer_references--);
- i915_gem_object_unlock(intel_fb->obj);
+ i915_gem_object_lock(obj);
+ WARN_ON(!obj->framebuffer_references--);
+ i915_gem_object_unlock(obj);
- i915_gem_object_put(intel_fb->obj);
+ i915_gem_object_put(obj);
kfree(intel_fb);
}
@@ -13926,8 +13928,7 @@ static int intel_user_framebuffer_create_handle(struct
drm_framebuffer *fb,
struct drm_file *file,
unsigned int *handle)
{
- struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
- struct drm_i915_gem_object *obj = intel_fb->obj;
+ struct drm_i915_gem_object *obj = intel_fb_obj(fb);
if (obj->userptr.mm) {
DRM_DEBUG("attempting to use a userptr for a framebuffer,
denied\n");
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c
b/drivers/gpu/drm/i915/intel_fbdev.c
index 65a3313723c9..fdef18488fb1 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/d
[Intel-gfx] [PATCH 2/4] drm/i915: Move GEM BO inside drm_framebuffer
Since drm_framebuffer can now store GEM objects directly, place them
there rather than in our own subclass.
Signed-off-by: Daniel Stone
Cc: Jani Nikula
Cc: Joonas Lahtinen
Cc: Rodrigo Vivi
Cc: intel-gfx@lists.freedesktop.org
---
drivers/gpu/drm/i915/intel_display.c | 15 ---
drivers/gpu/drm/i915/intel_drv.h | 3 +--
2 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 49b0772e9abc..e8100a4fc877 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13916,7 +13916,8 @@ static void intel_user_framebuffer_destroy(struct
drm_framebuffer *fb)
drm_framebuffer_cleanup(fb);
i915_gem_object_lock(obj);
- WARN_ON(!obj->framebuffer_references--);
+ WARN_ON(obj->framebuffer_references < fb->format->num_planes);
+ obj->framebuffer_references -= fb->format->num_planes;
i915_gem_object_unlock(obj);
i915_gem_object_put(obj);
@@ -14176,9 +14177,9 @@ static int intel_framebuffer_init(struct
intel_framebuffer *intel_fb,
i, fb->pitches[i], stride_alignment);
goto err;
}
- }
- intel_fb->obj = obj;
+ fb->obj[i] = &obj->base;
+ }
ret = intel_fill_fb_info(dev_priv, fb);
if (ret)
@@ -14190,6 +14191,14 @@ static int intel_framebuffer_init(struct
intel_framebuffer *intel_fb,
goto err;
}
+ /* We already hold one reference to the fb, but in case it's
+* multi-planar and we've placed multiple pointers in
+* drm_framebuffer, hold extra refs.
+*/
+ i915_gem_object_lock(obj);
+ obj->framebuffer_references += fb->format->num_planes - 1;
+ i915_gem_object_unlock(obj);
+
return 0;
err:
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 570f89b31544..d93ed9e59867 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -186,7 +186,6 @@ enum intel_output_type {
struct intel_framebuffer {
struct drm_framebuffer base;
- struct drm_i915_gem_object *obj;
struct intel_rotation_info rot_info;
/* for each plane in the normal GTT view */
@@ -985,7 +984,7 @@ struct cxsr_latency {
#define to_intel_framebuffer(x) container_of(x, struct intel_framebuffer, base)
#define to_intel_plane(x) container_of(x, struct intel_plane, base)
#define to_intel_plane_state(x) container_of(x, struct intel_plane_state, base)
-#define intel_fb_obj(x) (x ? to_intel_framebuffer(x)->obj : NULL)
+#define intel_fb_obj(x) (((x) && (x)->obj[0]) ? to_intel_bo((x)->obj[0]) :
NULL)
struct intel_hdmi {
i915_reg_t hdmi_reg;
--
2.16.2
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: Fix uabi regression by allowing garbage mode->type from userspace
On 21 March 2018 at 21:12, Ville Syrjala wrote: > Apparently xf86-video-vmware leaves the mode->type uninitialized > when feeding the mode to the kernel. Thus we have no choice but > to accept the garbage in. We'll just ignore any of the bits we > don't want. The mode type is just a hint anyway, and more > useful for the kernel->userspace direction. Reviewed-by: Daniel Stone ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t] meson: Chamelium depends on GSL
Chamelium support requires igt_frame to be built, which requires both
GSL and Pixman.
Signed-off-by: Daniel Stone
---
meson.build | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/meson.build b/meson.build
index ef7017cb..5b783e5d 100644
--- a/meson.build
+++ b/meson.build
@@ -76,7 +76,7 @@ if not xmlrpc.found() and xmlrpc_cmd.found()
endif
endif
-if pixman.found() and xmlrpc.found() and xmlrpc_util.found() and
xmlrpc_client.found()
+if pixman.found() and gsl.found() and xmlrpc.found() and xmlrpc_util.found()
and xmlrpc_client.found()
chamelium = declare_dependency(dependencies : [ pixman, xmlrpc,
xmlrpc_util, xmlrpc_client])
config.set('HAVE_CHAMELIUM', 1)
--
2.16.2
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 3/8] drm/i915: Add the missing Y/Yf modifiers for SKL+ sprites
Hi Ville,Given you've tested, my reservations are dropped, so this series is:Acked-by: Daniel Stone Sorry for the mobile client formatting.Cheers,Daniel___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 08/12] drm/i915: Add CCS capability for sprites
Hi, On 11 December 2017 at 12:08, Mika Kahola wrote: > On Mon, 2017-12-11 at 12:00 +0000, Daniel Stone wrote: >> Did you manage to test this? When I tried, the DDB/watermark >> allocation was too conservative for sprites, and never allowed enough >> blocks to be able to use anything but linear or X-tiled on sprite >> planes. The allocation was almost entirely taken up by the primary >> plane, even if that itself was later set to linear or X-tiled. > > I tried to test this but unfortunately this didn't apply on top of drm- > tip. The whole series dates back to August so a lot have changed since > then. > > I would need this patch for my CNL work so I would like to see this > merged. I would like to see this merged, but I would also like to see it ever working first. :) When I tried it at the time, even a 256x256 plane could not use Y-tiling or CCS, due to DDB exhaustion. Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 08/12] drm/i915: Add CCS capability for sprites
Hi Mika, On 11 December 2017 at 11:11, Mika Kahola wrote: > On Thu, 2017-08-24 at 22:10 +0300, ville.syrj...@linux.intel.com wrote: >> Allow sprites to scan out compressed framebuffers. >> >> Since different platforms have a different set of planes that >> support CCS let's add a small helper to determine whether a >> specific plane supports CCS or not. Currently that information >> is spread around in many places, and not all the pieces of >> code even agree with each other. >> >> In addition to allowing sprites to scan out compressed fbs, >> the other fix here is that we stop rejecting them on pipe C >> on CNL. > Unfortunately, this patch didn't apply cleanly on top of the latest > drm-tip. Overall, the patch looks good to me. Did you manage to test this? When I tried, the DDB/watermark allocation was too conservative for sprites, and never allowed enough blocks to be able to use anything but linear or X-tiled on sprite planes. The allocation was almost entirely taken up by the primary plane, even if that itself was later set to linear or X-tiled. I observed this on SKL with both 3200x1800 and 2560x1440 output sizes (and primary plane sizes; no scaling), as well as APL with 1920x1080. Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC PATCH 1/6] drm: Add Content Protection property
Hi Pavel, On 5 December 2017 at 17:34, Pavel Machek wrote: > Yes, so... This patch makes it more likely to see machines with locked > down kernels, preventing developers from working with systems their > own, running hardware. That is evil, and direct threat to Free > software movement. > > Users compiling their own kernels get no benefit from it. Actually it > looks like this only benefits Intel and Disney. We don't want that. With all due respect, you can't claim to speak for the entire kernel and FLOSS community of users and developers. The feature is optional: it does not enforce additional constraints on users, but exposes additional functionality already present in hardware, for those who wish to opt in to it. Those who wish to avoid it can do so, by simply not making active use of it. Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/7] Add Plane Color Properties
Hi Uma, On 10 November 2017 at 08:37, Shankar, Uma wrote: >>This is missing documentation on how plane colour management interacts with >>CRTC colour management. Is it a step before CRTC colour management is >>applied, or does it bypass CRTC colour management, or ... ? >> > > We can have color correction at 2 places in the Display hardware pipeline > 1. At plane level (before blending) > 2. At pipe level (after blending) > > Both can be used and function independently of each other. Typical use case > can be: > > 2 Layers planes/overlays with different color space and formats. These can be > converted to one > common color space using the plane level color correction, then blended > together. If needed > pipe color correction can be applied on the blended output (based on use case > and scenarios) This is roughly what I'd expected, but I don't believe the same to be true of all hardware, and it would certainly need to be documented. Thanks for the explanation! Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/7] Add Plane Color Properties
Hi Uma, On 7 November 2017 at 12:06, Uma Shankar wrote: > This patch series adds properties for plane color features. It adds > properties for degamma used to linearize data, CSC used for gamut > conversion, and gamma used to again non-linearize data as per panel > supported color space. These can be utilize by user space to convert > planes from one format to another, one color space to another etc. > > Usersapce can take smart blending decisions and utilize these hardware > supported plane color features to get accurate color profile. The same > can help in consistent color quality from source to panel taking > advantage of advanced color features in hardware. > > These patches just add the property interfaces and enable helper functions. This is missing documentation on how plane colour management interacts with CRTC colour management. Is it a step before CRTC colour management is applied, or does it bypass CRTC colour management, or ... ? Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/dmc: DMC 1.04 for Kabylake
Hi, On 2 November 2017 at 18:04, Pandiyan, Dhinakaran wrote: > On Thu, 2017-11-02 at 07:27 -0700, Rodrigo Vivi wrote: >> That's intentional. The idea is to send to linux-firmware only after it >> passes our CI. So, prepare already in a way that it is easy to just forward >> when >> that happens. >> >> But what I believe we can change is to send that in the cover-letter of >> the series. >> So cover-letter with pull-request that CI would get automatically, >> all related patches on the series, so right now it could be: >> patch 0: pull-request >> patch 1: kbl dmc 1.04 >> patch 2: skl dmc 1.27 > This patch updates only KBL firmware. Is there an upcoming 1.27 release > for SKL? If there is, it hasn't yet made it to 01.org either. Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t 00/22] RFC: meson build system support
On 5 September 2017 at 13:36, Daniel Vetter wrote: > Ok, this time around a proper patch series with cover letter and a pile of > fixes (bunch of them thanks to Eric) thrown on top. > > The motivation for this has 2 primary reasons: > > - I want a build system that's fast, especially for hacking on the > library. Currently with over 300 binaries the relinking step every time > you change the library is extremely painful. > > - I want a build system that I can hack on. After years of automake, I > still don't get it. After a few days of meson I have at least the > illusion I understand stuff. And that's with an obviously still fairly > fresh tool with the occasional sharp corner (lesson learned: if it > complains about the meson files, it's a missing comman nearby). > > - Finally there's the question of whether this will die like previous > attempts at a better build toolchain, or whether the meson/ninja combo > will win. There's a lot of very enthusastic initial conversion in > various X.org projects, and the people I've chatted are extremely > positive on this. I think meson/ninja could very well be the git of the > build: Painful to use in the first years, but has the fundamental rights > and will win in the end. > > Assuming we can get some consensus around this I'd like to merge it and > polish the meson support in-tree, it's kinda growing into a bigger series > already. And of course we need to keep autohell working for probably a > fairly long time, at least for the tools that distro install. Yes please. I've taken a couple of passes looking at it, and it seems fine to me. Acked-by: Daniel Stone Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 4/7] tests/kms_ccs: Test case where the CCS buffer was not provided
Hi Gabriel,
On 31 August 2017 at 06:58, Gabriel Krisman Bertazi
wrote:
> @@ -321,16 +325,19 @@ static void generate_fb(data_t *data, struct igt_fb *fb,
> size[1] = f.pitches[1] * ALIGN(ccs_height, 32);
>
> f.handles[0] = gem_create(data->drm_fd, size[0] + size[1]);
> - f.handles[1] = f.handles[0];
> - render_ccs(data, f.handles[1], f.offsets[1], size[1],
> - height, f.pitches[1]);
> +
> + if (!(data->flags & TEST_NO_AUX_BUFFER)) {
> + f.handles[1] = f.handles[0];
> + render_ccs(data, f.handles[1], f.offsets[1], size[1],
> + height, f.pitches[1]);
> + }
Doesn't this leave modifier[1] still set? If so, that'll get rejected
by core code already for a mismatch.
Cheers,
Daniel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 5/7] tests/kms_ccs: Test case where CCS and main buffer overlaps
On 31 August 2017 at 06:58, Gabriel Krisman Bertazi
wrote:
> @@ -321,7 +322,13 @@ static void generate_fb(data_t *data, struct igt_fb *fb,
> int ccs_height = ALIGN(height, 16) / 16;
> f.pitches[1] = ALIGN(ccs_width * 1, 128);
> f.modifier[1] = modifier;
> - f.offsets[1] = size[0];
> +
> + if (data->flags & TEST_BAD_CCS_OFFSET) {
> + /* Overlap CCS buffer with the color buffer. */
> + f.offsets[1] = 0;
How about size[0] - 1024? That will give us an aligned-to-128 value
which looks plausible, and should overlap actual colour data.
Cheers,
Daniel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 6/7] tests/kms_ccs: Test case where CCS is on a different BO
Hi Gabriel,
On 31 August 2017 at 06:58, Gabriel Krisman Bertazi
wrote:
> + if (data->flags & TEST_BAD_CCS_HANDLE) {
> + /* Put the CCS buffer on a different BO. */
> + f.handles[0] = gem_create(data->drm_fd, size[0]);
> + ccs_handle = gem_create(data->drm_fd, size[1]);
> + f.offsets[1] = 0;
This could be caught by offsets[1] < size[0] checking. Could you
please create both BOs with (size[0] + size[1]), and set (offset[1] ==
size[0])? In other words, the only thing differing at all is the BO.
Cheers,
Daniel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t v3 7/7] tests/kms_ccs: Test case for wrong aux buffer stripe size
Hi Gabriel, On 31 August 2017 at 07:18, Gabriel Krisman Bertazi wrote: > Two scenarios tested: > - unaligned stripe > - Stripe too small 'stride' in the commit message please. ;) But it is fine everywhere through the code. > @@ -323,7 +326,14 @@ static void generate_fb(data_t *data, struct igt_fb *fb, > */ > int ccs_width = ALIGN(width * 4, 32) / 32; > int ccs_height = ALIGN(height, 16) / 16; > - f.pitches[1] = ALIGN(ccs_width * 1, 128); > + int aux_stride = ALIGN(ccs_width * 1, 128); > + > + if (fb_flags & FB_MISALIGN_AUX_STRIDE) > + aux_stride = ccs_width; aux_stride -= 64 perhaps? > + else if (fb_flags & FB_SMALL_AUX_STRIDE) > + aux_stride = ALIGN(ccs_width/2, 128); This one seems OK, but maybe want to skip it in the unlikely case that (w <= 1024), since that already has the smallest possible valid stride at 128. Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Fail addfb ioctl if color and CCS buffers overlap
Hi Ville, On 4 September 2017 at 17:37, Ville Syrjälä wrote: > On Thu, Aug 31, 2017 at 04:52:15PM -0300, Gabriel Krisman Bertazi wrote: >> With this patch the new testcase igt@kms_ccs@pipe-X-invalid-ccs-offset >> succeeds. > > I don't think we actually want to reject overlap. I had a patch for that > years ago, but I decided to drop it because people might want to > interleave the planes in some interesting ways. Making the overlap > check accurate enough to allow that would be to total overkill. So IMO > it's perfectly fine to let the user shoot himself in the foot if they > mess up the offsets. Is that actually supported by any hardware renderer? If not, maybe the check should only be enabled for generations who support it. Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 00/12] drm/i915: Fix up the CCS code
Hi Daniel, On 25 August 2017 at 18:17, Daniel Vetter wrote: > Which of these do we need to cherry-pick over to -next-fixes? There's no > annotations about that. If the answer is "most" I'm leaning towards > disabling CCS for 4.14, minimal set would be ideal (and first in the patch > series). My opinion below; tl;dr is that I don't think most of them are super-critical. Ville obviously has a far stronger opinion than me on the shape of the code, so I'm fine with this series, which seems to mostly be a merge back of the delta between whatever Ville's latest branch was, and whatever the last patchset Ben sent out was. >> Ville Syrjälä (12): >> drm/i915: Treat fb->offsets[] as a raw byte offset instead of a linear >> offset This should land into -fixes. I trust Ville that it has no UABI impact, but seems like something to be very consistent on. >> drm/i915: Skip fence alignemnt check for the CCS plane Not sure if this is -fixes material really, just a cleanup? >> drm/i915: Switch over to the LLC/eLLC hotspot avoidance hash mode for >> CCS Not -fixes, performance optimisation. >> drm/i915: Add a comment exlaining CCS hsub/vsub Seems harmless to land to -fixes. >> drm/i915: Nuke a pointless unreachable() Ditto. >> drm/i915: Add the missing Y/Yf modifiers for SKL+ sprites Per my previous reply, NAK to landing at all, since DDB/WM allocation seems too broken for it to work. >> drm/i915: Clean up the sprite modifier checks Fine with this, but doesn't seem like -fixes material. >> drm/i915: Add CCS capability for sprites NAK, same reason as Y/Yf. >> drm/i915: Allow up to 32KB stride on SKL+ "sprites" Again doesn't seem like -fixes necessarily? >> drm: Fix modifiers_property kernel doc Good for -fixes. >> drm: Check that the plane supports the request format+modifier combo Good for core (not Intel) -fixes. >> drm/i915: Remove the pipe/plane ID checks from >> skl_check_ccs_aux_surface() Seems fine but probably not -fixes material; land in Intel after a merge? Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 5/8] drm/i915/guc: Change default GuC FW for SKL to v9.33
Hi Sagar, On 28 August 2017 at 10:56, Sagar Arun Kamble wrote: > This patch makes v9.33 firmware as default firmware for SKL. > This update includes (since v6.1): https://01.org/linuxgraphics/downloads/firmware does not include v9.33, only 6.1. Please do not push this patch until it has at least made it out for public release, and preferably into a linux-firmware tree as well. Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/12] drm/i915: Add the missing Y/Yf modifiers for SKL+ sprites
Hi, On 25 August 2017 at 12:34, Ville Syrjälä wrote: > On Fri, Aug 25, 2017 at 10:40:28AM +0100, Daniel Stone wrote: >> On 24 August 2017 at 20:10, wrote: >> > Y/Yf somehow dropped out from the SKL+ sprite modifier list. Add them >> > in. >> >> There's no 'somehow': >> https://lists.freedesktop.org/archives/intel-gfx/2017-August/134932.html >> >> I would prefer to not see this pushed whilst it doesn't actually work. > > Works fine here. Well, I should say it works just as well as it does for > the primary plane. There are no plane specific checks in the wm/ddb code > IIRC so if something is broken for sprites then it's most likely equally > broken for the primary plane. How did you test it? The failure mode I observed was that the primary plane had a giant allocation, having previously had plain Y-tiling or Y-CCS enabled. After switching the primary to linear or X-tiled, trying to configure a 256x256 sprite plane with either Y-tiled or CCS failed, as it only had a DDB allocation of 31 blocks, where it needed 33. So it's not that there are plane-specific checks rejecting anything, it's that the allocations never got drawn up to give the sprite plane enough room to do anything other than X-tiled. I saw this on both SKL (3200x1800 or 2560x1440) and APL (1920x1080). Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/12] drm/i915: Add the missing Y/Yf modifiers for SKL+ sprites
On 24 August 2017 at 20:10, wrote: > Y/Yf somehow dropped out from the SKL+ sprite modifier list. Add them > in. There's no 'somehow': https://lists.freedesktop.org/archives/intel-gfx/2017-August/134932.html I would prefer to not see this pushed whilst it doesn't actually work. Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/doc: Document ioctl errno value patterns
Hi, On 18 August 2017 at 10:21, Daniel Vetter wrote: > +Recommended IOCTL Return Values > +=== > + > +In theory a driver's IOCTL callback is only allowed to return very few error > +codes. In practice it's good to abuse a few more. This section documents > common > +practice within the DRM subsystem: > + > +ENOENT: > +Strictly speaking only when you try to open isn't there. There's a word from this sentence. > +We reuse that > +to signal any kind of object lookup failure, e.g. for unknown GEM > buffer > +object handles, unknown KMS object handles and similar cases. > + > +ENOSPC: > +Some drivers use this to differiante 'differentiate' Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t] syncobj: Add some wait and reset tests
Hi Jason,
On 9 August 2017 at 18:04, Jason Ekstrand wrote:
> +/* One one tenth of a second */
> +#define SHORT_TIME_NSEC 1ull
Er, a hundredth? Or only one, one tenth?
> +static void
> +test_wait_illegal_handle(int fd)
> +{
> + struct drm_syncobj_wait wait = { 0 };
> + uint32_t handle = 2;
Use 0.
> +static void
> +test_wait_for_submit_unsignaled(int fd)
> +{
> + uint32_t syncobj = syncobj_create(fd);
> + struct drm_syncobj_wait wait = { 0 };
> + int ret;
> +
> + wait.handles = to_user_pointer(&syncobj);
> + wait.count_handles = 1;
> + wait.flags = DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT;
> + wait.timeout_nsec = short_timeout();
> +
> + ret = ioctl(fd, DRM_IOCTL_SYNCOBJ_WAIT, &wait);
> + igt_assert(ret == -1 && errno == ETIME);
There's do_ioctl_err() for this pattern BTW, and I think that takes
care of EINTR as well.
> +static void
> +test_wait_signaled(int fd)
> +{
> + uint32_t syncobj = syncobj_create(fd);
> + struct drm_syncobj_wait wait = { 0 };
> + int ret;
> +
> + wait.handles = to_user_pointer(&syncobj);
> + wait.count_handles = 1;
> +
> + trigger_syncobj(fd, &syncobj, 1, false);
> +
> + wait.timeout_nsec = 0;
> + ret = ioctl(fd, DRM_IOCTL_SYNCOBJ_WAIT, &wait);
> + igt_warn_on(ret != -1 || errno != ETIME);
> +
> + wait.timeout_nsec = short_timeout();
> + ret = ioctl(fd, DRM_IOCTL_SYNCOBJ_WAIT, &wait);
> + igt_assert(ret == 0);
... and do_ioctl() for this pattern.
> +static bool
> +has_syncobj_wait(int fd)
> +{
> + struct drm_syncobj_wait wait = { 0 };
This probably needs a local_ definition.
> + uint64_t value;
> + int ret;
> +
> + if (drmGetCap(fd, DRM_CAP_SYNCOBJ, &value))
> + return false;
> + if (!value)
> + return false;
> +
> + /* Try waiting for zero sync objects should fail with EINVAL */
> + ret = ioctl(fd, DRM_IOCTL_SYNCOBJ_WAIT, &wait);
> + return ret == -1 && errno == EINVAL;
Unfortunately an unrecognised ioctl also leads to a failure with
EINVAL. Try another test for ioctl presence, e.g. do you get ENOENT if
you pass one handle to wait for, but that handle is 0 (invalid GEM
object ID)?
I couldn't see much else obvious, and it seems like a decent enough
workout of the wait API, so, with these and what Chris suggested:
Acked-by: Daniel Stone
Cheers,
Daniel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 6/6] [v4] drm/i915: Add support for CCS modifiers
Hi,
On 3 August 2017 at 12:00, Daniel Stone wrote:
> On 1 August 2017 at 17:58, Ben Widawsky wrote:
>> @@ -1240,6 +1253,19 @@ intel_sprite_plane_create(struct drm_i915_private
>> *dev_priv,
>> plane_formats = skl_plane_formats;
>> num_plane_formats = ARRAY_SIZE(skl_plane_formats);
>> modifiers = skl_plane_format_modifiers;
>> + } else if (INTEL_GEN(dev_priv) >= 9) {
>> + intel_plane->can_scale = true;
>> + state->scaler_id = -1;
>> +
>> + intel_plane->update_plane = skl_update_plane;
>> + intel_plane->disable_plane = skl_disable_plane;
>> +
>> + plane_formats = skl_plane_formats;
>> + num_plane_formats = ARRAY_SIZE(skl_plane_formats);
>> + if (pipe >= PIPE_C)
>
>
> if (pipe >= PIPE_C || plane >= PLANE_SPRITE1)
>
> cf. skl_check_ccs_aux_surface() which rejects CCS on anything other
> than PRIMARY/SPRITE0.
Turns out that should be 1 rather than PLANE_SPRITE1.
Anyway, I've pulled out CCS modifiers for all sprite planes in this
series. Whilst actually testing it, I discovered DDB allocations were
hopelessly broken.
Starting with a 1920x1080 primary plane which had (at some point) had
a CCS surface on it, and moving to a 1920x1080 _linear_ primary plane
with a 256x256 CCS sprite plane, I ended up with a DDB split of 443
primary / 32 plane. Y-tiling needs 33 blocks for even 256x256, so it
didn't work.
Given that, I've removed advertisement of Y, Yf, Y_CCS and Y_CCS, in
order to not give userspace false hope. Once DDB allocation is fixed,
we can start advertising these modifiers.
Cheers,
Daniel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t 1/8] tests/kms_ccs: Convert int/bool to enum
Rather than using TEST_UNCOMPRESSED / TEST_COMPRESSED as alternately
either an int or a bool, change it to a bitfield enum.
This will let us add more parameters later to control framebuffer
generation.
Signed-off-by: Daniel Stone
---
tests/kms_ccs.c | 29 -
1 file changed, 16 insertions(+), 13 deletions(-)
diff --git a/tests/kms_ccs.c b/tests/kms_ccs.c
index ef952f2b..14d23ac1 100644
--- a/tests/kms_ccs.c
+++ b/tests/kms_ccs.c
@@ -35,6 +35,10 @@ enum test_flags {
TEST_BAD_ROTATION_90= 1 << 4,
};
+enum test_fb_flags {
+ FB_COMPRESSED = 1 << 0,
+};
+
typedef struct {
int drm_fd;
igt_display_t display;
@@ -53,7 +57,7 @@ typedef struct {
#define RED0x00ff
-static void render_fb(data_t *data, bool compressed,
+static void render_fb(data_t *data, enum test_fb_flags fb_flags,
int height, unsigned int stride)
{
struct igt_fb *fb = &data->fb;
@@ -67,7 +71,7 @@ static void render_fb(data_t *data, bool compressed,
0, fb->size,
PROT_READ | PROT_WRITE);
- if (compressed) {
+ if (fb_flags & FB_COMPRESSED) {
/* In the compressed case, we want the top half of the
* surface to be uncompressed and the bottom half to be
* compressed.
@@ -128,7 +132,7 @@ static void render_ccs(data_t *data, uint32_t gem_handle,
munmap(ptr, size);
}
-static void display_fb(data_t *data, int compressed)
+static void display_fb(data_t *data, enum test_fb_flags fb_flags)
{
struct local_drm_mode_fb_cmd2 f = {};
struct igt_fb *fb = &data->fb;
@@ -146,7 +150,7 @@ static void display_fb(data_t *data, int compressed)
mode = igt_output_get_mode(data->output);
- if (compressed)
+ if (fb_flags & FB_COMPRESSED)
modifier = LOCAL_I915_FORMAT_MOD_Y_TILED_CCS;
else
modifier = LOCAL_I915_FORMAT_MOD_Y_TILED;
@@ -167,7 +171,7 @@ static void display_fb(data_t *data, int compressed)
f.offsets[0] = 0;
size[0] = f.pitches[0] * ALIGN(height, 32);
- if (compressed) {
+ if (fb_flags & FB_COMPRESSED) {
/* From the Sky Lake PRM, Vol 12, "Color Control Surface":
*
*"The compression state of the cache-line pair is
@@ -215,9 +219,9 @@ static void display_fb(data_t *data, int compressed)
fb->cairo_surface = NULL;
fb->domain = 0;
- render_fb(data, compressed, f.height, f.pitches[0]);
+ render_fb(data, fb_flags, f.height, f.pitches[0]);
- if (compressed)
+ if (fb_flags & FB_COMPRESSED)
render_ccs(data, f.handles[0], f.offsets[1], size[1],
f.height, f.pitches[1]);
@@ -238,25 +242,24 @@ static void display_fb(data_t *data, int compressed)
igt_debug_wait_for_keypress("ccs");
}
-#define TEST_UNCOMPRESSED 0
-#define TEST_COMPRESSED 1
-
static void test_output(data_t *data)
{
igt_display_t *display = &data->display;
igt_plane_t *primary;
igt_crc_t crc, ref_crc;
igt_pipe_crc_t *pipe_crc;
+ enum test_fb_flags fb_flags = 0;
igt_output_set_pipe(data->output, data->pipe);
if (data->flags & TEST_CRC) {
+
pipe_crc = igt_pipe_crc_new(data->drm_fd, data->pipe,
INTEL_PIPE_CRC_SOURCE_AUTO);
- display_fb(data, TEST_COMPRESSED);
+ display_fb(data, fb_flags | FB_COMPRESSED);
igt_pipe_crc_collect_crc(pipe_crc, &ref_crc);
- display_fb(data, TEST_UNCOMPRESSED);
+ display_fb(data, fb_flags);
igt_pipe_crc_collect_crc(pipe_crc, &crc);
igt_assert_crc_equal(&crc, &ref_crc);
@@ -267,7 +270,7 @@ static void test_output(data_t *data)
if (data->flags & TEST_BAD_PIXEL_FORMAT ||
data->flags & TEST_BAD_ROTATION_90) {
- display_fb(data, TEST_COMPRESSED);
+ display_fb(data, fb_flags | FB_COMPRESSED);
}
primary = igt_output_get_plane_type(data->output,
DRM_PLANE_TYPE_PRIMARY);
--
2.13.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t 4/8] tests/kms_ccs: Paramaterize color for framebuffer
Will be used in later patches.
Signed-off-by: Daniel Stone
---
tests/kms_ccs.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/tests/kms_ccs.c b/tests/kms_ccs.c
index c02a0433..50bb2ad6 100644
--- a/tests/kms_ccs.c
+++ b/tests/kms_ccs.c
@@ -48,14 +48,12 @@ typedef struct {
enum test_flags flags;
} data_t;
+#define RED0x00ff
#define COMPRESSED_RED 0x0fff
-#define COMPRESSED_GREEN 0x000ff00f
-#define COMPRESSED_BLUE0x0fff
#define CCS_UNCOMPRESSED 0x0
#define CCS_COMPRESSED 0x55
-#define RED0x00ff
static void render_fb(data_t *data, uint32_t gem_handle, unsigned int size,
enum test_fb_flags fb_flags,
@@ -63,6 +61,8 @@ static void render_fb(data_t *data, uint32_t gem_handle,
unsigned int size,
{
uint32_t *ptr;
unsigned int half_height, half_size;
+ uint32_t uncompressed_color = RED;
+ uint32_t compressed_color = COMPRESSED_RED;
int i;
ptr = gem_mmap__cpu(data->drm_fd, gem_handle, 0, size,
@@ -83,13 +83,13 @@ static void render_fb(data_t *data, uint32_t gem_handle,
unsigned int size,
half_size = half_height * stride;
for (i = 0; i < size / 4; i++) {
if (i < half_size / 4)
- ptr[i] = RED;
+ ptr[i] = uncompressed_color;
else
- ptr[i] = COMPRESSED_RED;
+ ptr[i] = compressed_color;
}
} else {
for (i = 0; i < size / 4; i++)
- ptr[i] = RED;
+ ptr[i] = uncompressed_color;
}
munmap(ptr, size);
--
2.13.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t 2/8] tests/kms_ccs: Split FB generation into helper
Create a new helper for generating and rendering the framebuffer, rather
than doing it inline with applying the configuration. This will be used
later to generate a different plane configuration.
Signed-off-by: Daniel Stone
---
tests/kms_ccs.c | 73 ++---
1 file changed, 39 insertions(+), 34 deletions(-)
diff --git a/tests/kms_ccs.c b/tests/kms_ccs.c
index 14d23ac1..cdc56f79 100644
--- a/tests/kms_ccs.c
+++ b/tests/kms_ccs.c
@@ -57,18 +57,15 @@ typedef struct {
#define RED0x00ff
-static void render_fb(data_t *data, enum test_fb_flags fb_flags,
+static void render_fb(data_t *data, uint32_t gem_handle, unsigned int size,
+ enum test_fb_flags fb_flags,
int height, unsigned int stride)
{
- struct igt_fb *fb = &data->fb;
uint32_t *ptr;
unsigned int half_height, half_size;
int i;
- igt_assert(fb->fb_id);
-
- ptr = gem_mmap__cpu(data->drm_fd, fb->gem_handle,
- 0, fb->size,
+ ptr = gem_mmap__cpu(data->drm_fd, gem_handle, 0, size,
PROT_READ | PROT_WRITE);
if (fb_flags & FB_COMPRESSED) {
@@ -84,18 +81,18 @@ static void render_fb(data_t *data, enum test_fb_flags
fb_flags,
*/
half_height = ALIGN(height, 128) / 2;
half_size = half_height * stride;
- for (i = 0; i < fb->size / 4; i++) {
+ for (i = 0; i < size / 4; i++) {
if (i < half_size / 4)
ptr[i] = RED;
else
ptr[i] = COMPRESSED_RED;
}
} else {
- for (i = 0; i < fb->size / 4; i++)
+ for (i = 0; i < size / 4; i++)
ptr[i] = RED;
}
- munmap(ptr, fb->size);
+ munmap(ptr, size);
}
static unsigned int
@@ -118,8 +115,7 @@ static void render_ccs(data_t *data, uint32_t gem_handle,
half_height = ALIGN(height, 128) / 2;
ccs_half_height = half_height / 16;
- ptr = gem_mmap__cpu(data->drm_fd, gem_handle,
- offset, size,
+ ptr = gem_mmap__cpu(data->drm_fd, gem_handle, offset, size,
PROT_READ | PROT_WRITE);
for (i = 0; i < size; i++) {
@@ -132,32 +128,23 @@ static void render_ccs(data_t *data, uint32_t gem_handle,
munmap(ptr, size);
}
-static void display_fb(data_t *data, enum test_fb_flags fb_flags)
+static void generate_fb(data_t *data, struct igt_fb *fb,
+ int width, int height,
+ enum test_fb_flags fb_flags)
{
struct local_drm_mode_fb_cmd2 f = {};
- struct igt_fb *fb = &data->fb;
- igt_display_t *display = &data->display;
- igt_plane_t *primary;
- drmModeModeInfo *mode;
- unsigned int width, height;
unsigned int size[2];
uint64_t modifier;
- enum igt_commit_style commit = COMMIT_UNIVERSAL;
int ret;
- if (data->display.is_atomic)
- commit = COMMIT_ATOMIC;
-
- mode = igt_output_get_mode(data->output);
-
if (fb_flags & FB_COMPRESSED)
modifier = LOCAL_I915_FORMAT_MOD_Y_TILED_CCS;
else
modifier = LOCAL_I915_FORMAT_MOD_Y_TILED;
f.flags = LOCAL_DRM_MODE_FB_MODIFIERS;
- f.width = ALIGN(mode->hdisplay, 16);
- f.height = ALIGN(mode->vdisplay, 8);
+ f.width = ALIGN(width, 16);
+ f.height = ALIGN(height, 8);
if (data->flags & TEST_BAD_PIXEL_FORMAT)
f.pixel_format = DRM_FORMAT_RGB565;
@@ -195,9 +182,13 @@ static void display_fb(data_t *data, enum test_fb_flags
fb_flags)
f.handles[0] = gem_create(data->drm_fd, size[0] + size[1]);
f.handles[1] = f.handles[0];
+ render_ccs(data, f.handles[1], f.offsets[1], size[1],
+ height, f.pitches[1]);
} else
f.handles[0] = gem_create(data->drm_fd, size[0]);
+ render_fb(data, f.handles[0], size[0], fb_flags, height, f.pitches[0]);
+
ret = drmIoctl(data->drm_fd, LOCAL_DRM_IOCTL_MODE_ADDFB2, &f);
if (data->flags & TEST_BAD_PIXEL_FORMAT) {
igt_assert_eq(ret, -1);
@@ -218,15 +209,30 @@ static void display_fb(data_t *data, enum test_fb_flags
fb_flags)
fb->size = size[0];
fb->cairo_surface = NULL;
fb->domain = 0;
+}
+
+static void try_config(data_t *data, enum test_fb_flags fb_flags)
+{
+ igt_display_t *display = &data->display;
+ igt_plane_t *primary;
+ drmModeModeInfo *drm_mode = igt_output_get_mode(data->output);
+ enum igt_commit_style commit;
+ int ret;
[Intel-gfx] [PATCH i-g-t 5/8] tests/kms_ccs: Reshuffle test name and loop
In preparation for also testing sprites.
Signed-off-by: Daniel Stone
---
tests/kms_ccs.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/tests/kms_ccs.c b/tests/kms_ccs.c
index 50bb2ad6..1a5cdcd4 100644
--- a/tests/kms_ccs.c
+++ b/tests/kms_ccs.c
@@ -342,13 +342,13 @@ igt_main
for (data.pipe = PIPE_A; data.pipe < IGT_MAX_PIPES; data.pipe++) {
data.flags = TEST_CRC;
- igt_subtest_f("pipe-%s-crc-basic", kmstest_pipe_name(data.pipe))
+ igt_subtest_f("pipe-%s-crc-primary-basic",
+ kmstest_pipe_name(data.pipe))
test(&data);
- }
- for (data.pipe = PIPE_A; data.pipe < IGT_MAX_PIPES; data.pipe++) {
- data.flags = TEST_CRC | TEST_ROTATE_180;
- igt_subtest_f("pipe-%s-crc-rotation-180",
kmstest_pipe_name(data.pipe))
+ data.flags |= TEST_ROTATE_180;
+ igt_subtest_f("pipe-%s-crc-primary-rotation-180",
+ kmstest_pipe_name(data.pipe))
test(&data);
}
--
2.13.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t 6/8] tests/kms_ccs: Test for supported modifier
Make sure the CCS modifier is supported on our plane, before we try to
use it on that plane.
Signed-off-by: Daniel Stone
---
tests/kms_ccs.c | 117 +++-
1 file changed, 116 insertions(+), 1 deletion(-)
diff --git a/tests/kms_ccs.c b/tests/kms_ccs.c
index 1a5cdcd4..e74a68af 100644
--- a/tests/kms_ccs.c
+++ b/tests/kms_ccs.c
@@ -54,6 +54,118 @@ typedef struct {
#define CCS_UNCOMPRESSED 0x0
#define CCS_COMPRESSED 0x55
+struct local_drm_format_modifier {
+ /* Bitmask of formats in get_plane format list this info applies to. The
+ * offset allows a sliding window of which 64 formats (bits).
+ *
+ * Some examples:
+ * In today's world with < 65 formats, and formats 0, and 2 are
+ * supported
+ * 0x0005
+ * ^-offset = 0, formats = 5
+ *
+ * If the number formats grew to 128, and formats 98-102 are
+ * supported with the modifier:
+ *
+ * 0x003c
+ * ^
+ * |__offset = 64, formats = 0x3c
+ *
+ */
+ uint64_t formats;
+ uint32_t offset;
+ uint32_t pad;
+
+ /* This modifier can be used with the format for this plane. */
+ uint64_t modifier;
+};
+
+struct local_drm_format_modifier_blob {
+#define LOCAL_FORMAT_BLOB_CURRENT 1
+ /* Version of this blob format */
+ uint32_t version;
+
+ /* Flags */
+ uint32_t flags;
+
+ /* Number of fourcc formats supported */
+ uint32_t count_formats;
+
+ /* Where in this blob the formats exist (in bytes) */
+ uint32_t formats_offset;
+
+ /* Number of drm_format_modifiers */
+ uint32_t count_modifiers;
+
+ /* Where in this blob the modifiers exist (in bytes) */
+ uint32_t modifiers_offset;
+
+ /* u32 formats[] */
+ /* struct drm_format_modifier modifiers[] */
+};
+
+static inline uint32_t *
+formats_ptr(struct local_drm_format_modifier_blob *blob)
+{
+ return (uint32_t *)(((char *)blob) + blob->formats_offset);
+}
+
+static inline struct local_drm_format_modifier *
+modifiers_ptr(struct local_drm_format_modifier_blob *blob)
+{
+ return (struct local_drm_format_modifier *)(((char *)blob) +
blob->modifiers_offset);
+}
+
+static void plane_require_ccs(data_t *data, igt_plane_t *plane, uint32_t
format)
+{
+ drmModePropertyBlobPtr blob;
+ struct local_drm_format_modifier_blob *blob_data;
+ struct local_drm_format_modifier *modifiers;
+ uint32_t *formats;
+ uint64_t blob_id;
+ bool ret;
+ int fmt_idx = -1;
+
+ ret = kmstest_get_property(data->drm_fd, plane->drm_plane->plane_id,
+ DRM_MODE_OBJECT_PLANE, "IN_FORMATS",
+ NULL, &blob_id, NULL);
+ igt_skip_on_f(ret == false, "IN_FORMATS not supported by kernel\n");
+ igt_skip_on_f(blob_id == 0, "IN_FORMATS not supported by plane\n");
+ blob = drmModeGetPropertyBlob(data->drm_fd, blob_id);
+ igt_assert(blob);
+ igt_assert_lte(sizeof(struct local_drm_format_modifier_blob),
+ blob->length);
+
+ blob_data = (struct local_drm_format_modifier_blob *) blob->data;
+ formats = formats_ptr(blob_data);
+ for (int i = 0; i < blob_data->count_formats; i++) {
+ if (formats[i] == format) {
+ fmt_idx = i;
+ break;
+ }
+ }
+
+ igt_skip_on_f(fmt_idx == -1,
+ "Format 0x%x not supported by plane\n", format);
+
+ modifiers = modifiers_ptr(blob_data);
+ for (int i = 0; i < blob_data->count_modifiers; i++) {
+ if (modifiers[i].modifier != LOCAL_I915_FORMAT_MOD_Y_TILED_CCS)
+ continue;
+
+ if (modifiers[i].offset > fmt_idx ||
+ fmt_idx > modifiers[i].offset + 63)
+ continue;
+
+ if (modifiers[i].formats &
+ (1UL << (fmt_idx - modifiers[i].offset)))
+ return;
+
+ igt_skip("i915 CCS modifier not supported for format\n");
+ }
+
+ igt_skip("i915 CCS modifier not supported by kernel for plane\n");
+}
static void render_fb(data_t *data, uint32_t gem_handle, unsigned int size,
enum test_fb_flags fb_flags,
@@ -222,12 +334,15 @@ static void try_config(data_t *data, enum test_fb_flags
fb_flags)
else
commit = COMMIT_UNIVERSAL;
+ primary = igt_output_get_plane_type(data->output,
+ DRM_PLANE_TYPE_PRIMARY);
+ plane_require_ccs(data, primary, DRM_FORMAT_XRGB);
+
generate_fb(data, &a
[Intel-gfx] [PATCH i-g-t 7/8] tests/kms_ccs: Split all tests into subtests
Some subtests were magically doing a for-each-pipe loop. Remove that,
and have all multi-pipe tests actually work across all pipes.
Signed-off-by: Daniel Stone
---
tests/kms_ccs.c | 62 +++--
1 file changed, 20 insertions(+), 42 deletions(-)
diff --git a/tests/kms_ccs.c b/tests/kms_ccs.c
index e74a68af..79856f97 100644
--- a/tests/kms_ccs.c
+++ b/tests/kms_ccs.c
@@ -369,6 +369,13 @@ static void test_output(data_t *data)
igt_pipe_crc_t *pipe_crc;
enum test_fb_flags fb_flags = 0;
+ igt_display_require_output_on_pipe(display, data->pipe);
+
+ /* Sets data->output with a valid output. */
+ for_each_valid_output_on_pipe(display, data->pipe, data->output) {
+ break;
+ }
+
igt_output_set_pipe(data->output, data->pipe);
if (data->flags & TEST_CRC) {
@@ -404,31 +411,6 @@ static void test_output(data_t *data)
igt_remove_fb(data->drm_fd, &data->fb);
}
-static void test(data_t *data)
-{
- igt_display_t *display = &data->display;
- int valid_tests = 0;
- enum pipe wanted_pipe = data->pipe;
-
- igt_skip_on(wanted_pipe >= display->n_pipes);
-
- for_each_pipe_with_valid_output(display, data->pipe, data->output) {
- if (wanted_pipe != PIPE_NONE && data->pipe != wanted_pipe)
- continue;
-
- test_output(data);
-
- valid_tests++;
-
- igt_info("\n%s on pipe %s, connector %s: PASSED\n\n",
-igt_subtest_name(),
-kmstest_pipe_name(data->pipe),
-igt_output_name(data->output));
- }
-
- igt_require_f(valid_tests, "no valid crtc/connector combinations
found\n");
-}
-
static data_t data;
igt_main
@@ -443,28 +425,24 @@ igt_main
igt_display_init(&data.display, data.drm_fd);
}
- igt_subtest_f("bad-pixel-format") {
+ for_each_pipe(&data.display, data.pipe) {
+ const char *pipe_name = kmstest_pipe_name(data.pipe);
+
data.flags = TEST_BAD_PIXEL_FORMAT;
- data.pipe = PIPE_NONE;
- test(&data);
- }
+ igt_subtest_f("pipe-%s-bad-pixel-format", pipe_name)
+ test_output(&data);
- igt_subtest_f("bad-rotation-90") {
data.flags = TEST_BAD_ROTATION_90;
- data.pipe = PIPE_NONE;
- test(&data);
- }
+ igt_subtest_f("pipe-%s-bad-rotation-90", pipe_name)
+ test_output(&data);
- for (data.pipe = PIPE_A; data.pipe < IGT_MAX_PIPES; data.pipe++) {
data.flags = TEST_CRC;
- igt_subtest_f("pipe-%s-crc-primary-basic",
- kmstest_pipe_name(data.pipe))
- test(&data);
-
- data.flags |= TEST_ROTATE_180;
- igt_subtest_f("pipe-%s-crc-primary-rotation-180",
- kmstest_pipe_name(data.pipe))
- test(&data);
+ igt_subtest_f("pipe-%s-crc-primary-basic", pipe_name)
+ test_output(&data);
+
+ data.flags = TEST_CRC | TEST_ROTATE_180;
+ igt_subtest_f("pipe-%s-crc-primary-rotation-180", pipe_name)
+ test_output(&data);
}
igt_fixture
--
2.13.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t 8/8] tests/kms_ccs: Test CCS on sprite planes
Also try to test CCS on available non-primary planes. However, as there
is not enough bandwidth to scan out both the primary and sprite planes
when using CCS (or even Y-tiled), fall back to linear for the primary
plane when using CCS for a sprite/cursor plane.
Signed-off-by: Daniel Stone
---
tests/kms_ccs.c | 74 +++--
1 file changed, 67 insertions(+), 7 deletions(-)
diff --git a/tests/kms_ccs.c b/tests/kms_ccs.c
index 79856f97..c544b36f 100644
--- a/tests/kms_ccs.c
+++ b/tests/kms_ccs.c
@@ -37,19 +37,24 @@ enum test_flags {
enum test_fb_flags {
FB_COMPRESSED = 1 << 0,
+ FB_HAS_PLANE= 1 << 1,
};
typedef struct {
int drm_fd;
igt_display_t display;
struct igt_fb fb;
+ struct igt_fb fb_sprite;
igt_output_t *output;
enum pipe pipe;
enum test_flags flags;
+ igt_plane_t *plane;
} data_t;
#define RED0x00ff
#define COMPRESSED_RED 0x0fff
+#define GREEN 0xff00
+#define COMPRESSED_GREEN 0x000ff00f
#define CCS_UNCOMPRESSED 0x0
#define CCS_COMPRESSED 0x55
@@ -173,8 +178,10 @@ static void render_fb(data_t *data, uint32_t gem_handle,
unsigned int size,
{
uint32_t *ptr;
unsigned int half_height, half_size;
- uint32_t uncompressed_color = RED;
- uint32_t compressed_color = COMPRESSED_RED;
+ uint32_t uncompressed_color = data->plane ? GREEN : RED;
+ uint32_t compressed_color =
+ data->plane ? COMPRESSED_GREEN : COMPRESSED_RED;
+ uint32_t bad_color = RED;
int i;
ptr = gem_mmap__cpu(data->drm_fd, gem_handle, 0, size,
@@ -200,8 +207,19 @@ static void render_fb(data_t *data, uint32_t gem_handle,
unsigned int size,
ptr[i] = compressed_color;
}
} else {
- for (i = 0; i < size / 4; i++)
- ptr[i] = uncompressed_color;
+ /* When we're displaying the primary plane underneath a
+* sprite plane, cut out a 128 x 128 area (less than the sprite)
+* plane size which we paint red, so we know easily if it's
+* bad.
+*/
+ for (i = 0; i < size / 4; i++) {
+ if ((fb_flags & FB_HAS_PLANE) &&
+i < (stride * 128) && (i % (stride / 4)) < 1) {
+ ptr[i] = bad_color;
+ } else {
+ ptr[i] = uncompressed_color;
+ }
+ }
}
munmap(ptr, size);
@@ -249,10 +267,17 @@ static void generate_fb(data_t *data, struct igt_fb *fb,
uint64_t modifier;
int ret;
+ /* Use either compressed or Y-tiled to test. However, given the lack of
+* available bandwidth, we use linear for the primary plane when
+* testing sprites, since we cannot fit two CCS planes into the
+* available FIFO configurations.
+*/
if (fb_flags & FB_COMPRESSED)
modifier = LOCAL_I915_FORMAT_MOD_Y_TILED_CCS;
- else
+ else if (!(fb_flags & FB_HAS_PLANE))
modifier = LOCAL_I915_FORMAT_MOD_Y_TILED;
+ else
+ modifier = 0;
f.flags = LOCAL_DRM_MODE_FB_MODIFIERS;
f.width = width;
@@ -338,8 +363,17 @@ static void try_config(data_t *data, enum test_fb_flags
fb_flags)
DRM_PLANE_TYPE_PRIMARY);
plane_require_ccs(data, primary, DRM_FORMAT_XRGB);
- generate_fb(data, &data->fb, drm_mode->hdisplay, drm_mode->vdisplay,
- fb_flags);
+ if (data->plane && fb_flags & FB_COMPRESSED) {
+ plane_require_ccs(data, data->plane, DRM_FORMAT_XRGB);
+ generate_fb(data, &data->fb, drm_mode->hdisplay,
+ drm_mode->vdisplay,
+ (fb_flags & ~FB_COMPRESSED) | FB_HAS_PLANE);
+ generate_fb(data, &data->fb_sprite, 256, 256, fb_flags);
+ } else {
+ generate_fb(data, &data->fb, drm_mode->hdisplay,
+ drm_mode->vdisplay, fb_flags);
+ }
+
if (data->flags & TEST_BAD_PIXEL_FORMAT)
return;
@@ -347,6 +381,12 @@ static void try_config(data_t *data, enum test_fb_flags
fb_flags)
igt_plane_set_size(primary, drm_mode->hdisplay, drm_mode->vdisplay);
igt_plane_set_fb(primary, &data->fb);
+ if (data->plane && fb_flags & FB_COMPRESSED) {
+ igt_plane_set_position(data->plane, 0, 0);
+ igt_plane_set_size(data->plane, 256, 256);
+
[Intel-gfx] [PATCH i-g-t 3/8] tests/kms_ccs: Remove excessive FB alignment
We don't need to align the framebuffer dimensions to the tile size. As long as the pitch is aligned to the tile width, and the BO dimensions can fit full tiles of both aligned pitch and aligned height, we don't need to claim the FB itself is larger. Signed-off-by: Daniel Stone --- tests/kms_ccs.c | 14 ++ 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/tests/kms_ccs.c b/tests/kms_ccs.c index cdc56f79..c02a0433 100644 --- a/tests/kms_ccs.c +++ b/tests/kms_ccs.c @@ -143,16 +143,14 @@ static void generate_fb(data_t *data, struct igt_fb *fb, modifier = LOCAL_I915_FORMAT_MOD_Y_TILED; f.flags = LOCAL_DRM_MODE_FB_MODIFIERS; - f.width = ALIGN(width, 16); - f.height = ALIGN(height, 8); + f.width = width; + f.height = height; if (data->flags & TEST_BAD_PIXEL_FORMAT) f.pixel_format = DRM_FORMAT_RGB565; else f.pixel_format = DRM_FORMAT_XRGB; - width = f.width; - height = f.height; f.pitches[0] = ALIGN(width * 4, 128); f.modifier[0] = modifier; f.offsets[0] = 0; @@ -173,12 +171,12 @@ static void generate_fb(data_t *data, struct igt_fb *fb, * 32x16. Since the main surface has a 32-bit format, we * need to multiply width by 4 to get bytes. */ - width = ALIGN(f.width * 4, 32) / 32; - height = ALIGN(f.height, 16) / 16; - f.pitches[1] = ALIGN(width * 1, 128); + int ccs_width = ALIGN(width * 4, 32) / 32; + int ccs_height = ALIGN(height, 16) / 16; + f.pitches[1] = ALIGN(ccs_width * 1, 128); f.modifier[1] = modifier; f.offsets[1] = size[0]; - size[1] = f.pitches[1] * ALIGN(height, 32); + size[1] = f.pitches[1] * ALIGN(ccs_height, 32); f.handles[0] = gem_create(data->drm_fd, size[0] + size[1]); f.handles[1] = f.handles[0]; -- 2.13.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH igt] tests/kms_ccs: Don't overallocate CCS surface
Due to a mix-up in kernel branches being used, I'd mangled Jason's original CCS test to hopelessly overallocate the CCS surface size. Restore it back to its original. Signed-off-by: Daniel Stone Cc: Jason Ekstrand --- tests/kms_ccs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/kms_ccs.c b/tests/kms_ccs.c index 0524a43e..a40d6c10 100644 --- a/tests/kms_ccs.c +++ b/tests/kms_ccs.c @@ -188,7 +188,7 @@ static void display_fb(data_t *data, int compressed) f.pitches[1] = ALIGN(width * 1, 128); f.modifier[1] = modifier; f.offsets[1] = size[0]; - size[1] = f.pitches[1] * ALIGN(f.height, 32); + size[1] = f.pitches[1] * ALIGN(height, 32); f.handles[0] = gem_create(data->drm_fd, size[0] + size[1]); f.handles[1] = f.handles[0]; -- 2.13.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] tests/kms_ccs: Fix the color/ccs surface generation
On 4 August 2017 at 15:56, Jason Ekstrand wrote: > On August 4, 2017 2:59:56 AM Daniel Stone wrote: >>> + width = ALIGN(f.width * 4, 32) / 32; >>> + height = ALIGN(f.height, 16) / 16; >>> + f.pitches[1] = ALIGN(width * 1, 128); >>> f.modifier[1] = modifier; >>> f.offsets[1] = size[0]; >>> - size[1] = f.pitches[1] * ALIGN(height, 64); >>> + size[1] = f.pitches[1] * ALIGN(height, 32); >> >> >> I changed this to f.height rather than height, because otherwise the >> kernel was rejecting the aux buffer for being too small. > > Congratulations, you found a bug in the kernel branch you're running. The > downsized height is definitely what we want and it works fine with my kernel > branch. Great. Which kernel are you running then? I'm running from here: https://git.collabora.com/cgit/user/daniels/linux.git/refs/heads Currently we have hsub/vsub defined as 16/8 (Vidya inverted this, but I never got a clear answer on why), tile_width as 128, and tile_height comes out as 32. Given the calculations in intel_fill_fb_info, I come out with the kernel demanding either 34816 bytes for CCS (using 16/8 hsub/vsub), or 20480 bytes (8/16) for a 1920x1080 framebuffer. Which kernel do you have, and how are you coming out with that calculation? Do we need to go back and re-figure out what pitch is? FWIW, ISL seems to get it right, according to the kernel. Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 i-g-t] tests/perf: fix build where system headers don't have Gen8 formats
On 4 August 2017 at 15:00, Lionel Landwerlin wrote: > v2: Use previous enum to define the new Gen8 enums (Petri) > > Signed-off-by: Lionel Landwerlin Tested-by: Daniel Stone ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH igt] tests/kms_properties: Don't set immutable properties
On 4 August 2017 at 14:38, Daniel Stone wrote: > If the kernel tells us it's immutable, trying to set it probably isn't > going to succeed. > > Fixes a failure seen with the IN_FORMATS property. Pushed a v2 which removed most of the list with Daniel Vetter's R-b from IRC. Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
