xxx_check_var

xxx_check_var

[Paul Mackerras]

When I look at atyfb_check_var or aty128fb_check_var, I see that they
will alter the contents of *info->par.  Isn't this a bad thing?  My
understanding was that after calling check_var, you don't necessarily
call set_par next (particularly if check_var returned an error).
Also I notice that atyfb_set_par and aty128fb_set_par don't look at
info->var, they simply set the hardware state based on the contents of
*info->par.

Looking at skeletonfb.c, it seems that this is the wrong behaviour.  I
had fixed the aty128fb.c driver in the linuxppc-2.5 tree.  James, if
you let me know whether the current behaviour is wrong or not, I'll
fix them and send you the patch.

Paul.

Re: xxx_check_var

[Benjamin Herrenschmidt]

On Tue, 2002-12-10 at 23:40, Paul Mackerras wrote:
> When I look at atyfb_check_var or aty128fb_check_var, I see that they
> will alter the contents of *info->par.  Isn't this a bad thing?  My

Yes, this wrong, and afaik, it's your original port to 2.5 that did that
;)

> understanding was that after calling check_var, you don't necessarily
> call set_par next (particularly if check_var returned an error).
> Also I notice that atyfb_set_par and aty128fb_set_par don't look at
> info->var, they simply set the hardware state based on the contents of
> *info->par.

Which is wrong too indeed

> Looking at skeletonfb.c, it seems that this is the wrong behaviour.  I
> had fixed the aty128fb.c driver in the linuxppc-2.5 tree.  James, if
> you let me know whether the current behaviour is wrong or not, I'll
> fix them and send you the patch.

I _think_ my radeonfb (in linuxppc-2.5) is right in this regard too.
Look at the initialization too, iirc, you had some non necessary stuff
in there (calling gen_set_disp, gen_set_var is plenty enough).

Ben.

 
> Paul.
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: xxx_check_var

[James Simmons]

> > When I look at atyfb_check_var or aty128fb_check_var, I see that they
> > will alter the contents of *info->par.  Isn't this a bad thing?  My
>
> Yes, this wrong, and afaik, it's your original port to 2.5 that did that
> ;)

Yeap. The idea of check_var is to validate a mode. Note modedb uses just
check_var. It is okay to READ the values in your par. You shouldn't alter
the values in par.

> > understanding was that after calling check_var, you don't necessarily
> > call set_par next (particularly if check_var returned an error).

Correct. Check_var is always called. Now if you wanted to just test a mode
via userland with the FB_ACTIVATE_TEST flag then only fb_check_var is
called. If you pass in FB_ACTIVATE_NOW then both fb_check_var and
fb_set_par will be called. Fb_set_par actually sets the hardware state.

> > Also I notice that atyfb_set_par and aty128fb_set_par don't look at
> > info->var, they simply set the hardware state based on the contents of
> > *info->par.
>
> Which is wrong too indeed

Actually you can look at info->var. Info->var has been validated so it can
be trusted. You don't need to stuff everything into par. You DO need to
change info->fix if the hardware state has changed.

> > Looking at skeletonfb.c, it seems that this is the wrong behaviour.  I
> > had fixed the aty128fb.c driver in the linuxppc-2.5 tree.  James, if
> > you let me know whether the current behaviour is wrong or not, I'll
> > fix them and send you the patch.

I hope me input helped.

BTW docs are on the way. I will work with Steven Luther on this the next
couple of days.

> I _think_ my radeonfb (in linuxppc-2.5) is right in this regard too.
> Look at the initialization too, iirc, you had some non necessary stuff
> in there (calling gen_set_disp, gen_set_var is plenty enough).

You go the logic down.

P.S

 For the pmu_sleep_notifier can you pass in a specific struct fb_info or
do you need to make a list of all of them?

Re: xxx_check_var

[Benjamin Herrenschmidt]

On Wed, 2002-12-11 at 07:32, James Simmons wrote:
> 
>  For the pmu_sleep_notifier can you pass in a specific struct fb_info or
> do you need to make a list of all of them?

So far, I need to make that list. But sooner or later, those notifiers
will go away and I'll use the PCI and/or new driver model PM features.

Ben.

Re: [Linux-fbdev-devel] Re: xxx_check_var

[Geert Uytterhoeven]

On Tue, 10 Dec 2002, James Simmons wrote:
> > > When I look at atyfb_check_var or aty128fb_check_var, I see that they
> > > will alter the contents of *info->par.  Isn't this a bad thing?  My
> >
> > Yes, this wrong, and afaik, it's your original port to 2.5 that did that
> > ;)
> 
> Yeap. The idea of check_var is to validate a mode. Note modedb uses just
> check_var. It is okay to READ the values in your par. You shouldn't alter
> the values in par.

Perhaps it makes sense to make the info parameter of fb_check_var() const to
prevent this from happening?

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

Re: [Linux-fbdev-devel] Re: xxx_check_var

[James Simmons]

> > > Yes, this wrong, and afaik, it's your original port to 2.5 that did that
> > > ;)
> > 
> > Yeap. The idea of check_var is to validate a mode. Note modedb uses just
> > check_var. It is okay to READ the values in your par. You shouldn't alter
> > the values in par.
> 
> Perhaps it makes sense to make the info parameter of fb_check_var() const to
> prevent this from happening?

Easy fix. I can whip up a quick patch for that.