[haiku-commits] Re: r41334 - haiku/trunk/src/add-ons/accelerants/radeon_hd

  • From: "Axel Dörfler" <axeld@xxxxxxxxxxxxxxxx>
  • To: haiku-commits@xxxxxxxxxxxxx
  • Date: Fri, 6 May 2011 09:34:17 +0200 (MEST)

kallisti5@xxxxxxxxxxx wrote:
> [...] change gDisplayMode to pointer for now; [...]

Why?
And looking at the code, I can only ask if you ever tested this?

> -static display_mode gDisplayMode;
> +static display_mode *gDisplayMode;

static globals should have the 's' instead of the 'g' prefix.

>       /* Populate modeline with temporary example */
[...]
> -     gDisplayMode.space = B_RGB32_LITTLE;    // Pixel configuration

You never allocate memory for gDisplayMode; it just points to NULL.

> @@ -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 statement 
would have been:
   *_currentMode = *gDisplayMode;

> +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.

Bye,
   Axel.


Other related posts: