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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 24 Aug 2020 05:18:30 +0300
From:   Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:     Tomi Valkeinen <tomi.valkeinen@...com>
Cc:     Swapnil Jakhade <sjakhade@...ence.com>, airlied@...ux.ie,
        daniel@...ll.ch, robh+dt@...nel.org, a.hajda@...sung.com,
        narmstrong@...libre.com, jonas@...boo.se, jernej.skrabec@...l.net,
        dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, mparab@...ence.com,
        yamonkar@...ence.com, jsarha@...com, nsekhar@...com,
        praneeth@...com
Subject: Re: [PATCH v8 2/3] drm: bridge: Add support for Cadence MHDP DPI/DP
 bridge

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