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-boun...@lists.freedesktop.org
<mesa-dev-boun...@lists.freedesktop.org>] On Behalf Of Emil Velikov
Sent: Wednesday, August 31, 2016 1:57 AM
To: Weng, Chuanbo <chuanbo.w...@intel.com>
Cc: ML mesa-dev <mesa-dev@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.w...@intel.com> wrote:
> Hi Emil,
> Thanks for your review!
> Please see my comments and questions below.
>
> -----Original Message-----
> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org
<mesa-dev-boun...@lists.freedesktop.org>] On
> Behalf Of Emil Velikov
> Sent: Tuesday, August 30, 2016 6:29 PM
> To: Weng, Chuanbo <chuanbo.w...@intel.com>
> Cc: ML mesa-dev <mesa-dev@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.w...@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.w...@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-sta...@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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
