mbox series

[v2,0/10] media: mediatek: vcodec: separate encoder and decoder

Message ID 20230607084901.28021-1-yunfei.dong@mediatek.com (mailing list archive)
Headers show
Series media: mediatek: vcodec: separate encoder and decoder | expand

Message

Yunfei Dong June 7, 2023, 8:48 a.m. UTC
With the driver more and more complex, encoder and decoder need to add more parameter
in shared struct 'mtk_vcodec_ctx' and 'mtk_vcodec_dev'. Encoder use about 40% and
decoder use 60% parameter. Need to allocate extra unused memory when encoder and decoder
working.

Separate encoder and decoder in different folder and use independent data struct.

patch 1 remove unused parameter.
patch 2~3 align fw and interrupt related interface.
patch 4~5 remove the dependency of debug log
patch 6~7 separate mtk_vcodec_ctx and mtk_vcodec_dev
patch 8 fix unreasonable parameter
patch 9 removed unused header files
patch 10 separate encoder and decoder in different folder
---
Changed from v1:
- Change pr_dbg to dev_dbg for mtk_v4l2_level and mtk_vcodec_dbg for patch 4.
- Change pr_err to dev_err for mtk_v4l2_err and mtk_vcodec_err for patch 5.
- Fix unreasonable parameter for patch 8.
---
Yunfei Dong (10):
  media: mediatek: vcodec: remove unused parameter
  media: mediatek: vcodec: align fw interface
  media: mediatek: vcodec: re-write shared interface
  media: mediatek: vcodec: remove the dependency of debug log
  mediatek: vcodec: replace pr_* with dev_* for v4l2 debug message
  mediatek: vcodec: separate struct mtk_vcodec_ctx
  mediatek: vcodec: separate struct mtk_vcodec_dev
  mediatek: vcodec: fix unreasonable parameter definition and style
  mediatek: vcodec: remove unused include header
  mediatek: vcodec: separete decoder and encoder

 .../media/platform/mediatek/vcodec/Makefile   |  55 +-
 .../platform/mediatek/vcodec/common/Makefile  |  21 +
 .../vcodec/common/mtk_vcodec_com_drv.h        | 147 +++++
 .../vcodec/{ => common}/mtk_vcodec_dbgfs.c    |  57 +-
 .../vcodec/{ => common}/mtk_vcodec_dbgfs.h    |  24 +-
 .../vcodec/{ => common}/mtk_vcodec_fw.c       |  21 +-
 .../vcodec/{ => common}/mtk_vcodec_fw.h       |   8 +-
 .../vcodec/{ => common}/mtk_vcodec_fw_priv.h  |  14 +-
 .../vcodec/{ => common}/mtk_vcodec_fw_scp.c   |  26 +-
 .../vcodec/{ => common}/mtk_vcodec_fw_vpu.c   |  64 +-
 .../mediatek/vcodec/common/mtk_vcodec_intr.c  |  68 +++
 .../vcodec/{ => common}/mtk_vcodec_intr.h     |   6 +-
 .../mediatek/vcodec/common/mtk_vcodec_util.c  | 165 ++++++
 .../mediatek/vcodec/common/mtk_vcodec_util.h  |  77 +++
 .../platform/mediatek/vcodec/decoder/Makefile |  25 +
 .../vcodec/{ => decoder}/mtk_vcodec_dec.c     | 179 +++---
 .../vcodec/{ => decoder}/mtk_vcodec_dec.h     |  10 +-
 .../vcodec/{ => decoder}/mtk_vcodec_dec_drv.c |  85 ++-
 .../vcodec/decoder/mtk_vcodec_dec_drv.h       | 306 ++++++++++
 .../vcodec/{ => decoder}/mtk_vcodec_dec_hw.c  |  16 +-
 .../vcodec/{ => decoder}/mtk_vcodec_dec_hw.h  |   6 +-
 .../vcodec/{ => decoder}/mtk_vcodec_dec_pm.c  |  43 +-
 .../vcodec/{ => decoder}/mtk_vcodec_dec_pm.h  |   6 +-
 .../{ => decoder}/mtk_vcodec_dec_stateful.c   | 118 ++--
 .../{ => decoder}/mtk_vcodec_dec_stateless.c  |  77 +--
 .../{ => decoder}/vdec/vdec_av1_req_lat_if.c  | 152 +++--
 .../vcodec/{ => decoder}/vdec/vdec_h264_if.c  |  71 ++-
 .../{ => decoder}/vdec/vdec_h264_req_common.c |   4 +-
 .../{ => decoder}/vdec/vdec_h264_req_common.h |   6 +-
 .../{ => decoder}/vdec/vdec_h264_req_if.c     |  54 +-
 .../vdec/vdec_h264_req_multi_if.c             | 102 ++--
 .../vdec/vdec_hevc_req_multi_if.c             |  90 +--
 .../vcodec/{ => decoder}/vdec/vdec_vp8_if.c   |  82 +--
 .../{ => decoder}/vdec/vdec_vp8_req_if.c      |  59 +-
 .../vcodec/{ => decoder}/vdec/vdec_vp9_if.c   | 162 +++---
 .../{ => decoder}/vdec/vdec_vp9_req_lat_if.c  | 105 ++--
 .../vcodec/{ => decoder}/vdec_drv_base.h      |   2 +-
 .../vcodec/{ => decoder}/vdec_drv_if.c        |  13 +-
 .../vcodec/{ => decoder}/vdec_drv_if.h        |  10 +-
 .../vcodec/{ => decoder}/vdec_ipi_msg.h       |   0
 .../vcodec/{ => decoder}/vdec_msg_queue.c     |  49 +-
 .../vcodec/{ => decoder}/vdec_msg_queue.h     |  12 +-
 .../vcodec/{ => decoder}/vdec_vpu_if.c        |  66 ++-
 .../vcodec/{ => decoder}/vdec_vpu_if.h        |   6 +-
 .../platform/mediatek/vcodec/encoder/Makefile |  11 +
 .../vcodec/{ => encoder}/mtk_vcodec_enc.c     | 278 +++++----
 .../vcodec/{ => encoder}/mtk_vcodec_enc.h     |  12 +-
 .../vcodec/{ => encoder}/mtk_vcodec_enc_drv.c |  68 +--
 .../vcodec/encoder/mtk_vcodec_enc_drv.h       | 245 ++++++++
 .../vcodec/{ => encoder}/mtk_vcodec_enc_pm.c  |  17 +-
 .../vcodec/{ => encoder}/mtk_vcodec_enc_pm.h  |   4 +-
 .../vcodec/{ => encoder}/venc/venc_h264_if.c  | 106 ++--
 .../vcodec/{ => encoder}/venc/venc_vp8_if.c   |  64 +-
 .../vcodec/{ => encoder}/venc_drv_base.h      |   4 +-
 .../vcodec/{ => encoder}/venc_drv_if.c        |  10 +-
 .../vcodec/{ => encoder}/venc_drv_if.h        |  11 +-
 .../vcodec/{ => encoder}/venc_ipi_msg.h       |   0
 .../vcodec/{ => encoder}/venc_vpu_if.c        |  67 ++-
 .../vcodec/{ => encoder}/venc_vpu_if.h        |   3 +-
 .../platform/mediatek/vcodec/mtk_vcodec_drv.h | 548 ------------------
 .../mediatek/vcodec/mtk_vcodec_intr.c         |  43 --
 .../mediatek/vcodec/mtk_vcodec_util.c         | 143 -----
 .../mediatek/vcodec/mtk_vcodec_util.h         |  83 ---
 63 files changed, 2400 insertions(+), 1986 deletions(-)
 create mode 100644 drivers/media/platform/mediatek/vcodec/common/Makefile
 create mode 100644 drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_com_drv.h
 rename drivers/media/platform/mediatek/vcodec/{ => common}/mtk_vcodec_dbgfs.c (76%)
 rename drivers/media/platform/mediatek/vcodec/{ => common}/mtk_vcodec_dbgfs.h (62%)
 rename drivers/media/platform/mediatek/vcodec/{ => common}/mtk_vcodec_fw.c (75%)
 rename drivers/media/platform/mediatek/vcodec/{ => common}/mtk_vcodec_fw.h (86%)
 rename drivers/media/platform/mediatek/vcodec/{ => common}/mtk_vcodec_fw_priv.h (75%)
 rename drivers/media/platform/mediatek/vcodec/{ => common}/mtk_vcodec_fw_scp.c (70%)
 rename drivers/media/platform/mediatek/vcodec/{ => common}/mtk_vcodec_fw_vpu.c (58%)
 create mode 100644 drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_intr.c
 rename drivers/media/platform/mediatek/vcodec/{ => common}/mtk_vcodec_intr.h (68%)
 create mode 100644 drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_util.c
 create mode 100644 drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_util.h
 create mode 100644 drivers/media/platform/mediatek/vcodec/decoder/Makefile
 rename drivers/media/platform/mediatek/vcodec/{ => decoder}/mtk_vcodec_dec.c (83%)
 rename drivers/media/platform/mediatek/vcodec/{ => decoder}/mtk_vcodec_dec.h (91%)
 rename drivers/media/platform/mediatek/vcodec/{ => decoder}/mtk_vcodec_dec_drv.c (83%)
 create mode 100644 drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h
 rename drivers/media/platform/mediatek/vcodec/{ => decoder}/mtk_vcodec_dec_hw.c (92%)
 rename drivers/media/platform/mediatek/vcodec/{ => decoder}/mtk_vcodec_dec_hw.h (92%)
 rename drivers/media/platform/mediatek/vcodec/{ => decoder}/mtk_vcodec_dec_pm.c (78%)
 rename drivers/media/platform/mediatek/vcodec/{ => decoder}/mtk_vcodec_dec_pm.h (61%)
 rename drivers/media/platform/mediatek/vcodec/{ => decoder}/mtk_vcodec_dec_stateful.c (79%)
 rename drivers/media/platform/mediatek/vcodec/{ => decoder}/mtk_vcodec_dec_stateless.c (85%)
 rename drivers/media/platform/mediatek/vcodec/{ => decoder}/vdec/vdec_av1_req_lat_if.c (92%)
 rename drivers/media/platform/mediatek/vcodec/{ => decoder}/vdec/vdec_h264_if.c (82%)
 rename drivers/media/platform/mediatek/vcodec/{ => decoder}/vdec/vdec_h264_req_common.c (98%)
 rename drivers/media/platform/mediatek/vcodec/{ => decoder}/vdec/vdec_h264_req_common.h (97%)
 rename drivers/media/platform/mediatek/vcodec/{ => decoder}/vdec/vdec_h264_req_if.c (87%)
 rename drivers/media/platform/mediatek/vcodec/{ => decoder}/vdec/vdec_h264_req_multi_if.c (87%)
 rename drivers/media/platform/mediatek/vcodec/{ => decoder}/vdec/vdec_hevc_req_multi_if.c (91%)
 rename drivers/media/platform/mediatek/vcodec/{ => decoder}/vdec/vdec_vp8_if.c (83%)
 rename drivers/media/platform/mediatek/vcodec/{ => decoder}/vdec/vdec_vp8_req_if.c (83%)
 rename drivers/media/platform/mediatek/vcodec/{ => decoder}/vdec/vdec_vp9_if.c (82%)
 rename drivers/media/platform/mediatek/vcodec/{ => decoder}/vdec/vdec_vp9_req_lat_if.c (94%)
 rename drivers/media/platform/mediatek/vcodec/{ => decoder}/vdec_drv_base.h (95%)
 rename drivers/media/platform/mediatek/vcodec/{ => decoder}/vdec_drv_if.c (85%)
 rename drivers/media/platform/mediatek/vcodec/{ => decoder}/vdec_drv_if.h (89%)
 rename drivers/media/platform/mediatek/vcodec/{ => decoder}/vdec_ipi_msg.h (100%)
 rename drivers/media/platform/mediatek/vcodec/{ => decoder}/vdec_msg_queue.c (85%)
 rename drivers/media/platform/mediatek/vcodec/{ => decoder}/vdec_msg_queue.h (96%)
 rename drivers/media/platform/mediatek/vcodec/{ => decoder}/vdec_vpu_if.c (71%)
 rename drivers/media/platform/mediatek/vcodec/{ => decoder}/vdec_vpu_if.h (97%)
 create mode 100644 drivers/media/platform/mediatek/vcodec/encoder/Makefile
 rename drivers/media/platform/mediatek/vcodec/{ => encoder}/mtk_vcodec_enc.c (82%)
 rename drivers/media/platform/mediatek/vcodec/{ => encoder}/mtk_vcodec_enc.h (78%)
 rename drivers/media/platform/mediatek/vcodec/{ => encoder}/mtk_vcodec_enc_drv.c (87%)
 create mode 100644 drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc_drv.h
 rename drivers/media/platform/mediatek/vcodec/{ => encoder}/mtk_vcodec_enc_pm.c (77%)
 rename drivers/media/platform/mediatek/vcodec/{ => encoder}/mtk_vcodec_enc_pm.h (78%)
 rename drivers/media/platform/mediatek/vcodec/{ => encoder}/venc/venc_h264_if.c (85%)
 rename drivers/media/platform/mediatek/vcodec/{ => encoder}/venc/venc_vp8_if.c (84%)
 rename drivers/media/platform/mediatek/vcodec/{ => encoder}/venc_drv_base.h (94%)
 rename drivers/media/platform/mediatek/vcodec/{ => encoder}/venc_drv_if.c (86%)
 rename drivers/media/platform/mediatek/vcodec/{ => encoder}/venc_drv_if.h (94%)
 rename drivers/media/platform/mediatek/vcodec/{ => encoder}/venc_ipi_msg.h (100%)
 rename drivers/media/platform/mediatek/vcodec/{ => encoder}/venc_vpu_if.c (77%)
 rename drivers/media/platform/mediatek/vcodec/{ => encoder}/venc_vpu_if.h (96%)
 delete mode 100644 drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h
 delete mode 100644 drivers/media/platform/mediatek/vcodec/mtk_vcodec_intr.c
 delete mode 100644 drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.c
 delete mode 100644 drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.h

Comments

Yunfei Dong June 8, 2023, 7:27 a.m. UTC | #1
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...
Nicolas Dufresne June 8, 2023, 1:11 p.m. UTC | #2
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...
AngeloGioacchino Del Regno June 8, 2023, 2:06 p.m. UTC | #3
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...
>
Nicolas Dufresne June 8, 2023, 3:17 p.m. UTC | #4
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...
> > 
>
Yunfei Dong June 14, 2023, 9:17 a.m. UTC | #5
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...
> > > 
> > 
>
AngeloGioacchino Del Regno June 14, 2023, 11:50 a.m. UTC | #6
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...
>>>>
>>>
>>