Re: [PATCH v2] media: venus : hfi: add venus image info into smem
Hi Bjorn, Thanks for your review comments. I have addressed all in the latest patch v3. couldn't think of a shorter name for variables without losing the readability so kept as it is. Thanks, Dikshita On 2021-03-30 09:29, Bjorn Andersson wrote: On Fri 26 Mar 01:33 CDT 2021, Dikshita Agarwal wrote: Fill fw version info into smem to be printed as part of soc info. Signed-off-by: Dikshita Agarwal Changes since v1: adressed comments from stephen. removed unwanted code. --- drivers/media/platform/qcom/venus/hfi_msgs.c | 21 +++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.c b/drivers/media/platform/qcom/venus/hfi_msgs.c index 06a1908..6b6d33c9 100644 --- a/drivers/media/platform/qcom/venus/hfi_msgs.c +++ b/drivers/media/platform/qcom/venus/hfi_msgs.c @@ -6,6 +6,7 @@ #include #include #include +#include #include #include "core.h" @@ -14,6 +15,10 @@ #include "hfi_msgs.h" #include "hfi_parser.h" +#define SMEM_IMG_VER_TBL 469 +#define VER_STR_SZ 128 +#define SMEM_IMG_INDEX_VENUS 14 * 128 14 is the index, 128 is the element size, so this is now an "offset". + static void event_seq_changed(struct venus_core *core, struct venus_inst *inst, struct hfi_msg_event_notify_pkt *pkt) { @@ -239,15 +244,27 @@ static void sys_get_prop_image_version(struct device *dev, struct hfi_msg_sys_property_info_pkt *pkt) { + size_t smem_blk_sz = 0; You shouldn't need to initialize smem_blk_sz if you check the return value of qcom_smem_get() first. + u8 *smem_tbl_ptr; + u8 *img_ver; int req_bytes; req_bytes = pkt->hdr.size - sizeof(*pkt); - if (req_bytes < 128 || !pkt->data[1] || pkt->num_properties > 1) + if (req_bytes < VER_STR_SZ || !pkt->data[1] || pkt->num_properties > 1) /* bad packet */ return; - dev_dbg(dev, VDBGL "F/W version: %s\n", (u8 *)>data[1]); + img_ver = (u8 *)>data[1]; + + dev_dbg(dev, VDBGL "F/W version: %s\n", img_ver); + + smem_tbl_ptr = qcom_smem_get(QCOM_SMEM_HOST_ANY, + SMEM_IMG_VER_TBL, _blk_sz); 80 chars is just a guideline and this looks prettier if you avoid the line wrap. That said, if you pick shorter names for smem_tbl_ptr and smem_blk_sz you probably even have to worry. + if ((SMEM_IMG_INDEX_VENUS + VER_STR_SZ) <= smem_blk_sz && + smem_tbl_ptr) In English you're trying to determine: "did qcom_smem_get() return a valid pointer and is the item's size at least as big as we need". So just write that in C: if (smem_tbl_ptr && smem_blk_sz >= SMEM_IMG_INDEX_VENUS + VER_STR_SZ) + memcpy(smem_tbl_ptr + SMEM_IMG_INDEX_VENUS, + img_ver, VER_STR_SZ); Again, please avoid the line wrap... Regards, Bjorn } static void hfi_sys_property_info(struct venus_core *core, -- 2.7.4
Re: [PATCH v2] media: venus : hfi: add venus image info into smem
On Fri 26 Mar 01:33 CDT 2021, Dikshita Agarwal wrote: > Fill fw version info into smem to be printed as part of > soc info. > > Signed-off-by: Dikshita Agarwal > > Changes since v1: > adressed comments from stephen. > removed unwanted code. > --- > drivers/media/platform/qcom/venus/hfi_msgs.c | 21 +++-- > 1 file changed, 19 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.c > b/drivers/media/platform/qcom/venus/hfi_msgs.c > index 06a1908..6b6d33c9 100644 > --- a/drivers/media/platform/qcom/venus/hfi_msgs.c > +++ b/drivers/media/platform/qcom/venus/hfi_msgs.c > @@ -6,6 +6,7 @@ > #include > #include > #include > +#include > #include > > #include "core.h" > @@ -14,6 +15,10 @@ > #include "hfi_msgs.h" > #include "hfi_parser.h" > > +#define SMEM_IMG_VER_TBL 469 > +#define VER_STR_SZ 128 > +#define SMEM_IMG_INDEX_VENUS 14 * 128 14 is the index, 128 is the element size, so this is now an "offset". > + > static void event_seq_changed(struct venus_core *core, struct venus_inst > *inst, > struct hfi_msg_event_notify_pkt *pkt) > { > @@ -239,15 +244,27 @@ static void > sys_get_prop_image_version(struct device *dev, > struct hfi_msg_sys_property_info_pkt *pkt) > { > + size_t smem_blk_sz = 0; You shouldn't need to initialize smem_blk_sz if you check the return value of qcom_smem_get() first. > + u8 *smem_tbl_ptr; > + u8 *img_ver; > int req_bytes; > > req_bytes = pkt->hdr.size - sizeof(*pkt); > > - if (req_bytes < 128 || !pkt->data[1] || pkt->num_properties > 1) > + if (req_bytes < VER_STR_SZ || !pkt->data[1] || pkt->num_properties > 1) > /* bad packet */ > return; > > - dev_dbg(dev, VDBGL "F/W version: %s\n", (u8 *)>data[1]); > + img_ver = (u8 *)>data[1]; > + > + dev_dbg(dev, VDBGL "F/W version: %s\n", img_ver); > + > + smem_tbl_ptr = qcom_smem_get(QCOM_SMEM_HOST_ANY, > +SMEM_IMG_VER_TBL, _blk_sz); 80 chars is just a guideline and this looks prettier if you avoid the line wrap. That said, if you pick shorter names for smem_tbl_ptr and smem_blk_sz you probably even have to worry. > + if ((SMEM_IMG_INDEX_VENUS + VER_STR_SZ) <= smem_blk_sz && > + smem_tbl_ptr) In English you're trying to determine: "did qcom_smem_get() return a valid pointer and is the item's size at least as big as we need". So just write that in C: if (smem_tbl_ptr && smem_blk_sz >= SMEM_IMG_INDEX_VENUS + VER_STR_SZ) > + memcpy(smem_tbl_ptr + SMEM_IMG_INDEX_VENUS, > +img_ver, VER_STR_SZ); Again, please avoid the line wrap... Regards, Bjorn > } > > static void hfi_sys_property_info(struct venus_core *core, > -- > 2.7.4 >
Re: [PATCH v2] media: venus : hfi: add venus image info into smem
Hi Dikshita, Thank you for the patch! Yet something to improve: [auto build test ERROR on linuxtv-media/master] [also build test ERROR on v5.12-rc4 next-20210326] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Dikshita-Agarwal/media-venus-hfi-add-venus-image-info-into-smem/20210326-143612 base: git://linuxtv.org/media_tree.git master config: mips-randconfig-m031-20210326 (attached as .config) compiler: mips64-linux-gcc (GCC) 9.3.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/984122d8297ec95607fc222c4852439c321cdcd8 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Dikshita-Agarwal/media-venus-hfi-add-venus-image-info-into-smem/20210326-143612 git checkout 984122d8297ec95607fc222c4852439c321cdcd8 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=mips If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot All errors (new ones prefixed by >>, old ones prefixed by <<): >> ERROR: modpost: "qcom_smem_get" >> [drivers/media/platform/qcom/venus/venus-core.ko] undefined! --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org .config.gz Description: application/gzip
Re: [PATCH v2] media: venus : hfi: add venus image info into smem
Hi Dikshita, Thank you for the patch! Yet something to improve: [auto build test ERROR on linuxtv-media/master] [also build test ERROR on v5.12-rc4 next-20210326] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Dikshita-Agarwal/media-venus-hfi-add-venus-image-info-into-smem/20210326-143612 base: git://linuxtv.org/media_tree.git master config: parisc-randconfig-r023-20210326 (attached as .config) compiler: hppa64-linux-gcc (GCC) 9.3.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/984122d8297ec95607fc222c4852439c321cdcd8 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Dikshita-Agarwal/media-venus-hfi-add-venus-image-info-into-smem/20210326-143612 git checkout 984122d8297ec95607fc222c4852439c321cdcd8 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=parisc If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot All errors (new ones prefixed by >>): hppa64-linux-ld: drivers/media/platform/qcom/venus/hfi_msgs.o: in function `.LC18': >> (.data.rel.ro+0x90): undefined reference to `qcom_smem_get' --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org .config.gz Description: application/gzip