[Mesa-dev] [PATCH 1/3] egl/android: Delete set_damage_region from egl dri vtbl

[Eric Anholt]

Harish Krupo <harish.krupo.kps at intel.com> writes:

> Eric Anholt <eric at anholt.net> writes:
>
>> Harish Krupo <harish.krupo.kps at intel.com> writes:
>>
>>> Eric Anholt <eric at anholt.net> writes:
>>>
>>>> Harish Krupo <harish.krupo.kps at intel.com> writes:
>>>>
>>>>> Hi Eric,
>>>>>
>>>>> Eric Anholt <eric at anholt.net> writes:
>>>>>
>>>>>> Harish Krupo <harish.krupo.kps at intel.com> writes:
>>>>>>
>>>>>>> The intension of the KHR_partial_update was not to send the damage back
>>>>>>> to the platform but to send the damage to the driver to ensure that the
>>>>>>> following rendering could be restricted to those regions.
>>>>>>> This patch removes the set_damage_region from the egl_dri vtbl and all
>>>>>>> the platfrom_*.c files.
>>>>>>> Then upcomming patches add a new dri2 interface for the drivers to
>>>>>>> implement
>>>>>>>
>>>>>>> Signed-off-by: Harish Krupo <harish.krupo.kps at intel.com>
>>>>>>
>>>>>> Why shouldn't the platform know about the damage region in a swap, if
>>>>>> it's available?  It looks like it was successfully used for Android, and
>>>>>> we should be using it for Present as well.
>>>>>
>>>>> From the spec [1], the damage region referred to by partial_update spec is
>>>>> the damaged part of the buffer when it is used again. The damage that the
>>>>> compositor/platform needs to know is the damage between the (n-1)th
>>>>> frame and the nth frame. Quoting from the spec:
>>>>> "   The surface damage for frame n is the difference between frame n and frame
>>>>>     (n-1), and represents the area that a compositor must recompose."
>>>>> This is the damage referred to by the swap_buffers_with_damage spec [2],
>>>>> whereas the partial_update damage region's objective is to restrict the subsequent
>>>>> rendering operations on the back buffer, to only those regions which have changed since
>>>>> that buffer was last used. This information is available as the buffer
>>>>> age. Some more information: [3].
>>>>
>>>> OK, let's document that in the new internal API you're adding then.
>>>> Things I'd want to know as an implementer of the hook:
>>>>
>>>> 1) Am I guaranteed that it's called before the frame is started?
>>>>
>>>
>>> No. When no damage region is set, the whole surface should be considered
>>> damaged. As a matter of fact, the damage region is set to full surface
>>> when the frame boundary is reached (i.e. swapbuffersXXX is called).
>>
>> The spec citation I was looking for was:
>>
>>     If any client API commands resulting in rendering to <surface> have been
>>     issued since eglSwapBuffers was last called with <surface>, or since the
>>     surface was created in case eglSwapBuffers has not yet been called on it,
>>     attempting to set the damage region will result in undefined framebuffer
>>     contents for the entire framebuffer.
>>
>> So, the driver should expect the partial damage region to be set before
>> any rendering has happened, and doesn't need to worry about doing things
>> right if it shows up later.
>>
>>>> 2) Is the behavior if the client draws outside of the partial update
>>>> damage region defined?  (is it "the driver must not change pixels
>>>> outside of the partial region" or "the driver might not change pixels
>>>> outside of the partial region")
>>>>
>>>
>>> If I have understood the spec correctly, then the damage regions set are
>>> a hint to the driver so that it can optimize the rendering by
>>> restricting the client's drawing commands to only the damaged region.
>>> In the current implementation, although the damage regions are sent back
>>> to the compositor instead of sending it to the driver, no issues are
>>> observed with the rendered output and it passes deqp tests. This
>>> supports the argument that the damages are only a hint.
>>
>> I was looking for documentation of this part of the spec in the method's
>> comment:
>>
>>     At all times, any client API rendering which falls outside of the damage
>>     region results in undefined framebuffer contents for the entire framebuffer.
>>     It is the client's responsibility to ensure that rendering is confined to
>>     the current damage area.
>>
>>>> 3) Is the client guaranteed to fully initialize pixels in the partial
>>>> update region, or might it depend on previous contents?
>>>
>>> If the above argument is right then it means that the client would
>>> actually initialize the pixels of the full buffer but expect that the
>>> driver renders only the damaged regions.
>>
>> The client can't initialize the full buffer, because of the spec quote
>> above.
>>
>> The actual relevant spec quote for this is:
>>
>>     If EGL_EXT_buffer_age is supported, the contents of the buffer inside the
>>     damage region may also be relied upon to contain the same content as the
>>     last time they were defined for the current back buffer.
>
> Thank you, understood it. I should have read the spec better :(.
> Also, generalizing Android/deqp's usage seems to be wrong. Android's
> deqp passed previously even when the driver wasn't restricting the
> rendering to only the damaged regions.
> Should I update these in the comments section of the extension?

Yes, please.  If we're building an API, we should be documenting what it
does.

>>     It is the client's responsibility to ensure that rendering is confined to
>>     the current damage area.
> quick question: How can the client ensure this?

Emit draw commands that only touch pixels within the area.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180720/d80d00a7/attachment.sig>