From: Tomi Valkeinen <tomi.valkeinen@ti.com> To: Archit Taneja <archit@ti.com> Cc: <linux-media@vger.kernel.org>, <linux-omap@vger.kernel.org>, <dagriego@biglakesoftware.com>, <dale@farnsworth.org>, <pawel@osciak.com>, <m.szyprowski@samsung.com>, <hverkuil@xs4all.nl>, <laurent.pinchart@ideasonboard.com> Subject: Re: [PATCH 2/6] v4l: ti-vpe: Add helpers for creating VPDMA descriptors Date: Mon, 5 Aug 2013 16:03:45 +0300 [thread overview] Message-ID: <51FFA2B1.1050809@ti.com> (raw) In-Reply-To: <51FF9517.6000406@ti.com> [-- Attachment #1: Type: text/plain, Size: 4730 bytes --] On 05/08/13 15:05, Archit Taneja wrote: > On Monday 05 August 2013 02:41 PM, Tomi Valkeinen wrote: >> There's quite a bit of code in these dump functions, and they are always >> called. I'm sure getting that data is good for debugging, but I presume >> they are quite useless for normal use. So I think they should be >> compiled in only if some Kconfig option is selected. > > Won't pr_debug() functions actually print something only when > CONFIG_DYNAMIC_DEBUG is selected or if the DEBUG is defined? They will If DEBUG is defined, they are always printed. If dynamic debug is in use, the user has to enable debug prints for VPE for the dumps to be printed. > still consume a lot of code, but it would just end up in dummy printk > calls, right? Yes. Well, I don't know VPE, so I can't really say how much those prints are needed or not. They just looked very verbose to me. I think we should have "normal" level debugging messages compiled in by default, and for "verbose" there should be a separate compile options. With verbose I mean something that may be useful if you are changing the code and want to verify it or debugging some very odd bug. I.e. for the developer of the driver. And with normal something that would be used when, say, somebody uses VPE for in his app, but things don't seem to be quite right, and there's need to get some info on what is going on. I.e. for "normal" user. But that's just my opinion, and it's obviously difficult to define those clearly =). To be honest, I don't know how much overhead very verbose kernel debug prints even cause. Maybe it's negligible. >>> +/* >>> + * data transfer descriptor >>> + * >>> + * All fields are 32 bits to make them endian neutral >> >> What does that mean? Why would 32bit fields make it endian neutral? > > > Each 32 bit field describes one word of the data descriptor. Each > descriptor has a number of parameters. > > If we look at the word 'xfer_length_height'. It's composed of height > (from bits 15:0) and width(from bits 31:16). If the word was expressed > using bit fields, we can describe the word(in big endian) as: > > struct vpdma_dtd { > ... > unsigned int xfer_width:16; > unsigned int xfer_height:16; > ... > ... > }; > > and in little endian as: > > struct vpdma_dtd { > ... > unsigned int xfer_height:16; > unsigned int xfer_width:16; > ... > ... > }; > > So this representation makes it endian dependent. Maybe the comment > should be improved saying that usage of u32 words instead of bit fields > prevents endian issues. No, I don't think that's correct. Endianness is about bytes, not 16 bit words. The above text doesn't make much sense to me. I haven't really worked with endiannes issues, but maybe __le32 and others should be used in the struct, if that struct is read by the HW. And use cpu_to_le32() & others to write those. But googling will probably give more info (I should read also =). >>> + */ >>> +struct vpdma_dtd { >>> + u32 type_ctl_stride; >>> + union { >>> + u32 xfer_length_height; >>> + u32 w1; >>> + }; >>> + dma_addr_t start_addr; >>> + u32 pkt_ctl; >>> + union { >>> + u32 frame_width_height; /* inbound */ >>> + dma_addr_t desc_write_addr; /* outbound */ >> >> Are you sure dma_addr_t is always 32 bit? > > I am not sure about this. Is this struct directly read by the HW, or written to HW? If so, I believe using dma_addr_t is very wrong here. Having a typedef like dma_addr_t hides the actual type used for it. So even if it currently would always be 32bit, there's no guarantee. >> >>> + }; >>> + union { >>> + u32 start_h_v; /* inbound */ >>> + u32 max_width_height; /* outbound */ >>> + }; >>> + u32 client_attr0; >>> + u32 client_attr1; >>> +}; >> >> I'm not sure if I understand the struct right, but presuming this one >> struct is used for both writing and reading, and certain set of fields >> is used for writes and other set for reads, would it make sense to have >> two different structs, instead of using unions? Although they do have >> many common fields, and the unions are a bit scattered there, so I don't >> know if that would be cleaner... > > It helps in a having a common debug function, I don't see much benefit > apart from that. I'll see if it's better to have them as separate structs. Ok. Does the struct have any bit or such that tells us if the current data is inbound or outbound? Tomi [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 901 bytes --]
WARNING: multiple messages have this Message-ID (diff)
From: Tomi Valkeinen <tomi.valkeinen@ti.com> To: Archit Taneja <archit@ti.com> Cc: linux-media@vger.kernel.org, linux-omap@vger.kernel.org, dagriego@biglakesoftware.com, dale@farnsworth.org, pawel@osciak.com, m.szyprowski@samsung.com, hverkuil@xs4all.nl, laurent.pinchart@ideasonboard.com Subject: Re: [PATCH 2/6] v4l: ti-vpe: Add helpers for creating VPDMA descriptors Date: Mon, 5 Aug 2013 16:03:45 +0300 [thread overview] Message-ID: <51FFA2B1.1050809@ti.com> (raw) In-Reply-To: <51FF9517.6000406@ti.com> [-- Attachment #1: Type: text/plain, Size: 4730 bytes --] On 05/08/13 15:05, Archit Taneja wrote: > On Monday 05 August 2013 02:41 PM, Tomi Valkeinen wrote: >> There's quite a bit of code in these dump functions, and they are always >> called. I'm sure getting that data is good for debugging, but I presume >> they are quite useless for normal use. So I think they should be >> compiled in only if some Kconfig option is selected. > > Won't pr_debug() functions actually print something only when > CONFIG_DYNAMIC_DEBUG is selected or if the DEBUG is defined? They will If DEBUG is defined, they are always printed. If dynamic debug is in use, the user has to enable debug prints for VPE for the dumps to be printed. > still consume a lot of code, but it would just end up in dummy printk > calls, right? Yes. Well, I don't know VPE, so I can't really say how much those prints are needed or not. They just looked very verbose to me. I think we should have "normal" level debugging messages compiled in by default, and for "verbose" there should be a separate compile options. With verbose I mean something that may be useful if you are changing the code and want to verify it or debugging some very odd bug. I.e. for the developer of the driver. And with normal something that would be used when, say, somebody uses VPE for in his app, but things don't seem to be quite right, and there's need to get some info on what is going on. I.e. for "normal" user. But that's just my opinion, and it's obviously difficult to define those clearly =). To be honest, I don't know how much overhead very verbose kernel debug prints even cause. Maybe it's negligible. >>> +/* >>> + * data transfer descriptor >>> + * >>> + * All fields are 32 bits to make them endian neutral >> >> What does that mean? Why would 32bit fields make it endian neutral? > > > Each 32 bit field describes one word of the data descriptor. Each > descriptor has a number of parameters. > > If we look at the word 'xfer_length_height'. It's composed of height > (from bits 15:0) and width(from bits 31:16). If the word was expressed > using bit fields, we can describe the word(in big endian) as: > > struct vpdma_dtd { > ... > unsigned int xfer_width:16; > unsigned int xfer_height:16; > ... > ... > }; > > and in little endian as: > > struct vpdma_dtd { > ... > unsigned int xfer_height:16; > unsigned int xfer_width:16; > ... > ... > }; > > So this representation makes it endian dependent. Maybe the comment > should be improved saying that usage of u32 words instead of bit fields > prevents endian issues. No, I don't think that's correct. Endianness is about bytes, not 16 bit words. The above text doesn't make much sense to me. I haven't really worked with endiannes issues, but maybe __le32 and others should be used in the struct, if that struct is read by the HW. And use cpu_to_le32() & others to write those. But googling will probably give more info (I should read also =). >>> + */ >>> +struct vpdma_dtd { >>> + u32 type_ctl_stride; >>> + union { >>> + u32 xfer_length_height; >>> + u32 w1; >>> + }; >>> + dma_addr_t start_addr; >>> + u32 pkt_ctl; >>> + union { >>> + u32 frame_width_height; /* inbound */ >>> + dma_addr_t desc_write_addr; /* outbound */ >> >> Are you sure dma_addr_t is always 32 bit? > > I am not sure about this. Is this struct directly read by the HW, or written to HW? If so, I believe using dma_addr_t is very wrong here. Having a typedef like dma_addr_t hides the actual type used for it. So even if it currently would always be 32bit, there's no guarantee. >> >>> + }; >>> + union { >>> + u32 start_h_v; /* inbound */ >>> + u32 max_width_height; /* outbound */ >>> + }; >>> + u32 client_attr0; >>> + u32 client_attr1; >>> +}; >> >> I'm not sure if I understand the struct right, but presuming this one >> struct is used for both writing and reading, and certain set of fields >> is used for writes and other set for reads, would it make sense to have >> two different structs, instead of using unions? Although they do have >> many common fields, and the unions are a bit scattered there, so I don't >> know if that would be cleaner... > > It helps in a having a common debug function, I don't see much benefit > apart from that. I'll see if it's better to have them as separate structs. Ok. Does the struct have any bit or such that tells us if the current data is inbound or outbound? Tomi [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 901 bytes --]
next prev parent reply other threads:[~2013-08-05 13:03 UTC|newest] Thread overview: 138+ messages / expand[flat|nested] mbox.gz Atom feed top 2013-08-02 14:03 [PATCH 0/6] v4l: VPE mem to mem driver Archit Taneja 2013-08-02 14:03 ` Archit Taneja 2013-08-02 14:03 ` [PATCH 1/6] v4l: ti-vpe: Create a vpdma helper library Archit Taneja 2013-08-02 14:03 ` Archit Taneja 2013-08-05 8:13 ` Tomi Valkeinen 2013-08-05 8:13 ` Tomi Valkeinen 2013-08-05 11:26 ` Archit Taneja 2013-08-05 11:26 ` Archit Taneja 2013-08-05 12:26 ` Tomi Valkeinen 2013-08-05 12:26 ` Tomi Valkeinen 2013-08-08 21:35 ` Laurent Pinchart 2013-08-14 10:19 ` Archit Taneja 2013-08-14 10:19 ` Archit Taneja 2013-08-08 22:04 ` Laurent Pinchart 2013-08-14 10:57 ` Archit Taneja 2013-08-14 10:57 ` Archit Taneja 2013-08-20 11:39 ` Laurent Pinchart 2013-08-20 12:51 ` Archit Taneja 2013-08-20 12:51 ` Archit Taneja 2013-08-20 13:16 ` Archit Taneja 2013-08-20 13:16 ` Archit Taneja 2013-08-20 13:56 ` Laurent Pinchart 2013-08-21 6:47 ` Archit Taneja 2013-08-21 6:47 ` Archit Taneja 2013-08-02 14:03 ` [PATCH 2/6] v4l: ti-vpe: Add helpers for creating VPDMA descriptors Archit Taneja 2013-08-02 14:03 ` Archit Taneja 2013-08-05 9:11 ` Tomi Valkeinen 2013-08-05 9:11 ` Tomi Valkeinen 2013-08-05 12:05 ` Archit Taneja 2013-08-05 12:05 ` Archit Taneja 2013-08-05 13:03 ` Tomi Valkeinen [this message] 2013-08-05 13:03 ` Tomi Valkeinen 2013-08-02 14:03 ` [PATCH 3/6] v4l: ti-vpe: Add VPE mem to mem driver Archit Taneja 2013-08-02 14:03 ` Archit Taneja 2013-08-02 14:36 ` Hans Verkuil 2013-08-02 14:55 ` Archit Taneja 2013-08-02 14:55 ` Archit Taneja 2013-08-05 9:18 ` Tomi Valkeinen 2013-08-05 9:18 ` Tomi Valkeinen 2013-08-02 14:03 ` [PATCH 4/6] v4l: ti-vpe: Add de-interlacer support in VPE Archit Taneja 2013-08-02 14:03 ` Archit Taneja 2013-08-02 14:40 ` Hans Verkuil 2013-08-02 14:03 ` [PATCH 5/6] arm: dra7xx: hwmod data: add VPE hwmod data and ocp_if info Archit Taneja 2013-08-02 14:03 ` Archit Taneja 2013-08-02 14:03 ` [PATCH 6/6] experimental: arm: dts: dra7xx: Add a DT node for VPE Archit Taneja 2013-08-02 14:03 ` Archit Taneja 2013-08-08 22:11 ` Laurent Pinchart 2013-10-25 10:35 ` Archit Taneja 2013-10-25 10:35 ` Archit Taneja 2013-12-03 10:08 ` Archit Taneja 2013-12-03 10:08 ` Archit Taneja 2013-08-20 11:00 ` [PATCH v2 0/6] v4l: VPE mem to mem driver Archit Taneja 2013-08-20 11:00 ` Archit Taneja 2013-08-20 11:00 ` [PATCH v2 1/6] v4l: ti-vpe: Create a vpdma helper library Archit Taneja 2013-08-20 11:00 ` Archit Taneja 2013-08-20 11:00 ` [PATCH v2 2/6] v4l: ti-vpe: Add helpers for creating VPDMA descriptors Archit Taneja 2013-08-20 11:00 ` Archit Taneja 2013-08-20 11:00 ` [PATCH v2 3/6] v4l: ti-vpe: Add VPE mem to mem driver Archit Taneja 2013-08-20 11:00 ` Archit Taneja 2013-08-20 11:00 ` [PATCH v2 4/6] v4l: ti-vpe: Add de-interlacer support in VPE Archit Taneja 2013-08-20 11:00 ` Archit Taneja 2013-08-20 11:00 ` [PATCH v2 5/6] arm: dra7xx: hwmod data: add VPE hwmod data and ocp_if info Archit Taneja 2013-08-20 11:00 ` Archit Taneja 2013-08-20 11:00 ` [PATCH v2 6/6] experimental: arm: dts: dra7xx: Add a DT node for VPE Archit Taneja 2013-08-20 11:00 ` Archit Taneja 2013-08-29 12:32 ` [PATCH v3 0/6] v4l: VPE mem to mem driver Archit Taneja 2013-08-29 12:32 ` Archit Taneja 2013-08-29 12:32 ` [PATCH v3 1/6] v4l: ti-vpe: Create a vpdma helper library Archit Taneja 2013-08-29 12:32 ` Archit Taneja 2013-08-29 12:32 ` [PATCH v3 2/6] v4l: ti-vpe: Add helpers for creating VPDMA descriptors Archit Taneja 2013-08-29 12:32 ` Archit Taneja 2013-08-29 12:32 ` [PATCH v3 3/6] v4l: ti-vpe: Add VPE mem to mem driver Archit Taneja 2013-08-29 12:32 ` Archit Taneja 2013-08-29 13:28 ` Hans Verkuil 2013-08-30 6:47 ` Archit Taneja 2013-08-30 6:47 ` Archit Taneja 2013-08-30 7:07 ` Hans Verkuil 2013-08-30 10:05 ` Archit Taneja 2013-08-30 10:05 ` Archit Taneja 2013-08-30 10:44 ` Hans Verkuil 2013-09-05 5:56 ` Archit Taneja 2013-09-05 5:56 ` Archit Taneja 2013-08-29 12:32 ` [PATCH v3 4/6] v4l: ti-vpe: Add de-interlacer support in VPE Archit Taneja 2013-08-29 12:32 ` Archit Taneja 2013-08-29 12:32 ` [PATCH v3 5/6] arm: dra7xx: hwmod data: add VPE hwmod data and ocp_if info Archit Taneja 2013-08-29 12:32 ` Archit Taneja 2013-08-29 12:42 ` Rajendra Nayak 2013-08-29 12:42 ` Rajendra Nayak 2013-08-29 13:42 ` Archit Taneja 2013-08-29 13:42 ` Archit Taneja 2013-08-29 12:32 ` [PATCH v3 6/6] experimental: arm: dts: dra7xx: Add a DT node for VPE Archit Taneja 2013-08-29 12:32 ` Archit Taneja 2013-09-06 10:12 ` [PATCH v4 0/4] v4l: VPE mem to mem driver Archit Taneja 2013-09-06 10:12 ` Archit Taneja 2013-09-06 10:12 ` [PATCH v4 1/4] v4l: ti-vpe: Create a vpdma helper library Archit Taneja 2013-09-06 10:12 ` Archit Taneja 2013-10-07 7:46 ` Hans Verkuil 2013-09-06 10:12 ` [PATCH v4 2/4] v4l: ti-vpe: Add helpers for creating VPDMA descriptors Archit Taneja 2013-09-06 10:12 ` Archit Taneja 2013-10-07 7:46 ` Hans Verkuil 2013-09-06 10:12 ` [PATCH v4 3/4] v4l: ti-vpe: Add VPE mem to mem driver Archit Taneja 2013-09-06 10:12 ` Archit Taneja 2013-10-07 7:55 ` Hans Verkuil 2013-10-07 9:16 ` Archit Taneja 2013-10-07 9:16 ` Archit Taneja 2013-10-07 9:34 ` Hans Verkuil 2013-10-07 10:22 ` Archit Taneja 2013-10-07 10:22 ` Archit Taneja 2013-10-07 14:02 ` Hans Verkuil 2013-10-07 14:34 ` Archit Taneja 2013-10-07 14:34 ` Archit Taneja 2013-09-06 10:12 ` [PATCH v4 4/4] v4l: ti-vpe: Add de-interlacer support in VPE Archit Taneja 2013-09-06 10:12 ` Archit Taneja 2013-10-07 7:57 ` Hans Verkuil 2013-09-16 6:59 ` [PATCH v4 0/4] v4l: VPE mem to mem driver Archit Taneja 2013-09-16 6:59 ` Archit Taneja 2013-10-07 6:39 ` Archit Taneja 2013-10-07 6:39 ` Archit Taneja 2013-10-09 14:29 ` [PATCH v5 3/4] v4l: ti-vpe: Add " Archit Taneja 2013-10-09 14:29 ` Archit Taneja 2013-10-11 7:46 ` Hans Verkuil 2013-10-15 13:47 ` Archit Taneja 2013-10-15 13:47 ` Archit Taneja 2013-10-15 13:51 ` Hans Verkuil 2013-10-15 14:13 ` Kamil Debski 2013-10-15 15:54 ` Kamil Debski 2013-10-16 5:08 ` Archit Taneja 2013-10-16 5:08 ` Archit Taneja 2013-10-16 5:36 ` [PATCH v5 0/4] v4l: " Archit Taneja 2013-10-16 5:36 ` Archit Taneja 2013-10-16 5:36 ` [PATCH v5 1/4] v4l: ti-vpe: Create a vpdma helper library Archit Taneja 2013-10-16 5:36 ` Archit Taneja 2013-10-16 5:36 ` [PATCH v5 2/4] v4l: ti-vpe: Add helpers for creating VPDMA descriptors Archit Taneja 2013-10-16 5:36 ` Archit Taneja 2013-10-16 5:36 ` [PATCH v5 3/4] v4l: ti-vpe: Add VPE mem to mem driver Archit Taneja 2013-10-16 5:36 ` Archit Taneja 2013-10-16 5:36 ` [PATCH v5 4/4] v4l: ti-vpe: Add de-interlacer support in VPE Archit Taneja 2013-10-16 5:36 ` Archit Taneja
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=51FFA2B1.1050809@ti.com \ --to=tomi.valkeinen@ti.com \ --cc=archit@ti.com \ --cc=dagriego@biglakesoftware.com \ --cc=dale@farnsworth.org \ --cc=hverkuil@xs4all.nl \ --cc=laurent.pinchart@ideasonboard.com \ --cc=linux-media@vger.kernel.org \ --cc=linux-omap@vger.kernel.org \ --cc=m.szyprowski@samsung.com \ --cc=pawel@osciak.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.