Re: [Intel-gfx] [PATCH] iommu/intel: quirk to disable DMAR for QM57 igfx
Hi Daniel, On Tue, Jan 22, 2019 at 11:46:39AM +0100, Daniel Vetter wrote: > Note that the string of platforms which have various issues with iommu > and igfx is very long, thus far we only disabled it where there's no > workaround to stop it from hanging the box, but otherwise left it > enabled. So if we make a policy change to also disable it anywhere > where it doesn't work well (instead of not at all), there's a pile > more platforms to switch. I think its best to just disable iommu for the igfx devices on these platforms. Can you pick up Eric's patch and extend it with the list of affected platforms? Thanks, Joerg ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/7] drm/i915/perf: add OA interrupt support
Any taker? -Lionel On 16/01/2019 15:36, Lionel Landwerlin wrote: Taking the RFC off this series. To quite the vTune team that tried the previous version : "It reduces data collection overhead in VTune by 11x. It is great!" The GPA team's report on the previous version was a drop in CPU consumption from 17~20% down to 2~3%. This version includes : - a fix for an issue reported by Chris on the IMR register access on Haswell - the ability to completely disable the i915 OA head/tail polling - a new ioctl on the perf stream file descript (not the i915 drm master/render node) to force i915 to look at the OA head/tail register (see explanation in last patch). Cheers, Lionel Landwerlin (7): drm/i915/perf: rework aging tail workaround drm/i915/perf: reset pollin when perf stream is enabled drm/i915/perf: only append status when data is available drm/i915/perf: add new open param to configure polling of OA buffer drm/i915: handle interrupts from the OA unit drm/i915/perf: add interrupt enabling parameter drm/i915/perf: add flushing ioctl drivers/gpu/drm/i915/i915_drv.h | 59 +++- drivers/gpu/drm/i915/i915_irq.c | 39 ++- drivers/gpu/drm/i915/i915_perf.c| 388 +++- drivers/gpu/drm/i915/i915_reg.h | 7 + drivers/gpu/drm/i915/intel_ringbuffer.c | 2 + include/uapi/drm/i915_drm.h | 35 +++ 6 files changed, 357 insertions(+), 173 deletions(-) -- 2.20.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 27/38] drm/i915: Introduce the i915_user_extension_method
Quoting Tvrtko Ursulin (2019-01-22 09:31:31)
>
> On 18/01/2019 14:00, Chris Wilson wrote:
> > +/*
> > + * SPDX-License-Identifier: MIT
> > + *
> > + * Copyright © 2018 Intel Corporation
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +
> > +#include "i915_user_extensions.h"
> > +
> > +int i915_user_extensions(struct i915_user_extension __user *ext,
> > + const i915_user_extension_fn *tbl,
> > + unsigned long count,
>
> I would be tempted to limit the count to unsigned int. 4 billion
> extensions should be enough for everyone. :)
I just picked the natural type, thinking having it the same width as
ARRAY_SIZE might save a few questions from semantic analysers.
> > +{
> > + while (ext) {
> > + int err;
> > + u64 x;
> > +
> > + cond_resched();
> > + if (signal_pending(current))
> > + return -EINTR;
>
> What was your thinking behind this? It feels like, since here we are not
> doing any explicit wait/sleeps, that at this level the code shouldn't
> bother with it.
This ties into the discussion vvv
> > + if (get_user(x, &ext->name))
> > + return -EFAULT;
> > +
> > + err = -EINVAL;
> > + if (x < count && tbl[x])
> > + err = tbl[x](ext, data);
> > + if (err)
> > + return err;
>
> We talked about providing unwind on failure ie. option for destructor
> call backs. You gave up on that?
The patch is simply called 'merde'. Yes, unwind on failure does not lend
itself to a simple tail call implementation :) And it doesn't lend
itself nicely to writing the stacked cleanup handlers either. (So I
stuck with the solution of just doing a single cleanup on failure, safe
in the knowledge that the most complicated case in this series is
trivial.)
Thinking about the issues with providing a stack for unwind, leads to
the nasty question of how big a stack exactly do we want to provide.
Limiting the chain is required for defense against misuse, but what
depth? For the chained parameter setup of a single shot context create,
we could easily be into the dozens of parameters and extensions blocks.
The extreme I've been contemplating is a multi-batch execbuf setup (with
all the fancy extensions for e.g. semi-privileged fancy register setup),
for that I've been thinking about how this interface would extend to
supporting many chained chunks. What makes a good upper bound for stack
depth? 32? 64? 512? Pick a number, it won't be enough for someone. (Now,
really passing that much information through an ioctl means our design
is broken ;)
So... The break on interrupt there was for the silly protection against
recursion, if it doesn't result in an invalid command.
Another reason the patch was called merde.
I think the chained API extension is very powerful. Being able to do
arbitrary things like a single-shot context creation ioctl and still be
able to redefine/extend the interface as needs demands, is compelling.
> > +/*
> > + * i915_user_extension: Base class for defining a chain of extensions
> > + *
> > + * Many interfaces need to grow over time. In most cases we can simply
> > + * extend the struct and have userspace pass in more data. Another option,
> > + * as demonstrated by Vulkan's approach to providing extensions for forward
> > + * and backward compatibility, is to use a list of optional structs to
> > + * provide those extra details.
> > + *
> > + * The key advantage to using an extension chain is that it allows us to
> > + * redefine the interface more easily than an ever growing struct of
> > + * increasing complexity, and for large parts of that interface to be
> > + * entirely optional. The downside is more pointer chasing; chasing across
> > + * the __user boundary with pointers encapsulated inside u64.
> > + */
> > +struct i915_user_extension {
> > + __u64 next_extension;
> > + __u64 name;
>
> s/name/id/ ?
I think name is common parlance for extensions/parameters? At least I've
been using it like it was :) And I was trying to retrospectively restrict
'id' for handles tracked by an idr! :)
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 23/34] drm/i915: Share per-timeline HWSP using a slab suballocator
On 21/01/2019 22:21, Chris Wilson wrote:
If we restrict ourselves to only using a cacheline for each timeline's
HWSP (we could go smaller, but want to avoid needless polluting
cachelines on different engines between different contexts), then we can
suballocate a single 4k page into 64 different timeline HWSP. By
treating each fresh allocation as a slab of 64 entries, we can keep it
around for the next 64 allocation attempts until we need to refresh the
slab cache.
John Harrison noted the issue of fragmentation leading to the same worst
case performance of one page per timeline as before, which can be
mitigated by adopting a freelist.
v2: Keep all partially allocated HWSP on a freelist
This is still without migration, so it is possible for the system to end
up with each timeline in its own page, but we ensure that no new
allocation would needless allocate a fresh page!
v3: Throw a selftest at the allocator to try and catch invalid cacheline
reuse.
Signed-off-by: Chris Wilson
Cc: John Harrison
---
drivers/gpu/drm/i915/i915_drv.h | 4 +
drivers/gpu/drm/i915/i915_timeline.c | 117 ---
drivers/gpu/drm/i915/i915_timeline.h | 1 +
drivers/gpu/drm/i915/i915_vma.h | 12 ++
drivers/gpu/drm/i915/selftests/i915_random.c | 33 -
drivers/gpu/drm/i915/selftests/i915_random.h | 3 +
.../gpu/drm/i915/selftests/i915_timeline.c| 140 ++
7 files changed, 282 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 364067f811f7..c00eaf2889fb 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1978,6 +1978,10 @@ struct drm_i915_private {
struct i915_gt_timelines {
struct mutex mutex; /* protects list, tainted by GPU */
struct list_head list;
+
+ /* Pack multiple timelines' seqnos into the same page */
+ spinlock_t hwsp_lock;
+ struct list_head hwsp_free_list;
} timelines;
struct list_head active_rings;
diff --git a/drivers/gpu/drm/i915/i915_timeline.c
b/drivers/gpu/drm/i915/i915_timeline.c
index 8d5792311a8f..69ee33dfa340 100644
--- a/drivers/gpu/drm/i915/i915_timeline.c
+++ b/drivers/gpu/drm/i915/i915_timeline.c
@@ -9,6 +9,12 @@
#include "i915_timeline.h"
#include "i915_syncmap.h"
+struct i915_timeline_hwsp {
+ struct i915_vma *vma;
+ struct list_head free_link;
+ u64 free_bitmap;
+};
+
static struct i915_vma *__hwsp_alloc(struct drm_i915_private *i915)
{
struct drm_i915_gem_object *obj;
@@ -27,28 +33,92 @@ static struct i915_vma *__hwsp_alloc(struct
drm_i915_private *i915)
return vma;
}
-static int hwsp_alloc(struct i915_timeline *timeline)
+static struct i915_vma *
+hwsp_alloc(struct i915_timeline *timeline, int *offset)
{
- struct i915_vma *vma;
+ struct drm_i915_private *i915 = timeline->i915;
+ struct i915_gt_timelines *gt = &i915->gt.timelines;
+ struct i915_timeline_hwsp *hwsp;
+ int cacheline;
- vma = __hwsp_alloc(timeline->i915);
- if (IS_ERR(vma))
- return PTR_ERR(vma);
+ BUILD_BUG_ON(BITS_PER_TYPE(u64) * CACHELINE_BYTES > PAGE_SIZE);
- timeline->hwsp_ggtt = vma;
- timeline->hwsp_offset = 0;
+ spin_lock(>->hwsp_lock);
- return 0;
+ /* hwsp_free_list only contains HWSP that have available cachelines */
+ hwsp = list_first_entry_or_null(>->hwsp_free_list,
+ typeof(*hwsp), free_link);
+ if (!hwsp) {
+ struct i915_vma *vma;
+
+ spin_unlock(>->hwsp_lock);
+
+ hwsp = kmalloc(sizeof(*hwsp), GFP_KERNEL);
+ if (!hwsp)
+ return ERR_PTR(-ENOMEM);
+
+ vma = __hwsp_alloc(i915);
+ if (IS_ERR(vma)) {
+ kfree(hwsp);
+ return vma;
+ }
+
+ vma->private = hwsp;
+ hwsp->vma = vma;
+ hwsp->free_bitmap = ~0ull;
+
+ spin_lock(>->hwsp_lock);
+ list_add(&hwsp->free_link, >->hwsp_free_list);
+ }
+
+ GEM_BUG_ON(!hwsp->free_bitmap);
+ cacheline = __ffs64(hwsp->free_bitmap);
+ hwsp->free_bitmap &= ~BIT_ULL(cacheline);
+ if (!hwsp->free_bitmap)
+ list_del(&hwsp->free_link);
+
+ spin_unlock(>->hwsp_lock);
+
+ GEM_BUG_ON(hwsp->vma->private != hwsp);
+
+ *offset = cacheline * CACHELINE_BYTES;
+ return hwsp->vma;
+}
+
+static void hwsp_free(struct i915_timeline *timeline)
+{
+ struct i915_gt_timelines *gt = &timeline->i915->gt.timelines;
+ struct i915_timeline_hwsp *hwsp;
+
+ hwsp = i915_timeline_hwsp(timeline);
+ if (!hwsp) /* leave global HWSP alone! */
Later you add i915_timeline_is_g
Re: [Intel-gfx] [PATCH] iommu/intel: quirk to disable DMAR for QM57 igfx
On Fri, Jan 18, 2019 at 12:17:05PM +, Eric Wong wrote:
> @@ -5411,6 +5411,7 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x2e20,
> quirk_iommu_g4x_gfx);
> DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x2e30, quirk_iommu_g4x_gfx);
> DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x2e40, quirk_iommu_g4x_gfx);
> DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x2e90, quirk_iommu_g4x_gfx);
> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x0044, quirk_iommu_g4x_gfx);
>
> static void quirk_iommu_rwbf(struct pci_dev *dev)
> {
> @@ -5457,7 +5458,6 @@ static void quirk_calpella_no_shadow_gtt(struct pci_dev
> *dev)
> }
> }
> DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x0040,
> quirk_calpella_no_shadow_gtt);
> -DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x0044,
> quirk_calpella_no_shadow_gtt);
> DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x0062,
> quirk_calpella_no_shadow_gtt);
> DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x006a,
> quirk_calpella_no_shadow_gtt);
This seems to make sense to me. Joonas, any comments or objections?
Regards,
Joerg
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] iommu/intel: quirk to disable DMAR for QM57 igfx
On Tue, Jan 22, 2019 at 11:39 AM Joerg Roedel wrote:
>
> On Fri, Jan 18, 2019 at 12:17:05PM +, Eric Wong wrote:
> > @@ -5411,6 +5411,7 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x2e20,
> > quirk_iommu_g4x_gfx);
> > DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x2e30, quirk_iommu_g4x_gfx);
> > DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x2e40, quirk_iommu_g4x_gfx);
> > DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x2e90, quirk_iommu_g4x_gfx);
> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x0044, quirk_iommu_g4x_gfx);
> >
> > static void quirk_iommu_rwbf(struct pci_dev *dev)
> > {
> > @@ -5457,7 +5458,6 @@ static void quirk_calpella_no_shadow_gtt(struct
> > pci_dev *dev)
> > }
> > }
> > DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x0040,
> > quirk_calpella_no_shadow_gtt);
> > -DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x0044,
> > quirk_calpella_no_shadow_gtt);
> > DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x0062,
> > quirk_calpella_no_shadow_gtt);
> > DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x006a,
> > quirk_calpella_no_shadow_gtt);
>
> This seems to make sense to me. Joonas, any comments or objections?
This is ironlake, which has a huge iommu hack in the gpu driver to
work around hard hangs, which:
- causes massive stalls and kills performance
- isn't well tested (it's the only one that needs this), so tends to break
So if we do this then imo we should:
- probably nuke that w/a too (check for needs_idle_maps and all the
related stuff in i915_gem_gtt.c)
- roll it out for all affected chips (i.e. need to include 0x0040).
Note that the string of platforms which have various issues with iommu
and igfx is very long, thus far we only disabled it where there's no
workaround to stop it from hanging the box, but otherwise left it
enabled. So if we make a policy change to also disable it anywhere
where it doesn't work well (instead of not at all), there's a pile
more platforms to switch.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] dma-buf: Enhance dma-fence tracing
On Tue, Jan 22, 2019 at 10:58 AM Chris Wilson wrote: > > Quoting Daniel Vetter (2019-01-22 09:11:53) > > On Tue, Jan 22, 2019 at 10:06 AM Chris Wilson > > wrote: > > > > > > Quoting Koenig, Christian (2019-01-22 08:49:30) > > > > Am 22.01.19 um 00:20 schrieb Chris Wilson: > > > > > Rather than every backend and GPU driver reinventing the same wheel > > > > > for > > > > > user level debugging of HW execution, the common dma-fence framework > > > > > should include the tracing infrastructure required for most client API > > > > > level flow visualisation. > > > > > > > > > > With these common dma-fence level tracepoints, the userspace tools can > > > > > establish a detailed view of the client <-> HW flow across different > > > > > kernels. There is a strong ask to have this available, so that the > > > > > userspace developer can effectively assess if they're doing a good job > > > > > about feeding the beast of a GPU hardware. > > > > > > > > > > In the case of needing to look into more fine-grained details of how > > > > > kernel internals work towards the goal of feeding the beast, the tools > > > > > may optionally amend the dma-fence tracing information with the driver > > > > > implementation specific. But for such cases, the tools should have a > > > > > graceful degradation in case the expected extra tracepoints have > > > > > changed or their format differs from the expected, as the kernel > > > > > implementation internals are not expected to stay the same. > > > > > > > > > > It is important to distinguish between tracing for the purpose of > > > > > client > > > > > flow visualisation and tracing for the purpose of low-level kernel > > > > > debugging. The latter is highly implementation specific, tied to > > > > > a particular HW and driver, whereas the former addresses a common goal > > > > > of user level tracing and likely a common set of userspace tools. > > > > > Having made the distinction that these tracepoints will be consumed > > > > > for > > > > > client API tooling, we raise the spectre of tracepoint ABI stability. > > > > > It > > > > > is hoped that by defining a common set of dma-fence tracepoints, we > > > > > avoid > > > > > the pitfall of exposing low level details and so restrict ourselves > > > > > only > > > > > to the high level flow that is applicable to all drivers and hardware. > > > > > Thus the reserved guarantee that this set of tracepoints will be > > > > > stable > > > > > (with the emphasis on depicting client <-> HW flow as opposed to > > > > > driver <-> HW). > > > > > > > > > > In terms of specific changes to the dma-fence tracing, we remove the > > > > > emission of the strings for every tracepoint (reserving them for > > > > > dma_fence_init for cases where they have unique dma_fence_ops, and > > > > > preferring to have descriptors for the whole fence context). strings > > > > > do > > > > > not pack as well into the ftrace ringbuffer and we would prefer to > > > > > reduce the amount of indirect callbacks required for frequent > > > > > tracepoint > > > > > emission. > > > > > > > > > > Signed-off-by: Chris Wilson > > > > > Cc: Joonas Lahtinen > > > > > Cc: Tvrtko Ursulin > > > > > Cc: Alex Deucher > > > > > Cc: "Christian König" > > > > > Cc: Eric Anholt > > > > > Cc: Pierre-Loup Griffais > > > > > Cc: Michael Sartain > > > > > Cc: Steven Rostedt > > > > > > > > In general yes please! If possible please separate out the changes to > > > > the common dma_fence infrastructure from the i915 changes. > > > > > > Sure, I was just stressing the impact: remove some randomly placed > > > internal debugging tracepoints, try to define useful ones instead :) > > > > > > On the list of things to do was to convert at least 2 other drivers > > > (I was thinking nouveau/msm for simplicity, vc4 for a simpler > > > introduction to drm_sched than amdgpu) over to be sure we have the right > > > tracepoints. > > > > I think sprinkling these over the scheduler (maybe just as an opt-in, > > for the case where the driver doesn't have some additional queueing > > somewhere) would be good. I haven't checked whether it fits, but would > > give you a bunch of drivers at once. It might also not cover all the > > cases (I guess the wait related ones would need to be somewhere else). > > And the other thing (that got explicitly asked for!) was that we have > some igt to make sure we don't surreptitiously break the tracepoints > in future. > > Another task would to devise the set of tracepoints to describe the > modesetting flow; that more or less is the flow of atomic helpers I > guess: prepare; wait-on-fences; commit; signal; cleanup. For system > snooping, knowing a target frame (msc or ust) and how late it was > delayed and the HW execution flow up to the frame and being able to tie > that back to the GL/VK client is the grand plan. Yeah with atomic helpers this should be doable, as long as the driver uses the commit tracking part of the helpers. That's the st
Re: [Intel-gfx] [PATCH] dma-buf: Enhance dma-fence tracing
Quoting Daniel Vetter (2019-01-22 09:11:53) > On Tue, Jan 22, 2019 at 10:06 AM Chris Wilson > wrote: > > > > Quoting Koenig, Christian (2019-01-22 08:49:30) > > > Am 22.01.19 um 00:20 schrieb Chris Wilson: > > > > Rather than every backend and GPU driver reinventing the same wheel for > > > > user level debugging of HW execution, the common dma-fence framework > > > > should include the tracing infrastructure required for most client API > > > > level flow visualisation. > > > > > > > > With these common dma-fence level tracepoints, the userspace tools can > > > > establish a detailed view of the client <-> HW flow across different > > > > kernels. There is a strong ask to have this available, so that the > > > > userspace developer can effectively assess if they're doing a good job > > > > about feeding the beast of a GPU hardware. > > > > > > > > In the case of needing to look into more fine-grained details of how > > > > kernel internals work towards the goal of feeding the beast, the tools > > > > may optionally amend the dma-fence tracing information with the driver > > > > implementation specific. But for such cases, the tools should have a > > > > graceful degradation in case the expected extra tracepoints have > > > > changed or their format differs from the expected, as the kernel > > > > implementation internals are not expected to stay the same. > > > > > > > > It is important to distinguish between tracing for the purpose of client > > > > flow visualisation and tracing for the purpose of low-level kernel > > > > debugging. The latter is highly implementation specific, tied to > > > > a particular HW and driver, whereas the former addresses a common goal > > > > of user level tracing and likely a common set of userspace tools. > > > > Having made the distinction that these tracepoints will be consumed for > > > > client API tooling, we raise the spectre of tracepoint ABI stability. It > > > > is hoped that by defining a common set of dma-fence tracepoints, we > > > > avoid > > > > the pitfall of exposing low level details and so restrict ourselves only > > > > to the high level flow that is applicable to all drivers and hardware. > > > > Thus the reserved guarantee that this set of tracepoints will be stable > > > > (with the emphasis on depicting client <-> HW flow as opposed to > > > > driver <-> HW). > > > > > > > > In terms of specific changes to the dma-fence tracing, we remove the > > > > emission of the strings for every tracepoint (reserving them for > > > > dma_fence_init for cases where they have unique dma_fence_ops, and > > > > preferring to have descriptors for the whole fence context). strings do > > > > not pack as well into the ftrace ringbuffer and we would prefer to > > > > reduce the amount of indirect callbacks required for frequent tracepoint > > > > emission. > > > > > > > > Signed-off-by: Chris Wilson > > > > Cc: Joonas Lahtinen > > > > Cc: Tvrtko Ursulin > > > > Cc: Alex Deucher > > > > Cc: "Christian König" > > > > Cc: Eric Anholt > > > > Cc: Pierre-Loup Griffais > > > > Cc: Michael Sartain > > > > Cc: Steven Rostedt > > > > > > In general yes please! If possible please separate out the changes to > > > the common dma_fence infrastructure from the i915 changes. > > > > Sure, I was just stressing the impact: remove some randomly placed > > internal debugging tracepoints, try to define useful ones instead :) > > > > On the list of things to do was to convert at least 2 other drivers > > (I was thinking nouveau/msm for simplicity, vc4 for a simpler > > introduction to drm_sched than amdgpu) over to be sure we have the right > > tracepoints. > > I think sprinkling these over the scheduler (maybe just as an opt-in, > for the case where the driver doesn't have some additional queueing > somewhere) would be good. I haven't checked whether it fits, but would > give you a bunch of drivers at once. It might also not cover all the > cases (I guess the wait related ones would need to be somewhere else). And the other thing (that got explicitly asked for!) was that we have some igt to make sure we don't surreptitiously break the tracepoints in future. Another task would to devise the set of tracepoints to describe the modesetting flow; that more or less is the flow of atomic helpers I guess: prepare; wait-on-fences; commit; signal; cleanup. For system snooping, knowing a target frame (msc or ust) and how late it was delayed and the HW execution flow up to the frame and being able to tie that back to the GL/VK client is the grand plan. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] Forced push done to drm-intel-next-queued
On Tue, Jan 15, 2019 at 12:46:50PM +0100, Hans de Goede wrote:
> Hi,
>
> On 15-01-19 10:56, Daniel Vetter wrote:
> > On Thu, Dec 27, 2018 at 04:24:51PM +0100, Hans de Goede wrote:
> > > Hi,
> > >
> > > On 27-12-18 15:42, Jani Nikula wrote:
> > > > On Tue, 25 Dec 2018, Hans de Goede wrote:
> > > > > Hi,
> > > > >
> > > > > As mentioned in the "I messed up drm-intel-next-queued!" mail-thread
> > > > > I made a big mistake yesterday:
> > > > >
> > > > > "Ugh, I just messed up drm-intel-next-queued big time.
> > > > >
> > > > > I somehow rebased my work on top of drm-tip (I believe I did the
> > > > > rebase
> > > > > in the wrong dir) and then after running a bunch of tests I
> > > > > did a "dim push-branch drm-intel-next-queued" which pushed the
> > > > > patches I intended to push rebased on top of drm-tip
> > > > > pushing drm-tip to dinq :(
> > > > >
> > > > > I'm so sorry about this.
> > > > >
> > > > > I just checked my reflog and the last commit before me messing
> > > > > up is commit d4de753526f4d99f541f1b6ed1d963005c09700c
> > > > > ("drm/i915: Unwind failure on pinning the gen7 ppgtt")"
> > > > >
> > > > > To fix this I've just done a forced push resetting
> > > > > drm-intel-next-queued to the mentioned d4de753526f4 commit.
> > > > >
> > > > > I first checked no-one pushed anything on top of my mess,
> > > > > but if you pushed anything to drm-intel-next-queued in the
> > > > > last 24 hours, please double-check it is there.
> > > > >
> > > > > Once more my apologies for this.
> > > >
> > > > It happens, don't worry about it. Thanks for being open about it instead
> > > > of trying to brush it under the rug.
> > >
> > > Thanks.
> > >
> > > > Did you pass -f to dim? I suspect drm-tip wouldn't pass the push checks
> > > > in dim without it. Perhaps we'll need to add more.
> > >
> > > No I did not pass -f, I did wonder myself how the push managed to
> > > proceed after my screw-up. Looking at how dim builds drm-tip, it seems
> > > it starts with dinq and then merges in other branches, so a push
> > > from a drm-tip based branch to dinq is a fast-forward (I think),
> > > so dinq is special in this case.
> >
> > Do you still have the git tree you've pushed wrongly and could publish it
> > somewhere? I'm suprised that dim push didn't catch this, we've added a few
> > more sanity checks last time around this happened. I'd have expected dim
> > to complain about all the patches lacking you're signed-off-by, since a
> > rebase would have changed a lot of patches to be committed by you.
>
> I'm afraid I don't have that tree around anymore. What I did was I
> accidentally
> rebased the 2 patches I wanted to push on top of drm-tip, so I pushed the
> last drm-tip push (including the integration manifest) with my 2 patches on
> top.
>
> So I pushed the latest unmodified state of drm-tip and none of the patches
> in there were re-commited by me during the rebase.
>
> Basically what I believe I pushed is a fast-forward from dinq to drm-tip +
> the 2 patches I intended to push.
Hm right we filter for committer, so if you're not the one who pushed the
very latest drm-tip then all the merge commits in there (and the
integration manifest) won't trigger and checks of dim push.
> I even did a gitk before pushing to graphically check what I was pushing
> (I always do this) and I saw my 2 patches on top of a remote branch, which
> looked fine. What I did not notice it was the wrong remote and the wrong
> branch. This is something which I've learned to also check now.
>
> One check which I believe would have caught this is checking there are no
> merge-commits in what is being pushed and require some commandline option
> to override this for special cases.
The problem with that is that you train maintainers (who routinely need to
push merge commits, that's their job) to ignore dim warnings. Which also
is not great. Jani&me had a few discussions and failed starts (had to back
a few changes out of dim again), not sure what exactly we could check more
now :-(
I guess mea culpa, perhaps we'll come up with a better idea in the future.
Thanks anyway for helping with debugging this tooling issue.
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✓ Fi.CI.IGT: success for series starting with [v2,1/7] drm/i915/crt: split out intel_crt_present() to platform specific setup
== Series Details ==
Series: series starting with [v2,1/7] drm/i915/crt: split out
intel_crt_present() to platform specific setup
URL : https://patchwork.freedesktop.org/series/55540/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_5459_full -> Patchwork_12003_full
Summary
---
**SUCCESS**
No regressions found.
Known issues
Here are the changes found in Patchwork_12003_full that come from known
issues:
### IGT changes ###
Issues hit
* igt@gem_exec_schedule@pi-ringfull-blt:
- shard-apl: NOTRUN -> FAIL [fdo#103158]
* igt@kms_content_protection@legacy:
- shard-apl: NOTRUN -> FAIL [fdo#108597]
* igt@kms_cursor_crc@cursor-128x42-sliding:
- shard-apl: PASS -> FAIL [fdo#103232]
* igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-indfb-draw-mmap-wc:
- shard-glk: PASS -> FAIL [fdo#103167]
* igt@kms_plane@plane-position-covered-pipe-c-planes:
- shard-apl: PASS -> FAIL [fdo#103166]
Possible fixes
* igt@kms_busy@extended-pageflip-hang-oldfb-render-b:
- shard-snb: {SKIP} [fdo#109271] / [fdo#109278] -> PASS
* igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-b:
- shard-apl: DMESG-WARN [fdo#107956] -> PASS +1
* igt@kms_ccs@pipe-b-crc-sprite-planes-basic:
- shard-glk: FAIL [fdo#108145] -> PASS
* igt@kms_cursor_crc@cursor-128x42-random:
- shard-apl: FAIL [fdo#103232] -> PASS +1
* igt@kms_draw_crc@draw-method-xrgb-mmap-gtt-untiled:
- shard-snb: {SKIP} [fdo#109271] -> PASS +4
* igt@kms_plane@plane-position-covered-pipe-c-planes:
- shard-glk: FAIL [fdo#103166] -> PASS
* igt@kms_rotation_crc@multiplane-rotation:
- shard-kbl: FAIL -> PASS
* igt@kms_rotation_crc@multiplane-rotation-cropping-top:
- shard-apl: DMESG-FAIL [fdo#108950] -> PASS
* igt@kms_setmode@basic:
- shard-apl: INCOMPLETE [fdo#103927] -> PASS
Warnings
* igt@kms_rotation_crc@multiplane-rotation-cropping-top:
- shard-glk: DMESG-FAIL [fdo#105763] / [fdo#106538] -> FAIL
[fdo#109381]
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#103158]: https://bugs.freedesktop.org/show_bug.cgi?id=103158
[fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
[fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
[fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
[fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
[fdo#105763]: https://bugs.freedesktop.org/show_bug.cgi?id=105763
[fdo#106538]: https://bugs.freedesktop.org/show_bug.cgi?id=106538
[fdo#107956]: https://bugs.freedesktop.org/show_bug.cgi?id=107956
[fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
[fdo#108597]: https://bugs.freedesktop.org/show_bug.cgi?id=108597
[fdo#108950]: https://bugs.freedesktop.org/show_bug.cgi?id=108950
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
[fdo#109381]: https://bugs.freedesktop.org/show_bug.cgi?id=109381
Participating hosts (7 -> 5)
--
Missing(2): shard-skl shard-iclb
Build changes
-
* Linux: CI_DRM_5459 -> Patchwork_12003
CI_DRM_5459: 0f693a275dd91391b476ada7481cf08f4fe610aa @
git://anongit.freedesktop.org/gfx-ci/linux
IGT_4780: 1c1612bdc36b44a704095e7b0ba5542818ce793f @
git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_12003: f6c1b42bdd25a3c26ffd12155b322eb57131deba @
git://anongit.freedesktop.org/gfx-ci/linux
piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @
git://anongit.freedesktop.org/piglit
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12003/
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/3] drm: Add a debug print for drm_modeset_backoff()
On Mon, Jan 21, 2019 at 10:24:30PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> Logs can get confusing when some operations are done multiple times
> due to the ww mutex backoff. Add a debug print into
> drm_modeset_backoff() so that at least the reason for the odd
> looking logs will be obvious.
>
> Signed-off-by: Ville Syrjälä
Makes sense. Reviewed-by: Daniel Vetter
> ---
> drivers/gpu/drm/drm_modeset_lock.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_modeset_lock.c
> b/drivers/gpu/drm/drm_modeset_lock.c
> index 81dd11901ffd..1277ff18d993 100644
> --- a/drivers/gpu/drm/drm_modeset_lock.c
> +++ b/drivers/gpu/drm/drm_modeset_lock.c
> @@ -295,6 +295,8 @@ int drm_modeset_backoff(struct drm_modeset_acquire_ctx
> *ctx)
> {
> struct drm_modeset_lock *contended = ctx->contended;
>
> + DRM_DEBUG_KMS("Retrying to avoid deadlock\n");
> +
> ctx->contended = NULL;
>
> if (WARN_ON(!contended))
> --
> 2.19.2
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/3] drm: Sync errno values for property lookup errors
On Mon, Jan 21, 2019 at 10:24:29PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> Use ENOENT consistently for the case where the requested property
> isn't found, and EINVAL for the case where the object has no
> properties whatsoever. Currenrly these are handled differently
> in the atomic and legacy codepaths.
>
> Signed-off-by: Ville Syrjälä
Matches
https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#recommended-ioctl-return-values
Reviewed-by: Daniel Vetter
Any igts that blow up with this? We should have at least some trying to do
invalid stuff ...
-Daniel
> ---
> drivers/gpu/drm/drm_atomic_uapi.c | 2 +-
> drivers/gpu/drm/drm_mode_object.c | 1 +
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c
> index 06390307e5a3..2a54f826cf65 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -1330,7 +1330,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
> DRM_DEBUG_ATOMIC("Object ID %d has no properties\n",
>obj_id);
> drm_mode_object_put(obj);
> - ret = -ENOENT;
> + ret = -EINVAL;
> goto out;
> }
>
> diff --git a/drivers/gpu/drm/drm_mode_object.c
> b/drivers/gpu/drm/drm_mode_object.c
> index e8dac94d576d..31730d935842 100644
> --- a/drivers/gpu/drm/drm_mode_object.c
> +++ b/drivers/gpu/drm/drm_mode_object.c
> @@ -527,6 +527,7 @@ int drm_mode_obj_set_property_ioctl(struct drm_device
> *dev, void *data,
> property = drm_mode_obj_find_prop_id(arg_obj, arg->prop_id);
> if (!property) {
> DRM_DEBUG_KMS("Unknown property ID %d\n", arg->prop_id);
> + ret = -ENOENT;
> goto out_unref;
> }
>
> --
> 2.19.2
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/3] drm: Add debug prints for the various object lookup errors
On Mon, Jan 21, 2019 at 10:24:28PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> Only some of the drm mode object lookups have a corresponding debug
> print for the lookup failure. That makes logs a bit hard to parse
> when you can't see where the bad object ID is being used. Add a bunch
> more debug prints, and unify their appearance.
>
> Signed-off-by: Ville Syrjälä
Instead of sprinkling these all over, what about the reverse route and
pushing this into drm_mode_object_find? We can dump id + object type, that
should be all we need really. If we go this way maybe add kerneldoc to the
various drm_*_find/lookup functions so this doesn't get copypasted again
...
-Daniel
> ---
> drivers/gpu/drm/drm_atomic_uapi.c | 5 +
> drivers/gpu/drm/drm_color_mgmt.c | 8 ++--
> drivers/gpu/drm/drm_connector.c | 5 -
> drivers/gpu/drm/drm_crtc.c| 12 +++-
> drivers/gpu/drm/drm_encoder.c | 4 +++-
> drivers/gpu/drm/drm_framebuffer.c | 4 +++-
> drivers/gpu/drm/drm_mode_object.c | 17 ++---
> drivers/gpu/drm/drm_plane.c | 13 +
> drivers/gpu/drm/drm_property.c| 12 +---
> drivers/gpu/drm/drm_vblank.c | 8 ++--
> 10 files changed, 66 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c
> index 9a1f41adfc67..06390307e5a3 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -1321,11 +1321,14 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>
> obj = drm_mode_object_find(dev, file_priv, obj_id,
> DRM_MODE_OBJECT_ANY);
> if (!obj) {
> + DRM_DEBUG_ATOMIC("Unknown object ID %d\n", obj_id);
> ret = -ENOENT;
> goto out;
> }
>
> if (!obj->properties) {
> + DRM_DEBUG_ATOMIC("Object ID %d has no properties\n",
> + obj_id);
> drm_mode_object_put(obj);
> ret = -ENOENT;
> goto out;
> @@ -1352,6 +1355,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>
> prop = drm_mode_obj_find_prop_id(obj, prop_id);
> if (!prop) {
> + DRM_DEBUG_ATOMIC("Unknown property ID %d\n",
> + prop_id);
> drm_mode_object_put(obj);
> ret = -ENOENT;
> goto out;
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c
> b/drivers/gpu/drm/drm_color_mgmt.c
> index 07dcf47daafe..a99ee15b8328 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -245,8 +245,10 @@ int drm_mode_gamma_set_ioctl(struct drm_device *dev,
> return -EOPNOTSUPP;
>
> crtc = drm_crtc_find(dev, file_priv, crtc_lut->crtc_id);
> - if (!crtc)
> + if (!crtc) {
> + DRM_DEBUG_KMS("Unknown CRTC ID %d\n", crtc_lut->crtc_id);
> return -ENOENT;
> + }
>
> if (crtc->funcs->gamma_set == NULL)
> return -ENOSYS;
> @@ -313,8 +315,10 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
> return -EOPNOTSUPP;
>
> crtc = drm_crtc_find(dev, file_priv, crtc_lut->crtc_id);
> - if (!crtc)
> + if (!crtc) {
> + DRM_DEBUG_KMS("Unknown CRTC ID %d\n", crtc_lut->crtc_id);
> return -ENOENT;
> + }
>
> /* memcpy into gamma store */
> if (crtc_lut->gamma_size != crtc->gamma_size)
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 847539645558..8745eb132fd4 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1952,8 +1952,11 @@ int drm_mode_getconnector(struct drm_device *dev, void
> *data,
> memset(&u_mode, 0, sizeof(struct drm_mode_modeinfo));
>
> connector = drm_connector_lookup(dev, file_priv,
> out_resp->connector_id);
> - if (!connector)
> + if (!connector) {
> + DRM_DEBUG_KMS("Unknown connector ID %d\n",
> + out_resp->connector_id);
> return -ENOENT;
> + }
>
> drm_connector_for_each_possible_encoder(connector, encoder, i)
> encoders_count++;
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 7dabbaf033a1..e5f234ffcd23 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -369,8 +369,10 @@ int drm_mode_getcrtc(struct drm_device *dev,
> return -EOPNOTSUPP;
>
> crtc = drm_crtc_find(dev, file_priv, crtc_resp->crtc_id);
> - if (!crtc)
> + if (!crtc) {
> + DRM_DEBUG_KMS("Unknown CRTC ID %d\n", crtc_resp->crtc_id);
> return -ENOENT;
> + }
>
> plane = crtc->primary;
>
> @@ -586
Re: [Intel-gfx] [PATCH 19/34] drm/i915: Tidy common test_bit probing of i915_request->fence.flags
On 21/01/2019 22:21, Chris Wilson wrote:
A repeated pattern is to test the signaled bit of our
request->fence.flags. Make this an inline to shorten a few lines and
remove unnecessary line continuations.
Signed-off-by: Chris Wilson
---
drivers/gpu/drm/i915/i915_irq.c | 3 +--
drivers/gpu/drm/i915/i915_request.c | 2 +-
drivers/gpu/drm/i915/i915_request.h | 5 +
drivers/gpu/drm/i915/intel_breadcrumbs.c | 3 +--
drivers/gpu/drm/i915/intel_lrc.c | 2 +-
drivers/gpu/drm/i915/intel_pm.c | 2 +-
drivers/gpu/drm/i915/intel_ringbuffer.c | 3 +--
7 files changed, 11 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 1abfc3fa76ad..5fd5080c4ccb 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1182,8 +1182,7 @@ static void notify_ring(struct intel_engine_cs *engine)
struct i915_request *waiter = wait->request;
if (waiter &&
- !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
- &waiter->fence.flags) &&
+ !i915_request_signaled(waiter) &&
intel_wait_check_request(wait, waiter))
rq = i915_request_get(waiter);
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 80232de8e2be..2721a356368f 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -198,7 +198,7 @@ static void __retire_engine_request(struct intel_engine_cs
*engine,
spin_unlock(&engine->timeline.lock);
spin_lock(&rq->lock);
- if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &rq->fence.flags))
+ if (!i915_request_signaled(rq))
dma_fence_signal_locked(&rq->fence);
if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &rq->fence.flags))
intel_engine_cancel_signaling(rq);
diff --git a/drivers/gpu/drm/i915/i915_request.h
b/drivers/gpu/drm/i915/i915_request.h
index d014b0605445..c0f084ca4f29 100644
--- a/drivers/gpu/drm/i915/i915_request.h
+++ b/drivers/gpu/drm/i915/i915_request.h
@@ -280,6 +280,11 @@ long i915_request_wait(struct i915_request *rq,
#define I915_WAIT_ALL BIT(3) /* used by i915_gem_object_wait() */
#define I915_WAIT_FOR_IDLE_BOOST BIT(4)
+static inline bool i915_request_signaled(const struct i915_request *rq)
+{
+ return test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &rq->fence.flags);
+}
+
static inline bool intel_engine_has_started(struct intel_engine_cs *engine,
u32 seqno);
static inline bool intel_engine_has_completed(struct intel_engine_cs *engine,
diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c
b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index 4fad93fe3678..b58915b8708b 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -631,8 +631,7 @@ static int intel_breadcrumbs_signaler(void *arg)
rq->signaling.wait.seqno = 0;
__list_del_entry(&rq->signaling.link);
-if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
- &rq->fence.flags)) {
+ if (!i915_request_signaled(rq)) {
list_add_tail(&rq->signaling.link,
&list);
i915_request_get(rq);
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index bc65d8006e16..464dd309fa99 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -855,7 +855,7 @@ static void execlists_cancel_requests(struct
intel_engine_cs *engine)
list_for_each_entry(rq, &engine->timeline.requests, link) {
GEM_BUG_ON(!rq->global_seqno);
- if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &rq->fence.flags))
+ if (i915_request_signaled(rq))
continue;
dma_fence_set_error(&rq->fence, -EIO);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 8b63afa3a221..fdc28a3d2936 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -6662,7 +6662,7 @@ void gen6_rps_boost(struct i915_request *rq,
if (!rps->enabled)
return;
- if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &rq->fence.flags))
+ if (i915_request_signaled(rq))
return;
/* Serializes with i915_request_retire() */
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 66dc8e2fa353..bc620ae297b4 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -876,8 +876,7 @@ static void cancel_requests(struct intel_engine_cs *engine)
Re: [Intel-gfx] [PATCH 18/34] drm/i915/selftests: Use common mock_engine::advance
On 21/01/2019 22:21, Chris Wilson wrote:
Replace the open-coding of advance with a call instead.
Signed-off-by: Chris Wilson
---
drivers/gpu/drm/i915/selftests/mock_engine.c | 17 +++--
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/selftests/mock_engine.c
b/drivers/gpu/drm/i915/selftests/mock_engine.c
index 968a7e139a67..386dfa7e2d5c 100644
--- a/drivers/gpu/drm/i915/selftests/mock_engine.c
+++ b/drivers/gpu/drm/i915/selftests/mock_engine.c
@@ -67,11 +67,10 @@ static struct mock_request *first_request(struct
mock_engine *engine)
link);
}
-static void advance(struct mock_engine *engine,
- struct mock_request *request)
+static void advance(struct mock_request *request)
{
list_del_init(&request->link);
- mock_seqno_advance(&engine->base, request->base.global_seqno);
+ mock_seqno_advance(request->base.engine, request->base.global_seqno);
}
static void hw_delay_complete(struct timer_list *t)
@@ -84,7 +83,7 @@ static void hw_delay_complete(struct timer_list *t)
/* Timer fired, first request is complete */
request = first_request(engine);
if (request)
- advance(engine, request);
+ advance(request);
/*
* Also immediately signal any subsequent 0-delay requests, but
@@ -96,7 +95,7 @@ static void hw_delay_complete(struct timer_list *t)
break;
}
- advance(engine, request);
+ advance(request);
}
spin_unlock(&engine->hw_lock);
@@ -180,7 +179,7 @@ static void mock_submit_request(struct i915_request
*request)
if (mock->delay)
mod_timer(&engine->hw_delay, jiffies + mock->delay);
else
- advance(engine, mock);
+ advance(mock);
}
spin_unlock_irq(&engine->hw_lock);
}
@@ -240,10 +239,8 @@ void mock_engine_flush(struct intel_engine_cs *engine)
del_timer_sync(&mock->hw_delay);
spin_lock_irq(&mock->hw_lock);
- list_for_each_entry_safe(request, rn, &mock->hw_queue, link) {
- list_del_init(&request->link);
- mock_seqno_advance(&mock->base, request->base.global_seqno);
- }
+ list_for_each_entry_safe(request, rn, &mock->hw_queue, link)
+ advance(request);
spin_unlock_irq(&mock->hw_lock);
}
Reviewed-by: Tvrtko Ursulin
Regards,
Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 27/38] drm/i915: Introduce the i915_user_extension_method
On 18/01/2019 14:00, Chris Wilson wrote:
An idea for extending uABI inspired by Vulkan's extension chains.
Instead of expanding the data struct for each ioctl every time we need
to add a new feature, define an extension chain instead. As we add
optional interfaces to control the ioctl, we define a new extension
struct that can be linked into the ioctl data only when required by the
user. The key advantage being able to ignore large control structs for
optional interfaces/extensions, while being able to process them in a
consistent manner.
In comparison to other extensible ioctls, the key difference is the
use of a linked chain of extension structs vs an array of tagged
pointers. For example,
struct drm_amdgpu_cs_chunk {
__u32 chunk_id;
__u32 length_dw;
__u64 chunk_data;
};
struct drm_amdgpu_cs_in {
__u32 ctx_id;
__u32 bo_list_handle;
__u32 num_chunks;
__u32 _pad;
__u64 chunks;
};
allows userspace to pass in array of pointers to extension structs, but
must therefore keep constructing that array along side the command stream.
In dynamic situations like that, a linked list is preferred and does not
similar from extra cache line misses as the extension structs themselves
s/similar/suffer/ I think.
must still be loaded separate to the chunks array.
v2: Apply the tail call optimisation directly to nip the worry of stack
overflow in the bud.
Signed-off-by: Chris Wilson
---
drivers/gpu/drm/i915/Makefile | 1 +
drivers/gpu/drm/i915/i915_user_extensions.c | 42 +
drivers/gpu/drm/i915/i915_user_extensions.h | 20 ++
include/uapi/drm/i915_drm.h | 20 ++
4 files changed, 83 insertions(+)
create mode 100644 drivers/gpu/drm/i915/i915_user_extensions.c
create mode 100644 drivers/gpu/drm/i915/i915_user_extensions.h
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 65ed00db..f206fbc85cee 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -45,6 +45,7 @@ i915-y := i915_drv.o \
i915_sw_fence.o \
i915_syncmap.o \
i915_sysfs.o \
+ i915_user_extensions.o \
intel_csr.o \
intel_device_info.o \
intel_pm.o \
diff --git a/drivers/gpu/drm/i915/i915_user_extensions.c
b/drivers/gpu/drm/i915/i915_user_extensions.c
new file mode 100644
index ..5d90c368f185
--- /dev/null
+++ b/drivers/gpu/drm/i915/i915_user_extensions.c
@@ -0,0 +1,42 @@
+/*
+ * SPDX-License-Identifier: MIT
+ *
+ * Copyright © 2018 Intel Corporation
+ */
+
+#include
+#include
+#include
+
+#include "i915_user_extensions.h"
+
+int i915_user_extensions(struct i915_user_extension __user *ext,
+const i915_user_extension_fn *tbl,
+unsigned long count,
I would be tempted to limit the count to unsigned int. 4 billion
extensions should be enough for everyone. :)
ABI can remain u64, but implementation I think in this form does not
need it.
+void *data)
+{
+ while (ext) {
+ int err;
+ u64 x;
+
+ cond_resched();
+ if (signal_pending(current))
+ return -EINTR;
What was your thinking behind this? It feels like, since here we are not
doing any explicit wait/sleeps, that at this level the code shouldn't
bother with it.
+
+ if (get_user(x, &ext->name))
+ return -EFAULT;
+
+ err = -EINVAL;
+ if (x < count && tbl[x])
+ err = tbl[x](ext, data);
+ if (err)
+ return err;
We talked about providing unwind on failure ie. option for destructor
call backs. You gave up on that?
+
+ if (get_user(x, &ext->next_extension))
+ return -EFAULT;
+
+ ext = u64_to_user_ptr(x);
+ }
+
+ return 0;
+}
diff --git a/drivers/gpu/drm/i915/i915_user_extensions.h
b/drivers/gpu/drm/i915/i915_user_extensions.h
new file mode 100644
index ..313a510b068a
--- /dev/null
+++ b/drivers/gpu/drm/i915/i915_user_extensions.h
@@ -0,0 +1,20 @@
+/*
+ * SPDX-License-Identifier: MIT
+ *
+ * Copyright © 2018 Intel Corporation
+ */
+
+#ifndef I915_USER_EXTENSIONS_H
+#define I915_USER_EXTENSIONS_H
+
+struct i915_user_extension;
+
+typedef int (*i915_user_extension_fn)(struct i915_user_extension __user *ext,
+ void *data);
+
+int i915_user_extensions(struct i915_user_extension __user *ext,
+const i915_user_extension_fn *tbl,
+unsigned long count,
+void *data);
+
+#endif /* I915_USER_EXTENSIONS_H */
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 298b2e197
Re: [Intel-gfx] [PATCH] drm/i915: Don't use the second dbuf slice on icl
Hi,
On Mon, Jan 21, 2019 at 9:01 PM Ville Syrjala
wrote:
>
> From: Ville Syrjälä
>
> The code managing the dbuf slices is borked and needs some
> real work to fix. In the meantime let's just stop using the
> second slice.
>
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/intel_pm.c | 10 --
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 8b63afa3a221..1e41c899ffe2 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3618,7 +3618,8 @@ static u8 intel_enabled_dbuf_slices_num(struct
> drm_i915_private *dev_priv)
> enabled_slices = 1;
>
> /* Gen prior to GEN11 have only one DBuf slice */
> - if (INTEL_GEN(dev_priv) < 11)
> + /* FIXME dbuf slice code is broken: see intel_get_ddb_size() */
> + if (1 || INTEL_GEN(dev_priv) < 11)
> return enabled_slices;
IMHO we may not need this, If we return from above we'll never disable
second slice in case it's enabled by bios.
Anyhow code changes in intel_get_ddb_size will take care of enabling
only one slice.
>
> if (I915_READ(DBUF_CTL_S2) & DBUF_POWER_STATE)
> @@ -3827,8 +3828,13 @@ static u16 intel_get_ddb_size(struct drm_i915_private
> *dev_priv,
>
> /*
> * 12GB/s is maximum BW supported by single DBuf slice.
> +*
> +* FIXME dbuf slice code is broken:
> +* - must wait for planes to stop using the slice before powering it
> off
AFAIR we're already doing it and disabling slice only after
update_crtcs, and skl_update_crtcs code is taking care of waiting for
vblank in case it's required.
> +* - plane straddling both slices is illegal in multi-pipe scenarios
This is something new :)
although this change introduce a major limitation with number and size
of planes we can display, yet
As code is broken and mentioned conditions need to be taken care of,
This change should be ok until proper fix.
~Mahesh
> +* - should validate we stay within the hw bandwidth limits
> */
> - if (num_active > 1 || total_data_bw >= GBps(12)) {
> + if (0 && (num_active > 1 || total_data_bw >= GBps(12))) {
> ddb->enabled_slices = 2;
> } else {
> ddb->enabled_slices = 1;
> --
> 2.19.2
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Don't use the second dbuf slice on icl
Hi,
On Mon, Jan 21, 2019 at 9:01 PM Ville Syrjala
wrote:
>
> From: Ville Syrjälä
>
> The code managing the dbuf slices is borked and needs some
> real work to fix. In the meantime let's just stop using the
> second slice.
>
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/intel_pm.c | 10 --
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
b/drivers/gpu/drm/i915/intel_pm.c
> index 8b63afa3a221..1e41c899ffe2 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3618,7 +3618,8 @@ static u8
intel_enabled_dbuf_slices_num(struct drm_i915_private *dev_priv)
> enabled_slices = 1;
>
> /* Gen prior to GEN11 have only one DBuf slice */
> - if (INTEL_GEN(dev_priv) < 11)
> + /* FIXME dbuf slice code is broken: see intel_get_ddb_size() */
> + if (1 || INTEL_GEN(dev_priv) < 11)
> return enabled_slices;
IMHO we may not need this, If we return from above we'll never disable
second slice in case it's enabled by bios.
Anyhow code change in intel_get_ddb_size will take care of enabling
only one slice.
>
> if (I915_READ(DBUF_CTL_S2) & DBUF_POWER_STATE)
> @@ -3827,8 +3828,13 @@ static u16 intel_get_ddb_size(struct
drm_i915_private *dev_priv,
>
> /*
> * 12GB/s is maximum BW supported by single DBuf slice.
> +*
> +* FIXME dbuf slice code is broken:
> +* - must wait for planes to stop using the slice before
powering it off
AFAIR we were already doing it and disabling slice only after
update_crtcs, and skl_update_crtc code is taking care of waiting for
vblank in case it's required.
> +* - plane straddling both slices is illegal in
multi-pipe scenarios
This is something new :)
although this change introduce a major limitation with number and size
of planes we can display, yet
As code is broken and mentioned conditions need to be taken care of,
This change should be ok until proper fix.
~Mahesh
> +* - should validate we stay within the hw bandwidth limits
> */
> - if (num_active > 1 || total_data_bw >= GBps(12)) {
> + if (0 && (num_active > 1 || total_data_bw >= GBps(12))) {
> ddb->enabled_slices = 2;
> } else {
> ddb->enabled_slices = 1;
> --
> 2.19.2
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 25/38] drm/i915/pmu: Always sample an active ringbuffer
On 18/01/2019 14:00, Chris Wilson wrote:
As we no longer have a precise indication of requests queued to an
engine, make no presumptions and just sample the ring registers to see
if the engine is busy.
Signed-off-by: Chris Wilson
---
drivers/gpu/drm/i915/i915_pmu.c | 47 +++--
1 file changed, 16 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index b1cb2d3cae16..452589a7e473 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -148,14 +148,6 @@ void i915_pmu_gt_unparked(struct drm_i915_private *i915)
spin_unlock_irq(&i915->pmu.lock);
}
-static bool grab_forcewake(struct drm_i915_private *i915, bool fw)
-{
- if (!fw)
- intel_uncore_forcewake_get(i915, FORCEWAKE_ALL);
-
- return true;
-}
-
static void
add_sample(struct i915_pmu_sample *sample, u32 val)
{
@@ -168,7 +160,6 @@ engines_sample(struct drm_i915_private *dev_priv, unsigned
int period_ns)
struct intel_engine_cs *engine;
enum intel_engine_id id;
intel_wakeref_t wakeref;
- bool fw = false;
if ((dev_priv->pmu.enable & ENGINE_SAMPLE_MASK) == 0)
return;
@@ -180,37 +171,31 @@ engines_sample(struct drm_i915_private *dev_priv,
unsigned int period_ns)
if (!wakeref)
return;
+ intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
for_each_engine(engine, dev_priv, id) {
- u32 current_seqno = intel_engine_get_seqno(engine);
- u32 last_seqno = intel_engine_last_submit(engine);
+ typeof(engine->pmu) *pmu = &engine->pmu;
Or name the struct?
u32 val;
- val = !i915_seqno_passed(current_seqno, last_seqno);
-
- if (val)
- add_sample(&engine->pmu.sample[I915_SAMPLE_BUSY],
- period_ns);
+ val = I915_READ_FW(RING_MI_MODE(engine->mmio_base));
+ if (val & MODE_IDLE)
+ continue;
- if (val && (engine->pmu.enable &
- (BIT(I915_SAMPLE_WAIT) | BIT(I915_SAMPLE_SEMA {
- fw = grab_forcewake(dev_priv, fw);
+ add_sample(&pmu->sample[I915_SAMPLE_BUSY], period_ns);
+ if (pmu->enable &
+ (BIT(I915_SAMPLE_WAIT) | BIT(I915_SAMPLE_SEMA))) {
val = I915_READ_FW(RING_CTL(engine->mmio_base));
- } else {
- val = 0;
- }
- if (val & RING_WAIT)
- add_sample(&engine->pmu.sample[I915_SAMPLE_WAIT],
- period_ns);
+ if (val & RING_WAIT)
+ add_sample(&pmu->sample[I915_SAMPLE_WAIT],
+ period_ns);
- if (val & RING_WAIT_SEMAPHORE)
- add_sample(&engine->pmu.sample[I915_SAMPLE_SEMA],
- period_ns);
+ if (val & RING_WAIT_SEMAPHORE)
+ add_sample(&pmu->sample[I915_SAMPLE_SEMA],
+ period_ns);
+ }
}
-
- if (fw)
- intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
+ intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
intel_runtime_pm_put(dev_priv, wakeref);
}
Reviewed-by: Tvrtko Ursulin
Regards,
Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] dma-buf: Enhance dma-fence tracing
On Tue, Jan 22, 2019 at 10:06 AM Chris Wilson wrote: > > Quoting Koenig, Christian (2019-01-22 08:49:30) > > Am 22.01.19 um 00:20 schrieb Chris Wilson: > > > Rather than every backend and GPU driver reinventing the same wheel for > > > user level debugging of HW execution, the common dma-fence framework > > > should include the tracing infrastructure required for most client API > > > level flow visualisation. > > > > > > With these common dma-fence level tracepoints, the userspace tools can > > > establish a detailed view of the client <-> HW flow across different > > > kernels. There is a strong ask to have this available, so that the > > > userspace developer can effectively assess if they're doing a good job > > > about feeding the beast of a GPU hardware. > > > > > > In the case of needing to look into more fine-grained details of how > > > kernel internals work towards the goal of feeding the beast, the tools > > > may optionally amend the dma-fence tracing information with the driver > > > implementation specific. But for such cases, the tools should have a > > > graceful degradation in case the expected extra tracepoints have > > > changed or their format differs from the expected, as the kernel > > > implementation internals are not expected to stay the same. > > > > > > It is important to distinguish between tracing for the purpose of client > > > flow visualisation and tracing for the purpose of low-level kernel > > > debugging. The latter is highly implementation specific, tied to > > > a particular HW and driver, whereas the former addresses a common goal > > > of user level tracing and likely a common set of userspace tools. > > > Having made the distinction that these tracepoints will be consumed for > > > client API tooling, we raise the spectre of tracepoint ABI stability. It > > > is hoped that by defining a common set of dma-fence tracepoints, we avoid > > > the pitfall of exposing low level details and so restrict ourselves only > > > to the high level flow that is applicable to all drivers and hardware. > > > Thus the reserved guarantee that this set of tracepoints will be stable > > > (with the emphasis on depicting client <-> HW flow as opposed to > > > driver <-> HW). > > > > > > In terms of specific changes to the dma-fence tracing, we remove the > > > emission of the strings for every tracepoint (reserving them for > > > dma_fence_init for cases where they have unique dma_fence_ops, and > > > preferring to have descriptors for the whole fence context). strings do > > > not pack as well into the ftrace ringbuffer and we would prefer to > > > reduce the amount of indirect callbacks required for frequent tracepoint > > > emission. > > > > > > Signed-off-by: Chris Wilson > > > Cc: Joonas Lahtinen > > > Cc: Tvrtko Ursulin > > > Cc: Alex Deucher > > > Cc: "Christian König" > > > Cc: Eric Anholt > > > Cc: Pierre-Loup Griffais > > > Cc: Michael Sartain > > > Cc: Steven Rostedt > > > > In general yes please! If possible please separate out the changes to > > the common dma_fence infrastructure from the i915 changes. > > Sure, I was just stressing the impact: remove some randomly placed > internal debugging tracepoints, try to define useful ones instead :) > > On the list of things to do was to convert at least 2 other drivers > (I was thinking nouveau/msm for simplicity, vc4 for a simpler > introduction to drm_sched than amdgpu) over to be sure we have the right > tracepoints. I think sprinkling these over the scheduler (maybe just as an opt-in, for the case where the driver doesn't have some additional queueing somewhere) would be good. I haven't checked whether it fits, but would give you a bunch of drivers at once. It might also not cover all the cases (I guess the wait related ones would need to be somewhere else). -Daniel > > One thing I'm wondering is why the enable_signaling trace point doesn't > > need to be exported any more. Is that only used internally in the common > > infrastructure? > > Right. Only used inside the core, and I don't see much call for making > it easy for drivers to fiddle around bypassing the core > enable_signaling/signal. (I'm not sure it's useful for client flow > either, it feels more like dma-fence debugging, but they can just > not listen to that tracepoint.) > -Chris > ___ > dri-devel mailing list > dri-de...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 14/34] drm/i915: Pull VM lists under the VM mutex.
On 21/01/2019 22:20, Chris Wilson wrote:
A starting point to counter the pervasive struct_mutex. For the goal of
avoiding (or at least blocking under them!) global locks during user
request submission, a simple but important step is being able to manage
each clients GTT separately. For which, we want to replace using the
struct_mutex as the guard for all things GTT/VM and switch instead to a
specific mutex inside i915_address_space.
Signed-off-by: Chris Wilson
Reviewed-by: Tvrtko Ursulin
Regards,
Tvrtko
---
drivers/gpu/drm/i915/i915_gem.c | 14 --
drivers/gpu/drm/i915/i915_gem_evict.c | 2 ++
drivers/gpu/drm/i915/i915_gem_gtt.c | 15 +--
drivers/gpu/drm/i915/i915_gem_shrinker.c| 4
drivers/gpu/drm/i915/i915_gem_stolen.c | 2 ++
drivers/gpu/drm/i915/i915_vma.c | 11 +++
drivers/gpu/drm/i915/selftests/i915_gem_evict.c | 3 +++
drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 3 +++
8 files changed, 46 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index f45186ddb236..538fa5404603 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -245,18 +245,19 @@ int
i915_gem_get_aperture_ioctl(struct drm_device *dev, void *data,
struct drm_file *file)
{
- struct drm_i915_private *dev_priv = to_i915(dev);
- struct i915_ggtt *ggtt = &dev_priv->ggtt;
+ struct i915_ggtt *ggtt = &to_i915(dev)->ggtt;
struct drm_i915_gem_get_aperture *args = data;
struct i915_vma *vma;
u64 pinned;
+ mutex_lock(&ggtt->vm.mutex);
+
pinned = ggtt->vm.reserved;
- mutex_lock(&dev->struct_mutex);
list_for_each_entry(vma, &ggtt->vm.bound_list, vm_link)
if (i915_vma_is_pinned(vma))
pinned += vma->node.size;
- mutex_unlock(&dev->struct_mutex);
+
+ mutex_unlock(&ggtt->vm.mutex);
args->aper_size = ggtt->vm.total;
args->aper_available_size = args->aper_size - pinned;
@@ -1529,20 +1530,21 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void
*data,
static void i915_gem_object_bump_inactive_ggtt(struct drm_i915_gem_object *obj)
{
- struct drm_i915_private *i915;
+ struct drm_i915_private *i915 = to_i915(obj->base.dev);
struct list_head *list;
struct i915_vma *vma;
GEM_BUG_ON(!i915_gem_object_has_pinned_pages(obj));
+ mutex_lock(&i915->ggtt.vm.mutex);
for_each_ggtt_vma(vma, obj) {
if (!drm_mm_node_allocated(&vma->node))
continue;
list_move_tail(&vma->vm_link, &vma->vm->bound_list);
}
+ mutex_unlock(&i915->ggtt.vm.mutex);
- i915 = to_i915(obj->base.dev);
spin_lock(&i915->mm.obj_lock);
list = obj->bind_count ? &i915->mm.bound_list : &i915->mm.unbound_list;
list_move_tail(&obj->mm.link, list);
diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c
b/drivers/gpu/drm/i915/i915_gem_evict.c
index 5cfe4b75e7d6..dc137701acb8 100644
--- a/drivers/gpu/drm/i915/i915_gem_evict.c
+++ b/drivers/gpu/drm/i915/i915_gem_evict.c
@@ -432,6 +432,7 @@ int i915_gem_evict_vm(struct i915_address_space *vm)
}
INIT_LIST_HEAD(&eviction_list);
+ mutex_lock(&vm->mutex);
list_for_each_entry(vma, &vm->bound_list, vm_link) {
if (i915_vma_is_pinned(vma))
continue;
@@ -439,6 +440,7 @@ int i915_gem_evict_vm(struct i915_address_space *vm)
__i915_vma_pin(vma);
list_add(&vma->evict_link, &eviction_list);
}
+ mutex_unlock(&vm->mutex);
ret = 0;
list_for_each_entry_safe(vma, next, &eviction_list, evict_link) {
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 2ad9070a54c1..49b00996a15e 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1931,7 +1931,10 @@ static struct i915_vma *pd_vma_create(struct
gen6_hw_ppgtt *ppgtt, int size)
vma->ggtt_view.type = I915_GGTT_VIEW_ROTATED; /* prevent fencing */
INIT_LIST_HEAD(&vma->obj_link);
+
+ mutex_lock(&vma->vm->mutex);
list_add(&vma->vm_link, &vma->vm->unbound_list);
+ mutex_unlock(&vma->vm->mutex);
return vma;
}
@@ -3504,9 +3507,10 @@ void i915_gem_restore_gtt_mappings(struct
drm_i915_private *dev_priv)
i915_check_and_clear_faults(dev_priv);
+ mutex_lock(&ggtt->vm.mutex);
+
/* First fill our portion of the GTT with scratch pages */
ggtt->vm.clear_range(&ggtt->vm, 0, ggtt->vm.total);
-
ggtt->vm.closed = true; /* skip rewriting PTE on VMA unbind */
/* clflush objects bound into the GGTT and rebind them. */
@@ -3516,19 +3520,26 @@ void i915_gem_restore_gtt_mappings(struct
drm_i915_private *dev_priv)
if (!(vma->flag
Re: [Intel-gfx] [PATCH] dma-buf: Enhance dma-fence tracing
Quoting Koenig, Christian (2019-01-22 08:49:30) > Am 22.01.19 um 00:20 schrieb Chris Wilson: > > Rather than every backend and GPU driver reinventing the same wheel for > > user level debugging of HW execution, the common dma-fence framework > > should include the tracing infrastructure required for most client API > > level flow visualisation. > > > > With these common dma-fence level tracepoints, the userspace tools can > > establish a detailed view of the client <-> HW flow across different > > kernels. There is a strong ask to have this available, so that the > > userspace developer can effectively assess if they're doing a good job > > about feeding the beast of a GPU hardware. > > > > In the case of needing to look into more fine-grained details of how > > kernel internals work towards the goal of feeding the beast, the tools > > may optionally amend the dma-fence tracing information with the driver > > implementation specific. But for such cases, the tools should have a > > graceful degradation in case the expected extra tracepoints have > > changed or their format differs from the expected, as the kernel > > implementation internals are not expected to stay the same. > > > > It is important to distinguish between tracing for the purpose of client > > flow visualisation and tracing for the purpose of low-level kernel > > debugging. The latter is highly implementation specific, tied to > > a particular HW and driver, whereas the former addresses a common goal > > of user level tracing and likely a common set of userspace tools. > > Having made the distinction that these tracepoints will be consumed for > > client API tooling, we raise the spectre of tracepoint ABI stability. It > > is hoped that by defining a common set of dma-fence tracepoints, we avoid > > the pitfall of exposing low level details and so restrict ourselves only > > to the high level flow that is applicable to all drivers and hardware. > > Thus the reserved guarantee that this set of tracepoints will be stable > > (with the emphasis on depicting client <-> HW flow as opposed to > > driver <-> HW). > > > > In terms of specific changes to the dma-fence tracing, we remove the > > emission of the strings for every tracepoint (reserving them for > > dma_fence_init for cases where they have unique dma_fence_ops, and > > preferring to have descriptors for the whole fence context). strings do > > not pack as well into the ftrace ringbuffer and we would prefer to > > reduce the amount of indirect callbacks required for frequent tracepoint > > emission. > > > > Signed-off-by: Chris Wilson > > Cc: Joonas Lahtinen > > Cc: Tvrtko Ursulin > > Cc: Alex Deucher > > Cc: "Christian König" > > Cc: Eric Anholt > > Cc: Pierre-Loup Griffais > > Cc: Michael Sartain > > Cc: Steven Rostedt > > In general yes please! If possible please separate out the changes to > the common dma_fence infrastructure from the i915 changes. Sure, I was just stressing the impact: remove some randomly placed internal debugging tracepoints, try to define useful ones instead :) On the list of things to do was to convert at least 2 other drivers (I was thinking nouveau/msm for simplicity, vc4 for a simpler introduction to drm_sched than amdgpu) over to be sure we have the right tracepoints. > One thing I'm wondering is why the enable_signaling trace point doesn't > need to be exported any more. Is that only used internally in the common > infrastructure? Right. Only used inside the core, and I don't see much call for making it easy for drivers to fiddle around bypassing the core enable_signaling/signal. (I'm not sure it's useful for client flow either, it feels more like dma-fence debugging, but they can just not listen to that tracepoint.) -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [v2,1/7] drm/i915/crt: split out intel_crt_present() to platform specific setup
== Series Details ==
Series: series starting with [v2,1/7] drm/i915/crt: split out
intel_crt_present() to platform specific setup
URL : https://patchwork.freedesktop.org/series/55540/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_5459 -> Patchwork_12003
Summary
---
**SUCCESS**
No regressions found.
External URL:
https://patchwork.freedesktop.org/api/1.0/series/55540/revisions/1/mbox/
Known issues
Here are the changes found in Patchwork_12003 that come from known issues:
### IGT changes ###
Issues hit
* igt@kms_busy@basic-flip-b:
- fi-gdg-551: PASS -> FAIL [fdo#103182]
* igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
- fi-apl-guc: PASS -> DMESG-WARN [fdo#108529] / [fdo#108566]
Possible fixes
* igt@kms_frontbuffer_tracking@basic:
- {fi-icl-u2}:FAIL [fdo#103167] -> PASS
* igt@pm_rpm@module-reload:
- {fi-icl-u2}:DMESG-WARN [fdo#108654] -> PASS
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
[fdo#103182]: https://bugs.freedesktop.org/show_bug.cgi?id=103182
[fdo#108529]: https://bugs.freedesktop.org/show_bug.cgi?id=108529
[fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
[fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
[fdo#108654]: https://bugs.freedesktop.org/show_bug.cgi?id=108654
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
Participating hosts (47 -> 41)
--
Additional (1): fi-glk-j4005
Missing(7): fi-kbl-soraka fi-ilk-m540 fi-hsw-peppy fi-byt-squawks
fi-bsw-cyan fi-pnv-d510 fi-bdw-samus
Build changes
-
* Linux: CI_DRM_5459 -> Patchwork_12003
CI_DRM_5459: 0f693a275dd91391b476ada7481cf08f4fe610aa @
git://anongit.freedesktop.org/gfx-ci/linux
IGT_4780: 1c1612bdc36b44a704095e7b0ba5542818ce793f @
git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_12003: f6c1b42bdd25a3c26ffd12155b322eb57131deba @
git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
f6c1b42bdd25 drm/i915/crt: simplify CRT VBT check on pre-VLV/DDI
8f9fa9fb8dff drm/i915/lvds: simplify gen 2 lvds presence
a74fbf6829d4 drm/i915: rename has_edp_a() to ilk_has_edp_a()
2574de0daa85 drm/i915/tv: only call intel_tv_init() on platforms that might
have TV
cdba4c424c11 drm/i915/lvds: nuke intel_lvds_supported()
66e5eb4fa89d drm/i915/lvds: only call intel_lvds_init() on platforms that might
have LVDS
3ae75b736cf4 drm/i915/crt: split out intel_crt_present() to platform specific
setup
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12003/
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] dma-buf: Enhance dma-fence tracing
Am 22.01.19 um 00:20 schrieb Chris Wilson: > Rather than every backend and GPU driver reinventing the same wheel for > user level debugging of HW execution, the common dma-fence framework > should include the tracing infrastructure required for most client API > level flow visualisation. > > With these common dma-fence level tracepoints, the userspace tools can > establish a detailed view of the client <-> HW flow across different > kernels. There is a strong ask to have this available, so that the > userspace developer can effectively assess if they're doing a good job > about feeding the beast of a GPU hardware. > > In the case of needing to look into more fine-grained details of how > kernel internals work towards the goal of feeding the beast, the tools > may optionally amend the dma-fence tracing information with the driver > implementation specific. But for such cases, the tools should have a > graceful degradation in case the expected extra tracepoints have > changed or their format differs from the expected, as the kernel > implementation internals are not expected to stay the same. > > It is important to distinguish between tracing for the purpose of client > flow visualisation and tracing for the purpose of low-level kernel > debugging. The latter is highly implementation specific, tied to > a particular HW and driver, whereas the former addresses a common goal > of user level tracing and likely a common set of userspace tools. > Having made the distinction that these tracepoints will be consumed for > client API tooling, we raise the spectre of tracepoint ABI stability. It > is hoped that by defining a common set of dma-fence tracepoints, we avoid > the pitfall of exposing low level details and so restrict ourselves only > to the high level flow that is applicable to all drivers and hardware. > Thus the reserved guarantee that this set of tracepoints will be stable > (with the emphasis on depicting client <-> HW flow as opposed to > driver <-> HW). > > In terms of specific changes to the dma-fence tracing, we remove the > emission of the strings for every tracepoint (reserving them for > dma_fence_init for cases where they have unique dma_fence_ops, and > preferring to have descriptors for the whole fence context). strings do > not pack as well into the ftrace ringbuffer and we would prefer to > reduce the amount of indirect callbacks required for frequent tracepoint > emission. > > Signed-off-by: Chris Wilson > Cc: Joonas Lahtinen > Cc: Tvrtko Ursulin > Cc: Alex Deucher > Cc: "Christian König" > Cc: Eric Anholt > Cc: Pierre-Loup Griffais > Cc: Michael Sartain > Cc: Steven Rostedt In general yes please! If possible please separate out the changes to the common dma_fence infrastructure from the i915 changes. One thing I'm wondering is why the enable_signaling trace point doesn't need to be exported any more. Is that only used internally in the common infrastructure? Apart from that I'm on sick leave today, so give me at least a few days to recover and take a closer look. Thanks, Christian. > --- > drivers/dma-buf/dma-fence.c | 9 +- > drivers/gpu/drm/i915/i915_gem_clflush.c | 5 + > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 1 - > drivers/gpu/drm/i915/i915_request.c | 16 +- > drivers/gpu/drm/i915/i915_timeline.c| 5 + > drivers/gpu/drm/i915/i915_trace.h | 134 --- > drivers/gpu/drm/i915/intel_guc_submission.c | 10 ++ > drivers/gpu/drm/i915/intel_lrc.c| 6 + > drivers/gpu/drm/i915/intel_ringbuffer.h | 2 + > include/trace/events/dma_fence.h| 177 +++- > 10 files changed, 214 insertions(+), 151 deletions(-) > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > index 3aa8733f832a..5c93ed34b1ff 100644 > --- a/drivers/dma-buf/dma-fence.c > +++ b/drivers/dma-buf/dma-fence.c > @@ -27,8 +27,15 @@ > #define CREATE_TRACE_POINTS > #include > > +EXPORT_TRACEPOINT_SYMBOL(dma_fence_context_create); > +EXPORT_TRACEPOINT_SYMBOL(dma_fence_context_destroy); > + > +EXPORT_TRACEPOINT_SYMBOL(dma_fence_await); > EXPORT_TRACEPOINT_SYMBOL(dma_fence_emit); > -EXPORT_TRACEPOINT_SYMBOL(dma_fence_enable_signal); > +EXPORT_TRACEPOINT_SYMBOL(dma_fence_execute_start); > +EXPORT_TRACEPOINT_SYMBOL(dma_fence_execute_end); > +EXPORT_TRACEPOINT_SYMBOL(dma_fence_wait_start); > +EXPORT_TRACEPOINT_SYMBOL(dma_fence_wait_end); > > static DEFINE_SPINLOCK(dma_fence_stub_lock); > static struct dma_fence dma_fence_stub; > diff --git a/drivers/gpu/drm/i915/i915_gem_clflush.c > b/drivers/gpu/drm/i915/i915_gem_clflush.c > index 8e74c23cbd91..435c1303ecc8 100644 > --- a/drivers/gpu/drm/i915/i915_gem_clflush.c > +++ b/drivers/gpu/drm/i915/i915_gem_clflush.c > @@ -22,6 +22,8 @@ >* >*/ > > +#include > + > #include "i915_drv.h" > #include "intel_frontbuffer.h" > #include "i915_gem_clflush.h" > @@ -73,6 +75,7 @@ static void i915_clflush_work(
Re: [Intel-gfx] [PATCH] drm/dp: use DRM_DEBUG_DP() instead of drm_dbg for logging
On Mon, 21 Jan 2019, Ville Syrjälä wrote:
> On Mon, Jan 21, 2019 at 01:27:58PM +0200, Jani Nikula wrote:
>> We have a wrapper for a reason.
>>
>> Signed-off-by: Jani Nikula
>
> Reviewed-by: Ville Syrjälä
Thanks, pushed to drm-misc-next.
BR,
Jani.
>
>> ---
>> drivers/gpu/drm/drm_dp_helper.c | 8
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_dp_helper.c
>> b/drivers/gpu/drm/drm_dp_helper.c
>> index 26835d174939..4def0bface85 100644
>> --- a/drivers/gpu/drm/drm_dp_helper.c
>> +++ b/drivers/gpu/drm/drm_dp_helper.c
>> @@ -194,11 +194,11 @@ drm_dp_dump_access(const struct drm_dp_aux *aux,
>> const char *arrow = request == DP_AUX_NATIVE_READ ? "->" : "<-";
>>
>> if (ret > 0)
>> -drm_dbg(DRM_UT_DP, "%s: 0x%05x AUX %s (ret=%3d) %*ph\n",
>> -aux->name, offset, arrow, ret, min(ret, 20), buffer);
>> +DRM_DEBUG_DP("%s: 0x%05x AUX %s (ret=%3d) %*ph\n",
>> + aux->name, offset, arrow, ret, min(ret, 20),
>> buffer);
>> else
>> -drm_dbg(DRM_UT_DP, "%s: 0x%05x AUX %s (ret=%3d)\n",
>> -aux->name, offset, arrow, ret);
>> +DRM_DEBUG_DP("%s: 0x%05x AUX %s (ret=%3d)\n",
>> + aux->name, offset, arrow, ret);
>> }
>>
>> /**
>> --
>> 2.20.1
>>
>> ___
>> dri-devel mailing list
>> dri-de...@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Jani Nikula, Intel Open Source Graphics Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/5] drm/i915/crt: split out intel_crt_present() to platform specific setup
On Mon, 21 Jan 2019, Ville Syrjälä wrote:
> On Mon, Jan 21, 2019 at 04:21:30PM +0200, Jani Nikula wrote:
>> With new platforms not having CRT support and most conditions in
>> intel_crt_present() being specific to DDI, split out the CRT
>> initialization to platform specific blocks in the if ladder. Add new
>> Pineview block for this.
>>
>> This puts intel_crt_init() more in line with the rest of the outputs,
>> and makes it slightly easier for the uninitiated to figure out which
>> platforms actually have what.
>>
>> Signed-off-by: Jani Nikula
>> ---
>> drivers/gpu/drm/i915/intel_display.c | 37 ++--
>> 1 file changed, 24 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 2fa9f4aec08e..e8bc297c60ab 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -14245,23 +14245,17 @@ static bool has_edp_a(struct drm_i915_private
>> *dev_priv)
>> return true;
>> }
>>
>> -static bool intel_crt_present(struct drm_i915_private *dev_priv)
>> +static bool intel_ddi_crt_present(struct drm_i915_private *dev_priv)
>> {
>> -if (INTEL_GEN(dev_priv) >= 9)
>> -return false;
>
> We should probably keep this in case the vbt is bonkers.
Oh, definitely, thanks for spotting!
>
>> -
>> if (IS_HSW_ULT(dev_priv) || IS_BDW_ULT(dev_priv))
>> return false;
>>
>> -if (IS_CHERRYVIEW(dev_priv))
>> -return false;
>> -
>> if (HAS_PCH_LPT_H(dev_priv) &&
>> I915_READ(SFUSE_STRAP) & SFUSE_STRAP_CRT_DISABLED)
>> return false;
>>
>> /* DDI E can't be used if DDI A requires 4 lanes */
>> -if (HAS_DDI(dev_priv) && I915_READ(DDI_BUF_CTL(PORT_A)) & DDI_A_4_LANES)
>> +if (I915_READ(DDI_BUF_CTL(PORT_A)) & DDI_A_4_LANES)
>> return false;
>>
>> if (!dev_priv->vbt.int_crt_support)
>> @@ -14323,9 +14317,6 @@ static void intel_setup_outputs(struct
>> drm_i915_private *dev_priv)
>> */
>> intel_lvds_init(dev_priv);
>>
>> -if (intel_crt_present(dev_priv))
>> -intel_crt_init(dev_priv);
>> -
>> if (IS_ICELAKE(dev_priv)) {
>> intel_ddi_init(dev_priv, PORT_A);
>> intel_ddi_init(dev_priv, PORT_B);
>> @@ -14354,6 +14345,9 @@ static void intel_setup_outputs(struct
>> drm_i915_private *dev_priv)
>> } else if (HAS_DDI(dev_priv)) {
>> int found;
>>
>> +if (intel_ddi_crt_present(dev_priv))
>> +intel_crt_init(dev_priv);
>> +
>> /*
>> * Haswell uses DDI functions to detect digital outputs.
>> * On SKL pre-D0 the strap isn't connected, so we assume
>> @@ -14385,6 +14379,10 @@ static void intel_setup_outputs(struct
>> drm_i915_private *dev_priv)
>>
>> } else if (HAS_PCH_SPLIT(dev_priv)) {
>> int found;
>> +
>> +if (dev_priv->vbt.int_crt_support)
>> +intel_crt_init(dev_priv);
>> +
>> dpd_is_edp = intel_dp_is_port_edp(dev_priv, PORT_D);
>>
>> if (has_edp_a(dev_priv))
>> @@ -14413,6 +14411,9 @@ static void intel_setup_outputs(struct
>> drm_i915_private *dev_priv)
>> } else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
>> bool has_edp, has_port;
>>
>> +if (IS_VALLEYVIEW(dev_priv) && dev_priv->vbt.int_crt_support)
>> +intel_crt_init(dev_priv);
>> +
>> /*
>> * The DP_DETECTED bit is the latched state of the DDC
>> * SDA pin at boot. However since eDP doesn't require DDC
>> @@ -14455,9 +14456,15 @@ static void intel_setup_outputs(struct
>> drm_i915_private *dev_priv)
>> }
>>
>> vlv_dsi_init(dev_priv);
>> -} else if (!IS_GEN(dev_priv, 2) && !IS_PINEVIEW(dev_priv)) {
>> +} else if (IS_PINEVIEW(dev_priv)) {
>> +if (dev_priv->vbt.int_crt_support)
>> +intel_crt_init(dev_priv);
>> +} else if (IS_GEN_RANGE(dev_priv, 3, 4)) {
>> bool found = false;
>>
>> +if (dev_priv->vbt.int_crt_support)
>> +intel_crt_init(dev_priv);
>> +
>> if (I915_READ(GEN3_SDVOB) & SDVO_DETECTED) {
>> DRM_DEBUG_KMS("probing SDVOB\n");
>> found = intel_sdvo_init(dev_priv, GEN3_SDVOB, PORT_B);
>> @@ -14489,8 +14496,12 @@ static void intel_setup_outputs(struct
>> drm_i915_private *dev_priv)
>>
>> if (IS_G4X(dev_priv) && (I915_READ(DP_D) & DP_DETECTED))
>> intel_dp_init(dev_priv, DP_D, PORT_D);
>> -} else if (IS_GEN(dev_priv, 2))
>> +} else if (IS_GEN(dev_priv, 2)) {
>> +if (dev_priv->vbt.int_crt_support)
>> +intel_crt_init(dev_priv);
>
> int_crt_support is always true for pre-vlv/pre-hsw so we could
> skip the check in most of these cases. Either way
Re: [Intel-gfx] [PATCH 4/5] drm/i915/tv: only call intel_tv_init() on platforms that might have TV
On Mon, 21 Jan 2019, Ville Syrjälä wrote:
> On Mon, Jan 21, 2019 at 04:21:33PM +0200, Jani Nikula wrote:
>> With most platforms not having TV support, only call intel_tv_init() on
>> platforms that might actually have TV, specifically gens 3 and 4.
>>
>> This puts intel_tv_init() more in line with the rest of the outputs, and
>> makes it slightly easier for the uninitiated to figure out which
>> platforms actually have what.
>>
>> Signed-off-by: Jani Nikula
>> ---
>> drivers/gpu/drm/i915/intel_display.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 4207ee0b83ce..6960004fdc94 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -14501,6 +14501,9 @@ static void intel_setup_outputs(struct
>> drm_i915_private *dev_priv)
>>
>> if (IS_G4X(dev_priv) && (I915_READ(DP_D) & DP_DETECTED))
>> intel_dp_init(dev_priv, DP_D, PORT_D);
>> +
>> +if (SUPPORTS_TV(dev_priv))
>> +intel_tv_init(dev_priv);
>
> Since PNV was split into its own thing I think this could actually
> be replaced with IS_MOBILE().
If I'd been able to get rid of SUPPORTS_TV() with that altogether, I
probably would've made the change. But there's still a user in CRC code,
so I left this as-is.
BR,
Jani.
>
> Either way
> Reviewed-by: Ville Syrjälä
>
>> } else if (IS_GEN(dev_priv, 2)) {
>> if (IS_MOBILE(dev_priv) && !IS_I830(dev_priv))
>> intel_lvds_init(dev_priv);
>> @@ -14511,9 +14514,6 @@ static void intel_setup_outputs(struct
>> drm_i915_private *dev_priv)
>> intel_dvo_init(dev_priv);
>> }
>>
>> -if (SUPPORTS_TV(dev_priv))
>> -intel_tv_init(dev_priv);
>> -
>> intel_psr_init(dev_priv);
>>
>> for_each_intel_encoder(&dev_priv->drm, encoder) {
>> --
>> 2.20.1
>>
>> ___
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Graphics Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2 3/7] drm/i915/lvds: nuke intel_lvds_supported()
Now that intel_lvds_init() is only called for platforms that might have
LVDS, move the remaining checks to intel_setup_outputs(), again similar
to other outputs, and remove the overlapping checks.
Reviewed-by: Ville Syrjälä
Signed-off-by: Jani Nikula
---
drivers/gpu/drm/i915/intel_display.c | 6 --
drivers/gpu/drm/i915/intel_lvds.c| 23 ---
2 files changed, 4 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 29a7dd4afe0e..db0f15242ccf 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14467,7 +14467,8 @@ static void intel_setup_outputs(struct drm_i915_private
*dev_priv)
} else if (IS_GEN_RANGE(dev_priv, 3, 4)) {
bool found = false;
- intel_lvds_init(dev_priv);
+ if (IS_MOBILE(dev_priv))
+ intel_lvds_init(dev_priv);
if (dev_priv->vbt.int_crt_support)
intel_crt_init(dev_priv);
@@ -14504,7 +14505,8 @@ static void intel_setup_outputs(struct drm_i915_private
*dev_priv)
if (IS_G4X(dev_priv) && (I915_READ(DP_D) & DP_DETECTED))
intel_dp_init(dev_priv, DP_D, PORT_D);
} else if (IS_GEN(dev_priv, 2)) {
- intel_lvds_init(dev_priv);
+ if (IS_MOBILE(dev_priv) && !IS_I830(dev_priv))
+ intel_lvds_init(dev_priv);
if (dev_priv->vbt.int_crt_support)
intel_crt_init(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_lvds.c
b/drivers/gpu/drm/i915/intel_lvds.c
index 46a5dfd5cdf7..815ed463d9c5 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -798,26 +798,6 @@ static bool compute_is_dual_link_lvds(struct
intel_lvds_encoder *lvds_encoder)
return (val & LVDS_CLKB_POWER_MASK) == LVDS_CLKB_POWER_UP;
}
-static bool intel_lvds_supported(struct drm_i915_private *dev_priv)
-{
- /*
-* With the introduction of the PCH we gained a dedicated
-* LVDS presence pin, use it.
-*/
- if (HAS_PCH_IBX(dev_priv) || HAS_PCH_CPT(dev_priv))
- return true;
-
- /*
-* Otherwise LVDS was only attached to mobile products,
-* except for the inglorious 830gm
-*/
- if (INTEL_GEN(dev_priv) <= 4 &&
- IS_MOBILE(dev_priv) && !IS_I830(dev_priv))
- return true;
-
- return false;
-}
-
/**
* intel_lvds_init - setup LVDS connectors on this device
* @dev_priv: i915 device
@@ -842,9 +822,6 @@ void intel_lvds_init(struct drm_i915_private *dev_priv)
u8 pin;
u32 allowed_scalers;
- if (!intel_lvds_supported(dev_priv))
- return;
-
/* Skip init on machines we know falsely report LVDS */
if (dmi_check_system(intel_no_lvds)) {
WARN(!dev_priv->vbt.int_lvds_support,
--
2.20.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2 6/7] drm/i915/lvds: simplify gen 2 lvds presence
Gen 2 mobile and not I830 is, in fact, I85X. Simplify.
Suggested-by: Ville Syrjälä
Signed-off-by: Jani Nikula
---
drivers/gpu/drm/i915/intel_display.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 9895ea566f99..ed3780f24638 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14508,7 +14508,7 @@ static void intel_setup_outputs(struct drm_i915_private
*dev_priv)
if (SUPPORTS_TV(dev_priv))
intel_tv_init(dev_priv);
} else if (IS_GEN(dev_priv, 2)) {
- if (IS_MOBILE(dev_priv) && !IS_I830(dev_priv))
+ if (IS_I85X(dev_priv))
intel_lvds_init(dev_priv);
if (dev_priv->vbt.int_crt_support)
--
2.20.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2 5/7] drm/i915: rename has_edp_a() to ilk_has_edp_a()
Clarify that the name is specific to ILK+ PCH platforms.
v2: prefix the name with ilk rather than pch (Ville)
Signed-off-by: Jani Nikula
---
drivers/gpu/drm/i915/intel_display.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 8e89f04ddd9c..9895ea566f99 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14231,7 +14231,7 @@ static int intel_encoder_clones(struct intel_encoder
*encoder)
return index_mask;
}
-static bool has_edp_a(struct drm_i915_private *dev_priv)
+static bool ilk_has_edp_a(struct drm_i915_private *dev_priv)
{
if (!IS_MOBILE(dev_priv))
return false;
@@ -14388,7 +14388,7 @@ static void intel_setup_outputs(struct drm_i915_private
*dev_priv)
dpd_is_edp = intel_dp_is_port_edp(dev_priv, PORT_D);
- if (has_edp_a(dev_priv))
+ if (ilk_has_edp_a(dev_priv))
intel_dp_init(dev_priv, DP_A, PORT_A);
if (I915_READ(PCH_HDMIB) & SDVO_DETECTED) {
--
2.20.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2 2/7] drm/i915/lvds: only call intel_lvds_init() on platforms that might have LVDS
With new platforms not having LVDS support, only call intel_lvds_init()
on platforms that might actually have LVDS. Move the comment about eDP
init to the PCH block where it's relevant.
This puts intel_lvds_init() more in line with the rest of the outputs,
and makes it slightly easier for the uninitiated to figure out which
platforms actually have what.
Reviewed-by: Ville Syrjälä
Signed-off-by: Jani Nikula
---
drivers/gpu/drm/i915/intel_display.c | 20 +---
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 9e0f34524d0b..29a7dd4afe0e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14313,13 +14313,6 @@ static void intel_setup_outputs(struct
drm_i915_private *dev_priv)
if (!HAS_DISPLAY(dev_priv))
return;
- /*
-* intel_edp_init_connector() depends on this completing first, to
-* prevent the registeration of both eDP and LVDS and the incorrect
-* sharing of the PPS.
-*/
- intel_lvds_init(dev_priv);
-
if (IS_ICELAKE(dev_priv)) {
intel_ddi_init(dev_priv, PORT_A);
intel_ddi_init(dev_priv, PORT_B);
@@ -14383,6 +14376,13 @@ static void intel_setup_outputs(struct
drm_i915_private *dev_priv)
} else if (HAS_PCH_SPLIT(dev_priv)) {
int found;
+ /*
+* intel_edp_init_connector() depends on this completing first,
+* to prevent the registration of both eDP and LVDS and the
+* incorrect sharing of the PPS.
+*/
+ intel_lvds_init(dev_priv);
+
if (dev_priv->vbt.int_crt_support)
intel_crt_init(dev_priv);
@@ -14460,11 +14460,15 @@ static void intel_setup_outputs(struct
drm_i915_private *dev_priv)
vlv_dsi_init(dev_priv);
} else if (IS_PINEVIEW(dev_priv)) {
+ intel_lvds_init(dev_priv);
+
if (dev_priv->vbt.int_crt_support)
intel_crt_init(dev_priv);
} else if (IS_GEN_RANGE(dev_priv, 3, 4)) {
bool found = false;
+ intel_lvds_init(dev_priv);
+
if (dev_priv->vbt.int_crt_support)
intel_crt_init(dev_priv);
@@ -14500,6 +14504,8 @@ static void intel_setup_outputs(struct drm_i915_private
*dev_priv)
if (IS_G4X(dev_priv) && (I915_READ(DP_D) & DP_DETECTED))
intel_dp_init(dev_priv, DP_D, PORT_D);
} else if (IS_GEN(dev_priv, 2)) {
+ intel_lvds_init(dev_priv);
+
if (dev_priv->vbt.int_crt_support)
intel_crt_init(dev_priv);
--
2.20.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2 4/7] drm/i915/tv: only call intel_tv_init() on platforms that might have TV
With most platforms not having TV support, only call intel_tv_init() on
platforms that might actually have TV, specifically gens 3 and 4.
This puts intel_tv_init() more in line with the rest of the outputs, and
makes it slightly easier for the uninitiated to figure out which
platforms actually have what.
Reviewed-by: Ville Syrjälä
Signed-off-by: Jani Nikula
---
drivers/gpu/drm/i915/intel_display.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index db0f15242ccf..8e89f04ddd9c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14504,6 +14504,9 @@ static void intel_setup_outputs(struct drm_i915_private
*dev_priv)
if (IS_G4X(dev_priv) && (I915_READ(DP_D) & DP_DETECTED))
intel_dp_init(dev_priv, DP_D, PORT_D);
+
+ if (SUPPORTS_TV(dev_priv))
+ intel_tv_init(dev_priv);
} else if (IS_GEN(dev_priv, 2)) {
if (IS_MOBILE(dev_priv) && !IS_I830(dev_priv))
intel_lvds_init(dev_priv);
@@ -14514,9 +14517,6 @@ static void intel_setup_outputs(struct drm_i915_private
*dev_priv)
intel_dvo_init(dev_priv);
}
- if (SUPPORTS_TV(dev_priv))
- intel_tv_init(dev_priv);
-
intel_psr_init(dev_priv);
for_each_intel_encoder(&dev_priv->drm, encoder) {
--
2.20.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/5] drm/i915/lvds: nuke intel_lvds_supported()
On Mon, 21 Jan 2019, Ville Syrjälä wrote:
> On Mon, Jan 21, 2019 at 04:21:32PM +0200, Jani Nikula wrote:
>> Now that intel_lvds_init() is only called for platforms that might have
>> LVDS, move the remaining checks to intel_setup_outputs(), again similar
>> to other outputs, and remove the overlapping checks.
>>
>> Signed-off-by: Jani Nikula
>> ---
>> drivers/gpu/drm/i915/intel_display.c | 6 --
>> drivers/gpu/drm/i915/intel_lvds.c| 23 ---
>> 2 files changed, 4 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 4b5704a87934..4207ee0b83ce 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -14464,7 +14464,8 @@ static void intel_setup_outputs(struct
>> drm_i915_private *dev_priv)
>
> Had to read the earlier patch twice to make sure we're not leaving
> ibx/cpt/ppt or pnv behind.
>
>> } else if (IS_GEN_RANGE(dev_priv, 3, 4)) {
>> bool found = false;
>>
>> -intel_lvds_init(dev_priv);
>> +if (IS_MOBILE(dev_priv))
>> +intel_lvds_init(dev_priv);
>>
>> if (dev_priv->vbt.int_crt_support)
>> intel_crt_init(dev_priv);
>> @@ -14501,7 +14502,8 @@ static void intel_setup_outputs(struct
>> drm_i915_private *dev_priv)
>> if (IS_G4X(dev_priv) && (I915_READ(DP_D) & DP_DETECTED))
>> intel_dp_init(dev_priv, DP_D, PORT_D);
>> } else if (IS_GEN(dev_priv, 2)) {
>> -intel_lvds_init(dev_priv);
>> +if (IS_MOBILE(dev_priv) && !IS_I830(dev_priv))
>
> aka. IS_I85X()
Made the change in a separate patch.
> Reviewed-by: Ville Syrjälä
Thanks,
Jani.
>
>> +intel_lvds_init(dev_priv);
>>
>> if (dev_priv->vbt.int_crt_support)
>> intel_crt_init(dev_priv);
>> diff --git a/drivers/gpu/drm/i915/intel_lvds.c
>> b/drivers/gpu/drm/i915/intel_lvds.c
>> index 46a5dfd5cdf7..815ed463d9c5 100644
>> --- a/drivers/gpu/drm/i915/intel_lvds.c
>> +++ b/drivers/gpu/drm/i915/intel_lvds.c
>> @@ -798,26 +798,6 @@ static bool compute_is_dual_link_lvds(struct
>> intel_lvds_encoder *lvds_encoder)
>> return (val & LVDS_CLKB_POWER_MASK) == LVDS_CLKB_POWER_UP;
>> }
>>
>> -static bool intel_lvds_supported(struct drm_i915_private *dev_priv)
>> -{
>> -/*
>> - * With the introduction of the PCH we gained a dedicated
>> - * LVDS presence pin, use it.
>> - */
>> -if (HAS_PCH_IBX(dev_priv) || HAS_PCH_CPT(dev_priv))
>> -return true;
>> -
>> -/*
>> - * Otherwise LVDS was only attached to mobile products,
>> - * except for the inglorious 830gm
>> - */
>> -if (INTEL_GEN(dev_priv) <= 4 &&
>> -IS_MOBILE(dev_priv) && !IS_I830(dev_priv))
>> -return true;
>> -
>> -return false;
>> -}
>> -
>> /**
>> * intel_lvds_init - setup LVDS connectors on this device
>> * @dev_priv: i915 device
>> @@ -842,9 +822,6 @@ void intel_lvds_init(struct drm_i915_private *dev_priv)
>> u8 pin;
>> u32 allowed_scalers;
>>
>> -if (!intel_lvds_supported(dev_priv))
>> -return;
>> -
>> /* Skip init on machines we know falsely report LVDS */
>> if (dmi_check_system(intel_no_lvds)) {
>> WARN(!dev_priv->vbt.int_lvds_support,
>> --
>> 2.20.1
>>
>> ___
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Graphics Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2 7/7] drm/i915/crt: simplify CRT VBT check on pre-VLV/DDI
The VBT int_crt_support can't be trusted on earlier platforms, and is
always set to true in intel_bios.c for pre-DDI and pre-VLV platforms. We
can simplify the output setup by unconditionally calling
intel_crt_init() for these platforms.
Suggested-by: Ville Syrjälä
Signed-off-by: Jani Nikula
---
drivers/gpu/drm/i915/intel_display.c | 15 ---
1 file changed, 4 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index ed3780f24638..d328599240cb 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14382,9 +14382,7 @@ static void intel_setup_outputs(struct drm_i915_private
*dev_priv)
* incorrect sharing of the PPS.
*/
intel_lvds_init(dev_priv);
-
- if (dev_priv->vbt.int_crt_support)
- intel_crt_init(dev_priv);
+ intel_crt_init(dev_priv);
dpd_is_edp = intel_dp_is_port_edp(dev_priv, PORT_D);
@@ -14461,17 +14459,14 @@ static void intel_setup_outputs(struct
drm_i915_private *dev_priv)
vlv_dsi_init(dev_priv);
} else if (IS_PINEVIEW(dev_priv)) {
intel_lvds_init(dev_priv);
-
- if (dev_priv->vbt.int_crt_support)
- intel_crt_init(dev_priv);
+ intel_crt_init(dev_priv);
} else if (IS_GEN_RANGE(dev_priv, 3, 4)) {
bool found = false;
if (IS_MOBILE(dev_priv))
intel_lvds_init(dev_priv);
- if (dev_priv->vbt.int_crt_support)
- intel_crt_init(dev_priv);
+ intel_crt_init(dev_priv);
if (I915_READ(GEN3_SDVOB) & SDVO_DETECTED) {
DRM_DEBUG_KMS("probing SDVOB\n");
@@ -14511,9 +14506,7 @@ static void intel_setup_outputs(struct drm_i915_private
*dev_priv)
if (IS_I85X(dev_priv))
intel_lvds_init(dev_priv);
- if (dev_priv->vbt.int_crt_support)
- intel_crt_init(dev_priv);
-
+ intel_crt_init(dev_priv);
intel_dvo_init(dev_priv);
}
--
2.20.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2 1/7] drm/i915/crt: split out intel_crt_present() to platform specific setup
With new platforms not having CRT support and most conditions in
intel_crt_present() being specific to DDI, split out the CRT
initialization to platform specific blocks in the if ladder. Add new
Pineview block for this.
This puts intel_crt_init() more in line with the rest of the outputs,
and makes it slightly easier for the uninitiated to figure out which
platforms actually have what.
v2: keep gen >= 9 check in intel_ddi_crt_present() (Ville)
Reviewed-by: Ville Syrjälä
Signed-off-by: Jani Nikula
---
drivers/gpu/drm/i915/intel_display.c | 34
1 file changed, 24 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 2fa9f4aec08e..9e0f34524d0b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14245,7 +14245,7 @@ static bool has_edp_a(struct drm_i915_private *dev_priv)
return true;
}
-static bool intel_crt_present(struct drm_i915_private *dev_priv)
+static bool intel_ddi_crt_present(struct drm_i915_private *dev_priv)
{
if (INTEL_GEN(dev_priv) >= 9)
return false;
@@ -14253,15 +14253,12 @@ static bool intel_crt_present(struct drm_i915_private
*dev_priv)
if (IS_HSW_ULT(dev_priv) || IS_BDW_ULT(dev_priv))
return false;
- if (IS_CHERRYVIEW(dev_priv))
- return false;
-
if (HAS_PCH_LPT_H(dev_priv) &&
I915_READ(SFUSE_STRAP) & SFUSE_STRAP_CRT_DISABLED)
return false;
/* DDI E can't be used if DDI A requires 4 lanes */
- if (HAS_DDI(dev_priv) && I915_READ(DDI_BUF_CTL(PORT_A)) & DDI_A_4_LANES)
+ if (I915_READ(DDI_BUF_CTL(PORT_A)) & DDI_A_4_LANES)
return false;
if (!dev_priv->vbt.int_crt_support)
@@ -14323,9 +14320,6 @@ static void intel_setup_outputs(struct drm_i915_private
*dev_priv)
*/
intel_lvds_init(dev_priv);
- if (intel_crt_present(dev_priv))
- intel_crt_init(dev_priv);
-
if (IS_ICELAKE(dev_priv)) {
intel_ddi_init(dev_priv, PORT_A);
intel_ddi_init(dev_priv, PORT_B);
@@ -14354,6 +14348,9 @@ static void intel_setup_outputs(struct drm_i915_private
*dev_priv)
} else if (HAS_DDI(dev_priv)) {
int found;
+ if (intel_ddi_crt_present(dev_priv))
+ intel_crt_init(dev_priv);
+
/*
* Haswell uses DDI functions to detect digital outputs.
* On SKL pre-D0 the strap isn't connected, so we assume
@@ -14385,6 +14382,10 @@ static void intel_setup_outputs(struct
drm_i915_private *dev_priv)
} else if (HAS_PCH_SPLIT(dev_priv)) {
int found;
+
+ if (dev_priv->vbt.int_crt_support)
+ intel_crt_init(dev_priv);
+
dpd_is_edp = intel_dp_is_port_edp(dev_priv, PORT_D);
if (has_edp_a(dev_priv))
@@ -14413,6 +14414,9 @@ static void intel_setup_outputs(struct drm_i915_private
*dev_priv)
} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
bool has_edp, has_port;
+ if (IS_VALLEYVIEW(dev_priv) && dev_priv->vbt.int_crt_support)
+ intel_crt_init(dev_priv);
+
/*
* The DP_DETECTED bit is the latched state of the DDC
* SDA pin at boot. However since eDP doesn't require DDC
@@ -14455,9 +14459,15 @@ static void intel_setup_outputs(struct
drm_i915_private *dev_priv)
}
vlv_dsi_init(dev_priv);
- } else if (!IS_GEN(dev_priv, 2) && !IS_PINEVIEW(dev_priv)) {
+ } else if (IS_PINEVIEW(dev_priv)) {
+ if (dev_priv->vbt.int_crt_support)
+ intel_crt_init(dev_priv);
+ } else if (IS_GEN_RANGE(dev_priv, 3, 4)) {
bool found = false;
+ if (dev_priv->vbt.int_crt_support)
+ intel_crt_init(dev_priv);
+
if (I915_READ(GEN3_SDVOB) & SDVO_DETECTED) {
DRM_DEBUG_KMS("probing SDVOB\n");
found = intel_sdvo_init(dev_priv, GEN3_SDVOB, PORT_B);
@@ -14489,8 +14499,12 @@ static void intel_setup_outputs(struct
drm_i915_private *dev_priv)
if (IS_G4X(dev_priv) && (I915_READ(DP_D) & DP_DETECTED))
intel_dp_init(dev_priv, DP_D, PORT_D);
- } else if (IS_GEN(dev_priv, 2))
+ } else if (IS_GEN(dev_priv, 2)) {
+ if (dev_priv->vbt.int_crt_support)
+ intel_crt_init(dev_priv);
+
intel_dvo_init(dev_priv);
+ }
if (SUPPORTS_TV(dev_priv))
intel_tv_init(dev_priv);
--
2.20.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: Split out drm_probe_helper.h
On Mon, Jan 21, 2019 at 11:13 PM Sam Ravnborg wrote: > > Hi Daniel et al. > > > > > > > Yeah the drm_crtc_helper.h header is a bit the miniature drmP.h for legacy > > > kms drivers. Just removing it from all the atomic drivers caused lots of > > > fallout, I expect even more if you entirely remove the includes it has. > > > Maybe a todo, care to pls create that patch since it's your idea? > > > > The main reason I bailed out initially was that this would create > > small changes to several otherwise seldomly touched files. > > And then we would later come and remove drmP.h - so lots of > > small but incremental changes to the same otherwise seldomly > > edited files. > > And the job was only partially done. > > > > I will try to experiment with an approach where I clean up the > > include/drm/*.h files a little (like suggested above, +delete drmP.h > > and maybe a bit more). > > > > Then to try on a driver by driver basis to make it build with a > > cleaned set of include files. > > I hope that the cleaned up driver can still build without the > > cleaned header files so the changes can be submitted piecemal. > > > > Will do so with an eye on the lesser maintained drivers to try it > > out to avoid creating too much chrunch for others. > > I have now a few patches queued, but the result is not too pretty. > I did the following: > > - For all files in include/drm/*.h the set of include files > were adjusted to the minimum number of files required to make > them build without any other files included first. > > Created one .c file for each .h file. Then included the .h > file and adjusted to the minimal set of include files. > In the process a lot of forwards were added. > > - Deleted drmP.h > > - Fixed build of a few drivers: sti, tilcdc, gma500, tve200, via > > Some observations: > > - Killing all the includes not needed in the headers files > results in a a lot of extra changes. > Examples: > drm_modseset_helper_vtables.h is no longer > included by anyone, so needs to be added in many files > > drm_atomic_state_helper.h is no longer included > by anyone so likewise needs to be added in many files > > - It is very tedious to do this properly. > The process I followed was: > - delete / comment out all include files > - add back the obvious from a quick scan of the code > - build - fix - build - fix - build - fix ... > - next file... > > - The result is errorprone as only the allyesconfig + allmodconfig > variants are tested. But reallife configurations are more diverse. > > Current diffstat: >111 files changed, 771 insertions(+), 401 deletions(-) > > This is for the 5 drivers alone and not the header cleanup. > So long story short - this is not good and not the way forward. > > I will try to come up with a few improvements to make the > headers files selfcontained, but restricted to the changes that > add forwards/include to avoid the chrunch in all the drivers. > > And then post for review a few patches to clean up some headers. > If the cleanup gets a go I will try to persuade the introduction > of these. > This will include, but will not be limited to, the above mentioned > drm_crtc_helper.h header file. > > For now too much time was already spent on this, so it is at the > moment pushed back on my TODO list. > This mail serve also as a kind of "where had I left", when/if I > pick this up again. > > If there are anyone that knows some tooling that can help in the > process of adjusting the header files I am all ears. Yeah in the process of splitting up drmP.h we've created a few smaller such piles of headers. I think in some cases it's just not going to be worth it to fully split them up, e.g. drm_crtc_helper.h is going to be a pure legacy helper, only needed by pre-atomic drivers. Splitting that up doesn't seem to useful to me. Similarly we might want drm_atomic_helper.h to keep pulling in the other helper headers. So probably going to be a judgement call on a case-by-case basis. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
