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

[BALATON Zoltan]

On Sun, 3 Mar 2019, Philippe Mathieu-Daudé wrote:
> On 3/3/19 1:46 PM, BALATON Zoltan wrote:
> > On Sun, 3 Mar 2019, Philippe Mathieu-Daudé wrote:
> > > > +??? case RBBM_STATUS:
> > >
> > > ????????? /* fall through */
> > >
> > > > +??? case GUI_STAT:
> > > > +??????? val = 64; /* free CMDFIFO entries */
> > > > +??????? break;
> >
> > Obviously fall through because there are no statements between case
> > labels. I only use /* fall thorugh */ comment where there are some
> > statements and a missing break but not in this case which should be
> > clear without comment.
>
> Apparently not enough obvious to me since I stopped here, since I don't
> know the difference/relation between RBBM_STATUS/GUI_STAT registers.

I did not mean obvious from the registers but obvious from having no 
statements between case labels. Multiple case labels for common cases in 
switch without fall through comment between them is used in several other 
places in QEMU as well. The fall through comment is only needed when there 
are some statements but no break at the end and code falls through to the 
next case. Then the comment tells that break was not forgotten but fall 
through is intended but no such comment is needed when multiple cases go 
to same place like here. It may look odd in a big switch where every other 
case has statements only these two are common ones but it's correct and 
consistent with other places and should be clear without comment what is 
meant. (I could add a comment here anyway to make it look more regular but 
I prefer code that's less verbose vertically so more lines fit on the 
screen.)

> > > > +??? if (s->dev_id == PCI_DEVICE_ID_ATI_RADEON_QY &&
> > > > +??????? s->vga.vram_size_mb < 16) {
> > > > +??????? warn_report("Too small video memory for device id");
> > > > +??????? s->vga.vram_size_mb = 16;
> > >
> > > I'd rather use error_setg() and return.
> >
> > Correcting the error if possible is friendlier IMO but this can be
> > debated what's the preferred behaviour. I'll leave it as it is for now.
>
> Well we are somehow lying to the user... modifying his hardware without
> his consent.

We do warn them though so not silently changing hardware. There are 
arguments for both ways, I can't decide and this can be changed anytime 
later so I go with what I like unless more votes for another way.

> > I could do any of the above but all this file is temporary for
> > development and meant to be removed once enough functions of the device
> > are implemented so for now I don't bother unless there's a strong
> > argument other than style preference to change it.
>
> I'd definitively keep it, and I now believe you should drop the
> DEBUG_ATI around. It is only used by tracing right? And tracing is
> already optimized. If it becomes someday unuseful to you, it can still
> be useful for the next one wanting to improve/fix your device.

We've discussed before why trace points are not convenient for debugging 
during development so for these I keep DPRINTF and only convert to traces 
parts that are mostly finished. This way it's also clear which messages 
are to be removed later and which are meant to be kept for tracing. If we 
choose to keep register names also after development for more detailed 
traces then this could be cleaned up but this probably would just add 
unused code for most of the time. So after development is finished I don't 
think it's useful to include reg names.

> > > > diff --git a/hw/display/trace-events b/hw/display/trace-events
> > > > index 37d3264bb2..6aed33eeaa 100644
> > > > --- a/hw/display/trace-events
> > > > +++ b/hw/display/trace-events
> > > > @@ -138,3 +138,7 @@ vga_cirrus_write_blt(uint32_t offset, uint32_t
> > > > val) "offset 0x%x, val 0x%x"
> > > > ?sii9022_read_reg(uint8_t addr, uint8_t val) "addr 0x%02x, val 0x%02x"
> > > > ?sii9022_write_reg(uint8_t addr, uint8_t val) "addr 0x%02x, val 0x%02x"
> > > > ?sii9022_switch_mode(const char *mode) "mode: %s"
> > > > +
> > > > +# hw/display/ati*.c
> > > > +ati_mm_read(unsigned int size, uint64_t addr, const char *name,
> > > > uint64_t val) "%d 0x%"HWADDR_PRIx " %s -> 0x%"PRIx64
> > > > +ati_mm_write(unsigned int size, uint64_t addr, const char *name,
> > > > uint64_t val) "%d 0x%"HWADDR_PRIx " %s <- 0x%"PRIx64
> > >
> > > "%u ...
> >
> > Should not matter as size that can only be 1 to 4 but %u is more correct
> > to print unsigned variable so I'll change this.
>
> The argument is unsigned, so the correct format is %u.
>
> This help reducing the thousands of warnings in QEMU about incorrect
> formatstring.

Yes, I've corrected this. I did not get warnings for it but maybe some 
compilers are pickier about this.

Regards,
BALATON Zoltan