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

[Harish Krupo]

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?

>     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?

Thank you
Regards
Harish Krupo