Re: [PATCH 1/4] dma-buf: change DMA-buf locking convention

["Koenig, Christian" <Christian.Koenig@amd.com>]

Am 17.09.19 um 14:31 schrieb Daniel Vetter:
> On Mon, Sep 16, 2019 at 02:23:13PM +0200, Christian König wrote:
>> Ping? Any further comment on this or can't we merge at least the locking
>> change?
> I was at plumbers ...
>> Christian.
>>
>> Am 11.09.19 um 12:53 schrieb Christian König:
>>> Am 03.09.19 um 10:05 schrieb Daniel Vetter:
>>>> On Thu, Aug 29, 2019 at 04:29:14PM +0200, Christian König wrote:
>>>>> This patch is a stripped down version of the locking changes
>>>>> necessary to support dynamic DMA-buf handling.
>>>>>
>>>>> For compatibility we cache the DMA-buf mapping as soon as
>>>>> exporter/importer disagree on the dynamic handling.
>>>>>
>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>>> ---
>>>>>    drivers/dma-buf/dma-buf.c | 90
>>>>> ++++++++++++++++++++++++++++++++++++---
>>>>>    include/linux/dma-buf.h   | 51 +++++++++++++++++++++-
>>>>>    2 files changed, 133 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>>>>> index 433d91d710e4..65052d52602b 100644
>>>>> --- a/drivers/dma-buf/dma-buf.c
>>>>> +++ b/drivers/dma-buf/dma-buf.c
>>>>> @@ -525,6 +525,10 @@ struct dma_buf *dma_buf_export(const struct
>>>>> dma_buf_export_info *exp_info)
>>>>>            return ERR_PTR(-EINVAL);
>>>>>        }
>>>>>    +    if (WARN_ON(exp_info->ops->cache_sgt_mapping &&
>>>>> +            exp_info->ops->dynamic_mapping))
>>>>> +        return ERR_PTR(-EINVAL);
>>>>> +
>>>>>        if (!try_module_get(exp_info->owner))
>>>>>            return ERR_PTR(-ENOENT);
>>>>>    @@ -645,10 +649,11 @@ void dma_buf_put(struct dma_buf *dmabuf)
>>>>>    EXPORT_SYMBOL_GPL(dma_buf_put);
>>>>>      /**
>>>>> - * dma_buf_attach - Add the device to dma_buf's attachments
>>>>> list; optionally,
>>>>> + * dma_buf_dynamic_attach - Add the device to dma_buf's
>>>>> attachments list; optionally,
>>>>>     * calls attach() of dma_buf_ops to allow device-specific
>>>>> attach functionality
>>>>> - * @dmabuf:    [in]    buffer to attach device to.
>>>>> - * @dev:    [in]    device to be attached.
>>>>> + * @dmabuf:        [in]    buffer to attach device to.
>>>>> + * @dev:        [in]    device to be attached.
>>>>> + * @dynamic_mapping:    [in]    calling convention for map/unmap
>>>>>     *
>>>>>     * Returns struct dma_buf_attachment pointer for this
>>>>> attachment. Attachments
>>>>>     * must be cleaned up by calling dma_buf_detach().
>>>>> @@ -662,8 +667,9 @@ 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(struct dma_buf *dmabuf,
>>>>> -                      struct device *dev)
>>>>> +struct dma_buf_attachment *
>>>>> +dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev,
>>>>> +               bool dynamic_mapping)
>>>>>    {
>>>>>        struct dma_buf_attachment *attach;
>>>>>        int ret;
>>>>> @@ -677,6 +683,7 @@ struct dma_buf_attachment
>>>>> *dma_buf_attach(struct dma_buf *dmabuf,
>>>>>          attach->dev = dev;
>>>>>        attach->dmabuf = dmabuf;
>>>>> +    attach->dynamic_mapping = dynamic_mapping;
>>>>>          mutex_lock(&dmabuf->lock);
>>>>>    @@ -685,16 +692,64 @@ struct dma_buf_attachment
>>>>> *dma_buf_attach(struct dma_buf *dmabuf,
>>>>>            if (ret)
>>>>>                goto err_attach;
>>>>>        }
>>>>> +    dma_resv_lock(dmabuf->resv, NULL);
>>>>>        list_add(&attach->node, &dmabuf->attachments);
>>>>> +    dma_resv_unlock(dmabuf->resv);
>>>>>          mutex_unlock(&dmabuf->lock);
>>>>>    +    /* When either the importer or the exporter can't handle dynamic
>>>>> +     * mappings we cache the mapping here to avoid issues with the
>>>>> +     * reservation object lock.
>>>>> +     */
>>>>> +    if (dma_buf_attachment_is_dynamic(attach) !=
>>>>> +        dma_buf_is_dynamic(dmabuf)) {
>>>>> +        struct sg_table *sgt;
>>>>> +
>>>>> +        if (dma_buf_is_dynamic(attach->dmabuf))
>>>>> +            dma_resv_lock(attach->dmabuf->resv, NULL);
>>>>> +
>>>>> +        sgt = dmabuf->ops->map_dma_buf(attach, DMA_BIDIRECTIONAL);
>>>> Now we're back to enforcing DMA_BIDI, which works nicely around the
>>>> locking pain, but apparently upsets the arm-soc folks who want to
>>>> control
>>>> this better.
>>> Take another look at dma_buf_map_attachment(), we still try to get the
>>> caching there for ARM.
>>>
>>> What we do here is to bidirectionally map the buffer to avoid the
>>> locking hydra when importer and exporter disagree on locking.
>>>
>>> So the ARM folks can easily avoid that by switching to dynamic locking
>>> for both.
> So you still break the contract between importer and exporter, except not
> for anything that's run in intel-gfx-ci so all is good?

No, the contract between importer and exporter stays exactly the same it 
is currently as long as you don't switch to dynamic dma-buf handling.

There is no functional change for the ARM folks here. The only change 
which takes effect is between i915 and amdgpu and that is perfectly 
covered by intel-gfx-ci.

Regards,
Christian.

>
> The other issue with "we solve this with caching the mapping": Currently
> map/unmap flush (at least on arm, at least on cases where it matters). If
> you just return the cached sg, then we don't do the flushing anymore,
> which might break importers/exporters in exactly the same way as just
> giving them the wrong mapping. There's zero differences between a BIDI,
> TO_CPU, or TO_DEVICE mapping, the only places where this matters is for
> cache flushing.
>
> So here's something that could actually work:
> - We cache the mapping.
> - We cache a bidirectional mapping.
> - We put the right dma_sg_sync_for_cpu/device calls in place for map/unmap
>    to give current importers/exporters the same behaviour they're used to
>    now.
>
> And yes the caching we've lifted might have broken something somewhere
> already. But generally you only hear about that long time after because
> arm vendors roll forward once every few years. Or something like that.
> -Daniel
>
>>> Regards,
>>> Christian.
>>>
>>>> That's why your previous version moved the caching into
>>>> map/unmap_sg, which resurrected the locking hydra.
>>>>
>>>> I think we're going a bit in circles here, and I don't have a good idea
>>>> either :-/
>>>> -Daniel
>>>>
>>>>> +        if (!sgt)
>>>>> +            sgt = ERR_PTR(-ENOMEM);
>>>>> +        if (IS_ERR(sgt)) {
>>>>> +            ret = PTR_ERR(sgt);
>>>>> +            goto err_unlock;
>>>>> +        }
>>>>> +        if (dma_buf_is_dynamic(attach->dmabuf))
>>>>> +            dma_resv_unlock(attach->dmabuf->resv);
>>>>> +        attach->sgt = sgt;
>>>>> +        attach->dir = DMA_BIDIRECTIONAL;
>>>>> +    }
>>>>> +
>>>>>        return attach;
>>>>>      err_attach:
>>>>>        kfree(attach);
>>>>>        mutex_unlock(&dmabuf->lock);
>>>>>        return ERR_PTR(ret);
>>>>> +
>>>>> +err_unlock:
>>>>> +    if (dma_buf_is_dynamic(attach->dmabuf))
>>>>> +        dma_resv_unlock(attach->dmabuf->resv);
>>>>> +
>>>>> +    dma_buf_detach(dmabuf, attach);
>>>>> +    return ERR_PTR(ret);
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(dma_buf_dynamic_attach);
>>>>> +
>>>>> +/**
>>>>> + * dma_buf_attach - Wrapper for dma_buf_dynamic_attach
>>>>> + * @dmabuf:    [in]    buffer to attach device to.
>>>>> + * @dev:    [in]    device to be attached.
>>>>> + *
>>>>> + * Wrapper to call dma_buf_dynamic_attach() for drivers which
>>>>> still use a static
>>>>> + * mapping.
>>>>> + */
>>>>> +struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
>>>>> +                      struct device *dev)
>>>>> +{
>>>>> +    return dma_buf_dynamic_attach(dmabuf, dev, false);
>>>>>    }
>>>>>    EXPORT_SYMBOL_GPL(dma_buf_attach);
>>>>>    @@ -711,11 +766,20 @@ void dma_buf_detach(struct dma_buf
>>>>> *dmabuf, struct dma_buf_attachment *attach)
>>>>>        if (WARN_ON(!dmabuf || !attach))
>>>>>            return;
>>>>>    -    if (attach->sgt)
>>>>> +    if (attach->sgt) {
>>>>> +        if (dma_buf_is_dynamic(attach->dmabuf))
>>>>> +            dma_resv_lock(attach->dmabuf->resv, NULL);
>>>>> +
>>>>>            dmabuf->ops->unmap_dma_buf(attach, attach->sgt, attach->dir);
>>>>>    +        if (dma_buf_is_dynamic(attach->dmabuf))
>>>>> +            dma_resv_unlock(attach->dmabuf->resv);
>>>>> +    }
>>>>> +
>>>>>        mutex_lock(&dmabuf->lock);
>>>>> +    dma_resv_lock(dmabuf->resv, NULL);
>>>>>        list_del(&attach->node);
>>>>> +    dma_resv_unlock(dmabuf->resv);
>>>>>        if (dmabuf->ops->detach)
>>>>>            dmabuf->ops->detach(dmabuf, attach);
>>>>>    @@ -749,6 +813,9 @@ struct sg_table
>>>>> *dma_buf_map_attachment(struct dma_buf_attachment *attach,
>>>>>        if (WARN_ON(!attach || !attach->dmabuf))
>>>>>            return ERR_PTR(-EINVAL);
>>>>>    +    if (dma_buf_attachment_is_dynamic(attach))
>>>>> +        dma_resv_assert_held(attach->dmabuf->resv);
>>>>> +
>>>>>        if (attach->sgt) {
>>>>>            /*
>>>>>             * Two mappings with different directions for the same
>>>>> @@ -761,6 +828,9 @@ struct sg_table
>>>>> *dma_buf_map_attachment(struct dma_buf_attachment *attach,
>>>>>            return attach->sgt;
>>>>>        }
>>>>>    +    if (dma_buf_is_dynamic(attach->dmabuf))
>>>>> +        dma_resv_assert_held(attach->dmabuf->resv);
>>>>> +
>>>>>        sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);
>>>>>        if (!sg_table)
>>>>>            sg_table = ERR_PTR(-ENOMEM);
>>>>> @@ -793,9 +863,15 @@ void dma_buf_unmap_attachment(struct
>>>>> dma_buf_attachment *attach,
>>>>>        if (WARN_ON(!attach || !attach->dmabuf || !sg_table))
>>>>>            return;
>>>>>    +    if (dma_buf_attachment_is_dynamic(attach))
>>>>> +        dma_resv_assert_held(attach->dmabuf->resv);
>>>>> +
>>>>>        if (attach->sgt == sg_table)
>>>>>            return;
>>>>>    +    if (dma_buf_is_dynamic(attach->dmabuf))
>>>>> +        dma_resv_assert_held(attach->dmabuf->resv);
>>>>> +
>>>>>        attach->dmabuf->ops->unmap_dma_buf(attach, sg_table, direction);
>>>>>    }
>>>>>    EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment);
>>>>> @@ -1219,10 +1295,12 @@ static int dma_buf_debug_show(struct
>>>>> seq_file *s, void *unused)
>>>>>            seq_puts(s, "\tAttached Devices:\n");
>>>>>            attach_count = 0;
>>>>>    +        dma_resv_lock(buf_obj->resv, NULL);
>>>>>            list_for_each_entry(attach_obj, &buf_obj->attachments,
>>>>> node) {
>>>>>                seq_printf(s, "\t%s\n", dev_name(attach_obj->dev));
>>>>>                attach_count++;
>>>>>            }
>>>>> +        dma_resv_unlock(buf_obj->resv);
>>>>>              seq_printf(s, "Total %d devices attached\n\n",
>>>>>                    attach_count);
>>>>> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
>>>>> index ec212cb27fdc..a8f8b2b812fd 100644
>>>>> --- a/include/linux/dma-buf.h
>>>>> +++ b/include/linux/dma-buf.h
>>>>> @@ -42,6 +42,17 @@ struct dma_buf_ops {
>>>>>          */
>>>>>        bool cache_sgt_mapping;
>>>>>    +    /**
>>>>> +     * @dynamic_mapping:
>>>>> +     *
>>>>> +     * If true the framework makes sure that the map/unmap_dma_buf
>>>>> +     * callbacks are always called with the dma_resv object locked.
>>>>> +     *
>>>>> +     * If false the framework makes ure that the map/unmap_dma_buf
>>>>> +     * callbacks are always called without the dma_resv object locked.
>>>>> +     */
>>>>> +    bool dynamic_mapping;
>>>>> +
>>>>>        /**
>>>>>         * @attach:
>>>>>         *
>>>>> @@ -109,6 +120,9 @@ struct dma_buf_ops {
>>>>>         * any other kind of sharing that the exporter might wish to make
>>>>>         * available to buffer-users.
>>>>>         *
>>>>> +     * This is always called with the dmabuf->resv object locked when
>>>>> +     * the dynamic_mapping flag is true.
>>>>> +     *
>>>>>         * Returns:
>>>>>         *
>>>>>         * A &sg_table scatter list of or the backing storage of
>>>>> the DMA buffer,
>>>>> @@ -327,6 +341,8 @@ struct dma_buf {
>>>>>     * @sgt: cached mapping.
>>>>>     * @dir: direction of cached mapping.
>>>>>     * @priv: exporter specific attachment data.
>>>>> + * @dynamic_mapping: true if dma_buf_map/unmap_attachment() is
>>>>> called with the
>>>>> + * dma_resv lock held.
>>>>>     *
>>>>>     * This structure holds the attachment information between the
>>>>> dma_buf buffer
>>>>>     * and its user device(s). The list contains one attachment
>>>>> struct per device
>>>>> @@ -343,6 +359,7 @@ struct dma_buf_attachment {
>>>>>        struct list_head node;
>>>>>        struct sg_table *sgt;
>>>>>        enum dma_data_direction dir;
>>>>> +    bool dynamic_mapping;
>>>>>        void *priv;
>>>>>    };
>>>>>    @@ -394,10 +411,39 @@ static inline void get_dma_buf(struct
>>>>> dma_buf *dmabuf)
>>>>>        get_file(dmabuf->file);
>>>>>    }
>>>>>    +/**
>>>>> + * dma_buf_is_dynamic - check if a DMA-buf uses dynamic mappings.
>>>>> + * @dmabuf: the DMA-buf to check
>>>>> + *
>>>>> + * Returns true if a DMA-buf exporter wants to be called with
>>>>> the dma_resv
>>>>> + * locked, false if it doesn't wants to be called with the lock held.
>>>>> + */
>>>>> +static inline bool dma_buf_is_dynamic(struct dma_buf *dmabuf)
>>>>> +{
>>>>> +    return dmabuf->ops->dynamic_mapping;
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * dma_buf_attachment_is_dynamic - check if a DMA-buf
>>>>> attachment uses dynamic
>>>>> + * mappinsg
>>>>> + * @attach: the DMA-buf attachment to check
>>>>> + *
>>>>> + * Returns true if a DMA-buf importer wants to call the
>>>>> map/unmap functions with
>>>>> + * the dma_resv lock held.
>>>>> + */
>>>>> +static inline bool
>>>>> +dma_buf_attachment_is_dynamic(struct dma_buf_attachment *attach)
>>>>> +{
>>>>> +    return attach->dynamic_mapping;
>>>>> +}
>>>>> +
>>>>>    struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
>>>>> -                            struct device *dev);
>>>>> +                      struct device *dev);
>>>>> +struct dma_buf_attachment *
>>>>> +dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev,
>>>>> +               bool dynamic_mapping);
>>>>>    void dma_buf_detach(struct dma_buf *dmabuf,
>>>>> -                struct dma_buf_attachment *dmabuf_attach);
>>>>> +            struct dma_buf_attachment *attach);
>>>>>      struct dma_buf *dma_buf_export(const struct
>>>>> dma_buf_export_info *exp_info);
>>>>>    @@ -409,6 +455,7 @@ struct sg_table
>>>>> *dma_buf_map_attachment(struct dma_buf_attachment *,
>>>>>                        enum dma_data_direction);
>>>>>    void dma_buf_unmap_attachment(struct dma_buf_attachment *,
>>>>> struct sg_table *,
>>>>>                    enum dma_data_direction);
>>>>> +void dma_buf_move_notify(struct dma_buf *dma_buf);
>>>>>    int dma_buf_begin_cpu_access(struct dma_buf *dma_buf,
>>>>>                     enum dma_data_direction dir);
>>>>>    int dma_buf_end_cpu_access(struct dma_buf *dma_buf,
>>>>> -- 
>>>>> 2.17.1
>>>>>