Re: [PATCH] Preferred resolution detection for VBE

[Vladimir 'φ-coder/phcoder' Serbinenko]

> +/* Call VESA BIOS 0x4f11 to get flat panel information, return status.  */
> +grub_vbe_status_t
> +grub_vbe_bios_get_flat_panel_info (struct grub_vbe_flat_panel_info 
> *flat_panel_info)
>   
I would prefer all new grub_vbe_bios_* to be static and not global since
they are adapter-specific and also require special considerations when
used (pointer has to be to lowmem).
> +static grub_err_t
> +grub_vbe_get_preferred_mode (unsigned int *width, unsigned int *height)
> +{
> +  grub_vbe_status_t status;
> +  grub_uint8_t ddc_level;
> +  struct grub_video_edid_info edid_info;
> +  struct grub_vbe_flat_panel_info flat_panel_info;
>   
You implicitly use with these 2 variables that the stack is in lowmem.
It's so now but may change in the future if we need to increase stack
size or move it out of current region for any reason. Could you use
GRUB_MEMORY_MACHINE_SCRATCH_ADDR instead?
> +static grub_err_t
> +grub_video_vbe_get_edid (struct grub_video_edid_info *edid_info)
> +{
> +  if (grub_vbe_bios_read_edid (edid_info) != GRUB_VBE_STATUS_OK)
> +    return grub_error (GRUB_ERR_BAD_DEVICE, "EDID information not 
> available");
> +
>   
get_edid shouldn't require any particular placement of edid_info but
currently it works only if edid_info is in lowmem.
> +
> +  if (checksum != 0)
> +    return grub_error (GRUB_ERR_BAD_DEVICE,
> +                    "invalid EDID checksum %d", checksum);
> +
> +  grub_errno = GRUB_ERR_NONE;
>   
Why did you need to reset grub_errno here? If it is really needed we
probably have a problem somewhere else
> +  return grub_errno;
> +}
> +
> +grub_err_t
> +grub_video_get_edid (struct grub_video_edid_info *edid_info)
> +{
> +  if (! grub_video_adapter_active)
> +    return grub_error (GRUB_ERR_BAD_DEVICE, "no video mode activated");
> +
> +  if (! grub_video_adapter_active->get_edid)
> +    return grub_error (GRUB_ERR_BAD_DEVICE,
> +                    "EDID information unavailable for this video mode");
> +
> +  if (grub_video_adapter_active->get_edid (edid_info) != GRUB_ERR_NONE)
> +    return grub_errno;
> +  if (grub_video_edid_checksum (edid_info) != GRUB_ERR_NONE)
> +    return grub_errno;
>   
In such cases the usual construction is:
err = ....;
if (err)
  return err;

> === modified file 'include/grub/i386/pc/vbe.h'
> --- include/grub/i386/pc/vbe.h        2010-09-15 22:37:30 +0000
> +++ include/grub/i386/pc/vbe.h        2010-12-14 17:03:13 +0000
> @@ -19,6 +19,8 @@
>  #ifndef GRUB_VBE_MACHINE_HEADER
>  #define GRUB_VBE_MACHINE_HEADER      1
>  
> +#include <grub/video.h>
> +
>  /* Default video mode to be used.  */
>  #define GRUB_VBE_DEFAULT_VIDEO_MODE     0x101
>  
> @@ -169,6 +171,21 @@ struct grub_vbe_palette_data
>    grub_uint8_t alignment;
>  } __attribute__ ((packed));
>  
> +struct grub_vbe_flat_panel_info
> +{
> +  grub_uint16_t horizontal_size;
> +  grub_uint16_t vertical_size;
> +  grub_uint16_t panel_type;
> +  grub_uint8_t red_bpp;
> +  grub_uint8_t green_bpp;
> +  grub_uint8_t blue_bpp;
> +  grub_uint8_t reserved_bpp;
> +  grub_uint32_t reserved_offscreen_mem_size;
> +  grub_vbe_farptr_t reserved_offscreen_mem_ptr;
> +
> +  grub_uint8_t reserved[14];
> +} __attribute__ ((packed));
> +
>   
Is this structure VBE specific? If it's not it should be moved to
generic headers.
On IRC I raised concern about possible endiannness issue but this patch
isn't actually affected: it seems to use only 8-bit fields.

-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko

signature.asc
Description: OpenPGP digital signature