[Mesa-dev] [PATCH 2/2] i965: return the fourcc saved in __DRIimage

[James Xiong]

On Thu, 5 Apr 2018 14:30:02 +0300
Tapani Pälli <tapani.palli at intel.com> wrote:

> On 04/05/2018 02:51 AM, James Xiong wrote:
> > From: "Xiong, James" <james.xiong at intel.com>
> > 
> > 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 <james.xiong at intel.com>
> > ---
> >   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: "<fourcc>, <num_planes> and <modifiers> 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);