All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] media: venus: fix wrong size on dma_free
@ 2017-10-09 12:24 Stanimir Varbanov
  2017-10-09 12:24 ` [PATCH 2/2] media: venus: venc: fix bytesused v4l2_plane field Stanimir Varbanov
  0 siblings, 1 reply; 6+ messages in thread
From: Stanimir Varbanov @ 2017-10-09 12:24 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Nicolas Dufresne, linux-media, linux-kernel, linux-arm-msm,
	Stanimir Varbanov

This change will fix an issue with dma_free size found with
DMA API debug enabled.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 drivers/media/platform/qcom/venus/hfi_venus.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
index 1caae8feaa36..734ce11b0ed0 100644
--- a/drivers/media/platform/qcom/venus/hfi_venus.c
+++ b/drivers/media/platform/qcom/venus/hfi_venus.c
@@ -344,7 +344,7 @@ static int venus_alloc(struct venus_hfi_device *hdev, struct mem_desc *desc,
 	desc->attrs = DMA_ATTR_WRITE_COMBINE;
 	desc->size = ALIGN(size, SZ_4K);
 
-	desc->kva = dma_alloc_attrs(dev, size, &desc->da, GFP_KERNEL,
+	desc->kva = dma_alloc_attrs(dev, desc->size, &desc->da, GFP_KERNEL,
 				    desc->attrs);
 	if (!desc->kva)
 		return -ENOMEM;
@@ -710,10 +710,8 @@ static int venus_interface_queues_init(struct venus_hfi_device *hdev)
 	if (ret)
 		return ret;
 
-	hdev->ifaceq_table.kva = desc.kva;
-	hdev->ifaceq_table.da = desc.da;
-	hdev->ifaceq_table.size = IFACEQ_TABLE_SIZE;
-	offset = hdev->ifaceq_table.size;
+	hdev->ifaceq_table = desc;
+	offset = IFACEQ_TABLE_SIZE;
 
 	for (i = 0; i < IFACEQ_NUM; i++) {
 		queue = &hdev->queues[i];
@@ -755,9 +753,7 @@ static int venus_interface_queues_init(struct venus_hfi_device *hdev)
 	if (ret) {
 		hdev->sfr.da = 0;
 	} else {
-		hdev->sfr.da = desc.da;
-		hdev->sfr.kva = desc.kva;
-		hdev->sfr.size = ALIGNED_SFR_SIZE;
+		hdev->sfr = desc;
 		sfr = hdev->sfr.kva;
 		sfr->buf_size = ALIGNED_SFR_SIZE;
 	}
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH 2/2] media: venus: venc: fix bytesused v4l2_plane field
  2017-10-09 12:24 [PATCH 1/2] media: venus: fix wrong size on dma_free Stanimir Varbanov
@ 2017-10-09 12:24 ` Stanimir Varbanov
  2017-10-09 12:31   ` Hans Verkuil
                     ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Stanimir Varbanov @ 2017-10-09 12:24 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Nicolas Dufresne, linux-media, linux-kernel, linux-arm-msm,
	Stanimir Varbanov

This fixes wrongly filled bytesused field of v4l2_plane structure
by include data_offset in the plane, Also fill data_offset and
bytesused for capture type of buffers only.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 drivers/media/platform/qcom/venus/venc.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
index 6f123a387cf9..9445ad492966 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -963,15 +963,16 @@ static void venc_buf_done(struct venus_inst *inst, unsigned int buf_type,
 	if (!vbuf)
 		return;
 
-	vb = &vbuf->vb2_buf;
-	vb->planes[0].bytesused = bytesused;
-	vb->planes[0].data_offset = data_offset;
-
 	vbuf->flags = flags;
 
 	if (type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
+		vb = &vbuf->vb2_buf;
+		vb2_set_plane_payload(vb, 0, bytesused + data_offset);
+		vb->planes[0].data_offset = data_offset;
 		vb->timestamp = timestamp_us * NSEC_PER_USEC;
 		vbuf->sequence = inst->sequence_cap++;
+
+		WARN_ON(vb2_get_plane_payload(vb, 0) > vb2_plane_size(vb, 0));
 	} else {
 		vbuf->sequence = inst->sequence_out++;
 	}
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] media: venus: venc: fix bytesused v4l2_plane field
  2017-10-09 12:24 ` [PATCH 2/2] media: venus: venc: fix bytesused v4l2_plane field Stanimir Varbanov
