[PATCH] media: venus: fix build on 32bit environments

[PATCH] media: venus: fix build on 32bit environments

[Mauro Carvalho Chehab]

As reported by jenkins@linuxtv.org, the build with i386 fails
with:

	ld: drivers/media/platform/qcom/venus/helpers.o: in function `venus_helper_load_scale_clocks':
	(.text+0x1d77): undefined reference to `__udivdi3'
	ld: (.text+0x1dce): undefined reference to `__udivdi3'
	make: *** [Makefile:1094: vmlinux] Error 1

That's because it divides an u32 bit integer by a u64 one.

Fix it by explicitly callind do_div.

That's said, why fps is a 64 bits integer?

Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
---
 drivers/media/platform/qcom/venus/helpers.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
index 5ea5d90f8e5f..09fa87e3c0a0 100644
--- a/drivers/media/platform/qcom/venus/helpers.c
+++ b/drivers/media/platform/qcom/venus/helpers.c
@@ -522,8 +522,14 @@ static unsigned long calculate_inst_freq(struct venus_inst *inst,
 	unsigned long vpp_freq = 0, vsp_freq = 0;
 	u64 fps = inst->fps;
 	u32 mbs_per_sec;
+	u64 tmp;
+
+
+	tmp = load_per_instance(inst);
+	do_div(tmp, inst->fps);
+
+	mbs_per_sec = (u32)tmp;
 
-	mbs_per_sec = load_per_instance(inst) / inst->fps;
 	vpp_freq = mbs_per_sec * inst->clk_data.codec_freq_data->vpp_freq;
 	/* 21 / 20 is overhead factor */
 	vpp_freq += vpp_freq / 20;
-- 
2.21.0

Re: [PATCH] media: venus: fix build on 32bit environments

[Stanimir Varbanov]

Hi Mauro,

Thanks for the fix!

On 10/7/19 4:37 PM, Mauro Carvalho Chehab wrote:
> As reported by jenkins@linuxtv.org, the build with i386 fails
> with:
> 
> 	ld: drivers/media/platform/qcom/venus/helpers.o: in function `venus_helper_load_scale_clocks':
> 	(.text+0x1d77): undefined reference to `__udivdi3'
> 	ld: (.text+0x1dce): undefined reference to `__udivdi3'
> 	make: *** [Makefile:1094: vmlinux] Error 1
> 
> That's because it divides an u32 bit integer by a u64 one.

General question, shouldn't such errors been catch from builder on the
pull request?

> 
> Fix it by explicitly callind do_div.
> 
> That's said, why fps is a 64 bits integer?

I don't have other explanation than - just to avoid casting after the
math in vdec/venc_s_parm() functions.

> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> ---
>  drivers/media/platform/qcom/venus/helpers.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
> index 5ea5d90f8e5f..09fa87e3c0a0 100644
> --- a/drivers/media/platform/qcom/venus/helpers.c
> +++ b/drivers/media/platform/qcom/venus/helpers.c
> @@ -522,8 +522,14 @@ static unsigned long calculate_inst_freq(struct venus_inst *inst,
>  	unsigned long vpp_freq = 0, vsp_freq = 0;
>  	u64 fps = inst->fps;
>  	u32 mbs_per_sec;
> +	u64 tmp;
> +

you have extra blank line here.

> +
> +	tmp = load_per_instance(inst);
> +	do_div(tmp, inst->fps);
> +
> +	mbs_per_sec = (u32)tmp;
>  
> -	mbs_per_sec = load_per_instance(inst) / inst->fps;
>  	vpp_freq = mbs_per_sec * inst->clk_data.codec_freq_data->vpp_freq;
>  	/* 21 / 20 is overhead factor */
>  	vpp_freq += vpp_freq / 20;
> 

I guess this fix should be squashed with the commit which introduce it :(

Note taken, always build patches on i386 :/

-- 
regards,
Stan

Re: [PATCH] media: venus: fix build on 32bit environments

[Mauro Carvalho Chehab]

Em Mon, 7 Oct 2019 17:38:53 +0300
Stanimir Varbanov <stanimir.varbanov@linaro.org> escreveu:

> Hi Mauro,
> 
> Thanks for the fix!
> 
> On 10/7/19 4:37 PM, Mauro Carvalho Chehab wrote:
> > As reported by jenkins@linuxtv.org, the build with i386 fails
> > with:
> > 
> > 	ld: drivers/media/platform/qcom/venus/helpers.o: in function `venus_helper_load_scale_clocks':
> > 	(.text+0x1d77): undefined reference to `__udivdi3'
> > 	ld: (.text+0x1dce): undefined reference to `__udivdi3'
> > 	make: *** [Makefile:1094: vmlinux] Error 1
> > 
> > That's because it divides an u32 bit integer by a u64 one.  
> 
> General question, shouldn't such errors been catch from builder on the
> pull request?

No, the pull request builder just builds drivers/media automatically
when a patch arrives patchwork.

This error only happens after a full build, when it tries to linkedit
vmlinux.

Due to time contraints, the complete build is done only after merging 
stuff at patchwork, as it may take hours to build for the platforms we
care.

My long term would be to push patches on a temporary tree, with would
start the builders. Only after all builders finish without issues, the
master one would be updated.

In thesis, jenkins supports it via pipelines. Basically, I would need to
setup a pipeline that:

1) it would fetch the latest tree on a common repository;

2) for each arch/config we support, it will start a builder;

3) after all builder process finishes, it will check if all builds
   went smoothly;

4) if everything runs smoothly, do a git fetch to the "permanent"
tree.

I quickly looked at Jenkins docs a few times: setting it doesn't
seem to be trivial, as it envolves learning a macro language that 
Jenkins uses internally. 

I failed to find a good documentation about the language it uses, and
was unable to find any example of a similar setup. All examples I
saw assumes that the tasks at the pipeline will use the same workspace.

I intend to seek for some time to better understand the pipeline
settings on Jenkins in the future.

> > Fix it by explicitly callind do_div.
> > 
> > That's said, why fps is a 64 bits integer?  
> 
> I don't have other explanation than - just to avoid casting after the
> math in vdec/venc_s_parm() functions.

IMO, avoiding the do_div() would be better than avoid the casting, but
that's your call.

> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> > ---
> >  drivers/media/platform/qcom/venus/helpers.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
> > index 5ea5d90f8e5f..09fa87e3c0a0 100644
> > --- a/drivers/media/platform/qcom/venus/helpers.c
> > +++ b/drivers/media/platform/qcom/venus/helpers.c
> > @@ -522,8 +522,14 @@ static unsigned long calculate_inst_freq(struct venus_inst *inst,
> >  	unsigned long vpp_freq = 0, vsp_freq = 0;
> >  	u64 fps = inst->fps;
> >  	u32 mbs_per_sec;
> > +	u64 tmp;
> > +  
> 
> you have extra blank line here.

Ok, I'll remove it.

> 
> > +
> > +	tmp = load_per_instance(inst);
> > +	do_div(tmp, inst->fps);
> > +
> > +	mbs_per_sec = (u32)tmp;
> >  
> > -	mbs_per_sec = load_per_instance(inst) / inst->fps;
> >  	vpp_freq = mbs_per_sec * inst->clk_data.codec_freq_data->vpp_freq;
> >  	/* 21 / 20 is overhead factor */
> >  	vpp_freq += vpp_freq / 20;
> >   
> 
> I guess this fix should be squashed with the commit which introduce it :(

Too late for that.

> Note taken, always build patches on i386 :/
> 



Thanks,
Mauro

Re: [PATCH] media: venus: fix build on 32bit environments

[Mauro Carvalho Chehab]

Em Mon, 7 Oct 2019 11:55:51 -0300
Mauro Carvalho Chehab <mchehab+samsung@kernel.org> escreveu:

> Em Mon, 7 Oct 2019 17:38:53 +0300
> Stanimir Varbanov <stanimir.varbanov@linaro.org> escreveu:
> 
> > Hi Mauro,
> > 
> > Thanks for the fix!
> > 
> > On 10/7/19 4:37 PM, Mauro Carvalho Chehab wrote:  
> > > As reported by jenkins@linuxtv.org, the build with i386 fails
> > > with:
> > > 
> > > 	ld: drivers/media/platform/qcom/venus/helpers.o: in function `venus_helper_load_scale_clocks':
> > > 	(.text+0x1d77): undefined reference to `__udivdi3'
> > > 	ld: (.text+0x1dce): undefined reference to `__udivdi3'
> > > 	make: *** [Makefile:1094: vmlinux] Error 1
> > > 
> > > That's because it divides an u32 bit integer by a u64 one.    
> > 
> > General question, shouldn't such errors been catch from builder on the
> > pull request?  
> 
> No, the pull request builder just builds drivers/media automatically
> when a patch arrives patchwork.
> 
> This error only happens after a full build, when it tries to linkedit
> vmlinux.
> 
> Due to time contraints, the complete build is done only after merging 
> stuff at patchwork, as it may take hours to build for the platforms we
> care.
> 
> My long term would be to push patches on a temporary tree, with would
> start the builders. Only after all builders finish without issues, the
> master one would be updated.
> 
> In thesis, jenkins supports it via pipelines. Basically, I would need to
> setup a pipeline that:
> 
> 1) it would fetch the latest tree on a common repository;
> 
> 2) for each arch/config we support, it will start a builder;
> 
> 3) after all builder process finishes, it will check if all builds
>    went smoothly;
> 
> 4) if everything runs smoothly, do a git fetch to the "permanent"
> tree.
> 
> I quickly looked at Jenkins docs a few times: setting it doesn't
> seem to be trivial, as it envolves learning a macro language that 
> Jenkins uses internally. 
> 
> I failed to find a good documentation about the language it uses, and
> was unable to find any example of a similar setup. All examples I
> saw assumes that the tasks at the pipeline will use the same workspace.
> 
> I intend to seek for some time to better understand the pipeline
> settings on Jenkins in the future.

Ok, I'm investing some time on trying to set a single pipeline that will
handle all Kernel builds at once. It should take some time for it
to run, but hopefully I may be able to replace it by the individual
build jobs and, on some future, use it to automate the push to my
master branch in order to happen only after passing the tests.

The pipeline is at:

	https://builder.linuxtv.org/job/media_kernel_pipeline/

Just the fetching time usually takes ~30 mins or so. So, it should
take a while to be sure that what I wrote would do the right thing.

Thanks,
Mauro