[Mesa-dev] [PATCH] st/vdpau: check that the format/bindings are valid

[Ilia Mirkin]

On Fri, Jan 17, 2014 at 3:19 AM, Christian König
<deathsimple at vodafone.de> wrote:
> Am 17.01.2014 04:19, schrieb Ilia Mirkin:
>
>> It's a bit unreasonable to rely on applications doing the queries and
>> then obeying their results.
>
>
> Apart from a minor style comment (see below) the patch looks good to me, but
> isn't that a bit superfluous? I mean if the format isn't supported
> resource_create should probably return NULL anyway.

Maybe? My understanding was that we weren't supposed to check for
error conditions like that, and indeed none of the nouveau drivers do.
Is that what should be happening? I heard somewhere that the idea of
gallium was not to check for illegal conditions, when there are
explicit things like is_format_supported going on. Should every
resource_create call start with if (!is_format_supported) return NULL?
I looked at a few drivers and it didn't seem like they did that, but I
could have missed it.

>
>
>
>>
>> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
>> ---
>>
>> We're starting to see people complaining about flash with newer versions
>> of
>> nouveau on nv30 cards. These cards don't actually expose any hw caps via
>> vdpau, but the API is still free to use the surfaces/etc. We were seeing
>> some
>> errors in the logs that appear to only be able to happen if someone has
>> managed to create an illegal-type surface. (And nv30 is pretty limited in
>> its
>> supported renderable format list.)
>
>
> Or is it just that you guys print en error message to stderr when the format
> isn't supported and you want to suppress that?

dmesg, actually, when the card reports, via interrupt, that an invalid
value was written to a method. It also causes rendering to break.

