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)
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 <imir...@alum.mit.edu>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
