| Message ID | acb431de2d9c7a497d54a548dfc7592eb2b9fe1c.1591471056.git.balaton@eik.bme.hu |
|---|---|
| State | New |
| Headers | show |
| Series | More sm501 fixes and optimisations | expand |
On Sat, 6 Jun 2020 at 20:22, BALATON Zoltan <balaton@eik.bme.hu> wrote: > > We don't need to add width to pitch when calculating last point, that > would reject valid ops within the card's local_mem. > > Fixes: b15a22bbcbe6a78dc3d88fe3134985e4cdd87de4 > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > --- > hw/display/sm501.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) Reviewed-by: Peter Maydell <peter.maydell@linaro.org> The calculations for sb/se/db/de all have a term which multiplies by (width + pitch), which makes me suspect they also need a similar fix ? thanks -- PMM
On Mon, 15 Jun 2020, Peter Maydell wrote: > On Sat, 6 Jun 2020 at 20:22, BALATON Zoltan <balaton@eik.bme.hu> wrote: >> >> We don't need to add width to pitch when calculating last point, that >> would reject valid ops within the card's local_mem. >> >> Fixes: b15a22bbcbe6a78dc3d88fe3134985e4cdd87de4 >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >> --- >> hw/display/sm501.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) > > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > > The calculations for sb/se/db/de all have a term which > multiplies by (width + pitch), which makes me suspect > they also need a similar fix ? Maybe. I'll have to check again. Actually is there a simpler way to check if two rectangles overlap when they are given with base, x, y, w, h, pitch where base is the first byte of the screen, pitch is length of one line and x,y is coordinates of top left corner and w,h is dimensions of the rect. Now that I think about it we also need to take into accounf the bytes per pixel value (1 << format) because base is given in bytes while others are in pixels so these formulae likely need some fixes. Pixman has some functions for these but those assume common base so to use those we would need to bring the two rectangles to common base which I could not find out how to do. Probably this is really simple for someone who already did a lot of these before. Regards, BALATON Zoltan
On Mon, 15 Jun 2020 at 17:40, BALATON Zoltan <balaton@eik.bme.hu> wrote: > > On Mon, 15 Jun 2020, Peter Maydell wrote: > > The calculations for sb/se/db/de all have a term which > > multiplies by (width + pitch), which makes me suspect > > they also need a similar fix ? > > Maybe. I'll have to check again. Actually is there a simpler way to check > if two rectangles overlap when they are given with base, x, y, w, h, pitch > where base is the first byte of the screen, pitch is length of one line > and x,y is coordinates of top left corner and w,h is dimensions of the > rect. Now that I think about it we also need to take into accounf the > bytes per pixel value (1 << format) because base is given in bytes while > others are in pixels so these formulae likely need some fixes. Pixman has > some functions for these but those assume common base so to use those we > would need to bring the two rectangles to common base which I could not > find out how to do. Probably this is really simple for someone who already > did a lot of these before. I think the thing that makes it particularly awkward is that the source and dest can have different pitches. That means it's not a simple "do two rectangles overlap" test because the dest area might not be a rectangle at all when looked at from the POV of the source. Do guests usually set src and dst pitch identical? If so it might be worth having a more accurate rectangle-overlap test for the common case and a looser check for the hard-to-handle case. I might have a think about this and draw some diagrams tomorrow :-) thanks -- PMM
On Mon, 15 Jun 2020, Peter Maydell wrote: > On Mon, 15 Jun 2020 at 17:40, BALATON Zoltan <balaton@eik.bme.hu> wrote: >> >> On Mon, 15 Jun 2020, Peter Maydell wrote: >>> The calculations for sb/se/db/de all have a term which >>> multiplies by (width + pitch), which makes me suspect >>> they also need a similar fix ? >> >> Maybe. I'll have to check again. Actually is there a simpler way to check >> if two rectangles overlap when they are given with base, x, y, w, h, pitch >> where base is the first byte of the screen, pitch is length of one line >> and x,y is coordinates of top left corner and w,h is dimensions of the >> rect. Now that I think about it we also need to take into accounf the >> bytes per pixel value (1 << format) because base is given in bytes while >> others are in pixels so these formulae likely need some fixes. Pixman has >> some functions for these but those assume common base so to use those we >> would need to bring the two rectangles to common base which I could not >> find out how to do. Probably this is really simple for someone who already >> did a lot of these before. > > I think the thing that makes it particularly awkward is that > the source and dest can have different pitches. That means it's Yes, different bases and different pitches make it difficult to compare and also to transform them to a common base to be able to do rectangle overlap test (I haven't checked but maybe the pitches could be relative prime so it's possible there's no common pitch to transform them to a single view). > not a simple "do two rectangles overlap" test because the dest > area might not be a rectangle at all when looked at from the > POV of the source. > > Do guests usually set src and dst pitch identical? If so it > might be worth having a more accurate rectangle-overlap test > for the common case and a looser check for the hard-to-handle > case. Quick test (just booted AmigaOS, opened some windows and moved them around a bit) shows same pitch is common but not always: 7610 sm501 2d engine regs : write addr=10, val=200020 4249 sm501 2d engine regs : write addr=10, val=4000400 2340 sm501 2d engine regs : write addr=10, val=4000080 462 sm501 2d engine regs : write addr=10, val=40000a0 453 sm501 2d engine regs : write addr=10, val=800080 434 sm501 2d engine regs : write addr=10, val=1200120 233 sm501 2d engine regs : write addr=10, val=4000020 162 sm501 2d engine regs : write addr=10, val=400040 144 sm501 2d engine regs : write addr=10, val=200080 141 sm501 2d engine regs : write addr=10, val=2000a0 75 sm501 2d engine regs : write addr=10, val=600060 62 sm501 2d engine regs : write addr=10, val=a000a0 62 sm501 2d engine regs : write addr=10, val=200400 38 sm501 2d engine regs : write addr=10, val=e000e0 38 sm501 2d engine regs : write addr=10, val=1000100 38 sm501 2d engine regs : write addr=10, val=10000a0 37 sm501 2d engine regs : write addr=10, val=4000040 36 sm501 2d engine regs : write addr=10, val=40000e0 34 sm501 2d engine regs : write addr=10, val=a00080 34 sm501 2d engine regs : write addr=10, val=4000120 33 sm501 2d engine regs : write addr=10, val=400080 25 sm501 2d engine regs : write addr=10, val=600080 23 sm501 2d engine regs : write addr=10, val=4000060 22 sm501 2d engine regs : write addr=10, val=1a001a0 and may depend on what the guest is doing so not sure it's worth the effort. (Probably same pitch is used when moving window and different when drawing something like text or icons but different pitch likely uses off-screen bitmap as source so overlap is not likely for those.) > I might have a think about this and draw some diagrams tomorrow :-) I'll wait for that to see if you can come up with something clever. I've tried to find a solution but could not come up with anything simpler than a loop checking lines which is probably as much effort as doing the blit itself and then doing blit with a temporary is probably the same so I went for just checking the obviously far apart blits (and even that seems to be easy to miss). So I'm happy if we can just fix the checks we have but if there are better suggestions please tell me. Regards, BALATON Zoltan
diff --git a/hw/display/sm501.c b/hw/display/sm501.c index edd8d24a76..5ae320ddc3 100644 --- a/hw/display/sm501.c +++ b/hw/display/sm501.c @@ -723,8 +723,8 @@ static void sm501_2d_operation(SM501State *s) dst_y -= height - 1; } - if (dst_base >= get_local_mem_size(s) || dst_base + - (dst_x + width + (dst_y + height) * (dst_pitch + width)) * + if (dst_base >= get_local_mem_size(s) || + dst_base + (dst_x + width + (dst_y + height) * dst_pitch) * (1 << format) >= get_local_mem_size(s)) { qemu_log_mask(LOG_GUEST_ERROR, "sm501: 2D op dest is outside vram.\n"); return; @@ -749,8 +749,8 @@ static void sm501_2d_operation(SM501State *s) src_y -= height - 1; } - if (src_base >= get_local_mem_size(s) || src_base + - (src_x + width + (src_y + height) * (src_pitch + width)) * + if (src_base >= get_local_mem_size(s) || + src_base + (src_x + width + (src_y + height) * src_pitch) * (1 << format) >= get_local_mem_size(s)) { qemu_log_mask(LOG_GUEST_ERROR, "sm501: 2D op src is outside vram.\n");
We don't need to add width to pitch when calculating last point, that would reject valid ops within the card's local_mem. Fixes: b15a22bbcbe6a78dc3d88fe3134985e4cdd87de4 Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> --- hw/display/sm501.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)