[haiku-commits] Re: r41334 - haiku/trunk/src/add-ons/accelerants/radeon_hd
- From: Alex von Gluck <kallisti5@xxxxxxxxxxx>
- To: haiku-commits@xxxxxxxxxxxxx
- Date: Fri, 6 May 2011 07:39:12 -0500
On Fri, 6 May 2011 09:34:17 +0200 (MEST)
"Axel Dörfler" <axeld@xxxxxxxxxxxxxxxx> wrote:
> kallisti5@xxxxxxxxxxx wrote:
> > [...] change gDisplayMode to pointer for now; [...]
>
> Why?
> And looking at the code, I can only ask if you ever tested this?
Nope, once I got home last night I undid the change after reviewing
my previous work and determining it was a bad call on my part. (see
r41341)
> > -static display_mode gDisplayMode;
> > +static display_mode *gDisplayMode;
>
> static globals should have the 's' instead of the 'g' prefix.
Can fix, one of those "it already there so I missed the typos".
> > /* Populate modeline with temporary example */
> [...]
> > - gDisplayMode.space = B_RGB32_LITTLE; // Pixel
> > configuration
>
> You never allocate memory for gDisplayMode; it just points to NULL.
Already fixed in r41341
> > @@ -350,9 +350,9 @@
> > status_t
> > radeon_get_display_mode(display_mode *_currentMode)
> > {
> > - TRACE(("radeon_get_display_mode()\n"));
> > + TRACE("%s\n", __func__);
> >
> > - *_currentMode = gDisplayMode;
> > + _currentMode = gDisplayMode;
>
> How is this supposed to work? You need to copy the mode, not assign
> it to a local variable that is never used for anything anymore. The
> correct statment would have been: *_currentMode = *gDisplayMode;
Already fixed in r41341
> > +status_t
> > +mode_sanity_check(display_mode *mode)
> > +{
> > + if (mode->timing.h_display <= 0
> > + || mode->timing.h_sync_start <= 0
>
> Is it really <= for sync_start?
> Aren't these unsigned values, anyway?
> And in any case, a sanity check might also want to take card
> parameters, and the monitor timings into account.
Nope:
http://dev.haiku-os.org/browser/haiku/trunk/headers/os/add-ons/graphics/Accelerant.h#L95
You're right, these are unsigned int's. My original (and still my
plan) is to do a few bitwise checks on the timing flags to ensure
they are within sane ranges, etc. I am also going to compare monitor
mode line timings and see if the mode lines are possible
values (like you said). I just ran out of time last night.
Also, for what it's worth... r41341 boot stably on my Radeon HD 5830.
http://pastebin.com/D02A4a8a
Too bad the mode line is crap and the image garbled though :D
-- Alex
Other related posts:
