| Message ID | 20210517115705.2141-1-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dma-buf: fix unintended pin/unpin warnings | expand |
On Mon, May 17, 2021 at 7:57 AM Christian König <ckoenig.leichtzumerken@gmail.com> wrote: > > DMA-buf internal users call the pin/unpin functions without having a > dynamic attachment. Avoid the warning and backtrace in the logs. > > Signed-off-by: Christian König <christian.koenig@amd.com> > Bugs: https://gitlab.freedesktop.org/drm/intel/-/issues/3481 > Fixes: c545781e1c55 ("dma-buf: doc polish for pin/unpin") > CC: stable@kernel.org Reviewed-by: Alex Deucher <alexander.deucher@amd.com> > --- > drivers/dma-buf/dma-buf.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index f264b70c383e..eadd1eaa2fb5 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -760,7 +760,7 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, > > if (dma_buf_is_dynamic(attach->dmabuf)) { > dma_resv_lock(attach->dmabuf->resv, NULL); > - ret = dma_buf_pin(attach); > + ret = dmabuf->ops->pin(attach); > if (ret) > goto err_unlock; > } > @@ -786,7 +786,7 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, > > err_unpin: > if (dma_buf_is_dynamic(attach->dmabuf)) > - dma_buf_unpin(attach); > + dmabuf->ops->unpin(attach); > > err_unlock: > if (dma_buf_is_dynamic(attach->dmabuf)) > @@ -843,7 +843,7 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) > __unmap_dma_buf(attach, attach->sgt, attach->dir); > > if (dma_buf_is_dynamic(attach->dmabuf)) { > - dma_buf_unpin(attach); > + dmabuf->ops->unpin(attach); > dma_resv_unlock(attach->dmabuf->resv); > } > } > @@ -956,7 +956,7 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, > if (dma_buf_is_dynamic(attach->dmabuf)) { > dma_resv_assert_held(attach->dmabuf->resv); > if (!IS_ENABLED(CONFIG_DMABUF_MOVE_NOTIFY)) { > - r = dma_buf_pin(attach); > + r = attach->dmabuf->ops->pin(attach); > if (r) > return ERR_PTR(r); > } > @@ -968,7 +968,7 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, > > if (IS_ERR(sg_table) && dma_buf_is_dynamic(attach->dmabuf) && > !IS_ENABLED(CONFIG_DMABUF_MOVE_NOTIFY)) > - dma_buf_unpin(attach); > + attach->dmabuf->ops->unpin(attach); > > if (!IS_ERR(sg_table) && attach->dmabuf->ops->cache_sgt_mapping) { > attach->sgt = sg_table; > -- > 2.25.1 >
On Mon, May 17, 2021 at 10:09:13AM -0400, Alex Deucher wrote: > On Mon, May 17, 2021 at 7:57 AM Christian König > <ckoenig.leichtzumerken@gmail.com> wrote: > > > > DMA-buf internal users call the pin/unpin functions without having a > > dynamic attachment. Avoid the warning and backtrace in the logs. > > > > Signed-off-by: Christian König <christian.koenig@amd.com> > > Bugs: https://gitlab.freedesktop.org/drm/intel/-/issues/3481 > > Fixes: c545781e1c55 ("dma-buf: doc polish for pin/unpin") > > CC: stable@kernel.org > > Reviewed-by: Alex Deucher <alexander.deucher@amd.com> Hm this means we're losing the dma_resv_assert_held check, do we have that in amdgpu callbacks to make sure we're not accidentally breaking stuff later on? But yeah lgtm, Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > --- > > drivers/dma-buf/dma-buf.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > > index f264b70c383e..eadd1eaa2fb5 100644 > > --- a/drivers/dma-buf/dma-buf.c > > +++ b/drivers/dma-buf/dma-buf.c > > @@ -760,7 +760,7 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, > > > > if (dma_buf_is_dynamic(attach->dmabuf)) { > > dma_resv_lock(attach->dmabuf->resv, NULL); > > - ret = dma_buf_pin(attach); > > + ret = dmabuf->ops->pin(attach); > > if (ret) > > goto err_unlock; > > } > > @@ -786,7 +786,7 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, > > > > err_unpin: > > if (dma_buf_is_dynamic(attach->dmabuf)) > > - dma_buf_unpin(attach); > > + dmabuf->ops->unpin(attach); > > > > err_unlock: > > if (dma_buf_is_dynamic(attach->dmabuf)) > > @@ -843,7 +843,7 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) > > __unmap_dma_buf(attach, attach->sgt, attach->dir); > > > > if (dma_buf_is_dynamic(attach->dmabuf)) { > > - dma_buf_unpin(attach); > > + dmabuf->ops->unpin(attach); > > dma_resv_unlock(attach->dmabuf->resv); > > } > > } > > @@ -956,7 +956,7 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, > > if (dma_buf_is_dynamic(attach->dmabuf)) { > > dma_resv_assert_held(attach->dmabuf->resv); > > if (!IS_ENABLED(CONFIG_DMABUF_MOVE_NOTIFY)) { > > - r = dma_buf_pin(attach); > > + r = attach->dmabuf->ops->pin(attach); > > if (r) > > return ERR_PTR(r); > > } > > @@ -968,7 +968,7 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, > > > > if (IS_ERR(sg_table) && dma_buf_is_dynamic(attach->dmabuf) && > > !IS_ENABLED(CONFIG_DMABUF_MOVE_NOTIFY)) > > - dma_buf_unpin(attach); > > + attach->dmabuf->ops->unpin(attach); > > > > if (!IS_ERR(sg_table) && attach->dmabuf->ops->cache_sgt_mapping) { > > attach->sgt = sg_table; > > -- > > 2.25.1 > >
Am 17.05.21 um 16:57 schrieb Daniel Vetter: > On Mon, May 17, 2021 at 10:09:13AM -0400, Alex Deucher wrote: >> On Mon, May 17, 2021 at 7:57 AM Christian König >> <ckoenig.leichtzumerken@gmail.com> wrote: >>> DMA-buf internal users call the pin/unpin functions without having a >>> dynamic attachment. Avoid the warning and backtrace in the logs. >>> >>> Signed-off-by: Christian König <christian.koenig@amd.com> >>> Bugs: https://gitlab.freedesktop.org/drm/intel/-/issues/3481 >>> Fixes: c545781e1c55 ("dma-buf: doc polish for pin/unpin") >>> CC: stable@kernel.org >> Reviewed-by: Alex Deucher <alexander.deucher@amd.com> > Hm this means we're losing the dma_resv_assert_held check, do we have that > in amdgpu callbacks to make sure we're not accidentally breaking stuff > later on? Mhm, well this is just for calling the pin/unpin internally from the DMA-buf framework itself. Need to double check, but I think all those cases either have a dma_resv_assert_held() or are locking the reservation themselves before calling the function. But yeah, rather good point. Christian. > > But yeah lgtm, Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > >>> --- >>> drivers/dma-buf/dma-buf.c | 10 +++++----- >>> 1 file changed, 5 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c >>> index f264b70c383e..eadd1eaa2fb5 100644 >>> --- a/drivers/dma-buf/dma-buf.c >>> +++ b/drivers/dma-buf/dma-buf.c >>> @@ -760,7 +760,7 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, >>> >>> if (dma_buf_is_dynamic(attach->dmabuf)) { >>> dma_resv_lock(attach->dmabuf->resv, NULL); >>> - ret = dma_buf_pin(attach); >>> + ret = dmabuf->ops->pin(attach); >>> if (ret) >>> goto err_unlock; >>> } >>> @@ -786,7 +786,7 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, >>> >>> err_unpin: >>> if (dma_buf_is_dynamic(attach->dmabuf)) >>> - dma_buf_unpin(attach); >>> + dmabuf->ops->unpin(attach); >>> >>> err_unlock: >>> if (dma_buf_is_dynamic(attach->dmabuf)) >>> @@ -843,7 +843,7 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) >>> __unmap_dma_buf(attach, attach->sgt, attach->dir); >>> >>> if (dma_buf_is_dynamic(attach->dmabuf)) { >>> - dma_buf_unpin(attach); >>> + dmabuf->ops->unpin(attach); >>> dma_resv_unlock(attach->dmabuf->resv); >>> } >>> } >>> @@ -956,7 +956,7 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, >>> if (dma_buf_is_dynamic(attach->dmabuf)) { >>> dma_resv_assert_held(attach->dmabuf->resv); >>> if (!IS_ENABLED(CONFIG_DMABUF_MOVE_NOTIFY)) { >>> - r = dma_buf_pin(attach); >>> + r = attach->dmabuf->ops->pin(attach); >>> if (r) >>> return ERR_PTR(r); >>> } >>> @@ -968,7 +968,7 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, >>> >>> if (IS_ERR(sg_table) && dma_buf_is_dynamic(attach->dmabuf) && >>> !IS_ENABLED(CONFIG_DMABUF_MOVE_NOTIFY)) >>> - dma_buf_unpin(attach); >>> + attach->dmabuf->ops->unpin(attach); >>> >>> if (!IS_ERR(sg_table) && attach->dmabuf->ops->cache_sgt_mapping) { >>> attach->sgt = sg_table; >>> -- >>> 2.25.1 >>>
On Thu, May 20, 2021 at 01:37:47PM +0200, Christian König wrote: > Am 17.05.21 um 16:57 schrieb Daniel Vetter: > > On Mon, May 17, 2021 at 10:09:13AM -0400, Alex Deucher wrote: > > > On Mon, May 17, 2021 at 7:57 AM Christian König > > > <ckoenig.leichtzumerken@gmail.com> wrote: > > > > DMA-buf internal users call the pin/unpin functions without having a > > > > dynamic attachment. Avoid the warning and backtrace in the logs. > > > > > > > > Signed-off-by: Christian König <christian.koenig@amd.com> > > > > Bugs: https://gitlab.freedesktop.org/drm/intel/-/issues/3481 > > > > Fixes: c545781e1c55 ("dma-buf: doc polish for pin/unpin") > > > > CC: stable@kernel.org > > > Reviewed-by: Alex Deucher <alexander.deucher@amd.com> > > Hm this means we're losing the dma_resv_assert_held check, do we have that > > in amdgpu callbacks to make sure we're not accidentally breaking stuff > > later on? > > Mhm, well this is just for calling the pin/unpin internally from the DMA-buf > framework itself. It's both, but currently I think no users for it (since it would be pinning for display). Maybe we could do __ internal wrappers to keep the lockdep checks? > Need to double check, but I think all those cases either have a > dma_resv_assert_held() or are locking the reservation themselves before > calling the function. Yeah right now it's still all good, but that changes easily :-) > But yeah, rather good point. Do you plan to type something that I can ack? -Daniel > > Christian. > > > > > But yeah lgtm, Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > > > --- > > > > drivers/dma-buf/dma-buf.c | 10 +++++----- > > > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > > > > index f264b70c383e..eadd1eaa2fb5 100644 > > > > --- a/drivers/dma-buf/dma-buf.c > > > > +++ b/drivers/dma-buf/dma-buf.c > > > > @@ -760,7 +760,7 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, > > > > > > > > if (dma_buf_is_dynamic(attach->dmabuf)) { > > > > dma_resv_lock(attach->dmabuf->resv, NULL); > > > > - ret = dma_buf_pin(attach); > > > > + ret = dmabuf->ops->pin(attach); > > > > if (ret) > > > > goto err_unlock; > > > > } > > > > @@ -786,7 +786,7 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, > > > > > > > > err_unpin: > > > > if (dma_buf_is_dynamic(attach->dmabuf)) > > > > - dma_buf_unpin(attach); > > > > + dmabuf->ops->unpin(attach); > > > > > > > > err_unlock: > > > > if (dma_buf_is_dynamic(attach->dmabuf)) > > > > @@ -843,7 +843,7 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) > > > > __unmap_dma_buf(attach, attach->sgt, attach->dir); > > > > > > > > if (dma_buf_is_dynamic(attach->dmabuf)) { > > > > - dma_buf_unpin(attach); > > > > + dmabuf->ops->unpin(attach); > > > > dma_resv_unlock(attach->dmabuf->resv); > > > > } > > > > } > > > > @@ -956,7 +956,7 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, > > > > if (dma_buf_is_dynamic(attach->dmabuf)) { > > > > dma_resv_assert_held(attach->dmabuf->resv); > > > > if (!IS_ENABLED(CONFIG_DMABUF_MOVE_NOTIFY)) { > > > > - r = dma_buf_pin(attach); > > > > + r = attach->dmabuf->ops->pin(attach); > > > > if (r) > > > > return ERR_PTR(r); > > > > } > > > > @@ -968,7 +968,7 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, > > > > > > > > if (IS_ERR(sg_table) && dma_buf_is_dynamic(attach->dmabuf) && > > > > !IS_ENABLED(CONFIG_DMABUF_MOVE_NOTIFY)) > > > > - dma_buf_unpin(attach); > > > > + attach->dmabuf->ops->unpin(attach); > > > > > > > > if (!IS_ERR(sg_table) && attach->dmabuf->ops->cache_sgt_mapping) { > > > > attach->sgt = sg_table; > > > > -- > > > > 2.25.1 > > > > >
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index f264b70c383e..eadd1eaa2fb5 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -760,7 +760,7 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, if (dma_buf_is_dynamic(attach->dmabuf)) { dma_resv_lock(attach->dmabuf->resv, NULL); - ret = dma_buf_pin(attach); + ret = dmabuf->ops->pin(attach); if (ret) goto err_unlock; } @@ -786,7 +786,7 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, err_unpin: if (dma_buf_is_dynamic(attach->dmabuf)) - dma_buf_unpin(attach); + dmabuf->ops->unpin(attach); err_unlock: if (dma_buf_is_dynamic(attach->dmabuf)) @@ -843,7 +843,7 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) __unmap_dma_buf(attach, attach->sgt, attach->dir); if (dma_buf_is_dynamic(attach->dmabuf)) { - dma_buf_unpin(attach); + dmabuf->ops->unpin(attach); dma_resv_unlock(attach->dmabuf->resv); } } @@ -956,7 +956,7 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, if (dma_buf_is_dynamic(attach->dmabuf)) { dma_resv_assert_held(attach->dmabuf->resv); if (!IS_ENABLED(CONFIG_DMABUF_MOVE_NOTIFY)) { - r = dma_buf_pin(attach); + r = attach->dmabuf->ops->pin(attach); if (r) return ERR_PTR(r); } @@ -968,7 +968,7 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, if (IS_ERR(sg_table) && dma_buf_is_dynamic(attach->dmabuf) && !IS_ENABLED(CONFIG_DMABUF_MOVE_NOTIFY)) - dma_buf_unpin(attach); + attach->dmabuf->ops->unpin(attach); if (!IS_ERR(sg_table) && attach->dmabuf->ops->cache_sgt_mapping) { attach->sgt = sg_table;
DMA-buf internal users call the pin/unpin functions without having a dynamic attachment. Avoid the warning and backtrace in the logs. Signed-off-by: Christian König <christian.koenig@amd.com> Bugs: https://gitlab.freedesktop.org/drm/intel/-/issues/3481 Fixes: c545781e1c55 ("dma-buf: doc polish for pin/unpin") CC: stable@kernel.org --- drivers/dma-buf/dma-buf.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)