From functionality's perspective, version check is not necessary. And I think the queryImage returns false if attribute is unsupported, so I prefer to not use version Check.
Thanks, Chuanbo Weng -----Original Message----- From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf Of Axel Davy Sent: Saturday, September 10, 2016 5:03 AM To: Weng, Chuanbo <chuanbo.w...@intel.com>; Emil Velikov <emil.l.veli...@gmail.com> Cc: ML mesa-dev <mesa-dev@lists.freedesktop.org> Subject: Re: [Mesa-dev] [PATCH v2 2/3] egl: return corresponding offset of EGLImage instead of 0. I'm not sure calling queryImage with an unsupported attribute is legal, thus I think a small check doesn't hurt. It'd give if (offsets) { offsets[0] = 0; if (dri2_dpy->image->base.version >= 13) { EGLint img_offset = 0; bool ret = dri2_dpy->image->queryImage(dri2_img->dri_image, __DRI_IMAGE_ATTRIB_OFFSET, &img_offset); if (ret) offsets[0] = img_offset; } } return EGL_TRUE; or alternatively, if you think not being able to give the offset indicates an error, if (offsets) { offsets[0] = 0; if (dri2_dpy->image->base.version >= 13) { EGLint img_offset = 0; bool ret = dri2_dpy->image->queryImage(dri2_img->dri_image, __DRI_IMAGE_ATTRIB_OFFSET, &img_offset); if (!ret) return EGL_FALSE; offsets[0] = img_offset; } } return EGL_TRUE; Axel Davy On 09/09/2016 17:34, Weng, Chuanbo wrote: > The comments from Emil can be summarized into the following code: > > if (offsets) { > offsets[0] = 0; > EGLint img_offset = 0; > bool ret = dri2_dpy->image->queryImage(dri2_img->dri_image, > __DRI_IMAGE_ATTRIB_OFFSET, &img_offset); > if(ret == true) > offsets[0] = img_offset; > } > > return EGL_TRUE; > > Emil, is it right? > > -----Original Message----- > From: Emil Velikov [mailto:emil.l.veli...@gmail.com] > Sent: Friday, September 9, 2016 8:32 PM > To: Weng, Chuanbo <chuanbo.w...@intel.com> > Cc: ML mesa-dev <mesa-dev@lists.freedesktop.org>; Axel Davy > <axel.d...@ens.fr> > Subject: Re: [PATCH v2 2/3] egl: return corresponding offset of EGLImage > instead of 0. > > On 9 September 2016 at 08:58, 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 bo. >> >> v2: Add version check of __DRIimageExtension implementation >> (Suggested by Axel Davy). >> > Just to elaborate what Axel was saying from another perspective: > > With current master the offset is explicitly zeroed, while with the > (v2) patch you'll _only_ set the value when you have v13 driver. > Thus using the loader + v12 driver you'll get a regression since a) > the offset will remain garbage and b) the function > (dri2_export_dma_buf_image_mesa) will fail. > > >> Signed-off-by: Chuanbo Weng <chuanbo.w...@intel.com> >> --- >> src/egl/drivers/dri2/egl_dri2.c | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/src/egl/drivers/dri2/egl_dri2.c >> b/src/egl/drivers/dri2/egl_dri2.c index 859612f..c529e55 100644 >> --- a/src/egl/drivers/dri2/egl_dri2.c >> +++ b/src/egl/drivers/dri2/egl_dri2.c >> @@ -2249,6 +2249,7 @@ dri2_export_dma_buf_image_mesa(_EGLDriver *drv, >> _EGLDisplay *disp, _EGLImage *im >> struct dri2_egl_image *dri2_img = dri2_egl_image(img); >> >> (void) drv; >> + EGLint img_offset = 0; >> >> /* rework later to provide multiple fds/strides/offsets */ >> if (fds) >> @@ -2259,8 +2260,14 @@ dri2_export_dma_buf_image_mesa(_EGLDriver *drv, >> _EGLDisplay *disp, _EGLImage *im >> dri2_dpy->image->queryImage(dri2_img->dri_image, >> __DRI_IMAGE_ATTRIB_STRIDE, >> strides); >> >> - if (offsets) >> + if (offsets){ >> offsets[0] = 0; >> + if(dri2_dpy->image->base.version >= 13){ > Please move the variable declaration in local scope and add space > between ){ > > Functionality wise we don't need the version check, esp. since you want to > set the offset only when queryImage() succeeds... > >> + dri2_dpy->image->queryImage(dri2_img->dri_image, >> + __DRI_IMAGE_ATTRIB_OFFSET, &img_offset); > ... which you don't seem to be checking here, so you'll effectively store/use > garbage. > > So unless Alex feels strongly for the version check I'd omit it, and fix the > rest of this patch. > > -Emil _______________________________________________ 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
