Re: Depreciated spi_master.transfer and "prepared spi messages" for an optimized pipelined-SPI-DMA-driver

[Martin Sperl <martin-d5rIkyn9cnPYtjvyW6yDsg@public.gmane.org>]

Hi Mark!

I am currently setting up a second RPI for my work on upstream kernels.
This still will take some more time, as I can only access it via a serial
console, as there is no upstream USB driver (which I need for the on-board
network card to work)... 

>> /* generic wrapper functions for device drivers to use */
>> static int bcm2835dma_spi_prepare_message(struct spi_device *spi,
>> 	struct spi_message *msg)
>> {
>> 	if(spi->master->prepare_message) {
>> 		return spi->master->prepare_message(spi,msg);
>> 	} else {
>> 		return 0;
>> 	}
>> }
> 
> Like I said earlier I'm concerned about the idea of drivers calling
> these functions directly, this should all be factored out of drivers
> since there's nothing really driver specific here.


Actually it is VERY driver specific!

The thing is that you can not easily detect if the same spi_message has 
been used and in some way changed without having to walk and compare the
structure to an existing checksum - and such computation would take say 
1/3rd of the time it takes to prepare the message again.

Then if it has been found to be different, you would first need to erase 
the old structure before creating the new one. 

So this kind of heuristics increases the CPU load resulting in 
unnecessary computations at minimal benefits.

So there is the need to "identify" a message somehow as "prepare-able"
(and thus conforming to some restrictions - like the one I had mentioned)
in the driver itself.

If you want to do this via a separate boolean in the spi_message structure
or if you do this by explicitly calling from the driver IMO does not make
much of a difference - the code needs to get optimized for this anyway.

The advantage of the explicit call is that those prepared messages can get 
released immediately when the driver is unloaded.  Otherwise they can only 
get released when the master is unloaded (or some LRU logic).

And if you do not release on driver unload, then in the worsted case you 
could load the driver again and the spi_message would get created in the
same location but the xfers and their pointers might point elsewhere.
And that would _definitely_ be a problem. (You could add cookies,...
but then you would have more computations still.)

For spi_async all the above checks could happen in an IRQ context,
so the code-path should _really_ be optimized!

Anyway: this mostly applies to drivers that need to be fast
and have high thru-put. And those would be the only ones that need
"special" handling. 

For those, that just use the "simple" sync API (variations of 
spi_write_then_read and similar), the framework may already have done the
prepare for its "hot-path" situation.
That way those "simple" drivers can get a benefit as well.

I do not think it is worth coding "complex" heuristics, like
checking X times for a message to be "identical" and only then taking 
the short-cut. That can lead to some very strange situations to debug.
So as far as I am concerned it is better to explicitly say in the driver:
yes we can.

It might be worth thinking of extending the interface for "typical" transfers
cases so that more use-cases get handled automatically (with prepared code).
But that would require reviewing/rewriting existing drivers to make use of 
those interfaces.

Finally: with the preare/unprepare calls in the master structure, the 
thing you risk is that each driver writes is own:
if (master->prepared_message)
	master->prepared_message(spi,message);
so better make that wrapper available...

Ciao,
	Martin

P.s: just looking at __spi_async already gives me some thought:
we are already iterating over the stucture once for the sanity-checks.
And that takes time already - cycles that could be saved for the "prepared"
message case entirely by short-cutting those "sanity-checks" and just call 
master->transfer(spi->message) immediately.

So integrating the "search" for prepared statements here and skipping
the tests in this case would also improve the thru-put...

Otherwise you would risk that a high-thruput driver directly calls
message->spi=spi;master->transfer(spi,message);
to avoid those unnecessary CPU cycles - especially if it is optimized.

Ok, you could reject the patch, but then you would ignore the root cause
WHY people do this!

One way around this could be that you would call a default "prepared"
statement code that would do the checks in the structure.

And that way any driver that does prepare its messages would get the
benefit independent of if the bus driver itself implements prepared 
messages or not.

It could boil down to something like this:
(this would require a flag marking the message as prepared)

static int __spi_sanity_check_message(struct spi_device *spi,
	struct spi_message *message)
{
	/* message sanity check code here */
}

/* set prepared_message to this in spi_register_master,
 * if prepare_message is NULL (not included)
 */
int __spi_prepare_message_default(struct spi_device *spi,
	struct spi_message *message)
{
	int ret=__spi_sanity_check_message(spi,message);
	if (!ret)
		message->is_prepared=1;
	return ret;
}

static int __spi_async(struct spi_device *spi, struct spi_message *message)
{
  int ret=0;
  /* the non prepared path */
  if (! message->is_prepared) {
    /* sanity checks here */
    ret=__spi_sanity_check_message(spi,message);
    if (ret)
	return ret;
  }
  /* and the "normal" call of transfer */
  message->spi = spi;
  message_status = -EINPROGRESS;
  return master->transfer(spi,message);
}

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html