From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4901DE6FE42 for ; Fri, 22 Sep 2023 14:51:49 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232021AbjIVOvx (ORCPT ); Fri, 22 Sep 2023 10:51:53 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51262 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232041AbjIVOvu (ORCPT ); Fri, 22 Sep 2023 10:51:50 -0400 Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 19A061B8 for ; Fri, 22 Sep 2023 07:51:41 -0700 (PDT) Received: from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi [213.243.189.158]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 0F444968; Fri, 22 Sep 2023 16:50:00 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1695394201; bh=hITO0GBwOof1UuQtb60cyuvvepFVwGDgum4MFpbAyZA=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=UvfmhVaM/XQVutGf2ienC+sPayRL0AH+oEeNTQqf1L9lti+Ww6qol40SC2CX2CSEG ls/ROHIRh/+izyBzrGI0sNbX7Fk70j4dkQaigsh29WCXKoXIsSttQ7bbACIMb7mSHT 4ctNuZ1y4pQik0gkNX+jTXQOdR354yNjZjX5W528= Date: Fri, 22 Sep 2023 17:51:51 +0300 From: Laurent Pinchart To: Tomi Valkeinen Cc: Sakari Ailus , linux-media@vger.kernel.org, Tianshu Qiu , Bingbu Cao , Jacopo Mondi , Rui Miguel Silva , Martin Kepplinger Subject: Re: [PATCH v3 11/12] media: v4l: subdev: Print debug information on frame descriptor Message-ID: <20230922145151.GI19112@pendragon.ideasonboard.com> References: <20230919121728.126781-12-sakari.ailus@linux.intel.com> <20230919133248.GD1505@pendragon.ideasonboard.com> <20230919152123.GA18426@pendragon.ideasonboard.com> <20230922095320.GE19112@pendragon.ideasonboard.com> <09af9c43-f9b8-d426-f7ce-959a6dea87d2@ideasonboard.com> <20230922102244.GG19112@pendragon.ideasonboard.com> <60f44e28-1166-026f-1bea-633de7d53298@ideasonboard.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <60f44e28-1166-026f-1bea-633de7d53298@ideasonboard.com> Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org On Fri, Sep 22, 2023 at 01:27:46PM +0300, Tomi Valkeinen wrote: > On 22/09/2023 13:22, Laurent Pinchart wrote: > > On Fri, Sep 22, 2023 at 01:16:21PM +0300, Tomi Valkeinen wrote: > >> On 22/09/2023 13:09, Sakari Ailus wrote: > >>> On Fri, Sep 22, 2023 at 12:53:20PM +0300, Laurent Pinchart wrote: > >>>> On Fri, Sep 22, 2023 at 09:41:01AM +0000, Sakari Ailus wrote: > >>>>> On Tue, Sep 19, 2023 at 06:21:23PM +0300, Laurent Pinchart wrote: > >>>>>> On Tue, Sep 19, 2023 at 02:49:29PM +0000, Sakari Ailus wrote: > >>>>>>> On Tue, Sep 19, 2023 at 04:32:48PM +0300, Laurent Pinchart wrote: > >>>>>>>> On Tue, Sep 19, 2023 at 03:17:27PM +0300, Sakari Ailus wrote: > >>>>>>>>> Print debug level information on returned frame descriptors. > >>>>>>>>> > >>>>>>>>> Signed-off-by: Sakari Ailus > >>>>>>>>> --- > >>>>>>>>> drivers/media/v4l2-core/v4l2-subdev.c | 32 ++++++++++++++++++++++++++- > >>>>>>>>> 1 file changed, 31 insertions(+), 1 deletion(-) > >>>>>>>>> > >>>>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > >>>>>>>>> index 7b087be3ff4f..504ca625b2bd 100644 > >>>>>>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c > >>>>>>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c > >>>>>>>>> @@ -15,6 +15,7 @@ > >>>>>>>>> #include > >>>>>>>>> #include > >>>>>>>>> #include > >>>>>>>>> +#include > >>>>>>>>> #include > >>>>>>>>> #include > >>>>>>>>> #include > >>>>>>>>> @@ -309,9 +310,38 @@ static int call_set_selection(struct v4l2_subdev *sd, > >>>>>>>>> static int call_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad, > >>>>>>>>> struct v4l2_mbus_frame_desc *fd) > >>>>>>>>> { > >>>>>>>>> + unsigned int i; > >>>>>>>>> + int ret; > >>>>>>>>> + > >>>>>>>>> memset(fd, 0, sizeof(*fd)); > >>>>>>>>> > >>>>>>>>> - return sd->ops->pad->get_frame_desc(sd, pad, fd); > >>>>>>>>> + ret = sd->ops->pad->get_frame_desc(sd, pad, fd); > >>>>>>>>> + if (ret) > >>>>>>>>> + return ret; > >>>>>>>>> + > >>>>>>>>> + dev_dbg(sd->dev, "Frame descriptor on pad %u, type %s\n", pad, > >>>>>>>>> + fd->type == V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL ? "parallel" : > >>>>>>>>> + fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2 ? "CSI-2" : > >>>>>>>>> + "unknown"); > >>>>>>>>> + > >>>>>>>>> + for (i = 0; i < fd->num_entries; i++) { > >>>>>>>>> + struct v4l2_mbus_frame_desc_entry *entry = &fd->entry[i]; > >>>>>>>>> + char buf[20] = ""; > >>>>>>>> > >>>>>>>> Should this be sized for the worst case ? The vc and dt should not be > >>>>>>>> large, but a buffer overflow on the stack in debug code if a subdev > >>>>>>>> returns an incorrect value would be bad. > >>>>>>> > >>>>>>> 17 should be enough but it's not useful to use a size not divisible by 4 in > >>>>>>> practice here. > >>>>>> > >>>>>> 18 with the terminating 0. But indeed, it's large enough as vc and dt > >>>>> > >>>>> I can count only 17 --- there's no newline. > >>>>> > >>>>> I guess it's most probably either of these then. X-) > >>>>> > >>>>>> are u8. I'm just a bit worried we're opening the door to hard to debug > >>>>>> problems if we later change the vc and dt types. > >>>>> > >>>>> I can add a WARN_ON() to cover this. > >>>>> > >>>>>>>>> + > >>>>>>>>> + if (fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2) > >>>>>>>>> + snprintf(buf, sizeof(buf), ", vc %u, dt 0x%2.2x", > >>>>>>>> > >>>>>>>> 0x%02x would be one character shorter ;-) Same below. > >>>>>>> > >>>>>>> It would be, but I prefer the above notation as it's more generic. > >>>>>> > >>>>>> Out of curiosity, how so ? > >>>>> > >>>>> It works with data that would span more than 9 characters when printed. > >>>> > >>>> And 0x%02x doesn't ? > >>> > >>> Ah, it should indeed work, 0 is actually a flag here and part of the field > >>> width or precision. I can use that in v4. > >> > >> Or %#04x, even shorter! > > > > That's different though. > > > > printf("0x%2.2x\n", 0); -> 0x00 > > printf("0x%2.2x\n", 1); -> 0x01 > > printf("0x%2.2x\n", 42); -> 0x2a > > > > printf("0x%02x\n", 0); -> 0x00 > > printf("0x%02x\n", 1); -> 0x01 > > printf("0x%02x\n", 42); -> 0x2a > > > > printf("%#2.2x\n", 0); -> 00 > > printf("%#2.2x\n", 1); -> 0x01 > > printf("%#2.2x\n", 42); -> 0x2a > > > > printf("%#02x\n", 0); -> 00 > > printf("%#02x\n", 1); -> 0x1 > > printf("%#02x\n", 42); -> 0x2a > > The length should be 4 there, not 2. But even after fixing that, the 0 > case is printed wrong. Interesting, I haven't noticed that before. I > wonder why it behaves that way... It's documented as such: # The value should be converted to an "alternate form". [...] For x and X conversions, a nonzero result has the string "0x" (or "0X" for X conversions) prepended to it. [...] > 0000 > 0x01 > 0x2a -- Regards, Laurent Pinchart