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

Drawing Horizontal Line

Based on comments I solved my problem with drawing the horizontal line by only writing 3 bytes to the video display instead of 4 bytes for each pixel. The extra byte was altering the color of the next pixel on the screen. My revised code looks like:

DrawPixel:
    push edx
    mov edx, 0
    mov eax, 0
    mov eax, esi
    mul ecx
    add eax, ebx
    jmp draw

draw:
    pop edx
    add edx, eax
    mov word[edx], 0x96fa
    mov byte[edx + 2], 0x32
    ret

Drawing Vertical Line

In the code that generates a vertical line I managed to fix the issue by replacing mov esi, ModeInfoBlock + 10h with movzx esi, word[ModeInfoBlock + 10h].

Because the movzx instruction moves the 16bit bytesPerScanLine value into the 32bit esi register and fills the rest with zeros. It stands for "move zero extend".

My revised vertical drawing code:

%include "../kernel/Services/Display/display.asm"
kernel:
    mov edi, 1920
    call drawLoop
    jmp $

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

doneLoop:
    ret

These are my final drawing functions:

;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
    mov eax, esi
    mul ecx
    add eax, ebx
    jmp draw

draw:
    pop edx
    add edx, eax
    mov word[edx], 0x96fa
    mov byte[edx + 2], 0x32
    ret

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     $