[PATCH] video: simplefb: add mode parsing function

[PATCH] video: simplefb: add mode parsing function

[Alexandre Courbot]

The naming scheme of simplefb's mode is precise enough to allow building
the mode structure from it instead of using a static list of modes. This
patch introduces a function that does this. In case exotic modes that
cannot be represented from their name alone are needed, the static list
of modes is still available as a backup.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
Stephen, please note that the "r5g6b5" mode initially supported
by the driver becomes "b5g6r5" with the new function. This is because
the least significant bits are defined first in the string - this
makes parsing much easier, notably for modes which do not fill whole
bytes (e.g. 15 bit modes). I hope this is not a problem - it's probably
better to do the change now while the driver is still new.

 .../bindings/video/simple-framebuffer.txt          | 12 +++-
 drivers/video/simplefb.c                           | 66 +++++++++++++++++++++-
 2 files changed, 73 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
index 3ea4605..f933b3d 100644
--- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
+++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
@@ -10,8 +10,16 @@ Required properties:
 - width: The width of the framebuffer in pixels.
 - height: The height of the framebuffer in pixels.
 - stride: The number of bytes in each line of the framebuffer.
-- format: The format of the framebuffer surface. Valid values are:
-  - r5g6b5 (16-bit pixels, d[15:11]=r, d[10:5]=g, d[4:0]=b).
+- format: The format of the framebuffer surface. Described as a sequence of
+	channel/nbbits pairs, where each pair describes how many bits are used
+	by a given color channel. Value channels are "r" (red), "g" (green),
+	"b" (blue) and "a" (alpha). Channels are listed starting in
+	bit-significance order. For instance, a format named "r5g6b5" describes
+	a 16-bit format where red is encoded in the 5 less significant bits,
+	green in the 6 following ones, and blue in the 5 last:
+				BBBBBGGG GGGRRRRR
+				^               ^
+			       MSB             LSB
 
 Example:
 
diff --git a/drivers/video/simplefb.c b/drivers/video/simplefb.c
index 8105c3d..1a1be71 100644
--- a/drivers/video/simplefb.c
+++ b/drivers/video/simplefb.c
@@ -21,6 +21,7 @@
  */
 
 #include <linux/errno.h>
+#include <linux/ctype.h>
 #include <linux/fb.h>
 #include <linux/io.h>
 #include <linux/module.h>
@@ -82,8 +83,64 @@ struct simplefb_format {
 	struct fb_bitfield transp;
 };
 
