lkml.org 
[lkml]   [2023]   [Dec]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v2 01/34] media: introduce common helpers for video firmware handling
    From
    On 18/12/2023 11:31, Dikshita Agarwal wrote:
    > Re-organize the video driver code by introducing a new folder
    > 'vcodec' and placing 'venus' driver code inside that.
    >
    > Introduce common helpers for trustzone based firmware
    > load/unload etc. which are placed in common folder
    > i.e. 'vcodec'.
    > Use these helpers in 'venus' driver. These helpers will be
    > used by 'iris' driver as well which is introduced later
    > in this patch series.
    >
    > Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
    > ---

    This is a very large patch, I think it needs to be broken up into
    smaller chunks.

    #1 Introduce common helper functions
    #2 Use common helper functions

    Its alot of code to try to eat in the one go.

    Could you consider making patches 1-3 a standalone series to reduce the
    amount of code to review here ?

    * 77e7025529d7c - (HEAD -> linux-stable-master-23-12-18-iris-v2) media:
    iris: add power management for encoder (21 hours ago)

    * ceb6a6f023fd3 - (tag: v6.7-rc6, linux-stable/master) Linux 6.7-rc6 (2
    days ago)

    git diff ceb6a6f023fd3 | wc -l

    21243

    Also I feel it wouild give more time for the changes to "digest" though
    upstream users and to find any unintended bugs.

    > +int load_fw(struct device *dev, const char *fw_name, phys_addr_t *mem_phys,
    > + size_t *mem_size, u32 pas_id, bool use_tz)
    > +{
    > + const struct firmware *firmware = NULL;
    > + struct reserved_mem *rmem;
    > + struct device_node *node;
    > + void *mem_virt = NULL;
    > + ssize_t fw_size = 0;
    > + int ret;
    > +
    > + if (!IS_ENABLED(CONFIG_QCOM_MDT_LOADER) ||
    > + (use_tz && !qcom_scm_is_available()))
    > + return -EPROBE_DEFER;
    > +
    > + if (!fw_name || !(*fw_name))
    > + return -EINVAL;

    The parameter check should come before the qcom_scm_is_available()

    No matter how many times you -EPROBE_DEFER -EINVAL is still -EINVAL.

    > +
    > + *mem_phys = 0;
    > + *mem_size = 0;

    I don't think you need this, you don't appear to use these variables
    before you assign them below.


    > +
    > + *mem_phys = rmem->base;
    > + *mem_size = rmem->size;

    > +
    > +int auth_reset_fw(u32 pas_id)
    > +{
    > + return qcom_scm_pas_auth_and_reset(pas_id);
    > +}
    > +
    > +void unload_fw(u32 pas_id)
    > +{
    > + qcom_scm_pas_shutdown(pas_id);
    > +}
    > +

    Do these wrapper functions add anything ? Some kind of validity check on
    the pas_id otherwise I'm not sure these are justified.

    > +int set_hw_state(bool resume)
    > +{
    > + return qcom_scm_set_remote_state(resume, 0);
    > +}
    > diff --git a/drivers/media/platform/qcom/vcodec/firmware.h b/drivers/media/platform/qcom/vcodec/firmware.h
    > new file mode 100644
    > index 0000000..7d410a8
    > --- /dev/null

    ---
    bod


    \
     
     \ /
      Last update: 2023-12-19 12:41    [W:3.324 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site