Re: [PATCH 1/8] video: Add helpers for decoding screen_info

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 3961 bytes --]

Hi

Am 29.01.24 um 11:41 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <tzimmermann@suse.de> writes:
> 
> Hello Thomas,
> 
>> The plain values as stored in struct screen_info need to be decoded
>> before being used. Add helpers that decode the type of video output
>> and the framebuffer I/O aperture.
>>
>> Old or non-x86 systems may not set the type of video directly, but
>> only indicate the presence by storing 0x01 in orig_video_isVGA. The
>> decoding logic in screen_info_video_type() takes this into account.
> 
> I always disliked how the orig_video_isVGA variable lost its meaning.
> 
>> It then follows similar code in vgacon's vgacon_startup() to detect
>> the video type from the given values.
>>
>> A call to screen_info_resources() returns all known resources of the
>> given screen_info. The resources' values have been taken from existing
>> code in vgacon and vga16fb. These drivers can later be converted to
>> use the new interfaces.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
> 
> Thanks for doing this! It's quite useful to have these helpers, since as
> you mention the screen_info data decoding is complex and the variables
> used to store the video type and modes are confusing / misleading.
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> 
> I just have a few comments below:
> 
>> +static inline bool __screen_info_has_ega_gfx(unsigned int mode)
>> +{
>> +	switch (mode) {
>> +	case 0x0d:	/* 320x200-4 */
>> +	case 0x0e:	/* 640x200-4 */
>> +	case 0x0f:	/* 640x350-1 */
>> +	case 0x10:	/* 640x350-4 */
> 
> I wonder if makes sense to define some constant macros for these modes? I
> know that check_mode_supported() in drivers/video/fbdev/vga16fb.c also use
> magic numbers but I believe that it could ease readability.

They are known by their numbers, but have no names. There's also no 
common practice or precedence I'm aware of.

OTOH, drivers like vga16fb should no longer have to test magic numbers 
at all. They bind to a certain type of device, such as ega-txt and 
vga-gfx, which implies a correctly set mode.

> 
>> +		return true;
>> +	default:
>> +		return false;
>> +	}
>> +}
>> +
>> +static inline bool __screen_info_has_vga_gfx(unsigned int mode)
>> +{
>> +	switch (mode) {
>> +	case 0x10:	/* 640x480-1 */
>> +	case 0x12:	/* 640x480-4 */
>> +	case 0x13:	/* 320-200-8 */
>> +	case 0x6a:	/* 800x600-4 (VESA) */
>> +		return true;
> 
> And same for these.
> 
> It can be a follow-up patch though.
> 
> [...]
> 
>> +int screen_info_resources(const struct screen_info *si, struct resource *r, size_t num)
>> +{
>> +	struct resource *pos = r;
>> +	unsigned int type = screen_info_video_type(si);
>> +	u64 base, size;
>> +
>> +	switch (type) {
>> +	case VIDEO_TYPE_MDA:
>> +		if (num > 0)
>> +			resource_init_io_named(pos++, 0x3b0, 12, "mda");
> 
> I see that drivers/video/fbdev/i740_reg.h has a #define MDA_BASE
> 0x3B0. Maybe move to a header in include/video along with the other
> constants mentioned above ?

That could go into <video/vga.h>. MDA_BASE (and CGA_BASE) from the same 
file are unused though.

Best regards
Thomas

> 
>> +		if (num > 1)
>> +			resource_init_io_named(pos++, 0x3bf, 0x01, "mda");
>> +		if (num > 2)
>> +			resource_init_mem_named(pos++, 0xb0000, 0x2000, "mda");
> 
> Same for these start addresses. I see that are also used by mdacon_startup()
> in drivers/video/console/mdacon.c, so some constants defined somewhere might
> be useful for that console driver too.
> 
> The comment also applies to all the other start addresses, since I see
> that those are used by other drivers (i810, vgacon, etc).
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]