Re: [PATCH v2,04/10] media: mediatek: vcodec: remove the dependency of debug log
From: AngeloGioacchino Del Regno
Date: Wed Jun 14 2023 - 07:50:28 EST
Il 14/06/23 11:17, Yunfei Dong (董云飞) ha scritto:
Hi AngeloGioacchino,
How do you think about Nicolas's suggestion?
Please don't top-post!
Nicolas' suggestion looks good. Please go on.
P.S.: Sorry for the late reply.
Cheers,
Angelo
On Thu, 2023-06-08 at 11:17 -0400, Nicolas Dufresne wrote:
External email : Please do not click links or open attachments until
you have verified the sender or the content.
Le jeudi 08 juin 2023 à 16:06 +0200, AngeloGioacchino Del Regno a
écrit :
Il 08/06/23 15:11, Nicolas Dufresne ha scritto:
Le jeudi 08 juin 2023 à 07:27 +0000, Yunfei Dong (董云飞) a écrit :
Hi Nicolas,
Thanks for your review.
On Wed, 2023-06-07 at 21:41 -0400, Nicolas Dufresne wrote:
External email : Please do not click links or open
attachments until
you have verified the sender or the content.
Hi Yunfei,
Le mercredi 07 juin 2023 à 16:48 +0800, Yunfei Dong a écrit :
'mtk_vcodec_debug' and 'mtk_vcodec_err' depends on
'mtk_vcodec_ctx'
to get the index of each instance, using the index directly
instead
of with 'mtk_vcodec_ctx'.
Signed-off-by: Yunfei Dong <yunfei.dong@xxxxxxxxxxxx>
---
.../mediatek/vcodec/mtk_vcodec_util.h | 26 ++-
.../vcodec/vdec/vdec_av1_req_lat_if.c | 105
+++++++-----
.../mediatek/vcodec/vdec/vdec_h264_if.c | 62 ++++-
--
.../mediatek/vcodec/vdec/vdec_h264_req_if.c | 39 +++--
.../vcodec/vdec/vdec_h264_req_multi_if.c | 80
+++++----
.../vcodec/vdec/vdec_hevc_req_multi_if.c | 67 ++++-
---
.../mediatek/vcodec/vdec/vdec_vp8_if.c | 54 ++++-
--
.../mediatek/vcodec/vdec/vdec_vp8_req_if.c | 46 +++
---
.../mediatek/vcodec/vdec/vdec_vp9_if.c | 152
++++++++++--
------
.../vcodec/vdec/vdec_vp9_req_lat_if.c | 84
++++++----
.../platform/mediatek/vcodec/vdec_vpu_if.c | 59 ++++-
--
.../mediatek/vcodec/venc/venc_h264_if.c | 86
+++++-----
.../mediatek/vcodec/venc/venc_vp8_if.c | 48 +++
---
.../platform/mediatek/vcodec/venc_vpu_if.c | 64 ++++-
---
14 files changed, 565 insertions(+), 407 deletions(-)
diff --git
a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.h
b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.h
index ecb0bdf3a4f4..ddc12c3e2983 100644
---
a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.h
+++
b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.h
@@ -31,9 +31,8 @@ struct mtk_vcodec_dev;
#define mtk_v4l2_err(fmt, args...) \
pr_err("[MTK_V4L2][ERROR] " fmt "\n", ##args)
-#define mtk_vcodec_err(h, fmt, args...)\
-pr_err("[MTK_VCODEC][ERROR][%d]: " fmt "\n",\
- ((struct mtk_vcodec_ctx *)(h)->ctx)->id, ##args)
+#define mtk_vcodec_err(plat_dev, inst_id, fmt,
args...) \
+dev_err(&(plat_dev)->dev, "[MTK_VCODEC][ERROR][%d]: " fmt
"\n",
inst_id, ##args)
#if defined(CONFIG_DEBUG_FS)
extern int mtk_v4l2_dbg_level;
@@ -46,27 +45,24 @@ extern int mtk_vcodec_dbg;
__func__, __LINE__, ##args); \
} while (0)
-#define mtk_vcodec_debug(h, fmt,
args...) \
-do { \
-if (mtk_vcodec_dbg) \
-dev_dbg(&(((struct mtk_vcodec_ctx *)(h)->ctx)->dev-
plat_dev-
dev), \
-"[MTK_VCODEC][%d]: %s, %d " fmt
"\n", \
-((struct mtk_vcodec_ctx *)(h)->ctx)-
id, \
-__func__, __LINE__,
##args); \
+#define mtk_vcodec_debug(plat_dev, inst_id, fmt,
args...) \
+do
{
\
+if
(mtk_vcodec_dbg)
\
+dev_dbg(&(plat_dev)->dev, "[MTK_VCODEC][%d]: %s, %d " fmt
"\n", \
At least in this patch, you systematically pass plat_dev as
<something>->ctx->dev->plat_dev, which is quite long and
verbose, any
reason we
can't just pass that <something> here ? We can follow the
same
structure path
for both encoder/decoder ?
In order to separate encode and decoder, need to define two
different
struct mtk_vcodec_dec_ctx and struct mtk_vcodec_enc_ctx.
struct mtk_vcodec_ctx won't be used again, need to use platform
device
to print dev_dbg and dev_err.
encoder and decoder using the same interface to print log
message.
Just a reminder, I'm just making suggestions, there is no strict
action required
here other then a discussion to try and make the logging a bit
more light.
My points was that C macros don't care about types, so if you
keep the path to
the platform device the same (ctx->dev->plat_dev), you could just
pass the ctx
as argument. What I don't know though myself, is if this is
actually feasible in
all code path, but considering you had access to the instance
previously, I
thought it should.
One macro used to access two different structures?
Please, no.
Its up to you. I do think this is an empty statement. Still believe
we avoid
this code "deterioration". One can always be creative to workaround
your
concerns.
struct base_ctx {
struct dev dev;
}
struct enc_ctx {
struct base_ctx;
...
}
struct src_ctx {
...
}
But this is in no way more safe then a naming convention, this is
macro calls,
its not typed.
Nicolas
In order to speed up the upstream progress, maybe we can discuss it in
chat.
Best Reagrds,
Yunfei Dong
Regards,
Angelo
regards,
Nicolas
Best Regards,
Yunfei Dong
+inst_id, __func__, __LINE__, ##args); \
} while (0)
#else
#define mtk_v4l2_debug(level, fmt, args...) pr_debug(fmt,
##args)
-#define mtk_vcodec_debug(h, fmt, args...)\
-pr_debug("[MTK_VCODEC][%d]: " fmt "\n",\
...snip...