From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with archive (Exim 4.43) id 1PU1nc-0001VQ-NA for mharc-grub-devel@gnu.org; Sat, 18 Dec 2010 13:46:32 -0500 Received: from [140.186.70.92] (port=38730 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PU1na-0001Uy-4O for grub-devel@gnu.org; Sat, 18 Dec 2010 13:46:30 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PU1nY-0001ek-Ka for grub-devel@gnu.org; Sat, 18 Dec 2010 13:46:30 -0500 Received: from mail-ww0-f49.google.com ([74.125.82.49]:47518) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PU1nY-0001ef-Aa for grub-devel@gnu.org; Sat, 18 Dec 2010 13:46:28 -0500 Received: by wwb17 with SMTP id 17so1738760wwb.30 for ; Sat, 18 Dec 2010 10:46:27 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:message-id:date:from :user-agent:mime-version:to:subject:references:in-reply-to :x-enigmail-version:content-type; bh=vlJ2+akd9EHZy/gzfbH/v2xkBXZm5cFbe8XyZwDUAE8=; b=tH+i7hw4HcnFUf1Fej78gUmKYuNzmx/VG7olG/vYXEfKs1syRKGP3OZFU9pm7rvc1O ilHX644rXbPnu0FQult3QvSYY1QxFCu8q8gi7dKfQK3qwg+AIit0280QLvDVrcHoOp49 HPllyMqe9bnf1K6wBjTd7q1BF6U6gjtVZfJgo= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=message-id:date:from:user-agent:mime-version:to:subject:references :in-reply-to:x-enigmail-version:content-type; b=OvCfRnwmKyLUYqPJHaaewnPpN0EKa5gxMb+UXICqWlFRFwKyKsOgk0sw162JCeThwK aRTUgHE2IOBKCHJHglyGy2FX/WPlQHd0TAzSZeiKLeuF0wmDbx3pKz5DGHYM8PeGwjXa XekC2/ayrWywoiPh8XznVkrkoX039ciOysIbA= Received: by 10.227.167.8 with SMTP id o8mr1435820wby.166.1292697987304; Sat, 18 Dec 2010 10:46:27 -0800 (PST) Received: from debian.bg45.phnet (229-110.62-81.cust.bluewin.ch [81.62.110.229]) by mx.google.com with ESMTPS id p4sm912807wer.5.2010.12.18.10.46.23 (version=TLSv1/SSLv3 cipher=RC4-MD5); Sat, 18 Dec 2010 10:46:26 -0800 (PST) Message-ID: <4D0D00F4.9060602@gmail.com> Date: Sat, 18 Dec 2010 19:44:04 +0100 From: =?UTF-8?B?VmxhZGltaXIgJ8+GLWNvZGVyL3BoY29kZXInIFNlcmJpbmVua28=?= User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.16) Gecko/20101211 Icedove/3.0.11 MIME-Version: 1.0 To: grub-devel@gnu.org References: <20101214163836.GR21862@riva.ucam.org> <20101214181320.GA27013@riva.ucam.org> <20101214190415.GU21862@riva.ucam.org> In-Reply-To: <20101214190415.GU21862@riva.ucam.org> X-Enigmail-Version: 1.0.1 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="------------enig21A838B01739AD307D3A3373" X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 2) Subject: Re: [PATCH] Preferred resolution detection for VBE X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: The development of GNU GRUB List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 18 Dec 2010 18:46:31 -0000 This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig21A838B01739AD307D3A3373 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable > +/* 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 *fl= at_panel_info) > =20 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; > =20 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) !=3D GRUB_VBE_STATUS_OK) > + return grub_error (GRUB_ERR_BAD_DEVICE, "EDID information not avai= lable"); > + > =20 get_edid shouldn't require any particular placement of edid_info but currently it works only if edid_info is in lowmem. > + > + if (checksum !=3D 0) > + return grub_error (GRUB_ERR_BAD_DEVICE, > + "invalid EDID checksum %d", checksum); > + > + grub_errno =3D GRUB_ERR_NONE; > =20 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) !=3D GRUB_ERR_NO= NE) > + return grub_errno; > + if (grub_video_edid_checksum (edid_info) !=3D GRUB_ERR_NONE) > + return grub_errno; > =20 In such cases the usual construction is: err =3D ....; if (err) return err; > =3D=3D=3D 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 > =20 > +#include > + > /* Default video mode to be used. */ > #define GRUB_VBE_DEFAULT_VIDEO_MODE 0x101 > =20 > @@ -169,6 +171,21 @@ struct grub_vbe_palette_data > grub_uint8_t alignment; > } __attribute__ ((packed)); > =20 > +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)); > + > =20 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. --=20 Regards Vladimir '=CF=86-coder/phcoder' Serbinenko --------------enig21A838B01739AD307D3A3373 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.10 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iF4EAREKAAYFAk0NAPQACgkQNak7dOguQglTjwEAwqhiaRnPJ5M2Pv97JPGE3wYD QmpzJfo9oFHIiHGNqPgA/i07jpW7xa6ntC5+Bxz7oSxlWIrERUFgdQ81T/zAuh/+ =9xZE -----END PGP SIGNATURE----- --------------enig21A838B01739AD307D3A3373--