* Re: Should I use FBINFO_VIRTFB?
From: Konrad Rzeszutek Wilk @ 2011-12-15 21:13 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <4EEA5DA7.3010406@freescale.com>
On Thu, Dec 15, 2011 at 02:50:47PM -0600, Timur Tabi wrote:
> I'm cleaning up my fbdev driver, and I just noticed FBINFO_VIRTFB:
>
> #define FBINFO_VIRTFB 0x0004 /* FB is System RAM, not device. */
>
> I am currently not setting this flag, but I am allocating my framebuffer in system ram via dma_alloc_coherent(). I don't see any good documentation for this flag, but I suspect I should be enabling it. What exactly does this flag do?
It basically inhibits from using VM_IO which should not be set on
System RAM. In your case you are using System RAM, so please do set it.
>
> I'd also like some explanation for these two macros, which appear to be related:
>
> #define FBINFO_PARTIAL_PAN_OK 0x0040 /* otw use pan only for double-buffering */
> #define FBINFO_READS_FAST 0x0080 /* soft-copy faster than rendering */
Not really. They have a different function, but you are better of looking in the code
to see how they are used.
>
> --
> Timur Tabi
> Linux kernel developer at Freescale
>
> --
> 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
^ permalink raw reply
* Re: [PATCH v4 3/5] powerpc/mpc5121: shared DIU framebuffer support
From: Anatolij Gustschin @ 2011-12-15 21:26 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1279893639-24333-4-git-send-email-agust@denx.de>
On Thu, 15 Dec 2011 17:27:53 +0000
Tabi Timur-B04825 <B04825@freescale.com> wrote:
> On Fri, Jul 23, 2010 at 9:00 AM, Anatolij Gustschin <agust@denx.de> wrote:
>
> > @@ -1471,7 +1476,9 @@ static int __devinit fsl_diu_probe(struct of_device *ofdev,
> > goto error2;
> > }
> >
> > - out_be32(&dr.diu_reg->diu_mode, 0); /* disable DIU anyway*/
> > + diu_mode = in_be32(&dr.diu_reg->diu_mode);
> > + if (diu_mode != MFB_MODE1)
> > + out_be32(&dr.diu_reg->diu_mode, 0); /* disable DIU */
>
> Anatolij,
>
> I know this patch is old, but I just noticed something odd about it
> that I need your help with.
>
> In the above snippet, you test for != MFB_MODE1. My understanding is
> that U-boot only supports modes 0 and 1, never modes 2 or 3. So
> diu_mode can only ever be 0 or 1. That means that that the above code
> is equivalent to:
>
> diu_mode = in_be32(&dr.diu_reg->diu_mode);
> if (diu_mode = 0)
> out_be32(&dr.diu_reg->diu_mode, 0); /* disable DIU */
>
> which is silly, because now we're writing 0 to diu_mode only if it's already 0.
>
> Am I missing something?
the intention of the above code snippet was:
not to disable DIU if it is in mode 1 (displaying splash screen)
and to disable DIU if it is in modes 2, 3 or 4 for some reason.
We cannot guarantee that the DIU is not in these modes. Even
if U-Boot didn't set these modes there is still a possibility
that such mode is configured. E.g. I've seen U-Boot binary
standalone applications for other display controllers initializing
the display controller.
But you are right. With this snippet, if the DIU is already
disabled, there will be not needed mode register access. So
the code should better look like:
diu_mode = in_be32(&dr.diu_reg->diu_mode);
if (diu_mode && diu_mode != MFB_MODE1)
out_be32(&dr.diu_reg->diu_mode, 0);
Anatolij
^ permalink raw reply
* Re: Should I use FBINFO_VIRTFB?
From: Timur Tabi @ 2011-12-15 22:10 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <4EEA5DA7.3010406@freescale.com>
Konrad Rzeszutek Wilk wrote:
>> > #define FBINFO_PARTIAL_PAN_OK 0x0040 /* otw use pan only for double-buffering */
>> > #define FBINFO_READS_FAST 0x0080 /* soft-copy faster than rendering */
> Not really. They have a different function, but you are better of looking in the code
> to see how they are used.
Well, I tried that, which is why I said, "I don't see any good documentation." FBINFO_PARTIAL_PAN_OK appears to be only used in one place:
case SCROLL_PAN_REDRAW:
if ((p->yscroll + count < 2 * (p->vrows - vc->vc_rows))
&& ((!scroll_partial && (b - t = vc->vc_rows))
|| (scroll_partial
&& (b - t - count >
3 * vc->vc_rows >> 2)))) {
if (t > 0)
fbcon_redraw_move(vc, p, 0, t, count);
ypan_up_redraw(vc, t, count);
if (vc->vc_rows - b > 0)
fbcon_redraw_move(vc, p, b,
vc->vc_rows - b, b);
} else
fbcon_redraw_move(vc, p, t + count, b - t - count, t);
fbcon_clear(vc, b - count, 0, count, vc->vc_cols);
break;
I can't parse this, and I can't figure out if my driver is better off with or without FBINFO_PARTIAL_PAN_OK.
I have the same problem with FBINFO_READS_FAST.
So I really would like someone explain these macros to me.
--
Timur Tabi
Linux kernel developer at Freescale
^ permalink raw reply
* Re: [PATCH 33/57] fbdev: sh_mobile_lcdc: Add sh_mobile_format_info()
From: Guennadi Liakhovetski @ 2011-12-15 22:17 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1323784972-24205-34-git-send-email-laurent.pinchart@ideasonboard.com>
On Tue, 13 Dec 2011, Laurent Pinchart wrote:
> The function returns a pointer to a structure describing a format based
> on its fourcc. Use the function where applicable instead of hardcoded
> switch-case statements.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/video/sh_mobile_lcdcfb.c | 174 ++++++++++++++++++++++----------------
> 1 files changed, 102 insertions(+), 72 deletions(-)
>
> diff --git a/drivers/video/sh_mobile_lcdcfb.c b/drivers/video/sh_mobile_lcdcfb.c
> index 3bc82ae..c6b6b9d 100644
> --- a/drivers/video/sh_mobile_lcdcfb.c
> +++ b/drivers/video/sh_mobile_lcdcfb.c
[snip]
> @@ -665,37 +726,20 @@ static void __sh_mobile_lcdc_start(struct sh_mobile_lcdc_priv *priv)
>
> /* Setup geometry, format, frame buffer memory and operation mode. */
> for (k = 0; k < ARRAY_SIZE(priv->ch); k++) {
> + const struct sh_mobile_lcdc_format_info *format;
> + u32 fourcc;
> +
> ch = &priv->ch[k];
> if (!ch->enabled)
> continue;
>
> sh_mobile_lcdc_geometry(ch);
>
> - switch (sh_mobile_format_fourcc(&ch->info->var)) {
> - case V4L2_PIX_FMT_RGB565:
> - tmp = LDDFR_PKF_RGB16;
> - break;
> - case V4L2_PIX_FMT_BGR24:
> - tmp = LDDFR_PKF_RGB24;
> - break;
> - case V4L2_PIX_FMT_BGR32:
> - tmp = LDDFR_PKF_ARGB32;
> - break;
> - case V4L2_PIX_FMT_NV12:
> - case V4L2_PIX_FMT_NV21:
> - tmp = LDDFR_CC | LDDFR_YF_420;
> - break;
> - case V4L2_PIX_FMT_NV16:
> - case V4L2_PIX_FMT_NV61:
> - tmp = LDDFR_CC | LDDFR_YF_422;
> - break;
> - case V4L2_PIX_FMT_NV24:
> - case V4L2_PIX_FMT_NV42:
> - tmp = LDDFR_CC | LDDFR_YF_444;
> - break;
> - }
> + fourcc = sh_mobile_format_fourcc(&ch->info->var);
> + format = sh_mobile_format_info(fourcc);
> + tmp = format->lddfr;
Why don't you just store a pointer to the selected format struct per
channel in sh_mobile_lcdc_channel_init() to avoid recalculation here? If
OTOH this can be a new info here from a hotplug event, the above risks an
Oops?
>
> - if (sh_mobile_format_is_yuv(&ch->info->var)) {
> + if (format->yuv) {
> switch (ch->info->var.colorspace) {
> case V4L2_COLORSPACE_REC709:
> tmp |= LDDFR_CF1;
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply
* Re: [PATCH 34/57] fbdev: sh_mobile_lcdc: Store the format in struct
From: Guennadi Liakhovetski @ 2011-12-15 22:29 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1323784972-24205-35-git-send-email-laurent.pinchart@ideasonboard.com>
On Tue, 13 Dec 2011, Laurent Pinchart wrote:
> Store the active format in the channel structure, and use it instead of
> parsing info->var all over the place when the format is needed.
Right, this is what I was wondering about, while looking at the previous
patch:-) But:
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/video/sh_mobile_lcdcfb.c | 21 ++++++++++-----------
> drivers/video/sh_mobile_lcdcfb.h | 4 +++-
> 2 files changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/video/sh_mobile_lcdcfb.c b/drivers/video/sh_mobile_lcdcfb.c
> index c6b6b9d..9829e01 100644
> --- a/drivers/video/sh_mobile_lcdcfb.c
> +++ b/drivers/video/sh_mobile_lcdcfb.c
[snip]
> @@ -1350,6 +1345,8 @@ static int sh_mobile_set_par(struct fb_info *info)
> info->fix.line_length = info->var.xres
> * info->var.bits_per_pixel / 8;
>
> + ch->format = sh_mobile_format_info(sh_mobile_format_fourcc(&info->var));
Cannot this be NULL? As far as I could trace it back, I'm not sure with
hotplug bits_per_pixel would be initialised correctly along the lines of
sh_mobile_fb_reconfig().
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply
* Re: [PATCH 1/2] OMAPDSS: DISPC: Update Fir Coefficients
From: Mahapatra, Chandrabhanu @ 2011-12-16 4:58 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-omap, linux-fbdev
In-Reply-To: <1323939331.2010.27.camel@deskari>
On Thu, Dec 15, 2011 at 2:25 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Wed, 2011-12-14 at 10:21 +0530, Chandrabhanu Mahapatra wrote:
>
>> +const struct dispc_coef *dispc_ovl_get_scale_coef(int inc, int five_taps)
>> +{
>> + int i;
>> + static const struct {
>> + int Mmin;
>> + int Mmax;
>> + const struct dispc_coef *coef_3;
>> + const struct dispc_coef *coef_5;
>> + } coefs[] = {
>> + { 26, 32, coef3_M32, coef5_M32 },
>> + { 22, 26, coef3_M26, coef5_M26 },
>> + { 19, 22, coef3_M22, coef5_M22 },
>> + { 16, 19, coef3_M19, coef5_M19 },
>> + { 14, 16, coef3_M16, coef5_M16 },
>> + { 13, 14, coef3_M14, coef5_M14 },
>> + { 12, 13, coef3_M13, coef5_M13 },
>> + { 11, 12, coef3_M12, coef5_M12 },
>> + { 10, 11, coef3_M11, coef5_M11 },
>> + { 9, 10, coef3_M10, coef5_M10 },
>> + { 8, 9, coef3_M9, coef5_M9 },
>> + { 3, 8, coef3_M8, coef5_M8 },
>> + /*
>> + * When upscaling more than two times, blockiness and outlines
>> + * around the image are observed when M8 tables are used. M11,
>> + * M16 and M19 tables are used to prevent this.
>> + */
>> + { 2, 3, coef3_M11, coef5_M11 },
>> + { 1, 2, coef3_M16, coef5_M16 },
>> + };
>> +
>> + inc /= 128;
>> + for (i = 0; i < ARRAY_LEN(coefs); ++i)
>> + if (inc > coefs[i].Mmin && inc <= coefs[i].Mmax)
>> + return five_taps ? coefs[i].coef_5 : coefs[i].coef_3;
>> + if (inc = 1)
>> + return five_taps ? coef3_M19 : coef5_M19;
>> + return NULL;
>> +}
>
> Why don't you handle the inc = 1 case the same as others? Just have an
> entry in the table for Mmin=0, Mmax = 1.
>
For inc=1 i.e. M=1 , scaling ratio is maximum as L/M=8. DISPC scaler
doesnot support upscaling more than 8 itmes. Having an (Mmin,Mmax] of
(0,1] will allow such cases.
> Also, I think it's a bit confusing that Mmin is exclusive and Mmax is
> inclusive in the comparison. It makes the table a bit hard to read, when
> looking at which entry is used for which inc. I'd recommend using
> inclusive comparison for both.
>
> Tomi
>
Having both inclusive will allow us to delete the extra comparison for
inc=1 but in my opinion having Mmin exclusive and Mmax inclusive
actually gives an clear idea of comparison. The tables mostly go by
the Mmax value.
For example, for inc& coef3/5_M26 table is selected, for inc"
coef3/5_M22 is selected etc.
If we have both Mmin and Mmax as inclusive above case becomes slightly
incoherent. Say for M& instead of coef3/5_M26 which seems more
obvious choice coef3/5_M32 is selected.
For both inclusive cases to work and avoid confusion and delete extra
comparison for inc=1 , I have to reverse the order of table entries
in "coef" table. But for that I will have to put the "When upscaling
more than two times, blockiness and outlines" comment at the beginning
of the table and then start with { 1, 2, coef3_M16, coef5_M16 }.
This will create even more confusion.
--
Chandrabhanu Mahapatra
Texas Instruments India Pvt. Ltd.
^ permalink raw reply
* Re: [PATCH 1/2] OMAPDSS: DISPC: Update Fir Coefficients
From: Tomi Valkeinen @ 2011-12-16 8:15 UTC (permalink / raw)
To: Mahapatra, Chandrabhanu; +Cc: linux-omap, linux-fbdev
In-Reply-To: <CAF0AtAtaCGpgeYxss0H_jbYWOwJJ_LW0W8-eqM20juKTAGassA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 4198 bytes --]
On Fri, 2011-12-16 at 10:26 +0530, Mahapatra, Chandrabhanu wrote:
> On Thu, Dec 15, 2011 at 2:25 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> > On Wed, 2011-12-14 at 10:21 +0530, Chandrabhanu Mahapatra wrote:
> >
> >> +const struct dispc_coef *dispc_ovl_get_scale_coef(int inc, int five_taps)
> >> +{
> >> + int i;
> >> + static const struct {
> >> + int Mmin;
> >> + int Mmax;
> >> + const struct dispc_coef *coef_3;
> >> + const struct dispc_coef *coef_5;
> >> + } coefs[] = {
> >> + { 26, 32, coef3_M32, coef5_M32 },
> >> + { 22, 26, coef3_M26, coef5_M26 },
> >> + { 19, 22, coef3_M22, coef5_M22 },
> >> + { 16, 19, coef3_M19, coef5_M19 },
> >> + { 14, 16, coef3_M16, coef5_M16 },
> >> + { 13, 14, coef3_M14, coef5_M14 },
> >> + { 12, 13, coef3_M13, coef5_M13 },
> >> + { 11, 12, coef3_M12, coef5_M12 },
> >> + { 10, 11, coef3_M11, coef5_M11 },
> >> + { 9, 10, coef3_M10, coef5_M10 },
> >> + { 8, 9, coef3_M9, coef5_M9 },
> >> + { 3, 8, coef3_M8, coef5_M8 },
> >> + /*
> >> + * When upscaling more than two times, blockiness and outlines
> >> + * around the image are observed when M8 tables are used. M11,
> >> + * M16 and M19 tables are used to prevent this.
> >> + */
> >> + { 2, 3, coef3_M11, coef5_M11 },
> >> + { 1, 2, coef3_M16, coef5_M16 },
> >> + };
> >> +
> >> + inc /= 128;
> >> + for (i = 0; i < ARRAY_LEN(coefs); ++i)
> >> + if (inc > coefs[i].Mmin && inc <= coefs[i].Mmax)
> >> + return five_taps ? coefs[i].coef_5 : coefs[i].coef_3;
> >> + if (inc == 1)
> >> + return five_taps ? coef3_M19 : coef5_M19;
> >> + return NULL;
> >> +}
> >
> > Why don't you handle the inc == 1 case the same as others? Just have an
> > entry in the table for Mmin=0, Mmax = 1.
> >
> For inc=1 i.e. M=1 , scaling ratio is maximum as L/M=8. DISPC scaler
> doesnot support upscaling more than 8 itmes. Having an (Mmin,Mmax] of
> (0,1] will allow such cases.
I don't think I understand. A table entry for 0,1 would match exactly
one inc value, which is 1. Which is the same as you do with the separate
if statement now.
> > Also, I think it's a bit confusing that Mmin is exclusive and Mmax is
> > inclusive in the comparison. It makes the table a bit hard to read, when
> > looking at which entry is used for which inc. I'd recommend using
> > inclusive comparison for both.
> >
> > Tomi
> >
> Having both inclusive will allow us to delete the extra comparison for
> inc==1 but in my opinion having Mmin exclusive and Mmax inclusive
> actually gives an clear idea of comparison. The tables mostly go by
> the Mmax value.
> For example, for inc=26 coef3/5_M26 table is selected, for inc=22
> coef3/5_M22 is selected etc.
> If we have both Mmin and Mmax as inclusive above case becomes slightly
> incoherent. Say for M=26 instead of coef3/5_M26 which seems more
> obvious choice coef3/5_M32 is selected.
I don't understand this either... If you now have:
{ 26, 32, coef3_M32, coef5_M32 },
{ 22, 26, coef3_M26, coef5_M26 },
It would be changed to
{ 27, 32, coef3_M32, coef5_M32 },
{ 23, 26, coef3_M26, coef5_M26 },
and it would match the same inc values as before after changing the Mmin
comparison to >=.
> For both inclusive cases to work and avoid confusion and delete extra
> comparison for inc==1 , I have to reverse the order of table entries
> in "coef" table. But for that I will have to put the "When upscaling
> more than two times, blockiness and outlines" comment at the beginning
> of the table and then start with { 1, 2, coef3_M16, coef5_M16 }.
> This will create even more confusion.
The ranges for the table elements are exclusive. The order doesn't
matter because one inc value can only match one table entry. So I have
to say I don't understand this comment either =). Am I missing
something?
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 1/2] OMAPDSS: DISPC: Update Fir Coefficients
From: Mahapatra, Chandrabhanu @ 2011-12-16 8:48 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-fbdev, linux-omap
In-Reply-To: <1324023305.1859.9.camel@deskari>
On Fri, Dec 16, 2011 at 1:45 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Fri, 2011-12-16 at 10:26 +0530, Mahapatra, Chandrabhanu wrote:
>> On Thu, Dec 15, 2011 at 2:25 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> > On Wed, 2011-12-14 at 10:21 +0530, Chandrabhanu Mahapatra wrote:
>> >
>> >> +const struct dispc_coef *dispc_ovl_get_scale_coef(int inc, int five_taps)
>> >> +{
>> >> + int i;
>> >> + static const struct {
>> >> + int Mmin;
>> >> + int Mmax;
>> >> + const struct dispc_coef *coef_3;
>> >> + const struct dispc_coef *coef_5;
>> >> + } coefs[] = {
>> >> + { 26, 32, coef3_M32, coef5_M32 },
>> >> + { 22, 26, coef3_M26, coef5_M26 },
>> >> + { 19, 22, coef3_M22, coef5_M22 },
>> >> + { 16, 19, coef3_M19, coef5_M19 },
>> >> + { 14, 16, coef3_M16, coef5_M16 },
>> >> + { 13, 14, coef3_M14, coef5_M14 },
>> >> + { 12, 13, coef3_M13, coef5_M13 },
>> >> + { 11, 12, coef3_M12, coef5_M12 },
>> >> + { 10, 11, coef3_M11, coef5_M11 },
>> >> + { 9, 10, coef3_M10, coef5_M10 },
>> >> + { 8, 9, coef3_M9, coef5_M9 },
>> >> + { 3, 8, coef3_M8, coef5_M8 },
>> >> + /*
>> >> + * When upscaling more than two times, blockiness and outlines
>> >> + * around the image are observed when M8 tables are used. M11,
>> >> + * M16 and M19 tables are used to prevent this.
>> >> + */
>> >> + { 2, 3, coef3_M11, coef5_M11 },
>> >> + { 1, 2, coef3_M16, coef5_M16 },
>> >> + };
>> >> +
>> >> + inc /= 128;
>> >> + for (i = 0; i < ARRAY_LEN(coefs); ++i)
>> >> + if (inc > coefs[i].Mmin && inc <= coefs[i].Mmax)
>> >> + return five_taps ? coefs[i].coef_5 : coefs[i].coef_3;
>> >> + if (inc = 1)
>> >> + return five_taps ? coef3_M19 : coef5_M19;
>> >> + return NULL;
>> >> +}
>> >
>> > Why don't you handle the inc = 1 case the same as others? Just have an
>> > entry in the table for Mmin=0, Mmax = 1.
>> >
>> For inc=1 i.e. M=1 , scaling ratio is maximum as L/M=8. DISPC scaler
>> doesnot support upscaling more than 8 itmes. Having an (Mmin,Mmax] of
>> (0,1] will allow such cases.
>
> I don't think I understand. A table entry for 0,1 would match exactly
> one inc value, which is 1. Which is the same as you do with the separate
> if statement now.
>
yes, you are right.
>> > Also, I think it's a bit confusing that Mmin is exclusive and Mmax is
>> > inclusive in the comparison. It makes the table a bit hard to read, when
>> > looking at which entry is used for which inc. I'd recommend using
>> > inclusive comparison for both.
>> >
>> > Tomi
>> >
>> Having both inclusive will allow us to delete the extra comparison for
>> inc=1 but in my opinion having Mmin exclusive and Mmax inclusive
>> actually gives an clear idea of comparison. The tables mostly go by
>> the Mmax value.
>> For example, for inc& coef3/5_M26 table is selected, for inc"
>> coef3/5_M22 is selected etc.
>> If we have both Mmin and Mmax as inclusive above case becomes slightly
>> incoherent. Say for M& instead of coef3/5_M26 which seems more
>> obvious choice coef3/5_M32 is selected.
>
> I don't understand this either... If you now have:
>
> { 26, 32, coef3_M32, coef5_M32 },
> { 22, 26, coef3_M26, coef5_M26 },
>
> It would be changed to
>
> { 27, 32, coef3_M32, coef5_M32 },
> { 23, 26, coef3_M26, coef5_M26 },
>
> and it would match the same inc values as before after changing the Mmin
> comparison to >=.
>
yes, I had mistakenly overlooked it.
>> For both inclusive cases to work and avoid confusion and delete extra
>> comparison for inc=1 , I have to reverse the order of table entries
>> in "coef" table. But for that I will have to put the "When upscaling
>> more than two times, blockiness and outlines" comment at the beginning
>> of the table and then start with { 1, 2, coef3_M16, coef5_M16 }.
>> This will create even more confusion.
>
> The ranges for the table elements are exclusive. The order doesn't
> matter because one inc value can only match one table entry. So I have
> to say I don't understand this comment either =). Am I missing
> something?
>
> Tomi
>
I mistakenly thought inc to be float which created all this confusion.
--
Chandrabhanu Mahapatra
Texas Instruments India Pvt. Ltd.
^ permalink raw reply
* Re: [PATCH 25/57] sh_mobile_lcdc: Add display notify callback to
From: Guennadi Liakhovetski @ 2011-12-16 9:40 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1323784972-24205-26-git-send-email-laurent.pinchart@ideasonboard.com>
On Tue, 13 Dec 2011, Laurent Pinchart wrote:
> The callback implements 3 notification events:
>
> - SH_MOBILE_LCDC_EVENT_DISPLAY_CONNECT notifies the LCDC that the
> display has been connected
> - SH_MOBILE_LCDC_EVENT_DISPLAY_DISCONNECT notifies the LCDC that the
> display has been disconnected
> - SH_MOBILE_LCDC_EVENT_DISPLAY_MODE notifies that LCDC that a display
> mode has been detected
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/video/sh_mobile_lcdcfb.c | 81 ++++++++++++++++++++++++++++++++++++++
> drivers/video/sh_mobile_lcdcfb.h | 10 +++++
> 2 files changed, 91 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/video/sh_mobile_lcdcfb.c b/drivers/video/sh_mobile_lcdcfb.c
> index 4b03aa5..21e5f10 100644
> --- a/drivers/video/sh_mobile_lcdcfb.c
> +++ b/drivers/video/sh_mobile_lcdcfb.c
> @@ -363,6 +363,86 @@ static void sh_mobile_lcdc_display_off(struct sh_mobile_lcdc_chan *ch)
> ch->tx_dev->ops->display_off(ch->tx_dev);
> }
>
> +static bool
> +sh_mobile_lcdc_must_reconfigure(struct sh_mobile_lcdc_chan *ch,
> + const struct fb_var_screeninfo *new_var)
> +{
> + struct fb_var_screeninfo *old_var = &ch->display_var;
> + struct fb_videomode old_mode;
> + struct fb_videomode new_mode;
> +
> + fb_var_to_videomode(&old_mode, old_var);
> + fb_var_to_videomode(&new_mode, new_var);
> +
> + dev_dbg(ch->info->dev, "Old %ux%u, new %ux%u\n",
> + old_mode.xres, old_mode.yres, new_mode.xres, new_mode.yres);
> +
> + if (fb_mode_is_equal(&old_mode, &new_mode)) {
> + /* It can be a different monitor with an equal video-mode */
> + old_var->width = new_var->width;
> + old_var->height = new_var->height;
> + return false;
> + }
> +
> + dev_dbg(ch->info->dev, "Switching %u -> %u lines\n",
> + old_mode.yres, new_mode.yres);
> + *old_var = *new_var;
> +
> + return true;
> +}
> +
> +static int sh_mobile_lcdc_display_notify(struct sh_mobile_lcdc_chan *ch,
> + enum sh_mobile_lcdc_entity_event event,
> + struct fb_var_screeninfo *var)
> +{
> + struct fb_info *info = ch->info;
> + int ret = 0;
> +
> + switch (event) {
> + case SH_MOBILE_LCDC_EVENT_DISPLAY_CONNECT:
> + /* HDMI plug in */
> + if (lock_fb_info(info)) {
> + console_lock();
> +
> + if (!sh_mobile_lcdc_must_reconfigure(ch, var) &&
> + info->state = FBINFO_STATE_RUNNING) {
> + /* First activation with the default monitor.
> + * Just turn on, if we run a resume here, the
> + * logo disappears.
> + */
> + info->var.width = var->width;
> + info->var.height = var->height;
> + sh_mobile_lcdc_display_on(ch);
> + } else {
> + /* New monitor or have to wake up */
> + fb_set_suspend(info, 0);
> + }
> +
> + console_unlock();
> + unlock_fb_info(info);
> + }
> + break;
> +
> + case SH_MOBILE_LCDC_EVENT_DISPLAY_DISCONNECT:
> + /* HDMI disconnect */
> + if (lock_fb_info(info)) {
> + console_lock();
> + fb_set_suspend(info, 1);
> + console_unlock();
> + unlock_fb_info(info);
> + }
> + break;
> +
> + case SH_MOBILE_LCDC_EVENT_DISPLAY_MODE:
> + /* Validate a proposed new mode */
> + var->bits_per_pixel = info->var.bits_per_pixel;
> + ret = info->fbops->fb_check_var(var, info);
You can call sh_mobile_lcdc_check_var() directly here too, right?
Thanks
Guennadi
> + break;
> + }
> +
> + return ret;
> +}
> +
> /* -----------------------------------------------------------------------------
> * Format helpers
> */
> @@ -1591,6 +1671,7 @@ sh_mobile_lcdc_channel_init(struct sh_mobile_lcdc_priv *priv,
> int i;
>
> mutex_init(&ch->open_lock);
> + ch->notify = sh_mobile_lcdc_display_notify;
>
> /* Allocate the frame buffer device. */
> ch->info = framebuffer_alloc(0, priv->dev);
> diff --git a/drivers/video/sh_mobile_lcdcfb.h b/drivers/video/sh_mobile_lcdcfb.h
> index 6fb956c..e2eb7af 100644
> --- a/drivers/video/sh_mobile_lcdcfb.h
> +++ b/drivers/video/sh_mobile_lcdcfb.h
> @@ -30,6 +30,12 @@ struct sh_mobile_lcdc_entity_ops {
> void (*display_off)(struct sh_mobile_lcdc_entity *entity);
> };
>
> +enum sh_mobile_lcdc_entity_event {
> + SH_MOBILE_LCDC_EVENT_DISPLAY_CONNECT,
> + SH_MOBILE_LCDC_EVENT_DISPLAY_DISCONNECT,
> + SH_MOBILE_LCDC_EVENT_DISPLAY_MODE,
> +};
> +
> struct sh_mobile_lcdc_entity {
> struct module *owner;
> const struct sh_mobile_lcdc_entity_ops *ops;
> @@ -70,6 +76,10 @@ struct sh_mobile_lcdc_chan {
> unsigned long base_addr_y;
> unsigned long base_addr_c;
> unsigned int pitch;
> +
> + int (*notify)(struct sh_mobile_lcdc_chan *ch,
> + enum sh_mobile_lcdc_entity_event event,
> + struct fb_var_screeninfo *var);
> };
>
> #endif
> --
> 1.7.3.4
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply
* Re: [PATCH 21/57] sh_mobile_lcdc: Add an lcdc channel pointer to sh_mobile_lcdc_entity
From: Laurent Pinchart @ 2011-12-16 9:52 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1323784972-24205-22-git-send-email-laurent.pinchart@ideasonboard.com>
Hi Guennadi,
On Thursday 15 December 2011 17:16:16 Guennadi Liakhovetski wrote:
> On Tue, 13 Dec 2011, Laurent Pinchart wrote:
> > The field will be used by the transmitter drivers to access
> > sh_mobile_lcdc_chan fields such as fb_info.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >
> > drivers/video/sh_mobile_lcdcfb.c | 5 ++++-
> > drivers/video/sh_mobile_lcdcfb.h | 2 ++
> > 2 files changed, 6 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/video/sh_mobile_lcdcfb.c
> > b/drivers/video/sh_mobile_lcdcfb.c index cb5ea3c..2dccfde 100644
> > --- a/drivers/video/sh_mobile_lcdcfb.c
> > +++ b/drivers/video/sh_mobile_lcdcfb.c
> > @@ -1497,8 +1497,10 @@ static int sh_mobile_lcdc_remove(struct
> > platform_device *pdev)
> >
> > if (!info || !info->device)
> >
> > continue;
> >
> > - if (ch->tx_dev)
> > + if (ch->tx_dev) {
> > + ch->tx_dev->lcdc = NULL;
> >
> > module_put(ch->cfg.tx_dev->dev.driver->owner);
> >
> > + }
> >
> > if (ch->sglist)
> >
> > vfree(ch->sglist);
> >
> > @@ -1608,6 +1610,7 @@ sh_mobile_lcdc_channel_init(struct
> > sh_mobile_lcdc_priv *priv,
> >
> > return -EINVAL;
> >
> > }
> > ch->tx_dev = platform_get_drvdata(cfg->tx_dev);
> >
> > + ch->tx_dev->lcdc = ch;
>
> I do not have a kernel, patched with your patches up to 20/57;-) so, I
> cannot verify - can ch->tx_dev at this point not be NULL?
I don't think so, as the
if (!cfg->tx_dev->dev.driver ||
!try_module_get(cfg->tx_dev->dev.driver->owner))
check above ensures that the transmitter driver is loaded and bound to the
device. As the transmitter driver calls platform_Set_drvdata() at probe time,
platform_get_drvdata() will not return NULL.
> > }
> >
> > /* Iterate through the modes to validate them and find the highest
> >
> > diff --git a/drivers/video/sh_mobile_lcdcfb.h
> > b/drivers/video/sh_mobile_lcdcfb.h index 9601b92..36cd564 100644
> > --- a/drivers/video/sh_mobile_lcdcfb.h
> > +++ b/drivers/video/sh_mobile_lcdcfb.h
> > @@ -19,6 +19,7 @@ struct fb_info;
> >
> > struct module;
> > struct sh_mobile_lcdc_entity;
> > struct sh_mobile_lcdc_priv;
> >
> > +struct sh_mobile_lcdc_chan;
> >
> > struct sh_mobile_lcdc_entity_ops {
> >
> > /* Display */
> >
> > @@ -30,6 +31,7 @@ struct sh_mobile_lcdc_entity_ops {
> >
> > struct sh_mobile_lcdc_entity {
> >
> > struct module *owner;
> > const struct sh_mobile_lcdc_entity_ops *ops;
> >
> > + struct sh_mobile_lcdc_chan *lcdc;
> >
> > };
> >
> > /*
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH 12/57] arm: mach-shmobile: Add LCDC tx_dev field to platform data
From: Laurent Pinchart @ 2011-12-16 10:04 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1323784972-24205-13-git-send-email-laurent.pinchart@ideasonboard.com>
Hi Guennadi,
On Thursday 15 December 2011 15:01:27 Guennadi Liakhovetski wrote:
> On Tue, 13 Dec 2011, Laurent Pinchart wrote:
> > Make sure the transmitter devices get registered before the associated
> > LCDC devices.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
[snip]
> > diff --git a/arch/arm/mach-shmobile/board-ap4evb.c
> > b/arch/arm/mach-shmobile/board-ap4evb.c index dca4860..e0a6b3d 100644
> > --- a/arch/arm/mach-shmobile/board-ap4evb.c
> > +++ b/arch/arm/mach-shmobile/board-ap4evb.c
[snip]
> > @@ -627,6 +553,86 @@ static struct platform_device *qhd_devices[]
> > __initdata = {
> >
> > };
> > #endif /* CONFIG_AP4EVB_QHD */
> >
> > +/* LCDC0 */
> > +static const struct fb_videomode ap4evb_lcdc_modes[] = {
>
> Hm, I must be missing something. I thought you were moving all the structs
> around to make them available for reference _without_ forward
> declarations. But you _do_ end up using a forward declaration anyway. Here
> and for HDMI below too. So, what's the point? Why not just forward declare
> that one struct, that's needed and leave the rest where it currently is
> in the file?
As we have circular dependencies forward declaration can't be avoided. I found
it cleaner to have devices data in registration order in the board file, but I
can remove this if you prefer.
> > + {
> > +#ifdef CONFIG_AP4EVB_QHD
> > + .name = "R63302(QHD)",
> > + .xres = 544,
> > + .yres = 961,
> > + .left_margin = 72,
> > + .right_margin = 600,
> > + .hsync_len = 16,
> > + .upper_margin = 8,
> > + .lower_margin = 8,
> > + .vsync_len = 2,
> > + .sync = FB_SYNC_VERT_HIGH_ACT | FB_SYNC_HOR_HIGH_ACT,
> > +#else
> > + .name = "WVGA Panel",
> > + .xres = 800,
> > + .yres = 480,
> > + .left_margin = 220,
> > + .right_margin = 110,
> > + .hsync_len = 70,
> > + .upper_margin = 20,
> > + .lower_margin = 5,
> > + .vsync_len = 5,
> > + .sync = 0,
> > +#endif
> > + },
> > +};
[snip]
> > diff --git a/arch/arm/mach-shmobile/board-mackerel.c
> > b/arch/arm/mach-shmobile/board-mackerel.c index 4ed0138..ccdd9fc 100644
> > --- a/arch/arm/mach-shmobile/board-mackerel.c
> > +++ b/arch/arm/mach-shmobile/board-mackerel.c
[snip]
> > @@ -1313,8 +1317,8 @@ static struct platform_device *mackerel_devices[]
> > __initdata = {
> > &sh_mmcif_device,
> > &ceu_device,
> > &mackerel_camera,
> > - &hdmi_lcdc_device,
> > &hdmi_device,
> > + &hdmi_lcdc_device,
> > &meram_device,
> > };
>
> Sorry, could you elaborate, why this is needed? If this is really
> important, then I'd prefer to test this before committing. If OTOH this is
> unimportant, and my assumption, that normally it's the platform driver
> registration order, that's important, not device registration order, is
> correct, then you don't have to swap the order?
If both the transmitter and LCDC drivers are built-in, the device probe order
is the device registration order. As we need the transmitter to be probed
first, it needs to be registered first.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH 27/57] fbdev: sh_mobile_lcdc: Pass a video mode to the notify callback
From: Laurent Pinchart @ 2011-12-16 10:10 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1323784972-24205-28-git-send-email-laurent.pinchart@ideasonboard.com>
Hi Guennadi,
On Thursday 15 December 2011 20:01:54 Guennadi Liakhovetski wrote:
> On Tue, 13 Dec 2011, Laurent Pinchart wrote:
> > Pass pointers to struct fb_videomode and struct fb_monspecs instead of
> > struct fb_var_screeninfo to the notify callback.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >
> > drivers/video/sh_mobile_hdmi.c | 59
> > +++++++++++++++++-------------------- drivers/video/sh_mobile_lcdcfb.c
> > | 40 ++++++++++++++----------- drivers/video/sh_mobile_lcdcfb.h |
> > 3 +-
> > 3 files changed, 51 insertions(+), 51 deletions(-)
>
> [snip]
>
> > diff --git a/drivers/video/sh_mobile_lcdcfb.c
> > b/drivers/video/sh_mobile_lcdcfb.c index 21e5f10..4ec216e 100644
> > --- a/drivers/video/sh_mobile_lcdcfb.c
> > +++ b/drivers/video/sh_mobile_lcdcfb.c
>
> [ditto]
>
> > @@ -433,12 +432,17 @@ static int sh_mobile_lcdc_display_notify(struct
> > sh_mobile_lcdc_chan *ch,
> >
> > }
> > break;
> >
> > - case SH_MOBILE_LCDC_EVENT_DISPLAY_MODE:
> > + case SH_MOBILE_LCDC_EVENT_DISPLAY_MODE: {
> > + struct fb_var_screeninfo var;
> > +
> >
> > /* Validate a proposed new mode */
> >
> > - var->bits_per_pixel = info->var.bits_per_pixel;
> > - ret = info->fbops->fb_check_var(var, info);
> > + fb_videomode_to_var(&var, mode);
> > + var.bits_per_pixel = info->var.bits_per_pixel;
> > + var.grayscale = info->var.grayscale;
> > + ret = info->fbops->fb_check_var(&var, info);
> >
> > break;
> >
> > }
> >
> > + }
>
> nitpick - please, realign:-)
It's common in driver code not to increase the identation level twice in a
case statement if braces are needed. However, those "braced" statements are
usually not the last ones in the switch. In this particular case this creates
an alignment issue at the end, as your correctly pointed out. However, I'd
like to avoid increasing the indentation of the whole block. Increasing the
indentation of the closing brace only also causes an alignment issue. I can
add a default case that returns an error to fix this, but that's adding code
to fix a cosmetic problem :-) What's your preference.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH 34/57] fbdev: sh_mobile_lcdc: Store the format in struct sh_mobile_lcdc_chan
From: Laurent Pinchart @ 2011-12-16 10:17 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1323784972-24205-35-git-send-email-laurent.pinchart@ideasonboard.com>
Hi Guennadi,
On Thursday 15 December 2011 23:29:29 Guennadi Liakhovetski wrote:
> On Tue, 13 Dec 2011, Laurent Pinchart wrote:
> > Store the active format in the channel structure, and use it instead of
> > parsing info->var all over the place when the format is needed.
>
> Right, this is what I was wondering about, while looking at the previous
> patch:-)
So I'll consider your comment to patch 33/57 to be addressed :-)
> But:
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >
> > drivers/video/sh_mobile_lcdcfb.c | 21 ++++++++++-----------
> > drivers/video/sh_mobile_lcdcfb.h | 4 +++-
> > 2 files changed, 13 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/video/sh_mobile_lcdcfb.c
> > b/drivers/video/sh_mobile_lcdcfb.c index c6b6b9d..9829e01 100644
> > --- a/drivers/video/sh_mobile_lcdcfb.c
> > +++ b/drivers/video/sh_mobile_lcdcfb.c
>
> [snip]
>
> > @@ -1350,6 +1345,8 @@ static int sh_mobile_set_par(struct fb_info *info)
> >
> > info->fix.line_length = info->var.xres
> >
> > * info->var.bits_per_pixel / 8;
> >
> > + ch->format > > sh_mobile_format_info(sh_mobile_format_fourcc(&info->var));
>
> Cannot this be NULL? As far as I could trace it back, I'm not sure with
> hotplug bits_per_pixel would be initialised correctly along the lines of
> sh_mobile_fb_reconfig().
You're right, sh_mobile_fb_reconfig() doesn't set the bits_per_pixel (and
other format-related) field. I'll fix this by initializing those fields from
the current var in sh_mobile_fb_reconfig() in patch 30/57, as that's the one
that introduces the problem.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH 25/57] sh_mobile_lcdc: Add display notify callback to sh_mobile_lcdc_chan
From: Laurent Pinchart @ 2011-12-16 10:22 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1323784972-24205-26-git-send-email-laurent.pinchart@ideasonboard.com>
Hi Guennadi,
On Friday 16 December 2011 10:40:48 Guennadi Liakhovetski wrote:
> On Tue, 13 Dec 2011, Laurent Pinchart wrote:
> > The callback implements 3 notification events:
> >
> > - SH_MOBILE_LCDC_EVENT_DISPLAY_CONNECT notifies the LCDC that the
> > display has been connected
> >
> > - SH_MOBILE_LCDC_EVENT_DISPLAY_DISCONNECT notifies the LCDC that the
> > display has been disconnected
> >
> > - SH_MOBILE_LCDC_EVENT_DISPLAY_MODE notifies that LCDC that a display
> > mode has been detected
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > diff --git a/drivers/video/sh_mobile_lcdcfb.c
> > b/drivers/video/sh_mobile_lcdcfb.c index 4b03aa5..21e5f10 100644
> > --- a/drivers/video/sh_mobile_lcdcfb.c
> > +++ b/drivers/video/sh_mobile_lcdcfb.c
> > @@ -363,6 +363,86 @@ static void sh_mobile_lcdc_display_off(struct
[snip]
> > +static int sh_mobile_lcdc_display_notify(struct sh_mobile_lcdc_chan *ch,
> > + enum sh_mobile_lcdc_entity_event event,
> > + struct fb_var_screeninfo *var)
> > +{
> > + struct fb_info *info = ch->info;
> > + int ret = 0;
> > +
> > + switch (event) {
> > + case SH_MOBILE_LCDC_EVENT_DISPLAY_CONNECT:
> > + /* HDMI plug in */
> > + if (lock_fb_info(info)) {
> > + console_lock();
> > +
> > + if (!sh_mobile_lcdc_must_reconfigure(ch, var) &&
> > + info->state = FBINFO_STATE_RUNNING) {
> > + /* First activation with the default monitor.
> > + * Just turn on, if we run a resume here, the
> > + * logo disappears.
> > + */
> > + info->var.width = var->width;
> > + info->var.height = var->height;
> > + sh_mobile_lcdc_display_on(ch);
> > + } else {
> > + /* New monitor or have to wake up */
> > + fb_set_suspend(info, 0);
> > + }
> > +
> > + console_unlock();
> > + unlock_fb_info(info);
> > + }
> > + break;
> > +
> > + case SH_MOBILE_LCDC_EVENT_DISPLAY_DISCONNECT:
> > + /* HDMI disconnect */
> > + if (lock_fb_info(info)) {
> > + console_lock();
> > + fb_set_suspend(info, 1);
> > + console_unlock();
> > + unlock_fb_info(info);
> > + }
> > + break;
> > +
> > + case SH_MOBILE_LCDC_EVENT_DISPLAY_MODE:
> > + /* Validate a proposed new mode */
> > + var->bits_per_pixel = info->var.bits_per_pixel;
> > + ret = info->fbops->fb_check_var(var, info);
>
> You can call sh_mobile_lcdc_check_var() directly here too, right?
This was a way to avoid a forward declaration. You would prefer a forward
declaration, right ? :-) I think you would be right.
> > + break;
> > + }
> > +
> > + return ret;
> > +}
> > +
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH 27/57] fbdev: sh_mobile_lcdc: Pass a video mode to the
From: Guennadi Liakhovetski @ 2011-12-16 10:33 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1323784972-24205-28-git-send-email-laurent.pinchart@ideasonboard.com>
On Fri, 16 Dec 2011, Laurent Pinchart wrote:
> Hi Guennadi,
>
> On Thursday 15 December 2011 20:01:54 Guennadi Liakhovetski wrote:
> > On Tue, 13 Dec 2011, Laurent Pinchart wrote:
> > > Pass pointers to struct fb_videomode and struct fb_monspecs instead of
> > > struct fb_var_screeninfo to the notify callback.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >
> > > drivers/video/sh_mobile_hdmi.c | 59
> > > +++++++++++++++++-------------------- drivers/video/sh_mobile_lcdcfb.c
> > > | 40 ++++++++++++++----------- drivers/video/sh_mobile_lcdcfb.h |
> > > 3 +-
> > > 3 files changed, 51 insertions(+), 51 deletions(-)
> >
> > [snip]
> >
> > > diff --git a/drivers/video/sh_mobile_lcdcfb.c
> > > b/drivers/video/sh_mobile_lcdcfb.c index 21e5f10..4ec216e 100644
> > > --- a/drivers/video/sh_mobile_lcdcfb.c
> > > +++ b/drivers/video/sh_mobile_lcdcfb.c
> >
> > [ditto]
> >
> > > @@ -433,12 +432,17 @@ static int sh_mobile_lcdc_display_notify(struct
> > > sh_mobile_lcdc_chan *ch,
> > >
> > > }
> > > break;
> > >
> > > - case SH_MOBILE_LCDC_EVENT_DISPLAY_MODE:
> > > + case SH_MOBILE_LCDC_EVENT_DISPLAY_MODE: {
> > > + struct fb_var_screeninfo var;
> > > +
> > >
> > > /* Validate a proposed new mode */
> > >
> > > - var->bits_per_pixel = info->var.bits_per_pixel;
> > > - ret = info->fbops->fb_check_var(var, info);
> > > + fb_videomode_to_var(&var, mode);
> > > + var.bits_per_pixel = info->var.bits_per_pixel;
> > > + var.grayscale = info->var.grayscale;
> > > + ret = info->fbops->fb_check_var(&var, info);
> > >
> > > break;
> > >
> > > }
> > >
> > > + }
> >
> > nitpick - please, realign:-)
>
> It's common in driver code not to increase the identation level twice in a
> case statement if braces are needed. However, those "braced" statements are
> usually not the last ones in the switch. In this particular case this creates
> an alignment issue at the end, as your correctly pointed out. However, I'd
> like to avoid increasing the indentation of the whole block. Increasing the
> indentation of the closing brace only also causes an alignment issue. I can
> add a default case that returns an error to fix this, but that's adding code
> to fix a cosmetic problem :-) What's your preference.
Personally, I don't like just using braces without a related statement
like "if," "do," etc. In such "case" cases I usually avoid adding own
braces and just put any declarations, that are needed there in the
enclosing block, e.g., the function. If you really don't want either, I
would probably prefer a style like
switch (x) {
case X:
{
int y;
my_statement(y);
break;
}
}
You could also do
switch (x) {
case X:
do {
int y;
my_statement(y);
break;
} while (0);
}
So, choose your poison:-)
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply
* Re: Should I use FBINFO_VIRTFB?
From: Geert Uytterhoeven @ 2011-12-16 11:53 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <4EEA5DA7.3010406@freescale.com>
On Thu, Dec 15, 2011 at 23:10, Timur Tabi <timur@freescale.com> wrote:
> Konrad Rzeszutek Wilk wrote:
>>> > #define FBINFO_PARTIAL_PAN_OK 0x0040 /* otw use pan only for double-buffering */
>>> > #define FBINFO_READS_FAST 0x0080 /* soft-copy faster than rendering */
>
>> Not really. They have a different function, but you are better of looking in the code
>> to see how they are used.
> I can't parse this, and I can't figure out if my driver is better off with or without FBINFO_PARTIAL_PAN_OK.
This depends on which of these two is fastest for scrolling the console:
- panning the virtual screen and redrawing the missing part: set
FBINFO_PARTIAL_PAN_OK
- copying or redrawing the screen: don't set FBINFO_PARTIAL_PAN_OK
> I have the same problem with FBINFO_READS_FAST.
You should set this flag if reading from frame buffer memory is a fast
operation.
On many graphics devices, reading from frame buffer memory is much slower
than writing. As you use system RAM, you probably want to set it.
If this flag is set, scrolling is implemented by copying memory around.
If not set, scrolling is implemented by redrawing the whole screen.
A simple way to find the optimal settings of both flags (all 4 combinations) is
running "clear; time cat big_text_file" and comparing the timing results
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
^ permalink raw reply
* Re: Should I use FBINFO_VIRTFB?
From: Timur Tabi @ 2011-12-16 18:18 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <4EEA5DA7.3010406@freescale.com>
Geert Uytterhoeven wrote:
> This depends on which of these two is fastest for scrolling the console:
> - panning the virtual screen and redrawing the missing part: set
> FBINFO_PARTIAL_PAN_OK
> - copying or redrawing the screen: don't set FBINFO_PARTIAL_PAN_OK
I'm a little confused about panning support in my driver (which I didn't write -- I'm just cleaning it up). There is .fb_pan_display function, but it only gets called early in the boot process, and only with xoffset=0 and yoffset=0. After that, it never seems to get called again. Under what circumstances is panning really used?
>
>> I have the same problem with FBINFO_READS_FAST.
>
> You should set this flag if reading from frame buffer memory is a fast
> operation.
> On many graphics devices, reading from frame buffer memory is much slower
> than writing. As you use system RAM, you probably want to set it.
I see a lot of drivers that set FBINFO_VIRTFB but don't set FBINFO_READS_FAST. Why?
> If this flag is set, scrolling is implemented by copying memory around.
> If not set, scrolling is implemented by redrawing the whole screen.
>
> A simple way to find the optimal settings of both flags (all 4 combinations) is
> running "clear; time cat big_text_file" and comparing the timing results
That's the funny thing -- I've been trying various combinations of FBINFO_VIRTFB, FBINFO_PARTIAL_PAN_OK, and FBINFO_READS_FAST, and I always get the same timing result. It seems to have no affect on performance.
--
Timur Tabi
Linux kernel developer at Freescale
^ permalink raw reply
* Re: [PATCH v4 3/5] powerpc/mpc5121: shared DIU framebuffer support
From: Timur Tabi @ 2011-12-16 18:24 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1279893639-24333-4-git-send-email-agust@denx.de>
Anatolij Gustschin wrote:
> We cannot guarantee that the DIU is not in these modes. Even
> if U-Boot didn't set these modes there is still a possibility
> that such mode is configured. E.g. I've seen U-Boot binary
> standalone applications for other display controllers initializing
> the display controller.
True, but modes 2 and 3 don't make any sense. 2 is just a color bar, and 3 writes back to memory (which means you need to have a reserved DMA buffer otherwise you'll crash). Have you really seen Linux boot with the DIU in either of these modes?
> But you are right. With this snippet, if the DIU is already
> disabled, there will be not needed mode register access. So
> the code should better look like:
>
> diu_mode = in_be32(&dr.diu_reg->diu_mode);
> if (diu_mode && diu_mode != MFB_MODE1)
> out_be32(&dr.diu_reg->diu_mode, 0);
Ok. I'm planning on cleaning up the driver so that it does not initialize the DIU until the .open call, and once that's done, I should be able to remove most of the code from your patch.
--
Timur Tabi
Linux kernel developer at Freescale
^ permalink raw reply
* Re: Should I use FBINFO_VIRTFB?
From: Geert Uytterhoeven @ 2011-12-16 20:16 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <4EEA5DA7.3010406@freescale.com>
On Fri, Dec 16, 2011 at 19:18, Timur Tabi <timur@freescale.com> wrote:
> Geert Uytterhoeven wrote:
>> This depends on which of these two is fastest for scrolling the console:
>> - panning the virtual screen and redrawing the missing part: set
>> FBINFO_PARTIAL_PAN_OK
>> - copying or redrawing the screen: don't set FBINFO_PARTIAL_PAN_OK
>
> I'm a little confused about panning support in my driver (which I didn't write -- I'm just cleaning it up). There is .fb_pan_display function, but it only gets called early in the boot process, and only with xoffset=0 and yoffset=0. After that, it never seems to get called again. Under what circumstances is panning really used?
Is yres_virtual larger than yres? Is the font size a multiple of ypanstep?
If not, panning is disabled.
>>> I have the same problem with FBINFO_READS_FAST.
>>
>> You should set this flag if reading from frame buffer memory is a fast
>> operation.
>> On many graphics devices, reading from frame buffer memory is much slower
>> than writing. As you use system RAM, you probably want to set it.
>
> I see a lot of drivers that set FBINFO_VIRTFB but don't set FBINFO_READS_FAST. Why?
>
>> If this flag is set, scrolling is implemented by copying memory around.
>> If not set, scrolling is implemented by redrawing the whole screen.
>>
>> A simple way to find the optimal settings of both flags (all 4 combinations) is
>> running "clear; time cat big_text_file" and comparing the timing results
>
> That's the funny thing -- I've been trying various combinations of FBINFO_VIRTFB, FBINFO_PARTIAL_PAN_OK, and FBINFO_READS_FAST, and I always get the same timing result. It seems to have no affect on performance.
If panning is disabled, FBINFO_PARTIAL_PAN_OK doesn't do anything.
For FBINFO_READS_FAST, I can imagine that with a fast CPU and a good
writeback cache, redrawing the screen is as fast as copying data around.
It just means your hardware is well balanced ;-)
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
^ permalink raw reply
* "ls > /dev/fb0" generates weird ioctls if panning is set
From: Timur Tabi @ 2011-12-16 21:49 UTC (permalink / raw)
To: linux-fbdev
If I do this:
echo "0,100" > /sys/devices/soc.0/fffe10000.display/graphics/fb0/pan
then whenever I do this:
ls > /dev/fb0
my driver receives the following three ioctls:
graphics fb0: unknown ioctl command (0x402C7413)
graphics fb0: dir=2 type='t' (74) nr\x19 sizeD
graphics fb0: unknown ioctl command (0x40087468)
graphics fb0: dir=2 type='t' (74) nr\x104 size=8
graphics fb0: unknown ioctl command (0x402C7413)
graphics fb0: dir=2 type='t' (74) nr\x19 sizeD
This is on PowerPC, where dir=2 means read.
The "echo" and "cat" commands don't generate these ioctls, so there's something special about the "ls" command. Does anyone know what's going on? ioctl-number.txt lists these ioctls for type 't':
't' 00-7F linux/if_ppp.h
't' 80-8F linux/isdn_ppp.h
't' 90 linux/toshiba.h
--
Timur Tabi
Linux kernel developer at Freescale
^ permalink raw reply
* Re: "ls > /dev/fb0" generates weird ioctls if panning is set
From: Timur Tabi @ 2011-12-16 22:31 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <4EEBBCEA.9020104@freescale.com>
Timur Tabi wrote:
> graphics fb0: unknown ioctl command (0x402C7413)
> graphics fb0: dir=2 type='t' (74) nr\x19 sizeD
> graphics fb0: unknown ioctl command (0x40087468)
> graphics fb0: dir=2 type='t' (74) nr\x104 size=8
> graphics fb0: unknown ioctl command (0x402C7413)
> graphics fb0: dir=2 type='t' (74) nr\x19 sizeD
I figured it out. These ioctls are:
#define TCGETS _IOR('t', 19, struct termios)
#define TIOCGWINSZ _IOR('t', 104, struct winsize)
I'm guessing that since I redirect stdout to /dev/fb0, Linux is treating /dev/fb0 as a terminal, and therefore it's receiving terminal ioctls.
Is this something that fbdev should be supporting, or am I wrong to use /dev/fb0 as a terminal?
--
Timur Tabi
Linux kernel developer at Freescale
^ permalink raw reply
* Re: [PATCH v4 3/5] powerpc/mpc5121: shared DIU framebuffer support
From: Anatolij Gustschin @ 2011-12-16 22:59 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1279893639-24333-4-git-send-email-agust@denx.de>
On Fri, 16 Dec 2011 12:24:49 -0600
Timur Tabi <timur@freescale.com> wrote:
> Anatolij Gustschin wrote:
>
> > We cannot guarantee that the DIU is not in these modes. Even
> > if U-Boot didn't set these modes there is still a possibility
> > that such mode is configured. E.g. I've seen U-Boot binary
> > standalone applications for other display controllers initializing
> > the display controller.
>
> True, but modes 2 and 3 don't make any sense. 2 is just a color bar,
> and 3 writes back to memory (which means you need to have a reserved
> DMA buffer otherwise you'll crash). Have you really seen Linux boot
> with the DIU in either of these modes?
No, but it doesn't mean that it is not possible.
> > But you are right. With this snippet, if the DIU is already
> > disabled, there will be not needed mode register access. So
> > the code should better look like:
> >
> > diu_mode = in_be32(&dr.diu_reg->diu_mode);
> > if (diu_mode && diu_mode != MFB_MODE1)
> > out_be32(&dr.diu_reg->diu_mode, 0);
>
> Ok. I'm planning on cleaning up the driver so that it does not
> initialize the DIU until the .open call, and once that's done,
> I should be able to remove most of the code from your patch.
Will DIU splash screen functionality be preserved?
Anatolij
^ permalink raw reply
* Re: [PATCH v4 3/5] powerpc/mpc5121: shared DIU framebuffer support
From: Timur Tabi @ 2011-12-16 23:00 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1279893639-24333-4-git-send-email-agust@denx.de>
Anatolij Gustschin wrote:
> Will DIU splash screen functionality be preserved?
That's the idea. The root cause is that the drive initializes the DIU during the probe, which is too early. It shouldn't touch the hardware until the open. At that point, Linux (X, console, whatever) wants to draw something.
--
Timur Tabi
Linux kernel developer at Freescale
^ permalink raw reply
* Re: "ls > /dev/fb0" generates weird ioctls if panning is set
From: Anatolij Gustschin @ 2011-12-16 23:03 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <4EEBBCEA.9020104@freescale.com>
On Fri, 16 Dec 2011 16:31:22 -0600
Timur Tabi <timur@freescale.com> wrote:
> Timur Tabi wrote:
>
> > graphics fb0: unknown ioctl command (0x402C7413)
> > graphics fb0: dir=2 type='t' (74) nr\x19 sizeD
> > graphics fb0: unknown ioctl command (0x40087468)
> > graphics fb0: dir=2 type='t' (74) nr\x104 size=8
> > graphics fb0: unknown ioctl command (0x402C7413)
> > graphics fb0: dir=2 type='t' (74) nr\x19 sizeD
>
> I figured it out. These ioctls are:
>
> #define TCGETS _IOR('t', 19, struct termios)
> #define TIOCGWINSZ _IOR('t', 104, struct winsize)
>
> I'm guessing that since I redirect stdout to /dev/fb0, Linux is
> treating /dev/fb0 as a terminal, and therefore it's receiving
> terminal ioctls.
>
> Is this something that fbdev should be supporting, or am I wrong
> to use /dev/fb0 as a terminal?
No. Do not redirect to /dev/fb0, use /dev/tty1 instead.
Anatolij
^ permalink raw reply
* Re: [PATCH v4] Resurrect Intel740 driver: i740fb
From: Andrew Morton @ 2011-12-17 1:12 UTC (permalink / raw)
To: Ondrej Zary
Cc: linux-fbdev, Florian Tobias Schandinat, Paul Mundt,
Kernel development list
In-Reply-To: <201112080024.24383.linux@rainbow-software.org>
On Thu, 8 Dec 2011 00:24:18 +0100
Ondrej Zary <linux@rainbow-software.org> wrote:
> This is a resurrection of an old (like 2.4.19) out-of-tree driver for
> Intel740 graphics cards and modify it for recent kernels. The old driver is
> located at: http://sourceforge.net/projects/i740fbdev/files/
>
> This is a new driver based on skeletonfb, using most of the low level HW code
> from the old driver. The DDC code is completely new.
>
> The driver was tested on two 8MB cards: Protac AG240D and Diamond Stealth II
> G460.
>
> Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
The original driver appears to have been written by Andrey Ulanov. It
would be nice to mention that in the changelog and it is desirable that
you seek his Signed-off-by:, please.
> Changes in v4:
> - shortened many lines to fit 80-char limit
> - removed useless inb_p and outb_p functions
> - converted DDC (and some other) code to i740outreg_mask
> - removed useless wm initialization in i740_calc_fifo()
> - ALIGN macro is used instead of manual alignment
> - removed inactive code
> - simplified i740_setcolreg()
> - info->var used for panning
> - fixed out-of-video-memory checking
It still generates a large number of checkpatch errors. If you've
reviewed those and made the decision to ignore them then OK(ish).
Otherwise, please check.
>
> ...
>
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/string.h>
> +#include <linux/mm.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/fb.h>
> +#include <linux/init.h>
> +#include <linux/pci.h>
> +#include <linux/pci_ids.h>
> +#include <linux/i2c.h>
> +#include <linux/i2c-algo-bit.h>
Should there have been a dependency on i2c in the Kconfig entry? If
not, has the driver been tested with is2 disabled?
> +#include <linux/console.h>
> +#include <video/vga.h>
> +
>
> ...
>
> +#define I740_RFREQ (1e6)
> +#define TARGET_MAX_N 30
> +
> +#define I740_FFIX 8
> +#define I740_REF_FREQ (u32) (66.66666666667 * (1 << I740_FFIX) + 0.5)
> +#define I740_MAX_VCO_FREQ (u32)(450.00000000000 * (1 << I740_FFIX) + 0.5)
There should be no floating point in the kernel, normally. Given the
casts, these will obviously not get any further than the compiler. But
if these could be redone to avoid the float, that would be nice.
> +#define I740_CALC_VCLKfix(m, n, p, d) ((((((m) * I740_REF_FREQ * (4 << ((d) << 1)))) / (n)) + ((1 << (p)) / 2)) / (1 << (p)))
This macro should be dragged out and shot. And it will cause bugs if
invoked using an expression with side-effects. There's no need to do
this to ourselves - please turn it into a nice, lower-case-named C
function with proper identifiers, code comments, etc.
> +static void i740_calc_vclk(u32 freq_hz, struct i740fb_par *par)
> +{
> + u32 freq = freq_hz / (u32)(1e6 / I740_RFREQ);
> + const u32 err_max > + freq / (u32)(I740_RFREQ / 0.005 / (1 << I740_FFIX) + 0.5);
> + const u32 err_target > + freq / (u32)(I740_RFREQ / 0.001 / (1 << I740_FFIX) + 0.5);
> + u32 err_best = (u32)(512.0 * (1 << I740_FFIX));
> + u32 f_err, f_vco;
> + int m_best = 0, n_best = 0, p_best = 0, d_best = 0;
> + int m, n, i;
> +
> + /* find log2(MAX_VCO_FREQ/f_target) */
> + for (i = 0; i < 16; i++)
> + if ((I740_MAX_VCO_FREQ) / (1 << i) < freq / (u32)(I740_RFREQ / (1 << I740_FFIX)))
Use the existing ilog2()?
> + break;
> + i--;
> + p_best = i;
> +
> + d_best = 0;
> + f_vco = (freq * (1 << p_best)) / (u32)(I740_RFREQ / (1 << I740_FFIX));
> + freq = freq / (u32)(I740_RFREQ / (1 << I740_FFIX));
> +
> + n = 2;
> + do {
> + n++;
> + m = ((f_vco * n) / I740_REF_FREQ + 2) / 4;
> +
> + if (m < 3)
> + m = 3;
> +
> + {
> + u32 f_out = I740_CALC_VCLKfix(m, n, p_best, d_best);
> +
> + f_err = (freq - f_out);
> +
> + if (abs(f_err) < err_max) {
> + m_best = m;
> + n_best = n;
> + err_best = f_err;
> + }
> + }
> + } while ((abs(f_err) >= err_target) &&
> + ((n <= TARGET_MAX_N) || (abs(err_best) > err_max)));
> +
> + if (abs(f_err) < err_target) {
> + m_best = m;
> + n_best = n;
> + }
> +
> + par->video_clk2_m = (m_best-2) & 0xFF;
> + par->video_clk2_n = (n_best-2) & 0xFF;
> + par->video_clk2_mn_msbs = ((((n_best-2) >> 4) & VCO_N_MSBS)
> + | (((m_best-2) >> 8) & VCO_M_MSBS));
> + par->video_clk2_div_sel > + ((p_best << 4) | (d_best ? 4 : 0) | REF_DIV_1);
> +}
> +
>
> ...
>
> +#ifndef MODULE
> +static int __init i740fb_setup(char *options)
> +{
> + char *opt;
> +
> + if (!options || !*options)
> + return 0;
> +
> + while ((opt = strsep(&options, ",")) != NULL) {
> + if (!*opt)
> + continue;
> +#ifdef CONFIG_MTRR
> + else if (!strncmp(opt, "mtrr:", 5))
> + mtrr = simple_strtoul(opt + 5, NULL, 0);
> +#endif
> + else
> + mode_option = opt;
> + }
> +
> + return 0;
> +}
> +#endif
> +
> +int __init i740fb_init(void)
> +{
> +#ifndef MODULE
> + char *option = NULL;
> +
> + if (fb_get_options("i740fb", &option))
> + return -ENODEV;
> + i740fb_setup(option);
> +#endif
> +
> + return pci_register_driver(&i740fb_driver);
> +}
hm, the "ifdef MODULE" stuff looks old-fashioned, but quite a few fbdev
drivers appear to be doing it.
>
> ...
>
Should we also be adding a documentation file for thei driver? Tell
people what the various module options do?
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox