>> +
>> + QemuRect dst;
>> + {
>> + unsigned dst_width = s->regs.dst_width;
>> + unsigned dst_height = s->regs.dst_height;
>> + unsigned dst_x = (s->regs.dp_cntl & DST_X_LEFT_TO_RIGHT ?
>> + s->regs.dst_x : s->regs.dst_x + 1 - dst_width);
>> + unsigned dst_y = (s->regs.dp_cntl & DST_Y_TOP_TO_BOTTOM ?
>> + s->regs.dst_y : s->regs.dst_y + 1 - dst_height);
>> + qemu_rect_init(&dst, dst_x, dst_y, dst_width, dst_height);
>> + }
>
> This is a bit unusual style putting variable init in a block. I'm not sure
> it's acceptable for QEMU, maybe you could put it in a static helper
> function which is more usual style.
>
Not a problem, I'll break these out into a helper in v3.
>> +
>> + QemuRect scissor;
>> + {
>> + uint16_t sc_left = s->regs.sc_top_left & 0x3fff;
>> + uint16_t sc_top = (s->regs.sc_top_left >> 16) & 0x3fff;
>> + uint16_t sc_right = s->regs.sc_bottom_right & 0x3fff;
>> + uint16_t sc_bottom = (s->regs.sc_bottom_right >> 16) & 0x3fff;
>> + qemu_rect_init(&scissor, sc_left, sc_top,
>> + sc_right - sc_left + 1, sc_bottom - sc_top + 1);
>> + }
>
> This could be checked on real hardware too what happens if you store
> something in reserved bits (the docs may suggest that e.g. SC_BOTTOM,
> SC_RIGHT and SC_BOTTOM_RIGHT might be the same register so writing
> reserved bits may overwrite others or masked out by hardware but it's not
> clear from docs; the rage128pro docs aren't even clear on what the limits
> are as the summary text in the section 3.28 gives different limits than
> individual register descriptions right after that). To simplify using
> these values I generally tried to apply reserved bits mask on write so no
> need to do that at read and using the values. Maybe these should do the
> same?
>
It looks like the scissor registers are masked at write! Bits 13:0 are set
on write and writing to reserved bits are just ignored. So you're absolutely
right we shouldn't be bothering with this on read. I'll update this in v3.
# Tested On: ATI Technologies Inc Rage 128 Pro Ultra TF
reserved scissor bits
======================================
** Initializing SC_BOTTOM to 0x0 **
** Initializing SC_RIGHT to 0x0 **
** Initializing SC_TOP to 0x0 **
** Initializing SC_LEFT to 0x0 **
Initial State
------------------------------------
SC_BOTTOM: 0x00000000
SC_RIGHT: 0x00000000
SC_TOP: 0x00000000
SC_LEFT: 0x00000000
** Setting SC_BOTTOM to 0xffffffff **
** Setting SC_TOP to 0xffffffff **
After State
------------------------------------
SC_BOTTOM: 0x00003fff <====== Masked at write (14-bit)
SC_RIGHT: 0x00000000
SC_TOP: 0x00003fff <====== Masked at write (14-bit)
SC_LEFT: 0x00000000
** Setting SC_RIGHT to 0xffffffff **
** Setting SC_LEFT to 0xffffffff **
After State
------------------------------------
SC_BOTTOM: 0x00003fff
SC_RIGHT: 0x00003fff <====== Masked at write (14-bit)
SC_TOP: 0x00003fff
SC_LEFT: 0x00003fff <====== Masked at write (14-bit)
** Setting SC_BOTTOM_RIGHT to 0xfeeefeee **
** Setting SC_TOP_LEFT to 0xfeeefeee **
After State
------------------------------------
SC_BOTTOM: 0x00003eee <====== Masked at write (14-bit)
SC_RIGHT: 0x00003eee <====== Masked at write (14-bit)
SC_TOP: 0x00003eee <====== Masked at write (14-bit)
SC_LEFT: 0x00003eee <====== Masked at write (14-bit)
