Re: [Qemu-devel] [PATCH] hw/display: Add basic ATI VGA emulation

[Mark Cave-Ayland]

On 12/02/2019 23:59, BALATON Zoltan wrote:

> Hello,
> 
> On Tue, 12 Feb 2019, Philippe Mathieu-Daudé wrote:
>> Hi Zoltan,
> 
> Thanks for the quick review and testing. I'll use your suggestions for the 
> other
> (mips) patches in a v2. For this one I'm not convinced.
> 
>> On 2/11/19 4:19 AM, BALATON Zoltan wrote:
> [...]
>>> +
>>> +static void ati_reg_write_offs(uint32_t *reg, int offs,
>>> +                               uint64_t data, unsigned int size)
>>> +{
>>> +    int shift, i;
>>> +    uint32_t mask;
>>> +
>>> +    for (i = 0; i < size; i++) {
>>> +        shift = (offs + i) * 8;
>>> +        mask = 0xffUL << shift;
>>> +        *reg &= ~mask;
>>> +        *reg |= (data & 0xff) << shift;
>>> +        data >>= 8;
>>
>> I'd have use a pair of extract32/deposit32 but this is probably easier
>> to singlestep.
> 
> You've told me that before but I have concerns about the asserts in those 
> functions
> which to me seem like unnecessary overhead in such low level functions so 
> unless
> these are removed or *_noassert versions introduced I'll stay away from them.
> 
> But I'm also not too happy about these *_offs functions but some registers 
> support
> 8/16/32 bit access and guest code seems to actually do this to update bits in 
> the
> middle of the register at an odd address. Best would be if I could just set 
> .impl.min
> = 4, .impl.max = 4 and .valid.min = 1 .valid.max = 4 for the mem region ops 
> but I'm
> not sure that would work or would it? If that's working maybe I should just 
> go with
> that instead.
> 
> [...]
>>> diff --git a/hw/display/ati_int.h b/hw/display/ati_int.h
>>> new file mode 100644
>>> index 0000000000..85d045517c
>>> --- /dev/null
>>> +++ b/hw/display/ati_int.h
>>> @@ -0,0 +1,67 @@
>>> +/*
>>> + * QEMU ATI SVGA emulation
>>> + *
>>> + * Copyright (c) 2019 BALATON Zoltan
>>> + *
>>> + * This work is licensed under the GNU GPL license version 2 or later.
>>> + */
>>> +
>>> +#include "qemu/osdep.h"
>>> +#include "hw/pci/pci.h"
>>> +#include "vga_int.h"
>>> +
>>> +#undef DEBUG_ATI
>>> +
>>> +#ifdef DEBUG_ATI
>>> +#define DPRINTF(fmt, ...) printf("%s: " fmt, __func__, ## __VA_ARGS__)
>>> +#else
>>> +#define DPRINTF(fmt, ...) do {} while (0)
>>
>> Please use tracepoints (you already add some!).
> 
> I won't and here's why: This is not a finished device model and I expect to 
> need to
> add debug logs and change them frequently during further development and for 
> such
> ad-hoc debugging DPRINF is still easier to use because I don't have to define 
> the
> format string at one file and use them somewhere else. With DPRINTF I can 
> just add a
> debug log at one place and change it easily without editing it at two 
> unrelated
> places so it's easier to work with. Once development is finished those that 
> we intend
> to leave in for later tracing can be converted to trace points (for which 
> trace point
> is better) and at that point remove the DPRINTF macro. We still have enough 
> DPRINTFs
> in QEMU so this should be OK. I've already added trace points to two such 
> places but
> even for those I almost considered ditching them when checkpatch insisted I 
> have to
> add 0x prefix to hex numbers (I don't like this because I know these are hex 
> and
> printing e.g. 0x8 instead of 8 is just distracting from the actual important 
> value
> which is what counts when I'm looking at a lot of these during debugging. 
> Anything
> that distracts from actual values and makes it harder to read (such as 
> timestamps and
> pids added by trace) is bad so I've considered going back to DPRINTF even for 
> those
> trace points but will see if I can live with these for now.) But those that 
> are still
> DPRINTFs won't be converted to trace but supposed to be removed when no 
> longer needed.
> 
> [...]
>> I don't understand well the display code, but the result works very
>> well, nice work :)
>>
>> Tested-by: Philippe Mathieu-Daudé <address@hidden>
> 
> Thanks, it's a start and currently only targeting Linux console with a lot 
> more to do
> for it to be more useful. But I have limited time for this so since it's 
> already
> useful to get mips_fulong2e working I thought that justifies including it now 
> so
> others have a chance to look at it and maybe even help to improve it which 
> can't
> happen if it's only sitting on my machine.

This looks interesting, however I never received the original via the mailing 
list.
Did it get held somewhere because its size?

Also it's probably worth pushing it to a suitable git repo since then it's 
easier for
people to pull and update as required.


ATB,

Mark.