Re: Rework TTMs busy handling
On 2024-01-09 09:34, Christian König wrote: > Am 09.01.24 um 09:14 schrieb Thomas Hellström: >> On Tue, 2024-01-09 at 08:47 +0100, Christian König wrote: >>> >>> I'm trying to make this functionality a bit more useful for years now >>> since we multiple reports that behavior of drivers can be suboptimal >>> when multiple placements be given. >>> >>> So basically instead of hacking around the TTM behavior in the driver >>> once more I've gone ahead and changed the idle/busy placement list >>> into idle/busy placement flags. This not only saves a bunch of code, >>> but also allows setting some placements as fallback which are used if >>> allocating from the preferred ones didn't worked. >> >> I also have some doubts about the naming "idle" vs "busy", since an >> elaborate eviction mechanism would probably at some point want to check >> for gpu idle vs gpu busy, and this might create some confusion moving >> forward for people confusing busy as in memory overcommit with busy as >> in gpu activity. >> >> I can't immediately think of something better, though. > > Yeah, I was wondering about that as well. Especially since I wanted to add > some more flags in the future when for example a bandwidth quota how much > memory can be moved in/out is exceeded. > > Something like phase1, phase2, phase3 etc..., but that's also not very > descriptive either. Maybe something like "desired" vs "fallback"? -- Earthling Michel Dänzer| https://redhat.com Libre software enthusiast | Mesa and Xwayland developer
Re: [Intel-gfx] [PATCH v10 01/15] dma-buf/dma-fence: Add deadline awareness
On 3/16/23 23:22, Sebastian Wick wrote: > On Thu, Mar 16, 2023 at 5:29 PM Rob Clark wrote: >> On Thu, Mar 16, 2023 at 2:26 AM Jonas Ådahl wrote: >>> On Wed, Mar 15, 2023 at 09:19:49AM -0700, Rob Clark wrote: >>>> On Wed, Mar 15, 2023 at 6:53 AM Jonas Ådahl wrote: >>>>> On Fri, Mar 10, 2023 at 09:38:18AM -0800, Rob Clark wrote: >>>>>> On Fri, Mar 10, 2023 at 7:45 AM Jonas Ådahl wrote: >>>>>>> >>>>>>>> + * >>>>>>>> + * To this end, deadline hint(s) can be set on a _fence via >>>>>>>> _fence_set_deadline. >>>>>>>> + * The deadline hint provides a way for the waiting driver, or >>>>>>>> userspace, to >>>>>>>> + * convey an appropriate sense of urgency to the signaling driver. >>>>>>>> + * >>>>>>>> + * A deadline hint is given in absolute ktime (CLOCK_MONOTONIC for >>>>>>>> userspace >>>>>>>> + * facing APIs). The time could either be some point in the future >>>>>>>> (such as >>>>>>>> + * the vblank based deadline for page-flipping, or the start of a >>>>>>>> compositor's >>>>>>>> + * composition cycle), or the current time to indicate an immediate >>>>>>>> deadline >>>>>>>> + * hint (Ie. forward progress cannot be made until this fence is >>>>>>>> signaled). >>>>>>> >>>>>>> Is it guaranteed that a GPU driver will use the actual start of the >>>>>>> vblank as the effective deadline? I have some memories of seing >>>>>>> something about vblank evasion browsing driver code, which I might have >>>>>>> misunderstood, but I have yet to find whether this is something >>>>>>> userspace can actually expect to be something it can rely on. >>>>>> >>>>>> I guess you mean s/GPU driver/display driver/ ? It makes things more >>>>>> clear if we talk about them separately even if they happen to be the >>>>>> same device. >>>>> >>>>> Sure, sorry about being unclear about that. >>>>> >>>>>> >>>>>> Assuming that is what you mean, nothing strongly defines what the >>>>>> deadline is. In practice there is probably some buffering in the >>>>>> display controller. For ex, block based (including bandwidth >>>>>> compressed) formats, you need to buffer up a row of blocks to >>>>>> efficiently linearize for scanout. So you probably need to latch some >>>>>> time before you start sending pixel data to the display. But details >>>>>> like this are heavily implementation dependent. I think the most >>>>>> reasonable thing to target is start of vblank. >>>>> >>>>> The driver exposing those details would be quite useful for userspace >>>>> though, so that it can delay committing updates to late, but not too >>>>> late. Setting a deadline to be the vblank seems easy enough, but it >>>>> isn't enough for scheduling the actual commit. >>>> >>>> I'm not entirely sure how that would even work.. but OTOH I think you >>>> are talking about something on the order of 100us? But that is a bit >>>> of another topic. >>> >>> Yes, something like that. But yea, it's not really related. Scheduling >>> commits closer to the deadline has more complex behavior than that too, >>> e.g. the need for real time scheduling, and knowing how long it usually >>> takes to create and commit and for the kernel to process. > > Vblank can be really long, especially with VRR where the additional > time you get to finish the frame comes from making vblank longer. > Using the start of vblank as a deadline makes VRR useless. Not really. We normally still want to aim for start of vblank with VRR, which would result in the maximum refresh rate. Missing that target just incurs less of a penalty than with fixed refresh rate. -- Earthling Michel Dänzer| https://redhat.com Libre software enthusiast | Mesa and Xwayland developer
Re: [Intel-gfx] [PATCH] Change the meaning of the fields in the ttm_place structure from pfn to bytes
On 3/3/23 08:16, Somalapuram Amaranath wrote:
> Change the ttm_place structure member fpfn, lpfn, mem_type to
> res_start, res_end, res_type.
> Change the unsigned to u64.
> Fix the dependence in all the DRM drivers and
> clean up PAGE_SHIFT operation.
>
> Signed-off-by: Somalapuram Amaranath
>
> [...]
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
> index 44367f03316f..5b5104e724e3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
> @@ -131,11 +131,12 @@ static int amdgpu_gtt_mgr_new(struct
> ttm_resource_manager *man,
> goto err_free;
> }
>
> - if (place->lpfn) {
> + if (place->res_end) {
> spin_lock(>lock);
> r = drm_mm_insert_node_in_range(>mm, >mm_nodes[0],
> - num_pages, tbo->page_alignment,
> - 0, place->fpfn, place->lpfn,
> + num_pages, tbo->page_alignment,
> 0,
> + place->res_start << PAGE_SHIFT,
> + place->res_end << PAGE_SHIFT,
> DRM_MM_INSERT_BEST);
This should be >> or no shift instead of <<, shouldn't it? Multiplying a value
in bytes by the page size doesn't make sense.
I didn't check the rest of the patch in detail, but it's easy introduce subtle
regressions with this kind of change. It'll require a lot of review & testing
scrutiny.
--
Earthling Michel Dänzer| https://redhat.com
Libre software enthusiast | Mesa and Xwayland developer
Re: [Intel-gfx] [PATCH v5 12/13] drm/i915/ttm: use cached system pages when evicting lmem
On 2021-09-30 14:27, Matthew Auld wrote:
> On 30/09/2021 11:04, Michel Dänzer wrote:
>> On 2021-09-29 13:54, Thomas Hellström wrote:
>>> On Mon, 2021-09-27 at 12:41 +0100, Matthew Auld wrote:
>>>> This should let us do an accelerated copy directly to the shmem pages
>>>> when temporarily moving lmem-only objects, where the i915-gem
>>>> shrinker
>>>> can later kick in to swap out the pages, if needed.
>>>>
>>>> Signed-off-by: Matthew Auld
>>>> Cc: Thomas Hellström
>>>> ---
>>>> drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 8
>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>>>> b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>>>> index 194e5f1deda8..46d57541c0b2 100644
>>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>>>> @@ -134,11 +134,11 @@ static enum ttm_caching
>>>> i915_ttm_select_tt_caching(const struct drm_i915_gem_object *obj)
>>>> {
>>>> /*
>>>> - * Objects only allowed in system get cached cpu-mappings.
>>>> - * Other objects get WC mapping for now. Even if in system.
>>>> + * Objects only allowed in system get cached cpu-mappings, or
>>>> when
>>>> + * evicting lmem-only buffers to system for swapping. Other
>>>> objects get
>>>> + * WC mapping for now. Even if in system.
>>>> */
>>>> - if (obj->mm.region->type == INTEL_MEMORY_SYSTEM &&
>>>> - obj->mm.n_placements <= 1)
>>>> + if (obj->mm.n_placements <= 1)
>>>> return ttm_cached;
>>>> return ttm_write_combined;
>>>
>>> We should be aware that with TTM, even evicted bos can be mapped by
>>> user-space while evicted, and this will appear to user-space like the
>>> WC-mapped object suddenly became WB-mapped. But it appears like mesa
>>> doesn't care about this as long as the mappings are fully coherent.
>>
>> FWIW, the Mesa radeonsi driver avoids surprises due to this (e.g. some path
>> which involves CPU access suddenly goes faster if the BO was evicted from
>> VRAM) by asking for WC mapping of BOs intended to be in VRAM even while
>> they're evicted (via the AMDGPU_GEM_CREATE_CPU_GTT_USWC flag).
>>
>
> Ok, so amdgpu just defaults to cached system memory, even for evicted VRAM,
> unless userspace requests USWC, in which case it will use WC?
Right.
--
Earthling Michel Dänzer| https://redhat.com
Libre software enthusiast | Mesa and Xwayland developer
Re: [Intel-gfx] [PATCH v5 12/13] drm/i915/ttm: use cached system pages when evicting lmem
On 2021-09-29 13:54, Thomas Hellström wrote:
> On Mon, 2021-09-27 at 12:41 +0100, Matthew Auld wrote:
>> This should let us do an accelerated copy directly to the shmem pages
>> when temporarily moving lmem-only objects, where the i915-gem
>> shrinker
>> can later kick in to swap out the pages, if needed.
>>
>> Signed-off-by: Matthew Auld
>> Cc: Thomas Hellström
>> ---
>> drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 8
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> index 194e5f1deda8..46d57541c0b2 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> @@ -134,11 +134,11 @@ static enum ttm_caching
>> i915_ttm_select_tt_caching(const struct drm_i915_gem_object *obj)
>> {
>> /*
>> - * Objects only allowed in system get cached cpu-mappings.
>> - * Other objects get WC mapping for now. Even if in system.
>> + * Objects only allowed in system get cached cpu-mappings, or
>> when
>> + * evicting lmem-only buffers to system for swapping. Other
>> objects get
>> + * WC mapping for now. Even if in system.
>> */
>> - if (obj->mm.region->type == INTEL_MEMORY_SYSTEM &&
>> - obj->mm.n_placements <= 1)
>> + if (obj->mm.n_placements <= 1)
>> return ttm_cached;
>>
>> return ttm_write_combined;
>
> We should be aware that with TTM, even evicted bos can be mapped by
> user-space while evicted, and this will appear to user-space like the
> WC-mapped object suddenly became WB-mapped. But it appears like mesa
> doesn't care about this as long as the mappings are fully coherent.
FWIW, the Mesa radeonsi driver avoids surprises due to this (e.g. some path
which involves CPU access suddenly goes faster if the BO was evicted from VRAM)
by asking for WC mapping of BOs intended to be in VRAM even while they're
evicted (via the AMDGPU_GEM_CREATE_CPU_GTT_USWC flag).
--
Earthling Michel Dänzer| https://redhat.com
Libre software enthusiast | Mesa and Xwayland developer
Re: [Intel-gfx] [PATCH v3] kcmp: Support selection of SYS_kcmp without CHECKPOINT_RESTORE
On 2021-02-12 1:57 p.m., Emil Velikov wrote: On Fri, 5 Feb 2021 at 22:01, Chris Wilson wrote: Userspace has discovered the functionality offered by SYS_kcmp and has started to depend upon it. In particular, Mesa uses SYS_kcmp for os_same_file_description() in order to identify when two fd (e.g. device or dmabuf) As you rightfully point out, SYS_kcmp is a bit of a two edged sword. While you mention the CONFIG issue, there is also a portability aspect (mesa runs on more than just linux) and as well as sandbox filtering of the extra syscall. Last time I looked, the latter was still an issue and mesa was using SYS_kcmp to compare device node fds. A far shorter and more portable solution is possible, so let me prepare a Mesa patch. Make sure to read my comments on https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/6881 first. :) -- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] kernel: Expose SYS_kcmp by default
On 2021-02-08 2:34 p.m., Daniel Vetter wrote: On Mon, Feb 8, 2021 at 12:49 PM Michel Dänzer wrote: On 2021-02-05 9:53 p.m., Daniel Vetter wrote: On Fri, Feb 5, 2021 at 7:37 PM Kees Cook wrote: On Fri, Feb 05, 2021 at 04:37:52PM +, Chris Wilson wrote: Userspace has discovered the functionality offered by SYS_kcmp and has started to depend upon it. In particular, Mesa uses SYS_kcmp for os_same_file_description() in order to identify when two fd (e.g. device or dmabuf) point to the same struct file. Since they depend on it for core functionality, lift SYS_kcmp out of the non-default CONFIG_CHECKPOINT_RESTORE into the selectable syscall category. Signed-off-by: Chris Wilson Cc: Kees Cook Cc: Andy Lutomirski Cc: Will Drewry Cc: Andrew Morton Cc: Dave Airlie Cc: Daniel Vetter Cc: Lucas Stach --- init/Kconfig | 11 +++ kernel/Makefile | 2 +- tools/testing/selftests/seccomp/seccomp_bpf.c | 2 +- 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/init/Kconfig b/init/Kconfig index b77c60f8b963..f62fca13ac5b 100644 --- a/init/Kconfig +++ b/init/Kconfig @@ -1194,6 +1194,7 @@ endif # NAMESPACES config CHECKPOINT_RESTORE bool "Checkpoint/restore support" select PROC_CHILDREN + select KCMP default n help Enables additional kernel features in a sake of checkpoint/restore. @@ -1737,6 +1738,16 @@ config ARCH_HAS_MEMBARRIER_CALLBACKS config ARCH_HAS_MEMBARRIER_SYNC_CORE bool +config KCMP + bool "Enable kcmp() system call" if EXPERT + default y I would expect this to be not default-y, especially if CHECKPOINT_RESTORE does a "select" on it. This is a really powerful syscall, but it is bounded by ptrace access controls, and uses pointer address obfuscation, so it may be okay to expose this. As it is, at least Ubuntu already has CONFIG_CHECKPOINT_RESTORE, so really, there's probably not much difference on exposure. So, if you drop the "default y", I'm fine with this. It was maybe stupid, but our userspace started relying on fd comaprison through sys_kcomp. So for better or worse, if you want to run the mesa3d gl/vk stacks, you need this. That's overstating things somewhat. The vast majority of applications will work fine regardless (as they did before Mesa started using this functionality). Only some special ones will run into issues, because the user-space drivers incorrectly assume two file descriptors reference different descriptions. Was maybe not the brighest ideas, but since enough distros had this enabled by defaults, Right, that (and the above) is why I considered it fair game to use. What should I have done instead? (TBH I was surprised that this functionality isn't generally available) Yeah that one is fine, but I thought we've discussed (irc or something) more uses for de-duping dma-buf and stuff like that. But quick grep says that hasn't landed yet, so I got a bit confused (or just dreamt). Looking at this again I'm kinda surprised the drmfd de-duping blows up on normal linux distros, but I guess it can all happen. One example: GEM handle name-spaces are per file description. If user-space incorrectly assumes two DRM fds are independent, when they actually reference the same file description, closing a GEM handle with one file descriptor will make it unusable with the other file descriptor as well. Ofc we can leave the default n, but the select if CONFIG_DRM is unfortunately needed I think. Per above, not sure this is really true. We seem to be going boom on linux distros now, maybe userspace got more creative in abusing stuff? I don't know what you're referring to. I've only seen maybe two or three reports from people who didn't enable CHECKPOINT_RESTORE in their self-built kernels. The entire thing is small enough that imo we don't really have to care, e.g. we also unconditionally select dma-buf, despite that on most systems there's only 1 gpu, and you're never going to end up with a buffer sharing case that needs any of that code (aside from the "here's an fd" part). But I guess we can limit to just KCMP_FILE like you suggest in another reply. Just feels a bit like overkill. Making KCMP_FILE gated by DRM makes as little sense to me as by CHECKPOINT_RESTORE. -- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] kernel: Expose SYS_kcmp by default
On 2021-02-08 12:49 p.m., Michel Dänzer wrote: On 2021-02-05 9:53 p.m., Daniel Vetter wrote: On Fri, Feb 5, 2021 at 7:37 PM Kees Cook wrote: On Fri, Feb 05, 2021 at 04:37:52PM +, Chris Wilson wrote: Userspace has discovered the functionality offered by SYS_kcmp and has started to depend upon it. In particular, Mesa uses SYS_kcmp for os_same_file_description() in order to identify when two fd (e.g. device or dmabuf) point to the same struct file. Since they depend on it for core functionality, lift SYS_kcmp out of the non-default CONFIG_CHECKPOINT_RESTORE into the selectable syscall category. Signed-off-by: Chris Wilson Cc: Kees Cook Cc: Andy Lutomirski Cc: Will Drewry Cc: Andrew Morton Cc: Dave Airlie Cc: Daniel Vetter Cc: Lucas Stach --- init/Kconfig | 11 +++ kernel/Makefile | 2 +- tools/testing/selftests/seccomp/seccomp_bpf.c | 2 +- 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/init/Kconfig b/init/Kconfig index b77c60f8b963..f62fca13ac5b 100644 --- a/init/Kconfig +++ b/init/Kconfig @@ -1194,6 +1194,7 @@ endif # NAMESPACES config CHECKPOINT_RESTORE bool "Checkpoint/restore support" select PROC_CHILDREN + select KCMP default n help Enables additional kernel features in a sake of checkpoint/restore. @@ -1737,6 +1738,16 @@ config ARCH_HAS_MEMBARRIER_CALLBACKS config ARCH_HAS_MEMBARRIER_SYNC_CORE bool +config KCMP + bool "Enable kcmp() system call" if EXPERT + default y I would expect this to be not default-y, especially if CHECKPOINT_RESTORE does a "select" on it. This is a really powerful syscall, but it is bounded by ptrace access controls, and uses pointer address obfuscation, so it may be okay to expose this. As it is, at least Ubuntu already has CONFIG_CHECKPOINT_RESTORE, so really, there's probably not much difference on exposure. So, if you drop the "default y", I'm fine with this. It was maybe stupid, but our userspace started relying on fd comaprison through sys_kcomp. So for better or worse, if you want to run the mesa3d gl/vk stacks, you need this. That's overstating things somewhat. The vast majority of applications will work fine regardless (as they did before Mesa started using this functionality). Only some special ones will run into issues, because the user-space drivers incorrectly assume two file descriptors reference different descriptions. Was maybe not the brighest ideas, but since enough distros had this enabled by defaults, Right, that (and the above) is why I considered it fair game to use. What should I have done instead? (TBH I was surprised that this functionality isn't generally available) In that spirit, an alternative might be to make KCMP_FILE available unconditionally, and the rest of SYS_kcmp only with CHECKPOINT_RESTORE as before. (Or maybe other parts of SYS_kcmp are generally useful as well?) -- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] kernel: Expose SYS_kcmp by default
On 2021-02-05 9:53 p.m., Daniel Vetter wrote: On Fri, Feb 5, 2021 at 7:37 PM Kees Cook wrote: On Fri, Feb 05, 2021 at 04:37:52PM +, Chris Wilson wrote: Userspace has discovered the functionality offered by SYS_kcmp and has started to depend upon it. In particular, Mesa uses SYS_kcmp for os_same_file_description() in order to identify when two fd (e.g. device or dmabuf) point to the same struct file. Since they depend on it for core functionality, lift SYS_kcmp out of the non-default CONFIG_CHECKPOINT_RESTORE into the selectable syscall category. Signed-off-by: Chris Wilson Cc: Kees Cook Cc: Andy Lutomirski Cc: Will Drewry Cc: Andrew Morton Cc: Dave Airlie Cc: Daniel Vetter Cc: Lucas Stach --- init/Kconfig | 11 +++ kernel/Makefile | 2 +- tools/testing/selftests/seccomp/seccomp_bpf.c | 2 +- 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/init/Kconfig b/init/Kconfig index b77c60f8b963..f62fca13ac5b 100644 --- a/init/Kconfig +++ b/init/Kconfig @@ -1194,6 +1194,7 @@ endif # NAMESPACES config CHECKPOINT_RESTORE bool "Checkpoint/restore support" select PROC_CHILDREN + select KCMP default n help Enables additional kernel features in a sake of checkpoint/restore. @@ -1737,6 +1738,16 @@ config ARCH_HAS_MEMBARRIER_CALLBACKS config ARCH_HAS_MEMBARRIER_SYNC_CORE bool +config KCMP + bool "Enable kcmp() system call" if EXPERT + default y I would expect this to be not default-y, especially if CHECKPOINT_RESTORE does a "select" on it. This is a really powerful syscall, but it is bounded by ptrace access controls, and uses pointer address obfuscation, so it may be okay to expose this. As it is, at least Ubuntu already has CONFIG_CHECKPOINT_RESTORE, so really, there's probably not much difference on exposure. So, if you drop the "default y", I'm fine with this. It was maybe stupid, but our userspace started relying on fd comaprison through sys_kcomp. So for better or worse, if you want to run the mesa3d gl/vk stacks, you need this. That's overstating things somewhat. The vast majority of applications will work fine regardless (as they did before Mesa started using this functionality). Only some special ones will run into issues, because the user-space drivers incorrectly assume two file descriptors reference different descriptions. Was maybe not the brighest ideas, but since enough distros had this enabled by defaults, Right, that (and the above) is why I considered it fair game to use. What should I have done instead? (TBH I was surprised that this functionality isn't generally available) it wasn't really discovered, and now we're shipping this everywhere. You're making it sound like this snuck in secretly somehow, which is not true of course. Ofc we can leave the default n, but the select if CONFIG_DRM is unfortunately needed I think. Per above, not sure this is really true. -- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v5 0/5] Asynchronous flip implementation for i915
On 2020-07-29 9:23 a.m., Kulkarni, Vandita wrote: > > On async flips, there needs to be some clarity/guideline on the behaviour and > event expectation from the > driver by user space. > Here are few assumptions that we have, > 1. Our understanding is that the user space doesn’t expect the timestamp for > async flips (but still expects vblank timestamp) , or > doesn’t do anything with that, same is the assumption wrt the flip sequence, > please correct us if we are wrong. > 2. In the sequence the user space still expects the counter that marks > vblanks. > 3. The user space can use different event types like DRM_EVENT_VBLANK or > DRM_EVENT_FLIP_COMPLETE > for getting the corresponding event. And their designs are still aligned to > this even in case of async. > > If there are any more expectations from the user space wrt to the event that > is being sent from the > driver in case of async flip, please let us know. > > If the user space doesn’t care much about the flip sequence then, we can just > not do anything like returning > the flip counter like this version is doing and just stick to returning of > the frame counter value(which marks vblanks). There's no such thing as a "flip sequence" in the KMS API. There's only the per-CRTC vblank counter. Each flip completion event needs to contain the value of that counter when the hardware completed the flip, regardless of whether it was an async flip or not. As for the timestamp in the event, I'm not sure what the expectations are for async flips, but I suspect it may not really matter. E.g. the timestamp calculated to correspond to the end of the previous vertical blank period might be fine. -- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v5 1/5] drm/i915: Add enable/disable flip done and flip done handler
On 2020-07-25 1:26 a.m., Paulo Zanoni wrote:
> Em seg, 2020-07-20 às 17:01 +0530, Karthik B S escreveu:
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c
>> b/drivers/gpu/drm/i915/i915_irq.c
>> index 1fa67700d8f4..95953b393941 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -697,14 +697,24 @@ u32 i915_get_vblank_counter(struct drm_crtc *crtc)
>> return (((high1 << 8) | low) + (pixel >= vbl_start)) & 0xff;
>> }
>>
>> +static u32 g4x_get_flip_counter(struct drm_crtc *crtc)
>> +{
>> +struct drm_i915_private *dev_priv = to_i915(crtc->dev);
>> +enum pipe pipe = to_intel_crtc(crtc)->pipe;
>> +
>> +return I915_READ(PIPE_FLIPCOUNT_G4X(pipe));
>> +}
>> +
>> u32 g4x_get_vblank_counter(struct drm_crtc *crtc)
>> {
>> struct drm_i915_private *dev_priv = to_i915(crtc->dev);
>> enum pipe pipe = to_intel_crtc(crtc)->pipe;
>>
>> +if (crtc->state->async_flip)
>> +return g4x_get_flip_counter(crtc);
>> +
>> return I915_READ(PIPE_FRMCOUNT_G4X(pipe));
>
> I don't understand the intention behind this, can you please clarify?
> This goes back to my reply of the cover letter. It seems that here
> we're going to alternate between two different counters in our vblank
> count. So if user space alternates between sometimes using async flips
> and sometimes using normal flip it's going to get some very weird
> deltas, isn't it? At least this is what I remember from when I played
> with these registers: FLIPCOUNT drifts away from FRMCOUNT when we start
> using async flips.
This definitely looks wrong. The counter value returned by the
get_vblank_counter hook is supposed to increment when a vertical blank
period occurs; page flips are not supposed to affect this in any way.
--
Earthling Michel Dänzer | https://redhat.com
Libre software enthusiast | Mesa and X developer
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v4 3/3] drm/i915/dp: Expose connector VRR info via debugfs
On 2020-04-14 2:44 p.m., Daniel Vetter wrote: > On Mon, Apr 13, 2020 at 10:08:07PM -0700, Manasi Navare wrote: >> From: Bhanuprakash Modem >> >> [Why] >> It's useful to know the min and max vrr range for IGT testing. >> >> [How] >> Expose the min and max vfreq for the connector via a debugfs file >> on the connector, "i915_vrr_info". >> >> Example usage: cat /sys/kernel/debug/dri/0/DP-1/i915_vrr_info >> >> v3: >> * Remove the unnecessary debug print (Manasi) >> v2: >> * Fix the typo in max_vfreq (Manasi) >> * Change the name of node to i915_vrr_info so we can add >> other vrr info for more debug info (Manasi) >> * Change the VRR capable to display Yes or No (Manasi) >> * Fix indentation checkpatch errors (Manasi) >> >> Signed-off-by: Bhanuprakash Modem >> Signed-off-by: Manasi Navare >> Cc: Jani Nikula >> Cc: Ville Syrjälä >> Tested-by: Manasi Navare > > So if I'm understanding things correctly AMD butchered the VRR stuff and > only exposes it when: > > - VRR_ENABLED is set Not really surprising? :) > - _and_ you're using the legacy page_flip path, atomic flip doesn't > support it Simon Ser has VRR working with sway using the atomic KMS API. > - _and_ the PAGE_FLIP_ASYNC flag is set. AFAIK it works both without and with PAGE_FLIP_ASYNC. (Async just means tearing if the flip is programmed outside of vertical blank) -- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and X developer ___ 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 2020-04-06 6:34 p.m., Rob Clark wrote: > > The ideal thing would be to be able to click any jobs that we want to > run, say "arm64_a630_gles31", and for gitlab to realize that it needs > to automatically trigger dependencies of that job (meson-arm64, and > arm_build+arm_test). But not sure if that is a thing gitlab can do. Not that I know of. The dependency handling is still pretty rudimentary in general. > Triggering just the first container jobs and having everything from > there run automatically would be ok if the current rules to filter out > unneeded jobs still apply, ie. a panfrost change isn't triggering > freedreno CI jobs and visa versa. But tbh I don't understand enough > of what that MR is doing to understand if that is what it does. (It > was suggested on IRC that this is probably what it does.) It is. Filtered jobs don't exist at all in the pipeline, so they can't be triggered by any means. :) -- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and X developer ___ 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 2020-03-01 6:46 a.m., Marek Olšák wrote: > For Mesa, we could run CI only when Marge pushes, so that it's a strictly > pre-merge CI. Thanks for the suggestion! I implemented something like this for Mesa: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/4432 -- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and X developer ___ 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 2020-02-29 8:46 p.m., Nicolas Dufresne wrote: > Le samedi 29 février 2020 à 19:14 +0100, Timur Kristóf a écrit : >> >> 1. I think we should completely disable running the CI on MRs which are >> marked WIP. Speaking from personal experience, I usually make a lot of >> changes to my MRs before they are merged, so it is a waste of CI >> resources. Interesting idea, do you want to create an MR implementing it? > In the mean time, you can help by taking the habit to use: > > git push -o ci.skip That breaks Marge Bot. > Notably, we would like to get rid of the post merge CI, as in a rebase > flow like we have in GStreamer, it's a really minor risk. That should be pretty easy, see Mesa and https://docs.gitlab.com/ce/ci/variables/predefined_variables.html. Something like this should work: rules: - if: '$CI_PROJECT_NAMESPACE != "gstreamer"' when: never This is another interesting idea we could consider for Mesa as well. It would however require (mostly) banning direct pushes to the main repository. >> 2. Maybe we could take this one step further and only allow the CI to >> be only triggered manually instead of automatically on every push. That would again break Marge Bot. -- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and X developer ___ 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 2020-02-28 12:02 p.m., Erik Faye-Lund wrote: > On Fri, 2020-02-28 at 10:43 +, Daniel Stone wrote: >> 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 > > I'm not sure it's more robust, but yeah that a useful tool too. > > The reason I'm skeptical about the robustness is that we'll miss > testing if this misses a path. Surely missing a path will be less likely / often to happen compared to an MR missing a label. (Users which aren't members of the project can't even set labels for an MR) -- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and X developer ___ 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 2020-02-28 10:28 a.m., Erik Faye-Lund wrote: > > We could also do stuff like reducing the amount of tests we run on each > commit, and punt some testing to a per-weekend test-run or someting > like that. We don't *need* to know about every problem up front, just > the stuff that's about to be released, really. The other stuff is just > nice to have. If it's too expensive, I would say drop it. I don't agree that pre-merge testing is just nice to have. A problem which is only caught after it lands in mainline has a much bigger impact than one which is already caught earlier. -- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Cast remain to unsigned long in eb_relocate_vma
On 2020-02-14 12:49 p.m., Jani Nikula wrote: > On Fri, 14 Feb 2020, Chris Wilson wrote: >> Quoting Jani Nikula (2020-02-14 06:36:15) >>> On Thu, 13 Feb 2020, Nathan Chancellor wrote: >>>> A recent commit in clang added -Wtautological-compare to -Wall, which is >>>> enabled for i915 after -Wtautological-compare is disabled for the rest >>>> of the kernel so we see the following warning on x86_64: >>>> >>>> ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1433:22: warning: >>>> result of comparison of constant 576460752303423487 with expression of >>>> type 'unsigned int' is always false >>>> [-Wtautological-constant-out-of-range-compare] >>>> if (unlikely(remain > N_RELOC(ULONG_MAX))) >>>> ^ >>>> ../include/linux/compiler.h:78:42: note: expanded from macro 'unlikely' >>>> # define unlikely(x)__builtin_expect(!!(x), 0) >>>> ^ >>>> 1 warning generated. >>>> >>>> It is not wrong in the case where ULONG_MAX > UINT_MAX but it does not >>>> account for the case where this file is built for 32-bit x86, where >>>> ULONG_MAX == UINT_MAX and this check is still relevant. >>>> >>>> Cast remain to unsigned long, which keeps the generated code the same >>>> (verified with clang-11 on x86_64 and GCC 9.2.0 on x86 and x86_64) and >>>> the warning is silenced so we can catch more potential issues in the >>>> future. >>>> >>>> Link: https://github.com/ClangBuiltLinux/linux/issues/778 >>>> Suggested-by: Michel Dänzer >>>> Signed-off-by: Nathan Chancellor >>> >>> Works for me as a workaround, >> >> But the whole point was that the compiler could see that it was >> impossible and not emit the code. Doesn't this break that? > > It seems that goal and the warning are fundamentally incompatible. Not really: if (sizeof(remain) >= sizeof(unsigned long) && unlikely(remain > N_RELOC(ULONG_MAX))) return -EINVAL; In contrast to the cast, this doesn't generate any machine code on 64-bit: https://godbolt.org/z/GmUE4S but still generates the same code on 32-bit: https://godbolt.org/z/hAoz8L -- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915: Disable -Wtautological-constant-out-of-range-compare
On 2020-02-12 6:07 p.m., Nathan Chancellor wrote: > On Wed, Feb 12, 2020 at 09:52:52AM +0100, Michel Dänzer wrote: >> On 2020-02-11 9:39 p.m., Nathan Chancellor wrote: >>> On Tue, Feb 11, 2020 at 10:41:48AM +0100, Michel Dänzer wrote: >>>> On 2020-02-11 7:13 a.m., Nathan Chancellor wrote: >>>>> A recent commit in clang added -Wtautological-compare to -Wall, which is >>>>> enabled for i915 so we see the following warning: >>>>> >>>>> ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1485:22: warning: >>>>> result of comparison of constant 576460752303423487 with expression of >>>>> type 'unsigned int' is always false >>>>> [-Wtautological-constant-out-of-range-compare] >>>>> if (unlikely(remain > N_RELOC(ULONG_MAX))) >>>>> ^ >>>>> >>>>> This warning only happens on x86_64 but that check is relevant for >>>>> 32-bit x86 so we cannot remove it. >>>> >>>> That's suprising. AFAICT N_RELOC(ULONG_MAX) works out to the same value >>>> in both cases, and remain is a 32-bit value in both cases. How can it be >>>> larger than N_RELOC(ULONG_MAX) on 32-bit (but not on 64-bit)? >>>> >>> >>> Hi Michel, >>> >>> Can't this condition be true when UINT_MAX == ULONG_MAX? >> >> Oh, right, I think I was wrongly thinking long had 64 bits even on 32-bit. >> >> >> Anyway, this suggests a possible better solution: >> >> #if UINT_MAX == ULONG_MAX >> if (unlikely(remain > N_RELOC(ULONG_MAX))) >> return -EINVAL; >> #endif >> >> >> Or if that can't be used for some reason, something like >> >> if (unlikely((unsigned long)remain > N_RELOC(ULONG_MAX))) >> return -EINVAL; >> >> should silence the warning. > > I do like this one better than the former. FWIW, one downside of this one compared to all alternatives (presumably) is that it might end up generating actual code even on 64-bit, which always ends up skipping the return. -- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915: Disable -Wtautological-constant-out-of-range-compare
On 2020-02-11 9:39 p.m., Nathan Chancellor wrote:
> On Tue, Feb 11, 2020 at 10:41:48AM +0100, Michel Dänzer wrote:
>> On 2020-02-11 7:13 a.m., Nathan Chancellor wrote:
>>> A recent commit in clang added -Wtautological-compare to -Wall, which is
>>> enabled for i915 so we see the following warning:
>>>
>>> ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1485:22: warning:
>>> result of comparison of constant 576460752303423487 with expression of
>>> type 'unsigned int' is always false
>>> [-Wtautological-constant-out-of-range-compare]
>>> if (unlikely(remain > N_RELOC(ULONG_MAX)))
>>> ^
>>>
>>> This warning only happens on x86_64 but that check is relevant for
>>> 32-bit x86 so we cannot remove it.
>>
>> That's suprising. AFAICT N_RELOC(ULONG_MAX) works out to the same value
>> in both cases, and remain is a 32-bit value in both cases. How can it be
>> larger than N_RELOC(ULONG_MAX) on 32-bit (but not on 64-bit)?
>>
>
> Hi Michel,
>
> Can't this condition be true when UINT_MAX == ULONG_MAX?
Oh, right, I think I was wrongly thinking long had 64 bits even on 32-bit.
Anyway, this suggests a possible better solution:
#if UINT_MAX == ULONG_MAX
if (unlikely(remain > N_RELOC(ULONG_MAX)))
return -EINVAL;
#endif
Or if that can't be used for some reason, something like
if (unlikely((unsigned long)remain > N_RELOC(ULONG_MAX)))
return -EINVAL;
should silence the warning.
Either of these should be better than completely disabling the warning
for the whole file.
--
Earthling Michel Dänzer | https://redhat.com
Libre software enthusiast | Mesa and X developer
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915: Disable -Wtautological-constant-out-of-range-compare
On 2020-02-11 7:13 a.m., Nathan Chancellor wrote: > A recent commit in clang added -Wtautological-compare to -Wall, which is > enabled for i915 so we see the following warning: > > ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1485:22: warning: > result of comparison of constant 576460752303423487 with expression of > type 'unsigned int' is always false > [-Wtautological-constant-out-of-range-compare] > if (unlikely(remain > N_RELOC(ULONG_MAX))) > ^ > > This warning only happens on x86_64 but that check is relevant for > 32-bit x86 so we cannot remove it. That's suprising. AFAICT N_RELOC(ULONG_MAX) works out to the same value in both cases, and remain is a 32-bit value in both cases. How can it be larger than N_RELOC(ULONG_MAX) on 32-bit (but not on 64-bit)? -- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/kms: Duct-tape for mode object lifetime checks
On 2019-09-17 2:09 p.m., Daniel Vetter wrote:
> commit 4f5368b5541a902f6596558b05f5c21a9770dd32
> Author: Daniel Vetter
> Date: Fri Jun 14 08:17:23 2019 +0200
>
> drm/kms: Catch mode_object lifetime errors
>
> uncovered a bit a mess in dp drivers. Most drivers (from a quick look,
> all except i915) register all the dp stuff in their init code, which
> is too early. With CONFIG_DRM_DP_AUX_CHARDEV this will blow up,
> because drm_dp_aux_register tries to add a child to a device in sysfs
> (the connector) which doesn't even exist yet.
>
> No one seems to have cared thus far. But with the above change I also
> moved the setting of dev->registered after the ->load callback, in an
> attempt to keep old drivers from hitting any WARN_ON backtraces. But
> that moved radeon.ko from the "working, by accident" to "now also
> broken" category.
>
> Since this is a huge mess I figured a revert would be simplest. But
> this check has already caught issues in i915:
>
> commit 1b9bd09630d4db4827cc04d358a41a16a6bc2cb0
> Author: Ville Syrjälä
> Date: Tue Aug 20 19:16:57 2019 +0300
>
> drm/i915: Do not create a new max_bpc prop for MST connectors
>
> Hence I'd like to retain it. Fix the radeon regression by moving the
> setting of dev->registered back to were it was, and stop the
> backtraces with an explicit check for dev->driver->load.
>
> Everyone else will stay as broken with CONFIG_DRM_DP_AUX_CHARDEV. The
> next patch will improve the kerneldoc and add a todo entry for this.
>
> Fixes: 4f5368b5541a ("drm/kms: Catch mode_object lifetime errors")
> Cc: Sean Paul
> Cc: Maarten Lankhorst
> Reported-by: Michel Dänzer
> Cc: Michel Dänzer
> Signed-off-by: Daniel Vetter
Reviewed-by: Michel Dänzer
Tested-by: Michel Dänzer
Thanks!
--
Earthling Michel Dänzer | https://redhat.com
Libre software enthusiast | Mesa and X developer
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC PATCH 0/3] Propose new struct drm_mem_region
On 2019-07-30 12:45 p.m., Daniel Vetter wrote: > On Tue, Jul 30, 2019 at 12:24 PM Koenig, Christian > wrote: >> Am 30.07.19 um 11:38 schrieb Daniel Vetter: >>> On Tue, Jul 30, 2019 at 08:45:57AM +, Koenig, Christian wrote: > >>>>> +#define DRM_MEM_SYSTEM 0 >>>>> +#define DRM_MEM_STOLEN 1 >>>> I think we need a better naming for that. >>>> >>>> STOLEN sounds way to much like stolen VRAM for integrated GPUs, but at >>>> least for TTM this is the system memory currently GPU accessible. >>> Yup this is wrong, for i915 we use this as stolen, for ttm it's the gpu >>> translation table window into system memory. Not the same thing at all. >> >> Thought so. The closest I have in mind is GTT, but everything else works >> as well. > > Would your GPU_MAPPED above work for TT? I think we'll also need > STOLEN, I'm even hearing noises that there's going to be stolen for > discrete vram for us ... Could i915 use DRM_MEM_PRIV for stolen? Or is there other hardware with something similar? -- Earthling Michel Dänzer | https://www.amd.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 1/1] drm/vblank: drop use of DRM_WAIT_ON()
On 2019-07-26 11:06 p.m., Sam Ravnborg wrote: > DRM_WAIT_ON() is from the deprecated drm_os_linux header and > the modern replacement is the wait_event_*. > > The return values differ, so a conversion is needed to > keep the original interface towards userspace. > Introduced a switch/case to make code obvious. > > Analysis from Michel Dänzer: > > The waiting condition rely on all relevant places where vblank_count > is modified calls wake_up(>queue). > > drm_handle_vblank(): > - Calls wake_up(>queue) > > drm_vblank_enable(): > - There is no need here because there can be no sleeping waiters > in the queue, because vblank->enabled == false immediately > terminates any waits. > > drm_crtc_accurate_vblank_count(): > - This is called from interrupt handlers, at least from > amdgpu_dm.c:dm_pflip_high_irq(). Not sure it needs to wake up > the queue though, the driver should call > drm_(crtc_)_handle_vblank anyway. > > drm_vblank_disable_and_save(): > - It can be called from an interrupt, via drm_handle_vblank -> > vblank_disable_fn. However, the only place where > drm_vblank_disable_and_save can be called with sleeping waiters > in the queue is in drm_crtc_vblank_off, which wakes up the queue > afterwards (which terminates all waits, because > vblank->enabled == false at this point). > > v3: > - Added analysis to changelog from Michel Dänzer > - Moved return result handling inside if (req_seq != seq) (Daniel V) > - Reused more of the former logic - resulting in simpler code > - Dropped Reviewed-by from Sean Paul as this is a new implmentation > > v2: > - Fix so the case where req_seq equals seq was handled properly > - quick hack to check if IGT became happy > - Only sent to igt, not to dri-devel > > Signed-off-by: Sam Ravnborg > Cc: "Michel Dänzer" > Cc: Sean Paul > Cc: Maarten Lankhorst > Cc: Maxime Ripard > Cc: David Airlie > Cc: Daniel Vetter Reviewed-by: Michel Dänzer -- Earthling Michel Dänzer | https://www.amd.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] dma-buf: Expand reservation_list to fill allocation
On 2019-07-12 10:03 a.m., Chris Wilson wrote:
> Since kmalloc() will round up the allocation to the next slab size or
> page, it will normally return a pointer to a memory block bigger than we
> asked for. We can query for the actual size of the allocated block using
> ksize() and expand our variable size reservation_list to take advantage
> of that extra space.
>
> Signed-off-by: Chris Wilson
> Cc: Christian König
> Cc: Michel Dänzer
> ---
> drivers/dma-buf/reservation.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c
> index a6ac2b3a0185..80ecc1283d15 100644
> --- a/drivers/dma-buf/reservation.c
> +++ b/drivers/dma-buf/reservation.c
> @@ -153,7 +153,9 @@ int reservation_object_reserve_shared(struct
> reservation_object *obj,
> RCU_INIT_POINTER(new->shared[j++], fence);
> }
> new->shared_count = j;
> - new->shared_max = max;
> + new->shared_max =
> + (ksize(new) - offsetof(typeof(*new), shared)) /
> + sizeof(*new->shared);
>
> preempt_disable();
> write_seqcount_begin(>seq);
> @@ -169,7 +171,7 @@ int reservation_object_reserve_shared(struct
> reservation_object *obj,
> return 0;
>
> /* Drop the references to the signaled fences */
> - for (i = k; i < new->shared_max; ++i) {
> + for (i = k; i < max; ++i) {
> struct dma_fence *fence;
>
> fence = rcu_dereference_protected(new->shared[i],
>
Nice, TIL about ksize(), wonder where else that could be used.
Reviewed-by: Michel Dänzer
P.S. According to scripts/get_maintainer.pl , this series should be sent
to more recipients for review.
--
Earthling Michel Dänzer | https://www.amd.com
Libre software enthusiast | Mesa and X developer
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: allow render capable master with DRM_AUTH ioctls
On 2019-07-03 7:10 p.m., Emil Velikov wrote: > From: Emil Velikov > > There are cases (in mesa and applications) where one would open the > primary node without properly authenticating the client. > > Sometimes we don't check if the authentication succeeds, but there's > also cases we simply forget to do it. > > The former was a case for Mesa where it did not not check the return > value of drmGetMagic() [1]. That was fixed recently although, there's > the question of older drivers or other apps that exbibit this behaviour. > > While omitting the call results in issues as seen in [2] and [3]. > > In the libva case, libva itself doesn't authenticate the DRM client and > the vaGetDisplayDRM documentation doesn't mention if the app should > either. > > As of today, the official vainfo utility doesn't authenticate. > > To workaround issues like these, some users resort to running their apps > under sudo. Which admittedly isn't always a good idea. > > Since any DRIVER_RENDER driver has sufficient isolation between clients, > we can use that, for unauthenticated [primary node] ioctls that require > DRM_AUTH. But only if the respective ioctl is tagged as DRM_RENDER_ALLOW. > > v2: > - Rework/simplify if check (Daniel V) > - Add examples to commit messages, elaborate. (Daniel V) > > v3: > - Use single unlikely (Daniel V) > > v4: > - Reapply patch, check for amdgpu/radeon inline. > > [1] > https://gitlab.freedesktop.org/mesa/mesa/blob/2bc1f5c2e70fe3b4d41f060af9859bc2a94c5b62/src/egl/drivers/dri2/platform_wayland.c#L1136 > [2] https://lists.freedesktop.org/archives/libva/2016-July/004185.html > [3] https://gitlab.freedesktop.org/mesa/kmscube/issues/1 > Testcase: igt/core_unauth_vs_render > Cc: intel-gfx@lists.freedesktop.org > Cc: Daniel Vetter > Signed-off-by: Emil Velikov > Reviewed-by: Daniel Vetter As discussed on IRC, IMHO this change requires more justification. The system I'm writing this on has vainfo 2.4.0 installed, which opens a render node and works fine without this change. Similarly, if kmscube hasn't been fixed to use a render node yet, surely it easily can. You're asserting that the problem is wide-spread, and that fixing all broken userspace isn't feasible, but I haven't seen any evidence supporting that. Since this change is essentially a workaround for broken userspace which can never have worked, and has the potential of subverting the ongoing transition from using primary nodes to render nodes in userspace code, there needs to be evidence supporting that the benefit outweighs the risk. -- Earthling Michel Dänzer | https://www.amd.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC][PATCH 0/2] drm: PATH prop for all connectors?
On 2019-06-27 9:35 a.m., Pekka Paalanen wrote: > > Are connector names in xrandr still using type_id in their names? Yes, they are. -- Earthling Michel Dänzer | https://www.amd.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/ioctl: Ditch DRM_UNLOCKED except for the legacy vblank ioctl
On 2019-05-29 11:30 a.m., Daniel Vetter wrote: > This completes Emil's series of removing DRM_UNLOCKED from modern > drivers. It's entirely cargo-culted since we ignore it on > non-DRIVER_LEGACY drivers since: > > commit ea487835e8876abf7ad909636e308c801a2bcda6 > Author: Daniel Vetter > Date: Mon Sep 28 21:42:40 2015 +0200 > > drm: Enforce unlocked ioctl operation for kms driver ioctls > > Now justifying why we can do this for legacy drives too (and hence > close the source of all the bogus copypasting) is a bit more involved. > DRM_UNLOCKED was introduced in: > > commit ed8b67040965e4fe695db333d5914e18ea5f146f > Author: Arnd Bergmann > Date: Wed Dec 16 22:17:09 2009 + > > drm: convert drm_ioctl to unlocked_ioctl > > As a immediate hack to keep i810 happy, which would have deadlocked > without this trickery. The old BKL is automatically dropped in > schedule(), and hence the i810 vs. mmap_sem deadlock didn't actually > cause a real deadlock. But with a mutex it would. The solution was to > annotate these as DRM_UNLOCKED and mark i810 unsafe on SMP machines. > > This conversion caused a regression, because unlike the BKL a mutex > isn't dropped over schedule (that thing again), which caused a vblank > wait in one thread to block the entire desktop and all its apps. Back > then we did vblank scheduling by blocking in the client, awesome isn't > it. This was fixed quickly in (ok not so quickly, took 2 years): > > commit 8f4ff2b06afcd6f151868474a432c603057eaf56 > Author: Ilija Hadzic > Date: Mon Oct 31 17:46:18 2011 -0400 > > drm: do not sleep on vblank while holding a mutex > > All the other DRM_UNLOCKED annotations for all the core ioctls was > work to reach finer-grained locking for modern drivers. This took > years, and culminated in: > > commit fdd5b877e9ebc2029e1373b4a3cd057329a9ab7a > Author: Daniel Vetter > Date: Sat Dec 10 22:52:54 2016 +0100 > > drm: Enforce BKL-less ioctls for modern drivers > > DRM_UNLOCKED was never required by any legacy drivers, except for the > vblank_wait IOCTL. Therefore we will not regress these old drivers by > going back to where we've been in 2011. For all modern drivers nothing > will change. > > To make this perfectly clear, also add a comment to DRM_UNLOCKED. > > Cc: Emil Velikov > Signed-off-by: Daniel Vetter > --- > drivers/gpu/drm/drm_ioctl.c | 139 ++------ > include/drm/drm_ioctl.h | 3 + > 2 files changed, 72 insertions(+), 70 deletions(-) Did you miss drivers/gpu/drm/drm_ioc32.c ? -- Earthling Michel Dänzer | https://www.amd.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v7 09/11] drm: uevent for connector status change
On 2019-05-21 9:52 a.m., Daniel Vetter wrote: > On Tue, May 21, 2019 at 8:55 AM Pekka Paalanen wrote: >> On Mon, 20 May 2019 18:11:07 +0200 >> Daniel Vetter wrote: >> >>> There's also a fairly easy fix for that -modesetting issue: We don't >>> expose atomic if the compositor has a process name of "Xserver". Brutal, >>> but gets the job done. Once X is fixed, we can give a new "I'm not totally >>> broken anymore" interface to get back at atomic. >> >> You mean "Xorg". Or maybe "X". Or maybe the setuid helper? Wait, do you >> check against the process issuing ioctl by ioctl, or the process that >> opened the device? Which would be logind? What about DRM leasing? ... > > In the Get/SetCaps ioctl we can do the check, which is called from X, > not logind. We just need some way to tell -modesetting apart from > everything else, and luckily there's not any other atomic X drivers. Not yet... As for a "I'm not totally broken anymore" interface, we did something like that (though kind of in the other direction) with RADEON_INFO_ACCEL_WORKING, but later RADEON_INFO_ACCEL_WORKING2 had to be added, because the former claimed acceleration was "working" in cases where it really wasn't... That kind of thing could become ugly in the long run if other Xorg driver start using atomic, and they'll inevitably be broken in different ways. -- Earthling Michel Dänzer | https://www.amd.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 1/3] drm: Add support for panic message output
On 2019-03-13 5:16 p.m., Kazlauskas, Nicholas wrote:
> On 3/13/19 11:54 AM, Christian König wrote:
>> Am 13.03.19 um 16:38 schrieb Michel Dänzer:
>>> On 2019-03-13 2:37 p.m., Christian König wrote:
>>>> Am 13.03.19 um 14:31 schrieb Ville Syrjälä:
>>>>> On Wed, Mar 13, 2019 at 10:35:08AM +0100, Michel Dänzer wrote:
>>>>>> On 2019-03-12 6:15 p.m., Noralf Trønnes wrote:
>>>>>>> Den 12.03.2019 17.17, skrev Ville Syrjälä:
>>>>>>>> On Tue, Mar 12, 2019 at 11:47:04AM +0100, Michel Dänzer wrote:
>>>>>>>>> On 2019-03-11 6:42 p.m., Noralf Trønnes wrote:
>>>>>>>>>> This adds support for outputting kernel messages on panic().
>>>>>>>>>> A kernel message dumper is used to dump the log. The dumper
>>>>>>>>>> iterates
>>>>>>>>>> over each DRM device and it's crtc's to find suitable
>>>>>>>>>> framebuffers.
>>>>>>>>>>
>>>>>>>>>> All the other dumpers are run before this one except mtdoops.
>>>>>>>>>> Only atomic drivers are supported.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Noralf Trønnes
>>>>>>>>>> ---
>>>>>>>>>> [...]
>>>>>>>>>>
>>>>>>>>>> diff --git a/include/drm/drm_framebuffer.h
>>>>>>>>>> b/include/drm/drm_framebuffer.h
>>>>>>>>>> index f0b34c977ec5..f3274798ecfe 100644
>>>>>>>>>> --- a/include/drm/drm_framebuffer.h
>>>>>>>>>> +++ b/include/drm/drm_framebuffer.h
>>>>>>>>>> @@ -94,6 +94,44 @@ struct drm_framebuffer_funcs {
>>>>>>>>>> struct drm_file *file_priv, unsigned flags,
>>>>>>>>>> unsigned color, struct drm_clip_rect *clips,
>>>>>>>>>> unsigned num_clips);
>>>>>>>>>> +
>>>>>>>>>> + /**
>>>>>>>>>> + * @panic_vmap:
>>>>>>>>>> + *
>>>>>>>>>> + * Optional callback for panic handling.
>>>>>>>>>> + *
>>>>>>>>>> + * For vmapping the selected framebuffer in a panic context.
>>>>>>>>>> Must
>>>>>>>>>> + * be super careful about locking (only trylocking allowed).
>>>>>>>>>> + *
>>>>>>>>>> + * RETURNS:
>>>>>>>>>> + *
>>>>>>>>>> + * NULL if it didn't work out, otherwise an opaque cookie
>>>>>>>>>> which is
>>>>>>>>>> + * passed to @panic_draw_xy. It can be anything: vmap area,
>>>>>>>>>> structure
>>>>>>>>>> + * with more details, just a few flags, ...
>>>>>>>>>> + */
>>>>>>>>>> + void *(*panic_vmap)(struct drm_framebuffer *fb);
>>>>>>>>> FWIW, the panic_vmap hook cannot work in general with the
>>>>>>>>> amdgpu/radeon
>>>>>>>>> drivers:
>>>>>>>>>
>>>>>>>>> Framebuffers are normally tiled, writing to them with the CPU
>>>>>>>>> results in
>>>>>>>>> garbled output.
>>>>>>>>>
>>>>>>> In which case the driver needs to support the ->panic_draw_xy
>>>>>>> callback,
>>>>>>> or maybe it's possible to make a generic helper for tiled buffers.
>>>>>> I'm afraid that won't help, at least not without porting big chunks of
>>>>>> https://gitlab.freedesktop.org/mesa/mesa/tree/master/src/amd/addrlib
>>>>>> into the kernel, none of which will be used for anything else.
>>>>>>
>>>>>>
>>>>>>>>> There would need to be a mechanism for switching scanout to a
>>>>>>>>> linear,
>>>>>>>>> CPU accessible framebuffer.
>>>>>>>> I suppose panic_vmap() could just provide a linear temp buffer
>>>>>>>> to the pa
Re: [Intel-gfx] [PATCH v2 1/3] drm: Add support for panic message output
On 2019-03-13 2:37 p.m., Christian König wrote:
> Am 13.03.19 um 14:31 schrieb Ville Syrjälä:
>> On Wed, Mar 13, 2019 at 10:35:08AM +0100, Michel Dänzer wrote:
>>> On 2019-03-12 6:15 p.m., Noralf Trønnes wrote:
>>>>
>>>> Den 12.03.2019 17.17, skrev Ville Syrjälä:
>>>>> On Tue, Mar 12, 2019 at 11:47:04AM +0100, Michel Dänzer wrote:
>>>>>> On 2019-03-11 6:42 p.m., Noralf Trønnes wrote:
>>>>>>> This adds support for outputting kernel messages on panic().
>>>>>>> A kernel message dumper is used to dump the log. The dumper iterates
>>>>>>> over each DRM device and it's crtc's to find suitable framebuffers.
>>>>>>>
>>>>>>> All the other dumpers are run before this one except mtdoops.
>>>>>>> Only atomic drivers are supported.
>>>>>>>
>>>>>>> Signed-off-by: Noralf Trønnes
>>>>>>> ---
>>>>>>> [...]
>>>>>>>
>>>>>>> diff --git a/include/drm/drm_framebuffer.h
>>>>>>> b/include/drm/drm_framebuffer.h
>>>>>>> index f0b34c977ec5..f3274798ecfe 100644
>>>>>>> --- a/include/drm/drm_framebuffer.h
>>>>>>> +++ b/include/drm/drm_framebuffer.h
>>>>>>> @@ -94,6 +94,44 @@ struct drm_framebuffer_funcs {
>>>>>>> struct drm_file *file_priv, unsigned flags,
>>>>>>> unsigned color, struct drm_clip_rect *clips,
>>>>>>> unsigned num_clips);
>>>>>>> +
>>>>>>> + /**
>>>>>>> + * @panic_vmap:
>>>>>>> + *
>>>>>>> + * Optional callback for panic handling.
>>>>>>> + *
>>>>>>> + * For vmapping the selected framebuffer in a panic context.
>>>>>>> Must
>>>>>>> + * be super careful about locking (only trylocking allowed).
>>>>>>> + *
>>>>>>> + * RETURNS:
>>>>>>> + *
>>>>>>> + * NULL if it didn't work out, otherwise an opaque cookie
>>>>>>> which is
>>>>>>> + * passed to @panic_draw_xy. It can be anything: vmap area,
>>>>>>> structure
>>>>>>> + * with more details, just a few flags, ...
>>>>>>> + */
>>>>>>> + void *(*panic_vmap)(struct drm_framebuffer *fb);
>>>>>> FWIW, the panic_vmap hook cannot work in general with the
>>>>>> amdgpu/radeon
>>>>>> drivers:
>>>>>>
>>>>>> Framebuffers are normally tiled, writing to them with the CPU
>>>>>> results in
>>>>>> garbled output.
>>>>>>
>>>> In which case the driver needs to support the ->panic_draw_xy callback,
>>>> or maybe it's possible to make a generic helper for tiled buffers.
>>> I'm afraid that won't help, at least not without porting big chunks of
>>> https://gitlab.freedesktop.org/mesa/mesa/tree/master/src/amd/addrlib
>>> into the kernel, none of which will be used for anything else.
>>>
>>>
>>>>>> There would need to be a mechanism for switching scanout to a linear,
>>>>>> CPU accessible framebuffer.
>>>>> I suppose panic_vmap() could just provide a linear temp buffer
>>>>> to the panic handler, and panic_unmap() could copy the contents
>>>>> over to the real fb.
>>> Copy how? Using a GPU engine?
>> CPU maybe? Though I suppose that won't work if the buffer isn't CPU
>> accesible :/
>
> Well we do have a debug path for accessing invisible memory with the CPU.
>
> E.g. three registers: DATA and auto increment OFFSET_LO/HI. So you can
> just read/write DATA over and over again if you want to access some memory.
Right. I assume that'll be very slow, but I guess it could do when the
memory isn't directly CPU accessible.
> But turning of tilling etc is still extremely tricky when the system is
> already unstable.
Maybe we could add a little hook to the display code, which just
disables tiling for scanout and maybe disables non-primary planes, but
doesn't touch anything else. Harry / Nicholas, does that seem feasible?
I'm coming around from "this is never going to work" to "it might
actually work" with our hardware...
--
Earthling Michel Dänzer | https://www.amd.com
Libre software enthusiast | Mesa and X developer
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 1/3] drm: Add support for panic message output
On 2019-03-12 6:15 p.m., Noralf Trønnes wrote:
>
>
> Den 12.03.2019 17.17, skrev Ville Syrjälä:
>> On Tue, Mar 12, 2019 at 11:47:04AM +0100, Michel Dänzer wrote:
>>> On 2019-03-11 6:42 p.m., Noralf Trønnes wrote:
>>>> This adds support for outputting kernel messages on panic().
>>>> A kernel message dumper is used to dump the log. The dumper iterates
>>>> over each DRM device and it's crtc's to find suitable framebuffers.
>>>>
>>>> All the other dumpers are run before this one except mtdoops.
>>>> Only atomic drivers are supported.
>>>>
>>>> Signed-off-by: Noralf Trønnes
>>>> ---
>>>> [...]
>>>>
>>>> diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
>>>> index f0b34c977ec5..f3274798ecfe 100644
>>>> --- a/include/drm/drm_framebuffer.h
>>>> +++ b/include/drm/drm_framebuffer.h
>>>> @@ -94,6 +94,44 @@ struct drm_framebuffer_funcs {
>>>> struct drm_file *file_priv, unsigned flags,
>>>> unsigned color, struct drm_clip_rect *clips,
>>>> unsigned num_clips);
>>>> +
>>>> + /**
>>>> + * @panic_vmap:
>>>> + *
>>>> + * Optional callback for panic handling.
>>>> + *
>>>> + * For vmapping the selected framebuffer in a panic context. Must
>>>> + * be super careful about locking (only trylocking allowed).
>>>> + *
>>>> + * RETURNS:
>>>> + *
>>>> + * NULL if it didn't work out, otherwise an opaque cookie which is
>>>> + * passed to @panic_draw_xy. It can be anything: vmap area, structure
>>>> + * with more details, just a few flags, ...
>>>> + */
>>>> + void *(*panic_vmap)(struct drm_framebuffer *fb);
>>>
>>> FWIW, the panic_vmap hook cannot work in general with the amdgpu/radeon
>>> drivers:
>>>
>>> Framebuffers are normally tiled, writing to them with the CPU results in
>>> garbled output.
>>>
>
> In which case the driver needs to support the ->panic_draw_xy callback,
> or maybe it's possible to make a generic helper for tiled buffers.
I'm afraid that won't help, at least not without porting big chunks of
https://gitlab.freedesktop.org/mesa/mesa/tree/master/src/amd/addrlib
into the kernel, none of which will be used for anything else.
>>> There would need to be a mechanism for switching scanout to a linear,
>>> CPU accessible framebuffer.
>>
>> I suppose panic_vmap() could just provide a linear temp buffer
>> to the panic handler, and panic_unmap() could copy the contents
>> over to the real fb.
Copy how? Using a GPU engine?
--
Earthling Michel Dänzer | https://www.amd.com
Libre software enthusiast | Mesa and X developer
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 1/3] drm: Add support for panic message output
On 2019-03-11 6:42 p.m., Noralf Trønnes wrote:
> This adds support for outputting kernel messages on panic().
> A kernel message dumper is used to dump the log. The dumper iterates
> over each DRM device and it's crtc's to find suitable framebuffers.
>
> All the other dumpers are run before this one except mtdoops.
> Only atomic drivers are supported.
>
> Signed-off-by: Noralf Trønnes
> ---
> [...]
>
> diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
> index f0b34c977ec5..f3274798ecfe 100644
> --- a/include/drm/drm_framebuffer.h
> +++ b/include/drm/drm_framebuffer.h
> @@ -94,6 +94,44 @@ struct drm_framebuffer_funcs {
>struct drm_file *file_priv, unsigned flags,
>unsigned color, struct drm_clip_rect *clips,
>unsigned num_clips);
> +
> + /**
> + * @panic_vmap:
> + *
> + * Optional callback for panic handling.
> + *
> + * For vmapping the selected framebuffer in a panic context. Must
> + * be super careful about locking (only trylocking allowed).
> + *
> + * RETURNS:
> + *
> + * NULL if it didn't work out, otherwise an opaque cookie which is
> + * passed to @panic_draw_xy. It can be anything: vmap area, structure
> + * with more details, just a few flags, ...
> + */
> + void *(*panic_vmap)(struct drm_framebuffer *fb);
FWIW, the panic_vmap hook cannot work in general with the amdgpu/radeon
drivers:
Framebuffers are normally tiled, writing to them with the CPU results in
garbled output.
With a discrete GPU having a large amount of VRAM, the framebuffer may
not be directly CPU accessible at all.
There would need to be a mechanism for switching scanout to a linear,
CPU accessible framebuffer.
--
Earthling Michel Dänzer | https://www.amd.com
Libre software enthusiast | Mesa and X developer
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] xf86-video-intel: 2 commits - src/sna/sna_blt.c src/sna/sna_display.c
On 2019-03-01 7:22 p.m., GitLab Mirror wrote: > src/sna/sna_blt.c |7 +-- > src/sna/sna_display.c | 20 ++-- > 2 files changed, 11 insertions(+), 16 deletions(-) > > New commits: > commit 6afed33b2d673d88674f0c76efe500ae414e8e1b > Author: Ville Syrjälä > Date: Fri Mar 1 11:26:07 2019 + > > sna: Switch back to hwcursor on the next cursor update > > Once we've switched to using the swcursor (possibly > due to the cursor ioctl failing) we currently keep > using the swcursor until the modeset. > > That's not particularly great as the swcursor has several > issues. Apart from the (presumably expected) flicker, > the cursor also tends to leave horrible trails behind > around dri2/3 windows (happens with tearfree at least). Sounds like the intel driver is allowing DRI2/Present page flipping while there's an SW cursor? That cannot work correctly, as the SW cursor image is missing in the newly flipped pixmap, and the SW cursor code will restore stale background contents to it when the cursor has to be drawn again. -- Earthling Michel Dänzer | https://www.amd.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t] igt: add timeline test cases
On 2018-12-07 11:01 a.m., Chunming Zhou wrote: > Signed-off-by: Chunming Zhou > --- > include/drm-uapi/drm.h | 33 ++ > lib/igt_syncobj.c| 204 +++ > lib/igt_syncobj.h| 19 + > tests/gem_ctx_bad_exec | Bin 0 -> 1284384 bytes Please run git rm tests/gem_ctx_bad_exec and re-send the patch. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/amdgpu: Use per-device driver_features to disable atomic
On 2018-09-13 6:31 p.m., Ville Syrjala wrote:
> From: Ville Syrjälä
>
> Disable atomic on a per-device basis instead of for all devices.
> Made possible by the new device.driver_features thing.
>
> Cc: Alex Deucher
> Cc: "Christian König"
> Cc: "David (ChunMing) Zhou"
> Cc: Harry Wentland
> Cc: Michel Dänzer
> Suggested-by: Michel Dänzer
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 12
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 6870909da926..8c1db96be070 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -816,17 +816,13 @@ static int amdgpu_pci_probe(struct pci_dev *pdev,
> if (ret)
> return ret;
>
> - /* warn the user if they mix atomic and non-atomic capable GPUs */
> - if ((kms_driver.driver_features & DRIVER_ATOMIC) && !supports_atomic)
> - DRM_ERROR("Mixing atomic and non-atomic capable GPUs!\n");
> - /* support atomic early so the atomic debugfs stuff gets created */
> - if (supports_atomic)
> - kms_driver.driver_features |= DRIVER_ATOMIC;
> -
> dev = drm_dev_alloc(_driver, >dev);
> if (IS_ERR(dev))
> return PTR_ERR(dev);
>
> + if (!supports_atomic)
> + dev->driver_features &= ~DRIVER_ATOMIC;
> +
> ret = pci_enable_device(pdev);
> if (ret)
> goto err_free;
> @@ -1078,7 +1074,7 @@ amdgpu_get_crtc_scanout_position(struct drm_device
> *dev, unsigned int pipe,
>
> static struct drm_driver kms_driver = {
> .driver_features =
> - DRIVER_USE_AGP |
> + DRIVER_USE_AGP | DRIVER_ATOMIC |
> DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED | DRIVER_GEM |
> DRIVER_PRIME | DRIVER_RENDER | DRIVER_MODESET | DRIVER_SYNCOBJ,
> .load = amdgpu_driver_load_kms,
>
Thanks Ville for taking care of this!
Reviewed-by: Michel Dänzer
If Alex agrees, probably best to push this to drm-misc-next as well.
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm: Introduce per-device driver_features
On 2018-09-13 4:29 p.m., Ville Syrjälä wrote: > On Thu, Sep 13, 2018 at 03:50:01PM +0200, Daniel Vetter wrote: >> On Thu, Sep 13, 2018 at 04:16:21PM +0300, Ville Syrjala wrote: >>> From: Ville Syrjälä >>> >>> We wish to control certain driver_features flags on a per-device basis >>> while still sharing a single drm_driver instance across all the >>> devices. To that end introduce device.driver_features. By default >>> it will be set to ~0 to not impose any limits beyond >>> driver.driver_features. Drivers can then clear specific flags >>> in the per-device bitmask to limit the capabilities of the device. >>> >>> An alternative approach would be to copy the driver_features from >>> the driver into the device in drm_dev_init(), however that would >>> require verifying that no driver is currently changing >>> driver.driver_features after drm_dev_init(). Hence the ~0 apporach >>> was easier. >>> >>> Ideally we'd also make drm_driver const but there is plenty of code >>> left that wants to mutate it (eg. various vfunc assignments). We'll >>> need to fix all that up before we can make it const. >>> >>> And while at it fix up the type of the feature flag passed to >>> drm_core_check_feature(). >>> >>> v2: Streamline the && vs. & (Chris) >>> s/int/u32/ in drm_core_check_feature() args >>> >>> Cc: Chris Wilson >>> Signed-off-by: Ville Syrjälä >> >> git grep DRIVER_ATOMIC -- drivers/gpu/drm/nouveau has a 2nd supporting >> case for this. Exactly same problem as we have here. Would be good to also >> convert that one, for a bit of OCD. > > Thanks for pointing it out. I'll cook it up and send separately after > this lands. I don't suppose you'd like to do amdgpu as well, while you're at it? :) Either way, this is awesome stuff! This patch is Reviewed-by: Michel Dänzer -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 1/3] drm: Add the optional .fb_modifier() hook
On 2018-03-13 04:20 PM, Daniel Vetter wrote: > On Tue, Mar 13, 2018 at 03:38:38PM +0100, Michel Dänzer wrote: >> On 2018-03-13 03:28 PM, Ville Syrjala wrote: >>> From: Ville Syrjälä <ville.syrj...@linux.intel.com> >>> >>> To make it possible for the core to check the fb pixel format and >>> modifier, we need to first ask the driver to deduce the modifier >>> when the request does not explicitly specify one. >>> >>> Add a new .fb_modifier() hook for that purpose and convert i915 >>> and vc4 to make use if it. All other drivers seem to currently >>> assume linear when the request does not specify anything else, >>> [...] >> >> That's not true at least for the amdgpu and radeon drivers. The tiling >> mode is communicated via BO metadata. > > But atm amdgpu and radeon also don't support explicit modifiers in the > kernel driver, so it again all checks out. Or should at least. Sounds like I misunderstood that this is trying to guess modifiers for all drivers. So far, so good if so. > Once you add modifier support, you need to wire up all the bits and the > rigth default selection, and it should again pan out. Well, this change allows a driver not to wire up the fb_modifier hook, and just assumes linear in that case. Seems like an accident waiting to happen, but I'll leave it to you guys. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 1/3] drm: Add the optional .fb_modifier() hook
On 2018-03-13 04:12 PM, Ville Syrjälä wrote: > On Tue, Mar 13, 2018 at 03:38:38PM +0100, Michel Dänzer wrote: >> On 2018-03-13 03:28 PM, Ville Syrjala wrote: >>> From: Ville Syrjälä <ville.syrj...@linux.intel.com> >>> >>> To make it possible for the core to check the fb pixel format and >>> modifier, we need to first ask the driver to deduce the modifier >>> when the request does not explicitly specify one. >>> >>> Add a new .fb_modifier() hook for that purpose and convert i915 >>> and vc4 to make use if it. All other drivers seem to currently >>> assume linear when the request does not specify anything else, >>> [...] >> >> That's not true at least for the amdgpu and radeon drivers. The tiling >> mode is communicated via BO metadata. > > Well, at least those driver don't do any bo tiling->modifier conversion. > Radeon doesn't do modifiers I suppose, so that part is probably OK. > amggpu might just be broken, not sure. amdgpu certainly wasn't broken before modifiers came along. If something broke with those, I'd say it's more accurate to say that's broken and needs to be fixed, rather than adding more incorrect assumptions based on it. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 1/3] drm: Add the optional .fb_modifier() hook
On 2018-03-13 03:28 PM, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrj...@linux.intel.com> > > To make it possible for the core to check the fb pixel format and > modifier, we need to first ask the driver to deduce the modifier > when the request does not explicitly specify one. > > Add a new .fb_modifier() hook for that purpose and convert i915 > and vc4 to make use if it. All other drivers seem to currently > assume linear when the request does not specify anything else, > [...] That's not true at least for the amdgpu and radeon drivers. The tiling mode is communicated via BO metadata. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/5] Fix deadlock on runtime suspend in DRM drivers
On 2018-02-14 03:08 PM, Sean Paul wrote: > On Wed, Feb 14, 2018 at 10:26:35AM +0100, Maarten Lankhorst wrote: >> Op 14-02-18 om 09:46 schreef Lukas Wunner: >>> Dear drm-misc maintainers, >>> >>> On Sun, Feb 11, 2018 at 10:38:28AM +0100, Lukas Wunner wrote: >>>> Fix a deadlock on hybrid graphics laptops that's been present since 2013: >>> This series has been reviewed, consent has been expressed by the most >>> interested parties, patch [1/5] which touches files outside drivers/gpu >>> has been acked and I've just out a v2 addressing the only objection >>> raised. My plan is thus to wait another two days for comments and, >>> barring further objections, push to drm-misc this weekend. >>> >>> However I'm struggling with the decision whether to push to next or >>> fixes. The series is marked for stable, however the number of >>> affected machines is limited and for an issue that's been present >>> for 5 years it probably doesn't matter if it soaks another two months >>> in linux-next befor it gets backported. Hence I tend to err on the >>> side of caution and push to next, however a case could be made that >>> fixes is more appropriate. >>> >>> I'm lacking experience making such decisions and would be interested >>> to learn how you'd handle this. >>> >>> Thanks, >>> >>> Lukas >> >> I would say fixes, it doesn't look particularly scary. :) > > Agreed. If it's good enough for stable, it's good enough for -fixes! It's not that simple, is it? Fast-tracking patches (some of which appear to be untested) to stable without an immediate cause for urgency seems risky to me. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH RFC 8/9] drm/i915: Allow default context priority to be set via cgroup parameter
On 2018-01-20 11:40 AM, Chris Wilson wrote: > > Along this vein, it's worthwhile pointing out that the current scheduler > is not even close to being the cgroup-enabled CFS implementation it > needs to be to call itself a scheduler. (It's more or less a no-op > scheduler.) It may be premature to start exposing hooks into it. Sounds like it may not be too late to abandon the separate i915 scheduler in favour of the common one used by amdgpu and etnaviv? -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC PATCH] drm/vblanks: Deal with HW vblank counter resets.
On 07/11/17 11:50 AM, Ville Syrjälä wrote: > On Tue, Nov 07, 2017 at 10:47:00AM +0100, Michel Dänzer wrote: >> On 07/11/17 07:26 AM, Dhinakaran Pandiyan wrote: >>> Some HW vblank counters reset due to power management events, which messes >>> up the vblank counting logic. This leads to screen freezes with user space >>> waiting on vblank events that may not occur if the counter keeps resetting. >>> >>> For e.g., After the HW vblank counter resets >>> [9.007359] [drm:drm_update_vblank_count [drm]] updating vblank count >>> on crtc 0: current=297, diff=4294965389, hw=5 hw_last=1912 >>> >>> So, fall back to the SW counter, computed using vblank timestamps >>> and frame duration, when the HW counter value deviates by 50% of the SW >>> computed value. >>> >>> I have tested this patch on my SKL laptop with i915.enable_psr=1 and it >>> *seems* to solve the screen freeze issue seen with PSR when DMC is loaded. >>> >>> Known issues: >>> 1) The 50% deviation margin is arbitrary. >>> 2) "Redundant vblirq ignored" messages are more frequent. >>> >>> I am sending this as an RFC to get feedback on whether the fall back >>> approach is sane and if it should be implemented in the core. >> >> Is there no way for the driver to know under which circumstances the >> reset to 0 might happen? If there is, maybe it could be solved by >> calling drm_crtc_vblank_off() before it might happen and >> drm_crtc_vblank_on() after it might have happened. >> >> Otherwise, might it be better not to use the HW counter at all when it's >> known not to be reliable? > > Yeah, I think we could just avoid using the HW counter whenever there's > a possibility of PSR being used on the crtc. > > Assuming we still want to use the HW counter on crtcs that can't do PSR, > we're going to need some kind of per-crtc mechanism to tell drm_vblank.c > which method to use. hwmode.private_flags is one option. We already use > it for choosing between the scanline counter and hardware timestamps when > calculating the scanout position. But in this case the flag wouldn't be > exactly private since drm_vblank.c would have to know about it as well. > If that is deemed to be a problem, then we might just need to add a bool > to struct drm_vblank_crtc to indicate that the hw counter is good, or > maybe even move the dev->max_vblank_count to live inside > struct drm_vblank_crtc. Or just allow setting struct drm_vblank_crtc's get_vblank_counter member to NULL? -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC PATCH] drm/vblanks: Deal with HW vblank counter resets.
On 07/11/17 07:26 AM, Dhinakaran Pandiyan wrote: > Some HW vblank counters reset due to power management events, which messes > up the vblank counting logic. This leads to screen freezes with user space > waiting on vblank events that may not occur if the counter keeps resetting. > > For e.g., After the HW vblank counter resets > [9.007359] [drm:drm_update_vblank_count [drm]] updating vblank count > on crtc 0: current=297, diff=4294965389, hw=5 hw_last=1912 > > So, fall back to the SW counter, computed using vblank timestamps > and frame duration, when the HW counter value deviates by 50% of the SW > computed value. > > I have tested this patch on my SKL laptop with i915.enable_psr=1 and it > *seems* to solve the screen freeze issue seen with PSR when DMC is loaded. > > Known issues: > 1) The 50% deviation margin is arbitrary. > 2) "Redundant vblirq ignored" messages are more frequent. > > I am sending this as an RFC to get feedback on whether the fall back > approach is sane and if it should be implemented in the core. Is there no way for the driver to know under which circumstances the reset to 0 might happen? If there is, maybe it could be solved by calling drm_crtc_vblank_off() before it might happen and drm_crtc_vblank_on() after it might have happened. Otherwise, might it be better not to use the HW counter at all when it's known not to be reliable? -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH xf86-video-amdgpu] Adapt to PixmapDirtyUpdateRec::src being a DrawablePtr
On 13/07/17 05:27 PM, Michel Dänzer wrote: > On 18/04/17 07:07 PM, Michel Dänzer wrote: >> From: Michel Dänzer <michel.daen...@amd.com> >> >> Signed-off-by: Michel Dänzer <michel.daen...@amd.com> >> --- >> >> Chris / Ilia / Ben, this should be manageable for the intel/nouveau >> drivers, right? > > Any feedback, guys? > > I want to push the xserver change soon-ish. I could probably come up > with a corresponding patch for nouveau in the worst case, but I'm afraid > not for intel. Chris, in response to your bugzilla comment https://bugs.freedesktop.org/show_bug.cgi?id=100086#c9 and patch https://patchwork.freedesktop.org/patch/143180/, I drafted how the ABI could be fixed to reflect current usage in the first comment to your patch. There was no response in a month, so I went ahead and wrote https://patchwork.freedesktop.org/patch/150938/ . After three months with no feedback, I followed up with the ping above. After almost another month without feedback from you, I pinged you about it on IRC, again no response. I'm afraid my patience has run out, I'll push the xserver patch next week. Until the xf86-video-intel SNA driver is fixed up, it'll continue being broken as the master screen driver with RandR 1.4 slave scanout. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/4] drm: Only lastclose on unload for legacy drivers
On 03/08/17 10:54 PM, Daniel Vetter wrote: > On Thu, Aug 3, 2017 at 1:17 AM, Daniel Vetter <daniel.vet...@ffwll.ch> wrote: >> On Wed, Aug 2, 2017 at 10:50 PM, Alex Deucher <alexdeuc...@gmail.com> wrote: >>> On Wed, Aug 2, 2017 at 7:56 AM, Daniel Vetter <daniel.vet...@ffwll.ch> >>> wrote: >>>> The only thing modern drivers are supposed to do in lastclose is >>>> restore the fb emulation state. Which is entirely optional, and >>>> there's really no reason to do that. So restrict it to legacy drivers >>>> (where the driver cleanup essentially happens in lastclose). >>> >>> vga_switcheroo_process_delayed_switch() gets called in lastclose. >>> Won't that need to get moved elsewhere for this to work? >> >> Hm right, I forgot the lazy way to do runtime pm by keeping the device >> alive as long as anyone has an open fd for it ... This shouldn't be a >> problem, since you need to unregister from vgaswitcheroo anyway on >> unload. Maybe that blows up, I'll check the code and augment the patch >> as needed. > > So I think there's 3 cases: > - Trying to unload the module. You can't do that while anyone has the > fd still open, so lastclose is guaranteeed to run. > - Forcefully unbinding the driver through sysfs. Not any better, since > the can_switch stuff checks for the open count, and so will delay the > delayed switch no matter what. Are you sure that this is working as intended? https://bugs.freedesktop.org/show_bug.cgi?id=100399 sounds like unbinding works even while Xorg is using the DRM device. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [Nouveau] [PATCH xf86-video-amdgpu] Adapt to PixmapDirtyUpdateRec::src being a DrawablePtr
On 14/07/17 11:24 AM, Ilia Mirkin wrote: > On Thu, Jul 13, 2017 at 10:14 PM, Michel Dänzer <mic...@daenzer.net> wrote: >> On 13/07/17 09:31 PM, Ilia Mirkin wrote: >>> On Thu, Jul 13, 2017 at 4:27 AM, Michel Dänzer <mic...@daenzer.net> wrote: >>>> On 18/04/17 07:07 PM, Michel Dänzer wrote: >>>>> From: Michel Dänzer <michel.daen...@amd.com> >>>>> >>>>> Signed-off-by: Michel Dänzer <michel.daen...@amd.com> >>>>> --- >>>>> >>>>> Chris / Ilia / Ben, this should be manageable for the intel/nouveau >>>>> drivers, right? >>>> >>>> Any feedback, guys? >>>> >>>> I want to push the xserver change soon-ish. I could probably come up >>>> with a corresponding patch for nouveau in the worst case, but I'm afraid >>>> not for intel. >>> >>> Sorry, I'm not 1000% clear on what this patch is doing. >> >> 100% should suffice. ;) >> >>> Is it literally just a type change from A to B and so code has to be fixed >>> up for the new API? >> >> That's basically it, corresponding to >> https://patchwork.freedesktop.org/patch/150938/ . > > From a brief glance at nouveau, it never uses PixmapDirtyUpdate, > except in src/nv_driver.c where I don't think the code would need to > be updated. > > https://cgit.freedesktop.org/nouveau/xf86-video-nouveau/tree/src/nv_driver.c#n552 There's also PixmapStart/StopDirtyTracking, though it looks like those might happen to work as well at runtime, if you don't mind the compiler warnings. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [Nouveau] [PATCH xf86-video-amdgpu] Adapt to PixmapDirtyUpdateRec::src being a DrawablePtr
On 13/07/17 09:31 PM, Ilia Mirkin wrote: > On Thu, Jul 13, 2017 at 4:27 AM, Michel Dänzer <mic...@daenzer.net> wrote: >> On 18/04/17 07:07 PM, Michel Dänzer wrote: >>> From: Michel Dänzer <michel.daen...@amd.com> >>> >>> Signed-off-by: Michel Dänzer <michel.daen...@amd.com> >>> --- >>> >>> Chris / Ilia / Ben, this should be manageable for the intel/nouveau >>> drivers, right? >> >> Any feedback, guys? >> >> I want to push the xserver change soon-ish. I could probably come up >> with a corresponding patch for nouveau in the worst case, but I'm afraid >> not for intel. > > Sorry, I'm not 1000% clear on what this patch is doing. 100% should suffice. ;) > Is it literally just a type change from A to B and so code has to be fixed > up for the new API? That's basically it, corresponding to https://patchwork.freedesktop.org/patch/150938/ . -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH xf86-video-amdgpu] Adapt to PixmapDirtyUpdateRec::src being a DrawablePtr
On 18/04/17 07:07 PM, Michel Dänzer wrote: > From: Michel Dänzer <michel.daen...@amd.com> > > Signed-off-by: Michel Dänzer <michel.daen...@amd.com> > --- > > Chris / Ilia / Ben, this should be manageable for the intel/nouveau > drivers, right? Any feedback, guys? I want to push the xserver change soon-ish. I could probably come up with a corresponding patch for nouveau in the worst case, but I'm afraid not for intel. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [Nouveau] [PATCH 01/11] drm/fb-helper: do a generic fb_setcmap helper in terms of crtc .gamma_set
On 21/06/17 05:14 PM, Daniel Vetter wrote:
> On Wed, Jun 21, 2017 at 04:59:31PM +0900, Michel Dänzer wrote:
>> On 21/06/17 04:38 PM, Daniel Vetter wrote:
>>> On Tue, Jun 20, 2017 at 09:25:25PM +0200, Peter Rosin wrote:
>>>> This makes the redundant fb helpers .load_lut, .gamma_set and .gamma_get
>>>> totally obsolete.
>>>>
>>>> I think the gamma_store can end up invalid on error. But the way I read
>>>> it, that can happen in drm_mode_gamma_set_ioctl as well, so why should
>>>> this pesky legacy fbdev stuff be any better?
>>>>
>>>> drm_fb_helper_save_lut_atomic justs saves the gamma lut for later. However,
>>>> it saves it to the gamma_store which should already be up to date with what
>>>> .gamma_get would return and is thus a nop. So, zap it.
>>>
>>> Removing drm_fb_helper_save_lut_atomic should be a separate patch I
>>> think.
>>>
>>>> Signed-off-by: Peter Rosin <p...@axentia.se>
>>
>> [...]
>>
>>>> @@ -1167,50 +1150,6 @@ void drm_fb_helper_set_suspend_unlocked(struct
>>>> drm_fb_helper *fb_helper,
>>>> }
>>>> EXPORT_SYMBOL(drm_fb_helper_set_suspend_unlocked);
>>>>
>>>> -static int setcolreg(struct drm_crtc *crtc, u16 red, u16 green,
>>>> - u16 blue, u16 regno, struct fb_info *info)
>>>> -{
>>>> - struct drm_fb_helper *fb_helper = info->par;
>>>> - struct drm_framebuffer *fb = fb_helper->fb;
>>>> -
>>>> - if (info->fix.visual == FB_VISUAL_TRUECOLOR) {
>>>
>>> This case here seems gone, and it was also the pièce de résistance when I
>>> tried tackling fbdev lut support. As far as I understand this stuff we do
>>> not support FB_VISUAL_TRUECOLOR palette, and all that bitshifting here is
>>> pointless. But I'm honestly not really clear.
>>>
>>> I think removing this case should also be a separate patch, and I'd very
>>> much prefer that someone with some fbdev-clue would ack it.
>>
>> No need for anyone with fbdev-clue, just run fbset -i. :)
Adding Bartlomiej and the linux-fbdev list, just in case I'm wrong below.
>> fbcon uses a TRUECOLOR visual at depths > 8.
>
> Then I understand even less what exactly this code does ... Should we keep
> it?
I think we need it. It updates the entries of info->pseudo_palette,
which is kind of a pseudocolor palette mapping 16 indices to pixel
values to write to the framebuffer.
If the setcolreg hook is removed, the setcmap hook needs to do this instead.
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 01/11] drm/fb-helper: do a generic fb_setcmap helper in terms of crtc .gamma_set
On 21/06/17 04:38 PM, Daniel Vetter wrote:
> On Tue, Jun 20, 2017 at 09:25:25PM +0200, Peter Rosin wrote:
>> This makes the redundant fb helpers .load_lut, .gamma_set and .gamma_get
>> totally obsolete.
>>
>> I think the gamma_store can end up invalid on error. But the way I read
>> it, that can happen in drm_mode_gamma_set_ioctl as well, so why should
>> this pesky legacy fbdev stuff be any better?
>>
>> drm_fb_helper_save_lut_atomic justs saves the gamma lut for later. However,
>> it saves it to the gamma_store which should already be up to date with what
>> .gamma_get would return and is thus a nop. So, zap it.
>
> Removing drm_fb_helper_save_lut_atomic should be a separate patch I
> think.
>
>> Signed-off-by: Peter Rosin <p...@axentia.se>
[...]
>> @@ -1167,50 +1150,6 @@ void drm_fb_helper_set_suspend_unlocked(struct
>> drm_fb_helper *fb_helper,
>> }
>> EXPORT_SYMBOL(drm_fb_helper_set_suspend_unlocked);
>>
>> -static int setcolreg(struct drm_crtc *crtc, u16 red, u16 green,
>> - u16 blue, u16 regno, struct fb_info *info)
>> -{
>> -struct drm_fb_helper *fb_helper = info->par;
>> -struct drm_framebuffer *fb = fb_helper->fb;
>> -
>> -if (info->fix.visual == FB_VISUAL_TRUECOLOR) {
>
> This case here seems gone, and it was also the pièce de résistance when I
> tried tackling fbdev lut support. As far as I understand this stuff we do
> not support FB_VISUAL_TRUECOLOR palette, and all that bitshifting here is
> pointless. But I'm honestly not really clear.
>
> I think removing this case should also be a separate patch, and I'd very
> much prefer that someone with some fbdev-clue would ack it.
No need for anyone with fbdev-clue, just run fbset -i. :) fbcon uses a
TRUECOLOR visual at depths > 8.
> It's a pre-existing bug, but should we also try to restore the fbdev lut
> in drm_fb_helper_restore_fbdev_mode_unlocked()? Would be yet another bug,
> but might be relevant for your use-case. Just try to run both an fbdev
> application and some kms-native thing, and then SIGKILL the native kms
> app.
>
> But since pre-existing not really required, and probably too much effort.
I suspect something like that indeed needs to be done though. E.g.
killing Xorg results in fbcon continuing to use the LUT set by Xorg.
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: Fix deadlock retry loop in page_flip_ioctl
On 22/05/17 10:59 PM, Daniel Vetter wrote:
> I failed to properly onion-wrap the unwind code: We acquire the vblank
> reference before we start with the wait-wound locking dance, hence we
> must make sure we retry before we drop the reference. Oops.
>
> v2: The vblank_put must be after the frambuffer_put (Michel). I suck at
> unwrapping code that doesn't use separate labels for each stage, but
> checks each pointer first ... While re-reading everything I also
> realized that we must clean up the fb refcounts, and specifically
> plane->old_fb before we drop the locks, either in the final unlocking,
> or in the w/w retry path. Hence the correct fix is to drop the
> vblank_put to the very bottom.
>
> Fixes: 29dc0d1de182 ("drm: Roll out acquire context for the page_flip ioctl")
> Cc: Harry Wentland <harry.wentl...@amd.com>
> Cc: Daniel Vetter <daniel.vet...@intel.com>
> Cc: Jani Nikula <jani.nik...@linux.intel.com>
> Cc: Sean Paul <seanp...@chromium.org>
> Cc: David Airlie <airl...@linux.ie>
> Cc: dri-de...@lists.freedesktop.org
> Reported-by: Tommi Rantala <tt.rant...@gmail.com>
> Cc: Tommi Rantala <tt.rant...@gmail.com>
> Cc: Michel Dänzer <mic...@daenzer.net>
> Signed-off-by: Daniel Vetter <daniel.vet...@intel.com>
> ---
> drivers/gpu/drm/drm_plane.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index fedd4d60d9cd..5dc8c4350602 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -948,8 +948,6 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> }
>
> out:
> - if (ret && crtc->funcs->page_flip_target)
> - drm_crtc_vblank_put(crtc);
> if (fb)
> drm_framebuffer_put(fb);
> if (crtc->primary->old_fb)
> @@ -964,5 +962,8 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> drm_modeset_drop_locks();
> drm_modeset_acquire_fini();
>
> + if (ret && crtc->funcs->page_flip_target)
> + drm_crtc_vblank_put(crtc);
> +
> return ret;
> }
>
Reviewed-by: Michel Dänzer <michel.daen...@amd.com>
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: Fix deadlock retry loop in page_flip_ioctl
On 22/05/17 04:31 PM, Daniel Vetter wrote:
> I failed to properly onion-wrap the unwind code: We acquire the vblank
> reference before we start with the wait-wound locking dance, hence we
> must make sure we retry before we drop the reference. Oops.
>
> Fixes: 29dc0d1de182 ("drm: Roll out acquire context for the page_flip ioctl")
> Cc: Harry Wentland <harry.wentl...@amd.com>
> Cc: Daniel Vetter <daniel.vet...@intel.com>
> Cc: Jani Nikula <jani.nik...@linux.intel.com>
> Cc: Sean Paul <seanp...@chromium.org>
> Cc: David Airlie <airl...@linux.ie>
> Cc: dri-de...@lists.freedesktop.org
> Reported-by: Tommi Rantala <tt.rant...@gmail.com>
> Cc: Tommi Rantala <tt.rant...@gmail.com>
> Cc: Michel Dänzer <mic...@daenzer.net>
> Signed-off-by: Daniel Vetter <daniel.vet...@intel.com>
> ---
> drivers/gpu/drm/drm_plane.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index fedd4d60d9cd..24c8a611b98a 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -948,6 +948,11 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> }
>
> out:
> + if (ret == -EDEADLK) {
> + drm_modeset_backoff();
> + goto retry;
> + }
> +
> if (ret && crtc->funcs->page_flip_target)
> drm_crtc_vblank_put(crtc);
> if (fb)
> @@ -956,11 +961,6 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> drm_framebuffer_put(crtc->primary->old_fb);
> crtc->primary->old_fb = NULL;
>
> - if (ret == -EDEADLK) {
> - drm_modeset_backoff();
> - goto retry;
> - }
I suspect the drm_framebuffer_put(fb) call still needs to happen before
the retry?
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH xserver] Make PixmapDirtyUpdateRec::src a DrawablePtr
From: Michel Dänzer <michel.daen...@amd.com>
This allows making the master screen's pixmap_dirty_list entries
explicitly reflect that we're now tracking the root window instead of
the screen pixmap, in order to allow Present page flipping on master
outputs while there are active slave outputs.
Define HAS_DIRTYTRACKING_DRAWABLE_SRC for drivers to check, but leave
HAS_DIRTYTRACKING_ROTATION defined as well to make things slightly
easier for drivers.
Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
---
The Start/StopFlippingPixmapTracking related changes are only compile
tested. The rest is smoke tested with the modesetting, amdgpu and radeon
drivers.
dix/pixmap.c | 24 +++-
hw/xfree86/drivers/modesetting/driver.c | 10 +-
hw/xfree86/drivers/modesetting/drmmode_display.c | 5 +++--
hw/xfree86/drivers/modesetting/drmmode_display.h | 2 +-
include/pixmap.h | 5 +++--
include/pixmapstr.h | 3 ++-
include/scrnintstr.h | 6 +++---
randr/randrstr.h | 2 +-
randr/rrcrtc.c | 20 +---
9 files changed, 38 insertions(+), 39 deletions(-)
diff --git a/dix/pixmap.c b/dix/pixmap.c
index b67a2e8a6..81ac5e2d8 100644
--- a/dix/pixmap.c
+++ b/dix/pixmap.c
@@ -181,12 +181,12 @@ PixmapDirtyDamageDestroy(DamagePtr damage, void *closure)
}
Bool
-PixmapStartDirtyTracking(PixmapPtr src,
+PixmapStartDirtyTracking(DrawablePtr src,
PixmapPtr slave_dst,
int x, int y, int dst_x, int dst_y,
Rotation rotation)
{
-ScreenPtr screen = src->drawable.pScreen;
+ScreenPtr screen = src->pScreen;
PixmapDirtyUpdatePtr dirty_update;
RegionPtr damageregion;
RegionRec dstregion;
@@ -204,8 +204,7 @@ PixmapStartDirtyTracking(PixmapPtr src,
dirty_update->dst_y = dst_y;
dirty_update->rotation = rotation;
dirty_update->damage = DamageCreate(NULL, PixmapDirtyDamageDestroy,
-DamageReportNone,
-TRUE, src->drawable.pScreen,
+DamageReportNone, TRUE, screen,
dirty_update);
if (rotation != RR_Rotate_0) {
@@ -241,16 +240,15 @@ PixmapStartDirtyTracking(PixmapPtr src,
RegionUnion(damageregion, damageregion, );
RegionUninit();
-DamageRegister(screen->root ? >root->drawable : >drawable,
- dirty_update->damage);
+DamageRegister(src, dirty_update->damage);
xorg_list_add(_update->ent, >pixmap_dirty_list);
return TRUE;
}
Bool
-PixmapStopDirtyTracking(PixmapPtr src, PixmapPtr slave_dst)
+PixmapStopDirtyTracking(DrawablePtr src, PixmapPtr slave_dst)
{
-ScreenPtr screen = src->drawable.pScreen;
+ScreenPtr screen = src->pScreen;
PixmapDirtyUpdatePtr ent, safe;
xorg_list_for_each_entry_safe(ent, safe, >pixmap_dirty_list, ent) {
@@ -269,8 +267,8 @@ PixmapDirtyCopyArea(PixmapPtr dst,
PixmapDirtyUpdatePtr dirty,
RegionPtr dirty_region)
{
-ScreenPtr pScreen = dirty->src->drawable.pScreen;
-DrawablePtr src = pScreen->root ? >root->drawable :
>src->drawable;
+DrawablePtr src = dirty->src;
+ScreenPtr pScreen = src->pScreen;
int n;
BoxPtr b;
GCPtr pGC;
@@ -309,7 +307,7 @@ PixmapDirtyCompositeRotate(PixmapPtr dst_pixmap,
PixmapDirtyUpdatePtr dirty,
RegionPtr dirty_region)
{
-ScreenPtr pScreen = dirty->src->drawable.pScreen;
+ScreenPtr pScreen = dirty->src->pScreen;
PictFormatPtr format = PictureWindowFormat(pScreen->root);
PicturePtr src, dst;
XID include_inferiors = IncludeInferiors;
@@ -318,7 +316,7 @@ PixmapDirtyCompositeRotate(PixmapPtr dst_pixmap,
int error;
src = CreatePicture(None,
->root->drawable,
+dirty->src,
format,
CPSubwindowMode,
_inferiors, serverClient, );
@@ -367,7 +365,7 @@ PixmapDirtyCompositeRotate(PixmapPtr dst_pixmap,
*/
Bool PixmapSyncDirtyHelper(PixmapDirtyUpdatePtr dirty)
{
-ScreenPtr pScreen = dirty->src->drawable.pScreen;
+ScreenPtr pScreen = dirty->src->pScreen;
RegionPtr region = DamageRegion(dirty->damage);
PixmapPtr dst;
SourceValidateProcPtr SourceValidate;
diff --git a/hw/xfree86/drivers/modesetting/driver.c
b/hw/xfree86/drivers/modesetting/driver.c
index d7030e5c2..8ce51a502 100644
--- a/hw/xfree86/drivers/modesetting/driver.c
+++ b/hw/xfree86/drivers/modesetting/driver.c
@@ -1219,12 +1219,1
[Intel-gfx] [PATCH xf86-video-amdgpu] Adapt to PixmapDirtyUpdateRec::src being a DrawablePtr
From: Michel Dänzer <michel.daen...@amd.com>
Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
---
Chris / Ilia / Ben, this should be manageable for the intel/nouveau
drivers, right?
src/amdgpu_drv.h | 26 ++
src/amdgpu_kms.c | 18 +-
src/drmmode_display.c | 8 ++--
3 files changed, 41 insertions(+), 11 deletions(-)
diff --git a/src/amdgpu_drv.h b/src/amdgpu_drv.h
index e5c44dc36..9e088e71a 100644
--- a/src/amdgpu_drv.h
+++ b/src/amdgpu_drv.h
@@ -170,6 +170,32 @@ typedef enum {
#define AMDGPU_PIXMAP_SHARING 1
#define amdgpu_is_gpu_screen(screen) (screen)->isGPU
#define amdgpu_is_gpu_scrn(scrn) (scrn)->is_gpu
+
+static inline ScreenPtr
+amdgpu_dirty_master(PixmapDirtyUpdatePtr dirty)
+{
+#ifdef HAS_DIRTYTRACKING_DRAWABLE_SRC
+ ScreenPtr screen = dirty->src->pScreen;
+#else
+ ScreenPtr screen = dirty->src->drawable.pScreen;
+#endif
+
+ if (screen->current_master)
+ return screen->current_master;
+
+ return screen;
+}
+
+static inline Bool
+amdgpu_dirty_src_equals(PixmapDirtyUpdatePtr dirty, PixmapPtr pixmap)
+{
+#ifdef HAS_DIRTYTRACKING_DRAWABLE_SRC
+ return dirty->src == >drawable;
+#else
+ return dirty->src == pixmap;
+#endif
+}
+
#else
#define amdgpu_is_gpu_screen(screen) 0
#define amdgpu_is_gpu_scrn(scrn) 0
diff --git a/src/amdgpu_kms.c b/src/amdgpu_kms.c
index 0182cbe0a..29d3d076f 100644
--- a/src/amdgpu_kms.c
+++ b/src/amdgpu_kms.c
@@ -448,7 +448,7 @@ dirty_region(PixmapDirtyUpdatePtr dirty)
static void
redisplay_dirty(PixmapDirtyUpdatePtr dirty, RegionPtr region)
{
- ScrnInfoPtr scrn = xf86ScreenToScrn(dirty->src->drawable.pScreen);
+ ScrnInfoPtr scrn = xf86ScreenToScrn(dirty->slave_dst->drawable.pScreen);
if (RegionNil(region))
goto out;
@@ -481,12 +481,12 @@ amdgpu_prime_scanout_update_abort(xf86CrtcPtr crtc, void
*event_data)
void
amdgpu_sync_shared_pixmap(PixmapDirtyUpdatePtr dirty)
{
- ScreenPtr master_screen = dirty->src->master_pixmap->drawable.pScreen;
+ ScreenPtr master_screen = amdgpu_dirty_master(dirty);
PixmapDirtyUpdatePtr ent;
RegionPtr region;
xorg_list_for_each_entry(ent, _screen->pixmap_dirty_list, ent) {
- if (ent->slave_dst != dirty->src)
+ if (!amdgpu_dirty_src_equals(dirty, ent->slave_dst))
continue;
region = dirty_region(ent);
@@ -501,7 +501,7 @@ amdgpu_sync_shared_pixmap(PixmapDirtyUpdatePtr dirty)
static Bool
master_has_sync_shared_pixmap(ScrnInfoPtr scrn, PixmapDirtyUpdatePtr dirty)
{
- ScreenPtr master_screen = dirty->src->master_pixmap->drawable.pScreen;
+ ScreenPtr master_screen = amdgpu_dirty_master(dirty);
return master_screen->SyncSharedPixmap != NULL;
}
@@ -517,7 +517,7 @@ slave_has_sync_shared_pixmap(ScrnInfoPtr scrn,
PixmapDirtyUpdatePtr dirty)
static void
call_sync_shared_pixmap(PixmapDirtyUpdatePtr dirty)
{
- ScreenPtr master_screen = dirty->src->master_pixmap->drawable.pScreen;
+ ScreenPtr master_screen = amdgpu_dirty_master(dirty);
master_screen->SyncSharedPixmap(dirty);
}
@@ -527,7 +527,7 @@ call_sync_shared_pixmap(PixmapDirtyUpdatePtr dirty)
static Bool
master_has_sync_shared_pixmap(ScrnInfoPtr scrn, PixmapDirtyUpdatePtr dirty)
{
- ScrnInfoPtr master_scrn =
xf86ScreenToScrn(dirty->src->master_pixmap->drawable.pScreen);
+ ScrnInfoPtr master_scrn = xf86ScreenToScrn(amdgpu_dirty_master(dirty));
return master_scrn->driverName == scrn->driverName;
}
@@ -581,7 +581,7 @@ amdgpu_prime_scanout_do_update(xf86CrtcPtr crtc, unsigned
scanout_id)
Bool ret = FALSE;
xorg_list_for_each_entry(dirty, >pixmap_dirty_list, ent) {
- if (dirty->src == scanoutpix && dirty->slave_dst ==
+ if (amdgpu_dirty_src_equals(dirty, scanoutpix) &&
dirty->slave_dst ==
drmmode_crtc->scanout[scanout_id ^
drmmode_crtc->tear_free].pixmap) {
RegionPtr region;
@@ -738,10 +738,10 @@ amdgpu_dirty_update(ScrnInfoPtr scrn)
PixmapDirtyUpdatePtr region_ent = ent;
if (master_has_sync_shared_pixmap(scrn, ent)) {
- ScreenPtr master_screen =
ent->src->master_pixmap->drawable.pScreen;
+ ScreenPtr master_screen =
amdgpu_dirty_master(ent);
xorg_list_for_each_entry(region_ent,
_screen->pixmap_dirty_list, ent) {
- if (region_ent->slave_dst == ent->src)
+ if (amdgpu_dirty_src_equals(ent,
region_ent->slave_dst))
break;
Re: [Intel-gfx] [PATCH] drm: Document code of conduct
On 11/04/17 03:48 PM, Daniel Vetter wrote: > freedesktop.org has adopted a formal code of conduct: > > https://www.fooishbar.org/blog/fdo-contributor-covenant/ > https://www.freedesktop.org/wiki/CodeOfConduct/ > > Besides formalizing things a bit more I don't think this changes > anything for us, we've already peer-enforced respectful and > constructive interactions since a long time. But it's good to document > things properly. > > Note: As Daniel Stone mentioned in the announcement fd.o admins > started chatting with the communities their hosting, which includs the > X.org foundation board, to figure out how to fan out enforcement and > allow projects to run things on their own (with fd.o still as the > fallback). So the details of enforcement (and appealing decisions) > might still change, but since this involves the board and lots more > people it'll take a while to get there. For now this is good enough I > think. > > For the text itself I went with the same blurb as the Wayland project, > didn't feel creative yet this early in the morning: > > https://cgit.freedesktop.org/wayland/wayland/commit/?id=0eefe99fe0683ae409b665a8b18cc7eb648c6c0c > > Cc: Daniel Stone <dani...@collabora.com> > Cc: Keith Packard <kei...@keithp.com> > Cc: tfh...@err.no > Signed-off-by: Daniel Vetter <daniel.vet...@intel.com> > --- > Documentation/gpu/introduction.rst | 11 +++ > 1 file changed, 11 insertions(+) > > diff --git a/Documentation/gpu/introduction.rst > b/Documentation/gpu/introduction.rst > index 05a82bdfbca4..0f5173e29bdc 100644 > --- a/Documentation/gpu/introduction.rst > +++ b/Documentation/gpu/introduction.rst > @@ -85,3 +85,14 @@ This means that there's a blackout-period of about one > month where feature work > can't be merged. The recommended way to deal with that is having a -next tree > that's always open, but making sure to not feed it into linux-next during the > blackout period. As an example, drm-misc works like that. > + > +Code of Conduct > +--- > + > +As a freedesktop.org project, dri-devel and the DRM community follows the > +Contributor Covenant, found at: > https://www.freedesktop.org/wiki/CodeOfConduct > + > +Please conduct yourself in a respectful and civilised manner when > +interacting with community members on mailing lists, IRC, or bug > +trackers. The community represents the project as a whole, and abusive > +or bullying behaviour is not tolerated by the project. > Acked-by: Michel Dänzer <michel.daen...@amd.com> -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/19] drm/vmwgfx: Drop the cursor locking hack
On 23/03/17 07:32 PM, Thomas Hellstrom wrote: > On 03/23/2017 11:10 AM, Daniel Vetter wrote: >> On Thu, Mar 23, 2017 at 09:35:25AM +0100, Thomas Hellstrom wrote: >>> On 03/23/2017 08:31 AM, Daniel Vetter wrote: >>>> On Thu, Mar 23, 2017 at 08:28:32AM +0100, Daniel Vetter wrote: >>>>> On Thu, Mar 23, 2017 at 07:22:31AM +0100, Thomas Hellstrom wrote: >>>>>> On 03/22/2017 10:50 PM, Daniel Vetter wrote: >>>>>>> It's been around forever, no one bothered to address the FIXME, so I >>>>>>> presume it's all fine. >>>>>>> >>>>>>> Cc: Sinclair Yeh <s...@vmware.com> >>>>>>> Cc: Thomas Hellstrom <thellst...@vmware.com> >>>>>>> Signed-off-by: Daniel Vetter <daniel.vet...@intel.com> >>>>>> NAK. We need to properly address this. Probably as part of the atomic >>>>>> update. >>>>> So could someone with vmwgfx understanding explain this? Note that the >>>>> FIXME was originally added by me years ago, because I wasn't sure (only >>>>> about 90%) that this is safe, and was essentially pleading for a vmwgfx >>>>> expert to review this? >>>>> >>>>> Since it didn't happen I presume it's not that terribly and probably safe >>>>> ... >>>>> >>>>> I'm still 90% sure that this is correct, but I'd love for a vmwgfx to >>>>> audit it. Replying with a NAK is kinda not the response I was hoping for >>>>> (and yes I guess I should have explained what's going on here better, but >>>>> it's just a git blame of the FIXME comment away). >>> So the code has been left in place because it works. Altering it now >>> will create unnecessary merge conflicts with the atomic code, and the >>> change isn't tested and audited which means we need to drop focus from >>> what we're doing and audit and test code that isn't going to be used >>> anyway for not apparent reason? But otoh put in the below context there >>> indeed is a reason. >>> >>> From a quick audit of the existing code it seems like at least >>> vmw_cursor_update_position is touching global device state so I think at >>> a minimum we need to take a spinlock in that function. Otherwise it >>> seems to be safe. >> Note that you're holding the crtc lock already, which gives you exclusion >> against concurrent page_flips, mode_sets and property changes. Note also >> that page_flips themselves also only hold the crtc lock, so you can run >> multiple page_flips in parallel on different crtc (iirc vmwgfx has >> multiple crtc, if not this discussion is entirely moot). >> >> tbh I'd be surprised if my patch really breaks something that hasn't been >> a pre-existing issue for a long time. The original commit which added this >> FIXME comment is from 2012. Note also that because it's a hack, you >> already have a pretty a real race with the core drm state keeping, and no >> one seems to have hit that either. >> >> I mean I can dig through vmwgfx code and do the audit, but it'll take a >> few hours and vmwgfx is it's own world, so much harder to understand (for >> me). >> > > I'm thinking of the situation when someone would call a cursor_set ioctl > in parallell for two crtcs at the same time and race writing the > position registers? > Note that the device has only a single global cursor. > Admittedly the effects of a race would probably be small, but I'd rather > see it being properly protected. Indeed, as long as userspace uses cursor positions (and images) on each CRTC which are consistent with a single cursor in a single framebuffer, it shouldn't matter in which order they write the registers. And if the per-CRTC positions aren't consistent like that, locking won't help either. Strictly speaking, the (virtual) hardware is too limited to support the legacy KMS cursor API. AFAIR e.g. weston at least used to make use of HW cursors for other surfaces, not sure that's currently the case though. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm: Refactor vblank sequence number comparison
On 22/03/17 07:06 PM, Chris Wilson wrote: > Move the repeated (a - b) <= (1 << 23) to its own function. > > v2: Catch the '1<<23' inside drm_handle_vblank() as well > > Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk> Reviewed-by: Michel Dänzer <michel.daen...@amd.com> -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 3/4] drm: Refactor vblank sequence number comparison
On 18/03/17 05:20 AM, Chris Wilson wrote:
> Move the repeated (a - b) <= (1 << 23) to its own function.
>
> Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrj...@linux.intel.com>
> Cc: Daniel Vetter <daniel.vet...@ffwll.ch>
> ---
> drivers/gpu/drm/drm_irq.c | 14 +-
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index a164cf51d093..253505da57bd 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -1492,6 +1492,11 @@ int drm_legacy_modeset_ctl(struct drm_device *dev,
> void *data,
> return 0;
> }
>
> +static inline bool vblank_passed(u32 seq, u32 ref)
> +{
> + return (seq - ref) <= (1 << 23);
> +}
As a followup, maybe we could try changing this to
return (s32)(seq - ref) >= 0;
> @@ -1542,7 +1547,7 @@ static int drm_queue_vblank_event(struct drm_device
> *dev, unsigned int pipe,
> vblwait->request.sequence);
>
> e->event.sequence = vblwait->request.sequence;
> - if ((seq - vblwait->request.sequence) <= (1 << 23)) {
> + if (vblank_passed(seq, vblwait->request.sequence)) {
> drm_vblank_put(dev, pipe);
> send_vblank_event(dev, e, seq, );
> vblwait->reply.sequence = seq;
> @@ -1632,9 +1637,8 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
> }
>
> if ((flags & _DRM_VBLANK_NEXTONMISS) &&
> - (seq - vblwait->request.sequence) <= (1 << 23)) {
> + vblank_passed(seq, vblwait->request.sequence))
> vblwait->request.sequence = seq + 1;
> - }
>
> if (flags & _DRM_VBLANK_EVENT) {
> /* must hold on to the vblank ref until the event fires
> @@ -1647,8 +1651,8 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
> DRM_DEBUG("waiting on vblank count %u, crtc %u\n",
> vblwait->request.sequence, pipe);
> DRM_WAIT_ON(ret, vblank->queue, 3 * HZ,
> - (drm_vblank_count(dev, pipe) -
> - vblwait->request.sequence) <= (1 << 23) ||
> + vblank_passed(drm_vblank_count(dev, pipe),
> + vblwait->request.sequence) ||
> !READ_ONCE(vblank->enabled));
> }
>
>
There's another instance in drm_handle_vblank_events.
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/5] drm/edid: Introduce drm_default_rgb_quant_range()
On 12/01/17 05:00 PM, Daniel Vetter wrote: > On Wed, Jan 11, 2017 at 06:31:52PM +0200, Ville Syrjälä wrote: >> On Wed, Jan 11, 2017 at 05:16:54PM +0100, Daniel Vetter wrote: >>> On Wed, Jan 11, 2017 at 02:57:22PM +0200, ville.syrj...@linux.intel.com >>> wrote: >>>> From: Ville Syrjälä <ville.syrj...@linux.intel.com> >>>> >>>> Make the code selecting the RGB quantization range a little less magicy >>>> by wrapping it up in a small helper. >>>> >>>> Signed-off-by: Ville Syrjälä <ville.syrj...@linux.intel.com> >>> >>> Needs cc: for driver maintainers, Eric for vc4 here. >> >> Eric was cc:d. I was just too lazy to add the cc:s to all the commit >> messages, and so i just used --cc on the whole lot. > > Hm, he's not on the cc: list over here (on the mails, not in the patch > body). That could be because he has "Avoid duplicate copies of messages?" enabled in his mailman subscription options. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/4] drm: Protect master->unique with dev->master_mutex
On 13/12/16 05:35 PM, Daniel Vetter wrote: > On Mon, Dec 12, 2016 at 01:23:46PM +, Emil Velikov wrote: >> On 10 December 2016 at 21:52, Daniel Vetter <daniel.vet...@ffwll.ch> wrote: >>> No one looks at the major/minor versions except the unique/busid >>> stuff. If we protect that with the master_mutex (since it also affects >>> the unique of each master, oh well) we can mark these two IOCTL with >>> DRM_UNLOCKED. >>> >>> While doing this I realized that the comment for the magic_map is >>> outdated, I've forgotten to update it in: >>> >>> commit d2b34ee62b409a03c6fe43c07b779983be51d017 >>> Author: Daniel Vetter <daniel.vet...@ffwll.ch> >>> Date: Fri Jun 17 09:33:21 2016 +0200 >>> >>> drm: Protect authmagic with master_mutex >>> >>> Cc: Chris Wilson <ch...@chris-wilson.co.uk> >>> Cc: Emil Velikov <emil.l.veli...@gmail.com> >>> Signed-off-by: Daniel Vetter <daniel.vet...@intel.com> >> Reviewed-by: Emil Velikov <emil.l.veli...@gmail.com> >> >> Side note: I've looked at "fixing" xf86-video-amdgpu and so far I'm at >> ~25 patches. Admittedly it includes some related fixes/cleanups. >> At some point we want to address the other KMS ddx - armsoc (ahem), >> ati, freedreno, intel, nouveau, omap, opentegra, vmwgfx, qxl and of >> course modesetting ;-) >> Regardless if some are superseded and/or barely used. > > Hm, I thought the grand plan is to use -modesetting almost everywhere and > forget about all the others? Maybe if you mean s/grand plan/pipe dream/ ... Specifically, wrt to DDX drivers for ATI/AMD GPUs: xf86-video-ati supports old GPUs on which glamor can't work, so -modesetting can never be a complete replacement. xf86-video-amdgpu is also used by the amdgpu-pro stack, with modifications which are probably unsuitable for -modesetting. There's also still a feature gap to -modesetting, e.g. the latter doesn't support TearFree yet, and there doesn't seem to be any interest to fix that. So I'm afraid we can't get rid of those anytime soon, if ever. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: Allow CAP_PRIME on !MODESET
On 07/12/16 11:49 PM, Daniel Vetter wrote:
> vgem (and our igt tests using vgem) need this. I suspect etnaviv will
> fare similarly.
>
> v2. Make it build. Oops.
>
> Fixes: d5264ed3823a ("drm: Return -ENOTSUPP when called for KMS cap with a
> non-KMS driver")
> Cc: Michel Dänzer <michel.daen...@amd.com>
> Cc: Alex Deucher <alexander.deuc...@amd.com>
> Cc: Chris Wilson <ch...@chris-wilson.co.uk>
> Reviewed-by: Chris Wilson <ch...@chris-wilson.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vet...@intel.com>
Thanks Daniel, and sorry I missed this, guess I was thinking of !MODESET
too much in terms of UMS and too little in terms of render-only drivers.
Reviewed-by: Michel Dänzer <michel.daen...@amd.com>
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] [RFC] drm: Nerf DRM_CONTROL nodes
On 06/12/16 10:42 AM, Mike Lothian wrote: > Hi > > This patch (in drm-next and drm-intel-nightly) stops my system from > booting, I don't see any errors, just a black screen and a reboot after > the kernel has been selected > > I have confirmed that reverting this patch gets those two branches > working again > > Sorry to be the bearer of bad news - I'm guessing this is PRIME related It's not. You can choose between https://patchwork.freedesktop.org/patch/125754/ or https://patchwork.freedesktop.org/patch/125644/ for the fix. :) -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: Fix error path in drm_mode_page_flip_ioctl()
On 08/09/16 02:23 AM, Imre Deak wrote:
> This fixes the error path for platforms that don't define the new
> page_flip_target() hook.
>
> Fixes: c229bfbbd04 ("drm: Add page_flip_target CRTC hook v2")
> Testcase: igt/kms_flip/basic-flip-vs-dpms
> CC: Michel Dänzer <michel.daen...@amd.com>
> Signed-off-by: Imre Deak <imre.d...@intel.com>
> ---
> drivers/gpu/drm/drm_crtc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 0f797ee..d84a0ea 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -2047,7 +2047,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> }
>
> out:
> - if (ret)
> + if (ret && crtc->funcs->page_flip_target)
> drm_crtc_vblank_put(crtc);
> if (fb)
> drm_framebuffer_unreference(fb);
>
Nice catch, thanks!
Reviewed-by: Michel Dänzer <michel.daen...@amd.com>
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/6] Allow DC state to reset the counter on screen enabled.
On 04.08.2016 06:33, Rodrigo Vivi wrote: > For now DC is only helping on screen off scenarios since PSR is disabled. > > But if we want to enable PSR first we need to make DC reliable with screen on. > Biggest challenge is to deal with vblank counters since frame counter register > is read only and can be reset in DC state. > > This series is one of possible approaches, but brings the down side of not > being possible to use runtime pm with vblank enabled. Some test cases needs > to be adapted to represent this new vision. Note that the frame counter register must not reset before drm_crtc_vblank_off / after drm_crtc_vblank_on, even while the vblank interrupt is disabled. Otherwise the DRM vblank counter as seen by userspace via the DRM_IOCTL_WAIT_VBLANK ioctl will jump around, because the core DRM code uses the frame counter register to keep track of how many vertical blank periods occurred since the interrupt was disabled. > But also this series is not fully tested. Apparently I have an issue yet > with flip-vs-expired-vblank_* tests and pm_rpm basic tests. Maybe some of the test failures are due to the above. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 10/11] drm: Use vblank timestamps to guesstimate how many vblanks were missed
On 15.09.2015 04:43, ville.syrj...@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrj...@linux.intel.com>
>
> When lacking am accurate hardware frame counter, we can fall back to
> using the vblank timestamps to guesstimagte how many vblanks have
> elapsed since the last time the vblank counter was updated.
>
> Take the oppostunity to unify the vblank_disable_and_save() and
> drm_handle_vblank_events() to call the same function
> (drm_update_vblank_count()) to perform the vblank updates.
It would be nice to keep the drm_update_vblank_count unification
separate. As it is, it's very hard to keep track of which parts of the
patch are for each logical change.
BTW, I think the fact that I was hitting the problem fixed by 209e4dbc
("drm/vblank: Use u32 consistently for vblank counters") within a few
days indicates that there's another bug which causes the counter to jump
forward with drm_vblank_on/off(). It may not manifest itself with
current Intel hardware because that has a full 32-bit hardware frame
counter, turning the related calculations into no-ops. I haven't had
time to investigate this further yet.
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/3] drm/radeon: Switch to drm_vblank_on/off
On 28.05.2015 18:03, Michel Dänzer wrote: On 28.05.2015 17:38, Daniel Vetter wrote: On Thu, May 28, 2015 at 04:11:53PM +0900, Michel Dänzer wrote: On 27.05.2015 18:41, Daniel Vetter wrote: On Wed, May 27, 2015 at 06:21:24PM +0900, Michel Dänzer wrote: On 27.05.2015 18:04, Daniel Vetter wrote: These should be functionally equivalent to the older per/post modeset functions, except that they block out drm_vblank_get right away. There's only the clock adjusting code (outside of pageflips) in readone which uses drm_vblank_get. But that code doesn't synchronize against concurrent modesets and instead handles any such races by waiting for the right vblank to arrive with a short timetout. The longer-term plan here is to switch all kms drivers to drm_vblank_on/off so that common code like pending event cleanup can be done there, while drm_vblank_pre/post_modeset will be purely drm internal for the old UMS ioctl. Note that the kerneldoc for pre/post_modeset is wrong since as Michel Dänzer correctly pointed out it works if only using pre/post_modeset. The trouble that lead to this comment is the very old version of drm_vblank_off to clear out pending events when disabling a pipe, which did seem to wreak havoc with the trick used by pre/post_modeset. Michel also expressed dissatisfaction with intel folks pushing new interfaces with bogus justifications. I still maintain that having a consistent set of vblank behaviour across kms drivers, separate from any old UMS functions is a useful goal. Cc: Michel Dänzer michel.daen...@amd.com Signed-off-by: Daniel Vetter daniel.vet...@intel.com Can you describe at least one tangible benefit this change provides for the radeon driver? Because I'm afraid that this might cause subtle breakage, and since we don't have any rigorous tests for this like in intel-gpu-tools (yet?), it might be painful to track it down. So, I'd like to have a good reason for taking the risk. right now at most a bit of code to clean out pending events on modeset disable, for somewhat consistent behaviour with other drivers. But in general it's fairly ill-defined what happens with vblank events. Yeah, while that's nice to have, I don't think it makes too much difference in practice. Anyway, I'm giving this patch a spin, and it does indeed cause userspace fallout, at least with DRI3/Present enabled, because the vblank and pageflip ioctls now return -EINVAL while the CRTC is off. However, it looks like fixing that up might not be too bad, so I'm cautiously optimistic for this change. But I'd like some more time for testing and fixing userspace. [...] Otoh asking for a vblank event on a dead pipe smells like a userspace bug and could result in stuck compositors. Not sure what's best here really. Agreed, and we're already careful not to do that with DRI2, just not yet with DRI3/Present (which isn't in any xf86-video-ati release yet). I've fixed up the DRI3/Present code as well. This patch (with maybe some cleanups to the commit log, in particular I'm not sure the third paragraph should be there) is Acked-and-Tested-by: Michel Dänzer michel.daen...@amd.com -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [-next] WARNING at i915_gem_track_fb
On 14.07.2015 22:41, Sergey Senozhatsky wrote: sometimes `xset dpms force off' just turns off the panel for a second, sometimes -- until I generate a `wakeup' event (key press, etc.) FWIW, the former case is because releasing the enter key generates an input event, which changes the DPMS state to on again. You can avoid that with something like sleep 1 xset dpms force off. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/3] drm/radeon: Switch to drm_vblank_on/off
On 27.05.2015 18:41, Daniel Vetter wrote: On Wed, May 27, 2015 at 06:21:24PM +0900, Michel Dänzer wrote: On 27.05.2015 18:04, Daniel Vetter wrote: These should be functionally equivalent to the older per/post modeset functions, except that they block out drm_vblank_get right away. There's only the clock adjusting code (outside of pageflips) in readone which uses drm_vblank_get. But that code doesn't synchronize against concurrent modesets and instead handles any such races by waiting for the right vblank to arrive with a short timetout. The longer-term plan here is to switch all kms drivers to drm_vblank_on/off so that common code like pending event cleanup can be done there, while drm_vblank_pre/post_modeset will be purely drm internal for the old UMS ioctl. Note that the kerneldoc for pre/post_modeset is wrong since as Michel Dänzer correctly pointed out it works if only using pre/post_modeset. The trouble that lead to this comment is the very old version of drm_vblank_off to clear out pending events when disabling a pipe, which did seem to wreak havoc with the trick used by pre/post_modeset. Michel also expressed dissatisfaction with intel folks pushing new interfaces with bogus justifications. I still maintain that having a consistent set of vblank behaviour across kms drivers, separate from any old UMS functions is a useful goal. Cc: Michel Dänzer michel.daen...@amd.com Signed-off-by: Daniel Vetter daniel.vet...@intel.com Can you describe at least one tangible benefit this change provides for the radeon driver? Because I'm afraid that this might cause subtle breakage, and since we don't have any rigorous tests for this like in intel-gpu-tools (yet?), it might be painful to track it down. So, I'd like to have a good reason for taking the risk. right now at most a bit of code to clean out pending events on modeset disable, for somewhat consistent behaviour with other drivers. But in general it's fairly ill-defined what happens with vblank events. Yeah, while that's nice to have, I don't think it makes too much difference in practice. Anyway, I'm giving this patch a spin, and it does indeed cause userspace fallout, at least with DRI3/Present enabled, because the vblank and pageflip ioctls now return -EINVAL while the CRTC is off. However, it looks like fixing that up might not be too bad, so I'm cautiously optimistic for this change. But I'd like some more time for testing and fixing userspace. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/3] drm/radeon: Switch to drm_vblank_on/off
On 28.05.2015 17:38, Daniel Vetter wrote: On Thu, May 28, 2015 at 04:11:53PM +0900, Michel Dänzer wrote: On 27.05.2015 18:41, Daniel Vetter wrote: On Wed, May 27, 2015 at 06:21:24PM +0900, Michel Dänzer wrote: On 27.05.2015 18:04, Daniel Vetter wrote: These should be functionally equivalent to the older per/post modeset functions, except that they block out drm_vblank_get right away. There's only the clock adjusting code (outside of pageflips) in readone which uses drm_vblank_get. But that code doesn't synchronize against concurrent modesets and instead handles any such races by waiting for the right vblank to arrive with a short timetout. The longer-term plan here is to switch all kms drivers to drm_vblank_on/off so that common code like pending event cleanup can be done there, while drm_vblank_pre/post_modeset will be purely drm internal for the old UMS ioctl. Note that the kerneldoc for pre/post_modeset is wrong since as Michel Dänzer correctly pointed out it works if only using pre/post_modeset. The trouble that lead to this comment is the very old version of drm_vblank_off to clear out pending events when disabling a pipe, which did seem to wreak havoc with the trick used by pre/post_modeset. Michel also expressed dissatisfaction with intel folks pushing new interfaces with bogus justifications. I still maintain that having a consistent set of vblank behaviour across kms drivers, separate from any old UMS functions is a useful goal. Cc: Michel Dänzer michel.daen...@amd.com Signed-off-by: Daniel Vetter daniel.vet...@intel.com Can you describe at least one tangible benefit this change provides for the radeon driver? Because I'm afraid that this might cause subtle breakage, and since we don't have any rigorous tests for this like in intel-gpu-tools (yet?), it might be painful to track it down. So, I'd like to have a good reason for taking the risk. right now at most a bit of code to clean out pending events on modeset disable, for somewhat consistent behaviour with other drivers. But in general it's fairly ill-defined what happens with vblank events. Yeah, while that's nice to have, I don't think it makes too much difference in practice. Anyway, I'm giving this patch a spin, and it does indeed cause userspace fallout, at least with DRI3/Present enabled, because the vblank and pageflip ioctls now return -EINVAL while the CRTC is off. However, it looks like fixing that up might not be too bad, so I'm cautiously optimistic for this change. But I'd like some more time for testing and fixing userspace. [...] Otoh asking for a vblank event on a dead pipe smells like a userspace bug and could result in stuck compositors. Not sure what's best here really. Agreed, and we're already careful not to do that with DRI2, just not yet with DRI3/Present (which isn't in any xf86-video-ati release yet). -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/3] drm/radeon: Switch to drm_vblank_on/off
On 27.05.2015 18:04, Daniel Vetter wrote:
These should be functionally equivalent to the older per/post modeset
functions, except that they block out drm_vblank_get right away.
There's only the clock adjusting code (outside of pageflips) in
readone which uses drm_vblank_get. But that code doesn't synchronize
against concurrent modesets and instead handles any such races by
waiting for the right vblank to arrive with a short timetout.
The longer-term plan here is to switch all kms drivers to
drm_vblank_on/off so that common code like pending event cleanup can
be done there, while drm_vblank_pre/post_modeset will be purely
drm internal for the old UMS ioctl.
Note that the kerneldoc for pre/post_modeset is wrong since as Michel
Dänzer correctly pointed out it works if only using pre/post_modeset.
The trouble that lead to this comment is the very old version of
drm_vblank_off to clear out pending events when disabling a pipe,
which did seem to wreak havoc with the trick used by pre/post_modeset.
Michel also expressed dissatisfaction with intel folks pushing new
interfaces with bogus justifications. I still maintain that having a
consistent set of vblank behaviour across kms drivers, separate from
any old UMS functions is a useful goal.
Cc: Michel Dänzer michel.daen...@amd.com
Signed-off-by: Daniel Vetter daniel.vet...@intel.com
Can you describe at least one tangible benefit this change provides for
the radeon driver?
Because I'm afraid that this might cause subtle breakage, and since we
don't have any rigorous tests for this like in intel-gpu-tools (yet?),
it might be painful to track it down.
So, I'd like to have a good reason for taking the risk.
drivers/gpu/drm/radeon/atombios_crtc.c | 4 ++--
drivers/gpu/drm/radeon/radeon_legacy_crtc.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/radeon/atombios_crtc.c
b/drivers/gpu/drm/radeon/atombios_crtc.c
index 42b2ea3fdcf3..77912fd48b69 100644
--- a/drivers/gpu/drm/radeon/atombios_crtc.c
+++ b/drivers/gpu/drm/radeon/atombios_crtc.c
@@ -274,13 +274,13 @@ void atombios_crtc_dpms(struct drm_crtc *crtc, int mode)
if (ASIC_IS_DCE3(rdev) !ASIC_IS_DCE6(rdev))
atombios_enable_crtc_memreq(crtc, ATOM_ENABLE);
atombios_blank_crtc(crtc, ATOM_DISABLE);
- drm_vblank_post_modeset(dev, radeon_crtc-crtc_id);
+ drm_vblank_on(dev, radeon_crtc-crtc_id);
radeon_crtc_load_lut(crtc);
break;
case DRM_MODE_DPMS_STANDBY:
case DRM_MODE_DPMS_SUSPEND:
case DRM_MODE_DPMS_OFF:
- drm_vblank_pre_modeset(dev, radeon_crtc-crtc_id);
+ drm_vblank_off(dev, radeon_crtc-crtc_id);
if (radeon_crtc-enabled)
atombios_blank_crtc(crtc, ATOM_ENABLE);
if (ASIC_IS_DCE3(rdev) !ASIC_IS_DCE6(rdev))
diff --git a/drivers/gpu/drm/radeon/radeon_legacy_crtc.c
b/drivers/gpu/drm/radeon/radeon_legacy_crtc.c
index 678b4386540d..4259e27f3983 100644
--- a/drivers/gpu/drm/radeon/radeon_legacy_crtc.c
+++ b/drivers/gpu/drm/radeon/radeon_legacy_crtc.c
@@ -330,13 +330,13 @@ static void radeon_crtc_dpms(struct drm_crtc *crtc, int
mode)
RADEON_CRTC_DISP_REQ_EN_B));
WREG32_P(RADEON_CRTC_EXT_CNTL, crtc_ext_cntl, ~(mask |
crtc_ext_cntl));
}
- drm_vblank_post_modeset(dev, radeon_crtc-crtc_id);
+ drm_vblank_on(dev, radeon_crtc-crtc_id);
radeon_crtc_load_lut(crtc);
break;
case DRM_MODE_DPMS_STANDBY:
case DRM_MODE_DPMS_SUSPEND:
case DRM_MODE_DPMS_OFF:
- drm_vblank_pre_modeset(dev, radeon_crtc-crtc_id);
+ drm_vblank_off(dev, radeon_crtc-crtc_id);
if (radeon_crtc-crtc_id)
WREG32_P(RADEON_CRTC2_GEN_CNTL, mask, ~(RADEON_CRTC2_EN
| mask));
else {
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: Defer disabling the vblank IRQ until the next interrupt (for instant-off)
On 02.04.2015 20:34, Chris Wilson wrote: On vblank instant-off systems, we can get into a situation where the cost of enabling and disabling the vblank IRQ around a drmWaitVblank query dominates. However, we know that if the user wants the current vblank counter, they are also very likely to immediately queue a vblank wait and so we can keep the interrupt around and only turn it off if we have no further vblank requests in the interrupt interval. After vblank event delivery there is a shadow of one vblank where the interrupt is kept alive for the user to query and queue another vblank event. Similarly, if the user is using blocking drmWaitVblanks, the interrupt will be disabled on the IRQ following the wait completion. However, if the user is simply querying the current vblank counter and timestamp, the interrupt will be disabled after every IRQ and the user will enabled it again on the first query following the IRQ. As I mentioned before, it might not be too hard to make querying the current counter work without enabling the interrupt. But this looks like a step in the right direction. Acked-by: Michel Dänzer michel.daen...@amd.com -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: Return current vblank value for drmWaitVBlank queries
On 18.03.2015 00:44, Chris Wilson wrote:
When userspace queries the current vblank for the CRTC, we can reply
with the cached value (using atomic reads to serialise with the vblank
interrupt as necessary) without having to touch registers. In the
instant disable case, this saves us from enabling/disabling the vblank
around every query, greatly reducing the number of registers read and
written.
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Cc: Imre Deak imre.d...@intel.com
Cc: Daniel Vetter daniel.vet...@intel.com
Cc: Ville Syrjälä ville.syrj...@linux.intel.com
Cc: Laurent Pinchart laurent.pinch...@ideasonboard.com
Cc: Dave Airlie airl...@redhat.com
---
drivers/gpu/drm/drm_irq.c | 15 ++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index c8a34476570a..6c4570082b65 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -1585,7 +1585,18 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
if (crtc = dev-num_crtcs)
return -EINVAL;
- vblank = dev-vblank[crtc];
+ /* Fast-path the query for the current value (without an event)
+ * to avoid having to enable/disable the vblank interrupts.
+ */
+ if ((vblwait-request.type (_DRM_VBLANK_TYPES_MASK |
_DRM_VBLANK_FLAGS_MASK)) == _DRM_VBLANK_RELATIVE
+ vblwait-request.sequence == 0) {
+ struct timeval now;
+
+ vblwait-reply.sequence = drm_vblank_count_and_time(dev, crtc,
now);
+ vblwait-reply.tval_sec = now.tv_sec;
+ vblwait-reply.tval_usec = now.tv_usec;
+ return 0;
+ }
drm_vblank_count_and_time() doesn't return the correct sequence number
while the vblank interrupt is disabled, does it? It returns the sequence
number from the last time vblank_disable_and_save() was called (when the
vblank interrupt was disabled). That's why drm_vblank_get() is needed here.
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: Return current vblank value for drmWaitVBlank queries
On 18.03.2015 11:53, Michel Dänzer wrote:
On 18.03.2015 00:44, Chris Wilson wrote:
When userspace queries the current vblank for the CRTC, we can reply
with the cached value (using atomic reads to serialise with the vblank
interrupt as necessary) without having to touch registers. In the
instant disable case, this saves us from enabling/disabling the vblank
around every query, greatly reducing the number of registers read and
written.
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Cc: Imre Deak imre.d...@intel.com
Cc: Daniel Vetter daniel.vet...@intel.com
Cc: Ville Syrjälä ville.syrj...@linux.intel.com
Cc: Laurent Pinchart laurent.pinch...@ideasonboard.com
Cc: Dave Airlie airl...@redhat.com
---
drivers/gpu/drm/drm_irq.c | 15 ++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index c8a34476570a..6c4570082b65 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -1585,7 +1585,18 @@ int drm_wait_vblank(struct drm_device *dev, void
*data,
if (crtc = dev-num_crtcs)
return -EINVAL;
-vblank = dev-vblank[crtc];
+/* Fast-path the query for the current value (without an event)
+ * to avoid having to enable/disable the vblank interrupts.
+ */
+if ((vblwait-request.type (_DRM_VBLANK_TYPES_MASK |
_DRM_VBLANK_FLAGS_MASK)) == _DRM_VBLANK_RELATIVE
+vblwait-request.sequence == 0) {
+struct timeval now;
+
+vblwait-reply.sequence = drm_vblank_count_and_time(dev, crtc,
now);
+vblwait-reply.tval_sec = now.tv_sec;
+vblwait-reply.tval_usec = now.tv_usec;
+return 0;
+}
drm_vblank_count_and_time() doesn't return the correct sequence number
while the vblank interrupt is disabled, does it? It returns the sequence
number from the last time vblank_disable_and_save() was called (when the
vblank interrupt was disabled). That's why drm_vblank_get() is needed here.
It might be possible to avoid drm_vblank_get() and use
drm_update_vblank_count() before (or in) drm_vblank_count_and_time()
instead when the vblank interrupt is disabled, but then you'd first need
to make it safe to call drm_update_vblank_count() several times while
the vblank interrupt is disabled, by making it update vblank-last at least.
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 7/8] drm/irq: Implement a generic vblank_wait function
On 31.07.2014 16:54, Daniel Vetter wrote: On Thu, Jul 31, 2014 at 3:14 AM, Michel Dänzer mic...@daenzer.net wrote: I think it would be better to refactor drm_wait_vblank() than to reinvent it. That's the ioctl implementation which spends most of its time decoding ioctl structures. If we take that out then there's about half a line which would be shared (since a lot of the stuff in there is ums gunk that we don't want to carry over to a kms-only internal interface). So imo that doesn't make sense. I'm referring to the core logic of waiting for a number of vblank periods or until the vblank period with a given sequence number, dealing with wraparound etc. The issues you guys were discussing for a new function were ironed out there long ago. -- Earthling Michel Dänzer| http://www.amd.com Libre software enthusiast |Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 7/8] drm/irq: Implement a generic vblank_wait function
On 30.07.2014 17:22, Daniel Vetter wrote:
On Wed, Jul 30, 2014 at 11:59:33AM +0900, Michel Dänzer wrote:
On 30.07.2014 06:32, Daniel Vetter wrote:
+ * due to lack of driver support or because the crtc is off.
+ */
+void drm_crtc_vblank_wait(struct drm_crtc *crtc)
+{
+ drm_vblank_wait(crtc-dev, drm_crtc_index(crtc));
+}
+EXPORT_SYMBOL(drm_crtc_vblank_wait);
+
+/**
Maybe the function names should be *_vblank_wait_next() or something to
clarify the purpose and reduce potential confusion versus drm_wait_vblank().
Yeah that name is just transferred from the i915 driver. What about
drm_wait_one_vblank()/drm_crtc_wait_one_vblank()?
I don't care that much :), go ahead.
--
Earthling Michel Dänzer| http://www.amd.com
Libre software enthusiast |Mesa and X developer
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/irq: Implement a generic vblank_wait function
On 30.07.2014 18:25, Daniel Vetter wrote: As usual in both a crtc index and a struct drm_crtc * version. The function assumes that no one drivers their display below 10Hz, and it will complain if the vblank wait takes longer than that. v2: Also check dev-max_vblank_counter since some drivers register a fake get_vblank_counter function. v3: Use drm_vblank_count instead of calling the low-level -get_vblank_counter callback. That way we'll get the sw-cooked counter for platforms without proper vblank support and so can ditch the max_vblank_counter check again. v4: Review from Michel Dänzer: - Restore lost notes about v3: - Spelling in kerneldoc. - Inline wait_event condition. - s/vblank_wait/wait_one_vblank/ Cc: Michel Dänzer mic...@daenzer.net Cc: Ville Syrjälä ville.syrj...@linux.intel.com Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch Reviewed-by: Michel Dänzer michel.daen...@amd.com -- Earthling Michel Dänzer| http://www.amd.com Libre software enthusiast |Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 7/8] drm/irq: Implement a generic vblank_wait function
On 31.07.2014 00:21, Thierry Reding wrote:
On Wed, Jul 30, 2014 at 05:36:21PM +0300, Ville Syrjälä wrote:
On Wed, Jul 30, 2014 at 04:20:25PM +0200, Thierry Reding wrote:
On Wed, Jul 30, 2014 at 05:32:28PM +0900, Michel Dänzer wrote:
On 30.07.2014 17:22, Daniel Vetter wrote:
On Wed, Jul 30, 2014 at 11:59:33AM +0900, Michel Dänzer wrote:
On 30.07.2014 06:32, Daniel Vetter wrote:
+ * due to lack of driver support or because the crtc is off.
+ */
+void drm_crtc_vblank_wait(struct drm_crtc *crtc)
+{
+ drm_vblank_wait(crtc-dev, drm_crtc_index(crtc));
+}
+EXPORT_SYMBOL(drm_crtc_vblank_wait);
+
+/**
Maybe the function names should be *_vblank_wait_next() or something to
clarify the purpose and reduce potential confusion versus
drm_wait_vblank().
Yeah that name is just transferred from the i915 driver. What about
drm_wait_one_vblank()/drm_crtc_wait_one_vblank()?
I don't care that much :), go ahead.
Just my two cents: our downstream kernel has a helper somewhat like this
which waits for a specified number of frames (apparently this is useful
for some panels that require up to 5 or 6 frames before they display the
correct image on screen). So perhaps something like this could work:
void drm_wait_vblank_count(struct drm_device *dev, unsigned int crtc,
unsigned int count)
{
u32 last;
int ret;
ret = drm_vblank_get(dev, crtc);
if (WARN_ON(ret))
return;
while (count--) {
last = drm_vblank_count(dev, crtc);
...
}
drm_vblank_put(dev, crtc);
}
Would be nicer to wait for an absolute vblank count instead IMO. Or
if you want to pass a relative count in just convert it to an absolute
count first and wait for it (taking wraparound into account obviously).
Hmm... would something like this work?
target = drm_vblank_count(dev, crtc) + count;
ret = wait_event_timeout(...,
drm_vblank_count(dev, crtc) == target,
...);
That should properly take into account wrap-around given that both sites
use drm_vblank_count().
I think it would be better to refactor drm_wait_vblank() than to
reinvent it.
--
Earthling Michel Dänzer| http://www.amd.com
Libre software enthusiast |Mesa and X developer
signature.asc
Description: OpenPGP digital signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 7/8] drm/irq: Implement a generic vblank_wait function
On 30.07.2014 06:32, Daniel Vetter wrote:
As usual in both a crtc index and a struct drm_crtc * version.
The function assumes that no one drivers their display below 10Hz, and
it will complain if the vblank wait takes longer than that.
v2: Also check dev-max_vblank_counter since some drivers register a
fake get_vblank_counter function.
What does that refer to? Can't find any other reference to
max_vblank_counter in the patch.
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 0de123afdb34..76024fdde452 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -999,6 +999,51 @@ void drm_crtc_vblank_put(struct drm_crtc *crtc)
EXPORT_SYMBOL(drm_crtc_vblank_put);
/**
+ * drm_vblank_wait - wait for one vblank
+ * @dev: DRM device
+ * @crtc: crtc index
+ *
+ * This waits for one vblank to pass on @crtc, using the irq driver
interfaces.
+ * It is a failure to call this when the vblank irq for @crtc is disable,
e.g.
Spelling: 'disabled'
+ * due to lack of driver support or because the crtc is off.
+ */
+void drm_vblank_wait(struct drm_device *dev, int crtc)
+{
+ int ret;
+ u32 last;
+
+ ret = drm_vblank_get(dev, crtc);
+ if (WARN_ON(ret))
+ return;
+
+ last = drm_vblank_count(dev, crtc);
+
+#define C (last != drm_vblank_count(dev, crtc))
+ ret = wait_event_timeout(dev-vblank[crtc].queue,
+ C, msecs_to_jiffies(100));
+
+ WARN_ON(ret == 0);
+#undef C
What's the point of the C macro?
+ drm_vblank_put(dev, crtc);
+}
+EXPORT_SYMBOL(drm_vblank_wait);
+
+/**
+ * drm_crtc_vblank_wait - wait for one vblank
+ * @crtc: DRM crtc
+ *
+ * This waits for one vblank to pass on @crtc, using the irq driver
interfaces.
+ * It is a failure to call this when the vblank irq for @crtc is disable,
e.g.
Same typo as above.
+ * due to lack of driver support or because the crtc is off.
+ */
+void drm_crtc_vblank_wait(struct drm_crtc *crtc)
+{
+ drm_vblank_wait(crtc-dev, drm_crtc_index(crtc));
+}
+EXPORT_SYMBOL(drm_crtc_vblank_wait);
+
+/**
Maybe the function names should be *_vblank_wait_next() or something to
clarify the purpose and reduce potential confusion versus drm_wait_vblank().
Looks good to me other than that.
--
Earthling Michel Dänzer| http://www.amd.com
Libre software enthusiast |Mesa and X developer
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 5/5] drm/i915: Allow vblank interrupts during modeset and eliminate some vblank races
On 29.05.2014 19:56, Daniel Vetter wrote: On Thu, May 29, 2014 at 6:11 AM, Michel Dänzer mic...@daenzer.net wrote: We could rename the off/on to pre/post_modeset if you like, but then someone gets to audit all the other drivers. That someone isn't going to be me. I appreciate your caution wrt other drivers, but I'm worried that having a second mechanism for keeping the DRM vblank counter consistent might cause subtle problems for drivers using the existing mechanism anyway. I think that risk is rather low. Only i915 has been stupid enough to use both drm_vblank_off + pre/post_modeset in the normal crtc enable/disable functions. Everyone else uses either or the other, except for tegra which uses pre/post in the -prepare/commit hooks and off in the crtc-disable callback. So should still get consistent results (since after -disable vblank consistency stops mattering). The reason we do all this is that we want to kill the delayed vblank put for i915, which is possible if you have a hw vblank counter. That should also be possible with pre/post_modeset. (Not sure eliminating it completely is a good idea for the reasons you mentioned before, but at least decreasing it significantly should be no problem. I think even dropping the default a lot for all drivers should be rather low risk) There's apparently more stuff wrong here still, so while we wrestle around probably best to leave other drivers out. I guess once this is all done I have to roll it out across all drivers so that we have consistent behaviour again ... Since AFAICT radeon isn't affected by the problem drm_vblank_off() was supposed to solve originally, I'd prefer if it didn't start using that and potentially suffer from all the trouble it seems to cause. -- Earthling Michel Dänzer| http://www.amd.com Libre software enthusiast |Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 5/5] drm/i915: Allow vblank interrupts during modeset and eliminate some vblank races
Digging out an ooold post of Daniel's... On 04.03.2014 18:13, Daniel Vetter wrote: On Tue, Feb 25, 2014 at 11:58:26AM +0900, Michel Dänzer wrote: When the pre/post-modeset hooks were originally added, it worked like this: the pre-modeset hook enabled the vblank interrupt, which updated the DRM vblank counter from the driver/HW counter. The post-modeset hook disabled the vblank interrupt again, which recorded the post-modeset driver/HW counter value. But the vblank code has changed a lot since then, not sure it still works like that. It still works like that, but there's two fundamental issues with this trick: - There's a race where the vblank state is fubar right between the completion of the modeset and before the first vblank happened. Can you provide more details about that? You mentioned on IRC that sometimes 'bogus' DRM vblank counter values are returned to userspace. The most likely cause of that would be drm_vblank_pre_modeset() being called too late, i.e. after the hardware counter was reset. (Or if you're reducing / eliminating the vblank disable timer, possibly the vblank interrupt getting disabled too early, i.e. before the hardware counter was reset) Speaking of reducing or disabling the vblank disable timer, that should be possible with drm_vblank_pre/post_modeset() as well. - It doesn't work across suspend/resume since no one re-enables the vblank interrupt. That sounds like a driver bug to me. The driver should re-enable the hardware interrupt on resume if the vblank interrupt is currently enabled by the DRM core. This seems to work fine for me with radeon. So, I'm afraid I'm still not convinced that the new vblank_off/on() functions and the ever-increasing series of fixes for problems related to them are the right (let alone only) solution for your problems. -- Earthling Michel Dänzer| http://www.amd.com Libre software enthusiast |Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 5/5] drm/i915: Allow vblank interrupts during modeset and eliminate some vblank races
On 28.05.2014 20:19, Ville Syrjälä wrote: On Wed, May 28, 2014 at 06:12:54PM +0900, Michel Dänzer wrote: Digging out an ooold post of Daniel's... On 04.03.2014 18:13, Daniel Vetter wrote: On Tue, Feb 25, 2014 at 11:58:26AM +0900, Michel Dänzer wrote: When the pre/post-modeset hooks were originally added, it worked like this: the pre-modeset hook enabled the vblank interrupt, which updated the DRM vblank counter from the driver/HW counter. The post-modeset hook disabled the vblank interrupt again, which recorded the post-modeset driver/HW counter value. But the vblank code has changed a lot since then, not sure it still works like that. It still works like that, but there's two fundamental issues with this trick: - There's a race where the vblank state is fubar right between the completion of the modeset and before the first vblank happened. Can you provide more details about that? You mentioned on IRC that sometimes 'bogus' DRM vblank counter values are returned to userspace. The most likely cause of that would be drm_vblank_pre_modeset() being called too late, i.e. after the hardware counter was reset. (Or if you're reducing / eliminating the vblank disable timer, possibly the vblank interrupt getting disabled too early, i.e. before the hardware counter was reset) The hardware counter reset is a problem: 1. vblank_disable_and_save() updates .last 2. modeset/dpms/suspend (hw counter is reset) 3. drm_vblank_get() - cur_vblank-.last == garbage The lack of drm_vblank_on() is a problem: 1. drm_vblank_get() 2. drm_vblank_off() 3. modeset/dpms/suspend 4. drm_vblank_get() - -EINVAL I'd summarize these as 'drm_vblank_off() considered harmful'. Another issue: 1. drm_vblank_get() 2. drm_vblank_put() 3. disable timer expires which updates .last ... 4. drm_vblank_off() updates .last again 5. modeset/dpms/suspend 6. drm_vblank_get() - sequence number doesn't account for the time between 3. and 4. I suppose this isn't a big issue, but I don't like leaking implementation details (the timer delay) into the sequence number. Yes, I guess drm_vblank_off() shouldn't call vblank_disable_and_save() if vblank is already disabled. Now this last one should actually work with the current drm_vblank_pre_modeset() since it does a drm_vblank_get() which will apply the cur_vblank-.last diff, but it also enables the vblank interrupt which is entirely pointless, and also wrong on Intel hardware (well, if we didn't have drm_vblank_off()). Our docs say that we shouldn't have the vblank interrupt enabled+unmasked while the pipe is off. That's a driver implementation detail. The driver isn't required to keep the hardware interrupt enabled all the time between the enable_vblank() and disable_vblank() hook calls. The DRM core just wants drm_handle_vblank() to be called for any vertical blank periods between them. Anyway it's not a very obvious way to do things. Ie. you're doing the drm_vblank_get() not because you actually want vblank interrupts, but because you want the side effects. No, that's not the only reason. It's also so that drm_handle_vblank() is called for any vertical blank periods occurring while the hardware counter might reset, so the DRM vblank counter gets incremented independently from the hardware counter. Speaking of reducing or disabling the vblank disable timer, that should be possible with drm_vblank_pre/post_modeset() as well. I get the impression that you're a bit hung up on the names :) Not at all. In fact, the pre/post_modeset names are slightly misleading, as they're not only about modesetting but about preventing the DRM vblank counter from jumping due to hardware counter jumps. We could rename the off/on to pre/post_modeset if you like, but then someone gets to audit all the other drivers. That someone isn't going to be me. I appreciate your caution wrt other drivers, but I'm worried that having a second mechanism for keeping the DRM vblank counter consistent might cause subtle problems for drivers using the existing mechanism anyway. -- Earthling Michel Dänzer| http://www.amd.com Libre software enthusiast |Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/12] drm/doc: Discourage usage of MODESET_CTL ioctl
On 15.05.2014 03:51, Daniel Vetter wrote: Leftover from the old days of ums and should be used any longer. Since commit 29935554b384b1b3a7377d6f0b03b21d18a61683 Author: Laurent Pinchart laurent.pinch...@ideasonboard.com Date: Wed May 30 00:58:09 2012 +0200 drm: Disallow DRM_IOCTL_MODESET_CTL for KMS drivers it is a complete no-Op for kms drivers. Cc: Laurent Pinchart laurent.pinch...@ideasonboard.com Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch --- Documentation/DocBook/drm.tmpl | 11 +-- 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl index 83dd0b043c28..e37a77a72b8b 100644 --- a/Documentation/DocBook/drm.tmpl +++ b/Documentation/DocBook/drm.tmpl @@ -2861,12 +2861,11 @@ int num_ioctls;/synopsis termDRM_IOCTL_MODESET_CTL/term listitem para -This should be called by application level drivers before and -after mode setting, since on many devices the vertical blank -counter is reset at that time. Internally, the DRM snapshots -the last vblank count when the ioctl is called with the -_DRM_PRE_MODESET command, so that the counter won't go backwards -(which is dealt with when _DRM_POST_MODESET is used). + This was only used for user-mode-settind drivers around + modesetting changes to allow the kernel to update the vblank + interrupt after mode setting, since on many devices the vertical + blank. This sentence looks mangled pretty badly. Did you have something like this in mind? This was only used for user-mode-setting drivers around modesetting changes to allow the kernel to keep the vertical blank counters consistent, since on many devices the hardware vertical blank counter is reset to 0 at some point during modeset changes. -- Earthling Michel Dänzer| http://www.amd.com Libre software enthusiast |Mesa and X developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 5/5] drm/i915: Allow vblank interrupts during modeset and eliminate some vblank races
On Mon, 2014-02-24 at 14:11 +0200, Ville Syrjälä wrote:
On Mon, Feb 24, 2014 at 12:48:55PM +0900, Michel Dänzer wrote:
On Fre, 2014-02-21 at 21:03 +0200, ville.syrj...@linux.intel.com wrote:
We can kill of the drm_vblank_{pre,post}_modeset() calls since those are
there simply to make drm_vblank_get() fail during a modeset.
Actually, their original purpose was to keep the DRM vblank counter
consistent across modesets, assuming the modeset resets the hardware
vblank counter.
I see. Well, actually I really don't. The code is too funky for me to
tell what it actually ends up doing. The obvious way would be to
resample the hardware counter at drm_vblank_post_modeset(), which the
code certainly doesn't do. But maybe it did something sensible in the
past.
When the pre/post-modeset hooks were originally added, it worked like
this: the pre-modeset hook enabled the vblank interrupt, which updated
the DRM vblank counter from the driver/HW counter. The post-modeset hook
disabled the vblank interrupt again, which recorded the post-modeset
driver/HW counter value.
But the vblank code has changed a lot since then, not sure it still
works like that.
--
Earthling Michel Dänzer| http://www.amd.com
Libre software enthusiast |Mesa and X developer
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 5/5] drm/i915: Allow vblank interrupts during modeset and eliminate some vblank races
On Fre, 2014-02-21 at 21:03 +0200, ville.syrj...@linux.intel.com wrote:
We can kill of the drm_vblank_{pre,post}_modeset() calls since those are
there simply to make drm_vblank_get() fail during a modeset.
Actually, their original purpose was to keep the DRM vblank counter
consistent across modesets, assuming the modeset resets the hardware
vblank counter.
--
Earthling Michel Dänzer| http://www.amd.com
Libre software enthusiast |Mesa and X developer
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm: add support for monotonic vblank timestamps
On Die, 2012-10-23 at 21:53 +0300, Imre Deak wrote: Jumps in the vblank and page flip event timestamps cause trouble for clients, so we should avoid them. The timestamp we get currently with gettimeofday can jump, so use instead monotonic timestamps. For backward compatibility use a module flag to revert back to using gettimeofday timestamps. Add also a DRM_CAP_TIMESTAMP_MONOTONIC flag that is simply a read only version of the module flag, so that clients can query this without depending on sysfs. Signed-off-by: Imre Deak imre.d...@intel.com drm_timestamp_monotonic should probably be a bool rather than an int. Looks good to me otherwise, as does patch 1. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Debian, X and DRI developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 4/4] drm: add support for raw monotonic vblank timestamps
On Fre, 2012-10-05 at 16:37 +0300, Imre Deak wrote: In practice we never want the timestamps for vblank and page flip events to be affected by time adjustments, so in addition to the gettimeofday timestamps we used so far add support for raw monotonic timestamps. For backward compatibility use flags to select between the old and new timestamp format. Note that with this change we will save the timestamp in both formats, for cases where multiple clients are expecting an event notification in different time formats. I wonder if all this trouble is really necessary. I honestly can't imagine any user of this API requiring non-monotonic timestamps and breaking with monotonic ones. I think it was simply a mistake that we didn't make them monotonic in the first place (or maybe it wasn't even possible when this API was first introduced). -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Debian, X and DRI developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 4/4] drm: add support for raw monotonic vblank timestamps
On Fre, 2012-10-05 at 16:59 +0300, Imre Deak wrote: On Fri, 2012-10-05 at 15:55 +0200, Michel Dänzer wrote: On Fre, 2012-10-05 at 16:37 +0300, Imre Deak wrote: In practice we never want the timestamps for vblank and page flip events to be affected by time adjustments, so in addition to the gettimeofday timestamps we used so far add support for raw monotonic timestamps. For backward compatibility use flags to select between the old and new timestamp format. Note that with this change we will save the timestamp in both formats, for cases where multiple clients are expecting an event notification in different time formats. I wonder if all this trouble is really necessary. I honestly can't imagine any user of this API requiring non-monotonic timestamps and breaking with monotonic ones. I think it was simply a mistake that we didn't make them monotonic in the first place (or maybe it wasn't even possible when this API was first introduced). Yea, I'd rather simply switch over to monotonic timestamps too. But that would break apps that already compare against the wall time for whatever purpose (for example A/V sync). Are there actually any such apps in the real world? Do they work when the wall time jumps? -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Debian, X and DRI developer ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
