Commit 600ce1a0faafeed1ce6bcfd421bc040b941cbbc1 ("fix clock setting
for Samsung SoC Framebuffer") introduced a mandatory refresh parameter
to the platform data for the S3C framebuffer but did not introduce any
validation code, causing existing platforms to divide by zero whenever
the framebuffer is configured, generating warnings and unusable output.
Add a WARN_ON(), in line with similar parameters, and check that we
have a non-zero refresh before we dividing by it. This gets usable
output on at least the SMDK6410, though without specifying a refresh
we see visible issues on some boots.
Signed-off-by: Mark Brown <[email protected]>
Cc: [email protected]
---
All mainline users of the s3c-fb driver are affected by this, none
provide refresh.
drivers/video/s3c-fb.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
index adf9632..0f2e8f6 100644
--- a/drivers/video/s3c-fb.c
+++ b/drivers/video/s3c-fb.c
@@ -224,7 +224,8 @@ static int s3c_fb_calc_pixclk(unsigned char id, struct s3c_fb *sfb, unsigned int
unsigned long clk = clk_get_rate(sfb->bus_clk);
unsigned int result;
- pixclk *= win->win_mode.refresh;
+ if (win->win_mode.refresh)
+ pixclk *= win->win_mode.refresh;
result = clk / pixclk;
dev_dbg(sfb->dev, "pixclk=%u, clk=%lu, div=%d (%lu)\n",
@@ -766,6 +767,7 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
WARN_ON(windata->max_bpp == 0);
WARN_ON(windata->win_mode.xres == 0);
WARN_ON(windata->win_mode.yres == 0);
+ WARN_ON(windata->win_mode.refresh == 0);
win = fbinfo->par;
var = &fbinfo->var;
--
1.6.6
On Wed, Jan 13, 2010 at 04:16:42PM +0000, Mark Brown wrote:
> Commit 600ce1a0faafeed1ce6bcfd421bc040b941cbbc1 ("fix clock setting
> for Samsung SoC Framebuffer") introduced a mandatory refresh parameter
> to the platform data for the S3C framebuffer but did not introduce any
> validation code, causing existing platforms to divide by zero whenever
> the framebuffer is configured, generating warnings and unusable output.
Hmm, having just read 600ce1a0faafeed1ce6bcfd421bc040b941cbbc1 it seems
the the original authour has has missed the point of what is happening
and thus possibly broken the driver.
We get passed the pixclk in the fb settings to produce the required
refresh rate for the given parameters. We do not need to think about
the refresh rate in the driver itself, the caller should have deal with
it!.
Secondly, s3c_fb_calc_pixclk() can only be called from the root fb,
as there is only one divider in the block used to generate the output
pixel clock for the LCD bus. Adding a window id to the s3c_fb_calc_pixclk()
is useless as the subordinate framebuffers for the overlay windows do not
have their own dividers. as noted in the code it only ever uses this
in the case of win_no == 0, so it was unecessarey.
Thirdly he's changed the code to calculate the divider to assume pixclk
is a frequency, and not a time-period. It clearly stats in the docs
that pixclk is in picoseconds. Our clock rate is in Hz.
It seems I wasn't cc'd on the original submission or I'd have said no.
I'd rather see this one reverted.
> Add a WARN_ON(), in line with similar parameters, and check that we
> have a non-zero refresh before we dividing by it. This gets usable
> output on at least the SMDK6410, though without specifying a refresh
> we see visible issues on some boots.
>
> Signed-off-by: Mark Brown <[email protected]>
> Cc: [email protected]
> ---
>
> All mainline users of the s3c-fb driver are affected by this, none
> provide refresh.
>
> drivers/video/s3c-fb.c | 4 +++-
> 1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
> index adf9632..0f2e8f6 100644
> --- a/drivers/video/s3c-fb.c
> +++ b/drivers/video/s3c-fb.c
> @@ -224,7 +224,8 @@ static int s3c_fb_calc_pixclk(unsigned char id, struct s3c_fb *sfb, unsigned int
> unsigned long clk = clk_get_rate(sfb->bus_clk);
> unsigned int result;
>
> - pixclk *= win->win_mode.refresh;
> + if (win->win_mode.refresh)
> + pixclk *= win->win_mode.refresh;
> result = clk / pixclk;
>
> dev_dbg(sfb->dev, "pixclk=%u, clk=%lu, div=%d (%lu)\n",
> @@ -766,6 +767,7 @@ static int __devinit s3c_fb_probe_win(struct s3c_fb *sfb, unsigned int win_no,
> WARN_ON(windata->max_bpp == 0);
> WARN_ON(windata->win_mode.xres == 0);
> WARN_ON(windata->win_mode.yres == 0);
> + WARN_ON(windata->win_mode.refresh == 0);
>
> win = fbinfo->par;
> var = &fbinfo->var;
> --
> 1.6.6
>
--
--
Ben
Q: What's a light-year?
A: One-third less calories than a regular year.
On Thu, Jan 14, 2010 at 07:36:17AM +0000, Ben Dooks wrote:
> Thirdly he's changed the code to calculate the divider to assume pixclk
> is a frequency, and not a time-period. It clearly stats in the docs
> that pixclk is in picoseconds. Our clock rate is in Hz.
> It seems I wasn't cc'd on the original submission or I'd have said no.
> I'd rather see this one reverted.
A revert also does the job for SMDK6410 (and does it better) so that
sounds good to me too - I'd been wondering why I couldn't figure out
what was supposed to go in there. I'll send something out with a
changelog shortly.
Commit 600ce1a0faafeed1ce6bcfd421bc040b941cbbc1. ("fix clock setting
for Samsung SoC Framebuffer") introduced a mandatory refresh parameter
to the platform data for the S3C framebuffer but did not introduce any
validation code, causing existing platforms (none of which have refresh
set) to divide by zero whenever the framebuffer is configured,
generating warnings and unusable output.
Ben Dooks noted several problems with the patch:
- The platform data supplies the pixclk directly and should already
have taken care of the refresh rate.
- The addition of a window ID parameter doesn't help since only the
root framebuffer can control the pixclk.
- pixclk is specified in picoseconds (rather than Hz) as the patch
assumed.
and suggests reverting the commit so do that. Without fixing this no
mainline user of the driver will produce output.
Signed-off-by: Mark Brown <[email protected]>
Cc: [email protected]
---
drivers/video/s3c-fb.c | 17 ++++++++++-------
1 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
index adf9632..e2e7afd 100644
--- a/drivers/video/s3c-fb.c
+++ b/drivers/video/s3c-fb.c
@@ -211,21 +211,23 @@ static int s3c_fb_check_var(struct fb_var_screeninfo *var,
/**
* s3c_fb_calc_pixclk() - calculate the divider to create the pixel clock.
- * @id: window id.
* @sfb: The hardware state.
* @pixclock: The pixel clock wanted, in picoseconds.
*
* Given the specified pixel clock, work out the necessary divider to get
* close to the output frequency.
*/
-static int s3c_fb_calc_pixclk(unsigned char id, struct s3c_fb *sfb, unsigned int pixclk)
+static int s3c_fb_calc_pixclk(struct s3c_fb *sfb, unsigned int pixclk)
{
- struct s3c_fb_pd_win *win = sfb->pdata->win[id];
unsigned long clk = clk_get_rate(sfb->bus_clk);
+ unsigned long long tmp;
unsigned int result;
- pixclk *= win->win_mode.refresh;
- result = clk / pixclk;
+ tmp = (unsigned long long)clk;
+ tmp *= pixclk;
+
+ do_div(tmp, 1000000000UL);
+ result = (unsigned int)tmp / 1000;
dev_dbg(sfb->dev, "pixclk=%u, clk=%lu, div=%d (%lu)\n",
pixclk, clk, result, clk / result);
@@ -265,7 +267,6 @@ static int s3c_fb_set_par(struct fb_info *info)
struct s3c_fb *sfb = win->parent;
void __iomem *regs = sfb->regs;
int win_no = win->index;
- u32 osdc_data = 0;
u32 data;
u32 pagewidth;
int clkdiv;
@@ -301,7 +302,7 @@ static int s3c_fb_set_par(struct fb_info *info)
/* use window 0 as the basis for the lcd output timings */
if (win_no == 0) {
- clkdiv = s3c_fb_calc_pixclk(win_no, sfb, var->pixclock);
+ clkdiv = s3c_fb_calc_pixclk(sfb, var->pixclock);
data = sfb->pdata->vidcon0;
data &= ~(VIDCON0_CLKVAL_F_MASK | VIDCON0_CLKDIR);
@@ -358,6 +359,8 @@ static int s3c_fb_set_par(struct fb_info *info)
data = var->xres * var->yres;
+ u32 osdc_data = 0;
+
osdc_data = VIDISD14C_ALPHA1_R(0xf) |
VIDISD14C_ALPHA1_G(0xf) |
VIDISD14C_ALPHA1_B(0xf);
--
1.6.6
On Thu, 14 Jan 2010 10:26:23 +0000
Mark Brown <[email protected]> wrote:
> Commit 600ce1a0faafeed1ce6bcfd421bc040b941cbbc1. ("fix clock setting
> for Samsung SoC Framebuffer") introduced a mandatory refresh parameter
> to the platform data for the S3C framebuffer but did not introduce any
> validation code, causing existing platforms (none of which have refresh
> set) to divide by zero whenever the framebuffer is configured,
> generating warnings and unusable output.
>
> Ben Dooks noted several problems with the patch:
>
> - The platform data supplies the pixclk directly and should already
> have taken care of the refresh rate.
> - The addition of a window ID parameter doesn't help since only the
> root framebuffer can control the pixclk.
> - pixclk is specified in picoseconds (rather than Hz) as the patch
> assumed.
>
> and suggests reverting the commit so do that. Without fixing this no
> mainline user of the driver will produce output.
>
Why on earth would you revert someone's patch and not tell them that
you're doing it?
This reversion will presumably break the Samsung SoC Framebuffer again.
How about we not do that?
> @@ -358,6 +359,8 @@ static int s3c_fb_set_par(struct fb_info *info)
>
> data = var->xres * var->yres;
>
> + u32 osdc_data = 0;
> +
> osdc_data = VIDISD14C_ALPHA1_R(0xf) |
> VIDISD14C_ALPHA1_G(0xf) |
> VIDISD14C_ALPHA1_B(0xf);
The reversion restores this incorrect c99-style declaration.
On Thu, Jan 14, 2010 at 03:06:58PM -0800, Andrew Morton wrote:
> Why on earth would you revert someone's patch and not tell them that
> you're doing it?
Through oversight, sorry - the original patch was an incremental update
to the driver rather than a reversion so I CCed the relevant maintainers
then and maintained the same CC list for the followup.
> This reversion will presumably break the Samsung SoC Framebuffer again.
> How about we not do that?
It's not entirely clear that there is any breakage to fix. Certainly
the SMDK6410 I have works perfectly fine with the reversion, and I've
had a report that the hct board does too (which makes two out of the
three mainline users AFAICT). Since none of the boards in mainline
supply a refresh parameter they'll all be failing with the current code.
Ben's analysis, which seemed reasonable to me, was that the patch is
based on a misunderstanding of the units in which the pixclk parameter
is specified. He wrote:
| Thirdly he's changed the code to calculate the divider to assume pixclk
| is a frequency, and not a time-period. It clearly stats in the docs
| that pixclk is in picoseconds. Our clock rate is in Hz.
My understanding is that the breakage that was believed to exist was
based on the driver misbehaving if you supply the pixclk as a frequency,
which isn't surprising since that's what it's looking for. The effect
of the patch is to change the interpretation of pixclk value so that a
refresh rate must also be specified but either way it should be possible
to specify a working configuration.
There's possibly a case to be made for changing the platform data over
to specify things differently, I'm not sufficiently familiar with the
area to say one way or another, but given that the original method works
and existing machine are broken by the change it seems safer to revert
at least for -rc.
On Fri, Jan 15, 2010 at 12:01:16AM +0000, Mark Brown wrote:
> My understanding is that the breakage that was believed to exist was
> based on the driver misbehaving if you supply the pixclk as a frequency,
> which isn't surprising since that's what it's looking for. The effect
^ not
obviously.