<div dir="ltr">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 [<a href="mailto:mesa-dev-bounces@lists.freedesktop.org">mailto:mesa-dev-bounces@lists.freedesktop.org</a>] On Behalf Of Emil Velikov Sent: Wednesday, August 31, 2016 1:57 AM To: Weng, Chuanbo <<a href="mailto:chuanbo.weng@intel.com">chuanbo.weng@intel.com</a>> Cc: ML mesa-dev <<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a>> Subject: Re: [Mesa-dev] [PATCH v2] egl: return corresponding offset of EGLImage instead of 0. On 30 August 2016 at 18:39, Weng, Chuanbo <<a href="mailto:chuanbo.weng@intel.com">chuanbo.weng@intel.com</a>> wrote: > Hi Emil, > Thanks for your review! > Please see my comments and questions below. > > -----Original Message----- > From: mesa-dev [<a href="mailto:mesa-dev-bounces@lists.freedesktop.org">mailto:mesa-dev-bounces@lists.freedesktop.org</a>] On > Behalf Of Emil Velikov > Sent: Tuesday, August 30, 2016 6:29 PM > To: Weng, Chuanbo <<a href="mailto:chuanbo.weng@intel.com">chuanbo.weng@intel.com</a>> > Cc: ML mesa-dev <<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a>> > Subject: Re: [Mesa-dev] [PATCH v2] egl: return corresponding offset of EGLImage instead of 0. > > On 25 August 2016 at 18:50, Chuanbo Weng <<a href="mailto:chuanbo.weng@intel.com">chuanbo.weng@intel.com</a>> 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 <<a href="mailto:chuanbo.weng@intel.com">chuanbo.weng@intel.com</a>> >> --- >> 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: <<a href="mailto:mesa-stable@lists.freedesktop.org">mesa-stable@lists.freedesktop.org</a>> [****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 <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a></div>