Re: [REVIEWv7 PATCH 07/12] vb2-dma-sg: add dmabuf import support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 12/01/2014 11:46 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Thursday 27 November 2014 10:02:14 Hans Verkuil wrote:
>> On 11/26/2014 10:00 PM, Laurent Pinchart wrote:
>>> On Tuesday 18 November 2014 13:51:03 Hans Verkuil wrote:
>>>> From: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>>>>
>>>> Add support for importing dmabuf to videobuf2-dma-sg.
>>>>
>>>> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>>>> Acked-by: Pawel Osciak <pawel@xxxxxxxxxx>
>>>> ---
>>>>
>>>>  drivers/media/v4l2-core/videobuf2-dma-sg.c | 149 ++++++++++++++++++++---
>>>>  1 file changed, 136 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/drivers/media/v4l2-core/videobuf2-dma-sg.c
>>>> b/drivers/media/v4l2-core/videobuf2-dma-sg.c index f671fab..ad6d5c7
>>>> 100644
>>>> --- a/drivers/media/v4l2-core/videobuf2-dma-sg.c
>>>> +++ b/drivers/media/v4l2-core/videobuf2-dma-sg.c
> 
> [snip]
> 
>>>> @@ -183,7 +192,11 @@ static void vb2_dma_sg_put(void *buf_priv)
>>>>  static void vb2_dma_sg_prepare(void *buf_priv)
>>>>  {
>>>>  	struct vb2_dma_sg_buf *buf = buf_priv;
>>>> -	struct sg_table *sgt = &buf->sg_table;
>>>> +	struct sg_table *sgt = buf->dma_sgt;
>>>> +
>>>> +	/* DMABUF exporter will flush the cache for us */
>>>> +	if (buf->db_attach)
>>>> +		return;
>>>
>>> Is this actually true ? If you look at the export code in patch 08/12, I
>>> don't see where the exporter would sync the buffer for the importer
>>> device.
>>
>> I think this was true at some point in the past. It ties in with my comment
>> for patch 06/12: cpu/device syncing for dma-buf is (and was) broken,
>> although nobody has noticed since none of the DMABUF-aware drivers that are
>> used as such today need CPU access to the buffer, or are only used on Intel
>> architectures where this is all moot. Patches 12-16 of my RFCv6 series
>> really fix this. This particular comment was copied from the dma-contig
>> version. The basic idea was that when the driver needs CPU access it will
>> call the vaddr memop, which will map the buffer for CPU access.
>>
>> However, I am not sure whether dmabuf actually did the right thing there
>> originally. Later dmabuf was extended with begin/end_for_cpu_access ops to
>> make this explicit, but that was never implemented in vb2. That's what the
>> second part of RFCv6 does.
>>
>> Right now dma-sg is bug-compatible with dma-contig.
>>
>> I spend 1-2 hours with Pawel in Düsseldorf figuring this out, it is not
>> exactly trivial to understand.
> 
> I agree that the situation is a mess, but we'll need to fix it one way or 
> another :-/ The more we wait the more painful it will be. Please note that the 
> problem isn't specific to CPU access, we need to sync for the device even in 
> direct device to device transfers (although in practice that shouldn't be 
> strictly required with the platforms we target today).

As I mentioned, I do have fixes for this. But I still need to implement that
in two more drivers where it wasn't obvious how it should be done. I hope to
be able to tackle that soon.

Regards,

	Hans

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux