[RFC,33/60] drm/i915/lmem: support pwrite
diff mbox series

Message ID 20200710115757.290984-34-matthew.auld@intel.com
State New
Headers show
Series
  • DG1 LMEM enabling
Related show

Commit Message

Matthew Auld July 10, 2020, 11:57 a.m. UTC
We need to add support for pwrite'ing an LMEM object.

Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Signed-off-by: Steve Hampson <steven.t.hampson@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_lmem.c | 86 ++++++++++++++++++++++++
 1 file changed, 86 insertions(+)

Comments

Dave Airlie July 13, 2020, 5:09 a.m. UTC | #1
On Fri, 10 Jul 2020 at 22:00, Matthew Auld <matthew.auld@intel.com> wrote:
>
> We need to add support for pwrite'ing an LMEM object.

why? DG1 is a discrete GPU, these interfaces we already gross and
overly hacky for integrated, I'd prefer not to drag them across into
discrete land.

same goes for pread.

You have no legacy userspace here, userspace needs change to support
LMEM, it can be fixed to avoid legacy ioctls paths.

Dave.
Matthew Auld July 14, 2020, 2:35 p.m. UTC | #2
On 13/07/2020 06:09, Dave Airlie wrote:
> On Fri, 10 Jul 2020 at 22:00, Matthew Auld <matthew.auld@intel.com> wrote:
>>
>> We need to add support for pwrite'ing an LMEM object.
> 
> why? DG1 is a discrete GPU, these interfaces we already gross and
> overly hacky for integrated, I'd prefer not to drag them across into
> discrete land.
> 
> same goes for pread.
> 
> You have no legacy userspace here, userspace needs change to support
> LMEM, it can be fixed to avoid legacy ioctls paths.

Ok, there have also been similar discussions internally in the past. I 
think one of the reasons was around IGT, and how keeping the 
pread/pwrite interface meant slightly less pain, also it's not much 
effort to implement for LMEM. If this is a NACK, then I guess the other 
idea was to somehow fallback to mmap and update IGT accordingly.

> 
> Dave.
>
Dave Airlie July 16, 2020, 12:43 a.m. UTC | #3
On Wed, 15 Jul 2020 at 00:35, Matthew Auld <matthew.auld@intel.com> wrote:
>
> On 13/07/2020 06:09, Dave Airlie wrote:
> > On Fri, 10 Jul 2020 at 22:00, Matthew Auld <matthew.auld@intel.com> wrote:
> >>
> >> We need to add support for pwrite'ing an LMEM object.
> >
> > why? DG1 is a discrete GPU, these interfaces we already gross and
> > overly hacky for integrated, I'd prefer not to drag them across into
> > discrete land.
> >
> > same goes for pread.
> >
> > You have no legacy userspace here, userspace needs change to support
> > LMEM, it can be fixed to avoid legacy ioctls paths.
>
> Ok, there have also been similar discussions internally in the past. I
> think one of the reasons was around IGT, and how keeping the
> pread/pwrite interface meant slightly less pain, also it's not much
> effort to implement for LMEM. If this is a NACK, then I guess the other
> idea was to somehow fallback to mmap and update IGT accordingly.

I just don't think we should have internal kernel interfaces for
mapping ram in the kernel address space, seems pointless, makes less
sense with a discrete GPU in the mix, so yes I think NAK for
pread/pwrite at least at this time.

I'd also like to see a hard no relocs policy for DG1 enforced in the kernel.

