[{"id":1775013,"web_url":"http://patchwork.ozlabs.org/comment/1775013/","msgid":"<38daee0d-0595-a7e4-69bb-5a2ddbd832b5@wwwdotorg.org>","list_archive_url":null,"date":"2017-09-25T23:01:23","subject":"Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder\n\tdriver","submitter":{"id":12517,"url":"http://patchwork.ozlabs.org/api/people/12517/","name":"Stephen Warren","email":"swarren@wwwdotorg.org"},"content":"On 09/25/2017 04:15 PM, Dmitry Osipenko wrote:\n> Video decoder, found on NVIDIA Tegra20 SoC, supports a standard set of\n> video formats like H.264 / MPEG-4 / WMV / VC1. Currently driver supports\n> decoding of CAVLC H.264 only.\n\nNote: I don't know anything much about video decoding on Tegra (just NV \ndesktop GPUs, and that was a while ago), but I had a couple small \ncomments on the DT binding:\n\n> diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt\n\n> +NVIDIA Tegra Video Decoder Engine\n> +\n> +Required properties:\n> +- compatible : \"nvidia,tegra20-vde\"\n> +- reg : Must contain 2 register ranges: registers and IRAM area.\n> +- reg-names : Must include the following entries:\n> + - regs\n> + - iram\n\nI think the IRAM region needs more explanation: What is the region used \nfor and by what? Can it be moved, and if so does the move need to be \nco-ordinated with any other piece of SW?\n\n> +- clocks : Must contain one entry, for the module clock.\n> + See ../clocks/clock-bindings.txt for details.\n> +- resets : Must contain an entry for each entry in reset-names.\n> + See ../reset/reset.txt for details.\n> +- reset-names : Must include the following entries:\n> + - vde\n\nLet's require a clock-names property too.\n--\nTo unsubscribe from this list: send the line \"unsubscribe linux-tegra\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-tegra-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y1KV7149hz9t2M\n\tfor ;\n\tTue, 26 Sep 2017 09:07:11 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S965398AbdIYXHH (ORCPT );\n\tMon, 25 Sep 2017 19:07:07 -0400","from avon.wwwdotorg.org ([104.237.132.123]:37578 \"EHLO\n\tavon.wwwdotorg.org\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S934948AbdIYXHF (ORCPT\n\t);\n\tMon, 25 Sep 2017 19:07:05 -0400","from [10.20.204.51] (thunderhill.nvidia.com [216.228.112.22])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128\n\tbits)) (No client certificate requested)\n\tby avon.wwwdotorg.org (Postfix) with ESMTPSA id 9837B1C035B;\n\tMon, 25 Sep 2017 17:01:26 -0600 (MDT)"],"X-Greylist":"delayed 338 seconds by postgrey-1.27 at vger.kernel.org;\n\tMon, 25 Sep 2017 19:07:05 EDT","X-Virus-Status":"Clean","X-Virus-Scanned":"clamav-milter 0.99.2 at avon.wwwdotorg.org","Subject":"Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder\n\tdriver","To":"Dmitry Osipenko ","Cc":"Thierry Reding ,\n\tJonathan Hunter ,\n\tGreg Kroah-Hartman ,\n\tRob Herring , linux-tegra@vger.kernel.org,\n\tdevel@driverdev.osuosl.org, devicetree@vger.kernel.org,\n\tlinux-kernel@vger.kernel.org","References":"\n\t<5c8b83775b982e6ee851c127444a8e839f422ad0.1506377430.git.digetx@gmail.com>","From":"Stephen Warren ","Message-ID":"<38daee0d-0595-a7e4-69bb-5a2ddbd832b5@wwwdotorg.org>","Date":"Mon, 25 Sep 2017 17:01:23 -0600","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"<5c8b83775b982e6ee851c127444a8e839f422ad0.1506377430.git.digetx@gmail.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Sender":"linux-tegra-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-tegra@vger.kernel.org"}},{"id":1775044,"web_url":"http://patchwork.ozlabs.org/comment/1775044/","msgid":"","list_archive_url":null,"date":"2017-09-25T23:45:23","subject":"Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder\n\tdriver","submitter":{"id":18124,"url":"http://patchwork.ozlabs.org/api/people/18124/","name":"Dmitry Osipenko","email":"digetx@gmail.com"},"content":"On 26.09.2017 02:01, Stephen Warren wrote:\n> On 09/25/2017 04:15 PM, Dmitry Osipenko wrote:\n>> Video decoder, found on NVIDIA Tegra20 SoC, supports a standard set of\n>> video formats like H.264 / MPEG-4 / WMV / VC1. Currently driver supports\n>> decoding of CAVLC H.264 only.\n> \n> Note: I don't know anything much about video decoding on Tegra (just NV desktop\n> GPUs, and that was a while ago), but I had a couple small comments on the DT\n> binding:\n> \n>> diff --git\n>> a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt\n>> b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt\n> \n>> +NVIDIA Tegra Video Decoder Engine\n>> +\n>> +Required properties:\n>> +- compatible : \"nvidia,tegra20-vde\"\n>> +- reg : Must contain 2 register ranges: registers and IRAM area.\n>> +- reg-names : Must include the following entries:\n>> +  - regs\n>> +  - iram\n> \n> I think the IRAM region needs more explanation: What is the region used for and\n> by what? Can it be moved, and if so does the move need to be co-ordinated with\n> any other piece of SW?\n> \n\nIRAM region is used by Video Decoder HW for internal use and some of decoding\nparameters are supplied via IRAM, like frames order list. AFAIK IRAM addresses\nare hardwired in HW and aren't movable, it is not 100% but I'm pretty sure.\nShould it be explained in the binding?\n\n>> +- clocks : Must contain one entry, for the module clock.\n>> +  See ../clocks/clock-bindings.txt for details.\n>> +- resets : Must contain an entry for each entry in reset-names.\n>> +  See ../reset/reset.txt for details.\n>> +- reset-names : Must include the following entries:\n>> +  - vde\n> \n> Let's require a clock-names property too.\n\nOkay, I'll add this property to the binding.","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-tegra-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"H5hvKg6Y\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y1LLL35SVz9t2Q\n\tfor ;\n\tTue, 26 Sep 2017 09:45:30 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S936467AbdIYXp3 (ORCPT );\n\tMon, 25 Sep 2017 19:45:29 -0400","from mail-wr0-f196.google.com ([209.85.128.196]:33612 \"EHLO\n\tmail-wr0-f196.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S933685AbdIYXp1 (ORCPT\n\t);\n\tMon, 25 Sep 2017 19:45:27 -0400","by mail-wr0-f196.google.com with SMTP id b9so952016wra.0;\n\tMon, 25 Sep 2017 16:45:27 -0700 (PDT)","from [192.168.1.145] (ppp109-252-90-109.pppoe.spdop.ru.\n\t[109.252.90.109]) by smtp.googlemail.com with ESMTPSA id\n\tn23sm1660437ljb.1.2017.09.25.16.45.24\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tMon, 25 Sep 2017 16:45:25 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=gmail.com; s=20161025;\n\th=subject:to:cc:references:from:message-id:date:user-agent\n\t:mime-version:in-reply-to:content-language:content-transfer-encoding; \n\tbh=nwWKkyxp+Nf0ZWsjo+iWhmrYl1bNXKhHyDyaYHYTVPs=;\n\tb=H5hvKg6Y+o5wdHJl5JlBZ8bqbXVRkyvkf0qIR6SoHazgEDMnx8vuNUegroMKaMVX8W\n\tw1hiqxtQ6M07GlFT15g9XDEmPDF1pUXTD099RCrGx8FEpNabvpQUa04Vi+M3yX/sNKOC\n\tyfIMIWNYX8tCWU9vH5sIO9o3By74wGDpoLfdlKZXuBPYom2alAi/kXVTguqcVCvgVR66\n\tnNeIIc2B9OZqxkrUFilst1K033vOo1WrWvqFYkpnr2/pH/YUQIOJZSybzD+2/7YjJajv\n\tk3FG4CjB+TtrX/PYaQjzSPqqOvYpzAM9wUMd6Fw1oAvPYKoefJjQ9NGhxcqc2zqOAcL9\n\tXB/g==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:subject:to:cc:references:from:message-id:date\n\t:user-agent:mime-version:in-reply-to:content-language\n\t:content-transfer-encoding;\n\tbh=nwWKkyxp+Nf0ZWsjo+iWhmrYl1bNXKhHyDyaYHYTVPs=;\n\tb=uLndRPtK+kRW9aYXf4efACQbAxOC6D3b7QR+v6Bt85KC+AXmz1vot4tWd04Qr7iHko\n\tT+TUJevoYe3O2iQh8S2Te2sNld8yJo15UINMwsyqy289lRceCLbT083QAE5KNKXSUnN1\n\teQA1+3c6K6Pia8mNlAJqs5phQPdGIT5zJYhW9IDJdi6IG8QTou99K0f3cdmlrdy1xIIV\n\ty+Dz9zbt5i03Qt8vjEFvtVkRHEEdA2P9ZF3xYTA4UOE+1xeCAJcab5pw3oeDGxreYrK3\n\te6T8sWK0iGTIEjZTrrhljgMHz6GADi/jeXpEqXQM9ytblHZqi38WlYZ83Yx2QlAFOY3s\n\t13lw==","X-Gm-Message-State":"AHPjjUjue6Z4xu7w3hCDvdaTTZRRxkWY46aW/23MnzwY+tGpNKZ6sHbP\n\ttHESC6AMS6xpcnd5uOwjbd44tzrZ","X-Google-Smtp-Source":"AOwi7QAzgyrsvECd/BdE66Cd9UAwOUe5fpCNY040Kcfhuln92iYL4uS+uDmgMyFnMSFmRmedkMNifw==","X-Received":"by 10.25.20.214 with SMTP id 83mr2652454lfu.184.1506383126156;\n\tMon, 25 Sep 2017 16:45:26 -0700 (PDT)","Subject":"Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder\n\tdriver","To":"Stephen Warren ","Cc":"Thierry Reding ,\n\tJonathan Hunter ,\n\tGreg Kroah-Hartman ,\n\tRob Herring , linux-tegra@vger.kernel.org,\n\tdevel@driverdev.osuosl.org, devicetree@vger.kernel.org,\n\tlinux-kernel@vger.kernel.org","References":"\n\t<5c8b83775b982e6ee851c127444a8e839f422ad0.1506377430.git.digetx@gmail.com>\n\t<38daee0d-0595-a7e4-69bb-5a2ddbd832b5@wwwdotorg.org>","From":"Dmitry Osipenko ","Message-ID":"","Date":"Tue, 26 Sep 2017 02:45:23 +0300","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<38daee0d-0595-a7e4-69bb-5a2ddbd832b5@wwwdotorg.org>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"8bit","Sender":"linux-tegra-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-tegra@vger.kernel.org"}},{"id":1775164,"web_url":"http://patchwork.ozlabs.org/comment/1775164/","msgid":"<52f5c05f-7060-4d0f-355f-6da372c9ded0@wwwdotorg.org>","list_archive_url":null,"date":"2017-09-26T05:11:35","subject":"Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder\n\tdriver","submitter":{"id":12517,"url":"http://patchwork.ozlabs.org/api/people/12517/","name":"Stephen Warren","email":"swarren@wwwdotorg.org"},"content":"On 09/25/2017 05:45 PM, Dmitry Osipenko wrote:\n> On 26.09.2017 02:01, Stephen Warren wrote:\n>> On 09/25/2017 04:15 PM, Dmitry Osipenko wrote:\n>>> Video decoder, found on NVIDIA Tegra20 SoC, supports a standard set of\n>>> video formats like H.264 / MPEG-4 / WMV / VC1. Currently driver supports\n>>> decoding of CAVLC H.264 only.\n>>\n>> Note: I don't know anything much about video decoding on Tegra (just NV desktop\n>> GPUs, and that was a while ago), but I had a couple small comments on the DT\n>> binding:\n>>\n>>> diff --git\n>>> a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt\n>>> b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt\n>>\n>>> +NVIDIA Tegra Video Decoder Engine\n>>> +\n>>> +Required properties:\n>>> +- compatible : \"nvidia,tegra20-vde\"\n>>> +- reg : Must contain 2 register ranges: registers and IRAM area.\n>>> +- reg-names : Must include the following entries:\n>>> + - regs\n>>> + - iram\n>>\n>> I think the IRAM region needs more explanation: What is the region used for and\n>> by what? Can it be moved, and if so does the move need to be co-ordinated with\n>> any other piece of SW?\n> \n> IRAM region is used by Video Decoder HW for internal use and some of decoding\n> parameters are supplied via IRAM, like frames order list. AFAIK IRAM addresses\n> are hardwired in HW and aren't movable, it is not 100% but I'm pretty sure.\n> Should it be explained in the binding?\n\nI think this should be briefly mentioned, yes. Otherwise at least people\nwho don't know the VDE HW well (like me) will wonder why on earth VDE\ninteracts with IRAM at all. I would have assumed all parameters were\nsupplied via registers or via descriptors in DRAM.\n\nThanks.\n--\nTo unsubscribe from this list: send the line \"unsubscribe linux-tegra\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-tegra-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y1TZg3R67z9s9Y\n\tfor ;\n\tTue, 26 Sep 2017 15:11:39 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S935958AbdIZFLh (ORCPT );\n\tTue, 26 Sep 2017 01:11:37 -0400","from avon.wwwdotorg.org ([104.237.132.123]:38142 \"EHLO\n\tavon.wwwdotorg.org\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S934183AbdIZFLg (ORCPT\n\t);\n\tTue, 26 Sep 2017 01:11:36 -0400","from [192.168.63.205] (c-73-181-65-182.hsd1.co.comcast.net\n\t[73.181.65.182])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128\n\tbits)) (No client certificate requested)\n\tby avon.wwwdotorg.org (Postfix) with ESMTPSA id 6F27C1C3452;\n\tMon, 25 Sep 2017 23:11:35 -0600 (MDT)"],"X-Virus-Status":"Clean","X-Virus-Scanned":"clamav-milter 0.99.2 at avon.wwwdotorg.org","Subject":"Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder\n\tdriver","To":"Dmitry Osipenko ","Cc":"Thierry Reding ,\n\tJonathan Hunter ,\n\tGreg Kroah-Hartman ,\n\tRob Herring , linux-tegra@vger.kernel.org,\n\tdevel@driverdev.osuosl.org, devicetree@vger.kernel.org,\n\tlinux-kernel@vger.kernel.org","References":"\n\t<5c8b83775b982e6ee851c127444a8e839f422ad0.1506377430.git.digetx@gmail.com>\n\t<38daee0d-0595-a7e4-69bb-5a2ddbd832b5@wwwdotorg.org>\n\t","From":"Stephen Warren ","Message-ID":"<52f5c05f-7060-4d0f-355f-6da372c9ded0@wwwdotorg.org>","Date":"Mon, 25 Sep 2017 23:11:35 -0600","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.1.1","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Sender":"linux-tegra-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-tegra@vger.kernel.org"}},{"id":1775402,"web_url":"http://patchwork.ozlabs.org/comment/1775402/","msgid":"<113675da-6b85-ef2f-03a9-84e5cb93c31b@gmail.com>","list_archive_url":null,"date":"2017-09-26T12:02:44","subject":"Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder\n\tdriver","submitter":{"id":18124,"url":"http://patchwork.ozlabs.org/api/people/18124/","name":"Dmitry Osipenko","email":"digetx@gmail.com"},"content":"On 26.09.2017 08:11, Stephen Warren wrote:\n> On 09/25/2017 05:45 PM, Dmitry Osipenko wrote:\n>> On 26.09.2017 02:01, Stephen Warren wrote:\n>>> On 09/25/2017 04:15 PM, Dmitry Osipenko wrote:\n>>>> Video decoder, found on NVIDIA Tegra20 SoC, supports a standard set of\n>>>> video formats like H.264 / MPEG-4 / WMV / VC1. Currently driver supports\n>>>> decoding of CAVLC H.264 only.\n>>>\n>>> Note: I don't know anything much about video decoding on Tegra (just NV desktop\n>>> GPUs, and that was a while ago), but I had a couple small comments on the DT\n>>> binding:\n>>>\n>>>> diff --git\n>>>> a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt\n>>>> b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt\n>>>\n>>>> +NVIDIA Tegra Video Decoder Engine\n>>>> +\n>>>> +Required properties:\n>>>> +- compatible : \"nvidia,tegra20-vde\"\n>>>> +- reg : Must contain 2 register ranges: registers and IRAM area.\n>>>> +- reg-names : Must include the following entries:\n>>>> + - regs\n>>>> + - iram\n>>>\n>>> I think the IRAM region needs more explanation: What is the region used for and\n>>> by what? Can it be moved, and if so does the move need to be co-ordinated with\n>>> any other piece of SW?\n>>\n>> IRAM region is used by Video Decoder HW for internal use and some of decoding\n>> parameters are supplied via IRAM, like frames order list. AFAIK IRAM addresses\n>> are hardwired in HW and aren't movable, it is not 100% but I'm pretty sure.\n>> Should it be explained in the binding?\n> \n> I think this should be briefly mentioned, yes. Otherwise at least people\n> who don't know the VDE HW well (like me) will wonder why on earth VDE\n> interacts with IRAM at all. I would have assumed all parameters were\n> supplied via registers or via descriptors in DRAM.\n> \n> Thanks.\n> \n\nI also forgot to mention that VDE scrubs that IRAM region on HW reset. So yeah,\nit's definitely a part of HW definition. I'll add a brief explanation to the\nbinding.","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-tegra-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"OY3K85KD\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y1fj65M0yz9tXf\n\tfor ;\n\tTue, 26 Sep 2017 22:02:50 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S935329AbdIZMCt (ORCPT );\n\tTue, 26 Sep 2017 08:02:49 -0400","from mail-lf0-f65.google.com ([209.85.215.65]:38425 \"EHLO\n\tmail-lf0-f65.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S934961AbdIZMCs (ORCPT\n\t);\n\tTue, 26 Sep 2017 08:02:48 -0400","by mail-lf0-f65.google.com with SMTP id m199so2574668lfe.5;\n\tTue, 26 Sep 2017 05:02:47 -0700 (PDT)","from [192.168.1.145] (ppp109-252-90-109.pppoe.spdop.ru.\n\t[109.252.90.109]) by smtp.googlemail.com with ESMTPSA id\n\tz69sm1363713lfd.78.2017.09.26.05.02.44\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tTue, 26 Sep 2017 05:02:45 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=gmail.com; s=20161025;\n\th=subject:to:cc:references:from:message-id:date:user-agent\n\t:mime-version:in-reply-to:content-language:content-transfer-encoding; \n\tbh=Pc9XdHYL4kfAHaV6magVAVmd0GLNfo//cjpbrq6z6JU=;\n\tb=OY3K85KDmDwJStMOhP9BqoLaLAD//QzugG2I/Vgekx8s6BtdXis2IZQvnhTeJ2dMVb\n\tA9+LcPwjC5AGI3ZVpopaBe30vsKhuLA13LgiELWBZj+xkJmqxtgZSqApnLm1ZWQhLgf9\n\tWBGSAZCGdtjaHwWZClfW9eLexf8jIQZOuLzB5mkSXNDcgNbmz7s/Rb83mI/USd7UmTgt\n\tRgpM5axMouf8tUICnCuDWOX0e3EZw0CVaYYgLVuGyTNhDsnVlUoA/5LSADJIJLjt7jnm\n\tEYGVrBQEmzwLV/DZLDqRa2wtgbsivfAMDW/x9Th8ca+N9j6/8G3t/F8kQ00o20rFxSzy\n\tWKXA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:subject:to:cc:references:from:message-id:date\n\t:user-agent:mime-version:in-reply-to:content-language\n\t:content-transfer-encoding;\n\tbh=Pc9XdHYL4kfAHaV6magVAVmd0GLNfo//cjpbrq6z6JU=;\n\tb=jALjxVlAlY+SnBxxR6vXqjjvw2dOL2FrEIdorwNBaoiG/ZVWeaFI8+Pp7nKFJXTWTu\n\tSNPT2fpAj5DdO6u4pNTN4Kj0sPZvSGiQv8PASlO3knxfNwW+Ig/OosroEoBwZRPS1WgP\n\txLAeF03mMBwmWoY6h8QSS3KHinCCPtgSPL8iGXaECsFCK68ciwcWcQwhEy9Iwsj+J+Wd\n\tk86Zs0WCc6XEmlhCSB98qQqrUa8O4+o1jlQX7hV9JvtKuyINtglHaZoCVZg3QTAPZdjR\n\t0fulUOwTK5DjfvD+kX4c99+8YAsllSU6nsAU8ALWJxKUpf1joptQLQcvqMTyapsvc5DX\n\tLBCw==","X-Gm-Message-State":"AHPjjUjXPXdVFgfR8qCrmtfYwP8JK2HCk3CDtX0XN+l7Aa/l9pki63IT\n\tO44Qq6loqh9G8JqJL59q8s9rjTW2","X-Google-Smtp-Source":"AOwi7QBidlTDK4/l0OBN2BgqIigfCOufH/g/3wobfpUU+Tvwt4Y1aaWj0//Y4l8JjYZn7ikpj1vrYQ==","X-Received":"by 10.46.97.1 with SMTP id v1mr3988733ljb.176.1506427366134;\n\tTue, 26 Sep 2017 05:02:46 -0700 (PDT)","Subject":"Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder\n\tdriver","To":"Stephen Warren ","Cc":"Thierry Reding ,\n\tJonathan Hunter ,\n\tGreg Kroah-Hartman ,\n\tRob Herring , linux-tegra@vger.kernel.org,\n\tdevel@driverdev.osuosl.org, devicetree@vger.kernel.org,\n\tlinux-kernel@vger.kernel.org","References":"\n\t<5c8b83775b982e6ee851c127444a8e839f422ad0.1506377430.git.digetx@gmail.com>\n\t<38daee0d-0595-a7e4-69bb-5a2ddbd832b5@wwwdotorg.org>\n\t\n\t<52f5c05f-7060-4d0f-355f-6da372c9ded0@wwwdotorg.org>","From":"Dmitry Osipenko ","Message-ID":"<113675da-6b85-ef2f-03a9-84e5cb93c31b@gmail.com>","Date":"Tue, 26 Sep 2017 15:02:44 +0300","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<52f5c05f-7060-4d0f-355f-6da372c9ded0@wwwdotorg.org>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Sender":"linux-tegra-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-tegra@vger.kernel.org"}},{"id":1776192,"web_url":"http://patchwork.ozlabs.org/comment/1776192/","msgid":"<20170927094557.yr2fl5arodjnh637@mwanda>","list_archive_url":null,"date":"2017-09-27T09:45:58","subject":"Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder\n\tdriver","submitter":{"id":9327,"url":"http://patchwork.ozlabs.org/api/people/9327/","name":"Dan Carpenter","email":"dan.carpenter@oracle.com"},"content":"I feel like this code is good enough to go into the regular kernel\ninstead of staging, but I don't really know what \"- properly handle\ndecoding faults\" means in this context. Does the driver Oops all the\ntime or what?\n\nAnyway, minor comments inline.\n\nOn Tue, Sep 26, 2017 at 01:15:42AM +0300, Dmitry Osipenko wrote:\n> diff --git a/drivers/staging/tegra-vde/Kconfig b/drivers/staging/tegra-vde/Kconfig\n> new file mode 100644\n> index 000000000000..b947c012a373\n> --- /dev/null\n> +++ b/drivers/staging/tegra-vde/Kconfig\n> @@ -0,0 +1,6 @@\n> +config TEGRA_VDE\n> +\ttristate \"NVIDIA Tegra20 video decoder driver\"\n> +\tdepends on ARCH_TEGRA_2x_SOC\n\nCould we get a || COMPILE_TEST here as well?\n\n> +\thelp\n> +\t Say Y here to enable support for a NVIDIA Tegra20 video decoder\n> +\t driver.\n> diff --git a/drivers/staging/tegra-vde/Makefile b/drivers/staging/tegra-vde/Makefile\n> new file mode 100644\n> index 000000000000..e7c0df1174bf\n> --- /dev/null\n> +++ b/drivers/staging/tegra-vde/Makefile\n> @@ -0,0 +1 @@\n> +obj-$(CONFIG_TEGRA_VDE)\t+= vde.o\n> diff --git a/drivers/staging/tegra-vde/TODO b/drivers/staging/tegra-vde/TODO\n> new file mode 100644\n> index 000000000000..533ddfc5190e\n> --- /dev/null\n> +++ b/drivers/staging/tegra-vde/TODO\n> @@ -0,0 +1,8 @@\n> +All TODO's require reverse-engineering to be done first, it is very\n> +unlikely that NVIDIA would ever release HW specs to public.\n> +\n> +TODO:\n> +\t- properly handle decoding faults\n> +\t- support more formats\n\nAdding more formats is not a reason to put this into staging instead of\nthe normal drivers/ dir.\n\n> +static int tegra_vde_setup_context(struct tegra_vde *vde,\n> +\t\t\t\t struct tegra_vde_h264_decoder_ctx *ctx,\n> +\t\t\t\t struct video_frame *dpb_frames,\n> +\t\t\t\t phys_addr_t bitstream_data_paddr,\n> +\t\t\t\t int bitstream_data_size,\n> +\t\t\t\t int macroblocks_nb)\n> +{\n> +\tstruct device *dev = vde->miscdev.parent;\n> +\tu32 value;\n> +\n> +\ttegra_vde_set_bits(vde->regs, 0xA, SXE(0xF0));\n> +\ttegra_vde_set_bits(vde->regs, 0xB, BSEV(CMDQUE_CONTROL));\n> +\ttegra_vde_set_bits(vde->regs, 0x8002, MBE(0x50));\n> +\ttegra_vde_set_bits(vde->regs, 0xA, MBE(0xA0));\n> +\ttegra_vde_set_bits(vde->regs, 0xA, PPE(0x14));\n> +\ttegra_vde_set_bits(vde->regs, 0xA, PPE(0x28));\n> +\ttegra_vde_set_bits(vde->regs, 0xA00, MCE(0x08));\n> +\ttegra_vde_set_bits(vde->regs, 0xA, TFE(0x00));\n> +\ttegra_vde_set_bits(vde->regs, 0x5, VDMA(0x04));\n> +\n> +\tVDE_WR(0x00000000, vde->regs + VDMA(0x1C));\n> +\tVDE_WR(0x00000000, vde->regs + VDMA(0x00));\n> +\tVDE_WR(0x00000007, vde->regs + VDMA(0x04));\n> +\tVDE_WR(0x00000007, vde->regs + FRAMEID(0x200));\n> +\tVDE_WR(0x00000005, vde->regs + TFE(0x04));\n> +\tVDE_WR(0x00000000, vde->regs + MBE(0x84));\n> +\tVDE_WR(0x00000010, vde->regs + SXE(0x08));\n> +\tVDE_WR(0x00000150, vde->regs + SXE(0x54));\n> +\tVDE_WR(0x0000054C, vde->regs + SXE(0x58));\n> +\tVDE_WR(0x00000E34, vde->regs + SXE(0x5C));\n> +\tVDE_WR(0x063C063C, vde->regs + MCE(0x10));\n> +\tVDE_WR(0x0003FC00, vde->regs + BSEV(INTR_STATUS));\n> +\tVDE_WR(0x0000150D, vde->regs + BSEV(BSE_CONFIG));\n> +\tVDE_WR(0x00000100, vde->regs + BSEV(BSE_INT_ENB));\n> +\tVDE_WR(0x00000000, vde->regs + BSEV(0x98));\n> +\tVDE_WR(0x00000060, vde->regs + BSEV(0x9C));\n> +\n> +\tmemset_io(vde->iram + 512, 0, macroblocks_nb / 2);\n> +\n> +\ttegra_setup_frameidx(vde->regs, dpb_frames, ctx->dpb_frames_nb,\n> +\t\t\t ctx->pic_width_in_mbs, ctx->pic_height_in_mbs);\n> +\n> +\ttegra_vde_setup_iram_tables(vde->iram, dpb_frames,\n> +\t\t\t\t ctx->dpb_frames_nb - 1,\n> +\t\t\t\t ctx->dpb_ref_frames_with_earlier_poc_nb);\n> +\n> +\tVDE_WR(0x00000000, vde->regs + BSEV(0x8C));\n> +\tVDE_WR(bitstream_data_paddr + bitstream_data_size,\n> +\t vde->regs + BSEV(0x54));\n> +\n> +\tvalue = ctx->pic_width_in_mbs << 11 | ctx->pic_height_in_mbs << 3;\n> +\n> +\tVDE_WR(value, vde->regs + BSEV(0x88));\n> +\n> +\tif (tegra_vde_wait_bsev(vde, false))\n> +\t\treturn -EIO;\n> +\n> +\tif (tegra_vde_push_bsev_icmdqueue(vde, 0x800003FC, false))\n\nPreserve the error code from tegra_vde_push_bsev_icmdqueue(). It's\nstill -EIO so this doesn't affect runtime.\n\n> +\t\treturn -EIO;\n> +\n> +\tvalue = 0x01500000;\n> +\tvalue |= ((vde->iram_lists_paddr + 512) >> 2) & 0xFFFF;\n> +\n> +\tif (tegra_vde_push_bsev_icmdqueue(vde, value, true))\n\nSame for all.\n\n> +\t\treturn -EIO;\n> +\n> +\tif (tegra_vde_push_bsev_icmdqueue(vde, 0x840F054C, false))\n> +\t\treturn -EIO;\n> +\n> +\tif (tegra_vde_push_bsev_icmdqueue(vde, 0x80000080, false))\n> +\t\treturn -EIO;\n> +\n> +\tvalue = 0x0E340000 | ((vde->iram_lists_paddr >> 2) & 0xFFFF);\n> +\n> +\tif (tegra_vde_push_bsev_icmdqueue(vde, value, true))\n> +\t\treturn -EIO;\n> +\n> +\tvalue = (1 << 23) | 5;\n\nI don't like these magic numbers.\n\n> +\tvalue |= ctx->pic_width_in_mbs << 11;\n> +\tvalue |= ctx->pic_height_in_mbs << 3;\n> +\n> +\tVDE_WR(value, vde->regs + SXE(0x10));\n> +\n> +\tvalue = !ctx->is_baseline_profile << 17;\n> +\tvalue |= ctx->level_idc << 13;\n> +\tvalue |= ctx->log2_max_pic_order_cnt_lsb << 7;\n> +\tvalue |= ctx->pic_order_cnt_type << 5;\n> +\tvalue |= ctx->log2_max_frame_num;\n> +\n> +\tVDE_WR(value, vde->regs + SXE(0x40));\n> +\n> +\tvalue = ctx->pic_init_qp << 25;\n> +\tvalue |= !!(ctx->deblocking_filter_control_present_flag) << 2;\n> +\tvalue |= !!ctx->pic_order_present_flag;\n> +\n> +\tVDE_WR(value, vde->regs + SXE(0x44));\n> +\n> +\tvalue = ctx->chroma_qp_index_offset;\n> +\tvalue |= ctx->num_ref_idx_l0_active_minus1 << 5;\n> +\tvalue |= ctx->num_ref_idx_l1_active_minus1 << 10;\n> +\tvalue |= !!ctx->constrained_intra_pred_flag << 15;\n> +\n> +\tVDE_WR(value, vde->regs + SXE(0x48));\n> +\n> +\tvalue = 0x0C000000;\n> +\tvalue |= !!(dpb_frames[0].flags & FLAG_IS_B_FRAME) << 24;\n> +\n> +\tVDE_WR(value, vde->regs + SXE(0x4C));\n> +\n> +\tvalue = 0x03800000;\n> +\tvalue |= min(bitstream_data_size, SZ_1M);\n> +\n> +\tVDE_WR(value, vde->regs + SXE(0x68));\n> +\n> +\tVDE_WR(bitstream_data_paddr, vde->regs + SXE(0x6C));\n> +\n> +\tvalue = (1 << 28) | 5;\n> +\tvalue |= ctx->pic_width_in_mbs << 11;\n> +\tvalue |= ctx->pic_height_in_mbs << 3;\n> +\n> +\tVDE_WR(value, vde->regs + MBE(0x80));\n> +\n> +\tvalue = 0x26800000;\n> +\tvalue |= ctx->level_idc << 4;\n> +\tvalue |= !ctx->is_baseline_profile << 1;\n> +\tvalue |= !!ctx->direct_8x8_inference_flag;\n> +\n> +\tVDE_WR(value, vde->regs + MBE(0x80));\n> +\n> +\tVDE_WR(0xF4000001, vde->regs + MBE(0x80));\n> +\tVDE_WR(0x20000000, vde->regs + MBE(0x80));\n> +\tVDE_WR(0xF4000101, vde->regs + MBE(0x80));\n> +\n> +\tvalue = 0x20000000;\n> +\tvalue |= ctx->chroma_qp_index_offset << 8;\n> +\n> +\tVDE_WR(value, vde->regs + MBE(0x80));\n> +\n> +\tif (tegra_vde_setup_mbe_frame_idx(vde->regs,\n> +\t\t\t\t\t ctx->pic_order_cnt_type == 0,\n> +\t\t\t\t\t ctx->dpb_frames_nb - 1)) {\n\n\nPreserve the error code.\n\n> +\t\tdev_err(dev, \"MBE frames setup failed\\n\");\n> +\t\treturn -EIO;\n> +\t}\n> +\n> +\ttegra_vde_mbe_set_0xa_reg(vde->regs, 0, 0x000009FC);\n> +\ttegra_vde_mbe_set_0xa_reg(vde->regs, 2, 0xF1DEAD00);\n> +\ttegra_vde_mbe_set_0xa_reg(vde->regs, 4, 0xF2DEAD00);\n> +\ttegra_vde_mbe_set_0xa_reg(vde->regs, 6, 0xF3DEAD00);\n> +\ttegra_vde_mbe_set_0xa_reg(vde->regs, 8, dpb_frames[0].aux_paddr);\n> +\n> +\tvalue = 0xFC000000;\n> +\tvalue |= !!(dpb_frames[0].flags & FLAG_IS_B_FRAME) << 2;\n> +\n> +\tif (!ctx->is_baseline_profile)\n> +\t\tvalue |= !!(dpb_frames[0].flags & FLAG_IS_REFERENCE) << 1;\n> +\n> +\tVDE_WR(value, vde->regs + MBE(0x80));\n> +\n> +\tif (tegra_vde_wait_mbe(vde->regs)) {\n> +\t\tdev_err(dev, \"MBE programming failed\\n\");\n> +\t\treturn -EIO;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +static void tegra_vde_decode_frame(struct tegra_vde *vde, int macroblocks_nb)\n> +{\n> +\tVDE_WR(0x00000001, vde->regs + BSEV(0x8C));\n> +\tVDE_WR(0x20000000 | (macroblocks_nb - 1), vde->regs + SXE(0x00));\n> +}\n> +\n> +static void tegra_vde_detach_and_put_dmabuf(struct dma_buf_attachment *a)\n> +{\n> +\tstruct dma_buf *dmabuf = a->dmabuf;\n> +\n> +\tif (IS_ERR_OR_NULL(a))\n\nYou just dereferenced this on the line before so it's too late.\n\nObviously we don't want to dereference either an error pointer or a NULL\nbut I'm wondering the significance of having it be both. Normally that\nwould mean that NULL is a special case of success. In other words,\nerror pointer means the hardware is broken but NULL means it's missing\nand not required. I guess I'm suggesting to just delete the check and\nmake sure we never set this to either NULL or ERR_PTR.\n\n> +\t\treturn;\n> +\n> +\tdma_buf_detach(dmabuf, a);\n> +\tdma_buf_put(dmabuf);\n> +}\n> +\n> +static int tegra_vde_attach_dmabuf(struct device *dev, int fd,\n> +\t\t\t\t unsigned long offset, int min_size,\n> +\t\t\t\t struct dma_buf_attachment **a,\n> +\t\t\t\t phys_addr_t *paddr, u32 *size,\n> +\t\t\t\t enum dma_data_direction dma_dir)\n> +{\n> +\tstruct dma_buf_attachment *attachment;\n> +\tstruct dma_buf *dmabuf;\n> +\tstruct sg_table *sgt;\n> +\n> +\t*a = NULL;\n> +\t*paddr = 0xFBDEAD00;\n> +\n> +\tdmabuf = dma_buf_get(fd);\n> +\tif (IS_ERR(dmabuf)) {\n> +\t\tdev_err(dev, \"Invalid dmabuf FD\\n\");\n> +\t\treturn PTR_ERR(dmabuf);\n> +\t}\n> +\n> +\tif ((u64)offset + min_size > dmabuf->size) {\n> +\t\tdev_err(dev, \"Too small dmabuf size %d @0x%lX, \"\n> +\t\t\t \"should be at least %d\\n\",\n> +\t\t\tdmabuf->size, offset, min_size);\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tattachment = dma_buf_attach(dmabuf, dev);\n> +\tif (IS_ERR(attachment)) {\n> +\t\tdev_err(dev, \"Failed to attach dmabuf\\n\");\n> +\t\tdma_buf_put(dmabuf);\n> +\t\treturn PTR_ERR(attachment);\n> +\t}\n> +\n> +\tsgt = dma_buf_map_attachment(attachment, dma_dir);\n> +\tif (IS_ERR(sgt)) {\n> +\t\tdev_err(dev, \"Failed to get dmabuf sg_table\\n\");\n> +\t\tdma_buf_detach(dmabuf, attachment);\n> +\t\tdma_buf_put(dmabuf);\n> +\t\treturn PTR_ERR(sgt);\n> +\t}\n> +\n> +\tif (sgt->nents != 1) {\n> +\t\tdev_err(dev, \"Sparsed DMA area is unsupported\\n\");\n\ns/Sparsed/Sparse/\n\n> +\t\tdma_buf_unmap_attachment(attachment, sgt, dma_dir);\n> +\t\tdma_buf_detach(dmabuf, attachment);\n> +\t\tdma_buf_put(dmabuf);\n> +\t\treturn -EINVAL;\n\nThis function would be cleaner using gotos to unwind. Pick the goto\nname to reflect what the goto does. For example, here it would be:\n\n\tif (sgt->nents != 1) {\n\t\tdev_err(dev, \"Sparse DMA area is unsupported\\n\");\n\t\tret = -EINVAL;\n\t\tgoto err_umap;\n\t}\n\n\n\n> +\t}\n> +\n> +\t*paddr = sg_dma_address(sgt->sgl) + offset;\n> +\n> +\tdma_buf_unmap_attachment(attachment, sgt, dma_dir);\n> +\n> +\t*a = attachment;\n> +\n> +\tif (size)\n> +\t\t*size = dmabuf->size - offset;\n> +\n> +\treturn 0;\n\n\treturn 0;\n\nerr_unmap:\n\tdma_buf_unmap_attachment(attachment, sgt, dma_dir);\nerr_detach:\n\tdma_buf_detach(dmabuf, attachment);\nerr_put:\n\tdma_buf_put(dmabuf);\n\treturn ret;\n\n> +}\n> +\n> +static int tegra_vde_attach_frame_dmabufs(struct device *dev,\n> +\t\t\t\t\t struct video_frame *frame,\n> +\t\t\t\t\t struct tegra_vde_h264_frame *source,\n> +\t\t\t\t\t enum dma_data_direction dma_dir,\n> +\t\t\t\t\t int is_baseline_profile, int csize)\n> +{\n> +\tint ret;\n> +\n> +\tret = tegra_vde_attach_dmabuf(dev, source->y_fd,\n> +\t\t\t\t source->y_offset, csize * 4,\n> +\t\t\t\t &frame->y_dmabuf_attachment,\n> +\t\t\t\t &frame->y_paddr, NULL, dma_dir);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tret = tegra_vde_attach_dmabuf(dev, source->cb_fd,\n> +\t\t\t\t source->cb_offset, csize,\n> +\t\t\t\t &frame->cb_dmabuf_attachment,\n> +\t\t\t\t &frame->cb_paddr, NULL, dma_dir);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tret = tegra_vde_attach_dmabuf(dev, source->cr_fd,\n> +\t\t\t\t source->cr_offset, csize,\n> +\t\t\t\t &frame->cr_dmabuf_attachment,\n> +\t\t\t\t &frame->cr_paddr, NULL, dma_dir);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tif (is_baseline_profile)\n> +\t\tframe->aux_paddr = 0xF4DEAD00;\n\nThe handling of is_baseline_profile is strange to me. It feels like we\nshould always check it before we use ->aux_paddr but we don't ever do\nthat.\n\n> +\telse\n> +\t\tret = tegra_vde_attach_dmabuf(dev, source->aux_fd,\n> +\t\t\t\t\t source->aux_offset, csize,\n> +\t\t\t\t\t &frame->aux_dmabuf_attachment,\n> +\t\t\t\t\t &frame->aux_paddr, NULL, dma_dir);\n\n\nThis function should have some error handling to undo the earlier\nattach calls if the latter ones fail. See below:\n\n\n> +\n> +\treturn ret;\n\n\treturn 0;\n\nerr_detach_cr:\n\ttegra_vde_detach_and_put_dmabuf(frame->cr_dmabuf_attachment);\nerr_detach_cb:\n\ttegra_vde_detach_and_put_dmabuf(frame->cb_dmabuf_attachment);\nerr_detach_y:\n\ttegra_vde_detach_and_put_dmabuf(frame->y_dmabuf_attachment);\n\n\treturn ret;\n\n\n> +}\n> +\n> +static void tegra_vde_deattach_frame_dmabufs(struct video_frame *frame)\n> +{\n> +\ttegra_vde_detach_and_put_dmabuf(frame->y_dmabuf_attachment);\n> +\ttegra_vde_detach_and_put_dmabuf(frame->cb_dmabuf_attachment);\n> +\ttegra_vde_detach_and_put_dmabuf(frame->cr_dmabuf_attachment);\n> +\ttegra_vde_detach_and_put_dmabuf(frame->aux_dmabuf_attachment);\n\n\nIt would be happier to write this in the reverse order so it matches\nthe error handling that I wrote for you.\n\n\n> +}\n> +\n> +static int tegra_vde_copy_and_validate_frame(struct device *dev,\n> +\t\t\t\t\t struct tegra_vde_h264_frame *frame,\n> +\t\t\t\t\t unsigned long vaddr)\n> +{\n> +\tif (copy_from_user(frame, (void __user *)vaddr, sizeof(*frame))) {\n> +\t\tdev_err(dev, \"Copying of H.264 frame from user failed\\n\");\n> +\t\treturn -EFAULT;\n\nError message are a funny thing and different people feel different\nways. These can be triggered by the user so they let you spam dmesg\nbut I can see how many of them would be useful. These ones for\ncopy_from_user() are not useful since we assume the programmer should\nknow that stuff. The error code seems enough.\n\n> +\t}\n> +\n> +\tif (frame->frame_num > 0x7FFFFF) {\n> +\t\tdev_err(dev, \"Bad frame_num %u\\n\", frame->frame_num);\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tif (frame->y_offset & 0xFF) {\n> +\t\tdev_err(dev, \"Bad y_offset 0x%X\\n\", frame->y_offset);\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tif (frame->cb_offset & 0xFF) {\n> +\t\tdev_err(dev, \"Bad cb_offset 0x%X\\n\", frame->cb_offset);\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tif (frame->cr_offset & 0xFF) {\n> +\t\tdev_err(dev, \"Bad cr_offset 0x%X\\n\", frame->cr_offset);\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +static int tegra_vde_validate_h264_ctx(struct device *dev,\n> +\t\t\t\t struct tegra_vde_h264_decoder_ctx *ctx)\n> +{\n> +\tif (ctx->dpb_frames_nb == 0 || ctx->dpb_frames_nb > 17) {\n> +\t\tdev_err(dev, \"Bad DPB size %u\\n\", ctx->dpb_frames_nb);\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tif (ctx->level_idc > 15) {\n> +\t\tdev_err(dev, \"Bad level value %u\\n\", ctx->level_idc);\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tif (ctx->pic_init_qp > 52) {\n> +\t\tdev_err(dev, \"Bad pic_init_qp value %u\\n\", ctx->pic_init_qp);\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tif (ctx->log2_max_pic_order_cnt_lsb > 16) {\n> +\t\tdev_err(dev, \"Bad log2_max_pic_order_cnt_lsb value %u\\n\",\n> +\t\t\tctx->log2_max_pic_order_cnt_lsb);\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tif (ctx->log2_max_frame_num > 16) {\n> +\t\tdev_err(dev, \"Bad log2_max_frame_num value %u\\n\",\n> +\t\t\tctx->log2_max_frame_num);\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tif (ctx->chroma_qp_index_offset > 31) {\n> +\t\tdev_err(dev, \"Bad chroma_qp_index_offset value %u\\n\",\n> +\t\t\tctx->chroma_qp_index_offset);\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tif (ctx->pic_order_cnt_type > 2) {\n> +\t\tdev_err(dev, \"Bad pic_order_cnt_type value %u\\n\",\n> +\t\t\tctx->pic_order_cnt_type);\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tif (ctx->num_ref_idx_l0_active_minus1 > 15) {\n> +\t\tdev_err(dev, \"Bad num_ref_idx_l0_active_minus1 value %u\\n\",\n> +\t\t\tctx->num_ref_idx_l0_active_minus1);\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tif (ctx->num_ref_idx_l1_active_minus1 > 15) {\n> +\t\tdev_err(dev, \"Bad num_ref_idx_l1_active_minus1 value %u\\n\",\n> +\t\t\tctx->num_ref_idx_l1_active_minus1);\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tif (!ctx->pic_width_in_mbs || ctx->pic_width_in_mbs > 127) {\n> +\t\tdev_err(dev, \"Bad pic_width_in_mbs value %u, min 1 max 127\\n\",\n> +\t\t\tctx->pic_width_in_mbs);\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tif (!ctx->pic_height_in_mbs || ctx->pic_height_in_mbs > 127) {\n> +\t\tdev_err(dev, \"Bad pic_height_in_mbs value %u, min 1 max 127\\n\",\n> +\t\t\tctx->pic_height_in_mbs);\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +static int tegra_vde_ioctl_decode_h264(struct tegra_vde *vde,\n> +\t\t\t\t unsigned long vaddr)\n> +{\n> +\tstruct tegra_vde_h264_decoder_ctx ctx;\n> +\tstruct tegra_vde_h264_frame frame;\n> +\tstruct device *dev = vde->miscdev.parent;\n> +\tstruct video_frame *dpb_frames = NULL;\n> +\tstruct dma_buf_attachment *bitstream_data_dmabuf_attachment = NULL;\n> +\tenum dma_data_direction dma_dir;\n> +\tphys_addr_t bitstream_data_paddr;\n> +\tphys_addr_t bsev_paddr;\n> +\tu32 bitstream_data_size;\n> +\tu32 macroblocks_nb;\n> +\tbool timeout = false;\n> +\tint i, ret;\n> +\n> +\tif (copy_from_user(&ctx, (void __user *)vaddr, sizeof(ctx))) {\n> +\t\tdev_err(dev, \"Copying of H.264 CTX from user failed\\n\");\n> +\t\treturn -EFAULT;\n> +\t}\n> +\n> +\tret = tegra_vde_validate_h264_ctx(dev, &ctx);\n> +\tif (ret)\n> +\t\treturn -EINVAL;\n> +\n> +\tret = tegra_vde_attach_dmabuf(dev, ctx.bitstream_data_fd,\n> +\t\t\t\t ctx.bitstream_data_offset, 0,\n> +\t\t\t\t &bitstream_data_dmabuf_attachment,\n> +\t\t\t\t &bitstream_data_paddr,\n> +\t\t\t\t &bitstream_data_size,\n> +\t\t\t\t DMA_TO_DEVICE);\n> +\tif (ret)\n> +\t\tgoto cleanup;\n\n\nI hate this label name. What are we cleaning up??? Now I have to set a\nbookmark so I can remember where I left and then scroll down to the\nbottom of the function and take a look.\n\n[pause]\n\nOK. I'm back. I call this \"one err\" style error handling. And it's\nvery bug prone. Please read my essay on the topic:\n\nhttps://plus.google.com/106378716002406849458/posts/dnanfhQ4mHQ\n\nThe bug is that we call tegra_vde_detach_and_put_dmabuf() with a NULL\npointer and there was that dereference before check that I mentioned\nearlier.\n\n> +\n> +\tdpb_frames = kcalloc(ctx.dpb_frames_nb, sizeof(*dpb_frames),\n> +\t\t\t GFP_KERNEL);\n> +\tif (!dpb_frames) {\n> +\t\tret = -ENOMEM;\n> +\t\tgoto cleanup;\n> +\t}\n> +\n> +\tmacroblocks_nb = ctx.pic_width_in_mbs * ctx.pic_height_in_mbs;\n> +\tvaddr = ctx.dpb_frames_ptr;\n> +\n> +\tfor (i = 0; i < ctx.dpb_frames_nb; i++) {\n> +\t\tret = tegra_vde_copy_and_validate_frame(dev, &frame, vaddr);\n> +\t\tif (ret)\n> +\t\t\tgoto cleanup;\n> +\n> +\t\tdpb_frames[i].flags = frame.flags;\n> +\t\tdpb_frames[i].frame_num = frame.frame_num;\n> +\n> +\t\tdma_dir = (i == 0) ? DMA_FROM_DEVICE : DMA_TO_DEVICE;\n> +\n> +\t\tret = tegra_vde_attach_frame_dmabufs(dev,\n> +\t\t\t\t\t\t &dpb_frames[i], &frame,\n> +\t\t\t\t\t\t dma_dir,\n> +\t\t\t\t\t\t ctx.is_baseline_profile,\n> +\t\t\t\t\t\t macroblocks_nb * 64);\n> +\t\tif (ret) {\n> +\t\t\ttegra_vde_deattach_frame_dmabufs(&dpb_frames[i]);\n> +\t\t\tgoto cleanup;\n> +\t\t}\n> +\n> +\t\tvaddr += sizeof(frame);\n> +\t}\n> +\n> +\tret = clk_prepare_enable(vde->clk);\n> +\tif (ret) {\n> +\t\tdev_err(dev, \"Failed to enable clk: %d\\n\", ret);\n> +\t\tgoto cleanup;\n> +\t}\n> +\n> +\tret = mutex_lock_interruptible(&vde->lock);\n> +\tif (ret)\n> +\t\tgoto clkgate;\n> +\n> +\tret = reset_control_deassert(vde->rst);\n> +\tif (ret) {\n> +\t\tdev_err(dev, \"Failed to deassert reset: %d\\n\", ret);\n> +\t\tclk_disable_unprepare(vde->clk);\n\nWe do a second clk_disable_unprepare(vde->clk); after the unlock.\nDelete this?\n\n> +\t\tgoto unlock;\n> +\t}\n> +\n> +\tret = tegra_vde_setup_context(vde, &ctx, dpb_frames,\n> +\t\t\t\t bitstream_data_paddr,\n> +\t\t\t\t bitstream_data_size,\n> +\t\t\t\t macroblocks_nb);\n> +\tif (ret)\n> +\t\tgoto reset;\n> +\n> +\ttegra_vde_decode_frame(vde, macroblocks_nb);\n> +\n> +\ttimeout = !wait_for_completion_io_timeout(&vde->decode_completion,\n> +\t\t\t\t\t\t TEGRA_VDE_TIMEOUT);\n> +\tif (timeout) {\n> +\t\tbsev_paddr = readl(vde->regs + BSEV(0x10));\n> +\t\tmacroblocks_nb = readl(vde->regs + SXE(0xC8)) & 0x1FFF;\n> +\n> +\t\tdev_err(dev, \"Decoding failed, \"\n> +\t\t\t\t\"read 0x%X bytes : %u macroblocks parsed\\n\",\n> +\t\t\tbsev_paddr ? bsev_paddr - bitstream_data_paddr : 0,\n> +\t\t\tmacroblocks_nb);\n> +\t}\n> +\n> +reset:\n> +\t/*\n> +\t * We rely on the VDE registers reset value, otherwise VDE would\n> +\t * cause bus lockup.\n> +\t */\n> +\tret = reset_control_assert(vde->rst);\n> +\tif (ret)\n> +\t\tdev_err(dev, \"Failed to assert reset: %d\\n\", ret);\n\nWe're overwriting \"ret\" here when we probably want to preserve the error\ncode from reset_control_deassert().\n\n> +\n> +\tif (timeout)\n> +\t\tret = -EIO;\n> +\n> +unlock:\n> +\tmutex_unlock(&vde->lock);\n> +\n> +clkgate:\n> +\tclk_disable_unprepare(vde->clk);\n> +\n> +cleanup:\n> +\tif (dpb_frames)\n> +\t\twhile (i--)\n> +\t\t\ttegra_vde_deattach_frame_dmabufs(&dpb_frames[i]);\n> +\n> +\tkfree(dpb_frames);\n> +\n> +\ttegra_vde_detach_and_put_dmabuf(bitstream_data_dmabuf_attachment);\n> +\n> +\treturn ret;\n> +}\n> +\n> +static long tegra_vde_unlocked_ioctl(struct file *filp,\n> +\t\t\t\t unsigned int cmd, unsigned long arg)\n> +{\n> +\tstruct miscdevice *miscdev = filp->private_data;\n> +\tstruct tegra_vde *vde = container_of(miscdev, struct tegra_vde,\n> +\t\t\t\t\t miscdev);\n> +\n> +\tswitch (cmd) {\n> +\tcase TEGRA_VDE_IOCTL_DECODE_H264:\n> +\t\treturn tegra_vde_ioctl_decode_h264(vde, arg);\n> +\t}\n> +\n> +\tdev_err(miscdev->parent, \"Invalid IOCTL command %u\\n\", cmd);\n> +\n> +\treturn -ENOTTY;\n> +}\n> +\n> +static const struct file_operations tegra_vde_fops = {\n> +\t.owner\t\t= THIS_MODULE,\n> +\t.unlocked_ioctl\t= tegra_vde_unlocked_ioctl,\n> +};\n> +\n> +static irqreturn_t tegra_vde_isr(int irq, void *data)\n> +{\n> +\tstruct tegra_vde *vde = data;\n> +\n> +\ttegra_vde_set_bits(vde->regs, 0, FRAMEID(0x208));\n> +\tcomplete(&vde->decode_completion);\n> +\n> +\treturn IRQ_HANDLED;\n> +}\n> +\n> +static int tegra_vde_probe(struct platform_device *pdev)\n> +{\n> +\tstruct resource *res_regs, *res_iram;\n> +\tstruct device *dev = &pdev->dev;\n> +\tstruct tegra_vde *vde;\n> +\tint ret;\n> +\n> +\tvde = devm_kzalloc(&pdev->dev, sizeof(*vde), GFP_KERNEL);\n> +\tif (!vde)\n> +\t\treturn -ENOMEM;\n> +\n> +\tplatform_set_drvdata(pdev, vde);\n> +\n> +\tres_regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, \"regs\");\n> +\tif (!res_regs)\n> +\t\treturn -ENODEV;\n> +\n> +\tres_iram = platform_get_resource_byname(pdev, IORESOURCE_MEM, \"iram\");\n> +\tif (!res_iram)\n> +\t\treturn -ENODEV;\n> +\n> +\tvde->irq = platform_get_irq_byname(pdev, \"sync-token\");\n> +\tif (vde->irq < 0)\n> +\t\treturn vde->irq;\n> +\n> +\tvde->regs = devm_ioremap_resource(dev, res_regs);\n> +\tif (IS_ERR(vde->regs))\n> +\t\treturn PTR_ERR(vde->regs);\n> +\n> +\tvde->iram = devm_ioremap_resource(dev, res_iram);\n> +\tif (IS_ERR(vde->iram))\n> +\t\treturn PTR_ERR(vde->iram);\n> +\n> +\tvde->clk = devm_clk_get(dev, NULL);\n> +\tif (IS_ERR(vde->clk)) {\n> +\t\tdev_err(dev, \"Could not get VDE clk\\n\");\n> +\t\treturn PTR_ERR(vde->clk);\n> +\t}\n> +\n> +\tvde->rst = devm_reset_control_get(dev, \"vde\");\n> +\tif (IS_ERR(vde->rst)) {\n> +\t\tdev_err(dev, \"Could not get VDE reset\\n\");\n> +\t\treturn PTR_ERR(vde->rst);\n> +\t}\n> +\n> +\tret = devm_request_irq(dev, vde->irq, tegra_vde_isr, IRQF_SHARED,\n> +\t\t\t dev_name(dev), vde);\n> +\tif (ret)\n> +\t\treturn -ENODEV;\n\nPresever the error code.\n\n> +\n> +\tret = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_VDEC,\n> +\t\t\t\t\t\tvde->clk, vde->rst);\n> +\tif (ret) {\n> +\t\tdev_err(&pdev->dev, \"Failed to power up VDE unit\\n\");\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tret = reset_control_assert(vde->rst);\n> +\tif (ret) {\n> +\t\tdev_err(dev, \"Failed to assert reset: %d\\n\", ret);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tclk_disable_unprepare(vde->clk);\n> +\n> +\tmutex_init(&vde->lock);\n> +\tinit_completion(&vde->decode_completion);\n> +\n> +\tvde->iram_lists_paddr = res_iram->start;\n> +\tvde->miscdev.minor = MISC_DYNAMIC_MINOR;\n> +\tvde->miscdev.name = \"tegra_vde\";\n> +\tvde->miscdev.fops = &tegra_vde_fops;\n> +\tvde->miscdev.parent = dev;\n> +\n> +\tret = misc_register(&vde->miscdev);\n> +\tif (ret)\n> +\t\treturn -ENODEV;\n\nPreserve the error code.\n\n> +\n> +\treturn 0;\n> +}\n> +\n\nregards,\ndan carpenter\n\n\n--\nTo unsubscribe from this list: send the line \"unsubscribe linux-tegra\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-tegra-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y2CdW3BQYz9sBd\n\tfor ;\n\tWed, 27 Sep 2017 19:46:39 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752331AbdI0JqY (ORCPT );\n\tWed, 27 Sep 2017 05:46:24 -0400","from userp1040.oracle.com ([156.151.31.81]:26233 \"EHLO\n\tuserp1040.oracle.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751968AbdI0JqW (ORCPT\n\t);\n\tWed, 27 Sep 2017 05:46:22 -0400","from userv0021.oracle.com (userv0021.oracle.com [156.151.31.71])\n\tby userp1040.oracle.com (Sentrion-MTA-4.3.2/Sentrion-MTA-4.3.2) with\n\tESMTP id v8R9kCgT013493\n\t(version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256\n\tverify=OK); Wed, 27 Sep 2017 09:46:13 GMT","from aserv0121.oracle.com (aserv0121.oracle.com [141.146.126.235])\n\tby userv0021.oracle.com (8.14.4/8.14.4) with ESMTP id\n\tv8R9kAC0021519\n\t(version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256\n\tverify=OK); Wed, 27 Sep 2017 09:46:11 GMT","from abhmp0006.oracle.com (abhmp0006.oracle.com [141.146.116.12])\n\tby aserv0121.oracle.com (8.14.4/8.13.8) with ESMTP id\n\tv8R9k9t2031270; Wed, 27 Sep 2017 09:46:10 GMT","from mwanda (/197.254.35.146)\n\tby default (Oracle Beehive Gateway v4.0)\n\twith ESMTP ; Wed, 27 Sep 2017 02:46:07 -0700"],"Date":"Wed, 27 Sep 2017 12:45:58 +0300","From":"Dan Carpenter ","To":"Dmitry Osipenko ","Cc":"Thierry Reding ,\n\tJonathan Hunter ,\n\tGreg Kroah-Hartman ,\n\tRob Herring , linux-tegra@vger.kernel.org,\n\tdevel@driverdev.osuosl.org, linux-kernel@vger.kernel.org,\n\tdevicetree@vger.kernel.org","Subject":"Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder\n\tdriver","Message-ID":"<20170927094557.yr2fl5arodjnh637@mwanda>","References":"\n\t<5c8b83775b982e6ee851c127444a8e839f422ad0.1506377430.git.digetx@gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<5c8b83775b982e6ee851c127444a8e839f422ad0.1506377430.git.digetx@gmail.com>","User-Agent":"NeoMutt/20170113 (1.7.2)","X-Source-IP":"userv0021.oracle.com [156.151.31.71]","Sender":"linux-tegra-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-tegra@vger.kernel.org"}},{"id":1776693,"web_url":"http://patchwork.ozlabs.org/comment/1776693/","msgid":"<2b77bfa7-1272-3d11-9190-326fa0f85f22@gmail.com>","list_archive_url":null,"date":"2017-09-27T23:28:04","subject":"Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder\n\tdriver","submitter":{"id":18124,"url":"http://patchwork.ozlabs.org/api/people/18124/","name":"Dmitry Osipenko","email":"digetx@gmail.com"},"content":"On 27.09.2017 12:45, Dan Carpenter wrote:\n> I feel like this code is good enough to go into the regular kernel\n> instead of staging, but I don't really know what \"- properly handle\n> decoding faults\" means in this context.\n\nAs Greg pointed out, this patch should be cc'ed to media maintainers for a\nreview. I'll cc them on V2, will see what they would suggest. Yeah, probably the\ndecoding faults isn't a very candidate for a TODO for staging.\n\n Does the driver Oops all the\n> time or what?\n> \n\nDriver works fine.\n\n> Anyway, minor comments inline.\n> \n\nThank you very much for the awesome review. I agree with the most of the comments.\n\n> On Tue, Sep 26, 2017 at 01:15:42AM +0300, Dmitry Osipenko wrote:\n>> diff --git a/drivers/staging/tegra-vde/Kconfig b/drivers/staging/tegra-vde/Kconfig\n>> new file mode 100644\n>> index 000000000000..b947c012a373\n>> --- /dev/null\n>> +++ b/drivers/staging/tegra-vde/Kconfig\n>> @@ -0,0 +1,6 @@\n>> +config TEGRA_VDE\n>> +\ttristate \"NVIDIA Tegra20 video decoder driver\"\n>> +\tdepends on ARCH_TEGRA_2x_SOC\n> \n> Could we get a || COMPILE_TEST here as well?\n> \n>> +\thelp\n>> +\t Say Y here to enable support for a NVIDIA Tegra20 video decoder\n>> +\t driver.\n>> diff --git a/drivers/staging/tegra-vde/Makefile b/drivers/staging/tegra-vde/Makefile\n>> new file mode 100644\n>> index 000000000000..e7c0df1174bf\n>> --- /dev/null\n>> +++ b/drivers/staging/tegra-vde/Makefile\n>> @@ -0,0 +1 @@\n>> +obj-$(CONFIG_TEGRA_VDE)\t+= vde.o\n>> diff --git a/drivers/staging/tegra-vde/TODO b/drivers/staging/tegra-vde/TODO\n>> new file mode 100644\n>> index 000000000000..533ddfc5190e\n>> --- /dev/null\n>> +++ b/drivers/staging/tegra-vde/TODO\n>> @@ -0,0 +1,8 @@\n>> +All TODO's require reverse-engineering to be done first, it is very\n>> +unlikely that NVIDIA would ever release HW specs to public.\n>> +\n>> +TODO:\n>> +\t- properly handle decoding faults\n>> +\t- support more formats\n> \n> Adding more formats is not a reason to put this into staging instead of\n> the normal drivers/ dir.\n> \n\nWell, it feels that the driver isn't very acceptable for the core drivers/. But\nmaybe it's a wrong feeling. Again, will see what media/ maintainers would suggest.\n\n>> +static int tegra_vde_setup_context(struct tegra_vde *vde,\n>> +\t\t\t\t struct tegra_vde_h264_decoder_ctx *ctx,\n>> +\t\t\t\t struct video_frame *dpb_frames,\n>> +\t\t\t\t phys_addr_t bitstream_data_paddr,\n>> +\t\t\t\t int bitstream_data_size,\n>> +\t\t\t\t int macroblocks_nb)\n>> +{\n>> +\tstruct device *dev = vde->miscdev.parent;\n>> +\tu32 value;\n>> +\n>> +\ttegra_vde_set_bits(vde->regs, 0xA, SXE(0xF0));\n>> +\ttegra_vde_set_bits(vde->regs, 0xB, BSEV(CMDQUE_CONTROL));\n>> +\ttegra_vde_set_bits(vde->regs, 0x8002, MBE(0x50));\n>> +\ttegra_vde_set_bits(vde->regs, 0xA, MBE(0xA0));\n>> +\ttegra_vde_set_bits(vde->regs, 0xA, PPE(0x14));\n>> +\ttegra_vde_set_bits(vde->regs, 0xA, PPE(0x28));\n>> +\ttegra_vde_set_bits(vde->regs, 0xA00, MCE(0x08));\n>> +\ttegra_vde_set_bits(vde->regs, 0xA, TFE(0x00));\n>> +\ttegra_vde_set_bits(vde->regs, 0x5, VDMA(0x04));\n>> +\n>> +\tVDE_WR(0x00000000, vde->regs + VDMA(0x1C));\n>> +\tVDE_WR(0x00000000, vde->regs + VDMA(0x00));\n>> +\tVDE_WR(0x00000007, vde->regs + VDMA(0x04));\n>> +\tVDE_WR(0x00000007, vde->regs + FRAMEID(0x200));\n>> +\tVDE_WR(0x00000005, vde->regs + TFE(0x04));\n>> +\tVDE_WR(0x00000000, vde->regs + MBE(0x84));\n>> +\tVDE_WR(0x00000010, vde->regs + SXE(0x08));\n>> +\tVDE_WR(0x00000150, vde->regs + SXE(0x54));\n>> +\tVDE_WR(0x0000054C, vde->regs + SXE(0x58));\n>> +\tVDE_WR(0x00000E34, vde->regs + SXE(0x5C));\n>> +\tVDE_WR(0x063C063C, vde->regs + MCE(0x10));\n>> +\tVDE_WR(0x0003FC00, vde->regs + BSEV(INTR_STATUS));\n>> +\tVDE_WR(0x0000150D, vde->regs + BSEV(BSE_CONFIG));\n>> +\tVDE_WR(0x00000100, vde->regs + BSEV(BSE_INT_ENB));\n>> +\tVDE_WR(0x00000000, vde->regs + BSEV(0x98));\n>> +\tVDE_WR(0x00000060, vde->regs + BSEV(0x9C));\n>> +\n>> +\tmemset_io(vde->iram + 512, 0, macroblocks_nb / 2);\n>> +\n>> +\ttegra_setup_frameidx(vde->regs, dpb_frames, ctx->dpb_frames_nb,\n>> +\t\t\t ctx->pic_width_in_mbs, ctx->pic_height_in_mbs);\n>> +\n>> +\ttegra_vde_setup_iram_tables(vde->iram, dpb_frames,\n>> +\t\t\t\t ctx->dpb_frames_nb - 1,\n>> +\t\t\t\t ctx->dpb_ref_frames_with_earlier_poc_nb);\n>> +\n>> +\tVDE_WR(0x00000000, vde->regs + BSEV(0x8C));\n>> +\tVDE_WR(bitstream_data_paddr + bitstream_data_size,\n>> +\t vde->regs + BSEV(0x54));\n>> +\n>> +\tvalue = ctx->pic_width_in_mbs << 11 | ctx->pic_height_in_mbs << 3;\n>> +\n>> +\tVDE_WR(value, vde->regs + BSEV(0x88));\n>> +\n>> +\tif (tegra_vde_wait_bsev(vde, false))\n>> +\t\treturn -EIO;\n>> +\n>> +\tif (tegra_vde_push_bsev_icmdqueue(vde, 0x800003FC, false))\n> \n> Preserve the error code from tegra_vde_push_bsev_icmdqueue(). It's\n> still -EIO so this doesn't affect runtime.\n> \n\nOkay, I'll propagate error code in all other places as well.\n\n>> +\t\treturn -EIO;\n>> +\n>> +\tvalue = 0x01500000;\n>> +\tvalue |= ((vde->iram_lists_paddr + 512) >> 2) & 0xFFFF;\n>> +\n>> +\tif (tegra_vde_push_bsev_icmdqueue(vde, value, true))\n> \n> Same for all.\n> \n>> +\t\treturn -EIO;\n>> +\n>> +\tif (tegra_vde_push_bsev_icmdqueue(vde, 0x840F054C, false))\n>> +\t\treturn -EIO;\n>> +\n>> +\tif (tegra_vde_push_bsev_icmdqueue(vde, 0x80000080, false))\n>> +\t\treturn -EIO;\n>> +\n>> +\tvalue = 0x0E340000 | ((vde->iram_lists_paddr >> 2) & 0xFFFF);\n>> +\n>> +\tif (tegra_vde_push_bsev_icmdqueue(vde, value, true))\n>> +\t\treturn -EIO;\n>> +\n>> +\tvalue = (1 << 23) | 5;\n> \n> I don't like these magic numbers.\n> \n\nI don't know what these bits do, at best I can replace them with a hex for\nconsistency.\n\n>> +\tvalue |= ctx->pic_width_in_mbs << 11;\n>> +\tvalue |= ctx->pic_height_in_mbs << 3;\n>> +\n>> +\tVDE_WR(value, vde->regs + SXE(0x10));\n>> +\n>> +\tvalue = !ctx->is_baseline_profile << 17;\n>> +\tvalue |= ctx->level_idc << 13;\n>> +\tvalue |= ctx->log2_max_pic_order_cnt_lsb << 7;\n>> +\tvalue |= ctx->pic_order_cnt_type << 5;\n>> +\tvalue |= ctx->log2_max_frame_num;\n>> +\n>> +\tVDE_WR(value, vde->regs + SXE(0x40));\n>> +\n>> +\tvalue = ctx->pic_init_qp << 25;\n>> +\tvalue |= !!(ctx->deblocking_filter_control_present_flag) << 2;\n>> +\tvalue |= !!ctx->pic_order_present_flag;\n>> +\n>> +\tVDE_WR(value, vde->regs + SXE(0x44));\n>> +\n>> +\tvalue = ctx->chroma_qp_index_offset;\n>> +\tvalue |= ctx->num_ref_idx_l0_active_minus1 << 5;\n>> +\tvalue |= ctx->num_ref_idx_l1_active_minus1 << 10;\n>> +\tvalue |= !!ctx->constrained_intra_pred_flag << 15;\n>> +\n>> +\tVDE_WR(value, vde->regs + SXE(0x48));\n>> +\n>> +\tvalue = 0x0C000000;\n>> +\tvalue |= !!(dpb_frames[0].flags & FLAG_IS_B_FRAME) << 24;\n>> +\n>> +\tVDE_WR(value, vde->regs + SXE(0x4C));\n>> +\n>> +\tvalue = 0x03800000;\n>> +\tvalue |= min(bitstream_data_size, SZ_1M);\n>> +\n>> +\tVDE_WR(value, vde->regs + SXE(0x68));\n>> +\n>> +\tVDE_WR(bitstream_data_paddr, vde->regs + SXE(0x6C));\n>> +\n>> +\tvalue = (1 << 28) | 5;\n>> +\tvalue |= ctx->pic_width_in_mbs << 11;\n>> +\tvalue |= ctx->pic_height_in_mbs << 3;\n>> +\n>> +\tVDE_WR(value, vde->regs + MBE(0x80));\n>> +\n>> +\tvalue = 0x26800000;\n>> +\tvalue |= ctx->level_idc << 4;\n>> +\tvalue |= !ctx->is_baseline_profile << 1;\n>> +\tvalue |= !!ctx->direct_8x8_inference_flag;\n>> +\n>> +\tVDE_WR(value, vde->regs + MBE(0x80));\n>> +\n>> +\tVDE_WR(0xF4000001, vde->regs + MBE(0x80));\n>> +\tVDE_WR(0x20000000, vde->regs + MBE(0x80));\n>> +\tVDE_WR(0xF4000101, vde->regs + MBE(0x80));\n>> +\n>> +\tvalue = 0x20000000;\n>> +\tvalue |= ctx->chroma_qp_index_offset << 8;\n>> +\n>> +\tVDE_WR(value, vde->regs + MBE(0x80));\n>> +\n>> +\tif (tegra_vde_setup_mbe_frame_idx(vde->regs,\n>> +\t\t\t\t\t ctx->pic_order_cnt_type == 0,\n>> +\t\t\t\t\t ctx->dpb_frames_nb - 1)) {\n> \n> \n> Preserve the error code.\n> \n>> +\t\tdev_err(dev, \"MBE frames setup failed\\n\");\n>> +\t\treturn -EIO;\n>> +\t}\n>> +\n>> +\ttegra_vde_mbe_set_0xa_reg(vde->regs, 0, 0x000009FC);\n>> +\ttegra_vde_mbe_set_0xa_reg(vde->regs, 2, 0xF1DEAD00);\n>> +\ttegra_vde_mbe_set_0xa_reg(vde->regs, 4, 0xF2DEAD00);\n>> +\ttegra_vde_mbe_set_0xa_reg(vde->regs, 6, 0xF3DEAD00);\n>> +\ttegra_vde_mbe_set_0xa_reg(vde->regs, 8, dpb_frames[0].aux_paddr);\n>> +\n>> +\tvalue = 0xFC000000;\n>> +\tvalue |= !!(dpb_frames[0].flags & FLAG_IS_B_FRAME) << 2;\n>> +\n>> +\tif (!ctx->is_baseline_profile)\n>> +\t\tvalue |= !!(dpb_frames[0].flags & FLAG_IS_REFERENCE) << 1;\n>> +\n>> +\tVDE_WR(value, vde->regs + MBE(0x80));\n>> +\n>> +\tif (tegra_vde_wait_mbe(vde->regs)) {\n>> +\t\tdev_err(dev, \"MBE programming failed\\n\");\n>> +\t\treturn -EIO;\n>> +\t}\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> +static void tegra_vde_decode_frame(struct tegra_vde *vde, int macroblocks_nb)\n>> +{\n>> +\tVDE_WR(0x00000001, vde->regs + BSEV(0x8C));\n>> +\tVDE_WR(0x20000000 | (macroblocks_nb - 1), vde->regs + SXE(0x00));\n>> +}\n>> +\n>> +static void tegra_vde_detach_and_put_dmabuf(struct dma_buf_attachment *a)\n>> +{\n>> +\tstruct dma_buf *dmabuf = a->dmabuf;\n>> +\n>> +\tif (IS_ERR_OR_NULL(a))\n> \n> You just dereferenced this on the line before so it's too late.\n> \n\nIndeed, good catch!\n\n> Obviously we don't want to dereference either an error pointer or a NULL\n> but I'm wondering the significance of having it be both. Normally that\n> would mean that NULL is a special case of success. In other words,\n> error pointer means the hardware is broken but NULL means it's missing\n> and not required. I guess I'm suggesting to just delete the check and\n> make sure we never set this to either NULL or ERR_PTR.\n> \n\nNULL here exactly means that attachment is missing and not required. But anyway\nI'll get rid of IS_ERR_OR_NULL, thanks for the suggestion.\n\n>> +\t\treturn;\n>> +\n>> +\tdma_buf_detach(dmabuf, a);\n>> +\tdma_buf_put(dmabuf);\n>> +}\n>> +\n>> +static int tegra_vde_attach_dmabuf(struct device *dev, int fd,\n>> +\t\t\t\t unsigned long offset, int min_size,\n>> +\t\t\t\t struct dma_buf_attachment **a,\n>> +\t\t\t\t phys_addr_t *paddr, u32 *size,\n>> +\t\t\t\t enum dma_data_direction dma_dir)\n>> +{\n>> +\tstruct dma_buf_attachment *attachment;\n>> +\tstruct dma_buf *dmabuf;\n>> +\tstruct sg_table *sgt;\n>> +\n>> +\t*a = NULL;\n>> +\t*paddr = 0xFBDEAD00;\n>> +\n>> +\tdmabuf = dma_buf_get(fd);\n>> +\tif (IS_ERR(dmabuf)) {\n>> +\t\tdev_err(dev, \"Invalid dmabuf FD\\n\");\n>> +\t\treturn PTR_ERR(dmabuf);\n>> +\t}\n>> +\n>> +\tif ((u64)offset + min_size > dmabuf->size) {\n>> +\t\tdev_err(dev, \"Too small dmabuf size %d @0x%lX, \"\n>> +\t\t\t \"should be at least %d\\n\",\n>> +\t\t\tdmabuf->size, offset, min_size);\n>> +\t\treturn -EINVAL;\n>> +\t}\n>> +\n>> +\tattachment = dma_buf_attach(dmabuf, dev);\n>> +\tif (IS_ERR(attachment)) {\n>> +\t\tdev_err(dev, \"Failed to attach dmabuf\\n\");\n>> +\t\tdma_buf_put(dmabuf);\n>> +\t\treturn PTR_ERR(attachment);\n>> +\t}\n>> +\n>> +\tsgt = dma_buf_map_attachment(attachment, dma_dir);\n>> +\tif (IS_ERR(sgt)) {\n>> +\t\tdev_err(dev, \"Failed to get dmabuf sg_table\\n\");\n>> +\t\tdma_buf_detach(dmabuf, attachment);\n>> +\t\tdma_buf_put(dmabuf);\n>> +\t\treturn PTR_ERR(sgt);\n>> +\t}\n>> +\n>> +\tif (sgt->nents != 1) {\n>> +\t\tdev_err(dev, \"Sparsed DMA area is unsupported\\n\");\n> \n> s/Sparsed/Sparse/\n> \n\nACK.\n\n>> +\t\tdma_buf_unmap_attachment(attachment, sgt, dma_dir);\n>> +\t\tdma_buf_detach(dmabuf, attachment);\n>> +\t\tdma_buf_put(dmabuf);\n>> +\t\treturn -EINVAL;\n> \n> This function would be cleaner using gotos to unwind. Pick the goto\n> name to reflect what the goto does. For example, here it would be:\n> \n\nOkay.\n\n> \tif (sgt->nents != 1) {\n> \t\tdev_err(dev, \"Sparse DMA area is unsupported\\n\");\n> \t\tret = -EINVAL;\n> \t\tgoto err_umap;\n> \t}\n> \n> \n> \n>> +\t}\n>> +\n>> +\t*paddr = sg_dma_address(sgt->sgl) + offset;\n>> +\n>> +\tdma_buf_unmap_attachment(attachment, sgt, dma_dir);\n>> +\n>> +\t*a = attachment;\n>> +\n>> +\tif (size)\n>> +\t\t*size = dmabuf->size - offset;\n>> +\n>> +\treturn 0;\n> \n> \treturn 0;\n> \n> err_unmap:\n> \tdma_buf_unmap_attachment(attachment, sgt, dma_dir);\n> err_detach:\n> \tdma_buf_detach(dmabuf, attachment);\n> err_put:\n> \tdma_buf_put(dmabuf);\n> \treturn ret;\n> \n>> +}\n>> +\n>> +static int tegra_vde_attach_frame_dmabufs(struct device *dev,\n>> +\t\t\t\t\t struct video_frame *frame,\n>> +\t\t\t\t\t struct tegra_vde_h264_frame *source,\n>> +\t\t\t\t\t enum dma_data_direction dma_dir,\n>> +\t\t\t\t\t int is_baseline_profile, int csize)\n>> +{\n>> +\tint ret;\n>> +\n>> +\tret = tegra_vde_attach_dmabuf(dev, source->y_fd,\n>> +\t\t\t\t source->y_offset, csize * 4,\n>> +\t\t\t\t &frame->y_dmabuf_attachment,\n>> +\t\t\t\t &frame->y_paddr, NULL, dma_dir);\n>> +\tif (ret)\n>> +\t\treturn ret;\n>> +\n>> +\tret = tegra_vde_attach_dmabuf(dev, source->cb_fd,\n>> +\t\t\t\t source->cb_offset, csize,\n>> +\t\t\t\t &frame->cb_dmabuf_attachment,\n>> +\t\t\t\t &frame->cb_paddr, NULL, dma_dir);\n>> +\tif (ret)\n>> +\t\treturn ret;\n>> +\n>> +\tret = tegra_vde_attach_dmabuf(dev, source->cr_fd,\n>> +\t\t\t\t source->cr_offset, csize,\n>> +\t\t\t\t &frame->cr_dmabuf_attachment,\n>> +\t\t\t\t &frame->cr_paddr, NULL, dma_dir);\n>> +\tif (ret)\n>> +\t\treturn ret;\n>> +\n>> +\tif (is_baseline_profile)\n>> +\t\tframe->aux_paddr = 0xF4DEAD00;\n> \n> The handling of is_baseline_profile is strange to me. It feels like we\n> should always check it before we use ->aux_paddr but we don't ever do\n> that.\n> \n\nIn a case of baseline profile, aux buffer isn't needed, HW should't use it. Aux\nphys address is set to a predefined and invalid address, so that in a case of\nVDE trying to use it, its invalid memory accesses would be reported in KMSG by\nmemory controller driver and the reported invalid addresses would be known to be\nassociated with the aux buffer. I'm not sure what you are meaning.\n\n>> +\telse\n>> +\t\tret = tegra_vde_attach_dmabuf(dev, source->aux_fd,\n>> +\t\t\t\t\t source->aux_offset, csize,\n>> +\t\t\t\t\t &frame->aux_dmabuf_attachment,\n>> +\t\t\t\t\t &frame->aux_paddr, NULL, dma_dir);\n> \n> \n> This function should have some error handling to undo the earlier\n> attach calls if the latter ones fail. See below:\n> \n> \n\nOkay.\n\n>> +\n>> +\treturn ret;\n> \n> \treturn 0;\n> \n> err_detach_cr:\n> \ttegra_vde_detach_and_put_dmabuf(frame->cr_dmabuf_attachment);\n> err_detach_cb:\n> \ttegra_vde_detach_and_put_dmabuf(frame->cb_dmabuf_attachment);\n> err_detach_y:\n> \ttegra_vde_detach_and_put_dmabuf(frame->y_dmabuf_attachment);\n> \n> \treturn ret;\n> \n> \n>> +}\n>> +\n>> +static void tegra_vde_deattach_frame_dmabufs(struct video_frame *frame)\n>> +{\n>> +\ttegra_vde_detach_and_put_dmabuf(frame->y_dmabuf_attachment);\n>> +\ttegra_vde_detach_and_put_dmabuf(frame->cb_dmabuf_attachment);\n>> +\ttegra_vde_detach_and_put_dmabuf(frame->cr_dmabuf_attachment);\n>> +\ttegra_vde_detach_and_put_dmabuf(frame->aux_dmabuf_attachment);\n> \n> \n> It would be happier to write this in the reverse order so it matches\n> the error handling that I wrote for you.\n> \n> \n\nOkay.\n\n>> +}\n>> +\n>> +static int tegra_vde_copy_and_validate_frame(struct device *dev,\n>> +\t\t\t\t\t struct tegra_vde_h264_frame *frame,\n>> +\t\t\t\t\t unsigned long vaddr)\n>> +{\n>> +\tif (copy_from_user(frame, (void __user *)vaddr, sizeof(*frame))) {\n>> +\t\tdev_err(dev, \"Copying of H.264 frame from user failed\\n\");\n>> +\t\treturn -EFAULT;\n> \n> Error message are a funny thing and different people feel different\n> ways. These can be triggered by the user so they let you spam dmesg\n> but I can see how many of them would be useful. These ones for\n> copy_from_user() are not useful since we assume the programmer should\n> know that stuff. The error code seems enough.\n> \n\nAgree.\n\n>> +\t}\n>> +\n>> +\tif (frame->frame_num > 0x7FFFFF) {\n>> +\t\tdev_err(dev, \"Bad frame_num %u\\n\", frame->frame_num);\n>> +\t\treturn -EINVAL;\n>> +\t}\n>> +\n>> +\tif (frame->y_offset & 0xFF) {\n>> +\t\tdev_err(dev, \"Bad y_offset 0x%X\\n\", frame->y_offset);\n>> +\t\treturn -EINVAL;\n>> +\t}\n>> +\n>> +\tif (frame->cb_offset & 0xFF) {\n>> +\t\tdev_err(dev, \"Bad cb_offset 0x%X\\n\", frame->cb_offset);\n>> +\t\treturn -EINVAL;\n>> +\t}\n>> +\n>> +\tif (frame->cr_offset & 0xFF) {\n>> +\t\tdev_err(dev, \"Bad cr_offset 0x%X\\n\", frame->cr_offset);\n>> +\t\treturn -EINVAL;\n>> +\t}\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> +static int tegra_vde_validate_h264_ctx(struct device *dev,\n>> +\t\t\t\t struct tegra_vde_h264_decoder_ctx *ctx)\n>> +{\n>> +\tif (ctx->dpb_frames_nb == 0 || ctx->dpb_frames_nb > 17) {\n>> +\t\tdev_err(dev, \"Bad DPB size %u\\n\", ctx->dpb_frames_nb);\n>> +\t\treturn -EINVAL;\n>> +\t}\n>> +\n>> +\tif (ctx->level_idc > 15) {\n>> +\t\tdev_err(dev, \"Bad level value %u\\n\", ctx->level_idc);\n>> +\t\treturn -EINVAL;\n>> +\t}\n>> +\n>> +\tif (ctx->pic_init_qp > 52) {\n>> +\t\tdev_err(dev, \"Bad pic_init_qp value %u\\n\", ctx->pic_init_qp);\n>> +\t\treturn -EINVAL;\n>> +\t}\n>> +\n>> +\tif (ctx->log2_max_pic_order_cnt_lsb > 16) {\n>> +\t\tdev_err(dev, \"Bad log2_max_pic_order_cnt_lsb value %u\\n\",\n>> +\t\t\tctx->log2_max_pic_order_cnt_lsb);\n>> +\t\treturn -EINVAL;\n>> +\t}\n>> +\n>> +\tif (ctx->log2_max_frame_num > 16) {\n>> +\t\tdev_err(dev, \"Bad log2_max_frame_num value %u\\n\",\n>> +\t\t\tctx->log2_max_frame_num);\n>> +\t\treturn -EINVAL;\n>> +\t}\n>> +\n>> +\tif (ctx->chroma_qp_index_offset > 31) {\n>> +\t\tdev_err(dev, \"Bad chroma_qp_index_offset value %u\\n\",\n>> +\t\t\tctx->chroma_qp_index_offset);\n>> +\t\treturn -EINVAL;\n>> +\t}\n>> +\n>> +\tif (ctx->pic_order_cnt_type > 2) {\n>> +\t\tdev_err(dev, \"Bad pic_order_cnt_type value %u\\n\",\n>> +\t\t\tctx->pic_order_cnt_type);\n>> +\t\treturn -EINVAL;\n>> +\t}\n>> +\n>> +\tif (ctx->num_ref_idx_l0_active_minus1 > 15) {\n>> +\t\tdev_err(dev, \"Bad num_ref_idx_l0_active_minus1 value %u\\n\",\n>> +\t\t\tctx->num_ref_idx_l0_active_minus1);\n>> +\t\treturn -EINVAL;\n>> +\t}\n>> +\n>> +\tif (ctx->num_ref_idx_l1_active_minus1 > 15) {\n>> +\t\tdev_err(dev, \"Bad num_ref_idx_l1_active_minus1 value %u\\n\",\n>> +\t\t\tctx->num_ref_idx_l1_active_minus1);\n>> +\t\treturn -EINVAL;\n>> +\t}\n>> +\n>> +\tif (!ctx->pic_width_in_mbs || ctx->pic_width_in_mbs > 127) {\n>> +\t\tdev_err(dev, \"Bad pic_width_in_mbs value %u, min 1 max 127\\n\",\n>> +\t\t\tctx->pic_width_in_mbs);\n>> +\t\treturn -EINVAL;\n>> +\t}\n>> +\n>> +\tif (!ctx->pic_height_in_mbs || ctx->pic_height_in_mbs > 127) {\n>> +\t\tdev_err(dev, \"Bad pic_height_in_mbs value %u, min 1 max 127\\n\",\n>> +\t\t\tctx->pic_height_in_mbs);\n>> +\t\treturn -EINVAL;\n>> +\t}\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> +static int tegra_vde_ioctl_decode_h264(struct tegra_vde *vde,\n>> +\t\t\t\t unsigned long vaddr)\n>> +{\n>> +\tstruct tegra_vde_h264_decoder_ctx ctx;\n>> +\tstruct tegra_vde_h264_frame frame;\n>> +\tstruct device *dev = vde->miscdev.parent;\n>> +\tstruct video_frame *dpb_frames = NULL;\n>> +\tstruct dma_buf_attachment *bitstream_data_dmabuf_attachment = NULL;\n>> +\tenum dma_data_direction dma_dir;\n>> +\tphys_addr_t bitstream_data_paddr;\n>> +\tphys_addr_t bsev_paddr;\n>> +\tu32 bitstream_data_size;\n>> +\tu32 macroblocks_nb;\n>> +\tbool timeout = false;\n>> +\tint i, ret;\n>> +\n>> +\tif (copy_from_user(&ctx, (void __user *)vaddr, sizeof(ctx))) {\n>> +\t\tdev_err(dev, \"Copying of H.264 CTX from user failed\\n\");\n>> +\t\treturn -EFAULT;\n>> +\t}\n>> +\n>> +\tret = tegra_vde_validate_h264_ctx(dev, &ctx);\n>> +\tif (ret)\n>> +\t\treturn -EINVAL;\n>> +\n>> +\tret = tegra_vde_attach_dmabuf(dev, ctx.bitstream_data_fd,\n>> +\t\t\t\t ctx.bitstream_data_offset, 0,\n>> +\t\t\t\t &bitstream_data_dmabuf_attachment,\n>> +\t\t\t\t &bitstream_data_paddr,\n>> +\t\t\t\t &bitstream_data_size,\n>> +\t\t\t\t DMA_TO_DEVICE);\n>> +\tif (ret)\n>> +\t\tgoto cleanup;\n> \n> \n> I hate this label name. What are we cleaning up??? Now I have to set a\n> bookmark so I can remember where I left and then scroll down to the\n> bottom of the function and take a look.\n> \n> [pause]\n> \n> OK. I'm back. I call this \"one err\" style error handling. And it's\n> very bug prone. Please read my essay on the topic:\n> \n> https://plus.google.com/106378716002406849458/posts/dnanfhQ4mHQ\n> \n> The bug is that we call tegra_vde_detach_and_put_dmabuf() with a NULL\n> pointer and there was that dereference before check that I mentioned\n> earlier.\n> \n\nAgain, this NULL pointer was intentional. But yes, I messed up the\nIS_ERR_OR_NULL/derefernce order and will get rid of it as you suggested. No\nobjections to what you wrote in the essay ;)\n\n>> +\n>> +\tdpb_frames = kcalloc(ctx.dpb_frames_nb, sizeof(*dpb_frames),\n>> +\t\t\t GFP_KERNEL);\n>> +\tif (!dpb_frames) {\n>> +\t\tret = -ENOMEM;\n>> +\t\tgoto cleanup;\n>> +\t}\n>> +\n>> +\tmacroblocks_nb = ctx.pic_width_in_mbs * ctx.pic_height_in_mbs;\n>> +\tvaddr = ctx.dpb_frames_ptr;\n>> +\n>> +\tfor (i = 0; i < ctx.dpb_frames_nb; i++) {\n>> +\t\tret = tegra_vde_copy_and_validate_frame(dev, &frame, vaddr);\n>> +\t\tif (ret)\n>> +\t\t\tgoto cleanup;\n>> +\n>> +\t\tdpb_frames[i].flags = frame.flags;\n>> +\t\tdpb_frames[i].frame_num = frame.frame_num;\n>> +\n>> +\t\tdma_dir = (i == 0) ? DMA_FROM_DEVICE : DMA_TO_DEVICE;\n>> +\n>> +\t\tret = tegra_vde_attach_frame_dmabufs(dev,\n>> +\t\t\t\t\t\t &dpb_frames[i], &frame,\n>> +\t\t\t\t\t\t dma_dir,\n>> +\t\t\t\t\t\t ctx.is_baseline_profile,\n>> +\t\t\t\t\t\t macroblocks_nb * 64);\n>> +\t\tif (ret) {\n>> +\t\t\ttegra_vde_deattach_frame_dmabufs(&dpb_frames[i]);\n>> +\t\t\tgoto cleanup;\n>> +\t\t}\n>> +\n>> +\t\tvaddr += sizeof(frame);\n>> +\t}\n>> +\n>> +\tret = clk_prepare_enable(vde->clk);\n>> +\tif (ret) {\n>> +\t\tdev_err(dev, \"Failed to enable clk: %d\\n\", ret);\n>> +\t\tgoto cleanup;\n>> +\t}\n>> +\n>> +\tret = mutex_lock_interruptible(&vde->lock);\n>> +\tif (ret)\n>> +\t\tgoto clkgate;\n>> +\n>> +\tret = reset_control_deassert(vde->rst);\n>> +\tif (ret) {\n>> +\t\tdev_err(dev, \"Failed to deassert reset: %d\\n\", ret);\n>> +\t\tclk_disable_unprepare(vde->clk);\n> \n> We do a second clk_disable_unprepare(vde->clk); after the unlock.\n> Delete this?\n> \n\nGood catch! I've reshuffled clk managment several times before..\n\n>> +\t\tgoto unlock;\n>> +\t}\n>> +\n>> +\tret = tegra_vde_setup_context(vde, &ctx, dpb_frames,\n>> +\t\t\t\t bitstream_data_paddr,\n>> +\t\t\t\t bitstream_data_size,\n>> +\t\t\t\t macroblocks_nb);\n>> +\tif (ret)\n>> +\t\tgoto reset;\n>> +\n>> +\ttegra_vde_decode_frame(vde, macroblocks_nb);\n>> +\n>> +\ttimeout = !wait_for_completion_io_timeout(&vde->decode_completion,\n>> +\t\t\t\t\t\t TEGRA_VDE_TIMEOUT);\n>> +\tif (timeout) {\n>> +\t\tbsev_paddr = readl(vde->regs + BSEV(0x10));\n>> +\t\tmacroblocks_nb = readl(vde->regs + SXE(0xC8)) & 0x1FFF;\n>> +\n>> +\t\tdev_err(dev, \"Decoding failed, \"\n>> +\t\t\t\t\"read 0x%X bytes : %u macroblocks parsed\\n\",\n>> +\t\t\tbsev_paddr ? bsev_paddr - bitstream_data_paddr : 0,\n>> +\t\t\tmacroblocks_nb);\n>> +\t}\n>> +\n>> +reset:\n>> +\t/*\n>> +\t * We rely on the VDE registers reset value, otherwise VDE would\n>> +\t * cause bus lockup.\n>> +\t */\n>> +\tret = reset_control_assert(vde->rst);\n>> +\tif (ret)\n>> +\t\tdev_err(dev, \"Failed to assert reset: %d\\n\", ret);\n> \n> We're overwriting \"ret\" here when we probably want to preserve the error\n> code from reset_control_deassert().\n> \n\nI think it doesn't really matter. It's very unlikely that the reset assert could\never fail, it might result in a further system hang on the next decode invocation.\n\n>> +\n>> +\tif (timeout)\n>> +\t\tret = -EIO;\n>> +\n>> +unlock:\n>> +\tmutex_unlock(&vde->lock);\n>> +\n>> +clkgate:\n>> +\tclk_disable_unprepare(vde->clk);\n>> +\n>> +cleanup:\n>> +\tif (dpb_frames)\n>> +\t\twhile (i--)\n>> +\t\t\ttegra_vde_deattach_frame_dmabufs(&dpb_frames[i]);\n>> +\n>> +\tkfree(dpb_frames);\n>> +\n>> +\ttegra_vde_detach_and_put_dmabuf(bitstream_data_dmabuf_attachment);\n>> +\n>> +\treturn ret;\n>> +}\n>> +\n>> +static long tegra_vde_unlocked_ioctl(struct file *filp,\n>> +\t\t\t\t unsigned int cmd, unsigned long arg)\n>> +{\n>> +\tstruct miscdevice *miscdev = filp->private_data;\n>> +\tstruct tegra_vde *vde = container_of(miscdev, struct tegra_vde,\n>> +\t\t\t\t\t miscdev);\n>> +\n>> +\tswitch (cmd) {\n>> +\tcase TEGRA_VDE_IOCTL_DECODE_H264:\n>> +\t\treturn tegra_vde_ioctl_decode_h264(vde, arg);\n>> +\t}\n>> +\n>> +\tdev_err(miscdev->parent, \"Invalid IOCTL command %u\\n\", cmd);\n>> +\n>> +\treturn -ENOTTY;\n>> +}\n>> +\n>> +static const struct file_operations tegra_vde_fops = {\n>> +\t.owner\t\t= THIS_MODULE,\n>> +\t.unlocked_ioctl\t= tegra_vde_unlocked_ioctl,\n>> +};\n>> +\n>> +static irqreturn_t tegra_vde_isr(int irq, void *data)\n>> +{\n>> +\tstruct tegra_vde *vde = data;\n>> +\n>> +\ttegra_vde_set_bits(vde->regs, 0, FRAMEID(0x208));\n>> +\tcomplete(&vde->decode_completion);\n>> +\n>> +\treturn IRQ_HANDLED;\n>> +}\n>> +\n>> +static int tegra_vde_probe(struct platform_device *pdev)\n>> +{\n>> +\tstruct resource *res_regs, *res_iram;\n>> +\tstruct device *dev = &pdev->dev;\n>> +\tstruct tegra_vde *vde;\n>> +\tint ret;\n>> +\n>> +\tvde = devm_kzalloc(&pdev->dev, sizeof(*vde), GFP_KERNEL);\n>> +\tif (!vde)\n>> +\t\treturn -ENOMEM;\n>> +\n>> +\tplatform_set_drvdata(pdev, vde);\n>> +\n>> +\tres_regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, \"regs\");\n>> +\tif (!res_regs)\n>> +\t\treturn -ENODEV;\n>> +\n>> +\tres_iram = platform_get_resource_byname(pdev, IORESOURCE_MEM, \"iram\");\n>> +\tif (!res_iram)\n>> +\t\treturn -ENODEV;\n>> +\n>> +\tvde->irq = platform_get_irq_byname(pdev, \"sync-token\");\n>> +\tif (vde->irq < 0)\n>> +\t\treturn vde->irq;\n>> +\n>> +\tvde->regs = devm_ioremap_resource(dev, res_regs);\n>> +\tif (IS_ERR(vde->regs))\n>> +\t\treturn PTR_ERR(vde->regs);\n>> +\n>> +\tvde->iram = devm_ioremap_resource(dev, res_iram);\n>> +\tif (IS_ERR(vde->iram))\n>> +\t\treturn PTR_ERR(vde->iram);\n>> +\n>> +\tvde->clk = devm_clk_get(dev, NULL);\n>> +\tif (IS_ERR(vde->clk)) {\n>> +\t\tdev_err(dev, \"Could not get VDE clk\\n\");\n>> +\t\treturn PTR_ERR(vde->clk);\n>> +\t}\n>> +\n>> +\tvde->rst = devm_reset_control_get(dev, \"vde\");\n>> +\tif (IS_ERR(vde->rst)) {\n>> +\t\tdev_err(dev, \"Could not get VDE reset\\n\");\n>> +\t\treturn PTR_ERR(vde->rst);\n>> +\t}\n>> +\n>> +\tret = devm_request_irq(dev, vde->irq, tegra_vde_isr, IRQF_SHARED,\n>> +\t\t\t dev_name(dev), vde);\n>> +\tif (ret)\n>> +\t\treturn -ENODEV;\n> \n> Presever the error code.\n> \n>> +\n>> +\tret = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_VDEC,\n>> +\t\t\t\t\t\tvde->clk, vde->rst);\n>> +\tif (ret) {\n>> +\t\tdev_err(&pdev->dev, \"Failed to power up VDE unit\\n\");\n>> +\t\treturn ret;\n>> +\t}\n>> +\n>> +\tret = reset_control_assert(vde->rst);\n>> +\tif (ret) {\n>> +\t\tdev_err(dev, \"Failed to assert reset: %d\\n\", ret);\n>> +\t\treturn ret;\n>> +\t}\n>> +\n>> +\tclk_disable_unprepare(vde->clk);\n>> +\n>> +\tmutex_init(&vde->lock);\n>> +\tinit_completion(&vde->decode_completion);\n>> +\n>> +\tvde->iram_lists_paddr = res_iram->start;\n>> +\tvde->miscdev.minor = MISC_DYNAMIC_MINOR;\n>> +\tvde->miscdev.name = \"tegra_vde\";\n>> +\tvde->miscdev.fops = &tegra_vde_fops;\n>> +\tvde->miscdev.parent = dev;\n>> +\n>> +\tret = misc_register(&vde->miscdev);\n>> +\tif (ret)\n>> +\t\treturn -ENODEV;\n> \n> Preserve the error code.\n> \n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-tegra-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"S6sUvySf\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y2Ysm5jndz9sRm\n\tfor ;\n\tThu, 28 Sep 2017 09:28:28 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752402AbdI0X2O (ORCPT );\n\tWed, 27 Sep 2017 19:28:14 -0400","from mail-wr0-f196.google.com ([209.85.128.196]:35037 \"EHLO\n\tmail-wr0-f196.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1752354AbdI0X2M (ORCPT\n\t);\n\tWed, 27 Sep 2017 19:28:12 -0400","by mail-wr0-f196.google.com with SMTP id n64so6869443wrb.2;\n\tWed, 27 Sep 2017 16:28:11 -0700 (PDT)","from [192.168.1.145] (ppp109-252-90-109.pppoe.spdop.ru.\n\t[109.252.90.109]) by smtp.googlemail.com with ESMTPSA id\n\t68sm23478ljr.47.2017.09.27.16.28.05\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tWed, 27 Sep 2017 16:28:06 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=gmail.com; s=20161025;\n\th=subject:to:cc:references:from:message-id:date:user-agent\n\t:mime-version:in-reply-to:content-language:content-transfer-encoding; \n\tbh=oIpsdLXuWb25cu+WsKmSzRztuCqLGFW4rTbPxE6SkiU=;\n\tb=S6sUvySfEseovwqnuARYwSk4fXOAHapmd2yGxsfz3J2xeUPNVmk3WbmRvIM5EccWmo\n\tJ5NLIqJy51KHEO67nw5FxJ0IoW4L32rsKMR43VXdJoevyCt8KH7p+39k7lQYsEnQ8n1R\n\tHLVE7PPGUMLuTYpcQ7Q6eRAPetzLOFQ0O5FwAastfe1DUlXmX+Pq5Q39stKBcks2YbEr\n\tUMuYEsubYnhcyMp8hIUNeE/9qZKr70FWlS+E+c3s6xZjomwrvYdE7wn1n4ZpXc9+NYFt\n\t+B+wis4bNb0q0R2OAMFz0biSQo9PtSv/XEOHtZ56vg6OKNq4GlIOlW2BDdyRji9KmlQo\n\tFgRg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:subject:to:cc:references:from:message-id:date\n\t:user-agent:mime-version:in-reply-to:content-language\n\t:content-transfer-encoding;\n\tbh=oIpsdLXuWb25cu+WsKmSzRztuCqLGFW4rTbPxE6SkiU=;\n\tb=Dfbxde8drgL7TJFw8NsLVWxUPBNjDKUG5jcZ3LTX3wlY6LHO5y0TO5CsKcIO7a3sSo\n\tUMRKzxfC93CJ2fvY4BWREq5fLHiu1COGNi0nQvCMfJcfieWZHsVmfXxeM9rgKDG+/UoZ\n\tnHdVqCbxC+9xbLntuEMuGNDixQFN2mozJkchpsQKr76eW/3RJFCV4x/zC4gcTt1AOUwZ\n\t0eNm36XLTeg8brEQ2B3ng1QSVnI/g6FgWiprFqm69tMiJz+eUNh7IfPt+h/XXx7nCygr\n\tSnkq6xIi8MCvWDnBMD/hzfflsV1Rje5oL56B9GDbuigBnpF6MdAFc3e9fSvXoqfcx21D\n\tuQEw==","X-Gm-Message-State":"AMCzsaUyDhYirg1DMpBRhk7w+VksLhE+CuEplMhVupY+0wgiV7XaQ/yB\n\t+ks4P3n6K9CWpim2tEkOJwDYhtqt","X-Google-Smtp-Source":"AOwi7QDweEAabIYfuEHsfVCPGA2VfjCBlteC3LEc+FWEq1U57Mmw7cLpfZUvA3Sen9bu4fnwIlMP0Q==","X-Received":"by 10.25.19.95 with SMTP id j92mr1014903lfi.138.1506554889873;\n\tWed, 27 Sep 2017 16:28:09 -0700 (PDT)","Subject":"Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder\n\tdriver","To":"Dan Carpenter ","Cc":"Thierry Reding ,\n\tJonathan Hunter ,\n\tGreg Kroah-Hartman ,\n\tRob Herring , linux-tegra@vger.kernel.org,\n\tdevel@driverdev.osuosl.org, linux-kernel@vger.kernel.org,\n\tdevicetree@vger.kernel.org","References":"\n\t<5c8b83775b982e6ee851c127444a8e839f422ad0.1506377430.git.digetx@gmail.com>\n\t<20170927094557.yr2fl5arodjnh637@mwanda>","From":"Dmitry Osipenko ","Message-ID":"<2b77bfa7-1272-3d11-9190-326fa0f85f22@gmail.com>","Date":"Thu, 28 Sep 2017 02:28:04 +0300","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<20170927094557.yr2fl5arodjnh637@mwanda>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Sender":"linux-tegra-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-tegra@vger.kernel.org"}},{"id":1776696,"web_url":"http://patchwork.ozlabs.org/comment/1776696/","msgid":"<001e6454-ad15-5e9d-cb49-277082b21687@gmail.com>","list_archive_url":null,"date":"2017-09-27T23:31:23","subject":"Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder\n\tdriver","submitter":{"id":18124,"url":"http://patchwork.ozlabs.org/api/people/18124/","name":"Dmitry Osipenko","email":"digetx@gmail.com"},"content":"On 27.09.2017 12:45, Dan Carpenter wrote:\n>> --- /dev/null\n>> +++ b/drivers/staging/tegra-vde/Kconfig\n>> @@ -0,0 +1,6 @@\n>> +config TEGRA_VDE\n>> +\ttristate \"NVIDIA Tegra20 video decoder driver\"\n>> +\tdepends on ARCH_TEGRA_2x_SOC\n> \n> Could we get a || COMPILE_TEST here as well?\n> \n\nGood point, I'll address it in V2.","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-tegra-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"VEx11ZFU\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y2YxF3Kmtz9t30\n\tfor ;\n\tThu, 28 Sep 2017 09:31:29 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752454AbdI0Xb2 (ORCPT );\n\tWed, 27 Sep 2017 19:31:28 -0400","from mail-wr0-f180.google.com ([209.85.128.180]:50060 \"EHLO\n\tmail-wr0-f180.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1752288AbdI0Xb1 (ORCPT\n\t);\n\tWed, 27 Sep 2017 19:31:27 -0400","by mail-wr0-f180.google.com with SMTP id h16so4843325wrf.6;\n\tWed, 27 Sep 2017 16:31:26 -0700 (PDT)","from [192.168.1.145] (ppp109-252-90-109.pppoe.spdop.ru.\n\t[109.252.90.109]) by smtp.googlemail.com with ESMTPSA id\n\ty86sm23273lje.54.2017.09.27.16.31.23\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tWed, 27 Sep 2017 16:31:24 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=gmail.com; s=20161025;\n\th=subject:to:cc:references:from:message-id:date:user-agent\n\t:mime-version:in-reply-to:content-language:content-transfer-encoding; \n\tbh=t2Y68zulxyrdzlcGQbCn1jphpBuU7dprrxvbltuf2MI=;\n\tb=VEx11ZFU5SyM5ozk4ijfgetbmWkU1XsSddeZpyRbv24gedYNo8zbNhzD2XHcfkDHJU\n\thgJ7KGDjKV2OEWcUHc3LsojHaa0oSE/OfLlqPlfGQyw6LEmsyXMayxjho52uFSgg7Ldj\n\tY9IfIgf8qJ51Cu5D30+ZCRybAOA6TtoQ0foMHB/SA57X2ROADjoltAUkQuUUwoGXzLFL\n\t3nH2Ut06HqySu2qJ7CaZdBXfJv5j/oObLCW6SMeNYmuPdU4sbDRgwHi2IQHnJ134WyWz\n\t6I6E3tTOpWM0gsWv4WfXnb6n6x3PXjFctyWS+wr0mOaWgtMaUV1d20ZiOvTk0AT5yG7D\n\t+X+w==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:subject:to:cc:references:from:message-id:date\n\t:user-agent:mime-version:in-reply-to:content-language\n\t:content-transfer-encoding;\n\tbh=t2Y68zulxyrdzlcGQbCn1jphpBuU7dprrxvbltuf2MI=;\n\tb=CLzwWODGTD3reWWgsu7m+7iAWUAlRPq8XM7nmObBvbvF9HEROrRMHp9ZOt0frTCG9K\n\tLZk9kn+VvSu7yCxdo+nhBtIrny1uQypk1Dy5wgiEh8EcvzFs/fTaEnuMK7hdfe6IOwTY\n\t2FIUst6QAqtTrkn8o2arVMWCS/eHZmeMez+mIsPv/lTCp2lXaKUhBb/rKxDv/BRikLyj\n\tjRWtqQtRyHdz7YHHewuLdrG0+VuD4NOh+Rs5un2djsg99Hp7h0xe9ixngpyPtGGo83kj\n\tmz4q9sDOAMkSY4blyjq98W9G8+cgnUZaGWnuktjwvUuBpVTqv/SWoXTxErNcCiStIMkv\n\t2iEg==","X-Gm-Message-State":"AHPjjUgAQqBHpc8Oy/aa2UMac4r4jm87o9ACHRls4x5rElMVDC1BC7n4\n\tAn7pVaHaJcHQxowqNnLQ4U/0d3m8","X-Google-Smtp-Source":"AOwi7QARwwjUjBahgSOFEe0MYCHpmEu29ysfFqi8qXiX+AcX5y0fiQSM0jW+mfjMZQro6002/Hh7ug==","X-Received":"by 10.25.33.2 with SMTP id h2mr1118099lfh.75.1506555085171;\n\tWed, 27 Sep 2017 16:31:25 -0700 (PDT)","Subject":"Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder\n\tdriver","To":"Dan Carpenter ","Cc":"Thierry Reding ,\n\tJonathan Hunter ,\n\tGreg Kroah-Hartman ,\n\tRob Herring , linux-tegra@vger.kernel.org,\n\tdevel@driverdev.osuosl.org, linux-kernel@vger.kernel.org,\n\tdevicetree@vger.kernel.org","References":"\n\t<5c8b83775b982e6ee851c127444a8e839f422ad0.1506377430.git.digetx@gmail.com>\n\t<20170927094557.yr2fl5arodjnh637@mwanda>","From":"Dmitry Osipenko ","Message-ID":"<001e6454-ad15-5e9d-cb49-277082b21687@gmail.com>","Date":"Thu, 28 Sep 2017 02:31:23 +0300","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<20170927094557.yr2fl5arodjnh637@mwanda>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Sender":"linux-tegra-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-tegra@vger.kernel.org"}},{"id":1776818,"web_url":"http://patchwork.ozlabs.org/comment/1776818/","msgid":"<20170928072308.s7npizjfnhkxh6zb@mwanda>","list_archive_url":null,"date":"2017-09-28T07:23:08","subject":"Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder\n\tdriver","submitter":{"id":9327,"url":"http://patchwork.ozlabs.org/api/people/9327/","name":"Dan Carpenter","email":"dan.carpenter@oracle.com"},"content":"On Thu, Sep 28, 2017 at 02:28:04AM +0300, Dmitry Osipenko wrote:\n> >> +\tif (is_baseline_profile)\n> >> +\t\tframe->aux_paddr = 0xF4DEAD00;\n> > \n> > The handling of is_baseline_profile is strange to me. It feels like we\n> > should always check it before we use ->aux_paddr but we don't ever do\n> > that.\n> > \n> \n> In a case of baseline profile, aux buffer isn't needed, HW should't use it. Aux\n> phys address is set to a predefined and invalid address, so that in a case of\n> VDE trying to use it, its invalid memory accesses would be reported in KMSG by\n> memory controller driver and the reported invalid addresses would be known to be\n> associated with the aux buffer. I'm not sure what you are meaning.\n\nIt's not used perhaps, but we do write it to the hardware, right?\n\n\ttegra_vde_write_iram_entry(iram_tables, 0, i, value, aux_paddr);\n\nIt's just strange.\n\nregards,\ndan carpenter\n--\nTo unsubscribe from this list: send the line \"unsubscribe linux-tegra\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-tegra-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y2mPr6779z9t5x\n\tfor ;\n\tThu, 28 Sep 2017 17:23:28 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751801AbdI1HX1 (ORCPT );\n\tThu, 28 Sep 2017 03:23:27 -0400","from userp1040.oracle.com ([156.151.31.81]:42915 \"EHLO\n\tuserp1040.oracle.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1750794AbdI1HX0 (ORCPT\n\t);\n\tThu, 28 Sep 2017 03:23:26 -0400","from userv0022.oracle.com (userv0022.oracle.com [156.151.31.74])\n\tby userp1040.oracle.com (Sentrion-MTA-4.3.2/Sentrion-MTA-4.3.2) with\n\tESMTP id v8S7NI1m026439\n\t(version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256\n\tverify=OK); Thu, 28 Sep 2017 07:23:19 GMT","from userv0122.oracle.com (userv0122.oracle.com [156.151.31.75])\n\tby userv0022.oracle.com (8.14.4/8.14.4) with ESMTP id v8S7NIVe018469\n\t(version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256\n\tverify=OK); Thu, 28 Sep 2017 07:23:18 GMT","from abhmp0012.oracle.com (abhmp0012.oracle.com [141.146.116.18])\n\tby userv0122.oracle.com (8.14.4/8.14.4) with ESMTP id\n\tv8S7NH9N011579; Thu, 28 Sep 2017 07:23:18 GMT","from mwanda (/197.254.35.146)\n\tby default (Oracle Beehive Gateway v4.0)\n\twith ESMTP ; Thu, 28 Sep 2017 00:23:17 -0700"],"Date":"Thu, 28 Sep 2017 10:23:08 +0300","From":"Dan Carpenter ","To":"Dmitry Osipenko ","Cc":"Thierry Reding ,\n\tJonathan Hunter ,\n\tGreg Kroah-Hartman ,\n\tRob Herring , linux-tegra@vger.kernel.org,\n\tdevel@driverdev.osuosl.org, linux-kernel@vger.kernel.org,\n\tdevicetree@vger.kernel.org","Subject":"Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder\n\tdriver","Message-ID":"<20170928072308.s7npizjfnhkxh6zb@mwanda>","References":"\n\t<5c8b83775b982e6ee851c127444a8e839f422ad0.1506377430.git.digetx@gmail.com>\n\t<20170927094557.yr2fl5arodjnh637@mwanda>\n\t<2b77bfa7-1272-3d11-9190-326fa0f85f22@gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<2b77bfa7-1272-3d11-9190-326fa0f85f22@gmail.com>","User-Agent":"NeoMutt/20170113 (1.7.2)","X-Source-IP":"userv0022.oracle.com [156.151.31.74]","Sender":"linux-tegra-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-tegra@vger.kernel.org"}},{"id":1776973,"web_url":"http://patchwork.ozlabs.org/comment/1776973/","msgid":"<69a019f1-8ce5-6b8b-8eb9-2ce9b8c2759d@gmail.com>","list_archive_url":null,"date":"2017-09-28T11:37:03","subject":"Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder\n\tdriver","submitter":{"id":18124,"url":"http://patchwork.ozlabs.org/api/people/18124/","name":"Dmitry Osipenko","email":"digetx@gmail.com"},"content":"On 28.09.2017 10:23, Dan Carpenter wrote:\n> On Thu, Sep 28, 2017 at 02:28:04AM +0300, Dmitry Osipenko wrote:\n>>>> +\tif (is_baseline_profile)\n>>>> +\t\tframe->aux_paddr = 0xF4DEAD00;\n>>>\n>>> The handling of is_baseline_profile is strange to me. It feels like we\n>>> should always check it before we use ->aux_paddr but we don't ever do\n>>> that.\n>>>\n>>\n>> In a case of baseline profile, aux buffer isn't needed, HW should't use it. Aux\n>> phys address is set to a predefined and invalid address, so that in a case of\n>> VDE trying to use it, its invalid memory accesses would be reported in KMSG by\n>> memory controller driver and the reported invalid addresses would be known to be\n>> associated with the aux buffer. I'm not sure what you are meaning.\n> \n> It's not used perhaps, but we do write it to the hardware, right?\n> \n\nThat's right. I'm pretty sure HW won't use the aux in a case of baseline\nprofile, haven't seen an evidence of it. But in order to be on a safe side, the\naddresses are initialized to an invalid value, so HW won't have a chance to\nsilently fetch/trash 'arbitrary' main memory and we will know if it tries to do it.\n\n\tif (baseline_profile)\n\t\tframe->aux_paddr = 0xF4DEAD00;\n\telse {\n\nSo here ^ all *used* frame entries are being initialized.\n\n> \ttegra_vde_write_iram_entry(iram_tables, 0, i, value, aux_paddr);\n> \n> It's just strange.\n> \n\nThere up to 16 reference video frames (already decoded) that could be used for\ndecoding of a video frame. Addresses of reference frames that shouldn't be used\nfor decoding of the current frame are set to an invalid address. Userspace my\nsupply wrong frames list or frames list setup code may contain an obscure bug\nand we will know about it.\n\n\t} else {\n\t\taux_paddr = 0xFADEAD00;\n\t\tvalue = 0;\n\t}\n\nHere ^ all *unused* frame entries are being initialized.\n\n\ttegra_vde_write_iram_entry(iram_tables, 0, i, value, aux_paddr);\n\nAnd here ^ these entries are written to the tables that are read by HW.","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-tegra-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"Kn8evm4H\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y2t2Y2yrBz9s0Z\n\tfor ;\n\tThu, 28 Sep 2017 21:37:09 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752039AbdI1LhI (ORCPT );\n\tThu, 28 Sep 2017 07:37:08 -0400","from mail-wr0-f193.google.com ([209.85.128.193]:34023 \"EHLO\n\tmail-wr0-f193.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1752023AbdI1LhH (ORCPT\n\t);\n\tThu, 28 Sep 2017 07:37:07 -0400","by mail-wr0-f193.google.com with SMTP id z1so1781699wre.1;\n\tThu, 28 Sep 2017 04:37:06 -0700 (PDT)","from [192.168.1.145] (ppp109-252-90-109.pppoe.spdop.ru.\n\t[109.252.90.109]) by smtp.googlemail.com with ESMTPSA id\n\tx13sm259912ljb.23.2017.09.28.04.37.03\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tThu, 28 Sep 2017 04:37:04 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=gmail.com; s=20161025;\n\th=subject:to:cc:references:from:message-id:date:user-agent\n\t:mime-version:in-reply-to:content-language:content-transfer-encoding; \n\tbh=YtVQC2vHsKLhyHhhaaaDR0XYMIWBj9nPnCYt3nevFHs=;\n\tb=Kn8evm4HK8to/9YV7Lsrmn6L4FZ4NWjrYKgKQWwe//UW4/ZoVkNNfwFAqJ6XEG32FZ\n\tOxizAHXCKezl90Qv3PmCUMIBZuDm7ILnnY1Kg+vVYMYtGxwBEJlK8ZP0iTZ/LeXGL9KM\n\tAw/d8jtBGHD2Pv1TbyJiqlEUQgsLb2PapMTUAEyHcVOx7ccCuZ3iFnUCymZ4NWtk44rj\n\tIHLyrugvSzHNNA2/PO9OZrtas9CZUudu8/EwJo/OCfMYG63fPefLc+6F0weyOqr4L6Yw\n\t09DjDYNrua6J/okZ3306hcmt4NHTHaZ9Sja2a4UHdqFxve3CP1CspCIgrKW2zN9w2+PV\n\t8/Bg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:subject:to:cc:references:from:message-id:date\n\t:user-agent:mime-version:in-reply-to:content-language\n\t:content-transfer-encoding;\n\tbh=YtVQC2vHsKLhyHhhaaaDR0XYMIWBj9nPnCYt3nevFHs=;\n\tb=LYEyxfzp8XIf2n+prmLM/XOykScq37jpxq2imVTIN8eEPeMjW38T7R5iGWwTCJeWS1\n\tfA86PyBfipmFvY9bBfw+oSDG0j6JjOKcH3bBsiLVZWjjCFZXLeyKlc5zsvisq9r/0T+6\n\tMudwScPhxhWQAk6ACi7YB46GEjWdNRPSdRHD7/MRZ5MfFhWO2ZWGpac1T2fJzoC4xEy5\n\tG2gXwYOFS0ZU4IKHa2Gqs4O+u/W+HDu9xn12pz8l4Vsrjy1RUsEatNVA9x8jWFaeRoIt\n\tThsLS390mehr+HJD/hV73smBEtzWf2V0kShQx8OHaCWUPaeendR++KnlCDDUdshzaplI\n\t5z5g==","X-Gm-Message-State":"AHPjjUiUbwSgE9ECQTGn+lfR1xTmdMf0bioVs9sXj1AtTcxSoa3eVpjl\n\t2VwXvKignY02Nl7pgspQkWFIVxNA","X-Google-Smtp-Source":"AOwi7QAqFCFpb7OSGaQzYfiVlmk3zb3qCXJxUtDhCZxrXfWnk1oYXXiwSxs/X5Xgt4tW3+zzUOscyw==","X-Received":"by 10.46.74.25 with SMTP id x25mr1923101lja.83.1506598625456;\n\tThu, 28 Sep 2017 04:37:05 -0700 (PDT)","Subject":"Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder\n\tdriver","To":"Dan Carpenter ","Cc":"Thierry Reding ,\n\tJonathan Hunter ,\n\tGreg Kroah-Hartman ,\n\tRob Herring , linux-tegra@vger.kernel.org,\n\tdevel@driverdev.osuosl.org, linux-kernel@vger.kernel.org,\n\tdevicetree@vger.kernel.org","References":"\n\t<5c8b83775b982e6ee851c127444a8e839f422ad0.1506377430.git.digetx@gmail.com>\n\t<20170927094557.yr2fl5arodjnh637@mwanda>\n\t<2b77bfa7-1272-3d11-9190-326fa0f85f22@gmail.com>\n\t<20170928072308.s7npizjfnhkxh6zb@mwanda>","From":"Dmitry Osipenko ","Message-ID":"<69a019f1-8ce5-6b8b-8eb9-2ce9b8c2759d@gmail.com>","Date":"Thu, 28 Sep 2017 14:37:03 +0300","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<20170928072308.s7npizjfnhkxh6zb@mwanda>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Sender":"linux-tegra-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-tegra@vger.kernel.org"}}]