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

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

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.

>> [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.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel