[Mesa-dev] [PATCH v2] egl: return corresponding offset of EGLImage instead of 0.

[Emil Velikov]

On 30 August 2016 at 18:39, Weng, Chuanbo <chuanbo.weng at intel.com> wrote:
> Hi Emil,
>         Thanks for your review!
>         Please see my comments  and questions below.
>
> -----Original Message-----
> From: mesa-dev [mailto:mesa-dev-bounces at lists.freedesktop.org] On Behalf Of Emil Velikov
> Sent: Tuesday, August 30, 2016 6:29 PM
> To: Weng, Chuanbo <chuanbo.weng at intel.com>
> Cc: ML mesa-dev <mesa-dev at lists.freedesktop.org>
> Subject: Re: [Mesa-dev] [PATCH v2] egl: return corresponding offset of EGLImage instead of 0.
>
> On 25 August 2016 at 18:50, Chuanbo Weng <chuanbo.weng at intel.com> wrote:
>> The offset should not always be 0. For example, if EGLImage is created
>> from a 2D texture with EGL_GL_TEXTURE_LEVEL=1, then the offset should
>> be the actual start of miplevel 1 in drm bo.
>>
>> v2: version bump on the EGL image interface and add gallium pieces.
>>
>> Signed-off-by: Chuanbo Weng <chuanbo.weng at intel.com>
>> ---
>>  include/GL/internal/dri_interface.h      | 4 +++-
>>  src/egl/drivers/dri2/egl_dri2.c          | 3 ++-
>>  src/gallium/state_trackers/dri/dri2.c    | 8 +++++++-
>>  src/gbm/backends/dri/gbm_dri.c           | 5 +++--
>>  src/mesa/drivers/dri/i965/intel_screen.c | 9 +++++++--
> I'm likely the only one here, but I'd suggest splitting things up:
> - introduce flag - include/
> - wire up loaders, one at a time - src/{egl,gbm}
> - implement in driver, one at a time - src/{mesa,gallium}
> [****Chuanbo****] I'm not sure whether I understand correctly. Do you mean that I should split this patch into
> three patches as you suggested?
>
If you want to go the extreme - five (if you implement the gallium
bits), three otherwise. Before doing anything I'd stick around for a
second for others to voice their preference [against].


>> -   if (!dri->image || dri->image->base.version < 12) {
>> +   if (!dri->image || dri->image->base.version < 12 ||
>> + !dri->image->mapImage) {
> Hmm we want these NULL checks split out and ported to older loader(s).
> Otherwise we will get NULL deref when using older loader + newer dri module(s).
> [****Chuanbo****] So we just need to leave it as original?
>
Split it into a preparatory patch, adding the following tag.
CC: <mesa-stable at lists.freedesktop.org>

>
>> --- a/src/mesa/drivers/dri/i965/intel_screen.c
>> +++ b/src/mesa/drivers/dri/i965/intel_screen.c
>
>> +    .mapImage                           = NULL,
>> +    .unmapImage                         = NULL,
> Consider looking into/implementing the map/unmap interfaces. IIRC there was an implementation (slightly* different one) a while back [1].
> You really want this if you're thinking for Android and/or CrOS ARC + a better gralloc [2].
> [****Chuanbo****] Is it required to implement map/unmap interfaces? Since I'm not interested in Android and/or CrOS ARC + a better gralloc.
> And I find that blitImage and getCapabilities are not implemented when bump verison. So I don’t think it's required to implement map/unmap
> interface.
>
You don't have to, thus the "consider". Just trying to get someone
from Intel side excited/involved into this.

-Emil

/me wishes that MS implements a client which can do proper quoting or
people move to one that can
</rant>