[SeaBIOS] Re: [PATCH] vbe: implement function 09h (get/set palette data)

[Kevin O'Connor]

On Mon, Mar 11, 2024 at 06:08:22PM -0700, Daniel Verkamp wrote:
> On Mon, Mar 11, 2024 at 9:23 AM Kevin O'Connor  wrote:
> > On Sun, Mar 10, 2024 at 10:32:39PM -0700, Daniel Verkamp wrote:
> > > Since the VBE mode attributes indicate that all modes are not VGA
> > > compatible, applications must use VBE function 09h to manipulate the
> > > palette rather than directly accessing the VGA registers.
> > >
> > > This implementation uses the standard VGA registers for all hardware,
> > > which may not be appropriate; I only verified qemu -device VGA.
> > >
> > > Without this patch, the get/set palette function returns an error code,
> > > so programs that use 8-bit indexed color modes fail. For example, Quake
> > > (DOS) printed "Error: Unable to load VESA palette" and exited when
> > > trying to set a SVGA mode like 640x480, but with the patch it succeeds.
> > > This fixes qemu issue #251 and #1862.
> > >
> > > https://gitlab.com/qemu-project/qemu/-/issues/251
> > > https://gitlab.com/qemu-project/qemu/-/issues/1862
> >
> > Interesting, thanks.
> >
> > If I understand the vbe_104f09() requirements, it does basically the
> > same thing as handle_101012() and handle_101017(), but with a slightly
> > different memory layout.
> >
> > Have you considered implementing vbe_104f09() using the existing
> > stdvga_dac_read() and stdvga_dac_write() calls?  That is, check for
> > CONFIG_VGA_STDVGA_PORTS at the start of vbe_104f09() (if that
> > definition is not set then it is not valid to access the dac
> > registers).  If it is set, then loop through each pallete request,
> > translate the pallette request to/from the existing dac format, and
> > call stdvga_dac_read/write().  The advantage of this approach is that
> > we maintain only one low-level mechanisms for accessing the dac
> > pallette registers.
> >
> > Cheers,
> > -Kevin
>
> Rewriting the buffer in place is a clever idea; I'll give that a try.
> I guess it should be okay to rewrite the buffer in place as long as we
> restore it to the original format after a "set" call so from the
> caller's perspective, the buffer is not modified by a "read-only"
> function. It can probably be arranged so that this happens naturally -
> convert to stdvga format before calling either read or write, then
> convert to VBE format after the call (which would be needed for "get"
> anyway).

Rewriting the buffer I guess is possible, but just to be clear I was
more thinking about splitting the request up into many smaller
requests.

Something like (totally untested):

int
vbe_set_palette_colors(u16 seg, struct vbe_palette_entry *pal_far, u8 start, int count)
{
    while (count) {
        u8 rgb[3] = { pal_far->red, pal_far->green, pal_far->blue };
        stdvga_dac_write(GET_SEG(SS), rgb, start, 1);
        pal_far++;
        start++;
        count--;
    }
    return 0;
}

int
vbe_get_palette_colors(u16 seg, struct vbe_palette_entry *pal_far, u8 start, int count)
{
    while (count) {
        u8 rgb[3];
        stdvga_dac_read(GET_SEG(SS), rgb, start, 1);
        pal_far->red = rgb[0];
        pal_far->green = rgb[1];
        pal_far->blue = rgb[2];
        pal_far++;
        start++;
        count--;
    }
    return 0;
}

It's also totally fine to redefine stdvga_dac_read/write() and/or
introduce a stdhw_dac_read/write().  Whatever the interface is though,
I do think that vbe_104f09(), handle_101010(), handle_101012(),
handle_101015(), handle_101017() should use that same interface.

Cheers,
-Kevin