media: amphion: fix some error related with undefined reference to __divdi3
Commit Message
1. use ns_to_timespec64 instead of division method
2. use timespec64_to_ns instead of custom macro
3. use 'val >> 1' instead of ' val / 2'
4. remove unused custom macro
5. don't modify minus timestamp
6. remove some unused debug timestamp information
Signed-off-by: Ming Qian <ming.qian@nxp.com>
Reported-by: kernel test robot <lkp@intel.com>
---
drivers/media/platform/amphion/vdec.c | 35 --------------------
drivers/media/platform/amphion/vpu_helpers.h | 3 --
drivers/media/platform/amphion/vpu_malone.c | 24 ++++++++------
drivers/media/platform/amphion/vpu_v4l2.c | 5 +--
drivers/media/platform/amphion/vpu_windsor.c | 18 +++++-----
5 files changed, 22 insertions(+), 63 deletions(-)
Comments
From: Ming Qian
> Sent: 09 March 2022 05:02
...
> 3. use 'val >> 1' instead of ' val / 2'
The compiler should do that anyway.
Especially for unsigned values.
And it has the wrong (different) rounding for negative values.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
> -----Original Message-----
> From: David Laight [mailto:David.Laight@ACULAB.COM]
> Sent: Wednesday, March 9, 2022 9:27 PM
> To: Ming Qian <ming.qian@nxp.com>; mchehab@kernel.org;
> shawnguo@kernel.org; robh+dt@kernel.org; s.hauer@pengutronix.de
> Cc: hverkuil-cisco@xs4all.nl; kernel@pengutronix.de; festevam@gmail.com;
> dl-linux-imx <linux-imx@nxp.com>; Aisheng Dong <aisheng.dong@nxp.com>;
> linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Subject: [EXT] RE: [PATCH] media: amphion: fix some error related with
> undefined reference to __divdi3
>
> Caution: EXT Email
>
> From: Ming Qian
> > Sent: 09 March 2022 05:02
> ...
> > 3. use 'val >> 1' instead of ' val / 2'
>
> The compiler should do that anyway.
>
> Especially for unsigned values.
> And it has the wrong (different) rounding for negative values.
>
> David
>
Hi David,
Yes, you are right, if the value is negative, the behavior is wrong.
But here, the value type is u32, so I think it's OK.
Ming
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1
> 1PT, UK Registration No: 1397386 (Wales)
On 2022-03-10 08:36, Ming Qian wrote:
>> -----Original Message-----
>> From: David Laight [mailto:David.Laight@ACULAB.COM]
>> Sent: Wednesday, March 9, 2022 9:27 PM
>> To: Ming Qian <ming.qian@nxp.com>; mchehab@kernel.org;
>> shawnguo@kernel.org; robh+dt@kernel.org; s.hauer@pengutronix.de
>> Cc: hverkuil-cisco@xs4all.nl; kernel@pengutronix.de; festevam@gmail.com;
>> dl-linux-imx <linux-imx@nxp.com>; Aisheng Dong <aisheng.dong@nxp.com>;
>> linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>> Subject: [EXT] RE: [PATCH] media: amphion: fix some error related with
>> undefined reference to __divdi3
>>
>> Caution: EXT Email
>>
>> From: Ming Qian
>>> Sent: 09 March 2022 05:02
>> ...
>>> 3. use 'val >> 1' instead of ' val / 2'
>>
>> The compiler should do that anyway.
>>
>> Especially for unsigned values.
>> And it has the wrong (different) rounding for negative values.
>>
>> David
>>
>
> Hi David,
> Yes, you are right, if the value is negative, the behavior is wrong.
> But here, the value type is u32, so I think it's OK.
Well, it depends on the semantic intent, really. If you're packing a
bitfield which encodes bits 31:1 of some value then a shift is the most
appropriate operation. However if you're literally calculating half of a
value for, say, a 50% threshold level, or the number of 16-bit words
represented by a byte length, then semantically it's a division, so it
should use the divide operator rather than obfuscating it behind a
shift. Constant division is something that even the most basic
optimising compiler should handle with ease.
One more thing that's not the fault of this patch, but stood out in the
context:
@@ -1566,7 +1568,7 @@ static bool vpu_malone_check_ready(struct
vpu_shared_addr *shared, u32 instance)
u32 wptr = desc->wptr;
u32 used = (wptr + size - rptr) % size;
- if (!size || used < size / 2)
+ if (!size || used < (size >> 1))
return true;
return false;
That's not safe: if "size" is 0 then the undefined behaviour has already
happened before the "!size" check is reached. If "size" really can be 0,
then it needs to be checked *before* it is used as a divisor to
calculate "used".
Robin.
> >>
> >> From: Ming Qian
> >>> Sent: 09 March 2022 05:02
> >> ...
> >>> 3. use 'val >> 1' instead of ' val / 2'
> >>
> >> The compiler should do that anyway.
> >>
> >> Especially for unsigned values.
> >> And it has the wrong (different) rounding for negative values.
> >>
> >> David
> >>
> >
> > Hi David,
> > Yes, you are right, if the value is negative, the behavior is wrong.
> > But here, the value type is u32, so I think it's OK.
>
> Well, it depends on the semantic intent, really. If you're packing a bitfield
> which encodes bits 31:1 of some value then a shift is the most appropriate
> operation. However if you're literally calculating half of a value for, say, a 50%
> threshold level, or the number of 16-bit words represented by a byte length,
> then semantically it's a division, so it should use the divide operator rather
> than obfuscating it behind a shift. Constant division is something that even
> the most basic optimising compiler should handle with ease.
>
Hi Robin,
Thanks for the detailed explanation, and I agree with you, I will use " / 2" in the v2 patch as it's indeed calculating half of a value.
> One more thing that's not the fault of this patch, but stood out in the
> context:
>
> @@ -1566,7 +1568,7 @@ static bool vpu_malone_check_ready(struct
> vpu_shared_addr *shared, u32 instance)
> u32 wptr = desc->wptr;
> u32 used = (wptr + size - rptr) % size;
>
> - if (!size || used < size / 2)
> + if (!size || used < (size >> 1))
> return true;
>
> return false;
>
> That's not safe: if "size" is 0 then the undefined behaviour has already
> happened before the "!size" check is reached. If "size" really can be 0, then it
> needs to be checked *before* it is used as a divisor to calculate "used".
>
> Robin.
Yes, it's problem, and Dan has also pointed it, I 'll fix it in another patch.
Ming
@@ -65,9 +65,6 @@ struct vdec_t {
u32 drain;
u32 ts_pre_count;
u32 frame_depth;
- s64 ts_start;
- s64 ts_input;
- s64 timestamp;
};
static const struct vpu_format vdec_formats[] = {
@@ -693,7 +690,6 @@ static void vdec_buf_done(struct vpu_inst *inst, struct vpu_frame_info *frame)
v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_DONE);
vpu_inst_lock(inst);
- vdec->timestamp = frame->timestamp;
vdec->display_frame_count++;
vpu_inst_unlock(inst);
dev_dbg(inst->dev, "[%d] decoded : %d, display : %d, sequence : %d\n",
@@ -713,9 +709,6 @@ static void vdec_stop_done(struct vpu_inst *inst)
vdec->params.end_flag = 0;
vdec->drain = 0;
vdec->ts_pre_count = 0;
- vdec->timestamp = VPU_INVALID_TIMESTAMP;
- vdec->ts_start = VPU_INVALID_TIMESTAMP;
- vdec->ts_input = VPU_INVALID_TIMESTAMP;
vdec->params.frame_count = 0;
vdec->decoded_frame_count = 0;
vdec->display_frame_count = 0;
@@ -1228,7 +1221,6 @@ static int vdec_process_output(struct vpu_inst *inst, struct vb2_buffer *vb)
struct vdec_t *vdec = inst->priv;
struct vb2_v4l2_buffer *vbuf;
struct vpu_rpc_buffer_desc desc;
- s64 timestamp;
u32 free_space;
int ret;
@@ -1252,12 +1244,6 @@ static int vdec_process_output(struct vpu_inst *inst, struct vb2_buffer *vb)
if (free_space < vb2_get_plane_payload(vb, 0) + 0x40000)
return -ENOMEM;
- timestamp = vb->timestamp;
- if (timestamp >= 0 && vdec->ts_start < 0)
- vdec->ts_start = timestamp;
- if (vdec->ts_input < timestamp)
- vdec->ts_input = timestamp;
-
ret = vpu_iface_input_frame(inst, vb);
if (ret < 0)
return -ENOMEM;
@@ -1333,9 +1319,6 @@ static void vdec_abort(struct vpu_inst *inst)
vdec->params.end_flag = 0;
vdec->drain = 0;
vdec->ts_pre_count = 0;
- vdec->timestamp = VPU_INVALID_TIMESTAMP;
- vdec->ts_start = VPU_INVALID_TIMESTAMP;
- vdec->ts_input = VPU_INVALID_TIMESTAMP;
vdec->params.frame_count = 0;
vdec->decoded_frame_count = 0;
vdec->display_frame_count = 0;
@@ -1550,21 +1533,6 @@ static int vdec_get_debug_info(struct vpu_inst *inst, char *str, u32 size, u32 i
vdec->codec_info.frame_rate.numerator,
vdec->codec_info.frame_rate.denominator);
break;
- case 10:
- {
- s64 timestamp = vdec->timestamp;
- s64 ts_start = vdec->ts_start;
- s64 ts_input = vdec->ts_input;
-
- num = scnprintf(str, size, "timestamp = %9lld.%09lld(%9lld.%09lld, %9lld.%09lld)\n",
- timestamp / NSEC_PER_SEC,
- timestamp % NSEC_PER_SEC,
- ts_start / NSEC_PER_SEC,
- ts_start % NSEC_PER_SEC,
- ts_input / NSEC_PER_SEC,
- ts_input % NSEC_PER_SEC);
- }
- break;
default:
break;
}
@@ -1599,9 +1567,6 @@ static void vdec_init(struct file *file)
vdec = inst->priv;
vdec->frame_depth = VDEC_FRAME_DEPTH;
- vdec->timestamp = VPU_INVALID_TIMESTAMP;
- vdec->ts_start = VPU_INVALID_TIMESTAMP;
- vdec->ts_input = VPU_INVALID_TIMESTAMP;
memset(&f, 0, sizeof(f));
f.type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
@@ -11,9 +11,6 @@ struct vpu_pair {
u32 dst;
};
-#define MAKE_TIMESTAMP(s, ns) (((s32)(s) * NSEC_PER_SEC) + (ns))
-#define VPU_INVALID_TIMESTAMP MAKE_TIMESTAMP(-1, 0)
-
int vpu_helper_find_in_array_u8(const u8 *array, u32 size, u32 x);
bool vpu_helper_check_type(struct vpu_inst *inst, u32 type);
const struct vpu_format *vpu_helper_find_format(struct vpu_inst *inst, u32 type, u32 pixelfmt);
@@ -14,6 +14,7 @@
#include <linux/platform_device.h>
#include <linux/delay.h>
#include <linux/rational.h>
+#include <linux/time64.h>
#include <media/videobuf2-v4l2.h>
#include <media/videobuf2-dma-contig.h>
#include <linux/videodev2.h>
@@ -725,9 +726,9 @@ static void vpu_malone_pack_fs_alloc(struct vpu_rpc_event *pkt,
*/
if (fs->luma_addr == fs->chroma_addr)
fs->chroma_addr = fs->luma_addr + fs->luma_size;
- pkt->data[2] = fs->luma_addr + fs->luma_size / 2;
+ pkt->data[2] = fs->luma_addr + (fs->luma_size >> 1);
pkt->data[3] = fs->chroma_addr;
- pkt->data[4] = fs->chroma_addr + fs->chromau_size / 2;
+ pkt->data[4] = fs->chroma_addr + (fs->chromau_size >> 1);
pkt->data[5] = fs->bytesperline;
} else {
pkt->data[2] = fs->luma_size;
@@ -748,14 +749,12 @@ static void vpu_malone_pack_fs_release(struct vpu_rpc_event *pkt,
static void vpu_malone_pack_timestamp(struct vpu_rpc_event *pkt,
struct vpu_ts_info *info)
{
+ struct timespec64 ts = ns_to_timespec64(info->timestamp);
+
pkt->hdr.num = 3;
- if (info->timestamp < 0) {
- pkt->data[0] = (u32)-1;
- pkt->data[1] = 0;
- } else {
- pkt->data[0] = info->timestamp / NSEC_PER_SEC;
- pkt->data[1] = info->timestamp % NSEC_PER_SEC;
- }
+
+ pkt->data[0] = ts.tv_sec;
+ pkt->data[1] = ts.tv_nsec;
pkt->data[2] = info->size;
}
@@ -916,6 +915,8 @@ static void vpu_malone_unpack_rel_frame(struct vpu_rpc_event *pkt,
static void vpu_malone_unpack_buff_rdy(struct vpu_rpc_event *pkt,
struct vpu_dec_pic_info *info)
{
+ struct timespec64 ts = { pkt->data[9], pkt->data[10] };
+
info->id = pkt->data[0];
info->luma = pkt->data[1];
info->stride = pkt->data[3];
@@ -923,7 +924,8 @@ static void vpu_malone_unpack_buff_rdy(struct vpu_rpc_event *pkt,
info->skipped = 1;
else
info->skipped = 0;
- info->timestamp = MAKE_TIMESTAMP(pkt->data[9], pkt->data[10]);
+
+ info->timestamp = timespec64_to_ns(&ts);
}
int vpu_malone_unpack_msg_data(struct vpu_rpc_event *pkt, void *data)
@@ -1566,7 +1568,7 @@ static bool vpu_malone_check_ready(struct vpu_shared_addr *shared, u32 instance)
u32 wptr = desc->wptr;
u32 used = (wptr + size - rptr) % size;
- if (!size || used < size / 2)
+ if (!size || used < (size >> 1))
return true;
return false;
@@ -459,11 +459,8 @@ static void vpu_vb2_buf_queue(struct vb2_buffer *vb)
struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
struct vpu_inst *inst = vb2_get_drv_priv(vb->vb2_queue);
- if (V4L2_TYPE_IS_OUTPUT(vb->type)) {
+ if (V4L2_TYPE_IS_OUTPUT(vb->type))
vbuf->sequence = inst->sequence++;
- if ((s64)vb->timestamp < 0)
- vb->timestamp = VPU_INVALID_TIMESTAMP;
- }
v4l2_m2m_buf_queue(inst->fh.m2m_ctx, vbuf);
vpu_process_output_buffer(inst);
@@ -12,6 +12,7 @@
#include <linux/of_device.h>
#include <linux/of_address.h>
#include <linux/platform_device.h>
+#include <linux/time64.h>
#include <media/videobuf2-v4l2.h>
#include <media/videobuf2-dma-contig.h>
#include "vpu.h"
@@ -682,7 +683,6 @@ static struct vpu_pair windsor_msgs[] = {
int vpu_windsor_pack_cmd(struct vpu_rpc_event *pkt, u32 index, u32 id, void *data)
{
int ret;
- s64 timestamp;
ret = vpu_find_dst_by_src(windsor_cmds, ARRAY_SIZE(windsor_cmds), id);
if (ret < 0)
@@ -691,15 +691,12 @@ int vpu_windsor_pack_cmd(struct vpu_rpc_event *pkt, u32 index, u32 id, void *dat
pkt->hdr.num = 0;
pkt->hdr.index = index;
if (id == VPU_CMD_ID_FRAME_ENCODE) {
+ s64 timestamp = *(s64 *)data;
+ struct timespec64 ts = ns_to_timespec64(timestamp);
+
pkt->hdr.num = 2;
- timestamp = *(s64 *)data;
- if (timestamp < 0) {
- pkt->data[0] = (u32)-1;
- pkt->data[1] = 0;
- } else {
- pkt->data[0] = timestamp / NSEC_PER_SEC;
- pkt->data[1] = timestamp % NSEC_PER_SEC;
- }
+ pkt->data[0] = ts.tv_sec;
+ pkt->data[1] = ts.tv_nsec;
}
return 0;
@@ -714,6 +711,7 @@ static void vpu_windsor_unpack_pic_info(struct vpu_rpc_event *pkt, void *data)
{
struct vpu_enc_pic_info *info = data;
struct windsor_pic_info *windsor = (struct windsor_pic_info *)pkt->data;
+ struct timespec64 ts = { windsor->tv_s, windsor->tv_ns };
info->frame_id = windsor->frame_id;
switch (windsor->pic_type) {
@@ -736,7 +734,7 @@ static void vpu_windsor_unpack_pic_info(struct vpu_rpc_event *pkt, void *data)
info->frame_size = windsor->frame_size;
info->wptr = get_ptr(windsor->str_buff_wptr);
info->crc = windsor->frame_crc;
- info->timestamp = MAKE_TIMESTAMP(windsor->tv_s, windsor->tv_ns);
+ info->timestamp = timespec64_to_ns(&ts);
}
static void vpu_windsor_unpack_mem_req(struct vpu_rpc_event *pkt, void *data)