date:20220628

Re: [Intel-gfx] [PATCH][next] treewide: uapi: Replace zero-length arrays with flexible-array members

2022-06-28 Thread Geert Uytterhoeven

Hi Gustavo,

Thanks for your patch!

On Mon, Jun 27, 2022 at 8:04 PM Gustavo A. R. Silva
 wrote:
> There is a regular need in the kernel to provide a way to declare
> having a dynamically sized set of trailing elements in a structure.
> Kernel code should always use “flexible array members”[1] for these
> cases. The older style of one-element or zero-length arrays should
> no longer be used[2].

These rules apply to the kernel, but uapi is not considered part of the
kernel, so different rules apply.  Uapi header files should work with
whatever compiler that can be used for compiling userspace.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915: ttm for stolen (rev5)

2022-06-28 Thread Tvrtko Ursulin

On 27/06/2022 18:08, Robert Beckett wrote:

On 22/06/2022 10:05, Tvrtko Ursulin wrote:

On 21/06/2022 20:11, Robert Beckett wrote:

On 21/06/2022 18:37, Patchwork wrote:

*Patch Details*
*Series:* drm/i915: ttm for stolen (rev5)
*URL:* https://patchwork.freedesktop.org/series/101396/

*State:* failure
*Details:*
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_101396v5/index.html

CI Bug Log - changes from CI_DRM_11790 -> Patchwork_101396v5

Summary

*FAILURE*

Serious unknown changes coming with Patchwork_101396v5 absolutely
need to be

verified manually.

If you think the reported changes have nothing to do with the changes
introduced in Patchwork_101396v5, please notify your bug team to
allow them
to document this new failure mode, which will reduce false positives
in CI.

External URL:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_101396v5/index.html

Participating hosts (40 -> 41)

Additional (2): fi-icl-u2 bat-dg2-9
Missing (1): fi-bdw-samus

Possible new issues

Here are the unknown changes that may have been introduced in
Patchwork_101396v5:

IGT changes

Possible regressions

* igt@i915_selftest@live@reset:
o bat-adlp-4: PASS

-> DMESG-FAIL

I keep hitting clobbered pages during engine resets on bat-adlp-4.
It seems to happen most of the time on that machine and occasionally
on bat-adlp-6.

Should bat-adlp-4 be considered an unreliable machine like bat-adlp-6
is for now?

Alternatively, seeing the history of this in

commit 3da3c5c1c9825c24168f27b021339e90af37e969 "drm/i915: Exclude
low pages (128KiB) of stolen from use"

could this be an indication that maybe the original issue is worse on
adlp machines?
I have only ever seen page page 135 or 136 clobbered across many runs
via trybot, so it looks fairly consistent.

Though excluding the use of over 540K of stolen might be too severe.

Don't know but I see that on the latest version you even hit pages
165/166.

Any history of hitting this in CI without your series? If not, are
there some other changes which could explain it? Are you touching the
selftest itself?

Hexdump of the clobbered page looks quite complex. Especially
POISON_FREE. Any idea how that ends up there?

(see
https://intel-gfx-ci.01.org/tree/drm-tip/Trybot_105517v4/fi-rkl-guc/igt@i915_selftest@l...@reset.html#dmesg-warnings702)

after lots of slow debug via CI, it looks like the issue is that a ring
buffer was allocated and taking up that page during the initial crc
capture in the test, but by the time it came to check for corruption, it
had been freed from that page.

The test has a number of weaknesses:

1. the busy check is done twice, without taking in to account any change
in between. I assume previously this could be relied on never to occur,
but now it can for some reason (more on that later)

You mean the stolen page used/unused test? Probably the premise is that
the test controls the driver completely ie. is the sole user and the two
checks are run at the time where nothing else could have changed the state.

With the nerfed request (as with GuC) this actually should hold. In the
generic case I am less sure, my working knowledge faded a bit, but
perhaps there was something guaranteeing the spinner couldn't have been
retired yet at the time of the second check. Would need clarifying at
least in comments.

2. the engine reset returns early with an error for guc submission
engines, but it is silently ignored in the test. Perhaps it should
ignore guc submission engines as it is a largely useless test for those
situations.

Yes looks dodgy indeed. You will need to summon the owners of the GuC
backend to comment on this.

However even if the test should be skipped with GuC it is extremely
interesting that you are hitting this so I suspect there is a more
serious issue at play.

A quick obvious fix is to have a busy bitmask that remembers each page's
busy state initially and only check for corruption if it was busy during
both checks.

However, the main question is why this is occurring now with my changes.
I have added more debug to check where the stolen memory is being freed,
but the first run last night didn't hit the issue for once.
I am running again now, will report back if I figure out where it is
being freed.

I am pretty sure the "corruption" (which isn't actually corruption) is
from a ring buffer.
The POISON_FREE is the only difference between the captured before and
after dumps:

[0040] 0280 6b6b6b6b 6b6b6b6b 6b6b6b6b 6b6b6b6b 6b6b6b6b
6b6b6b6b

with the 2nd dword being the MI_ARB_

Re: [Intel-gfx] [PATCH v3 3/3] drm/doc/rfc: VM_BIND uapi definition

2022-06-28 Thread Tvrtko Ursulin




On 27/06/2022 19:58, Zeng, Oak wrote:



Thanks,
Oak


-Original Message-
From: Tvrtko Ursulin 
Sent: June 27, 2022 4:30 AM
To: Zeng, Oak ; Landwerlin, Lionel G
; Vishwanathapura, Niranjana

Cc: Zanoni, Paulo R ; intel-
g...@lists.freedesktop.org; dri-de...@lists.freedesktop.org; Hellstrom,
Thomas ; Wilson, Chris P
; Vetter, Daniel ;
christian.koe...@amd.com; Auld, Matthew 
Subject: Re: [Intel-gfx] [PATCH v3 3/3] drm/doc/rfc: VM_BIND uapi definition


On 24/06/2022 21:23, Zeng, Oak wrote:

Let's compare "tlb invalidate at vm unbind" vs "tlb invalidate at backing

storage":


Correctness:
consider this sequence of:
1. unbind va1 from pa1,
2. then bind va1 to pa2. //user space has the freedom to do this as it
manages virtual address space 3. Submit shader code using va1, 4. Then
retire pa1.

If you don't perform tlb invalidate at step #1, in step #3, shader will use

stale entries in tlb and pa1 will be used for the shader. User want to use pa2.
So I don't think invalidate tlb at step #4 make correctness.

Define step 3. Is it a new execbuf? If so then there will be a TLB flush there.
Unless the plan is to stop doing that with eb3 but I haven't picked up on that
anywhere so far.


In Niranjana's latest patch series, he removed the TLB flushing from vm_unbind. 
He also said explicitly TLB invalidation will be performed at job submission 
and backing storage releasing time, which is the existing behavior of the 
current i915 driver.

I think if we invalidate TLB on each vm_unbind, then we don't need to 
invalidate at submission and backing storage releasing. It doesn't make a lot 
of sense to me to perform a tlb invalidation at execbuf time. Maybe it is a 
behavior for the old implicit binding programming model. For vm_bind and eb3, 
we separate the binding and job submission into two APIs. It is more natural 
the TLB invalidation be coupled with the vm bind/unbind, not job submission. So 
in my opinion we should remove tlb invalidation from submission and backing 
storage releasing and add it to vm unbind. This is method is cleaner to me.


You can propose this model (not flushing in eb3) but I have my doubts. 
Consider the pointlessness of flushing on N unbinds for 99% of clients 
which are not infinite compute batch. And consider how you make the 
behaviour consistent on all platforms (selective vs global tlb flush).


Also note that this discussion is orthogonal to unbind vs backing store 
release.



Regarding performance, we don't have data. In my opinion, we should make things 
work in a most straight forward way as the first step. Then consider 
performance improvement if necessary. Consider some delayed tlb invalidation at 
submission and backing release time without performance data support wasn't a 
good decision.


It is quite straightforward though. ;) It aligns with the eb2 model and 
argument can be made backing store release is (much) less frequent than 
unbind (consider softpin where client could trigger a lot of pointless 
flushes).


Regards,

Tvrtko

Re: [Intel-gfx] [PATCH] drm/i915/gt: handle null ptr at sg traversing

2022-06-28 Thread Matthew Auld


On 27/06/2022 18:35, Ramalingam C wrote:

When calculating the starting address for ccs data in smem scatterlist,
handle the NULL pointer returned from sg_next, incase of scatterlist
less than required size..


Do we have some more information on how we can hit this? Is this a 
programmer error? Do we have a testcase?




Signed-off-by: Ramalingam C 
---
  drivers/gpu/drm/i915/gt/intel_migrate.c | 13 ++---
  1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_migrate.c 
b/drivers/gpu/drm/i915/gt/intel_migrate.c
index 2c35324b5f68..c206fb4f4186 100644
--- a/drivers/gpu/drm/i915/gt/intel_migrate.c
+++ b/drivers/gpu/drm/i915/gt/intel_migrate.c
@@ -669,7 +669,7 @@ calculate_chunk_sz(struct drm_i915_private *i915, bool 
src_is_lmem,
}
  }
  
-static void get_ccs_sg_sgt(struct sgt_dma *it, u32 bytes_to_cpy)

+static int get_ccs_sg_sgt(struct sgt_dma *it, u32 bytes_to_cpy)
  {
u32 len;
  
@@ -684,9 +684,13 @@ static void get_ccs_sg_sgt(struct sgt_dma *it, u32 bytes_to_cpy)

bytes_to_cpy -= len;
  
  		it->sg = __sg_next(it->sg);

+   if (!it->sg)
+   return -EINVAL;
it->dma = sg_dma_address(it->sg);
it->max = it->dma + sg_dma_len(it->sg);
} while (bytes_to_cpy);
+
+   return 0;
  }
  
  int

@@ -745,8 +749,11 @@ intel_context_migrate_copy(struct intel_context *ce,
 * Need to fix it.
 */
ccs_bytes_to_cpy = src_sz != dst_sz ? GET_CCS_BYTES(i915, 
bytes_to_cpy) : 0;
-   if (ccs_bytes_to_cpy)
-   get_ccs_sg_sgt(&it_ccs, bytes_to_cpy);
+   if (ccs_bytes_to_cpy) {
+   err = get_ccs_sg_sgt(&it_ccs, bytes_to_cpy);
+   if (err)
+   return err;
+   }
}
  
  	src_offset = 0;

Re: [Intel-gfx] [PATCH] drm/i915/gt: handle null ptr at sg traversing

2022-06-28 Thread Ramalingam C

On 2022-06-28 at 10:40:56 +0100, Matthew Auld wrote:
> On 27/06/2022 18:35, Ramalingam C wrote:
> > When calculating the starting address for ccs data in smem scatterlist,
> > handle the NULL pointer returned from sg_next, incase of scatterlist
> > less than required size..
> 
> Do we have some more information on how we can hit this? Is this a
> programmer error? Do we have a testcase?
Typically We will never get NULL at this point, as we allocate the smem
of sz equal to lmem obj size + requiured ccs size. So we will never run
into NULL when we traverse the sg for the size of lmem in smem's sg.

IF there is NULL returned in this scenario we could report BUG_ON or let
it NPD or return the error code.

But either way couldn't think of a scenario when this will hit. after
thinking further seems to be leaving the NPD itself sufficient as other
error handling also not doing good job at it. Please share your thoughts

Ram
> 
> > 
> > Signed-off-by: Ramalingam C 
> > ---
> >   drivers/gpu/drm/i915/gt/intel_migrate.c | 13 ++---
> >   1 file changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_migrate.c 
> > b/drivers/gpu/drm/i915/gt/intel_migrate.c
> > index 2c35324b5f68..c206fb4f4186 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_migrate.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_migrate.c
> > @@ -669,7 +669,7 @@ calculate_chunk_sz(struct drm_i915_private *i915, bool 
> > src_is_lmem,
> > }
> >   }
> > -static void get_ccs_sg_sgt(struct sgt_dma *it, u32 bytes_to_cpy)
> > +static int get_ccs_sg_sgt(struct sgt_dma *it, u32 bytes_to_cpy)
> >   {
> > u32 len;
> > @@ -684,9 +684,13 @@ static void get_ccs_sg_sgt(struct sgt_dma *it, u32 
> > bytes_to_cpy)
> > bytes_to_cpy -= len;
> > it->sg = __sg_next(it->sg);
> > +   if (!it->sg)
> > +   return -EINVAL;
> > it->dma = sg_dma_address(it->sg);
> > it->max = it->dma + sg_dma_len(it->sg);
> > } while (bytes_to_cpy);
> > +
> > +   return 0;
> >   }
> >   int
> > @@ -745,8 +749,11 @@ intel_context_migrate_copy(struct intel_context *ce,
> >  * Need to fix it.
> >  */
> > ccs_bytes_to_cpy = src_sz != dst_sz ? GET_CCS_BYTES(i915, 
> > bytes_to_cpy) : 0;
> > -   if (ccs_bytes_to_cpy)
> > -   get_ccs_sg_sgt(&it_ccs, bytes_to_cpy);
> > +   if (ccs_bytes_to_cpy) {
> > +   err = get_ccs_sg_sgt(&it_ccs, bytes_to_cpy);
> > +   if (err)
> > +   return err;
> > +   }
> > }
> > src_offset = 0;

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 1/3] tests/i915/gem_eio: fix uaf

2022-06-28 Thread Das, Nirmoy


The series is Reviewed-by: Nirmoy Das 

On 6/27/2022 6:10 PM, Matthew Auld wrote:

