| Message ID | 20190426123638.40221-2-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/12] dma-buf: add struct dma_buf_attach_info | expand |
On Fri, Apr 26, 2019 at 02:36:28PM +0200, Christian König wrote: > Add optional explicit pinning callbacks instead of implicitly assume the > exporter pins the buffer when a mapping is created. > > v2: move in patchset and pin the dma-buf in the old mapping code paths. > > Signed-off-by: Christian König <christian.koenig@amd.com> > --- > drivers/dma-buf/dma-buf.c | 49 ++++++++++++++++++++++++++++++++++++++- > include/linux/dma-buf.h | 38 +++++++++++++++++++++++++----- > 2 files changed, 80 insertions(+), 7 deletions(-) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 50b4c6af04c7..0656dcf289be 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -529,6 +529,41 @@ void dma_buf_put(struct dma_buf *dmabuf) > } > EXPORT_SYMBOL_GPL(dma_buf_put); > > +/** > + * dma_buf_pin - Lock down the DMA-buf > + * > + * @dmabuf: [in] DMA-buf to lock down. > + * > + * Returns: > + * 0 on success, negative error code on failure. > + */ > +int dma_buf_pin(struct dma_buf *dmabuf) I think this should be on the attachment, not on the buffer. Or is the idea that a pin is for the entire buffer, and all subsequent dma_buf_map_attachment must work for all attachments? I think this matters for sufficiently contrived p2p scenarios. Either way, docs need to clarify this. > +{ > + int ret = 0; > + > + reservation_object_assert_held(dmabuf->resv); > + > + if (dmabuf->ops->pin) > + ret = dmabuf->ops->pin(dmabuf); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(dma_buf_pin); > + > +/** > + * dma_buf_unpin - Remove lock from DMA-buf > + * > + * @dmabuf: [in] DMA-buf to unlock. > + */ > +void dma_buf_unpin(struct dma_buf *dmabuf) > +{ > + reservation_object_assert_held(dmabuf->resv); > + > + if (dmabuf->ops->unpin) > + dmabuf->ops->unpin(dmabuf); > +} > +EXPORT_SYMBOL_GPL(dma_buf_unpin); > + > /** > * dma_buf_attach - Add the device to dma_buf's attachments list; optionally, > * calls attach() of dma_buf_ops to allow device-specific attach functionality > @@ -548,7 +583,8 @@ EXPORT_SYMBOL_GPL(dma_buf_put); > * accessible to @dev, and cannot be moved to a more suitable place. This is > * indicated with the error code -EBUSY. > */ > -struct dma_buf_attachment *dma_buf_attach(const struct dma_buf_attach_info *info) > +struct dma_buf_attachment * > +dma_buf_attach(const struct dma_buf_attach_info *info) > { > struct dma_buf *dmabuf = info->dmabuf; > struct dma_buf_attachment *attach; > @@ -625,12 +661,19 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, > enum dma_data_direction direction) > { > struct sg_table *sg_table; > + int r; > > might_sleep(); > > if (WARN_ON(!attach || !attach->dmabuf)) > return ERR_PTR(-EINVAL); > > + reservation_object_lock(attach->dmabuf->resv, NULL); > + r = dma_buf_pin(attach->dmabuf); > + reservation_object_unlock(attach->dmabuf->resv); This adds an unconditional reservat lock to map/unmap, which is think pisses off drivers. This gets fixed later on with the caching, but means the series is broken here. Also, that super-fine grained split-up makes it harder for me to review the docs, since only until the very end are all the bits present for full dynamic dma-buf mappings. I think it'd be best to squash all the patches from pin up to the one that adds the invalidate callback into one patch. It's all changes to dma-buf.[hc] only anyway. If that is too big we can think about how to split it up again, but at least for me the current splitting doesn't make sense at all. > + if (r) > + return ERR_PTR(r); > + > sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction); > if (!sg_table) > sg_table = ERR_PTR(-ENOMEM); Leaks the pin if we fail here. > @@ -660,6 +703,10 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach, > > attach->dmabuf->ops->unmap_dma_buf(attach, sg_table, > direction); > + > + reservation_object_lock(attach->dmabuf->resv, NULL); > + dma_buf_unpin(attach->dmabuf); > + reservation_object_unlock(attach->dmabuf->resv); > } > EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment); > > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h > index 2c312dfd31a1..0321939b1c3d 100644 > --- a/include/linux/dma-buf.h > +++ b/include/linux/dma-buf.h > @@ -51,6 +51,34 @@ struct dma_buf_attachment; > * @vunmap: [optional] unmaps a vmap from the buffer > */ > struct dma_buf_ops { > + /** > + * @pin: > + * > + * This is called by dma_buf_pin and lets the exporter know that the > + * DMA-buf can't be moved any more. > + * > + * This is called with the dmabuf->resv object locked. > + * > + * This callback is optional. > + * > + * Returns: > + * > + * 0 on success, negative error code on failure. > + */ > + int (*pin)(struct dma_buf *); > + > + /** > + * @unpin: > + * > + * This is called by dma_buf_unpin and lets the exporter know that the > + * DMA-buf can be moved again. > + * > + * This is called with the dmabuf->resv object locked. > + * > + * This callback is optional. "This callback is optional, but must be provided if @pin is." Same for @pin I think, plus would be good to check in dma_buf_export that you have both or neither with WARN_ON(!!exp_info->ops->pin == !!exp_info->ops->unpin); > + */ > + void (*unpin)(struct dma_buf *); > + > /** > * @attach: > * > @@ -95,9 +123,7 @@ struct dma_buf_ops { > * > * This is called by dma_buf_map_attachment() and is used to map a > * shared &dma_buf into device address space, and it is mandatory. It > - * can only be called if @attach has been called successfully. This > - * essentially pins the DMA buffer into place, and it cannot be moved > - * any more > + * can only be called if @attach has been called successfully. I think dropping this outright isn't correct, since for all current dma-buf exporters it's still what they should be doing. We just need to make this conditional on @pin and @unpin not being present: "If @pin and @unpin are not provided this essentially pins the DMA buffer into place, and it cannot be moved any more." > * > * This call may sleep, e.g. when the backing storage first needs to be > * allocated, or moved to a location suitable for all currently attached > @@ -135,9 +161,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. Same here, add a "If @pin and @unpin are not provided this should ..." qualifier instead of deleting. Cheers, Daniel > - * It should also unpin the backing storage if this is the last mapping > - * of the DMA buffer, it the exporter supports backing storage > - * migration. > */ > void (*unmap_dma_buf)(struct dma_buf_attachment *, > struct sg_table *, > @@ -386,6 +409,9 @@ static inline void get_dma_buf(struct dma_buf *dmabuf) > get_file(dmabuf->file); > } > > +int dma_buf_pin(struct dma_buf *dmabuf); > +void dma_buf_unpin(struct dma_buf *dmabuf); > + > struct dma_buf_attachment * > dma_buf_attach(const struct dma_buf_attach_info *info); > void dma_buf_detach(struct dma_buf *dmabuf, > -- > 2.17.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Am 29.04.19 um 10:40 schrieb Daniel Vetter: > On Fri, Apr 26, 2019 at 02:36:28PM +0200, Christian König wrote: >> Add optional explicit pinning callbacks instead of implicitly assume the >> exporter pins the buffer when a mapping is created. >> >> v2: move in patchset and pin the dma-buf in the old mapping code paths. >> >> Signed-off-by: Christian König <christian.koenig@amd.com> >> --- >> drivers/dma-buf/dma-buf.c | 49 ++++++++++++++++++++++++++++++++++++++- >> include/linux/dma-buf.h | 38 +++++++++++++++++++++++++----- >> 2 files changed, 80 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c >> index 50b4c6af04c7..0656dcf289be 100644 >> --- a/drivers/dma-buf/dma-buf.c >> +++ b/drivers/dma-buf/dma-buf.c >> @@ -529,6 +529,41 @@ void dma_buf_put(struct dma_buf *dmabuf) >> } >> EXPORT_SYMBOL_GPL(dma_buf_put); >> >> +/** >> + * dma_buf_pin - Lock down the DMA-buf >> + * >> + * @dmabuf: [in] DMA-buf to lock down. >> + * >> + * Returns: >> + * 0 on success, negative error code on failure. >> + */ >> +int dma_buf_pin(struct dma_buf *dmabuf) > I think this should be on the attachment, not on the buffer. Or is the > idea that a pin is for the entire buffer, and all subsequent > dma_buf_map_attachment must work for all attachments? I think this matters > for sufficiently contrived p2p scenarios. This is indeed for the DMA-buf, cause we are pinning the underlying backing store and not just one attachment. Key point is I want to call this in the exporter itself in the long run. E.g. that the exporter stops working with its internal special handling functions and uses this one instead. > Either way, docs need to clarify this. Going to add a bit more here. > >> +{ >> + int ret = 0; >> + >> + reservation_object_assert_held(dmabuf->resv); >> + >> + if (dmabuf->ops->pin) >> + ret = dmabuf->ops->pin(dmabuf); >> + >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(dma_buf_pin); >> + >> +/** >> + * dma_buf_unpin - Remove lock from DMA-buf >> + * >> + * @dmabuf: [in] DMA-buf to unlock. >> + */ >> +void dma_buf_unpin(struct dma_buf *dmabuf) >> +{ >> + reservation_object_assert_held(dmabuf->resv); >> + >> + if (dmabuf->ops->unpin) >> + dmabuf->ops->unpin(dmabuf); >> +} >> +EXPORT_SYMBOL_GPL(dma_buf_unpin); >> + >> /** >> * dma_buf_attach - Add the device to dma_buf's attachments list; optionally, >> * calls attach() of dma_buf_ops to allow device-specific attach functionality >> @@ -548,7 +583,8 @@ EXPORT_SYMBOL_GPL(dma_buf_put); >> * accessible to @dev, and cannot be moved to a more suitable place. This is >> * indicated with the error code -EBUSY. >> */ >> -struct dma_buf_attachment *dma_buf_attach(const struct dma_buf_attach_info *info) >> +struct dma_buf_attachment * >> +dma_buf_attach(const struct dma_buf_attach_info *info) >> { >> struct dma_buf *dmabuf = info->dmabuf; >> struct dma_buf_attachment *attach; >> @@ -625,12 +661,19 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, >> enum dma_data_direction direction) >> { >> struct sg_table *sg_table; >> + int r; >> >> might_sleep(); >> >> if (WARN_ON(!attach || !attach->dmabuf)) >> return ERR_PTR(-EINVAL); >> >> + reservation_object_lock(attach->dmabuf->resv, NULL); >> + r = dma_buf_pin(attach->dmabuf); >> + reservation_object_unlock(attach->dmabuf->resv); > This adds an unconditional reservat lock to map/unmap, which is think > pisses off drivers. This gets fixed later on with the caching, but means > the series is broken here. > > Also, that super-fine grained split-up makes it harder for me to review > the docs, since only until the very end are all the bits present for full > dynamic dma-buf mappings. > > I think it'd be best to squash all the patches from pin up to the one that > adds the invalidate callback into one patch. It's all changes to > dma-buf.[hc] only anyway. If that is too big we can think about how to > split it up again, but at least for me the current splitting doesn't make > sense at all. Fine with me, if you think that is easier to review. It just gave me a big headache to add all that logic at the same time. > >> + if (r) >> + return ERR_PTR(r); >> + >> sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction); >> if (!sg_table) >> sg_table = ERR_PTR(-ENOMEM); > Leaks the pin if we fail here. Going to fix that. > >> @@ -660,6 +703,10 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach, >> >> attach->dmabuf->ops->unmap_dma_buf(attach, sg_table, >> direction); >> + >> + reservation_object_lock(attach->dmabuf->resv, NULL); >> + dma_buf_unpin(attach->dmabuf); >> + reservation_object_unlock(attach->dmabuf->resv); >> } >> EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment); >> >> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h >> index 2c312dfd31a1..0321939b1c3d 100644 >> --- a/include/linux/dma-buf.h >> +++ b/include/linux/dma-buf.h >> @@ -51,6 +51,34 @@ struct dma_buf_attachment; >> * @vunmap: [optional] unmaps a vmap from the buffer >> */ >> struct dma_buf_ops { >> + /** >> + * @pin: >> + * >> + * This is called by dma_buf_pin and lets the exporter know that the >> + * DMA-buf can't be moved any more. >> + * >> + * This is called with the dmabuf->resv object locked. >> + * >> + * This callback is optional. >> + * >> + * Returns: >> + * >> + * 0 on success, negative error code on failure. >> + */ >> + int (*pin)(struct dma_buf *); >> + >> + /** >> + * @unpin: >> + * >> + * This is called by dma_buf_unpin and lets the exporter know that the >> + * DMA-buf can be moved again. >> + * >> + * This is called with the dmabuf->resv object locked. >> + * >> + * This callback is optional. > "This callback is optional, but must be provided if @pin is." Same for > @pin I think, plus would be good to check in dma_buf_export that you have > both or neither with > > WARN_ON(!!exp_info->ops->pin == !!exp_info->ops->unpin); > >> + */ >> + void (*unpin)(struct dma_buf *); >> + >> /** >> * @attach: >> * >> @@ -95,9 +123,7 @@ struct dma_buf_ops { >> * >> * This is called by dma_buf_map_attachment() and is used to map a >> * shared &dma_buf into device address space, and it is mandatory. It >> - * can only be called if @attach has been called successfully. This >> - * essentially pins the DMA buffer into place, and it cannot be moved >> - * any more >> + * can only be called if @attach has been called successfully. > I think dropping this outright isn't correct, since for all current > dma-buf exporters it's still what they should be doing. We just need to > make this conditional on @pin and @unpin not being present: > > "If @pin and @unpin are not provided this essentially pins the DMA > buffer into place, and it cannot be moved any more." > >> * >> * This call may sleep, e.g. when the backing storage first needs to be >> * allocated, or moved to a location suitable for all currently attached >> @@ -135,9 +161,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. > Same here, add a "If @pin and @unpin are not provided this should ..." > qualifier instead of deleting. > > Cheers, Daniel > > >> - * It should also unpin the backing storage if this is the last mapping >> - * of the DMA buffer, it the exporter supports backing storage >> - * migration. >> */ >> void (*unmap_dma_buf)(struct dma_buf_attachment *, >> struct sg_table *, >> @@ -386,6 +409,9 @@ static inline void get_dma_buf(struct dma_buf *dmabuf) >> get_file(dmabuf->file); >> } >> >> +int dma_buf_pin(struct dma_buf *dmabuf); >> +void dma_buf_unpin(struct dma_buf *dmabuf); >> + >> struct dma_buf_attachment * >> dma_buf_attach(const struct dma_buf_attach_info *info); >> void dma_buf_detach(struct dma_buf *dmabuf, >> -- >> 2.17.1 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Apr 30, 2019 at 03:42:22PM +0200, Christian König wrote: > Am 29.04.19 um 10:40 schrieb Daniel Vetter: > > On Fri, Apr 26, 2019 at 02:36:28PM +0200, Christian König wrote: > > > Add optional explicit pinning callbacks instead of implicitly assume the > > > exporter pins the buffer when a mapping is created. > > > > > > v2: move in patchset and pin the dma-buf in the old mapping code paths. > > > > > > Signed-off-by: Christian König <christian.koenig@amd.com> > > > --- > > > drivers/dma-buf/dma-buf.c | 49 ++++++++++++++++++++++++++++++++++++++- > > > include/linux/dma-buf.h | 38 +++++++++++++++++++++++++----- > > > 2 files changed, 80 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > > > index 50b4c6af04c7..0656dcf289be 100644 > > > --- a/drivers/dma-buf/dma-buf.c > > > +++ b/drivers/dma-buf/dma-buf.c > > > @@ -529,6 +529,41 @@ void dma_buf_put(struct dma_buf *dmabuf) > > > } > > > EXPORT_SYMBOL_GPL(dma_buf_put); > > > +/** > > > + * dma_buf_pin - Lock down the DMA-buf > > > + * > > > + * @dmabuf: [in] DMA-buf to lock down. > > > + * > > > + * Returns: > > > + * 0 on success, negative error code on failure. > > > + */ > > > +int dma_buf_pin(struct dma_buf *dmabuf) > > I think this should be on the attachment, not on the buffer. Or is the > > idea that a pin is for the entire buffer, and all subsequent > > dma_buf_map_attachment must work for all attachments? I think this matters > > for sufficiently contrived p2p scenarios. > > This is indeed for the DMA-buf, cause we are pinning the underlying backing > store and not just one attachment. You can't move the buffer either way, so from that point there's no difference. I more meant from an account/api point of view of whether it's ok to pin a buffer you haven't even attached to yet. From the code it seems like first you always want to attach, hence it would make sense to put the pin/unpin onto the attachment. That might also help with debugging, we could check whether everyone balances their pin/unpin correctly (instead of just counting for the overall dma-buf). There's also a slight semantic difference between a pin on an attachment (which would mean this attachment is always going to be mappable and wont move around), whereas a pin on the entire dma-buf means the entire dma-buf and all it's attachment must always be mappable. Otoh, global pin is probably easier, you just need to check all current attachments again whether they still work or whether you might need to move the buffer around to a more suitable place (e.g. if you not all can do p2p it needs to go into system ram before it's pinned). For the backing storage you only want one per-bo ->pinned_count, that's clear, my suggestion was more about whether having more information about who's pinning is useful. Exporters can always just ignore that added information. > Key point is I want to call this in the exporter itself in the long run. > E.g. that the exporter stops working with its internal special handling > functions and uses this one instead. Hm, not exactly following why the exporter needs to call dma_buf_pin/unpin, instead of whatever is used to implement that. Or do you mean that you want a ->pinned_count in dma_buf itself, so that there's only one book-keeping for this? > > Either way, docs need to clarify this. > > Going to add a bit more here. > > > > > > +{ > > > + int ret = 0; > > > + > > > + reservation_object_assert_held(dmabuf->resv); > > > + > > > + if (dmabuf->ops->pin) > > > + ret = dmabuf->ops->pin(dmabuf); > > > + > > > + return ret; > > > +} > > > +EXPORT_SYMBOL_GPL(dma_buf_pin); > > > + > > > +/** > > > + * dma_buf_unpin - Remove lock from DMA-buf > > > + * > > > + * @dmabuf: [in] DMA-buf to unlock. > > > + */ > > > +void dma_buf_unpin(struct dma_buf *dmabuf) > > > +{ > > > + reservation_object_assert_held(dmabuf->resv); > > > + > > > + if (dmabuf->ops->unpin) > > > + dmabuf->ops->unpin(dmabuf); > > > +} > > > +EXPORT_SYMBOL_GPL(dma_buf_unpin); > > > + > > > /** > > > * dma_buf_attach - Add the device to dma_buf's attachments list; optionally, > > > * calls attach() of dma_buf_ops to allow device-specific attach functionality > > > @@ -548,7 +583,8 @@ EXPORT_SYMBOL_GPL(dma_buf_put); > > > * accessible to @dev, and cannot be moved to a more suitable place. This is > > > * indicated with the error code -EBUSY. > > > */ > > > -struct dma_buf_attachment *dma_buf_attach(const struct dma_buf_attach_info *info) > > > +struct dma_buf_attachment * > > > +dma_buf_attach(const struct dma_buf_attach_info *info) > > > { > > > struct dma_buf *dmabuf = info->dmabuf; > > > struct dma_buf_attachment *attach; > > > @@ -625,12 +661,19 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, > > > enum dma_data_direction direction) > > > { > > > struct sg_table *sg_table; > > > + int r; > > > might_sleep(); > > > if (WARN_ON(!attach || !attach->dmabuf)) > > > return ERR_PTR(-EINVAL); > > > + reservation_object_lock(attach->dmabuf->resv, NULL); > > > + r = dma_buf_pin(attach->dmabuf); > > > + reservation_object_unlock(attach->dmabuf->resv); > > This adds an unconditional reservat lock to map/unmap, which is think > > pisses off drivers. This gets fixed later on with the caching, but means > > the series is broken here. > > > > Also, that super-fine grained split-up makes it harder for me to review > > the docs, since only until the very end are all the bits present for full > > dynamic dma-buf mappings. > > > > I think it'd be best to squash all the patches from pin up to the one that > > adds the invalidate callback into one patch. It's all changes to > > dma-buf.[hc] only anyway. If that is too big we can think about how to > > split it up again, but at least for me the current splitting doesn't make > > sense at all. > > Fine with me, if you think that is easier to review. It just gave me a big > headache to add all that logic at the same time. I think the big headache is unavoidable, since it's all linked very closely together. Hence why I think this is easier to review, defacto it's dma-buf v2. Treating it as a clean-slate thing (but with backwards compat) instead of as an evolution feels better (after the first few attempts of a split up series). Cheers, Daniel > > > > > > + if (r) > > > + return ERR_PTR(r); > > > + > > > sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction); > > > if (!sg_table) > > > sg_table = ERR_PTR(-ENOMEM); > > Leaks the pin if we fail here. > > Going to fix that. > > > > > > @@ -660,6 +703,10 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach, > > > attach->dmabuf->ops->unmap_dma_buf(attach, sg_table, > > > direction); > > > + > > > + reservation_object_lock(attach->dmabuf->resv, NULL); > > > + dma_buf_unpin(attach->dmabuf); > > > + reservation_object_unlock(attach->dmabuf->resv); > > > } > > > EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment); > > > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h > > > index 2c312dfd31a1..0321939b1c3d 100644 > > > --- a/include/linux/dma-buf.h > > > +++ b/include/linux/dma-buf.h > > > @@ -51,6 +51,34 @@ struct dma_buf_attachment; > > > * @vunmap: [optional] unmaps a vmap from the buffer > > > */ > > > struct dma_buf_ops { > > > + /** > > > + * @pin: > > > + * > > > + * This is called by dma_buf_pin and lets the exporter know that the > > > + * DMA-buf can't be moved any more. > > > + * > > > + * This is called with the dmabuf->resv object locked. > > > + * > > > + * This callback is optional. > > > + * > > > + * Returns: > > > + * > > > + * 0 on success, negative error code on failure. > > > + */ > > > + int (*pin)(struct dma_buf *); > > > + > > > + /** > > > + * @unpin: > > > + * > > > + * This is called by dma_buf_unpin and lets the exporter know that the > > > + * DMA-buf can be moved again. > > > + * > > > + * This is called with the dmabuf->resv object locked. > > > + * > > > + * This callback is optional. > > "This callback is optional, but must be provided if @pin is." Same for > > @pin I think, plus would be good to check in dma_buf_export that you have > > both or neither with > > > > WARN_ON(!!exp_info->ops->pin == !!exp_info->ops->unpin); > > > > > + */ > > > + void (*unpin)(struct dma_buf *); > > > + > > > /** > > > * @attach: > > > * > > > @@ -95,9 +123,7 @@ struct dma_buf_ops { > > > * > > > * This is called by dma_buf_map_attachment() and is used to map a > > > * shared &dma_buf into device address space, and it is mandatory. It > > > - * can only be called if @attach has been called successfully. This > > > - * essentially pins the DMA buffer into place, and it cannot be moved > > > - * any more > > > + * can only be called if @attach has been called successfully. > > I think dropping this outright isn't correct, since for all current > > dma-buf exporters it's still what they should be doing. We just need to > > make this conditional on @pin and @unpin not being present: > > > > "If @pin and @unpin are not provided this essentially pins the DMA > > buffer into place, and it cannot be moved any more." > > > > > * > > > * This call may sleep, e.g. when the backing storage first needs to be > > > * allocated, or moved to a location suitable for all currently attached > > > @@ -135,9 +161,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. > > Same here, add a "If @pin and @unpin are not provided this should ..." > > qualifier instead of deleting. > > > > Cheers, Daniel > > > > > > > - * It should also unpin the backing storage if this is the last mapping > > > - * of the DMA buffer, it the exporter supports backing storage > > > - * migration. > > > */ > > > void (*unmap_dma_buf)(struct dma_buf_attachment *, > > > struct sg_table *, > > > @@ -386,6 +409,9 @@ static inline void get_dma_buf(struct dma_buf *dmabuf) > > > get_file(dmabuf->file); > > > } > > > +int dma_buf_pin(struct dma_buf *dmabuf); > > > +void dma_buf_unpin(struct dma_buf *dmabuf); > > > + > > > struct dma_buf_attachment * > > > dma_buf_attach(const struct dma_buf_attach_info *info); > > > void dma_buf_detach(struct dma_buf *dmabuf, > > > -- > > > 2.17.1 > > > > > > _______________________________________________ > > > dri-devel mailing list > > > dri-devel@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel >
Am 30.04.19 um 15:59 schrieb Daniel Vetter: > On Tue, Apr 30, 2019 at 03:42:22PM +0200, Christian König wrote: >> Am 29.04.19 um 10:40 schrieb Daniel Vetter: >>> On Fri, Apr 26, 2019 at 02:36:28PM +0200, Christian König wrote: >>>> [SNIP] >>>> +/** >>>> + * dma_buf_pin - Lock down the DMA-buf >>>> + * >>>> + * @dmabuf: [in] DMA-buf to lock down. >>>> + * >>>> + * Returns: >>>> + * 0 on success, negative error code on failure. >>>> + */ >>>> +int dma_buf_pin(struct dma_buf *dmabuf) >>> I think this should be on the attachment, not on the buffer. Or is the >>> idea that a pin is for the entire buffer, and all subsequent >>> dma_buf_map_attachment must work for all attachments? I think this matters >>> for sufficiently contrived p2p scenarios. >> This is indeed for the DMA-buf, cause we are pinning the underlying backing >> store and not just one attachment. > You can't move the buffer either way, so from that point there's no > difference. I more meant from an account/api point of view of whether it's > ok to pin a buffer you haven't even attached to yet. From the code it > seems like first you always want to attach, hence it would make sense to > put the pin/unpin onto the attachment. That might also help with > debugging, we could check whether everyone balances their pin/unpin > correctly (instead of just counting for the overall dma-buf). > > There's also a slight semantic difference between a pin on an attachment > (which would mean this attachment is always going to be mappable and wont > move around), whereas a pin on the entire dma-buf means the entire dma-buf > and all it's attachment must always be mappable. Otoh, global pin is > probably easier, you just need to check all current attachments again > whether they still work or whether you might need to move the buffer > around to a more suitable place (e.g. if you not all can do p2p it needs > to go into system ram before it's pinned). > > For the backing storage you only want one per-bo ->pinned_count, that's > clear, my suggestion was more about whether having more information about > who's pinning is useful. Exporters can always just ignore that added > information. > >> Key point is I want to call this in the exporter itself in the long run. >> E.g. that the exporter stops working with its internal special handling >> functions and uses this one instead. > Hm, not exactly following why the exporter needs to call > dma_buf_pin/unpin, instead of whatever is used to implement that. Or do > you mean that you want a ->pinned_count in dma_buf itself, so that there's > only one book-keeping for this? Yes, exactly that is one of the final goals of this. We could of course use the attachment here, but that would disqualify the exporter calling this directly without an attachment. Regards, Christian.
On Tue, Apr 30, 2019 at 04:26:32PM +0200, Christian König wrote: > Am 30.04.19 um 15:59 schrieb Daniel Vetter: > > On Tue, Apr 30, 2019 at 03:42:22PM +0200, Christian König wrote: > > > Am 29.04.19 um 10:40 schrieb Daniel Vetter: > > > > On Fri, Apr 26, 2019 at 02:36:28PM +0200, Christian König wrote: > > > > > [SNIP] > > > > > +/** > > > > > + * dma_buf_pin - Lock down the DMA-buf > > > > > + * > > > > > + * @dmabuf: [in] DMA-buf to lock down. > > > > > + * > > > > > + * Returns: > > > > > + * 0 on success, negative error code on failure. > > > > > + */ > > > > > +int dma_buf_pin(struct dma_buf *dmabuf) > > > > I think this should be on the attachment, not on the buffer. Or is the > > > > idea that a pin is for the entire buffer, and all subsequent > > > > dma_buf_map_attachment must work for all attachments? I think this matters > > > > for sufficiently contrived p2p scenarios. > > > This is indeed for the DMA-buf, cause we are pinning the underlying backing > > > store and not just one attachment. > > You can't move the buffer either way, so from that point there's no > > difference. I more meant from an account/api point of view of whether it's > > ok to pin a buffer you haven't even attached to yet. From the code it > > seems like first you always want to attach, hence it would make sense to > > put the pin/unpin onto the attachment. That might also help with > > debugging, we could check whether everyone balances their pin/unpin > > correctly (instead of just counting for the overall dma-buf). > > > > There's also a slight semantic difference between a pin on an attachment > > (which would mean this attachment is always going to be mappable and wont > > move around), whereas a pin on the entire dma-buf means the entire dma-buf > > and all it's attachment must always be mappable. Otoh, global pin is > > probably easier, you just need to check all current attachments again > > whether they still work or whether you might need to move the buffer > > around to a more suitable place (e.g. if you not all can do p2p it needs > > to go into system ram before it's pinned). > > > > For the backing storage you only want one per-bo ->pinned_count, that's > > clear, my suggestion was more about whether having more information about > > who's pinning is useful. Exporters can always just ignore that added > > information. > > > > > Key point is I want to call this in the exporter itself in the long run. > > > E.g. that the exporter stops working with its internal special handling > > > functions and uses this one instead. > > Hm, not exactly following why the exporter needs to call > > dma_buf_pin/unpin, instead of whatever is used to implement that. Or do > > you mean that you want a ->pinned_count in dma_buf itself, so that there's > > only one book-keeping for this? > > Yes, exactly that is one of the final goals of this. > > We could of course use the attachment here, but that would disqualify the > exporter calling this directly without an attachment. Hm exporter calling dma_buf_pin/unpin sounds like one seriously inverted lasagna :-) I do understand the goal, all these imported/exporter special cases in code are a bit silly, but I think the proper fix would be if you just always import a buffer, even the private ones, allocated against some exporter-only thing. Then it's always the same function to call. But I'm not sure this is a good reasons to guide the dma-buf interfaces for everyone else. Moving pin to the attachment sounds like a better idea (if the above is the only reason to keep it on the dma-buf). -Daniel
Am 30.04.19 um 16:34 schrieb Daniel Vetter: > [CAUTION: External Email] > > On Tue, Apr 30, 2019 at 04:26:32PM +0200, Christian König wrote: >> Am 30.04.19 um 15:59 schrieb Daniel Vetter: >>> On Tue, Apr 30, 2019 at 03:42:22PM +0200, Christian König wrote: >>>> Am 29.04.19 um 10:40 schrieb Daniel Vetter: >>>>> On Fri, Apr 26, 2019 at 02:36:28PM +0200, Christian König wrote: >>>>>> [SNIP] >>>>>> +/** >>>>>> + * dma_buf_pin - Lock down the DMA-buf >>>>>> + * >>>>>> + * @dmabuf: [in] DMA-buf to lock down. >>>>>> + * >>>>>> + * Returns: >>>>>> + * 0 on success, negative error code on failure. >>>>>> + */ >>>>>> +int dma_buf_pin(struct dma_buf *dmabuf) >>>>> I think this should be on the attachment, not on the buffer. Or is the >>>>> idea that a pin is for the entire buffer, and all subsequent >>>>> dma_buf_map_attachment must work for all attachments? I think this matters >>>>> for sufficiently contrived p2p scenarios. >>>> This is indeed for the DMA-buf, cause we are pinning the underlying backing >>>> store and not just one attachment. >>> You can't move the buffer either way, so from that point there's no >>> difference. I more meant from an account/api point of view of whether it's >>> ok to pin a buffer you haven't even attached to yet. From the code it >>> seems like first you always want to attach, hence it would make sense to >>> put the pin/unpin onto the attachment. That might also help with >>> debugging, we could check whether everyone balances their pin/unpin >>> correctly (instead of just counting for the overall dma-buf). >>> >>> There's also a slight semantic difference between a pin on an attachment >>> (which would mean this attachment is always going to be mappable and wont >>> move around), whereas a pin on the entire dma-buf means the entire dma-buf >>> and all it's attachment must always be mappable. Otoh, global pin is >>> probably easier, you just need to check all current attachments again >>> whether they still work or whether you might need to move the buffer >>> around to a more suitable place (e.g. if you not all can do p2p it needs >>> to go into system ram before it's pinned). >>> >>> For the backing storage you only want one per-bo ->pinned_count, that's >>> clear, my suggestion was more about whether having more information about >>> who's pinning is useful. Exporters can always just ignore that added >>> information. >>> >>>> Key point is I want to call this in the exporter itself in the long run. >>>> E.g. that the exporter stops working with its internal special handling >>>> functions and uses this one instead. >>> Hm, not exactly following why the exporter needs to call >>> dma_buf_pin/unpin, instead of whatever is used to implement that. Or do >>> you mean that you want a ->pinned_count in dma_buf itself, so that there's >>> only one book-keeping for this? >> Yes, exactly that is one of the final goals of this. >> >> We could of course use the attachment here, but that would disqualify the >> exporter calling this directly without an attachment. > Hm exporter calling dma_buf_pin/unpin sounds like one seriously inverted > lasagna :-) > > I do understand the goal, all these imported/exporter special cases in > code are a bit silly, but I think the proper fix would be if you just > always import a buffer, even the private ones, allocated against some > exporter-only thing. Then it's always the same function to call. > > But I'm not sure this is a good reasons to guide the dma-buf interfaces > for everyone else. Moving pin to the attachment sounds like a better idea > (if the above is the only reason to keep it on the dma-buf). Yeah, that's on my mind as well. But I'm running into a chicken and egg problem here again. Basically we need to convert the drivers to do their internal operation using the DMA-buf as top level object first and then we can switch all internal operation to using a "normal" attachment. Additional to that it simply doesn't looks like the right idea to use the attachment as parameter here. After all we pin the underlying DMA-buf and NOT the attachment. Christian. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Tue, Apr 30, 2019 at 4:41 PM Koenig, Christian <Christian.Koenig@amd.com> wrote: > > Am 30.04.19 um 16:34 schrieb Daniel Vetter: > > [CAUTION: External Email] > > > > On Tue, Apr 30, 2019 at 04:26:32PM +0200, Christian König wrote: > >> Am 30.04.19 um 15:59 schrieb Daniel Vetter: > >>> On Tue, Apr 30, 2019 at 03:42:22PM +0200, Christian König wrote: > >>>> Am 29.04.19 um 10:40 schrieb Daniel Vetter: > >>>>> On Fri, Apr 26, 2019 at 02:36:28PM +0200, Christian König wrote: > >>>>>> [SNIP] > >>>>>> +/** > >>>>>> + * dma_buf_pin - Lock down the DMA-buf > >>>>>> + * > >>>>>> + * @dmabuf: [in] DMA-buf to lock down. > >>>>>> + * > >>>>>> + * Returns: > >>>>>> + * 0 on success, negative error code on failure. > >>>>>> + */ > >>>>>> +int dma_buf_pin(struct dma_buf *dmabuf) > >>>>> I think this should be on the attachment, not on the buffer. Or is the > >>>>> idea that a pin is for the entire buffer, and all subsequent > >>>>> dma_buf_map_attachment must work for all attachments? I think this matters > >>>>> for sufficiently contrived p2p scenarios. > >>>> This is indeed for the DMA-buf, cause we are pinning the underlying backing > >>>> store and not just one attachment. > >>> You can't move the buffer either way, so from that point there's no > >>> difference. I more meant from an account/api point of view of whether it's > >>> ok to pin a buffer you haven't even attached to yet. From the code it > >>> seems like first you always want to attach, hence it would make sense to > >>> put the pin/unpin onto the attachment. That might also help with > >>> debugging, we could check whether everyone balances their pin/unpin > >>> correctly (instead of just counting for the overall dma-buf). > >>> > >>> There's also a slight semantic difference between a pin on an attachment > >>> (which would mean this attachment is always going to be mappable and wont > >>> move around), whereas a pin on the entire dma-buf means the entire dma-buf > >>> and all it's attachment must always be mappable. Otoh, global pin is > >>> probably easier, you just need to check all current attachments again > >>> whether they still work or whether you might need to move the buffer > >>> around to a more suitable place (e.g. if you not all can do p2p it needs > >>> to go into system ram before it's pinned). > >>> > >>> For the backing storage you only want one per-bo ->pinned_count, that's > >>> clear, my suggestion was more about whether having more information about > >>> who's pinning is useful. Exporters can always just ignore that added > >>> information. > >>> > >>>> Key point is I want to call this in the exporter itself in the long run. > >>>> E.g. that the exporter stops working with its internal special handling > >>>> functions and uses this one instead. > >>> Hm, not exactly following why the exporter needs to call > >>> dma_buf_pin/unpin, instead of whatever is used to implement that. Or do > >>> you mean that you want a ->pinned_count in dma_buf itself, so that there's > >>> only one book-keeping for this? > >> Yes, exactly that is one of the final goals of this. > >> > >> We could of course use the attachment here, but that would disqualify the > >> exporter calling this directly without an attachment. > > Hm exporter calling dma_buf_pin/unpin sounds like one seriously inverted > > lasagna :-) > > > > I do understand the goal, all these imported/exporter special cases in > > code are a bit silly, but I think the proper fix would be if you just > > always import a buffer, even the private ones, allocated against some > > exporter-only thing. Then it's always the same function to call. > > > > But I'm not sure this is a good reasons to guide the dma-buf interfaces > > for everyone else. Moving pin to the attachment sounds like a better idea > > (if the above is the only reason to keep it on the dma-buf). > > Yeah, that's on my mind as well. But I'm running into a chicken and egg > problem here again. Yeah the usual refactor story :-/ > Basically we need to convert the drivers to do their internal operation > using the DMA-buf as top level object first and then we can switch all > internal operation to using a "normal" attachment. > > Additional to that it simply doesn't looks like the right idea to use > the attachment as parameter here. After all we pin the underlying > DMA-buf and NOT the attachment. We pin both imo - I'd be really surprised as an importer if I attach, pin, and then the exporter tells me I can't map the attachment I just made anymore because the buffer is in the wrong place. That's imo what pin really should make sure is assured - we already require this for the static importers to be true (which is also why caching makes sense). Hence for me global pin = pin all the attachments. I also dropped some questions on the amdgpu patches to hopefully better understand all this with actual implementations. btw totally different thought: For dynamic exporters, should we drop the cached sgt on the floor if it's not in use? the drm_prime.c helpers don't need this since they map all the time, but afaiui at least v4l and i915 don't hang onto a mapping forever, only when actually needed. -Daniel
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 50b4c6af04c7..0656dcf289be 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -529,6 +529,41 @@ void dma_buf_put(struct dma_buf *dmabuf) } EXPORT_SYMBOL_GPL(dma_buf_put); +/** + * dma_buf_pin - Lock down the DMA-buf + * + * @dmabuf: [in] DMA-buf to lock down. + * + * Returns: + * 0 on success, negative error code on failure. + */ +int dma_buf_pin(struct dma_buf *dmabuf) +{ + int ret = 0; + + reservation_object_assert_held(dmabuf->resv); + + if (dmabuf->ops->pin) + ret = dmabuf->ops->pin(dmabuf); + + return ret; +} +EXPORT_SYMBOL_GPL(dma_buf_pin); + +/** + * dma_buf_unpin - Remove lock from DMA-buf + * + * @dmabuf: [in] DMA-buf to unlock. + */ +void dma_buf_unpin(struct dma_buf *dmabuf) +{ + reservation_object_assert_held(dmabuf->resv); + + if (dmabuf->ops->unpin) + dmabuf->ops->unpin(dmabuf); +} +EXPORT_SYMBOL_GPL(dma_buf_unpin); + /** * dma_buf_attach - Add the device to dma_buf's attachments list; optionally, * calls attach() of dma_buf_ops to allow device-specific attach functionality @@ -548,7 +583,8 @@ EXPORT_SYMBOL_GPL(dma_buf_put); * accessible to @dev, and cannot be moved to a more suitable place. This is * indicated with the error code -EBUSY. */ -struct dma_buf_attachment *dma_buf_attach(const struct dma_buf_attach_info *info) +struct dma_buf_attachment * +dma_buf_attach(const struct dma_buf_attach_info *info) { struct dma_buf *dmabuf = info->dmabuf; struct dma_buf_attachment *attach; @@ -625,12 +661,19 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, enum dma_data_direction direction) { struct sg_table *sg_table; + int r; might_sleep(); if (WARN_ON(!attach || !attach->dmabuf)) return ERR_PTR(-EINVAL); + reservation_object_lock(attach->dmabuf->resv, NULL); + r = dma_buf_pin(attach->dmabuf); + reservation_object_unlock(attach->dmabuf->resv); + if (r) + return ERR_PTR(r); + sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction); if (!sg_table) sg_table = ERR_PTR(-ENOMEM); @@ -660,6 +703,10 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach, attach->dmabuf->ops->unmap_dma_buf(attach, sg_table, direction); + + reservation_object_lock(attach->dmabuf->resv, NULL); + dma_buf_unpin(attach->dmabuf); + reservation_object_unlock(attach->dmabuf->resv); } EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment); diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index 2c312dfd31a1..0321939b1c3d 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -51,6 +51,34 @@ struct dma_buf_attachment; * @vunmap: [optional] unmaps a vmap from the buffer */ struct dma_buf_ops { + /** + * @pin: + * + * This is called by dma_buf_pin and lets the exporter know that the + * DMA-buf can't be moved any more. + * + * This is called with the dmabuf->resv object locked. + * + * This callback is optional. + * + * Returns: + * + * 0 on success, negative error code on failure. + */ + int (*pin)(struct dma_buf *); + + /** + * @unpin: + * + * This is called by dma_buf_unpin and lets the exporter know that the + * DMA-buf can be moved again. + * + * This is called with the dmabuf->resv object locked. + * + * This callback is optional. + */ + void (*unpin)(struct dma_buf *); + /** * @attach: * @@ -95,9 +123,7 @@ struct dma_buf_ops { * * This is called by dma_buf_map_attachment() and is used to map a * shared &dma_buf into device address space, and it is mandatory. It - * can only be called if @attach has been called successfully. This - * essentially pins the DMA buffer into place, and it cannot be moved - * any more + * can only be called if @attach has been called successfully. * * This call may sleep, e.g. when the backing storage first needs to be * allocated, or moved to a location suitable for all currently attached @@ -135,9 +161,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. */ void (*unmap_dma_buf)(struct dma_buf_attachment *, struct sg_table *, @@ -386,6 +409,9 @@ static inline void get_dma_buf(struct dma_buf *dmabuf) get_file(dmabuf->file); } +int dma_buf_pin(struct dma_buf *dmabuf); +void dma_buf_unpin(struct dma_buf *dmabuf); + struct dma_buf_attachment * dma_buf_attach(const struct dma_buf_attach_info *info); void dma_buf_detach(struct dma_buf *dmabuf,
Add optional explicit pinning callbacks instead of implicitly assume the exporter pins the buffer when a mapping is created. v2: move in patchset and pin the dma-buf in the old mapping code paths. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/dma-buf/dma-buf.c | 49 ++++++++++++++++++++++++++++++++++++++- include/linux/dma-buf.h | 38 +++++++++++++++++++++++++----- 2 files changed, 80 insertions(+), 7 deletions(-)