Do not go through the panning buffer unless the address wraps in the middle
of the line.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/display/vga-helpers.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/hw/display/vga-helpers.h b/hw/display/vga-helpers.h
index 29933562c45..60ddb27d946 100644
--- a/hw/display/vga-helpers.h
+++ b/hw/display/vga-helpers.h
@@ -265,6 +265,18 @@ static void *vga_draw_line8d2(VGACommonState *vga, uint8_t *d,
 
     palette = vga->last_palette;
     hpel = (hpel >> 1) & 3;
+
+    /* For 256 color modes, we can adjust the source address and write directly
+     * to the destination, even if horizontal pel panning is active.  However,
+     * the loop below assumes that the address does not wrap in the middle of a
+     * plane.  If that happens...
+     */
+    if (addr + (width >> 3) * 4 < VGA_VRAM_SIZE) {
+        addr += hpel * 4;
+        hpel = 0;
+    }
+
+    /* ... use the panning buffer as in planar modes.  */
     if (hpel) {
         width += 8;
         d = vga->panning_buf;
-- 
2.43.0

On Sun, 31 Dec 2023, Paolo Bonzini wrote:
> Do not go through the panning buffer unless the address wraps in the middle
> of the line.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> hw/display/vga-helpers.h | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/hw/display/vga-helpers.h b/hw/display/vga-helpers.h
> index 29933562c45..60ddb27d946 100644
> --- a/hw/display/vga-helpers.h
> +++ b/hw/display/vga-helpers.h
> @@ -265,6 +265,18 @@ static void *vga_draw_line8d2(VGACommonState *vga, uint8_t *d,
>
>     palette = vga->last_palette;
>     hpel = (hpel >> 1) & 3;
> +
> +    /* For 256 color modes, we can adjust the source address and write directly
> +     * to the destination, even if horizontal pel panning is active.  However,
> +     * the loop below assumes that the address does not wrap in the middle of a
> +     * plane.  If that happens...
> +     */
> +    if (addr + (width >> 3) * 4 < VGA_VRAM_SIZE) {
> +        addr += hpel * 4;
> +        hpel = 0;
> +    }
> +
> +    /* ... use the panning buffer as in planar modes.  */
>     if (hpel) {
>         width += 8;
>         d = vga->panning_buf;

Is it possible to do these checks once in vga.c and instead of changing 
the return value of the draw functions pass panning_buf as d if needed? 
Maybe that way the draw funcs could be left unchanged?

Regards,
BALATON Zoltan

Il dom 31 dic 2023, 17:27 BALATON Zoltan <balaton@eik.bme.hu> ha scritto:

> >     palette = vga->last_palette;
> >     hpel = (hpel >> 1) & 3;
> > +
> > +    /* For 256 color modes, we can adjust the source address and write
> directly
> > +     * to the destination, even if horizontal pel panning is active.
> However,
> > +     * the loop below assumes that the address does not wrap in the
> middle of a
> > +     * plane.  If that happens...
> > +     */
> > +    if (addr + (width >> 3) * 4 < VGA_VRAM_SIZE) {
> > +        addr += hpel * 4;
> > +        hpel = 0;
> > +    }
> > +
> > +    /* ... use the panning buffer as in planar modes.  */
> >     if (hpel) {
> >         width += 8;
> >         d = vga->panning_buf;
>
> Is it possible to do these checks once in vga.c and instead of changing
> the return value of the draw functions pass panning_buf as d if needed?
> Maybe that way the draw funcs could be left unchanged?
>

As of the previous patch it could, here however the logic for whether to
use the panning_buf depends on the drawing function; 8d2 is special and
different from the others.

I can remove the optimization, it's not super Important; but it's kind of
obvious to do it for the VGA 256-color modes, since they're the only ones
with the same bit alignment for all pixels.

Paolo


> Regards,
> BALATON Zoltan
>
>

On Mon, 1 Jan 2024, Paolo Bonzini wrote:
> Il dom 31 dic 2023, 17:27 BALATON Zoltan <balaton@eik.bme.hu> ha scritto:
>
>>>     palette = vga->last_palette;
>>>     hpel = (hpel >> 1) & 3;
>>> +
>>> +    /* For 256 color modes, we can adjust the source address and write
>> directly
>>> +     * to the destination, even if horizontal pel panning is active.
>> However,
>>> +     * the loop below assumes that the address does not wrap in the
>> middle of a
>>> +     * plane.  If that happens...
>>> +     */
>>> +    if (addr + (width >> 3) * 4 < VGA_VRAM_SIZE) {
>>> +        addr += hpel * 4;
>>> +        hpel = 0;
>>> +    }
>>> +
>>> +    /* ... use the panning buffer as in planar modes.  */
>>>     if (hpel) {
>>>         width += 8;
>>>         d = vga->panning_buf;
>>
>> Is it possible to do these checks once in vga.c and instead of changing
>> the return value of the draw functions pass panning_buf as d if needed?
>> Maybe that way the draw funcs could be left unchanged?
>>
>
> As of the previous patch it could, here however the logic for whether to
> use the panning_buf depends on the drawing function; 8d2 is special and
> different from the others.
>
> I can remove the optimization, it's not super Important; but it's kind of
> obvious to do it for the VGA 256-color modes, since they're the only ones
> with the same bit alignment for all pixels.

I guess that means it's not easy to do so my question is not so imprtant 
then.

Regards,
BALATON Zoltan