* [PATCH] video: ARM CLCD: Ensure bits-per-pixel is a power of 2 and <= 32 @ 2014-08-19 13:07 Jon Medhurst (Tixy) 2014-08-19 14:40 ` Russell King - ARM Linux 0 siblings, 1 reply; 3+ messages in thread From: Jon Medhurst (Tixy) @ 2014-08-19 13:07 UTC (permalink / raw) To: linux-arm-kernel If the device-tree specifies a max-memory-bandwidth property then the CLCD driver uses that to calculate the bits-per-pixel supported, however, it doesn't ensure that the result is a sane value, i.e. a power of 2 and <= 32 as the rest of the code assumes. Acked-by: Pawel Moll <pawel.moll@arm.com> Signed-off-by: Jon Medhurst <tixy@linaro.org> --- This fixes code which is new in 3.17 (commit d10715be03) and so I assume is a candidate for adding to a coming -rc ? Without the fix, people can be left (as I was) with a blank non-functioning screen even if they create a valid device-tree for the new driver functionality. drivers/video/fbdev/amba-clcd.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/drivers/video/fbdev/amba-clcd.c b/drivers/video/fbdev/amba-clcd.c index beadd3e..98b66b7 100644 --- a/drivers/video/fbdev/amba-clcd.c +++ b/drivers/video/fbdev/amba-clcd.c @@ -24,6 +24,7 @@ #include <linux/list.h> #include <linux/amba/bus.h> #include <linux/amba/clcd.h> +#include <linux/bitops.h> #include <linux/clk.h> #include <linux/hardirq.h> #include <linux/dma-mapping.h> @@ -650,6 +651,7 @@ static int clcdfb_of_init_display(struct clcd_fb *fb) { struct device_node *endpoint; int err; + int bpp; u32 max_bandwidth; u32 tft_r0b0g0[3]; @@ -667,11 +669,15 @@ static int clcdfb_of_init_display(struct clcd_fb *fb) err = of_property_read_u32(fb->dev->dev.of_node, "max-memory-bandwidth", &max_bandwidth); - if (!err) - fb->panel->bpp = 8 * max_bandwidth / (fb->panel->mode.xres * + if (!err) { + bpp = 8 * max_bandwidth / (fb->panel->mode.xres * fb->panel->mode.yres * fb->panel->mode.refresh); - else - fb->panel->bpp = 32; + bpp = rounddown_pow_of_two(bpp); + if (bpp > 32) + bpp = 32; + } else + bpp = 32; + fb->panel->bpp = bpp; #ifdef CONFIG_CPU_BIG_ENDIAN fb->panel->cntl |= CNTL_BEBO; -- 2.0.1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] video: ARM CLCD: Ensure bits-per-pixel is a power of 2 and <= 32 2014-08-19 13:07 [PATCH] video: ARM CLCD: Ensure bits-per-pixel is a power of 2 and <= 32 Jon Medhurst (Tixy) @ 2014-08-19 14:40 ` Russell King - ARM Linux 2014-08-20 8:27 ` Jon Medhurst (Tixy) 0 siblings, 1 reply; 3+ messages in thread From: Russell King - ARM Linux @ 2014-08-19 14:40 UTC (permalink / raw) To: linux-arm-kernel On Tue, Aug 19, 2014 at 02:07:31PM +0100, Jon Medhurst (Tixy) wrote: > If the device-tree specifies a max-memory-bandwidth property then > the CLCD driver uses that to calculate the bits-per-pixel supported, > however, it doesn't ensure that the result is a sane value, i.e. a > power of 2 and <= 32 as the rest of the code assumes. > > Acked-by: Pawel Moll <pawel.moll@arm.com> > Signed-off-by: Jon Medhurst <tixy@linaro.org> > --- > > This fixes code which is new in 3.17 (commit d10715be03) and so I assume > is a candidate for adding to a coming -rc ? Without the fix, people can > be left (as I was) with a blank non-functioning screen even if they > create a valid device-tree for the new driver functionality. > > drivers/video/fbdev/amba-clcd.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/video/fbdev/amba-clcd.c b/drivers/video/fbdev/amba-clcd.c > index beadd3e..98b66b7 100644 > --- a/drivers/video/fbdev/amba-clcd.c > +++ b/drivers/video/fbdev/amba-clcd.c > @@ -24,6 +24,7 @@ > #include <linux/list.h> > #include <linux/amba/bus.h> > #include <linux/amba/clcd.h> > +#include <linux/bitops.h> > #include <linux/clk.h> > #include <linux/hardirq.h> > #include <linux/dma-mapping.h> > @@ -650,6 +651,7 @@ static int clcdfb_of_init_display(struct clcd_fb *fb) > { > struct device_node *endpoint; > int err; > + int bpp; > u32 max_bandwidth; > u32 tft_r0b0g0[3]; > > @@ -667,11 +669,15 @@ static int clcdfb_of_init_display(struct clcd_fb *fb) > > err = of_property_read_u32(fb->dev->dev.of_node, "max-memory-bandwidth", > &max_bandwidth); > - if (!err) > - fb->panel->bpp = 8 * max_bandwidth / (fb->panel->mode.xres * > + if (!err) { > + bpp = 8 * max_bandwidth / (fb->panel->mode.xres * > fb->panel->mode.yres * fb->panel->mode.refresh); This calculation is wrong in any case - this is the naïeve calculation which assumes that the bandwidth is: x * y * (bpp / 8) * refresh That isn't the maximum bandwidth, it's the average bandwidth across a full frame. If we're interested in limiting the maximum bandwidth, because the hardware can't cope with fetching the data above a certain rate, then we need a different method. We know the pixel rate. We know how many memory bits are fetched for each pixel. So: peak_bandwidth = pixel_clock * bpp / 8 Let's take 32bpp, 1024x768@60Hz, which has a pixel clock of 65MHz. Using the first, we get: 1024 * 768 * 4 * 60 = 180MiB/s. Using the second: 65 * 4 = 248MiB/s. To see why there's this discrepency, realise that there's 320 clocks where there's no pixel activity (so no memory fetches) per line, and 38 lines where there are no memory fetches. That's 320 * 768 + 1344 * 38 = 296832 pixel clocks where there's no memory fetches, out of a total of 1344 * 806 = 1083264 total pixel clocks in a frame. So, that's about 27% of the frame where the memory subsystem is inactive - which accounts for the difference in the above figure. So, if you are limiting the bandwidth of the mode due to the available bus bandwidth to the controller (which is implied by the property name), then you want to use the second equation to limit the peak bandwidth, and not the average bandwidth. -- FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up according to speedtest.net. ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] video: ARM CLCD: Ensure bits-per-pixel is a power of 2 and <= 32 2014-08-19 14:40 ` Russell King - ARM Linux @ 2014-08-20 8:27 ` Jon Medhurst (Tixy) 0 siblings, 0 replies; 3+ messages in thread From: Jon Medhurst (Tixy) @ 2014-08-20 8:27 UTC (permalink / raw) To: linux-arm-kernel On Tue, 2014-08-19 at 15:40 +0100, Russell King - ARM Linux wrote: > On Tue, Aug 19, 2014 at 02:07:31PM +0100, Jon Medhurst (Tixy) wrote: > > If the device-tree specifies a max-memory-bandwidth property then > > the CLCD driver uses that to calculate the bits-per-pixel supported, > > however, it doesn't ensure that the result is a sane value, i.e. a > > power of 2 and <= 32 as the rest of the code assumes. > > > > Acked-by: Pawel Moll <pawel.moll@arm.com> > > Signed-off-by: Jon Medhurst <tixy@linaro.org> > > --- > > > > This fixes code which is new in 3.17 (commit d10715be03) and so I assume > > is a candidate for adding to a coming -rc ? Without the fix, people can > > be left (as I was) with a blank non-functioning screen even if they > > create a valid device-tree for the new driver functionality. > > > > drivers/video/fbdev/amba-clcd.c | 14 ++++++++++---- > > 1 file changed, 10 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/video/fbdev/amba-clcd.c b/drivers/video/fbdev/amba-clcd.c > > index beadd3e..98b66b7 100644 > > --- a/drivers/video/fbdev/amba-clcd.c > > +++ b/drivers/video/fbdev/amba-clcd.c > > @@ -24,6 +24,7 @@ > > #include <linux/list.h> > > #include <linux/amba/bus.h> > > #include <linux/amba/clcd.h> > > +#include <linux/bitops.h> > > #include <linux/clk.h> > > #include <linux/hardirq.h> > > #include <linux/dma-mapping.h> > > @@ -650,6 +651,7 @@ static int clcdfb_of_init_display(struct clcd_fb *fb) > > { > > struct device_node *endpoint; > > int err; > > + int bpp; > > u32 max_bandwidth; > > u32 tft_r0b0g0[3]; > > > > @@ -667,11 +669,15 @@ static int clcdfb_of_init_display(struct clcd_fb *fb) > > > > err = of_property_read_u32(fb->dev->dev.of_node, "max-memory-bandwidth", > > &max_bandwidth); > > - if (!err) > > - fb->panel->bpp = 8 * max_bandwidth / (fb->panel->mode.xres * > > + if (!err) { > > + bpp = 8 * max_bandwidth / (fb->panel->mode.xres * > > fb->panel->mode.yres * fb->panel->mode.refresh); > > This calculation is wrong in any case - this is the naïeve calculation > which assumes that the bandwidth is: > > x * y * (bpp / 8) * refresh > > That isn't the maximum bandwidth, it's the average bandwidth across a > full frame. > > If we're interested in limiting the maximum bandwidth, because the > hardware can't cope with fetching the data above a certain rate, then > we need a different method. > > We know the pixel rate. We know how many memory bits are fetched for > each pixel. So: > > peak_bandwidth = pixel_clock * bpp / 8 That all sounds logical and reasonable to me, especially as the binding doc describes the property as "maximum bandwidth in bytes per second that the cell's memory interface can handle". I'll update the patch to also correct the calculation as you suggest. -- Tixy ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-08-20 8:27 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-08-19 13:07 [PATCH] video: ARM CLCD: Ensure bits-per-pixel is a power of 2 and <= 32 Jon Medhurst (Tixy) 2014-08-19 14:40 ` Russell King - ARM Linux 2014-08-20 8:27 ` Jon Medhurst (Tixy)
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).