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

[Emil Velikov]

On 15/02/14 20:03, Ilia Mirkin wrote:
> 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.
> 
Except that the OMX state tracker does not call get_video_param() before
coming here :\ (I know there is no omx-nouveau target, yet)

Any objections if I convert this to a switch and just assert(0) ?

>>        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.)
> 
Cool, I'll drop those.

Cheers
-Emil
>> +                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>
>