[Mesa-dev] [PATCH] i965: Fix bugs in intel_from_planar

[Daniel Stone]

From: Jason Ekstrand 

This commit fixes two bugs in intel_from_planar.  First, if the planar
format was non-NULL but only had a single plane, we were falling through
to the planar case.  If we had a CCS modifier and plane == 1, we would
return NULL instead of the CCS plane.  Second, if we did end up in the
planar_format == NULL case and the modifier was DRM_FORMAT_MOD_INVALID,
we would end up segfaulting in isl_drm_modifier_has_aux.

Cc: mesa-sta...@lists.freedesktop.org
Fixes: 8f6e54c92966bb94a3f05f2cc7ea804273e125ad
Signed-off-by: Daniel Stone 
---
 src/mesa/drivers/dri/i965/intel_screen.c | 56 +---
 1 file changed, 29 insertions(+), 27 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/intel_screen.c 
b/src/mesa/drivers/dri/i965/intel_screen.c
index 61d33311142..7dfaec63779 100644
--- a/src/mesa/drivers/dri/i965/intel_screen.c
+++ b/src/mesa/drivers/dri/i965/intel_screen.c
@@ -1312,42 +1312,44 @@ intel_query_dma_buf_modifiers(__DRIscreen *_screen, int 
fourcc, int max,
 static __DRIimage *
 intel_from_planar(__DRIimage *parent, int plane, void *loaderPrivate)
 {
-int width, height, offset, stride, dri_format, index;
-const struct intel_image_format *f;
+int width, height, offset, stride, dri_format;
 __DRIimage *image;
 
-if (parent == NULL) {
+if (parent == NULL)
return NULL;
-} else if (parent->planar_format == NULL) {
-   const bool is_aux =
-  isl_drm_modifier_has_aux(parent->modifier) && plane == 1;
-   if (!is_aux)
-  return NULL;
-
-   width = parent->width;
-   height = parent->height;
-   dri_format = parent->dri_format;
-   offset = parent->aux_offset;
-   stride = parent->aux_pitch;
-} else {
-   /* Planar formats don't support aux buffers/images */
-   assert(!isl_drm_modifier_has_aux(parent->modifier));
-   f = parent->planar_format;
 
-   if (plane >= f->nplanes)
-  return NULL;
+width = parent->width;
+height = parent->height;
 
-   width = parent->width >> f->planes[plane].width_shift;
-   height = parent->height >> f->planes[plane].height_shift;
+const struct intel_image_format *f = parent->planar_format;
+
+if (f && plane < f->nplanes) {
+   /* Use the planar format definition. */
+   width >>= f->planes[plane].width_shift;
+   height >>= f->planes[plane].height_shift;
dri_format = f->planes[plane].dri_format;
-   index = f->planes[plane].buffer_index;
+   int index = f->planes[plane].buffer_index;
offset = parent->offsets[index];
stride = parent->strides[index];
+} else if (plane == 0) {
+   /* The only plane of a non-planar image: copy the parent definition
+* directly. */
+   dri_format = parent->dri_format;
+   offset = parent->offset;
+   stride = parent->pitch;
+} else if (plane == 1 && parent->modifier != DRM_FORMAT_MOD_INVALID &&
+   isl_drm_modifier_has_aux(parent->modifier)) {
+   /* Auxiliary plane */
+   dri_format = parent->dri_format;
+   offset = parent->aux_offset;
+   stride = parent->aux_pitch;
+} else {
+   return NULL;
+}
 
-   if (offset + height * stride > parent->bo->size) {
-  _mesa_warning(NULL, "intel_create_sub_image: subimage out of 
bounds");
-  return NULL;
-   }
+if (offset + height * stride > parent->bo->size) {
+   _mesa_warning(NULL, "intel_create_sub_image: subimage out of bounds");
+   return NULL;
 }
 
 image = intel_allocate_image(parent->screen, dri_format, loaderPrivate);
-- 
2.14.3

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: Fix bugs in intel_from_planar

[Jason Ekstrand]

On Mon, Feb 12, 2018 at 9:45 AM, Daniel Stone  wrote:

> Hi,
>
> On 12 February 2018 at 17:16, Jason Ekstrand  wrote:
> > On Mon, Feb 12, 2018 at 6:36 AM, Daniel Stone 
> wrote:
> >> On further thought, I don't see how this could work at all. If we call
> >> fromPlanar on plane 0 of a non-planar image with an aux plane, we need
> >> to land in the first branch (due to the assert). But since plane 0 is
> >> strictly less than the number of planes 1, we don't.
> >>
> >> I think something like this: https://hastebin.com/axuqorenax
> >
> > I like that.  Mind making a proper patch out of it (not based on this
> one)
> > and sending to the list?  Between the four of us, maybe we can write this
> > correctly. :-)
>
> Oh yeah. I'm just waiting for my repo to get reactivated for CI
> runners, so I can find out what I broke with that strawman. :)
>

Eh.  Just send it to the list.  I can run it through CI.

--Jason
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: Fix bugs in intel_from_planar

[Daniel Stone]

Hi,

On 12 February 2018 at 17:16, Jason Ekstrand  wrote:
> On Mon, Feb 12, 2018 at 6:36 AM, Daniel Stone  wrote:
>> On further thought, I don't see how this could work at all. If we call
>> fromPlanar on plane 0 of a non-planar image with an aux plane, we need
>> to land in the first branch (due to the assert). But since plane 0 is
>> strictly less than the number of planes 1, we don't.
>>
>> I think something like this: https://hastebin.com/axuqorenax
>
> I like that.  Mind making a proper patch out of it (not based on this one)
> and sending to the list?  Between the four of us, maybe we can write this
> correctly. :-)

Oh yeah. I'm just waiting for my repo to get reactivated for CI
runners, so I can find out what I broke with that strawman. :)

Cheers,
Daniel
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: Fix bugs in intel_from_planar

[Jason Ekstrand]

On Mon, Feb 12, 2018 at 6:36 AM, Daniel Stone  wrote:

> Hi,
>
> On 10 February 2018 at 06:50, Jason Ekstrand  wrote:
> > -} else if (parent->planar_format == NULL) {
> > +
> > +const struct intel_image_format *f = parent->planar_format;
> > +const int nplanes = f ? f->nplanes : 1;
> > +
> > +if (plane > nplanes) {
> > +   if (parent->modifier == DRM_FORMAT_MOD_INVALID)
> > +  return NULL;
> > +
> > const bool is_aux =
> >isl_drm_modifier_has_aux(parent->modifier) && plane == 1;
> > if (!is_aux)
> > @@ -1332,10 +1338,6 @@ intel_from_planar(__DRIimage *parent, int plane,
> void *loaderPrivate)
> >  } else {
> > /* Planar formats don't support aux buffers/images */
> > assert(!isl_drm_modifier_has_aux(parent->modifier));
> > -   f = parent->planar_format;
> > -
> > -   if (plane >= f->nplanes)
> > -  return NULL;
>
> On further thought, I don't see how this could work at all. If we call
> fromPlanar on plane 0 of a non-planar image with an aux plane, we need
> to land in the first branch (due to the assert). But since plane 0 is
> strictly less than the number of planes 1, we don't.
>
> I think something like this: https://hastebin.com/axuqorenax
>

I like that.  Mind making a proper patch out of it (not based on this one)
and sending to the list?  Between the four of us, maybe we can write this
correctly. :-)

--Jason
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: Fix bugs in intel_from_planar

[Daniel Stone]

Hi,

On 10 February 2018 at 06:50, Jason Ekstrand  wrote:
> -} else if (parent->planar_format == NULL) {
> +
> +const struct intel_image_format *f = parent->planar_format;
> +const int nplanes = f ? f->nplanes : 1;
> +
> +if (plane > nplanes) {
> +   if (parent->modifier == DRM_FORMAT_MOD_INVALID)
> +  return NULL;
> +
> const bool is_aux =
>isl_drm_modifier_has_aux(parent->modifier) && plane == 1;
> if (!is_aux)
> @@ -1332,10 +1338,6 @@ intel_from_planar(__DRIimage *parent, int plane, void 
> *loaderPrivate)
>  } else {
> /* Planar formats don't support aux buffers/images */
> assert(!isl_drm_modifier_has_aux(parent->modifier));
> -   f = parent->planar_format;
> -
> -   if (plane >= f->nplanes)
> -  return NULL;

On further thought, I don't see how this could work at all. If we call
fromPlanar on plane 0 of a non-planar image with an aux plane, we need
to land in the first branch (due to the assert). But since plane 0 is
strictly less than the number of planes 1, we don't.

I think something like this: https://hastebin.com/axuqorenax

Cheers,
Daniel
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: Fix bugs in intel_from_planar

[Daniel Stone]

Hi Jason,

On 10 February 2018 at 06:50, Jason Ekstrand  wrote:
>  intel_from_planar(__DRIimage *parent, int plane, void *loaderPrivate)
>  {
>  int width, height, offset, stride, dri_format, index;
> -const struct intel_image_format *f;
>  __DRIimage *image;
>
> -if (parent == NULL) {
> +if (parent == NULL)
> return NULL;
> -} else if (parent->planar_format == NULL) {
> +
> +const struct intel_image_format *f = parent->planar_format;
> +const int nplanes = f ? f->nplanes : 1;
> +
> +if (plane > nplanes) {

This should be >= ...

> +   if (parent->modifier == DRM_FORMAT_MOD_INVALID)
> +  return NULL;
> +
> const bool is_aux =
>isl_drm_modifier_has_aux(parent->modifier) && plane == 1;

Or this will never be true.

Cheers,
Daniel
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] i965: Fix bugs in intel_from_planar

[Jason Ekstrand]

This commit fixes two bugs in intel_from_planar.  First, if the planar
format was non-NULL but only had a single plane, we were falling through
to the planar case.  If we had a CCS modifier and plane == 1, we would
return NULL instead of the CCS plane.  Second, if we did end up in the
planar_format == NULL case and the modifier was DRM_FORMAT_MOD_INVALID,
we would end up segfaulting in isl_drm_modifier_has_aux.

Cc: mesa-sta...@lists.freedesktop.org
Fixes: 8f6e54c92966bb94a3f05f2cc7ea804273e125ad
---
 src/mesa/drivers/dri/i965/intel_screen.c | 16 +---
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/intel_screen.c 
b/src/mesa/drivers/dri/i965/intel_screen.c
index 1f866cf..d4878f1 100644
--- a/src/mesa/drivers/dri/i965/intel_screen.c
+++ b/src/mesa/drivers/dri/i965/intel_screen.c
@@ -1313,12 +1313,18 @@ static __DRIimage *
 intel_from_planar(__DRIimage *parent, int plane, void *loaderPrivate)
 {
 int width, height, offset, stride, dri_format, index;
-const struct intel_image_format *f;
 __DRIimage *image;
 
-if (parent == NULL) {
+if (parent == NULL)
return NULL;
-} else if (parent->planar_format == NULL) {
+
+const struct intel_image_format *f = parent->planar_format;
+const int nplanes = f ? f->nplanes : 1;
+
+if (plane > nplanes) {
+   if (parent->modifier == DRM_FORMAT_MOD_INVALID)
+  return NULL;
+
const bool is_aux =
   isl_drm_modifier_has_aux(parent->modifier) && plane == 1;
if (!is_aux)
@@ -1332,10 +1338,6 @@ intel_from_planar(__DRIimage *parent, int plane, void 
*loaderPrivate)
 } else {
/* Planar formats don't support aux buffers/images */
assert(!isl_drm_modifier_has_aux(parent->modifier));
-   f = parent->planar_format;
-
-   if (plane >= f->nplanes)
-  return NULL;
 
width = parent->width >> f->planes[plane].width_shift;
height = parent->height >> f->planes[plane].height_shift;
-- 
2.5.0.400.gff86faf

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev