Hi Bingbu,
On Tue, 2023-10-24 at 19:29 +0800, bingbu.cao@xxxxxxxxx wrote:
> From: Bingbu Cao <bingbu.cao@xxxxxxxxx>
>
> Input system driver do basic isys hardware setup and irq handling
> and work with fwnode and v4l2 to register the ISYS v4l2 devices.
>
> Signed-off-by: Bingbu Cao <bingbu.cao@xxxxxxxxx>
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> Reported-by: Claus Stovgaard <claus.stovgaard@xxxxxxxxx>
> ---
> drivers/media/pci/intel/ipu6/ipu6-isys.c | 1345 ++++++++++++++++++++++
> drivers/media/pci/intel/ipu6/ipu6-isys.h | 201 ++++
> 2 files changed, 1546 insertions(+)
> create mode 100644 drivers/media/pci/intel/ipu6/ipu6-isys.c
> create mode 100644 drivers/media/pci/intel/ipu6/ipu6-isys.h
>
> diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys.c b/drivers/media/pci/intel/ipu6/ipu6-isys.c
> new file mode 100644
> index 000000000000..2ea0c9ad6f16
> --- /dev/null
> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys.c
...
> +struct isys_fw_msgs *ipu6_get_fw_msg_buf(struct ipu6_isys_stream *stream)
> +{
> + struct ipu6_isys *isys = stream->isys;
> + struct device *dev = &isys->adev->auxdev.dev;
> + struct isys_fw_msgs *msg;
> + unsigned long flags;
> + int ret;
> +
> + spin_lock_irqsave(&isys->listlock, flags);
> + if (list_empty(&isys->framebuflist)) {
> + spin_unlock_irqrestore(&isys->listlock, flags);
> + dev_dbg(dev, "Frame list empty\n");
> +
> + ret = alloc_fw_msg_bufs(isys, 5);
> + if (ret < 0)
> + return NULL;
> +
> + spin_lock_irqsave(&isys->listlock, flags);
> + if (list_empty(&isys->framebuflist)) {
> + spin_unlock_irqrestore(&isys->listlock, flags);
> + dev_err(dev, "Frame list empty\n");
> + return NULL;
> + }
> + }
> + msg = list_last_entry(&isys->framebuflist, struct isys_fw_msgs, head);
> + list_move(&msg->head, &isys->framebuflist_fw);
> + spin_unlock_irqrestore(&isys->listlock, flags);
> + memset(&msg->fw_msg, 0, sizeof(msg->fw_msg));
> +
> + return msg;
> +}
> +
> +void ipu6_cleanup_fw_msg_bufs(struct ipu6_isys *isys)
> +{
> + struct isys_fw_msgs *fwmsg, *fwmsg0;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&isys->listlock, flags);
> + list_for_each_entry_safe(fwmsg, fwmsg0, &isys->framebuflist_fw, head)
> + list_move(&fwmsg->head, &isys->framebuflist);
> + spin_unlock_irqrestore(&isys->listlock, flags);
> +}
> +
> +void ipu6_put_fw_msg_buf(struct ipu6_isys *isys, u64 data)
> +{
> + struct isys_fw_msgs *msg;
> + unsigned long flags;
> + u64 *ptr = (u64 *)data;
> +
> + if (!ptr)
> + return;
> +
> + spin_lock_irqsave(&isys->listlock, flags);
> + msg = container_of(ptr, struct isys_fw_msgs, fw_msg.dummy);
The dummy field is only there to do this container_of, but ptr and msg
point to the same address, since the union dummy is in is the first
field. So the container_of is exactly the same as simply casting the
pointer.
> + list_move(&msg->head, &isys->framebuflist);
> + spin_unlock_irqrestore(&isys->listlock, flags);
> +}
...
> +static int isys_isr_one(struct ipu6_bus_device *adev)
> +{
> + struct ipu6_isys *isys = ipu6_bus_get_drvdata(adev);
> + struct ipu6_fw_isys_resp_info_abi *resp;
> + struct ipu6_isys_stream *stream;
> + struct ipu6_isys_csi2 *csi2 = NULL;
> + u64 ts;
> +
> + if (!isys->fwcom)
> + return 1;
> +
> + resp = ipu6_fw_isys_get_resp(isys->fwcom, IPU6_BASE_MSG_RECV_QUEUES);
> + if (!resp)
> + return 1;
> +
> + ts = (u64)resp->timestamp[1] << 32 | resp->timestamp[0];
> +
> + if (resp->error_info.error == IPU6_FW_ISYS_ERROR_STREAM_IN_SUSPENSION)
> + /* Suspension is kind of special case: not enough buffers */
> + dev_dbg(&adev->auxdev.dev,
> + "FW error resp %02d %s, stream %u, error SUSPENSION, details %d, timestamp 0x%16.16llx, pin %d\n",
> + resp->type,
> + fw_msg[resp_type_to_index(resp->type)].msg,
> + resp->stream_handle,
> + resp->error_info.error_details,
> + fw_msg[resp_type_to_index(resp->type)].valid_ts ?
> + ts : 0, resp->pin_id);
> + else if (resp->error_info.error)
> + dev_dbg(&adev->auxdev.dev,
> + "FW error resp %02d %s, stream %u, error %d, details %d, timestamp 0x%16.16llx, pin %d\n",
> + resp->type,
> + fw_msg[resp_type_to_index(resp->type)].msg,
> + resp->stream_handle,
> + resp->error_info.error, resp->error_info.error_details,
> + fw_msg[resp_type_to_index(resp->type)].valid_ts ?
> + ts : 0, resp->pin_id);
> + else
> + dev_dbg(&adev->auxdev.dev,
> + "FW resp %02d %s, stream %u, timestamp 0x%16.16llx, pin %d\n",
> + resp->type,
> + fw_msg[resp_type_to_index(resp->type)].msg,
> + resp->stream_handle,
> + fw_msg[resp_type_to_index(resp->type)].valid_ts ?
> + ts : 0, resp->pin_id);
> +
> + if (resp->stream_handle >= IPU6_ISYS_MAX_STREAMS) {
> + dev_err(&adev->auxdev.dev, "bad stream handle %u\n",
> + resp->stream_handle);
> + goto leave;
> + }
> +
> + stream = ipu6_isys_query_stream_by_handle(isys, resp->stream_handle);
> + if (!stream) {
> + dev_err(&adev->auxdev.dev, "stream of stream_handle %u is unused\n",
> + resp->stream_handle);
> + goto leave;
> + }
> + stream->error = resp->error_info.error;
> +
> + csi2 = ipu6_isys_subdev_to_csi2(stream->asd);
> +
> + switch (resp->type) {
> + case IPU6_FW_ISYS_RESP_TYPE_STREAM_OPEN_DONE:
> + complete(&stream->stream_open_completion);
> + break;
> + case IPU6_FW_ISYS_RESP_TYPE_STREAM_CLOSE_ACK:
> + complete(&stream->stream_close_completion);
> + break;
> + case IPU6_FW_ISYS_RESP_TYPE_STREAM_START_ACK:
> + complete(&stream->stream_start_completion);
> + break;
> + case IPU6_FW_ISYS_RESP_TYPE_STREAM_START_AND_CAPTURE_ACK:
> + complete(&stream->stream_start_completion);
> + break;
> + case IPU6_FW_ISYS_RESP_TYPE_STREAM_STOP_ACK:
> + complete(&stream->stream_stop_completion);
> + break;
> + case IPU6_FW_ISYS_RESP_TYPE_STREAM_FLUSH_ACK:
> + complete(&stream->stream_stop_completion);
> + break;
> + case IPU6_FW_ISYS_RESP_TYPE_PIN_DATA_READY:
> + /*
> + * firmware only release the capture msg until software
> + * get pin_data_ready event
> + */
> + ipu6_put_fw_msg_buf(ipu6_bus_get_drvdata(adev), resp->buf_id);
Is there a difference between IPU4 and IPU6 here? `buf_id` is always 0
on IPU4, so this doesn't release anything.
The PIN_DATA_READY response is sent multiple times per call to
ipu6_get_fw_msg_buf, so if I'm not mistaken, this can only be correct
if the firmware only sends the buf_id in one of the PIN_DATA_READY
responses.
The correct pointer to release seems to be passed in the various _ACK
responses on IPU4. I think it would make a lot more sense to release
the buffer there. But of course, if there is a bug in the firmware we
might have to do something else.
> + if (resp->pin_id < IPU6_ISYS_OUTPUT_PINS &&
> + stream->output_pins[resp->pin_id].pin_ready)
> + stream->output_pins[resp->pin_id].pin_ready(stream,
> + resp);
> + else
> + dev_warn(&adev->auxdev.dev,
> + "%d:No data pin ready handler for pin id %d\n",
> + resp->stream_handle, resp->pin_id);
> + if (csi2)
> + ipu6_isys_csi2_error(csi2);
> +
> + break;
> + case IPU6_FW_ISYS_RESP_TYPE_STREAM_CAPTURE_ACK:
> + break;
> + case IPU6_FW_ISYS_RESP_TYPE_STREAM_START_AND_CAPTURE_DONE:
> + case IPU6_FW_ISYS_RESP_TYPE_STREAM_CAPTURE_DONE:
> + break;
> + case IPU6_FW_ISYS_RESP_TYPE_FRAME_SOF:
> +
> + ipu6_isys_csi2_sof_event_by_stream(stream);
> + stream->seq[stream->seq_index].sequence =
> + atomic_read(&stream->sequence) - 1;
> + stream->seq[stream->seq_index].timestamp = ts;
> + dev_dbg(&adev->auxdev.dev,
> + "sof: handle %d: (index %u), timestamp 0x%16.16llx\n",
> + resp->stream_handle,
> + stream->seq[stream->seq_index].sequence, ts);
> + stream->seq_index = (stream->seq_index + 1)
> + % IPU6_ISYS_MAX_PARALLEL_SOF;
> + break;
> + case IPU6_FW_ISYS_RESP_TYPE_FRAME_EOF:
> + ipu6_isys_csi2_eof_event_by_stream(stream);
> + dev_dbg(&adev->auxdev.dev,
> + "eof: handle %d: (index %u), timestamp 0x%16.16llx\n",
> + resp->stream_handle,
> + stream->seq[stream->seq_index].sequence, ts);
> + break;
> + case IPU6_FW_ISYS_RESP_TYPE_STATS_DATA_READY:
> + break;
> + default:
> + dev_err(&adev->auxdev.dev, "%d:unknown response type %u\n",
> + resp->stream_handle, resp->type);
> + break;
> + }
> +
> + ipu6_isys_put_stream(stream);
> +leave:
> + ipu6_fw_isys_put_resp(isys->fwcom, IPU6_BASE_MSG_RECV_QUEUES);
> + return 0;
> +}
...
> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys.h
...
> +struct isys_fw_msgs {
> + union {
> + u64 dummy;
> + struct ipu6_fw_isys_frame_buff_set_abi frame;
> + struct ipu6_fw_isys_stream_cfg_data_abi stream;
> + } fw_msg;
> + struct list_head head;
> + dma_addr_t dma_addr;
> +};
> +
> +struct isys_fw_msgs *ipu6_get_fw_msg_buf(struct ipu6_isys_stream *stream);
> +void ipu6_put_fw_msg_buf(struct ipu6_isys *isys, u64 data);
I it would be easier to understand these functions if the
ipu6_put_fw_msg_buf function, took the same type that the get function
returns.
/Andreas
