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

[PATCH v2] 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.

It also changes the order in which colors are declared from MSB first to
the more standard LSB first.

Signed-off-by: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
Changes from v1:
- amended documentation following Stephen's suggestion
- allow parsing of bitfields larger than 9 bits
- made it clear that the parsing order of bits is changed with respect
  to the original patch

Andrew, since this patch introduces a (small) change in the DT bindings,
could we try to merge it during the -rc cycle so we don't have to come
with a more complex solution in the future?

 .../bindings/video/simple-framebuffer.txt          | 12 +++-
 drivers/video/simplefb.c                           | 72 +++++++++++++++++++++-
 2 files changed, 80 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
index 3ea4605..18d03e2 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/num-bits pairs, where each pair describes how many bits are used
+	by a given color channel. Value channels are "r" (red), "g" (green),
+	"b" (blue), "a" (alpha) and "x" (unused). Channels are listed in bit
+	order, starting from the LSB. 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 e2e9e3e..1430752 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,72 @@ struct simplefb_format {
 	struct fb_bitfield transp;
 };
 
+static struct simplefb_format *simplefb_parse_format(struct device *dev,
+						     const char *str)
+{
+	struct simplefb_format *format;
+	unsigned int offset = 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 = NULL;
+		int length = 0;
+
+		switch (str[i++]) {
+		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;
+		case 'x':
+		case 'X':
+			break;
+		default:
+			goto error;
+		}
+
+		if (!isdigit(str[i]))
+			goto error;
+
+		while (isdigit(str[i])) {
+			length = length * 10 + (str[i++] - '0');
+		}
+
+		if (field) {
+			field->offset = offset;
+			field->length = length;
+		}
+
+		offset += length;
+	}
+
+	format->bits_per_pixel = (offset + 7) & ~0x7;
+	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[] = {
-	{ "r5g6b5", 16, {11, 5}, {5, 6}, {0, 5}, {0, 0} },
 };
 
 struct simplefb_params {
@@ -131,7 +196,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 (IS_ERR(params->format)) {
 		dev_err(&pdev->dev, "Invalid format value\n");
 		return -EINVAL;
 	}
-- 
1.8.3

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

[David Herrmann]

Hi

On Mon, May 27, 2013 at 5:53 AM, Alexandre Courbot <acourbot@nvidia.com> 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.
>
> It also changes the order in which colors are declared from MSB first to
> the more standard LSB first.
>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
> Changes from v1:
> - amended documentation following Stephen's suggestion
> - allow parsing of bitfields larger than 9 bits
> - made it clear that the parsing order of bits is changed with respect
>   to the original patch
>
> Andrew, since this patch introduces a (small) change in the DT bindings,
> could we try to merge it during the -rc cycle so we don't have to come
> with a more complex solution in the future?
>
>  .../bindings/video/simple-framebuffer.txt          | 12 +++-
>  drivers/video/simplefb.c                           | 72 +++++++++++++++++++++-
>  2 files changed, 80 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> index 3ea4605..18d03e2 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/num-bits pairs, where each pair describes how many bits are used
> +       by a given color channel. Value channels are "r" (red), "g" (green),
> +       "b" (blue), "a" (alpha) and "x" (unused). Channels are listed in bit
> +       order, starting from the LSB. 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

Is there a specific reason why we need arbitrary RGB formats? I have a
KMS/DRM driver based on dvbe that can provide the exact same
functionality as this fbdev driver (including an fbdev
backwards-compat layer). However, DRM does not allow arbitrary formats
but rather provides a huge list of supported formats.

If we apply this patch it will get very hard to support this with a
KMS driver. So any reason why we cannot use the DRM FOURCC constants
instead?

The dvbe proposal is available here: (also see the two following patches)
  http://lkml.indiana.edu/hypermail/linux/kernel/1302.2/00274.html

it provides a simple DRM driver that uses VESA / VBE. I am currently
trying to rework it to "SimpleDRM" which can support arbitrary
firmware framebuffers.

Cheers
David

>  Example:
>
> diff --git a/drivers/video/simplefb.c b/drivers/video/simplefb.c
> index e2e9e3e..1430752 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,72 @@ struct simplefb_format {
>         struct fb_bitfield transp;
>  };
>
> +static struct simplefb_format *simplefb_parse_format(struct device *dev,
> +                                                    const char *str)
> +{
> +       struct simplefb_format *format;
> +       unsigned int offset = 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 = NULL;
> +               int length = 0;
> +
> +               switch (str[i++]) {
> +               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;
> +               case 'x':
> +               case 'X':
> +                       break;
> +               default:
> +                       goto error;
> +               }
> +
> +               if (!isdigit(str[i]))
> +                       goto error;
> +
> +               while (isdigit(str[i])) {
> +                       length = length * 10 + (str[i++] - '0');
> +               }
> +
> +               if (field) {
> +                       field->offset = offset;
> +                       field->length = length;
> +               }
> +
> +               offset += length;
> +       }
> +
> +       format->bits_per_pixel = (offset + 7) & ~0x7;
> +       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[] = {
> -       { "r5g6b5", 16, {11, 5}, {5, 6}, {0, 5}, {0, 0} },
>  };
>
>  struct simplefb_params {
> @@ -131,7 +196,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 (IS_ERR(params->format)) {
>                 dev_err(&pdev->dev, "Invalid format value\n");
>                 return -EINVAL;
>         }
> --
> 1.8.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Alexandre Courbot]

Hi David,

On Tue, May 28, 2013 at 3:22 AM, David Herrmann <dh.herrmann-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> If we apply this patch it will get very hard to support this with a
> KMS driver. So any reason why we cannot use the DRM FOURCC constants
> instead?

I don't see any. Maybe Stephen can confirm.

> The dvbe proposal is available here: (also see the two following patches)
>   http://lkml.indiana.edu/hypermail/linux/kernel/1302.2/00274.html
>
> it provides a simple DRM driver that uses VESA / VBE. I am currently
> trying to rework it to "SimpleDRM" which can support arbitrary
> firmware framebuffers.

Ok. I am personally fine with using FOURCC codes for describing video
modes. Maybe we can just drop this patch if it gets in your way.

Alex.

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

[Stephen Warren]

On 05/26/2013 09:53 PM, 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.
> 
> It also changes the order in which colors are declared from MSB first to
> the more standard LSB first.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
> Changes from v1:
> - amended documentation following Stephen's suggestion
> - allow parsing of bitfields larger than 9 bits
> - made it clear that the parsing order of bits is changed with respect
>   to the original patch
> 
> Andrew, since this patch introduces a (small) change in the DT bindings,
> could we try to merge it during the -rc cycle so we don't have to come
> with a more complex solution in the future?

So, I think we shouldn't change the DT binding at all now. The original
driver is now merged for 3.10, and I think it's far too late to take
code that implements new features for 3.10. This means that we couldn't
merge this patch until 3.11. However, by then, we shouldn't be changing
the DT binding in incompatible ways. Olof has already published some
U-Boot binaries that use the current binding, and on IRC indicated he'd
prefer not to change the binding because of that.

As such, we should either:

a) Just add new entries into the format array that already exists in the
driver. Given David's response, this might be simplest.

b) Extend the DT binding in a 100% backwards-compatible way, which would
mean defaulting the bit-order to match the existing 1 format, and adding
a new Boolean property to indicate that the format string is specified
in the other order.

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

[Alexandre Courbot]

On Tue, May 28, 2013 at 1:13 PM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> On 05/26/2013 09:53 PM, 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.
>>
>> It also changes the order in which colors are declared from MSB first to
>> the more standard LSB first.
>>
>> Signed-off-by: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>> Changes from v1:
>> - amended documentation following Stephen's suggestion
>> - allow parsing of bitfields larger than 9 bits
>> - made it clear that the parsing order of bits is changed with respect
>>   to the original patch
>>
>> Andrew, since this patch introduces a (small) change in the DT bindings,
>> could we try to merge it during the -rc cycle so we don't have to come
>> with a more complex solution in the future?
>
> So, I think we shouldn't change the DT binding at all now. The original
> driver is now merged for 3.10, and I think it's far too late to take
> code that implements new features for 3.10. This means that we couldn't
> merge this patch until 3.11. However, by then, we shouldn't be changing
> the DT binding in incompatible ways. Olof has already published some
> U-Boot binaries that use the current binding, and on IRC indicated he'd
> prefer not to change the binding because of that.
>
> As such, we should either:
>
> a) Just add new entries into the format array that already exists in the
> driver. Given David's response, this might be simplest.
>
> b) Extend the DT binding in a 100% backwards-compatible way, which would
> mean defaulting the bit-order to match the existing 1 format, and adding
> a new Boolean property to indicate that the format string is specified
> in the other order.

a) is definitely simpler, so let's do that and drop this patch. Sorry
about the noise.

Alex.

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

[Olof Johansson]

On Mon, May 27, 2013 at 10:13:09PM -0600, Stephen Warren wrote:
> On 05/26/2013 09:53 PM, 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.
> > 
> > It also changes the order in which colors are declared from MSB first to
> > the more standard LSB first.
> > 
> > Signed-off-by: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > ---
> > Changes from v1:
> > - amended documentation following Stephen's suggestion
> > - allow parsing of bitfields larger than 9 bits
> > - made it clear that the parsing order of bits is changed with respect
> >   to the original patch
> > 
> > Andrew, since this patch introduces a (small) change in the DT bindings,
> > could we try to merge it during the -rc cycle so we don't have to come
> > with a more complex solution in the future?
> 
> So, I think we shouldn't change the DT binding at all now. The original
> driver is now merged for 3.10, and I think it's far too late to take
> code that implements new features for 3.10. This means that we couldn't
> merge this patch until 3.11. However, by then, we shouldn't be changing
> the DT binding in incompatible ways. Olof has already published some
> U-Boot binaries that use the current binding, and on IRC indicated he'd
> prefer not to change the binding because of that.
> 
> As such, we should either:
> 
> a) Just add new entries into the format array that already exists in the
> driver. Given David's response, this might be simplest.

I think so too. Is this even needed? Do we have any users of the newer formats
or is it just someone trying to feature-creep this driver to make the "simple"
part of the name inaccurate? :)

> b) Extend the DT binding in a 100% backwards-compatible way, which would
> mean defaulting the bit-order to match the existing 1 format, and adding
> a new Boolean property to indicate that the format string is specified
> in the other order.

Or just add a new property that has higher priority for the newer format
strings, and make the driver use that if it exists, otherwise fall back.


-Olof

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

[Stephen Warren]

On 05/31/2013 11:12 PM, Olof Johansson wrote:
> On Mon, May 27, 2013 at 10:13:09PM -0600, Stephen Warren wrote:
>> On 05/26/2013 09:53 PM, 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.
...
>> As such, we should either:
>>
>> a) Just add new entries into the format array that already exists in the
>> driver. Given David's response, this might be simplest.
> 
> I think so too. Is this even needed? Do we have any users of the newer formats
> or is it just someone trying to feature-creep this driver to make the "simple"
> part of the name inaccurate? :)

Alex is working on board support for a new NVIDIA board, where IIRC the
bootloader sets up some 32-bit ARGB format for the panel.

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

[Olof Johansson]

On Sat, Jun 1, 2013 at 10:07 PM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> On 05/31/2013 11:12 PM, Olof Johansson wrote:
>> On Mon, May 27, 2013 at 10:13:09PM -0600, Stephen Warren wrote:
>>> On 05/26/2013 09:53 PM, 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.
> ...
>>> As such, we should either:
>>>
>>> a) Just add new entries into the format array that already exists in the
>>> driver. Given David's response, this might be simplest.
>>
>> I think so too. Is this even needed? Do we have any users of the newer formats
>> or is it just someone trying to feature-creep this driver to make the "simple"
>> part of the name inaccurate? :)
>
> Alex is working on board support for a new NVIDIA board, where IIRC the
> bootloader sets up some 32-bit ARGB format for the panel.

Ah, ok. Let's just implement that one format too then. :)


-Olof