| Message ID | 1680848758-3947-2-git-send-email-quic_dikshita@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | fix decoder issues with firmware version check | expand |
On 7.04.2023 08:25, Dikshita Agarwal wrote: > Add firmware version based checks to enable/disable > features for different SOCs. > > Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com> > Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com> > Signed-off-by: Viswanath Boma <quic_vboma@quicinc.com> > Tested-by: Nathan Hebert <nhebert@chromium.org> > --- Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> One extra question: some firmware builds have a TYPE-n suffix like PROD-1 in: 14:VIDEO.VE.6.0-00042-PROD-1 Is the -1 a sign of an incremental build, or some "point release" of a given fw revision? Does it matter as far as this checking function goes? Konrad > drivers/media/platform/qcom/venus/core.h | 20 ++++++++++++++++++++ > drivers/media/platform/qcom/venus/hfi_msgs.c | 11 +++++++++-- > 2 files changed, 29 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h > index 32551c2..9d1e4b2 100644 > --- a/drivers/media/platform/qcom/venus/core.h > +++ b/drivers/media/platform/qcom/venus/core.h > @@ -202,6 +202,11 @@ struct venus_core { > unsigned int core0_usage_count; > unsigned int core1_usage_count; > struct dentry *root; > + struct venus_img_version { > + u32 major; > + u32 minor; > + u32 rev; > + } venus_ver; > }; > > struct vdec_controls { > @@ -500,4 +505,19 @@ venus_caps_by_codec(struct venus_core *core, u32 codec, u32 domain) > return NULL; > } > > +static inline int > +is_fw_rev_or_newer(struct venus_core *core, u32 vmajor, u32 vminor, u32 vrev) > +{ > + return ((core)->venus_ver.major == vmajor && > + (core)->venus_ver.minor == vminor && > + (core)->venus_ver.rev >= vrev); > +} > + > +static inline int > +is_fw_rev_or_older(struct venus_core *core, u32 vmajor, u32 vminor, u32 vrev) > +{ > + return ((core)->venus_ver.major == vmajor && > + (core)->venus_ver.minor == vminor && > + (core)->venus_ver.rev <= vrev); > +} > #endif > diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.c b/drivers/media/platform/qcom/venus/hfi_msgs.c > index df96db3..07ac0fc 100644 > --- a/drivers/media/platform/qcom/venus/hfi_msgs.c > +++ b/drivers/media/platform/qcom/venus/hfi_msgs.c > @@ -248,9 +248,10 @@ static void hfi_sys_init_done(struct venus_core *core, struct venus_inst *inst, > } > > static void > -sys_get_prop_image_version(struct device *dev, > +sys_get_prop_image_version(struct venus_core *core, > struct hfi_msg_sys_property_info_pkt *pkt) > { > + struct device *dev = core->dev; > u8 *smem_tbl_ptr; > u8 *img_ver; > int req_bytes; > @@ -263,6 +264,12 @@ sys_get_prop_image_version(struct device *dev, > return; > > img_ver = pkt->data; > + if (IS_V4(core)) > + sscanf(img_ver, "14:VIDEO.VE.%u.%u-%u-PROD", > + &core->venus_ver.major, &core->venus_ver.minor, &core->venus_ver.rev); > + else if (IS_V6(core)) > + sscanf(img_ver, "14:VIDEO.VPU.%u.%u-%u-PROD", > + &core->venus_ver.major, &core->venus_ver.minor, &core->venus_ver.rev); > > dev_dbg(dev, VDBGL "F/W version: %s\n", img_ver); > > @@ -286,7 +293,7 @@ static void hfi_sys_property_info(struct venus_core *core, > > switch (pkt->property) { > case HFI_PROPERTY_SYS_IMAGE_VERSION: > - sys_get_prop_image_version(dev, pkt); > + sys_get_prop_image_version(core, pkt); > break; > default: > dev_dbg(dev, VDBGL "unknown property data\n");
Hi Dikshita, Thanks for the patch. On 7.04.23 г. 9:25 ч., Dikshita Agarwal wrote: > Add firmware version based checks to enable/disable > features for different SOCs. > > Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com> > Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com> > Signed-off-by: Viswanath Boma <quic_vboma@quicinc.com> > Tested-by: Nathan Hebert <nhebert@chromium.org> > --- > drivers/media/platform/qcom/venus/core.h | 20 ++++++++++++++++++++ > drivers/media/platform/qcom/venus/hfi_msgs.c | 11 +++++++++-- > 2 files changed, 29 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h > index 32551c2..9d1e4b2 100644 > --- a/drivers/media/platform/qcom/venus/core.h > +++ b/drivers/media/platform/qcom/venus/core.h > @@ -202,6 +202,11 @@ struct venus_core { > unsigned int core0_usage_count; > unsigned int core1_usage_count; > struct dentry *root; > + struct venus_img_version { > + u32 major; > + u32 minor; > + u32 rev; > + } venus_ver; > }; > > struct vdec_controls { > @@ -500,4 +505,19 @@ venus_caps_by_codec(struct venus_core *core, u32 codec, u32 domain) > return NULL; > } > > +static inline int > +is_fw_rev_or_newer(struct venus_core *core, u32 vmajor, u32 vminor, u32 vrev) > +{ > + return ((core)->venus_ver.major == vmajor && > + (core)->venus_ver.minor == vminor && > + (core)->venus_ver.rev >= vrev); > +} > + > +static inline int > +is_fw_rev_or_older(struct venus_core *core, u32 vmajor, u32 vminor, u32 vrev) > +{ > + return ((core)->venus_ver.major == vmajor && > + (core)->venus_ver.minor == vminor && > + (core)->venus_ver.rev <= vrev); > +} IMO those two should return bool > #endif > diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.c b/drivers/media/platform/qcom/venus/hfi_msgs.c > index df96db3..07ac0fc 100644 > --- a/drivers/media/platform/qcom/venus/hfi_msgs.c > +++ b/drivers/media/platform/qcom/venus/hfi_msgs.c > @@ -248,9 +248,10 @@ static void hfi_sys_init_done(struct venus_core *core, struct venus_inst *inst, > } > > static void > -sys_get_prop_image_version(struct device *dev, > +sys_get_prop_image_version(struct venus_core *core, > struct hfi_msg_sys_property_info_pkt *pkt) > { > + struct device *dev = core->dev; > u8 *smem_tbl_ptr; > u8 *img_ver; > int req_bytes; > @@ -263,6 +264,12 @@ sys_get_prop_image_version(struct device *dev, > return; > > img_ver = pkt->data; > + if (IS_V4(core)) > + sscanf(img_ver, "14:VIDEO.VE.%u.%u-%u-PROD", > + &core->venus_ver.major, &core->venus_ver.minor, &core->venus_ver.rev); > + else if (IS_V6(core)) > + sscanf(img_ver, "14:VIDEO.VPU.%u.%u-%u-PROD", > + &core->venus_ver.major, &core->venus_ver.minor, &core->venus_ver.rev); > what about if IS_V1? > dev_dbg(dev, VDBGL "F/W version: %s\n", img_ver); this will crash for v1. > > @@ -286,7 +293,7 @@ static void hfi_sys_property_info(struct venus_core *core, > > switch (pkt->property) { > case HFI_PROPERTY_SYS_IMAGE_VERSION: > - sys_get_prop_image_version(dev, pkt); > + sys_get_prop_image_version(core, pkt); > break; > default: > dev_dbg(dev, VDBGL "unknown property data\n");
On 11.04.2023 12:59, Konrad Dybcio wrote: > > > On 9.04.2023 07:18, Stanimir Varbanov wrote: >> Hi Dikshita, >> >> Thanks for the patch. >> >> On 7.04.23 г. 9:25 ч., Dikshita Agarwal wrote: >>> Add firmware version based checks to enable/disable >>> features for different SOCs. >>> >>> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com> >>> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com> >>> Signed-off-by: Viswanath Boma <quic_vboma@quicinc.com> >>> Tested-by: Nathan Hebert <nhebert@chromium.org> >>> --- >>> drivers/media/platform/qcom/venus/core.h | 20 ++++++++++++++++++++ >>> drivers/media/platform/qcom/venus/hfi_msgs.c | 11 +++++++++-- >>> 2 files changed, 29 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h >>> index 32551c2..9d1e4b2 100644 >>> --- a/drivers/media/platform/qcom/venus/core.h >>> +++ b/drivers/media/platform/qcom/venus/core.h >>> @@ -202,6 +202,11 @@ struct venus_core { >>> unsigned int core0_usage_count; >>> unsigned int core1_usage_count; >>> struct dentry *root; >>> + struct venus_img_version { >>> + u32 major; >>> + u32 minor; >>> + u32 rev; >>> + } venus_ver; >>> }; >>> struct vdec_controls { >>> @@ -500,4 +505,19 @@ venus_caps_by_codec(struct venus_core *core, u32 codec, u32 domain) >>> return NULL; >>> } >>> +static inline int >>> +is_fw_rev_or_newer(struct venus_core *core, u32 vmajor, u32 vminor, u32 vrev) >>> +{ >>> + return ((core)->venus_ver.major == vmajor && >>> + (core)->venus_ver.minor == vminor && >>> + (core)->venus_ver.rev >= vrev); >>> +} >>> + >>> +static inline int >>> +is_fw_rev_or_older(struct venus_core *core, u32 vmajor, u32 vminor, u32 vrev) >>> +{ >>> + return ((core)->venus_ver.major == vmajor && >>> + (core)->venus_ver.minor == vminor && >>> + (core)->venus_ver.rev <= vrev); >>> +} >> >> IMO those two should return bool >> >>> #endif >>> diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.c b/drivers/media/platform/qcom/venus/hfi_msgs.c >>> index df96db3..07ac0fc 100644 >>> --- a/drivers/media/platform/qcom/venus/hfi_msgs.c >>> +++ b/drivers/media/platform/qcom/venus/hfi_msgs.c >>> @@ -248,9 +248,10 @@ static void hfi_sys_init_done(struct venus_core *core, struct venus_inst *inst, >>> } >>> static void >>> -sys_get_prop_image_version(struct device *dev, >>> +sys_get_prop_image_version(struct venus_core *core, >>> struct hfi_msg_sys_property_info_pkt *pkt) >>> { >>> + struct device *dev = core->dev; >>> u8 *smem_tbl_ptr; >>> u8 *img_ver; >>> int req_bytes; >>> @@ -263,6 +264,12 @@ sys_get_prop_image_version(struct device *dev, >>> return; >>> img_ver = pkt->data; >>> + if (IS_V4(core)) >>> + sscanf(img_ver, "14:VIDEO.VE.%u.%u-%u-PROD", >>> + &core->venus_ver.major, &core->venus_ver.minor, &core->venus_ver.rev); >>> + else if (IS_V6(core)) >>> + sscanf(img_ver, "14:VIDEO.VPU.%u.%u-%u-PROD", >>> + &core->venus_ver.major, &core->venus_ver.minor, &core->venus_ver.rev); >>> >> >> what about if IS_V1? > Whooops, I missed that in my review as well... > > Looks like the 8916 and 8996 FWs fall under the VIDEO.VE case > as well, that's the QC_VERSION_STRING they have.. On top of that, my 8350 fw reports: F/W version: 14:video-firmware.1.0-3fb5add1d3ac96f8f74facd537845a6ceb5a99e4 Konrad > > Perhaps this could be an > > if (IS_V6) > .. > else > .. > > Konrad >> >>> dev_dbg(dev, VDBGL "F/W version: %s\n", img_ver); >> >> this will crash for v1. >> >>> @@ -286,7 +293,7 @@ static void hfi_sys_property_info(struct venus_core *core, >>> switch (pkt->property) { >>> case HFI_PROPERTY_SYS_IMAGE_VERSION: >>> - sys_get_prop_image_version(dev, pkt); >>> + sys_get_prop_image_version(core, pkt); >>> break; >>> default: >>> dev_dbg(dev, VDBGL "unknown property data\n"); >>
On 4/8/2023 12:45 PM, Konrad Dybcio wrote: > > On 7.04.2023 08:25, Dikshita Agarwal wrote: >> Add firmware version based checks to enable/disable >> features for different SOCs. >> >> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com> >> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com> >> Signed-off-by: Viswanath Boma <quic_vboma@quicinc.com> >> Tested-by: Nathan Hebert <nhebert@chromium.org> >> --- > Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> > > One extra question: some firmware builds have a TYPE-n suffix like > PROD-1 in: > > 14:VIDEO.VE.6.0-00042-PROD-1 > > Is the -1 a sign of an incremental build, or some "point release" of a > given fw revision? Does it matter as far as this checking function > goes? this part of string gets added based on the tool version used to build the firmware. this doesn't matter and can be ignored. Thanks, Dikshita > Konrad >> drivers/media/platform/qcom/venus/core.h | 20 ++++++++++++++++++++ >> drivers/media/platform/qcom/venus/hfi_msgs.c | 11 +++++++++-- >> 2 files changed, 29 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h >> index 32551c2..9d1e4b2 100644 >> --- a/drivers/media/platform/qcom/venus/core.h >> +++ b/drivers/media/platform/qcom/venus/core.h >> @@ -202,6 +202,11 @@ struct venus_core { >> unsigned int core0_usage_count; >> unsigned int core1_usage_count; >> struct dentry *root; >> + struct venus_img_version { >> + u32 major; >> + u32 minor; >> + u32 rev; >> + } venus_ver; >> }; >> >> struct vdec_controls { >> @@ -500,4 +505,19 @@ venus_caps_by_codec(struct venus_core *core, u32 codec, u32 domain) >> return NULL; >> } >> >> +static inline int >> +is_fw_rev_or_newer(struct venus_core *core, u32 vmajor, u32 vminor, u32 vrev) >> +{ >> + return ((core)->venus_ver.major == vmajor && >> + (core)->venus_ver.minor == vminor && >> + (core)->venus_ver.rev >= vrev); >> +} >> + >> +static inline int >> +is_fw_rev_or_older(struct venus_core *core, u32 vmajor, u32 vminor, u32 vrev) >> +{ >> + return ((core)->venus_ver.major == vmajor && >> + (core)->venus_ver.minor == vminor && >> + (core)->venus_ver.rev <= vrev); >> +} >> #endif >> diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.c b/drivers/media/platform/qcom/venus/hfi_msgs.c >> index df96db3..07ac0fc 100644 >> --- a/drivers/media/platform/qcom/venus/hfi_msgs.c >> +++ b/drivers/media/platform/qcom/venus/hfi_msgs.c >> @@ -248,9 +248,10 @@ static void hfi_sys_init_done(struct venus_core *core, struct venus_inst *inst, >> } >> >> static void >> -sys_get_prop_image_version(struct device *dev, >> +sys_get_prop_image_version(struct venus_core *core, >> struct hfi_msg_sys_property_info_pkt *pkt) >> { >> + struct device *dev = core->dev; >> u8 *smem_tbl_ptr; >> u8 *img_ver; >> int req_bytes; >> @@ -263,6 +264,12 @@ sys_get_prop_image_version(struct device *dev, >> return; >> >> img_ver = pkt->data; >> + if (IS_V4(core)) >> + sscanf(img_ver, "14:VIDEO.VE.%u.%u-%u-PROD", >> + &core->venus_ver.major, &core->venus_ver.minor, &core->venus_ver.rev); >> + else if (IS_V6(core)) >> + sscanf(img_ver, "14:VIDEO.VPU.%u.%u-%u-PROD", >> + &core->venus_ver.major, &core->venus_ver.minor, &core->venus_ver.rev); >> >> dev_dbg(dev, VDBGL "F/W version: %s\n", img_ver); >> >> @@ -286,7 +293,7 @@ static void hfi_sys_property_info(struct venus_core *core, >> >> switch (pkt->property) { >> case HFI_PROPERTY_SYS_IMAGE_VERSION: >> - sys_get_prop_image_version(dev, pkt); >> + sys_get_prop_image_version(core, pkt); >> break; >> default: >> dev_dbg(dev, VDBGL "unknown property data\n");
On 4/9/2023 10:48 AM, Stanimir Varbanov wrote: > Hi Dikshita, > > Thanks for the patch. > > On 7.04.23 г. 9:25 ч., Dikshita Agarwal wrote: >> Add firmware version based checks to enable/disable >> features for different SOCs. >> >> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com> >> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com> >> Signed-off-by: Viswanath Boma <quic_vboma@quicinc.com> >> Tested-by: Nathan Hebert <nhebert@chromium.org> >> --- >> drivers/media/platform/qcom/venus/core.h | 20 ++++++++++++++++++++ >> drivers/media/platform/qcom/venus/hfi_msgs.c | 11 +++++++++-- >> 2 files changed, 29 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/media/platform/qcom/venus/core.h >> b/drivers/media/platform/qcom/venus/core.h >> index 32551c2..9d1e4b2 100644 >> --- a/drivers/media/platform/qcom/venus/core.h >> +++ b/drivers/media/platform/qcom/venus/core.h >> @@ -202,6 +202,11 @@ struct venus_core { >> unsigned int core0_usage_count; >> unsigned int core1_usage_count; >> struct dentry *root; >> + struct venus_img_version { >> + u32 major; >> + u32 minor; >> + u32 rev; >> + } venus_ver; >> }; >> struct vdec_controls { >> @@ -500,4 +505,19 @@ venus_caps_by_codec(struct venus_core *core, u32 >> codec, u32 domain) >> return NULL; >> } >> +static inline int >> +is_fw_rev_or_newer(struct venus_core *core, u32 vmajor, u32 vminor, >> u32 vrev) >> +{ >> + return ((core)->venus_ver.major == vmajor && >> + (core)->venus_ver.minor == vminor && >> + (core)->venus_ver.rev >= vrev); >> +} >> + >> +static inline int >> +is_fw_rev_or_older(struct venus_core *core, u32 vmajor, u32 vminor, >> u32 vrev) >> +{ >> + return ((core)->venus_ver.major == vmajor && >> + (core)->venus_ver.minor == vminor && >> + (core)->venus_ver.rev <= vrev); >> +} > > IMO those two should return bool sure, will update. > >> #endif >> diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.c >> b/drivers/media/platform/qcom/venus/hfi_msgs.c >> index df96db3..07ac0fc 100644 >> --- a/drivers/media/platform/qcom/venus/hfi_msgs.c >> +++ b/drivers/media/platform/qcom/venus/hfi_msgs.c >> @@ -248,9 +248,10 @@ static void hfi_sys_init_done(struct venus_core >> *core, struct venus_inst *inst, >> } >> static void >> -sys_get_prop_image_version(struct device *dev, >> +sys_get_prop_image_version(struct venus_core *core, >> struct hfi_msg_sys_property_info_pkt *pkt) >> { >> + struct device *dev = core->dev; >> u8 *smem_tbl_ptr; >> u8 *img_ver; >> int req_bytes; >> @@ -263,6 +264,12 @@ sys_get_prop_image_version(struct device *dev, >> return; >> img_ver = pkt->data; >> + if (IS_V4(core)) >> + sscanf(img_ver, "14:VIDEO.VE.%u.%u-%u-PROD", >> + &core->venus_ver.major, &core->venus_ver.minor, >> &core->venus_ver.rev); >> + else if (IS_V6(core)) >> + sscanf(img_ver, "14:VIDEO.VPU.%u.%u-%u-PROD", >> + &core->venus_ver.major, &core->venus_ver.minor, >> &core->venus_ver.rev); > > what about if IS_V1? the conditional code above is added only for v6 and v4. The behavior for v1 remain same as before. img_ver = pkt->data > >> dev_dbg(dev, VDBGL "F/W version: %s\n", img_ver); > > this will crash for v1. I didn't get why would it crash, could you pls explain? > >> @@ -286,7 +293,7 @@ static void hfi_sys_property_info(struct >> venus_core *core, >> switch (pkt->property) { >> case HFI_PROPERTY_SYS_IMAGE_VERSION: >> - sys_get_prop_image_version(dev, pkt); >> + sys_get_prop_image_version(core, pkt); >> break; >> default: >> dev_dbg(dev, VDBGL "unknown property data\n"); >
diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h index 32551c2..9d1e4b2 100644 --- a/drivers/media/platform/qcom/venus/core.h +++ b/drivers/media/platform/qcom/venus/core.h @@ -202,6 +202,11 @@ struct venus_core { unsigned int core0_usage_count; unsigned int core1_usage_count; struct dentry *root; + struct venus_img_version { + u32 major; + u32 minor; + u32 rev; + } venus_ver; }; struct vdec_controls { @@ -500,4 +505,19 @@ venus_caps_by_codec(struct venus_core *core, u32 codec, u32 domain) return NULL; } +static inline int +is_fw_rev_or_newer(struct venus_core *core, u32 vmajor, u32 vminor, u32 vrev) +{ + return ((core)->venus_ver.major == vmajor && + (core)->venus_ver.minor == vminor && + (core)->venus_ver.rev >= vrev); +} + +static inline int +is_fw_rev_or_older(struct venus_core *core, u32 vmajor, u32 vminor, u32 vrev) +{ + return ((core)->venus_ver.major == vmajor && + (core)->venus_ver.minor == vminor && + (core)->venus_ver.rev <= vrev); +} #endif diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.c b/drivers/media/platform/qcom/venus/hfi_msgs.c index df96db3..07ac0fc 100644 --- a/drivers/media/platform/qcom/venus/hfi_msgs.c +++ b/drivers/media/platform/qcom/venus/hfi_msgs.c @@ -248,9 +248,10 @@ static void hfi_sys_init_done(struct venus_core *core, struct venus_inst *inst, } static void -sys_get_prop_image_version(struct device *dev, +sys_get_prop_image_version(struct venus_core *core, struct hfi_msg_sys_property_info_pkt *pkt) { + struct device *dev = core->dev; u8 *smem_tbl_ptr; u8 *img_ver; int req_bytes; @@ -263,6 +264,12 @@ sys_get_prop_image_version(struct device *dev, return; img_ver = pkt->data; + if (IS_V4(core)) + sscanf(img_ver, "14:VIDEO.VE.%u.%u-%u-PROD", + &core->venus_ver.major, &core->venus_ver.minor, &core->venus_ver.rev); + else if (IS_V6(core)) + sscanf(img_ver, "14:VIDEO.VPU.%u.%u-%u-PROD", + &core->venus_ver.major, &core->venus_ver.minor, &core->venus_ver.rev); dev_dbg(dev, VDBGL "F/W version: %s\n", img_ver); @@ -286,7 +293,7 @@ static void hfi_sys_property_info(struct venus_core *core, switch (pkt->property) { case HFI_PROPERTY_SYS_IMAGE_VERSION: - sys_get_prop_image_version(dev, pkt); + sys_get_prop_image_version(core, pkt); break; default: dev_dbg(dev, VDBGL "unknown property data\n");