Dave.
Matthew Auld July 16, 2020, 10:11 a.m. UTC | #4
On 16/07/2020 01:43, Dave Airlie wrote:
> On Wed, 15 Jul 2020 at 00:35, Matthew Auld <matthew.auld@intel.com> wrote:
>>
>> On 13/07/2020 06:09, Dave Airlie wrote:
>>> On Fri, 10 Jul 2020 at 22:00, Matthew Auld <matthew.auld@intel.com> wrote:
>>>>
>>>> We need to add support for pwrite'ing an LMEM object.
>>>
>>> why? DG1 is a discrete GPU, these interfaces we already gross and
>>> overly hacky for integrated, I'd prefer not to drag them across into
>>> discrete land.
>>>
>>> same goes for pread.
>>>
>>> You have no legacy userspace here, userspace needs change to support
>>> LMEM, it can be fixed to avoid legacy ioctls paths.
>>
>> Ok, there have also been similar discussions internally in the past. I
>> think one of the reasons was around IGT, and how keeping the
>> pread/pwrite interface meant slightly less pain, also it's not much
>> effort to implement for LMEM. If this is a NACK, then I guess the other
>> idea was to somehow fallback to mmap and update IGT accordingly.
> 
> I just don't think we should have internal kernel interfaces for
> mapping ram in the kernel address space, seems pointless, makes less
> sense with a discrete GPU in the mix, so yes I think NAK for
> pread/pwrite at least at this time.

Ok.

> 
> I'd also like to see a hard no relocs policy for DG1 enforced in the kernel.

Ok, just checking, is that the case even if we don't require extra code 
to support it? We recently dropped the CPU reloc path completely, in 
favour of single GPU reloc path, and so no special code is required to 
support LMEM, it should just work. IGT of course makes heavy use of 
relocs, so that would need an overhaul.

> 
> Dave.
>
Dave Airlie July 19, 2020, 9:52 p.m. UTC | #5
On Thu, 16 Jul 2020 at 20:11, Matthew Auld <matthew.auld@intel.com> wrote:
>
> On 16/07/2020 01:43, Dave Airlie wrote:
> > On Wed, 15 Jul 2020 at 00:35, Matthew Auld <matthew.auld@intel.com> wrote:
> >>
> >> On 13/07/2020 06:09, Dave Airlie wrote:
> >>> On Fri, 10 Jul 2020 at 22:00, Matthew Auld <matthew.auld@intel.com> wrote:
> >>>>
> >>>> We need to add support for pwrite'ing an LMEM object.
> >>>
> >>> why? DG1 is a discrete GPU, these interfaces we already gross and
> >>> overly hacky for integrated, I'd prefer not to drag them across into
> >>> discrete land.
> >>>
> >>> same goes for pread.
> >>>
> >>> You have no legacy userspace here, userspace needs change to support
> >>> LMEM, it can be fixed to avoid legacy ioctls paths.
> >>
> >> Ok, there have also been similar discussions internally in the past. I
> >> think one of the reasons was around IGT, and how keeping the
> >> pread/pwrite interface meant slightly less pain, also it's not much
> >> effort to implement for LMEM. If this is a NACK, then I guess the other
> >> idea was to somehow fallback to mmap and update IGT accordingly.
> >
> > I just don't think we should have internal kernel interfaces for
> > mapping ram in the kernel address space, seems pointless, makes less
> > sense with a discrete GPU in the mix, so yes I think NAK for
> > pread/pwrite at least at this time.
>
> Ok.
>
> >
> > I'd also like to see a hard no relocs policy for DG1 enforced in the kernel.
>
> Ok, just checking, is that the case even if we don't require extra code
> to support it? We recently dropped the CPU reloc path completely, in
> favour of single GPU reloc path, and so no special code is required to
> support LMEM, it should just work. IGT of course makes heavy use of
> relocs, so that would need an overhaul.

The GPU reloc path is optimising a path that we simply shouldn't need
or be using.

IGT tests relocs, ripping out relocs should reduce the amount of
testing IGT has to do and reduce CI run times. Why carry the techincal
debt deliberately.

I expect the kernel team to be a bit more authorative inside Intel on
why uAPI gets exposed and why, it seems like everytime there is an
attempt to limit the tech debt of carrying forward unnecessary uAPIs
there is some push back for media driver or IGT. Fix stuff and be
harder in pushing back on carrying unneeded interfaces forward so we
future products are less mired in pointless debt. DG1 uAPI should
really be a chance to full review the legacy of integrated graphics +
pre-48-bit VM interfaces and they should all be turned off for DG1 so
that future discrete GPUs can move forward cleaner. I really shouldn't
be the one enforcing this, the i915 team needs to be a bit
authoritative on what is necessary to support.

Dave.
Joonas Lahtinen Aug. 7, 2020, 9:32 a.m. UTC | #6
Quoting Dave Airlie (2020-07-20 00:52:19)
> On Thu, 16 Jul 2020 at 20:11, Matthew Auld <matthew.auld@intel.com> wrote:
> >
> > On 16/07/2020 01:43, Dave Airlie wrote:
> > > On Wed, 15 Jul 2020 at 00:35, Matthew Auld <matthew.auld@intel.com> wrote:
> > >>
> > >> On 13/07/2020 06:09, Dave Airlie wrote:
> > >>> On Fri, 10 Jul 2020 at 22:00, Matthew Auld <matthew.auld@intel.com> wrote:
> > >>>>
> > >>>> We need to add support for pwrite'ing an LMEM object.
> > >>>
> > >>> why? DG1 is a discrete GPU, these interfaces we already gross and
> > >>> overly hacky for integrated, I'd prefer not to drag them across into
> > >>> discrete land.
> > >>>
> > >>> same goes for pread.
> > >>>
> > >>> You have no legacy userspace here, userspace needs change to support
> > >>> LMEM, it can be fixed to avoid legacy ioctls paths.
> > >>
> > >> Ok, there have also been similar discussions internally in the past. I
> > >> think one of the reasons was around IGT, and how keeping the
> > >> pread/pwrite interface meant slightly less pain, also it's not much
> > >> effort to implement for LMEM. If this is a NACK, then I guess the other
> > >> idea was to somehow fallback to mmap and update IGT accordingly.
> > >
> > > I just don't think we should have internal kernel interfaces for
> > > mapping ram in the kernel address space, seems pointless, makes less
> > > sense with a discrete GPU in the mix, so yes I think NAK for
> > > pread/pwrite at least at this time.
> >
> > Ok.
> >
> > >
> > > I'd also like to see a hard no relocs policy for DG1 enforced in the kernel.
> >
> > Ok, just checking, is that the case even if we don't require extra code
> > to support it? We recently dropped the CPU reloc path completely, in
> > favour of single GPU reloc path, and so no special code is required to
> > support LMEM, it should just work. IGT of course makes heavy use of
> > relocs, so that would need an overhaul.
> 
> The GPU reloc path is optimising a path that we simply shouldn't need
> or be using.
> 
> IGT tests relocs, ripping out relocs should reduce the amount of
> testing IGT has to do and reduce CI run times. Why carry the techincal
> debt deliberately.

We still have to optimize and keep the the relocations for the older
generations, where they are used. So can't really be eliminated from
codebase as much of the code is shared.

Agreed on the benefit in the more distant future coming from dropping
the relocations, once pre-Gen12 hardware is no more.

Note that IGT also uses relocations indirectly in non-relocation-specific
testtests, so there is quite some work according to our validation team.

Wrt this RFC, as no extra code is needed, it is faster to get stack
up and running with relocations. It also keeps the changes between
iGFX and dGFX minimal, which should help debugging. So that path was
taken to get the functional RFC out as fast as possible.

Moving away from relocations in both IGT and media driver is being
discussed and worked on. See below.

> I expect the kernel team to be a bit more authorative inside Intel on
> why uAPI gets exposed and why, it seems like everytime there is an
> attempt to limit the tech debt of carrying forward unnecessary uAPIs
> there is some push back for media driver or IGT. Fix stuff and be
> harder in pushing back on carrying unneeded interfaces forward so we
> future products are less mired in pointless debt. DG1 uAPI should
> really be a chance to full review the legacy of integrated graphics +
> pre-48-bit VM interfaces and they should all be turned off for DG1 so
> that future discrete GPUs can move forward cleaner. I really shouldn't
> be the one enforcing this, the i915 team needs to be a bit
> authoritative on what is necessary to support.

