Re: [PATCH 02/24] display/xlnx_dp: Fix to realize "i2c-ddc" and "aux-to-i2c-bridge"
Peter Maydell writes: > On Tue, 19 May 2020 at 06:09, Markus Armbruster wrote: >> I figure the "device becomes real only on realize" thing is actually >> more myth than thing. > > It's not a myth, it's an API guarantee thing. If you don't realize > the device you create before you use it then you're in the world of > unspecified behaviour, and anything could happen: maybe it works, > maybe it doesn't, maybe it works today and breaks tomorrow. It's a myth in the sense "we want it to be that way, but it often ain't" :) Of course your right in that it is also a case of "use the interface the specified way, or else".
Re: [PATCH 02/24] display/xlnx_dp: Fix to realize "i2c-ddc" and "aux-to-i2c-bridge"
On Tue, 19 May 2020 at 06:09, Markus Armbruster wrote: > I figure the "device becomes real only on realize" thing is actually > more myth than thing. It's not a myth, it's an API guarantee thing. If you don't realize the device you create before you use it then you're in the world of unspecified behaviour, and anything could happen: maybe it works, maybe it doesn't, maybe it works today and breaks tomorrow. thanks -- PMM
Re: [PATCH 02/24] display/xlnx_dp: Fix to realize "i2c-ddc" and "aux-to-i2c-bridge"
Philippe Mathieu-Daudé writes: > On 5/18/20 12:30 PM, Peter Maydell wrote: >> On Mon, 18 May 2020 at 06:04, Markus Armbruster wrote: >>> >>> xlnx_dp_init() creates these two devices, but they're never realized. >>> Affects machine xlnx-zcu102. >>> >>> I wonder how this ever worked. If the "device becomes real only on >>> realize" thing actually works, then we've always been missing these >>> two devices, yet nobody noticed. >> >> It depends entirely on the implementation of the device. >> If it happens to do nothing in the realize method that >> matters Also in the realize methods of supertypes. >> (eg i2c-ddc has no realize method and does the limited >> amount of initialization it needs in instance_init) then the >> device will (by lucky accident) work just fine. Yes. >> We should really ideally have an assert() in the DeviceClass >> reset that the device was realized, so we can keep this kind >> of bug out of the codebase. (Last time I looked it wasn't obvious >> exactly where to put the assert now that we have both legacy-reset >> and three-phase-reset, unfortunately.) > > Your wish came true in the last patch of this series! #24: > https://www.mail-archive.com/qemu-devel@nongnu.org/msg704239.html Not exactly what Peter asked for, but hopefully close enough for practical purposes.
Re: [PATCH 02/24] display/xlnx_dp: Fix to realize "i2c-ddc" and "aux-to-i2c-bridge"
Fred Konrad writes: > Le 5/18/20 à 7:03 AM, Markus Armbruster a écrit : >> xlnx_dp_init() creates these two devices, but they're never realized. >> Affects machine xlnx-zcu102. >> >> I wonder how this ever worked. If the "device becomes real only on >> realize" thing actually works, then we've always been missing these >> two devices, yet nobody noticed. > > I can't tell, but it used to work back in 2016 since these devices were > required > to have a working framebuffer. I don't doubt you. I figure the "device becomes real only on realize" thing is actually more myth than thing.
Re: [PATCH 02/24] display/xlnx_dp: Fix to realize "i2c-ddc" and "aux-to-i2c-bridge"
On Sun, May 17, 2020 at 10:14 PM Markus Armbruster wrote: > > xlnx_dp_init() creates these two devices, but they're never realized. > Affects machine xlnx-zcu102. > > I wonder how this ever worked. If the "device becomes real only on > realize" thing actually works, then we've always been missing these > two devices, yet nobody noticed. > > Fix by realizing them in xlnx_dp_realize(). > > Fixes: 58ac482a66de09a7590f705e53fc6a3fb8a055e8 > Cc: KONRAD Frederic > Cc: Alistair Francis > Cc: "Edgar E. Iglesias" > Cc: Peter Maydell > Cc: qemu-...@nongnu.org > Signed-off-by: Markus Armbruster Reviewed-by: Alistair Francis Alistair > --- > hw/display/xlnx_dp.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c > index 3e5fb44e06..bdc229a51e 100644 > --- a/hw/display/xlnx_dp.c > +++ b/hw/display/xlnx_dp.c > @@ -1264,9 +1264,13 @@ static void xlnx_dp_realize(DeviceState *dev, Error > **errp) > DisplaySurface *surface; > struct audsettings as; > > +qdev_init_nofail(DEVICE(s->aux_bus->bridge)); > + > qdev_init_nofail(DEVICE(s->dpcd)); > aux_map_slave(AUX_SLAVE(s->dpcd), 0x); > > +qdev_init_nofail(DEVICE(s->edid)); > + > s->console = graphic_console_init(dev, 0, _dp_gfx_ops, s); > surface = qemu_console_surface(s->console); > xlnx_dpdma_set_host_data_location(s->dpdma, DP_GRAPHIC_DMA_CHANNEL, > -- > 2.21.1 > >
Re: [PATCH 02/24] display/xlnx_dp: Fix to realize "i2c-ddc" and "aux-to-i2c-bridge"
On 5/18/20 12:30 PM, Peter Maydell wrote: On Mon, 18 May 2020 at 06:04, Markus Armbruster wrote: xlnx_dp_init() creates these two devices, but they're never realized. Affects machine xlnx-zcu102. I wonder how this ever worked. If the "device becomes real only on realize" thing actually works, then we've always been missing these two devices, yet nobody noticed. It depends entirely on the implementation of the device. If it happens to do nothing in the realize method that matters (eg i2c-ddc has no realize method and does the limited amount of initialization it needs in instance_init) then the device will (by lucky accident) work just fine. We should really ideally have an assert() in the DeviceClass reset that the device was realized, so we can keep this kind of bug out of the codebase. (Last time I looked it wasn't obvious exactly where to put the assert now that we have both legacy-reset and three-phase-reset, unfortunately.) Your wish came true in the last patch of this series! #24: https://www.mail-archive.com/qemu-devel@nongnu.org/msg704239.html thanks -- PMM
Re: [PATCH 02/24] display/xlnx_dp: Fix to realize "i2c-ddc" and "aux-to-i2c-bridge"
On Mon, 18 May 2020 at 06:04, Markus Armbruster wrote: > > xlnx_dp_init() creates these two devices, but they're never realized. > Affects machine xlnx-zcu102. > > I wonder how this ever worked. If the "device becomes real only on > realize" thing actually works, then we've always been missing these > two devices, yet nobody noticed. It depends entirely on the implementation of the device. If it happens to do nothing in the realize method that matters (eg i2c-ddc has no realize method and does the limited amount of initialization it needs in instance_init) then the device will (by lucky accident) work just fine. We should really ideally have an assert() in the DeviceClass reset that the device was realized, so we can keep this kind of bug out of the codebase. (Last time I looked it wasn't obvious exactly where to put the assert now that we have both legacy-reset and three-phase-reset, unfortunately.) thanks -- PMM
Re: [PATCH 02/24] display/xlnx_dp: Fix to realize "i2c-ddc" and "aux-to-i2c-bridge"
On Mon, May 18, 2020 at 07:03:46AM +0200, Markus Armbruster wrote: > xlnx_dp_init() creates these two devices, but they're never realized. > Affects machine xlnx-zcu102. > > I wonder how this ever worked. If the "device becomes real only on > realize" thing actually works, then we've always been missing these > two devices, yet nobody noticed. > > Fix by realizing them in xlnx_dp_realize(). Reviewed-by: Edgar E. Iglesias > > Fixes: 58ac482a66de09a7590f705e53fc6a3fb8a055e8 > Cc: KONRAD Frederic > Cc: Alistair Francis > Cc: "Edgar E. Iglesias" > Cc: Peter Maydell > Cc: qemu-...@nongnu.org > Signed-off-by: Markus Armbruster > --- > hw/display/xlnx_dp.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c > index 3e5fb44e06..bdc229a51e 100644 > --- a/hw/display/xlnx_dp.c > +++ b/hw/display/xlnx_dp.c > @@ -1264,9 +1264,13 @@ static void xlnx_dp_realize(DeviceState *dev, Error > **errp) > DisplaySurface *surface; > struct audsettings as; > > +qdev_init_nofail(DEVICE(s->aux_bus->bridge)); > + > qdev_init_nofail(DEVICE(s->dpcd)); > aux_map_slave(AUX_SLAVE(s->dpcd), 0x); > > +qdev_init_nofail(DEVICE(s->edid)); > + > s->console = graphic_console_init(dev, 0, _dp_gfx_ops, s); > surface = qemu_console_surface(s->console); > xlnx_dpdma_set_host_data_location(s->dpdma, DP_GRAPHIC_DMA_CHANNEL, > -- > 2.21.1 >
Re: [PATCH 02/24] display/xlnx_dp: Fix to realize "i2c-ddc" and "aux-to-i2c-bridge"
Le 5/18/20 à 7:03 AM, Markus Armbruster a écrit : xlnx_dp_init() creates these two devices, but they're never realized. Affects machine xlnx-zcu102. I wonder how this ever worked. If the "device becomes real only on realize" thing actually works, then we've always been missing these two devices, yet nobody noticed. I can't tell, but it used to work back in 2016 since these devices were required to have a working framebuffer. Fix by realizing them in xlnx_dp_realize(). Fixes: 58ac482a66de09a7590f705e53fc6a3fb8a055e8 Cc: KONRAD Frederic Cc: Alistair Francis Cc: "Edgar E. Iglesias" Cc: Peter Maydell Cc: qemu-...@nongnu.org Signed-off-by: Markus Armbruster --- hw/display/xlnx_dp.c | 4 1 file changed, 4 insertions(+) diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c index 3e5fb44e06..bdc229a51e 100644 --- a/hw/display/xlnx_dp.c +++ b/hw/display/xlnx_dp.c @@ -1264,9 +1264,13 @@ static void xlnx_dp_realize(DeviceState *dev, Error **errp) DisplaySurface *surface; struct audsettings as; +qdev_init_nofail(DEVICE(s->aux_bus->bridge)); + qdev_init_nofail(DEVICE(s->dpcd)); aux_map_slave(AUX_SLAVE(s->dpcd), 0x); +qdev_init_nofail(DEVICE(s->edid)); + s->console = graphic_console_init(dev, 0, _dp_gfx_ops, s); surface = qemu_console_surface(s->console); xlnx_dpdma_set_host_data_location(s->dpdma, DP_GRAPHIC_DMA_CHANNEL,
