Am 17.01.2014 09:37, schrieb Ilia Mirkin:
On Fri, Jan 17, 2014 at 3:19 AM, Christian König <[email protected]> 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 <[email protected]> --- 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;
_______________________________________________ mesa-dev mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/mesa-dev
