On 2020-09-23 3:17 PM, Andy Shevchenko wrote:
On Wed, Sep 23, 2020 at 02:24:56PM +0200, Cezary Rojewski wrote:
Implement IRQ handlers for immediate and delayed replies and notifications. Communication is synchronous and allows for serialization of maximum one message at a time.
DSP may respond with ADSP_PENDING status for a request - known as delayed reply - and when situation occurs, framework keeps the lock and awaits upcoming response through IPCD channel which is handled in bottom-half. Immediate replies spawn no BH at all as their processing is very short.
...
+static void catpt_dsp_send_tx(struct catpt_dev *cdev,
const struct catpt_ipc_msg *tx)+{
- u32 header = tx->header | CATPT_IPCC_BUSY;
- if (tx->size)
This check is not needed.
It's really the battle between: avoiding memcpy(size=0) and size-check. In the past, reviewers favored the former.
Ack.
memcpy_toio(catpt_outbox_addr(cdev), tx->data, tx->size);- catpt_writel_shim(cdev, IPCC, header);
+}
...
- ret = ipc->rx.rsp.status;
- if (reply) {
reply->header = ipc->rx.header;So, even in case of error the header is still updated?
As per protocol, it is paramount to save the reply header regardless of IPC status as long as communication with DSP is not severed AKA timeout. Header contains actual error code provided by DSP firmware. While existing catpt's implementation isn't granular in regard to error codes, nothing stops the developer to do so and react differently depending on the value of error code received.
Without storing the header, caller is blind about the severity of an error.
if (!ret && reply->data && reply->size)
memcpy(reply->data, ipc->rx.data, ipc->rx.size);This I didn't get. You copy data by using source size?!
- }
- return ret;
I guess the above piece may be refactored, but I don't know how until it is clear why it's written like this.
...
Well, ipc->rx.size equals reply->size as long as IPC procedure is in progress and reply != NULL. So either ipc->rx.size or reply->size suffices.
IPC protocol is quite simple here (albeit cAVS arguably is even simpler):
OUTBOX <channel: request - reply model> TX: copy request header to appropriate register, copy payload to outbox <IRQ> RX: copy reply header, copy returned payload only in successful case
INBOX <channel: notification model> No TX: no direct requests coming from HOST RX: notifications from DSP, same as RX for OUTBOX applies here
Reply header is always yielded for possibly granular error handling. Payload is omitted if SUCCESS is not the value of msg status.
Before the message is sent (catpt_dsp_do_send_msg()), it is validated against maximum payload size and ipc status fields are re-initialized - catpt_ipc_msg_init(). From them on, msg completion is awaited.
If we are dealing with standard IPC, reply should arrive no later than within 300ms (in vast majority of cases, this won't reach 30ms) and be found within outbox's RX. In case of heavy IPCs e.g.: reset offload-stream, reply (ack) is yielded immediately from DSP, no processing is done between and instead, DSP delegates the heavy task for later. Response for that heavy task completion should arrive no latter than 300ms from the moment initial reply (ack) was received and this time, inbox's RX is used instead.
So, since catpt_dsp_copy_rx() knows nothing about caller's reply (pointer to store the received data into), rx.size is assigned to reply->size during the initialization phase so it can be reused during that very moment - copying rx as there is no need to access SRAM if no payload is expected.
+static void catpt_dsp_copy_rx(struct catpt_dev *cdev, u32 header) +{
- struct catpt_ipc *ipc = &cdev->ipc;
- ipc->rx.header = header;
- if (ipc->rx.size && ipc->rx.rsp.status == CATPT_REPLY_SUCCESS) {
Check for size is redundant.
memcpy_fromio(ipc->rx.data, catpt_outbox_addr(cdev),ipc->rx.size);- }
Can be done like
if (status != SUCCESS) return; memcpy_fromio(...);
+}
Ack for both. The reduced indentation is very good suggestion, thanks!
Czarek