diff mbox

video: mx3_camera: Allocate camera object via kzalloc

Message ID 1329761467-14417-1-git-send-email-festevam@gmail.com (mailing list archive)
State Rejected, archived
Headers

Commit Message

Fabio Estevam Feb. 20, 2012, 6:11 p.m. UTC 
  Align mx3_camera driver with the other soc camera driver implementations
by allocating the camera object via kzalloc.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 drivers/media/video/mx3_camera.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Comments

Guennadi Liakhovetski Feb. 20, 2012, 6:17 p.m. UTC | #1 
On Mon, 20 Feb 2012, Fabio Estevam wrote:

> Align mx3_camera driver with the other soc camera driver implementations
> by allocating the camera object via kzalloc.

Sorry, any specific reason, why you think this "aligning" is so important? 
I personally don't see any.

Thanks
Guennadi

> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
>  drivers/media/video/mx3_camera.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/media/video/mx3_camera.c b/drivers/media/video/mx3_camera.c
> index 7452277..cccd574 100644
> --- a/drivers/media/video/mx3_camera.c
> +++ b/drivers/media/video/mx3_camera.c
> @@ -1159,7 +1159,7 @@ static int __devinit mx3_camera_probe(struct platform_device *pdev)
>  		goto egetres;
>  	}
>  
> -	mx3_cam = vzalloc(sizeof(*mx3_cam));
> +	mx3_cam = kzalloc(sizeof(*mx3_cam), GFP_KERNEL);
>  	if (!mx3_cam) {
>  		dev_err(&pdev->dev, "Could not allocate mx3 camera object\n");
>  		err = -ENOMEM;
> -- 
> 1.7.1
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Fabio Estevam Feb. 20, 2012, 6:23 p.m. UTC | #2 
On Mon, Feb 20, 2012 at 4:17 PM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> On Mon, 20 Feb 2012, Fabio Estevam wrote:
>
>> Align mx3_camera driver with the other soc camera driver implementations
>> by allocating the camera object via kzalloc.
>
> Sorry, any specific reason, why you think this "aligning" is so important?

Not really.

Just compared it with all other soc camera drivers I found and
mx3_camera was the only one that uses "vzalloc"

Any specific reason that requires mx3_camera to use "vzalloc" instead
of "kzalloc"?

Tested with kzalloc and it worked fine on my mx31pdk.

Regards,

Fabio Estevam
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mauro Carvalho Chehab March 20, 2012, 2:01 a.m. UTC | #3 
Em 20-02-2012 16:23, Fabio Estevam escreveu:
> On Mon, Feb 20, 2012 at 4:17 PM, Guennadi Liakhovetski
> <g.liakhovetski@gmx.de> wrote:
>> On Mon, 20 Feb 2012, Fabio Estevam wrote:
>>
>>> Align mx3_camera driver with the other soc camera driver implementations
>>> by allocating the camera object via kzalloc.
>>
>> Sorry, any specific reason, why you think this "aligning" is so important?
> 
> Not really.
> 
> Just compared it with all other soc camera drivers I found and
> mx3_camera was the only one that uses "vzalloc"
> 
> Any specific reason that requires mx3_camera to use "vzalloc" instead
> of "kzalloc"?

kzalloc() is more restrictive than vzalloc(). With v*alloc, it will allocate
a virtual memory area, with can be discontinuous, while kzalloc will get
a continuous area.

The DMA logic need to be prepared for virtual memory, if v*alloc() is used
(e. g. using videobuf2-vmalloc).

As it is currently including media/videobuf2-dma-contig.h, I this patch
makes sense on my eyes.

> 
> Tested with kzalloc and it worked fine on my mx31pdk.

If the driver is working with vzalloc, then maybe it is due to some arch-specific
implementation for v*alloc. It shouldn't be working like that.

Regards,
Mauro
> 
> Regards,
> 
> Fabio Estevam
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Guennadi Liakhovetski March 20, 2012, 7:57 a.m. UTC | #4 
On Mon, 19 Mar 2012, Mauro Carvalho Chehab wrote:

> Em 20-02-2012 16:23, Fabio Estevam escreveu:
> > On Mon, Feb 20, 2012 at 4:17 PM, Guennadi Liakhovetski
> > <g.liakhovetski@gmx.de> wrote:
> >> On Mon, 20 Feb 2012, Fabio Estevam wrote:
> >>
> >>> Align mx3_camera driver with the other soc camera driver implementations
> >>> by allocating the camera object via kzalloc.
> >>
> >> Sorry, any specific reason, why you think this "aligning" is so important?
> > 
> > Not really.
> > 
> > Just compared it with all other soc camera drivers I found and
> > mx3_camera was the only one that uses "vzalloc"
> > 
> > Any specific reason that requires mx3_camera to use "vzalloc" instead
> > of "kzalloc"?
> 
> kzalloc() is more restrictive than vzalloc(). With v*alloc, it will allocate
> a virtual memory area, with can be discontinuous, while kzalloc will get
> a continuous area.
> 
> The DMA logic need to be prepared for virtual memory, if v*alloc() is used
> (e. g. using videobuf2-vmalloc).
> 
> As it is currently including media/videobuf2-dma-contig.h, I this patch
> makes sense on my eyes.

Don't think so. vzalloc() is used in mx3_camera to allocate driver private 
data objects and are never used for DMA, so, it doesn't have any 
restrictions on contiguity, coherency, alignment etc.

One could argue, that since the struct is anyway smaller than 1 page, it 
anyway will be allocated in a physically contiguous memory area (will it?) 
and so, maybe, kmalloc() is less heavy weight than vmalloc() and might 
save a couple of CPU cycles, but I don't think it's anything important, 
that we should be optimising for.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Fabio Estevam March 20, 2012, 12:37 p.m. UTC | #5 
Hi Guennadi,

On 3/20/12, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:

> Don't think so. vzalloc() is used in mx3_camera to allocate driver private
> data objects and are never used for DMA, so, it doesn't have any
> restrictions on contiguity, coherency, alignment etc.

Is this valid only for mx3_camera driver?

All other soc camera drivers use kzalloc.

What makes mx3_camera different in this respect?
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Guennadi Liakhovetski March 20, 2012, 12:40 p.m. UTC | #6 
On Tue, 20 Mar 2012, Fabio Estevam wrote:

> Hi Guennadi,
> 
> On 3/20/12, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> 
> > Don't think so. vzalloc() is used in mx3_camera to allocate driver private
> > data objects and are never used for DMA, so, it doesn't have any
> > restrictions on contiguity, coherency, alignment etc.
> 
> Is this valid only for mx3_camera driver?

No

> All other soc camera drivers use kzalloc.
> 
> What makes mx3_camera different in this respect?

Nothing

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Fabio Estevam March 20, 2012, 12:45 p.m. UTC | #7 
On 3/20/12, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:

>> Is this valid only for mx3_camera driver?
>
> No
>
>> All other soc camera drivers use kzalloc.
>>
>> What makes mx3_camera different in this respect?
>
> Nothing

Ok, so isn't my patch correct then?
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mauro Carvalho Chehab March 20, 2012, 12:53 p.m. UTC | #8 
Em 20-03-2012 04:57, Guennadi Liakhovetski escreveu:
> On Mon, 19 Mar 2012, Mauro Carvalho Chehab wrote:
> 
>> Em 20-02-2012 16:23, Fabio Estevam escreveu:
>>> On Mon, Feb 20, 2012 at 4:17 PM, Guennadi Liakhovetski
>>> <g.liakhovetski@gmx.de> wrote:
>>>> On Mon, 20 Feb 2012, Fabio Estevam wrote:
>>>>
>>>>> Align mx3_camera driver with the other soc camera driver implementations
>>>>> by allocating the camera object via kzalloc.
>>>>
>>>> Sorry, any specific reason, why you think this "aligning" is so important?
>>>
>>> Not really.
>>>
>>> Just compared it with all other soc camera drivers I found and
>>> mx3_camera was the only one that uses "vzalloc"
>>>
>>> Any specific reason that requires mx3_camera to use "vzalloc" instead
>>> of "kzalloc"?
>>
>> kzalloc() is more restrictive than vzalloc(). With v*alloc, it will allocate
>> a virtual memory area, with can be discontinuous, while kzalloc will get
>> a continuous area.
>>
>> The DMA logic need to be prepared for virtual memory, if v*alloc() is used
>> (e. g. using videobuf2-vmalloc).
>>
>> As it is currently including media/videobuf2-dma-contig.h, I this patch
>> makes sense on my eyes.
> 
> Don't think so. vzalloc() is used in mx3_camera to allocate driver private 
> data objects and are never used for DMA, so, it doesn't have any 
> restrictions on contiguity, coherency, alignment etc.

OK. In this case, using v*alloc()/vfree() won't be that different than k*alloc()/k*free().

> One could argue, that since the struct is anyway smaller than 1 page, it 
> anyway will be allocated in a physically contiguous memory area (will it?) 
> and so, maybe, kmalloc() is less heavy weight than vmalloc() and might 
> save a couple of CPU cycles, but I don't think it's anything important, 
> that we should be optimising for.

Yes. 

There's another aspect to consider there: the vmalloc space is limited
(there's a boot time parameter to regulate its size)[1]. I dunno why, nor
what are the consequences of using a bigger value, but I think a big vmalloc
size creates a big table to map between virtual/physical memory space.

Yet, a single page is far below the vmaloc default max size,
except if the system has very severe memory constraints.

[1] On x86, I think that the default is 256MB. Several video adapters
eat a lot of space there. I use a bigger value here, otherwise my 3-head
system won't initialize all screens.

Regards,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Guennadi Liakhovetski March 20, 2012, 1:11 p.m. UTC | #9 
On Tue, 20 Mar 2012, Fabio Estevam wrote:

> On 3/20/12, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> 
> >> Is this valid only for mx3_camera driver?
> >
> > No
> >
> >> All other soc camera drivers use kzalloc.
> >>
> >> What makes mx3_camera different in this respect?
> >
> > Nothing
> 
> Ok, so isn't my patch correct then?

No. It doesn't improve anything.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sril March 20, 2012, 1:20 p.m. UTC | #10 
Hi,
I acquire this usb stick 07ca:a835 and it's still does not work properly.

Now, with the af9035 patch from 
http://openpli.git.sourceforge.net/git/gitweb.cgi?p=openpli/openembedded;a=blob_plain;f=recipes/linux/linux-etxx00/dvb-usb-af9035.patch;hb=HEAD

the tv interface is recognize but have trouble with kaffeine, tvtime and gnome-dvb-daemon.

Here's a trace from gnome : "af9033: I2C read failed reg:0047".
From kaffeine : "kaffeine(3817) DvbDevice::frontendEvent: tuning failed".
From tvtime, nothing card doesn't appear : probably missing conf, it's ok.

This message try to follow Andrej Podzimekand Gianluca Gennari's messages on 02/07/2012.

Does someone got ideas about what to do to correct this ?

kernel : 3.2.11 with patch noticed. No externe v4l at all during construct. Compile fine :

Mar 18 16:09:43 localhost kernel: [  305.726981] dvb-usb: found a 'Avermedia AverTV Volar HD & HD PRO (A835)' in cold state, will try to load a firmware
Mar 18 16:09:43 localhost kernel: [  305.742050] dvb-usb: downloading firmware from file 'dvb-usb-af9035-01.fw'
Mar 18 16:09:43 localhost kernel: [  306.039886] dvb-usb: found a 'Avermedia AverTV Volar HD & HD PRO (A835)' in warm state.
Mar 18 16:09:43 localhost kernel: [  306.040032] dvb-usb: will pass the complete MPEG2 transport stream to the software demuxer.
Mar 18 16:09:43 localhost kernel: [  306.040406] DVB: registering new adapter (Avermedia AverTV Volar HD & HD PRO (A835))
Mar 18 16:09:43 localhost kernel: [  306.078104] af9033: firmware version: LINK:11.15.10.0 OFDM:5.48.10.0
Mar 18 16:09:43 localhost kernel: [  306.080355] DVB: registering adapter 0 frontend 0 (Afatech AF9033 DVB-T)...
Mar 18 16:09:43 localhost kernel: [  306.129981] tda18218: NXP TDA18218HN successfully identified.
Mar 18 16:09:43 localhost kernel: [  306.131483] dvb-usb: Avermedia AverTV Volar HD & HD PRO (A835) successfully initialized and connected.
Mar 18 16:09:43 localhost kernel: [  306.140531] usbcore: registered new interface driver dvb_usb_af9035

Best regards.
See ya.

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/media/video/mx3_camera.c b/drivers/media/video/mx3_camera.c
index 7452277..cccd574 100644
--- a/drivers/media/video/mx3_camera.c
+++ b/drivers/media/video/mx3_camera.c
@@ -1159,7 +1159,7 @@  static int __devinit mx3_camera_probe(struct platform_device *pdev)
 		goto egetres;
 	}
 
-	mx3_cam = vzalloc(sizeof(*mx3_cam));
+	mx3_cam = kzalloc(sizeof(*mx3_cam), GFP_KERNEL);
 	if (!mx3_cam) {
 		dev_err(&pdev->dev, "Could not allocate mx3 camera object\n");
 		err = -ENOMEM;