>
>
>>
>> This adds checks to make sure that the surfaces are valid for the screen
>> in
>> question. Hopefully I haven't misunderstood gallium or the vdpau state
>> tracker... My quick test with mplayer (but without hw accel) seems to
>> work.
>>
>>   src/gallium/state_trackers/vdpau/bitmap.c        |  5 +++--
>>   src/gallium/state_trackers/vdpau/output.c        | 16 ++++++++++------
>>   src/gallium/state_trackers/vdpau/vdpau_private.h |  8 ++++++++
>>   3 files changed, 21 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/gallium/state_trackers/vdpau/bitmap.c
>> b/src/gallium/state_trackers/vdpau/bitmap.c
>> index 469f3e8..24dbc42 100644
>> --- a/src/gallium/state_trackers/vdpau/bitmap.c
>> +++ b/src/gallium/state_trackers/vdpau/bitmap.c
>> @@ -43,7 +43,7 @@ vlVdpBitmapSurfaceCreate(VdpDevice device,
>>                            VdpBitmapSurface *surface)
>>   {
>>      struct pipe_context *pipe;
>> -   struct pipe_resource res_tmpl, *res;
>> +   struct pipe_resource res_tmpl, *res = NULL;
>>      struct pipe_sampler_view sv_templ;
>>        vlVdpBitmapSurface *vlsurface = NULL;
>> @@ -79,7 +79,8 @@ vlVdpBitmapSurfaceCreate(VdpDevice device,
>>      res_tmpl.usage = frequently_accessed ? PIPE_USAGE_DYNAMIC :
>> PIPE_USAGE_STATIC;
>>        pipe_mutex_lock(dev->mutex);
>> -   res = pipe->screen->resource_create(pipe->screen, &res_tmpl);
>> +   if (CheckSurfaceParams(pipe->screen, &res_tmpl))
>> +      res = pipe->screen->resource_create(pipe->screen, &res_tmpl);
>
>
> I would rather code it like this:
>
> if (!CheckSurfaceParams(pipe->screen, &res_tmpl))
>     goto error_not supported;
>
> It's a bit more obvious when you don't know the code.
>
> That off course implies that we have error handling coded like this, feel
> free to change it if it isn't.

OK, will update and resend.

>
> Christian.
>
>
>>      if (!res) {
>>         pipe_mutex_unlock(dev->mutex);
>>         FREE(dev);
>> diff --git a/src/gallium/state_trackers/vdpau/output.c
>> b/src/gallium/state_trackers/vdpau/output.c
>> index e4e1433..76c4611 100644
>> --- a/src/gallium/state_trackers/vdpau/output.c
>> +++ b/src/gallium/state_trackers/vdpau/output.c
>> @@ -48,7 +48,7 @@ vlVdpOutputSurfaceCreate(VdpDevice device,
>>                            VdpOutputSurface  *surface)
>>   {
>>      struct pipe_context *pipe;
>> -   struct pipe_resource res_tmpl, *res;
>> +   struct pipe_resource res_tmpl, *res = NULL;
>>      struct pipe_sampler_view sv_templ;
>>      struct pipe_surface surf_templ;
>>   @@ -83,7 +83,9 @@ vlVdpOutputSurfaceCreate(VdpDevice device,
>>      res_tmpl.usage = PIPE_USAGE_STATIC;
>>        pipe_mutex_lock(dev->mutex);
>> -   res = pipe->screen->resource_create(pipe->screen, &res_tmpl);
>> +
>> +   if (CheckSurfaceParams(pipe->screen, &res_tmpl))
>> +      res = pipe->screen->resource_create(pipe->screen, &res_tmpl);
>>      if (!res) {
>>         pipe_mutex_unlock(dev->mutex);
>>         FREE(dev);
>> @@ -117,7 +119,7 @@ vlVdpOutputSurfaceCreate(VdpDevice device,
>>         FREE(dev);
>>         return VDP_STATUS_ERROR;
>>      }
>> -
>> +
>>      pipe_resource_reference(&res, NULL);
>>        vl_compositor_init_state(&vlsurface->cstate, pipe);
>> @@ -278,7 +280,7 @@ vlVdpOutputSurfacePutBitsIndexed(VdpOutputSurface
>> surface,
>>      enum pipe_format index_format;
>>      enum pipe_format colortbl_format;
>>   -   struct pipe_resource *res, res_tmpl;
>> +   struct pipe_resource *res = NULL, res_tmpl;
>>      struct pipe_sampler_view sv_tmpl;
>>      struct pipe_sampler_view *sv_idx = NULL, *sv_tbl = NULL;
>>   @@ -326,7 +328,8 @@ vlVdpOutputSurfacePutBitsIndexed(VdpOutputSurface
>> surface,
>>      pipe_mutex_lock(vlsurface->device->mutex);
>>      vlVdpResolveDelayedRendering(vlsurface->device, NULL, NULL);
>>   -   res = context->screen->resource_create(context->screen, &res_tmpl);
>> +   if (CheckSurfaceParams(context->screen, &res_tmpl))
>> +      res = context->screen->resource_create(context->screen, &res_tmpl);
>>      if (!res)
>>         goto error_resource;
>>   @@ -359,7 +362,8 @@ vlVdpOutputSurfacePutBitsIndexed(VdpOutputSurface
>> surface,
>>      res_tmpl.usage = PIPE_USAGE_STAGING;
>>      res_tmpl.bind = PIPE_BIND_SAMPLER_VIEW;
>>   -   res = context->screen->resource_create(context->screen, &res_tmpl);
>> +   if (CheckSurfaceParams(context->screen, &res_tmpl))
>> +      res = context->screen->resource_create(context->screen, &res_tmpl);
>>      if (!res)
>>         goto error_resource;
>>   diff --git a/src/gallium/state_trackers/vdpau/vdpau_private.h
>> b/src/gallium/state_trackers/vdpau/vdpau_private.h
>> index 60196ac..6c2c019 100644
>> --- a/src/gallium/state_trackers/vdpau/vdpau_private.h
>> +++ b/src/gallium/state_trackers/vdpau/vdpau_private.h
>> @@ -332,6 +332,14 @@ RectToPipeBox(const VdpRect *rect, struct
>> pipe_resource *res)
>>      return box;
>>   }
>>   +static inline bool
>> +CheckSurfaceParams(struct pipe_screen *screen,
>> +                   const struct pipe_resource *templ)
>> +{
>> +   return screen->is_format_supported(
>> +         screen, templ->format, templ->target, templ->nr_samples,
>> templ->bind);
>> +}
>> +
>>   typedef struct
>>   {
>>      struct vl_screen *vscreen;
>
>