[Mesa-dev] [PATCH 3/3] gallium: push offset down to driver

[Emil Velikov]

On 17 May 2016 at 09:26, Stanimir Varbanov <stanimir.varbanov at linaro.org> wrote:
> Hi,
>
> On 05/17/2016 10:31 AM, Christian König wrote:
>> Am 16.05.2016 um 22:16 schrieb Rob Clark:
>>> On Mon, May 16, 2016 at 3:53 PM, Emil Velikov
>>> <emil.l.velikov at gmail.com> wrote:
>>>> On 15 May 2016 at 12:34, Stanimir Varbanov
>>>> <stanimir.varbanov at linaro.org> wrote:
>>>>> Push offset down to drivers when importing dmabuf. This is needed
>>>>> to more fully support EGL_EXT_image_dma_buf_import when a non-zero
>>>>> offset is specified.
>>>>>
>>>> How did you test this, on which hardware ? Please add such information
>>>> to the commit message.
>>>>
>>>> My knowledge in the area is far from perfect, although it seems that
>>>> you're pushing the offset check as opposed to the actual offset.
>>>> Afaics there is nothing in between that makes use of it (the offset)
>>>> so this patch should be mostly a no-op. See below why "mostly".
>>>>
>>>> Perhaps you meant to nuke the in-drivers offset checking, or there's
>>>> some more work needed in the area ?
>>> jfwiw, freedreno already handles the offset, egl just needs to pass it
>>> down.  This is why you aren't seeing a hunk for freedreno.
>>>
I somehow (mis)read the patch like it introduces the offset check for
freedreno as well. Oops.
Thanks for the correction.

>>> The checks to other drivers for offset!=0 would presumably be removed
>>> when they add support for non-zero offsets.  So IMO this change is
>>> correct, at least for the drivers the ignore the offset.
>>>
Having a closer look, I agree with you, barring the r600/radeonsi bug.
Although it should be cleaner if one simply pushes the != 0 check down
into the winsys 'layer'.
If that's too evasive atm, let's have a comment next to the include
(just as freedreno does it), please ?

Even though we are not interested in running freedreno on windows,
that does not make the separation a bad design, right ? One can
write/wire-up a remote simulator based on virgl's vtest approach (in
addition to the local simulators like the vc4 one).

Imho keeping all the 'skeletons in one closet' (yes, not the ideal
metaphor) is always nice ;-)


>
> My fault, I had to be more careful. Will look deeper in radeon and
> amdgpu as well as vdpau. Thanks for catching the mistake.
>
Don't worry about it. We all make mistakes, the review process helps
catch most of them :-)

-Emil