@ 2017-10-09 12:31   ` Hans Verkuil
  2017-10-09 22:17     ` Stanimir Varbanov
  2017-10-09 23:41   ` Nicolas Dufresne
  2017-10-10  7:52   ` [PATCH v2] " Stanimir Varbanov
  2 siblings, 1 reply; 6+ messages in thread
From: Hans Verkuil @ 2017-10-09 12:31 UTC (permalink / raw)
  To: Stanimir Varbanov
  Cc: Nicolas Dufresne, linux-media, linux-kernel, linux-arm-msm

On 09/10/17 14:24, Stanimir Varbanov wrote:
> This fixes wrongly filled bytesused field of v4l2_plane structure
> by include data_offset in the plane, Also fill data_offset and
> bytesused for capture type of buffers only.
> 
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/venc.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
> index 6f123a387cf9..9445ad492966 100644
> --- a/drivers/media/platform/qcom/venus/venc.c
> +++ b/drivers/media/platform/qcom/venus/venc.c
> @@ -963,15 +963,16 @@ static void venc_buf_done(struct venus_inst *inst, unsigned int buf_type,
>  	if (!vbuf)
>  		return;
>  
> -	vb = &vbuf->vb2_buf;
> -	vb->planes[0].bytesused = bytesused;
> -	vb->planes[0].data_offset = data_offset;
> -
>  	vbuf->flags = flags;
>  
>  	if (type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
> +		vb = &vbuf->vb2_buf;
> +		vb2_set_plane_payload(vb, 0, bytesused + data_offset);
> +		vb->planes[0].data_offset = data_offset;
>  		vb->timestamp = timestamp_us * NSEC_PER_USEC;
>  		vbuf->sequence = inst->sequence_cap++;
> +
> +		WARN_ON(vb2_get_plane_payload(vb, 0) > vb2_plane_size(vb, 0));

It's good to have this, but this really should never happen. Because if it is,
then you'll have a memory overwrite. I hope the DMA engine will prevent this?

Just wondering how this works.

The patch looks good otherwise, but that WARN_ON is a bit scary.

Regards,

	Hans

>  	} else {
>  		vbuf->sequence = inst->sequence_out++;
>  	}
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] media: venus: venc: fix bytesused v4l2_plane field
  2017-10-09 12:31   ` Hans Verkuil
@ 2017-10-09 22:17     ` Stanimir Varbanov
  0 siblings, 0 replies; 6+ messages in thread
From: Stanimir Varbanov @ 2017-10-09 22:17 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: Nicolas Dufresne, linux-media, linux-kernel, linux-arm-msm

Hans,

On  9.10.2017 15:31, Hans Verkuil wrote:
> On 09/10/17 14:24, Stanimir Varbanov wrote:
>> This fixes wrongly filled bytesused field of v4l2_plane structure
>> by include data_offset in the plane, Also fill data_offset and
>> bytesused for capture type of buffers only.
>>
>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>> ---
>>   drivers/media/platform/qcom/venus/venc.c | 9 +++++----
>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
>> index 6f123a387cf9..9445ad492966 100644
>> --- a/drivers/media/platform/qcom/venus/venc.c
>> +++ b/drivers/media/platform/qcom/venus/venc.c
>> @@ -963,15 +963,16 @@ static void venc_buf_done(struct venus_inst *inst, unsigned int buf_type,
>>   	if (!vbuf)
>>   		return;
>>   
>> -	vb = &vbuf->vb2_buf;
>> -	vb->planes[0].bytesused = bytesused;
>> -	vb->planes[0].data_offset = data_offset;
>> -
>>   	vbuf->flags = flags;
>>   
>>   	if (type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
>> +		vb = &vbuf->vb2_buf;
>> +		vb2_set_plane_payload(vb, 0, bytesused + data_offset);
>> +		vb->planes[0].data_offset = data_offset;
>>   		vb->timestamp = timestamp_us * NSEC_PER_USEC;
>>   		vbuf->sequence = inst->sequence_cap++;
>> +
>> +		WARN_ON(vb2_get_plane_payload(vb, 0) > vb2_plane_size(vb, 0));
> 
> It's good to have this, but this really should never happen. Because if it is,
> then you'll have a memory overwrite. I hope the DMA engine will prevent this? >
> Just wondering how this works.
> 
> The patch looks good otherwise, but that WARN_ON is a bit scary.

Infact it is not so scary as it looks like, the IOMMU will catch this 
and generate a fault. So most probably we will never come to the WARN, 
thus the warning is pointless so will delete it.

regards,
Stan

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] media: venus: venc: fix bytesused v4l2_plane field
  2017-10-09 12:24 ` [PATCH 2/2] media: venus: venc: fix bytesused v4l2_plane field Stanimir Varbanov
  2017-10-09 12:31   ` Hans Verkuil
@ 2017-10-09 23:41   ` Nicolas Dufresne
  2017-10-10  7:52   ` [PATCH v2] " Stanimir Varbanov
  2 siblings, 0 replies; 6+ messages in thread
From: Nicolas Dufresne @ 2017-10-09 23:41 UTC (permalink / raw)
  To: Stanimir Varbanov, Hans Verkuil; +Cc: linux-media, linux-kernel, linux-arm-msm

I confirm this works properly now. This was tested with GStreamer with
the following command:

  gst-launch-1.0 videotestsrc ! v4l2vp8enc ! v4l2vp8dec ! kmssink

And the following patch, which is work in progress to implement
data_offset.

  https://gitlab.collabora.com/nicolas/gst-plugins-good/commit/aaedee9a37e396657568a70fc0110375e14fb4fa

Le lundi 09 octobre 2017 à 15:24 +0300, Stanimir Varbanov a écrit :
> This fixes wrongly filled bytesused field of v4l2_plane structure
> by include data_offset in the plane, Also fill data_offset and
> bytesused for capture type of buffers only.
> 
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>

Tested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

> ---
>  drivers/media/platform/qcom/venus/venc.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
> index 6f123a387cf9..9445ad492966 100644
> --- a/drivers/media/platform/qcom/venus/venc.c
> +++ b/drivers/media/platform/qcom/venus/venc.c
> @@ -963,15 +963,16 @@ static void venc_buf_done(struct venus_inst *inst, unsigned int buf_type,
>  	if (!vbuf)
>  		return;
>  
> -	vb = &vbuf->vb2_buf;
> -	vb->planes[0].bytesused = bytesused;
> -	vb->planes[0].data_offset = data_offset;
> -
>  	vbuf->flags = flags;
>  
>  	if (type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
> +		vb = &vbuf->vb2_buf;
> +		vb2_set_plane_payload(vb, 0, bytesused + data_offset);
> +		vb->planes[0].data_offset = data_offset;
>  		vb->timestamp = timestamp_us * NSEC_PER_USEC;
>  		vbuf->sequence = inst->sequence_cap++;
> +
> +		WARN_ON(vb2_get_plane_payload(vb, 0) > vb2_plane_size(vb, 0));
>  	} else {
>  		vbuf->sequence = inst->sequence_out++;
>  	}

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH v2] media: venus: venc: fix bytesused v4l2_plane field
  2017-10-09 12:24 ` [PATCH 2/2] media: venus: venc: fix bytesused v4l2_plane field Stanimir Varbanov
  2017-10-09 12:31   ` Hans Verkuil
  2017-10-09 23:41   ` Nicolas Dufresne
@ 2017-10-10  7:52   ` Stanimir Varbanov
  2 siblings, 0 replies; 6+ messages in thread
From: Stanimir Varbanov @ 2017-10-10  7:52 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Nicolas Dufresne, linux-media, linux-kernel, linux-arm-msm,
	Stanimir Varbanov

This fixes wrongly filled bytesused field of v4l2_plane structure
by include data_offset in the plane, Also fill data_offset and
bytesused for capture type of buffers only.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
v2: Just delete pointless WARN_ON.

 drivers/media/platform/qcom/venus/venc.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
index 6f123a387cf9..3fcf0e9b7b29 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -963,13 +963,12 @@ static void venc_buf_done(struct venus_inst *inst, unsigned int buf_type,
 	if (!vbuf)
 		return;
 
-	vb = &vbuf->vb2_buf;
-	vb->planes[0].bytesused = bytesused;
-	vb->planes[0].data_offset = data_offset;
-
 	vbuf->flags = flags;
 
 	if (type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
+		vb = &vbuf->vb2_buf;
+		vb2_set_plane_payload(vb, 0, bytesused + data_offset);
+		vb->planes[0].data_offset = data_offset;
 		vb->timestamp = timestamp_us * NSEC_PER_USEC;
 		vbuf->sequence = inst->sequence_cap++;
 	} else {
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-10-10  7:53 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-09 12:24 [PATCH 1/2] media: venus: fix wrong size on dma_free Stanimir Varbanov
2017-10-09 12:24 ` [PATCH 2/2] media: venus: venc: fix bytesused v4l2_plane field Stanimir Varbanov
2017-10-09 12:31   ` Hans Verkuil
2017-10-09 22:17     ` Stanimir Varbanov
2017-10-09 23:41   ` Nicolas Dufresne
2017-10-10  7:52   ` [PATCH v2] " Stanimir Varbanov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.