Re: [Mesa-dev] [PATCH 2/2] i965: return the fourcc saved in __DRIimage
On Thu, 5 Apr 2018 20:57:46 +0300
Tapani Pälli wrote:
> On 05.04.2018 18:43, James Xiong wrote:
> > On Thu, 5 Apr 2018 14:30:02 +0300
> > Tapani Pälli wrote:
> >
> >> On 04/05/2018 02:51 AM, James Xiong wrote:
> >>> From: "Xiong, James"
> >>>
> >>> The planar_format in __DRIimage contains the original fourcc
> >>> used to create the image, if it's set, return the saved fourcc
> >>> directly; Otherwise fall back to the old way.
> >>>
> >>> Also we should validate the input parameter "value" first as it
> >>> might be NULL based on the SPEC.
> >>>
> >>> v2: fall back to intel_lookup_fourcc() when planar_format is NULL
> >>> (by Dongwon & Matt Roper)
> >>>
> >>> Signed-off-by: Xiong, James
> >>> ---
> >>>src/mesa/drivers/dri/i965/intel_screen.c | 15 ---
> >>>1 file changed, 12 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/src/mesa/drivers/dri/i965/intel_screen.c
> >>> b/src/mesa/drivers/dri/i965/intel_screen.c index 7df8bc4..aeecef3
> >>> 100644 --- a/src/mesa/drivers/dri/i965/intel_screen.c
> >>> +++ b/src/mesa/drivers/dri/i965/intel_screen.c
> >>> @@ -388,10 +388,16 @@ intel_image_format_lookup(int fourcc)
> >>> return NULL;
> >>>}
> >>>
> >>> -static boolean intel_lookup_fourcc(int dri_format, int *fourcc)
> >>> +static boolean
> >>> +intel_image_get_fourcc(__DRIimage *image, int *fourcc)
> >>>{
> >>> + if (image->planar_format) {
> >>> + *fourcc = image->planar_format->fourcc;
> >>> + return true;
> >>> + }
> >>> +
> >>> for (unsigned i = 0; i < ARRAY_SIZE(intel_image_formats);
> >>> i++) {
> >>> - if (intel_image_formats[i].planes[0].dri_format ==
> >>> dri_format) {
> >>> + if (intel_image_formats[i].planes[0].dri_format ==
> >>> image->dri_format) { *fourcc = intel_image_formats[i].fourcc;
> >>> return true;
> >>> }
> >>> @@ -844,6 +850,9 @@ intel_create_image_with_modifiers(__DRIscreen
> >>> *dri_screen, static GLboolean
> >>>intel_query_image(__DRIimage *image, int attrib, int *value)
> >>>{
> >>> + if (value == NULL)
> >>> + return false;
> >>> +
> >>
> >> I would remove this check, we've been fine many years without it.
> > The function spec does say: ", and
> > may be NULL, in which case no value is retrieved."
> > it's better to stick to it and have an extra check than
> > segmentation fault, what do you say?
>
> Function modified here 'intel_query_image' is part of DRI interface
> (dri_interface.h) with no such spec, it is used from many existing
> places and none of them seem to be passing NULL.
>
> Now that I know you are using eglExportDMABUFImageMESA, I can also
> see that dri2_export_dma_buf_image_mesa implementation does not call
> this API when 'fds', 'strides' or 'offsets' is NULL, it checks this
> in the implementation before calling.
>
You are right it's already been checked in the upper level. I will
remove it.
>
> >>
> >>> switch (attrib) {
> >>> case __DRI_IMAGE_ATTRIB_STRIDE:
> >>> *value = image->pitch;
> >>> @@ -870,7 +879,7 @@ intel_query_image(__DRIimage *image, int
> >>> attrib, int *value) case __DRI_IMAGE_ATTRIB_FD:
> >>> return !brw_bo_gem_export_to_prime(image->bo, value);
> >>> case __DRI_IMAGE_ATTRIB_FOURCC:
> >>> - return intel_lookup_fourcc(image->dri_format, value);
> >>> + return intel_image_get_fourcc(image, value);
> >>> case __DRI_IMAGE_ATTRIB_NUM_PLANES:
> >>> if (isl_drm_modifier_has_aux(image->modifier)) {
> >>> assert(!image->planar_format ||
> >>> image->planar_format->nplanes == 1);
> >
>
> // Tapani
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/2] i965: return the fourcc saved in __DRIimage
On 05.04.2018 18:43, James Xiong wrote:
On Thu, 5 Apr 2018 14:30:02 +0300
Tapani Pälli wrote:
On 04/05/2018 02:51 AM, James Xiong wrote:
From: "Xiong, James"
The planar_format in __DRIimage contains the original fourcc
used to create the image, if it's set, return the saved fourcc
directly; Otherwise fall back to the old way.
Also we should validate the input parameter "value" first as it
might be NULL based on the SPEC.
v2: fall back to intel_lookup_fourcc() when planar_format is NULL
(by Dongwon & Matt Roper)
Signed-off-by: Xiong, James
---
src/mesa/drivers/dri/i965/intel_screen.c | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/intel_screen.c
b/src/mesa/drivers/dri/i965/intel_screen.c index 7df8bc4..aeecef3
100644 --- a/src/mesa/drivers/dri/i965/intel_screen.c
+++ b/src/mesa/drivers/dri/i965/intel_screen.c
@@ -388,10 +388,16 @@ intel_image_format_lookup(int fourcc)
return NULL;
}
-static boolean intel_lookup_fourcc(int dri_format, int *fourcc)
+static boolean
+intel_image_get_fourcc(__DRIimage *image, int *fourcc)
{
+ if (image->planar_format) {
+ *fourcc = image->planar_format->fourcc;
+ return true;
+ }
+
for (unsigned i = 0; i < ARRAY_SIZE(intel_image_formats); i++)
{
- if (intel_image_formats[i].planes[0].dri_format ==
dri_format) {
+ if (intel_image_formats[i].planes[0].dri_format ==
image->dri_format) { *fourcc = intel_image_formats[i].fourcc;
return true;
}
@@ -844,6 +850,9 @@ intel_create_image_with_modifiers(__DRIscreen
*dri_screen, static GLboolean
intel_query_image(__DRIimage *image, int attrib, int *value)
{
+ if (value == NULL)
+ return false;
+
I would remove this check, we've been fine many years without it.
The function spec does say: ", and may
be NULL, in which case no value is retrieved."
it's better to stick to it and have an extra check than
segmentation fault, what do you say?
Function modified here 'intel_query_image' is part of DRI interface
(dri_interface.h) with no such spec, it is used from many existing
places and none of them seem to be passing NULL.
Now that I know you are using eglExportDMABUFImageMESA, I can also see
that dri2_export_dma_buf_image_mesa implementation does not call this
API when 'fds', 'strides' or 'offsets' is NULL, it checks this in the
implementation before calling.
switch (attrib) {
case __DRI_IMAGE_ATTRIB_STRIDE:
*value = image->pitch;
@@ -870,7 +879,7 @@ intel_query_image(__DRIimage *image, int
attrib, int *value) case __DRI_IMAGE_ATTRIB_FD:
return !brw_bo_gem_export_to_prime(image->bo, value);
case __DRI_IMAGE_ATTRIB_FOURCC:
- return intel_lookup_fourcc(image->dri_format, value);
+ return intel_image_get_fourcc(image, value);
case __DRI_IMAGE_ATTRIB_NUM_PLANES:
if (isl_drm_modifier_has_aux(image->modifier)) {
assert(!image->planar_format ||
image->planar_format->nplanes == 1);
// Tapani
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/2] i965: return the fourcc saved in __DRIimage
On Thu, 5 Apr 2018 14:30:02 +0300
Tapani Pälli wrote:
> On 04/05/2018 02:51 AM, James Xiong wrote:
> > From: "Xiong, James"
> >
> > The planar_format in __DRIimage contains the original fourcc
> > used to create the image, if it's set, return the saved fourcc
> > directly; Otherwise fall back to the old way.
> >
> > Also we should validate the input parameter "value" first as it
> > might be NULL based on the SPEC.
> >
> > v2: fall back to intel_lookup_fourcc() when planar_format is NULL
> >(by Dongwon & Matt Roper)
> >
> > Signed-off-by: Xiong, James
> > ---
> > src/mesa/drivers/dri/i965/intel_screen.c | 15 ---
> > 1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/intel_screen.c
> > b/src/mesa/drivers/dri/i965/intel_screen.c index 7df8bc4..aeecef3
> > 100644 --- a/src/mesa/drivers/dri/i965/intel_screen.c
> > +++ b/src/mesa/drivers/dri/i965/intel_screen.c
> > @@ -388,10 +388,16 @@ intel_image_format_lookup(int fourcc)
> > return NULL;
> > }
> >
> > -static boolean intel_lookup_fourcc(int dri_format, int *fourcc)
> > +static boolean
> > +intel_image_get_fourcc(__DRIimage *image, int *fourcc)
> > {
> > + if (image->planar_format) {
> > + *fourcc = image->planar_format->fourcc;
> > + return true;
> > + }
> > +
> > for (unsigned i = 0; i < ARRAY_SIZE(intel_image_formats); i++)
> > {
> > - if (intel_image_formats[i].planes[0].dri_format ==
> > dri_format) {
> > + if (intel_image_formats[i].planes[0].dri_format ==
> > image->dri_format) { *fourcc = intel_image_formats[i].fourcc;
> >return true;
> > }
> > @@ -844,6 +850,9 @@ intel_create_image_with_modifiers(__DRIscreen
> > *dri_screen, static GLboolean
> > intel_query_image(__DRIimage *image, int attrib, int *value)
> > {
> > + if (value == NULL)
> > + return false;
> > +
>
> I would remove this check, we've been fine many years without it.
The function spec does say: ", and may
be NULL, in which case no value is retrieved."
it's better to stick to it and have an extra check than
segmentation fault, what do you say?
>
> > switch (attrib) {
> > case __DRI_IMAGE_ATTRIB_STRIDE:
> > *value = image->pitch;
> > @@ -870,7 +879,7 @@ intel_query_image(__DRIimage *image, int
> > attrib, int *value) case __DRI_IMAGE_ATTRIB_FD:
> > return !brw_bo_gem_export_to_prime(image->bo, value);
> > case __DRI_IMAGE_ATTRIB_FOURCC:
> > - return intel_lookup_fourcc(image->dri_format, value);
> > + return intel_image_get_fourcc(image, value);
> > case __DRI_IMAGE_ATTRIB_NUM_PLANES:
> > if (isl_drm_modifier_has_aux(image->modifier)) {
> >assert(!image->planar_format ||
> > image->planar_format->nplanes == 1);
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/2] i965: return the fourcc saved in __DRIimage
On 04/05/2018 02:51 AM, James Xiong wrote:
From: "Xiong, James"
The planar_format in __DRIimage contains the original fourcc
used to create the image, if it's set, return the saved fourcc
directly; Otherwise fall back to the old way.
Also we should validate the input parameter "value" first as it
might be NULL based on the SPEC.
v2: fall back to intel_lookup_fourcc() when planar_format is NULL
(by Dongwon & Matt Roper)
Signed-off-by: Xiong, James
---
src/mesa/drivers/dri/i965/intel_screen.c | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/intel_screen.c
b/src/mesa/drivers/dri/i965/intel_screen.c
index 7df8bc4..aeecef3 100644
--- a/src/mesa/drivers/dri/i965/intel_screen.c
+++ b/src/mesa/drivers/dri/i965/intel_screen.c
@@ -388,10 +388,16 @@ intel_image_format_lookup(int fourcc)
return NULL;
}
-static boolean intel_lookup_fourcc(int dri_format, int *fourcc)
+static boolean
+intel_image_get_fourcc(__DRIimage *image, int *fourcc)
{
+ if (image->planar_format) {
+ *fourcc = image->planar_format->fourcc;
+ return true;
+ }
+
for (unsigned i = 0; i < ARRAY_SIZE(intel_image_formats); i++) {
- if (intel_image_formats[i].planes[0].dri_format == dri_format) {
+ if (intel_image_formats[i].planes[0].dri_format == image->dri_format) {
*fourcc = intel_image_formats[i].fourcc;
return true;
}
@@ -844,6 +850,9 @@ intel_create_image_with_modifiers(__DRIscreen *dri_screen,
static GLboolean
intel_query_image(__DRIimage *image, int attrib, int *value)
{
+ if (value == NULL)
+ return false;
+
I would remove this check, we've been fine many years without it.
switch (attrib) {
case __DRI_IMAGE_ATTRIB_STRIDE:
*value = image->pitch;
@@ -870,7 +879,7 @@ intel_query_image(__DRIimage *image, int attrib, int *value)
case __DRI_IMAGE_ATTRIB_FD:
return !brw_bo_gem_export_to_prime(image->bo, value);
case __DRI_IMAGE_ATTRIB_FOURCC:
- return intel_lookup_fourcc(image->dri_format, value);
+ return intel_image_get_fourcc(image, value);
case __DRI_IMAGE_ATTRIB_NUM_PLANES:
if (isl_drm_modifier_has_aux(image->modifier)) {
assert(!image->planar_format || image->planar_format->nplanes == 1);
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 2/2] i965: return the fourcc saved in __DRIimage
From: "Xiong, James"
The planar_format in __DRIimage contains the original fourcc
used to create the image, if it's set, return the saved fourcc
directly; Otherwise fall back to the old way.
Also we should validate the input parameter "value" first as it
might be NULL based on the SPEC.
v2: fall back to intel_lookup_fourcc() when planar_format is NULL
(by Dongwon & Matt Roper)
Signed-off-by: Xiong, James
---
src/mesa/drivers/dri/i965/intel_screen.c | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/intel_screen.c
b/src/mesa/drivers/dri/i965/intel_screen.c
index 7df8bc4..aeecef3 100644
--- a/src/mesa/drivers/dri/i965/intel_screen.c
+++ b/src/mesa/drivers/dri/i965/intel_screen.c
@@ -388,10 +388,16 @@ intel_image_format_lookup(int fourcc)
return NULL;
}
-static boolean intel_lookup_fourcc(int dri_format, int *fourcc)
+static boolean
+intel_image_get_fourcc(__DRIimage *image, int *fourcc)
{
+ if (image->planar_format) {
+ *fourcc = image->planar_format->fourcc;
+ return true;
+ }
+
for (unsigned i = 0; i < ARRAY_SIZE(intel_image_formats); i++) {
- if (intel_image_formats[i].planes[0].dri_format == dri_format) {
+ if (intel_image_formats[i].planes[0].dri_format == image->dri_format) {
*fourcc = intel_image_formats[i].fourcc;
return true;
}
@@ -844,6 +850,9 @@ intel_create_image_with_modifiers(__DRIscreen *dri_screen,
static GLboolean
intel_query_image(__DRIimage *image, int attrib, int *value)
{
+ if (value == NULL)
+ return false;
+
switch (attrib) {
case __DRI_IMAGE_ATTRIB_STRIDE:
*value = image->pitch;
@@ -870,7 +879,7 @@ intel_query_image(__DRIimage *image, int attrib, int *value)
case __DRI_IMAGE_ATTRIB_FD:
return !brw_bo_gem_export_to_prime(image->bo, value);
case __DRI_IMAGE_ATTRIB_FOURCC:
- return intel_lookup_fourcc(image->dri_format, value);
+ return intel_image_get_fourcc(image, value);
case __DRI_IMAGE_ATTRIB_NUM_PLANES:
if (isl_drm_modifier_has_aux(image->modifier)) {
assert(!image->planar_format || image->planar_format->nplanes == 1);
--
2.7.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
