Message ID | 20230607084901.28021-1-yunfei.dong@mediatek.com (mailing list archive) |
---|---|
Headers | show |
Series | media: mediatek: vcodec: separate encoder and decoder | expand |
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@mediatek.com> > > --- > > .../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. 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...
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@mediatek.com> > > > --- > > > .../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. 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...
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@mediatek.com> >>>> --- >>>> .../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. 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... >
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@mediatek.com> > > > > > --- > > > > > .../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 > > 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... > > >
Hi AngeloGioacchino, How do you think about Nicolas's suggestion? 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@mediatek.com> > > > > > > --- > > > > > > .../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... > > > > > >
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@mediatek.com> >>>>>>> --- >>>>>>> .../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... >>>> >>> >>