Re: [Intel-gfx] [PATCH] drm/i915: avoid concurrent writes to aux_inv

[Chris Wilson]

Quoting fei.y...@intel.com (2022-03-02 18:26:57)
> From: Fei Yang 
> 
> GPU hangs have been observed when multiple engines write to the
> same aux_inv register at the same time. To avoid this each engine
> should only invalidate its own auxiliary table. The function
> gen12_emit_flush_xcs() currently invalidate the auxiliary table for
> all engines because the rq->engine is not necessarily the engine
> eventually carrying out the request, and potentially the engine
> could even be a virtual one (with engine->instance being -1).
> With this patch, auxiliary table invalidation is done only for the
> engine executing the request. And the mmio address for the aux_inv
> register is set after the engine instance becomes certain.
> 
> Signed-off-by: Chris Wilson 
> Signed-off-by: Fei Yang 
> ---
>  drivers/gpu/drm/i915/gt/gen8_engine_cs.c  | 41 ---
>  .../drm/i915/gt/intel_execlists_submission.c  | 38 +
>  drivers/gpu/drm/i915/i915_request.h   |  2 +
>  3 files changed, 47 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c 
> b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> index b1b9c3fd7bf9..af62e2bc2c9b 100644
> --- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> @@ -165,30 +165,6 @@ static u32 preparser_disable(bool state)
> return MI_ARB_CHECK | 1 << 8 | state;
>  }
>  
> -static i915_reg_t aux_inv_reg(const struct intel_engine_cs *engine)
> -{
> -   static const i915_reg_t vd[] = {
> -   GEN12_VD0_AUX_NV,
> -   GEN12_VD1_AUX_NV,
> -   GEN12_VD2_AUX_NV,
> -   GEN12_VD3_AUX_NV,
> -   };
> -
> -   static const i915_reg_t ve[] = {
> -   GEN12_VE0_AUX_NV,
> -   GEN12_VE1_AUX_NV,
> -   };
> -
> -   if (engine->class == VIDEO_DECODE_CLASS)
> -   return vd[engine->instance];
> -
> -   if (engine->class == VIDEO_ENHANCEMENT_CLASS)
> -   return ve[engine->instance];
> -
> -   GEM_BUG_ON("unknown aux_inv reg\n");
> -   return INVALID_MMIO_REG;
> -}
> -
>  static u32 *gen12_emit_aux_table_inv(const i915_reg_t inv_reg, u32 *cs)
>  {
> *cs++ = MI_LOAD_REGISTER_IMM(1);
> @@ -288,7 +264,7 @@ int gen12_emit_flush_xcs(struct i915_request *rq, u32 
> mode)
> if (mode & EMIT_INVALIDATE)
> aux_inv = rq->engine->mask & ~BIT(BCS0);
> if (aux_inv)
> -   cmd += 2 * hweight32(aux_inv) + 2;
> +   cmd += 4;
>  
> cs = intel_ring_begin(rq, cmd);
> if (IS_ERR(cs))
> @@ -319,16 +295,13 @@ int gen12_emit_flush_xcs(struct i915_request *rq, u32 
> mode)
> *cs++ = 0; /* value */
>  
> if (aux_inv) { /* hsdes: 1809175790 */
> -   struct intel_engine_cs *engine;
> -   unsigned int tmp;
> -
> -   *cs++ = MI_LOAD_REGISTER_IMM(hweight32(aux_inv));
> -   for_each_engine_masked(engine, rq->engine->gt, aux_inv, tmp) {
> -   *cs++ = i915_mmio_reg_offset(aux_inv_reg(engine));
> -   *cs++ = AUX_INV;
> -   }
> +   *cs++ = MI_LOAD_REGISTER_IMM(1);
> +   rq->vd_ve_aux_inv = cs;
> +   *cs++ = 0; /* address to be set at submission to HW */
> +   *cs++ = AUX_INV;
> *cs++ = MI_NOOP;
> -   }
> +   } else
> +   rq->vd_ve_aux_inv = NULL;
>  
> if (mode & EMIT_INVALIDATE)
> *cs++ = preparser_disable(false);
> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c 
> b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> index 1c602d4ae297..a018de6dcac5 100644
> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> @@ -1258,6 +1258,34 @@ static bool completed(const struct i915_request *rq)
> return __i915_request_is_complete(rq);
>  }
>  
> +static i915_reg_t aux_inv_reg(const struct intel_engine_cs *engine)
> +{
> +   static const i915_reg_t vd[] = {
> +   GEN12_VD0_AUX_NV,
> +   GEN12_VD1_AUX_NV,
> +   GEN12_VD2_AUX_NV,
> +   GEN12_VD3_AUX_NV,
> +   };
> +
> +   static const i915_reg_t ve[] = {
> +   GEN12_VE0_AUX_NV,
> +   GEN12_VE1_AUX_NV,
> +   };
> +
> +   if (engine->class == VIDEO_DECODE_CLASS) {
> +   GEM_BUG_ON(engine->instance >= ARRAY_SIZE(vd));
> +   return vd[engine->insta

Re: [Intel-gfx] [PATCH 2/2] drm/i915: Rename gt to gt0

[Chris Wilson]

Quoting Andi Shyti (2021-11-17 13:34:56)
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c 
> b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> index 089fb4658b216..0bbf8c0c42eac 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> @@ -817,7 +817,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane,
>  * maximum clocks following a vblank miss (see do_rps_boost()).
>  */
> if (!state->rps_interactive) {
> -   intel_rps_mark_interactive(_priv->gt.rps, true);
> +   intel_rps_mark_interactive(_priv->gt0.rps, true);

This should be across all gt, so probably wants a fresh interface that
takes i915 and does for_each_gt in a later patch. (Since we could be
rendering on a remote tile to present on a display.)

> state->rps_interactive = true;
> }
>  
> @@ -851,7 +851,7 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
> return;
>  
> if (state->rps_interactive) {
> -   intel_rps_mark_interactive(_priv->gt.rps, false);
> +   intel_rps_mark_interactive(_priv->gt0.rps, false);
> state->rps_interactive = false;
> }
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c 
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 0ceee8ac66717..d4fcd8f236476 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -838,7 +838,7 @@ __intel_display_resume(struct drm_device *dev,
>  static bool gpu_reset_clobbers_display(struct drm_i915_private *dev_priv)
>  {
> return (INTEL_INFO(dev_priv)->gpu_reset_clobbers_display &&
> -   intel_has_gpu_reset(_priv->gt));
> +   intel_has_gpu_reset(_priv->gt0));

All these display consumers probably want to use
dev_priv->ggtt->vm.gt, since the scanout capable GGTT would seem to be
the defining feature.

to_scanout_gt(i915) ?

>  static bool pxp_is_borked(struct drm_i915_gem_object *obj)
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c 
> b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index ebd775cb1661c..c62253d0af044 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -237,7 +237,7 @@ static int proto_context_set_persistence(struct 
> drm_i915_private *i915,
>  * colateral damage, and we should not pretend we can by
>  * exposing the interface.
>  */
> -   if (!intel_has_reset_engine(>gt))
> +   if (!intel_has_reset_engine(>gt0))
> return -ENODEV;

Prep for all gt. A lot of these need an all-gt interface so we don't
have for_each_gt spread all other the place.

> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c 
> b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
> index ef22d4ed66ad6..69ad407eb15f3 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
> @@ -166,7 +166,7 @@ static struct dma_fence *i915_ttm_accel_move(struct 
> ttm_buffer_object *bo,
> enum i915_cache_level src_level, dst_level;
> int ret;
>  
> -   if (!i915->gt.migrate.context || intel_gt_is_wedged(>gt))
> +   if (!i915->gt0.migrate.context || intel_gt_is_wedged(>gt0))

This should already be looking at lmem->gt

> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_user.c 
> b/drivers/gpu/drm/i915/gt/intel_engine_user.c
> index 8f8bea08e734d..176ea5c7d422f 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_user.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_user.c
> @@ -116,7 +116,7 @@ static void set_scheduler_caps(struct drm_i915_private 
> *i915)
> disabled |= (I915_SCHEDULER_CAP_ENABLED |
>  I915_SCHEDULER_CAP_PRIORITY);
>  
> -   if (intel_uc_uses_guc_submission(>gt.uc))
> +   if (intel_uc_uses_guc_submission(>gt0.uc))

This shouldn't be looking at gt at all, but if it must, that information
must be coming via engine->gt. Kind of renders the mapping moot
currently.
> diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c 
> b/drivers/gpu/drm/i915/gt/intel_rps.c
> index 07ff7ba7b2b71..63089e671a242 100644
> --- a/drivers/gpu/drm/i915/gt/intel_rps.c
> +++ b/drivers/gpu/drm/i915/gt/intel_rps.c
> @@ -2302,7 +2302,7 @@ unsigned long i915_read_mch_val(void)
> return 0;
>  
> with_intel_runtime_pm(>runtime_pm, wakeref) {
> -   struct intel_ips *ips = >gt.rps.ips;
> +   struct intel_ips *ips = >gt0.rps.ips;

Make mchdev_get() return the gt or rps, at the slight cost of making the
drm_dev_put() more complicated (but can be pushed into a mchdev_put for
symmetry).

> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c 
> b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index a9727447c0379..4bfedc04f5c70 100644

Re: [Intel-gfx] [PATCH] drm/i915: remove IS_ACTIVE

[Chris Wilson]

Quoting Lucas De Marchi (2021-10-01 08:40:41)
> When trying to bring IS_ACTIVE to linux/kconfig.h I thought it wouldn't
> provide much value just encapsulating it in a boolean context. So I also
> added the support for handling undefined macros as the IS_ENABLED()
> counterpart. However the feedback received from Masahiro Yamada was that
> it is too ugly, not providing much value. And just wrapping in a boolean
> context is too dumb - we could simply open code it.
> 
> As detailed in commit babaab2f4738 ("drm/i915: Encapsulate kconfig
> constant values inside boolean predicates"), the IS_ACTIVE macro was
> added to workaround a compilation warning. However after checking again
> our current uses of IS_ACTIVE it turned out there is only
> 1 case in which it would potentially trigger a warning. All the others
>   can simply use the shorter version, without wrapping it in any macro.
> And even that single one didn't trigger any warning in gcc 10.3.
> 
> So here I'm dialing all the way back to simply removing the macro. If it
> triggers warnings in future we may change the few cases to check for > 0
> or != 0. Another possibility would be to use the great "not not
> operator" for all positive checks, which would allow us to maintain
> consistency.  However let's try first the simplest form though, hopefully
> we don't hit broken compilers spitting a warning:

You didn't prevent the compilation warning this re-introduces.

drivers/gpu/drm/i915/i915_config.c:11 i915_fence_context_timeout() warn: should 
this be a bitwise op?
drivers/gpu/drm/i915/i915_request.c:1679 i915_request_wait() warn: should this 
be a bitwise op?
-Chris

Re: [PATCH] i915: Drop relocation support on all new hardware (v3)

[Chris Wilson]

Quoting Zbigniew Kempczyński (2021-03-11 11:44:32)
> On Wed, Mar 10, 2021 at 03:50:07PM -0600, Jason Ekstrand wrote:
> > The Vulkan driver in Mesa for Intel hardware never uses relocations if
> > it's running on a version of i915 that supports at least softpin which
> > all versions of i915 supporting Gen12 do.  On the OpenGL side, Gen12+ is
> > only supported by iris which never uses relocations.  The older i965
> > driver in Mesa does use relocations but it only supports Intel hardware
> > through Gen11 and has been deprecated for all hardware Gen9+.  The
> > compute driver also never uses relocations.  This only leaves the media
> > driver which is supposed to be switching to softpin going forward.
> > Making softpin a requirement for all future hardware seems reasonable.
> > 
> > Rejecting relocations starting with Gen12 has the benefit that we don't
> > have to bother supporting it on platforms with local memory.  Given how
> > much CPU touching of memory is required for relocations, not having to
> > do so on platforms where not all memory is directly CPU-accessible
> > carries significant advantages.
> > 
> > v2 (Jason Ekstrand):
> >  - Allow TGL-LP platforms as they've already shipped
> > 
> > v3 (Jason Ekstrand):
> >  - WARN_ON platforms with LMEM support in case the check is wrong
> 
> I was asked to review of this patch. It works along with expected
> IGT check https://patchwork.freedesktop.org/patch/423361/?series=82954=25
> 
> Before I'll give you r-b - isn't i915_gem_execbuffer2_ioctl() better place
> to do for loop just after copy_from_user() and check relocation_count?
> We have an access to exec2_list there, we know the gen so we're able to say
> relocations are not supported immediate, without entering 
> i915_gem_do_execbuffer().

There's a NORELOC flag you can enforce as mandatory. That's trivial for
userspace to set, really makes sure they are aware of the change afoot,
and i915_gem_ceck_execbuffer() will perform the validation upfront with
the other flag checks.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] Revert "drm/i915: Propagate errors on awaiting already signaled fences"

[Chris Wilson]

Quoting Daniel Vetter (2021-03-11 16:01:46)
> On Fri, Mar 05, 2021 at 11:05:46AM -0600, Jason Ekstrand wrote:
> > This reverts commit 9e31c1fe45d555a948ff66f1f0e3fe1f83ca63f7.  Ever
> > since that commit, we've been having issues where a hang in one client
> > can propagate to another.  In particular, a hang in an app can propagate
> > to the X server which causes the whole desktop to lock up.
> > 
> > Signed-off-by: Jason Ekstrand 
> > Reported-by: Marcin Slusarz 
> > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/3080
> > Fixes: 9e31c1fe45d5 ("drm/i915: Propagate errors on awaiting already 
> > signaled fences")
> 
> Yeah I suggested to just go with the revert, so I guess on my to give you
> the explainer to be added to the commit message.

If you took the patch this was copied from and only revert on the
dma-resv, things do not break horribly.

> Error propagation along fences sound like a good idea, but as your bug
> shows, surprising consequences, since propagating errors across security
> boundaries is not a good thing.
> 
> What we do have is track the hangs on the ctx, and report information to
> userspace using RESET_STATS.

And by the fence, which is far more precise.

> That's how arb_robustness works. Also, if my
> understanding is still correct, the EIO from execbuf is when your context
> is banned (because not recoverable or too many hangs). And in all these
> cases it's up to userspace to figure out what is all impacted and should
> be reported to the application, that's not on the kernel to guess and
> automatically propagate.
> 
> What's more, we're also building more features on top of ctx error
> reporting with RESET_STATS ioctl: Encrypted buffers use the same, and the
> userspace fence wait also relies on that mechanism. So it is the path
> going forward for reporting gpu hangs and resets to userspace.

That ioctl is not a solid basis, it never did quite work as expected and
we kept realising the limitations of the information and the accuracy
that it could report.
 
> So all together that's why I think we should just bury this idea again as
> not quite the direction we want to go to, hence why I think the revert is
> the right option here.

No, as shown by igt it's a critical issue that we have to judicially
chose which errors to ignore. And it's not just the ability to subvert
gen7 and gen9, it's the error tracking employed for preempting contexts
among others.  Hence go with the original patch to undo the propagation
along dma-resv.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] gpu: drm: i915: fix error return code of igt_buddy_alloc_smoke()

[Chris Wilson]

Quoting Jia-Ju Bai (2021-03-08 08:59:52)
> When i915_random_order() returns NULL to order, no error return code of
> igt_buddy_alloc_smoke() is assigned.
> To fix this bug, err is assigned with -EINVAL in this case.

It would not be EINVAL since that is used for a reference failure, but
in this case the idea was to return 0 as no testing was done and the
ENOMEM was raised before testing began i.e. not an internal and
unexpected driver allocation failure.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] gpu: drm: i915: fix error return code of igt_threaded_blt()

[Chris Wilson]

Quoting Jia-Ju Bai (2021-03-08 09:07:22)
> When kcalloc() returns NULL to tsk or thread, no error code of 
> igt_threaded_blt() is returned.
> To fix this bug, -ENOMEM is returned as error code.

Because we decided to skip the test if it could not be run due to
insufficient memory, as opposed to an internal allocation failure from
the driver.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] Revert "drm/i915: Propagate errors on awaiting already signaled fences"

[Chris Wilson]

Quoting Jason Ekstrand (2021-03-05 17:05:46)
> This reverts commit 9e31c1fe45d555a948ff66f1f0e3fe1f83ca63f7.  Ever
> since that commit, we've been having issues where a hang in one client
> can propagate to another.  In particular, a hang in an app can propagate
> to the X server which causes the whole desktop to lock up.

The fence error handling is required to prevent user's circumventing
incomplete work, such as security validation or escaping isolation.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] dma-buf: Fix confusion of dynamic dma-buf vs dynamic attachment

[Chris Wilson]

Commit c545781e1c55 ("dma-buf: doc polish for pin/unpin") disagrees with
the introduction of dynamism in commit: bb42df4662a4 ("dma-buf: add
dynamic DMA-buf handling v15") resulting in warning spew on
importing dma-buf. Silence the warning from the latter by only pinning
the attachment if the attachment rather than the dmabuf is to be
dynamic.

Fixes: bb42df4662a4 ("dma-buf: add dynamic DMA-buf handling v15")
Fixes: c545781e1c55 ("dma-buf: doc polish for pin/unpin")
Signed-off-by: Chris Wilson 
Cc: Daniel Vetter 
Cc: Christian König 
Cc:  # v5.7+
---
 drivers/dma-buf/dma-buf.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index f264b70c383e..09f5ae458515 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -758,8 +758,8 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct 
device *dev,
dma_buf_is_dynamic(dmabuf)) {
struct sg_table *sgt;
 
-   if (dma_buf_is_dynamic(attach->dmabuf)) {
-   dma_resv_lock(attach->dmabuf->resv, NULL);
+   if (dma_buf_attachment_is_dynamic(attach)) {
+   dma_resv_lock(dmabuf->resv, NULL);
ret = dma_buf_pin(attach);
if (ret)
goto err_unlock;
@@ -772,8 +772,9 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct 
device *dev,
ret = PTR_ERR(sgt);
goto err_unpin;
}
-   if (dma_buf_is_dynamic(attach->dmabuf))
-   dma_resv_unlock(attach->dmabuf->resv);
+   if (dma_buf_attachment_is_dynamic(attach))
+   dma_resv_unlock(dmabuf->resv);
+
attach->sgt = sgt;
attach->dir = DMA_BIDIRECTIONAL;
}
-- 
2.20.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm/i915/gem: Add a check for object size for corner cases

[Chris Wilson]

Quoting Ram Moon, AnandX (2021-02-16 12:05:23)
> Hi Chris,
> 
> -Original Message-
> From: dri-devel  On Behalf Of Chris 
> Wilson
> Sent: Monday, February 15, 2021 6:10 PM
> To: Auld, Matthew ; Ram Moon, AnandX 
> ; Surendrakumar Upadhyay, TejaskumarX 
> ; Ursulin, Tvrtko 
> ; Jani Nikula ; 
> dri-devel@lists.freedesktop.org; intel-...@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/gem: Add a check for object size 
> for corner cases
> 
> Quoting Ram Moon, AnandX (2021-02-15 12:29:17)
> > Hi Chris,
> > 
> > -----Original Message-
> > From: dri-devel  On Behalf Of 
> > Chris Wilson
> > Sent: Wednesday, February 10, 2021 4:15 PM
> > To: Ram Moon, AnandX ; Jani Nikula 
> > ; Auld, Matthew ; 
> > Surendrakumar Upadhyay, TejaskumarX 
> > ; Ursulin, Tvrtko 
> > ; dri-devel@lists.freedesktop.org; 
> > intel-...@lists.freedesktop.org
> > Cc: Ram Moon, AnandX 
> > Subject: Re: [Intel-gfx] [PATCH] drm/i915/gem: Add a check for object 
> > size for corner cases
> > 
> > Quoting Anand Moon (2021-02-10 07:59:29)
> > > Add check for object size to return appropriate error -E2BIG or 
> > > -EINVAL to avoid WARM_ON and successful return for some testcase.
> > 
> > No. You miss the point of having those warnings. We need to inspect the 
> > code to remove the last remaining "int pagenum", and then we can remove the 
> > GEM_WARN_ON((sz) >> PAGE_SHIFT > INT_MAX). These are not emitted for users, 
> > only for us to motivate us into finally fixing the code.
> > -Chris
> > 
> > Yes, I got your point these check are meant to catch up integer overflow.
> > 
> > I have check with the IGT testcase case  _gem_create_ and 
> > _gem_userptr_blits_ which fails pass size *-1ull << 32*  left shift 
> > and *0~* which leads to integer overflow ie  _negative_ size passed to 
> > create  i915_gem_create via ioctl  this leads to GM_WARN_ON.
> > 
> > Can we drop these testcase so that we don't break the kernel ?
> 
> The kernel rejects the ioctl with the expected errno. We leave a warning 
> purely for the benefit of CI, only when compiled for debugging and not by 
> default, so that we have a persistent reminder to do the code review.
> What's broken?
> -Chris
> 
> All though the testcase return with appropriate error we observe kernel taint 
> see below.

Which is an intentional taint added for CI so that we get a warning and
a visible bug so that we can allocate resources to _fix_ the underlying
problems in the code.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm/i915/gem: Add a check for object size for corner cases

[Chris Wilson]

Quoting Ram Moon, AnandX (2021-02-15 12:29:17)
> Hi Chris,
> 
> -Original Message-
> From: dri-devel  On Behalf Of Chris 
> Wilson
> Sent: Wednesday, February 10, 2021 4:15 PM
> To: Ram Moon, AnandX ; Jani Nikula 
> ; Auld, Matthew ; 
> Surendrakumar Upadhyay, TejaskumarX 
> ; Ursulin, Tvrtko 
> ; dri-devel@lists.freedesktop.org; 
> intel-...@lists.freedesktop.org
> Cc: Ram Moon, AnandX 
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/gem: Add a check for object size 
> for corner cases
> 
> Quoting Anand Moon (2021-02-10 07:59:29)
> > Add check for object size to return appropriate error -E2BIG or 
> > -EINVAL to avoid WARM_ON and successful return for some testcase.
> 
> No. You miss the point of having those warnings. We need to inspect the code 
> to remove the last remaining "int pagenum", and then we can remove the 
> GEM_WARN_ON((sz) >> PAGE_SHIFT > INT_MAX). These are not emitted for users, 
> only for us to motivate us into finally fixing the code.
> -Chris
> 
> Yes, I got your point these check are meant to catch up integer overflow.
> 
> I have check with the IGT testcase case  _gem_create_ and _gem_userptr_blits_ 
>  
> which fails pass size *-1ull << 32*  left shift and *0~* which leads to 
> integer overflow 
> ie  _negative_ size passed to create  i915_gem_create via ioctl  this leads 
> to GM_WARN_ON. 
> 
> Can we drop these testcase so that we don't break the kernel ?

The kernel rejects the ioctl with the expected errno. We leave a warning
purely for the benefit of CI, only when compiled for debugging and not by
default, so that we have a persistent reminder to do the code review.
What's broken?
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 3/3] drm/i915/gt: Export device and per-process runtimes via procfs

[Chris Wilson]

Quoting Emil Velikov (2021-02-12 15:45:04)
> On Fri, 12 Feb 2021 at 15:16, Chris Wilson  wrote:
> >
> > Quoting Emil Velikov (2021-02-12 14:57:56)
> > > Hi Chris,
> > >
> > > On Thu, 4 Feb 2021 at 12:11, Chris Wilson  
> > > wrote:
> > > >
> > > > Register with /proc/gpu to provide the client runtimes for generic
> > > > top-like overview, e.g. gnome-system-monitor can use this information to
> > > > show the per-process multi-GPU usage.
> > > >
> > > Exposing this information to userspace sounds great IMHO and like the
> > > proposed "channels" for the device engines.
> > > If it were me, I would have the channel names a) exposed to userspace
> > > and b) be a "fixed set".
> >
> > - Total
> > - Graphics
> > - Compute
> > - Unified
> > - Video
> > - Copy
> > - Display
> > - Other
> >
> > Enough versatility for the foreseeable future?
> > But plan for extension.
> >
> With a bit of documentation about "unified" (is it a metric also
> counted towards any of the rest) it would be perfect.

With unified I was trying to find a place to things that are neither
wholly graphics nor compute, as some may prefer not to categorise
themselves as one or the other. Also whether or not some cores are more
compute than others (so should there be an AI/RT/ALU?)

> For future extension one might consider splitting video into
> encoder/decoder/post-processing.

Ok, I wasn't sure how commonly those functions were split on different
HW.

> > The other aspect then is the capacity of each channel. We can keep it
> > simple as the union/average (whichever the driver has to hand) runtime in
> > nanoseconds over all IP blocks within a channel.
> 
> Not sure what you mean with capacity. Are you referring to having
> multiple instances of the same engine (say 3 separate copy engines)?
> Personally I'm inclined to keep these separate entries, since some
> hardware can have multiple ones.
> 
> For example - before the latest changes nouveau had 8 copy engines,
> 3+3 video 'generic' video (enc,dec)oder engines, amongst others.

Yes, most HW have multiple engines within a family. Trying to keep it
simple, I thought presenting just one runtime metric for the whole
channel. Especially for the single-line per device format I had picked :)

If we switch to a more extensible format,

-'$device0' : 
-$channel0 : {
Total : $total # avg/union over all engines
Engines : [ $0, $1, ... ]
}
...

-'$device1' : 
...

Using the same fixed channel names, and dev_name(), pesky concerns such
as keeping it as a simple scanf can be forgotten.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 3/3] drm/i915/gt: Export device and per-process runtimes via procfs

