Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 30 Mar 2022 08:15:41 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Nicolas Dufresne <nicolas.dufresne@collabora.com>
Cc:     Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        kernel@collabora.com, linux-media@vger.kernel.org,
        linux-rockchip@lists.infradead.org, linux-staging@lists.linux.dev,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 19/24] media: rkvdec-h264: Add field decoding support
Message-ID: <20220330051541.GE3293@kadam>
References: <20220328195936.82552-1-nicolas.dufresne@collabora.com>
 <20220328195936.82552-20-nicolas.dufresne@collabora.com>
 <20220329081321.GV3293@kadam>
 <f7e5b577bc48ba16befbed47ac96c363ce861f48.camel@collabora.com>
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <f7e5b577bc48ba16befbed47ac96c363ce861f48.camel@collabora.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?iso-8859-1?Q?TjGe4fMk/zve6C1e4WctENFILGr6RIoM9KHYZI8UtHVfg5HAla9ZjHyX7f?=
 =?iso-8859-1?Q?uRE6nwUjikyRkwYbBMasa5wvkqxrtJriW+6b9PMlZwNID/8F6+dFUnfZLu?=
 =?iso-8859-1?Q?8lqWm5wzaVfuz+BIg4Op+j+CiaAWkQ5y7SygUFIbqWlml55xk/ho3pxD3v?=
 =?iso-8859-1?Q?9cR4oa33FZJkitS0R3W9xotZgLstwheiWh3A0nDFMQ22l5FmLZqwqR7Fur?=
 =?iso-8859-1?Q?tJn+GUurB/K54dafcg8UDn+6MhXAdyvSSj/sxs5VijxRl5e0nQE5Y6k8pl?=
 =?iso-8859-1?Q?CgHbDCN7NlWFN87iZgxThJllQ+m4jdh0AnuCA5ao6wbJAILnm1+V+nkMaD?=
 =?iso-8859-1?Q?tSde7MTwSgKHoX7IqAqCGaBNm7SWssB8CtsxDVvhVvmwajmj5Bh7qWLsGZ?=
 =?iso-8859-1?Q?pB1+EY5tAdiW09rrTn0pEM88Wna04c4exGktshGtq2zF0vtUGhVMH62u2Y?=
 =?iso-8859-1?Q?CkCgM6BcQ2Pv16b3mDydpXFDSg0m2Qkq4tg35UwhuKUEGwjBAPBaGXk+3Q?=
 =?iso-8859-1?Q?Qh1eRSMV2pV34NUhRy+F7ac0oz0X/lB5ySluZnC9XVZxjb+Iwdx5rQltt9?=
 =?iso-8859-1?Q?a6lYtCli1rD6Su4i4lCqogAqthQexMReLKw9g6Vg4nUeTb9JTDyPczxeXo?=
 =?iso-8859-1?Q?oZ9RcdEAzVA47kugvf4yYjMC8+56D2OJmsEYkoCs6a0vMfRk2FgyXHJ02G?=
 =?iso-8859-1?Q?Yycn4BcrJ2WJVrtINCD5rzeUbZ94eZQT9hpGWgPYI9JuEQWjceeweSA6jV?=
 =?iso-8859-1?Q?9NWCebrcK5MK+Uh5cpuq7ztqfFTtCizLhbxkcsdV+rvgwm8/Wfz/khbALB?=
 =?iso-8859-1?Q?N3YGVw+eP1JXfNHTUpmbd1lbZdoe6m2wROBlvtj0yKQ/n/lQNcXMHar7vk?=
 =?iso-8859-1?Q?Leq+YEGmvgGXqq/oQ2USbNagDGmWFIhF7IlVF6lfCSXV+2PYSAYLNu1fml?=
 =?iso-8859-1?Q?W+2ngVN9P0yQg8ReQLGEZEShm+ZWM2jH7nJkR6fz2KvSq4lRm+uvxCCzzM?=
 =?iso-8859-1?Q?D0de0Fdl1/xw7iGbMdAVZKyf3FivhLU2Y/LMNDQDnk4k3UoyfFeb/v95Xo?=
 =?iso-8859-1?Q?wMU8Rc9xWTVqzT1NVU62j1KBJnsy+Y+ma8esZgSHi9CTPgDzkJdOuqWK7/?=
 =?iso-8859-1?Q?0761JY96/QqxIwm8gGQ+Pp7YiWK+GiFCwmzoL/gM0tdBpUM17axfj3T5T/?=
 =?iso-8859-1?Q?tY4T5Dcz6eDt8tdeoyM7QVAzxIDSooAdCBvE7Ip0THi9ZcLrHNfSaFqSnN?=
 =?iso-8859-1?Q?Z1wSpZIA9Ir4UBjlkgWUdCQ/Mg6Sr2kKxr54H/w3U6TIDNYMTM4MZwMFj4?=
 =?iso-8859-1?Q?+e4+oqEyv9e5sJGnA+YmWuGV/CC9YfX8Imd+sc2sdwGMzgWOVp1YBt56sJ?=
 =?iso-8859-1?Q?0WOqhE+lAarVjanu+ErwSSiUJ4kELufCHhoPzzYmqMlXOxIWNpKH/xoJf9?=
 =?iso-8859-1?Q?bg6HXDjeRSgZs3LT/hMv02FNv1BXuHumYpMGctjLaFQbjSu/4C6Z/r7Y/n?=
 =?iso-8859-1?Q?mqC9lK+z2ribxSk5ANcC0lsAm8pAsMfVj3kBJuP+OzIhWguomCWcxeniRl?=
 =?iso-8859-1?Q?GZAB8d7Ug2OEhE6gGP5AeP7TekeTXiuo67Rv/GMEzeu4TXBeK4YDJi54U5?=
 =?iso-8859-1?Q?IhD9AUEvZ3D4ZgwLI2jzT0bV8aIkKAzS6fRxxBRb/J++d+hsKv84K/7Uy3?=
 =?iso-8859-1?Q?KVH707IbWjWs+P9YDaZdNCZKPRLx7nq6MBqAoKaHh7PJOrOAeu+zVl+60/?=
 =?iso-8859-1?Q?jDKPgz9XrE1eunXyrt2DxKsOp7EvTQ6VQLII9tHrVqRiy0k4Nw+DO7MdQt?=
 =?iso-8859-1?Q?kFbUwZ7cwO0TtmItOQ4MDPI67Gt49Ts=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 14fc6e7a-3a21-4801-3d6f-08da120c5d00
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2365.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Mar 2022 05:15:54.0394
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Q6njO69VmgHGH1ClM2vVqMaHbkSOOnlT9CO46xOqwuF7+VxhTCpXXZ5A83RGs6RSMGBPwUPurtPA4+Ld29blHYJcVTO3c4dFh4aAVqzdkHA=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR10MB5371
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10301 signatures=695566
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 spamscore=0
 adultscore=0 mlxscore=0 phishscore=0 bulkscore=0 suspectscore=0
 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2202240000 definitions=main-2203300025
X-Proofpoint-GUID: gE3UrIer5vmbwH62G5nyInAlAybyT16z
X-Proofpoint-ORIG-GUID: gE3UrIer5vmbwH62G5nyInAlAybyT16z
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIMWL_WL_MED,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, Mar 29, 2022 at 04:54:55PM -0400, Nicolas Dufresne wrote:
> Le mardi 29 mars 2022 ? 11:13 +0300, Dan Carpenter a ?crit?:
> > On Mon, Mar 28, 2022 at 03:59:31PM -0400, Nicolas Dufresne wrote:
> > > @@ -738,23 +735,26 @@ static void lookup_ref_buf_idx(struct rkvdec_ctx *ctx,
> > >  		struct vb2_queue *cap_q = &m2m_ctx->cap_q_ctx.q;
> > >  		int buf_idx = -1;
> > >  
> > > -		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
> > > +		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE) {
> > >  			buf_idx = vb2_find_timestamp(cap_q,
> > >  						     dpb[i].reference_ts, 0);
> > > +			if (buf_idx < 0)
> > > +				pr_debug("No buffer for reference_ts %llu",
> > > +					 dpb[i].reference_ts);
> > 
> > pr_debug() is too quiet.  Make it pr_err().  Set buf_idx to zero instead
> > leaving it as an error code.
> 
> Thanks for the suggestion, I'm just a bit uncomfortable using pr_err() for
> something that is not a driver error, but userland error. Perhaps you can
> educate me on the policy in this regard, but malicous userland being able to
> flood the logs very easily is my main concern here.
> 
> About the negative idx, it is being used set dpb_valid later on. H.264 error
> resilience requires that these frames should be marked as "unexisting" but still
> occupy space in the DPB, this is more or less what I'm trying to implement here.
> Setting it to 0 would basically mean to refer to DPB index 0, which is
> relatively random pick. I believe your suggestion is not taking into
> consideration what the code is doing, but it would fall in some poor-man
> concealment which I would rather leave to the userland.
> 

To be honest, I just saw that it was a negative idx and freaked out.  I
didn't look at any context...

You're right that we don't to allow the user to spam the dmesg.  Ideally
we would return an error.  A second best solution is to do a pr_err_once().

> > >  	for (j = 0; j < RKVDEC_NUM_REFLIST; j++) {
> > > -		for (i = 0; i < h264_ctx->reflists.num_valid; i++) {
> > > -			u8 dpb_valid = run->ref_buf_idx[i] >= 0;
> > > -			u8 idx = 0;
> > > +		for (i = 0; i < builder->num_valid; i++) {
> > > +			struct v4l2_h264_reference *ref;
> > > +			u8 dpb_valid;
> > > +			u8 bottom;
> > 
> > These would be better as type bool.
> 
> I never used a bool for bit operations before, but I guess that can work, thanks
> for the suggestion. As this deviates from the original code, I suppose I should
> make this a separate patch ?

I just saw the name and wondered why it was a u8.  bool does make more
sense and works fine for the bitwise stuff.  But I don't really care at
all.

regards,
dan carpenter