-struct simplefb_format simplefb_formats[] = {
-	{ "r5g6b5", 16, {11, 5}, {5, 6}, {0, 5}, {0, 0} },
+static struct simplefb_format *simplefb_parse_format(struct device *dev,
+						     const char *str)
+{
+	struct simplefb_format *format;
+	unsigned int cpt = 0;
+	unsigned int i = 0;
+
+	format = devm_kzalloc(dev, sizeof(*format), GFP_KERNEL);
+	if (!format)
+		return ERR_PTR(-ENOMEM);
+
+	while (str[i] != 0) {
+		struct fb_bitfield *field = 0;
+		char c = str[i++];
+
+		switch (c) {
+		case 'r':
+		case 'R':
+			field = &format->red;
+			break;
+		case 'g':
+		case 'G':
+			field = &format->green;
+			break;
+		case 'b':
+		case 'B':
+			field = &format->blue;
+			break;
+		case 'a':
+		case 'A':
+			field = &format->transp;
+			break;
+		default:
+			goto error;
+		}
+
+		c = str[i++];
+		if (c = 0 || !isdigit(c))
+			goto error;
+
+		field->offset = cpt;
+		field->length = c - '0';
+
+		cpt += field->length;
+	}
+
+	format->bits_per_pixel = ((cpt + 7) / 8) * 8;
+	format->name = str;
+	return format;
+
+error:
+	dev_err(dev, "Invalid format string\n");
+	return ERR_PTR(-EINVAL);
+}
+
+/* if you use exotic modes that simplefb_parse_format cannot decode, you can
+   specify them here. */
+static struct simplefb_format simplefb_formats[] = {
 };
 
 struct simplefb_params {
@@ -131,7 +188,10 @@ static int simplefb_parse_dt(struct platform_device *pdev,
 		params->format = &simplefb_formats[i];
 		break;
 	}
-	if (!params->format) {
+	if (!params->format)
+		params->format = simplefb_parse_format(&pdev->dev, format);
+
+	if (!params->format || IS_ERR(params->format)) {
 		dev_err(&pdev->dev, "Invalid format value\n");
 		return -EINVAL;
 	}
-- 
1.8.2.3

Re: [PATCH] video: simplefb: add mode parsing function

[Stephen Warren]

On 05/23/2013 02:03 AM, Alexandre Courbot wrote:
> The naming scheme of simplefb's mode is precise enough to allow building
> the mode structure from it instead of using a static list of modes. This
> patch introduces a function that does this. In case exotic modes that
> cannot be represented from their name alone are needed, the static list
> of modes is still available as a backup.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
> Stephen, please note that the "r5g6b5" mode initially supported
> by the driver becomes "b5g6r5" with the new function. This is because
> the least significant bits are defined first in the string - this
> makes parsing much easier, notably for modes which do not fill whole
> bytes (e.g. 15 bit modes). I hope this is not a problem - it's probably
> better to do the change now while the driver is still new.

Hmm. searchin Google (and even looking at the VDPAU spec I myself wrote)
does imply that format names like this do typically list the LSBs first.

I guess the problem is that when I decided to change from rgb565 to a
machine-parsable r5g6b5, I didn't think enough to realize that the field
order in the name "rgb565" didn't follow the same convention as when you
write out the fields in the style "r5g6b5"/"b5g6r5":-(

I guess this driver and DT binding are only in linux-next and targeted
at 3.11, and didn't make 3.10, so I'd assert it's probably still OK to
change the DT binding, if this patch also gets into 3.11.

The only two users of it I know are:

a) From U-Boot on the Raspberry Pi, and those patches haven't been
accepted into U-Boot yet.

b) From U-Boot on the Samsung ARM Chromebook yet. I know Olof built a
U-Boot that uses them and it'd linked from the chromiumos.org web pages.
I assume it's not too horrible for him to update them. I don't know how
widely the patch that enables simple-fb in that binary is distributed
though. Olof, care to comment on how much pain it'd be to change?

> diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt

> -- format: The format of the framebuffer surface. Valid values are:
> -  - r5g6b5 (16-bit pixels, d[15:11]=r, d[10:5]=g, d[4:0]=b).
> +- format: The format of the framebuffer surface. Described as a sequence of
> +	channel/nbbits pairs, where each pair describes how many bits are used

Change nbbits to bitcount, num-bits, nr-bits?

> +	by a given color channel. Value channels are "r" (red), "g" (green),
> +	"b" (blue) and "a" (alpha).

I think you'll need "x" too, to represent any gaps/unused bits.

> Channels are listed starting in bit-significance order.

I would explicitly add ", starting from the LSB." to the end there.
Perhaps drop "-significance" too?

> For instance, a format named "r5g6b5" describes
> +	a 16-bit format where red is encoded in the 5 less significant bits,
> +	green in the 6 following ones, and blue in the 5 last:
> +				BBBBBGGG GGGRRRRR
> +				^               ^
> +			       MSB             LSB

> diff --git a/drivers/video/simplefb.c b/drivers/video/simplefb.c

> +static struct simplefb_format *simplefb_parse_format(struct device *dev,
> +						     const char *str)
> +{
> +	struct simplefb_format *format;
> +	unsigned int cpt = 0;

What does cpt mean? Something like bit or bitnum or bitindex might be
more descriptive.

> +	unsigned int i = 0;
> +
> +	format = devm_kzalloc(dev, sizeof(*format), GFP_KERNEL);
> +	if (!format)
> +		return ERR_PTR(-ENOMEM);

Can we just add some storage into the FB object itself for this, so we
don't need to make a special allocation? The first parameter to
framebuffer_alloc() allows allocating that, although you would have to
rejig the order of probe() a bit to do that:-( Perhaps it's fine as you
wrote it.

> +		field->offset = cpt;
> +		field->length = c - '0';

What about R10G10B10A2? Something like strtol() is needed here.

> +
> +		cpt += field->length;
> +	}
> +
> +	format->bits_per_pixel = ((cpt + 7) / 8) * 8;

Should this error-check that isn't > 32?

> +	if (!params->format || IS_ERR(params->format)) {

That's just saying IS_ERR_OR_NULL(params->format). It'd be better to
pick one thing to represent errors; either NULL or an error-pointer. I
think having simplefb_parse_format() just return NULL on error would be
best; the caller can't really do anything useful with the information
anyway.

Re: [PATCH] video: simplefb: add mode parsing function

[Geert Uytterhoeven]

On Thu, May 23, 2013 at 6:27 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> +
>> +             cpt += field->length;
>> +     }
>> +
>> +     format->bits_per_pixel = ((cpt + 7) / 8) * 8;
>
> Should this error-check that isn't > 32?

So pixels can't be larger than 32 bits?
IIRC, some SGI and Sun graphics cards had e.g. 80 bit pixels (incl. Z buffer).

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] video: simplefb: add mode parsing function

[Stephen Warren]

On 05/23/2013 10:57 AM, Geert Uytterhoeven wrote:
> On Thu, May 23, 2013 at 6:27 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>> +
>>> +             cpt += field->length;
>>> +     }
>>> +
>>> +     format->bits_per_pixel = ((cpt + 7) / 8) * 8;
>>
>> Should this error-check that isn't > 32?
> 
> So pixels can't be larger than 32 bits?
> IIRC, some SGI and Sun graphics cards had e.g. 80 bit pixels (incl. Z buffer).

That's a good point.

Out of curiosity, how does the FB core treat these format definitions?
Are they expected to fit into a 16-/32-/64-/128- bit power-of-two
bit-size, or are they treated as a string of bytes that get serialized
into memory LSB first (or perhaps MSB first on BE systems?)

The difference would be that from a CPU perspective only, if you pack
the RGB components into a u32, then write that to RAM as a u32, then the
in-memory byte-by-byte order is different on different endian systems,
whereas if the FB core treats it as a series of bytes only, then
presumably the in-memory byte-by-byte order is identical irrespective of
host CPU endianness.

Re: [PATCH] video: simplefb: add mode parsing function

[Geert Uytterhoeven]

On Thu, May 23, 2013 at 9:21 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 05/23/2013 10:57 AM, Geert Uytterhoeven wrote:
>> On Thu, May 23, 2013 at 6:27 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>> +
>>>> +             cpt += field->length;
>>>> +     }
>>>> +
>>>> +     format->bits_per_pixel = ((cpt + 7) / 8) * 8;
>>>
>>> Should this error-check that isn't > 32?
>>
>> So pixels can't be larger than 32 bits?
>> IIRC, some SGI and Sun graphics cards had e.g. 80 bit pixels (incl. Z buffer).
>
> That's a good point.
>
> Out of curiosity, how does the FB core treat these format definitions?
> Are they expected to fit into a 16-/32-/64-/128- bit power-of-two
> bit-size, or are they treated as a string of bytes that get serialized
> into memory LSB first (or perhaps MSB first on BE systems?)
>
> The difference would be that from a CPU perspective only, if you pack
> the RGB components into a u32, then write that to RAM as a u32, then the
> in-memory byte-by-byte order is different on different endian systems,
> whereas if the FB core treats it as a series of bytes only, then
> presumably the in-memory byte-by-byte order is identical irrespective of
> host CPU endianness.

I don't think any of the current FB drivers support these.

Conceptually, a (packed) frame buffer is just a block of memory,
containing lines
of line_length bytes.
Each line contains xres_virtual pixels, each of bits_per_pixel bits.

As long as bits_per_pixel is an integer multiple of 8, it's
more-or-less well-defined.
We did have issues with endianness on e.g. bits_per_pixel = 4 (2 pixels in
one byte, which order?), bits_per_pixel = 16 (byteswapped RGB565), etc.
If bits_per_pixel is larger than 32, and not an integer multiple of 32,
you indeed can have more of them...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] video: simplefb: add mode parsing function

[Alex Courbot]

On 05/24/2013 01:27 AM, Stephen Warren wrote:
>> Stephen, please note that the "r5g6b5" mode initially supported
>> by the driver becomes "b5g6r5" with the new function. This is because
>> the least significant bits are defined first in the string - this
>> makes parsing much easier, notably for modes which do not fill whole
>> bytes (e.g. 15 bit modes). I hope this is not a problem - it's probably
>> better to do the change now while the driver is still new.
>
> Hmm. searchin Google (and even looking at the VDPAU spec I myself wrote)
> does imply that format names like this do typically list the LSBs first.
>
> I guess the problem is that when I decided to change from rgb565 to a
> machine-parsable r5g6b5, I didn't think enough to realize that the field
> order in the name "rgb565" didn't follow the same convention as when you
> write out the fields in the style "r5g6b5"/"b5g6r5":-(
>
> I guess this driver and DT binding are only in linux-next and targeted
> at 3.11, and didn't make 3.10, so I'd assert it's probably still OK to
> change the DT binding, if this patch also gets into 3.11.

Great, keeping it like that then.

>> -- format: The format of the framebuffer surface. Valid values are:
>> -  - r5g6b5 (16-bit pixels, d[15:11]=r, d[10:5]=g, d[4:0]=b).
>> +- format: The format of the framebuffer surface. Described as a sequence of
>> +	channel/nbbits pairs, where each pair describes how many bits are used
>
> Change nbbits to bitcount, num-bits, nr-bits?

Ok.

>> +	by a given color channel. Value channels are "r" (red), "g" (green),
>> +	"b" (blue) and "a" (alpha).
>
> I think you'll need "x" too, to represent any gaps/unused bits.

Are there such formats? 15-bit formats do exist but AFAIK they just drop 
the MSB. Anyway, this can be done easily, so I won't argue. :)

>> Channels are listed starting in bit-significance order.
>
> I would explicitly add ", starting from the LSB." to the end there.
> Perhaps drop "-significance" too?

Agreed, sounds better.

>> +static struct simplefb_format *simplefb_parse_format(struct device *dev,
>> +						     const char *str)
>> +{
>> +	struct simplefb_format *format;
>> +	unsigned int cpt = 0;
>
> What does cpt mean? Something like bit or bitnum or bitindex might be
> more descriptive.

Fixed.

>> +	unsigned int i = 0;
>> +
>> +	format = devm_kzalloc(dev, sizeof(*format), GFP_KERNEL);
>> +	if (!format)
>> +		return ERR_PTR(-ENOMEM);
>
> Can we just add some storage into the FB object itself for this, so we
> don't need to make a special allocation? The first parameter to
> framebuffer_alloc() allows allocating that, although you would have to
> rejig the order of probe() a bit to do that:-( Perhaps it's fine as you
> wrote it.

The less allocations the better - if using framebuffer_alloc does not 
make things more confusing, I'll gladly use that solution.

>> +		field->offset = cpt;
>> +		field->length = c - '0';
>
> What about R10G10B10A2? Something like strtol() is needed here.

True. That's probably more color depth than humans can perceive, but 
will make the mantis shrimp happy.

>> +
>> +		cpt += field->length;
>> +	}
>> +
>> +	format->bits_per_pixel = ((cpt + 7) / 8) * 8;
>
> Should this error-check that isn't > 32?

That would make the mantis shrimp sad.

>> +	if (!params->format || IS_ERR(params->format)) {
>
> That's just saying IS_ERR_OR_NULL(params->format). It'd be better to
> pick one thing to represent errors; either NULL or an error-pointer. I
> think having simplefb_parse_format() just return NULL on error would be
> best; the caller can't really do anything useful with the information
> anyway.

Weird - I actually removed that NULL check since the parse function can 
only return a valid pointed an error code. Probably forgot to 
format-patch again after that.

It seems more customary to let the function that fails decide the error 
code and have it propagated by its caller (even though in the current 
case there is no much variety in the returned error codes). If you see 
no problem with it, I will stick to that way of doing.

Thanks for the review - will send an update soon.
Alex.


-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Re: [PATCH] video: simplefb: add mode parsing function

[Stephen Warren]

On 05/24/2013 01:30 AM, Alex Courbot wrote:
> On 05/24/2013 01:27 AM, Stephen Warren wrote:
>>> Stephen, please note that the "r5g6b5" mode initially supported
>>> by the driver becomes "b5g6r5" with the new function. This is because
>>> the least significant bits are defined first in the string - this
>>> makes parsing much easier, notably for modes which do not fill whole
>>> bytes (e.g. 15 bit modes). I hope this is not a problem - it's probably
>>> better to do the change now while the driver is still new.
...
>>> +    by a given color channel. Value channels are "r" (red), "g"
>>> (green),
>>> +    "b" (blue) and "a" (alpha).
>>
>> I think you'll need "x" too, to represent any gaps/unused bits.
> 
> Are there such formats? 15-bit formats do exist but AFAIK they just drop
> the MSB. Anyway, this can be done easily, so I won't argue. :)

Yes, I believe there are e.g. both a2r10g10b10 and b10g10r10a2 for
example, and it's quite common to replace a with x, especially for
scanout buffers. Google certainly has hits for that.

>>> +    if (!params->format || IS_ERR(params->format)) {
>>
>> That's just saying IS_ERR_OR_NULL(params->format). It'd be better to
>> pick one thing to represent errors; either NULL or an error-pointer. I
>> think having simplefb_parse_format() just return NULL on error would be
>> best; the caller can't really do anything useful with the information
>> anyway.
> 
> Weird - I actually removed that NULL check since the parse function can
> only return a valid pointed an error code. Probably forgot to
> format-patch again after that.
> 
> It seems more customary to let the function that fails decide the error
> code and have it propagated by its caller (even though in the current
> case there is no much variety in the returned error codes). If you see
> no problem with it, I will stick to that way of doing.

Using just an error-return is probably fine. I was going to say that the
table lookup might propagate a NULL all the way through to that check,
and hence require both checks above, but I guess that case can't happen,
since if there is no table entry, then simplefb_parse_format() will
always be called, so that's fine.