The patches were sent out as RFC to collect comments. Based on the
comments, we're expediting the work to eliminate the use of relocations.

Regards, Joonas
Joonas Lahtinen Aug. 7, 2020, 9:46 a.m. UTC | #7
Quoting Dave Airlie (2020-07-13 08:09:30)
> On Fri, 10 Jul 2020 at 22:00, Matthew Auld <matthew.auld@intel.com> wrote:
> >
> > We need to add support for pwrite'ing an LMEM object.
> 
> why? DG1 is a discrete GPU, these interfaces we already gross and
> overly hacky for integrated, I'd prefer not to drag them across into
> discrete land.
> 
> same goes for pread.
> 
> You have no legacy userspace here, userspace needs change to support
> LMEM, it can be fixed to avoid legacy ioctls paths.

(This answer is really along the same lines as related to the
relocations, which I sent earlier in the end of this thread)

PREAD/PWRITE are used by IGT tests indirectly in the testing, as
a means to validate test end results as an example. So IGT
reworking is needed not to lose testing coverage when the
functionality is disabled.

Same reasoning as with relocations, as to why this is included
in the RFC; it will get a functional stack with least changes and
is virtually no extra effort to carry. It's recognized that once
pre-Gen12 hardware ceases to exist, there are optimization
opportunities.

Based on the feedback to this Request for Comments series, we are
expediting the work on those IGT reworks.

Regards, Joonas
Dave Airlie Aug. 9, 2020, 9:06 p.m. UTC | #8
On Fri, 7 Aug 2020 at 19:46, Joonas Lahtinen
<joonas.lahtinen@linux.intel.com> wrote:
>
> Quoting Dave Airlie (2020-07-13 08:09:30)
> > On Fri, 10 Jul 2020 at 22:00, Matthew Auld <matthew.auld@intel.com> wrote:
> > >
> > > We need to add support for pwrite'ing an LMEM object.
> >
> > why? DG1 is a discrete GPU, these interfaces we already gross and
> > overly hacky for integrated, I'd prefer not to drag them across into
> > discrete land.
> >
> > same goes for pread.
> >
> > You have no legacy userspace here, userspace needs change to support
> > LMEM, it can be fixed to avoid legacy ioctls paths.
>
> (This answer is really along the same lines as related to the
> relocations, which I sent earlier in the end of this thread)
>
> PREAD/PWRITE are used by IGT tests indirectly in the testing, as
> a means to validate test end results as an example. So IGT
> reworking is needed not to lose testing coverage when the
> functionality is disabled.
>
> Same reasoning as with relocations, as to why this is included
> in the RFC; it will get a functional stack with least changes and
> is virtually no extra effort to carry. It's recognized that once
> pre-Gen12 hardware ceases to exist, there are optimization
> opportunities.

Why do you have to wait until pre-GEN12 hw ceases to exist?

There are clear optimization opportunities since Broadwell, there have
been clear pointless optimisations done to the kernel because
userspace was stuck to the legacy paths way past when it made sense.

I'm pretty sure the media driver is broadwell+ in terms of hw support,
but it's all relocation heavy, like why isn't that the priority of the
kernel team to just go fix it rather than make the kernel long term
harder to maintain. Siloed development is an artifact of *your*
orgchart, and shouldn't leak into the upstream kernel development if
not required.

Would IGT ever have been fixed up? it's test code, it should have been
roadmap and resourced to fix up instead of writing kernel code for
platforms that don't require it. You can valdiate IGT on the older
hardware anyways.

If you need test interfaces in the kernel, please put them under
BROKEN or somewhere that only CI finds them, until you can rework CI
tests, but we need to be a bit more stringent on carrying forward
interfaces that aren't needed by modern HW, or else you will never get
to pre-GEN12 disappearing, like in reality it should in a lot of cases
be pre-GEN8.

Dave.

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_lmem.c b/drivers/gpu/drm/i915/gem/i915_gem_lmem.c
index 9142ba299aa1..e324328639d2 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_lmem.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_lmem.c
@@ -94,6 +94,91 @@  static int lmem_pread(struct drm_i915_gem_object *obj,
 	return ret;
 }
 
+static int lmem_pwrite(struct drm_i915_gem_object *obj,
+		       const struct drm_i915_gem_pwrite *arg)
+{
+	struct drm_i915_private *i915 = to_i915(obj->base.dev);
+	struct intel_runtime_pm *rpm = &i915->runtime_pm;
+	intel_wakeref_t wakeref;
+	struct dma_fence *fence;
+	char __user *user_data;
+	unsigned int offset;
+	unsigned long idx;
+	u64 remain;
+	int ret;
+
+	ret = i915_gem_object_wait(obj,
+				   I915_WAIT_INTERRUPTIBLE,
+				   MAX_SCHEDULE_TIMEOUT);
+	if (ret)
+		return ret;
+
+	ret = i915_gem_object_pin_pages(obj);
+	if (ret)
+		return ret;
+
+	i915_gem_object_lock(obj);
+	ret = i915_gem_object_set_to_wc_domain(obj, true);
+	if (ret) {
+		i915_gem_object_unlock(obj);
+		goto out_unpin;
+	}
+
+	fence = i915_gem_object_lock_fence(obj);
+	i915_gem_object_unlock(obj);
+	if (!fence) {
+		ret = -ENOMEM;
+		goto out_unpin;
+	}
+
+	wakeref = intel_runtime_pm_get(rpm);
+
+	remain = arg->size;
+	user_data = u64_to_user_ptr(arg->data_ptr);
+	offset = offset_in_page(arg->offset);
+	for (idx = arg->offset >> PAGE_SHIFT; remain; idx++) {
+		unsigned long unwritten;
+		void __iomem *vaddr;
+		int length;
+
+		length = remain;
+		if (offset + length > PAGE_SIZE)
+			length = PAGE_SIZE - offset;
+
+		vaddr = i915_gem_object_lmem_io_map_page_atomic(obj, idx);
+		if (!vaddr) {
+			ret = -ENOMEM;
+			goto out_put;
+		}
+
+		unwritten = __copy_from_user_inatomic_nocache((void __force*)vaddr + offset,
+							      user_data, length);
+		io_mapping_unmap_atomic(vaddr);
+		if (unwritten) {
+			vaddr = i915_gem_object_lmem_io_map_page(obj, idx);
+			unwritten = copy_from_user((void __force*)vaddr + offset,
+						   user_data, length);
+			io_mapping_unmap(vaddr);
+		}
+		if (unwritten) {
+			ret = -EFAULT;
+			goto out_put;
+		}
+
+		remain -= length;
+		user_data += length;
+		offset = 0;
+	}
+
+out_put:
+	intel_runtime_pm_put(rpm, wakeref);
+	i915_gem_object_unlock_fence(obj, fence);
+out_unpin:
+	i915_gem_object_unpin_pages(obj);
+
+	return ret;
+}
+
 const struct drm_i915_gem_object_ops i915_gem_lmem_obj_ops = {
 	.name = "i915_gem_object_lmem",
 	.flags = I915_GEM_OBJECT_HAS_IOMEM,
@@ -103,6 +188,7 @@  const struct drm_i915_gem_object_ops i915_gem_lmem_obj_ops = {
 	.release = i915_gem_object_release_memory_region,
 
 	.pread = lmem_pread,
+	.pwrite = lmem_pwrite,
 };
 
 void __iomem *