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

[Ilia Mirkin]

On Sat, Feb 15, 2014 at 3:17 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> 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)

Bugs happen. The OMX st should be fixed.

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

instead of returning null? fine by me. you could do the same in
nouveau_video.c as well.

>
>>>        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>
>>
>