[Intel-gfx] [PATCH 1/5] dma-buf: add dynamic DMA-buf handling v14

[Daniel Vetter]

On Tue, Feb 18, 2020 at 2:20 PM Christian König
<ckoenig.leichtzumerken at gmail.com> wrote:
>
> Am 05.11.19 um 11:20 schrieb Daniel Vetter:
> > On Tue, Oct 29, 2019 at 11:40:45AM +0100, Christian König wrote:
> > [SNIP]
> >> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> >> index d377b4ca66bf..ce293cee76ed 100644
> >> --- a/drivers/dma-buf/dma-buf.c
> >> +++ b/drivers/dma-buf/dma-buf.c
> >> @@ -529,6 +529,10 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info)
> >>                  exp_info->ops->dynamic_mapping))
> >>              return ERR_PTR(-EINVAL);
> >>
> >> +    if (WARN_ON(!exp_info->ops->dynamic_mapping &&
> >> +                (exp_info->ops->pin || exp_info->ops->unpin)))
> >> +            return ERR_PTR(-EINVAL);
> > Imo make this stronger, have a dynamic mapping iff there's both a pin and
> > unpin function. Otherwise this doesn't make a lot of sense to me.
>
> I want to avoid that for the initial implementation. So far dynamic only
> meant that we have the new locking semantics.
>
> We could make that mandatory after this patch set when amdgpu is
> migrated and has implemented the necessary callbacks.

Ok if we go with CONFIG_EXPERIMENTAL_DYN_DMABUF or whatever it's going
to be called I'm totally ok if we just note this somewhere as a FIXME
(maybe just inline in a code comment next to the main #ifdef in
dma-buf.h. Same for all your other comments below.

Cheers, Daniel

>
> >> [SNIP]
> >> @@ -821,13 +877,23 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
> >>              return attach->sgt;
> >>      }
> >>
> >> -    if (dma_buf_is_dynamic(attach->dmabuf))
> >> +    if (dma_buf_is_dynamic(attach->dmabuf)) {
> >>              dma_resv_assert_held(attach->dmabuf->resv);
> >> +            if (!attach->importer_ops->move_notify) {
> > Imo just require ->move_notify for importers that give you an ops
> > function. Doesn't really make sense to allow dynamic without support
> > ->move_notify.
>
> Same thing here. We could make that mandatory and clean it up after
> migrating amdgpu.
>
> >> [SNIP]
> >> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
> >> index af73f835c51c..7456bb937635 100644
> >> --- a/include/linux/dma-buf.h
> >> +++ b/include/linux/dma-buf.h
> >> @@ -93,14 +93,40 @@ struct dma_buf_ops {
> >>       */
> >>      void (*detach)(struct dma_buf *, struct dma_buf_attachment *);
> >>
> >> +    /**
> >> +     * @pin:
> >> +     *
> >> +     * This is called by dma_buf_pin and lets the exporter know that the
> >> +     * DMA-buf can't be moved any more.
> > I think we should add a warning here that pinning is only ok for limited
> > use-cases (like scanout or similar), and not as part of general buffer
> > management.
> >
> > i915 uses temporary pins through it's execbuf management (and everywhere
> > else), so we have a _lot_ of people in dri-devel with quite different
> > ideas of what this might be for :-)
>
> Yeah, that is also a good idea for us. Wrote a one liner, but you might
> want to double check the wording.
>
> >> [SNIP]
> >> @@ -141,9 +167,6 @@ struct dma_buf_ops {
> >>       *
> >>       * This is called by dma_buf_unmap_attachment() and should unmap and
> >>       * release the &sg_table allocated in @map_dma_buf, and it is mandatory.
> >> -     * It should also unpin the backing storage if this is the last mapping
> >> -     * of the DMA buffer, it the exporter supports backing storage
> >> -     * migration.
> > This is still valid for non-dynamic exporters. Imo keep but clarify that.
>
> OK, changed.
>
> >> [SNIP]
> >> @@ -438,16 +491,19 @@ static inline bool dma_buf_is_dynamic(struct dma_buf *dmabuf)
> >>   static inline bool
> >>   dma_buf_attachment_is_dynamic(struct dma_buf_attachment *attach)
> >>   {
> >> -    return attach->dynamic_mapping;
> >> +    return !!attach->importer_ops;
> > Hm why not do the same for exporters, and make them dynamic iff they have
> > pin/unpin?
>
> Same thing as before, to migrate amdgpu to the new interface first and
> then make it mandatory.
>
> I think I will just write a cleanup patch into the series which comes
> after the amdgpu changes.
>
> Thanks,
> Christian.



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch