| Message ID | 1369626821-28494-1-git-send-email-acourbot@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
Hi David, On Tue, May 28, 2013 at 3:22 AM, David Herrmann <dh.herrmann@gmail.com> 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.
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.
On Tue, May 28, 2013 at 1:13 PM, Stephen Warren <swarren@wwwdotorg.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@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. a) is definitely simpler, so let's do that and drop this patch. Sorry about the noise. Alex.
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@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. 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
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.
On Sat, Jun 1, 2013 at 10:07 PM, Stephen Warren <swarren@wwwdotorg.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
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; }
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(-)