[Chris Wilson]

Quoting Emil Velikov (2021-02-12 14:57:56)
> Hi Chris,
> 
> On Thu, 4 Feb 2021 at 12:11, Chris Wilson  wrote:
> >
> > Register with /proc/gpu to provide the client runtimes for generic
> > top-like overview, e.g. gnome-system-monitor can use this information to
> > show the per-process multi-GPU usage.
> >
> Exposing this information to userspace sounds great IMHO and like the
> proposed "channels" for the device engines.
> If it were me, I would have the channel names a) exposed to userspace
> and b) be a "fixed set".

- Total
- Graphics
- Compute
- Unified
- Video
- Copy
- Display
- Other

Enough versatility for the foreseeable future?
But plan for extension.

The other aspect then is the capacity of each channel. We can keep it
simple as the union/average (whichever the driver has to hand) runtime in
nanoseconds over all IP blocks within a channel.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm/i915/gem: Add a check for object size for corner cases

[Chris Wilson]

Quoting Anand Moon (2021-02-10 07:59:29)
> Add check for object size to return appropriate error -E2BIG or -EINVAL
> to avoid WARM_ON and sucessfull return for some testcase.

No. You miss the point of having those warnings. We need to inspect the
code to remove the last remaining "int pagenum", and then we can remove
the GEM_WARN_ON((sz) >> PAGE_SHIFT > INT_MAX). These are not emitted for
users, only for us to motivate us into finally fixing the code.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3] kcmp: Support selection of SYS_kcmp without CHECKPOINT_RESTORE

[Chris Wilson]

Userspace has discovered the functionality offered by SYS_kcmp and has
started to depend upon it. In particular, Mesa uses SYS_kcmp for
os_same_file_description() in order to identify when two fd (e.g. device
or dmabuf) point to the same struct file. Since they depend on it for
core functionality, lift SYS_kcmp out of the non-default
CONFIG_CHECKPOINT_RESTORE into the selectable syscall category.

Rasmus Villemoes also pointed out that systemd uses SYS_kcmp to
deduplicate the per-service file descriptor store.

Note that some distributions such as Ubuntu are already enabling
CHECKPOINT_RESTORE in their configs and so, by extension, SYS_kcmp.

References: https://gitlab.freedesktop.org/drm/intel/-/issues/3046
Signed-off-by: Chris Wilson 
Cc: Kees Cook 
Cc: Andy Lutomirski 
Cc: Will Drewry 
Cc: Andrew Morton 
Cc: Dave Airlie 
Cc: Daniel Vetter 
Cc: Lucas Stach 
Cc: Rasmus Villemoes 
Cc: Cyrill Gorcunov 
Cc: sta...@vger.kernel.org
Acked-by: Daniel Vetter  # DRM depends on kcmp
Acked-by: Rasmus Villemoes  # systemd uses kcmp

---
v2:
  - Default n.
  - Borrrow help message from man kcmp.
  - Export get_epoll_tfile_raw_ptr() for CONFIG_KCMP
v3:
  - Select KCMP for CONFIG_DRM
---
 drivers/gpu/drm/Kconfig   |  3 +++
 fs/eventpoll.c|  4 ++--
 include/linux/eventpoll.h |  2 +-
 init/Kconfig  | 11 +++
 kernel/Makefile   |  2 +-
 tools/testing/selftests/seccomp/seccomp_bpf.c |  2 +-
 6 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 0973f408d75f..af6c6d214d91 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -15,6 +15,9 @@ menuconfig DRM
select I2C_ALGOBIT
select DMA_SHARED_BUFFER
select SYNC_FILE
+# gallium uses SYS_kcmp for os_same_file_description() to de-duplicate
+# device and dmabuf fd. Let's make sure that is available for our userspace.
+   select KCMP
help
  Kernel-level support for the Direct Rendering Infrastructure (DRI)
  introduced in XFree86 4.0. If you say Y here, you need to select
diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index a829af074eb5..3196474cbe24 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -979,7 +979,7 @@ static struct epitem *ep_find(struct eventpoll *ep, struct 
file *file, int fd)
return epir;
 }
 
-#ifdef CONFIG_CHECKPOINT_RESTORE
+#ifdef CONFIG_KCMP
 static struct epitem *ep_find_tfd(struct eventpoll *ep, int tfd, unsigned long 
toff)
 {
struct rb_node *rbp;
@@ -1021,7 +1021,7 @@ struct file *get_epoll_tfile_raw_ptr(struct file *file, 
int tfd,
 
return file_raw;
 }
-#endif /* CONFIG_CHECKPOINT_RESTORE */
+#endif /* CONFIG_KCMP */
 
 /**
  * Adds a new entry to the tail of the list in a lockless way, i.e.
diff --git a/include/linux/eventpoll.h b/include/linux/eventpoll.h
index 0350393465d4..593322c946e6 100644
--- a/include/linux/eventpoll.h
+++ b/include/linux/eventpoll.h
@@ -18,7 +18,7 @@ struct file;
 
 #ifdef CONFIG_EPOLL
 
-#ifdef CONFIG_CHECKPOINT_RESTORE
+#ifdef CONFIG_KCMP
 struct file *get_epoll_tfile_raw_ptr(struct file *file, int tfd, unsigned long 
toff);
 #endif
 
diff --git a/init/Kconfig b/init/Kconfig
index b77c60f8b963..9cc7436b2f73 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1194,6 +1194,7 @@ endif # NAMESPACES
 config CHECKPOINT_RESTORE
bool "Checkpoint/restore support"
select PROC_CHILDREN
+   select KCMP
default n
help
  Enables additional kernel features in a sake of checkpoint/restore.
@@ -1737,6 +1738,16 @@ config ARCH_HAS_MEMBARRIER_CALLBACKS
 config ARCH_HAS_MEMBARRIER_SYNC_CORE
bool
 
+config KCMP
+   bool "Enable kcmp() system call" if EXPERT
+   help
+ Enable the kernel resource comparison system call. It provides
+ user-space with the ability to compare two processes to see if they
+ share a common resource, such as a file descriptor or even virtual
+ memory space.
+
+ If unsure, say N.
+
 config RSEQ
bool "Enable rseq() system call" if EXPERT
default y
diff --git a/kernel/Makefile b/kernel/Makefile
index aa7368c7eabf..320f1f3941b7 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -51,7 +51,7 @@ obj-y += livepatch/
 obj-y += dma/
 obj-y += entry/
 
-obj-$(CONFIG_CHECKPOINT_RESTORE) += kcmp.o
+obj-$(CONFIG_KCMP) += kcmp.o
 obj-$(CONFIG_FREEZER) += freezer.o
 obj-$(CONFIG_PROFILING) += profile.o
 obj-$(CONFIG_STACKTRACE) += stacktrace.o
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c 
b/tools/testing/selftests/seccomp/seccomp_bpf.c
index 26c72f2b61b1..1b6c7d33c4ff 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -315,7 +315,7 @@ TEST(kcmp)
ret = __filecmp(getpid(), getpid(), 1, 1);
EXPECT_EQ(ret, 0);

Re: [PATCH v2] kernel: Expose SYS_kcmp by default

[Chris Wilson]

Quoting Kees Cook (2021-02-05 21:20:33)
> On Fri, Feb 05, 2021 at 09:16:01PM +0000, Chris Wilson wrote:
> > The subject should of course be changed, as it is no longer being
> > enabled by default.
> 
> "default n" is redundant.

I thought being explicit would be preferred. There are a few other
default n, so at least it's not the odd-one-out!

> I thought Daniel said CONFIG_DRM needed to
> "select" it too, though?

Yes. We will need to select it for any DRM driver so that the Vulkan/GL
stacks can rely on having SYS_kcmp. That deserves to be handled and
explain within drm/Kconfig, and as they are already shipping with calls
to SYS_kcmp we may have to ask for a stable backport.

> Otherwise, yeah, this looks good. Was the
> export due to the 0-day bot failure reports?

Yes.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] kernel: Expose SYS_kcmp by default

[Chris Wilson]

The subject should of course be changed, as it is no longer being
enabled by default.

Something like

kcmp: Support selection of SYS_kcmp without CHECKPOINT_RESTORE

Quoting Chris Wilson (2021-02-05 21:06:10)
> Userspace has discovered the functionality offered by SYS_kcmp and has
> started to depend upon it. In particular, Mesa uses SYS_kcmp for
> os_same_file_description() in order to identify when two fd (e.g. device
> or dmabuf) point to the same struct file. Since they depend on it for
> core functionality, lift SYS_kcmp out of the non-default
> CONFIG_CHECKPOINT_RESTORE into the selectable syscall category.
> 
> Note that some distributions such as Ubuntu are already enabling
> CHECKPOINT_RESTORE in their configs and so, by extension, SYS_kcmp.
> 
> References: https://gitlab.freedesktop.org/drm/intel/-/issues/3046
> Signed-off-by: Chris Wilson 
> Cc: Kees Cook 
> Cc: Andy Lutomirski 
> Cc: Will Drewry 
> Cc: Andrew Morton 
> Cc: Dave Airlie 
> Cc: Daniel Vetter 
> Cc: Lucas Stach 
> Acked-by: Daniel Vetter  # DRM depends on SYS_kcmp
> 
> ---
> v2:
>   - Default n.
>   - Borrrow help message from man kcmp.
>   - Export get_epoll_tfile_raw_ptr() for CONFIG_KCMP
> ---
>  fs/eventpoll.c|  4 ++--
>  include/linux/eventpoll.h |  2 +-
>  init/Kconfig  | 12 
>  kernel/Makefile   |  2 +-
>  tools/testing/selftests/seccomp/seccomp_bpf.c |  2 +-
>  5 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/eventpoll.c b/fs/eventpoll.c
> index a829af074eb5..3196474cbe24 100644
> --- a/fs/eventpoll.c
> +++ b/fs/eventpoll.c
> @@ -979,7 +979,7 @@ static struct epitem *ep_find(struct eventpoll *ep, 
> struct file *file, int fd)
> return epir;
>  }
>  
> -#ifdef CONFIG_CHECKPOINT_RESTORE
> +#ifdef CONFIG_KCMP
>  static struct epitem *ep_find_tfd(struct eventpoll *ep, int tfd, unsigned 
> long toff)
>  {
> struct rb_node *rbp;
> @@ -1021,7 +1021,7 @@ struct file *get_epoll_tfile_raw_ptr(struct file *file, 
> int tfd,
>  
> return file_raw;
>  }
> -#endif /* CONFIG_CHECKPOINT_RESTORE */
> +#endif /* CONFIG_KCMP */
>  
>  /**
>   * Adds a new entry to the tail of the list in a lockless way, i.e.
> diff --git a/include/linux/eventpoll.h b/include/linux/eventpoll.h
> index 0350393465d4..593322c946e6 100644
> --- a/include/linux/eventpoll.h
> +++ b/include/linux/eventpoll.h
> @@ -18,7 +18,7 @@ struct file;
>  
>  #ifdef CONFIG_EPOLL
>  
> -#ifdef CONFIG_CHECKPOINT_RESTORE
> +#ifdef CONFIG_KCMP
>  struct file *get_epoll_tfile_raw_ptr(struct file *file, int tfd, unsigned 
> long toff);
>  #endif
>  
> diff --git a/init/Kconfig b/init/Kconfig
> index b77c60f8b963..1b75141bc18b 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1194,6 +1194,7 @@ endif # NAMESPACES
>  config CHECKPOINT_RESTORE
> bool "Checkpoint/restore support"
> select PROC_CHILDREN
> +   select KCMP
> default n
> help
>   Enables additional kernel features in a sake of checkpoint/restore.
> @@ -1737,6 +1738,17 @@ config ARCH_HAS_MEMBARRIER_CALLBACKS
>  config ARCH_HAS_MEMBARRIER_SYNC_CORE
> bool
>  
> +config KCMP
> +   bool "Enable kcmp() system call" if EXPERT
> +   default n
> +   help
> + Enable the kernel resource comparison system call. It provides
> + user-space with the ability to compare two processes to see if they
> + share a common resource, such as a file descriptor or even virtual
> + memory space.
> +
> + If unsure, say N.
> +
>  config RSEQ
> bool "Enable rseq() system call" if EXPERT
> default y
> diff --git a/kernel/Makefile b/kernel/Makefile
> index aa7368c7eabf..320f1f3941b7 100644
> --- a/kernel/Makefile
> +++ b/kernel/Makefile
> @@ -51,7 +51,7 @@ obj-y += livepatch/
>  obj-y += dma/
>  obj-y += entry/
>  
> -obj-$(CONFIG_CHECKPOINT_RESTORE) += kcmp.o
> +obj-$(CONFIG_KCMP) += kcmp.o
>  obj-$(CONFIG_FREEZER) += freezer.o
>  obj-$(CONFIG_PROFILING) += profile.o
>  obj-$(CONFIG_STACKTRACE) += stacktrace.o
> diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c 
> b/tools/testing/selftests/seccomp/seccomp_bpf.c
> index 26c72f2b61b1..1b6c7d33c4ff 100644
> --- a/tools/testing/selftests/seccomp/seccomp_bpf.c
> +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
> @@ -315,7 +315,7 @@ TEST(kcmp)
> ret = __filecmp(getpid(), getpid(), 1, 1);
> EXPECT_EQ(ret, 0);
> if (ret != 0 && errno == ENOSYS)
> -   SKIP(return, "Kernel does not support kcmp() (missing 
> CONFIG_CHECKPOINT_RESTORE?)");
> +   SKIP(return, "Kernel does not support kcmp() (missing 
> CONFIG_KCMP?)");
>  }
>  
>  TEST(mode_strict_support)
> -- 
> 2.20.1
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2] kernel: Expose SYS_kcmp by default

[Chris Wilson]

Userspace has discovered the functionality offered by SYS_kcmp and has
started to depend upon it. In particular, Mesa uses SYS_kcmp for
os_same_file_description() in order to identify when two fd (e.g. device
or dmabuf) point to the same struct file. Since they depend on it for
core functionality, lift SYS_kcmp out of the non-default
CONFIG_CHECKPOINT_RESTORE into the selectable syscall category.

Note that some distributions such as Ubuntu are already enabling
CHECKPOINT_RESTORE in their configs and so, by extension, SYS_kcmp.

References: https://gitlab.freedesktop.org/drm/intel/-/issues/3046
Signed-off-by: Chris Wilson 
Cc: Kees Cook 
Cc: Andy Lutomirski 
Cc: Will Drewry 
Cc: Andrew Morton 
Cc: Dave Airlie 
Cc: Daniel Vetter 
Cc: Lucas Stach 
Acked-by: Daniel Vetter  # DRM depends on SYS_kcmp

---
v2:
  - Default n.
  - Borrrow help message from man kcmp.
  - Export get_epoll_tfile_raw_ptr() for CONFIG_KCMP
---
 fs/eventpoll.c|  4 ++--
 include/linux/eventpoll.h |  2 +-
 init/Kconfig  | 12 
 kernel/Makefile   |  2 +-
 tools/testing/selftests/seccomp/seccomp_bpf.c |  2 +-
 5 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index a829af074eb5..3196474cbe24 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -979,7 +979,7 @@ static struct epitem *ep_find(struct eventpoll *ep, struct 
file *file, int fd)
return epir;
 }
 
-#ifdef CONFIG_CHECKPOINT_RESTORE
+#ifdef CONFIG_KCMP
 static struct epitem *ep_find_tfd(struct eventpoll *ep, int tfd, unsigned long 
toff)
 {
struct rb_node *rbp;
@@ -1021,7 +1021,7 @@ struct file *get_epoll_tfile_raw_ptr(struct file *file, 
int tfd,
 
return file_raw;
 }
-#endif /* CONFIG_CHECKPOINT_RESTORE */
+#endif /* CONFIG_KCMP */
 
 /**
  * Adds a new entry to the tail of the list in a lockless way, i.e.
diff --git a/include/linux/eventpoll.h b/include/linux/eventpoll.h
index 0350393465d4..593322c946e6 100644
--- a/include/linux/eventpoll.h
+++ b/include/linux/eventpoll.h
@@ -18,7 +18,7 @@ struct file;
 
 #ifdef CONFIG_EPOLL
 
-#ifdef CONFIG_CHECKPOINT_RESTORE
+#ifdef CONFIG_KCMP
 struct file *get_epoll_tfile_raw_ptr(struct file *file, int tfd, unsigned long 
toff);
 #endif
 
diff --git a/init/Kconfig b/init/Kconfig
index b77c60f8b963..1b75141bc18b 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1194,6 +1194,7 @@ endif # NAMESPACES
 config CHECKPOINT_RESTORE
bool "Checkpoint/restore support"
select PROC_CHILDREN
+   select KCMP
default n
help
  Enables additional kernel features in a sake of checkpoint/restore.
@@ -1737,6 +1738,17 @@ config ARCH_HAS_MEMBARRIER_CALLBACKS
 config ARCH_HAS_MEMBARRIER_SYNC_CORE
bool
 
+config KCMP
+   bool "Enable kcmp() system call" if EXPERT
+   default n
+   help
+ Enable the kernel resource comparison system call. It provides
+ user-space with the ability to compare two processes to see if they
+ share a common resource, such as a file descriptor or even virtual
+ memory space.
+
+ If unsure, say N.
+
 config RSEQ
bool "Enable rseq() system call" if EXPERT
default y
diff --git a/kernel/Makefile b/kernel/Makefile
index aa7368c7eabf..320f1f3941b7 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -51,7 +51,7 @@ obj-y += livepatch/
 obj-y += dma/
 obj-y += entry/
 
-obj-$(CONFIG_CHECKPOINT_RESTORE) += kcmp.o
+obj-$(CONFIG_KCMP) += kcmp.o
 obj-$(CONFIG_FREEZER) += freezer.o
 obj-$(CONFIG_PROFILING) += profile.o
 obj-$(CONFIG_STACKTRACE) += stacktrace.o
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c 
b/tools/testing/selftests/seccomp/seccomp_bpf.c
index 26c72f2b61b1..1b6c7d33c4ff 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -315,7 +315,7 @@ TEST(kcmp)
ret = __filecmp(getpid(), getpid(), 1, 1);
EXPECT_EQ(ret, 0);
if (ret != 0 && errno == ENOSYS)
-   SKIP(return, "Kernel does not support kcmp() (missing 
CONFIG_CHECKPOINT_RESTORE?)");
+   SKIP(return, "Kernel does not support kcmp() (missing 
CONFIG_KCMP?)");
 }
 
 TEST(mode_strict_support)
-- 
2.20.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] kernel: Expose SYS_kcmp by default

[Chris Wilson]

Userspace has discovered the functionality offered by SYS_kcmp and has
started to depend upon it. In particular, Mesa uses SYS_kcmp for
os_same_file_description() in order to identify when two fd (e.g. device
or dmabuf) point to the same struct file. Since they depend on it for
core functionality, lift SYS_kcmp out of the non-default
CONFIG_CHECKPOINT_RESTORE into the selectable syscall category.

Signed-off-by: Chris Wilson 
Cc: Kees Cook 
Cc: Andy Lutomirski 
Cc: Will Drewry 
Cc: Andrew Morton 
Cc: Dave Airlie 
Cc: Daniel Vetter 
Cc: Lucas Stach 
---
 init/Kconfig  | 11 +++
 kernel/Makefile   |  2 +-
 tools/testing/selftests/seccomp/seccomp_bpf.c |  2 +-
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/init/Kconfig b/init/Kconfig
index b77c60f8b963..f62fca13ac5b 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1194,6 +1194,7 @@ endif # NAMESPACES
 config CHECKPOINT_RESTORE
bool "Checkpoint/restore support"
select PROC_CHILDREN
+   select KCMP
default n
help
  Enables additional kernel features in a sake of checkpoint/restore.
@@ -1737,6 +1738,16 @@ config ARCH_HAS_MEMBARRIER_CALLBACKS
 config ARCH_HAS_MEMBARRIER_SYNC_CORE
bool
 
+config KCMP
+   bool "Enable kcmp() system call" if EXPERT
+   default y
+   help
+ Enable the file descriptor comparison system call. It provides
+ user-space with the ability to compare two fd to see if they
+ point to the same file, and check other attributes.
+
+ If unsure, say Y.
+
 config RSEQ
bool "Enable rseq() system call" if EXPERT
default y
diff --git a/kernel/Makefile b/kernel/Makefile
index aa7368c7eabf..320f1f3941b7 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -51,7 +51,7 @@ obj-y += livepatch/
 obj-y += dma/
 obj-y += entry/
 
-obj-$(CONFIG_CHECKPOINT_RESTORE) += kcmp.o
+obj-$(CONFIG_KCMP) += kcmp.o
 obj-$(CONFIG_FREEZER) += freezer.o
 obj-$(CONFIG_PROFILING) += profile.o
 obj-$(CONFIG_STACKTRACE) += stacktrace.o
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c 
b/tools/testing/selftests/seccomp/seccomp_bpf.c
index 26c72f2b61b1..1b6c7d33c4ff 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -315,7 +315,7 @@ TEST(kcmp)
ret = __filecmp(getpid(), getpid(), 1, 1);
EXPECT_EQ(ret, 0);
if (ret != 0 && errno == ENOSYS)
-   SKIP(return, "Kernel does not support kcmp() (missing 
CONFIG_CHECKPOINT_RESTORE?)");
+   SKIP(return, "Kernel does not support kcmp() (missing 
CONFIG_KCMP?)");
 }
 
 TEST(mode_strict_support)
-- 
2.20.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/i915: Autoselect CONFIG_CHECKPOINT_RESTORE for SYS_kcmp

[Chris Wilson]

gallium (iris) depends on os_same_file_description() to disambiguate
screens and so avoid importing the same screen fd twice as two distinct
entities (that share all the kernel resources, so actions on screen
affect the other and would cause random faiure). As they depend on it,
so must we. os_same_file_description() uses SYS_kcmp to check the file
tables for the equivalent struct file, but SYS_kcmp is hidden behind
CONFIG_CHECKPOINT_RESTORE. As this is not default, we must select it for
ourselves to ensure that our userspace is fully supported.

Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/3046
Signed-off-by: Chris Wilson 
Cc: Jani Nikula 
Cc: Daniel Vetter 
---
 drivers/gpu/drm/i915/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
index 1e1cb245fca7..470a5214bd33 100644
--- a/drivers/gpu/drm/i915/Kconfig
+++ b/drivers/gpu/drm/i915/Kconfig
@@ -21,6 +21,7 @@ config DRM_I915
select ACPI_VIDEO if ACPI
select ACPI_BUTTON if ACPI
select SYNC_FILE
+   select CHECKPOINT_RESTORE # gallium depends on SYS_kcmp
select IOSF_MBI
select CRC32
select SND_HDA_I915 if SND_HDA_CORE
-- 
2.20.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC 3/3] drm/i915/gt: Export device and per-process runtimes via procfs

[Chris Wilson]

Register with /proc/gpu to provide the client runtimes for generic
top-like overview, e.g. gnome-system-monitor can use this information to
show the per-process multi-GPU usage.

Signed-off-by: Chris Wilson 
---
 drivers/gpu/drm/i915/Makefile|  1 +
 drivers/gpu/drm/i915/gt/intel_gt.c   |  5 ++
 drivers/gpu/drm/i915/gt/intel_gt_proc.c  | 66 
 drivers/gpu/drm/i915/gt/intel_gt_proc.h  | 14 +
 drivers/gpu/drm/i915/gt/intel_gt_types.h |  3 ++
 5 files changed, 89 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/gt/intel_gt_proc.c
 create mode 100644 drivers/gpu/drm/i915/gt/intel_gt_proc.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index ce01634d4ea7..16171f65f5d1 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -104,6 +104,7 @@ gt-y += \
gt/intel_gt_irq.o \
gt/intel_gt_pm.o \
gt/intel_gt_pm_irq.o \
+   gt/intel_gt_proc.o \
gt/intel_gt_requests.o \
gt/intel_gtt.o \
gt/intel_llc.o \
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c 
b/drivers/gpu/drm/i915/gt/intel_gt.c
index ca76f93bc03d..72199c13330d 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -12,6 +12,7 @@
 #include "intel_gt_buffer_pool.h"
 #include "intel_gt_clock_utils.h"
 #include "intel_gt_pm.h"
+#include "intel_gt_proc.h"
 #include "intel_gt_requests.h"
 #include "intel_mocs.h"
 #include "intel_rc6.h"
@@ -373,6 +374,8 @@ void intel_gt_driver_register(struct intel_gt *gt)
intel_rps_driver_register(>rps);
 
debugfs_gt_register(gt);
+
+   intel_gt_driver_register__proc(gt);
 }
 
 static int intel_gt_init_scratch(struct intel_gt *gt, unsigned int size)
@@ -656,6 +659,8 @@ void intel_gt_driver_unregister(struct intel_gt *gt)
 {
intel_wakeref_t wakeref;
 
+   intel_gt_driver_unregister__proc(gt);
+
intel_rps_driver_unregister(>rps);
 
/*
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_proc.c 
b/drivers/gpu/drm/i915/gt/intel_gt_proc.c
new file mode 100644
index ..42db22326c7c
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/intel_gt_proc.c
@@ -0,0 +1,66 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2020 Intel Corporation
+ */
+
+#include 
+
+#include "i915_drm_client.h"
+#include "i915_drv.h"
+#include "intel_gt.h"
+#include "intel_gt_pm.h"
+#include "intel_gt_proc.h"
+
+static void proc_runtime_pid(struct intel_gt *gt,
+struct pid *pid,
+struct proc_gpu_runtime *rt)
+{
+   struct i915_drm_clients *clients = >i915->clients;
+
+   BUILD_BUG_ON(MAX_ENGINE_CLASS >= ARRAY_SIZE(rt->channel));
+
+   rt->device = i915_drm_clients_get_runtime(clients, pid, rt->channel);
+   rt->nchannel = MAX_ENGINE_CLASS + 1;
+}
+
+static void proc_runtime_device(struct intel_gt *gt,
+   struct pid *pid,
+   struct proc_gpu_runtime *rt)
+{
+   struct intel_engine_cs *engine;
+   enum intel_engine_id id;
+   ktime_t dummy;
+
+   rt->nchannel = 0;
+   for_each_engine(engine, gt, id) {
+   rt->channel[rt->nchannel++] =
+   intel_engine_get_busy_time(engine, );
+   if (rt->nchannel == ARRAY_SIZE(rt->channel))
+   break;
+   }
+   rt->device = intel_gt_get_awake_time(gt);
+}
+
+static void proc_runtime(struct proc_gpu *pg,
+struct pid *pid,
+struct proc_gpu_runtime *rt)
+{
+   struct intel_gt *gt = container_of(pg, typeof(*gt), proc);
+
+   strscpy(rt->name, dev_name(gt->i915->drm.dev), sizeof(rt->name));
+   if (pid)
+   proc_runtime_pid(gt, pid, rt);
+   else
+   proc_runtime_device(gt, pid, rt);
+}
+
+void intel_gt_driver_register__proc(struct intel_gt *gt)
+{
+   gt->proc.fn = proc_runtime;
+   proc_gpu_register(>proc);
+}
+
+void intel_gt_driver_unregister__proc(struct intel_gt *gt)
+{
+   proc_gpu_unregister(>proc);
+}
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_proc.h 
b/drivers/gpu/drm/i915/gt/intel_gt_proc.h
new file mode 100644
index ..7a9bff0fb020
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/intel_gt_proc.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2020 Intel Corporation
+ */
+
+#ifndef INTEL_GT_PROC_H
+#define INTEL_GT_PROC_H
+
+struct intel_gt;
+
+void intel_gt_driver_register__proc(struct intel_gt *gt);
+void intel_gt_driver_unregister__proc(struct intel_gt *gt);
+
+#endif /* INTEL_GT_PROC_H */
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h 
b/drivers/gpu/drm/i915/gt/intel_gt_types.h
index 626af37c7790..3fc6d9741764 100644
--- a/drivers/gpu/drm/i915/gt

[RFC 2/3] drm/i915: Look up clients by pid

[Chris Wilson]

Use the pid to find associated clients, and report their runtime. This
will be used to provide the information via procfs.

Signed-off-by: Chris Wilson 
---
 drivers/gpu/drm/i915/i915_drm_client.c | 70 +++---
 drivers/gpu/drm/i915/i915_drm_client.h | 12 +++--
 2 files changed, 73 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drm_client.c 
b/drivers/gpu/drm/i915/i915_drm_client.c
index 1f8b08a413d4..52d9ae97ba25 100644
--- a/drivers/gpu/drm/i915/i915_drm_client.c
+++ b/drivers/gpu/drm/i915/i915_drm_client.c
@@ -26,6 +26,9 @@ void i915_drm_clients_init(struct i915_drm_clients *clients,
 
clients->next_id = 0;
xa_init_flags(>xarray, XA_FLAGS_ALLOC);
+
+   hash_init(clients->pids);
+   spin_lock_init(>pid_lock);
 }
 
 static ssize_t
@@ -95,6 +98,50 @@ show_busy(struct device *kdev, struct device_attribute 
*attr, char *buf)
return sysfs_emit(buf, "%llu\n", total);
 }
 
+u64 i915_drm_clients_get_runtime(struct i915_drm_clients *clients,
+struct pid *pid,
+u64 *rt)
+{
+   struct i915_drm_client_name *name;
+   u64 total = 0;
+   u64 t;
+
+   memset64(rt, 0, MAX_ENGINE_CLASS + 1);
+
+   rcu_read_lock();
+   hash_for_each_possible_rcu(clients->pids, name, node, pid_nr(pid)) {
+   struct i915_drm_client *client = name->client;
+   struct list_head *list = >ctx_list;
+   struct i915_gem_context *ctx;
+   int i;
+
+   if (name->pid != pid)
+   continue;
+
+   for (i = 0; i < ARRAY_SIZE(client->past_runtime); i++) {
+   t = atomic64_read(>past_runtime[i]);
+   rt[i] += t;
+   total += t;
+   }
+
+   list_for_each_entry_rcu(ctx, list, client_link) {
+   struct i915_gem_engines_iter it;
+   struct intel_context *ce;
+
+   for_each_gem_engine(ce,
+   rcu_dereference(ctx->engines),
+   it) {
+   t = intel_context_get_total_runtime_ns(ce);
+   rt[ce->engine->class] += t;
+   total += t;
+   }
+   }
+   }
+   rcu_read_unlock();
+
+   return total;
+}
+
 static const char * const uabi_class_names[] = {
[I915_ENGINE_CLASS_RENDER] = "0",
[I915_ENGINE_CLASS_COPY] = "1",
@@ -242,7 +289,10 @@ __i915_drm_client_register(struct i915_drm_client *client,
if (!name)
return -ENOMEM;
 
+   spin_lock(>pid_lock);
+   hash_add_rcu(clients->pids, >node, pid_nr(name->pid));
RCU_INIT_POINTER(client->name, name);
+   spin_unlock(>pid_lock);
 
if (!clients->root)
return 0; /* intel_fbdev_init registers a client before sysfs */
@@ -254,20 +304,25 @@ __i915_drm_client_register(struct i915_drm_client *client,
return 0;
 
 err_sysfs:
+   spin_lock(>pid_lock);
RCU_INIT_POINTER(client->name, NULL);
+   hash_del_rcu(>node);
+   spin_unlock(>pid_lock);
call_rcu(>rcu, free_name);
return ret;
 }
 
 static void __i915_drm_client_unregister(struct i915_drm_client *client)
 {
+   struct i915_drm_clients *clients = client->clients;
struct i915_drm_client_name *name;
 
__client_unregister_sysfs(client);
 
-   mutex_lock(>update_lock);
+   spin_lock(>pid_lock);
name = rcu_replace_pointer(client->name, NULL, true);
-   mutex_unlock(>update_lock);
+   hash_del_rcu(>node);
+   spin_unlock(>pid_lock);
 
call_rcu(>rcu, free_name);
 }
@@ -294,7 +349,6 @@ i915_drm_client_add(struct i915_drm_clients *clients, 
struct task_struct *task)
return ERR_PTR(-ENOMEM);
 
kref_init(>kref);
-   mutex_init(>update_lock);
spin_lock_init(>ctx_lock);
INIT_LIST_HEAD(>ctx_list);
 
@@ -339,16 +393,20 @@ int
 i915_drm_client_update(struct i915_drm_client *client,
   struct task_struct *task)
 {
+   struct i915_drm_clients *clients = client->clients;
struct i915_drm_client_name *name;
 
name = get_name(client, task);
if (!name)
return -ENOMEM;
 
-   mutex_lock(>update_lock);
-   if (name->pid != rcu_dereference_protected(client->name, true)->pid)
+   spin_lock(>pid_lock);
+   if (name->pid != rcu_dereference_protected(client->name, true)->pid) {
+   hash_add_rcu(clients->pids, >node, pid_nr(name->pid));
name = rcu_replace_pointer(client->name, name, true);
-   mutex_unlock

[RFC 1/3] proc: Show GPU runtimes

[Chris Wilson]

Present an interface for system monitors to watch the GPU usage as a
whole and by individual applications. By consolidating the information
into a canonical location, we have a single interface that can track the
utilisation of all GPU devices and sub-devices. This is preferrable to
asking the system monitors to walk the sysfs, or other interfaces, of
each device and parse the custom information presented by each driver.

Opens:
- Should we try to name each channel so that it can be shown in UI?

In gnome-system-monitor, we would have a task list:
Process ... GPU0% GPU1%
and charts that would show the GPU% on/next the CPU overview.

Then we could have a futher expansion of a GPU% into per-channel
utilisation. That would be useful to check to see what is saturating a
particular channel, e.g. find the video decoder bottleneck.

Signed-off-by: Chris Wilson 
---
 fs/proc/Makefile |  1 +
 fs/proc/base.c   |  2 +
 fs/proc/gpu.c| 83 
 fs/proc/internal.h   |  6 +++
 include/linux/proc_gpu.h | 33 
 5 files changed, 125 insertions(+)
 create mode 100644 fs/proc/gpu.c
 create mode 100644 include/linux/proc_gpu.h

diff --git a/fs/proc/Makefile b/fs/proc/Makefile
index bd08616ed8ba..bdc42b592e3e 100644
--- a/fs/proc/Makefile
+++ b/fs/proc/Makefile
@@ -16,6 +16,7 @@ proc-y+= cmdline.o
 proc-y += consoles.o
 proc-y += cpuinfo.o
 proc-y += devices.o
+proc-y += gpu.o
 proc-y += interrupts.o
 proc-y += loadavg.o
 proc-y += meminfo.o
diff --git a/fs/proc/base.c b/fs/proc/base.c
index b3422cda2a91..062298f5f6c8 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -3266,6 +3266,7 @@ static const struct pid_entry tgid_base_stuff[] = {
 #ifdef CONFIG_SECCOMP_CACHE_DEBUG
ONE("seccomp_cache", S_IRUSR, proc_pid_seccomp_cache),
 #endif
+   ONE("gpu", S_IRUGO, proc_pid_gpu),
 };
 
 static int proc_tgid_base_readdir(struct file *file, struct dir_context *ctx)
@@ -3598,6 +3599,7 @@ static const struct pid_entry tid_base_stuff[] = {
 #ifdef CONFIG_SECCOMP_CACHE_DEBUG
ONE("seccomp_cache", S_IRUSR, proc_pid_seccomp_cache),
 #endif
+   ONE("gpu", S_IRUGO, proc_pid_gpu),
 };
 
 static int proc_tid_base_readdir(struct file *file, struct dir_context *ctx)
diff --git a/fs/proc/gpu.c b/fs/proc/gpu.c
new file mode 100644
index ..7264bf1f2f7b
--- /dev/null
+++ b/fs/proc/gpu.c
@@ -0,0 +1,83 @@
+// SPDX-License-Identifier: GPL-2.0
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "internal.h"
+
+static LIST_HEAD(gpu);
+static DEFINE_SPINLOCK(lock);
+
+void proc_gpu_register(struct proc_gpu *pg)
+{
+   spin_lock();
+   list_add_tail(>link, );
+   spin_unlock();
+}
+EXPORT_SYMBOL_GPL(proc_gpu_register);
+
+void proc_gpu_unregister(struct proc_gpu *pg)
+{
+   spin_lock();
+   list_del(>link);
+   spin_unlock();
+}
+EXPORT_SYMBOL_GPL(proc_gpu_unregister);
+
+static void print_runtime(struct seq_file *m, const struct proc_gpu_runtime 
*rt)
+{
+   int i;
+
+   seq_printf(m, "%llu", rt->device);
+
+   for (i = 0; i < rt->nchannel; i++)
+   seq_printf(m, " %llu", rt->channel[i]);
+
+   seq_printf(m, " %s\n", rt->name);
+}
+
+int proc_pid_gpu(struct seq_file *m, struct pid_namespace *ns,
+struct pid *pid, struct task_struct *task)
+{
+   struct proc_gpu *p, *pn, mark = {};
+   struct proc_gpu_runtime rt;
+
+   spin_lock();
+   list_for_each_entry_safe(p, pn, , link) {
+   if (!p->fn)
+   continue;
+
+   rt.name[0] = '\0';
+   p->fn(p, pid, );
+   if (!rt.name[0])
+   continue;
+
+   list_add(, >link);
+   spin_unlock();
+
+   print_runtime(m, );
+
+   spin_lock();
+   list_safe_reset_next(, pn, link);
+   list_del();
+   }
+   spin_unlock();
+
+   return 0;
+}
+
+static int proc_gpu_show(struct seq_file *m, void *v)
+{
+   return proc_pid_gpu(m, NULL, NULL, NULL);
+}
+
+static int __init proc_gpu_init(void)
+{
+   proc_create_single("gpu", 0, NULL, proc_gpu_show);
+   return 0;
+}
+fs_initcall(proc_gpu_init);
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index f60b379dcdc7..08bf45bec975 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -221,6 +221,12 @@ void set_proc_pid_nlink(void);
 extern struct inode *proc_get_inode(struct super_block *, struct 
proc_dir_entry *);
 extern void proc_entry_rundown(struct proc_dir_entry *);
 
+/*
+ * proc_gpu.c
+ */
+int proc_pid_gpu(struct seq_file *m, struct pid_namespace *ns,
+struct pid *pid, struct task_struct *task);
+
 /*
  * proc_namespaces.c
  */
diff --git a/include/linux/proc_gpu.h b/include/linux/proc_gpu.h
new file mod

Re: [Intel-gfx] v5.11-rc5 BUG kmalloc-1k (Not tainted): Redzone overwritten

[Chris Wilson]

Quoting Jani Nikula (2021-01-28 13:23:48)
> 
> A number of our CI systems are hitting redzone overwritten errors after
> s2idle, with the errors introduced between v5.11-rc4 and v5.11-rc5. See
> snippet below, full logs for one affected machine at [1].
> 
> Known issue?

Fwiw, I think this should be fixed by

commit 08d60e5999540110576e7c1346d486220751b7f9
Author: John Ogness 
Date:   Sun Jan 24 21:33:28 2021 +0106

printk: fix string termination for record_print_text()

Commit f0e386ee0c0b ("printk: fix buffer overflow potential for
print_text()") added string termination in record_print_text().
However it used the wrong base pointer for adding the terminator.
This led to a 0-byte being written somewhere beyond the buffer.

Use the correct base pointer when adding the terminator.

Fixes: f0e386ee0c0b ("printk: fix buffer overflow potential for 
print_text()")
Reported-by: Sven Schnelle 
Signed-off-by: John Ogness 
Signed-off-by: Petr Mladek 
Link: 
https://lore.kernel.org/r/20210124202728.4718-1-john.ogn...@linutronix.de

din should be rolled forward, but there's yet another regression in rc6
breaking suspend on all machines.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/i915: Remove unreachable code

[Chris Wilson]

Quoting Vinicius Tinti (2021-01-30 12:34:11)
> On Fri, Jan 29, 2021 at 08:55:54PM +0000, Chris Wilson wrote:
> > Quoting Vinicius Tinti (2021-01-29 18:15:19)
> > > By enabling -Wunreachable-code-aggressive on Clang the following code
> > > paths are unreachable.
> > 
> > That code exists as commentary and, especially for sdvo, library
> > functions that we may need in future.
> 
> I would argue that this code could be removed since it is in git history.
> It can be restored when needed.
> 
> This will make the code cleaner.

It doesn't change the control flow, so no complexity argument. It
removes documentation from the code, so I have the opposite opinion.

> > The ivb-gt1 case => as we now set the gt level for ivb, should we not
> > enable the optimisation for ivb unaffected by the w/a? Just no one has
> > taken the time to see if it causes a regression.
> 
> I don't know. I just found out that the code is unreachable.
> 
> > For error state, the question remains whether we should revert to
> > uncompressed data if the compressed stream is larger than the original.
> 
> I don't know too.
> 
> In this last two cases the code could be commented and the decisions
> and problems explained in the comment section.

They already are, that is the point.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/i915: Remove unreachable code

[Chris Wilson]

Quoting Vinicius Tinti (2021-01-29 18:15:19)
> By enabling -Wunreachable-code-aggressive on Clang the following code
> paths are unreachable.

That code exists as commentary and, especially for sdvo, library
functions that we may need in future.

The ivb-gt1 case => as we now set the gt level for ivb, should we not
enable the optimisation for ivb unaffected by the w/a? Just no one has
taken the time to see if it causes a regression.

For error state, the question remains whether we should revert to
uncompressed data if the compressed stream is larger than the original.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] linux-next: Tree for Jan 27 (drm/i915)

[Chris Wilson]

Quoting Randy Dunlap (2021-01-27 20:28:05)
> On 1/27/21 11:30 AM, Randy Dunlap wrote:
> > On 1/27/21 11:08 AM, Randy Dunlap wrote:
> >> On 1/27/21 6:44 AM, Stephen Rothwell wrote:
> >>> Hi all,
> >>>
> >>> Note: the patch file has failed to upload :-(
> >>>
> >>> Changes since 20210125:
> >>>
> >>
> >> on x86_64:
> >>
> >> ../drivers/gpu/drm/i915/i915_gem.c: In function ‘i915_gem_freeze_late’:
> >> ../drivers/gpu/drm/i915/i915_gem.c:1182:2: error: implicit declaration of 
> >> function ‘wbinvd_on_all_cpus’; did you mean ‘wrmsr_on_cpus’? 
> >> [-Werror=implicit-function-declaration]
> >>   wbinvd_on_all_cpus();
> >>
> > 
> > My apologies: this was in Andrew's mmotm 2021-01-25.
> > Sorry about that.
> 
> 
> and now I see that it also happens in today's linux-next.

The fix is in the tree that should be feeding into linux-next, so I
trust it will resolve itself shortly. Along with the WERROR depends
snafu.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v5 4/5] drm/i915: Don't assign to struct drm_device.pdev

[Chris Wilson]

Quoting Thomas Zimmermann (2021-01-27 12:41:34)
> Using struct drm_device.pdev is deprecated. Don't assign it. Users
> should upcast from struct drm_device.dev.
> 
> Signed-off-by: Thomas Zimmermann 
> Cc: Jani Nikula 
> Cc: Joonas Lahtinen 
> Cc: Rodrigo Vivi 
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 700aeb923fcd..954ad590089c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -787,7 +787,6 @@ i915_driver_create(struct pci_dev *pdev, const struct 
> pci_device_id *ent)
> if (IS_ERR(i915))
> return i915;
>  
> -   i915->drm.pdev = pdev;
> pci_set_drvdata(pdev, i915);
>  
> /* Device parameters start as a copy of module parameters. */

Stick the mock
-   i915->drm.pdev = pdev;
in this patch, and I'm happy.

With that, the series is
Reviewed-by: Chris Wilson 
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 1/5] drm/i915: Remove references to struct drm_device.pdev

[Chris Wilson]

Quoting Thomas Zimmermann (2021-01-27 12:41:31)
> diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c 
> b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> index 0188f877cab2..2a07a008de2e 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> @@ -146,7 +146,6 @@ struct drm_i915_private *mock_gem_device(void)
> }
>  
> pci_set_drvdata(pdev, i915);
> -   i915->drm.pdev = pdev;

Strictly this removal is still too early. Though it's a mock device and
we shouldn't be touching the pci_dev that often, semantically those
accesses are not removed until later.

>  
> dev_pm_domain_set(>dev, _domain);
> pm_runtime_enable(>dev);
> diff --git a/drivers/gpu/drm/i915/selftests/mock_gtt.c 
> b/drivers/gpu/drm/i915/selftests/mock_gtt.c
> index 7270fc8ca801..5c7ae40bba63 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_gtt.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_gtt.c
> @@ -74,7 +74,7 @@ struct i915_ppgtt *mock_ppgtt(struct drm_i915_private 
> *i915, const char *name)
> ppgtt->vm.i915 = i915;
> ppgtt->vm.total = round_down(U64_MAX, PAGE_SIZE);
> ppgtt->vm.file = ERR_PTR(-ENODEV);
> -   ppgtt->vm.dma = >drm.pdev->dev;
> +   ppgtt->vm.dma = i915->drm.dev;

We can remove this shorthand later.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/i915/gt: use new tasklet API for execution list

[Chris Wilson]

Quoting Emil Renner Berthing (2021-01-26 15:01:55)
> This converts the driver to use the new tasklet API introduced in
> commit 12cc923f1ccc ("tasklet: Introduce new initialization API")
> 
> Signed-off-by: Emil Renner Berthing 
> 
> ---
> v2: Rebased on drm-intel-next

Ta. Saves me having to do the fixup.

Reviewed-by: Chris Wilson 

Will be applied to drm-intel-gt-next which is scheduled for inclusion in
5.13. It should apply against the 5.12 merge window if there's a tree
through which you want to migrate the tasklet API faster.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/i915/gem: fix non-SMP build failure

[Chris Wilson]

Quoting Arnd Bergmann (2021-01-25 12:25:34)
> From: Arnd Bergmann 
> 
> The x86-specific wbinvd_on_all_cpus() function is exported
> through asm/smp.h, causing a build failure in the i915 driver
> when SMP is disabled:
> 
> drivers/gpu/drm/i915/i915_gem.c:1182:2: error: implicit declaration of 
> function 'wbinvd_on_all_cpus' [-Werror,-Wimplicit-function-declaration]

I thought the code was already in i915_gem_pm.c (which included smp.h);
it is now.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] i915: Fix DRM_I915_WERROR dependencies

[Chris Wilson]

Quoting Arnd Bergmann (2021-01-25 12:26:44)
> From: Arnd Bergmann 
> 
> CONFIG_DRM_I915_DEBUG now selects CONFIG_DRM_I915_WERROR, but fails
> to honor its dependencies:
> 
> WARNING: unmet direct dependencies detected for DRM_I915_WERROR
>   Depends on [n]: HAS_IOMEM [=y] && DRM_I915 [=m] && EXPERT [=y] && 
> !COMPILE_TEST [=y]
>   Selected by [m]:
>   - DRM_I915_DEBUG [=y] && HAS_IOMEM [=y] && EXPERT [=y] && DRM_I915 [=m]

DRM_I915_DEBUG now depends on !COMPILE_TEST and EXPERT.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/i915/gvt: fix uninitialized return in intel_gvt_update_reg_whitelist()

[Chris Wilson]

Quoting Dan Carpenter (2021-01-25 08:48:30)
> Smatch found an uninitialized variable bug in this code:
> 
> drivers/gpu/drm/i915/gvt/cmd_parser.c:3191 
> intel_gvt_update_reg_whitelist()
> error: uninitialized symbol 'ret'.
> 
> The first thing that Smatch complains about is that "ret" isn't set if
> we don't enter the "for_each_engine(engine, _priv->gt, id) {" loop.
> Presumably we always have at least one engine so that's a false
> positive.
> 
> But it's definitely a bug to not set "ret" if i915_gem_object_pin_map()
> fails.

True.
 
> Let's fix the bug and silence the false positive.
> 
> Fixes: 493f30cd086e ("drm/i915/gvt: parse init context to update cmd 
> accessible reg whitelist")
> Signed-off-by: Dan Carpenter 
Reviewed-by: Chris Wilson 
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/i915/selftest: Fix potential memory leak

[Chris Wilson]

Quoting Pan Bian (2021-01-22 01:56:40)
> Object out is not released on path that no VMA instance found. The root
> cause is jumping to an unexpected label on the error path.

Wouldn't the root cause be whatever caused the allocation to fail?

Language notwithstanding,
Reviewed-by: Chris Wilson 
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 3/6] drm/i915/gt: Remove references to struct drm_device.pdev

[Chris Wilson]

Quoting Thomas Zimmermann (2021-01-18 13:14:17)
> Using struct drm_device.pdev is deprecated. Convert i915 to struct
> drm_device.dev. No functional changes.

This needs to be before or in the previous patch, as that patch removed
assignment of i915->drm.pdev.

Or the removal of the assignment moved to the end as a separate patch.
That makes more sense.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/i915/userptr: detect un-GUP-able pages early

[Chris Wilson]

Quoting Chris Wilson (2021-01-15 16:56:42)
> Quoting Jinoh Kang (2021-01-15 16:23:31)
> > If GUP-ineligible pages are passed to a GEM userptr object, -EFAULT is
> > returned only when the object is actually bound.
> > 
> > The xf86-video-intel userspace driver cannot differentiate this
> > condition, and marks the GPU as wedged.
> 
> The idea was to call gem_set_domain on the object to validate the pages
> after creation. I only did that for read-only... I did however make mesa
> use set-domain for validation.

Hmm, I remember a reason why we wanted to be lazy in populating the
pages was that we would often be called to create a map that was never
used. So the additional gup + bind was measurable, and we would rather
just have a probe.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/i915/userptr: detect un-GUP-able pages early

[Chris Wilson]

Quoting Jinoh Kang (2021-01-15 16:23:31)
> If GUP-ineligible pages are passed to a GEM userptr object, -EFAULT is
> returned only when the object is actually bound.
> 
> The xf86-video-intel userspace driver cannot differentiate this
> condition, and marks the GPU as wedged.

The idea was to call gem_set_domain on the object to validate the pages
after creation. I only did that for read-only... I did however make mesa
use set-domain for validation.

As a question how are you getting to call userptr on something that
wasn't passed by SHM ipc?

> This not only disables graphics
> acceleration but may also cripple other functions such as VT switch.

That should be a non-sequitur; certainly VT switch works without ever
using the GPU.

> Solve this by "prefaulting" user pages on GEM object creation, testing
> whether all pages are eligible for get_user_pages() in the process.
> On failure, return -EFAULT so that userspace can fallback to software
> blitting.

See https://patchwork.freedesktop.org/series/33449/ for adding PROBE |
POPULATE flags.

But we can just use set-domain.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] dma-buf: Add debug option

[Chris Wilson]

Quoting Daniel Vetter (2021-01-15 15:52:26)
> +static void mangle_sg_table(struct sg_table *sg_table)
> +{
> +#ifdef CONFIG_DMABUF_DEBUG
> +   int i;
> +   struct scatterlist *sg;
> +
> +   if (!sg_table)

if (!IS_ENABLED(CONFIG_DMABUF_DEBUG) || IS_ERR_OR_NULL(sg_table))
> +   return;

Although NULL is not meant to be returned.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm-buf: Add debug option

[Chris Wilson]

Quoting Daniel Vetter (2021-01-14 09:47:40)
> On Thu, Jan 14, 2021 at 09:45:37AM +0000, Chris Wilson wrote:
> > Quoting Daniel Vetter (2021-01-14 09:30:32)
> > > On Thu, Jan 14, 2021 at 10:23 AM Chris Wilson  
> > > wrote:
> > > > The only other problem I see with the implementation is that there's
> > > > nothing that says that each dmabuf->ops->map_dma_buf() returns a new
> > > > sg_table, so we may end up undoing the xor. Or should each dma-buf
> > > > return a fresh dma-mapping for iommu isolation?
> > > 
> > > Maybe I screwed it up, but that's why I extracted the little helpers:
> > > We scramble when we get the sgtable from exporter, and unscramble
> > > before we pass it back. dma-buf.c does some caching and will hand back
> > > the same sgtable, but for that case we don't re-scramble.
> > 
> > The attachment is only mapped once, but there can be more than one
> > attachment, and the backend could return the same sg_table for each
> > mapping. Conceivably, it could return its own private sg_table where it
> > wants to maintain the struct page. Seems like just adding a sentence to
> > @map_dma_buf to clarify that each call should return a new sg_table will
> > suffice.
> 
> Ah yes good point, will augment (once CI stops being angry at me).

Fwiw, with a quick explanation of "don't do this" in the docs,
Reviewed-by: Chris Wilson 
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [BUG] on reboot: bisected to: drm/i915: Shut down displays gracefully on reboot

[Chris Wilson]

Quoting Steven Rostedt (2021-01-14 21:32:06)
> On reboot, one of my test boxes now triggers the following warning:

057fe3535eb3 ("drm/i915: Disable RPM wakeref assertions during driver shutdown")
is included with the drm-intel-fixes PR.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm-buf: Add debug option

[Chris Wilson]

Quoting Daniel Vetter (2021-01-14 09:30:32)
> On Thu, Jan 14, 2021 at 10:23 AM Chris Wilson  
> wrote:
> > The only other problem I see with the implementation is that there's
> > nothing that says that each dmabuf->ops->map_dma_buf() returns a new
> > sg_table, so we may end up undoing the xor. Or should each dma-buf
> > return a fresh dma-mapping for iommu isolation?
> 
> Maybe I screwed it up, but that's why I extracted the little helpers:
> We scramble when we get the sgtable from exporter, and unscramble
> before we pass it back. dma-buf.c does some caching and will hand back
> the same sgtable, but for that case we don't re-scramble.

The attachment is only mapped once, but there can be more than one
attachment, and the backend could return the same sg_table for each
mapping. Conceivably, it could return its own private sg_table where it
wants to maintain the struct page. Seems like just adding a sentence to
@map_dma_buf to clarify that each call should return a new sg_table will
suffice.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm-buf: Add debug option

[Chris Wilson]

Quoting Daniel Vetter (2021-01-14 09:02:57)
> On Wed, Jan 13, 2021 at 10:08 PM Chris Wilson  
> wrote:
> > Quoting Daniel Vetter (2021-01-13 20:50:11)
> > > On Wed, Jan 13, 2021 at 4:43 PM Chris Wilson  
> > > wrote:
> > > >
> > > > Quoting Daniel Vetter (2021-01-13 14:06:04)
> > > > > We have too many people abusing the struct page they can get at but
> > > > > really shouldn't in importers. Aside from that the backing page might
> > > > > simply not exist (for dynamic p2p mappings) looking at it and using it
> > > > > e.g. for mmap can also wreak the page handling of the exporter
> > > > > completely. Importers really must go through the proper interface like
> > > > > dma_buf_mmap for everything.
> > > >
> > > > If the exporter doesn't want to expose the struct page, why are they
> > > > setting it in the exported sg_table?
> > >
> > > You need to store it somewhere, otherwise the dma-api doesn't work.
> > > Essentially this achieves clearing/resetting the struct page pointer,
> > > without additional allocations somewhere, or tons of driver changes
> > > (since presumably the driver does keep track of the struct page
> > > somewhere too).
> >
> > Only for mapping, and that's before the export -- if there's even a
> > struct page to begin with.
> >
> > > Also as long as we have random importers looking at struct page we
> > > can't just remove it, or crashes everywhere. So it has to be some
> > > debug option you can disable.
> >
> > Totally agreed that nothing generic can rely on pages being transported
> > via dma-buf, and memfd is there if you do want a suitable transport. The
> > one I don't know about is dma-buf heap, do both parties there consent to
> > transport pages via the dma-buf? i.e. do they have special cases for
> > import/export between heaps?
> 
> heaps shouldn't be any different wrt the interface exposed to
> importers. Adding John just in case I missed something.
> 
> I think the only problem we have is that the first import for ttm
> simply pulled out the struct page and ignored the sgtable otherwise,
> then that copypasted to places and we're still have some of that left.
> Although it's a lot better. So largely the problem is importers being
> a bit silly.
> 
> I also think I should change the defaulty y to default y if
> DMA_API_DEBUG or something like that, to make sure it's actually
> enabled often enough.

It felt overly draconian, but other than the open question of dma-buf
heaps (which I realise that we need some CI coverage for), I can't
think of a good reason to argue for hiding a struct page transport
within dma-buf.

The only other problem I see with the implementation is that there's
nothing that says that each dmabuf->ops->map_dma_buf() returns a new
sg_table, so we may end up undoing the xor. Or should each dma-buf
return a fresh dma-mapping for iommu isolation?
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm-buf: Add debug option

[Chris Wilson]

Quoting Daniel Vetter (2021-01-13 20:50:11)
> On Wed, Jan 13, 2021 at 4:43 PM Chris Wilson  wrote:
> >
> > Quoting Daniel Vetter (2021-01-13 14:06:04)
> > > We have too many people abusing the struct page they can get at but
> > > really shouldn't in importers. Aside from that the backing page might
> > > simply not exist (for dynamic p2p mappings) looking at it and using it
> > > e.g. for mmap can also wreak the page handling of the exporter
> > > completely. Importers really must go through the proper interface like
> > > dma_buf_mmap for everything.
> >
> > If the exporter doesn't want to expose the struct page, why are they
> > setting it in the exported sg_table?
> 
> You need to store it somewhere, otherwise the dma-api doesn't work.
> Essentially this achieves clearing/resetting the struct page pointer,
> without additional allocations somewhere, or tons of driver changes
> (since presumably the driver does keep track of the struct page
> somewhere too).

Only for mapping, and that's before the export -- if there's even a
struct page to begin with.
 
> Also as long as we have random importers looking at struct page we
> can't just remove it, or crashes everywhere. So it has to be some
> debug option you can disable.

Totally agreed that nothing generic can rely on pages being transported
via dma-buf, and memfd is there if you do want a suitable transport. The
one I don't know about is dma-buf heap, do both parties there consent to
transport pages via the dma-buf? i.e. do they have special cases for
import/export between heaps?
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm-buf: Add debug option

[Chris Wilson]

Quoting Daniel Vetter (2021-01-13 14:06:04)
> We have too many people abusing the struct page they can get at but
> really shouldn't in importers. Aside from that the backing page might
> simply not exist (for dynamic p2p mappings) looking at it and using it
> e.g. for mmap can also wreak the page handling of the exporter
> completely. Importers really must go through the proper interface like
> dma_buf_mmap for everything.

If the exporter doesn't want to expose the struct page, why are they
setting it in the exported sg_table?
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] [v2] i915: fix shift warning

[Chris Wilson]

Quoting Arnd Bergmann (2021-01-03 13:51:44)
> From: Arnd Bergmann 
> 
> Randconfig builds on 32-bit machines show lots of warnings for
> the i915 driver for passing a 32-bit value into __const_hweight64():
> 
> drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:2584:9: error: shift count >= 
> width of type [-Werror,-Wshift-count-overflow]
> return hweight64(VDBOX_MASK(>gt));
>^~~~
> include/asm-generic/bitops/const_hweight.h:29:49: note: expanded from macro 
> 'hweight64'
>  #define hweight64(w) (__builtin_constant_p(w) ? __const_hweight64(w) : 
> __arch_hweight64(w))
> 
> Change it to hweight_long() to avoid the warning.
> 
> Signed-off-by: Arnd Bergmann 
Reviewed-by:: Chris Wilson 
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] i915: fix shift warning

[Chris Wilson]

Quoting Arnd Bergmann (2021-01-02 11:23:20)
> On Wed, Dec 30, 2020 at 4:56 PM Chris Wilson  wrote:
> >
> > Quoting Arnd Bergmann (2020-12-30 15:39:14)
> > > From: Arnd Bergmann 
> > >
> > > Randconfig builds on 32-bit machines show lots of warnings for
> > > the i915 driver for incorrect bit masks like:
> >
> > mask is a u8.
> >
> > VCS0 is 2, I915_MAX_VCS 4
> >
> > (u8 & GENMASK(5, 2)) >> 2
> 
> Ah right, I misread the warning then.
> 
> > > drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:2584:9: error: shift count 
> > > >= width of type [-Werror,-Wshift-count-overflow]
> > > return hweight64(VDBOX_MASK(>gt));
> > >^~~~
> > > include/asm-generic/bitops/const_hweight.h:29:49: note: expanded from 
> > > macro 'hweight64'
> > >  #define hweight64(w) (__builtin_constant_p(w) ? __const_hweight64(w) : 
> > > __arch_hweight64(w))
> >
> > So it's upset by hweight64() on the unsigned long?
> 
> I suspect what is going on is that clang once again warns because it performs
> more code checks before dead-code elimination than gcc does. The warning is
> for the __const_hweight64() case, which is not actually used here because the
> input is not a compile-time constant.
> 
> > So hweight_long?
> 
> That seems to work, I'll send a new version with that.
> 
> > Or use a cast, hweight8((intel_engine_mask_t)VDMASK())?
> >
> > static __always_inline int engine_count(intel_engine_mask_t mask)
> > {
> > return sizeof(mask) == 1 ? hweight8(mask) :
> > sizeof(mask) == 2 ? hweight16(mask) :
> > sizeof(mask) == 4 ? hweight32(mask) :
> > hweight64(mask);
> > }
> 
> Fine with me as well. If you prefer that way, I'll let you handle that.

While we wait for a generic hweight(), lets use hweight_long() here.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] i915: fix shift warning

[Chris Wilson]

Quoting Arnd Bergmann (2020-12-30 15:39:14)
> From: Arnd Bergmann 
> 
> Randconfig builds on 32-bit machines show lots of warnings for
> the i915 driver for incorrect bit masks like:

mask is a u8.

VCS0 is 2, I915_MAX_VCS 4

(u8 & GENMASK(5, 2)) >> 2

> drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:2584:9: error: shift count >= 
> width of type [-Werror,-Wshift-count-overflow]
> return hweight64(VDBOX_MASK(>gt));
>^~~~
> include/asm-generic/bitops/const_hweight.h:29:49: note: expanded from macro 
> 'hweight64'
>  #define hweight64(w) (__builtin_constant_p(w) ? __const_hweight64(w) : 
> __arch_hweight64(w))

So it's upset by hweight64() on the unsigned long?
So hweight_long?

Or use a cast, hweight8((intel_engine_mask_t)VDMASK())?

static __always_inline int engine_count(intel_engine_mask_t mask)
{
return sizeof(mask) == 1 ? hweight8(mask) :
sizeof(mask) == 2 ? hweight16(mask) :
sizeof(mask) == 4 ? hweight32(mask) :
hweight64(mask);
}
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/i915: remove h from printk format specifier

[Chris Wilson]

Quoting t...@redhat.com (2020-12-15 14:41:01)
> From: Tom Rix 
> 
> See Documentation/core-api/printk-formats.rst.
> h should no longer be used in the format specifier for printk.

It's understood by format_decode().
* 'h', 'l', or 'L' for integer fields

At least reference commit cbacb5ab0aa0 ("docs: printk-formats: Stop
encouraging use of unnecessary %h[xudi] and %hh[xudi]") as to why the
printk-formats.rst was altered so we know the code is merely in bad
taste and not using undefined behaviour of printk.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm/i915: Check the correct variable

[Chris Wilson]

Quoting Andi Shyti (2020-12-03 11:12:24)
> Hi Dan,
> 
> > There is a copy and paste bug in this code.  It's supposed to check
> > "obj2" instead of checking "obj" a second time.
> > 
> > Fixes: 80f0b679d6f0 ("drm/i915: Add an implementation for i915_gem_ww_ctx 
> > locking, v2.")
> > Signed-off-by: Dan Carpenter 
> > ---
> >  drivers/gpu/drm/i915/selftests/i915_gem.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/selftests/i915_gem.c 
> > b/drivers/gpu/drm/i915/selftests/i915_gem.c
> > index 23a6132c5f4e..412e21604a05 100644
> > --- a/drivers/gpu/drm/i915/selftests/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/selftests/i915_gem.c
> > @@ -211,8 +211,8 @@ static int igt_gem_ww_ctx(void *arg)
> >   return PTR_ERR(obj);
> >  
> >   obj2 = i915_gem_object_create_internal(i915, PAGE_SIZE);
> > - if (IS_ERR(obj)) {
> > - err = PTR_ERR(obj);
> > + if (IS_ERR(obj2)) {
> > + err = PTR_ERR(obj2);
> 
> Reviewed-by: Andi Shyti 

I gave up waiting for patchwork to spot the original patch, and pushed.

Thanks,
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/i915: Check the correct variable

[Chris Wilson]

Quoting Dan Carpenter (2020-12-03 08:45:17)
> There is a copy and paste bug in this code.  It's supposed to check
> "obj2" instead of checking "obj" a second time.
> 
> Fixes: 80f0b679d6f0 ("drm/i915: Add an implementation for i915_gem_ww_ctx 
> locking, v2.")
> Signed-off-by: Dan Carpenter 
> ---
>  drivers/gpu/drm/i915/selftests/i915_gem.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/selftests/i915_gem.c 
> b/drivers/gpu/drm/i915/selftests/i915_gem.c
> index 23a6132c5f4e..412e21604a05 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_gem.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_gem.c
> @@ -211,8 +211,8 @@ static int igt_gem_ww_ctx(void *arg)
> return PTR_ERR(obj);
>  
> obj2 = i915_gem_object_create_internal(i915, PAGE_SIZE);
> -   if (IS_ERR(obj)) {
> -   err = PTR_ERR(obj);
> +   if (IS_ERR(obj2)) {
> +   err = PTR_ERR(obj2);

Reviewed-by: Chris Wilson 
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 092/162] drm/i915/uapi: introduce drm_i915_gem_create_ext

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:06:08)
> Same old gem_create but with now with extensions support. This is needed
> to support various upcoming usecases. For now we use the extensions
> mechanism to support setting an immutable-priority-list of potential
> placements, at creation time.
> 
> If we wish to set the placements/regions we can simply do:
> 
> struct drm_i915_gem_object_param region_param = { … }; /* Unchanged */
> struct drm_i915_gem_create_ext_setparam setparam_region = {
> .base = { .name = I915_GEM_CREATE_EXT_SETPARAM },
> .param = region_param,
> }
> 
> struct drm_i915_gem_create_ext create_ext = {
> .size = 16 * PAGE_SIZE,
> .extensions = (uintptr_t)_region,
> };
> int err = ioctl(fd, DRM_IOCTL_I915_GEM_CREATE_EXT, _ext);
> if (err) ...

Looking at the existing gem_create, there is no detection of an
unsupported extension. That is there is no rejection of new userspace
asking for placement on an old kernel. (As erroneous as that would be
for many other reasons.)

Unless I've missed something, we need a new ioctl number for CREATEv2.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 124/162] drm/i915/lmem: allocate HWSP in lmem

[Chris Wilson]

Quoting Matthew Auld (2020-11-30 17:17:16)
> On 27/11/2020 13:55, Chris Wilson wrote:
> > Quoting Matthew Auld (2020-11-27 12:06:40)
> >> From: Michel Thierry 
> > 
> > Rationale goes here.
> > 
> > Is this wise? HWSP is very frequently read by the CPU, and expected to
> > be cached on the CPU.
> > 
> > What do the performance profiles indicate?
> 
> Do you have a recommendation for an existing selftest or IGT to help 
> measure this?
> 
> Also are you suggesting moving this to system memory, or just using a 
> different mapping type, if it's placed in local memory? Or maybe try 
> both? Although I'm pretty sceptical about !wc for local memory.

A lot of worries go out of the window if this can be in system memory
and snooped.

For measuring, I suspect there is a lot of chaff that needs to be
removed before individual microbenchmarks like perf/request discern any
difference; although that would be a starting point. We do a lot of
completion checking during execlists interrupt processing, and there we
(cpu profiles at least) are sensitive to uncached reads.

We can trivially construct a benchmark that only shows the impact of the
WC reads; but the point where I think we would first notice from userspace
is client wakeup latency scaling: benchmarks/gem_latency, which was once
a point of major concern. Nowadays, we can couple that with a second
concern about inducing system latency from interrupt processing time.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 118/162] drm/i915/dg1: Reserve first 1MB of local memory

[Chris Wilson]

Quoting Matthew Auld (2020-11-30 11:09:57)
> On 27/11/2020 13:52, Chris Wilson wrote:
> > Quoting Matthew Auld (2020-11-27 12:06:34)
> >> From: Imre Deak 
> >>
> >> On DG1 A0/B0 steppings the first 1MB of local memory must be reserved.
> >> One reason for this is that the 0xA-0xB range is not accessible
> >> by the display, probably since this region is redirected to another
> >> memory location for legacy VGA compatibility.
> >>
> >> BSpec: 50586
> >> Testcase: igt/kms_big_fb/linear-64bpp-rotate-0
> >> Signed-off-by: Imre Deak 
> >> ---
> >>   drivers/gpu/drm/i915/intel_region_lmem.c | 52 
> >>   1 file changed, 52 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_region_lmem.c 
> >> b/drivers/gpu/drm/i915/intel_region_lmem.c
> >> index 939cf0d195a5..eafef7034680 100644
> >> --- a/drivers/gpu/drm/i915/intel_region_lmem.c
> >> +++ b/drivers/gpu/drm/i915/intel_region_lmem.c
> >> @@ -137,6 +137,48 @@ intel_setup_fake_lmem(struct drm_i915_private *i915)
> >>  return mem;
> >>   }
> >>   
> >> +static void get_legacy_lowmem_region(struct intel_uncore *uncore,
> >> +u64 *start, u32 *size)
> >> +{
> >> +   *start = 0;
> >> +   *size = 0;
> >> +
> >> +   if (!IS_DG1_REVID(uncore->i915, DG1_REVID_A0, DG1_REVID_B0))
> >> +   return;
> >> +
> >> +   *size = SZ_1M;
> >> +
> >> +   DRM_DEBUG_DRIVER("LMEM: reserved legacy low-memory 
> >> [0x%llx-0x%llx]\n",
> >> +*start, *start + *size);
> >> +}
> >> +
> >> +static int reserve_lowmem_region(struct intel_uncore *uncore,
> >> +struct intel_memory_region *mem)
> >> +{
> >> +   u64 reserve_start;
> >> +   u64 reserve_end;
> >> +   u64 region_start;
> >> +   u32 region_size;
> >> +   int ret;
> >> +
> >> +   get_legacy_lowmem_region(uncore, _start, _size);
> >> +   reserve_start = region_start;
> >> +   reserve_end = region_start + region_size;
> >> +
> >> +   if (!reserve_end)
> >> +   return 0;
> >> +
> >> +   DRM_INFO("LMEM: reserving low-memory region [0x%llx-0x%llx]\n",
> >> +reserve_start, reserve_end);
> >> +   ret = i915_buddy_alloc_range(>mm, >reserved,
> >> +reserve_start,
> >> +reserve_end - reserve_start);
> > 
> > Isn't this now relative to the stolen offset? Should this be reserved,
> > or excluded like stolen?
> 
> AFAIK stolen is just snipped off at the end of lmem, so I don't think it 
> really matters if we exclude or reserve.

Right, misread, thought it was moving the start point.

> But for this if we exclude then 
> the region.start might have "strange" alignment, which is annoying since 
> alloc(some_power_of_two) might not give us the expected alignment, 
> whereas if we reserve then the allocator is aware, and so we should get 
> the proper alignment. Maybe you have better ideas with how to handle 
> this, but I think keeping the alignment property is nice.

The only tweak I would look at is making this reservation be the
property of the VGA decode. But if this promises not to live into
production, kiss.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 107/162] drm/i915: setup GPU device lmem region

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:06:23)
> From: CQ Tang 
> 
> The lmem region needs to remove the stolen part.
> 
> Cc: Joonas Lahtinen 
> Cc: Matthew Auld 
> Cc: Abdiel Janulgue 
> Cc: Chris P Wilson 
> Cc: Balestrieri, Francesco 
> Cc: Niranjana Vishwanathapura 
> Cc: Venkata S Dhanalakota 
> Cc: Neel Desai 
> Cc: Matthew Brost 
> Cc: Sudeep Dutt 
> Signed-off-by: CQ Tang 
> ---
>  drivers/gpu/drm/i915/i915_reg.h  |  2 ++
>  drivers/gpu/drm/i915/intel_region_lmem.c | 11 +++
>  2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 1af1966ac461..0e01ea0cb0a4 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -12066,6 +12066,8 @@ enum skl_power_gate {
>  #define GEN12_LMEM_CFG_ADDR_MMIO(0xcf58)
>  #define   LMEM_ENABLE  (1 << 31)
>  
> +#define GEN12_GSMBASE  _MMIO(0x108100)
> +
>  /* gamt regs */
>  #define GEN8_L3_LRA_1_GPGPU _MMIO(0x4dd4)
>  #define   GEN8_L3_LRA_1_GPGPU_DEFAULT_VALUE_BDW  0x67F1427F /* max/min for 
> LRA1/2 */
> diff --git a/drivers/gpu/drm/i915/intel_region_lmem.c 
> b/drivers/gpu/drm/i915/intel_region_lmem.c
> index e98582c76de1..7f2b31d469b0 100644
> --- a/drivers/gpu/drm/i915/intel_region_lmem.c
> +++ b/drivers/gpu/drm/i915/intel_region_lmem.c
> @@ -140,20 +140,23 @@ intel_setup_fake_lmem(struct drm_i915_private *i915)
>  static struct intel_memory_region *
>  setup_lmem(struct drm_i915_private *dev_priv)

Am I wrong in thinking lmem should be under gt?

>  {
> +   struct intel_uncore *uncore = _priv->uncore;
> struct pci_dev *pdev = dev_priv->drm.pdev;
> struct intel_memory_region *mem;
> resource_size_t io_start;
> -   resource_size_t size;
> +   resource_size_t lmem_size;
>  
> /* Enables Local Memory functionality in GAM */
> I915_WRITE(GEN12_LMEM_CFG_ADDR, I915_READ(GEN12_LMEM_CFG_ADDR) | 
> LMEM_ENABLE);
>  
> +   /* Stolen starts from GSMBASE on DG1 */
> +   lmem_size = intel_uncore_read64(uncore, GEN12_GSMBASE);
> +
> io_start = pci_resource_start(pdev, 2);
> -   size = pci_resource_len(pdev, 2);

Sanitycheck the two.

size = min(size, lmem_size);

>  
> mem = intel_memory_region_create(dev_priv,
>  0,
> -size,
> +lmem_size,

Ok, stolen is at tail not start. 

>  I915_GTT_PAGE_SIZE_4K,
>  io_start,
>  _region_lmem_ops);
> @@ -162,7 +165,7 @@ setup_lmem(struct drm_i915_private *dev_priv)
> DRM_INFO("Intel graphics LMEM IO start: %llx\n",
>  (u64)mem->io_start);
> DRM_INFO("Intel graphics LMEM size: %llx\n",
> -(u64)size);
> +(u64)lmem_size);

Use the correct printf-formats, %pa.

> }
>  
> return mem;
> -- 
> 2.26.2
>
-
Intel Corporation (UK) Limited
Registered No. 1134945 (England)
Registered Office: Pipers Way, Swindon SN3 1RJ
VAT No: 860 2173 47

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [Intel-gfx] [PATCH] drm/i915/ehl: Remove require_force_probe protection

[Chris Wilson]

Quoting Pandey, Hariom (2020-10-28 11:55:04)
> Ok, I have initiated the steps to upgrade the CI machine's silicon & BIOS.

The single ehl we have in CI is still failing to enter rc6, both in the
selftest and runtime testing. And I note that RAPL doesn't recognise it,
so it doesn't report the power consumption.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 006/162] drm/i915: split gen8+ flush and bb_start emission functions to their own file

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:04:42)
> From: Daniele Ceraolo Spurio 
> 
> These functions are independent from the backend used and can therefore
> be split out of the exelists submission file, so they can be re-used by
> the upcoming GuC submission backend.
> 
> Based on a patch by Chris Wilson.
> 
> Signed-off-by: Daniele Ceraolo Spurio 
> Cc: Chris P Wilson 
> Cc: Tvrtko Ursulin 
> Reviewed-by: John Harrison 
> ---
>  drivers/gpu/drm/i915/Makefile |   1 +
>  drivers/gpu/drm/i915/gt/gen8_engine_cs.c  | 393 ++
>  drivers/gpu/drm/i915/gt/gen8_engine_cs.h  |  26 ++
>  .../drm/i915/gt/intel_execlists_submission.c  | 385 +
>  4 files changed, 421 insertions(+), 384 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/gt/gen8_engine_cs.c
>  create mode 100644 drivers/gpu/drm/i915/gt/gen8_engine_cs.h
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index aedbd8f52be8..f9ef5199b124 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -82,6 +82,7 @@ gt-y += \
> gt/gen6_engine_cs.o \
> gt/gen6_ppgtt.o \
> gt/gen7_renderclear.o \
> +   gt/gen8_engine_cs.o \
> gt/gen8_ppgtt.o \
> gt/intel_breadcrumbs.o \
> gt/intel_context.o \
> diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c 
> b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> new file mode 100644
> index ..a96fe108685e
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> @@ -0,0 +1,393 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2014 Intel Corporation
> + */
> +
> +#include "i915_drv.h"
> +#include "intel_execlists_submission.h" /* XXX */
> +#include "intel_gpu_commands.h"
> +#include "intel_ring.h"
> +
> +int gen8_emit_flush_render(struct i915_request *request, u32 mode)

Refresh the names to make the recent schemes.
(rcs when specific, xcs when not)
-Chris
-
Intel Corporation (UK) Limited
Registered No. 1134945 (England)
Registered Office: Pipers Way, Swindon SN3 1RJ
VAT No: 860 2173 47

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 005/162] drm/i915/gt: Rename lrc.c to execlists_submission.c

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:04:41)
> From: Chris Wilson 
> 
> We want to separate the utility functions for controlling the logical
> ring context from the execlists submission mechanism (which is an
> overgrown scheduler).
> 
> This is similar to Daniele's work to split up the files, but being
> selfish I wanted to base it after my own changes to intel_lrc.c petered
> out.

Note in accordance with recent intel_ring_submission.c vs
intel_ring_scheduler.c, this would be intel_execlists_scheduler.c
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 004/162] drm/i915/gt: Move move context layout registers and offsets to lrc_reg.h

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:04:40)
> From: Chris Wilson 
> 
> Cleanup intel_lrc.h by moving some of the residual common register
> definitions into intel_lrc_reg.h, prior to rebranding and splitting off
> the submission backends.
> 
> v2: keep the SCHEDULE enum in the old file, since it is specific to the
> gvt usage of the execlists submission backend (John)
> 
> Signed-off-by: Chris Wilson 
> Signed-off-by: Daniele Ceraolo Spurio  #v2
> Cc: John Harrison 
> Reviewed-by: Tvrtko Ursulin 
> ---
>  drivers/gpu/drm/i915/gt/intel_engine_cs.c |  2 +-
>  drivers/gpu/drm/i915/gt/intel_gt_irq.c|  1 +
>  drivers/gpu/drm/i915/gt/intel_lrc.h   | 39 ---
>  drivers/gpu/drm/i915/gt/intel_lrc_reg.h   | 39 +++
>  drivers/gpu/drm/i915/gvt/mmio_context.h   |  2 ++
>  5 files changed, 43 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c 
> b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index d4e988b2816a..02ea16b29c9f 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -36,7 +36,7 @@
>  #include "intel_gt.h"
>  #include "intel_gt_requests.h"
>  #include "intel_gt_pm.h"
> -#include "intel_lrc.h"
> +#include "intel_lrc_reg.h"
>  #include "intel_reset.h"
>  #include "intel_ring.h"
>  
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.c 
> b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
> index 257063a57101..9830342aa6f4 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_irq.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
> @@ -11,6 +11,7 @@
>  #include "intel_breadcrumbs.h"
>  #include "intel_gt.h"
>  #include "intel_gt_irq.h"
> +#include "intel_lrc_reg.h"
>  #include "intel_uncore.h"
>  #include "intel_rps.h"
>  
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.h 
> b/drivers/gpu/drm/i915/gt/intel_lrc.h
> index 802585a308e9..9116b46844a2 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.h
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.h
> @@ -34,45 +34,6 @@ struct i915_request;
>  struct intel_context;
>  struct intel_engine_cs;
>  
> -/* Execlists regs */
> -#define RING_ELSP(base)_MMIO((base) + 0x230)
> -#define RING_EXECLIST_STATUS_LO(base)  _MMIO((base) + 0x234)
> -#define RING_EXECLIST_STATUS_HI(base)  _MMIO((base) + 0x234 + 4)
> -#define RING_CONTEXT_CONTROL(base) _MMIO((base) + 0x244)
> -#define  CTX_CTRL_INHIBIT_SYN_CTX_SWITCH   (1 << 3)
> -#define  CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT   (1 << 0)
> -#define   CTX_CTRL_RS_CTX_ENABLE   (1 << 1)
> -#define  CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT  (1 << 2)
> -#define  GEN12_CTX_CTRL_OAR_CONTEXT_ENABLE (1 << 8)
> -#define RING_CONTEXT_STATUS_PTR(base)  _MMIO((base) + 0x3a0)
> -#define RING_EXECLIST_SQ_CONTENTS(base)_MMIO((base) + 0x510)
> -#define RING_EXECLIST_CONTROL(base)_MMIO((base) + 0x550)
> -
> -#define  EL_CTRL_LOAD  (1 << 0)
> -
> -/* The docs specify that the write pointer wraps around after 5h, "After 
> status
> - * is written out to the last available status QW at offset 5h, this pointer
> - * wraps to 0."
> - *
> - * Therefore, one must infer than even though there are 3 bits available, 6 
> and
> - * 7 appear to be * reserved.
> - */
> -#define GEN8_CSB_ENTRIES 6
> -#define GEN8_CSB_PTR_MASK 0x7
> -#define GEN8_CSB_READ_PTR_MASK (GEN8_CSB_PTR_MASK << 8)
> -#define GEN8_CSB_WRITE_PTR_MASK (GEN8_CSB_PTR_MASK << 0)
> -
> -#define GEN11_CSB_ENTRIES 12
> -#define GEN11_CSB_PTR_MASK 0xf
> -#define GEN11_CSB_READ_PTR_MASK (GEN11_CSB_PTR_MASK << 8)
> -#define GEN11_CSB_WRITE_PTR_MASK (GEN11_CSB_PTR_MASK << 0)
> -
> -#define MAX_CONTEXT_HW_ID (1<<21) /* exclusive */
> -#define MAX_GUC_CONTEXT_HW_ID (1 << 20) /* exclusive */
> -#define GEN11_MAX_CONTEXT_HW_ID (1<<11) /* exclusive */
> -/* in Gen12 ID 0x7FF is reserved to indicate idle */
> -#define GEN12_MAX_CONTEXT_HW_ID(GEN11_MAX_CONTEXT_HW_ID - 1)
> -
>  enum {
> INTEL_CONTEXT_SCHEDULE_IN = 0,
> INTEL_CONTEXT_SCHEDULE_OUT,
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc_reg.h 
> b/drivers/gpu/drm/i915/gt/intel_lrc_reg.h
> index 1b51f7b9a5c3..b2e03ce35599 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc_reg.h
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc_reg.h
> @@ -52,4 +52,43 @@
>  #define GEN8_EXECLISTS_STATUS_BUF 0x370
>  #define GEN11_EXECLI

Re: [RFC PATCH 001/162] drm/i915/selftest: also consider non-contiguous objects

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:04:37)
> In igt_ppgtt_sanity_check we should also exercise the non-contiguous
> option for LMEM, since this will give us slightly different sg layouts
> and alignment.
> 
> Signed-off-by: Matthew Auld 
Reviewed-by: Chris Wilson 
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/i915: remove trailing semicolon in macro definition

[Chris Wilson]

Quoting t...@redhat.com (2020-11-27 16:28:28)
> From: Tom Rix 
> 
> The macro use will already have a semicolon.
> 
> Signed-off-by: Tom Rix 
> ---
>  drivers/gpu/drm/i915/intel_device_info.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_device_info.c 
> b/drivers/gpu/drm/i915/intel_device_info.c
> index e67cec8fa2aa..ef767f04c37c 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.c
> +++ b/drivers/gpu/drm/i915/intel_device_info.c
> @@ -104,7 +104,7 @@ void intel_device_info_print_static(const struct 
> intel_device_info *info,
> drm_printf(p, "ppgtt-type: %d\n", info->ppgtt_type);
> drm_printf(p, "dma_mask_size: %u\n", info->dma_mask_size);
>  
> -#define PRINT_FLAG(name) drm_printf(p, "%s: %s\n", #name, yesno(info->name));
> +#define PRINT_FLAG(name) drm_printf(p, "%s: %s\n", #name, yesno(info->name))
> DEV_INFO_FOR_EACH_FLAG(PRINT_FLAG);

I thought that this was a macro that avoided adding the ';' to each
invocation. Perhaps another time.

Reviewed-by: Chris Wilson 
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 092/162] drm/i915/uapi: introduce drm_i915_gem_create_ext

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:06:08)
> +int
> +i915_gem_create_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file)
> +{
> +   struct drm_i915_private *i915 = to_i915(dev);
> +   struct create_ext ext_data = { .i915 = i915 };
> +   struct drm_i915_gem_create_ext *args = data;
> +   int ret;
> +
> +   i915_gem_flush_free_objects(i915);
> +
> +   ret = i915_user_extensions(u64_to_user_ptr(args->extensions),
> +  create_extensions,
> +  ARRAY_SIZE(create_extensions),
> +  _data);
> +   if (ret)
> +   goto err_free;
> +
> +   if (!ext_data.placements) {
> +   struct intel_memory_region **placements;
> +   enum intel_memory_type mem_type = INTEL_MEMORY_SYSTEM;
> +
> +   placements = kmalloc(sizeof(struct intel_memory_region *),
> +GFP_KERNEL);
> +   if (!placements)
> +   return -ENOMEM;
> +
> +   placements[0] = intel_memory_region_by_type(i915, mem_type);
> +
> +   ext_data.placements = placements;
> +   ext_data.n_placements = 1;
> +   }
> +
> +   ret = i915_gem_create(file,
> + ext_data.placements,
> + ext_data.n_placements,
> + >size, >handle);
> +   if (!ret)
> +   return 0;

Applying the extensions has to happen after creating the vanilla object.

It literally is the equivalent of applying the setparam ioctl to a fresh
object.

Look at the PXP series for how badly wrong this goes if you try it this
way around.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 160/162] drm/i915/dg1: Fix GPU hang due to shmemfs page drop

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:07:16)
> From: Venkata Ramana Nayana 
> 
> This is to fix a bug in upstream
> commit a6326a4f8ffb ("drm/i915/gt: Keep a no-frills swappable copy of the 
> default context state")
> 
> We allocate context state obj ce->state from lmem, so in 
> __engines_record_defaults(),
> we call shmem_create_from_object(). Because it is lmem object, this call will
> create a new shmemfs file, copy the contents into it, and return the file
> pointer and assign to engine->default_state. Of course ce->state lmem object
> is freed at the end of function __engines_record_redefaults().
> 
> Because a new shmemfs file is create for engine->default_state,
> and more importantly, we DON'T mark the pages dirty after we write into it,
> the OS page cache eviction will drop these pages.
> 
> Now with the test move forward, it will create new request/context, and will
> copy the saved engine->default_state into ce->state. If the default_state
> pages are dropped during page cache eviction, the copying will get new pages,
> and copy garbage from the new pages. Next, ce->state will have wrong
> instruction and causes GPU to hang.
> 
> The fixing is very simple, we just mark the shmemfs pages to be dirty when
> writing into it, and also mark the pages to accessed when read/write to them.
> 
> Fixes: a6326a4f8ffb("drm/i915/gt: Keep a no-frills swappable copy of the 
> default context state")

A bug fix, send it. But please write a concise changelog first.

I missed setting the dirty bit, and so the contents were not being saved
on swap out as expected. Impact is severe; any context created after
resume may be gibberish.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 157/162] drm/i915: Improve accuracy of eviction stats

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:07:13)
> From: Tvrtko Ursulin 
> 
> Current code uses jiffie time to do the accounting and then does:
> 
>   diff = jiffies - start;
>   msec = diff * 1000 / HZ;
>   ...
>   atomic_long_add(msec, >time_swap_out_ms);
> 
> If we assume jiffie can be as non-granular as 10ms and that the current
> accounting records all evictions faster than one jiffie as infinite speed,
> we can end up over-estimating the reported eviction throughput.
> 
> Fix this by accumulating ktime_t and only dividing to more user friendly
> granularity at presentation time (debugfs read).
> 
> At the same time consolidate the code a bit and convert from multiple
> atomics to single seqlock per stat.
> 
> Signed-off-by: Tvrtko Ursulin 
> Cc: CQ Tang 
> Cc: Sudeep Dutt 
> Cc: Mika Kuoppala 

A lot of effort to fix up patches after the fact, might as well make it
a real PMU interface.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 150/162] drm/i915: need consider system BO snoop for dgfx

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:07:06)
> From: CQ Tang 
> 
> When cache_level is NONE, we check HAS_LLC(i915).
> But additionally for DGFX, we also need to check
> HAS_SNOOP(i915) on system memory object to use
> I915_BO_CACHE_COHERENT_FOR_READ. on dg1, has_llc=0, and
> has_snoop=1. Otherwise, we set obj->cache_choerent=0 and
> have performance impact.
> 
> Cc: Chris P Wilson 
> Cc: Ramalingam C 
> Cc: Sudeep Dutt 
> Cc: Matthew Auld 
> Signed-off-by: CQ Tang 
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_object.c | 16 +++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c 
> b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> index ddb448f275eb..be603171c444 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> @@ -95,6 +95,20 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj,
> mutex_init(>mm.get_dma_page.lock);
>  }
>  
> +static bool i915_gem_object_use_llc(struct drm_i915_gem_object *obj)
> +{
> +   struct drm_i915_private *i915 = to_i915(obj->base.dev);
> +
> +   if (HAS_LLC(i915))
> +   return true;
> +
> +   if (IS_DGFX(i915) && HAS_SNOOP(i915) &&
> +   !i915_gem_object_is_lmem(obj))
> +   return true;
> +
> +   return false;
> +}
> +
>  /**
>   * Mark up the object's coherency levels for a given cache_level
>   * @obj: #drm_i915_gem_object
> @@ -108,7 +122,7 @@ void i915_gem_object_set_cache_coherency(struct 
> drm_i915_gem_object *obj,
> if (cache_level != I915_CACHE_NONE)
> obj->cache_coherent = (I915_BO_CACHE_COHERENT_FOR_READ |
>I915_BO_CACHE_COHERENT_FOR_WRITE);
> -   else if (HAS_LLC(to_i915(obj->base.dev)))
> +   else if (i915_gem_object_use_llc(obj))
> obj->cache_coherent = I915_BO_CACHE_COHERENT_FOR_READ;
> else
> obj->cache_coherent = 0;

You must also define obj->cache_level correctly. You can not just assume
the object will be snooped.
-Chris
-
Intel Corporation (UK) Limited
Registered No. 1134945 (England)
Registered Office: Pipers Way, Swindon SN3 1RJ
VAT No: 860 2173 47

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 148/162] drm/i915: suspend/resume enable blitter eviction

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:07:04)
> From: Venkata Ramana Nayana 
> 
> In suspend mode use blitter eviction before disable the runtime
> interrupts and in resume use blitter after the gem resume happens.

Consider add it to the suspend prepare function.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 147/162] drm/i915/gt: Allocate default ctx objects in SMEM

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:07:03)
> From: Venkata Ramana Nayana 
> 
> If record default objects are created in LMEM and in suspend
> pin the pages of obj (src) and use blitter for eviction. But
> during request creation using blitter context and try to pin the same
> default object, to restore the ctx with default HW values, will leads to
> the dead lock situation. To avoid this, safe to keep these
> objects in SMEM.

Dead patch. Default object state should be recorded as shmemfs.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 146/162] drm/i915/pm: suspend and restore ppgtt mapping

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:07:02)
> From: Prathap Kumar Valsan 
> 
> During suspend we will lose all page tables as they are allocated in
> LMEM. In-order to  make sure that the contexts do not access the
> corrupted page table after we restore, we are evicting all vma's that
> are bound to vm's. This includes kernel vm.
> 
> During resume, we are restoring the page tables back to scratch page.
> 
> Signed-off-by: Prathap Kumar Valsan 
> Signed-off-by: Venkata Ramana Nayana 
> Cc: CQ Tang 
> ---
>  drivers/gpu/drm/i915/gt/gen8_ppgtt.c  |  13 
>  drivers/gpu/drm/i915/gt/gen8_ppgtt.h  |   2 +
>  drivers/gpu/drm/i915/gt/intel_ppgtt.c |   4 +
>  drivers/gpu/drm/i915/i915_drv.c   | 102 +++---
>  4 files changed, 112 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c 
> b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> index b6fcebeef02a..704cab807e0b 100644
> --- a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> +++ b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> @@ -775,3 +775,16 @@ struct i915_ppgtt *gen8_ppgtt_create(struct intel_gt *gt)
> kfree(ppgtt);
> return ERR_PTR(err);
>  }
> +
> +void gen8_restore_ppgtt_mappings(struct i915_address_space *vm)
> +{
> +   const unsigned int count = gen8_pd_top_count(vm);
> +   int i;
> +
> +   for (i = 1; i <= vm->top; i++)
> +   fill_px(vm->scratch[i], vm->scratch[i - 1]->encode);
> +
> +   fill_page_dma(px_base(i915_vm_to_ppgtt(vm)->pd),
> + vm->scratch[vm->top]->encode, count);
> +}
> +
> diff --git a/drivers/gpu/drm/i915/gt/gen8_ppgtt.h 
> b/drivers/gpu/drm/i915/gt/gen8_ppgtt.h
> index 76a08b9c1f5c..3fa4b95aaabd 100644
> --- a/drivers/gpu/drm/i915/gt/gen8_ppgtt.h
> +++ b/drivers/gpu/drm/i915/gt/gen8_ppgtt.h
> @@ -6,8 +6,10 @@
>  #ifndef __GEN8_PPGTT_H__
>  #define __GEN8_PPGTT_H__
>  
> +struct i915_address_space;
>  struct intel_gt;
>  
> +void gen8_restore_ppgtt_mappings(struct i915_address_space *vm);
>  struct i915_ppgtt *gen8_ppgtt_create(struct intel_gt *gt);
>  
>  #endif
> diff --git a/drivers/gpu/drm/i915/gt/intel_ppgtt.c 
> b/drivers/gpu/drm/i915/gt/intel_ppgtt.c
> index 34a02643bb75..9b3eacd12a7e 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ppgtt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ppgtt.c
> @@ -9,6 +9,8 @@
>  #include "intel_gtt.h"
>  #include "gem/i915_gem_lmem.h"
>  #include "gem/i915_gem_region.h"
> +#include "gem/i915_gem_context.h"
> +#include "gem/i915_gem_region.h"
>  #include "gen6_ppgtt.h"
>  #include "gen8_ppgtt.h"
>  
> @@ -317,3 +319,5 @@ void ppgtt_init(struct i915_ppgtt *ppgtt, struct intel_gt 
> *gt)
> ppgtt->vm.vma_ops.set_pages   = ppgtt_set_pages;
> ppgtt->vm.vma_ops.clear_pages = clear_pages;
>  }
> +
> +
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index e8c4931fc818..7115f4db5043 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -64,6 +64,7 @@
>  #include "gem/i915_gem_context.h"
>  #include "gem/i915_gem_ioctls.h"
>  #include "gem/i915_gem_mman.h"
> +#include "gt/gen8_ppgtt.h"
>  #include "gt/intel_gt.h"
>  #include "gt/intel_gt_pm.h"
>  #include "gt/intel_rc6.h"
> @@ -1136,13 +1137,13 @@ static int intel_dmem_evict_buffers(struct drm_device 
> *dev, bool in_suspend)
>  
> mutex_unlock(>objects.lock);
>  
> -   if (in_suspend)
> -   i915_gem_object_unbind(obj, 0);
> -
> if (in_suspend) {
> obj->swapto = NULL;
> obj->evicted = false;
> obj->do_swapping = true;
> +
> +   i915_gem_object_unbind(obj, 0);
> +
> ret = 
> __i915_gem_object_put_pages(obj);
> obj->do_swapping = false;
> if (ret) {
> @@ -1176,6 +1177,43 @@ static int intel_dmem_evict_buffers(struct drm_device 
> *dev, bool in_suspend)
> return ret;
>  }
>  
> +static int i915_gem_suspend_ppgtt_mappings(struct drm_i915_private *i915)
> +{
> +   struct i915_gem_context *ctx, *cn;
> +   int ret;
> +
> +   spin_lock(>gem.contexts.lock);
> +   list_for_each_entry_safe(ctx, cn, >gem.contexts.list, link) {

Wrong list. Bad starting point from GEM.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 144/162] drm/i915: Reset blitter context when unpark engine

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:07:00)
> From: Venkata Ramana Nayana 
> 
> We are only doing it now for kernel_context. We also need to do for the
> copy engine  blitter context.
> 
> Signed-off-by: Venkata Ramana Nayana 
> ---
>  drivers/gpu/drm/i915/gt/intel_engine_pm.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c 
> b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> index 1b2009b4dcb7..69c8ea70d1e8 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> @@ -66,6 +66,11 @@ static int __engine_unpark(struct intel_wakeref *wf)
> ce->ops->reset(ce);
> }

Add a list of pinned volatile contexts to the engine that must be
restored across resume.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 143/162] drm/i915: suspend/resume eviction

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:06:59)
> +static int intel_dmem_evict_buffers(struct drm_device *dev, bool in_suspend)
> +{
> +   struct drm_i915_private *i915 = to_i915(dev);
> +   struct drm_i915_gem_object *obj;
> +   struct intel_memory_region *mem;
> +   int id, ret = 0;
> +
> +   /*
> +* FIXME: Presently using memcpy,
> +* will replace with blitter once
> +* fix the issues.
> +*/

Why hasn't it been fixed then?
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 141/162] drm/i915: Lmem eviction statistics by category

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:06:57)
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 82f431cc38cd..6f0ab363bdee 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1225,6 +1225,11 @@ struct drm_i915_private {
> atomic_long_t num_bytes_swapped_in;
> atomic_long_t time_swap_out_ms;
> atomic_long_t time_swap_in_ms;
> +
> +   atomic_long_t num_bytes_swapped_out_memcpy;
> +   atomic_long_t num_bytes_swapped_in_memcpy;
> +   atomic_long_t time_swap_out_ms_memcpy;
> +   atomic_long_t time_swap_in_ms_memcpy;

See earlier comments about why this will be rejected.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 140/162] drm/i915: window_blt_copy is used for swapin and swapout

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:06:56)
> From: Ramalingam C 
> 
> window_blt_copy feature is used for swapin and swapout based on the i915
> module parameter called enable_eviction.

A module parameter?
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 137/162] drm/i915: blt copy between objs using pre-created vma windows

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:06:53)
> +int i915_window_blt_copy(struct drm_i915_gem_object *dst,
> +struct drm_i915_gem_object *src)
> +{
> +   struct drm_i915_private *i915 = to_i915(src->base.dev);
> +   struct intel_context *ce = i915->gt.engine[BCS0]->blitter_context;
> +   bool src_is_lmem = i915_gem_object_is_lmem(src);
> +   bool dst_is_lmem = i915_gem_object_is_lmem(dst);
> +   struct scatterlist *last_sgl;
> +   struct i915_vma *src_vma, *dst_vma;
> +   struct i915_request *rq;
> +   u64 cur_win_sz, blt_copied, offset;
> +   long timeout;
> +   u32 size;
> +   int err;
> +
> +   src_vma = src_is_lmem ? i915->mm.lmem_window[0] :
> +   i915->mm.smem_window[0];
> +   dst_vma = dst_is_lmem ? i915->mm.lmem_window[1] :
> +   i915->mm.smem_window[1];
> +
> +   if (!src_vma || !dst_vma)
> +   return -ENODEV;
> +
> +   blt_copied = 0;
> +
> +   err = i915_window_blt_copy_prepare_obj(src);
> +   if (err)
> +   return err;
> +
> +   err = i915_window_blt_copy_prepare_obj(dst);
> +   if (err) {
> +   i915_gem_object_unpin_pages(src);
> +   return err;
> +   }
> +
> +   mutex_lock(>mm.window_mutex);
> +   src_vma->obj = src;
> +   dst_vma->obj = dst;
> +   do {
> +   cur_win_sz = min_t(u64, BLT_WINDOW_SZ,
> +  (src->base.size - blt_copied));
> +   offset = blt_copied >> PAGE_SHIFT;
> +   size = ALIGN(cur_win_sz, src->mm.region->min_page_size) >>
> +  PAGE_SHIFT;
> +   intel_partial_pages_for_sg_table(src, src_vma->pages, offset,
> +size, _sgl);
> +
> +   /*
> +* Insert pages into vm, expects the pages to the full
> +* length of VMA. But we may have the pages of <= vma_size.
> +* Hence altering the vma size to match the total size of
> +* the pages attached.
> +*/
> +   src_vma->size = size << PAGE_SHIFT;
> +   i915_insert_vma_pages(src_vma, src_is_lmem);
> +   sg_unmark_end(last_sgl);
> +
> +   /*
> +* Source obj size could be smaller than the dst obj size,
> +* due to the varying min_page_size of the mem regions the
> +* obj belongs to. But when we insert the pages into vm,
> +* the total size of the pages supposed to be multiples of
> +* the min page size of that mem region.
> +*/
> +   size = ALIGN(cur_win_sz, dst->mm.region->min_page_size) >>
> +  PAGE_SHIFT;
> +   intel_partial_pages_for_sg_table(dst, dst_vma->pages, offset,
> +size, _sgl);
> +
> +   dst_vma->size = size << PAGE_SHIFT;
> +   i915_insert_vma_pages(dst_vma, dst_is_lmem);
> +   sg_unmark_end(last_sgl);
> +
> +   rq = i915_request_create(ce);
> +   if (IS_ERR(rq)) {
> +   err = PTR_ERR(rq);
> +   break;
> +   }
> +   if (rq->engine->emit_init_breadcrumb) {
> +   err = rq->engine->emit_init_breadcrumb(rq);
> +   if (unlikely(err)) {
> +   DRM_ERROR("init_breadcrumb failed. %d\n", 
> err);
> +   break;
> +   }
> +   }
> +   err = i915_window_blt_copy_batch_prepare(rq, src_vma, dst_vma,
> +cur_win_sz);
> +   if (err) {
> +   DRM_ERROR("Batch preparation failed. %d\n", err);
> +   i915_request_set_error_once(rq, -EIO);
> +   }
> +
> +   i915_request_get(rq);
> +   i915_request_add(rq);
> +
> +   timeout = i915_request_wait(rq, 0, MAX_SCHEDULE_TIMEOUT);

Locked waits.

> +   if (timeout < 0) {
> +   DRM_ERROR("BLT Request is not completed. %ld\n",
> + timeout);
> +   err = timeout;
> +   i915_request_put(rq);
> +   break;
> +   }
> +
> +   blt_copied += cur_win_sz;
> +   err = 0;
> +   i915_request_put(rq);
> +   flush_work(>gt.engine[BCS0]->retire_work);

Papering (doubtful the paper is successful) over bugs by introducing a
whole load more.

This fails the basic premise that eviction must be pipelined. The PTE
are transient and can be written prior to the copy and kept within the
non-preemptible window of the blt. Thus allowing many evictions to
scheduled in 

Re: [Intel-gfx] [RFC PATCH 134/162] drm/i915/dg1: Measure swap in/out timing stats

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:06:50)
> From: Sudeep Dutt 
> 
> Signed-off-by: Sudeep Dutt 
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_region.c | 16 ++--
>  drivers/gpu/drm/i915/i915_debugfs.c|  3 +++
>  drivers/gpu/drm/i915/i915_drv.h|  2 ++
>  3 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_region.c 
> b/drivers/gpu/drm/i915/gem/i915_gem_region.c
> index ed108dbcb34e..4fab9f6b4bee 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_region.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_region.c
> @@ -15,6 +15,7 @@ i915_gem_object_swapout_pages(struct drm_i915_gem_object 
> *obj,
>  {
> struct drm_i915_private *i915 = to_i915(obj->base.dev);
> struct drm_i915_gem_object *dst, *src;
> +   unsigned long start, diff, msec;
> int err;
>  
> GEM_BUG_ON(obj->swapto);
> @@ -24,6 +25,7 @@ i915_gem_object_swapout_pages(struct drm_i915_gem_object 
> *obj,
> GEM_BUG_ON(!i915->params.enable_eviction);
>  
> assert_object_held(obj);
> +   start = jiffies;
>  
> /* create a shadow object on smem region */
> dst = i915_gem_object_create_shmem(i915, obj->base.size);
> @@ -64,8 +66,12 @@ i915_gem_object_swapout_pages(struct drm_i915_gem_object 
> *obj,
> else
> i915_gem_object_put(dst);
>  
> -   if (!err)
> +   if (!err) {
> +   diff = jiffies - start;
> +   msec = diff * 1000 / HZ;
> +   atomic_long_add(msec, >time_swap_out_ms);
> atomic_long_add(sizes, >num_bytes_swapped_out);
> +   }

This can be done using a kprobe, and with prettier statistics as builtin
functionality.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 133/162] drm/i915/dg1: Track swap in/out stats via debugfs

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:06:49)
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1366b53ac8c9..7b1e95d494e6 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1214,6 +1214,9 @@ struct drm_i915_private {
>  * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your 
> patch
>  * will be rejected. Instead look for a better place.
>  */
> +
> +   atomic_long_t num_bytes_swapped_out;
> +   atomic_long_t num_bytes_swapped_in;

Enough said. Don't mindlessly add fields.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 128/162] drm/i915/dg1: intel_memory_region_evict() changes for eviction

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:06:44)
> From: CQ Tang 
> 
> Function i915_gem_shrink_memory_region() is changed to
> intel_memory_region_evict() and moved from i915_gem_shrinker.c
> to intel_memory_region.c, this function is used to handle local
> memory swapping, in addition to evict purgeable objects only.

We really do not want to conflate the system shrinker with eviction.
Reservation based eviction looks nothing like the shrinker.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 126/162] drm/i915/gem: Update shmem available memory

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:06:42)
> From: Bommu Krishnaiah 
> 
> Update shmem available memory in “intel_memory_region”

Was avail ever set?
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 125/162] drm/i915/lmem: Limit block size to 4G

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:06:41)
> From: Venkata Sandeep Dhanalakota 
> 
> when allocating pages to lmem object of size 4G or greater
> we allocate memory blocks from buddy system.

Any lmem object is from the buddy system.

> In this scenario
> buddy sytem can allocate blocks that can have size >= 4G and
> these blocks require >32b to represent block size with these
> blocks we run into an issue with sg list construction because
> sg->length field is only 32b wide.

Just say the when using scatterlist, the maximum segment size is 4G. In
fact, we can ask sg what the backend maximum is, and use that as our max
order.

The only question is whether this merits a flag, or we just assume that
the buddy allocator is only used for objects and so always presented via
sg?
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 124/162] drm/i915/lmem: allocate HWSP in lmem

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:06:40)
> From: Michel Thierry 

Rationale goes here.

Is this wise? HWSP is very frequently read by the CPU, and expected to
be cached on the CPU.

What do the performance profiles indicate?
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 118/162] drm/i915/dg1: Reserve first 1MB of local memory

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:06:34)
> From: Imre Deak 
> 
> On DG1 A0/B0 steppings the first 1MB of local memory must be reserved.
> One reason for this is that the 0xA-0xB range is not accessible
> by the display, probably since this region is redirected to another
> memory location for legacy VGA compatibility.
> 
> BSpec: 50586
> Testcase: igt/kms_big_fb/linear-64bpp-rotate-0
> Signed-off-by: Imre Deak 
> ---
>  drivers/gpu/drm/i915/intel_region_lmem.c | 52 
>  1 file changed, 52 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_region_lmem.c 
> b/drivers/gpu/drm/i915/intel_region_lmem.c
> index 939cf0d195a5..eafef7034680 100644
> --- a/drivers/gpu/drm/i915/intel_region_lmem.c
> +++ b/drivers/gpu/drm/i915/intel_region_lmem.c
> @@ -137,6 +137,48 @@ intel_setup_fake_lmem(struct drm_i915_private *i915)
> return mem;
>  }
>  
> +static void get_legacy_lowmem_region(struct intel_uncore *uncore,
> +u64 *start, u32 *size)
> +{
> +   *start = 0;
> +   *size = 0;
> +
> +   if (!IS_DG1_REVID(uncore->i915, DG1_REVID_A0, DG1_REVID_B0))
> +   return;
> +
> +   *size = SZ_1M;
> +
> +   DRM_DEBUG_DRIVER("LMEM: reserved legacy low-memory [0x%llx-0x%llx]\n",
> +*start, *start + *size);
> +}
> +
> +static int reserve_lowmem_region(struct intel_uncore *uncore,
> +struct intel_memory_region *mem)
> +{
> +   u64 reserve_start;
> +   u64 reserve_end;
> +   u64 region_start;
> +   u32 region_size;
> +   int ret;
> +
> +   get_legacy_lowmem_region(uncore, _start, _size);
> +   reserve_start = region_start;
> +   reserve_end = region_start + region_size;
> +
> +   if (!reserve_end)
> +   return 0;
> +
> +   DRM_INFO("LMEM: reserving low-memory region [0x%llx-0x%llx]\n",
> +reserve_start, reserve_end);
> +   ret = i915_buddy_alloc_range(>mm, >reserved,
> +reserve_start,
> +reserve_end - reserve_start);

Isn't this now relative to the stolen offset? Should this be reserved,
or excluded like stolen?
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 103/162] drm/i915: allocate context from LMEM

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:06:19)
> Based on a patch from Michel Thierry.
> 
> Signed-off-by: Matthew Auld 
> Cc: Joonas Lahtinen 
> Cc: Abdiel Janulgue 
> ---
>  .../drm/i915/gt/intel_execlists_submission.c  | 31 ++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c 
> b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> index 582a9044727e..c640b90711fd 100644
> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> @@ -108,6 +108,8 @@
>   */
>  #include 
>  
> +#include "gem/i915_gem_lmem.h"
> +
>  #include "i915_drv.h"
>  #include "i915_perf.h"
>  #include "i915_trace.h"
> @@ -4660,6 +4662,21 @@ static struct intel_timeline *pinned_timeline(struct 
> intel_context *ce)
>  page_unmask_bits(tl));
>  }
>  
> +static int context_clear_lmem(struct drm_i915_gem_object *ctx_obj)
> +{
> +   void *vaddr;
> +
> +   vaddr = i915_gem_object_pin_map(ctx_obj, I915_MAP_WC);
> +   if (IS_ERR(vaddr))
> +   return PTR_ERR(vaddr);
> +
> +   memset64(vaddr, 0, ctx_obj->base.size / sizeof(u64));
> +
> +   i915_gem_object_unpin_map(ctx_obj);

What? We copy over the entire object with the default state.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 101/162] drm/i915/gtt/dg1: add PTE_LM plumbing for PPGTT

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:06:17)
> For the PTEs we get an LM bit, to signal whether the page resides in
> SMEM or LMEM.
> 
> Signed-off-by: Matthew Auld 
> Cc: Joonas Lahtinen 
> Cc: Abdiel Janulgue 
> Signed-off-by: Daniele Ceraolo Spurio 
> Signed-off-by: Niranjana Vishwanathapura 
> Signed-off-by: Venkata Sandeep Dhanalakota 
> ---
>  drivers/gpu/drm/i915/gt/gen8_ppgtt.c  | 35 ++-
>  drivers/gpu/drm/i915/gt/intel_gtt.h   |  3 +++
>  drivers/gpu/drm/i915/gt/intel_ppgtt.c |  4 +++
>  3 files changed, 36 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c 
> b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> index e2f1dfc48d43..b6fcebeef02a 100644
> --- a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> +++ b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> @@ -5,6 +5,7 @@
>  
>  #include 
>  
> +#include "gem/i915_gem_lmem.h"
>  #include "gen8_ppgtt.h"
>  #include "i915_scatterlist.h"
>  #include "i915_trace.h"
> @@ -50,6 +51,21 @@ static u64 gen8_pte_encode(dma_addr_t addr,
> return pte;
>  }
>  
> +static u64 gen12_pte_encode(dma_addr_t addr,
> +   enum i915_cache_level level,
> +   u32 flags)
> +{
> +   gen8_pte_t pte = addr | _PAGE_PRESENT | _PAGE_RW;
> +
> +   if (unlikely(flags & PTE_READ_ONLY))
> +   pte &= ~_PAGE_RW;
> +
> +   if (flags & PTE_LM)
> +   pte |= GEN12_PPGTT_PTE_LM;
> +
> +   return pte;
> +}
> +
>  static void gen8_ppgtt_notify_vgt(struct i915_ppgtt *ppgtt, bool create)
>  {
> struct drm_i915_private *i915 = ppgtt->vm.i915;
> @@ -365,7 +381,7 @@ gen8_ppgtt_insert_pte(struct i915_ppgtt *ppgtt,
>   u32 flags)
>  {
> struct i915_page_directory *pd;
> -   const gen8_pte_t pte_encode = gen8_pte_encode(0, cache_level, flags);
> +   const gen8_pte_t pte_encode = ppgtt->vm.pte_encode(0, cache_level, 
> flags);

We don't need the vfunc, since that flag will not be sent for gen8.

That bit test will be cheaper than the retpoline.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 098/162] drm/i915/gtt: map the PD up front

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:06:14)
> We need to general our accessor for the page directories and tables from
> using the simple kmap_atomic to support local memory, and this setup
> must be done on acquisition of the backing storage prior to entering
> fence execution contexts. Here we replace the kmap with the object
> maping code that for simple single page shmemfs object will return a
> plain kmap, that is then kept for the lifetime of the page directory.
> 
> Signed-off-by: Matthew Auld 
> Signed-off-by: Chris Wilson 

We are going to really struggle with this on 32b :(
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 097/162] drm/i915: Distinction of memory regions

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:06:13)
> From: Zbigniew Kempczyński 
> 
> IGTs should be able to choose testing strategy depending on memory
> regions and its sizes. Add region instance number to make this
> easier and descriptive.
> 
> Cc: Matthew Auld 
> Cc: Ramalingam C 
> Cc: Tvrtko Ursulin 
> Cc: Adam Miszczak 
> Signed-off-by: Zbigniew Kempczyński 
> ---
>  drivers/gpu/drm/i915/intel_memory_region.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_memory_region.c 
> b/drivers/gpu/drm/i915/intel_memory_region.c
> index 1f26bc06ec20..cea44ddebe46 100644
> --- a/drivers/gpu/drm/i915/intel_memory_region.c
> +++ b/drivers/gpu/drm/i915/intel_memory_region.c
> @@ -329,6 +329,10 @@ int intel_memory_regions_hw_probe(struct 
> drm_i915_private *i915)
> mem->instance = instance;
> mem->gt = >gt;
>  
> +   if (HAS_LMEM(mem->i915) && type != INTEL_MEMORY_SYSTEM)
> +   intel_memory_region_set_name(mem, "%s%u",
> +mem->name, 
> mem->instance);

sprintf(mem->name, "%s", mem->name)

is that even defined behaviour?
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 093/162] drm/i915/lmem: allocate cmd ring in lmem

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:06:09)
> From: Michel Thierry 
> 
> Signed-off-by: Michel Thierry 
> Signed-off-by: Matthew Auld 
> Cc: Joonas Lahtinen 
> Cc: Abdiel Janulgue 
> ---
>  drivers/gpu/drm/i915/gt/intel_ring.c | 15 +++
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_ring.c 
> b/drivers/gpu/drm/i915/gt/intel_ring.c
> index d636c6ed88b7..aa75e644f3f2 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ring.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ring.c
> @@ -4,6 +4,7 @@
>   * Copyright © 2019 Intel Corporation
>   */
>  
> +#include "gem/i915_gem_lmem.h"
>  #include "gem/i915_gem_object.h"
>  #include "i915_drv.h"
>  #include "i915_vma.h"
> @@ -111,10 +112,16 @@ static struct i915_vma *create_ring_vma(struct 
> i915_ggtt *ggtt, int size)
> struct i915_vma *vma;
>  
> obj = ERR_PTR(-ENODEV);
> -   if (i915_ggtt_has_aperture(ggtt))
> -   obj = i915_gem_object_create_stolen(i915, size);
> -   if (IS_ERR(obj))
> -   obj = i915_gem_object_create_internal(i915, size);
> +   if (HAS_LMEM(i915)) {
> +   obj = i915_gem_object_create_lmem(i915, size,
> + I915_BO_ALLOC_CONTIGUOUS |
> + I915_BO_ALLOC_VOLATILE);

Just create, and keep trying when !lmem returns an error.

Why contiguous, it's vmapped anyway?

> +   } else {
> +   if (i915_ggtt_has_aperture(ggtt))
> +   obj = i915_gem_object_create_stolen(i915, size);
> +   if (IS_ERR(obj))
> +   obj = i915_gem_object_create_internal(i915, size);
> +   }
> if (IS_ERR(obj))
> return ERR_CAST(obj);
>  
> -- 
> 2.26.2
> 
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 092/162] drm/i915/uapi: introduce drm_i915_gem_create_ext

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:06:08)
> Same old gem_create but with now with extensions support. This is needed
> to support various upcoming usecases. For now we use the extensions
> mechanism to support setting an immutable-priority-list of potential
> placements, at creation time.
> 
> If we wish to set the placements/regions we can simply do:
> 
> struct drm_i915_gem_object_param region_param = { … }; /* Unchanged */
> struct drm_i915_gem_create_ext_setparam setparam_region = {
> .base = { .name = I915_GEM_CREATE_EXT_SETPARAM },
> .param = region_param,
> }
> 
> struct drm_i915_gem_create_ext create_ext = {
> .size = 16 * PAGE_SIZE,
> .extensions = (uintptr_t)_region,
> };
> int err = ioctl(fd, DRM_IOCTL_I915_GEM_CREATE_EXT, _ext);
> if (err) ...
> 
> If we use the normal gem_create or gem_create_ext without the
> extensions/placements then we still get the old behaviour with only
> placing the object in system memory.
> 
> One important change here is the returned size will now be rounded up to
> the correct size, depending on the list of placements, where we might
> have minimum page-size restrictions on some platforms when dealing with
> device local-memory.
> 
> Also, we still keep around the i915_gem_object_setparam ioctl, although
> that is now restricted by the placement list(i.e we are not allowed to
> add new placements), and longer term that will be going away wrt setting
> placements, since it was deemed that the kernel doesn't need to support
> a dynamic list of placements, which is now solidified by this uapi
> change.
> 
> Testcase: igt/gem_create/create-ext-placement-sanity-check
> Testcase: igt/gem_create/create-ext-placement-each
> Testcase: igt/gem_create/create-ext-placement-all
> Signed-off-by: Matthew Auld 
> Signed-off-by: CQ Tang 
> Cc: Joonas Lahtinen 
> ---
>  drivers/gpu/drm/i915/Makefile |   1 +
>  drivers/gpu/drm/i915/gem/i915_gem_create.c| 398 ++
>  drivers/gpu/drm/i915/gem/i915_gem_object.c|   2 +
>  .../gpu/drm/i915/gem/i915_gem_object_types.h  |   9 +
>  drivers/gpu/drm/i915/gem/i915_gem_region.c|   4 +
>  drivers/gpu/drm/i915/i915_drv.c   |   2 +-
>  drivers/gpu/drm/i915/i915_gem.c   | 103 +
>  drivers/gpu/drm/i915/intel_memory_region.c|  20 +
>  drivers/gpu/drm/i915/intel_memory_region.h|   4 +
>  include/uapi/drm/i915_drm.h   |  60 +++
>  10 files changed, 500 insertions(+), 103 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_create.c
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index ec361d61230b..3955134feca7 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -134,6 +134,7 @@ gem-y += \
> gem/i915_gem_clflush.o \
> gem/i915_gem_client_blt.o \
> gem/i915_gem_context.o \
> +   gem/i915_gem_create.o \
> gem/i915_gem_dmabuf.o \
> gem/i915_gem_domain.o \
> gem/i915_gem_execbuffer.o \
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c 
> b/drivers/gpu/drm/i915/gem/i915_gem_create.c
> new file mode 100644
> index ..6f6dd4f1ce7e
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c
> @@ -0,0 +1,398 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2020 Intel Corporation
> + */
> +
> +#include "gem/i915_gem_ioctls.h"
> +#include "gem/i915_gem_lmem.h"
> +#include "gem/i915_gem_object_blt.h"
> +#include "gem/i915_gem_region.h"
> +
> +#include "i915_drv.h"
> +#include "i915_user_extensions.h"
> +
> +static u32 max_page_size(struct intel_memory_region **placements,
> +int n_placements)
> +{
> +   u32 max_page_size = 0;
> +   int i;
> +
> +   for (i = 0; i < n_placements; ++i) {
> +   max_page_size = max_t(u32, max_page_size,
> + placements[i]->min_page_size);
> +   }
> +
> +   GEM_BUG_ON(!max_page_size);
> +   return max_page_size;
> +}
> +
> +static int
> +i915_gem_create(struct drm_file *file,
> +   struct intel_memory_region **placements,
> +   int n_placements,
> +   u64 *size_p,
> +   u32 *handle_p)
> +{
> +   struct drm_i915_gem_object *obj;
> +   u32 handle;
> +   u64 size;
> +   int ret;
> +
> +   size = round_up(*size_p, max_page_size(placements, n_placements));
> +   if (size == 0)
> +   return -EINVAL;
> +
> +   /* For most of the ABI (e.g. mmap) we think in system pages */
> +   GEM_BUG_ON(!IS_ALIGNED(size, PAGE_SIZE));
> +
> +   /* Allocate the new object */
> +   obj = i915_gem_object_create_region(placements[0], size, 0);
> +   if (IS_ERR(obj))
> +   return PTR_ERR(obj);
> +
> +   if (i915_gem_object_is_lmem(obj)) {
> +   struct intel_gt *gt = obj->mm.region->gt;
> +   struct intel_context *ce = 

Re: [PATCH] drm/modes: Switch to 64bit maths to avoid integer overflow

[Chris Wilson]

Quoting Ville Syrjälä (2020-10-30 14:43:46)
> On Fri, Oct 30, 2020 at 02:19:45PM +0000, Chris Wilson wrote:
> > Quoting Ville Syrjala (2020-10-22 20:42:56)
> > > From: Ville Syrjälä 
> > > 
> > > The new >8k CEA modes have dotclocks reaching 5.94 GHz, which
> > > means our clock*1000 will now overflow the 32bit unsigned
> > > integer. Switch to 64bit maths to avoid it.
> > > 
> > > Cc: sta...@vger.kernel.org
> > > Reported-by: Randy Dunlap 
> > > Signed-off-by: Ville Syrjälä 
> > > ---
> > > An interesting question how many other place might suffer from similar
> > > overflows. I think i915 should be mostly OK. The one place I know we use
> > > Hz instead kHz is the hsw DPLL code, which I would prefer we also change
> > > to use kHz. The other concern is whether we have any potential overflows
> > > before we check this against the platform's max dotclock.
> > > 
> > > I do have this unreviewed igt series 
> > > https://patchwork.freedesktop.org/series/69531/ which extends the
> > > current testing with some other forms of invalid modes. Could probably
> > > extend that with a mode.clock=INT_MAX test to see if anything else might
> > > trip up.
> > > 
> > > No idea about other drivers.
> > > 
> > >  drivers/gpu/drm/drm_modes.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> > > index 501b4fe55a3d..511cde5c7fa6 100644
> > > --- a/drivers/gpu/drm/drm_modes.c
> > > +++ b/drivers/gpu/drm/drm_modes.c
> > > @@ -762,7 +762,7 @@ int drm_mode_vrefresh(const struct drm_display_mode 
> > > *mode)
> > > if (mode->htotal == 0 || mode->vtotal == 0)
> > > return 0;
> > >  
> > > -   num = mode->clock * 1000;
> > > +   num = mode->clock;
> > > den = mode->htotal * mode->vtotal;
> > 
> > You don't want to promote den to u64 while you are here? We are at
> > 8kx4k, throw in dblscan and some vscan, and we could soon have wacky
> > refresh rates.
> 
> i915 has 16kx8k hard limit currently, and we reject vscan>1
> (wish we could also reject DBLSCAN). So we should not hit
> that, at least not yet. Other drivers might not be so strict
> I guess.
> 
> I have a nagging feeling that other places are in danger of
> overflows if we try to push the current limits significantly.
> But I guess no real harm in going full 64bit here, except
> maybe making it a bit slower.

Reviewed-by: Chris Wilson 
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm/i915: fix error return code in check_partial_mapping()

[Chris Wilson]

Quoting Luo Meng (2020-11-25 01:29:38)
> Fix to return a negative error code from the error handling case
> instead of 0 in function check_partial_mapping(), as done elsewhere
> in this function.

It's not an error, just the end of testing.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] intel: Do not assert on unknown chips in drm_intel_decode_context_alloc

[Chris Wilson]

Quoting Tvrtko Ursulin (2020-11-19 13:42:07)
> 
> On 18/11/2020 17:04, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2020-11-18 16:36:01)
> >> From: Tvrtko Ursulin 
> >>
> >> There is this long standing nit of igt/tools/intel_error_decode asserting
> >> when you feed it an error state from a GPU the local libdrm does not know
> >> of.
> >>
> >> To fix this I need a tweak in drm_intel_decode_context_alloc to make it
> >> not assert but just return NULL (which seems an already possible return
> >> value).
> >>
> >> Signed-off-by: Tvrtko Ursulin 
> > 
> > Good riddance,
> > Reviewed-by: Chris Wilson 
> 
> Thanks, now how can push to drm and is there some testing to be 
> triggered before, or after?

cd intel; for i in tests/gen*.sh; do $i; done

But clearly I haven't built libdrm since automake was dropped.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] intel: Do not assert on unknown chips in drm_intel_decode_context_alloc

[Chris Wilson]

Quoting Tvrtko Ursulin (2020-11-18 16:36:01)
> From: Tvrtko Ursulin 
> 
> There is this long standing nit of igt/tools/intel_error_decode asserting
> when you feed it an error state from a GPU the local libdrm does not know
> of.
> 
> To fix this I need a tweak in drm_intel_decode_context_alloc to make it
> not assert but just return NULL (which seems an already possible return
> value).
> 
> Signed-off-by: Tvrtko Ursulin 

Good riddance,
Reviewed-by: Chris Wilson 
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] Revert "drm: convert drm_atomic_uapi.c to new debug helpers"

[Chris Wilson]

Total wipeout in boot!

<7>[3.739908] i915 :00:02.0: 
[drm:__drm_fb_helper_initial_config_and_unlock] test CRTC 0 primary plane
<7>[3.739916] i915 :00:02.0: 
[drm:__drm_fb_helper_initial_config_and_unlock] test CRTC 1 primary plane
9] Hardware name: Hewlett-Packard HP Pro 3500 Series/2ABF, BIOS 8.11 10/24/2012
<4>[3.754904] Workqueue: events_unbound async_run_entry_fn
<4>[3.754908] RIP: 0010:drm_atomic_set_crtc_for_connector+0xe0/0x120
<4>[3.754910] Code: 24 28 be 10 00 00 00 48 c7 c2 60 b0 38 82 48 8b 78 18 
ff 75 20 8b 85 d8 00 00 00 50 e8 89 45 ff ff 58 31 c0 5a 5b 5d 41 5c c3 <48> 8b 
04 25 00 00 00 00 41 8b 4c 24 28 49 89 d9 be 10 00 00 00 4d
<4>[3.754911] RSP: 0018:c92bfa48 EFLAGS: 00010246
<4>[3.754912] RAX: 0005 RBX: 88800ff1a318 RCX: 
0005
<4>[3.754913] RDX: 816d04f0 RSI: 82388e71 RDI: 
88800fc51038
<4>[3.754914] RBP:  R08: 88810414dc10 R09: 
fffe
<4>[3.754915] R10: 682c1dc7 R11: 24f563d5 R12: 
88800fc51000
<4>[3.754916] R13:  R14: 88800ff1a318 R15: 
88801aab9a00
<4>[3.754918] FS:  () GS:88811b48() 
knlGS:
c9/0x130
<4>[3.755084]  do_bind_con_driver+0x1e5/0x2d0
<4>[3.755087]  do_take_over_console+0x10e/0x180
<4>[3.755089]  do_fbcon_takeover+0x53/0xb0
<4>[3.755092]  register_framebuffer+0x22d/0x310
<4>[3.755095]  __drm_fb_helper_initial_config_and_unlock+0x35d/0x530
<4>[3.755190]  intel_fbdev_initial_config+0xf/0x20 [i915]
<4>[3.755192]  async_run_entry_fn+0x34/0x160
<4>[3.755195]  process_one_work+0x270/0x5c0
<4>[3.755199]  worker_thread+0x37/0x380
<4>[3.755201]  ? process_one_work+0x5c0/0x5c0
<4>[3.755203]  kthread+0x146/0x170
<4>[3.755205]  ? kthread_park+0x80/0x80
<4>[3.755208]  ret_from_fork+0x22/0x30
<4>[3.755211] Modules linked in: i915 mei_hdcp x86_pkg_temp_thermal 
coretemp crct10dif_pclmul crc32_pclmul ghash_clmulni_intel snd_hda_intel 
snd_intel_dspcfg snd_hda_codec r8169 snd_hwdep snd_hda_core realtek mei_me 
snd_pcm mei lpc_ich prime_numbers
<4>[3.755224] CR2: 0000
<4>[3.755226] ---[ end trace df071a2078bd01b3 ]---

Fixes: e3aae683e861 ("drm: convert drm_atomic_uapi.c to new debug helpers")
Signed-off-by: Chris Wilson 
Cc: Simon Ser 
Cc: Daniel Vetter 
Cc: Sam Ravnborg 
---
 drivers/gpu/drm/drm_atomic_uapi.c | 113 +-
 1 file changed, 47 insertions(+), 66 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c 
b/drivers/gpu/drm/drm_atomic_uapi.c
index 9df7f2a170e3..d26077ed518a 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -85,15 +85,13 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state 
*state,
 
drm_mode_copy(>mode, mode);
state->enable = true;
-   drm_dbg_atomic(crtc->dev,
-  "Set [MODE:%s] for [CRTC:%d:%s] state %p\n",
-  mode->name, crtc->base.id, crtc->name, state);
+   DRM_DEBUG_ATOMIC("Set [MODE:%s] for [CRTC:%d:%s] state %p\n",
+mode->name, crtc->base.id, crtc->name, state);
} else {
memset(>mode, 0, sizeof(state->mode));
state->enable = false;
-   drm_dbg_atomic(crtc->dev,
-  "Set [NOMODE] for [CRTC:%d:%s] state %p\n",
-  crtc->base.id, crtc->name, state);
+   DRM_DEBUG_ATOMIC("Set [NOMODE] for [CRTC:%d:%s] state %p\n",
+crtc->base.id, crtc->name, state);
}
 
return 0;
@@ -130,35 +128,31 @@ int drm_atomic_set_mode_prop_for_crtc(struct 
drm_crtc_state *state,
int ret;
 
if (blob->length != sizeof(struct drm_mode_modeinfo)) {
-   drm_dbg_atomic(crtc->dev,
-  "[CRTC:%d:%s] bad mode blob length: 
%zu\n",
-  crtc->base.id, crtc->name,
-  blob->length);
+   DRM_DEBUG_ATOMIC("[CRTC:%d:%s] bad mode blob length: 
%zu\n",
+crtc->base.id, crtc->name,
+blob->length);
return -EINVAL;
}
 
ret = drm_mode_convert_umode(crtc->dev,
 >mode, blob->data);
if (ret) {
-   drm_dbg_atomic(crtc-

Re: [PATCH] drm/modes: Switch to 64bit maths to avoid integer overflow

[Chris Wilson]

Quoting Ville Syrjala (2020-10-22 20:42:56)
> From: Ville Syrjälä 
> 
> The new >8k CEA modes have dotclocks reaching 5.94 GHz, which
> means our clock*1000 will now overflow the 32bit unsigned
> integer. Switch to 64bit maths to avoid it.
> 
> Cc: sta...@vger.kernel.org
> Reported-by: Randy Dunlap 
> Signed-off-by: Ville Syrjälä 
> ---
> An interesting question how many other place might suffer from similar
> overflows. I think i915 should be mostly OK. The one place I know we use
> Hz instead kHz is the hsw DPLL code, which I would prefer we also change
> to use kHz. The other concern is whether we have any potential overflows
> before we check this against the platform's max dotclock.
> 
> I do have this unreviewed igt series 
> https://patchwork.freedesktop.org/series/69531/ which extends the
> current testing with some other forms of invalid modes. Could probably
> extend that with a mode.clock=INT_MAX test to see if anything else might
> trip up.
> 
> No idea about other drivers.
> 
>  drivers/gpu/drm/drm_modes.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 501b4fe55a3d..511cde5c7fa6 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -762,7 +762,7 @@ int drm_mode_vrefresh(const struct drm_display_mode *mode)
> if (mode->htotal == 0 || mode->vtotal == 0)
> return 0;
>  
> -   num = mode->clock * 1000;
> +   num = mode->clock;
> den = mode->htotal * mode->vtotal;

You don't want to promote den to u64 while you are here? We are at
8kx4k, throw in dblscan and some vscan, and we could soon have wacky
refresh rates.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2] drm: Quieten [zero] EDID carping

[Chris Wilson]

We have a few displays in CI that always report their EDID as a bunch of
zeroes. This is consistent behaviour, so one assumes intentional
indication of an "absent" EDID. Flagging these consistent warnings
detracts from CI.

One option would be to ignore the zero EDIDs as intentional behaviour,
but Ville would like to keep the information available for debugging.
The simple alternative then is to reduce the loglevel for all the EDID
dumping from WARN to DEBUG so the information is present but not annoy
CI. Note that the bad EDID dumping is already only shown if
drm.debug=KMS, it's just the loglevel chosen was set to be caught by CI
if it ever occurred as it was expected to be an internal error not
external.

Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/2203
Signed-off-by: Chris Wilson 
Cc: Ville Syrjälä 
---
 drivers/gpu/drm/drm_edid.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 631125b46e04..c7363af731b4 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1844,7 +1844,7 @@ static void connector_bad_edid(struct drm_connector 
*connector,
if (connector->bad_edid_counter++ && !drm_debug_enabled(DRM_UT_KMS))
return;
 
-   drm_warn(connector->dev, "%s: EDID is invalid:\n", connector->name);
+   drm_dbg_kms(connector->dev, "%s: EDID is invalid:\n", connector->name);
for (i = 0; i < num_blocks; i++) {
u8 *block = edid + i * EDID_LENGTH;
char prefix[20];
@@ -1856,7 +1856,7 @@ static void connector_bad_edid(struct drm_connector 
*connector,
else
sprintf(prefix, "\t[%02x] GOOD ", i);
 
-   print_hex_dump(KERN_WARNING,
+   print_hex_dump(KERN_DEBUG,
   prefix, DUMP_PREFIX_NONE, 16, 1,
   block, EDID_LENGTH, false);
}
-- 
2.20.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm: Silence zero EDID carping

[Chris Wilson]

Quoting Ville Syrjälä (2020-10-29 14:07:46)
> On Thu, Oct 29, 2020 at 11:00:30AM +0000, Chris Wilson wrote:
> > We have a few displays in CI that always report their EDID as a bunch of
> > zeroes. This is consistent behavioud, so one assumes intentional
> > indication of an "absent" EDID. Let us treat is as such by silently
> > reporting the zero edid using connector->null_edid_counter, leaving the
> > loud carp to EDID that violate their checksums or otherwise return
> > unexpected illegal data upon reading. These are more likely to be
> > inconsistent bad connections rather than being intended.
> 
> I don't think null_edid_counter is actually used by anything.
> So apart from wondering why the mode list has turned strange
> is there some way I can still see from the logs that the
> EDID has become all zeroes?

The ones in question, it's every time we read the EDID it comes back
zero. I am betting that transient everything-is-zero rather than
spurious data is rare enough not to worry about.

An alternative would be to pass the log level to the bad_edid dumper, or
just make it debug for even gibberish edids?
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm: Silence zero EDID carping

[Chris Wilson]

We have a few displays in CI that always report their EDID as a bunch of
zeroes. This is consistent behavioud, so one assumes intentional
indication of an "absent" EDID. Let us treat is as such by silently
reporting the zero edid using connector->null_edid_counter, leaving the
loud carp to EDID that violate their checksums or otherwise return
unexpected illegal data upon reading. These are more likely to be
inconsistent bad connections rather than being intended.

Signed-off-by: Chris Wilson 
---
 drivers/gpu/drm/drm_edid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 631125b46e04..94549805a204 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1951,7 +1951,7 @@ struct edid *drm_do_get_edid(struct drm_connector 
*connector,
break;
if (i == 0 && drm_edid_is_zero(edid, EDID_LENGTH)) {
connector->null_edid_counter++;
-   goto carp;
+   goto out;
}
}
if (i == 4)
-- 
2.20.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: linux-next: Signed-off-by missing for commit in the drm-intel-fixes tree

[Chris Wilson]

Quoting Stephen Rothwell (2020-10-28 21:28:23)
> Hi all,
> 
> Commit
> 
>   d13208a88f41 ("lockdep: Fix nr_unused_locks")
> 
> is missing a Signed-off-by from its author.
> 
> Also, the author's email name is missing the leading 'P'.

And it shouldn't be in the drm-intel-fixes tree.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm/i915/ehl: Remove require_force_probe protection

[Chris Wilson]

Quoting Rodrigo Vivi (2020-10-19 19:29:36)
> 
> I just checked the CI picture and it looks much better indeed.
> 
> Only bad case being the gt_pm, which is also failing on other platforms.

Not nearly in the same manner. CI is indicating that there is no RC6
entry and no power saving at all; neither in the selftests nor visible
from userspace. That is a critical battery eating bug.

If there's a patch to fix it for ehl and jsl, send it to CI for proving.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm/vgem: Replace vgem_object_funcs with the common drm shmem helper

[Chris Wilson]

Quoting Daniel Vetter (2020-10-12 15:12:50)
> On Mon, Oct 12, 2020 at 03:01:09PM +0100, Chris Wilson wrote:
> > Quoting Daniel Vetter (2020-10-12 14:55:07)
> > > On Mon, Oct 12, 2020 at 12:49 PM Chris Wilson  
> > > wrote:
> > > > Quoting Daniel Vetter (2020-10-09 17:16:06)
> > > > > On Fri, Oct 9, 2020 at 12:21 PM Chris Wilson 
> > > > >  wrote:
> > > > > >
> > > > > > vgem is a minimalistic driver that provides shmemfs objects to
> > > > > > userspace that may then be used as an in-memory surface and 
> > > > > > transported
> > > > > > across dma-buf to other drivers. Since it's introduction,
> > > > > > drm_gem_shmem_helper now provides the same shmemfs facilities and 
> > > > > > so we
> > > > > > can trim vgem to wrap the helper.
> > > > > >
> > > > > > Signed-off-by: Chris Wilson 
> > > > > > ---
> > > > > >  drivers/gpu/drm/Kconfig |   1 +
> > > > > >  drivers/gpu/drm/vgem/vgem_drv.c | 281 
> > > > > > ++--
> > > > > >  drivers/gpu/drm/vgem/vgem_drv.h |  11 --
> > > > > >  3 files changed, 13 insertions(+), 280 deletions(-)
> > > > >
> > > > > Nice diffstat :-)
> > > > >
> > > > > Reviewed-by: Daniel Vetter 
> > > >
> > > > Unfortunately I had to drop the drm_gem_prime_mmap() since the existing
> > > > expectation is that we hand the faulthandler off to shmemfs so we can
> > > > release the module while the memory is exported.
> > > 
> > > That sounds like a broken igt. Once we have refcounting for
> > > outstanding dma_fence/buf or anything else we'll block unloading of
> > > the module (not unbinding of the driver). Which one is that?
> > 
> > The dma-buf is closed; all that remains is the mmap. Then from the
> > perspective of the module, there is no reference back to the module
> > since we delegate handling of the mmap back to the owner, the shmemfs
> > builtin. That allows us to remove the module as its object code is no
> > longer required.
> 
> Oh I know how that's possible, I wonder about which testcase encodes that.
> Because it really shouldn't, since that's quite far away from the rough
> consensus that we cobbled together on dri-devel a few months ago about how
> hotunplug should work. If it's a vgem test, meh, we can change that
> whenever. But if it's a generic test that falls over on vgem, then we need
> to teach it better assumptions.

We intentionally copied the module unload behaviour from i915.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm/vgem: Replace vgem_object_funcs with the common drm shmem helper

[Chris Wilson]

Quoting Daniel Vetter (2020-10-12 14:55:07)
> On Mon, Oct 12, 2020 at 12:49 PM Chris Wilson  
> wrote:
> > Quoting Daniel Vetter (2020-10-09 17:16:06)
> > > On Fri, Oct 9, 2020 at 12:21 PM Chris Wilson  
> > > wrote:
> > > >
> > > > vgem is a minimalistic driver that provides shmemfs objects to
> > > > userspace that may then be used as an in-memory surface and transported
> > > > across dma-buf to other drivers. Since it's introduction,
> > > > drm_gem_shmem_helper now provides the same shmemfs facilities and so we
> > > > can trim vgem to wrap the helper.
> > > >
> > > > Signed-off-by: Chris Wilson 
> > > > ---
> > > >  drivers/gpu/drm/Kconfig |   1 +
> > > >  drivers/gpu/drm/vgem/vgem_drv.c | 281 ++--
> > > >  drivers/gpu/drm/vgem/vgem_drv.h |  11 --
> > > >  3 files changed, 13 insertions(+), 280 deletions(-)
> > >
> > > Nice diffstat :-)
> > >
> > > Reviewed-by: Daniel Vetter 
> >
> > Unfortunately I had to drop the drm_gem_prime_mmap() since the existing
> > expectation is that we hand the faulthandler off to shmemfs so we can
> > release the module while the memory is exported.
> 
> That sounds like a broken igt. Once we have refcounting for
> outstanding dma_fence/buf or anything else we'll block unloading of
> the module (not unbinding of the driver). Which one is that?

The dma-buf is closed; all that remains is the mmap. Then from the
perspective of the module, there is no reference back to the module
since we delegate handling of the mmap back to the owner, the shmemfs
builtin. That allows us to remove the module as its object code is no
longer required.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/vkms: Switch to shmem helpers

[Chris Wilson]

Quoting Daniel Vetter (2020-10-10 00:21:55)
> Inspired by a patch by Chris Wilson for vgem. Plus this gives us vmap
> at the gem bo level, which we need for generic fbdev emulation.
> 
> Luckily shmem also tracks ->vaddr, so we just need to adjust the code
> all over a bit to make this fit.
> 
> Also wire up handle_to_fd, dunno why that was missing.
> 
> Signed-off-by: Daniel Vetter 
> Cc: Chris Wilson 
> Cc: Maarten Lankhorst 
> Cc: Maxime Ripard 
> Cc: Thomas Zimmermann 
> Cc: David Airlie 
> Cc: Daniel Vetter 
> Cc: Rodrigo Siqueira 
> Cc: Melissa Wen 
> Cc: Haneen Mohammed 
> ---
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c 
> b/drivers/gpu/drm/vkms/vkms_composer.c
> index 33c031f27c2c..66c6842d70db 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -5,6 +5,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 

>  static void vkms_release(struct drm_device *dev)
>  {
> @@ -91,9 +82,11 @@ static struct drm_driver vkms_driver = {
> .driver_features= DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM,
> .release= vkms_release,
> .fops   = _driver_fops,
> -   .dumb_create= vkms_dumb_create,
> +   .dumb_create= drm_gem_shmem_dumb_create,

Something worth pointing out is that will create an uncached (well WC)
buffer, but since it is being exported with prime, that is probably for
the better. It might be worth using a memcpy_from_wc() for writeback/CRC
calculations. E.g.

> @@ -129,15 +130,15 @@ static void compose_cursor(struct vkms_composer 
> *cursor_composer,
>void *vaddr_out)
>  {
> +   blend(vaddr_out, cursor_shmem_obj->vaddr,
>   primary_composer, cursor_composer);
>  }
>  
> @@ -147,20 +148,20 @@ static int compose_planes(void **vaddr_out,
>  {
> +   memcpy(*vaddr_out, shmem_obj->vaddr, shmem_obj->base.size);

would benefit from WC accessors.

On the other hand, if the load is so small no one notices, it can wait.

Do we have anything that uses vkms in CI?

Reviewed-by: Chris Wilson 
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] drm/vgem: Replace vgem_object_funcs with the common drm shmem helper

[Chris Wilson]

vgem is a minimalistic driver that provides shmemfs objects to
userspace that may then be used as an in-memory surface and transported
across dma-buf to other drivers. Since vgem's introduction,
drm_gem_shmem_helper now provides the same shmemfs facilities and so we
can trim vgem to wrap the helper.

v2: The gem prime mmap helper does not handle exchanging the vma->file,
so skip the conversion to the common prime helper and leave the patch
only converting to the common shmem helper.

Signed-off-by: Chris Wilson 
Reviewed-by: Daniel Vetter  #v1
---
 drivers/gpu/drm/Kconfig |   1 +
 drivers/gpu/drm/vgem/vgem_drv.c | 284 +++-
 drivers/gpu/drm/vgem/vgem_drv.h |  11 --
 3 files changed, 25 insertions(+), 271 deletions(-)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 147d61b9674e..db2ff76638cd 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -278,6 +278,7 @@ source "drivers/gpu/drm/i915/Kconfig"
 config DRM_VGEM
tristate "Virtual GEM provider"
depends on DRM
+   select DRM_GEM_SHMEM_HELPER
help
  Choose this option to get a virtual graphics memory manager,
  as used by Mesa's software renderer for enhanced performance.
diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c
index f38dd590fa45..6811ac518509 100644
--- a/drivers/gpu/drm/vgem/vgem_drv.c
+++ b/drivers/gpu/drm/vgem/vgem_drv.c
@@ -38,6 +38,7 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -50,87 +51,11 @@
 #define DRIVER_MAJOR   1
 #define DRIVER_MINOR   0
 
-static const struct drm_gem_object_funcs vgem_gem_object_funcs;
-
 static struct vgem_device {
struct drm_device drm;
struct platform_device *platform;
 } *vgem_device;
 
-static void vgem_gem_free_object(struct drm_gem_object *obj)
-{
-   struct drm_vgem_gem_object *vgem_obj = to_vgem_bo(obj);
-
-   kvfree(vgem_obj->pages);
-   mutex_destroy(_obj->pages_lock);
-
-   if (obj->import_attach)
-   drm_prime_gem_destroy(obj, vgem_obj->table);
-
-   drm_gem_object_release(obj);
-   kfree(vgem_obj);
-}
-
-static vm_fault_t vgem_gem_fault(struct vm_fault *vmf)
-{
-   struct vm_area_struct *vma = vmf->vma;
-   struct drm_vgem_gem_object *obj = vma->vm_private_data;
-   /* We don't use vmf->pgoff since that has the fake offset */
-   unsigned long vaddr = vmf->address;
-   vm_fault_t ret = VM_FAULT_SIGBUS;
-   loff_t num_pages;
-   pgoff_t page_offset;
-   page_offset = (vaddr - vma->vm_start) >> PAGE_SHIFT;
-
-   num_pages = DIV_ROUND_UP(obj->base.size, PAGE_SIZE);
-
-   if (page_offset >= num_pages)
-   return VM_FAULT_SIGBUS;
-
-   mutex_lock(>pages_lock);
-   if (obj->pages) {
-   get_page(obj->pages[page_offset]);
-   vmf->page = obj->pages[page_offset];
-   ret = 0;
-   }
-   mutex_unlock(>pages_lock);
-   if (ret) {
-   struct page *page;
-
-   page = shmem_read_mapping_page(
-   file_inode(obj->base.filp)->i_mapping,
-   page_offset);
-   if (!IS_ERR(page)) {
-   vmf->page = page;
-   ret = 0;
-   } else switch (PTR_ERR(page)) {
-   case -ENOSPC:
-   case -ENOMEM:
-   ret = VM_FAULT_OOM;
-   break;
-   case -EBUSY:
-   ret = VM_FAULT_RETRY;
-   break;
-   case -EFAULT:
-   case -EINVAL:
-   ret = VM_FAULT_SIGBUS;
-   break;
-   default:
-   WARN_ON(PTR_ERR(page));
-   ret = VM_FAULT_SIGBUS;
-   break;
-   }
-
-   }
-   return ret;
-}
-
-static const struct vm_operations_struct vgem_gem_vm_ops = {
-   .fault = vgem_gem_fault,
-   .open = drm_gem_vm_open,
-   .close = drm_gem_vm_close,
-};
-
 static int vgem_open(struct drm_device *dev, struct drm_file *file)
 {
struct vgem_file *vfile;
@@ -159,33 +84,15 @@ static void vgem_postclose(struct drm_device *dev, struct 
drm_file *file)
kfree(vfile);
 }
 
-static struct drm_vgem_gem_object *__vgem_gem_create(struct drm_device *dev,
-   unsigned long size)
+static struct drm_gem_shmem_object *__vgem_gem_create(struct drm_device *dev,
+ unsigned long size)
 {
-   struct drm_vgem_gem_object *obj;
-   int ret;
-
-   obj = kzalloc(sizeof(*obj), GFP_KERN