../tests/i915/gem_eio.c:277:20: warning: pointer ‘ctx’ used after ‘free’ 
[-Wuse-after-free]
   277 | igt_assert(igt_sysfs_printf(ctx->debugfs, "i915_drop_caches",
../lib/igt_core.h:667:20: note: in definition of macro ‘igt_assert’
   667 | do { if (!(expr)) \
   |^~~~
../tests/i915/gem_eio.c:274:9: note: call to ‘free’ here
   274 | free(ctx);

Signed-off-by: Matthew Auld 
Cc: Gwan-gyeong Mun 
---
  tests/i915/gem_eio.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/i915/gem_eio.c b/tests/i915/gem_eio.c
index 913a21f9..6cbae6eb 100644
--- a/tests/i915/gem_eio.c
+++ b/tests/i915/gem_eio.c
@@ -270,11 +270,11 @@ static void hang_handler(union sigval arg)
  igt_nsec_elapsed(&ctx->delay) / 1000.0);
  
  	igt_assert_eq(timer_delete(ctx->timer), 0);

-   free(ctx);
  
  	/* flush any excess work before we start timing our reset */

igt_assert(igt_sysfs_printf(ctx->debugfs, "i915_drop_caches",
"%d", DROP_RCU));
+   free(ctx);
  
  	igt_nsec_elapsed(ts);

igt_assert(igt_sysfs_printf(dir, "i915_wedged", "%llu", -1ull));

Re: [Intel-gfx] [PATCH i-g-t 1/3] tests/i915/gem_eio: fix uaf

2022-06-28 Thread Gwan-gyeong Mun


Looks good to me.

Reviewed-by: Gwan-gyeong Mun 

However, the use after free build issue did not occur only with the "$ 
meson build && ninja -C build" build command guided by the igt 
README.md. How did you check it?


Br,

G.G.


On 6/27/22 7:10 PM, Matthew Auld wrote:

../tests/i915/gem_eio.c:277:20: warning: pointer ‘ctx’ used after ‘free’ 
[-Wuse-after-free]
   277 | igt_assert(igt_sysfs_printf(ctx->debugfs, "i915_drop_caches",
../lib/igt_core.h:667:20: note: in definition of macro ‘igt_assert’
   667 | do { if (!(expr)) \
   |^~~~
../tests/i915/gem_eio.c:274:9: note: call to ‘free’ here
   274 | free(ctx);

Signed-off-by: Matthew Auld 
Cc: Gwan-gyeong Mun 
---
  tests/i915/gem_eio.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/i915/gem_eio.c b/tests/i915/gem_eio.c
index 913a21f9..6cbae6eb 100644
--- a/tests/i915/gem_eio.c
+++ b/tests/i915/gem_eio.c
@@ -270,11 +270,11 @@ static void hang_handler(union sigval arg)
  igt_nsec_elapsed(&ctx->delay) / 1000.0);
  
  	igt_assert_eq(timer_delete(ctx->timer), 0);

-   free(ctx);
  
  	/* flush any excess work before we start timing our reset */

igt_assert(igt_sysfs_printf(ctx->debugfs, "i915_drop_caches",
"%d", DROP_RCU));
+   free(ctx);
  
  	igt_nsec_elapsed(ts);

igt_assert(igt_sysfs_printf(dir, "i915_wedged", "%llu", -1ull));

Re: [Intel-gfx] [PATCH] drm/i915: Fix error code in icl_compute_combo_phy_dpll()

2022-06-28 Thread Ville Syrjälä

On Fri, Jun 24, 2022 at 12:55:52PM +, Souza, Jose wrote:
> On Fri, 2022-06-24 at 09:39 +0300, Dan Carpenter wrote:
> > This function is supposed to return zero or negative error codes but it
> > accidentally returns true on failure.
> 
> Reviewed-by: José Roberto de Souza 

Thanks for the patch and review. Pushed to drm-intel-next.

> 
> > 
> > Fixes: 92a020747d6c ("drm/i915: Split shared dpll .get_dplls() into compute 
> > and get phases")
> > Signed-off-by: Dan Carpenter 
> > ---
> >  drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c 
> > b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > index ddae7e42ac46..118598c9a809 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > @@ -3184,7 +3184,7 @@ static int icl_compute_combo_phy_dpll(struct 
> > intel_atomic_state *state,
> > struct icl_port_dpll *port_dpll =
> > &crtc_state->icl_port_dplls[ICL_PORT_DPLL_DEFAULT];
> > struct skl_wrpll_params pll_params = {};
> > -   bool ret;
> > +   int ret;
> >  
> > if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI) ||
> > intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DSI))
> 

-- 
Ville Syrjälä
Intel

[Intel-gfx] [PATCH i-g-t] tests/i915/kms_big_fb: trigger async flip with a dummy flip

2022-06-28 Thread Arun R Murthy

In oder to trigger the async flip, a dummy flip is required after sync
flip so as to update the watermarks for async in KMD which happens as
part of this dummy flip. Thereafter async memory update will act as a
trigger register.
Capturing the CRC is done after the async flip as async flip at some
times can consume fairly a vblank period time.

Signed-off-by: Arun R Murthy 
---
 tests/i915/kms_big_fb.c | 29 +++--
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/tests/i915/kms_big_fb.c b/tests/i915/kms_big_fb.c
index d50fde45..6caf3c31 100644
--- a/tests/i915/kms_big_fb.c
+++ b/tests/i915/kms_big_fb.c
@@ -465,7 +465,7 @@ static bool test_pipe(data_t *data)
 static bool
 max_hw_stride_async_flip_test(data_t *data)
 {
-   uint32_t ret, startframe;
+   uint32_t ret, frame;
const uint32_t w = data->output->config.default_mode.hdisplay,
   h = data->output->config.default_mode.vdisplay;
igt_plane_t *primary;
@@ -519,7 +519,19 @@ max_hw_stride_async_flip_test(data_t *data)
  DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
 
igt_wait_for_vblank(data->drm_fd, 
data->display.pipes[primary->pipe->pipe].crtc_offset);
-   startframe = kmstest_get_vblank(data->drm_fd, data->pipe, 0) + 
1;
+   /*
+* In older platforms (<= Gen10), async address update bit is 
double buffered.
+* So flip timestamp can be verified only from the second flip.
+* The first async flip just enables the async address update.
+* In platforms greater than DISPLAY13 the first async flip 
will be discarded
+* in order to change the watermark levels as per the 
optimization. Hence the
+* subsequent async flips will actually do the asynchronous 
flips.
+*/
+   ret = drmModePageFlip(data->drm_fd, 
data->output->config.crtc->crtc_id,
+ 
data->big_fb_flip[i].fb_id,
+ DRM_MODE_PAGE_FLIP_ASYNC, 
NULL);
+   igt_wait_for_vblank(data->drm_fd, 
data->display.pipes[primary->pipe->pipe].crtc_offset);
+   frame = kmstest_get_vblank(data->drm_fd, data->pipe, 0) + 1;
 
for (int j = 0; j < 2; j++) {
do {
@@ -528,23 +540,20 @@ max_hw_stride_async_flip_test(data_t *data)
  DRM_MODE_PAGE_FLIP_ASYNC, 
NULL);
} while (ret == -EBUSY);
igt_assert(ret == 0);
+   igt_pipe_crc_get_for_frame(data->drm_fd, data->pipe_crc,
+  frame, &compare_crc);
 
+   frame = kmstest_get_vblank(data->drm_fd, data->pipe, 0) 
+ 1;
do {
ret = drmModePageFlip(data->drm_fd, 
data->output->config.crtc->crtc_id,
  data->big_fb.fb_id,
  DRM_MODE_PAGE_FLIP_ASYNC, 
NULL);
} while (ret == -EBUSY);
igt_assert(ret == 0);
+   igt_pipe_crc_get_for_frame(data->drm_fd, data->pipe_crc,
+  frame, &async_crc);
}
 
-   igt_pipe_crc_get_for_frame(data->drm_fd, data->pipe_crc,
-  startframe, &compare_crc);
-   igt_pipe_crc_get_for_frame(data->drm_fd, data->pipe_crc,
-  startframe + 1, &async_crc);
-
-   igt_assert_f(kmstest_get_vblank(data->drm_fd, data->pipe, 0) -
-startframe == 1, "lost frames\n");
-
igt_assert_f(igt_check_crc_equal(&compare_crc, 
&async_crc)^(i^1),
 "CRC failure with async flip, crc %s match for 
checked round\n",
 i?"should":"shouldn't");
-- 
2.25.1

Re: [Intel-gfx] [PATCH] drm/fourcc: Document the Intel CCS modifiers' CC plane expected pitch

2022-06-28 Thread Imre Deak

On Sat, Jun 25, 2022 at 12:38:50AM +0300, Chery, Nanley G wrote:
> +Jordan (FYI)
> 
> I think the commit message has an extra "color" next to "CC".

IIUC, i915 calls all the planes contained in a framebuffer "color
planes" for consistency (so main-, aux-, CC color planes).

> With or without that dropped,
> 
> Reviewed-by: Nanley Chery 

The patch is pushed to drm-misc-next, thanks for the reviews.

--Imre

> Thanks for the fix.
> 
> > -Original Message-
> > From: Deak, Imre 
> > Sent: Thursday, June 23, 2022 10:50 AM
> > To: dri-de...@lists.freedesktop.org
> > Cc: intel-gfx@lists.freedesktop.org; Chery, Nanley G 
> > 
> > Subject: [PATCH] drm/fourcc: Document the Intel CCS modifiers' CC plane 
> > expected pitch
> >
> > The driver expects the pitch of the Intel CCS CC color planes to be
> > 64 bytes aligned, adjust the modifier descriptions accordingly.
> >
> > Cc: Nanley Chery 
> > Signed-off-by: Imre Deak 
> > ---
> >  include/uapi/drm/drm_fourcc.h | 8 
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> > index f1972154a5940..c1b4cfda75075 100644
> > --- a/include/uapi/drm/drm_fourcc.h
> > +++ b/include/uapi/drm/drm_fourcc.h
> > @@ -559,7 +559,7 @@ extern "C" {
> >   *
> >   * The main surface is Y-tiled and is at plane index 0 whereas CCS is 
> > linear
> >   * and at index 1. The clear color is stored at index 2, and the pitch 
> > should
> > - * be ignored. The clear color structure is 256 bits. The first 128 bits
> > + * be 64 bytes aligned. The clear color structure is 256 bits. The first 
> > 128 bits
> >   * represents Raw Clear Color Red, Green, Blue and Alpha color each 
> > represented
> >   * by 32 bits. The raw clear color is consumed by the 3d engine and 
> > generates
> >   * the converted clear color of size 64 bits. The first 32 bits store the 
> > Lower
> > @@ -612,9 +612,9 @@ extern "C" {
> >   * outside of the GEM object in a reserved memory area dedicated for the
> >   * storage of the CCS data for all RC/RC_CC/MC compressible GEM objects. 
> > The
> >   * main surface pitch is required to be a multiple of four Tile 4 widths. 
> > The
> > - * clear color is stored at plane index 1 and the pitch should be ignored. 
> > The
> > - * format of the 256 bits of clear color data matches the one used for the
> > - * I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC modifier, see its description
> > + * clear color is stored at plane index 1 and the pitch should be 64 bytes
> > + * aligned. The format of the 256 bits of clear color data matches the one 
> > used
> > + * for the I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC modifier, see its 
> > description
> >   * for details.
> >   */
> >  #define I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC fourcc_mod_code(INTEL, 12)
> > --
> > 2.30.2
>

Re: [Intel-gfx] [PATCH i-g-t 2/3] tests/i915/kms_mmap_write_crc: handle missing gem_get_caching()

2022-06-28 Thread Gwan-gyeong Mun


Looks good to me.

Reviewed-by: Gwan-gyeong Mun 

On 6/27/22 7:10 PM, Matthew Auld wrote:

The kernel is meant to force the caching level for the object to
CACHE_NONE or CACHE_WT when first scanning out the object, since the
display engine is not coherent (assuming userspace hasn't already done
this). On discrete we no longer support set/get_caching, but we can only
do the scanout from lmem, which can only be mapped as WC and so should
always be coherent for scanout. Adjust the test and ensure it still
passes as expected.

Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5303
Signed-off-by: Matthew Auld 
Cc: Gwan-gyeong Mun 
---
  tests/i915/kms_mmap_write_crc.c | 11 +++
  1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/tests/i915/kms_mmap_write_crc.c b/tests/i915/kms_mmap_write_crc.c
index b17e5fdb..da7312d6 100644
--- a/tests/i915/kms_mmap_write_crc.c
+++ b/tests/i915/kms_mmap_write_crc.c
@@ -78,7 +78,6 @@ static void test(data_t *data)
drmModeModeInfo *mode;
cairo_t *cr;
char *ptr;
-   uint32_t caching;
void *buf;
igt_crc_t crc;
  
@@ -102,9 +101,13 @@ static void test(data_t *data)

igt_plane_set_fb(data->primary, &data->fb[0]);
igt_display_commit(display);
  
-	/* make sure caching mode has become UC/WT */

-   caching = gem_get_caching(data->drm_fd, fb->gem_handle);
-   igt_assert(caching == I915_CACHING_NONE || caching == 
I915_CACHING_DISPLAY);
+   if (!gem_has_lmem(data->drm_fd)) {
+   uint32_t caching;
+
+   /* make sure caching mode has become UC/WT */
+   caching = gem_get_caching(data->drm_fd, fb->gem_handle);
+   igt_assert(caching == I915_CACHING_NONE || caching == 
I915_CACHING_DISPLAY);
+   }
  
  	/*

 * firstly demonstrate the need for DMA_BUF_SYNC_START 
("begin_cpu_access")

Re: [Intel-gfx] [PATCH i-g-t 1/3] tests/i915/gem_eio: fix uaf

2022-06-28 Thread Matthew Auld


On 28/06/2022 11:24, Gwan-gyeong Mun wrote:

Looks good to me.

Reviewed-by: Gwan-gyeong Mun 

However, the use after free build issue did not occur only with the "$ 
meson build && ninja -C build" build command guided by the igt 
README.md. How did you check it?


Hmm, I assume it's just a difference in compiler version or so. I have: 
gcc (GCC) 12.1.1 20220507 (Red Hat 12.1.1-1).




Br,

G.G.


On 6/27/22 7:10 PM, Matthew Auld wrote:
../tests/i915/gem_eio.c:277:20: warning: pointer ‘ctx’ used after 
‘free’ [-Wuse-after-free]
   277 | igt_assert(igt_sysfs_printf(ctx->debugfs, 
"i915_drop_caches",

../lib/igt_core.h:667:20: note: in definition of macro ‘igt_assert’
   667 | do { if (!(expr)) \
   |    ^~~~
../tests/i915/gem_eio.c:274:9: note: call to ‘free’ here
   274 | free(ctx);

Signed-off-by: Matthew Auld 
Cc: Gwan-gyeong Mun 
---
  tests/i915/gem_eio.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/i915/gem_eio.c b/tests/i915/gem_eio.c
index 913a21f9..6cbae6eb 100644
--- a/tests/i915/gem_eio.c
+++ b/tests/i915/gem_eio.c
@@ -270,11 +270,11 @@ static void hang_handler(union sigval arg)
    igt_nsec_elapsed(&ctx->delay) / 1000.0);
  igt_assert_eq(timer_delete(ctx->timer), 0);
-    free(ctx);
  /* flush any excess work before we start timing our reset */
  igt_assert(igt_sysfs_printf(ctx->debugfs, "i915_drop_caches",
  "%d", DROP_RCU));
+    free(ctx);
  igt_nsec_elapsed(ts);
  igt_assert(igt_sysfs_printf(dir, "i915_wedged", "%llu", -1ull));

[Intel-gfx] [PATCH i-g-t 1/3] lib/igt_device_scan: fix dangling pointer

2022-06-28 Thread Matthew Auld

It looks like the linkto is out of scope:

../lib/igt_device_scan.c: In function ‘igt_device_add_attr’:
../lib/igt_device_scan.c:368:57: warning: dangling pointer ‘v’ to ‘linkto’ may 
be used [-Wdangling-pointer=]
  368 | g_hash_table_insert(dev->attrs_ht, strdup(key), strdup(v));
  | ^
../lib/igt_device_scan.c:351:22: note: ‘linkto’ declared here
  351 | char linkto[PATH_MAX];

Signed-off-by: Matthew Auld 
Cc: Petri Latvala 
---
 lib/igt_device_scan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c
index a1cee7a4..5d1d4258 100644
--- a/lib/igt_device_scan.c
+++ b/lib/igt_device_scan.c
@@ -338,6 +338,7 @@ static void igt_device_add_attr(struct igt_device *dev,
const char *key, const char *value)
 {
const char *v = value;
+   char linkto[PATH_MAX];
 
if (!key)
return;
@@ -348,7 +349,6 @@ static void igt_device_add_attr(struct igt_device *dev,
if (!v) {
struct stat st;
char path[PATH_MAX];
-   char linkto[PATH_MAX];
int len;
 
snprintf(path, sizeof(path), "%s/%s", dev->syspath, key);
-- 
2.36.1

[Intel-gfx] [PATCH i-g-t 2/3] tests/kms_cursor_crc: fix truncated warning

2022-06-28 Thread Matthew Auld

Looks reasonable to just increase the size of 'name' to avoid the
potential truncation:

../tests/kms_cursor_crc.c: In function ‘run_size_tests.constprop’:
../tests/kms_cursor_crc.c:699:50: warning: ‘%d’ directive output may be 
truncated writing between 1 and 11 bytes into a region of size between 4 and 14 
[-Wformat-truncation=]
  699 | snprintf(name, sizeof(name), "%dx%d", w, h);
  |  ^~
../tests/kms_cursor_crc.c:699:46: note: directive argument in the range 
[-2147483648, 1024]
  699 | snprintf(name, sizeof(name), "%dx%d", w, h);
  |  ^~~
In file included from /usr/include/stdio.h:894,
 from ../lib/igt_core.h:38,
 from ../lib/drmtest.h:39,
 from ../lib/igt.h:27,
 from ../tests/kms_cursor_crc.c:25:
In function ‘snprintf’,
inlined from ‘run_size_tests.constprop’ at ../tests/kms_cursor_crc.c:699:3:
/usr/include/bits/stdio2.h:71:10: note: ‘__builtin___snprintf_chk’ output 
between 4 and 24 bytes into a destination of size 16
   71 |   return __builtin___snprintf_chk (__s, __n, __USE_FORTIFY_LEVEL - 1,
  |  ^~~~
   72 |__glibc_objsize (__s), __fmt,
  |~
   73 |__va_arg_pack ());
  |~

Signed-off-by: Matthew Auld 
Cc: Petri Latvala 
---
 tests/kms_cursor_crc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/kms_cursor_crc.c b/tests/kms_cursor_crc.c
index 513c9715..131fdb0a 100644
--- a/tests/kms_cursor_crc.c
+++ b/tests/kms_cursor_crc.c
@@ -691,7 +691,7 @@ static void test_rapid_movement(data_t *data)
 static void run_size_tests(data_t *data, enum pipe pipe,
   int w, int h)
 {
-   char name[16];
+   char name[32];
 
if (w == 0 && h == 0)
strcpy(name, "max-size");
-- 
2.36.1

[Intel-gfx] [PATCH i-g-t 3/3] tests/amdgpu/amdgpu_command_submission: fix uaf

2022-06-28 Thread Matthew Auld

../lib/amdgpu/amd_command_submission.c: In function 
‘amdgpu_command_submission_write_linear_helper’:
../lib/amdgpu/amd_command_submission.c:201:13: warning: pointer ‘ring_context’ 
used after ‘free’ [-Wuse-after-free]
  201 | r = amdgpu_cs_ctx_free(ring_context->context_handle);
  | ^~~~
../lib/amdgpu/amd_command_submission.c:199:9: note: call to ‘free’ here
  199 | free(ring_context);
  | ^~

Signed-off-by: Matthew Auld 
Cc: Petri Latvala 
---
 lib/amdgpu/amd_command_submission.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/amdgpu/amd_command_submission.c 
b/lib/amdgpu/amd_command_submission.c
index 4dc4df95..16939653 100644
--- a/lib/amdgpu/amd_command_submission.c
+++ b/lib/amdgpu/amd_command_submission.c
@@ -196,10 +196,10 @@ void 
amdgpu_command_submission_write_linear_helper(amdgpu_device_handle device,
}
/* clean resources */
free(ring_context->pm4);
-   free(ring_context);
/* end of test */
r = amdgpu_cs_ctx_free(ring_context->context_handle);
igt_assert_eq(r, 0);
+   free(ring_context);
 }
 
 
-- 
2.36.1

Re: [Intel-gfx] [PATCH v3 3/3] drm/doc/rfc: VM_BIND uapi definition

2022-06-28 Thread Zeng, Oak



Thanks,
Oak

> -Original Message-
> From: Tvrtko Ursulin 
> Sent: June 28, 2022 4:58 AM
> To: Zeng, Oak ; Landwerlin, Lionel G
> ; Vishwanathapura, Niranjana
> 
> Cc: Zanoni, Paulo R ; intel-
> g...@lists.freedesktop.org; dri-de...@lists.freedesktop.org; Hellstrom,
> Thomas ; Wilson, Chris P
> ; Vetter, Daniel ;
> christian.koe...@amd.com; Auld, Matthew 
> Subject: Re: [Intel-gfx] [PATCH v3 3/3] drm/doc/rfc: VM_BIND uapi definition
> 
> 
> On 27/06/2022 19:58, Zeng, Oak wrote:
> >
> >
> > Thanks,
> > Oak
> >
> >> -Original Message-
> >> From: Tvrtko Ursulin 
> >> Sent: June 27, 2022 4:30 AM
> >> To: Zeng, Oak ; Landwerlin, Lionel G
> >> ; Vishwanathapura, Niranjana
> >> 
> >> Cc: Zanoni, Paulo R ; intel-
> >> g...@lists.freedesktop.org; dri-de...@lists.freedesktop.org;
> >> Hellstrom, Thomas ; Wilson, Chris P
> >> ; Vetter, Daniel ;
> >> christian.koe...@amd.com; Auld, Matthew 
> >> Subject: Re: [Intel-gfx] [PATCH v3 3/3] drm/doc/rfc: VM_BIND uapi
> >> definition
> >>
> >>
> >> On 24/06/2022 21:23, Zeng, Oak wrote:
> >>> Let's compare "tlb invalidate at vm unbind" vs "tlb invalidate at
> >>> backing
> >> storage":
> >>>
> >>> Correctness:
> >>> consider this sequence of:
> >>> 1. unbind va1 from pa1,
> >>> 2. then bind va1 to pa2. //user space has the freedom to do this as
> >>> it manages virtual address space 3. Submit shader code using va1, 4.
> >>> Then retire pa1.
> >>>
> >>> If you don't perform tlb invalidate at step #1, in step #3, shader
> >>> will use
> >> stale entries in tlb and pa1 will be used for the shader. User want to use
> pa2.
> >> So I don't think invalidate tlb at step #4 make correctness.
> >>
> >> Define step 3. Is it a new execbuf? If so then there will be a TLB flush
> there.
> >> Unless the plan is to stop doing that with eb3 but I haven't picked
> >> up on that anywhere so far.
> >
> > In Niranjana's latest patch series, he removed the TLB flushing from
> vm_unbind. He also said explicitly TLB invalidation will be performed at job
> submission and backing storage releasing time, which is the existing behavior
> of the current i915 driver.
> >
> > I think if we invalidate TLB on each vm_unbind, then we don't need to
> invalidate at submission and backing storage releasing. It doesn't make a lot
> of sense to me to perform a tlb invalidation at execbuf time. Maybe it is a
> behavior for the old implicit binding programming model. For vm_bind and
> eb3, we separate the binding and job submission into two APIs. It is more
> natural the TLB invalidation be coupled with the vm bind/unbind, not job
> submission. So in my opinion we should remove tlb invalidation from
> submission and backing storage releasing and add it to vm unbind. This is
> method is cleaner to me.
> 
> You can propose this model (not flushing in eb3) but I have my doubts.
> Consider the pointlessness of flushing on N unbinds for 99% of clients which
> are not infinite compute batch. And consider how you make the behaviour
> consistent on all platforms (selective vs global tlb flush).

When I thought about eb3, compute workload and ulls were also in the picture. 
Under ulls, user mode keep submitting job without calling execbuf (it uses a 
semaphore to notify HW of the new batch). The execbuf + backing release flush 
has a correctness issue as I pointed out. Now we decided eb3 is only for mesa, 
not for compute, we don't have this correctness problem for now. We can close 
this conversation for now and revive it when we move to Xe and vm bind for 
compute.

Regards,
Oak


> 
> Also note that this discussion is orthogonal to unbind vs backing store 
> release.
> 
> > Regarding performance, we don't have data. In my opinion, we should
> make things work in a most straight forward way as the first step. Then
> consider performance improvement if necessary. Consider some delayed tlb
> invalidation at submission and backing release time without performance
> data support wasn't a good decision.
> 
> It is quite straightforward though. ;) It aligns with the eb2 model and
> argument can be made backing store release is (much) less frequent than
> unbind (consider softpin where client could trigger a lot of pointless 
> flushes).
> 
> Regards,
> 
> Tvrtko

Re: [Intel-gfx] [PATCH i-g-t 1/3] tests/i915/gem_eio: fix uaf

2022-06-28 Thread Gwan-gyeong Mun





On 6/28/22 3:47 PM, Matthew Auld wrote:

On 28/06/2022 11:24, Gwan-gyeong Mun wrote:

Looks good to me.

Reviewed-by: Gwan-gyeong Mun 

However, the use after free build issue did not occur only with the "$ 
meson build && ninja -C build" build command guided by the igt 
README.md. How did you check it?


Hmm, I assume it's just a difference in compiler version or so. I have: 
gcc (GCC) 12.1.1 20220507 (Red Hat 12.1.1-1).



Thanks for sharing your compiling environment info.
My gcc says its version is 11.1.0.
I'll try with the version you mentioned.

$ gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-pc-linux-gnu/11.1.0/lto-wrapper
Target: x86_64-pc-linux-gnu
Configured with: /build/gcc/src/gcc/configure --prefix=/usr 
--libdir=/usr/lib --libexecdir=/usr/lib --mandir=/usr/share/man 
--infodir=/usr/share/info --with-bugurl=https://bugs.archlinux.org/ 
--enable-languages=c,c++,ada,fortran,go,lto,objc,obj-c++,d --with-isl 
--with-linker-hash-style=gnu --with-system-zlib --enable-__cxa_atexit 
--enable-cet=auto --enable-checking=release --enable-clocale=gnu 
--enable-default-pie --enable-default-ssp --enable-gnu-indirect-function 
--enable-gnu-unique-object --enable-install-libiberty 
--enable-linker-build-id --enable-lto --enable-multilib --enable-plugin 
--enable-shared --enable-threads=posix --disable-libssp 
--disable-libstdcxx-pch --disable-libunwind-exceptions --disable-werror 
gdc_include_dir=/usr/include/dlang/gdc

Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 11.1.0 (GCC)


Thanks,
G.G.



Br,

G.G.


On 6/27/22 7:10 PM, Matthew Auld wrote:
../tests/i915/gem_eio.c:277:20: warning: pointer ‘ctx’ used after 
‘free’ [-Wuse-after-free]
   277 | igt_assert(igt_sysfs_printf(ctx->debugfs, 
"i915_drop_caches",

../lib/igt_core.h:667:20: note: in definition of macro ‘igt_assert’
   667 | do { if (!(expr)) \
   |    ^~~~
../tests/i915/gem_eio.c:274:9: note: call to ‘free’ here
   274 | free(ctx);

Signed-off-by: Matthew Auld 
Cc: Gwan-gyeong Mun 
---
  tests/i915/gem_eio.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/i915/gem_eio.c b/tests/i915/gem_eio.c
index 913a21f9..6cbae6eb 100644
--- a/tests/i915/gem_eio.c
+++ b/tests/i915/gem_eio.c
@@ -270,11 +270,11 @@ static void hang_handler(union sigval arg)
    igt_nsec_elapsed(&ctx->delay) / 1000.0);
  igt_assert_eq(timer_delete(ctx->timer), 0);
-    free(ctx);
  /* flush any excess work before we start timing our reset */
  igt_assert(igt_sysfs_printf(ctx->debugfs, "i915_drop_caches",
  "%d", DROP_RCU));
+    free(ctx);
  igt_nsec_elapsed(ts);
  igt_assert(igt_sysfs_printf(dir, "i915_wedged", "%llu", -1ull));

Re: [Intel-gfx] [PATCH 5/6] drm/i915/gt: Serialize GRDOM access between multiple engine resets

2022-06-28 Thread Mauro Carvalho Chehab

Hi Tvrtko,

On Fri, 24 Jun 2022 09:34:21 +0100
Tvrtko Ursulin  wrote:

> On 23/06/2022 12:17, Andi Shyti wrote:
> > Hi Mauro,
> > 
> > On Wed, Jun 15, 2022 at 04:27:39PM +0100, Mauro Carvalho Chehab wrote:  
> >> From: Chris Wilson 
> >>
> >> Don't allow two engines to be reset in parallel, as they would both
> >> try to select a reset bit (and send requests to common registers)
> >> and wait on that register, at the same time. Serialize control of
> >> the reset requests/acks using the uncore->lock, which will also ensure
> >> that no other GT state changes at the same time as the actual reset.
> >>
> >> Fixes: 7938d61591d3 ("drm/i915: Flush TLBs before releasing backing store")
> >>
> >> Reported-by: Mika Kuoppala 
> >> Signed-off-by: Chris Wilson 
> >> Cc: Mika Kuoppala 
> >> Cc: Andi Shyti 
> >> Cc: sta...@vger.kernel.org
> >> Acked-by: Thomas Hellström 
> >> Signed-off-by: Mauro Carvalho Chehab   
> > 
> > Reviewed-by: Andi Shyti   
> 
> Notice I had a bunch of questions and asks in this series so please do 
> not merge until those are addressed.
> 
> In this particular patch (and some others) for instance Fixes: tag, at 
> least against that sha, shouldn't be there.

Hmm... I sent an answer to your points, but I can't see it at:


https://lore.kernel.org/all/160e613f-a0a8-18ff-5d4b-249d4280c...@linux.intel.com/

Maybe it got lost somewhere, I dunno.

Yeah, indeed the fixes tag on patch 5/6 should be removed as this is not
directly related to changeset 7938d61591d3. Yet, this one is required for
patch 6 to work.

The other patches on this series, though, are modifying the code 
introduced by changeset 7938d61591d3.

Patch 2 is clearly a workaround needed for TLB cache invalidation to
work on some GPUs. So, while not related to Broadwell, they're also
fixing some TLB cache issues. So, IMO, it should keep the fixes.

I tried to port just the two serialize patches to drm-tip, in order
to solve the issues on Broadwell, but it didn't work, as the logic 
inside the spinlock could be calling schedule() with a spinlock hold:
 
Jun 14 17:38:48 silver kernel: [   23.227813] BUG: sleeping function 
called from invalid context at drivers/gpu/drm/i915/intel_uncore.c:2496
Jun 14 17:38:48 silver kernel: [   23.227816] in_atomic(): 1, 
irqs_disabled(): 1, non_block: 0, pid: 37, name: kworker/u8:1
Jun 14 17:38:48 silver kernel: [   23.227818] preempt_count: 1, 
expected: 0
Jun 14 17:38:48 silver kernel: [   23.227819] RCU nest depth: 0, 
expected: 0
Jun 14 17:38:48 silver kernel: [   23.227820] 5 locks held by 
kworker/u8:1/37:
Jun 14 17:38:48 silver kernel: [   23.227822]  #0: 88811159b538 
((wq_completion)i915){+.+.}-{0:0}, at: process_one_work+0x1e0/0x580
Jun 14 17:38:48 silver kernel: [   23.227831]  #1: c9183e60 
((work_completion)(&(&i915->mm.free_work)->work)){+.+.}-{0:0}, at: 
process_one_work+0x1e0/0x580
Jun 14 17:38:48 silver kernel: [   23.227837]  #2: 88811b34c5e8 
(reservation_ww_class_mutex){+.+.}-{3:3}, at: 
__i915_gem_free_objects+0xba/0x210 [i915]
Jun 14 17:38:48 silver kernel: [   23.228283]  #3: 88810a66c2d8 
(>->tlb_invalidate_lock){+.+.}-{3:3}, at: intel_gt_invalidate_tlbs+0xe7/0x4d0 
[i915]
Jun 14 17:38:48 silver kernel: [   23.228663]  #4: 88810a668f28 
(&uncore->lock){-.-.}-{2:2}, at: intel_gt_invalidate_tlbs+0x115/0x4d0 [i915]

I didn't investigate the root cause, but it seems related to PM, so 
patches 1 and 3 seem to be required for the serialization logic
to actually work.

So, I would keep the Fixes: tag mentioning changeset 7938d61591d3
on patches: 1, 2, 3 and 6.

Yet, IMO the entire series should be merged on -stable.

If that's OK for you and there's no additional issues to be
addressed, I'll submit a v2 of this series removing the Fixes tag
from patches 4 and 5.

Regards,
Mauro

[Intel-gfx] [PATCH v2] drm/i915: use DISPLAY_VER() instead of accessing match_info directly

2022-06-28 Thread Jani Nikula

We've just set up device info in i915_driver_create() so we can use
DISPLAY_VER() intead of looking at match_info directly.

Semantically we want to check the display version instead of the
graphics version, and for the earlier platforms they are always the
same.

v2: Use DISPLAY_VER() instead of GRAPHICS_VER() (Ville)

Signed-off-by: Jani Nikula 
Reviewed-by: Ville Syrjälä 
---
 drivers/gpu/drm/i915/i915_driver.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_driver.c 
b/drivers/gpu/drm/i915/i915_driver.c
index 6e5849c1086f..b2e14cd76d7e 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -847,8 +847,6 @@ i915_driver_create(struct pci_dev *pdev, const struct 
pci_device_id *ent)
  */
 int i915_driver_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
-   const struct intel_device_info *match_info =
-   (struct intel_device_info *)ent->driver_data;
struct drm_i915_private *i915;
int ret;
 
@@ -857,7 +855,7 @@ int i915_driver_probe(struct pci_dev *pdev, const struct 
pci_device_id *ent)
return PTR_ERR(i915);
 
/* Disable nuclear pageflip by default on pre-ILK */
-   if (!i915->params.nuclear_pageflip && match_info->graphics.ver < 5)
+   if (!i915->params.nuclear_pageflip && DISPLAY_VER(i915) < 5)
i915->drm.driver_features &= ~DRIVER_ATOMIC;
 
ret = pci_enable_device(pdev);
-- 
2.30.2

Re: [Intel-gfx] [PATCH 01/16] drm/i915: use GRAPHICS_VER() instead of accessing match_info directly

2022-06-28 Thread Jani Nikula

On Thu, 23 Jun 2022, Ville Syrjälä  wrote:
> On Mon, Jun 20, 2022 at 11:37:40AM +0300, Jani Nikula wrote:
>> We've just set up device info in i915_driver_create() so we can use
>> GRAPHICS_VER() intead of looking at match_info directly.
>> 
>> Signed-off-by: Jani Nikula 
>> ---
>>  drivers/gpu/drm/i915/i915_driver.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_driver.c 
>> b/drivers/gpu/drm/i915/i915_driver.c
>> index d26dcca7e654..aeec3dfe3ebf 100644
>> --- a/drivers/gpu/drm/i915/i915_driver.c
>> +++ b/drivers/gpu/drm/i915/i915_driver.c
>> @@ -829,8 +829,6 @@ i915_driver_create(struct pci_dev *pdev, const struct 
>> pci_device_id *ent)
>>   */
>>  int i915_driver_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>  {
>> -const struct intel_device_info *match_info =
>> -(struct intel_device_info *)ent->driver_data;
>>  struct drm_i915_private *i915;
>>  int ret;
>>  
>> @@ -839,7 +837,7 @@ int i915_driver_probe(struct pci_dev *pdev, const struct 
>> pci_device_id *ent)
>>  return PTR_ERR(i915);
>>  
>>  /* Disable nuclear pageflip by default on pre-ILK */
>> -if (!i915->params.nuclear_pageflip && match_info->graphics.ver < 5)
>> +if (!i915->params.nuclear_pageflip && GRAPHICS_VER(i915) < 5)
>
> Should also be switched to DISPLAY_VER(), but that could be done as a
> separate patch too.
>
> Reviewed-by: Ville Syrjälä 

Thanks, I've sent this separately with s/GRAPHICS_VER/DISPLAY_VER/.

BR,
Jani.


>
>>  i915->drm.driver_features &= ~DRIVER_ATOMIC;
>>  
>>  ret = pci_enable_device(pdev);
>> -- 
>> 2.30.2

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915/guc/slpc: Add a new SLPC selftest (rev4)

2022-06-28 Thread Belgaumkar, Vinay



From: Patchwork 
Sent: Monday, June 27, 2022 10:00 PM
To: Belgaumkar, Vinay 
Cc: intel-gfx@lists.freedesktop.org
Subject: ✗ Fi.CI.BAT: failure for drm/i915/guc/slpc: Add a new SLPC selftest 
(rev4)

Patch Details
Series:
drm/i915/guc/slpc: Add a new SLPC selftest (rev4)
URL:
https://patchwork.freedesktop.org/series/105005/
State:
failure
Details:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105005v4/index.html
CI Bug Log - changes from CI_DRM_11816 -> Patchwork_105005v4
Summary

FAILURE

Serious unknown changes coming with Patchwork_105005v4 absolutely need to be
verified manually.

If you think the reported changes have nothing to do with the changes
introduced in Patchwork_105005v4, please notify your bug team to allow them
to document this new failure mode, which will reduce false positives in CI.

External URL: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105005v4/index.html

Participating hosts (40 -> 39)

Additional (3): fi-hsw-4770 bat-adlm-1 fi-icl-u2
Missing (4): bat-dg2-8 fi-rkl-11600 fi-bdw-samus bat-dg1-5

Possible new issues

Here are the unknown changes that may have been introduced in 
Patchwork_105005v4:

IGT changes
Possible regressions

  *   igt@i915_selftest@live@workarounds:
 *   fi-bdw-5557u: 
PASS
 -> 
INCOMPLETE
This failure is not related to the patch, bdw does not have SLPC.
Thanks,
Vinay.
Suppressed

The following results come from untrusted machines, tests, or statuses.
They do not affect the overall result.

  *   igt@i915_selftest@live@perf:
 *   {bat-adln-1}: NOTRUN -> 
DMESG-FAIL

Known issues

Here are the changes found in Patchwork_105005v4 that come from known issues:

IGT changes
Issues hit

  *   igt@gem_huc_copy@huc-copy:
 *   fi-icl-u2: NOTRUN -> 
SKIP
 (i915#2190)
  *   igt@gem_lmem_swapping@random-engines:
 *   fi-icl-u2: NOTRUN -> 
SKIP
 (i915#4613) +3 similar 
issues
  *   igt@i915_pm_backlight@basic-brightness:
 *   fi-hsw-4770: NOTRUN -> 
SKIP
 (fdo#109271 / 
i915#3012)
  *   igt@i915_pm_rpm@module-reload:
 *   fi-cfl-8109u: 
PASS
 -> 
DMESG-FAIL
 (i915#62)
 *   bat-adlp-4: 
PASS
 -> 
DMESG-WARN
 (i915#3576) +1 similar 
issue
  *   igt@i915_selftest@live@execlists:
 *   fi-bsw-nick: 
PASS
 -> 
INCOMPLETE
 (i915#5847)
  *   igt@i915_selftest@live@hangcheck:
 *   fi-hsw-g3258: 
PASS
 -> 
INCOMPLETE
 (i915#4785)
  *   igt@i915_selftest@live@ring_submission:
 *   fi-cfl-8109u: 
PASS
 -> 
DMESG-WARN
 (i915#5904) +11 similar 
issues
  *   igt@i915_suspend@basic-s2idle-without-i915:
 *   fi-cfl-8109u: 
PASS
 -> 
DMESG-WARN

Re: [Intel-gfx] [PATCH 5/6] drm/i915/gt: Serialize GRDOM access between multiple engine resets

2022-06-28 Thread Tvrtko Ursulin




Hi,

On 27/06/2022 10:00, Mauro Carvalho Chehab (by way of Mauro Carvalho 
Chehab ) wrote:

Hi Tvrtko,

On Fri, 24 Jun 2022 09:34:21 +0100
Tvrtko Ursulin  wrote:


On 23/06/2022 12:17, Andi Shyti wrote:

Hi Mauro,

On Wed, Jun 15, 2022 at 04:27:39PM +0100, Mauro Carvalho Chehab wrote:

From: Chris Wilson 

Don't allow two engines to be reset in parallel, as they would both
try to select a reset bit (and send requests to common registers)
and wait on that register, at the same time. Serialize control of
the reset requests/acks using the uncore->lock, which will also ensure
that no other GT state changes at the same time as the actual reset.

Fixes: 7938d61591d3 ("drm/i915: Flush TLBs before releasing backing store")

Reported-by: Mika Kuoppala 
Signed-off-by: Chris Wilson 
Cc: Mika Kuoppala 
Cc: Andi Shyti 
Cc: sta...@vger.kernel.org
Acked-by: Thomas Hellström 
Signed-off-by: Mauro Carvalho Chehab 


Reviewed-by: Andi Shyti 


Notice I had a bunch of questions and asks in this series so please do
not merge until those are addressed.

In this particular patch (and some others) for instance Fixes: tag, at
least against that sha, shouldn't be there.


Hmm... I sent an answer to your points, but I can't see it at:


https://lore.kernel.org/all/160e613f-a0a8-18ff-5d4b-249d4280c...@linux.intel.com/

Maybe it got lost somewhere, I dunno.


Yeah, no replies received on my end I'm afraid.



Yeah, indeed the fixes tag on patch 5/6 should be removed as this is not
directly related to changeset 7938d61591d3. Yet, this one is required for
patch 6 to work.

The other patches on this series, though, are modifying the code
introduced by changeset 7938d61591d3.


Modifying the code does not strictly means something is a fix for a 
certain patch.



Patch 2 is clearly a workaround needed for TLB cache invalidation to
work on some GPUs. So, while not related to Broadwell, they're also
fixing some TLB cache issues. So, IMO, it should keep the fixes.


Umesh commented that patch 2 is not needed - who is right then? :)


I tried to port just the two serialize patches to drm-tip, in order
to solve the issues on Broadwell, but it didn't work, as the logic
inside the spinlock could be calling schedule() with a spinlock hold:
  
	Jun 14 17:38:48 silver kernel: [   23.227813] BUG: sleeping function called from invalid context at drivers/gpu/drm/i915/intel_uncore.c:2496

Jun 14 17:38:48 silver kernel: [   23.227816] in_atomic(): 1, 
irqs_disabled(): 1, non_block: 0, pid: 37, name: kworker/u8:1
Jun 14 17:38:48 silver kernel: [   23.227818] preempt_count: 1, 
expected: 0
Jun 14 17:38:48 silver kernel: [   23.227819] RCU nest depth: 0, 
expected: 0
Jun 14 17:38:48 silver kernel: [   23.227820] 5 locks held by 
kworker/u8:1/37:
Jun 14 17:38:48 silver kernel: [   23.227822]  #0: 88811159b538 
((wq_completion)i915){+.+.}-{0:0}, at: process_one_work+0x1e0/0x580
Jun 14 17:38:48 silver kernel: [   23.227831]  #1: c9183e60 
((work_completion)(&(&i915->mm.free_work)->work)){+.+.}-{0:0}, at: 
process_one_work+0x1e0/0x580
Jun 14 17:38:48 silver kernel: [   23.227837]  #2: 88811b34c5e8 
(reservation_ww_class_mutex){+.+.}-{3:3}, at: 
__i915_gem_free_objects+0xba/0x210 [i915]
Jun 14 17:38:48 silver kernel: [   23.228283]  #3: 88810a66c2d8 
(>->tlb_invalidate_lock){+.+.}-{3:3}, at: intel_gt_invalidate_tlbs+0xe7/0x4d0 
[i915]
Jun 14 17:38:48 silver kernel: [   23.228663]  #4: 88810a668f28 
(&uncore->lock){-.-.}-{2:2}, at: intel_gt_invalidate_tlbs+0x115/0x4d0 [i915]

I didn't investigate the root cause, but it seems related to PM, so
patches 1 and 3 seem to be required for the serialization logic
to actually work.


Yes that is clear, what is needed is the split of the for_each_engine 
loop into request and wait.


But question is how much backporting trouble will the _extra_ changes 
patch 1 brings create.


In the ideal world patch 1 wouldn't be an optimising one, I mean adding 
skipping of TLB invalidations on idle engines but just the loop split. 
That would make it smaller and more suitable for Cc: stable. Because 
both i915_gem_pages.c and intel_gt_pm.h hunks wouldn't even be there. 
And the refactor in intel_gt_invalidate_tlbs would be smaller since it 
wouldn't be adding the engine awake checks...



So, I would keep the Fixes: tag mentioning changeset 7938d61591d3
on patches: 1, 2, 3 and 6.


... which for me means a different patch 1, followed by patch 6 (moved 
to be patch 2) would be ideal stable material.


Then we have the current patch 2 which is open/unknown (to me at least).

And the rest seem like optimisations which shouldn't be tagged as fixes.

Apart from patch 5 which should be cc: stable, but no fixes as agreed.

Could you please double check if what I am suggesting here is feasible 
to implement and if it is just send those minimal patches out alone?


Maybe it even makes sense to squash such 1&2 into a single patch.

Again

73 matches

Mail list logo