[Mesa-dev] [PATCH] st/vdpau: check that the format/bindings are valid
Christian König
deathsimple at vodafone.de
Fri Jan 17 00:49:51 PST 2014
Am 17.01.2014 09:37, schrieb 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.
Good argument. Sounds like you're right we should probably check that in
the state tracker than.
>>
>>
>>> 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.
With that fixed the patch has my rb.
Christian.
>
>> 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;
>>
More information about the mesa-dev
mailing list