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

[Weng Chuanbo]

Hi Emil,

               (Seems something wrong with my company's email, so I use
this one)

               I understand your meaning except the NULL check part. Please
see my questions below.



-----Original Message-----

From: mesa-dev [mailto:mesa-dev-bounces at lists.freedesktop.org
<mesa-dev-bounces at lists.freedesktop.org>] On Behalf Of Emil Velikov

Sent: Wednesday, August 31, 2016 1:57 AM

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

[****Chuanbo****] Could you explain " we want these NULL checks split out
and ported to older loader " more detailed?

And what's older loaders? What's newer dri modules?

>From my understanding, the only path in mesa code invokes mapImage is
gbm_dri_bo_map. So if apply my patch code above, no NULL deref will happen.



>

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

mesa-dev mailing list

mesa-dev at lists.freedesktop.org

https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160906/abea22ee/attachment-0001.html>