Re: [Mesa-dev] [PATCH] egl/dri2: Allow modifiers to add FDs to imports

2017-08-09 Thread Daniel Stone 
On 8 August 2017 at 09:48, Tapani Pälli  wrote:
> On 08/08/2017 10:37 AM, Philipp Zabel wrote:
>> On Tue, 2017-08-08 at 07:29 +0300, Tapani Pälli wrote:
 Since this increments plane_n, Should a check be added that the
 corresponding DMABufPlanFds[i] is present?
>>>
>>> Check for the fd is right above this check.
>>
>>
>> I see this right above:
>>
>>if (attrs->DMABufPlaneFds[i].IsPresent ||
>>attrs->DMABufPlaneOffsets[i].IsPresent ||
>>attrs->DMABufPlanePitches[i].IsPresent ||
>>attrs->DMABufPlaneModifiersLo[i].IsPresent ||
>>attrs->DMABufPlaneModifiersHi[i].IsPresent) {
>>
>> If modifiers are present, this is always true, regardless of whether the
>> fd is present.
>>
>> The loop that checks for fd presence even before that only loops up to
>> the number of planes determined by the non-modified fourcc.
>
> Ah that is correct, my bad. It would be possible to swim through with having
> modifiers and not fd present.

Thanks for the review both - v2 sent which makes it a bit more neat
and also fixes the bug.

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

Re: [Mesa-dev] [PATCH] egl/dri2: Allow modifiers to add FDs to imports

2017-08-08 Thread Tapani Pälli 



On 08/08/2017 10:37 AM, Philipp Zabel wrote:

On Tue, 2017-08-08 at 07:29 +0300, Tapani Pälli wrote:


On 08/07/2017 03:05 PM, Philipp Zabel wrote:

On Mon, 2017-07-31 at 18:35 +0100, Daniel Stone wrote:

When using dmabuf import, make sure that the modifier is actually
allowed to add planes to the base format, as implied by the comment.

Signed-off-by: Daniel Stone 
---
   src/egl/drivers/dri2/egl_dri2.c | 4 +++-
   1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
index b73dcd72b6..76294897a5 100644
--- a/src/egl/drivers/dri2/egl_dri2.c
+++ b/src/egl/drivers/dri2/egl_dri2.c
@@ -2166,8 +2166,10 @@ dri2_check_dma_buf_format(const _EGLImageAttribs *attrs)
 *  this extension."
 */
if (attrs->DMABufPlaneModifiersLo[i].IsPresent &&
- attrs->DMABufPlaneModifiersHi[i].IsPresent)
+ attrs->DMABufPlaneModifiersHi[i].IsPresent) {
+plane_n = i + 1;


Since this increments plane_n, Should a check be added that the
corresponding DMABufPlanFds[i] is present?


Check for the fd is right above this check.


I see this right above:

   if (attrs->DMABufPlaneFds[i].IsPresent ||
   attrs->DMABufPlaneOffsets[i].IsPresent ||
   attrs->DMABufPlanePitches[i].IsPresent ||
   attrs->DMABufPlaneModifiersLo[i].IsPresent ||
   attrs->DMABufPlaneModifiersHi[i].IsPresent) {

If modifiers are present, this is always true, regardless of whether the
fd is present.

The loop that checks for fd presence even before that only loops up to
the number of planes determined by the non-modified fourcc.


Ah that is correct, my bad. It would be possible to swim through with 
having modifiers and not fd present.


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

Re: [Mesa-dev] [PATCH] egl/dri2: Allow modifiers to add FDs to imports

2017-08-08 Thread Philipp Zabel 
On Tue, 2017-08-08 at 07:29 +0300, Tapani Pälli wrote:
> 
> On 08/07/2017 03:05 PM, Philipp Zabel wrote:
> > On Mon, 2017-07-31 at 18:35 +0100, Daniel Stone wrote:
> >> When using dmabuf import, make sure that the modifier is actually
> >> allowed to add planes to the base format, as implied by the comment.
> >>
> >> Signed-off-by: Daniel Stone 
> >> ---
> >>   src/egl/drivers/dri2/egl_dri2.c | 4 +++-
> >>   1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/src/egl/drivers/dri2/egl_dri2.c 
> >> b/src/egl/drivers/dri2/egl_dri2.c
> >> index b73dcd72b6..76294897a5 100644
> >> --- a/src/egl/drivers/dri2/egl_dri2.c
> >> +++ b/src/egl/drivers/dri2/egl_dri2.c
> >> @@ -2166,8 +2166,10 @@ dri2_check_dma_buf_format(const _EGLImageAttribs 
> >> *attrs)
> >> *  this extension."
> >> */
> >>if (attrs->DMABufPlaneModifiersLo[i].IsPresent &&
> >> - attrs->DMABufPlaneModifiersHi[i].IsPresent)
> >> + attrs->DMABufPlaneModifiersHi[i].IsPresent) {
> >> +plane_n = i + 1;
> > 
> > Since this increments plane_n, Should a check be added that the
> > corresponding DMABufPlanFds[i] is present?
> 
> Check for the fd is right above this check.

I see this right above:

  if (attrs->DMABufPlaneFds[i].IsPresent || 
  attrs->DMABufPlaneOffsets[i].IsPresent ||
  attrs->DMABufPlanePitches[i].IsPresent ||
  attrs->DMABufPlaneModifiersLo[i].IsPresent ||
  attrs->DMABufPlaneModifiersHi[i].IsPresent) {

If modifiers are present, this is always true, regardless of whether the
fd is present.

The loop that checks for fd presence even before that only loops up to
the number of planes determined by the non-modified fourcc.

regards
Philipp

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

Re: [Mesa-dev] [PATCH] egl/dri2: Allow modifiers to add FDs to imports

2017-08-07 Thread Tapani Pälli 



On 08/07/2017 03:05 PM, Philipp Zabel wrote:

On Mon, 2017-07-31 at 18:35 +0100, Daniel Stone wrote:

When using dmabuf import, make sure that the modifier is actually
allowed to add planes to the base format, as implied by the comment.

Signed-off-by: Daniel Stone 
---
  src/egl/drivers/dri2/egl_dri2.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
index b73dcd72b6..76294897a5 100644
--- a/src/egl/drivers/dri2/egl_dri2.c
+++ b/src/egl/drivers/dri2/egl_dri2.c
@@ -2166,8 +2166,10 @@ dri2_check_dma_buf_format(const _EGLImageAttribs *attrs)
*  this extension."
*/
   if (attrs->DMABufPlaneModifiersLo[i].IsPresent &&
- attrs->DMABufPlaneModifiersHi[i].IsPresent)
+ attrs->DMABufPlaneModifiersHi[i].IsPresent) {
+plane_n = i + 1;


Since this increments plane_n, Should a check be added that the
corresponding DMABufPlanFds[i] is present?


Check for the fd is right above this check.


What if there are holes in DMABufPlaneModifiersLo/Hi?


  continue;
+   }
  
   _eglError(EGL_BAD_ATTRIBUTE, "too many plane attributes");

   return 0;


Reviewed-by: Philipp Zabel 

regards
Philipp

___
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

Re: [Mesa-dev] [PATCH] egl/dri2: Allow modifiers to add FDs to imports

2017-08-07 Thread Philipp Zabel 
On Mon, 2017-07-31 at 18:35 +0100, Daniel Stone wrote:
> When using dmabuf import, make sure that the modifier is actually
> allowed to add planes to the base format, as implied by the comment.
> 
> Signed-off-by: Daniel Stone 
> ---
>  src/egl/drivers/dri2/egl_dri2.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
> index b73dcd72b6..76294897a5 100644
> --- a/src/egl/drivers/dri2/egl_dri2.c
> +++ b/src/egl/drivers/dri2/egl_dri2.c
> @@ -2166,8 +2166,10 @@ dri2_check_dma_buf_format(const _EGLImageAttribs 
> *attrs)
>*  this extension."
>*/
>   if (attrs->DMABufPlaneModifiersLo[i].IsPresent &&
> - attrs->DMABufPlaneModifiersHi[i].IsPresent)
> + attrs->DMABufPlaneModifiersHi[i].IsPresent) {
> +plane_n = i + 1;

Since this increments plane_n, Should a check be added that the
corresponding DMABufPlanFds[i] is present?
What if there are holes in DMABufPlaneModifiersLo/Hi?

>  continue;
> + }
>  
>   _eglError(EGL_BAD_ATTRIBUTE, "too many plane attributes");
>   return 0;

Reviewed-by: Philipp Zabel 

regards
Philipp

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

Re: [Mesa-dev] [PATCH] egl/dri2: Allow modifiers to add FDs to imports

2017-08-07 Thread Tapani Pälli 



On 07/31/2017 08:35 PM, Daniel Stone wrote:

When using dmabuf import, make sure that the modifier is actually
allowed to add planes to the base format, as implied by the comment.

Signed-off-by: Daniel Stone 
---
  src/egl/drivers/dri2/egl_dri2.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
index b73dcd72b6..76294897a5 100644
--- a/src/egl/drivers/dri2/egl_dri2.c
+++ b/src/egl/drivers/dri2/egl_dri2.c
@@ -2166,8 +2166,10 @@ dri2_check_dma_buf_format(const _EGLImageAttribs *attrs)
*  this extension."
*/
   if (attrs->DMABufPlaneModifiersLo[i].IsPresent &&
- attrs->DMABufPlaneModifiersHi[i].IsPresent)
+ attrs->DMABufPlaneModifiersHi[i].IsPresent) {
+plane_n = i + 1;



Yep, this implements what the comment there says. Just to sanity check, 
*any* given modifier gives me rights to have additional fds/planes? If 
this is fine, then:


Reviewed-by: Tapani Pälli 



  continue;
+   }
  
   _eglError(EGL_BAD_ATTRIBUTE, "too many plane attributes");

   return 0;



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

