Re: [PATCH v3] drm/dp: Use large transactions for I2C over AUX

[Simon Farnsworth <simon.farnsworth@onelan.co.uk>]

[-- Attachment #1.1: Type: text/plain, Size: 3063 bytes --]

On Thursday 29 January 2015 16:36:55 Ville Syrjälä wrote:
> On Thu, Jan 29, 2015 at 02:24:09PM +0000, Simon Farnsworth wrote:
> > On Thursday 29 January 2015 15:30:36 you wrote:
> > > On Tue, Jan 27, 2015 at 03:43:49PM +0000, Simon Farnsworth wrote:
--snip--
> > > > +	DRM_DEBUG_KMS("Partial I2C reply: requested %zu bytes got %d bytes\n", msg->size, ret);
> > > > +	drain_msg = *msg;
> > > > +	drain_msg.size -= ret;
> > > > +	drain_msg.buffer += ret;
> > > > +	while (drain_msg.size != 0) {
> > > > +		drain_bytes = drm_dp_i2c_do_msg(aux, &drain_msg);
> > > > +		if (drain_bytes <= 0)
> > > > +			return drain_bytes == 0 ? -EPROTO : drain_bytes;
> > > > +		drain_msg.size -= drain_bytes;
> > > > +		drain_msg.buffer += drain_bytes;
> > > > +	}
> > > 
> > > Somehow I don't like the duplicated code end up having here. So
> > > putting it all in a single loop would seem nicer to me. Maybe
> > > something along these lines?
> > > 
> > > struct drm_dp_aux_msg msg = *orig_msg;
> > > int ret = msg.size;
> > > while (msg.size > 0) {
> > > 	int err = drm_dp_i2c_do_msg(aux, &msg);
> > > 	if (err <= 0)
> > > 		return err == 0 ? -EPROTO : err;
> > > 
> > > 	if (err < msg.size && err < ret) {
> > > 		DRM_DEBUG_KMS("Partial I2C reply: requested %zu
> > > 			       bytes got %d bytes\n", msg.size, err);
> > > 		ret = err;
> > > 	}
> > > 
> > > 	msg.size -= err;
> > > 	msg.buffer += err;
> > > }
> > > 
> > > It would also reduce the returned preferred transfer size further if we
> > > (for whatever reason) got an even shorter reply while we're draining.
> > >
> > I'm not sure that that's the right behaviour, though. If we assume a 3 byte
> > FIFO in a device that does partial reads, we ask for 16 bytes, causing a
> > partial response that's 3 bytes long. We then drain out the remaining 13
> > bytes of the initial request (in case it's set up a 16 byte I2C transfer),
> > and the last of the reads is guaranteed to be 1 byte long.
> 
> My proposed code wouldn't reduce the transfer size in that case due to
> the err<msg.size check. So it only considers shrinking the transfer size
> when the reply came back with less data than was requested.
>
I see, yes. So the only time it'd reduce further is if the hypothetical
device gave a 3 byte response to the 16 byte transfer, then 2 byte responses
to the remaining requests that try to drain the FIFO.

Given that this is now in the realms of "if hardware designers took pleasure
in making their hardware horrible", rather than just "HW designer takes a
short cut and lets software handle the pain", I'll take your code for v4.
> > 
> > We then shrink to 1 byte transfers, when the device would be capable of 3
> > byte transfers, and could possibly perform better with 3 byte transfers
> > rather than 1.
> > 
> > Having said that, this is all hypothetical until we find devices that do
> > partial replies - no-one's been able to find such a device so far.
> > 

-- 
Simon Farnsworth
Software Engineer
ONELAN Ltd
http://www.onelan.com

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel