On 04/27/20 13:11, Gerd Hoffmann wrote:
>>> - size = (hwaddr)linesize * height;
>>> - data = cpu_physical_memory_map(addr, &size, false);
>>> - if (size != (hwaddr)linesize * height) {
>>> - cpu_physical_memory_unmap(data, size, 0, 0);
>>> + mapsize = size = stride * (height - 1) + linesize;
>>> + data = cpu_physical_memory_map(addr, &mapsize, false);
>>> + if (size != mapsize) {
>>> + cpu_physical_memory_unmap(data, mapsize, 0, 0);
>>> return NULL;
>>> }
>>>
>>> surface = qemu_create_displaysurface_from(width, height,
>>> - format, linesize, data);
>>> + format, stride, data);
>>> pixman_image_set_destroy_function(surface->image,
>>> ramfb_unmap_display_surface, NULL);
>>>
>>>
>>
>> I don't understand two things about this patch:
>>
>> - "stride" can still be smaller than "linesize" (scanlines can still
>> overlap). Why is that not a problem?
>
> Why it should be? It is the guests choice. Not a very useful one, but
> hey, if the guest prefers it that way we are at your service ...
>
> We only must make sure our size calculations are correct. The patch
> does that. I think we can also outlaw stride < linesize if you are
> happier with that alternative approach. I doubt we have any guests
> relying on this working.
OK, thanks. I agree -- if it doesn't break QEMU, then we can let guests
break themselves.
>
>> - assuming "stride > linesize" (i.e., strictly larger), we don't seem to
>> map the last complete stride, and that seems to be intentional. Is that
>> safe with regard to qemu_create_displaysurface_from()? Ultimately the
>> stride is passed to pixman_image_create_bits(), and the underlying
>> "data" may not be large enough for that. What am I missing?
>
> Lets take a real-world example. Wayland rounds up width and height to
> multiples of 64 (probably for tiling on modern GPUs). So with 800x600
> you get an allocation of 832x640, like this:
>
> ###########** <- y 0
> ###########**
> ###########**
> ###########**
> ###########** <- y 600
> ************* <- y 640
>
> ^ ^ ^----- x 832
> | +------- x 800
> +----------------- x 0
>
> where "#" is image data and "*" is unused padding space. Pixman will
> access all "#", so we are mapping the region from the first "#" to the
> last "#", including the unused padding on each scanline, except for the
> last scanline. Any unused scanlines at the bottom are excluded too
> (ramfb doesn't even know whenever they exist).
>
> The unused padding is only mapped because it is the easiest way to
> handle things, not because we need it. Also the padding is typically
> *alot* smaller than PAGE_SIZE, so we couldn't exclude it from the
> mapping even if we would like to ;)
OK. If pixman only accesses the "#" marks, then it should be OK.
>
>> Hm... bonus question: qemu_create_displaysurface_from() still accepts
>> "linesize" as a signed int. (Not sure about pixman_image_create_bits().)
>> Should we do something specific to prevent that value from being
>> negative? The guest gives us a uint32_t.
>
> Not fully sure we can do that without breaking something, might be a
> negative stride is used for upside down images (last scanline comes
> first in memory).
Ugh... Upside down images???... Well, OK, I guess. :)
For the followup patch:
Acked-by: Laszlo Ersek <ler...@redhat.com>
Laszlo
