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

[Javier Martinez Canillas <javierm@redhat.com>]

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.

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

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

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat