[Mesa-dev] [PATCH 2/3] nouveau: advertise only supported video entrypoints

[Ilia Mirkin]

On Sat, Feb 15, 2014 at 2:30 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> The nv84 code (vp2)
>  - bitstream h264
>  - idct and bitstream mpeg12
> Generic video (vpe2)
>  - mc and idct mpeg12
>
> Signed-off-by: Emil Velikov <emil.l.velikov at gmail.com>
> ---
>
> Seems like some explicit checks would be more robust when
> new code is added. Better to deny support for the user than
> crash of feed junk data to the gpu.
>
>  src/gallium/drivers/nouveau/nouveau_video.c   | 12 ++++++++----
>  src/gallium/drivers/nouveau/nv50/nv84_video.c | 20 +++++++++++++++-----
>  2 files changed, 23 insertions(+), 9 deletions(-)
>
> diff --git a/src/gallium/drivers/nouveau/nouveau_video.c b/src/gallium/drivers/nouveau/nouveau_video.c
> index 8795c9d..9c4711b 100644
> --- a/src/gallium/drivers/nouveau/nouveau_video.c
> +++ b/src/gallium/drivers/nouveau/nouveau_video.c
> @@ -510,8 +510,7 @@ nouveau_create_decoder(struct pipe_context *context,
>     int ret;
>     bool is8274 = screen->device->chipset > 0x80;
>
> -   debug_printf("Acceleration level: %s\n", templ->entrypoint <= PIPE_VIDEO_ENTRYPOINT_BITSTREAM ? "bit":
> -                                            templ->entrypoint == PIPE_VIDEO_ENTRYPOINT_IDCT ? "IDCT" : "MC");
> +   debug_printf("Acceleration level: %s\n", templ->entrypoint == PIPE_VIDEO_ENTRYPOINT_IDCT ? "IDCT" : "MC");
>
>     if (getenv("XVMC_VL"))
>        goto vl;
> @@ -838,8 +837,13 @@ nouveau_screen_get_video_param(struct pipe_screen *pscreen,
>  {
>     switch (param) {
>     case PIPE_VIDEO_CAP_SUPPORTED:
> -      return entrypoint >= PIPE_VIDEO_ENTRYPOINT_IDCT &&
> -         u_reduce_video_profile(profile) == PIPE_VIDEO_FORMAT_MPEG12;
> +      switch (entrypoint) {
> +      case PIPE_VIDEO_ENTRYPOINT_IDCT:
> +      case PIPE_VIDEO_ENTRYPOINT_MC:
> +         return u_reduce_video_profile(profile) == PIPE_VIDEO_FORMAT_MPEG12;
> +      default:
> +         return 0;
> +      }
>     case PIPE_VIDEO_CAP_NPOT_TEXTURES:
>        return 1;
>     case PIPE_VIDEO_CAP_MAX_WIDTH:
> diff --git a/src/gallium/drivers/nouveau/nv50/nv84_video.c b/src/gallium/drivers/nouveau/nv50/nv84_video.c
> index a39f572..d0a3580 100644
> --- a/src/gallium/drivers/nouveau/nv50/nv84_video.c
> +++ b/src/gallium/drivers/nouveau/nv50/nv84_video.c
> @@ -281,8 +281,9 @@ nv84_create_decoder(struct pipe_context *context,
>        return vl_create_decoder(context, templ);
>
>     if ((is_h264 && templ->entrypoint != PIPE_VIDEO_ENTRYPOINT_BITSTREAM) ||
> -       (is_mpeg12 && templ->entrypoint > PIPE_VIDEO_ENTRYPOINT_IDCT)) {
> -      debug_printf("%x\n", templ->entrypoint);
> +       (is_mpeg12 && (templ->entrypoint != PIPE_VIDEO_ENTRYPOINT_BITSTREAM ||
> +                      templ->entrypoint != PIPE_VIDEO_ENTRYPOINT_IDCT))) {
> +      debug_printf("invalid entrypoint %x\n", templ->entrypoint);

This code was added before the format checks had an entrypoint in
them. I think this whole if statement can safely be removed now -- the
expectation is that gallium callers perform the required checks
(except that it wasn't possible before, hence this logic). But if you
_really_ want to have the check here, may as well flip it to the
switch style.

>        return NULL;
>     }
>
> @@ -812,9 +813,18 @@ nv84_screen_get_video_param(struct pipe_screen *pscreen,
>     switch (param) {
>     case PIPE_VIDEO_CAP_SUPPORTED:
>        codec = u_reduce_video_profile(profile);
> -      return (codec == PIPE_VIDEO_FORMAT_MPEG4_AVC ||
> -              codec == PIPE_VIDEO_FORMAT_MPEG12) &&
> -         firmware_present(pscreen, codec);
> +      switch (codec) {
> +      case PIPE_VIDEO_FORMAT_MPEG12:
> +         return ((entrypoint == PIPE_VIDEO_ENTRYPOINT_BITSTREAM) ||
> +                 (entrypoint == PIPE_VIDEO_ENTRYPOINT_IDCT)) &&

You don't need the extra parens around a == b. (Same below.)

> +                firmware_present(pscreen, codec);
> +      case PIPE_VIDEO_FORMAT_MPEG4_AVC:
> +         return (entrypoint == PIPE_VIDEO_ENTRYPOINT_BITSTREAM) &&
> +                firmware_present(pscreen, codec);
> +      default:
> +         return 0;
> +      }
> +
>     case PIPE_VIDEO_CAP_NPOT_TEXTURES:
>        return 1;
>     case PIPE_VIDEO_CAP_MAX_WIDTH:
> --
> 1.8.5.5

With the above small changes,

Reviewed-by: Ilia Mirkin <imirkin at alum.mit.edu>