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