Incorrect results drawing horizontal and vertical lines to the LFB returned by VBE

I finally managed to draw a cyan pixel on the screen using the VESA BIOS Extensions (1920px * 1080px, 24bpp).

;esi = bytes per scan line
;edx = physical address of linear framebuffer memory.
;ebx = x coord * 3
;ecx = y coord

DrawPixel:
    push edx
    mov edx, 0
    mov eax, 0
    lea eax, [esi]
    ;mov ecx, 0
    mul ecx
    add eax, ebx
    jmp draw

draw:
    pop edx
    add edx, eax
    mov ebx, 0x3296fa

    mov [edx], ebx
    ret

I tried to draw a cyan horizontal line on the screen using a "for loop" this way:

mov edi, 1920
call drawLoop
jmp $

drawLoop:
    dec edi                                       ;decrease edi
    cmp edi, 0                                    ;is edi equal to zero?
    jl doneLoop                                   ;then return
    imul ebx, edi, 3                              ;multiply edi by three and save the result in ebx
    mov ecx, 0                                    ;y = 0
    mov esi, ModeInfoBlock + 10h
    mov edx, dword[ModeInfoBlock + 28h]
    call DrawPixel                                ;Draw it!
    jmp drawLoop                                  ;run this again

doneLoop:
    ret

However, this doesn't work: it draws a green line instead.

When I try to draw a vertical line again with the draw/draw pixel code, it doesn't work either. It plots pixels with random colors everywhere. Here's how I use the DrawPixel function to draw a vertical line:

%include "../kernel/Services/Display/display.asm"

kernel:
    mov edi, 1080
    call drawLoop
    jmp $

drawLoop:
    dec edi
    cmp edi, 0
    jl doneLoop
    mov ecx, edi
    mov ebx, 0
    mov esi, ModeInfoBlock + 10h
    mov edx, dword[ModeInfoBlock + 28h]
    call DrawPixel
    jmp drawLoop

doneLoop:
    ret

Any way to solve these problems?

Solution

Let's start by re-writing the DrawPixel routine. Currently it's a bit of a mess!

There's no point in using the mul instruction that will clobber the EDX register needlessly. Better use a variant of imul.

And instead of using mov eax, 0 lea eax, [si] to load the EAX register, why don't you simply write mov eax, esi?

There's also an error to consider. Because you're working on a 24-bit true color screen, writing a whole dword (32 bits) will change part of an adjacent pixel.

;esi = bytes per scan line
;edx = physical address of linear framebuffer memory.
;ebx = x coord * 3
;ecx = y coord

; IN (ebx,ecx,edx,esi) OUT () MOD (eax)
DrawPixel:
    mov     eax, esi                ; BytesPerScanLine
    imul    eax, ecx                ; BytesPerScanLine * Y
    add     eax, ebx                ; BytesPerScanLine * Y + X * 3
    mov     word [edx+eax], 0x96FA  ; Low word of RGB triplet
    mov     byte [edx+eax+2], 0x32  ; High byte of RGB triplet
    ret

This new routine now only modifies the EAX register

The main part has problems of its own:

The mov esi, ModeInfoBlock + 10h will not retrieve the BytesPerScanLine info. For that to happen you would need movzx esi, word [ModeInfoBlock + 10h]

The loop uses 2 branches on every iteration. It's perfectly possible to write the loop with a single branch.

Next is my version of it. Because the new DrawPixel routine preserves all the registers (except EAX) great simplifications are possible:

    xor     ebx, ebx                         ; X = 0  -> EBX = X * 3
    xor     ecx, ecx                         ; Y = 0
    movzx   esi, word [ModeInfoBlock + 10h]  ; BytesPerScanLine
    mov     edx, [ModeInfoBlock + 28h]       ; PhysBasePtr
    call    drawLoop
    jmp     $

drawLoop:
    call    DrawPixel                        ; Modifies EAX
    add     ebx, 3                           ; Like X = X + 1
    cmp     ebx, 1920*3                      ; Length of the line is 1920 pixels
    jb      drawLoop
    ret

My version draws this horizontal line from left to right. I believe it could be a tiny bit faster than drawing from the right to the left.

Instead of using a separate loop counter (EDI), I control the loop via the tripled-X coordinate. Amongst other benefits (like speed because cmpand jb pair up nicely) this relieves the pressure on register use.

Better horizontal and vertical line drawing routines

Especially for drawing horizontal and vertical lines, it is not a good idea to repeatedly call a DrawPixel routine. It's a waste of time to calculate the pixel's address over and over again. Below I show a couple routines specifically for these tasks.

I've added some extra changes:

You should not burden the main program with the technical details of addressing the video memory. Have the graphics routines retrieve the BytesPerScanLine and PhysBasePtr values.
The main program should deal with pixels at the (X,Y) level. That "times 3" stuff again is a technical detail that belongs to the graphics routines.
Hard coding the color in the drawing routines is very much not flexible.

; IN (eax,ebx,ecx,edx) OUT () MOD (eax)
; EAX = X
; EBX = Y
; ECX = Color
; EDX = Line length
HLine:
    push    edx
    push    edi
    movzx   edi, word [ModeInfoBlock + 10h]  ; BytesPerScanLine
    imul    edi, ebx                         ; BytesPerScanLine * Y
    imul    eax, 3                           ; X * 3
    add     edi, eax                         ; BytesPerScanLine * Y + X * 3
    add     edi, [ModeInfoBlock + 28h]       ; ... + PhysBasePtr
    mov     eax, ecx                         ; Color 24 bits
    shr     eax, 8
    imul    edx, 3                           ; Line length * 3
    add     edx, edi                         ; Address of the end of line
.a: mov     [edi], cx                        ; Low word of RGB triplet
    mov     [edi+2], ah                      ; High byte of RGB triplet
    add     edi, 3                           ; Like (X + 1)
    cmp     edi, edx
    jb      .a
    pop     edi
    pop     edx
    ret

The above HLine routine draws a horizontal line from left to right.

; IN (eax,ebx,ecx,edx) OUT () MOD (eax)
; EAX = X
; EBX = Y
; ECX = Color
; EDX = Line length
VLine:
    push    edx
    push    esi
    push    edi
    movzx   esi, word [ModeInfoBlock + 10h]  ; BytesPerScanLine
    mov     edi, esi
    imul    edi, ebx                         ; BytesPerScanLine * Y
    imul    eax, 3                           ; X * 3
    add     edi, eax                         ; BytesPerScanLine * Y + X * 3
    add     edi, [ModeInfoBlock + 28h]       ; ... + PhysBasePtr
    mov     eax, ecx                         ; Color 24 bits
    shr     eax, 8
    imul    edx, esi                         ; Line length * BytesPerScanLine
    add     edx, edi                         ; Address of the end of line
.a: mov     [edi], cx                        ; Low word of RGB triplet
    mov     [edi+2], ah                      ; High byte of RGB triplet
    add     edi, esi                         ; Like (Y + 1)
    cmp     edi, edx
    jb      .a
    pop     edi
    pop     esi
    pop     edx
    ret

The above VLine routine draws a vertical line from top to bottom.

This is how you could use these:

Main:
    xor     eax, eax                         ; X = 0
    xor     ebx, ebx                         ; Y = 0
    mov     ecx, 0x003296FA                  ; Color cyan
    mov     edx, 1920                        ; Line length
    call    HLine                            ; -> (EAX)
    mov     edx, 1080
    call    VLine                            ; -> (EAX)
    jmp     $