Re: [Mesa-dev] [PATCH] egl/dri2: Allow modifiers to add FDs to imports

2017-08-07 Thread Daniel Stone 
On 31 July 2017 at 18:35, Daniel Stone  wrote:
> When using dmabuf import, make sure that the modifier is actually
> allowed to add planes to the base format, as implied by the comment.

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

[Mesa-dev] [PATCH] egl/dri2: Allow modifiers to add FDs to imports

2017-07-31 Thread Daniel Stone 
When using dmabuf import, make sure that the modifier is actually
allowed to add planes to the base format, as implied by the comment.

Signed-off-by: Daniel Stone 
---
 src/egl/drivers/dri2/egl_dri2.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
index b73dcd72b6..76294897a5 100644
--- a/src/egl/drivers/dri2/egl_dri2.c
+++ b/src/egl/drivers/dri2/egl_dri2.c
@@ -2166,8 +2166,10 @@ dri2_check_dma_buf_format(const _EGLImageAttribs *attrs)
   *  this extension."
   */
  if (attrs->DMABufPlaneModifiersLo[i].IsPresent &&
- attrs->DMABufPlaneModifiersHi[i].IsPresent)
+ attrs->DMABufPlaneModifiersHi[i].IsPresent) {
+plane_n = i + 1;
 continue;
+   }
 
  _eglError(EGL_BAD_ATTRIBUTE, "too many plane attributes");
  return 0;
-- 
2.13.3

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