| Message ID | 1683193152-5808-2-git-send-email-quic_dikshita@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fix decoder issues with firmware version check | expand |
On 4.05.2023 11:39, Dikshita Agarwal wrote: > Add firmware version based checks to enable/disable > features for different SOCs. > > Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com> > Signed-off-by: Viswanath Boma <quic_vboma@quicinc.com> > Signed-off-by: Dikshita Agarwal <quic_dikshita@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 | 27 +++++++++++++++++++++++++-- > 2 files changed, 45 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h > index 32551c2..2f2176f 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 bool > +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 bool > +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..4854863 100644 > --- a/drivers/media/platform/qcom/venus/hfi_msgs.c > +++ b/drivers/media/platform/qcom/venus/hfi_msgs.c > @@ -248,13 +248,16 @@ 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; > size_t smem_blk_sz; > + int ret; > + u8 *ver_str; > > req_bytes = pkt->hdr.size - sizeof(*pkt); > > @@ -263,6 +266,26 @@ sys_get_prop_image_version(struct device *dev, > return; > > img_ver = pkt->data; > + if (IS_V6(core) && core->res->num_vpp_pipes == 1) { > + ret = sscanf(img_ver, "14:video-firmware.%u.%u-%u", > + &core->venus_ver.major, &core->venus_ver.minor, &core->venus_ver.rev); This is still not perfect, 8350 has 4 vpp pipes and its firmware is also denominated with "video-firmware".. perhaps we can just try each variant until we reach ret == 3? > + if (ret != 2) { 3? Konrad > + dev_dbg(dev, VDBGL "error reading F/W version\n"); > + return; > + } > + } else { > + if (IS_V6(core)) > + ver_str = "14:VIDEO.VPU.%u.%u-%u"; > + else > + ver_str = "14:VIDEO.VE.%u.%u-%u"; > + > + ret = sscanf(img_ver, "14:VIDEO.VE.%u.%u-%u", > + &core->venus_ver.major, &core->venus_ver.minor, &core->venus_ver.rev); > + if (ret != 3) { > + dev_dbg(dev, VDBGL "error reading F/W version\n"); > + return; > + } > + } > > dev_dbg(dev, VDBGL "F/W version: %s\n", img_ver); > > @@ -286,7 +309,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 5/4/2023 3:12 PM, Konrad Dybcio wrote: > > On 4.05.2023 11:39, Dikshita Agarwal wrote: >> Add firmware version based checks to enable/disable >> features for different SOCs. >> >> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com> >> Signed-off-by: Viswanath Boma <quic_vboma@quicinc.com> >> Signed-off-by: Dikshita Agarwal <quic_dikshita@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 | 27 +++++++++++++++++++++++++-- >> 2 files changed, 45 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h >> index 32551c2..2f2176f 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 bool >> +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 bool >> +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..4854863 100644 >> --- a/drivers/media/platform/qcom/venus/hfi_msgs.c >> +++ b/drivers/media/platform/qcom/venus/hfi_msgs.c >> @@ -248,13 +248,16 @@ 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; >> size_t smem_blk_sz; >> + int ret; >> + u8 *ver_str; >> >> req_bytes = pkt->hdr.size - sizeof(*pkt); >> >> @@ -263,6 +266,26 @@ sys_get_prop_image_version(struct device *dev, >> return; >> >> img_ver = pkt->data; >> + if (IS_V6(core) && core->res->num_vpp_pipes == 1) { >> + ret = sscanf(img_ver, "14:video-firmware.%u.%u-%u", >> + &core->venus_ver.major, &core->venus_ver.minor, &core->venus_ver.rev); > This is still not perfect, 8350 has 4 vpp pipes and its firmware is > also denominated with "video-firmware".. perhaps we can just try > each variant until we reach ret == 3? sc7280 onward firmware have image string as "video-firmware". Support for 8350 is not yet added in venus driver, any required change for the same can be done when support will be added for the same. >> + if (ret != 2) { > 3? this image version string doesn't return valid revision hence checking against 2 (major and minor versions) Thanks, Dikshita > > Konrad >> + dev_dbg(dev, VDBGL "error reading F/W version\n"); >> + return; >> + } >> + } else { >> + if (IS_V6(core)) >> + ver_str = "14:VIDEO.VPU.%u.%u-%u"; >> + else >> + ver_str = "14:VIDEO.VE.%u.%u-%u"; >> + >> + ret = sscanf(img_ver, "14:VIDEO.VE.%u.%u-%u", >> + &core->venus_ver.major, &core->venus_ver.minor, &core->venus_ver.rev); >> + if (ret != 3) { >> + dev_dbg(dev, VDBGL "error reading F/W version\n"); >> + return; >> + } >> + } >> >> dev_dbg(dev, VDBGL "F/W version: %s\n", img_ver); >> >> @@ -286,7 +309,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.05.2023 11:49, Dikshita Agarwal wrote: > > On 5/4/2023 3:12 PM, Konrad Dybcio wrote: >> >> On 4.05.2023 11:39, Dikshita Agarwal wrote: >>> Add firmware version based checks to enable/disable >>> features for different SOCs. >>> >>> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com> >>> Signed-off-by: Viswanath Boma <quic_vboma@quicinc.com> >>> Signed-off-by: Dikshita Agarwal <quic_dikshita@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 | 27 +++++++++++++++++++++++++-- >>> 2 files changed, 45 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h >>> index 32551c2..2f2176f 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 bool >>> +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 bool >>> +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..4854863 100644 >>> --- a/drivers/media/platform/qcom/venus/hfi_msgs.c >>> +++ b/drivers/media/platform/qcom/venus/hfi_msgs.c >>> @@ -248,13 +248,16 @@ 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; >>> size_t smem_blk_sz; >>> + int ret; >>> + u8 *ver_str; >>> req_bytes = pkt->hdr.size - sizeof(*pkt); >>> @@ -263,6 +266,26 @@ sys_get_prop_image_version(struct device *dev, >>> return; >>> img_ver = pkt->data; >>> + if (IS_V6(core) && core->res->num_vpp_pipes == 1) { >>> + ret = sscanf(img_ver, "14:video-firmware.%u.%u-%u", >>> + &core->venus_ver.major, &core->venus_ver.minor, &core->venus_ver.rev); >> This is still not perfect, 8350 has 4 vpp pipes and its firmware is >> also denominated with "video-firmware".. perhaps we can just try >> each variant until we reach ret == 3? > > sc7280 onward firmware have image string as "video-firmware". > > Support for 8350 is not yet added in venus driver, any required change for the same can be done > > when support will be added for the same. I understand, but that doesn't mean this can't be improved: ret = sscanf(.. video-firmware .. ) if (ret == 2) { /* video-firmware string doesn't provide rev info */ ver.rev = (something); goto done; } ret = sscanf(.. VIDEO.VE .. ) if (ret == 3) goto done; ret = sscanf(.. VIDEO.VPU .. ) if (ret != 3) { /* We ran out of options! */ return -EINVAL; } done: // continue the code flow > >>> + if (ret != 2) { >> 3? > > this image version string doesn't return valid revision hence checking against 2 (major and minor versions) So why are you filling the revision field with an invalid value? let's take a version string from a firmware I have on hand: 14:video-firmware.1.0-3fb5add1d3ac96f8f74facd537845a6ceb5a99e4 this will evaluate to: maj = 1 min = 0 rev = 3 since it's incorrect, drop the last argument and initialize it with something like UINT_MAX or 0xdeadbeef On a note, you left ver_str unused in this patch, so it doesn't work for VIDEO.VPU anyway Konrad > > Thanks, > > Dikshita > >> >> Konrad >>> + dev_dbg(dev, VDBGL "error reading F/W version\n"); >>> + return; >>> + } >>> + } else { >>> + if (IS_V6(core)) >>> + ver_str = "14:VIDEO.VPU.%u.%u-%u"; >>> + else >>> + ver_str = "14:VIDEO.VE.%u.%u-%u"; >>> + >>> + ret = sscanf(img_ver, "14:VIDEO.VE.%u.%u-%u", >>> + &core->venus_ver.major, &core->venus_ver.minor, &core->venus_ver.rev); >>> + if (ret != 3) { >>> + dev_dbg(dev, VDBGL "error reading F/W version\n"); >>> + return; >>> + } >>> + } >>> dev_dbg(dev, VDBGL "F/W version: %s\n", img_ver); >>> @@ -286,7 +309,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..2f2176f 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 bool +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 bool +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..4854863 100644 --- a/drivers/media/platform/qcom/venus/hfi_msgs.c +++ b/drivers/media/platform/qcom/venus/hfi_msgs.c @@ -248,13 +248,16 @@ 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; size_t smem_blk_sz; + int ret; + u8 *ver_str; req_bytes = pkt->hdr.size - sizeof(*pkt); @@ -263,6 +266,26 @@ sys_get_prop_image_version(struct device *dev, return; img_ver = pkt->data; + if (IS_V6(core) && core->res->num_vpp_pipes == 1) { + ret = sscanf(img_ver, "14:video-firmware.%u.%u-%u", + &core->venus_ver.major, &core->venus_ver.minor, &core->venus_ver.rev); + if (ret != 2) { + dev_dbg(dev, VDBGL "error reading F/W version\n"); + return; + } + } else { + if (IS_V6(core)) + ver_str = "14:VIDEO.VPU.%u.%u-%u"; + else + ver_str = "14:VIDEO.VE.%u.%u-%u"; + + ret = sscanf(img_ver, "14:VIDEO.VE.%u.%u-%u", + &core->venus_ver.major, &core->venus_ver.minor, &core->venus_ver.rev); + if (ret != 3) { + dev_dbg(dev, VDBGL "error reading F/W version\n"); + return; + } + } dev_dbg(dev, VDBGL "F/W version: %s\n", img_ver); @@ -286,7 +309,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");