[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] hw/display: Add basic ATI VGA emulation
From: |
Mark Cave-Ayland |
Subject: |
Re: [Qemu-devel] [PATCH] hw/display: Add basic ATI VGA emulation |
Date: |
Wed, 13 Feb 2019 07:06:29 +0000 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 |
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.
- [Qemu-devel] [PATCH] hw/display: Add basic ATI VGA emulation, BALATON Zoltan, 2019/02/10
- Re: [Qemu-devel] [PATCH] hw/display: Add basic ATI VGA emulation, Philippe Mathieu-Daudé, 2019/02/11
- Re: [Qemu-devel] [PATCH] hw/display: Add basic ATI VGA emulation, BALATON Zoltan, 2019/02/12
- Re: [Qemu-devel] [PATCH] hw/display: Add basic ATI VGA emulation, Peter Maydell, 2019/02/19
- Re: [Qemu-devel] [PATCH] hw/display: Add basic ATI VGA emulation, BALATON Zoltan, 2019/02/19
- Re: [Qemu-devel] [PATCH] hw/display: Add basic ATI VGA emulation, BALATON Zoltan, 2019/02/20
- Re: [Qemu-devel] [PATCH] hw/display: Add basic ATI VGA emulation, BALATON Zoltan, 2019/02/20
- Re: [Qemu-devel] [PATCH] hw/display: Add basic ATI VGA emulation, Peter Maydell, 2019/02/21
Re: [Qemu-devel] [PATCH] hw/display: Add basic ATI VGA emulation, Gerd Hoffmann, 2019/02/19