On Sat, Feb 15, 2014 at 3:17 PM, Emil Velikov <emil.l.veli...@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.veli...@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.veli...@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 <imir...@alum.mit.edu>
>>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
