Re: [Intel-gfx] [PATCH 2/2] drm/i915: Protect engine request list with spinlock
On Tue, Feb 24, 2015 at 12:58:19AM +0100, Daniel Vetter wrote: > On Thu, Feb 19, 2015 at 04:41:12PM +, Chris Wilson wrote: > > On Thu, Feb 19, 2015 at 06:18:55PM +0200, Mika Kuoppala wrote: > > > There are multiple players interested in the ring->request_list > > > state. Request submission can happen in kernel or user context, > > > idle worker is going through request list to free items. And then there > > > is hangcheck worker which tries to figure out if particular ring is > > > healthy by peeking at the request list among other things. And if > > > judged stuck by hangcheck, error state is colleted. Which in turns > > > needs access to ring->request_list. > > > > We have discussed this before. Hangcheck does not need the lock so long > > as it is serialised with deletion. List processing with hangcheck during > > concurrent addition is safe. > > > > For example, I expect the request locking to look like > > > > http://cgit.freedesktop.org/~ickle/linux-2.6/tree/drivers/gpu/drm/i915/i915_gem_request.c#n691 > > I think longer-term with per-engine reset and fun stuff like that we > probably want the spinlock, just to avoid too many headaches with locking > auditing. For the execbuf fastpath it should just be one more spinlock per > ioctl, so hopefully bearable. But it is not even the locking bug that breaks capture, so what's the point? -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v4] drm/i915: avoid processing spurious/shared interrupts in low-power states
Atm, it's possible that the interrupt handler is called when the device
is in D3 or some other low-power state. It can be due to another device
that is still in D0 state and shares the interrupt line with i915, or on
some platforms there could be spurious interrupts even without sharing
the interrupt line. The latter case was reported by Klaus Ethgen using a
Lenovo x61p machine (gen 4). He noticed this issue via a system
suspend/resume hang and bisected it to the following commit:
commit e11aa362308f5de467ce355a2a2471321b15a35c
Author: Jesse Barnes
Date: Wed Jun 18 09:52:55 2014 -0700
drm/i915: use runtime irq suspend/resume in freeze/thaw
This is a problem, since in low-power states IIR will always read
0x resulting in an endless IRQ servicing loop.
Fix this by handling interrupts only when the driver explicitly enables
them and so it's guaranteed that the interrupt registers return a valid
value.
Note that this issue existed even before the above commit, since during
runtime suspend/resume we never unregistered the handler.
v2:
- clarify the purpose of smp_mb() vs. synchronize_irq() in the
code comment (Chris)
v3:
- no need for an explicit smp_mb(), we can assume that synchronize_irq()
and the mmio read/writes in the install hooks provide for this (Daniel)
- remove code comment as the remaining synchronize_irq() is self
explanatory (Daniel)
v4:
- drm_irq_uninstall() implies synchronize_irq(), so no need to call it
explicitly (Daniel)
Reference: https://lkml.org/lkml/2015/2/11/205
Reported-and-bisected-by: Klaus Ethgen
Signed-off-by: Imre Deak
Reviewed-by: Daniel Vetter
---
drivers/gpu/drm/i915/i915_irq.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 9073119..1561248 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1889,6 +1889,9 @@ static irqreturn_t valleyview_irq_handler(int irq, void
*arg)
u32 iir, gt_iir, pm_iir;
irqreturn_t ret = IRQ_NONE;
+ if (!intel_irqs_enabled(dev_priv))
+ return IRQ_NONE;
+
while (true) {
/* Find, clear, then process each source of interrupt */
@@ -1933,6 +1936,9 @@ static irqreturn_t cherryview_irq_handler(int irq, void
*arg)
u32 master_ctl, iir;
irqreturn_t ret = IRQ_NONE;
+ if (!intel_irqs_enabled(dev_priv))
+ return IRQ_NONE;
+
for (;;) {
master_ctl = I915_READ(GEN8_MASTER_IRQ) &
~GEN8_MASTER_IRQ_CONTROL;
iir = I915_READ(VLV_IIR);
@@ -2205,6 +2211,9 @@ static irqreturn_t ironlake_irq_handler(int irq, void
*arg)
u32 de_iir, gt_iir, de_ier, sde_ier = 0;
irqreturn_t ret = IRQ_NONE;
+ if (!intel_irqs_enabled(dev_priv))
+ return IRQ_NONE;
+
/* We get interrupts on unclaimed registers, so check for this before we
* do any I915_{READ,WRITE}. */
intel_uncore_check_errors(dev);
@@ -2276,6 +2285,9 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
enum pipe pipe;
u32 aux_mask = GEN8_AUX_CHANNEL_A;
+ if (!intel_irqs_enabled(dev_priv))
+ return IRQ_NONE;
+
if (IS_GEN9(dev))
aux_mask |= GEN9_AUX_CHANNEL_B | GEN9_AUX_CHANNEL_C |
GEN9_AUX_CHANNEL_D;
@@ -3768,6 +3780,9 @@ static irqreturn_t i8xx_irq_handler(int irq, void *arg)
I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT |
I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT;
+ if (!intel_irqs_enabled(dev_priv))
+ return IRQ_NONE;
+
iir = I915_READ16(IIR);
if (iir == 0)
return IRQ_NONE;
@@ -3948,6 +3963,9 @@ static irqreturn_t i915_irq_handler(int irq, void *arg)
I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT;
int pipe, ret = IRQ_NONE;
+ if (!intel_irqs_enabled(dev_priv))
+ return IRQ_NONE;
+
iir = I915_READ(IIR);
do {
bool irq_received = (iir & ~flip_mask) != 0;
@@ -4168,6 +4186,9 @@ static irqreturn_t i965_irq_handler(int irq, void *arg)
I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT |
I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT;
+ if (!intel_irqs_enabled(dev_priv))
+ return IRQ_NONE;
+
iir = I915_READ(IIR);
for (;;) {
@@ -4517,6 +4538,7 @@ void intel_runtime_pm_disable_interrupts(struct
drm_i915_private *dev_priv)
{
dev_priv->dev->driver->irq_uninstall(dev_priv->dev);
dev_priv->pm.irqs_enabled = false;
+ synchronize_irq(dev_priv->dev->irq);
}
/**
--
2.1.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t 2/4] lib: use defines for igt_simple_init and igt_subtest_init
On 23 February 2015 at 23:49, Daniel Vetter wrote:
> What's the benifit here? Would be nice to add that to the commit message
> with a short sentence. Series lgtm otherwise, imo you could push it as-is.
It is partly as preparation for the next patch and partly just a
simplification to remove an extra function call. I'll add a suitable
comment to the commit message.
> -Daniel
>
> On Wed, Feb 18, 2015 at 05:06:14PM +, Thomas Wood wrote:
>> Signed-off-by: Thomas Wood
>> ---
>> lib/igt_core.c | 34 --
>> lib/igt_core.h | 36
>> 2 files changed, 32 insertions(+), 38 deletions(-)
>>
>> diff --git a/lib/igt_core.c b/lib/igt_core.c
>> index eef338b..afecdf1 100644
>> --- a/lib/igt_core.c
>> +++ b/lib/igt_core.c
>> @@ -696,40 +696,6 @@ int igt_subtest_init_parse_opts(int argc, char **argv,
>> enum igt_log_level igt_log_level = IGT_LOG_INFO;
>>
>> /**
>> - * igt_subtest_init:
>> - * @argc: argc from the test's main()
>> - * @argv: argv from the test's main()
>> - *
>> - * This initializes the for tests with subtests without the need for
>> additional
>> - * cmdline options. It is just a simplified version of
>> - * igt_subtest_init_parse_opts().
>> - *
>> - * If there's not a reason to the contrary it's less error prone to just
>> use an
>> - * #igt_main block instead of stitching the tests's main() function together
>> - * manually.
>> - */
>> -void igt_subtest_init(int argc, char **argv)
>> -{
>> - igt_subtest_init_parse_opts(argc, argv, NULL, NULL, NULL, NULL);
>> -}
>> -
>> -/**
>> - * igt_simple_init:
>> - * @argc: argc from the test's main()
>> - * @argv: argv from the test's main()
>> - *
>> - * This initializes a simple test without any support for subtests.
>> - *
>> - * If there's not a reason to the contrary it's less error prone to just
>> use an
>> - * #igt_simple_main block instead of stitching the tests's main() function
>> together
>> - * manually.
>> - */
>> -void igt_simple_init(int argc, char **argv)
>> -{
>> - common_init(argc, argv, NULL, NULL, NULL, NULL);
>> -}
>> -
>> -/**
>> * igt_simple_init_parse_opts:
>> * @argc: argc from the test's main()
>> * @argv: argv from the test's main()
>> diff --git a/lib/igt_core.h b/lib/igt_core.h
>> index 0086945..88b47bf 100644
>> --- a/lib/igt_core.h
>> +++ b/lib/igt_core.h
>> @@ -106,7 +106,6 @@ void __igt_fixture_end(void) __attribute__((noreturn));
>>
>> /* subtest infrastructure */
>> jmp_buf igt_subtest_jmpbuf;
>> -void igt_subtest_init(int argc, char **argv);
>> typedef int (*igt_opt_handler_t)(int opt, int opt_index);
>> #ifndef __GTK_DOC_IGNORE__ /* gtkdoc wants to document this forward decl */
>> struct option;
>> @@ -117,6 +116,22 @@ int igt_subtest_init_parse_opts(int argc, char **argv,
>> const char *help_str,
>> igt_opt_handler_t extra_opt_handler);
>>
>> +
>> +/**
>> + * igt_subtest_init:
>> + * @argc: argc from the test's main()
>> + * @argv: argv from the test's main()
>> + *
>> + * This initializes the for tests with subtests without the need for
>> additional
>> + * cmdline options. It is just a simplified version of
>> + * igt_subtest_init_parse_opts().
>> + *
>> + * If there's not a reason to the contrary it's less error prone to just
>> use an
>> + * #igt_main block instead of stitching the test's main() function together
>> + * manually.
>> + */
>> +#define igt_subtest_init(argc, argv) igt_subtest_init_parse_opts(argc,
>> argv, NULL, NULL, NULL, NULL);
>> +
>> bool __igt_run_subtest(const char *subtest_name);
>> #define __igt_tokencat2(x, y) x ## y
>>
>> @@ -180,13 +195,13 @@ bool igt_only_list_subtests(void);
>> #define igt_main \
>> static void igt_tokencat(__real_main, __LINE__)(void); \
>> int main(int argc, char **argv) { \
>> - igt_subtest_init(argc, argv); \
>> + igt_subtest_init_parse_opts(argc, argv, NULL, NULL, NULL,
>> NULL); \
>> igt_tokencat(__real_main, __LINE__)(); \
>> igt_exit(); \
>> } \
>> static void igt_tokencat(__real_main, __LINE__)(void) \
>>
>> -void igt_simple_init(int argc, char **argv);
>> +
>> void igt_simple_init_parse_opts(int argc, char **argv,
>> const char *extra_short_opts,
>> struct option *extra_long_opts,
>> @@ -194,6 +209,19 @@ void igt_simple_init_parse_opts(int argc, char **argv,
>> igt_opt_handler_t extra_opt_handler);
>>
>> /**
>> + * igt_simple_init:
>> + * @argc: argc from the test's main()
>> + * @argv: argv from the test's main()
>> + *
>> + * This initializes a simple test without any support for subtests.
>> + *
>> + * If there's not a reason to the contrary it's less error prone to just
>> use an
>> + * #igt_simple_main block instead of stitching the test's main() function
>> together
>> + * manually.
>> + */
>> +#define igt_simple_init(argc, argv) igt
Re: [Intel-gfx] [PATCH] drm/i915: Shift driver's HWSP usage out of reserved range
> -Original Message- > From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel Vetter > Sent: Monday, February 23, 2015 11:28 PM > To: Gordon, David S > Cc: Daniel, Thomas; intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Shift driver's HWSP usage out of > reserved range > > On Thu, Feb 19, 2015 at 02:58:48PM +, Dave Gordon wrote: > > On 18/02/15 11:48, Thomas Daniel wrote: > > > As of Gen6, the general purpose area of the hardware status page has > > > shrunk > and > > > now begins at dword 0x30. i915 driver uses dword 0x20 to store the seqno > which > > > is now reserved. So shift our HWSP dwords up into the general purpose > range > > > before this bites us. > > It would be really interesting to know what exactly the hw does with > offsets below 0x30 ... it might explain some of the bugs we've seen. Can > you please digg that out so that I can amend the commit message? All documentation I've seen just says "Reserved" for current hardware including SKL so we can't rely on any particular usage. That's why I just put "Reserved" for these dwords in the comment. Thomas. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Protect engine request list with spinlock
On Tue, Feb 24, 2015 at 11:39:08AM +0100, Daniel Vetter wrote: > On Tue, Feb 24, 2015 at 08:31:18AM +, Chris Wilson wrote: > > On Tue, Feb 24, 2015 at 12:58:19AM +0100, Daniel Vetter wrote: > > > On Thu, Feb 19, 2015 at 04:41:12PM +, Chris Wilson wrote: > > > > On Thu, Feb 19, 2015 at 06:18:55PM +0200, Mika Kuoppala wrote: > > > > > There are multiple players interested in the ring->request_list > > > > > state. Request submission can happen in kernel or user context, > > > > > idle worker is going through request list to free items. And then > > > > > there > > > > > is hangcheck worker which tries to figure out if particular ring is > > > > > healthy by peeking at the request list among other things. And if > > > > > judged stuck by hangcheck, error state is colleted. Which in turns > > > > > needs access to ring->request_list. > > > > > > > > We have discussed this before. Hangcheck does not need the lock so long > > > > as it is serialised with deletion. List processing with hangcheck during > > > > concurrent addition is safe. > > > > > > > > For example, I expect the request locking to look like > > > > > > > > http://cgit.freedesktop.org/~ickle/linux-2.6/tree/drivers/gpu/drm/i915/i915_gem_request.c#n691 > > > > > > I think longer-term with per-engine reset and fun stuff like that we > > > probably want the spinlock, just to avoid too many headaches with locking > > > auditing. For the execbuf fastpath it should just be one more spinlock per > > > ioctl, so hopefully bearable. > > > > But it is not even the locking bug that breaks capture, so what's the > > point? > > Oh I've read the patch as general prep work for more finegrained reset > support not as a fix for the referenced bug. I guess the bug is just the > usual incoherent seqno/irq thing that's been plagueing us ever since gen6? I presumed Mika wants to fix that hangcheck and capture may explode as requests are completed concurrently. The bug that I expect will remain is that we peek at the bo without locks during capture. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v4] drm/i915: avoid processing spurious/shared interrupts in low-power states
On Tue, Feb 24, 2015 at 11:42:20AM +0100, Daniel Vetter wrote: > On Tue, Feb 24, 2015 at 09:29:15AM +, Chris Wilson wrote: > > On Tue, Feb 24, 2015 at 11:14:30AM +0200, Imre Deak wrote: > > > Atm, it's possible that the interrupt handler is called when the device > > > is in D3 or some other low-power state. It can be due to another device > > > that is still in D0 state and shares the interrupt line with i915, or on > > > some platforms there could be spurious interrupts even without sharing > > > the interrupt line. The latter case was reported by Klaus Ethgen using a > > > Lenovo x61p machine (gen 4). He noticed this issue via a system > > > suspend/resume hang and bisected it to the following commit: > > > > > > commit e11aa362308f5de467ce355a2a2471321b15a35c > > > Author: Jesse Barnes > > > Date: Wed Jun 18 09:52:55 2014 -0700 > > > > > > drm/i915: use runtime irq suspend/resume in freeze/thaw > > > > > > This is a problem, since in low-power states IIR will always read > > > 0x resulting in an endless IRQ servicing loop. > > > > > > Fix this by handling interrupts only when the driver explicitly enables > > > them and so it's guaranteed that the interrupt registers return a valid > > > value. > > > > > > Note that this issue existed even before the above commit, since during > > > runtime suspend/resume we never unregistered the handler. > > > > > > v2: > > > - clarify the purpose of smp_mb() vs. synchronize_irq() in the > > > code comment (Chris) > > > > > > v3: > > > - no need for an explicit smp_mb(), we can assume that synchronize_irq() > > > and the mmio read/writes in the install hooks provide for this (Daniel) > > > - remove code comment as the remaining synchronize_irq() is self > > > explanatory (Daniel) > > > > Why make the assumption though? I thought the comments documenting the > > interaction between the irq enablements, the irq handler and shared > > interrupts was beneficial. At the very least the assumption should be > > made explicit through comments in the code - because I am not convinced > > that a cached write will be flushed by an uncached write to another area > > of memory. In particular, note that on the gen most troubled by rogue > > irqs (gen4), we do not have any memory barriers in the mmio paths. > > The synchronize_irq is a fairly massive barrier and I've figured the name > is descriptive enough to make clear what's going on. At least I've felt > any comment on top would be redundant. > > Also the hard rule for adding comments to explicit barriers is mostly a > reminder that you always need barriers on both sides, and the comment > then must explain where the other side is in the code. Imo with > synchronize_irq it's clear that the other side is the irq handler already. > > What do you want to clarify on top of that in the comment? The smp_mb() was being applied to the enable path, not the disable + sync_irq path. Also we now have a large body of lore regarding interrupt issues on gen4 but we haven't been documenting any of it. At the bare minimum we need to explain why we call sync_irq and how that interacts with the intel_irqs_enabled() checks and shared interrupts. Even perhaps adding intel_runtime_pm_enabled_irq() as a wrapper for the irq handlers to cross-reference against the runtime_pm tricks - otherwise, there is a good likelihood that in the years to come, we might forget how we end up inside a disabled interrupt handler and why we are not allowed to touch the hardware at that point. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: page table abstractions
From: Ben Widawsky
When we move to dynamic page allocation, keeping page_directory and pagetabs as
separate structures will help to break actions into simpler tasks.
To help transition the code nicely there is some wasted space in gen6/7.
This will be ameliorated shortly.
Following the x86 pagetable terminology:
PDPE = struct i915_page_directory_pointer_entry.
PDE = struct i915_page_directory_entry [page_directory].
PTE = struct i915_page_table_entry [page_tables].
v2: fixed mismatches after clean-up/rebase.
v3: Clarify the names of the multiple levels of page tables (Daniel)
v4: Addressing Mika's review comments.
s/gen8_free_page_directories/gen8_free_page_directory and free the
page tables for the directory there.
In gen8_ppgtt_allocate_page_directories, do not leak previously allocated
pt in case the page_directory alloc fails.
Update error return handling in gen8_ppgtt_alloc.
v5: Do not leak pt on error in gen6_ppgtt_allocate_page_tables. (Mika)
Cc: Mika Kuoppala
Signed-off-by: Ben Widawsky
Signed-off-by: Michel Thierry (v2+)
---
drivers/gpu/drm/i915/i915_gem_gtt.c | 180 +++-
drivers/gpu/drm/i915/i915_gem_gtt.h | 23 -
2 files changed, 111 insertions(+), 92 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index e54b2a0..b4dee34 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -338,7 +338,8 @@ static void gen8_ppgtt_clear_range(struct
i915_address_space *vm,
I915_CACHE_LLC, use_scratch);
while (num_entries) {
- struct page *page_table = ppgtt->gen8_pt_pages[pdpe][pde];
+ struct i915_page_directory_entry *pd =
&ppgtt->pdp.page_directory[pdpe];
+ struct page *page_table = pd->page_tables[pde].page;
last_pte = pte + num_entries;
if (last_pte > GEN8_PTES_PER_PAGE)
@@ -382,8 +383,12 @@ static void gen8_ppgtt_insert_entries(struct
i915_address_space *vm,
if (WARN_ON(pdpe >= GEN8_LEGACY_PDPES))
break;
- if (pt_vaddr == NULL)
- pt_vaddr = kmap_atomic(ppgtt->gen8_pt_pages[pdpe][pde]);
+ if (pt_vaddr == NULL) {
+ struct i915_page_directory_entry *pd =
&ppgtt->pdp.page_directory[pdpe];
+ struct page *page_table = pd->page_tables[pde].page;
+
+ pt_vaddr = kmap_atomic(page_table);
+ }
pt_vaddr[pte] =
gen8_pte_encode(sg_page_iter_dma_address(&sg_iter),
@@ -407,29 +412,33 @@ static void gen8_ppgtt_insert_entries(struct
i915_address_space *vm,
}
}
-static void gen8_free_page_tables(struct page **pt_pages)
+static void gen8_free_page_tables(struct i915_page_directory_entry *pd)
{
int i;
- if (pt_pages == NULL)
+ if (pd->page_tables == NULL)
return;
for (i = 0; i < GEN8_PDES_PER_PAGE; i++)
- if (pt_pages[i])
- __free_pages(pt_pages[i], 0);
+ if (pd->page_tables[i].page)
+ __free_page(pd->page_tables[i].page);
}
-static void gen8_ppgtt_free(const struct i915_hw_ppgtt *ppgtt)
+static void gen8_free_page_directory(struct i915_page_directory_entry *pd)
+{
+ gen8_free_page_tables(pd);
+ kfree(pd->page_tables);
+ __free_page(pd->page);
+}
+
+static void gen8_ppgtt_free(struct i915_hw_ppgtt *ppgtt)
{
int i;
for (i = 0; i < ppgtt->num_pd_pages; i++) {
- gen8_free_page_tables(ppgtt->gen8_pt_pages[i]);
- kfree(ppgtt->gen8_pt_pages[i]);
+ gen8_free_page_directory(&ppgtt->pdp.page_directory[i]);
kfree(ppgtt->gen8_pt_dma_addr[i]);
}
-
- __free_pages(ppgtt->pd_pages, get_order(ppgtt->num_pd_pages <<
PAGE_SHIFT));
}
static void gen8_ppgtt_unmap_pages(struct i915_hw_ppgtt *ppgtt)
@@ -464,86 +473,77 @@ static void gen8_ppgtt_cleanup(struct i915_address_space
*vm)
gen8_ppgtt_free(ppgtt);
}
-static struct page **__gen8_alloc_page_tables(void)
+static int gen8_ppgtt_allocate_dma(struct i915_hw_ppgtt *ppgtt)
{
- struct page **pt_pages;
int i;
- pt_pages = kcalloc(GEN8_PDES_PER_PAGE, sizeof(struct page *),
GFP_KERNEL);
- if (!pt_pages)
- return ERR_PTR(-ENOMEM);
-
- for (i = 0; i < GEN8_PDES_PER_PAGE; i++) {
- pt_pages[i] = alloc_page(GFP_KERNEL);
- if (!pt_pages[i])
- goto bail;
+ for (i = 0; i < ppgtt->num_pd_pages; i++) {
+ ppgtt->gen8_pt_dma_addr[i] = kcalloc(GEN8_PDES_PER_PAGE,
+sizeof(dma_addr_t),
+GFP_KERNEL);
+ if (!ppgtt->gen8_pt_dma_addr[i])
+
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Protect engine request list with spinlock
Chris Wilson writes: > On Tue, Feb 24, 2015 at 11:39:08AM +0100, Daniel Vetter wrote: >> On Tue, Feb 24, 2015 at 08:31:18AM +, Chris Wilson wrote: >> > On Tue, Feb 24, 2015 at 12:58:19AM +0100, Daniel Vetter wrote: >> > > On Thu, Feb 19, 2015 at 04:41:12PM +, Chris Wilson wrote: >> > > > On Thu, Feb 19, 2015 at 06:18:55PM +0200, Mika Kuoppala wrote: >> > > > > There are multiple players interested in the ring->request_list >> > > > > state. Request submission can happen in kernel or user context, >> > > > > idle worker is going through request list to free items. And then >> > > > > there >> > > > > is hangcheck worker which tries to figure out if particular ring is >> > > > > healthy by peeking at the request list among other things. And if >> > > > > judged stuck by hangcheck, error state is colleted. Which in turns >> > > > > needs access to ring->request_list. >> > > > >> > > > We have discussed this before. Hangcheck does not need the lock so long >> > > > as it is serialised with deletion. List processing with hangcheck >> > > > during >> > > > concurrent addition is safe. >> > > > >> > > > For example, I expect the request locking to look like >> > > > >> > > > http://cgit.freedesktop.org/~ickle/linux-2.6/tree/drivers/gpu/drm/i915/i915_gem_request.c#n691 >> > > >> > > I think longer-term with per-engine reset and fun stuff like that we >> > > probably want the spinlock, just to avoid too many headaches with locking >> > > auditing. For the execbuf fastpath it should just be one more spinlock >> > > per >> > > ioctl, so hopefully bearable. >> > >> > But it is not even the locking bug that breaks capture, so what's the >> > point? >> >> Oh I've read the patch as general prep work for more finegrained reset >> support not as a fix for the referenced bug. I guess the bug is just the >> usual incoherent seqno/irq thing that's been plagueing us ever since gen6? > > I presumed Mika wants to fix that hangcheck and capture may explode as > requests are completed concurrently. The bug that I expect will remain > is that we peek at the bo without locks during capture. > -Chris > What I think is the failure mode on [1] is: Request gets added to the ring but not yet into the ring->request_list, gpu finishes it and updates the hw status page. Hangcheck runs and sees that request_list does not contain the supposed request and complains that the hangcheck was activated on idle ring. https://bugs.freedesktop.org/show_bug.cgi?id=88651 -Mika > -- > Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/skl: handle all pixel formats in skylake_update_primary_plane()
On Mon, Feb 23, 2015 at 03:15:58PM +0200, Jani Nikula wrote: > On Mon, 16 Feb 2015, Damien Lespiau wrote: > > On Tue, Feb 10, 2015 at 01:15:49PM +0200, Jani Nikula wrote: > >> skylake_update_primary_plane() did not handle all pixel formats returned > >> by skl_format_to_fourcc(). Handle alpha similar to skl_update_plane(). > >> > >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89052 > >> Signed-off-by: Jani Nikula > > > > Given the discussion with Ville, it's quite likely we'll default to > > alpha blending for pre-multiplied fbs (for plane supporting alpha), even > > with the blending properties added. In that context, we can provide a > > single, fixed, (but useful) blending mode before we get to implement the > > full thing. So: > > > > Reviewed-by: Damien Lespiau > > Pushed to drm-intel-fixes, thanks for the review. Well, the patch still isn't quite right. intel_primary_formats_gen4[] defines which formats the core will let slip through into the driver, and even with this patch we're not handling them all. -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Fix a use after free, and unbalanced refcounting
On Mon, 23 Feb 2015, "Daniel, Thomas" wrote:
>> -Original Message-
>> From: Intel-gfx [mailto:intel-gfx-boun...@lists.freedesktop.org] On Behalf Of
>> Nick Hoath
>> Sent: Thursday, February 19, 2015 4:31 PM
>> To: intel-gfx@lists.freedesktop.org
>> Subject: [Intel-gfx] [PATCH] drm/i915: Fix a use after free, and unbalanced
>> refcounting
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=88652
>>
>> When converting from implicitly tracked execlist queue items to ref counted
>> requests, not all frees of requests were replaced with unrefs, and extraneous
>> refs/unrefs of contexts were added.
>> Correct the unbalanced refcount & replace the frees.
>> Remove a noisy warning when hitting the request creation path.
>>
>> drm_i915_gem_request and intel_context are both kref reference counted
>> structures. Upon allocation, drm_i915_gem_request's ref count should be
>> bumped using kref_init. When a context is assigned to the request,
>> the context's reference count should be bumped using
>> i915_gem_context_reference.
>> i915_gem_request_reference will reduce the context reference count when
>> the request is freed.
>>
>> Problem introduced in
>> commit 6d3d8274bc45de4babb62d64562d92af984dd238
>> Author: Nick Hoath
>> AuthorDate: Thu Jan 15 13:10:39 2015 +
>>
>> drm/i915: Subsume intel_ctx_submit_request in to drm_i915_gem_request
>>
>> v2: Added comments explaining how the ctx pointer and the request object
>> should
>> be ref-counted. Removed noisy warning.
>>
>> v3: Cleaned up the language used in the commit & the header
>> description (Thanks David Gordon)
>>
>> Signed-off-by: Nick Hoath
>> ---
>> drivers/gpu/drm/i915/i915_drv.h | 11 ++-
>> drivers/gpu/drm/i915/i915_gem.c | 3 +--
>> drivers/gpu/drm/i915/intel_lrc.c | 8
>> 4 files changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index 2dedd43..956fe26 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2121,6 +2121,9 @@ void i915_gem_track_fb(struct drm_i915_gem_object
>> *old,
>> * number comparisons on buffer last_read|write_seqno. It also allows an
>> * emission time to be associated with the request for tracking how far
>> ahead
>> * of the GPU the submission is.
>> + *
>> + * The requests are reference counted, so upon creation they should have an
>> + * initial reference taken using kref_init
>> */
>> struct drm_i915_gem_request {
>> struct kref ref;
>> @@ -2144,7 +2147,16 @@ struct drm_i915_gem_request {
>> /** Position in the ringbuffer of the end of the whole request */
>> u32 tail;
>>
>> -/** Context related to this request */
>> +/**
>> + * Context related to this request
>> + * Contexts are refcounted, so when this request is associated with a
>> + * context, we must increment the context's refcount, to guarantee that
>> + * it persists while any request is linked to it. Requests themselves
>> + * are also refcounted, so the request will only be freed when the last
>> + * reference to it is dismissed, and the code in
>> + * i915_gem_request_free() will then decrement the refcount on the
>> + * context.
>> + */
>> struct intel_context *ctx;
>>
>> /** Batch buffer related to this request if any */
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c
>> b/drivers/gpu/drm/i915/i915_gem.c
>> index 61134ab..996f60f 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -2664,8 +2664,7 @@ static void i915_gem_reset_ring_cleanup(struct
>> drm_i915_private *dev_priv,
>> if (submit_req->ctx != ring->default_context)
>> intel_lr_context_unpin(ring, submit_req->ctx);
>>
>> -i915_gem_context_unreference(submit_req->ctx);
>> -kfree(submit_req);
>> +i915_gem_request_unreference(submit_req);
>> }
>>
>> /*
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c
>> b/drivers/gpu/drm/i915/intel_lrc.c
>> index aafcef3..62a2b2a 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> @@ -512,18 +512,19 @@ static int execlists_context_queue(struct
>> intel_engine_cs *ring,
>> * If there isn't a request associated with this submission,
>> * create one as a temporary holder.
>> */
>> -WARN(1, "execlist context submission without request");
>> request = kzalloc(sizeof(*request), GFP_KERNEL);
>> if (request == NULL)
>> return -ENOMEM;
>> request->ring = ring;
>> request->ctx = to;
>> +kref_init(&request->ref);
>> +request->uniq = dev_priv->request_uniq++;
>> +i915_gem_context_reference(request->ctx);
>> } else {
>> +i915_gem_request_reference(request);
>> WARN_ON(
Re: [Intel-gfx] [PATCH] drm/i915: Do not invalidate obj->pages under mempressure
On Tue, 10 Feb 2015, Jani Nikula wrote:
> On Mon, 09 Feb 2015, Daniel Vetter wrote:
>> From: Chris Wilson
>>
>> This (partially) reverts
>>
>> commit 5537252b6b6d71fb1a8ed7395a8e5babf91953fd
>> Author: Chris Wilson
>> Date: Tue Mar 25 13:23:06 2014 +
>>
>> drm/i915: Invalidate our pages under memory pressure
>>
>> It appears given the right workload, that pages which are swapped out
>> more than once are incorrectly invalidated and discarded. I had presumed
>> that the swapin would mark the pages dirty again and so preserve them
>> against the next cycle of invalidation - that appears to be false, and
>> leads to memory corruption (even leak of stale pages to userspace).
>>
>> v2: Do a more throughrought revert and als get rid of the hunk in
>> gem_free_objects which we've tried to patch up already in
>>
>> commit 340fbd8ca1c7d6006a6b6afe716c10007bbfde85
>> Author: Chris Wilson
>> Date: Thu May 22 09:16:52 2014 +0100
>>
>> drm/i915: Only discard backing storage on releasing the last ref
>>
>> This means this patch also fully reverts this fixup. Apparently this
>> is just too tricky.
>>
>> Reported-by: Sean V Kelley
>> Signed-off-by: Chris Wilson
>> Cc: Sean V Kelley
>> Cc: sta...@vger.kernel.org
>> Cc: Chris Wilson
>> Cc: Jani Nikula
>> Signed-off-by: Daniel Vetter (v2)
>
> Pushed this one to drm-intel-next-fixes, thanks for the patch, and v2 of
> the patch.
For completeness, this one was dropped again on Feb 11.
Jani.
>
> BR,
> Jani.
>
>> ---
>> drivers/gpu/drm/i915/i915_gem.c | 49
>> ++---
>> 1 file changed, 2 insertions(+), 47 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c
>> b/drivers/gpu/drm/i915/i915_gem.c
>> index 36f1093e3c63..39e2af9b5fef 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -1946,26 +1946,6 @@ i915_gem_object_truncate(struct drm_i915_gem_object
>> *obj)
>> obj->madv = __I915_MADV_PURGED;
>> }
>>
>> -/* Try to discard unwanted pages */
>> -static void
>> -i915_gem_object_invalidate(struct drm_i915_gem_object *obj)
>> -{
>> -struct address_space *mapping;
>> -
>> -switch (obj->madv) {
>> -case I915_MADV_DONTNEED:
>> -i915_gem_object_truncate(obj);
>> -case __I915_MADV_PURGED:
>> -return;
>> -}
>> -
>> -if (obj->base.filp == NULL)
>> -return;
>> -
>> -mapping = file_inode(obj->base.filp)->i_mapping,
>> -invalidate_mapping_pages(mapping, 0, (loff_t)-1);
>> -}
>> -
>> static void
>> i915_gem_object_put_pages_gtt(struct drm_i915_gem_object *obj)
>> {
>> @@ -2028,7 +2008,8 @@ i915_gem_object_put_pages(struct drm_i915_gem_object
>> *obj)
>> ops->put_pages(obj);
>> obj->pages = NULL;
>>
>> -i915_gem_object_invalidate(obj);
>> +if (i915_gem_object_is_purgeable(obj))
>> +i915_gem_object_truncate(obj);
>>
>> return 0;
>> }
>> @@ -4458,30 +4439,6 @@ struct drm_i915_gem_object
>> *i915_gem_alloc_object(struct drm_device *dev,
>> return obj;
>> }
>>
>> -static bool discard_backing_storage(struct drm_i915_gem_object *obj)
>> -{
>> -/* If we are the last user of the backing storage (be it shmemfs
>> - * pages or stolen etc), we know that the pages are going to be
>> - * immediately released. In this case, we can then skip copying
>> - * back the contents from the GPU.
>> - */
>> -
>> -if (obj->madv != I915_MADV_WILLNEED)
>> -return false;
>> -
>> -if (obj->base.filp == NULL)
>> -return true;
>> -
>> -/* At first glance, this looks racy, but then again so would be
>> - * userspace racing mmap against close. However, the first external
>> - * reference to the filp can only be obtained through the
>> - * i915_gem_mmap_ioctl() which safeguards us against the user
>> - * acquiring such a reference whilst we are in the middle of
>> - * freeing the object.
>> - */
>> -return atomic_long_read(&obj->base.filp->f_count) == 1;
>> -}
>> -
>> void i915_gem_free_object(struct drm_gem_object *gem_obj)
>> {
>> struct drm_i915_gem_object *obj = to_intel_bo(gem_obj);
>> @@ -4524,8 +4481,6 @@ void i915_gem_free_object(struct drm_gem_object
>> *gem_obj)
>>
>> if (WARN_ON(obj->pages_pin_count))
>> obj->pages_pin_count = 0;
>> -if (discard_backing_storage(obj))
>> -obj->madv = I915_MADV_DONTNEED;
>> i915_gem_object_put_pages(obj);
>> i915_gem_object_free_mmap_offset(obj);
>>
>> --
>> 2.1.4
>>
>
> --
> Jani Nikula, Intel Open Source Technology Center
--
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 17/53] drm/i915: Split i915_ppgtt_init_hw() in half - generic and per ring
> -Original Message-
> From: Intel-gfx [mailto:intel-gfx-boun...@lists.freedesktop.org] On Behalf Of
> john.c.harri...@intel.com
> Sent: Thursday, February 19, 2015 5:17 PM
> To: Intel-GFX@Lists.FreeDesktop.Org
> Subject: [Intel-gfx] [PATCH 17/53] drm/i915: Split i915_ppgtt_init_hw() in
> half -
> generic and per ring
>
> From: John Harrison
>
> The i915_gem_init_hw() function calls a bunch of smaller initialisation
> functions. Multiple of which have generic sections and per ring sections. This
> means multiple passes are done over the rings. Each pass writes data to the
> ring
> which floats around in that ring's OLR until some random point in the future
> when an add_request() is done by some random other piece of code.
>
> This patch breaks i915_ppgtt_init_hw() in two with the per ring initialisation
> now being done in i915_ppgtt_init_ring(). The ring looping is now done at the
> top level in i915_gem_init_hw().
>
> For: VIZ-5115
> Signed-off-by: John Harrison
> ---
> drivers/gpu/drm/i915/i915_gem.c | 25 +++--
> drivers/gpu/drm/i915/i915_gem_gtt.c | 25 -
> drivers/gpu/drm/i915/i915_gem_gtt.h |1 +
> 3 files changed, 32 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> b/drivers/gpu/drm/i915/i915_gem.c
> index 51f719c..9bc60d7 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4844,19 +4844,32 @@ i915_gem_init_hw(struct drm_device *dev)
>*/
> init_unused_rings(dev);
>
> + ret = i915_ppgtt_init_hw(dev);
> + if (ret) {
> + DRM_ERROR("PPGTT enable HW failed %d\n", ret);
> + return ret;
> + }
> +
> + /* Need to do basic initialisation of all rings first: */
> for_each_ring(ring, dev_priv, i) {
> ret = ring->init_hw(ring);
> if (ret)
> return ret;
> }
>
> - for (i = 0; i < NUM_L3_SLICES(dev); i++)
> - i915_gem_l3_remap(&dev_priv->ring[RCS], i);
> + /* Now it is safe to go back round and do everything else: */
> + for_each_ring(ring, dev_priv, i) {
> + if (ring->id == RCS) {
> + for (i = 0; i < NUM_L3_SLICES(dev); i++)
> + i915_gem_l3_remap(ring, i);
> + }
>
> - ret = i915_ppgtt_init_hw(dev);
> - if (ret && ret != -EIO) {
> - DRM_ERROR("PPGTT enable failed %d\n", ret);
> - i915_gem_cleanup_ringbuffer(dev);
> + ret = i915_ppgtt_init_ring(ring);
> + if (ret && ret != -EIO) {
> + DRM_ERROR("PPGTT enable ring #%d failed %d\n", i,
> ret);
> + i915_gem_cleanup_ringbuffer(dev);
> + return ret;
> + }
> }
>
> ret = i915_gem_context_enable(dev_priv);
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
> b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index e54b2a0..428d2f6 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -1206,11 +1206,6 @@ int i915_ppgtt_init(struct drm_device *dev, struct
> i915_hw_ppgtt *ppgtt)
>
> int i915_ppgtt_init_hw(struct drm_device *dev)
> {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_engine_cs *ring;
> - struct i915_hw_ppgtt *ppgtt = dev_priv->mm.aliasing_ppgtt;
> - int i, ret = 0;
> -
> /* In the case of execlists, PPGTT is enabled by the context descriptor
>* and the PDPs are contained within the context itself. We don't
>* need to do anything here. */
> @@ -1229,16 +1224,20 @@ int i915_ppgtt_init_hw(struct drm_device *dev)
> else
> MISSING_CASE(INTEL_INFO(dev)->gen);
>
> - if (ppgtt) {
> - for_each_ring(ring, dev_priv, i) {
> - ret = ppgtt->switch_mm(ppgtt, ring);
> - if (ret != 0)
> - return ret;
> - }
> - }
> + return 0;
> +}
>
> - return ret;
> +int i915_ppgtt_init_ring(struct intel_engine_cs *ring)
> +{
> + struct drm_i915_private *dev_priv = ring->dev->dev_private;
> + struct i915_hw_ppgtt *ppgtt = dev_priv->mm.aliasing_ppgtt;
> +
> + if (!ppgtt)
> + return 0;
> +
> + return ppgtt->switch_mm(ppgtt, ring);
> }
This breaks alias PPGTT for execlists.
I915_ppgtt_init_hw() is a noop for execlists mode, but the new
i915_ppgtt_init_ring() will try to do a switch_mm() which should not be done
for execlists.
Thomas.
> +
> struct i915_hw_ppgtt *
> i915_ppgtt_create(struct drm_device *dev, struct drm_i915_file_private
> *fpriv)
> {
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h
> b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 8f76990..5a6cef9 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -300,6 +300,7 @@ void i915_global_gtt_cleanup(struct drm_device *dev);
>
> i
Re: [Intel-gfx] [PATCH v4] drm/i915: avoid processing spurious/shared interrupts in low-power states
On Tue, 24 Feb 2015, Imre Deak wrote:
> Atm, it's possible that the interrupt handler is called when the device
> is in D3 or some other low-power state. It can be due to another device
> that is still in D0 state and shares the interrupt line with i915, or on
> some platforms there could be spurious interrupts even without sharing
> the interrupt line. The latter case was reported by Klaus Ethgen using a
> Lenovo x61p machine (gen 4). He noticed this issue via a system
> suspend/resume hang and bisected it to the following commit:
>
> commit e11aa362308f5de467ce355a2a2471321b15a35c
> Author: Jesse Barnes
> Date: Wed Jun 18 09:52:55 2014 -0700
>
> drm/i915: use runtime irq suspend/resume in freeze/thaw
>
> This is a problem, since in low-power states IIR will always read
> 0x resulting in an endless IRQ servicing loop.
>
> Fix this by handling interrupts only when the driver explicitly enables
> them and so it's guaranteed that the interrupt registers return a valid
> value.
>
> Note that this issue existed even before the above commit, since during
> runtime suspend/resume we never unregistered the handler.
>
> v2:
> - clarify the purpose of smp_mb() vs. synchronize_irq() in the
> code comment (Chris)
>
> v3:
> - no need for an explicit smp_mb(), we can assume that synchronize_irq()
> and the mmio read/writes in the install hooks provide for this (Daniel)
> - remove code comment as the remaining synchronize_irq() is self
> explanatory (Daniel)
>
> v4:
> - drm_irq_uninstall() implies synchronize_irq(), so no need to call it
> explicitly (Daniel)
>
> Reference: https://lkml.org/lkml/2015/2/11/205
> Reported-and-bisected-by: Klaus Ethgen
> Signed-off-by: Imre Deak
> Reviewed-by: Daniel Vetter
Pushed to drm-intel-fixes, cc: stable, thanks for the patch and review.
BR,
Jani.
> ---
> drivers/gpu/drm/i915/i915_irq.c | 22 ++
> 1 file changed, 22 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 9073119..1561248 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1889,6 +1889,9 @@ static irqreturn_t valleyview_irq_handler(int irq, void
> *arg)
> u32 iir, gt_iir, pm_iir;
> irqreturn_t ret = IRQ_NONE;
>
> + if (!intel_irqs_enabled(dev_priv))
> + return IRQ_NONE;
> +
> while (true) {
> /* Find, clear, then process each source of interrupt */
>
> @@ -1933,6 +1936,9 @@ static irqreturn_t cherryview_irq_handler(int irq, void
> *arg)
> u32 master_ctl, iir;
> irqreturn_t ret = IRQ_NONE;
>
> + if (!intel_irqs_enabled(dev_priv))
> + return IRQ_NONE;
> +
> for (;;) {
> master_ctl = I915_READ(GEN8_MASTER_IRQ) &
> ~GEN8_MASTER_IRQ_CONTROL;
> iir = I915_READ(VLV_IIR);
> @@ -2205,6 +2211,9 @@ static irqreturn_t ironlake_irq_handler(int irq, void
> *arg)
> u32 de_iir, gt_iir, de_ier, sde_ier = 0;
> irqreturn_t ret = IRQ_NONE;
>
> + if (!intel_irqs_enabled(dev_priv))
> + return IRQ_NONE;
> +
> /* We get interrupts on unclaimed registers, so check for this before we
>* do any I915_{READ,WRITE}. */
> intel_uncore_check_errors(dev);
> @@ -2276,6 +2285,9 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
> enum pipe pipe;
> u32 aux_mask = GEN8_AUX_CHANNEL_A;
>
> + if (!intel_irqs_enabled(dev_priv))
> + return IRQ_NONE;
> +
> if (IS_GEN9(dev))
> aux_mask |= GEN9_AUX_CHANNEL_B | GEN9_AUX_CHANNEL_C |
> GEN9_AUX_CHANNEL_D;
> @@ -3768,6 +3780,9 @@ static irqreturn_t i8xx_irq_handler(int irq, void *arg)
> I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT |
> I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT;
>
> + if (!intel_irqs_enabled(dev_priv))
> + return IRQ_NONE;
> +
> iir = I915_READ16(IIR);
> if (iir == 0)
> return IRQ_NONE;
> @@ -3948,6 +3963,9 @@ static irqreturn_t i915_irq_handler(int irq, void *arg)
> I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT;
> int pipe, ret = IRQ_NONE;
>
> + if (!intel_irqs_enabled(dev_priv))
> + return IRQ_NONE;
> +
> iir = I915_READ(IIR);
> do {
> bool irq_received = (iir & ~flip_mask) != 0;
> @@ -4168,6 +4186,9 @@ static irqreturn_t i965_irq_handler(int irq, void *arg)
> I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT |
> I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT;
>
> + if (!intel_irqs_enabled(dev_priv))
> + return IRQ_NONE;
> +
> iir = I915_READ(IIR);
>
> for (;;) {
> @@ -4517,6 +4538,7 @@ void intel_runtime_pm_disable_interrupts(struct
> drm_i915_private *dev_priv)
> {
> dev_priv->dev->driver->irq_uninstall(dev_priv->dev);
> dev_priv->pm.irqs_enabled = false;
> + synchronize_irq(dev_priv->dev->irq);
> }
>
> /**
Re: [Intel-gfx] [PATCH] drm/i915: Check obj->vma_list under the struct_mutex
On Thu, 12 Feb 2015, Daniel Vetter wrote:
> On Thu, Feb 12, 2015 at 07:53:18AM +, Chris Wilson wrote:
>> When we walk the list of vma, or even for protecting against concurrent
>> framebuffer creation, we must hold the struct_mutex or else a second
>> thread can corrupt the list as we walk it.
>>
>> Fixes regression from
>> commit d7f46fc4e7323887494db13f063a8e59861fefb0
>> Author: Ben Widawsky
>> Date: Fri Dec 6 14:10:55 2013 -0800
>>
>> drm/i915: Make pin count per VMA
>>
>> References: https://bugs.freedesktop.org/show_bug.cgi?id=89085
>> Signed-off-by: Chris Wilson
>> ---
>> drivers/gpu/drm/i915/i915_gem_tiling.c | 7 ---
>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c
>> b/drivers/gpu/drm/i915/i915_gem_tiling.c
>> index 7a24bd1a51f6..6377b22269ad 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_tiling.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
>> @@ -335,9 +335,10 @@ i915_gem_set_tiling(struct drm_device *dev, void *data,
>> return -EINVAL;
>> }
>>
>> +mutex_lock(&dev->struct_mutex);
>> if (i915_gem_obj_is_pinned(obj) || obj->framebuffer_references) {
>
> Since the removal of userspace pinning we shouldn't be able to see pinned
> objects here which are _not_ framebuffers too. But we still need the lock
> for synchronization and to avoid races, but perhaps we could drop the list
> walk?
>
> Either way this is
>
> Reviewed-by: Daniel Vetter
> Cc: sta...@vger.kernel.org (we have some vague evidence that it blows up
> at last)
Pushed to drm-intel-fixes, thanks for the patch and review.
BR,
Jani.
>
> I've also audited all the other callers of is_pinned, the only other
> suspicious one is the one in capture_bo. Perhaps we should also move that
> over to obj->framebuffer_references?
>
> Cheers, Daniel
>> -drm_gem_object_unreference_unlocked(&obj->base);
>> -return -EBUSY;
>> +ret = -EBUSY;
>> +goto err;
>> }
>>
>> if (args->tiling_mode == I915_TILING_NONE) {
>> @@ -369,7 +370,6 @@ i915_gem_set_tiling(struct drm_device *dev, void *data,
>> }
>> }
>>
>> -mutex_lock(&dev->struct_mutex);
>> if (args->tiling_mode != obj->tiling_mode ||
>> args->stride != obj->stride) {
>> /* We need to rebind the object if its current allocation
>> @@ -424,6 +424,7 @@ i915_gem_set_tiling(struct drm_device *dev, void *data,
>> obj->bit_17 = NULL;
>> }
>>
>> +err:
>> drm_gem_object_unreference(&obj->base);
>> mutex_unlock(&dev->struct_mutex);
>>
>> --
>> 2.1.4
>>
>> ___
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v5 03/32] drm/i915: Create page table allocators
Michel Thierry writes:
> From: Ben Widawsky
>
> As we move toward dynamic page table allocation, it becomes much easier
> to manage our data structures if break do things less coarsely by
> breaking up all of our actions into individual tasks. This makes the
> code easier to write, read, and verify.
>
> Aside from the dissection of the allocation functions, the patch
> statically allocates the page table structures without a page directory.
> This remains the same for all platforms,
>
> The patch itself should not have much functional difference. The primary
> noticeable difference is the fact that page tables are no longer
> allocated, but rather statically declared as part of the page directory.
> This has non-zero overhead, but things gain non-trivial complexity as a
> result.
>
I don't quite understand the last sentence here. We gain overhead and
complexity.
s/non-trivial/trivial?
> This patch exists for a few reasons:
> 1. Splitting out the functions allows easily combining GEN6 and GEN8
> code. Page tables have no difference based on GEN8. As we'll see in a
> future patch when we add the DMA mappings to the allocations, it
> requires only one small change to make work, and error handling should
> just fall into place.
>
> 2. Unless we always want to allocate all page tables under a given PDE,
> we'll have to eventually break this up into an array of pointers (or
> pointer to pointer).
>
> 3. Having the discrete functions is easier to review, and understand.
> All allocations and frees now take place in just a couple of locations.
> Reviewing, and catching leaks should be easy.
>
> 4. Less important: the GFP flags are confined to one location, which
> makes playing around with such things trivial.
>
> v2: Updated commit message to explain why this patch exists
>
> v3: For lrc, s/pdp.page_directory[i].daddr/pdp.page_directory[i]->daddr/
>
> v4: Renamed free_pt/pd_single functions to unmap_and_free_pt/pd (Daniel)
>
> v5: Added additional safety checks in gen8 clear/free/unmap.
>
> v6: Use WARN_ON and return -EINVAL in alloc_pt_range (Mika).
>
> Cc: Mika Kuoppala
> Signed-off-by: Ben Widawsky
> Signed-off-by: Michel Thierry (v3+)
> ---
> drivers/gpu/drm/i915/i915_gem_gtt.c | 252
>
> drivers/gpu/drm/i915/i915_gem_gtt.h | 4 +-
> drivers/gpu/drm/i915/intel_lrc.c| 16 +--
> 3 files changed, 178 insertions(+), 94 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
> b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index eb0714c..65c77e5 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -279,6 +279,98 @@ static gen6_gtt_pte_t iris_pte_encode(dma_addr_t addr,
> return pte;
> }
>
> +static void unmap_and_free_pt(struct i915_page_table_entry *pt)
> +{
> + if (WARN_ON(!pt->page))
> + return;
> + __free_page(pt->page);
> + kfree(pt);
> +}
> +
> +static struct i915_page_table_entry *alloc_pt_single(void)
> +{
> + struct i915_page_table_entry *pt;
> +
> + pt = kzalloc(sizeof(*pt), GFP_KERNEL);
> + if (!pt)
> + return ERR_PTR(-ENOMEM);
> +
> + pt->page = alloc_page(GFP_KERNEL | __GFP_ZERO);
> + if (!pt->page) {
> + kfree(pt);
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + return pt;
> +}
> +
> +/**
> + * alloc_pt_range() - Allocate a multiple page tables
> + * @pd: The page directory which will have at least @count
> entries
> + * available to point to the allocated page tables.
> + * @pde: First page directory entry for which we are allocating.
> + * @count: Number of pages to allocate.
> + *
> + * Allocates multiple page table pages and sets the appropriate entries in
> the
> + * page table structure within the page directory. Function cleans up after
> + * itself on any failures.
> + *
> + * Return: 0 if allocation succeeded.
> + */
> +static int alloc_pt_range(struct i915_page_directory_entry *pd, uint16_t
> pde, size_t count)
> +{
> + int i, ret;
> +
> + /* 512 is the max page tables per page_directory on any platform. */
> + if (WARN_ON(pde + count > GEN6_PPGTT_PD_ENTRIES))
> + return -EINVAL;
> +
> + for (i = pde; i < pde + count; i++) {
> + struct i915_page_table_entry *pt = alloc_pt_single();
> +
> + if (IS_ERR(pt)) {
> + ret = PTR_ERR(pt);
> + goto err_out;
> + }
> + WARN(pd->page_tables[i],
> + "Leaking page directory entry %d (%pa)\n",
> + i, pd->page_tables[i]);
> + pd->page_tables[i] = pt;
> + }
> +
> + return 0;
> +
> +err_out:
> + while (i--)
> + unmap_and_free_pt(pd->page_tables[i]);
This is suspicious as it is non symmetrical of how we allocate. If the
plan is to free everything below pde, that should be mentioned in the
comment above.
On this patch we call this with pde == 0, but
Re: [Intel-gfx] [RFC 09/12] drm/i915/config: Add VBT settings configuration.
On Thu, Feb 12, 2015 at 03:41:35PM -0800, Bob Paauwe wrote:
> Add a new section with subsections to the ACPI configuration table
> that mimics much of the information typically stored in the VBT/option
> ROM. This allows for a way to override incorrect VBT data or to provide
> the configuration if VBT is not present. Lack of VBT is common in
> embedded systems.
>
> Any data found in the configuration tables will replace the driver's
> vbt structure.
>
> MIPI DSI configuration is not implmemnted at this time.
>
> Signed-off-by: Bob Paauwe
I'm not too happy with the duplicate of standards this creates. Is there
no way to just load a vbt blob into the acpi table somewhere and require
that it perfectly matches the vbt "standard" for the given platforms,
warts and all included?
As Jani points out we already have vbt headaches, it would be good if we
only have those once ;-)
-Daniel
> ---
> drivers/gpu/drm/i915/i915_dma.c | 2 +
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/intel_config.c | 334
>
> drivers/gpu/drm/i915/intel_drv.h| 4 +-
> 4 files changed, 340 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 9501360..9119e9b 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -852,6 +852,8 @@ int i915_driver_load(struct drm_device *dev, unsigned
> long flags)
> if (intel_vgpu_active(dev))
> I915_WRITE(vgtif_reg(display_ready), VGT_DRV_DISPLAY_READY);
>
> + intel_config_vbt(dev_priv);
> +
> i915_setup_sysfs(dev);
>
> if (INTEL_INFO(dev)->num_pipes) {
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1580702..a60511e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1657,6 +1657,7 @@ struct intel_config_info {
> struct list_head connector_list;
> struct list_head plane_list;
> struct list_head wa_list;
> + struct list_head vbt_list;
> };
>
>
> diff --git a/drivers/gpu/drm/i915/intel_config.c
> b/drivers/gpu/drm/i915/intel_config.c
> index ab14928..c1b06b7 100644
> --- a/drivers/gpu/drm/i915/intel_config.c
> +++ b/drivers/gpu/drm/i915/intel_config.c
> @@ -215,6 +215,7 @@ void intel_config_init(struct drm_device *dev)
> INIT_LIST_HEAD(&info->connector_list);
> INIT_LIST_HEAD(&info->plane_list);
> INIT_LIST_HEAD(&info->wa_list);
> + INIT_LIST_HEAD(&info->vbt_list);
>
> handle = ACPI_HANDLE(dev->dev);
> if (!handle) {
> @@ -241,6 +242,7 @@ void intel_config_init(struct drm_device *dev)
> #define i915_COMPONENT_CONNECTOR "CNCT"
> #define i915_COMPONENT_PLANE "PLNS"
> #define i915_COMPONENT_WORKAROUND "WRKS"
> +#define i915_COMPONENT_VBIOSTABLE "VBT"
>
> /* Build lists */
> list_for_each_entry(component, &info->base.adev->children, node) {
> @@ -267,6 +269,18 @@ void intel_config_init(struct drm_device *dev)
> } else if (strcmp(cname, i915_COMPONENT_WORKAROUND) == 0) {
> if (!alloc_new_node(cl, &info->wa_list))
> goto bail;
> + } else if (strcmp(cname, i915_COMPONENT_VBIOSTABLE) == 0) {
> + /* Add the main VBT node */
> + if (!alloc_new_node(cl, &info->vbt_list))
> + goto bail;
> +
> + /* This adds all the sub device nodes */
> + list_for_each_entry(cl, &component->children, node) {
> + if (!alloc_new_node(cl, &info->vbt_list))
> + goto bail;
> + }
> + } else {
> + DRM_DEBUG_DRIVER("i915/config: Unknown component :
> %s\n", cname);
> }
> }
>
> @@ -284,6 +298,8 @@ bail:
> kfree(new_node);
> list_for_each_entry_safe(new_node, tmp, &info->wa_list, node)
> kfree(new_node);
> + list_for_each_entry_safe(new_node, tmp, &info->vbt_list, node)
> + kfree(new_node);
>
> kfree(info);
> dev_priv->config_info = NULL;
> @@ -318,6 +334,8 @@ void intel_config_shutdown(struct drm_device *dev)
> kfree(n);
> list_for_each_entry_safe(n, tmp, &info->wa_list, node)
> kfree(n);
> + list_for_each_entry_safe(n, tmp, &info->vbt_list, node)
> + kfree(n);
>
> /* Unload any dynamically loaded ACPI property table */
> handle = ACPI_HANDLE(dev->dev);
> @@ -395,6 +413,8 @@ static struct list_head *cfg_type_to_list(struct
> intel_config_info *info,
> return &info->plane_list;
> case CFG_WORKAROUND:
> return &info->wa_list;
> + case CFG_VBT:
> + return &info->vbt_list;
> }
> return NULL;
> }
> @@ -433,6 +453,320 @@ bool intel_config_get_integer(struct drm
Re: [Intel-gfx] [PATCH v2 0/7] Added missing changes for Turbo feature on SKL
On Wed, Feb 18, 2015 at 07:31:13PM +0530, akash.g...@intel.com wrote: > From: Akash Goel > > This patch series add missing changes, required for proper functioning of the > Turbo feature on SKL. > Addressed review comments from Damien & Chris. Another item on the TODO list (for whoever is going to fix this), it seems it'd be advisable to change gen9_enable_rc6() to use the new GT_INTERVAL_FROM_US() macro. -- Damien ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/7] drm/i915/skl: Updated the gen6_set_rps function
On Wed, Feb 18, 2015 at 07:31:11PM +0530, akash.g...@intel.com wrote: > From: Akash Goel > > On SKL, the frequency programmed in RPNSWREQ (A008) register > has to be in units of 16.66 MHZ. So updated the gen6_set_rps > function, as per this change. > > Signed-off-by: Akash Goel > Reviewed-by: Lespiau, Damien Please don't use the Outlook way "lastname, firstname" here :) -- Damien ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 6/7] drm/i915/skl: Updated the 'i915_frequency_info' debugs function
On Wed, Feb 18, 2015 at 07:31:18PM +0530, akash.g...@intel.com wrote:
> From: Akash Goel
>
> Added support for SKL in the 'i915_frequency_info' debugfs function
>
> v2: Added missing conversion to 50MHZ for reqf & cagf (Damien)
>
> Signed-off-by: Akash Goel
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 25 +
> 1 file changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 9af17fb..5fb0121 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1089,7 +1089,7 @@ static int i915_frequency_info(struct seq_file *m, void
> *unused)
> seq_printf(m, "Current P-state: %d\n",
> (rgvstat & MEMSTAT_PSTATE_MASK) >>
> MEMSTAT_PSTATE_SHIFT);
> } else if (IS_GEN6(dev) || (IS_GEN7(dev) && !IS_VALLEYVIEW(dev)) ||
> -IS_BROADWELL(dev)) {
> +IS_BROADWELL(dev) || IS_GEN9(dev)) {
> u32 gt_perf_status = I915_READ(GEN6_GT_PERF_STATUS);
> u32 rp_state_limits = I915_READ(GEN6_RP_STATE_LIMITS);
> u32 rp_state_cap = I915_READ(GEN6_RP_STATE_CAP);
> @@ -1108,11 +1108,17 @@ static int i915_frequency_info(struct seq_file *m,
> void *unused)
> intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
>
> reqf = I915_READ(GEN6_RPNSWREQ);
> - reqf &= ~GEN6_TURBO_DISABLE;
> - if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> - reqf >>= 24;
> - else
> - reqf >>= 25;
> + if (IS_GEN9(dev)) {
> + reqf >>= 23;
> + /* Convert to 50 MHZ units from 16.667 MHZ */
> + reqf /= GEN9_FREQ_SCALER;
> + } else {
> + reqf &= ~GEN6_TURBO_DISABLE;
> + if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> + reqf >>= 24;
> + else
> + reqf >>= 25;
> + }
> reqf = intel_gpu_freq(dev_priv, reqf);
>
> rpmodectl = I915_READ(GEN6_RP_CONTROL);
> @@ -1128,7 +1134,10 @@ static int i915_frequency_info(struct seq_file *m,
> void *unused)
> rpprevdown = I915_READ(GEN6_RP_PREV_DOWN);
> if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> cagf = (rpstat & HSW_CAGF_MASK) >> HSW_CAGF_SHIFT;
> - else
> + else if (IS_GEN9(dev)) {
> + cagf = (rpstat & GEN9_CAGF_MASK) >> GEN9_CAGF_SHIFT;
> + cagf /= GEN9_FREQ_SCALER;
> + } else
> cagf = (rpstat & GEN6_CAGF_MASK) >> GEN6_CAGF_SHIFT;
I would put gen9 first, then hsw/bsw, then gen6.
Also there's now a gt_act_freq_mhz_show() that needs some love too.
> cagf = intel_gpu_freq(dev_priv, cagf);
>
> @@ -1152,7 +1161,7 @@ static int i915_frequency_info(struct seq_file *m, void
> *unused)
> pm_ier, pm_imr, pm_isr, pm_iir, pm_mask);
> seq_printf(m, "GT_PERF_STATUS: 0x%08x\n", gt_perf_status);
> seq_printf(m, "Render p-state ratio: %d\n",
> -(gt_perf_status & 0xff00) >> 8);
> +(gt_perf_status & (IS_GEN9(dev) ? 0x1ff00 : 0xff00))
> >> 8);
> seq_printf(m, "Render p-state VID: %d\n",
> gt_perf_status & 0xff);
> seq_printf(m, "Render p-state limit: %d\n",
> --
> 1.9.2
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: fix failure to power off after hibernate
On ti, 2015-02-24 at 15:58 +0100, Bjørn Mork wrote:
> This fixes a bug where hibernation completes, but the system
> fails to power off after the image has been saved.
>
> Bisection lead to commit da2bc1b9db33 ("drm/i915: add poweroff_late
> handler") which added a .poweroff_late hook pointing to the same
> function as the .freeze_late hook, without any justification or
> explanation why this would be correct, except "for consistency".
>
> The assumption that this "makes the power off handling identical to
> the S3 suspend and S4 freeze handling" is completely bogus. As clearly
> documented in Documentation/power/devices.txt, the poweroff* hooks
> are part of the hibernation API along with the freeze* hooks. The
> phases involved in hibernation are:
>
>prepare, freeze, freeze_late, freeze_noirq, thaw_noirq, thaw_early,
>thaw, complete, prepare, poweroff, poweroff_late, poweroff_noirq
>
> With the above sequence in mind, it should be fairly obvious that you
> cannot save registers and disable the device in both the freeze_late
> and poweroff_late phases. Or as Documentation/power/devices.txt
> explains it:
>
> The poweroff, poweroff_late and poweroff_noirq callbacks should do
> essentially
> the same things as the suspend, suspend_late and suspend_noirq callbacks,
> respectively. The only notable difference is that they need not store the
> device register values, because the registers should already have been stored
> during the freeze, freeze_late or freeze_noirq phases.
>
> The "consistency" between suspend and hibernate was already in
> place, with freeze_late pointing to the same function as suspend_late.
> There is no need for any poweroff_late hook in this driver.
The poweroff handlers undo the actions of the thaw handlers. As the
original commit stated saving the registers is not needed there, but
it's also not a big overhead and there should be no problem doing it. We
are planning to optimize the hibernation sequence by for example not
shutting down the display between freeze and thaw, and also getting rid
of unnecessary steps at the power off phase. But before that we want to
actually unify things rather than having special cases, as maintaining
the special code paths caused already quite a lot of problems for us so
far.
Reverting the commit may hide some other issue, so before doing that
could you try the following patch:
http://lists.freedesktop.org/archives/intel-gfx/2015-February/060529.html
If with that you still get the hang could you try on top of that the
patch below, first having only pci_set_power_state uncommented, then
both pci_set_power_state and pci_disable_device uncommented?
Thanks,
Imre
diff --git a/drivers/gpu/drm/i915/i915_drv.c
b/drivers/gpu/drm/i915/i915_drv.c
index 4badb23..dc91d4b 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -968,6 +968,23 @@ static int i915_pm_suspend_late(struct device *dev)
return i915_drm_suspend_late(drm_dev);
}
+static int i915_pm_poweroff_late(struct device *dev)
+{
+ struct drm_device *drm_dev = dev_to_i915(dev)->dev;
+ struct drm_i915_private *dev_priv = drm_dev->dev_private;
+ int ret;
+
+ ret = intel_suspend_complete(dev_priv);
+
+ if (ret)
+ return ret;
+
+ pci_disable_device(drm_dev->pdev);
+// pci_set_power_state(drm_dev->pdev, PCI_D3hot);
+
+ return 0;
+}
+
static int i915_pm_resume_early(struct device *dev)
{
struct drm_device *drm_dev = dev_to_i915(dev)->dev;
@@ -1535,7 +1552,7 @@ static const struct dev_pm_ops i915_pm_ops = {
.thaw_early = i915_pm_resume_early,
.thaw = i915_pm_resume,
.poweroff = i915_pm_suspend,
- .poweroff_late = i915_pm_suspend_late,
+ .poweroff_late = i915_pm_poweroff_late,
.restore_early = i915_pm_resume_early,
.restore = i915_pm_resume,
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 01/12] drm/i915/config: Initial framework
On Thu, Feb 12, 2015 at 03:41:27PM -0800, Bob Paauwe wrote:
> This adds an init-time configuration framework that parses configuration
> data from an ACPI property table. The table is assumed to have well
> defined sub-device property tables that correspond to the various
> driver components. Initially the following sub-device tables are
> defined:
>
> CRTC (CRTC)
>The CRTC sub-device table contains additional sub-device tables
>where each one corresponds to a CRTC. Each CRTC sub-device must
>include a property called "id" whose value matches the driver's
>crtc id. Additional properties for the CRTC are used to configure
>the crtc.
>
> Connector (CNCT)
>The CNCT sub-device table contains additional sub-device tables
>where each one corresponds to a connector. Each of the connector
>sub-device tables must include a property called "name" whose value
>matches a connector name assigned by the driver (see later patch
>for output name function). Additional connector properties can
>be set through these tables.
>
> Plane (PLNS)
>The PLNS sub-device table contains additional sub-device tables
>where each one corresponds to a plane. [this needs additional work]
>
> In addition, the main device property table for the device may
> contain configuration information that applies to general driver
> configuration.
>
> The framework includes a couple of helper functions to access the
> configuration data.
>
>intel_config_get_integer() will look up a configuration property
>and return the integer value associated with it.
>
>intel_config_init__property() will look up a
>configuration property and assign the value to a drm
>property of the same name. These functions are used to
>initialize drm property instances to specific values.
>
> Signed-off-by: Bob Paauwe
Like Jani I'm mostly concerned about maintaining yet another slightly
different definition of paramters and settings here - we already have (or
will soon grow) per crtc/plane/connector properties for atomic.
My high-level idea for this was that we reuse the atomic support (once we
have it) and just take the acpi tables as a slightly different source for
an atomic update. Since the design of a name/value pair for objects is
very similar to what we do with the atomic ioctl this hopefully should map
fairly well.
Internally in the driver the only work needed to do would be to add any
missing properties. All the acpi atomic update code should be fully i915
agnostic and only use generic atomic update interfaces.
Would this work or do I overlook something important here.
-Daniel
> ---
> drivers/gpu/drm/i915/Makefile | 3 +-
> drivers/gpu/drm/i915/i915_dma.c | 4 +
> drivers/gpu/drm/i915/i915_drv.h | 16 ++
> drivers/gpu/drm/i915/i915_params.c | 6 +
> drivers/gpu/drm/i915/intel_config.c | 542
>
> drivers/gpu/drm/i915/intel_drv.h| 28 ++
> 6 files changed, 598 insertions(+), 1 deletion(-)
> create mode 100644 drivers/gpu/drm/i915/intel_config.c
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index f025e7f..462de19 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -12,7 +12,8 @@ i915-y := i915_drv.o \
>i915_suspend.o \
> i915_sysfs.o \
> intel_pm.o \
> - intel_runtime_pm.o
> + intel_runtime_pm.o \
> + intel_config.o
>
> i915-$(CONFIG_COMPAT) += i915_ioc32.o
> i915-$(CONFIG_DEBUG_FS) += i915_debugfs.o
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 5804aa5..9501360 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -656,6 +656,8 @@ int i915_driver_load(struct drm_device *dev, unsigned
> long flags)
> dev->dev_private = dev_priv;
> dev_priv->dev = dev;
>
> + intel_config_init(dev);
> +
> /* Setup the write-once "constant" device info */
> device_info = (struct intel_device_info *)&dev_priv->info;
> memcpy(device_info, info, sizeof(dev_priv->info));
> @@ -929,6 +931,8 @@ int i915_driver_unload(struct drm_device *dev)
>
> acpi_video_unregister();
>
> + intel_config_shutdown(dev);
> +
> if (drm_core_check_feature(dev, DRIVER_MODESET))
> intel_fbdev_fini(dev);
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2dedd43..165091c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1645,6 +1645,20 @@ struct i915_virtual_gpu {
> bool active;
> };
>
> +struct intel_config_node {
> + struct acpi_device *adev;
> + struct list_head node;
> + struct list_head list;
> +};
> +
> +struct intel_config_info {
> + struct intel_config_node base;
> + struct list_head crtc_list;
> + struct list_head connector_list;
> + struct list_head plane_list;
>
[Intel-gfx] [PATCH v6 09/32] drm/i915: Finish gen6/7 dynamic page table allocation
From: Ben Widawsky
This patch continues on the idea from the previous patch. From here on,
in the steady state, PDEs are all pointing to the scratch page table (as
recommended in the spec). When an object is allocated in the VA range,
the code will determine if we need to allocate a page for the page
table. Similarly when the object is destroyed, we will remove, and free
the page table pointing the PDE back to the scratch page.
Following patches will work to unify the code a bit as we bring in GEN8
support. GEN6 and GEN8 are different enough that I had a hard time to
get to this point with as much common code as I do.
The aliasing PPGTT must pre-allocate all of the page tables. There are a
few reasons for this. Two trivial ones: aliasing ppgtt goes through the
ggtt paths, so it's hard to maintain, we currently do not restore the
default context (assuming the previous force reload is indeed
necessary). Most importantly though, the only way (it seems from
empirical evidence) to invalidate the CS TLBs on non-render ring is to
either use ring sync (which requires actually stopping the rings in
order to synchronize when the sync completes vs. where you are in
execution), or to reload DCLV. Since without full PPGTT we do not ever
reload the DCLV register, there is no good way to achieve this. The
simplest solution is just to not support dynamic page table
creation/destruction in the aliasing PPGTT.
We could always reload DCLV, but this seems like quite a bit of excess
overhead only to save at most 2MB-4k of memory for the aliasing PPGTT
page tables.
v2: Make the page table bitmap declared inside the function (Chris)
Simplify the way scratching address space works.
Move the alloc/teardown tracepoints up a level in the call stack so that
both all implementations get the trace.
v3: Updated trace event to spit out a name
v4: Aliasing ppgtt is now initialized differently (in setup global gtt)
v5: Rebase to latest code. Also removed unnecessary aliasing ppgtt check
for trace, as it is no longer possible after the PPGTT cleanup patch series
of a couple of months ago (Daniel).
v6: Implement changes from code review (Daniel):
- allocate/teardown_va_range calls added.
- Add a scratch page allocation helper (only need the address).
- Move trace events to a new patch.
- Use updated mark_tlbs_dirty.
- Moved pt preallocation for aliasing ppgtt into gen6_ppgtt_init.
v7: teardown_va_range removed (Daniel).
In init, gen6_ppgtt_clear_range call is only needed for aliasing ppgtt.
v8: Rebase after s/page_tables/page_table/.
Cc: Daniel Vetter
Signed-off-by: Ben Widawsky
Signed-off-by: Michel Thierry (v4+)
---
drivers/gpu/drm/i915/i915_debugfs.c | 3 +-
drivers/gpu/drm/i915/i915_gem.c | 9 +++
drivers/gpu/drm/i915/i915_gem_gtt.c | 125 +++-
drivers/gpu/drm/i915/i915_gem_gtt.h | 3 +
4 files changed, 123 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index 4d07030..e8ad450 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2181,6 +2181,8 @@ static void gen6_ppgtt_info(struct seq_file *m, struct
drm_device *dev)
seq_printf(m, "PP_DIR_BASE_READ: 0x%08x\n",
I915_READ(RING_PP_DIR_BASE_READ(ring)));
seq_printf(m, "PP_DIR_DCLV: 0x%08x\n",
I915_READ(RING_PP_DIR_DCLV(ring)));
}
+ seq_printf(m, "ECOCHK: 0x%08x\n\n", I915_READ(GAM_ECOCHK));
+
if (dev_priv->mm.aliasing_ppgtt) {
struct i915_hw_ppgtt *ppgtt = dev_priv->mm.aliasing_ppgtt;
@@ -2197,7 +2199,6 @@ static void gen6_ppgtt_info(struct seq_file *m, struct
drm_device *dev)
get_pid_task(file->pid, PIDTYPE_PID)->comm);
idr_for_each(&file_priv->context_idr, per_file_ctx, m);
}
- seq_printf(m, "ECOCHK: 0x%08x\n", I915_READ(GAM_ECOCHK));
}
static int i915_ppgtt_info(struct seq_file *m, void *data)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 61134ab..312b7d2 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3599,6 +3599,15 @@ search_free:
if (ret)
goto err_remove_node;
+ /* allocate before insert / bind */
+ if (vma->vm->allocate_va_range) {
+ ret = vma->vm->allocate_va_range(vma->vm,
+ vma->node.start,
+ vma->node.size);
+ if (ret)
+ goto err_remove_node;
+ }
+
trace_i915_vma_bind(vma, flags);
ret = i915_vma_bind(vma, obj->cache_level,
flags & PIN_GLOBAL ? GLOBAL_BIND : 0);
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index bd8e876..29cda58 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@
[Intel-gfx] [PATCH v6 07/32] drm/i915: Track page table reload need
From: Ben Widawsky
This patch was formerly known as, "Force pd restore when PDEs change,
gen6-7." I had to change the name because it is needed for GEN8 too.
The real issue this is trying to solve is when a new object is mapped
into the current address space. The GPU does not snoop the new mapping
so we must do the gen specific action to reload the page tables.
GEN8 and GEN7 do differ in the way they load page tables for the RCS.
GEN8 does so with the context restore, while GEN7 requires the proper
load commands in the command streamer. Non-render is similar for both.
Caveat for GEN7
The docs say you cannot change the PDEs of a currently running context.
We never map new PDEs of a running context, and expect them to be
present - so I think this is okay. (We can unmap, but this should also
be okay since we only unmap unreferenced objects that the GPU shouldn't
be tryingto va->pa xlate.) The MI_SET_CONTEXT command does have a flag
to signal that even if the context is the same, force a reload. It's
unclear exactly what this does, but I have a hunch it's the right thing
to do.
The logic assumes that we always emit a context switch after mapping new
PDEs, and before we submit a batch. This is the case today, and has been
the case since the inception of hardware contexts. A note in the comment
let's the user know.
It's not just for gen8. If the current context has mappings change, we
need a context reload to switch
v2: Rebased after ppgtt clean up patches. Split the warning for aliasing
and true ppgtt options. And do not break aliasing ppgtt, where to->ppgtt
is always null.
v3: Invalidate PPGTT TLBs inside alloc_va_range.
v4: Rename ppgtt_invalidate_tlbs to mark_tlbs_dirty and move
pd_dirty_rings from i915_address_space to i915_hw_ppgtt. Fixes when
neither ctx->ppgtt and aliasing_ppgtt exist.
v5: Removed references to teardown_va_range.
Signed-off-by: Ben Widawsky
Signed-off-by: Michel Thierry (v2+)
---
drivers/gpu/drm/i915/i915_gem_context.c| 29 -
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 11 +++
drivers/gpu/drm/i915/i915_gem_gtt.c| 11 +++
drivers/gpu/drm/i915/i915_gem_gtt.h| 1 +
4 files changed, 47 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
b/drivers/gpu/drm/i915/i915_gem_context.c
index 6206d27..437cdcc 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -569,8 +569,20 @@ static inline bool should_skip_switch(struct
intel_engine_cs *ring,
struct intel_context *from,
struct intel_context *to)
{
- if (from == to && !to->remap_slice)
- return true;
+ struct drm_i915_private *dev_priv = ring->dev->dev_private;
+
+ if (to->remap_slice)
+ return false;
+
+ if (to->ppgtt) {
+ if (from == to && !test_bit(ring->id,
+ &to->ppgtt->pd_dirty_rings))
+ return true;
+ } else if (dev_priv->mm.aliasing_ppgtt) {
+ if (from == to && !test_bit(ring->id,
+ &dev_priv->mm.aliasing_ppgtt->pd_dirty_rings))
+ return true;
+ }
return false;
}
@@ -587,9 +599,8 @@ needs_pd_load_pre(struct intel_engine_cs *ring, struct
intel_context *to)
static bool
needs_pd_load_post(struct intel_engine_cs *ring, struct intel_context *to)
{
- return (!to->legacy_hw_ctx.initialized ||
- i915_gem_context_is_default(to)) &&
- to->ppgtt && IS_GEN8(ring->dev);
+ return IS_GEN8(ring->dev) &&
+ (to->ppgtt || &to->ppgtt->pd_dirty_rings);
}
static int do_switch(struct intel_engine_cs *ring,
@@ -634,6 +645,12 @@ static int do_switch(struct intel_engine_cs *ring,
ret = to->ppgtt->switch_mm(to->ppgtt, ring);
if (ret)
goto unpin_out;
+
+ /* Doing a PD load always reloads the page dirs */
+ if (to->ppgtt)
+ clear_bit(ring->id, &to->ppgtt->pd_dirty_rings);
+ else
+ clear_bit(ring->id,
&dev_priv->mm.aliasing_ppgtt->pd_dirty_rings);
}
if (ring != &dev_priv->ring[RCS]) {
@@ -672,6 +689,8 @@ static int do_switch(struct intel_engine_cs *ring,
*/
if (!to->legacy_hw_ctx.initialized || i915_gem_context_is_default(to))
hw_flags |= MI_RESTORE_INHIBIT;
+ else if (to->ppgtt && test_and_clear_bit(ring->id,
&to->ppgtt->pd_dirty_rings))
+ hw_flags |= MI_FORCE_RESTORE;
ret = mi_set_context(ring, to, hw_flags);
if (ret)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 82636aa..24757ee 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/dr
[Intel-gfx] [PATCH v6 05/32] drm/i915: Track GEN6 page table usage
From: Ben Widawsky
Instead of implementing the full tracking + dynamic allocation, this
patch does a bit less than half of the work, by tracking and warning on
unexpected conditions. The tracking itself follows which PTEs within a
page table are currently being used for objects. The next patch will
modify this to actually allocate the page tables only when necessary.
With the current patch there isn't much in the way of making a gen
agnostic range allocation function. However, in the next patch we'll add
more specificity which makes having separate functions a bit easier to
manage.
One important change introduced here is that DMA mappings are
created/destroyed at the same page directories/tables are
allocated/deallocated.
Notice that aliasing PPGTT is not managed here. The patch which actually
begins dynamic allocation/teardown explains the reasoning for this.
v2: s/pdp.page_directory/pdp.page_directorys
Make a scratch page allocation helper
v3: Rebase and expand commit message.
v4: Allocate required pagetables only when it is needed, _bind_to_vm
instead of bind_vma (Daniel).
v5: Rebased to remove the unnecessary noise in the diff, also:
- PDE mask is GEN agnostic, renamed GEN6_PDE_MASK to I915_PDE_MASK.
- Removed unnecessary checks in gen6_alloc_va_range.
- Changed map/unmap_px_single macros to use dma functions directly and
be part of a static inline function instead.
- Moved drm_device plumbing through page tables operation to its own
patch.
- Moved allocate/teardown_va_range calls until they are fully
implemented (in subsequent patch).
- Merged pt and scratch_pt unmap_and_free path.
- Moved scratch page allocator helper to the patch that will use it.
v6: Reduce complexity by not tearing down pagetables dynamically, the
same can be achieved while freeing empty vms. (Daniel)
v7: s/i915_dma_map_px_single/i915_dma_map_single
s/gen6_write_pdes/gen6_write_pde
Prevent a NULL case when only GGTT is available. (Mika)
v8: Rebased after s/page_tables/page_table/.
Cc: Daniel Vetter
Cc: Mika Kuoppala
Signed-off-by: Ben Widawsky
Signed-off-by: Michel Thierry (v3+)
---
drivers/gpu/drm/i915/i915_gem_gtt.c | 198 +---
drivers/gpu/drm/i915/i915_gem_gtt.h | 75 ++
2 files changed, 211 insertions(+), 62 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index e05488e..f9354c7 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -278,29 +278,88 @@ static gen6_gtt_pte_t iris_pte_encode(dma_addr_t addr,
return pte;
}
-static void unmap_and_free_pt(struct i915_page_table_entry *pt, struct
drm_device *dev)
+#define i915_dma_unmap_single(px, dev) \
+ __i915_dma_unmap_single((px)->daddr, dev)
+
+static inline void __i915_dma_unmap_single(dma_addr_t daddr,
+ struct drm_device *dev)
+{
+ struct device *device = &dev->pdev->dev;
+
+ dma_unmap_page(device, daddr, 4096, PCI_DMA_BIDIRECTIONAL);
+}
+
+/**
+ * i915_dma_map_single() - Create a dma mapping for a page table/dir/etc.
+ * @px:Page table/dir/etc to get a DMA map for
+ * @dev: drm device
+ *
+ * Page table allocations are unified across all gens. They always require a
+ * single 4k allocation, as well as a DMA mapping. If we keep the structs
+ * symmetric here, the simple macro covers us for every page table type.
+ *
+ * Return: 0 if success.
+ */
+#define i915_dma_map_single(px, dev) \
+ i915_dma_map_page_single((px)->page, (dev), &(px)->daddr)
+
+static inline int i915_dma_map_page_single(struct page *page,
+ struct drm_device *dev,
+ dma_addr_t *daddr)
+{
+ struct device *device = &dev->pdev->dev;
+
+ *daddr = dma_map_page(device, page, 0, 4096, PCI_DMA_BIDIRECTIONAL);
+ return dma_mapping_error(device, *daddr);
+}
+
+static void unmap_and_free_pt(struct i915_page_table_entry *pt,
+ struct drm_device *dev)
{
if (WARN_ON(!pt->page))
return;
+
+ i915_dma_unmap_single(pt, dev);
__free_page(pt->page);
+ kfree(pt->used_ptes);
kfree(pt);
}
static struct i915_page_table_entry *alloc_pt_single(struct drm_device *dev)
{
struct i915_page_table_entry *pt;
+ const size_t count = INTEL_INFO(dev)->gen >= 8 ?
+ GEN8_PTES_PER_PAGE : I915_PPGTT_PT_ENTRIES;
+ int ret = -ENOMEM;
pt = kzalloc(sizeof(*pt), GFP_KERNEL);
if (!pt)
return ERR_PTR(-ENOMEM);
+ pt->used_ptes = kcalloc(BITS_TO_LONGS(count), sizeof(*pt->used_ptes),
+ GFP_KERNEL);
+
+ if (!pt->used_ptes)
+ goto fail_bitmap;
+
pt->page = alloc_page(GFP_KERNEL | __GFP_ZERO);
- if (!pt->page) {
- kfree(pt);
- return ERR_PTR(-ENOMEM);
-
[Intel-gfx] [PATCH v6 08/32] drm/i915: Initialize all contexts
From: Ben Widawsky
The problem is we're going to switch to a new context, which could be
the default context. The plan was to use restore inhibit, which would be
fine, except if we are using dynamic page tables (which we will). If we
use dynamic page tables and we don't load new page tables, the previous
page tables might go away, and future operations will fault.
CTXA runs.
switch to default, restore inhibit
CTXA dies and has its address space taken away.
Run CTXB, tries to save using the context A's address space - this
fails.
The general solution is to make sure every context has it's own state,
and its own address space. For cases when we must restore inhibit, first
thing we do is load a valid address space. I thought this would be
enough, but apparently there are references within the context itself
which will refer to the old address space - therefore, we also must
reinitialize.
It was tricky to track this down as we don't have much insight into what
happens in a context save.
This is required for the next patch which enables dynamic page tables.
v2: to->ppgtt is only valid in full ppgtt.
Signed-off-by: Ben Widawsky
Signed-off-by: Michel Thierry (v2)
---
drivers/gpu/drm/i915/i915_gem_context.c | 25 +++--
1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
b/drivers/gpu/drm/i915/i915_gem_context.c
index 437cdcc..6a583c3 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -596,13 +596,6 @@ needs_pd_load_pre(struct intel_engine_cs *ring, struct
intel_context *to)
(ring != &dev_priv->ring[RCS])) && to->ppgtt;
}
-static bool
-needs_pd_load_post(struct intel_engine_cs *ring, struct intel_context *to)
-{
- return IS_GEN8(ring->dev) &&
- (to->ppgtt || &to->ppgtt->pd_dirty_rings);
-}
-
static int do_switch(struct intel_engine_cs *ring,
struct intel_context *to)
{
@@ -683,20 +676,24 @@ static int do_switch(struct intel_engine_cs *ring,
/* GEN8 does *not* require an explicit reload if the PDPs have been
* setup, and we do not wish to move them.
-*
-* XXX: If we implemented page directory eviction code, this
-* optimization needs to be removed.
*/
- if (!to->legacy_hw_ctx.initialized || i915_gem_context_is_default(to))
+ if (!to->legacy_hw_ctx.initialized) {
hw_flags |= MI_RESTORE_INHIBIT;
- else if (to->ppgtt && test_and_clear_bit(ring->id,
&to->ppgtt->pd_dirty_rings))
+ /* NB: If we inhibit the restore, the context is not allowed to
+* die because future work may end up depending on valid address
+* space. This means we must enforce that a page table load
+* occur when this occurs. */
+ } else if (to->ppgtt && test_and_clear_bit(ring->id,
&to->ppgtt->pd_dirty_rings))
hw_flags |= MI_FORCE_RESTORE;
ret = mi_set_context(ring, to, hw_flags);
if (ret)
goto unpin_out;
- if (needs_pd_load_post(ring, to)) {
+ if (IS_GEN8(ring->dev) && to->ppgtt && (hw_flags & MI_RESTORE_INHIBIT))
{
+ /* We have a valid page directory (scratch) to switch to. This
+* allows the old VM to be freed. Note that if anything occurs
+* between the set context, and here, we are f*cked */
ret = to->ppgtt->switch_mm(to->ppgtt, ring);
/* The hardware context switch is emitted, but we haven't
* actually changed the state - so it's probably safe to bail
@@ -746,7 +743,7 @@ static int do_switch(struct intel_engine_cs *ring,
i915_gem_context_unreference(from);
}
- uninitialized = !to->legacy_hw_ctx.initialized && from == NULL;
+ uninitialized = !to->legacy_hw_ctx.initialized;
to->legacy_hw_ctx.initialized = true;
done:
--
2.1.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v6 24/32] drm/i915/bdw: Add ppgtt info for dynamic pages
From: Ben Widawsky
Note that there is no gen8 ppgtt debug_dump function yet.
Signed-off-by: Ben Widawsky
Signed-off-by: Michel Thierry
---
drivers/gpu/drm/i915/i915_debugfs.c | 19 ++-
drivers/gpu/drm/i915/i915_gem_gtt.c | 32
drivers/gpu/drm/i915/i915_gem_gtt.h | 9 +
3 files changed, 51 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index e85da9d..c877957 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2165,7 +2165,6 @@ static void gen6_ppgtt_info(struct seq_file *m, struct
drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
struct intel_engine_cs *ring;
- struct drm_file *file;
int i;
if (INTEL_INFO(dev)->gen == 6)
@@ -2189,14 +2188,6 @@ static void gen6_ppgtt_info(struct seq_file *m, struct
drm_device *dev)
ppgtt->debug_dump(ppgtt, m);
}
-
- list_for_each_entry_reverse(file, &dev->filelist, lhead) {
- struct drm_i915_file_private *file_priv = file->driver_priv;
-
- seq_printf(m, "proc: %s\n",
- get_pid_task(file->pid, PIDTYPE_PID)->comm);
- idr_for_each(&file_priv->context_idr, per_file_ctx, m);
- }
}
static int i915_ppgtt_info(struct seq_file *m, void *data)
@@ -2204,6 +2195,7 @@ static int i915_ppgtt_info(struct seq_file *m, void *data)
struct drm_info_node *node = m->private;
struct drm_device *dev = node->minor->dev;
struct drm_i915_private *dev_priv = dev->dev_private;
+ struct drm_file *file;
int ret = mutex_lock_interruptible(&dev->struct_mutex);
if (ret)
@@ -2215,6 +2207,15 @@ static int i915_ppgtt_info(struct seq_file *m, void
*data)
else if (INTEL_INFO(dev)->gen >= 6)
gen6_ppgtt_info(m, dev);
+ list_for_each_entry_reverse(file, &dev->filelist, lhead) {
+ struct drm_i915_file_private *file_priv = file->driver_priv;
+
+ seq_printf(m, "\nproc: %s\n",
+ get_pid_task(file->pid, PIDTYPE_PID)->comm);
+ idr_for_each(&file_priv->context_idr, per_file_ctx,
+(void *)(unsigned long)m);
+ }
+
intel_runtime_pm_put(dev_priv);
mutex_unlock(&dev->struct_mutex);
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index f613377..9f16db7 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2128,6 +2128,38 @@ static void gen8_ggtt_clear_range(struct
i915_address_space *vm,
readl(gtt_base);
}
+void gen8_for_every_pdpe_pde(struct i915_hw_ppgtt *ppgtt,
+void (*callback)(struct
i915_page_directory_pointer_entry *pdp,
+ struct i915_page_directory_entry
*pd,
+ struct i915_page_table_entry *pt,
+ unsigned pdpe,
+ unsigned pde,
+ void *data),
+void *data)
+{
+ uint64_t start = ppgtt->base.start;
+ uint64_t length = ppgtt->base.total;
+ uint64_t pdpe, pde, temp;
+
+ struct i915_page_directory_entry *pd;
+ struct i915_page_table_entry *pt;
+
+ gen8_for_each_pdpe(pd, &ppgtt->pdp, start, length, temp, pdpe) {
+ uint64_t pd_start = start, pd_length = length;
+ int i;
+
+ if (pd == NULL) {
+ for (i = 0; i < GEN8_PDES_PER_PAGE; i++)
+ callback(&ppgtt->pdp, NULL, NULL, pdpe, i,
data);
+ continue;
+ }
+
+ gen8_for_each_pde(pt, pd, pd_start, pd_length, temp, pde) {
+ callback(&ppgtt->pdp, pd, pt, pdpe, pde, data);
+ }
+ }
+}
+
static void gen6_ggtt_clear_range(struct i915_address_space *vm,
uint64_t start,
uint64_t length,
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h
b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 1004e0f..f74afa6 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -483,6 +483,15 @@ static inline size_t gen8_pde_count(uint64_t addr,
uint64_t length)
return i915_pde_index(end, GEN8_PDE_SHIFT) - i915_pde_index(addr,
GEN8_PDE_SHIFT);
}
+void gen8_for_every_pdpe_pde(struct i915_hw_ppgtt *ppgtt,
+void (*callback)(struct
i915_page_directory_pointer_entry *pdp,
+ struct i915_page_directory_entry
*pd,
+ struct i915_page_table_entry *pt,
+
[Intel-gfx] [PATCH v6 06/32] drm/i915: Extract context switch skip and pd load logic
From: Ben Widawsky
We have some fanciness coming up. This patch just breaks out the logic
of context switch skip, pd load pre, and pd load post.
v2: Use new functions to replace the logic right away (Daniel)
Cc: Daniel Vetter
Signed-off-by: Ben Widawsky
Signed-off-by: Michel Thierry (v2)
---
drivers/gpu/drm/i915/i915_gem_context.c | 40 +
1 file changed, 31 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
b/drivers/gpu/drm/i915/i915_gem_context.c
index 755b415..6206d27 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -565,6 +565,33 @@ mi_set_context(struct intel_engine_cs *ring,
return ret;
}
+static inline bool should_skip_switch(struct intel_engine_cs *ring,
+ struct intel_context *from,
+ struct intel_context *to)
+{
+ if (from == to && !to->remap_slice)
+ return true;
+
+ return false;
+}
+
+static bool
+needs_pd_load_pre(struct intel_engine_cs *ring, struct intel_context *to)
+{
+ struct drm_i915_private *dev_priv = ring->dev->dev_private;
+
+ return ((INTEL_INFO(ring->dev)->gen < 8) ||
+ (ring != &dev_priv->ring[RCS])) && to->ppgtt;
+}
+
+static bool
+needs_pd_load_post(struct intel_engine_cs *ring, struct intel_context *to)
+{
+ return (!to->legacy_hw_ctx.initialized ||
+ i915_gem_context_is_default(to)) &&
+ to->ppgtt && IS_GEN8(ring->dev);
+}
+
static int do_switch(struct intel_engine_cs *ring,
struct intel_context *to)
{
@@ -573,9 +600,6 @@ static int do_switch(struct intel_engine_cs *ring,
u32 hw_flags = 0;
bool uninitialized = false;
struct i915_vma *vma;
- bool needs_pd_load_pre = ((INTEL_INFO(ring->dev)->gen < 8) ||
- (ring != &dev_priv->ring[RCS])) && to->ppgtt;
- bool needs_pd_load_post = false;
int ret, i;
if (from != NULL && ring == &dev_priv->ring[RCS]) {
@@ -583,7 +607,7 @@ static int do_switch(struct intel_engine_cs *ring,
BUG_ON(!i915_gem_obj_is_pinned(from->legacy_hw_ctx.rcs_state));
}
- if (from == to && !to->remap_slice)
+ if (should_skip_switch(ring, from, to))
return 0;
/* Trying to pin first makes error handling easier. */
@@ -601,7 +625,7 @@ static int do_switch(struct intel_engine_cs *ring,
*/
from = ring->last_context;
- if (needs_pd_load_pre) {
+ if (needs_pd_load_pre(ring, to)) {
/* Older GENs and non render rings still want the load first,
* "PP_DCLV followed by PP_DIR_BASE register through Load
* Register Immediate commands in Ring Buffer before submitting
@@ -646,16 +670,14 @@ static int do_switch(struct intel_engine_cs *ring,
* XXX: If we implemented page directory eviction code, this
* optimization needs to be removed.
*/
- if (!to->legacy_hw_ctx.initialized || i915_gem_context_is_default(to)) {
+ if (!to->legacy_hw_ctx.initialized || i915_gem_context_is_default(to))
hw_flags |= MI_RESTORE_INHIBIT;
- needs_pd_load_post = to->ppgtt && IS_GEN8(ring->dev);
- }
ret = mi_set_context(ring, to, hw_flags);
if (ret)
goto unpin_out;
- if (needs_pd_load_post) {
+ if (needs_pd_load_post(ring, to)) {
ret = to->ppgtt->switch_mm(to->ppgtt, ring);
/* The hardware context switch is emitted, but we haven't
* actually changed the state - so it's probably safe to bail
--
2.1.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v6 22/32] drm/i915/bdw: Abstract PDP usage
From: Ben Widawsky
Up until now, ppgtt->pdp has always been the root of our page tables.
Legacy 32b addresses acted like it had 1 PDP with 4 PDPEs.
In preparation for 4 level page tables, we need to stop use ppgtt->pdp
directly unless we know it's what we want. The future structure will use
ppgtt->pml4 for the top level, and the pdp is just one of the entries
being pointed to by a pml4e.
This patch addresses some carelessness done throughout development wrt
assumptions made of the root page tables.
v2: Updated after dynamic page allocation changes.
v3: Rebase after s/page_tables/page_table/.
Signed-off-by: Ben Widawsky
Signed-off-by: Michel Thierry (v2+)
---
drivers/gpu/drm/i915/i915_gem_gtt.c | 123
1 file changed, 70 insertions(+), 53 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 2a453fd..50583a4 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -560,6 +560,7 @@ static void gen8_ppgtt_clear_range(struct
i915_address_space *vm,
{
struct i915_hw_ppgtt *ppgtt =
container_of(vm, struct i915_hw_ppgtt, base);
+ struct i915_page_directory_pointer_entry *pdp = &ppgtt->pdp; /* FIXME:
48b */
gen8_gtt_pte_t *pt_vaddr, scratch_pte;
unsigned pdpe = start >> GEN8_PDPE_SHIFT & GEN8_PDPE_MASK;
unsigned pde = start >> GEN8_PDE_SHIFT & GEN8_PDE_MASK;
@@ -575,10 +576,10 @@ static void gen8_ppgtt_clear_range(struct
i915_address_space *vm,
struct i915_page_table_entry *pt;
struct page *page_table;
- if (WARN_ON(!ppgtt->pdp.page_directory[pdpe]))
+ if (WARN_ON(!pdp->page_directory[pdpe]))
continue;
- pd = ppgtt->pdp.page_directory[pdpe];
+ pd = pdp->page_directory[pdpe];
if (WARN_ON(!pd->page_table[pde]))
continue;
@@ -620,6 +621,7 @@ static void gen8_ppgtt_insert_entries(struct
i915_address_space *vm,
{
struct i915_hw_ppgtt *ppgtt =
container_of(vm, struct i915_hw_ppgtt, base);
+ struct i915_page_directory_pointer_entry *pdp = &ppgtt->pdp; /* FIXME:
48b */
gen8_gtt_pte_t *pt_vaddr;
unsigned pdpe = start >> GEN8_PDPE_SHIFT & GEN8_PDPE_MASK;
unsigned pde = start >> GEN8_PDE_SHIFT & GEN8_PDE_MASK;
@@ -630,7 +632,7 @@ static void gen8_ppgtt_insert_entries(struct
i915_address_space *vm,
for_each_sg_page(pages->sgl, &sg_iter, pages->nents, 0) {
if (pt_vaddr == NULL) {
- struct i915_page_directory_entry *pd =
ppgtt->pdp.page_directory[pdpe];
+ struct i915_page_directory_entry *pd =
pdp->page_directory[pdpe];
struct i915_page_table_entry *pt = pd->page_table[pde];
struct page *page_table = pt->page;
@@ -708,16 +710,17 @@ static void gen8_free_page_tables(struct
i915_page_directory_entry *pd, struct d
static void gen8_ppgtt_unmap_pages(struct i915_hw_ppgtt *ppgtt)
{
struct pci_dev *hwdev = ppgtt->base.dev->pdev;
+ struct i915_page_directory_pointer_entry *pdp = &ppgtt->pdp; /* FIXME:
48b */
int i, j;
- for_each_set_bit(i, ppgtt->pdp.used_pdpes,
+ for_each_set_bit(i, pdp->used_pdpes,
I915_PDPES_PER_PDP(ppgtt->base.dev)) {
struct i915_page_directory_entry *pd;
- if (WARN_ON(!ppgtt->pdp.page_directory[i]))
+ if (WARN_ON(!pdp->page_directory[i]))
continue;
- pd = ppgtt->pdp.page_directory[i];
+ pd = pdp->page_directory[i];
if (!pd->daddr)
pci_unmap_page(hwdev, pd->daddr, PAGE_SIZE,
PCI_DMA_BIDIRECTIONAL);
@@ -743,15 +746,21 @@ static void gen8_ppgtt_free(struct i915_hw_ppgtt *ppgtt)
{
int i;
- for_each_set_bit(i, ppgtt->pdp.used_pdpes,
- I915_PDPES_PER_PDP(ppgtt->base.dev)) {
- if (WARN_ON(!ppgtt->pdp.page_directory[i]))
- continue;
+ if (!USES_FULL_48BIT_PPGTT(ppgtt->base.dev)) {
+ for_each_set_bit(i, ppgtt->pdp.used_pdpes,
+I915_PDPES_PER_PDP(ppgtt->base.dev)) {
+ if (WARN_ON(!ppgtt->pdp.page_directory[i]))
+ continue;
- gen8_free_page_tables(ppgtt->pdp.page_directory[i],
ppgtt->base.dev);
- unmap_and_free_pd(ppgtt->pdp.page_directory[i],
ppgtt->base.dev);
+ gen8_free_page_tables(ppgtt->pdp.page_directory[i],
+ ppgtt->base.dev);
+ unmap_and_free_pd(ppgtt->pdp.page_directory[i],
+ ppgtt->base.dev);
+ }
+
[Intel-gfx] [PATCH v6 11/32] drm/i915/bdw: Use dynamic allocation idioms on free
From: Ben Widawsky
The page directory freer is left here for now as it's still useful given
that GEN8 still preallocates. Once the allocation functions are broken
up into more discrete chunks, we'll follow suit and destroy this
leftover piece.
v2: Match trace_i915_va_teardown params
v3: Multiple rebases.
v4: Updated to use unmap_and_free_pt.
v5: teardown_va_range logic no longer needed.
v6: Rebase after s/page_tables/page_table/.
Signed-off-by: Ben Widawsky
Signed-off-by: Michel Thierry (v2+)
---
drivers/gpu/drm/i915/i915_gem_gtt.c | 26 ++--
drivers/gpu/drm/i915/i915_gem_gtt.h | 47 +
2 files changed, 60 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 94cdd99..e03b2c8 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -607,19 +607,6 @@ static void gen8_free_page_tables(struct
i915_page_directory_entry *pd, struct d
}
}
-static void gen8_ppgtt_free(struct i915_hw_ppgtt *ppgtt)
-{
- int i;
-
- for (i = 0; i < ppgtt->num_pd_pages; i++) {
- if (WARN_ON(!ppgtt->pdp.page_directory[i]))
- continue;
-
- gen8_free_page_tables(ppgtt->pdp.page_directory[i],
ppgtt->base.dev);
- unmap_and_free_pd(ppgtt->pdp.page_directory[i]);
- }
-}
-
static void gen8_ppgtt_unmap_pages(struct i915_hw_ppgtt *ppgtt)
{
struct pci_dev *hwdev = ppgtt->base.dev->pdev;
@@ -652,6 +639,19 @@ static void gen8_ppgtt_unmap_pages(struct i915_hw_ppgtt
*ppgtt)
}
}
+static void gen8_ppgtt_free(struct i915_hw_ppgtt *ppgtt)
+{
+ int i;
+
+ for (i = 0; i < ppgtt->num_pd_pages; i++) {
+ if (WARN_ON(!ppgtt->pdp.page_directory[i]))
+ continue;
+
+ gen8_free_page_tables(ppgtt->pdp.page_directory[i],
ppgtt->base.dev);
+ unmap_and_free_pd(ppgtt->pdp.page_directory[i]);
+ }
+}
+
static void gen8_ppgtt_cleanup(struct i915_address_space *vm)
{
struct i915_hw_ppgtt *ppgtt =
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h
b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 5918131..1f5c136 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -383,6 +383,53 @@ static inline uint32_t gen6_pde_index(uint32_t addr)
return i915_pde_index(addr, GEN6_PDE_SHIFT);
}
+#define gen8_for_each_pde(pt, pd, start, length, temp, iter) \
+ for (iter = gen8_pde_index(start), pt = (pd)->page_table[iter]; \
+length > 0 && iter < GEN8_PDES_PER_PAGE; \
+pt = (pd)->page_table[++iter], \
+temp = ALIGN(start+1, 1 << GEN8_PDE_SHIFT) - start,\
+temp = min(temp, length), \
+start += temp, length -= temp)
+
+#define gen8_for_each_pdpe(pd, pdp, start, length, temp, iter) \
+ for (iter = gen8_pdpe_index(start), pd = (pdp)->page_directory[iter];
\
+length > 0 && iter < GEN8_LEGACY_PDPES;\
+pd = (pdp)->page_directory[++iter],
\
+temp = ALIGN(start+1, 1 << GEN8_PDPE_SHIFT) - start, \
+temp = min(temp, length), \
+start += temp, length -= temp)
+
+/* Clamp length to the next page_directory boundary */
+static inline uint64_t gen8_clamp_pd(uint64_t start, uint64_t length)
+{
+ uint64_t next_pd = ALIGN(start + 1, 1 << GEN8_PDPE_SHIFT);
+
+ if (next_pd > (start + length))
+ return length;
+
+ return next_pd - start;
+}
+
+static inline uint32_t gen8_pte_index(uint64_t address)
+{
+ return i915_pte_index(address, GEN8_PDE_SHIFT);
+}
+
+static inline uint32_t gen8_pde_index(uint64_t address)
+{
+ return i915_pde_index(address, GEN8_PDE_SHIFT);
+}
+
+static inline uint32_t gen8_pdpe_index(uint64_t address)
+{
+ return (address >> GEN8_PDPE_SHIFT) & GEN8_PDPE_MASK;
+}
+
+static inline uint32_t gen8_pml4e_index(uint64_t address)
+{
+ BUG(); /* For 64B */
+}
+
int i915_gem_gtt_init(struct drm_device *dev);
void i915_gem_init_global_gtt(struct drm_device *dev);
void i915_global_gtt_cleanup(struct drm_device *dev);
--
2.1.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v6 15/32] drm/i915: num_pd_pages/num_pd_entries isn't useful
From: Ben Widawsky
These values are never quite useful for dynamic allocations of the page
tables. Getting rid of them will help prevent later confusion.
v2: Updated to use unmap_and_free_pd functions.
v3: Updated gen8_ppgtt_free after teardown logic was removed.
v4: Rebase after s/page_tables/page_table/.
Signed-off-by: Ben Widawsky
Signed-off-by: Michel Thierry (v2+)
---
drivers/gpu/drm/i915/i915_debugfs.c | 2 --
drivers/gpu/drm/i915/i915_gem_gtt.c | 72 -
drivers/gpu/drm/i915/i915_gem_gtt.h | 7 ++--
3 files changed, 28 insertions(+), 53 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index e8ad450..e85da9d 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2149,8 +2149,6 @@ static void gen8_ppgtt_info(struct seq_file *m, struct
drm_device *dev)
if (!ppgtt)
return;
- seq_printf(m, "Page directories: %d\n", ppgtt->num_pd_pages);
- seq_printf(m, "Page tables: %d\n", ppgtt->num_pd_entries);
for_each_ring(ring, dev_priv, unused) {
seq_printf(m, "%s\n", ring->name);
for (i = 0; i < 4; i++) {
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 2e4db77..bddfcc2 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -613,9 +613,7 @@ static void gen8_ppgtt_unmap_pages(struct i915_hw_ppgtt
*ppgtt)
struct pci_dev *hwdev = ppgtt->base.dev->pdev;
int i, j;
- for (i = 0; i < ppgtt->num_pd_pages; i++) {
- /* TODO: In the future we'll support sparse mappings, so this
-* will have to change. */
+ for (i = 0; i < GEN8_LEGACY_PDPES; i++) {
if (!ppgtt->pdp.page_directory[i]->daddr)
continue;
@@ -644,7 +642,7 @@ static void gen8_ppgtt_free(struct i915_hw_ppgtt *ppgtt)
{
int i;
- for (i = 0; i < ppgtt->num_pd_pages; i++) {
+ for (i = 0; i < GEN8_LEGACY_PDPES; i++) {
if (WARN_ON(!ppgtt->pdp.page_directory[i]))
continue;
@@ -705,21 +703,13 @@ static int gen8_ppgtt_alloc_page_directories(struct
i915_page_directory_pointer_
pdp->page_directory[pdpe] = alloc_pd_single();
if (IS_ERR(ppgtt->pdp.page_directory[pdpe]))
goto unwind_out;
-
- ppgtt->num_pd_pages++;
}
- BUG_ON(ppgtt->num_pd_pages > GEN8_LEGACY_PDPES);
-
return 0;
unwind_out:
- while (pdpe--) {
+ while (pdpe--)
unmap_and_free_pd(ppgtt->pdp.page_directory[pdpe]);
- ppgtt->num_pd_pages--;
- }
-
- WARN_ON(ppgtt->num_pd_pages);
return -ENOMEM;
}
@@ -742,12 +732,8 @@ static int gen8_ppgtt_alloc(struct i915_hw_ppgtt *ppgtt,
ppgtt->base.dev);
if (ret)
goto err_out;
-
- ppgtt->num_pd_entries += GEN8_PDES_PER_PAGE;
}
- BUG_ON(pdpe > ppgtt->num_pd_pages);
-
return 0;
err_out:
@@ -808,7 +794,6 @@ static int gen8_ppgtt_setup_page_tables(struct
i915_hw_ppgtt *ppgtt,
static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt, uint64_t size)
{
const int max_pdp = DIV_ROUND_UP(size, 1 << 30);
- const int min_pt_pages = GEN8_PDES_PER_PAGE * max_pdp;
int i, j, ret;
if (size % (1<<30))
@@ -872,12 +857,6 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt,
uint64_t size)
ppgtt->base.cleanup = gen8_ppgtt_cleanup;
ppgtt->base.clear_range(&ppgtt->base, 0, ppgtt->base.total, true);
-
- DRM_DEBUG_DRIVER("Allocated %d pages for page directories (%d
wasted)\n",
-ppgtt->num_pd_pages, ppgtt->num_pd_pages - max_pdp);
- DRM_DEBUG_DRIVER("Allocated %d pages for page tables (%lld wasted)\n",
-ppgtt->num_pd_entries,
-(ppgtt->num_pd_entries - min_pt_pages) + size %
(1<<30));
return 0;
bail:
@@ -888,26 +867,20 @@ bail:
static void gen6_dump_ppgtt(struct i915_hw_ppgtt *ppgtt, struct seq_file *m)
{
- struct drm_i915_private *dev_priv = ppgtt->base.dev->dev_private;
struct i915_address_space *vm = &ppgtt->base;
- gen6_gtt_pte_t __iomem *pd_addr;
+ struct i915_page_table_entry *unused;
gen6_gtt_pte_t scratch_pte;
uint32_t pd_entry;
- int pte, pde;
+ uint32_t pte, pde, temp;
+ uint32_t start = ppgtt->base.start, length = ppgtt->base.total;
scratch_pte = vm->pte_encode(vm->scratch.addr, I915_CACHE_LLC, true, 0);
- pd_addr = (gen6_gtt_pte_t __iomem *)dev_priv->gtt.gsm +
- ppgtt->pd.pd_offset / sizeof(gen6_gtt_pte_t);
-
- seq_printf(m, " VM %p (pd_offset %x-%x):\n", vm,
- ppgtt->pd.pd_offset,
-
[Intel-gfx] [PATCH v6 30/32] drm/i915/bdw: Add 4 level support in insert_entries and clear_range
When 48b is enabled, gen8_ppgtt_insert_entries needs to read the Page Map
Level 4 (PML4), before it selects which Page Directory Pointer (PDP)
it will write to.
Similarly, gen8_ppgtt_clear_range needs to get the correct PDP/PD range.
Also add a scratch page for PML4.
This patch was inspired by Ben's "Depend exclusively on map and
unmap_vma".
v2: Rebase after s/page_tables/page_table/.
Signed-off-by: Michel Thierry
---
drivers/gpu/drm/i915/i915_gem_gtt.c | 66 ++---
drivers/gpu/drm/i915/i915_gem_gtt.h | 12 +++
2 files changed, 67 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 166daf4..842ce93 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -676,24 +676,52 @@ static void gen8_ppgtt_clear_pte_range(struct
i915_page_directory_pointer_entry
}
}
+static void gen8_ppgtt_clear_range_4lvl(struct i915_hw_ppgtt *ppgtt,
+ gen8_gtt_pte_t scratch_pte,
+ uint64_t start,
+ uint64_t length)
+{
+ struct i915_page_directory_pointer_entry *pdp;
+ uint64_t templ4, templ3, pml4e, pdpe;
+
+ gen8_for_each_pml4e(pdp, &ppgtt->pml4, start, length, templ4, pml4e) {
+ struct i915_page_directory_entry *pd;
+ uint64_t pdp_len = gen8_clamp_pdp(start, length);
+ uint64_t pdp_start = start;
+
+ gen8_for_each_pdpe(pd, pdp, pdp_start, pdp_len, templ3, pdpe) {
+ uint64_t pd_len = gen8_clamp_pd(pdp_start, pdp_len);
+ uint64_t pd_start = pdp_start;
+
+ gen8_ppgtt_clear_pte_range(pdp, pd_start, pd_len,
+ scratch_pte,
!HAS_LLC(ppgtt->base.dev));
+ }
+ }
+}
+
static void gen8_ppgtt_clear_range(struct i915_address_space *vm,
- uint64_t start,
- uint64_t length,
+ uint64_t start, uint64_t length,
bool use_scratch)
{
struct i915_hw_ppgtt *ppgtt =
- container_of(vm, struct i915_hw_ppgtt, base);
- struct i915_page_directory_pointer_entry *pdp = &ppgtt->pdp; /* FIXME:
48b */
-
+ container_of(vm, struct i915_hw_ppgtt, base);
gen8_gtt_pte_t scratch_pte = gen8_pte_encode(ppgtt->base.scratch.addr,
I915_CACHE_LLC,
use_scratch);
- gen8_ppgtt_clear_pte_range(pdp, start, length, scratch_pte,
!HAS_LLC(vm->dev));
+ if (!USES_FULL_48BIT_PPGTT(vm->dev)) {
+ struct i915_page_directory_pointer_entry *pdp = &ppgtt->pdp;
+
+ gen8_ppgtt_clear_pte_range(pdp, start, length, scratch_pte,
+ !HAS_LLC(ppgtt->base.dev));
+ } else {
+ gen8_ppgtt_clear_range_4lvl(ppgtt, scratch_pte, start, length);
+ }
}
static void gen8_ppgtt_insert_pte_entries(struct
i915_page_directory_pointer_entry *pdp,
struct sg_page_iter *sg_iter,
uint64_t start,
+ size_t pages,
enum i915_cache_level cache_level,
const bool flush)
{
@@ -704,7 +732,7 @@ static void gen8_ppgtt_insert_pte_entries(struct
i915_page_directory_pointer_ent
pt_vaddr = NULL;
- while (__sg_page_iter_next(sg_iter)) {
+ while (pages-- && __sg_page_iter_next(sg_iter)) {
if (pt_vaddr == NULL) {
struct i915_page_directory_entry *pd =
pdp->page_directory[pdpe];
struct i915_page_table_entry *pt = pd->page_table[pde];
@@ -742,11 +770,26 @@ static void gen8_ppgtt_insert_entries(struct
i915_address_space *vm,
u32 unused)
{
struct i915_hw_ppgtt *ppgtt = container_of(vm, struct i915_hw_ppgtt,
base);
- struct i915_page_directory_pointer_entry *pdp = &ppgtt->pdp; /* FIXME:
48b */
+ struct i915_page_directory_pointer_entry *pdp;
struct sg_page_iter sg_iter;
__sg_page_iter_start(&sg_iter, pages->sgl, sg_nents(pages->sgl), 0);
- gen8_ppgtt_insert_pte_entries(pdp, &sg_iter, start, cache_level,
!HAS_LLC(vm->dev));
+
+ if (!USES_FULL_48BIT_PPGTT(vm->dev)) {
+ pdp = &ppgtt->pdp;
+ gen8_ppgtt_insert_pte_entries(pdp, &sg_iter, start,
+ sg_nents(pages->sgl),
+ cache_level, !HAS_LLC(vm->dev));
+ } else {
+ struct i915_pml4 *pml4;
+ unsigned pml4e = gen8_pml4e_index(start);
+
+ pml4 = &pp
[Intel-gfx] [PATCH v6 10/32] drm/i915: Add dynamic page trace events
Traces for page directories and tables allocation and map.
v2: Removed references to teardown.
v3: bitmap_scnprintf has been deprecated.
Signed-off-by: Michel Thierry
---
drivers/gpu/drm/i915/i915_gem.c | 2 +
drivers/gpu/drm/i915/i915_gem_gtt.c | 5 ++
drivers/gpu/drm/i915/i915_trace.h | 95 +
3 files changed, 102 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 312b7d2..4e51275 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3601,6 +3601,8 @@ search_free:
/* allocate before insert / bind */
if (vma->vm->allocate_va_range) {
+ trace_i915_va_alloc(vma->vm, vma->node.start, vma->node.size,
+ VM_TO_TRACE_NAME(vma->vm));
ret = vma->vm->allocate_va_range(vma->vm,
vma->node.start,
vma->node.size);
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 29cda58..94cdd99 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1210,6 +1210,7 @@ static int gen6_alloc_va_range(struct i915_address_space
*vm,
ppgtt->pd.page_table[pde] = pt;
set_bit(pde, new_page_tables);
+ trace_i915_page_table_entry_alloc(vm, pde, start,
GEN6_PDE_SHIFT);
}
start = start_save;
@@ -1225,6 +1226,10 @@ static int gen6_alloc_va_range(struct i915_address_space
*vm,
if (test_and_clear_bit(pde, new_page_tables))
gen6_write_pde(&ppgtt->pd, pde, pt);
+ trace_i915_page_table_entry_map(vm, pde, pt,
+gen6_pte_index(start),
+gen6_pte_count(start, length),
+I915_PPGTT_PT_ENTRIES);
bitmap_or(pt->used_ptes, tmp_bitmap, pt->used_ptes,
I915_PPGTT_PT_ENTRIES);
}
diff --git a/drivers/gpu/drm/i915/i915_trace.h
b/drivers/gpu/drm/i915/i915_trace.h
index f004d3d..0038dc2 100644
--- a/drivers/gpu/drm/i915/i915_trace.h
+++ b/drivers/gpu/drm/i915/i915_trace.h
@@ -156,6 +156,101 @@ TRACE_EVENT(i915_vma_unbind,
__entry->obj, __entry->offset, __entry->size, __entry->vm)
);
+#define VM_TO_TRACE_NAME(vm) \
+ (i915_is_ggtt(vm) ? "GGTT" : \
+ "Private VM")
+
+DECLARE_EVENT_CLASS(i915_va,
+ TP_PROTO(struct i915_address_space *vm, u64 start, u64 length, const
char *name),
+ TP_ARGS(vm, start, length, name),
+
+ TP_STRUCT__entry(
+ __field(struct i915_address_space *, vm)
+ __field(u64, start)
+ __field(u64, end)
+ __string(name, name)
+ ),
+
+ TP_fast_assign(
+ __entry->vm = vm;
+ __entry->start = start;
+ __entry->end = start + length;
+ __assign_str(name, name);
+ ),
+
+ TP_printk("vm=%p (%s), 0x%llx-0x%llx",
+ __entry->vm, __get_str(name), __entry->start, __entry->end)
+);
+
+DEFINE_EVENT(i915_va, i915_va_alloc,
+TP_PROTO(struct i915_address_space *vm, u64 start, u64 length,
const char *name),
+TP_ARGS(vm, start, length, name)
+);
+
+DECLARE_EVENT_CLASS(i915_page_table_entry,
+ TP_PROTO(struct i915_address_space *vm, u32 pde, u64 start, u64
pde_shift),
+ TP_ARGS(vm, pde, start, pde_shift),
+
+ TP_STRUCT__entry(
+ __field(struct i915_address_space *, vm)
+ __field(u32, pde)
+ __field(u64, start)
+ __field(u64, end)
+ ),
+
+ TP_fast_assign(
+ __entry->vm = vm;
+ __entry->pde = pde;
+ __entry->start = start;
+ __entry->end = (start + (1ULL << pde_shift)) & ~((1ULL <<
pde_shift)-1);
+ ),
+
+ TP_printk("vm=%p, pde=%d (0x%llx-0x%llx)",
+ __entry->vm, __entry->pde, __entry->start, __entry->end)
+);
+
+DEFINE_EVENT(i915_page_table_entry, i915_page_table_entry_alloc,
+TP_PROTO(struct i915_address_space *vm, u32 pde, u64 start, u64
pde_shift),
+TP_ARGS(vm, pde, start, pde_shift)
+);
+
+/* Avoid extra math because we only support two sizes. The format is defined by
+ * bitmap_scnprintf. Each 32 bits is 8 HEX digits followed by comma */
+#define TRACE_PT_SIZE(bits) \
+ bits) == 1024) ? 288 : 144) + 1)
+
+DECLARE_EVENT_CLASS(i915_page_table_entry_update,
+ TP_PROTO(struct i915_address_space *vm, u32 pde,
+struct i915_page_table_entry *pt, u32 first, u32 len, size_t
bits),
+ TP_ARGS(vm, pde, pt, first, len, bits),
+
+ TP_STRUCT__entry(
+ __field(struct i915_address_space *, vm)
+ __field(u32, p
[Intel-gfx] [PATCH v6 13/32] drm/i915/bdw: pagetable allocation rework
From: Ben Widawsky
Start using gen8_for_each_pde macro to allocate page tables.
v2: teardown_va_range references removed.
v3: Rebase after s/page_tables/page_table/.
Signed-off-by: Ben Widawsky
Signed-off-by: Michel Thierry (v2+)
---
drivers/gpu/drm/i915/i915_gem_gtt.c | 46 +++--
1 file changed, 29 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index ade8edd..762c535 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -661,22 +661,27 @@ static void gen8_ppgtt_cleanup(struct i915_address_space
*vm)
gen8_ppgtt_free(ppgtt);
}
-static int gen8_ppgtt_allocate_page_tables(struct i915_hw_ppgtt *ppgtt)
+static int gen8_ppgtt_alloc_pagetabs(struct i915_page_directory_entry *pd,
+uint64_t start,
+uint64_t length,
+struct drm_device *dev)
{
- int i, ret;
+ struct i915_page_table_entry *unused;
+ uint64_t temp;
+ uint32_t pde;
- for (i = 0; i < ppgtt->num_pd_pages; i++) {
- ret = alloc_pt_range(ppgtt->pdp.page_directory[i],
-0, GEN8_PDES_PER_PAGE, ppgtt->base.dev);
- if (ret)
+ gen8_for_each_pde(unused, pd, start, length, temp, pde) {
+ BUG_ON(unused);
+ pd->page_table[pde] = alloc_pt_single(dev);
+ if (IS_ERR(pd->page_table[pde]))
goto unwind_out;
}
return 0;
unwind_out:
- while (i--)
- gen8_free_page_tables(ppgtt->pdp.page_directory[i],
ppgtt->base.dev);
+ while (pde--)
+ unmap_and_free_pt(pd->page_table[pde], dev);
return -ENOMEM;
}
@@ -719,20 +724,28 @@ unwind_out:
}
static int gen8_ppgtt_alloc(struct i915_hw_ppgtt *ppgtt,
- const int max_pdp)
+ uint64_t start,
+ uint64_t length)
{
+ struct i915_page_directory_entry *pd;
+ uint64_t temp;
+ uint32_t pdpe;
int ret;
- ret = gen8_ppgtt_alloc_page_directories(&ppgtt->pdp, ppgtt->base.start,
- ppgtt->base.total);
+ ret = gen8_ppgtt_alloc_page_directories(&ppgtt->pdp, start, length);
if (ret)
return ret;
- ret = gen8_ppgtt_allocate_page_tables(ppgtt);
- if (ret)
- goto err_out;
+ gen8_for_each_pdpe(pd, &ppgtt->pdp, start, length, temp, pdpe) {
+ ret = gen8_ppgtt_alloc_pagetabs(pd, start, length,
+ ppgtt->base.dev);
+ if (ret)
+ goto err_out;
+
+ ppgtt->num_pd_entries += GEN8_PDES_PER_PAGE;
+ }
- ppgtt->num_pd_entries = max_pdp * GEN8_PDES_PER_PAGE;
+ BUG_ON(pdpe > ppgtt->num_pd_pages);
return 0;
@@ -802,10 +815,9 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt,
uint64_t size)
ppgtt->base.start = 0;
ppgtt->base.total = size;
- BUG_ON(ppgtt->base.total == 0);
/* 1. Do all our allocations for page directories and page tables. */
- ret = gen8_ppgtt_alloc(ppgtt, max_pdp);
+ ret = gen8_ppgtt_alloc(ppgtt, ppgtt->base.start, ppgtt->base.total);
if (ret)
return ret;
--
2.1.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v6 04/32] drm/i915: Plumb drm_device through page tables operations
The next patch in the series will require it for alloc_pt_single.
v2: Rebased after s/page_tables/page_table/.
Signed-off-by: Michel Thierry
---
drivers/gpu/drm/i915/i915_gem_gtt.c | 29 -
1 file changed, 16 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 81c1dba..e05488e 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -142,7 +142,6 @@ static int sanitize_enable_ppgtt(struct drm_device *dev,
int enable_ppgtt)
return has_aliasing_ppgtt ? 1 : 0;
}
-
static void ppgtt_bind_vma(struct i915_vma *vma,
enum i915_cache_level cache_level,
u32 flags);
@@ -279,7 +278,7 @@ static gen6_gtt_pte_t iris_pte_encode(dma_addr_t addr,
return pte;
}
-static void unmap_and_free_pt(struct i915_page_table_entry *pt)
+static void unmap_and_free_pt(struct i915_page_table_entry *pt, struct
drm_device *dev)
{
if (WARN_ON(!pt->page))
return;
@@ -287,7 +286,7 @@ static void unmap_and_free_pt(struct i915_page_table_entry
*pt)
kfree(pt);
}
-static struct i915_page_table_entry *alloc_pt_single(void)
+static struct i915_page_table_entry *alloc_pt_single(struct drm_device *dev)
{
struct i915_page_table_entry *pt;
@@ -317,7 +316,9 @@ static struct i915_page_table_entry *alloc_pt_single(void)
*
* Return: 0 if allocation succeeded.
*/
-static int alloc_pt_range(struct i915_page_directory_entry *pd, uint16_t pde,
size_t count)
+static int alloc_pt_range(struct i915_page_directory_entry *pd, uint16_t pde,
size_t count,
+ struct drm_device *dev)
+
{
int i, ret;
@@ -326,7 +327,7 @@ static int alloc_pt_range(struct i915_page_directory_entry
*pd, uint16_t pde, si
return -EINVAL;
for (i = pde; i < pde + count; i++) {
- struct i915_page_table_entry *pt = alloc_pt_single();
+ struct i915_page_table_entry *pt = alloc_pt_single(dev);
if (IS_ERR(pt)) {
ret = PTR_ERR(pt);
@@ -342,7 +343,7 @@ static int alloc_pt_range(struct i915_page_directory_entry
*pd, uint16_t pde, si
err_out:
while (i-- > pde)
- unmap_and_free_pt(pd->page_table[i]);
+ unmap_and_free_pt(pd->page_table[i], dev);
return ret;
}
@@ -521,7 +522,7 @@ static void gen8_ppgtt_insert_entries(struct
i915_address_space *vm,
}
}
-static void gen8_free_page_tables(struct i915_page_directory_entry *pd)
+static void gen8_free_page_tables(struct i915_page_directory_entry *pd, struct
drm_device *dev)
{
int i;
@@ -532,7 +533,7 @@ static void gen8_free_page_tables(struct
i915_page_directory_entry *pd)
if (WARN_ON(!pd->page_table[i]))
continue;
- unmap_and_free_pt(pd->page_table[i]);
+ unmap_and_free_pt(pd->page_table[i], dev);
pd->page_table[i] = NULL;
}
}
@@ -545,7 +546,7 @@ static void gen8_ppgtt_free(struct i915_hw_ppgtt *ppgtt)
if (WARN_ON(!ppgtt->pdp.page_directory[i]))
continue;
- gen8_free_page_tables(ppgtt->pdp.page_directory[i]);
+ gen8_free_page_tables(ppgtt->pdp.page_directory[i],
ppgtt->base.dev);
unmap_and_free_pd(ppgtt->pdp.page_directory[i]);
}
}
@@ -597,7 +598,7 @@ static int gen8_ppgtt_allocate_page_tables(struct
i915_hw_ppgtt *ppgtt)
for (i = 0; i < ppgtt->num_pd_pages; i++) {
ret = alloc_pt_range(ppgtt->pdp.page_directory[i],
-0, GEN8_PDES_PER_PAGE);
+0, GEN8_PDES_PER_PAGE, ppgtt->base.dev);
if (ret)
goto unwind_out;
}
@@ -606,7 +607,7 @@ static int gen8_ppgtt_allocate_page_tables(struct
i915_hw_ppgtt *ppgtt)
unwind_out:
while (i--)
- gen8_free_page_tables(ppgtt->pdp.page_directory[i]);
+ gen8_free_page_tables(ppgtt->pdp.page_directory[i],
ppgtt->base.dev);
return -ENOMEM;
}
@@ -1087,7 +1088,7 @@ static void gen6_ppgtt_free(struct i915_hw_ppgtt *ppgtt)
int i;
for (i = 0; i < ppgtt->num_pd_entries; i++)
- unmap_and_free_pt(ppgtt->pd.page_table[i]);
+ unmap_and_free_pt(ppgtt->pd.page_table[i], ppgtt->base.dev);
unmap_and_free_pd(&ppgtt->pd);
}
@@ -1152,7 +1153,9 @@ static int gen6_ppgtt_alloc(struct i915_hw_ppgtt *ppgtt)
if (ret)
return ret;
- ret = alloc_pt_range(&ppgtt->pd, 0, ppgtt->num_pd_entries);
+ ret = alloc_pt_range(&ppgtt->pd, 0, ppgtt->num_pd_entries,
+ ppgtt->base.dev);
+
if (ret) {
drm_mm_remove_node(&ppgtt->node);
return ret;
--
2.1.1
_
[Intel-gfx] [PATCH v6 14/32] drm/i915/bdw: Update pdp switch and point unused PDPs to scratch page
From: Ben Widawsky
One important part of this patch is we now write a scratch page
directory into any unused PDP descriptors. This matters for 2 reasons,
first, we're not allowed to just use 0, or an invalid pointer, and second,
we must wipe out any previous contents from the last context.
The latter point only matters with full PPGTT. The former point only
effect platforms with less than 4GB memory.
v2: Updated commit message to point that we must set unused PDPs to the
scratch page.
Signed-off-by: Ben Widawsky
Signed-off-by: Michel Thierry (v2)
---
drivers/gpu/drm/i915/i915_gem_gtt.c | 29 ++---
drivers/gpu/drm/i915/i915_gem_gtt.h | 5 -
2 files changed, 22 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 762c535..2e4db77 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -442,8 +442,9 @@ static struct i915_page_directory_entry
*alloc_pd_single(void)
}
/* Broadwell Page Directory Pointer Descriptors */
-static int gen8_write_pdp(struct intel_engine_cs *ring, unsigned entry,
- uint64_t val)
+static int gen8_write_pdp(struct intel_engine_cs *ring,
+ unsigned entry,
+ dma_addr_t addr)
{
int ret;
@@ -455,10 +456,10 @@ static int gen8_write_pdp(struct intel_engine_cs *ring,
unsigned entry,
intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
intel_ring_emit(ring, GEN8_RING_PDP_UDW(ring, entry));
- intel_ring_emit(ring, (u32)(val >> 32));
+ intel_ring_emit(ring, upper_32_bits(addr));
intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
intel_ring_emit(ring, GEN8_RING_PDP_LDW(ring, entry));
- intel_ring_emit(ring, (u32)(val));
+ intel_ring_emit(ring, lower_32_bits(addr));
intel_ring_advance(ring);
return 0;
@@ -469,12 +470,12 @@ static int gen8_mm_switch(struct i915_hw_ppgtt *ppgtt,
{
int i, ret;
- /* bit of a hack to find the actual last used pd */
- int used_pd = ppgtt->num_pd_entries / GEN8_PDES_PER_PAGE;
-
- for (i = used_pd - 1; i >= 0; i--) {
- dma_addr_t addr = ppgtt->pdp.page_directory[i]->daddr;
- ret = gen8_write_pdp(ring, i, addr);
+ for (i = GEN8_LEGACY_PDPES - 1; i >= 0; i--) {
+ struct i915_page_directory_entry *pd =
ppgtt->pdp.page_directory[i];
+ dma_addr_t pd_daddr = pd ? pd->daddr : ppgtt->scratch_pd->daddr;
+ /* The page directory might be NULL, but we need to clear out
+* whatever the previous context might have used. */
+ ret = gen8_write_pdp(ring, i, pd_daddr);
if (ret)
return ret;
}
@@ -816,10 +817,16 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt,
uint64_t size)
ppgtt->base.start = 0;
ppgtt->base.total = size;
+ ppgtt->scratch_pd = alloc_pt_scratch(ppgtt->base.dev);
+ if (IS_ERR(ppgtt->scratch_pd))
+ return PTR_ERR(ppgtt->scratch_pd);
+
/* 1. Do all our allocations for page directories and page tables. */
ret = gen8_ppgtt_alloc(ppgtt, ppgtt->base.start, ppgtt->base.total);
- if (ret)
+ if (ret) {
+ unmap_and_free_pt(ppgtt->scratch_pd, ppgtt->base.dev);
return ret;
+ }
/*
* 2. Create DMA mappings for the page directories and page tables.
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h
b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 1f5c136..7a16627 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -306,7 +306,10 @@ struct i915_hw_ppgtt {
struct i915_page_directory_entry pd;
};
- struct i915_page_table_entry *scratch_pt;
+ union {
+ struct i915_page_table_entry *scratch_pt;
+ struct i915_page_table_entry *scratch_pd; /* Just need the
daddr */
+ };
struct drm_i915_file_private *file_priv;
--
2.1.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v6 26/32] drm/i915/bdw: Add 4 level switching infrastructure
From: Ben Widawsky
Map is easy, it's the same register as the PDP descriptor 0, but it only
has one entry.
v2: PML4 update in legacy context switch is left for historic reasons,
the preferred mode of operation is with lrc context based submission.
Signed-off-by: Ben Widawsky
Signed-off-by: Michel Thierry
---
drivers/gpu/drm/i915/i915_gem_gtt.c | 56 +
drivers/gpu/drm/i915/i915_gem_gtt.h | 4 ++-
drivers/gpu/drm/i915/i915_reg.h | 1 +
3 files changed, 55 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 4c921d0..88a3c49 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -193,6 +193,9 @@ static inline gen8_ppgtt_pde_t gen8_pde_encode(struct
drm_device *dev,
return pde;
}
+#define gen8_pdpe_encode gen8_pde_encode
+#define gen8_pml4e_encode gen8_pde_encode
+
static gen6_gtt_pte_t snb_pte_encode(dma_addr_t addr,
enum i915_cache_level level,
bool valid, u32 unused)
@@ -592,8 +595,8 @@ static int gen8_write_pdp(struct intel_engine_cs *ring,
return 0;
}
-static int gen8_mm_switch(struct i915_hw_ppgtt *ppgtt,
- struct intel_engine_cs *ring)
+static int gen8_legacy_mm_switch(struct i915_hw_ppgtt *ppgtt,
+struct intel_engine_cs *ring)
{
int i, ret;
@@ -610,6 +613,12 @@ static int gen8_mm_switch(struct i915_hw_ppgtt *ppgtt,
return 0;
}
+static int gen8_48b_mm_switch(struct i915_hw_ppgtt *ppgtt,
+ struct intel_engine_cs *ring)
+{
+ return gen8_write_pdp(ring, 0, ppgtt->pml4.daddr);
+}
+
static void gen8_ppgtt_clear_range(struct i915_address_space *vm,
uint64_t start,
uint64_t length,
@@ -753,6 +762,37 @@ static void gen8_map_pagetable_range(struct
i915_address_space *vm,
kunmap_atomic(page_directory);
}
+static void gen8_map_page_directory(struct i915_page_directory_pointer_entry
*pdp,
+ struct i915_page_directory_entry *pd,
+ int index,
+ struct drm_device *dev)
+{
+ gen8_ppgtt_pdpe_t *page_directorypo;
+ gen8_ppgtt_pdpe_t pdpe;
+
+ /* We do not need to clflush because no platform requiring flush
+* supports 64b pagetables. */
+ if (!USES_FULL_48BIT_PPGTT(dev))
+ return;
+
+ page_directorypo = kmap_atomic(pdp->page);
+ pdpe = gen8_pdpe_encode(dev, pd->daddr, I915_CACHE_LLC);
+ page_directorypo[index] = pdpe;
+ kunmap_atomic(page_directorypo);
+}
+
+static void gen8_map_page_directory_pointer(struct i915_pml4 *pml4,
+ struct
i915_page_directory_pointer_entry *pdp,
+ int index,
+ struct drm_device *dev)
+{
+ gen8_ppgtt_pml4e_t *pagemap = kmap_atomic(pml4->page);
+ gen8_ppgtt_pml4e_t pml4e = gen8_pml4e_encode(dev, pdp->daddr,
I915_CACHE_LLC);
+ BUG_ON(!USES_FULL_48BIT_PPGTT(dev));
+ pagemap[index] = pml4e;
+ kunmap_atomic(pagemap);
+}
+
static void gen8_free_page_tables(struct i915_page_directory_entry *pd, struct
drm_device *dev)
{
int i;
@@ -1124,6 +1164,7 @@ static int gen8_alloc_va_range_3lvl(struct
i915_address_space *vm,
set_bit(pdpe, pdp->used_pdpes);
gen8_map_pagetable_range(vm, pd, start, length);
+ gen8_map_page_directory(pdp, pd, pdpe, dev);
}
free_gen8_temp_bitmaps(new_page_dirs, new_page_tables, pdpes);
@@ -1192,6 +1233,8 @@ static int gen8_alloc_va_range_4lvl(struct
i915_address_space *vm,
ret = gen8_alloc_va_range_3lvl(vm, pdp, start, length);
if (ret)
goto err_out;
+
+ gen8_map_page_directory_pointer(pml4, pdp, pml4e, vm->dev);
}
bitmap_or(pml4->used_pml4es, new_pdps, pml4->used_pml4es,
@@ -1251,14 +1294,14 @@ static int gen8_ppgtt_init_common(struct i915_hw_ppgtt
*ppgtt, uint64_t size)
ppgtt->base.cleanup = gen8_ppgtt_cleanup;
ppgtt->base.insert_entries = gen8_ppgtt_insert_entries;
- ppgtt->switch_mm = gen8_mm_switch;
-
if (USES_FULL_48BIT_PPGTT(ppgtt->base.dev)) {
int ret = pml4_init(ppgtt);
if (ret) {
unmap_and_free_pt(ppgtt->scratch_pd, ppgtt->base.dev);
return ret;
}
+
+ ppgtt->switch_mm = gen8_48b_mm_switch;
} else {
int ret = __pdp_init(&ppgtt->pdp, false);
if (ret) {
@@ -1266,6 +1309,7 @@ static int gen8_ppgtt_init_common(struct i915_hw_ppgtt
*ppgtt, uint64_t size)
[Intel-gfx] [PATCH v6 01/32] drm/i915: page table abstractions
From: Ben Widawsky
When we move to dynamic page allocation, keeping page_directory and pagetabs as
separate structures will help to break actions into simpler tasks.
To help transition the code nicely there is some wasted space in gen6/7.
This will be ameliorated shortly.
Following the x86 pagetable terminology:
PDPE = struct i915_page_directory_pointer_entry.
PDE = struct i915_page_directory_entry [page_directory].
PTE = struct i915_page_table_entry [page_tables].
v2: fixed mismatches after clean-up/rebase.
v3: Clarify the names of the multiple levels of page tables (Daniel)
v4: Addressing Mika's review comments.
s/gen8_free_page_directories/gen8_free_page_directory and free the
page tables for the directory there.
In gen8_ppgtt_allocate_page_directories, do not leak previously allocated
pt in case the page_directory alloc fails.
Update error return handling in gen8_ppgtt_alloc.
v5: Do not leak pt on error in gen6_ppgtt_allocate_page_tables. (Mika)
v6: s/page_tables/page_table/. (Mika)
Cc: Mika Kuoppala
Signed-off-by: Ben Widawsky
Signed-off-by: Michel Thierry (v2+)
Reviewed-by: Mika Kuoppala
---
drivers/gpu/drm/i915/i915_gem_gtt.c | 180 +++-
drivers/gpu/drm/i915/i915_gem_gtt.h | 23 -
2 files changed, 111 insertions(+), 92 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index e54b2a0..874d9cc 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -338,7 +338,8 @@ static void gen8_ppgtt_clear_range(struct
i915_address_space *vm,
I915_CACHE_LLC, use_scratch);
while (num_entries) {
- struct page *page_table = ppgtt->gen8_pt_pages[pdpe][pde];
+ struct i915_page_directory_entry *pd =
&ppgtt->pdp.page_directory[pdpe];
+ struct page *page_table = pd->page_table[pde].page;
last_pte = pte + num_entries;
if (last_pte > GEN8_PTES_PER_PAGE)
@@ -382,8 +383,12 @@ static void gen8_ppgtt_insert_entries(struct
i915_address_space *vm,
if (WARN_ON(pdpe >= GEN8_LEGACY_PDPES))
break;
- if (pt_vaddr == NULL)
- pt_vaddr = kmap_atomic(ppgtt->gen8_pt_pages[pdpe][pde]);
+ if (pt_vaddr == NULL) {
+ struct i915_page_directory_entry *pd =
&ppgtt->pdp.page_directory[pdpe];
+ struct page *page_table = pd->page_table[pde].page;
+
+ pt_vaddr = kmap_atomic(page_table);
+ }
pt_vaddr[pte] =
gen8_pte_encode(sg_page_iter_dma_address(&sg_iter),
@@ -407,29 +412,33 @@ static void gen8_ppgtt_insert_entries(struct
i915_address_space *vm,
}
}
-static void gen8_free_page_tables(struct page **pt_pages)
+static void gen8_free_page_tables(struct i915_page_directory_entry *pd)
{
int i;
- if (pt_pages == NULL)
+ if (pd->page_table == NULL)
return;
for (i = 0; i < GEN8_PDES_PER_PAGE; i++)
- if (pt_pages[i])
- __free_pages(pt_pages[i], 0);
+ if (pd->page_table[i].page)
+ __free_page(pd->page_table[i].page);
}
-static void gen8_ppgtt_free(const struct i915_hw_ppgtt *ppgtt)
+static void gen8_free_page_directory(struct i915_page_directory_entry *pd)
+{
+ gen8_free_page_tables(pd);
+ kfree(pd->page_table);
+ __free_page(pd->page);
+}
+
+static void gen8_ppgtt_free(struct i915_hw_ppgtt *ppgtt)
{
int i;
for (i = 0; i < ppgtt->num_pd_pages; i++) {
- gen8_free_page_tables(ppgtt->gen8_pt_pages[i]);
- kfree(ppgtt->gen8_pt_pages[i]);
+ gen8_free_page_directory(&ppgtt->pdp.page_directory[i]);
kfree(ppgtt->gen8_pt_dma_addr[i]);
}
-
- __free_pages(ppgtt->pd_pages, get_order(ppgtt->num_pd_pages <<
PAGE_SHIFT));
}
static void gen8_ppgtt_unmap_pages(struct i915_hw_ppgtt *ppgtt)
@@ -464,86 +473,77 @@ static void gen8_ppgtt_cleanup(struct i915_address_space
*vm)
gen8_ppgtt_free(ppgtt);
}
-static struct page **__gen8_alloc_page_tables(void)
+static int gen8_ppgtt_allocate_dma(struct i915_hw_ppgtt *ppgtt)
{
- struct page **pt_pages;
int i;
- pt_pages = kcalloc(GEN8_PDES_PER_PAGE, sizeof(struct page *),
GFP_KERNEL);
- if (!pt_pages)
- return ERR_PTR(-ENOMEM);
-
- for (i = 0; i < GEN8_PDES_PER_PAGE; i++) {
- pt_pages[i] = alloc_page(GFP_KERNEL);
- if (!pt_pages[i])
- goto bail;
+ for (i = 0; i < ppgtt->num_pd_pages; i++) {
+ ppgtt->gen8_pt_dma_addr[i] = kcalloc(GEN8_PDES_PER_PAGE,
+sizeof(dma_addr_t),
+GFP_KERNEL);
+
[Intel-gfx] [PATCH v6 21/32] drm/i915/bdw: Make pdp allocation more dynamic
From: Ben Widawsky
This transitional patch doesn't do much for the existing code. However,
it should make upcoming patches to use the full 48b address space a bit
easier to swallow. The patch also introduces the PML4, ie. the new top
level structure of the page tables.
v2: Renamed pdp_free to be similar to pd/pt (unmap_and_free_pdp).
v3: To facilitate testing, 48b mode will be available on Broadwell and
GEN9+, when i915.enable_ppgtt = 3.
v4: Rebase after s/page_tables/page_table/.
Signed-off-by: Ben Widawsky
Signed-off-by: Michel Thierry (v2)
---
drivers/gpu/drm/i915/i915_drv.h | 7 ++-
drivers/gpu/drm/i915/i915_gem_gtt.c | 109 +---
drivers/gpu/drm/i915/i915_gem_gtt.h | 41 +++---
3 files changed, 127 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3cc0196..662d6c1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2433,7 +2433,12 @@ struct drm_i915_cmd_table {
#define HAS_HW_CONTEXTS(dev) (INTEL_INFO(dev)->gen >= 6)
#define HAS_LOGICAL_RING_CONTEXTS(dev) (INTEL_INFO(dev)->gen >= 8)
#define USES_PPGTT(dev)(i915.enable_ppgtt)
-#define USES_FULL_PPGTT(dev) (i915.enable_ppgtt == 2)
+#define USES_FULL_PPGTT(dev) (i915.enable_ppgtt >= 2)
+#ifdef CONFIG_64BIT
+# define USES_FULL_48BIT_PPGTT(dev)(i915.enable_ppgtt == 3)
+#else
+# define USES_FULL_48BIT_PPGTT(dev)false
+#endif
#define HAS_OVERLAY(dev) (INTEL_INFO(dev)->has_overlay)
#define OVERLAY_NEEDS_PHYSICAL(dev)
(INTEL_INFO(dev)->overlay_needs_physical)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index b9dfc56..2a453fd 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -100,10 +100,18 @@ static int sanitize_enable_ppgtt(struct drm_device *dev,
int enable_ppgtt)
{
bool has_aliasing_ppgtt;
bool has_full_ppgtt;
+ bool has_full_64bit_ppgtt;
has_aliasing_ppgtt = INTEL_INFO(dev)->gen >= 6;
has_full_ppgtt = INTEL_INFO(dev)->gen >= 7;
+#ifdef CONFIG_64BIT
+ has_full_64bit_ppgtt = IS_BROADWELL(dev) ||
+ INTEL_INFO(dev)->gen >= 9 && false; /* FIXME:
64b */
+#else
+ has_full_64bit_ppgtt = false;
+#endif
+
if (intel_vgpu_active(dev))
has_full_ppgtt = false; /* emulation is too hard */
@@ -121,6 +129,9 @@ static int sanitize_enable_ppgtt(struct drm_device *dev,
int enable_ppgtt)
if (enable_ppgtt == 2 && has_full_ppgtt)
return 2;
+ if (enable_ppgtt == 3 && has_full_64bit_ppgtt)
+ return 3;
+
#ifdef CONFIG_INTEL_IOMMU
/* Disable ppgtt on SNB if VT-d is on. */
if (INTEL_INFO(dev)->gen == 6 && intel_iommu_gfx_mapped) {
@@ -461,6 +472,45 @@ free_pd:
return ERR_PTR(ret);
}
+static void __pdp_fini(struct i915_page_directory_pointer_entry *pdp)
+{
+ kfree(pdp->used_pdpes);
+ kfree(pdp->page_directory);
+ /* HACK */
+ pdp->page_directory = NULL;
+}
+
+static void unmap_and_free_pdp(struct i915_page_directory_pointer_entry *pdp,
+ struct drm_device *dev)
+{
+ __pdp_fini(pdp);
+ if (USES_FULL_48BIT_PPGTT(dev))
+ kfree(pdp);
+}
+
+static int __pdp_init(struct i915_page_directory_pointer_entry *pdp,
+ struct drm_device *dev)
+{
+ size_t pdpes = I915_PDPES_PER_PDP(dev);
+
+ pdp->used_pdpes = kcalloc(BITS_TO_LONGS(pdpes),
+ sizeof(unsigned long),
+ GFP_KERNEL);
+ if (!pdp->used_pdpes)
+ return -ENOMEM;
+
+ pdp->page_directory = kcalloc(pdpes, sizeof(*pdp->page_directory),
GFP_KERNEL);
+ if (!pdp->page_directory) {
+ kfree(pdp->used_pdpes);
+ /* the PDP might be the statically allocated top level. Keep it
+* as clean as possible */
+ pdp->used_pdpes = NULL;
+ return -ENOMEM;
+ }
+
+ return 0;
+}
+
/* Broadwell Page Directory Pointer Descriptors */
static int gen8_write_pdp(struct intel_engine_cs *ring,
unsigned entry,
@@ -490,7 +540,7 @@ static int gen8_mm_switch(struct i915_hw_ppgtt *ppgtt,
{
int i, ret;
- for (i = GEN8_LEGACY_PDPES - 1; i >= 0; i--) {
+ for (i = 3; i >= 0; i--) {
struct i915_page_directory_entry *pd =
ppgtt->pdp.page_directory[i];
dma_addr_t pd_daddr = pd ? pd->daddr : ppgtt->scratch_pd->daddr;
/* The page directory might be NULL, but we need to clear out
@@ -579,9 +629,6 @@ static void gen8_ppgtt_insert_entries(struct
i915_address_space *vm,
pt_vaddr = NULL;
for_each_sg_page(pages->sgl, &sg_iter, pages->nents, 0) {
- if (WARN_ON(pdpe >= GEN8_LEGACY_PDPES))
-
[Intel-gfx] [PATCH v6 32/32] drm/i915/bdw: Flip the 48b switch
Use 48b addresses if hw supports it and i915.enable_ppgtt=3.
Aliasing PPGTT remains 32b only.
Signed-off-by: Michel Thierry
---
drivers/gpu/drm/i915/i915_gem_gtt.c | 7 ++-
drivers/gpu/drm/i915/i915_params.c | 2 +-
2 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 842ce93..fda5907 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -107,7 +107,7 @@ static int sanitize_enable_ppgtt(struct drm_device *dev,
int enable_ppgtt)
#ifdef CONFIG_64BIT
has_full_64bit_ppgtt = IS_BROADWELL(dev) ||
- INTEL_INFO(dev)->gen >= 9 && false; /* FIXME:
64b */
+ INTEL_INFO(dev)->gen >= 9;
#else
has_full_64bit_ppgtt = false;
#endif
@@ -1076,9 +1076,6 @@ static int gen8_ppgtt_alloc_page_directories(struct
i915_address_space *vm,
BUG_ON(!bitmap_empty(new_pds, pdpes));
- /* FIXME: PPGTT container_of won't work for 64b */
- BUG_ON((start + length) > 0x8ULL);
-
gen8_for_each_pdpe(pd, pdp, start, length, temp, pdpe) {
if (pd)
continue;
@@ -1397,7 +1394,7 @@ static int gen8_aliasing_ppgtt_init(struct i915_hw_ppgtt
*ppgtt)
{
struct drm_device *dev = ppgtt->base.dev;
struct drm_i915_private *dev_priv = dev->dev_private;
- struct i915_page_directory_pointer_entry *pdp = &ppgtt->pdp; /* FIXME:
48b */
+ struct i915_page_directory_pointer_entry *pdp = &ppgtt->pdp; /* FIXME:
48b? */
struct i915_page_directory_entry *pd;
uint64_t temp, start = 0, size = dev_priv->gtt.base.total;
uint32_t pdpe;
diff --git a/drivers/gpu/drm/i915/i915_params.c
b/drivers/gpu/drm/i915/i915_params.c
index 44f2262..1cd43b0 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -119,7 +119,7 @@ MODULE_PARM_DESC(enable_hangcheck,
module_param_named_unsafe(enable_ppgtt, i915.enable_ppgtt, int, 0400);
MODULE_PARM_DESC(enable_ppgtt,
"Override PPGTT usage. "
- "(-1=auto [default], 0=disabled, 1=aliasing, 2=full)");
+ "(-1=auto [default], 0=disabled, 1=aliasing, 2=full, 3=full_64b)");
module_param_named(enable_execlists, i915.enable_execlists, int, 0400);
MODULE_PARM_DESC(enable_execlists,
--
2.1.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/7] drm/i915/skl: Allow scanning out Y and Yf fbs
On Mon, Feb 23, 2015 at 03:55:56PM +, Tvrtko Ursulin wrote:
> From: Damien Lespiau
>
> Skylake is able to scannout those tiling formats. We need to allow them
> in the ADDFB ioctl and tell the harware about it.
>
> v2: Rebased for addfb2 interface. (Tvrtko Ursulin)
> v3: Rebased for fb modifier changes. (Tvrtko Ursulin)
> v4: Don't allow Y tiled fbs just yet. (Tvrtko Ursulin)
> v5: Check for stride alignment and max pitch. (Tvrtko Ursulin)
> v6: Simplify maximum pitch check. (Ville Syrjälä)
> v7: Drop the gen9 check since requirements are no different. (Ville Syrjälä)
>
> Signed-off-by: Damien Lespiau
> Signed-off-by: Tvrtko Ursulin
> Cc: Ville Syrjälä
While Tvrtko had the courtesy of keeping me as the original author, it's
really two authors, so the r-b has some meaning. Anyway:
Reviewed-by: Damien Lespiau
--
Damien
> ---
> drivers/gpu/drm/i915/intel_display.c | 115
> ---
> drivers/gpu/drm/i915/intel_drv.h | 2 +
> drivers/gpu/drm/i915/intel_sprite.c | 18 --
> 3 files changed, 95 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 6e70748..a523d84 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2728,6 +2728,34 @@ static void ironlake_update_primary_plane(struct
> drm_crtc *crtc,
> POSTING_READ(reg);
> }
>
> +u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier,
> + uint32_t pixel_format)
> +{
> + u32 bits_per_pixel = drm_format_plane_cpp(pixel_format, 0) * 8;
> +
> + /*
> + * The stride is either expressed as a multiple of 64 bytes
> + * chunks for linear buffers or in number of tiles for tiled
> + * buffers.
> + */
> + switch (fb_modifier) {
> + case DRM_FORMAT_MOD_NONE:
> + return 64;
> + case I915_FORMAT_MOD_X_TILED:
> + return 512;
> + case I915_FORMAT_MOD_Y_TILED:
> + return 128;
> + case I915_FORMAT_MOD_Yf_TILED:
> + if (bits_per_pixel == 8)
> + return 64;
> + else
> + return 128;
> + default:
> + MISSING_CASE(fb_modifier);
> + return 64;
> + }
> +}
> +
> static void skylake_update_primary_plane(struct drm_crtc *crtc,
>struct drm_framebuffer *fb,
>int x, int y)
> @@ -2735,10 +2763,9 @@ static void skylake_update_primary_plane(struct
> drm_crtc *crtc,
> struct drm_device *dev = crtc->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - struct intel_framebuffer *intel_fb;
> struct drm_i915_gem_object *obj;
> int pipe = intel_crtc->pipe;
> - u32 plane_ctl, stride;
> + u32 plane_ctl, stride_div;
>
> if (!intel_crtc->primary_enabled) {
> I915_WRITE(PLANE_CTL(pipe, 0), 0);
> @@ -2782,29 +2809,30 @@ static void skylake_update_primary_plane(struct
> drm_crtc *crtc,
> BUG();
> }
>
> - intel_fb = to_intel_framebuffer(fb);
> - obj = intel_fb->obj;
> -
> - /*
> - * The stride is either expressed as a multiple of 64 bytes chunks for
> - * linear buffers or in number of tiles for tiled buffers.
> - */
> switch (fb->modifier[0]) {
> case DRM_FORMAT_MOD_NONE:
> - stride = fb->pitches[0] >> 6;
> break;
> case I915_FORMAT_MOD_X_TILED:
> plane_ctl |= PLANE_CTL_TILED_X;
> - stride = fb->pitches[0] >> 9;
> + break;
> + case I915_FORMAT_MOD_Y_TILED:
> + plane_ctl |= PLANE_CTL_TILED_Y;
> + break;
> + case I915_FORMAT_MOD_Yf_TILED:
> + plane_ctl |= PLANE_CTL_TILED_YF;
> break;
> default:
> - BUG();
> + MISSING_CASE(fb->modifier[0]);
> }
>
> plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE;
> if (crtc->primary->state->rotation == BIT(DRM_ROTATE_180))
> plane_ctl |= PLANE_CTL_ROTATE_180;
>
> + obj = intel_fb_obj(fb);
> + stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
> +fb->pixel_format);
> +
> I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
>
> DRM_DEBUG_KMS("Writing base %08lX %d,%d,%d,%d pitch=%d\n",
> @@ -2817,7 +2845,7 @@ static void skylake_update_primary_plane(struct
> drm_crtc *crtc,
> I915_WRITE(PLANE_SIZE(pipe, 0),
> (intel_crtc->config->pipe_src_h - 1) << 16 |
> (intel_crtc->config->pipe_src_w - 1));
> - I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
> + I915_WRITE(PLANE_STRIDE(pipe, 0), fb->pitches[0] / stride_div);
> I915_WRITE(PLANE_SURF(pipe, 0), i915_gem_obj_ggtt_offset(obj));
>
> POSTING_R
Re: [Intel-gfx] Hibernation test
On ke, 2015-02-18 at 18:43 +0200, Imre Deak wrote: > On ke, 2015-02-11 at 16:46 +0200, David Weinehall wrote: > > intel-gpu-tools currently has a bunch of tests for suspend, > > but currently none (that I could find) for hibernate. > > > > Attached is a rudimentary patch to add said test. It does so > > by repurposing the drv_suspend driver to handle both suspend > > and hibernate, since the difference is miniscule. > > > > I decided to split the suspend/autoresume functions in > > igt_aux.c though, to be able to leave the igt_system_uspend_autoresume() > > function unchanged (the other option would be to > > introduce a boolean function argument and have that > > decide what parameters to pass to rtcwake). > > > > The timeout passed to rtcwake probably needs tuning (it might > > even need to be dynamically adjusted, since the time hibernation takes > > varies wildly depending on the amount of non-cache memory in use). > > I think in general we should try to keep the existing subtest names, but > I couldn't find any concrete problem changing them in this case. The > patch looks ok to me (please inline the patch next time): > Reviewed-by: Imre Deak Thanks for the patch, I pushed it to igt. --Imre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/7] drm/i915/skl: Adjust intel_fb_align_height() for Yb/Yf tiling
On Mon, Feb 23, 2015 at 03:55:57PM +, Tvrtko Ursulin wrote:
> From: Damien Lespiau
>
> We now need the bpp of the fb as Yf tiling has different tile widths
> depending on it.
>
> v2: Rebased for the new addfb2 interface. (Tvrtko Ursulin)
> v3: Rebased for fb modifier changes. (Tvrtko Ursulin)
>
> Signed-off-by: Damien Lespiau
> Signed-off-by: Tvrtko Ursulin
> Reviewed-by: Tvrtko Ursulin
Might want to add a MISSING_CASE() here as well. For the record, the
vertical alignments for Yf are taken from a dodgy looking document found
by chance.
We don't have a 128bpp format supported by display
Now that I think about it, for the horizontal stride, BSpec says:
- 8 bpp -> 64 bytes
- * bpp -> 128 bytes
Given that a tile is a page size this would give for the vertical
alignment:
- 8 bpp -> 64 byes (that's what we have)
- * -> 32 bytes
So the 64bpp cases look a bit suspicious. Given that one the goals for
this new tiling format is to have tiles as square as possible (in terms
of pixels, not byte size), it'd make sense to have different strides
constraints for the 64bpp format, so it could be BSpec being wrong on
the horizontal stride constraint for 64bpp?
--
Damien
> ---
> drivers/gpu/drm/i915/intel_display.c | 31 +--
> 1 file changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index a523d84..4f0033a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2195,9 +2195,36 @@ intel_fb_align_height(struct drm_device *dev, int
> height,
> uint64_t fb_format_modifier)
> {
> int tile_height;
> + uint32_t bits_per_pixel;
>
> - tile_height = fb_format_modifier == I915_FORMAT_MOD_X_TILED ?
> - (IS_GEN2(dev) ? 16 : 8) : 1;
> + switch (fb_format_modifier) {
> + case I915_FORMAT_MOD_X_TILED:
> + tile_height = IS_GEN2(dev) ? 16 : 8;
> + break;
> + case I915_FORMAT_MOD_Y_TILED:
> + tile_height = 32;
> + break;
> + case I915_FORMAT_MOD_Yf_TILED:
> + bits_per_pixel = drm_format_plane_cpp(pixel_format, 0) * 8;
> + switch (bits_per_pixel) {
> + default:
> + case 8:
> + tile_height = 64;
> + break;
> + case 16:
> + case 32:
> + tile_height = 32;
> + break;
> + case 64:
> + case 128:
> + tile_height = 16;
> + break;
> + }
> + break;
> + default:
> + tile_height = 1;
> + break;
> + }
>
> return ALIGN(height, tile_height);
> }
> --
> 2.3.0
>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 5/7] drm/i915/skl: Adjust get_plane_config() to support Yb/Yf tiling
On Mon, Feb 23, 2015 at 03:55:59PM +, Tvrtko Ursulin wrote:
> From: Damien Lespiau
>
> v2: Rebased for addfb2 interface and consolidated a bit. (Tvrtko Ursulin)
> v3: Rebased for fb modifier changes. (Tvrtko Ursulin)
>
> Signed-off-by: Damien Lespiau
> Signed-off-by: Tvrtko Ursulin
> Reviewed-by: Tvrtko Ursulin
> ---
This patch looks like it could reuse the newly introduced
intel_fb_stride_alignment(). Otherwise:
Reviewed-by: Damien Lespiau
> drivers/gpu/drm/i915/intel_display.c | 45
> ++--
> 1 file changed, 28 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 358a97e..c622b11 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7718,7 +7718,7 @@ skylake_get_initial_plane_config(struct intel_crtc
> *crtc,
> {
> struct drm_device *dev = crtc->base.dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> - u32 val, base, offset, stride_mult;
> + u32 val, base, offset, stride_mult, tiling;
> int pipe = crtc->pipe;
> int fourcc, pixel_format;
> int aligned_height;
> @@ -7737,11 +7737,6 @@ skylake_get_initial_plane_config(struct intel_crtc
> *crtc,
> if (!(val & PLANE_CTL_ENABLE))
> goto error;
>
> - if (val & PLANE_CTL_TILED_MASK) {
> - plane_config->tiling = I915_TILING_X;
> - fb->modifier[0] = I915_FORMAT_MOD_X_TILED;
> - }
> -
> pixel_format = val & PLANE_CTL_FORMAT_MASK;
> fourcc = skl_format_to_fourcc(pixel_format,
> val & PLANE_CTL_ORDER_RGBX,
> @@ -7749,6 +7744,33 @@ skylake_get_initial_plane_config(struct intel_crtc
> *crtc,
> fb->pixel_format = fourcc;
> fb->bits_per_pixel = drm_format_plane_cpp(fourcc, 0) * 8;
>
> + tiling = val & PLANE_CTL_TILED_MASK;
> + switch (tiling) {
> + case PLANE_CTL_TILED_LINEAR:
> + fb->modifier[0] = DRM_FORMAT_MOD_NONE;
> + stride_mult = 64;
> + break;
> + case PLANE_CTL_TILED_X:
> + plane_config->tiling = I915_TILING_X;
> + fb->modifier[0] = I915_FORMAT_MOD_X_TILED;
> + stride_mult = 512;
> + break;
> + case PLANE_CTL_TILED_Y:
> + fb->modifier[0] = I915_FORMAT_MOD_Y_TILED;
> + stride_mult = 128;
> + break;
> + case PLANE_CTL_TILED_YF:
> + fb->modifier[0] = I915_FORMAT_MOD_Yf_TILED;
> + if (fb->bits_per_pixel == 8)
> + stride_mult = 64;
> + else
> + stride_mult = 128;
> + break;
> + default:
> + MISSING_CASE(tiling);
> + goto error;
> + }
> +
> base = I915_READ(PLANE_SURF(pipe, 0)) & 0xf000;
> plane_config->base = base;
>
> @@ -7759,17 +7781,6 @@ skylake_get_initial_plane_config(struct intel_crtc
> *crtc,
> fb->width = ((val >> 0) & 0x1fff) + 1;
>
> val = I915_READ(PLANE_STRIDE(pipe, 0));
> - switch (plane_config->tiling) {
> - case I915_TILING_NONE:
> - stride_mult = 64;
> - break;
> - case I915_TILING_X:
> - stride_mult = 512;
> - break;
> - default:
> - MISSING_CASE(plane_config->tiling);
> - goto error;
> - }
> fb->pitches[0] = (val & 0x3ff) * stride_mult;
>
> aligned_height = intel_fb_align_height(dev, fb->height,
> --
> 2.3.0
>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Fix frontbuffer false positve.
On Mon, Feb 23, 2015 at 6:13 PM, Matt Roper wrote: > On Mon, Feb 23, 2015 at 05:52:24PM -0800, Rodrigo Vivi wrote: >> Hi Daniel, >> >> It seems that one check with one good commit followed by many zeroed >> intel_crtc->atomic commits is again in place. > > Can you elaborate on what you mean by this? With atomic it's possible > to have a check with no commit after it (if the check or prepare fail, > or if it's a 'test only' operation), but if you're seeing commits > without corresponding checks, that sounds like a bug. Ah so yes, this is the bug, when moving a cursor we have many commits without checks. So it is only one check followed by many commits... so it commits with intel_crtc->atomic all zeroed by a previous finish_commit. > > Can you provide a dmesg with drm.debug turned on so we can see what's > going on? Or add some dump_stack()'s so that we can see the backtrace > that led us to this situation. I lost those logs, but if you put one print in check and one in commit you also should be able to see that since I heard about this false positive from many people. > > Actually, I wonder if the problem is actually the opposite of what you > say above. Now that I look at this again, we only zero out > intel_crtc->atomic in intel_finish_crtc_commit which is part of the > commit path. From what I heard and what I saw on the logs I thought it was ok to have 1 check than as many commits as possible without the check. If that was the case we would have a fail that is to zero the structure on finish_commit. But seems this isn't the case. Sorry. > So if you had a check that never got a corresponding > commit, there might still be garbage left over in there. No it is the opposite. Commits with no check. causing commit of intel_crtc->atomic zeroed. > Ultimately we > should be handling all of this with real intel_crtc_state handling which > we don't quite have yet. Maybe in the meantime we need to be clearing > out intel_crtc->atomic at the beginning of a top-level atomic > transaction? I'll send a patch that makes this change for you to try > shortly. Thanks! I thought about this but got afraid that clearing this on top of check could cause a race since one plane doing a check could clean the atomic being commited on cursor movement, unless we hat that for planes, but then we would have to iterate over all planes on finish_commit. We can also put this patch that fix the only known issue so far with a FIXME comment while we don't have the final fix. > > > Matt > >> >> So I'm getting that annoying false positives on latest -nightly. >> >> Shouldn't we just merge this patch while atomic modeset design doesn't >> get fixed at all? >> >> Unfortunately nothing comes to my mind than moving all >> intel_crtc->atomic set to commit time and let pre_commit just with >> pm_get... >> Ohter than that just a full redesign of atomic modeset. >> >> Thanks, >> Rodrigo. >> >> >> On Fri, Feb 13, 2015 at 12:48 AM, Daniel Vetter wrote: >> > On Thu, Feb 12, 2015 at 05:17:04PM -0800, Rodrigo Vivi wrote: >> >> No, we had solved old frontbuffer false positives... some missing >> >> flush somewhere at that time... >> >> >> >> So, I added a bunch of printk and I insist that it is conceptually >> >> wrong to set intel_crtc_atomic_commit on check times when you do >> >> memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic)); >> >> on every finish_commit. >> >> >> >> With exception of atomic.disabled_planes I believe the rest shouldn't >> >> work in the way it is implemented because you can have one check >> >> followed by many commits, but after the first commit all atomic >> >> variables are zeroed, except the disabled_planes that is set outside >> >> check... >> > >> > Ok here's the trouble: Every commit should have at exactly one check for >> > the new state objects. Unfortunately in the transition that seems to have >> > been lost for some cases. >> > >> >> For instance: on every cursor movement atomic.fb_bits was 0x000 when >> >> it should be 0x002. This is why this patch solved the false positive, >> >> i.e. setting it on commit instead on check time we get it propperly >> >> set. One of the problems is the false positive but also it breaks >> >> entirely SW tracking on VLV/CHV >> >> >> >> I believe wait_for flips, update_fbc, watermarks, etc should keep the >> >> value got on check for the commit or the check should be done at >> >> commit plane instead of on check. >> >> >> >> I started doing a patch here to move all atomic sets from check to >> >> commit functions but gave up on middle when I noticed the >> >> prepare_commit would almost get empty... >> > >> > All state precomputation must be done in check, at commit time you have a >> > lot less information since the old state is somewhat gone. You can still >> > get at it, but as soon as we add an async flip queue that will get really >> > ugly. The current placement is imo the correct one. Instead we need to >> > figure out where we're doing a ->commit wit
Re: [Intel-gfx] [PATCH 2/7] drm/i915/skl: Allow scanning out Y and Yf fbs
On Mon, Feb 23, 2015 at 03:55:56PM +, Tvrtko Ursulin wrote:
> From: Damien Lespiau
>
> Skylake is able to scannout those tiling formats. We need to allow them
> in the ADDFB ioctl and tell the harware about it.
>
> v2: Rebased for addfb2 interface. (Tvrtko Ursulin)
> v3: Rebased for fb modifier changes. (Tvrtko Ursulin)
> v4: Don't allow Y tiled fbs just yet. (Tvrtko Ursulin)
> v5: Check for stride alignment and max pitch. (Tvrtko Ursulin)
> v6: Simplify maximum pitch check. (Ville Syrjälä)
> v7: Drop the gen9 check since requirements are no different. (Ville Syrjälä)
>
> Signed-off-by: Damien Lespiau
> Signed-off-by: Tvrtko Ursulin
> Cc: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/intel_display.c | 115
> ---
> drivers/gpu/drm/i915/intel_drv.h | 2 +
> drivers/gpu/drm/i915/intel_sprite.c | 18 --
> 3 files changed, 95 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 6e70748..a523d84 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2728,6 +2728,34 @@ static void ironlake_update_primary_plane(struct
> drm_crtc *crtc,
> POSTING_READ(reg);
> }
>
> +u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier,
> + uint32_t pixel_format)
> +{
> + u32 bits_per_pixel = drm_format_plane_cpp(pixel_format, 0) * 8;
> +
> + /*
> + * The stride is either expressed as a multiple of 64 bytes
> + * chunks for linear buffers or in number of tiles for tiled
> + * buffers.
> + */
> + switch (fb_modifier) {
> + case DRM_FORMAT_MOD_NONE:
> + return 64;
> + case I915_FORMAT_MOD_X_TILED:
if (gen2)
return 128;
> + return 512;
> + case I915_FORMAT_MOD_Y_TILED:
> + return 128;
In theory we could check gen2 and HAS_128_BYTE_Y_TILING() here, but
since old platforms didn't do Y tiled scanout anyway that's not really
needed. But maybe toss in a comment about that so that people don't
start wondering why this doesn't care about that stuff?
> + case I915_FORMAT_MOD_Yf_TILED:
> + if (bits_per_pixel == 8)
> + return 64;
> + else
> + return 128;
> + default:
> + MISSING_CASE(fb_modifier);
> + return 64;
> + }
> +}
> +
> static void skylake_update_primary_plane(struct drm_crtc *crtc,
>struct drm_framebuffer *fb,
>int x, int y)
> @@ -2735,10 +2763,9 @@ static void skylake_update_primary_plane(struct
> drm_crtc *crtc,
> struct drm_device *dev = crtc->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - struct intel_framebuffer *intel_fb;
> struct drm_i915_gem_object *obj;
> int pipe = intel_crtc->pipe;
> - u32 plane_ctl, stride;
> + u32 plane_ctl, stride_div;
>
> if (!intel_crtc->primary_enabled) {
> I915_WRITE(PLANE_CTL(pipe, 0), 0);
> @@ -2782,29 +2809,30 @@ static void skylake_update_primary_plane(struct
> drm_crtc *crtc,
> BUG();
> }
>
> - intel_fb = to_intel_framebuffer(fb);
> - obj = intel_fb->obj;
> -
> - /*
> - * The stride is either expressed as a multiple of 64 bytes chunks for
> - * linear buffers or in number of tiles for tiled buffers.
> - */
> switch (fb->modifier[0]) {
> case DRM_FORMAT_MOD_NONE:
> - stride = fb->pitches[0] >> 6;
> break;
> case I915_FORMAT_MOD_X_TILED:
> plane_ctl |= PLANE_CTL_TILED_X;
> - stride = fb->pitches[0] >> 9;
> + break;
> + case I915_FORMAT_MOD_Y_TILED:
> + plane_ctl |= PLANE_CTL_TILED_Y;
> + break;
> + case I915_FORMAT_MOD_Yf_TILED:
> + plane_ctl |= PLANE_CTL_TILED_YF;
> break;
> default:
> - BUG();
> + MISSING_CASE(fb->modifier[0]);
> }
>
> plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE;
> if (crtc->primary->state->rotation == BIT(DRM_ROTATE_180))
> plane_ctl |= PLANE_CTL_ROTATE_180;
>
> + obj = intel_fb_obj(fb);
> + stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
> +fb->pixel_format);
> +
> I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
>
> DRM_DEBUG_KMS("Writing base %08lX %d,%d,%d,%d pitch=%d\n",
> @@ -2817,7 +2845,7 @@ static void skylake_update_primary_plane(struct
> drm_crtc *crtc,
> I915_WRITE(PLANE_SIZE(pipe, 0),
> (intel_crtc->config->pipe_src_h - 1) << 16 |
> (intel_crtc->config->pipe_src_w - 1));
> - I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
> + I915_WRITE(PLANE_STRIDE(pipe, 0),
Re: [Intel-gfx] [RFC 01/12] drm/i915/config: Initial framework
On Tue, 24 Feb 2015 17:17:18 +0100 Daniel Vetter wrote: > On Thu, Feb 12, 2015 at 03:41:27PM -0800, Bob Paauwe wrote: > > This adds an init-time configuration framework that parses configuration > > data from an ACPI property table. The table is assumed to have well > > defined sub-device property tables that correspond to the various > > driver components. Initially the following sub-device tables are > > defined: > > > > CRTC (CRTC) > >The CRTC sub-device table contains additional sub-device tables > >where each one corresponds to a CRTC. Each CRTC sub-device must > >include a property called "id" whose value matches the driver's > >crtc id. Additional properties for the CRTC are used to configure > >the crtc. > > > > Connector (CNCT) > >The CNCT sub-device table contains additional sub-device tables > >where each one corresponds to a connector. Each of the connector > >sub-device tables must include a property called "name" whose value > >matches a connector name assigned by the driver (see later patch > >for output name function). Additional connector properties can > >be set through these tables. > > > > Plane (PLNS) > >The PLNS sub-device table contains additional sub-device tables > >where each one corresponds to a plane. [this needs additional work] > > > > In addition, the main device property table for the device may > > contain configuration information that applies to general driver > > configuration. > > > > The framework includes a couple of helper functions to access the > > configuration data. > > > >intel_config_get_integer() will look up a configuration property > >and return the integer value associated with it. > > > >intel_config_init__property() will look up a > >configuration property and assign the value to a drm > >property of the same name. These functions are used to > >initialize drm property instances to specific values. > > > > Signed-off-by: Bob Paauwe > > Like Jani I'm mostly concerned about maintaining yet another slightly > different definition of paramters and settings here - we already have (or > will soon grow) per crtc/plane/connector properties for atomic. For drm properties, the idea was to expand the existing hard coded default values with configurable defaults. For the pre-atomic connector properties, when the property instance was created, they were created with a specific default value. I haven't looked closely at the atomic property sets yet to see if this interface also makes sense with them, but that is on my near-term todo list. Most of this code had been developed prior to the atomic patches. > > My high-level idea for this was that we reuse the atomic support (once we > have it) and just take the acpi tables as a slightly different source for > an atomic update. Since the design of a name/value pair for objects is > very similar to what we do with the atomic ioctl this hopefully should map > fairly well. Yes, I believe it will map easily also, and plan to look at this next. > > Internally in the driver the only work needed to do would be to add any > missing properties. All the acpi atomic update code should be fully i915 > agnostic and only use generic atomic update interfaces. ACPI isn't really driver agnostic since it's only available to IA based drivers. So I'm not sure I follow your thinking here. Maybe it will make more sense to me as I look at the atomic properties. I needed to get some fresh eyes looking at this which is why I posted it. I appreciate the feedback and will be incorporating it into the next version. > > Would this work or do I overlook something important here. > -Daniel > > > --- > > drivers/gpu/drm/i915/Makefile | 3 +- > > drivers/gpu/drm/i915/i915_dma.c | 4 + > > drivers/gpu/drm/i915/i915_drv.h | 16 ++ > > drivers/gpu/drm/i915/i915_params.c | 6 + > > drivers/gpu/drm/i915/intel_config.c | 542 > > > > drivers/gpu/drm/i915/intel_drv.h| 28 ++ > > 6 files changed, 598 insertions(+), 1 deletion(-) > > create mode 100644 drivers/gpu/drm/i915/intel_config.c > > > > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile > > index f025e7f..462de19 100644 > > --- a/drivers/gpu/drm/i915/Makefile > > +++ b/drivers/gpu/drm/i915/Makefile > > @@ -12,7 +12,8 @@ i915-y := i915_drv.o \ > >i915_suspend.o \ > > i915_sysfs.o \ > > intel_pm.o \ > > - intel_runtime_pm.o > > + intel_runtime_pm.o \ > > + intel_config.o > > > > i915-$(CONFIG_COMPAT) += i915_ioc32.o > > i915-$(CONFIG_DEBUG_FS) += i915_debugfs.o > > diff --git a/drivers/gpu/drm/i915/i915_dma.c > > b/drivers/gpu/drm/i915/i915_dma.c > > index 5804aa5..9501360 100644 > > --- a/drivers/gpu/drm/i915/i915_dma.c > > +++ b/drivers/gpu/drm/i915/i915_dma.c > > @@ -656,6 +656,8 @@ int i915_driver_load(struct drm_device *dev, unsigned > > long flags) > > de
Re: [Intel-gfx] [PATCH] drm/i915: Clear crtc atomic flags at beginning of transaction
It doesn't fix.
Let's me grab some new logs...
On Tue, Feb 24, 2015 at 9:43 AM, Rodrigo Vivi wrote:
> Hi Matt,
>
> It probably doesn't fix because the issue is to have many commits
> without a check, having many commits with intel_crtc->atomic zeroed
> already.
> imho cleaning it more wouldn't solve it... but anyway, let me try.
>
> Thanks,
> Rodrigo.
>
> On Mon, Feb 23, 2015 at 6:14 PM, Matt Roper wrote:
>> Once we have full atomic modeset, these kind of flags should be in a
>> real intel_crtc_state that's tracked properly. In the meantime, make
>> sure we clear out any old flags at the beginning of a transaction so
>> that we don't wind up seeing leftover flags from old transactions that
>> were checked, but never went to the commit step.
>>
>> Cc: Rodrigo Vivi
>> Signed-off-by: Matt Roper
>> ---
>> Rodrigo, does this solve the frontbuffer problems you're seeing? I haven't
>> had
>> time to test this myself, but I think this will fix at least one problem that
>> may or may not have been the source of your troubles.
>>
>> drivers/gpu/drm/i915/intel_atomic.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
>> b/drivers/gpu/drm/i915/intel_atomic.c
>> index 011b896..fe51e23 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>> @@ -76,6 +76,8 @@ int intel_atomic_check(struct drm_device *dev,
>> state->allow_modeset = false;
>> for (i = 0; i < ncrtcs; i++) {
>> struct intel_crtc *crtc = to_intel_crtc(state->crtcs[i]);
>> + if (crtc)
>> + memset(&crtc->atomic, 0, sizeof(crtc->atomic));
>> if (crtc && crtc->pipe != nuclear_pipe)
>> not_nuclear = true;
>> }
>> --
>> 1.8.5.1
>>
>
>
>
> --
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 07/12] drm/i915/config: Get workaround information from configuration.
On Tue, 24 Feb 2015 14:51:31 +0100
Daniel Vetter wrote:
> On Thu, Feb 12, 2015 at 03:41:33PM -0800, Bob Paauwe wrote:
> > Add ability to parse a list of workarounds from the ACPI table.
> > Initially, this expects all workarounds listed to be valid and
> > they replace the hard coded list initialized in init_workarounds_ring().
> >
> > The main benefit of this is the ability to add/remove workarounds
> > at runtime. It may also be useful to "adjust" the workaround
> > list for a new GPU prior to fixed support being available in the
> > driver.
> >
> > Signed-off-by: Bob Paauwe
>
> This one here freaks me out. We /should/ be fast enough with rolling out
> workarounds through backports to stable kernels and product trees that
> this shouldn't ever matter. And for a lot of workarounds this is too
> limited since we also need to emit some of them to the ring on each engine
> init or sometimes even context load.
>
> As-is it looks a lot more like a backdoor to the driver to enable some
> features no one wants to talk about in public ;-)
> -Daniel
Yeah, I don't know that we have any valid use-case for this. It
really is just an idea. Maybe a bad idea. It was something that looked
like it could map into the configuration space so I coded it up.
I did think it might be useful if we could disable workarounds for early
steppings that may not be applicable for later steppings without the
need to release a new version of the driver. As it sits, it doesn't
support this use-case so would need a bit more work. For some embedded
appliance type devices, customers are sometimes more willing to take a
configuration change than a new driver.
The patches for this should be easy to drop without effecting anything
else.
>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 1 +
> > drivers/gpu/drm/i915/intel_config.c | 101
> > +++-
> > drivers/gpu/drm/i915/intel_drv.h| 4 +-
> > 3 files changed, 104 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 165091c..1580702 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1656,6 +1656,7 @@ struct intel_config_info {
> > struct list_head crtc_list;
> > struct list_head connector_list;
> > struct list_head plane_list;
> > + struct list_head wa_list;
> > };
> >
> >
> > diff --git a/drivers/gpu/drm/i915/intel_config.c
> > b/drivers/gpu/drm/i915/intel_config.c
> > index fb495ed..ab14928 100644
> > --- a/drivers/gpu/drm/i915/intel_config.c
> > +++ b/drivers/gpu/drm/i915/intel_config.c
> > @@ -214,6 +214,7 @@ void intel_config_init(struct drm_device *dev)
> > INIT_LIST_HEAD(&info->crtc_list);
> > INIT_LIST_HEAD(&info->connector_list);
> > INIT_LIST_HEAD(&info->plane_list);
> > + INIT_LIST_HEAD(&info->wa_list);
> >
> > handle = ACPI_HANDLE(dev->dev);
> > if (!handle) {
> > @@ -239,6 +240,7 @@ void intel_config_init(struct drm_device *dev)
> > #define i915_COMPONENT_CRTC "CRTC"
> > #define i915_COMPONENT_CONNECTOR "CNCT"
> > #define i915_COMPONENT_PLANE "PLNS"
> > +#define i915_COMPONENT_WORKAROUND "WRKS"
> >
> > /* Build lists */
> > list_for_each_entry(component, &info->base.adev->children, node) {
> > @@ -262,6 +264,9 @@ void intel_config_init(struct drm_device *dev)
> > if (!alloc_new_node(cl, &info->crtc_list))
> > goto bail;
> > }
> > + } else if (strcmp(cname, i915_COMPONENT_WORKAROUND) == 0) {
> > + if (!alloc_new_node(cl, &info->wa_list))
> > + goto bail;
> > }
> > }
> >
> > @@ -277,6 +282,8 @@ bail:
> > kfree(new_node);
> > list_for_each_entry_safe(new_node, tmp, &info->plane_list, node)
> > kfree(new_node);
> > + list_for_each_entry_safe(new_node, tmp, &info->wa_list, node)
> > + kfree(new_node);
> >
> > kfree(info);
> > dev_priv->config_info = NULL;
> > @@ -309,6 +316,8 @@ void intel_config_shutdown(struct drm_device *dev)
> > kfree(n);
> > list_for_each_entry_safe(n, tmp, &info->plane_list, node)
> > kfree(n);
> > + list_for_each_entry_safe(n, tmp, &info->wa_list, node)
> > + kfree(n);
> >
> > /* Unload any dynamically loaded ACPI property table */
> > handle = ACPI_HANDLE(dev->dev);
> > @@ -384,6 +393,8 @@ static struct list_head *cfg_type_to_list(struct
> > intel_config_info *info,
> > return &info->connector_list;
> > case CFG_PLANE:
> > return &info->plane_list;
> > + case CFG_WORKAROUND:
> > + return &info->wa_list;
> > }
> > return NULL;
> > }
> > @@ -404,7 +415,6 @@ bool intel_config_get_integer(struct drm_i915_private
> > *dev_priv,
> > struct intel_config_info *info = dev_priv->config_info;
> > struct intel_config_node *n;
Re: [Intel-gfx] [PATCH] drm/i915: Fix frontbuffer false positve.
On Tue, Feb 24, 2015 at 09:32:25AM -0800, Rodrigo Vivi wrote: > On Mon, Feb 23, 2015 at 6:13 PM, Matt Roper wrote: > > On Mon, Feb 23, 2015 at 05:52:24PM -0800, Rodrigo Vivi wrote: > >> Hi Daniel, > >> > >> It seems that one check with one good commit followed by many zeroed > >> intel_crtc->atomic commits is again in place. > > > > Can you elaborate on what you mean by this? With atomic it's possible > > to have a check with no commit after it (if the check or prepare fail, > > or if it's a 'test only' operation), but if you're seeing commits > > without corresponding checks, that sounds like a bug. > > Ah so yes, this is the bug, when moving a cursor we have many commits > without checks. So it is only one check followed by many commits... so > it commits with intel_crtc->atomic all zeroed by a previous > finish_commit. > > > > > Can you provide a dmesg with drm.debug turned on so we can see what's > > going on? Or add some dump_stack()'s so that we can see the backtrace > > that led us to this situation. > > I lost those logs, but if you put one print in check and one in commit > you also should be able to see that since I heard about this false > positive from many people. Hmm. Are all of these people using a specific platform? I only have access to IVB hardware at the moment, so it could be that the problem lies on codepaths I just don't exercise on my system. Most of the atomic stuff is pretty platform-agnostic, but maybe there's something I overlooked. > > > > Actually, I wonder if the problem is actually the opposite of what you > > say above. Now that I look at this again, we only zero out > > intel_crtc->atomic in intel_finish_crtc_commit which is part of the > > commit path. > > From what I heard and what I saw on the logs I thought it was ok to > have 1 check than as many commits as possible without the check. > If that was the case we would have a fail that is to zero the > structure on finish_commit. But seems this isn't the case. Sorry. I guess I should clarify terminology a little bit since this gets kind of confusing... For atomic, we have a top-level atomic_state structure that contains everything that we want to update as part of an atomic transaction (multiple planes, crtc's, etc.). At the moment, we only support a subset of atomic, so you'll only ever get one crtc being modified, but there could be multiple planes (primary, cursor, 1 or more sprite planes). There's a sub-state structure (plane_state, crtc_state, etc.) for each DRM object being updated. At the top level of atomic handling we do "check" and then "commit" with the top-level atomic state. If you trace down through the codeflow, the top-level check here will actually call a plane-specific check function on each plane state during the top-level check, and then the top-level commit function will call the plane-specific commit function on each plane involved. So when I say "1 or more checks, followed by exactly one commit" I'm referring to the top-level check that deals with the top-level atomic state. If you're looking at the plane state specifically, you might see more of an n:n relationship if your transaction is updating multiple planes together (one check call per plane being updated, possibly followed by one commit call for each of them), but the clearing of intel_crtc->atomic should still happen at the end, after all of the individual planes have been committed. > > So if you had a check that never got a corresponding > > commit, there might still be garbage left over in there. > > No it is the opposite. Commits with no check. causing commit of > intel_crtc->atomic zeroed. > > > Ultimately we > > should be handling all of this with real intel_crtc_state handling which > > we don't quite have yet. Maybe in the meantime we need to be clearing > > out intel_crtc->atomic at the beginning of a top-level atomic > > transaction? I'll send a patch that makes this change for you to try > > shortly. > > Thanks! I thought about this but got afraid that clearing this on top > of check could cause a race since one plane doing a check could clean > the atomic being commited on cursor movement, unless we hat that for > planes, but then we would have to iterate over all planes on > finish_commit. > > We can also put this patch that fix the only known issue so far with a > FIXME comment while we don't have the final fix. Well, based on what you say above, it sounds like it probably won't solve your issue, so we'll need to explore some more. I still haven't managed to reproduce this myself, so I'm not sure whether I'm working on a hardware platform that won't trigger the bug, or whether I'm just unlucky. Is there a specific igt test you've been using that triggers this reliably for you? Matt > > > > > > > Matt > > > >> > >> So I'm getting that annoying false positives on latest -nightly. > >> > >> Shouldn't we just merge this patch while atomic modeset design doesn't > >> get fixed at all? > >> > >> Unf
Re: [Intel-gfx] [PATCH v2] drm/i915/skl: Make sure to allocate mininum sizes in the DDB
On Mon, Feb 09, 2015 at 01:35:10PM +, Damien Lespiau wrote:
> I overlooked the fact that we need to allocate a minimum 8 blocks and
> that just allocating the planes depending on how much they need to fetch
> from the DDB in proportion of how much memory bw is necessary for the
> whole display can lead to cases where we don't respect those minima (and
> thus overrun).
>
> So, instead, start by allocating 8 blocks to each active display plane
> and then allocate the remaining blocks like before.
>
> v2: Rebase on top of -nightly
>
> Cc: Mahesh Kumar
> Signed-off-by: Damien Lespiau
OK after reading through the spec the minimum[] thing makes sense, so
Reviewed-by: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/intel_pm.c | 22 ++
> 1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index bebefe7..f6c7e53 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2502,6 +2502,7 @@ skl_allocate_pipe_ddb(struct drm_crtc *crtc,
> enum pipe pipe = intel_crtc->pipe;
> struct skl_ddb_entry *alloc = &ddb->pipe[pipe];
> uint16_t alloc_size, start, cursor_blocks;
> + uint16_t minimum[I915_MAX_PLANES];
> unsigned int total_data_rate;
> int plane;
>
> @@ -2520,9 +2521,21 @@ skl_allocate_pipe_ddb(struct drm_crtc *crtc,
> alloc_size -= cursor_blocks;
> alloc->end -= cursor_blocks;
>
> + /* 1. Allocate the mininum required blocks for each active plane */
> + for_each_plane(pipe, plane) {
> + const struct intel_plane_wm_parameters *p;
> +
> + p = ¶ms->plane[plane];
> + if (!p->enabled)
> + continue;
> +
> + minimum[plane] = 8;
> + alloc_size -= minimum[plane];
> + }
> +
> /*
> - * Each active plane get a portion of the remaining space, in
> - * proportion to the amount of data they need to fetch from memory.
> + * 2. Distribute the remaining space in proportion to the amount of
> + * data each plane needs to fetch from memory.
>*
>* FIXME: we may not allocate every single block here.
>*/
> @@ -2544,8 +2557,9 @@ skl_allocate_pipe_ddb(struct drm_crtc *crtc,
>* promote the expression to 64 bits to avoid overflowing, the
>* result is < available as data_rate / total_data_rate < 1
>*/
> - plane_blocks = div_u64((uint64_t)alloc_size * data_rate,
> -total_data_rate);
> + plane_blocks = minimum[plane];
> + plane_blocks += div_u64((uint64_t)alloc_size * data_rate,
> + total_data_rate);
>
> ddb->plane[pipe][plane].start = start;
> ddb->plane[pipe][plane].end = start + plane_blocks;
> --
> 1.8.3.1
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Fix frontbuffer false positve.
Atomic bits needs to be set when cursor check function is returning 0
and intel_crtc is active.
v2: When putting more debug prints I notice the solution was simpler
than I thought. AMS design is solid, just this return was wrong.
Sorry for the noise.
Cc: Matt Roper
Signed-off-by: Rodrigo Vivi
---
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 8ccf033..5fb83ba 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12198,7 +12198,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
}
if (fb == crtc->cursor->fb)
- return 0;
+ goto finish;
if (fb->modifier[0] != DRM_FORMAT_MOD_NONE) {
DRM_DEBUG_KMS("cursor cannot be tiled\n");
--
2.1.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 09/12] drm/i915/config: Add VBT settings configuration.
On Tue, 24 Feb 2015 14:57:48 +0100
Daniel Vetter wrote:
> On Thu, Feb 12, 2015 at 03:41:35PM -0800, Bob Paauwe wrote:
> > Add a new section with subsections to the ACPI configuration table
> > that mimics much of the information typically stored in the VBT/option
> > ROM. This allows for a way to override incorrect VBT data or to provide
> > the configuration if VBT is not present. Lack of VBT is common in
> > embedded systems.
> >
> > Any data found in the configuration tables will replace the driver's
> > vbt structure.
> >
> > MIPI DSI configuration is not implmemnted at this time.
> >
> > Signed-off-by: Bob Paauwe
>
> I'm not too happy with the duplicate of standards this creates. Is there
> no way to just load a vbt blob into the acpi table somewhere and require
> that it perfectly matches the vbt "standard" for the given platforms,
> warts and all included?
That's actually fairly easy to do. I believe that the opregion code is
just that. So the existing vbt code in the driver does that today. The
acpi property code can't have an opregion embedded as a property, I did
look into that, but you can still create array properties and one of
those could hold a vbt blob. Another alternative would be to have a
property array for each vbt section.
What I was trying for here was a way to override the driver defaults
for specific settings in the case where the platform did not have an
ACPI opregion or boot firmware with a vbt table. I wasn't really
trying to create a mechanism to create an entire vbt table. But the
parsing of the properties does get pretty messy.
>
> As Jani points out we already have vbt headaches, it would be good if we
> only have those once ;-)
> -Daniel
I've been trying to think of a better way but not really coming up with
something that scales well. EMGD took the approach of create
configuration for only those values that we had customer requests for.
And that was primarily just the panel power up/down timing sequence and
some backlight control. We never tried to replicate everything that
could be set via vbt.
I've also been looking at splitting up the existing vbt structure in
some way so that both the exiting vbt code and the configuration code
could be used to set component configuration. Something like
set_crt_ddc_pin() in the intel_crt code that gets called by either
(or both) the vbt parsing code or the configuration framework.
But creating set_ functions for all the existing values in
the vbt structure seems like it might be too much overhead.
Bob
>
> > ---
> > drivers/gpu/drm/i915/i915_dma.c | 2 +
> > drivers/gpu/drm/i915/i915_drv.h | 1 +
> > drivers/gpu/drm/i915/intel_config.c | 334
> >
> > drivers/gpu/drm/i915/intel_drv.h| 4 +-
> > 4 files changed, 340 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c
> > b/drivers/gpu/drm/i915/i915_dma.c
> > index 9501360..9119e9b 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -852,6 +852,8 @@ int i915_driver_load(struct drm_device *dev, unsigned
> > long flags)
> > if (intel_vgpu_active(dev))
> > I915_WRITE(vgtif_reg(display_ready), VGT_DRV_DISPLAY_READY);
> >
> > + intel_config_vbt(dev_priv);
> > +
> > i915_setup_sysfs(dev);
> >
> > if (INTEL_INFO(dev)->num_pipes) {
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 1580702..a60511e 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1657,6 +1657,7 @@ struct intel_config_info {
> > struct list_head connector_list;
> > struct list_head plane_list;
> > struct list_head wa_list;
> > + struct list_head vbt_list;
> > };
> >
> >
> > diff --git a/drivers/gpu/drm/i915/intel_config.c
> > b/drivers/gpu/drm/i915/intel_config.c
> > index ab14928..c1b06b7 100644
> > --- a/drivers/gpu/drm/i915/intel_config.c
> > +++ b/drivers/gpu/drm/i915/intel_config.c
> > @@ -215,6 +215,7 @@ void intel_config_init(struct drm_device *dev)
> > INIT_LIST_HEAD(&info->connector_list);
> > INIT_LIST_HEAD(&info->plane_list);
> > INIT_LIST_HEAD(&info->wa_list);
> > + INIT_LIST_HEAD(&info->vbt_list);
> >
> > handle = ACPI_HANDLE(dev->dev);
> > if (!handle) {
> > @@ -241,6 +242,7 @@ void intel_config_init(struct drm_device *dev)
> > #define i915_COMPONENT_CONNECTOR "CNCT"
> > #define i915_COMPONENT_PLANE "PLNS"
> > #define i915_COMPONENT_WORKAROUND "WRKS"
> > +#define i915_COMPONENT_VBIOSTABLE "VBT"
> >
> > /* Build lists */
> > list_for_each_entry(component, &info->base.adev->children, node) {
> > @@ -267,6 +269,18 @@ void intel_config_init(struct drm_device *dev)
> > } else if (strcmp(cname, i915_COMPONENT_WORKAROUND) == 0) {
> > if (!alloc_new_node(cl, &info->wa_list))
> > goto bail;
> > + } else if (strcmp(
Re: [Intel-gfx] [PATCH 6/7] drm/i915/skl: Update watermarks for Y tiling
On Mon, Feb 23, 2015 at 03:56:00PM +, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin
>
> Display watermarks need different programming for different tiling
> modes.
>
> Set the relevant flag so this happens during the plane commit and
> add relevant data into a structure made available to the watermark
> computation code.
>
> v2: Pass in tiling info to sprite plane updates as well.
> v3: Rebased for plane handling changes.
> v4: Handle fb == NULL when plane is disabled.
> v5: Refactored for addfb2 interface.
> v6: Refactored for fb modifier changes.
> v7: Updated for atomic commit by only updating watermarks when tiling changes.
>
> Signed-off-by: Tvrtko Ursulin
> Acked-by: Ander Conselvan de Oliveira
> Acked-by: Matt Roper
> ---
> drivers/gpu/drm/i915/intel_display.c | 6 ++
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> drivers/gpu/drm/i915/intel_pm.c | 33 -
> drivers/gpu/drm/i915/intel_sprite.c | 6 ++
> 4 files changed, 41 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index c622b11..74d4923 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11985,6 +11985,12 @@ intel_check_primary_plane(struct drm_plane *plane,
> INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
>
> intel_crtc->atomic.update_fbc = true;
> +
> + /* Update watermarks on tiling changes. */
> + if (!plane->state->fb || !state->base.fb ||
> + plane->state->fb->modifier[0] !=
> + state->base.fb->modifier[0])
> + intel_crtc->atomic.update_wm = true;
> }
>
> return 0;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 399d2b2..b124548 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -501,6 +501,7 @@ struct intel_plane_wm_parameters {
> uint8_t bytes_per_pixel;
> bool enabled;
> bool scaled;
> + u64 tiling;
> };
>
> struct intel_plane {
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index f7c9938..006e635 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2662,6 +2662,7 @@ static void skl_compute_wm_pipe_parameters(struct
> drm_crtc *crtc,
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> enum pipe pipe = intel_crtc->pipe;
> struct drm_plane *plane;
> + struct drm_framebuffer *fb;
> int i = 1; /* Index for sprite planes start */
>
> p->active = intel_crtc_active(crtc);
> @@ -2677,6 +2678,14 @@ static void skl_compute_wm_pipe_parameters(struct
> drm_crtc *crtc,
> crtc->primary->fb->bits_per_pixel / 8;
> p->plane[0].horiz_pixels = intel_crtc->config->pipe_src_w;
> p->plane[0].vert_pixels = intel_crtc->config->pipe_src_h;
> + p->plane[0].tiling = DRM_FORMAT_MOD_NONE;
> + fb = crtc->primary->fb;
> + /*
> + * Framebuffer can be NULL on plane disable, but it does not
> + * matter for watermarks if we assume no tiling in that case.
> + */
> + if (fb)
> + p->plane[0].tiling = fb->modifier[0];
>
> p->cursor.enabled = true;
> p->cursor.bytes_per_pixel = 4;
> @@ -2702,6 +2711,7 @@ static bool skl_compute_plane_wm(struct
> skl_pipe_wm_parameters *p,
> {
> uint32_t method1, method2, plane_bytes_per_line, res_blocks, res_lines;
> uint32_t result_bytes;
> + uint32_t y_tile_minimum;
>
> if (mem_value == 0 || !p->active || !p_params->enabled)
> return false;
> @@ -2718,11 +2728,16 @@ static bool skl_compute_plane_wm(struct
> skl_pipe_wm_parameters *p,
> plane_bytes_per_line = p_params->horiz_pixels *
> p_params->bytes_per_pixel;
>
> - /* For now xtile and linear */
> - if (((ddb_allocation * 512) / plane_bytes_per_line) >= 1)
> - result_bytes = min(method1, method2);
> - else
> - result_bytes = method1;
> + if (p_params->tiling == I915_FORMAT_MOD_Y_TILED ||
> + p_params->tiling == I915_FORMAT_MOD_Yf_TILED) {
> + y_tile_minimum = plane_bytes_per_line * 4;
> + result_bytes = max(method2, y_tile_minimum);
> + } else {
> + if (((ddb_allocation * 512) / plane_bytes_per_line) >= 1)
> + result_bytes = min(method1, method2);
> + else
> + result_bytes = method1;
> + }
While this was what was documented at some point, it's not anymore.
Would need an extra patch before this one to introduce
plane_blocks_per_line and then this one on top.
>
> res_blocks = DIV_ROUND_UP(result_bytes, 512) + 1;
> res_lin
Re: [Intel-gfx] [PATCH 7/7] drm/i915/skl: Allow Y (and Yf) frame buffer creation
On Mon, Feb 23, 2015 at 03:56:01PM +, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin
>
> By this patch all underlying bits have been implemented and this
> patch actually enables the feature.
>
> Signed-off-by: Tvrtko Ursulin
> ---
Ville noticed that it didn't seem we were properly returning -EINVAL
when userspace sets reserved bits in the modifiers field. Otherwise,
what's here seems to progress in the right direction:
Reviewed-by: Damien Lespiau
--
Damien
> drivers/gpu/drm/i915/intel_display.c | 13 -
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 74d4923..f100086 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12781,6 +12781,14 @@ static int intel_framebuffer_init(struct drm_device
> *dev,
> DRM_DEBUG("tiling_mode doesn't match fb modifier\n");
> return -EINVAL;
> }
> +
> + if (INTEL_INFO(dev)->gen < 9 &&
> + (mode_cmd->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
> + mode_cmd->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) {
> + DRM_DEBUG("Unsupported tiling 0x%llx!\n",
> + mode_cmd->modifier[0]);
> + return -EINVAL;
> + }
> } else {
> if (obj->tiling_mode == I915_TILING_X)
> mode_cmd->modifier[0] = I915_FORMAT_MOD_X_TILED;
> @@ -12790,11 +12798,6 @@ static int intel_framebuffer_init(struct drm_device
> *dev,
> }
> }
>
> - if (mode_cmd->modifier[0] == I915_FORMAT_MOD_Y_TILED) {
> - DRM_DEBUG("hardware does not support tiling Y\n");
> - return -EINVAL;
> - }
> -
> stride_alignment = intel_fb_stride_alignment(dev, mode_cmd->modifier[0],
>mode_cmd->pixel_format);
> if (mode_cmd->pitches[0] & (stride_alignment - 1)) {
> --
> 2.3.0
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2] lib/igt_kms.c: remove tests dependency on VT /dev/tty0
Required to run on any recent, freon-based and X11-free ChromeOS release.
Signed-off-by: Marc Herbert
---
Changes from v1:
- igt_debug() instead of igt_warn()
- return KD_GRAPHICS instead of -1UL
- print previous mode in debug statements. Among others this help a tiny
bit with the now confusing debug output ("cannot change" immediately
followed by a misleading "mode changed").
Note: a naive "mv /dev/tty0 /dev/tty0.orig" is typically enough to test
this patch.
lib/igt_kms.c | 22 ++
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index d0c3690..9c131f0 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -299,12 +299,26 @@ int kmstest_get_pipe_from_crtc_id(int fd, int crtc_id)
return pfci.pipe;
}
+/*
+ * Returns: the previous mode, or KD_GRAPHICS if no /dev/tty0 was
+ * found and nothing was done.
+ */
static signed long set_vt_mode(unsigned long mode)
{
int fd;
unsigned long prev_mode;
+ static const char TTY0[] = "/dev/tty0";
+
+ if (access(TTY0, F_OK)) {
+ /* errno message should be "No such file". Do not
+ hardcode but ask strerror() in the very unlikely
+ case something else happened. */
+ igt_debug("VT: %s: %s, cannot change its mode\n",
+ TTY0, strerror(errno));
+ return KD_GRAPHICS;
+ }
- fd = open("/dev/tty0", O_RDONLY);
+ fd = open(TTY0, O_RDONLY);
if (fd < 0)
return -errno;
@@ -336,10 +350,10 @@ void kmstest_restore_vt_mode(void)
if (orig_vt_mode != -1UL) {
ret = set_vt_mode(orig_vt_mode);
- orig_vt_mode = -1UL;
igt_assert(ret >= 0);
- igt_debug("VT: original mode restored\n");
+ igt_debug("VT: original mode 0x%lx restored\n", orig_vt_mode);
+ orig_vt_mode = -1UL;
}
}
@@ -366,7 +380,7 @@ void kmstest_set_vt_graphics_mode(void)
igt_assert(ret >= 0);
orig_vt_mode = ret;
- igt_debug("VT: graphics mode set\n");
+ igt_debug("VT: graphics mode set (mode was 0x%lx)\n", ret);
}
--
1.9.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v6 00/32] PPGTT dynamic page allocations and 48b addressing
On Tue, Feb 24, 2015 at 04:22:33PM +, Michel Thierry wrote: > This patchset addresses comments from v5 by Mika, specially some rename > changes > touched several patches. > > For GEN8, it has also been extended to work in logical ring submission (lrc) > mode, as it will be the preferred mode of operation. > I also tried to update the lrc code at the same time the ppgtt refactoring > occurred, leaving only one patch that is exclusively for lrc. > > I'm also now including the required patches for PPGTT with 48b addressing. > In order expand the GPU address space, a 4th level translation is added, the > Page Map Level 4 (PML4). This PML4 has 256 PML4 Entries (PML4E), PML4[0-255], > each pointing to a PDP. > > For now, this feature will only be available in BDW and GEN9, in LRC > submission > mode (execlists) and when i915.enable_ppgtt=3 is set. > Also note that this expanded address space is only available for full PPGTT, > aliasing PPGTT remains 32b. > > This list can be seen in 3 parts: > [01-10] Add page table allocation for GEN6/GEN7 > [11-20] Enable dynamic allocation in GEN8,for both legacy and > execlist submission modes. > [21-32] PML4 support in BDW and GEN9+. > > Ben Widawsky (26): > drm/i915: page table abstractions > drm/i915: Complete page table structures > drm/i915: Create page table allocators > drm/i915: Track GEN6 page table usage > drm/i915: Extract context switch skip and pd load logic > drm/i915: Track page table reload need > drm/i915: Initialize all contexts > drm/i915: Finish gen6/7 dynamic page table allocation > drm/i915/bdw: Use dynamic allocation idioms on free > drm/i915/bdw: page directories rework allocation > drm/i915/bdw: pagetable allocation rework > drm/i915/bdw: Update pdp switch and point unused PDPs to scratch page > drm/i915: num_pd_pages/num_pd_entries isn't useful > drm/i915: Extract PPGTT param from page_directory alloc > drm/i915/bdw: Split out mappings > drm/i915/bdw: begin bitmap tracking > drm/i915/bdw: Dynamic page table allocations > drm/i915/bdw: Make pdp allocation more dynamic > drm/i915/bdw: Abstract PDP usage > drm/i915/bdw: Add dynamic page trace events > drm/i915/bdw: Add ppgtt info for dynamic pages > drm/i915/bdw: implement alloc/free for 4lvl > drm/i915/bdw: Add 4 level switching infrastructure > drm/i915/bdw: Generalize PTE writing for GEN8 PPGTT > drm/i915: Plumb sg_iter through va allocation ->maps > drm/i915: Expand error state's address width to 64b > > Michel Thierry (6): > drm/i915: Plumb drm_device through page tables operations > drm/i915: Add dynamic page trace events > drm/i915/bdw: Support dynamic pdp updates in lrc mode > drm/i915/bdw: Support 64 bit PPGTT in lrc mode > drm/i915/bdw: Add 4 level support in insert_entries and clear_range > drm/i915/bdw: Flip the 48b switch When just a few patches changed (which I suspect is the case here) please don't resend the entire series, but only resend the individual patches in-reply-to their earlier versions. Resending the entire series too often tends to split up the discussions between multiple threads, so should be used cautiously. My approach is that I don't resend the entire series except when all the patches have changed. And I only resend when the review round has reached a conclusion. While the review is ongoing doing incremental updates of the series is imo much better. But when resending the entire series, please start a new thread. Otherwise it again starts to become unclear which versions of which patches go together. And a quick aside if you fear that a patch causes subsequent patches to no longer apply without a rebase: I can deal with a lot of small conflicts quickly when merging. And if that doesn't cut it I'll just ask for a resend when needed. Just a quick reminder of patch resending bkms, intel-gfx is a really busy place so everyone needs to strive for best signal/noise ratio. Thanks, 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 http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Fix frontbuffer false positve.
On Tue, Feb 24, 2015 at 10:44:19AM -0800, Matt Roper wrote:
> On Tue, Feb 24, 2015 at 10:36:32AM -0800, Rodrigo Vivi wrote:
> > Atomic bits needs to be set when cursor check function is returning 0
> > and intel_crtc is active.
> >
> > v2: When putting more debug prints I notice the solution was simpler
> > than I thought. AMS design is solid, just this return was wrong.
> > Sorry for the noise.
> >
> > Cc: Matt Roper
> > Signed-off-by: Rodrigo Vivi
>
> Reviewed-by: Matt Roper
>
> > ---
> > 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 8ccf033..5fb83ba 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -12198,7 +12198,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
> > }
> >
> > if (fb == crtc->cursor->fb)
> > - return 0;
> > + goto finish;
Shouldn't we just delete this check entirely? gcc will do it anyway for
us. When you do that you can also append the history I've dug out for
this.
The original regression seems to have been introduced in the original
check/commit split:
commit 757f9a3e5b8a812af0c213099a5b31cb423f4d3c
Author: Gustavo Padovan
Date: Wed Sep 24 14:20:24 2014 -0300
drm/i915: move check of intel_crtc_cursor_set_obj() out
Which already cause other trouble, resulting in the check getting moved in
commit e391ea882b1a04fb3f559287ac694652a3cd9da9
Author: Gustavo Padovan
Date: Wed Sep 24 14:20:25 2014 -0300
drm/i915: Fix not checking cursor and object sizes
The frontbuffer tracking itself only was broken when we shifted it into
the check/commit logic with:
commit 32b7eeec4d1e861230b09d437e95d76c86ff4a68
Author: Matt Roper
Date: Wed Dec 24 07:59:06 2014 -0800
drm/i915: Refactor work that can sleep out of commit (v7)
Cheers, Daniel
> >
> > if (fb->modifier[0] != DRM_FORMAT_MOD_NONE) {
> > DRM_DEBUG_KMS("cursor cannot be tiled\n");
> > --
> > 2.1.0
> >
>
> --
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
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
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/7] drm/i915/skl: Updated the gen6_set_rps function
On Tue, Feb 24, 2015 at 03:22:54PM +, Damien Lespiau wrote: > On Wed, Feb 18, 2015 at 07:31:11PM +0530, akash.g...@intel.com wrote: > > From: Akash Goel > > > > On SKL, the frequency programmed in RPNSWREQ (A008) register > > has to be in units of 16.66 MHZ. So updated the gen6_set_rps > > function, as per this change. > > > > Signed-off-by: Akash Goel > > Reviewed-by: Lespiau, Damien > > Please don't use the Outlook way "lastname, firstname" here :) Another one: r-b tags should be treated like signatures and only perfectly copypasted. Writing your own is considered forgery ;-) Just another reason to use exactly the string provided. So same strict rules as with sob really. Cheers, 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 http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [BISECTED REGRESSION v3.18->v3.19-rc1] drm/i915: failure to poweroff after hibernation
Hello, My Lenovo Thinkpad X301 has failed to power off after saving the hibernation image ever since v3.19-rc1. This is a regression since v3.18. The regression is still present i v4.0-rc1. The symptoms are: Hibernation proceeds as usual, writing a complete image. But instead of powering off the laptop stays on in a "dead" state, with fans running and the "sleep" LED blinking. The only way out of this state is by hard poweroff (pressing the power button for 4+ seconds). The system can successfully resume after this, proving that the hibernation was complete. I consider the bug somewhat critical as the system will continue to draw power until it is forcibly powered off, or the battery is completely dead. This causes unexpected battery usage and unnecessary battery wear if the power-off failure goes unnoticed. I finally took the time to bisect the problem, and ended up with this surprisingly obvious result: da2bc1b9db3351addd293e5b82757efe1f77ed1d is the first bad commit commit da2bc1b9db3351addd293e5b82757efe1f77ed1d Author: Imre Deak Date: Thu Oct 23 19:23:26 2014 +0300 drm/i915: add poweroff_late handler The suspend_late handler saves some registers and powers off the device, so it doesn't have a big overhead. Calling it at S4 poweroff_late time makes the power off handling identical to the S3 suspend and S4 freeze handling, so do this for consistency. Signed-off-by: Imre Deak Reviewed-by: Ville Syrjälä Signed-off-by: Daniel Vetter :04 04 367eee899c6c2b2a669e2e46f68529dad0e1f7a3 78c7571e2b18dc0fb77161b8a3e32288bd4cbee8 M drivers The bisect log was: # bad: [97bf6af1f928216fd6c5a66e8a57bfa95a659672] Linux 3.19-rc1 # good: [b2776bf7149bddd1f4161f14f79520f17fc1d71d] Linux 3.18 git bisect start 'v3.19-rc1' 'v3.18' # good: [70e71ca0af244f48a5dcf56dc435243792e3a495] Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next git bisect good 70e71ca0af244f48a5dcf56dc435243792e3a495 # bad: [988adfdffdd43cfd841df734664727993076d7cb] Merge branch 'drm-next' of git://people.freedesktop.org/~airlied/linux git bisect bad 988adfdffdd43cfd841df734664727993076d7cb # good: [e7cf773d431a63a2417902696fcc9e0ebdc83bbe] Merge tag 'usb-3.19-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb git bisect good e7cf773d431a63a2417902696fcc9e0ebdc83bbe # bad: [1a92b7a241dcf06a92d84219b4124dcf420ae315] Merge branch 'linux-3.19' of git://anongit.freedesktop.org/git/nouveau/linux-2.6 into drm-next git bisect bad 1a92b7a241dcf06a92d84219b4124dcf420ae315 # bad: [fd172d0c47fddff801d998e38c3efdd236ed082f] Merge tag 'drm-intel-next-2014-11-07-fixups' of git://anongit.freedesktop.org/drm-intel into drm-next git bisect bad fd172d0c47fddff801d998e38c3efdd236ed082f # bad: [820d2d77482810702758381808bdbb64595298e2] drm/i915/audio: pass intel_encoder on to platform specific ELD functions git bisect bad 820d2d77482810702758381808bdbb64595298e2 # good: [11c9b6c628c646894e6ef53f92cfd33a814ee553] drm/i915: Tighting frontbuffer tracking around flips git bisect good 11c9b6c628c646894e6ef53f92cfd33a814ee553 # good: [0b14cbd2f58199a024acbe2994bb27533c97d756] drm/i915: remove dead code from legacy suspend handler git bisect good 0b14cbd2f58199a024acbe2994bb27533c97d756 # good: [f4a12ead50580c17c3641ac1a453e68b5a5195dd] drm/i915: remove unused restore_gtt_mappings optimization during suspend git bisect good f4a12ead50580c17c3641ac1a453e68b5a5195dd # bad: [aff437667b93c3d65576b02628885687c72e1b3b] drm/i915: Move flags describing VMA mappings into the VMA git bisect bad aff437667b93c3d65576b02628885687c72e1b3b # bad: [da2bc1b9db3351addd293e5b82757efe1f77ed1d] drm/i915: add poweroff_late handler git bisect bad da2bc1b9db3351addd293e5b82757efe1f77ed1d # good: [f2476ae65e6159b41168bc41c630e9fbb1d72dde] drm/i915: disable/re-enable PCI device around S4 freeze/thaw git bisect good f2476ae65e6159b41168bc41c630e9fbb1d72dde # good: [5e365c391aeffe8b53d6952c28a68bd5fc856390] drm/i915: sanitize suspend/resume helper function names git bisect good 5e365c391aeffe8b53d6952c28a68bd5fc856390 My rather old system has a GM45 chip, but I would expect the bug to show up on most (all?) i915 systems: nemi:/tmp# lspci -vvvnns 2 00:02.0 VGA compatible controller [0300]: Intel Corporation Mobile 4 Series Chipset Integrated Graphics Controller [8086:2a42] (rev 07) (prog-if 00 [VGA controller]) Subsystem: Lenovo Device [17aa:20e4] Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+ Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=fast >TAbort- SERR- [disabled] Capabilities: [90] MSI: Enable+ Count=1/1 Maskable- 64bit- Address: fee0100c Data: 41c2 Capabilities: [d0] Power Management version 3 Flags: PMEClk- DSI+ D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-) Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME- Kernel
Re: [Intel-gfx] [PATCH] drm: Fix deadlock due to getconnector locking changes
On Sun, Feb 22, 2015 at 11:38:36AM +0100, Daniel Vetter wrote:
> In
>
> daniel@phenom:~/linux/src$ git show ccfc08655
> commit ccfc08655d5fd5076828f45fb09194c070f2f63a
> Author: Rob Clark
> Date: Thu Dec 18 16:01:48 2014 -0500
>
> drm: tweak getconnector locking
>
> We need to extend the locking to cover connector->state reading for
> atomic drivers, but the above commit was a bit too eager and also
> included the fill_modes callback. Which on i915 on old platforms using
> load detection needs to acquire modeset locks, resulting in a deadlock
> on output probing.
>
> Reported-by: Marc Finet
> Cc: Marc Finet
> Cc: robdcl...@gmail.com
> Signed-off-by: Daniel Vetter
Tested-by: Marc Finet
Thanks.
> ---
> drivers/gpu/drm/drm_crtc.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index b15d720eda4c..ce5f1193ecd6 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -2180,7 +2180,6 @@ int drm_mode_getconnector(struct drm_device *dev, void
> *data,
> DRM_DEBUG_KMS("[CONNECTOR:%d:?]\n", out_resp->connector_id);
>
> mutex_lock(&dev->mode_config.mutex);
> - drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>
> connector = drm_connector_find(dev, out_resp->connector_id);
> if (!connector) {
> @@ -2210,6 +2209,8 @@ int drm_mode_getconnector(struct drm_device *dev, void
> *data,
> out_resp->mm_height = connector->display_info.height_mm;
> out_resp->subpixel = connector->display_info.subpixel_order;
> out_resp->connection = connector->status;
> +
> + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> encoder = drm_connector_get_encoder(connector);
> if (encoder)
> out_resp->encoder_id = encoder->base.id;
> --
> 2.1.4
>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: fix failure to power off after hibernate
Imre Deak writes: > The poweroff handlers undo the actions of the thaw handlers. As the > original commit stated saving the registers is not needed there, but > it's also not a big overhead and there should be no problem doing it. We > are planning to optimize the hibernation sequence by for example not > shutting down the display between freeze and thaw, and also getting rid > of unnecessary steps at the power off phase. But before that we want to > actually unify things rather than having special cases, as maintaining > the special code paths caused already quite a lot of problems for us so > far. That sounds like a worthy goal. I don't understand what you hope to achieve by having a poweroff_late hook, since there aren't really anything useful left to do at the point it is called, but if you want a dummy callback there for code structure reasons then fine. But you cannot just run around breaking stuff while slowly moving towards this goal over multiple releases... v3.19 is currently broken and that seems totally unnecessary. In any case: You should have noticed this problem while testing your patches. The breakage is 100% reproducible. Unfortunately I had to do a bisect to realize what you had done to the i915 driver, something I unfortunately didn't find time to do before v3.19 was released. But I do find it unnecessary to release with such bugs. Any attempt to exercise the code path you modified would have revealed the bug. > Reverting the commit may hide some other issue, so before doing that > could you try the following patch: > http://lists.freedesktop.org/archives/intel-gfx/2015-February/060529.html Makes no difference. I assume that patch fixes an unrelated bug? The age and reported symptoms indicates so. Note that I am reporting a regression introduced after v3.18, while that seems to fix a bug introduced in v3.17. Both v3.17 and v3.18 (including v3.18.6), as well as earlier releases, work fine for me. > If with that you still get the hang could you try on top of that the > patch below, first having only pci_set_power_state uncommented, then > both pci_set_power_state and pci_disable_device uncommented? That patch fixes the problem, with only pci_set_power_state commented out. Do you still want me to try with pci_disable_device() commented out as well? Bjørn ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [KERNEL] Re: [KERNEL] Regression bug in drm/i915, Wrong assumption in commit e11aa36 breaks suspend on at least lenovo x61
-BEGIN PGP SIGNED MESSAGE- Hash: SHA512 Hi, Am Mi den 18. Feb 2015 um 16:39 schrieb Jani Nikula: > On Tue, 17 Feb 2015, Klaus Ethgen wrote: > > After solving the conflicts, I applied the revert (see attachment) to > > v3.18.7. I think it should also apply to the current head. With that > > patch, suspend is working again on that version. > > > > However, I have not to deep knowledge of that subsystem, so please, > > someone who have, have a deeper look into it. I especially do not know > > if the lines in .../intel_pm.c are correct or better leaving them as > > they are in v3.18.7. > > > > I want to have it working on a version that I know is stable before > > asking to pull it to head. > > Hi Klaus, we fear this patch may hide the actual cause. That might be. But that might be a second step, to find the actual cause. I think first the patch that causes the problem should be fixed/reverted to have a working kernel and the next step would be to find the real cause. I would happily help to solve both. > Would be useful to get a better description of what happens, Ask.. English is not my mother thong and I am not that good in finding descriptions. ;-) > along with a dmesg with drm.debug=14 module parameter set. Oh, I would be happy to be able to provide that. But if you look at the first mail in this thread, I asked days before how to debug that cause. The problem is that it stops in the middle of the suspend right after switching of screen. There is no way to get it back working after that and so no dmesg. I have to hard switch off the laptop after that. > This might clutter the mailing list, would you mind filing a bug at > [1] and attach the info there? One another account to just fill one bug? Or is there a way to fill the bug without creating an account? I would really like to not create another account. I still have that many from systems that forced me to create an account for just one bug report. Am Mi den 18. Feb 2015 um 17:24 schrieb Imre Deak: > In addition to the above could you also try the following patch and > provide a dmesg log on the bugzilla ticket - at this point only to try > to narrow down the issue?: > [PATCH] Yes, I can do that. But can you tell me how to get dmesg output when the problem happened? After that the box is unusable and I have to hard switch off it. There is no way to get any dmesg output after the bug happened. Regards Klaus Ps. I can forward the first two messages that I posted on kernel ML. That should also be available online. - -- Klaus Ethgen http://www.ethgen.ch/ pub 4096R/4E20AF1C 2011-05-16 Klaus Ethgen Fingerprint: 85D4 CA42 952C 949B 1753 62B3 79D0 B06F 4E20 AF1C -BEGIN PGP SIGNATURE- Version: GnuPG v1 iQGcBAEBCgAGBQJU5OIMAAoJEKZ8CrGAGfas/MUL/1EYB/BKzdXl2JKyktSqt/TF ojXfGKXF6RPYukwIU4+ACXUVGWyZ2llWetbNxrP48MBjBHvXfPLUG0FcxagtE8hH yw61xt3uhO6YPso4fBBZfaCeiU2A5sO3RDb5yQ9kKdpK1GkrCQoG1VsQRKDFZGdl 99dEXQ4mMFMv27HR22YKtVnqW+Iwzdifu3NcdVw7ioqfZrMlRJSPmJQpJ7avfqFO qQzTOv9tFC2G29j6K1J4IeDRd5uagE/IoMQWMF1hvDJlx9iSr6dHSbq/7hZ6aOKZ nVWJ8mF5VB8ReqwI6j+akP+SgShCSG1HqdBJ57/Z2ygDys1N4Yr2EzJ1Ee8qyYp/ ibjwTbCfNKR4Ugq95WY8Dhpr9nUXDB+0pyVYjLvUABkkguDchQeQdUkY50IUodVh 82QkGoATJx+CLY7o+xNRj0W8m/e/VuSMPiJwxwNgJbmipPgG0JhlptRCO5Go8VCA woyPNWdaiLriPIo19mr2NTK8BlV2pT2HD6D7ehlJ7Q== =CE8V -END PGP SIGNATURE- ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Fix frontbuffer false positve.
This return 0 without setting atomic bits on fb == crtc->cursor->fb
where causing frontbuffer false positives.
According to Daniel:
The original regression seems to have been introduced in the original
check/commit split:
commit 757f9a3e5b8a812af0c213099a5b31cb423f4d3c
Author: Gustavo Padovan
Date: Wed Sep 24 14:20:24 2014 -0300
drm/i915: move check of intel_crtc_cursor_set_obj() out
Which already cause other trouble, resulting in the check getting moved in
commit e391ea882b1a04fb3f559287ac694652a3cd9da9
Author: Gustavo Padovan
Date: Wed Sep 24 14:20:25 2014 -0300
drm/i915: Fix not checking cursor and object sizes
The frontbuffer tracking itself only was broken when we shifted it into
the check/commit logic with:
commit 32b7eeec4d1e861230b09d437e95d76c86ff4a68
Author: Matt Roper
Date: Wed Dec 24 07:59:06 2014 -0800
drm/i915: Refactor work that can sleep out of commit (v7)
v2: When putting more debug prints I notice the solution was simpler
than I thought. AMS design is solid, just this return was wrong.
Sorry for the noise.
v3: Remove the entire chunck that would probably
be removed by gcc anyway. (by Daniel)
Cc: Jani Nikula
Cc: Gustavo Padovan
Cc: Matt Roper
Signed-off-by: Rodrigo Vivi
---
drivers/gpu/drm/i915/intel_display.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 8ccf033..900dcaa 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12197,9 +12197,6 @@ intel_check_cursor_plane(struct drm_plane *plane,
return -ENOMEM;
}
- if (fb == crtc->cursor->fb)
- return 0;
-
if (fb->modifier[0] != DRM_FORMAT_MOD_NONE) {
DRM_DEBUG_KMS("cursor cannot be tiled\n");
ret = -EINVAL;
--
2.1.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 6/7] drm/i915/skl: Update watermarks for Y tiling
On Mon, Feb 23, 2015 at 03:56:00PM +, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin
>
> Display watermarks need different programming for different tiling
> modes.
>
> Set the relevant flag so this happens during the plane commit and
> add relevant data into a structure made available to the watermark
> computation code.
>
> v2: Pass in tiling info to sprite plane updates as well.
> v3: Rebased for plane handling changes.
> v4: Handle fb == NULL when plane is disabled.
> v5: Refactored for addfb2 interface.
> v6: Refactored for fb modifier changes.
> v7: Updated for atomic commit by only updating watermarks when tiling changes.
>
> Signed-off-by: Tvrtko Ursulin
> Acked-by: Ander Conselvan de Oliveira
> Acked-by: Matt Roper
> ---
> drivers/gpu/drm/i915/intel_display.c | 6 ++
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> drivers/gpu/drm/i915/intel_pm.c | 33 -
> drivers/gpu/drm/i915/intel_sprite.c | 6 ++
> 4 files changed, 41 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index c622b11..74d4923 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11985,6 +11985,12 @@ intel_check_primary_plane(struct drm_plane *plane,
> INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
>
> intel_crtc->atomic.update_fbc = true;
> +
> + /* Update watermarks on tiling changes. */
> + if (!plane->state->fb || !state->base.fb ||
> + plane->state->fb->modifier[0] !=
> + state->base.fb->modifier[0])
> + intel_crtc->atomic.update_wm = true;
Imo this is fragile. We should unconditionally recomupte watermarks imo
and just ellide the hw update if nothing changed. Spending a few cpu
cycles on this per frame shouldn't hurt, and it will avoid duplicated
logic and checks for watermark recomputation splattered all over the code.
I know that it's kinda there already, but still minimal enough to patch
up. But if Matt knows that this will get removed again with the two-stage
wm code then I'm ok with leaving it in, with a FIXME comment added.
-Daniel
> }
>
> return 0;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 399d2b2..b124548 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -501,6 +501,7 @@ struct intel_plane_wm_parameters {
> uint8_t bytes_per_pixel;
> bool enabled;
> bool scaled;
> + u64 tiling;
> };
>
> struct intel_plane {
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index f7c9938..006e635 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2662,6 +2662,7 @@ static void skl_compute_wm_pipe_parameters(struct
> drm_crtc *crtc,
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> enum pipe pipe = intel_crtc->pipe;
> struct drm_plane *plane;
> + struct drm_framebuffer *fb;
> int i = 1; /* Index for sprite planes start */
>
> p->active = intel_crtc_active(crtc);
> @@ -2677,6 +2678,14 @@ static void skl_compute_wm_pipe_parameters(struct
> drm_crtc *crtc,
> crtc->primary->fb->bits_per_pixel / 8;
> p->plane[0].horiz_pixels = intel_crtc->config->pipe_src_w;
> p->plane[0].vert_pixels = intel_crtc->config->pipe_src_h;
> + p->plane[0].tiling = DRM_FORMAT_MOD_NONE;
> + fb = crtc->primary->fb;
> + /*
> + * Framebuffer can be NULL on plane disable, but it does not
> + * matter for watermarks if we assume no tiling in that case.
> + */
> + if (fb)
> + p->plane[0].tiling = fb->modifier[0];
>
> p->cursor.enabled = true;
> p->cursor.bytes_per_pixel = 4;
> @@ -2702,6 +2711,7 @@ static bool skl_compute_plane_wm(struct
> skl_pipe_wm_parameters *p,
> {
> uint32_t method1, method2, plane_bytes_per_line, res_blocks, res_lines;
> uint32_t result_bytes;
> + uint32_t y_tile_minimum;
>
> if (mem_value == 0 || !p->active || !p_params->enabled)
> return false;
> @@ -2718,11 +2728,16 @@ static bool skl_compute_plane_wm(struct
> skl_pipe_wm_parameters *p,
> plane_bytes_per_line = p_params->horiz_pixels *
> p_params->bytes_per_pixel;
>
> - /* For now xtile and linear */
> - if (((ddb_allocation * 512) / plane_bytes_per_line) >= 1)
> - result_bytes = min(method1, method2);
> - else
> - result_bytes = method1;
> + if (p_params->tiling == I915_FORMAT_MOD_Y_TILED ||
> + p_params->tiling == I915_FORMAT_MOD_Yf_TILED) {
> + y_tile_minimum = plane_bytes_per_line * 4;
> + result_bytes = m
Re: [Intel-gfx] [PATCH] drm/i915: Fix frontbuffer false positve.
On Tue, Feb 24, 2015 at 01:37:54PM -0800, Rodrigo Vivi wrote:
> This return 0 without setting atomic bits on fb == crtc->cursor->fb
> where causing frontbuffer false positives.
>
> According to Daniel:
>
> The original regression seems to have been introduced in the original
> check/commit split:
>
> commit 757f9a3e5b8a812af0c213099a5b31cb423f4d3c
> Author: Gustavo Padovan
> Date: Wed Sep 24 14:20:24 2014 -0300
>
> drm/i915: move check of intel_crtc_cursor_set_obj() out
>
> Which already cause other trouble, resulting in the check getting moved in
>
> commit e391ea882b1a04fb3f559287ac694652a3cd9da9
> Author: Gustavo Padovan
> Date: Wed Sep 24 14:20:25 2014 -0300
>
> drm/i915: Fix not checking cursor and object sizes
>
> The frontbuffer tracking itself only was broken when we shifted it into
> the check/commit logic with:
>
> commit 32b7eeec4d1e861230b09d437e95d76c86ff4a68
> Author: Matt Roper
> Date: Wed Dec 24 07:59:06 2014 -0800
>
> drm/i915: Refactor work that can sleep out of commit (v7)
>
> v2: When putting more debug prints I notice the solution was simpler
> than I thought. AMS design is solid, just this return was wrong.
> Sorry for the noise.
>
> v3: Remove the entire chunck that would probably
> be removed by gcc anyway. (by Daniel)
>
> Cc: Jani Nikula
> Cc: Gustavo Padovan
> Cc: Matt Roper
> Signed-off-by: Rodrigo Vivi
Not sure I understand Daniel's remark about gcc optimization, but the
change still looks good to me.
Reviewed-by: Matt Roper
> ---
> drivers/gpu/drm/i915/intel_display.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 8ccf033..900dcaa 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12197,9 +12197,6 @@ intel_check_cursor_plane(struct drm_plane *plane,
> return -ENOMEM;
> }
>
> - if (fb == crtc->cursor->fb)
> - return 0;
> -
> if (fb->modifier[0] != DRM_FORMAT_MOD_NONE) {
> DRM_DEBUG_KMS("cursor cannot be tiled\n");
> ret = -EINVAL;
> --
> 2.1.0
>
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 7/7] drm/i915/skl: Allow Y (and Yf) frame buffer creation
On Mon, Feb 23, 2015 at 03:56:01PM +, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin
>
> By this patch all underlying bits have been implemented and this
> patch actually enables the feature.
>
> Signed-off-by: Tvrtko Ursulin
> ---
> drivers/gpu/drm/i915/intel_display.c | 13 -
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 74d4923..f100086 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12781,6 +12781,14 @@ static int intel_framebuffer_init(struct drm_device
> *dev,
> DRM_DEBUG("tiling_mode doesn't match fb modifier\n");
> return -EINVAL;
> }
> +
> + if (INTEL_INFO(dev)->gen < 9 &&
> + (mode_cmd->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
> + mode_cmd->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) {
> + DRM_DEBUG("Unsupported tiling 0x%llx!\n",
> + mode_cmd->modifier[0]);
> + return -EINVAL;
> + }
> } else {
> if (obj->tiling_mode == I915_TILING_X)
> mode_cmd->modifier[0] = I915_FORMAT_MOD_X_TILED;
> @@ -12790,11 +12798,6 @@ static int intel_framebuffer_init(struct drm_device
> *dev,
> }
> }
>
> - if (mode_cmd->modifier[0] == I915_FORMAT_MOD_Y_TILED) {
> - DRM_DEBUG("hardware does not support tiling Y\n");
> - return -EINVAL;
> - }
Imo the clearer code would be to add a
switch (mode_cmd->modifier[0]) {
}
here and then shovel the platform checks into the supgroups. At least
that's how we tend to roll these since it reduces the risks that a case is
forgotten when the enumeration is extended. That would also have caught
that we don't reject random garbage in the default: case.
-Daniel
> -
> stride_alignment = intel_fb_stride_alignment(dev, mode_cmd->modifier[0],
>mode_cmd->pixel_format);
> if (mode_cmd->pitches[0] & (stride_alignment - 1)) {
> --
> 2.3.0
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
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
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t 12/12] testdisplay/skl: Add command line options for Yb/Yf tiled fbs
On Mon, Feb 23, 2015 at 03:57:55PM +, Tvrtko Ursulin wrote:
> From: Damien Lespiau
>
> Signed-off-by: Damien Lespiau
testdisplay is a bit an awkward test, mostly used by QA for manual
testing. I think we also need some basic kms_setmode subtest to use the
new tiling modes on skl in one of the automated testcases. Otherwise I
fear this will bitrot fast.
-Daniel
> ---
> tests/testdisplay.c | 16 ++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/tests/testdisplay.c b/tests/testdisplay.c
> index 64ce4d7..dab9e12 100644
> --- a/tests/testdisplay.c
> +++ b/tests/testdisplay.c
> @@ -51,6 +51,7 @@
>
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -71,8 +72,10 @@
> #include
> #include
>
> -#define SUBTEST_OPTS 1
> +#define SUBTEST_OPTS 1
> #define HELP_DESCRIPTION 2
> +#define Yb_OPT3
> +#define Yf_OPT4
>
> static int tio_fd;
> struct termios saved_tio;
> @@ -544,7 +547,7 @@ int update_display(void)
> return 1;
> }
>
> -static char optstr[] = "3hiaf:s:d:p:mrto:j:";
> +static char optstr[] = "3hiaf:s:d:p:mrto:j:y";
>
> static void __attribute__((noreturn)) usage(char *name, char opt)
> {
> @@ -645,6 +648,8 @@ int main(int argc, char **argv)
> {"run-subtest", 1, 0, SUBTEST_OPTS},
> {"help-description", 0, 0, HELP_DESCRIPTION},
> {"help", 0, 0, 'h'},
> + {"yb", 0, 0, Yb_OPT},
> + {"yf", 0, 0, Yf_OPT},
> { 0, 0, 0, 0 }
> };
>
> @@ -697,6 +702,13 @@ int main(int argc, char **argv)
> case 't':
> tiling = LOCAL_I915_FORMAT_MOD_X_TILED;
> break;
> + case 'y':
> + case Yb_OPT:
> + tiling = LOCAL_I915_FORMAT_MOD_Y_TILED;
> + break;
> + case Yf_OPT:
> + tiling = LOCAL_I915_FORMAT_MOD_Yf_TILED;
> + break;
> case 'r':
> qr_code = 1;
> break;
> --
> 2.3.0
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
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
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t 00/12] Testing the Y tiled display
On Mon, Feb 23, 2015 at 03:57:43PM +, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin > > Starting with Skylake the display engine can scan out Y tiled objects. (Both > legacy Y tiled, and the new Yf format.) > > This series takes the original work by Damien Lespiau and converts it to use > the > new frame buffer modifiers instead of object set/get tiling. Some patches > needed > to be dropped, some added and some refactored. > > v2: Refactored for fb modifier changes. > > Damien Lespiau (7): > lib: Extract igt_buf_write_to_png() from gem_render_copy > lib/skl: Add gen9 specific igt_blitter_fast_copy() > lib: Don't give a struct igt_buf * to fast_copy_pitch() > lib: Split two helpers to build fast copy's dword0 and dword1 > lib: Provide a raw version of the gen9 fast copy blits > lib: Allow the creation of Ys/Yf tiled FBs > testdisplay/skl: Add command line options for Yb/Yf tiled fbs > > Tvrtko Ursulin (5): > tests/kms_addfb: Add support for fb modifiers > tests/kms_addfb: Y tiled testcases > tiling: Convert framebuffer helpers to use fb modifiers > lib: Add support for new extension to the ADDFB2 ioctl. > lib/igt_fb: Use new ADDFB2 extension for new tiling modes Test coverage looks good to me overall, just two minor things: - functional automated test (replied to the testdisplay patch) - catching invalid modifiers and making sure y/yf tiling is reject unconditionally on pre-gen9 seem to be missing too. Detail checking that we have full coverage is imo for the in-depth review. Kernel patches also look good, just a few minor comments. Cheers, Daniel > > lib/igt_fb.c| 163 + > lib/igt_fb.h| 10 +- > lib/igt_kms.h | 1 + > lib/intel_batchbuffer.c | 281 > > lib/intel_batchbuffer.h | 37 ++ > lib/intel_reg.h | 18 +++ > lib/ioctl_wrappers.c| 49 > lib/ioctl_wrappers.h| 41 +++ > tests/gem_render_copy.c | 24 +--- > tests/kms_3d.c | 2 +- > tests/kms_addfb.c | 135 - > tests/kms_cursor_crc.c | 8 +- > tests/kms_fbc_crc.c | 4 +- > tests/kms_fence_pin_leak.c | 4 +- > tests/kms_flip.c| 6 +- > tests/kms_flip_event_leak.c | 4 +- > tests/kms_flip_tiling.c | 7 +- > tests/kms_mmio_vs_cs_flip.c | 12 +- > tests/kms_pipe_crc_basic.c | 2 +- > tests/kms_plane.c | 8 +- > tests/kms_psr_sink_crc.c| 8 +- > tests/kms_pwrite_crc.c | 4 +- > tests/kms_render.c | 8 +- > tests/kms_rotation_crc.c| 4 +- > tests/kms_setmode.c | 2 +- > tests/kms_sink_crc_basic.c | 6 +- > tests/kms_universal_plane.c | 18 +-- > tests/pm_lpsp.c | 2 +- > tests/pm_rpm.c | 26 ++-- > tests/testdisplay.c | 20 +++- > 30 files changed, 795 insertions(+), 119 deletions(-) > > -- > 2.3.0 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- 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 http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 09/12] drm/i915/config: Add VBT settings configuration.
On Tue, 24 Feb 2015 21:52:16 +0100 Daniel Vetter wrote: > On Tue, Feb 24, 2015 at 10:37:10AM -0800, Bob Paauwe wrote: > > On Tue, 24 Feb 2015 14:57:48 +0100 > > Daniel Vetter wrote: > > > As Jani points out we already have vbt headaches, it would be good if we > > > only have those once ;-) > > > -Daniel > > > > I've been trying to think of a better way but not really coming up with > > something that scales well. EMGD took the approach of create > > configuration for only those values that we had customer requests for. > > And that was primarily just the panel power up/down timing sequence and > > some backlight control. We never tried to replicate everything that > > could be set via vbt. > > If it's only a very few select things we could just add them as atomic > properties to connectors. No need for anything special, and we could even > use that to debug panel issues at runtime. > > We need a lot of the dp things already anyway for the dp validation stuff, > but since that was debug Dave shot down my idea to just go with props. But > here we have another user, and this one will have full abi compat > requirements. So no longer any reasons to not go with props imo. > > Or else we just smash an entire vbt into acpi, for my that's ok (we need > to deal with vbt anyway). > -Daniel Funny... As I was looking at the atomic code I had the same thought to use atomic properties but dismissed it because I thought it would be too much overhead to do that for all of the vbt flags/values. But it it's limited, at least initially, to what we know we need that would work and this whole bit magically goes away. Thanks! Looks like it's time for me to really start digging into atomic. Bob ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] lib/igt_kms.c: remove tests dependency on VT /dev/tty0
On Tue, Feb 24, 2015 at 12:29:22PM -0800, Marc Herbert wrote:
> Required to run on any recent, freon-based and X11-free ChromeOS release.
>
> Signed-off-by: Marc Herbert
> ---
>
> Changes from v1:
> - igt_debug() instead of igt_warn()
> - return KD_GRAPHICS instead of -1UL
> - print previous mode in debug statements. Among others this help a tiny
> bit with the now confusing debug output ("cannot change" immediately
> followed by a misleading "mode changed").
Thanks for the patch, merged to igt.
-Daniel
>
> Note: a naive "mv /dev/tty0 /dev/tty0.orig" is typically enough to test
> this patch.
>
> lib/igt_kms.c | 22 ++
> 1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index d0c3690..9c131f0 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -299,12 +299,26 @@ int kmstest_get_pipe_from_crtc_id(int fd, int crtc_id)
> return pfci.pipe;
> }
>
> +/*
> + * Returns: the previous mode, or KD_GRAPHICS if no /dev/tty0 was
> + * found and nothing was done.
> + */
> static signed long set_vt_mode(unsigned long mode)
> {
> int fd;
> unsigned long prev_mode;
> + static const char TTY0[] = "/dev/tty0";
> +
> + if (access(TTY0, F_OK)) {
> + /* errno message should be "No such file". Do not
> +hardcode but ask strerror() in the very unlikely
> +case something else happened. */
> + igt_debug("VT: %s: %s, cannot change its mode\n",
> + TTY0, strerror(errno));
> + return KD_GRAPHICS;
> + }
>
> - fd = open("/dev/tty0", O_RDONLY);
> + fd = open(TTY0, O_RDONLY);
> if (fd < 0)
> return -errno;
>
> @@ -336,10 +350,10 @@ void kmstest_restore_vt_mode(void)
>
> if (orig_vt_mode != -1UL) {
> ret = set_vt_mode(orig_vt_mode);
> - orig_vt_mode = -1UL;
>
> igt_assert(ret >= 0);
> - igt_debug("VT: original mode restored\n");
> + igt_debug("VT: original mode 0x%lx restored\n", orig_vt_mode);
> + orig_vt_mode = -1UL;
> }
> }
>
> @@ -366,7 +380,7 @@ void kmstest_set_vt_graphics_mode(void)
> igt_assert(ret >= 0);
> orig_vt_mode = ret;
>
> - igt_debug("VT: graphics mode set\n");
> + igt_debug("VT: graphics mode set (mode was 0x%lx)\n", ret);
> }
>
>
> --
> 1.9.3
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
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
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] linux-next: manual merge of the drm-intel tree with the drm-intel-fixes tree
Hi all,
Today's linux-next merge of the drm-intel tree got a conflict in
drivers/gpu/drm/i915/intel_display.c between commit f37b5c2be897
("drm/i915: Align initial plane backing objects correctly") from the
drm-intel-fixes tree and commit 6bf129df6ffa ("drm/i915: Use an
intermediate variable to avoid repeating ourselves") from the drm-intel
tree.
I fixed it up (see below) and can carry the fix as necessary (no action
is required).
--
Cheers,
Stephen Rothwells...@canb.auug.org.au
diff --cc drivers/gpu/drm/i915/intel_display.c
index bf19a5cce4a8,2ac93909cfc5..
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@@ -2371,11 -2376,8 +2376,12 @@@ intel_alloc_plane_obj(struct intel_crt
struct drm_device *dev = crtc->base.dev;
struct drm_i915_gem_object *obj = NULL;
struct drm_mode_fb_cmd2 mode_cmd = { 0 };
+ struct drm_framebuffer *fb = &plane_config->fb->base;
- u32 base = plane_config->base;
+ u32 base_aligned = round_down(plane_config->base, PAGE_SIZE);
+ u32 size_aligned = round_up(plane_config->base + plane_config->size,
+ PAGE_SIZE);
+
+ size_aligned -= base_aligned;
if (plane_config->size == 0)
return false;
@@@ -6640,9 -6678,10 +6693,10 @@@ i9xx_get_initial_plane_config(struct in
fb->pitches[0] = val & 0xffc0;
aligned_height = intel_fb_align_height(dev, fb->height,
- plane_config->tiling);
+ fb->pixel_format,
+ fb->modifier[0]);
- plane_config->size = PAGE_ALIGN(fb->pitches[0] * aligned_height);
+ plane_config->size = fb->pitches[0] * aligned_height;
DRM_DEBUG_KMS("pipe/plane %c/%d with fb: size=%dx%d@%d, offset=%x,
pitch %d, size 0x%x\n",
pipe_name(pipe), plane, fb->width, fb->height,
@@@ -7677,9 -7721,10 +7736,10 @@@ skylake_get_initial_plane_config(struc
fb->pitches[0] = (val & 0x3ff) * stride_mult;
aligned_height = intel_fb_align_height(dev, fb->height,
- plane_config->tiling);
+ fb->pixel_format,
+ fb->modifier[0]);
- plane_config->size = ALIGN(fb->pitches[0] * aligned_height, PAGE_SIZE);
+ plane_config->size = fb->pitches[0] * aligned_height;
DRM_DEBUG_KMS("pipe %c with fb: size=%dx%d@%d, offset=%x, pitch %d,
size 0x%x\n",
pipe_name(pipe), fb->width, fb->height,
@@@ -7768,9 -7818,10 +7833,10 @@@ ironlake_get_initial_plane_config(struc
fb->pitches[0] = val & 0xffc0;
aligned_height = intel_fb_align_height(dev, fb->height,
- plane_config->tiling);
+ fb->pixel_format,
+ fb->modifier[0]);
- plane_config->size = PAGE_ALIGN(fb->pitches[0] * aligned_height);
+ plane_config->size = fb->pitches[0] * aligned_height;
DRM_DEBUG_KMS("pipe %c with fb: size=%dx%d@%d, offset=%x, pitch %d,
size 0x%x\n",
pipe_name(pipe), fb->width, fb->height,
pgpPOEZTX3uwS.pgp
Description: OpenPGP digital signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/2] drm/i915/skl: Add support for edp1.4 low vswing
Based upon vbt's vswing preemph settings value select the appropriate
translations for edp.
v2: Incorporating bspec changes for vswing and preemph levels, adding edp
translation table. Removed HSW from selection 9 which is specific to skl and
correcting the returning of level2 from max pre emph (Damien)
v3: Rebasing on top of renaming patches. Adding level(3,0) since level(2,2) as
mentioned in bspec is invalid as per edp spec. Also changed the determining of
size of the table selected (Satheesh).
v4: Adding level 3 in max voltage selection if low vswing is selected (Satheesh)
v5: Add a comment stating that skl_ddi_translations_edp is for eDP 1.4
low vswing panels.
v6: Updating recommended DDI translation table for edp 1.4
Reviewed-by: Satheeshakrishna M (v4)
Reviewed-by: Damien Lespiau (v6)
Signed-off-by: Sonika Jindal
Signed-off-by: Damien Lespiau
---
drivers/gpu/drm/i915/intel_ddi.c | 46 +-
drivers/gpu/drm/i915/intel_dp.c | 12 --
2 files changed, 50 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index f14e8a2..985d531 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -139,6 +139,21 @@ static const struct ddi_buf_trans
skl_ddi_translations_dp[] = {
{ 0x4014, 0x0087 },
};
+/* eDP 1.4 low vswing translation parameters */
+static const struct ddi_buf_trans skl_ddi_translations_edp[] = {
+ { 0x0018, 0x00a8 },
+ { 0x2016, 0x00ab },
+ { 0x6012, 0x00a2 },
+ { 0x8010, 0x0088 },
+ { 0x0018, 0x00ab },
+ { 0x4014, 0x00a2 },
+ { 0x6012, 0x00a6 },
+ { 0x0018, 0x00a2 },
+ { 0x5013, 0x009c },
+ { 0x0018, 0x0088 },
+};
+
+
static const struct ddi_buf_trans skl_ddi_translations_hdmi[] = {
/* Idx NT mV T mVdb */
{ 0x0018, 0x00a0 }, /* 0: 400 400 0 */
@@ -187,7 +202,8 @@ static void intel_prepare_ddi_buffers(struct drm_device
*dev, enum port port)
{
struct drm_i915_private *dev_priv = dev->dev_private;
u32 reg;
- int i, n_hdmi_entries, hdmi_800mV_0dB;
+ int i, n_hdmi_entries, n_dp_entries, n_edp_entries, hdmi_800mV_0dB,
+ size;
int hdmi_level = dev_priv->vbt.ddi_port_info[port].hdmi_level_shift;
const struct ddi_buf_trans *ddi_translations_fdi;
const struct ddi_buf_trans *ddi_translations_dp;
@@ -198,7 +214,15 @@ static void intel_prepare_ddi_buffers(struct drm_device
*dev, enum port port)
if (IS_SKYLAKE(dev)) {
ddi_translations_fdi = NULL;
ddi_translations_dp = skl_ddi_translations_dp;
- ddi_translations_edp = skl_ddi_translations_dp;
+ n_dp_entries = ARRAY_SIZE(skl_ddi_translations_dp);
+ if (dev_priv->vbt.edp_low_vswing) {
+ ddi_translations_edp = skl_ddi_translations_edp;
+ n_edp_entries = ARRAY_SIZE(skl_ddi_translations_edp);
+ } else {
+ ddi_translations_edp = skl_ddi_translations_dp;
+ n_edp_entries = ARRAY_SIZE(skl_ddi_translations_dp);
+ }
+
ddi_translations_hdmi = skl_ddi_translations_hdmi;
n_hdmi_entries = ARRAY_SIZE(skl_ddi_translations_hdmi);
hdmi_800mV_0dB = 7;
@@ -207,6 +231,8 @@ static void intel_prepare_ddi_buffers(struct drm_device
*dev, enum port port)
ddi_translations_dp = bdw_ddi_translations_dp;
ddi_translations_edp = bdw_ddi_translations_edp;
ddi_translations_hdmi = bdw_ddi_translations_hdmi;
+ n_edp_entries = ARRAY_SIZE(bdw_ddi_translations_edp);
+ n_dp_entries = ARRAY_SIZE(bdw_ddi_translations_dp);
n_hdmi_entries = ARRAY_SIZE(bdw_ddi_translations_hdmi);
hdmi_800mV_0dB = 7;
} else if (IS_HASWELL(dev)) {
@@ -214,6 +240,7 @@ static void intel_prepare_ddi_buffers(struct drm_device
*dev, enum port port)
ddi_translations_dp = hsw_ddi_translations_dp;
ddi_translations_edp = hsw_ddi_translations_dp;
ddi_translations_hdmi = hsw_ddi_translations_hdmi;
+ n_dp_entries = n_edp_entries =
ARRAY_SIZE(hsw_ddi_translations_dp);
n_hdmi_entries = ARRAY_SIZE(hsw_ddi_translations_hdmi);
hdmi_800mV_0dB = 6;
} else {
@@ -222,6 +249,8 @@ static void intel_prepare_ddi_buffers(struct drm_device
*dev, enum port port)
ddi_translations_fdi = bdw_ddi_translations_fdi;
ddi_translations_dp = bdw_ddi_translations_dp;
ddi_translations_hdmi = bdw_ddi_translations_hdmi;
+ n_edp_entries = ARRAY_SIZE(bdw_ddi_translations_edp);
+ n_dp_entr
[Intel-gfx] [PATCH 0/2] Support for low voltage swing
Skylake supports low voltage swing in edp 1.4. The translation table is selected based upon the vbt entry for selecting low vswing These patches are being pulled from -internal to -nightly. Sonika Jindal (2): drm/i915/skl: Support for edp low_vswing param in vbt drm/i915/skl: Add support for edp1.4 low vswing drivers/gpu/drm/i915/i915_drv.h |1 + drivers/gpu/drm/i915/intel_bios.c |7 ++ drivers/gpu/drm/i915/intel_bios.h |1 + drivers/gpu/drm/i915/intel_ddi.c | 46 - drivers/gpu/drm/i915/intel_dp.c | 12 -- 5 files changed, 59 insertions(+), 8 deletions(-) -- 1.7.10.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 7/7] drm/i915/skl: Allow Y (and Yf) frame buffer creation
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang...@intel.com) Task id: 5814 -Summary- Platform Delta drm-intel-nightly Series Applied PNV 281/281 281/281 ILK 305/305 305/305 SNB -1 326/326 325/326 IVB 380/380 380/380 BYT 294/294 294/294 HSW -1 421/421 420/421 BDW -1 316/316 315/316 -Detailed- Platform Testdrm-intel-nightly Series Applied SNB igt_kms_plane_plane-position-hole-pipe-B-plane-1 DMESG_WARN(12)PASS(3) DMESG_WARN(1)PASS(1) HSW igt_gem_storedw_loop_vebox DMESG_WARN(2)PASS(1) DMESG_WARN(1)PASS(1) *BDW igt_gem_gtt_hog PASS(6) DMESG_WARN(1)PASS(1) Note: You need to pay more attention to line start with '*' ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
