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

[Laurent Pinchart]

Hi Tomi,

On Fri, Aug 14, 2020 at 12:29:35PM +0300, Tomi Valkeinen wrote:
> 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.

I'm fine with TODO optimization.

-- 
Regards,

Laurent Pinchart