| Message ID | 20230906095542.3280699-3-sarah.walker@imgtec.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Imagination Technologies PowerVR DRM driver | expand |
Hi Sarah, On Wed, Sep 06, 2023 at 10:55:24AM +0100, Sarah Walker wrote: > From: Donald Robson <donald.robson@imgtec.com> > > Signed-off-by: Donald Robson <donald.robson@imgtec.com> Sorry, this applied to your previous versions too but I only caught it right now. When you submit a patch on someone else's behalf, you need to add your Signed-off-by. That's also true when you're the committer of a patch you didn't write. This one, and patch 1, are affected. Also, generally speaking, it's a good to write a commit log for a patch to at least provide some context on what you want to achieve. Maxime
On Wed, 2023-09-06 at 13:35 +0200, Maxime Ripard wrote: > Hi Sarah, > > On Wed, Sep 06, 2023 at 10:55:24AM +0100, Sarah Walker wrote: > > From: Donald Robson <donald.robson@imgtec.com> > > > > Signed-off-by: Donald Robson <donald.robson@imgtec.com> > > Sorry, this applied to your previous versions too but I only caught it > right now. When you submit a patch on someone else's behalf, you need to > add your Signed-off-by. That's also true when you're the committer of a > patch you didn't write. > > This one, and patch 1, are affected. > > Also, generally speaking, it's a good to write a commit log for a patch > to at least provide some context on what you want to achieve. Sorry for this, will get these issues addressed before the next patchset. Sarah
On Wed, 06 Sep 2023, Sarah Walker <sarah.walker@imgtec.com> wrote: > From: Donald Robson <donald.robson@imgtec.com> > > Signed-off-by: Donald Robson <donald.robson@imgtec.com> > --- > include/drm/drm_gpuva_mgr.h | 27 +++++++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > > diff --git a/include/drm/drm_gpuva_mgr.h b/include/drm/drm_gpuva_mgr.h > index ed8d50200cc3..be7b3a6d7e67 100644 > --- a/include/drm/drm_gpuva_mgr.h > +++ b/include/drm/drm_gpuva_mgr.h > @@ -703,4 +703,31 @@ void drm_gpuva_remap(struct drm_gpuva *prev, > > void drm_gpuva_unmap(struct drm_gpuva_op_unmap *op); > > +/** > + * drm_gpuva_op_remap_get_unmap_range() - Helper to get the start and range of > + * the unmap stage of a remap op. > + * @op: Remap op. > + * @start_addr: Output pointer for the start of the required unmap. > + * @range: Output pointer for the length of the required unmap. > + * > + * These parameters can then be used by the caller to unmap memory pages that > + * are no longer required. > + */ > +static __always_inline void IMO __always_inline *always* requires a justification in the commit message. BR, Jani. > +drm_gpuva_op_remap_get_unmap_range(const struct drm_gpuva_op_remap *op, > + u64 *start_addr, u64 *range) > +{ > + const u64 va_start = op->prev ? > + op->prev->va.addr + op->prev->va.range : > + op->unmap->va->va.addr; > + const u64 va_end = op->next ? > + op->next->va.addr : > + op->unmap->va->va.addr + op->unmap->va->va.range; > + > + if (start_addr) > + *start_addr = va_start; > + if (range) > + *range = va_end - va_start; > +} > + > #endif /* __DRM_GPUVA_MGR_H__ */
On Thu, 2023-09-07 at 15:14 +0300, Jani Nikula wrote: > On Wed, 06 Sep 2023, Sarah Walker <sarah.walker@imgtec.com> wrote: > > From: Donald Robson <donald.robson@imgtec.com> > > > > Signed-off-by: Donald Robson <donald.robson@imgtec.com> > > --- > > include/drm/drm_gpuva_mgr.h | 27 +++++++++++++++++++++++++++ > > 1 file changed, 27 insertions(+) > > > > diff --git a/include/drm/drm_gpuva_mgr.h b/include/drm/drm_gpuva_mgr.h > > index ed8d50200cc3..be7b3a6d7e67 100644 > > --- a/include/drm/drm_gpuva_mgr.h > > +++ b/include/drm/drm_gpuva_mgr.h > > @@ -703,4 +703,31 @@ void drm_gpuva_remap(struct drm_gpuva *prev, > > > > void drm_gpuva_unmap(struct drm_gpuva_op_unmap *op); > > > > +/** > > + * drm_gpuva_op_remap_get_unmap_range() - Helper to get the start and range of > > + * the unmap stage of a remap op. > > + * @op: Remap op. > > + * @start_addr: Output pointer for the start of the required unmap. > > + * @range: Output pointer for the length of the required unmap. > > + * > > + * These parameters can then be used by the caller to unmap memory pages that > > + * are no longer required. > > + */ > > +static __always_inline void > > IMO __always_inline *always* requires a justification in the commit > message. > > BR, > Jani. Hi Jani, I went with __always_inline because I can't see this being used more than once per driver. I can add that to the commit message, but is that suitable justification? I could move it to the source file or make it a macro if you prefer. Thanks, Donald > > > > +drm_gpuva_op_remap_get_unmap_range(const struct drm_gpuva_op_remap *op, > > + u64 *start_addr, u64 *range) > > +{ > > + const u64 va_start = op->prev ? > > + op->prev->va.addr + op->prev->va.range : > > + op->unmap->va->va.addr; > > + const u64 va_end = op->next ? > > + op->next->va.addr : > > + op->unmap->va->va.addr + op->unmap->va->va.range; > > + > > + if (start_addr) > > + *start_addr = va_start; > > + if (range) > > + *range = va_end - va_start; > > +} > > + > > #endif /* __DRM_GPUVA_MGR_H__ */
On Thu, 07 Sep 2023, Donald Robson <Donald.Robson@imgtec.com> wrote: > On Thu, 2023-09-07 at 15:14 +0300, Jani Nikula wrote: >> On Wed, 06 Sep 2023, Sarah Walker <sarah.walker@imgtec.com> wrote: >> > From: Donald Robson <donald.robson@imgtec.com> >> > >> > Signed-off-by: Donald Robson <donald.robson@imgtec.com> >> > --- >> > include/drm/drm_gpuva_mgr.h | 27 +++++++++++++++++++++++++++ >> > 1 file changed, 27 insertions(+) >> > >> > diff --git a/include/drm/drm_gpuva_mgr.h b/include/drm/drm_gpuva_mgr.h >> > index ed8d50200cc3..be7b3a6d7e67 100644 >> > --- a/include/drm/drm_gpuva_mgr.h >> > +++ b/include/drm/drm_gpuva_mgr.h >> > @@ -703,4 +703,31 @@ void drm_gpuva_remap(struct drm_gpuva *prev, >> > >> > void drm_gpuva_unmap(struct drm_gpuva_op_unmap *op); >> > >> > +/** >> > + * drm_gpuva_op_remap_get_unmap_range() - Helper to get the start and range of >> > + * the unmap stage of a remap op. >> > + * @op: Remap op. >> > + * @start_addr: Output pointer for the start of the required unmap. >> > + * @range: Output pointer for the length of the required unmap. >> > + * >> > + * These parameters can then be used by the caller to unmap memory pages that >> > + * are no longer required. >> > + */ >> > +static __always_inline void >> >> IMO __always_inline *always* requires a justification in the commit >> message. >> >> BR, >> Jani. > > Hi Jani, > I went with __always_inline because I can't see this being used more than once per driver. > I can add that to the commit message, but is that suitable justification? I could move > it to the source file or make it a macro if you prefer. My personal opinion is that static inlines in general should always have a performance justification. If there isn't one, it should be a regular function. Static inlines leak the abstractions and often make the header dependencies worse. Not everyone agrees, of course. More than anything I was looking for justification on __always_inline rather than just inline, though. BR, Jani. > Thanks, > Donald >> >> >> > +drm_gpuva_op_remap_get_unmap_range(const struct drm_gpuva_op_remap *op, >> > + u64 *start_addr, u64 *range) >> > +{ >> > + const u64 va_start = op->prev ? >> > + op->prev->va.addr + op->prev->va.range : >> > + op->unmap->va->va.addr; >> > + const u64 va_end = op->next ? >> > + op->next->va.addr : >> > + op->unmap->va->va.addr + op->unmap->va->va.range; >> > + >> > + if (start_addr) >> > + *start_addr = va_start; >> > + if (range) >> > + *range = va_end - va_start; >> > +} >> > + >> > #endif /* __DRM_GPUVA_MGR_H__ */
diff --git a/include/drm/drm_gpuva_mgr.h b/include/drm/drm_gpuva_mgr.h index ed8d50200cc3..be7b3a6d7e67 100644 --- a/include/drm/drm_gpuva_mgr.h +++ b/include/drm/drm_gpuva_mgr.h @@ -703,4 +703,31 @@ void drm_gpuva_remap(struct drm_gpuva *prev, void drm_gpuva_unmap(struct drm_gpuva_op_unmap *op); +/** + * drm_gpuva_op_remap_get_unmap_range() - Helper to get the start and range of + * the unmap stage of a remap op. + * @op: Remap op. + * @start_addr: Output pointer for the start of the required unmap. + * @range: Output pointer for the length of the required unmap. + * + * These parameters can then be used by the caller to unmap memory pages that + * are no longer required. + */ +static __always_inline void +drm_gpuva_op_remap_get_unmap_range(const struct drm_gpuva_op_remap *op, + u64 *start_addr, u64 *range) +{ + const u64 va_start = op->prev ? + op->prev->va.addr + op->prev->va.range : + op->unmap->va->va.addr; + const u64 va_end = op->next ? + op->next->va.addr : + op->unmap->va->va.addr + op->unmap->va->va.range; + + if (start_addr) + *start_addr = va_start; + if (range) + *range = va_end - va_start; +} + #endif /* __DRM_GPUVA_MGR_H__ */