Hi Bryan,
On 9/5/2024 6:40 PM, Bryan O'Donoghue wrote:
> On 27/08/2024 11:05, Dikshita Agarwal via B4 Relay wrote:
>> From: Dikshita Agarwal <quic_dikshita@xxxxxxxxxxx>
>>
>> Host firmware interface (HFI) is well defined set of interfaces
>> for communication between host driver and firmware.
>> The command and responses are exchanged in form of packets.
>> One or multiple packets are grouped under packet header.
>> Each packet has packet type which describes the specific HFI
>> and payload which holds the corresponding value for that HFI.
>>
>> Signed-off-by: Dikshita Agarwal <quic_dikshita@xxxxxxxxxxx>
>> ---
>> drivers/media/platform/qcom/iris/Makefile | 5 +
>> drivers/media/platform/qcom/iris/iris_core.c | 26 ++-
>> drivers/media/platform/qcom/iris/iris_core.h | 20 ++
>> drivers/media/platform/qcom/iris/iris_hfi_common.c | 56 +++++
>> drivers/media/platform/qcom/iris/iris_hfi_common.h | 60 ++++++
>> drivers/media/platform/qcom/iris/iris_hfi_gen1.h | 3 +
>> .../platform/qcom/iris/iris_hfi_gen1_command.c | 61 ++++++
>> .../platform/qcom/iris/iris_hfi_gen1_defines.h | 94 +++++++++
>> .../platform/qcom/iris/iris_hfi_gen1_response.c | 174 ++++++++++++++++
>> drivers/media/platform/qcom/iris/iris_hfi_gen2.h | 4 +
>> .../platform/qcom/iris/iris_hfi_gen2_command.c | 72 +++++++
>> .../platform/qcom/iris/iris_hfi_gen2_defines.h | 46 +++++
>> .../platform/qcom/iris/iris_hfi_gen2_packet.c | 164 +++++++++++++++
>> .../platform/qcom/iris/iris_hfi_gen2_packet.h | 69 +++++++
>> .../platform/qcom/iris/iris_hfi_gen2_response.c | 229 +++++++++++++++++++++
>> drivers/media/platform/qcom/iris/iris_hfi_queue.c | 201 ++++++++++++++++++
>> drivers/media/platform/qcom/iris/iris_hfi_queue.h | 5 +
>> .../platform/qcom/iris/iris_platform_common.h | 15 ++
>> .../platform/qcom/iris/iris_platform_sm8250.c | 3 +
>> .../platform/qcom/iris/iris_platform_sm8550.c | 14 ++
>> drivers/media/platform/qcom/iris/iris_probe.c | 40 ++++
>> drivers/media/platform/qcom/iris/iris_state.c | 15 ++
>> drivers/media/platform/qcom/iris/iris_state.h | 4 +
>> drivers/media/platform/qcom/iris/iris_vpu_common.c | 42 ++++
>> drivers/media/platform/qcom/iris/iris_vpu_common.h | 3 +
>> 25 files changed, 1424 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/qcom/iris/Makefile
>> b/drivers/media/platform/qcom/iris/Makefile
>> index 95f4e92fe085..d1f0b933df3d 100644
>> --- a/drivers/media/platform/qcom/iris/Makefile
>> +++ b/drivers/media/platform/qcom/iris/Makefile
>> @@ -1,12 +1,17 @@
>> iris-objs += iris_core.o \
>> iris_firmware.o \
>> + iris_hfi_common.o \
>> iris_hfi_gen1_command.o \
>> + iris_hfi_gen1_response.o \
>> iris_hfi_gen2_command.o \
>> + iris_hfi_gen2_packet.o \
>> + iris_hfi_gen2_response.o \
>> iris_hfi_queue.o \
>> iris_platform_sm8250.o \
>> iris_platform_sm8550.o \
>> iris_probe.o \
>> iris_resources.o \
>> + iris_state.o \
>> iris_vidc.o \
>> iris_vpu_common.o \
>> diff --git a/drivers/media/platform/qcom/iris/iris_core.c
>> b/drivers/media/platform/qcom/iris/iris_core.c
>> index 5ad66ac113ae..92458d7f1e36 100644
>> --- a/drivers/media/platform/qcom/iris/iris_core.c
>> +++ b/drivers/media/platform/qcom/iris/iris_core.c
>> @@ -17,6 +17,26 @@ void iris_core_deinit(struct iris_core *core)
>> mutex_unlock(&core->lock);
>> }
>> +static int iris_wait_for_system_response(struct iris_core *core)
>> +{
>> + u32 hw_response_timeout_val;
>> + int ret;
>> +
>> + if (core->state == IRIS_CORE_ERROR)
>> + return -EIO;
>> +
>> + hw_response_timeout_val = core->iris_platform_data->hw_response_timeout;
>> +
>> + ret = wait_for_completion_timeout(&core->core_init_done,
>> + msecs_to_jiffies(hw_response_timeout_val));
>> + if (!ret) {
>> + iris_change_core_state(core, IRIS_CORE_ERROR);
>> + return -ETIMEDOUT;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> int iris_core_init(struct iris_core *core)
>> {
>> int ret;
>> @@ -44,9 +64,13 @@ int iris_core_init(struct iris_core *core)
>> if (ret)
>> goto error_unload_fw;
>> + ret = iris_hfi_core_init(core);
>> + if (ret)
>> + goto error_unload_fw;
>> +
>> mutex_unlock(&core->lock);
>> - return 0;
>> + return iris_wait_for_system_response(core);
>> error_unload_fw:
>> iris_fw_unload(core);
>> diff --git a/drivers/media/platform/qcom/iris/iris_core.h
>> b/drivers/media/platform/qcom/iris/iris_core.h
>> index 13c5932f9110..409f9822807d 100644
>> --- a/drivers/media/platform/qcom/iris/iris_core.h
>> +++ b/drivers/media/platform/qcom/iris/iris_core.h
>> @@ -9,11 +9,15 @@
>> #include <linux/types.h>
>> #include <media/v4l2-device.h>
>> +#include "iris_hfi_common.h"
>> #include "iris_hfi_queue.h"
>> #include "iris_platform_common.h"
>> #include "iris_resources.h"
>> #include "iris_state.h"
>> +#define IRIS_FW_VERSION_LENGTH 128
>> +#define IFACEQ_CORE_PKT_SIZE (1024 * 4)
>> +
>> /**
>> * struct iris_core - holds core parameters valid for all instances
>> *
>> @@ -40,6 +44,14 @@
>> * @message_queue: shared interface queue to receive responses from firmware
>> * @debug_queue: shared interface queue to receive debug info from firmware
>> * @lock: a lock for this strucure
>> + * @response_packet: a pointer to response packet from fw to driver
>> + * @header_id: id of packet header
>> + * @packet_id: id of packet
>> + * @hfi_ops: iris hfi command ops
>> + * @hfi_response_ops: iris hfi response ops
>> + * @core_init_done: structure of signal completion for system response
>> + * @intr_status: interrupt status
>> + * @sys_error_handler: a delayed work for handling system fatal error
>> */
>> struct iris_core {
>> @@ -66,6 +78,14 @@ struct iris_core {
>> struct iris_iface_q_info message_queue;
>> struct iris_iface_q_info debug_queue;
>> struct mutex lock; /* lock for core related operations */
>> + u8 *response_packet;
>> + u32 header_id;
>> + u32 packet_id;
>> + const struct iris_hfi_command_ops *hfi_ops;
>> + const struct iris_hfi_response_ops *hfi_response_ops;
>> + struct completion core_init_done;
>> + u32 intr_status;
>> + struct delayed_work sys_error_handler;
>> };
>> int iris_core_init(struct iris_core *core);
>> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_common.c
>> b/drivers/media/platform/qcom/iris/iris_hfi_common.c
>> new file mode 100644
>> index 000000000000..a5a28029d8d1
>> --- /dev/null
>> +++ b/drivers/media/platform/qcom/iris/iris_hfi_common.c
>> @@ -0,0 +1,56 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +#include "iris_core.h"
>> +#include "iris_hfi_common.h"
>> +#include "iris_vpu_common.h"
>> +
>> +int iris_hfi_core_init(struct iris_core *core)
>> +{
>> + const struct iris_hfi_command_ops *hfi_ops = core->hfi_ops;
>> + int ret;
>> +
>> + ret = hfi_ops->sys_init(core);
>> + if (ret)
>> + return ret;
>> +
>> + ret = hfi_ops->sys_image_version(core);
>> + if (ret)
>> + return ret;
>> +
>> + return hfi_ops->sys_interframe_powercollapse(core);
>> +}
>> +
>> +irqreturn_t iris_hfi_isr(int irq, void *data)
>> +{
>> + disable_irq_nosync(irq);
>> +
>> + return IRQ_WAKE_THREAD;
>> +}
>> +
>> +irqreturn_t iris_hfi_isr_handler(int irq, void *data)
>> +{
>> + struct iris_core *core = data;
>> +
>> + if (!core)
>> + return IRQ_NONE;
>> +
>> + mutex_lock(&core->lock);
>> + if (core->state != IRIS_CORE_INIT) {
>> + mutex_unlock(&core->lock);
>> + goto exit;
>> + }
>> +
>> + iris_vpu_clear_interrupt(core);
>> + mutex_unlock(&core->lock);
>> +
>> + core->hfi_response_ops->hfi_response_handler(core);
>> +
>> +exit:
>> + if (!iris_vpu_watchdog(core, core->intr_status))
>> + enable_irq(irq);
>> +
>> + return IRQ_HANDLED;
>> +}
>> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_common.h
>> b/drivers/media/platform/qcom/iris/iris_hfi_common.h
>> new file mode 100644
>> index 000000000000..c3d5b899cf60
>> --- /dev/null
>> +++ b/drivers/media/platform/qcom/iris/iris_hfi_common.h
>> @@ -0,0 +1,60 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +#ifndef _IRIS_HFI_COMMON_H_
>> +#define _IRIS_HFI_COMMON_H_
>> +
>> +#include <linux/types.h>
>> +#include <media/v4l2-device.h>
>> +
>> +struct iris_core;
>> +
>> +enum hfi_packet_port_type {
>> + HFI_PORT_NONE = 0x00000000,
>> + HFI_PORT_BITSTREAM = 0x00000001,
>> + HFI_PORT_RAW = 0x00000002,
>> +};
>> +
>> +enum hfi_packet_payload_info {
>> + HFI_PAYLOAD_NONE = 0x00000000,
>> + HFI_PAYLOAD_U32 = 0x00000001,
>> + HFI_PAYLOAD_S32 = 0x00000002,
>> + HFI_PAYLOAD_U64 = 0x00000003,
>> + HFI_PAYLOAD_S64 = 0x00000004,
>> + HFI_PAYLOAD_STRUCTURE = 0x00000005,
>> + HFI_PAYLOAD_BLOB = 0x00000006,
>> + HFI_PAYLOAD_STRING = 0x00000007,
>> + HFI_PAYLOAD_Q16 = 0x00000008,
>> + HFI_PAYLOAD_U32_ENUM = 0x00000009,
>> + HFI_PAYLOAD_32_PACKED = 0x0000000a,
>> + HFI_PAYLOAD_U32_ARRAY = 0x0000000b,
>> + HFI_PAYLOAD_S32_ARRAY = 0x0000000c,
>> + HFI_PAYLOAD_64_PACKED = 0x0000000d,
>> +};
>> +
>> +enum hfi_packet_host_flags {
>> + HFI_HOST_FLAGS_NONE = 0x00000000,
>> + HFI_HOST_FLAGS_INTR_REQUIRED = 0x00000001,
>> + HFI_HOST_FLAGS_RESPONSE_REQUIRED = 0x00000002,
>> + HFI_HOST_FLAGS_NON_DISCARDABLE = 0x00000004,
>> + HFI_HOST_FLAGS_GET_PROPERTY = 0x00000008,
>> +};
>> +
>> +struct iris_hfi_command_ops {
>> + int (*sys_init)(struct iris_core *core);
>> + int (*sys_image_version)(struct iris_core *core);
>> + int (*sys_interframe_powercollapse)(struct iris_core *core);
>> +};
>> +
>> +struct iris_hfi_response_ops {
>> + void (*hfi_response_handler)(struct iris_core *core);
>> +};
>> +
>> +int iris_hfi_core_init(struct iris_core *core);
>> +
>> +irqreturn_t iris_hfi_isr(int irq, void *data);
>> +irqreturn_t iris_hfi_isr_handler(int irq, void *data);
>> +
>> +#endif
>> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen1.h
>> b/drivers/media/platform/qcom/iris/iris_hfi_gen1.h
>> index b02f629a9cdc..15edbb359c71 100644
>> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen1.h
>> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen1.h
>> @@ -6,8 +6,11 @@
>> #ifndef _IRIS_HFI_GEN1_H_
>> #define _IRIS_HFI_GEN1_H_
>> +struct iris_core;
>> struct iris_inst;
>> +void iris_hfi_gen1_command_ops_init(struct iris_core *core);
>> +void iris_hfi_gen1_response_ops_init(struct iris_core *core);
>> struct iris_inst *iris_hfi_gen1_get_instance(void);
>> #endif
>> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
>> b/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
>> index 20c68f4ffb72..8f045ef56163 100644
>> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
>> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
>> @@ -4,8 +4,69 @@
>> */
>> #include "iris_hfi_gen1.h"
>> +#include "iris_hfi_gen1_defines.h"
>> #include "iris_instance.h"
>> +static int iris_hfi_gen1_sys_init(struct iris_core *core)
>> +{
>> + struct hfi_sys_init_pkt sys_init_pkt;
>> +
>> + sys_init_pkt.hdr.size = sizeof(sys_init_pkt);
>> + sys_init_pkt.hdr.pkt_type = HFI_CMD_SYS_INIT;
>> + sys_init_pkt.arch_type = HFI_VIDEO_ARCH_OX;
>> +
>> + return iris_hfi_queue_cmd_write_locked(core, &sys_init_pkt,
>> sys_init_pkt.hdr.size);
>> +}
>> +
>> +static int iris_hfi_gen1_sys_image_version(struct iris_core *core)
>> +{
>> + struct hfi_sys_get_property_pkt packet;
>> +
>> + packet.hdr.size = sizeof(packet);
>> + packet.hdr.pkt_type = HFI_CMD_SYS_GET_PROPERTY;
>> + packet.num_properties = 1;
>> + packet.data = HFI_PROPERTY_SYS_IMAGE_VERSION;
>> +
>> + return iris_hfi_queue_cmd_write_locked(core, &packet, packet.hdr.size);
>> +}
>> +
>> +static int iris_hfi_gen1_sys_interframe_powercollapse(struct iris_core *core)
>> +{
>> + struct hfi_sys_set_property_pkt *pkt;
>> + struct hfi_enable *hfi;
>> + u32 packet_size;
>> + u32 ret;
>> +
>> + packet_size = struct_size(pkt, data, 1) + sizeof(*hfi);
>> + pkt = kzalloc(packet_size, GFP_KERNEL);
>> + if (!pkt)
>> + return -ENOMEM;
>> +
>> + hfi = (struct hfi_enable *)&pkt->data[1];
>> +
>> + pkt->hdr.size = packet_size;
>> + pkt->hdr.pkt_type = HFI_CMD_SYS_SET_PROPERTY;
>> + pkt->num_properties = 1;
>> + pkt->data[0] = HFI_PROPERTY_SYS_CODEC_POWER_PLANE_CTRL;
>> + hfi->enable = true;
>> +
>> + ret = iris_hfi_queue_cmd_write_locked(core, pkt, pkt->hdr.size);
>> + kfree(pkt);
>> +
>> + return ret;
>> +}
>> +
>> +static const struct iris_hfi_command_ops iris_hfi_gen1_command_ops = {
>> + .sys_init = iris_hfi_gen1_sys_init,
>> + .sys_image_version = iris_hfi_gen1_sys_image_version,
>> + .sys_interframe_powercollapse = iris_hfi_gen1_sys_interframe_powercollapse,
>> +};
>> +
>> +void iris_hfi_gen1_command_ops_init(struct iris_core *core)
>> +{
>> + core->hfi_ops = &iris_hfi_gen1_command_ops;
>> +}
>> +
>> struct iris_inst *iris_hfi_gen1_get_instance(void)
>> {
>> return kzalloc(sizeof(struct iris_inst), GFP_KERNEL);
>> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen1_defines.h
>> b/drivers/media/platform/qcom/iris/iris_hfi_gen1_defines.h
>> new file mode 100644
>> index 000000000000..5c07d6a29863
>> --- /dev/null
>> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen1_defines.h
>> @@ -0,0 +1,94 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +#ifndef _IRIS_HFI_GEN1_DEFINES_H_
>> +#define _IRIS_HFI_GEN1_DEFINES_H_
>> +
>> +#include <linux/types.h>
>> +
>> +#define HFI_VIDEO_ARCH_OX 0x1
>> +#define HFI_ERR_NONE 0x0
>> +
>> +#define HFI_CMD_SYS_INIT 0x10001
>> +#define HFI_CMD_SYS_SET_PROPERTY 0x10005
>> +#define HFI_CMD_SYS_GET_PROPERTY 0x10006
>> +
>> +#define HFI_PROPERTY_SYS_CODEC_POWER_PLANE_CTRL 0x5
>> +#define HFI_PROPERTY_SYS_IMAGE_VERSION 0x6
>> +
>> +#define HFI_EVENT_SYS_ERROR 0x1
>> +
>> +#define HFI_MSG_SYS_INIT 0x20001
>> +#define HFI_MSG_SYS_COV 0x20009
>> +#define HFI_MSG_SYS_PROPERTY_INFO 0x2000a
>> +
>> +#define HFI_MSG_EVENT_NOTIFY 0x21001
>> +
>> +struct hfi_pkt_hdr {
>> + u32 size;
>> + u32 pkt_type;
>> +};
>> +
>> +struct hfi_sys_init_pkt {
>> + struct hfi_pkt_hdr hdr;
>> + u32 arch_type;
>> +};
>> +
>> +struct hfi_sys_set_property_pkt {
>> + struct hfi_pkt_hdr hdr;
>> + u32 num_properties;
>> + u32 data[];
>> +};
>> +
>> +struct hfi_sys_get_property_pkt {
>> + struct hfi_pkt_hdr hdr;
>> + u32 num_properties;
>> + u32 data;
>> +};
>> +
>> +struct hfi_msg_event_notify_pkt {
>> + struct hfi_pkt_hdr hdr;
>> + u32 event_id;
>> + u32 event_data1;
>> + u32 event_data2;
>> + u32 ext_event_data[];
>> +};
>> +
>> +struct hfi_msg_sys_init_done_pkt {
>> + struct hfi_pkt_hdr hdr;
>> + u32 error_type;
>> + u32 num_properties;
>> + u32 data[];
>> +};
>> +
>> +struct hfi_msg_sys_property_info_pkt {
>> + struct hfi_pkt_hdr hdr;
>> + u32 num_properties;
>> + u32 property;
>> + u8 data[];
>> +};
>> +
>> +struct hfi_enable {
>> + u32 enable;
>> +};
>> +
>> +struct hfi_msg_sys_debug_pkt {
>> + struct hfi_pkt_hdr hdr;
>> + u32 msg_type;
>> + u32 msg_size;
>> + u32 time_stamp_hi;
>> + u32 time_stamp_lo;
>> + u8 msg_data[];
>> +};
>> +
>> +struct hfi_msg_sys_coverage_pkt {
>> + struct hfi_pkt_hdr hdr;
>> + u32 msg_size;
>> + u32 time_stamp_hi;
>> + u32 time_stamp_lo;
>> + u8 msg_data[];
>> +};
>> +
>> +#endif
>> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c
>> b/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c
>> new file mode 100644
>> index 000000000000..3eb2ce99c614
>> --- /dev/null
>> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c
>> @@ -0,0 +1,174 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +#include "iris_hfi_gen1.h"
>> +#include "iris_hfi_gen1_defines.h"
>> +#include "iris_instance.h"
>> +
>> +static void
>> +iris_hfi_gen1_sys_event_notify(struct iris_core *core, void *packet)
>> +{
>> + struct hfi_msg_event_notify_pkt *pkt = packet;
>> +
>> + if (pkt->event_id == HFI_EVENT_SYS_ERROR)
>> + dev_err(core->dev, "sys error (type: %x, data1:%x, data2:%x)\n",
>> + pkt->event_id, pkt->event_data1, pkt->event_data2);
>> +
>> + iris_change_core_state(core, IRIS_CORE_ERROR);
>> + schedule_delayed_work(&core->sys_error_handler, msecs_to_jiffies(10));
>> +}
>> +
>> +static void iris_hfi_gen1_sys_init_done(struct iris_core *core, void *packet)
>> +{
>> + struct hfi_msg_sys_init_done_pkt *pkt = packet;
>> +
>> + if (pkt->error_type != HFI_ERR_NONE) {
>> + iris_change_core_state(core, IRIS_CORE_ERROR);
>> + return;
>> + }
>> +
>> + complete(&core->core_init_done);
>> +}
>> +
>> +static void
>> +iris_hfi_gen1_sys_get_prop_image_version(struct iris_core *core,
>> + struct hfi_msg_sys_property_info_pkt *pkt)
>> +{
>> + char fw_version[IRIS_FW_VERSION_LENGTH];
>> + u8 *str_image_version;
>> + int req_bytes;
>> + u32 i;
>> +
>> + req_bytes = pkt->hdr.size - sizeof(*pkt);
>> +
>> + if (req_bytes < IRIS_FW_VERSION_LENGTH - 1 || !pkt->data[0] ||
>> pkt->num_properties > 1)
>> + /* bad packet */
>> + return;
>> +
>> + str_image_version = pkt->data;
>> + if (!str_image_version)
>> + return;
>> +
>> + for (i = 0; i < IRIS_FW_VERSION_LENGTH - 1; i++) {
>> + if (str_image_version[i] != '\0')
>> + fw_version[i] = str_image_version[i];
>> + else
>> + fw_version[i] = ' ';
>> + }
>> + fw_version[i] = '\0';
>> +
>> + dev_dbg(core->dev, "firmware version: %s\n", fw_version);
>> +}
>
> You silently fail here alot in this function i.e. it returns void if it works or
> it doesn't work.
>
> Either make this an integer returning a pass/fail state or make the failure path
> visible with a jump to a dev_dbg or a dev_err or any other sort of printout that
> is either always an error or an error visible in debug mode so that such a
> failure can be found and fixed.
Ok, sounds good.
>
>> +
>> +static void iris_hfi_gen1_sys_property_info(struct iris_core *core, void
>> *packet)
>> +{
>> + struct hfi_msg_sys_property_info_pkt *pkt = packet;
>> +
>> + if (!pkt->num_properties) {
>> + dev_dbg(core->dev, "no properties\n");
>> + return;
>> + }
>> +
>> + switch (pkt->property) {
>> + case HFI_PROPERTY_SYS_IMAGE_VERSION:
>> + iris_hfi_gen1_sys_get_prop_image_version(core, pkt);
>> + break;
>> + default:
>> + dev_dbg(core->dev, "unknown property data\n");
>> + break;
>> + }
>> +}
>> +
>> +struct iris_hfi_gen1_response_pkt_info {
>> + u32 pkt;
>> + u32 pkt_sz;
>> +};
>> +
>> +static const struct iris_hfi_gen1_response_pkt_info pkt_infos[] = {
>> + {
>> + .pkt = HFI_MSG_EVENT_NOTIFY,
>> + .pkt_sz = sizeof(struct hfi_msg_event_notify_pkt),
>> + },
>> + {
>> + .pkt = HFI_MSG_SYS_INIT,
>> + .pkt_sz = sizeof(struct hfi_msg_sys_init_done_pkt),
>> + },
>> + {
>> + .pkt = HFI_MSG_SYS_PROPERTY_INFO,
>> + .pkt_sz = sizeof(struct hfi_msg_sys_property_info_pkt),
>> + },
>> +};
>> +
>> +static void iris_hfi_gen1_handle_response(struct iris_core *core, void
>> *response)
>> +{
>> + const struct iris_hfi_gen1_response_pkt_info *pkt_info;
>> + struct device *dev = core->dev;
>> + struct hfi_pkt_hdr *hdr;
>> + bool found = false;
>> + unsigned int i;
>> +
>> + hdr = (struct hfi_pkt_hdr *)response;
>> +
>> + for (i = 0; i < ARRAY_SIZE(pkt_infos); i++) {
>> + pkt_info = &pkt_infos[i];
>> + if (pkt_info->pkt != hdr->pkt_type)
>> + continue;
>> + found = true;
>> + break;
>> + }
>> +
>> + if (!found || hdr->size < pkt_info->pkt_sz) {
>> + dev_err(dev, "bad packet size (%d should be %d, pkt type:%x, found
>> %d)\n",
>> + hdr->size, pkt_info->pkt_sz, hdr->pkt_type, found);
>> +
>> + return;
>> + }
>> +
>> + if (hdr->pkt_type == HFI_MSG_SYS_INIT)
>> + iris_hfi_gen1_sys_init_done(core, hdr);
>> + else if (hdr->pkt_type == HFI_MSG_SYS_PROPERTY_INFO)
>> + iris_hfi_gen1_sys_property_info(core, hdr);
>> + else if (hdr->pkt_type == HFI_MSG_EVENT_NOTIFY)
>> + iris_hfi_gen1_sys_event_notify(core, hdr);
>> +}
>> +
>> +static void iris_hfi_gen1_flush_debug_queue(struct iris_core *core, u8 *packet)
>> +{
>> + struct hfi_msg_sys_coverage_pkt *pkt;
>> +
>> + while (!iris_hfi_queue_dbg_read(core, packet)) {
>> + pkt = (struct hfi_msg_sys_coverage_pkt *)packet;
>> +
>> + if (pkt->hdr.pkt_type != HFI_MSG_SYS_COV) {
>> + struct hfi_msg_sys_debug_pkt *pkt =
>> + (struct hfi_msg_sys_debug_pkt *)packet;
>> +
>> + dev_dbg(core->dev, "%s", pkt->msg_data);
>> + }
>> + }
>
> This loop looks funny. What's the intention here, to execute this loop so long
> as iris_hfi_queue_read() is empty, basically ?
Yes, thats correct.
>
> Loads of questions like how to you guarantee that happens ? Why return -ENODATA
> if the queue is not empty.
For debug queue, video firmware would write the debug packets while driver would
read them. When the read and write indices are same, loop would break and
indicates no data to read.
>
> But mostly I'd like to know how you know this loop will break ?
>
> Same question for the other usage of iris_hfi_queue_dbg_read().
Same logic as explained above.
>
>> +}
>> +
>> +static void iris_hfi_gen1_response_handler(struct iris_core *core)
>> +{
>> + memset(core->response_packet, 0, sizeof(struct hfi_pkt_hdr));
>> + while (!iris_hfi_queue_msg_read(core, core->response_packet)) {
>> + iris_hfi_gen1_handle_response(core, core->response_packet);
>> + if (core->state != IRIS_CORE_INIT)
>> + break;
>> +
>> + memset(core->response_packet, 0, sizeof(struct hfi_pkt_hdr));
>> + }
>> +
>> + iris_hfi_gen1_flush_debug_queue(core, core->response_packet);
>> +}
>> +
>> +static const struct iris_hfi_response_ops iris_hfi_gen1_response_ops = {
>> + .hfi_response_handler = iris_hfi_gen1_response_handler,
>> +};
>> +
>> +void iris_hfi_gen1_response_ops_init(struct iris_core *core)
>> +{
>> + core->hfi_response_ops = &iris_hfi_gen1_response_ops;
>> +}
>> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen2.h
>> b/drivers/media/platform/qcom/iris/iris_hfi_gen2.h
>> index 4f9748cbe0e3..6ec83984fda9 100644
>> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen2.h
>> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen2.h
>> @@ -8,6 +8,8 @@
>> #include "iris_instance.h"
>> +struct iris_core;
>> +
>> /**
>> * struct iris_inst_hfi_gen2 - holds per video instance parameters for hfi_gen2
>> *
>> @@ -17,6 +19,8 @@ struct iris_inst_hfi_gen2 {
>> struct iris_inst inst;
>> };
>> +void iris_hfi_gen2_command_ops_init(struct iris_core *core);
>> +void iris_hfi_gen2_response_ops_init(struct iris_core *core);
>> struct iris_inst *iris_hfi_gen2_get_instance(void);
>> #endif
>> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen2_command.c
>> b/drivers/media/platform/qcom/iris/iris_hfi_gen2_command.c
>> index 3ee33c8befae..807266858d93 100644
>> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen2_command.c
>> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen2_command.c
>> @@ -4,6 +4,78 @@
>> */
>> #include "iris_hfi_gen2.h"
>> +#include "iris_hfi_gen2_packet.h"
>> +
>> +#define NUM_SYS_INIT_PACKETS 8
>> +
>> +static int iris_hfi_gen2_sys_init(struct iris_core *core)
>> +{
>> + struct iris_hfi_header *hdr;
>> + u32 packet_size;
>> + int ret;
>> +
>> + packet_size = sizeof(*hdr) +
>> + NUM_SYS_INIT_PACKETS * (sizeof(struct iris_hfi_packet) + sizeof(u32));
>
> You can just make that into a define
>
> sizeof(*hdr) == sizeof (struct iris_hfi_header) - fixed
> NUM_SYS_INIT_PACKETS = define already and is fixed
> (sizeof(struct iris_hfi_packet) + sizeof(u32) also fixed
>
> There's nothing to calculate here - you can just bung it into a define at the
> top of the file and use the resulting HFI_PACKET_SIZE directly.
Looks good.
>
>> + hdr = kzalloc(packet_size, GFP_KERNEL);
>> + if (!hdr)
>> + return -ENOMEM;
>> +
>> + iris_hfi_gen2_packet_sys_init(core, hdr);
>> + ret = iris_hfi_queue_cmd_write_locked(core, hdr, hdr->size);
>> +
>> + kfree(hdr);
>> +
>> + return ret;
>> +}
>> +
>> +static int iris_hfi_gen2_sys_image_version(struct iris_core *core)
>> +{
>> + struct iris_hfi_header *hdr;
>> + u32 packet_size;
>> + int ret;
>> +
>> + packet_size = sizeof(*hdr) + sizeof(struct iris_hfi_packet);
>
> ditto
>
>> + hdr = kzalloc(packet_size, GFP_KERNEL);
>> + if (!hdr)
>> + return -ENOMEM;
>> +
>> + iris_hfi_gen2_packet_image_version(core, hdr);
>> + ret = iris_hfi_queue_cmd_write_locked(core, hdr, hdr->size);
>> +
>> + kfree(hdr);
>> +
>> + return ret;
>> +}
>> +
>> +static int iris_hfi_gen2_sys_interframe_powercollapse(struct iris_core *core)
>> +{
>> + struct iris_hfi_header *hdr;
>> + u32 packet_size;
>> + int ret;
>> +
>> + packet_size = sizeof(*hdr) + sizeof(struct iris_hfi_packet) + sizeof(u32);
>> + hdr = kzalloc(packet_size, GFP_KERNEL);
>> + if (!hdr)
>> + return -ENOMEM;
>> +
>> + iris_hfi_gen2_packet_sys_interframe_powercollapse(core, hdr);
>> + ret = iris_hfi_queue_cmd_write_locked(core, hdr, hdr->size);
>> +
>> + kfree(hdr);
>> +
>> + return ret;
>> +}
>> +
>> +static const struct iris_hfi_command_ops iris_hfi_gen2_command_ops = {
>> + .sys_init = iris_hfi_gen2_sys_init,
>> + .sys_image_version = iris_hfi_gen2_sys_image_version,
>> + .sys_interframe_powercollapse = iris_hfi_gen2_sys_interframe_powercollapse,
>> +};
>> +
>> +void iris_hfi_gen2_command_ops_init(struct iris_core *core)
>> +{
>> + core->hfi_ops = &iris_hfi_gen2_command_ops;
>> +}
>> struct iris_inst *iris_hfi_gen2_get_instance(void)
>> {
>> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen2_defines.h
>> b/drivers/media/platform/qcom/iris/iris_hfi_gen2_defines.h
>> new file mode 100644
>> index 000000000000..3e3e4ddfe21f
>> --- /dev/null
>> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen2_defines.h
>> @@ -0,0 +1,46 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +#ifndef _IRIS_HFI_GEN2_DEFINES_H_
>> +#define _IRIS_HFI_GEN2_DEFINES_H_
>> +
>> +#include <linux/types.h>
>> +
>> +#define HFI_VIDEO_ARCH_LX 0x1
>> +
>> +#define HFI_CMD_BEGIN 0x01000000
>> +#define HFI_CMD_INIT 0x01000001
>> +#define HFI_CMD_END 0x01FFFFFF
>> +
>> +#define HFI_PROP_BEGIN 0x03000000
>> +#define HFI_PROP_IMAGE_VERSION 0x03000001
>> +#define HFI_PROP_INTRA_FRAME_POWER_COLLAPSE 0x03000002
>> +#define HFI_PROP_UBWC_MAX_CHANNELS 0x03000003
>> +#define HFI_PROP_UBWC_MAL_LENGTH 0x03000004
>> +#define HFI_PROP_UBWC_HBB 0x03000005
>> +#define HFI_PROP_UBWC_BANK_SWZL_LEVEL1 0x03000006
>> +#define HFI_PROP_UBWC_BANK_SWZL_LEVEL2 0x03000007
>> +#define HFI_PROP_UBWC_BANK_SWZL_LEVEL3 0x03000008
>> +#define HFI_PROP_UBWC_BANK_SPREADING 0x03000009
>> +#define HFI_PROP_END 0x03FFFFFF
>> +
>> +#define HFI_SYSTEM_ERROR_BEGIN 0x05000000
>> +#define HFI_SYS_ERROR_WD_TIMEOUT 0x05000001
>> +#define HFI_SYSTEM_ERROR_END 0x05FFFFFF
>> +
>> +enum hfi_packet_firmware_flags {
>> + HFI_FW_FLAGS_SUCCESS = 0x00000001,
>> + HFI_FW_FLAGS_INFORMATION = 0x00000002,
>> + HFI_FW_FLAGS_SESSION_ERROR = 0x00000004,
>> + HFI_FW_FLAGS_SYSTEM_ERROR = 0x00000008,
>> +};
>> +
>> +struct hfi_debug_header {
>> + u32 size;
>> + u32 debug_level;
>> + u32 reserved[2];
>> +};
>> +
>> +#endif
>> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen2_packet.c
>> b/drivers/media/platform/qcom/iris/iris_hfi_gen2_packet.c
>> new file mode 100644
>> index 000000000000..8266eae5ff94
>> --- /dev/null
>> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen2_packet.c
>> @@ -0,0 +1,164 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +#include "iris_hfi_common.h"
>> +#include "iris_hfi_gen2.h"
>> +#include "iris_hfi_gen2_packet.h"
>> +
>> +static void iris_hfi_gen2_create_header(struct iris_hfi_header *hdr,
>> + u32 session_id, u32 header_id)
>> +{
>> + memset(hdr, 0, sizeof(*hdr));
>> +
>> + hdr->size = sizeof(*hdr);
>> + hdr->session_id = session_id;
>> + hdr->header_id = header_id;
>> + hdr->num_packets = 0;
>> +}
>> +
>> +static void iris_hfi_gen2_create_packet(struct iris_hfi_header *hdr, u32
>> pkt_type,
>> + u32 pkt_flags, u32 payload_type, u32 port,
>> + u32 packet_id, void *payload, u32 payload_size)
>> +{
>> + struct iris_hfi_packet *pkt;
>> + u32 pkt_size;
>> +
>> + pkt = (struct iris_hfi_packet *)((u8 *)hdr + hdr->size);
>> + pkt_size = sizeof(*pkt) + payload_size;
>> +
>> + memset(pkt, 0, pkt_size);
>> + pkt->size = pkt_size;
>> + pkt->type = pkt_type;
>> + pkt->flags = pkt_flags;
>> + pkt->payload_info = payload_type;
>> + pkt->port = port;
>> + pkt->packet_id = packet_id;
>> + if (payload_size)
>> + memcpy(&pkt->payload[0], payload, payload_size);
>
> Do you know that the bounds here are always correct => sizeof(pkt->payload) >=
> payload_size always ?
payload_size would be either of sizeof(u32) or sizeof(iris_hfi_buffer). We can
safely consider it as less than size of pkt->payload.
>
>> +
>> + hdr->num_packets++;
>> + hdr->size += pkt->size;
>> +}
>> +
>> +void iris_hfi_gen2_packet_sys_init(struct iris_core *core, struct
>> iris_hfi_header *hdr)
>> +{
>> + u32 payload = 0;
>> +
>> + iris_hfi_gen2_create_header(hdr, 0, core->header_id++);
>> +
>> + payload = HFI_VIDEO_ARCH_LX;
>> + iris_hfi_gen2_create_packet(hdr,
>> + HFI_CMD_INIT,
>> + (HFI_HOST_FLAGS_RESPONSE_REQUIRED |
>> + HFI_HOST_FLAGS_INTR_REQUIRED |
>> + HFI_HOST_FLAGS_NON_DISCARDABLE),
>> + HFI_PAYLOAD_U32,
>> + HFI_PORT_NONE,
>> + core->packet_id++,
>> + &payload,
>> + sizeof(u32));
>> +
>> + payload = core->iris_platform_data->ubwc_config->max_channels;
>> + iris_hfi_gen2_create_packet(hdr,
>> + HFI_PROP_UBWC_MAX_CHANNELS,
>> + HFI_HOST_FLAGS_NONE,
>> + HFI_PAYLOAD_U32,
>> + HFI_PORT_NONE,
>> + core->packet_id++,
>> + &payload,
>> + sizeof(u32));
>> +
>> + payload = core->iris_platform_data->ubwc_config->mal_length;
>> + iris_hfi_gen2_create_packet(hdr,
>> + HFI_PROP_UBWC_MAL_LENGTH,
>> + HFI_HOST_FLAGS_NONE,
>> + HFI_PAYLOAD_U32,
>> + HFI_PORT_NONE,
>> + core->packet_id++,
>> + &payload,
>> + sizeof(u32));
>> +
>> + payload = core->iris_platform_data->ubwc_config->highest_bank_bit;
>> + iris_hfi_gen2_create_packet(hdr,
>> + HFI_PROP_UBWC_HBB,
>> + HFI_HOST_FLAGS_NONE,
>> + HFI_PAYLOAD_U32,
>> + HFI_PORT_NONE,
>> + core->packet_id++,
>> + &payload,
>> + sizeof(u32));
>> +
>> + payload = core->iris_platform_data->ubwc_config->bank_swzl_level;
>> + iris_hfi_gen2_create_packet(hdr,
>> + HFI_PROP_UBWC_BANK_SWZL_LEVEL1,
>> + HFI_HOST_FLAGS_NONE,
>> + HFI_PAYLOAD_U32,
>> + HFI_PORT_NONE,
>> + core->packet_id++,
>> + &payload,
>> + sizeof(u32));
>> +
>> + payload = core->iris_platform_data->ubwc_config->bank_swz2_level;
>> + iris_hfi_gen2_create_packet(hdr,
>> + HFI_PROP_UBWC_BANK_SWZL_LEVEL2,
>> + HFI_HOST_FLAGS_NONE,
>> + HFI_PAYLOAD_U32,
>> + HFI_PORT_NONE,
>> + core->packet_id++,
>> + &payload,
>> + sizeof(u32));
>> +
>> + payload = core->iris_platform_data->ubwc_config->bank_swz3_level;
>> + iris_hfi_gen2_create_packet(hdr,
>> + HFI_PROP_UBWC_BANK_SWZL_LEVEL3,
>> + HFI_HOST_FLAGS_NONE,
>> + HFI_PAYLOAD_U32,
>> + HFI_PORT_NONE,
>> + core->packet_id++,
>> + &payload,
>> + sizeof(u32));
>> +
>> + payload = core->iris_platform_data->ubwc_config->bank_spreading;
>> + iris_hfi_gen2_create_packet(hdr,
>> + HFI_PROP_UBWC_BANK_SPREADING,
>> + HFI_HOST_FLAGS_NONE,
>> + HFI_PAYLOAD_U32,
>> + HFI_PORT_NONE,
>> + core->packet_id++,
>> + &payload,
>> + sizeof(u32));
>> +}
>> +
>> +void iris_hfi_gen2_packet_image_version(struct iris_core *core, struct
>> iris_hfi_header *hdr)
>> +{
>> + iris_hfi_gen2_create_header(hdr, 0, core->header_id++);
>> +
>> + iris_hfi_gen2_create_packet(hdr,
>> + HFI_PROP_IMAGE_VERSION,
>> + (HFI_HOST_FLAGS_RESPONSE_REQUIRED |
>> + HFI_HOST_FLAGS_INTR_REQUIRED |
>> + HFI_HOST_FLAGS_GET_PROPERTY),
>> + HFI_PAYLOAD_NONE,
>> + HFI_PORT_NONE,
>> + core->packet_id++,
>> + NULL, 0);
>> +}
>> +
>> +void iris_hfi_gen2_packet_sys_interframe_powercollapse(struct iris_core *core,
>> + struct iris_hfi_header *hdr)
>> +{
>> + u32 payload = 1; /* HFI_TRUE */
>> +
>> + iris_hfi_gen2_create_header(hdr, 0 /*session_id*/, core->header_id++);
>> +
>> + iris_hfi_gen2_create_packet(hdr,
>> + HFI_PROP_INTRA_FRAME_POWER_COLLAPSE,
>> + HFI_HOST_FLAGS_NONE,
>> + HFI_PAYLOAD_U32,
>> + HFI_PORT_NONE,
>> + core->packet_id++,
>> + &payload,
>> + sizeof(u32));
>> +}
>> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen2_packet.h
>> b/drivers/media/platform/qcom/iris/iris_hfi_gen2_packet.h
>> new file mode 100644
>> index 000000000000..eba109efeb76
>> --- /dev/null
>> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen2_packet.h
>> @@ -0,0 +1,69 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +#ifndef _IRIS_HFI_GEN2_PACKET_H_
>> +#define _IRIS_HFI_GEN2_PACKET_H_
>> +
>> +#include "iris_hfi_gen2_defines.h"
>> +
>> +struct iris_core;
>> +
>> +/**
>> + * struct iris_hfi_header
>> + *
>> + * @size: size of the total packet in bytes including hfi_header
>> + * @session_id: For session level hfi_header session_id is non-zero.
>> + * For system level hfi_header session_id is zero.
>> + * @header_id: unique header id for each hfi_header
>> + * @reserved: reserved for future use
>> + * @num_packets: number of hfi_packet that are included with the hfi_header
>> + */
>> +struct iris_hfi_header {
>> + u32 size;
>> + u32 session_id;
>> + u32 header_id;
>> + u32 reserved[4];
>> + u32 num_packets;
>> +};
>> +
>> +/**
>> + * struct iris_hfi_packet
>> + *
>> + * @size: size of the hfi_packet in bytes including payload
>> + * @type: one of the below hfi_packet types:
>> + * HFI_CMD_*,
>> + * HFI_PROP_*,
>> + * HFI_ERROR_*,
>> + * HFI_INFO_*,
>> + * HFI_SYS_ERROR_*
>> + * @flags: hfi_packet flags. It is represented as bit masks.
>> + * host packet flags are "enum hfi_packet_host_flags"
>> + * firmware packet flags are "enum hfi_packet_firmware_flags"
>> + * @payload_info: payload information indicated by "enum
>> hfi_packet_payload_info"
>> + * @port: hfi_packet port type indicated by "enum hfi_packet_port_type"
>> + * This is bitmask and may be applicable to multiple ports.
>> + * @packet_id: host hfi_packet contains unique packet id.
>> + * firmware returns host packet id in response packet
>> + * wherever applicable. If not applicable firmware sets it to zero.
>> + * @reserved: reserved for future use.
>> + * @payload: flexible array of payload having additional packet information.
>> + */
>> +struct iris_hfi_packet {
>> + u32 size;
>> + u32 type;
>> + u32 flags;
>> + u32 payload_info;
>> + u32 port;
>> + u32 packet_id;
>> + u32 reserved[2];
>> + u32 payload[];
>> +};
>> +
>> +void iris_hfi_gen2_packet_sys_init(struct iris_core *core, struct
>> iris_hfi_header *hdr);
>> +void iris_hfi_gen2_packet_image_version(struct iris_core *core, struct
>> iris_hfi_header *hdr);
>> +void iris_hfi_gen2_packet_sys_interframe_powercollapse(struct iris_core *core,
>> + struct iris_hfi_header *hdr);
>> +
>> +#endif
>> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen2_response.c
>> b/drivers/media/platform/qcom/iris/iris_hfi_gen2_response.c
>> new file mode 100644
>> index 000000000000..e208a5ae664a
>> --- /dev/null
>> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen2_response.c
>> @@ -0,0 +1,229 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +#include "iris_hfi_gen2.h"
>> +#include "iris_hfi_gen2_defines.h"
>> +#include "iris_hfi_gen2_packet.h"
>> +#include "iris_vpu_common.h"
>> +
>> +struct iris_hfi_gen2_core_hfi_range {
>> + u32 begin;
>> + u32 end;
>> + int (*handle)(struct iris_core *core, struct iris_hfi_packet *pkt);
>> +};
>> +
>> +static int iris_hfi_gen2_validate_packet(u8 *response_pkt, u8 *core_resp_pkt)
>> +{
>> + u32 response_pkt_size = 0;
>> + u8 *response_limit;
>> +
>> + response_limit = core_resp_pkt + IFACEQ_CORE_PKT_SIZE;
>> +
>> + response_pkt_size = *(u32 *)response_pkt;
>> + if (!response_pkt_size)
>> + return -EINVAL;
>> +
>> + if (response_pkt_size < sizeof(struct iris_hfi_packet))
>> + return -EINVAL;
>> +
>> + if (response_pkt + response_pkt_size > response_limit)
>> + return -EINVAL;
>> +
>> + return 0;
>> +}
>> +
>> +static int iris_hfi_gen2_validate_hdr_packet(struct iris_core *core, struct
>> iris_hfi_header *hdr)
>> +{
>> + struct iris_hfi_packet *packet;
>> + int i, ret = 0;
>> + u8 *pkt;
>> +
>> + if (hdr->size < sizeof(*hdr) + sizeof(*packet))
>> + return -EINVAL;
>> +
>> + pkt = (u8 *)((u8 *)hdr + sizeof(*hdr));
>> +
>> + for (i = 0; i < hdr->num_packets; i++) {
>> + packet = (struct iris_hfi_packet *)pkt;
>> + ret = iris_hfi_gen2_validate_packet(pkt, core->response_packet);
>> + if (ret)
>> + return ret;
>> +
>> + pkt += packet->size;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static int iris_hfi_gen2_handle_system_error(struct iris_core *core,
>> + struct iris_hfi_packet *pkt)
>> +{
>> + dev_err(core->dev, "received system error of type %#x\n", pkt->type);
>> +
>> + iris_change_core_state(core, IRIS_CORE_ERROR);
>> + schedule_delayed_work(&core->sys_error_handler, msecs_to_jiffies(10));
>> +
>> + return 0;
>> +}
>> +
>> +static int iris_hfi_gen2_handle_system_init(struct iris_core *core,
>> + struct iris_hfi_packet *pkt)
>> +{
>> + if (!(pkt->flags & HFI_FW_FLAGS_SUCCESS)) {
>> + iris_change_core_state(core, IRIS_CORE_ERROR);
>> + return 0;
>> + }
>> +
>> + complete(&core->core_init_done);
>> +
>> + return 0;
>> +}
>> +
>> +static int iris_hfi_gen2_handle_image_version_property(struct iris_core *core,
>> + struct iris_hfi_packet *pkt)
>> +{
>> + char fw_version[IRIS_FW_VERSION_LENGTH];
>> + u8 *str_image_version;
>> + u32 req_bytes;
>> + u32 i = 0;
>> +
>> + req_bytes = pkt->size - sizeof(*pkt);
>> + if (req_bytes < IRIS_FW_VERSION_LENGTH - 1)
>> + return -EINVAL;
>> +
>> + str_image_version = (u8 *)pkt + sizeof(*pkt);
>> +
>> + for (i = 0; i < IRIS_FW_VERSION_LENGTH - 1; i++) {
>> + if (str_image_version[i] != '\0')
>> + fw_version[i] = str_image_version[i];
>> + else
>> + fw_version[i] = ' ';
>> + }
>> + fw_version[i] = '\0';
>> +
>> + dev_dbg(core->dev, "firmware version: %s\n", fw_version);
>> +
>> + return 0;
>> +}
>> +
>> +static int iris_hfi_gen2_handle_system_property(struct iris_core *core,
>> + struct iris_hfi_packet *pkt)
>> +{
>> + int ret = 0;
>> +
>> + switch (pkt->type) {
>> + case HFI_PROP_IMAGE_VERSION:
>> + ret = iris_hfi_gen2_handle_image_version_property(core, pkt);
>> + break;
>> + default:
>> + break;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static int iris_hfi_gen2_handle_system_response(struct iris_core *core,
>> + struct iris_hfi_header *hdr)
>> +{
>> + struct iris_hfi_packet *packet;
>> + u8 *pkt, *start_pkt;
>> + int ret = 0;
>> + int i, j;
>> + static const struct iris_hfi_gen2_core_hfi_range range[] = {
>> + {HFI_SYSTEM_ERROR_BEGIN, HFI_SYSTEM_ERROR_END,
>> iris_hfi_gen2_handle_system_error },
>> + {HFI_PROP_BEGIN, HFI_PROP_END,
>> iris_hfi_gen2_handle_system_property },
>> + {HFI_CMD_BEGIN, HFI_CMD_END,
>> iris_hfi_gen2_handle_system_init },
>> + };
>> +
>> + start_pkt = (u8 *)((u8 *)hdr + sizeof(*hdr));
>> + for (i = 0; i < ARRAY_SIZE(range); i++) {
>> + pkt = start_pkt;
>> + for (j = 0; j < hdr->num_packets; j++) {
>> + packet = (struct iris_hfi_packet *)pkt;
>> + if (packet->flags & HFI_FW_FLAGS_SYSTEM_ERROR) {
>> + ret = iris_hfi_gen2_handle_system_error(core, packet);
>> + return ret;
>> + }
>> +
>> + if (packet->type > range[i].begin && packet->type < range[i].end) {
>> + ret = range[i].handle(core, packet);
>> + if (ret)
>> + return ret;
>> +
>> + if (packet->type > HFI_SYSTEM_ERROR_BEGIN &&
>> + packet->type < HFI_SYSTEM_ERROR_END)
>> + return 0;
>> + }
>> + pkt += packet->size;
>> + }
>> + }
>
> You step the pkt pointer on each iteration of the j loop but then reinitialise
> it to the original start_pkt inside of each i loop.
>
> So you'll always process the same packet in the i loop no ?
>
> Is that the intention ?
Yes, thats the idea. Driver should parse system error among all the packets,
followed by properties associated then the command.
>> +
>> + return ret;
>> +}
>> +
>> +static int iris_hfi_gen2_handle_response(struct iris_core *core, void *response)
>> +{
>> + struct iris_hfi_header *hdr;
>> + int ret;
>> +
>> + hdr = (struct iris_hfi_header *)response;
>> + ret = iris_hfi_gen2_validate_hdr_packet(core, hdr);
>> + if (ret)
>> + return iris_hfi_gen2_handle_system_error(core, NULL);
>> +
>> + return iris_hfi_gen2_handle_system_response(core, hdr);
>> +}
>> +
>> +static void iris_hfi_gen2_flush_debug_queue(struct iris_core *core, u8 *packet)
>> +{
>> + struct hfi_debug_header *pkt;
>> + u8 *log;
>> +
>> + while (!iris_hfi_queue_dbg_read(core, packet)) {
>> + pkt = (struct hfi_debug_header *)packet;
>> +
>> + if (pkt->size < sizeof(*pkt))
>> + continue;
>> +
>> + if (pkt->size >= IFACEQ_CORE_PKT_SIZE)
>> + continue;
>> +
>> + packet[pkt->size] = '\0';
>> + log = (u8 *)packet + sizeof(*pkt) + 1;
>> + dev_dbg(core->dev, "%s", log);
>> + }
>> +}
>
> This more of a busy/wait than a flush. I asked previously how you know the other
> usage of iris_hfi_queue_dbg_read() would break. Similar question here, also is
> the "popping" of the log/stack/whatever-you-call-it that
> iris_hfi_queue_dbg_read() consumes immediate or can it stall ?
>
> Do you need to have some kind of delay between reads ?
The idea is to read the debug packets while handling response from firmware.
Lets say driver read till read index equals write index, and there are more
debug packets written later, then the same would be popped in next response
handling.
>
> I really asking how you know this loop terminates, if it needs to be
> error-checked to ensure it terminates and if we you need "inter-frame" delays -
> to stop the CPU spinning while the data is delivered up ?
>
>
>> +
>> +static void iris_hfi_gen2_response_handler(struct iris_core *core)
>> +{
>> + if (iris_vpu_watchdog(core, core->intr_status)) {
>> + struct iris_hfi_packet pkt = {.type = HFI_SYS_ERROR_WD_TIMEOUT};
>> +
>> + dev_err(core->dev, "cpu watchdog error received\n");
>> + iris_change_core_state(core, IRIS_CORE_ERROR);
>> + iris_hfi_gen2_handle_system_error(core, &pkt);
>> +
>> + return;
>> + }
>> +
>> + memset(core->response_packet, 0, sizeof(struct iris_hfi_header));
>> + while (!iris_hfi_queue_msg_read(core, core->response_packet)) {
>> + iris_hfi_gen2_handle_response(core, core->response_packet);
>> + if (core->state != IRIS_CORE_INIT)
>> + break;
>> + memset(core->response_packet, 0, sizeof(struct iris_hfi_header));
>> + }
>> +
>> + iris_hfi_gen2_flush_debug_queue(core, core->response_packet);
>> +}
>> +
>> +static const struct iris_hfi_response_ops iris_hfi_gen2_response_ops = {
>> + .hfi_response_handler = iris_hfi_gen2_response_handler,
>> +};
>> +
>> +void iris_hfi_gen2_response_ops_init(struct iris_core *core)
>> +{
>> + core->hfi_response_ops = &iris_hfi_gen2_response_ops;
>> +}
>> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_queue.c
>> b/drivers/media/platform/qcom/iris/iris_hfi_queue.c
>> index 11938880b8cd..b24d4640fea9 100644
>> --- a/drivers/media/platform/qcom/iris/iris_hfi_queue.c
>> +++ b/drivers/media/platform/qcom/iris/iris_hfi_queue.c
>> @@ -5,6 +5,207 @@
>> #include "iris_core.h"
>> #include "iris_hfi_queue.h"
>> +#include "iris_vpu_common.h"
>> +
>> +static int iris_hfi_queue_write(struct iris_iface_q_info *qinfo, void
>> *packet, u32 packet_size)
>> +{
>> + u32 empty_space, read_idx, write_idx, new_write_idx;
>> + struct iris_hfi_queue_header *queue;
>> + u32 *write_ptr;
>> + u32 residue;
>> +
>> + queue = qinfo->qhdr;
>> +
>> + read_idx = queue->read_idx * sizeof(u32);
>> + write_idx = queue->write_idx * sizeof(u32);
>> +
>> + if (write_idx < read_idx)
>> + empty_space = read_idx - write_idx;
>> + else
>> + empty_space = IFACEQ_QUEUE_SIZE - (write_idx - read_idx);
>> + if (empty_space < packet_size)
>> + return -ENOSPC;
>> +
>> + queue->tx_req = 0;
>> +
>> + new_write_idx = write_idx + packet_size;
>> + write_ptr = (u32 *)((u8 *)qinfo->kernel_vaddr + write_idx);
>> +
>> + if (write_ptr < (u32 *)qinfo->kernel_vaddr ||
>> + write_ptr > (u32 *)(qinfo->kernel_vaddr +
>> + IFACEQ_QUEUE_SIZE))
>> + return -EINVAL;
>> +
>> + if (new_write_idx < IFACEQ_QUEUE_SIZE) {
>> + memcpy(write_ptr, packet, packet_size);
>> + } else {
>> + residue = new_write_idx - IFACEQ_QUEUE_SIZE;
>> + memcpy(write_ptr, packet, (packet_size - residue));
>> + memcpy(qinfo->kernel_vaddr,
>> + packet + (packet_size - residue), residue);
>> + new_write_idx = residue;
>> + }
>> +
>> + /* Make sure packet is written before updating the write index */
>> + mb();
>> + queue->write_idx = new_write_idx / sizeof(u32);
>> +
>> + /* Make sure write index is updated before an interrupt is raised */
>> + mb();
>> +
>> + return 0;
>> +}
>> +
>> +static int iris_hfi_queue_read(struct iris_iface_q_info *qinfo, void *packet)
>> +{
>> + u32 read_idx, write_idx, new_read_idx;
>> + struct iris_hfi_queue_header *queue;
>> + u32 packet_size, residue;
>> + u32 receive_request = 0;
>> + u32 *read_ptr;
>> + int ret = 0;
>> +
>> + queue = qinfo->qhdr;
>> +
>> + if (queue->queue_type == IFACEQ_MSGQ_ID)
>> + receive_request = 1;
>> +
>> + read_idx = queue->read_idx * sizeof(u32);
>> + write_idx = queue->write_idx * sizeof(u32);
>> +
>> + if (read_idx == write_idx) {
>> + queue->rx_req = receive_request;
>> + /* Ensure qhdr is updated in main memory */
>> + mb();
>> + return -ENODATA;
>> + }
>> +
>> + read_ptr = qinfo->kernel_vaddr + read_idx;
>> + if (read_ptr < (u32 *)qinfo->kernel_vaddr ||
>> + read_ptr > (u32 *)(qinfo->kernel_vaddr +
>> + IFACEQ_QUEUE_SIZE - sizeof(*read_ptr)))
>> + return -ENODATA;
>> +
>> + packet_size = *read_ptr;
>> + if (!packet_size)
>> + return -EINVAL;
>> +
>> + new_read_idx = read_idx + packet_size;
>> + if (packet_size <= IFACEQ_CORE_PKT_SIZE) {
>> + if (new_read_idx < IFACEQ_QUEUE_SIZE) {
>> + memcpy(packet, read_ptr, packet_size);
>> + } else {
>> + residue = new_read_idx - IFACEQ_QUEUE_SIZE;
>> + memcpy(packet, read_ptr, (packet_size - residue));
>> + memcpy((packet + (packet_size - residue)),
>> + qinfo->kernel_vaddr, residue);
>> + new_read_idx = residue;
>> + }
>> + } else {
>> + new_read_idx = write_idx;
>> + ret = -EBADMSG;
>> + }
>> +
>> + queue->rx_req = receive_request;
>> +
>> + queue->read_idx = new_read_idx / sizeof(u32);
>> + /* Ensure qhdr is updated in main memory */
>> + mb();
>> +
>> + return ret;
>> +}
>> +
>> +int iris_hfi_queue_cmd_write_locked(struct iris_core *core, void *pkt, u32
>> pkt_size)
>> +{
>> + struct iris_iface_q_info *q_info;
>> +
>> + if (!mutex_is_locked(&core->lock))
>> + return -EINVAL;
>
> Can this function be called without the mutex being locked ?
>
> If not then you should have some kind of dev_err() with this no ? Otherwise
> browsing the code here IDT this check is needed.
This being driver implicit calls, and as the api suggest to be invoked from
locked context, the check can be dropped.
>
> I'd suggest either drop or be more noisy about the bug you encountered.
>
>> +
>> + if (core->state != IRIS_CORE_INIT)
>> + return -EINVAL;
>> +
>> + q_info = &core->command_queue;
>> + if (!q_info || !q_info->kernel_vaddr || !pkt) {
>> + dev_err(core->dev, "cannot write to shared command queue\n");
>> + return -ENODATA;
>> + }
>> +
>> + if (!iris_hfi_queue_write(q_info, pkt, pkt_size)) {
>> + iris_vpu_raise_interrupt(core);
>> + } else {
>> + dev_err(core->dev, "queue full\n");
>> + return -ENODATA;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +int iris_hfi_queue_cmd_write(struct iris_core *core, void *pkt, u32 pkt_size)
>> +{
>> + int ret;
>> +
>> + mutex_lock(&core->lock);
>> + ret = iris_hfi_queue_cmd_write_locked(core, pkt, pkt_size);
>> + if (ret)
>> + dev_err(core->dev, "iris_hfi_queue_cmd_write_locked failed with
>> %d\n", ret);
>> +
>> + mutex_unlock(&core->lock);
>> +
>> + return ret;
>> +}
>> +
>> +int iris_hfi_queue_msg_read(struct iris_core *core, void *pkt)
>> +{
>> + struct iris_iface_q_info *q_info;
>> + int ret = 0;
>> +
>> + mutex_lock(&core->lock);
>> + if (core->state != IRIS_CORE_INIT) {
>> + ret = -EINVAL;
>> + goto unlock;
>> + }
>> +
>> + q_info = &core->message_queue;
>> + if (iris_hfi_queue_read(q_info, pkt)) {
>> + ret = -ENODATA;
>
> Its a very curious response code to return "ENODATA" when you actually have data..
On a good working scenario, NODATA would indicate when all packets are read.
>
> Why not return
>
> < 0 err code
> 0 no data
>> 0 data
>
> ?I do not see that info be useful to caller to make any informative decision. It
just cares for data (then continue) or exit.
>
>> + goto unlock;
>> + }
>> +
>> +unlock:
>> + mutex_unlock(&core->lock);
>> +
>> + return ret;
>> +}
>> +
>> +int iris_hfi_queue_dbg_read(struct iris_core *core, void *pkt)
>> +{
>> + struct iris_iface_q_info *q_info;
>> + int ret = 0;
>> +
>> + mutex_lock(&core->lock);
>> + if (core->state != IRIS_CORE_INIT) {
>> + ret = -EINVAL;
>> + goto unlock;
>> + }
>
> I'm going to challenge this logic here. Can this function be called except when
> state == IRIS_CORE_INIT ?
>
> Surely you should get into the IRIS_CORE_INIT state before progressing this far
> into your code's runtime ?
>
> In which case a plethora of runtime checks for the validatity of your
> state-machine are redundant.
>
> Far better to make a concrete transition from one state to another than to have
> a bunch of defensive coding checks which are redundant.
>
> A blanket statement for this driver.
>
> Please consider.
Certainly. As you know, during the offline reviews, there were few more such
checks and we have dropped them wherever not required. I do not see a possible
case for response thread processing with core in non-init state. Let me revisit
this part and update.
>
>> +
>> + q_info = &core->debug_queue;
>> + if (!q_info || !q_info->kernel_vaddr || !pkt) {
>> + dev_err(core->dev, "cannot read from shared debug queue\n");
>> + ret = -ENODATA;
>> + goto unlock;
>> + }
>> +
>> + if (iris_hfi_queue_read(q_info, pkt)) {
> Should this really be returning an error and shoiuld it be the same error as
> above ?
This is explained earlier in this discussion. Same applies here about returning
ENODATA.
>
>> + ret = -ENODATA;
>> + goto unlock;
>> + }
>> +
>> +unlock:
>> + mutex_unlock(&core->lock);
>> +
>> + return ret;
>> +}
>
> [1]
> Am I reading this function right. It returs -ENODATA to indicate both an error
> state and to indicate termination to the calling function ?
Yes, in both case, the caller would process the packets read so far and continue.
>
> ---
> bod
Regards,
Vikash
