Re: [PATCH] usb: gadget: storage_common: make FSG_NUM_BUFFERSvariable size

[Per Forlin]

Hi Michal,

2011/8/8 Michal Nazarewicz <mina86@xxxxxxxxxx>:
> On Mon, 08 Aug 2011 02:22:14 +0200, Per Forlin <per.forlin@xxxxxxxxxx>
> wrote:
>>
>> FSG_NUM_BUFFERS is set to 2 as default.
>> Usually 2 buffers are enough to establish a good
>> double buffering pipeline. But when dealing with expensive
>> request preparation (i.e. dma_map) there may be benefits of
>> increasing the number of buffers. There is an extra cost for
>> every first request, the others are prepared in parallell with
>> an ongoing transfer. Every time all buffers are consumed there is
>> an additional cost for the next first request. Increasing the number
>> of buffers decreases the risk of running out of buffers.
>
> [... benchmark snip ...]
>
>> There may not be one optimal number for all boards. That is the
>> reason for adding the number to Kconfig,
>
> It could actually be turned into a run-time configuration, but
> that's a bit more intrusive,
I consider this too but couldn't see how this would be done nicely.

>> Signed-off-by: Per Forlin <per.forlin@xxxxxxxxxx>
>
> Acked-by: Michal Nazarewicz <mina86@xxxxxxxxxx>
>
>
> Some not very relevant comments below:
Thanks for these pointers. I'll update.

>
>> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
>> index 029e288..bbd17f3 100644
>> --- a/drivers/usb/gadget/Kconfig
>> +++ b/drivers/usb/gadget/Kconfig
>> @@ -96,6 +96,21 @@ config USB_GADGET_VBUS_DRAW
>>           This value will be used except for system-specific gadget
>>           drivers that have more specific information.
>> +config USB_GADGET_STORAGE_NUM_BUFFERS
>> +       int "Number of storage pipline buffers"
>
> “pipeline”, missing “e”.
Will fix.

>
>> +       range 2 64
>> +       default 2
>> +       help
>> +         Usually 2 buffers are enough to establish a good
>> +         double buffering pipeline. But when dealing with expensive
>
> s/double//.  By definition, “double” buffering uses two buffers, isn't it?
> So if we change number of buffers, it's no longer “double”.
>
Good point!

>> +         request preparation (i.e. dma_map) there may be benefits of
>> +         increasing the number of buffers. There is an extra cost for
>> +         every first request, the others are prepared in parallell with
>
> “parallel”, too many “l”s.
>
Will fix

>> +         an ongoing transfer. Every time all buffers are consumed there
>> is
>> +         an additional cost for the next first request. Increasing the
>> number
>
> “Next first request” sounds a bit illogical to me.
>
I'll remove "next" to avoid confusion.

>> +         of buffers decreases the risk of running out of buffers.
>> +         If unsure, say 2.
>> +
>>  config USB_GADGET_SELECTED
>>        boolean
>> diff --git a/drivers/usb/gadget/storage_common.c
>> b/drivers/usb/gadget/storage_common.c
>> index 1fa4f70..512d9cf 100644
>> --- a/drivers/usb/gadget/storage_common.c
>> +++ b/drivers/usb/gadget/storage_common.c
>> @@ -262,8 +262,8 @@ static struct fsg_lun *fsg_lun_from_dev(struct device
>> *dev)
>>  #define EP0_BUFSIZE    256
>>  #define DELAYED_STATUS (EP0_BUFSIZE + 999)     /* An impossibly large
>> value */
>> -/* Number of buffers we will use.  2 is enough for double-buffering */
>> -#define FSG_NUM_BUFFERS        2
>> +/* Number of buffers we will use.  2 is usually enough for
>> double-buffering */
>
> s/double-buffering/good buffering pipeline/?
>
Will fix.

>> +#define FSG_NUM_BUFFERS        CONFIG_USB_GADGET_STORAGE_NUM_BUFFERS
>> /* Default size of buffer length. */
>>  #define FSG_BUFLEN     ((u32)16384)
>
> --
> Best regards,                                         _     _
> .o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
> ..o | Computer Science,  Michal "mina86" Nazarewicz    (o o)
> ooo +-----<email/xmpp: mnazarewicz@xxxxxxxxxx>-----ooO--(_)--Ooo--
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/