| Message ID | 1530517447-29296-1-git-send-email-vgarodia@codeaurora.org (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Delegated to: | Hans Verkuil |
| Headers |
Received: from vger.kernel.org ([209.132.180.67]) by www.linuxtv.org with esmtp (Exim 4.84_2) (envelope-from <linux-media-owner@vger.kernel.org>) id 1fZtVh-0001MQ-DP; Mon, 02 Jul 2018 07:44:33 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933366AbeGBHoU (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Mon, 2 Jul 2018 03:44:20 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:38464 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933055AbeGBHoS (ORCPT <rfc822; linux-media@vger.kernel.org>); Mon, 2 Jul 2018 03:44:18 -0400 Received: by smtp.codeaurora.org (Postfix, from userid 1000) id 8B33660B13; Mon, 2 Jul 2018 07:44:17 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org; s=default; t=1530517457; bh=kwjdomwJ8eCm4yLXX3FoyXKbJRyqrHN+jIm6LRWq8rI=; h=From:To:Cc:Subject:Date:From; b=DfxRtaYiSLQgkJd9BJRKBvZXkazTqKwXb94MWS3gvpHl3W+UllojrNzj/dsi7i/eX k/K6XIBcdUjB205xbX9WAXB4G4auonHUgVCs2FiEzNTB3JY/xj3hx6NV061dBfyUwh uEJfT2sEtT81Ct9hLuXa00S2lz85+vc5B7nZYRc8= X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on pdx-caf-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.8 required=2.0 tests=ALL_TRUSTED,BAYES_00, DKIM_SIGNED, T_DKIM_INVALID autolearn=no autolearn_force=no version=3.4.0 Received: from vgarodia-linux.qualcomm.com (blr-c-bdr-fw-01_globalnat_allzones-outside.qualcomm.com [103.229.19.19]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: vgarodia@smtp.codeaurora.org) by smtp.codeaurora.org (Postfix) with ESMTPSA id 23ADA602A7; Mon, 2 Jul 2018 07:44:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org; s=default; t=1530517457; bh=kwjdomwJ8eCm4yLXX3FoyXKbJRyqrHN+jIm6LRWq8rI=; h=From:To:Cc:Subject:Date:From; b=DfxRtaYiSLQgkJd9BJRKBvZXkazTqKwXb94MWS3gvpHl3W+UllojrNzj/dsi7i/eX k/K6XIBcdUjB205xbX9WAXB4G4auonHUgVCs2FiEzNTB3JY/xj3hx6NV061dBfyUwh uEJfT2sEtT81Ct9hLuXa00S2lz85+vc5B7nZYRc8= DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 23ADA602A7 Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none (p=none dis=none) header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=none smtp.mailfrom=vgarodia@codeaurora.org From: Vikash Garodia <vgarodia@codeaurora.org> To: stanimir.varbanov@linaro.org Cc: linux-media@vger.kernel.org, linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, acourbot@chromium.org, vgarodia@codeaurora.org Subject: [PATCH] venus: vdec: fix decoded data size Date: Mon, 2 Jul 2018 13:14:07 +0530 Message-Id: <1530517447-29296-1-git-send-email-vgarodia@codeaurora.org> X-Mailer: git-send-email 1.9.1 Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org |
Commit Message
Vikash Garodia
July 2, 2018, 7:44 a.m. UTC
Exisiting code returns the max of the decoded
size and buffer size. It turns out that buffer
size is always greater due to hardware alignment
requirement. As a result, payload size given to
client is incorrect. This change ensures that
the bytesused is assigned to actual payload size.
Change-Id: Ie6f3429c0cb23f682544748d181fa4fa63ca2e28
Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
---
drivers/media/platform/qcom/venus/vdec.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Comments
Hi Vikash, On 2.07.2018 10:44, Vikash Garodia wrote: > Exisiting code returns the max of the decoded > size and buffer size. It turns out that buffer > size is always greater due to hardware alignment > requirement. As a result, payload size given to > client is incorrect. This change ensures that > the bytesused is assigned to actual payload size. > > Change-Id: Ie6f3429c0cb23f682544748d181fa4fa63ca2e28 > Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org> > --- > drivers/media/platform/qcom/venus/vdec.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c > index d079aeb..ada1d2f 100644 > --- a/drivers/media/platform/qcom/venus/vdec.c > +++ b/drivers/media/platform/qcom/venus/vdec.c > @@ -890,7 +890,7 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type, > > vb = &vbuf->vb2_buf; > vb->planes[0].bytesused = > - max_t(unsigned int, opb_sz, bytesused); > + min_t(unsigned int, opb_sz, bytesused); I cannot remember the exact reason to make it this way, might be an issue with vp8 or some mpeg2/4 on v1 which I workaround by this way. I'll test the patch on v1 & v3 and come back. regards, Stan
Hi Vikash, On 07/02/2018 10:44 AM, Vikash Garodia wrote: > Exisiting code returns the max of the decoded > size and buffer size. It turns out that buffer > size is always greater due to hardware alignment > requirement. As a result, payload size given to > client is incorrect. This change ensures that > the bytesused is assigned to actual payload size. > > Change-Id: Ie6f3429c0cb23f682544748d181fa4fa63ca2e28 > Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org> > --- > drivers/media/platform/qcom/venus/vdec.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c > index d079aeb..ada1d2f 100644 > --- a/drivers/media/platform/qcom/venus/vdec.c > +++ b/drivers/media/platform/qcom/venus/vdec.c > @@ -890,7 +890,7 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type, > > vb = &vbuf->vb2_buf; > vb->planes[0].bytesused = > - max_t(unsigned int, opb_sz, bytesused); > + min_t(unsigned int, opb_sz, bytesused); Most probably my intension was to avoid bytesused == 0, but that is allowed from v4l2 driver -> userspace direction Could you drop min/max_t macros at all and use bytesused directly i.e. vb2_set_plane_payload(vb, 0, bytesused)
Le mercredi 18 juillet 2018 à 14:31 +0300, Stanimir Varbanov a écrit : > Hi Vikash, > > On 07/02/2018 10:44 AM, Vikash Garodia wrote: > > Exisiting code returns the max of the decoded > > size and buffer size. It turns out that buffer > > size is always greater due to hardware alignment > > requirement. As a result, payload size given to > > client is incorrect. This change ensures that > > the bytesused is assigned to actual payload size. > > > > Change-Id: Ie6f3429c0cb23f682544748d181fa4fa63ca2e28 > > Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org> > > --- > > drivers/media/platform/qcom/venus/vdec.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/media/platform/qcom/venus/vdec.c > > b/drivers/media/platform/qcom/venus/vdec.c > > index d079aeb..ada1d2f 100644 > > --- a/drivers/media/platform/qcom/venus/vdec.c > > +++ b/drivers/media/platform/qcom/venus/vdec.c > > @@ -890,7 +890,7 @@ static void vdec_buf_done(struct venus_inst > > *inst, unsigned int buf_type, > > > > vb = &vbuf->vb2_buf; > > vb->planes[0].bytesused = > > - max_t(unsigned int, opb_sz, bytesused); > > + min_t(unsigned int, opb_sz, bytesused); > > Most probably my intension was to avoid bytesused == 0, but that is > allowed from v4l2 driver -> userspace direction It remains bad practice since it was used by decoders to indicate the last buffer. Some userspace (some GStreamer versions) will stop working if you start returning 0. > > Could you drop min/max_t macros at all and use bytesused directly > i.e. > > vb2_set_plane_payload(vb, 0, bytesused)
Hi, On 07/18/2018 04:26 PM, Nicolas Dufresne wrote: > Le mercredi 18 juillet 2018 à 14:31 +0300, Stanimir Varbanov a écrit : >> Hi Vikash, >> >> On 07/02/2018 10:44 AM, Vikash Garodia wrote: >>> Exisiting code returns the max of the decoded >>> size and buffer size. It turns out that buffer >>> size is always greater due to hardware alignment >>> requirement. As a result, payload size given to >>> client is incorrect. This change ensures that >>> the bytesused is assigned to actual payload size. >>> >>> Change-Id: Ie6f3429c0cb23f682544748d181fa4fa63ca2e28 >>> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org> >>> --- >>> drivers/media/platform/qcom/venus/vdec.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/media/platform/qcom/venus/vdec.c >>> b/drivers/media/platform/qcom/venus/vdec.c >>> index d079aeb..ada1d2f 100644 >>> --- a/drivers/media/platform/qcom/venus/vdec.c >>> +++ b/drivers/media/platform/qcom/venus/vdec.c >>> @@ -890,7 +890,7 @@ static void vdec_buf_done(struct venus_inst >>> *inst, unsigned int buf_type, >>> >>> vb = &vbuf->vb2_buf; >>> vb->planes[0].bytesused = >>> - max_t(unsigned int, opb_sz, bytesused); >>> + min_t(unsigned int, opb_sz, bytesused); >> >> Most probably my intension was to avoid bytesused == 0, but that is >> allowed from v4l2 driver -> userspace direction > > It remains bad practice since it was used by decoders to indicate the > last buffer. Some userspace (some GStreamer versions) will stop working > if you start returning 0. I think it is legal v4l2 driver to return bytesused = 0 when userspace issues streamoff on both queues before EOS, no? Simply because the capture buffers are empty.
On 07/18/2018 04:37 PM, Stanimir Varbanov wrote: > Hi, > > On 07/18/2018 04:26 PM, Nicolas Dufresne wrote: >> Le mercredi 18 juillet 2018 à 14:31 +0300, Stanimir Varbanov a écrit : >>> Hi Vikash, >>> >>> On 07/02/2018 10:44 AM, Vikash Garodia wrote: >>>> Exisiting code returns the max of the decoded >>>> size and buffer size. It turns out that buffer >>>> size is always greater due to hardware alignment >>>> requirement. As a result, payload size given to >>>> client is incorrect. This change ensures that >>>> the bytesused is assigned to actual payload size. >>>> >>>> Change-Id: Ie6f3429c0cb23f682544748d181fa4fa63ca2e28 >>>> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org> >>>> --- >>>> drivers/media/platform/qcom/venus/vdec.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/media/platform/qcom/venus/vdec.c >>>> b/drivers/media/platform/qcom/venus/vdec.c >>>> index d079aeb..ada1d2f 100644 >>>> --- a/drivers/media/platform/qcom/venus/vdec.c >>>> +++ b/drivers/media/platform/qcom/venus/vdec.c >>>> @@ -890,7 +890,7 @@ static void vdec_buf_done(struct venus_inst >>>> *inst, unsigned int buf_type, >>>> >>>> vb = &vbuf->vb2_buf; >>>> vb->planes[0].bytesused = >>>> - max_t(unsigned int, opb_sz, bytesused); >>>> + min_t(unsigned int, opb_sz, bytesused); >>> >>> Most probably my intension was to avoid bytesused == 0, but that is >>> allowed from v4l2 driver -> userspace direction >> >> It remains bad practice since it was used by decoders to indicate the >> last buffer. Some userspace (some GStreamer versions) will stop working >> if you start returning 0. > > I think it is legal v4l2 driver to return bytesused = 0 when userspace > issues streamoff on both queues before EOS, no? Simply because the > capture buffers are empty. > Going through some of the older pending patches I found this one: So is this patch right or wrong? It's not clear to me. Regards, Hans
Hi Hans, On 09/17/2018 01:00 PM, Hans Verkuil wrote: > On 07/18/2018 04:37 PM, Stanimir Varbanov wrote: >> Hi, >> >> On 07/18/2018 04:26 PM, Nicolas Dufresne wrote: >>> Le mercredi 18 juillet 2018 à 14:31 +0300, Stanimir Varbanov a écrit : >>>> Hi Vikash, >>>> >>>> On 07/02/2018 10:44 AM, Vikash Garodia wrote: >>>>> Exisiting code returns the max of the decoded >>>>> size and buffer size. It turns out that buffer >>>>> size is always greater due to hardware alignment >>>>> requirement. As a result, payload size given to >>>>> client is incorrect. This change ensures that >>>>> the bytesused is assigned to actual payload size. >>>>> >>>>> Change-Id: Ie6f3429c0cb23f682544748d181fa4fa63ca2e28 >>>>> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org> >>>>> --- >>>>> drivers/media/platform/qcom/venus/vdec.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/media/platform/qcom/venus/vdec.c >>>>> b/drivers/media/platform/qcom/venus/vdec.c >>>>> index d079aeb..ada1d2f 100644 >>>>> --- a/drivers/media/platform/qcom/venus/vdec.c >>>>> +++ b/drivers/media/platform/qcom/venus/vdec.c >>>>> @@ -890,7 +890,7 @@ static void vdec_buf_done(struct venus_inst >>>>> *inst, unsigned int buf_type, >>>>> >>>>> vb = &vbuf->vb2_buf; >>>>> vb->planes[0].bytesused = >>>>> - max_t(unsigned int, opb_sz, bytesused); >>>>> + min_t(unsigned int, opb_sz, bytesused); >>>> >>>> Most probably my intension was to avoid bytesused == 0, but that is >>>> allowed from v4l2 driver -> userspace direction >>> >>> It remains bad practice since it was used by decoders to indicate the >>> last buffer. Some userspace (some GStreamer versions) will stop working >>> if you start returning 0. >> >> I think it is legal v4l2 driver to return bytesused = 0 when userspace >> issues streamoff on both queues before EOS, no? Simply because the >> capture buffers are empty. >> > > Going through some of the older pending patches I found this one: > > So is this patch right or wrong? I'm not sure either, let's not applying it for now (if Nicolas is right this will break gstreamer plugin).
On 09/17/2018 04:30 PM, Stanimir Varbanov wrote: > Hi Hans, > > On 09/17/2018 01:00 PM, Hans Verkuil wrote: >> On 07/18/2018 04:37 PM, Stanimir Varbanov wrote: >>> Hi, >>> >>> On 07/18/2018 04:26 PM, Nicolas Dufresne wrote: >>>> Le mercredi 18 juillet 2018 à 14:31 +0300, Stanimir Varbanov a écrit : >>>>> Hi Vikash, >>>>> >>>>> On 07/02/2018 10:44 AM, Vikash Garodia wrote: >>>>>> Exisiting code returns the max of the decoded >>>>>> size and buffer size. It turns out that buffer >>>>>> size is always greater due to hardware alignment >>>>>> requirement. As a result, payload size given to >>>>>> client is incorrect. This change ensures that >>>>>> the bytesused is assigned to actual payload size. >>>>>> >>>>>> Change-Id: Ie6f3429c0cb23f682544748d181fa4fa63ca2e28 >>>>>> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org> >>>>>> --- >>>>>> drivers/media/platform/qcom/venus/vdec.c | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/media/platform/qcom/venus/vdec.c >>>>>> b/drivers/media/platform/qcom/venus/vdec.c >>>>>> index d079aeb..ada1d2f 100644 >>>>>> --- a/drivers/media/platform/qcom/venus/vdec.c >>>>>> +++ b/drivers/media/platform/qcom/venus/vdec.c >>>>>> @@ -890,7 +890,7 @@ static void vdec_buf_done(struct venus_inst >>>>>> *inst, unsigned int buf_type, >>>>>> >>>>>> vb = &vbuf->vb2_buf; >>>>>> vb->planes[0].bytesused = >>>>>> - max_t(unsigned int, opb_sz, bytesused); >>>>>> + min_t(unsigned int, opb_sz, bytesused); >>>>> >>>>> Most probably my intension was to avoid bytesused == 0, but that is >>>>> allowed from v4l2 driver -> userspace direction >>>> >>>> It remains bad practice since it was used by decoders to indicate the >>>> last buffer. Some userspace (some GStreamer versions) will stop working >>>> if you start returning 0. >>> >>> I think it is legal v4l2 driver to return bytesused = 0 when userspace >>> issues streamoff on both queues before EOS, no? Simply because the >>> capture buffers are empty. >>> >> >> Going through some of the older pending patches I found this one: >> >> So is this patch right or wrong? > > I'm not sure either, let's not applying it for now (if Nicolas is right > this will break gstreamer plugin). > OK, I marked this as Rejected. If you change your mind it can be reposted :-) Regards, Hans
diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c index d079aeb..ada1d2f 100644 --- a/drivers/media/platform/qcom/venus/vdec.c +++ b/drivers/media/platform/qcom/venus/vdec.c @@ -890,7 +890,7 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type, vb = &vbuf->vb2_buf; vb->planes[0].bytesused = - max_t(unsigned int, opb_sz, bytesused); + min_t(unsigned int, opb_sz, bytesused); vb->planes[0].data_offset = data_offset; vb->timestamp = timestamp_us * NSEC_PER_USEC; vbuf->sequence = inst->sequence_cap++;