RE: [PATCH 14/14] Doc/gpu/rfc/i915: i915 DG2 uAPI
> -Original Message- > From: C, Ramalingam > Sent: Monday, October 11, 2021 9:12 AM > To: dri-devel ; intel-gfx g...@lists.freedesktop.org> > Cc: Daniel Vetter ; Auld, Matthew > ; Tang, CQ ; Hellstrom, > Thomas ; C, Ramalingam > ; Daniel Vetter > Subject: [PATCH 14/14] Doc/gpu/rfc/i915: i915 DG2 uAPI > > Details of the new features getting added as part of DG2 enabling and their > implicit impact on the uAPI. > > Signed-off-by: Ramalingam C > cc: Daniel Vetter > cc: Matthew Auld > --- > Documentation/gpu/rfc/i915_dg2.rst | 47 > ++ > Documentation/gpu/rfc/index.rst| 3 ++ > 2 files changed, 50 insertions(+) > create mode 100644 Documentation/gpu/rfc/i915_dg2.rst > > diff --git a/Documentation/gpu/rfc/i915_dg2.rst > b/Documentation/gpu/rfc/i915_dg2.rst > new file mode 100644 > index ..a83ca26cd758 > --- /dev/null > +++ b/Documentation/gpu/rfc/i915_dg2.rst > @@ -0,0 +1,47 @@ > + > +I915 DG2 RFC Section > + > + > +Upstream plan > += > +Plan to upstream the DG2 enabling is: > + > +* Merge basic HW enabling for DG2(Still without pciid) > +* Merge the 64k support for lmem > +* Merge the flat CCS enabling patches > +* Add the pciid for DG2 and enable the DG2 in CI > + > + > +64K page support for lmem > += > +On DG2 hw, local-memory supports minimum GTT page size of 64k only. 4k > is not supported anymore. > + > +DG2 hw dont support the 64k(lmem) and 4k(smem) pages in the same > ppgtt > +Page table. Refer the struct drm_i915_gem_create_ext for the implication > of handling the 64k page size. > + > +.. kernel-doc:: include/uapi/drm/i915_drm.h > +:functions: drm_i915_gem_create_ext > + > + > +flat CCS support for lmem > += > +Gen 12+ devices support 3D surfaces compression and compression > +formats. This is accomplished by an additional compression control state > (CCS) stored for each surface. General introduction, OK. > + > +Gen 12 devices(TGL and DG1) stores compression state in a separate region > of memory. > +It is managed by userspace and has an associated set of userspace > +managed page tables used by hardware for address translation. I don't know the purpose of this paragraph, do we need to mention TGL/DG1? This is "Gen 12", not "Gen 12+" in first paragraph. > + > +In Gen 12.5 devices(XEXPSDV and DG2) Flat CCS is introduced to replace > +the userspace managed AUX pagetable with the flat indexed region of > +device memory for storing the compression state Because this is DG2 document, do we need to mention XeHP SDV? > + > +GOP Driver steals a chunk of memory for the CCS surface corresponding > +to the entire range of local memory. The memory required for the CCS of > +the entire local memory is > +1/256 of the main local memory. The Gop driver will also program a > +secure register (XEHPSDV_FLAT_CCS_BASE_ADDR 0x4910) with this address > value. I think it is not necessary to say the CCS base register. This is internal detail. > + > +So the Total local memory available for driver allocation is Total lmem > +size - CCS data size Well, we need to minus the GTT, lmem stolen (DG2 only), and WOPCM. Maybe just say, total local memory available is smaller because of other reserved regions. > + > +Flat CCS data needs to be cleared when a lmem object is allocated. And > +CCS data can be copied in and out of CCS region through > XY_CTRL_SURF_COPY_BLT. OK. --CQ > diff --git a/Documentation/gpu/rfc/index.rst > b/Documentation/gpu/rfc/index.rst index 91e93a705230..afb320ed4028 > 100644 > --- a/Documentation/gpu/rfc/index.rst > +++ b/Documentation/gpu/rfc/index.rst > @@ -20,6 +20,9 @@ host such documentation: > > i915_gem_lmem.rst > > +.. toctree:: > +i915_dg2.rst > + > .. toctree:: > > i915_scheduler.rst > -- > 2.20.1
RE: [Intel-gfx] [PATCH] drm/i915: Perform execbuffer object locking as a separate step
> -Original Message-
> From: Intel-gfx On Behalf Of
> Thomas Hellström
> Sent: Tuesday, June 15, 2021 4:36 AM
> To: intel-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> Cc: Thomas Hellström ; Auld, Matthew
>
> Subject: [Intel-gfx] [PATCH] drm/i915: Perform execbuffer object locking as a
> separate step
>
> To help avoid evicting already resident buffers from the batch we're
> processing, perform locking as a separate step.
>
> Signed-off-by: Thomas Hellström
> ---
> .../gpu/drm/i915/gem/i915_gem_execbuffer.c| 25 --
> -
> 1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index 201fed19d120..394eb40c95b5 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -922,21 +922,38 @@ static int eb_lookup_vmas(struct i915_execbuffer
> *eb)
> return err;
> }
>
> -static int eb_validate_vmas(struct i915_execbuffer *eb)
> +static int eb_lock_vmas(struct i915_execbuffer *eb)
> {
> unsigned int i;
> int err;
>
> - INIT_LIST_HEAD(>unbound);
> -
> for (i = 0; i < eb->buffer_count; i++) {
> - struct drm_i915_gem_exec_object2 *entry = >exec[i];
> struct eb_vma *ev = >vma[i];
> struct i915_vma *vma = ev->vma;
>
> err = i915_gem_object_lock(vma->obj, >ww);
> if (err)
> return err;
> + }
> +
> + return 0;
> +}
> +
> +static int eb_validate_vmas(struct i915_execbuffer *eb) {
> + unsigned int i;
> + int err;
> +
> + INIT_LIST_HEAD(>unbound);
> +
> + err = eb_lock_vmas(eb);
> + if (err)
> + return err;
> +
> + for (i = 0; i < eb->buffer_count; i++) {
> + struct drm_i915_gem_exec_object2 *entry = >exec[i];
> + struct eb_vma *ev = >vma[i];
> + struct i915_vma *vma = ev->vma;
>
> err = eb_pin_vma(eb, entry, ev);
> if (err == -EDEADLK)
Thomas, just checked eb_pin_vma(), it calls i915_vma_pin_ww(), if the object is
already locked, under what condition these calls still return -EDEADLK?
--CQ
> --
> 2.31.1
>
> ___
> Intel-gfx mailing list
> intel-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
RE: [Intel-gfx] [PATCH v4 14/17] drm/i915/pxp: User interface for Protected buffer
> -Original Message-
> From: Intel-gfx On Behalf Of
> Daniele Ceraolo Spurio
> Sent: Monday, May 24, 2021 10:48 PM
> To: intel-...@lists.freedesktop.org
> Cc: Vetter, Daniel ; Huang Sean Z
> ; dri-devel@lists.freedesktop.org; Chris Wilson
> ; Kondapally Kalyan
> ; Bommu, Krishnaiah
>
> Subject: [Intel-gfx] [PATCH v4 14/17] drm/i915/pxp: User interface for
> Protected buffer
>
> From: Bommu Krishnaiah
>
> This api allow user mode to create Protected buffers. Only contexts marked
> as protected are allowed to operate on protected buffers.
>
> We only allow setting the flags at creation time.
>
> All protected objects that have backing storage will be considered invalid
> when the session is destroyed and they won't be usable anymore.
Then these protected objects will be hanging in the system till user call
gem_close() to free them?
If the objects won't be usable anymore, why don't we automatically free these
objects when the session is destroyed?
How is a session started/destroyed? From the code, intel_pxp_init() is called
when loading i915 driver, so I think session lifetime is the same as i915
driver lifetime.
Can we start multiple sessions after loading the driver?
--CQ
>
> Given that the PXP HW supports multiple modes (but we currently only care
> about one), a flag variable has been reserved in the structure used in the
> create_ext ioctl for possible future updates.
>
> This is a rework of the original code by Bommu Krishnaiah. I've kept
> authorship unchanged since significant chunks have not been modified.
>
> v2: split context changes, fix defines and improve documentation (Chris),
> add object invalidation logic
> v3: fix spinlock definition and usage, only validate objects when
> they're first added to a context lut, only remove them once (Chris),
> make protected context flag not mandatory in protected object execbuf
> to avoid abuse (Lionel)
> v4: rebase to new gem_create_ext
>
> Signed-off-by: Bommu Krishnaiah
> Signed-off-by: Daniele Ceraolo Spurio
> Cc: Telukuntla Sreedhar
> Cc: Kondapally Kalyan
> Cc: Gupta Anshuman
> Cc: Huang Sean Z
> Cc: Chris Wilson
> Cc: Lionel Landwerlin
> Cc: Jason Ekstrand
> Cc: Daniel Vetter
> ---
> drivers/gpu/drm/i915/gem/i915_gem_create.c| 26
> .../gpu/drm/i915/gem/i915_gem_execbuffer.c| 15 +++
> drivers/gpu/drm/i915/gem/i915_gem_object.c| 6 +++
> drivers/gpu/drm/i915/gem/i915_gem_object.h| 12 ++
> .../gpu/drm/i915/gem/i915_gem_object_types.h | 13 ++
> drivers/gpu/drm/i915/pxp/intel_pxp.c | 41 +++
> drivers/gpu/drm/i915/pxp/intel_pxp.h | 13 ++
> drivers/gpu/drm/i915/pxp/intel_pxp_types.h| 5 +++
> include/uapi/drm/i915_drm.h | 33 ++-
> 9 files changed, 163 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c
> b/drivers/gpu/drm/i915/gem/i915_gem_create.c
> index 548ddf39d853..c14be3882c35 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_create.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c
> @@ -6,6 +6,7 @@
> #include "gem/i915_gem_ioctls.h"
> #include "gem/i915_gem_lmem.h"
> #include "gem/i915_gem_region.h"
> +#include "pxp/intel_pxp.h"
>
> #include "i915_drv.h"
> #include "i915_trace.h"
> @@ -99,7 +100,11 @@ i915_gem_setup(struct drm_i915_gem_object *obj,
> u64 size)
>
> GEM_BUG_ON(size != obj->base.size);
>
> + if (obj->user_flags & I915_GEM_OBJECT_PROTECTED)
> + intel_pxp_object_add(obj);
> +
> trace_i915_gem_object_create(obj);
> +
> return 0;
> }
>
> @@ -344,8 +349,29 @@ static int ext_set_placements(struct
> i915_user_extension __user *base,
> return set_placements(, data);
> }
>
> +static int ext_set_protected(struct i915_user_extension __user *base,
> +void *data) {
> + struct drm_i915_gem_create_ext_protected_content ext;
> + struct create_ext *ext_data = data;
> +
> + if (copy_from_user(, base, sizeof(ext)))
> + return -EFAULT;
> +
> + if (ext.flags)
> + return -EINVAL;
> +
> + if (!intel_pxp_is_enabled(_data->i915->gt.pxp))
> + return -ENODEV;
> +
> + ext_data->vanilla_object->user_flags |=
> I915_GEM_OBJECT_PROTECTED;
> +
> + return 0;
> +}
> +
> +
> static const i915_user_extension_fn create_extensions[] = {
> [I915_GEM_CREATE_EXT_MEMORY_REGIONS] =
> ext_set_placements,
> + [I915_GEM_CREATE_EXT_PROTECTED_CONTENT] =
> ext_set_protected,
> };
>
> /**
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index c08e28847064..5dd813d04a9f 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -839,6 +839,21 @@ static struct i915_vma *eb_lookup_vma(struct
> i915_execbuffer *eb, u32 handle)
> if (unlikely(!obj))
> return ERR_PTR(-ENOENT);
>
> +
RE: [Intel-gfx] [PATCH v2 4/7] drm/i915/gtt/dgfx: place the PD in LMEM
> -Original Message-
> From: Intel-gfx On Behalf Of
> Matthew Auld
> Sent: Tuesday, April 27, 2021 1:54 AM
> To: intel-...@lists.freedesktop.org
> Cc: dri-devel@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH v2 4/7] drm/i915/gtt/dgfx: place the PD in LMEM
>
> It's a requirement that for dgfx we place all the paging structures in device
> local-memory.
>
> v2: use i915_coherent_map_type()
> v3: improve the shared dma-resv object comment
>
> Signed-off-by: Matthew Auld
> Cc: Tvrtko Ursulin
> ---
> drivers/gpu/drm/i915/gt/gen8_ppgtt.c | 5 -
> drivers/gpu/drm/i915/gt/intel_gtt.c | 30 +---
> drivers/gpu/drm/i915/gt/intel_gtt.h | 1 +
> 3 files changed, 32 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> index f83496836f0f..11fb5df45a0f 100644
> --- a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> +++ b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> @@ -712,7 +712,10 @@ struct i915_ppgtt *gen8_ppgtt_create(struct intel_gt
> *gt)
>*/
> ppgtt->vm.has_read_only = !IS_GEN_RANGE(gt->i915, 11, 12);
>
> - ppgtt->vm.alloc_pt_dma = alloc_pt_dma;
> + if (HAS_LMEM(gt->i915))
> + ppgtt->vm.alloc_pt_dma = alloc_pt_lmem;
Here we might want to allocate lmem from the 'gt' in the argument, however,
below inside alloc_pt_lmem(), we always allocate lmem to tile0.
Is this desired?
--CQ
> + else
> + ppgtt->vm.alloc_pt_dma = alloc_pt_dma;
>
> err = gen8_init_scratch(>vm);
> if (err)
> diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.c
> b/drivers/gpu/drm/i915/gt/intel_gtt.c
> index d386b89e2758..061c39d2ad51 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gtt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gtt.c
> @@ -7,10 +7,26 @@
>
> #include
>
> +#include "gem/i915_gem_lmem.h"
> #include "i915_trace.h"
> #include "intel_gt.h"
> #include "intel_gtt.h"
>
> +struct drm_i915_gem_object *alloc_pt_lmem(struct i915_address_space
> +*vm, int sz) {
> + struct drm_i915_gem_object *obj;
> +
> + obj = i915_gem_object_create_lmem(vm->i915, sz, 0);
> + /*
> + * Ensure all paging structures for this vm share the same dma-resv
> + * object underneath, with the idea that one object_lock() will lock
> + * them all at once.
> + */
> + if (!IS_ERR(obj))
> + obj->base.resv = >resv;
> + return obj;
> +}
> +
> struct drm_i915_gem_object *alloc_pt_dma(struct i915_address_space *vm,
> int sz) {
> struct drm_i915_gem_object *obj;
> @@ -19,7 +35,11 @@ struct drm_i915_gem_object *alloc_pt_dma(struct
> i915_address_space *vm, int sz)
> i915_gem_shrink_all(vm->i915);
>
> obj = i915_gem_object_create_internal(vm->i915, sz);
> - /* ensure all dma objects have the same reservation class */
> + /*
> + * Ensure all paging structures for this vm share the same dma-resv
> + * object underneath, with the idea that one object_lock() will lock
> + * them all at once.
> + */
> if (!IS_ERR(obj))
> obj->base.resv = >resv;
> return obj;
> @@ -27,9 +47,11 @@ struct drm_i915_gem_object *alloc_pt_dma(struct
> i915_address_space *vm, int sz)
>
> int map_pt_dma(struct i915_address_space *vm, struct
> drm_i915_gem_object *obj) {
> + enum i915_map_type type;
> void *vaddr;
>
> - vaddr = i915_gem_object_pin_map_unlocked(obj, I915_MAP_WB);
> + type = i915_coherent_map_type(vm->i915, obj, true);
> + vaddr = i915_gem_object_pin_map_unlocked(obj, type);
> if (IS_ERR(vaddr))
> return PTR_ERR(vaddr);
>
> @@ -39,9 +61,11 @@ int map_pt_dma(struct i915_address_space *vm,
> struct drm_i915_gem_object *obj)
>
> int map_pt_dma_locked(struct i915_address_space *vm, struct
> drm_i915_gem_object *obj) {
> + enum i915_map_type type;
> void *vaddr;
>
> - vaddr = i915_gem_object_pin_map(obj, I915_MAP_WB);
> + type = i915_coherent_map_type(vm->i915, obj, true);
> + vaddr = i915_gem_object_pin_map(obj, type);
> if (IS_ERR(vaddr))
> return PTR_ERR(vaddr);
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.h
> b/drivers/gpu/drm/i915/gt/intel_gtt.h
> index 40e486704558..44ce27c51631 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gtt.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gtt.h
> @@ -527,6 +527,7 @@ int setup_scratch_page(struct i915_address_space
> *vm); void free_scratch(struct i915_address_space *vm);
>
> struct drm_i915_gem_object *alloc_pt_dma(struct i915_address_space *vm,
> int sz);
> +struct drm_i915_gem_object *alloc_pt_lmem(struct i915_address_space
> +*vm, int sz);
> struct i915_page_table *alloc_pt(struct i915_address_space *vm); struct
> i915_page_directory *alloc_pd(struct i915_address_space *vm); struct
> i915_page_directory *__alloc_pd(int npde);
> --
> 2.26.3
>
> ___
> Intel-gfx mailing list
> intel-...@lists.freedesktop.org
>
RE: [PATCH 1/3] drm: Restore driver.preclose() for all to use
> -Original Message- > From: Chris Wilson > Sent: Tuesday, July 28, 2020 9:28 AM > To: Daniel Vetter ; Dave Airlie > Cc: intel-gfx ; stable > ; Gustavo Padovan > ; Tang, CQ ; dri- > devel ; Vetter, Daniel > > Subject: Re: [PATCH 1/3] drm: Restore driver.preclose() for all to use > > Quoting Daniel Vetter (2020-07-27 20:32:45) > > On Thu, Jul 23, 2020 at 7:21 PM Chris Wilson > wrote: > > > > > > An unfortunate sequence of events, but it turns out there is a valid > > > usecase for being able to free/decouple the driver objects before > > > they are freed by the DRM core. In particular, if we have a pointer > > > into a drm core object from inside a driver object, that pointer > > > needs to be nerfed *before* it is freed so that concurrent access > > > (e.g. debugfs) does not following the dangling pointer. > > > > > > The legacy marker was adding in the code movement from drp_fops.c to > > > drm_file.c > > > > I might fumble a lot, but not this one: > > > > commit 45c3d213a400c952ab7119f394c5293bb6877e6b > > Author: Daniel Vetter > > Date: Mon May 8 10:26:33 2017 +0200 > > > > drm: Nerf the preclose callback for modern drivers > > Gah, when I going through the history it looked like it appeared out of > nowhere. > > > Also looking at the debugfs hook that has some rather adventurous > > stuff going on I think, feels a bit like a kitchensink with batteries > > included. If that's really all needed I'd say iterate the contexts by > > first going over files, then the ctx (which arent shared anyway) and > > the problem should also be gone. > > Or we could cut out the middlelayer and put the release under the driver > control with a call to the drm_release() when the driver is ready. Chiris, can explain this idea, or post a patch ? --CQ > -Chris ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH 1/3] drm: Restore driver.preclose() for all to use
> -Original Message-
> From: Daniel Vetter
> Sent: Monday, July 27, 2020 12:33 PM
> To: Chris Wilson ; Dave Airlie
> Cc: intel-gfx ; stable
> ; Gustavo Padovan
> ; Tang, CQ ; dri-
> devel ; Vetter, Daniel
>
> Subject: Re: [PATCH 1/3] drm: Restore driver.preclose() for all to use
>
> On Thu, Jul 23, 2020 at 7:21 PM Chris Wilson
> wrote:
> >
> > An unfortunate sequence of events, but it turns out there is a valid
> > usecase for being able to free/decouple the driver objects before they
> > are freed by the DRM core. In particular, if we have a pointer into a
> > drm core object from inside a driver object, that pointer needs to be
> > nerfed *before* it is freed so that concurrent access (e.g. debugfs)
> > does not following the dangling pointer.
> >
> > The legacy marker was adding in the code movement from drp_fops.c to
> > drm_file.c
>
> I might fumble a lot, but not this one:
>
> commit 45c3d213a400c952ab7119f394c5293bb6877e6b
> Author: Daniel Vetter
> Date: Mon May 8 10:26:33 2017 +0200
>
> drm: Nerf the preclose callback for modern drivers
>
> Also looking at the debugfs hook that has some rather adventurous stuff
> going on I think, feels a bit like a kitchensink with batteries included. If
> that's
> really all needed I'd say iterate the contexts by first going over files,
> then the
> ctx (which arent shared anyway) and the problem should also be gone.
Debugfs code can jump in after drm_gem_release() (where file->object_idr is
destroyed), but before postclose(). At this window, everything is fine for
debugfs context accessing except the file->object_idr.
--CQ
> -Daniel
>
> > References: 9acdac68bcdc ("drm: rename drm_fops.c to drm_file.c")
> > Signed-off-by: Chris Wilson
> > Cc: Daniel Vetter
> > Cc: Gustavo Padovan
> > Cc: CQ Tang
> > Cc: # v4.12+
> > ---
> > drivers/gpu/drm/drm_file.c | 3 +--
> > 1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> > index 0ac4566ae3f4..7b4258d6f7cc 100644
> > --- a/drivers/gpu/drm/drm_file.c
> > +++ b/drivers/gpu/drm/drm_file.c
> > @@ -258,8 +258,7 @@ void drm_file_free(struct drm_file *file)
> > (long)old_encode_dev(file->minor->kdev->devt),
> > atomic_read(>open_count));
> >
> > - if (drm_core_check_feature(dev, DRIVER_LEGACY) &&
> > - dev->driver->preclose)
> > + if (dev->driver->preclose)
> > dev->driver->preclose(dev, file);
> >
> > if (drm_core_check_feature(dev, DRIVER_LEGACY))
> > --
> > 2.20.1
> >
> > ___
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH 2/3] drm/i915/gem: Move context decoupling from postclose to preclose
> -Original Message-
> From: Chris Wilson
> Sent: Thursday, July 23, 2020 10:21 AM
> To: intel-...@lists.freedesktop.org
> Cc: dri-devel@lists.freedesktop.org; Chris Wilson ;
> Tang, CQ ; Vetter, Daniel ;
> sta...@vger.kernel.org
> Subject: [PATCH 2/3] drm/i915/gem: Move context decoupling from
> postclose to preclose
>
> Since the GEM contexts refer to other GEM state, we need to nerf those
> pointers before that state is freed during drm_gem_release(). We need to
> move i915_gem_context_close() from the postclose callback to the preclose.
>
> In particular, debugfs likes to peek into the GEM contexts, and from there
> peek at the drm core objects. If the context is closed during the peeking, we
> may attempt to dereference a stale core object.
>
> Signed-off-by: Chris Wilson
> Cc: CQ Tang
> Cc: Daniel Vetter
> Cc: sta...@vger.kernel.org
> ---
> drivers/gpu/drm/i915/i915_drv.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c
> b/drivers/gpu/drm/i915/i915_drv.c index 5fd5af4bc855..15242a8c70f7 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1114,11 +1114,15 @@ static void i915_driver_lastclose(struct
> drm_device *dev)
> vga_switcheroo_process_delayed_switch();
> }
>
> +static void i915_driver_preclose(struct drm_device *dev, struct
> +drm_file *file) {
> + i915_gem_context_close(file);
> +}
> +
> static void i915_driver_postclose(struct drm_device *dev, struct drm_file
> *file) {
> struct drm_i915_file_private *file_priv = file->driver_priv;
>
> - i915_gem_context_close(file);
> i915_gem_release(dev, file);
Now we separate i915_gem_context_close() from i915_gem_release() and other
freeing code in postclose(), is there any side effect to allow code to run in
between?
Can we move all postclose() code into preclose()?
--CQ
>
> kfree_rcu(file_priv, rcu);
> @@ -1850,6 +1854,7 @@ static struct drm_driver driver = {
> .release = i915_driver_release,
> .open = i915_driver_open,
> .lastclose = i915_driver_lastclose,
> + .preclose = i915_driver_preclose,
> .postclose = i915_driver_postclose,
>
> .gem_close_object = i915_gem_close_object,
> --
> 2.20.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [Intel-gfx] [PATCH v3 03/37] drm/i915/region: support basic eviction
> -Original Message-
> From: Intel-gfx [mailto:intel-gfx-boun...@lists.freedesktop.org] On Behalf
> Of Chris Wilson
> Sent: Saturday, August 10, 2019 3:19 AM
> To: Auld, Matthew ; intel-
> g...@lists.freedesktop.org
> Cc: dri-devel@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH v3 03/37] drm/i915/region: support basic
> eviction
>
> Quoting Matthew Auld (2019-08-09 23:26:09)
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c
> > b/drivers/gpu/drm/i915/i915_gem.c index 6ff01a404346..8735dea74809
> > 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -1105,6 +1105,23 @@ i915_gem_madvise_ioctl(struct drm_device
> *dev, void *data,
> > !i915_gem_object_has_pages(obj))
> > i915_gem_object_truncate(obj);
> >
> > + if (obj->mm.region) {
> > + mutex_lock(>mm.region->obj_lock);
> > +
> > + switch (obj->mm.madv) {
> > + case I915_MADV_WILLNEED:
> > + list_move(>mm.region_link,
> > + >mm.region->objects);
> > + break;
> > + default:
> > + list_move(>mm.region_link,
> > + >mm.region->purgeable);
> > + break;
> > + }
> > +
> > + mutex_unlock(>mm.region->obj_lock);
> > + }
> > +
> > args->retained = obj->mm.madv != __I915_MADV_PURGED;
>
> Little bit of an impedance mismatch, I hope this turns out fine when
> everything is a memory region.
>
> > mutex_unlock(>mm.lock);
> >
> > diff --git a/drivers/gpu/drm/i915/intel_memory_region.c
> > b/drivers/gpu/drm/i915/intel_memory_region.c
> > index ef12e462acb8..3a3caaadea1f 100644
> > --- a/drivers/gpu/drm/i915/intel_memory_region.c
> > +++ b/drivers/gpu/drm/i915/intel_memory_region.c
> > @@ -12,6 +12,51 @@ const u32 intel_region_map[] = {
> > [INTEL_MEMORY_STOLEN] = BIT(INTEL_STOLEN +
> > INTEL_MEMORY_TYPE_SHIFT) | BIT(0), };
> >
> > +static int
> > +intel_memory_region_evict(struct intel_memory_region *mem,
> > + resource_size_t target,
> > + unsigned int flags) {
> > + struct drm_i915_gem_object *obj;
> > + resource_size_t found;
> > + int err;
> > +
> > + err = 0;
> > + found = 0;
> > +
> > + mutex_lock(>obj_lock);
> > + list_for_each_entry(obj, >purgeable, mm.region_link) {
> > + if (!i915_gem_object_has_pages(obj))
> > + continue;
> > +
> > + if (READ_ONCE(obj->pin_global))
> > + continue;
> > +
> > + if (atomic_read(>bind_count))
> > + continue;
> > +
> > + mutex_unlock(>obj_lock);
> > +
> > + __i915_gem_object_put_pages(obj, I915_MM_SHRINKER);
>
> So we don't really care about the object being bound then? As all we care
> about is the page's pin_count.
>
> So instead of obj->pin_global, obj->bind_bound, you just want
>
> if (atomic_read(>pages.pin_count))
> continue;
>
> as the quick check to see if it is worth preceding.
>
> > + mutex_lock_nested(>mm.lock, I915_MM_SHRINKER);
> > + if (!i915_gem_object_has_pages(obj)) {
> > + obj->mm.madv = __I915_MADV_PURGED;
> > + found += obj->base.size;
> > + }
> > + mutex_unlock(>mm.lock);
>
> The locking here accomplishes what? You just want a boolean from
> put_pages().
I have the same question. But looked the i915_gem_shrink() function, it has
similar code. Do we prevent any race condition here?
I want to use this function for swapping so hope to understand more.
--CQ
>
> > +
> > + if (found >= target)
> > + return 0;
> > +
> > + mutex_lock(>obj_lock);
> > + }
> > +
> > + err = -ENOSPC;
> > + mutex_unlock(>obj_lock);
> > + return err;
> > +}
> ___
> Intel-gfx mailing list
> intel-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
