Re: [PATCH v8 2/3] drm: bridge: Add support for Cadence MHDP DPI/DP bridge

[Tomi Valkeinen]

On 11/08/2020 05:36, Laurent Pinchart wrote:

>> +static int cdns_mhdp_mailbox_write(struct cdns_mhdp_device *mhdp, u8 val)
>> +{
>> +	int ret, full;
>> +
>> +	WARN_ON(!mutex_is_locked(&mhdp->mbox_mutex));
>> +
>> +	ret = readx_poll_timeout(readl, mhdp->regs + CDNS_MAILBOX_FULL,
>> +				 full, !full, MAILBOX_RETRY_US,
>> +				 MAILBOX_TIMEOUT_US);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	writel(val, mhdp->regs + CDNS_MAILBOX_TX_DATA);
>> +
>> +	return 0;
>> +}
> 
> As commented previously, I think there's room for optimization here. Two
> options that I think should be investigated are using the mailbox
> interrupts, and only polling for the first byte of the message
> (depending on whether the firmware implementation can guarantee that
> when the first byte is available, the rest of the message will be
> immediately available too). This can be done on top of this patch
> though.

I made some tests on this.

I cannot see mailbox_write ever looping, mailbox is never full. So in this case the
readx_poll_timeout() call is there just for safety to catch the cases where something has gone
totally wrong or perhaps once in a while the mbox can be full for a tiny moment. But we always do
need to check CDNS_MAILBOX_FULL before each write to CDNS_MAILBOX_TX_DATA, so we can as well use
readx_poll_timeout for that to catch the odd cases (afaics, there's no real overhead if the exit
condition is true immediately).

mailbox_read polls sometimes. Most often it does not poll, as the data is ready in the mbox, and in
these cases the situation is the same as for mailbox_write.

The cases where it does poll are related to things where the fw has to wait for something. The
longest poll waits seemed to be EDID read (16 ms wait) and adjusting LT (1.7 ms wait). And afaics,
when the first byte of the received message is there, the rest of the bytes will be available
without wait.

For mailbox_write and for most mailbox_reads I think using interrupts makes no sense, as the
overhead would be big.

For those few long read operations, interrupts would make sense. I guess a simple way to handle this
would be to add a new function, wait_for_mbox_data() or such, which would use the interrupts to wait
for mbox not empty. This function could be used in selected functions (edid, LT) after
cdns_mhdp_mailbox_send().

Although I think it's not that bad currently, MAILBOX_RETRY_US is 1ms, so it's quite lazy polling,
so perhaps this can be considered TODO optimization.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki