| Message ID | 20200305124525.14555-3-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw, ui, virtfs-proxy-helper: Reduce QEMU .data/.rodata/.bss footprint | expand |
On Thu, Mar 05, 2020 at 01:45:18PM +0100, Philippe Mathieu-Daudé wrote: > This buffer is only used by the adlib audio device. Move it to > the .heap to release 32KiB of .bss (size reported on x86_64 host). > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/audio/fmopl.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/hw/audio/fmopl.c b/hw/audio/fmopl.c > index 173a7521f2..356d4dfbca 100644 > --- a/hw/audio/fmopl.c > +++ b/hw/audio/fmopl.c > @@ -186,7 +186,7 @@ static int32_t *VIB_TABLE; > > /* envelope output curve table */ > /* attack + decay + OFF */ > -static int32_t ENV_CURVE[2*EG_ENT+1]; > +static int32_t *ENV_CURVE; > > /* multiple table */ > #define ML 2 > @@ -1090,6 +1090,7 @@ FM_OPL *OPLCreate(int clock, int rate) > OPL->clock = clock; > OPL->rate = rate; > OPL->max_ch = max_ch; > + ENV_CURVE = g_new(int32_t, 2 * EG_ENT + 1); > /* init grobal tables */ > OPL_initialize(OPL); > /* reset chip */ > @@ -1127,6 +1128,7 @@ void OPLDestroy(FM_OPL *OPL) > #endif > OPL_UnLockTable(); > free(OPL); > + g_free(ENV_CURVE); Just for curiosity, here the entire fmopl.c is indented with tabs. In this case, is it better to continue with the tabs or use spaces for new changes? Anyway the changes LGTM: Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
On Thu, Mar 05, 2020 at 02:44:03PM +0100, Stefano Garzarella wrote: > On Thu, Mar 05, 2020 at 01:45:18PM +0100, Philippe Mathieu-Daudé wrote: > > This buffer is only used by the adlib audio device. Move it to > > the .heap to release 32KiB of .bss (size reported on x86_64 host). > > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > --- > > hw/audio/fmopl.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/hw/audio/fmopl.c b/hw/audio/fmopl.c > > index 173a7521f2..356d4dfbca 100644 > > --- a/hw/audio/fmopl.c > > +++ b/hw/audio/fmopl.c > > @@ -186,7 +186,7 @@ static int32_t *VIB_TABLE; > > > > /* envelope output curve table */ > > /* attack + decay + OFF */ > > -static int32_t ENV_CURVE[2*EG_ENT+1]; > > +static int32_t *ENV_CURVE; > > > > /* multiple table */ > > #define ML 2 > > @@ -1090,6 +1090,7 @@ FM_OPL *OPLCreate(int clock, int rate) > > OPL->clock = clock; > > OPL->rate = rate; > > OPL->max_ch = max_ch; > > + ENV_CURVE = g_new(int32_t, 2 * EG_ENT + 1); Should we use g_new0() ? > > /* init grobal tables */ > > OPL_initialize(OPL); > > /* reset chip */ > > @@ -1127,6 +1128,7 @@ void OPLDestroy(FM_OPL *OPL) > > #endif > > OPL_UnLockTable(); > > free(OPL); > > + g_free(ENV_CURVE); > > Just for curiosity, here the entire fmopl.c is indented with tabs. > > In this case, is it better to continue with the tabs or use spaces > for new changes? > > Anyway the changes LGTM: > Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
On 3/5/20 2:44 PM, Stefano Garzarella wrote: > On Thu, Mar 05, 2020 at 01:45:18PM +0100, Philippe Mathieu-Daudé wrote: >> This buffer is only used by the adlib audio device. Move it to >> the .heap to release 32KiB of .bss (size reported on x86_64 host). >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> hw/audio/fmopl.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/hw/audio/fmopl.c b/hw/audio/fmopl.c >> index 173a7521f2..356d4dfbca 100644 >> --- a/hw/audio/fmopl.c >> +++ b/hw/audio/fmopl.c >> @@ -186,7 +186,7 @@ static int32_t *VIB_TABLE; >> >> /* envelope output curve table */ >> /* attack + decay + OFF */ >> -static int32_t ENV_CURVE[2*EG_ENT+1]; >> +static int32_t *ENV_CURVE; >> >> /* multiple table */ >> #define ML 2 >> @@ -1090,6 +1090,7 @@ FM_OPL *OPLCreate(int clock, int rate) >> OPL->clock = clock; >> OPL->rate = rate; >> OPL->max_ch = max_ch; >> + ENV_CURVE = g_new(int32_t, 2 * EG_ENT + 1); >> /* init grobal tables */ >> OPL_initialize(OPL); >> /* reset chip */ >> @@ -1127,6 +1128,7 @@ void OPLDestroy(FM_OPL *OPL) >> #endif >> OPL_UnLockTable(); >> free(OPL); >> + g_free(ENV_CURVE); > > Just for curiosity, here the entire fmopl.c is indented with tabs. > > In this case, is it better to continue with the tabs or use spaces > for new changes? checkpatch.pl doesn't allow us to use spaces. > > Anyway the changes LGTM: > Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> > Thanks!
On 3/5/20 2:48 PM, Stefano Garzarella wrote: > On Thu, Mar 05, 2020 at 02:44:03PM +0100, Stefano Garzarella wrote: >> On Thu, Mar 05, 2020 at 01:45:18PM +0100, Philippe Mathieu-Daudé wrote: >>> This buffer is only used by the adlib audio device. Move it to >>> the .heap to release 32KiB of .bss (size reported on x86_64 host). >>> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>> --- >>> hw/audio/fmopl.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/audio/fmopl.c b/hw/audio/fmopl.c >>> index 173a7521f2..356d4dfbca 100644 >>> --- a/hw/audio/fmopl.c >>> +++ b/hw/audio/fmopl.c >>> @@ -186,7 +186,7 @@ static int32_t *VIB_TABLE; >>> >>> /* envelope output curve table */ >>> /* attack + decay + OFF */ >>> -static int32_t ENV_CURVE[2*EG_ENT+1]; >>> +static int32_t *ENV_CURVE; >>> >>> /* multiple table */ >>> #define ML 2 >>> @@ -1090,6 +1090,7 @@ FM_OPL *OPLCreate(int clock, int rate) >>> OPL->clock = clock; >>> OPL->rate = rate; >>> OPL->max_ch = max_ch; >>> + ENV_CURVE = g_new(int32_t, 2 * EG_ENT + 1); > > Should we use g_new0() ? No because the array is filled before being used. I can add a note about this. > >>> /* init grobal tables */ >>> OPL_initialize(OPL); >>> /* reset chip */ >>> @@ -1127,6 +1128,7 @@ void OPLDestroy(FM_OPL *OPL) >>> #endif >>> OPL_UnLockTable(); >>> free(OPL); >>> + g_free(ENV_CURVE); >> >> Just for curiosity, here the entire fmopl.c is indented with tabs. >> >> In this case, is it better to continue with the tabs or use spaces >> for new changes? >> >> Anyway the changes LGTM: >> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> >
On Thu, Mar 05, 2020 at 02:49:37PM +0100, Philippe Mathieu-Daudé wrote: > On 3/5/20 2:44 PM, Stefano Garzarella wrote: > > On Thu, Mar 05, 2020 at 01:45:18PM +0100, Philippe Mathieu-Daudé wrote: > > > This buffer is only used by the adlib audio device. Move it to > > > the .heap to release 32KiB of .bss (size reported on x86_64 host). > > > > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > > --- > > > hw/audio/fmopl.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/hw/audio/fmopl.c b/hw/audio/fmopl.c > > > index 173a7521f2..356d4dfbca 100644 > > > --- a/hw/audio/fmopl.c > > > +++ b/hw/audio/fmopl.c > > > @@ -186,7 +186,7 @@ static int32_t *VIB_TABLE; > > > /* envelope output curve table */ > > > /* attack + decay + OFF */ > > > -static int32_t ENV_CURVE[2*EG_ENT+1]; > > > +static int32_t *ENV_CURVE; > > > /* multiple table */ > > > #define ML 2 > > > @@ -1090,6 +1090,7 @@ FM_OPL *OPLCreate(int clock, int rate) > > > OPL->clock = clock; > > > OPL->rate = rate; > > > OPL->max_ch = max_ch; > > > + ENV_CURVE = g_new(int32_t, 2 * EG_ENT + 1); > > > /* init grobal tables */ > > > OPL_initialize(OPL); > > > /* reset chip */ > > > @@ -1127,6 +1128,7 @@ void OPLDestroy(FM_OPL *OPL) > > > #endif > > > OPL_UnLockTable(); > > > free(OPL); > > > + g_free(ENV_CURVE); > > > > Just for curiosity, here the entire fmopl.c is indented with tabs. > > > > In this case, is it better to continue with the tabs or use spaces > > for new changes? > > checkpatch.pl doesn't allow us to use spaces. ITYM 'tabs' here. IMHO this is a case where the cure is worse than the disease. In priority order I think we generally rank: * Exclusive use of space indent (best) * Exclusive use of tab indent (bad) * Mixture of space & tab indent (terrible) IOW, this is a case where I would suggest either: - Have a cleanup commit first that eliminates all tabs or - Continue to use tabs consistently & ignore checkpatch Regards, Daniel
On Thu, Mar 05, 2020 at 02:50:47PM +0100, Philippe Mathieu-Daudé wrote: > On 3/5/20 2:48 PM, Stefano Garzarella wrote: > > On Thu, Mar 05, 2020 at 02:44:03PM +0100, Stefano Garzarella wrote: > > > On Thu, Mar 05, 2020 at 01:45:18PM +0100, Philippe Mathieu-Daudé wrote: > > > > This buffer is only used by the adlib audio device. Move it to > > > > the .heap to release 32KiB of .bss (size reported on x86_64 host). > > > > > > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > > > --- > > > > hw/audio/fmopl.c | 4 +++- > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/hw/audio/fmopl.c b/hw/audio/fmopl.c > > > > index 173a7521f2..356d4dfbca 100644 > > > > --- a/hw/audio/fmopl.c > > > > +++ b/hw/audio/fmopl.c > > > > @@ -186,7 +186,7 @@ static int32_t *VIB_TABLE; > > > > /* envelope output curve table */ > > > > /* attack + decay + OFF */ > > > > -static int32_t ENV_CURVE[2*EG_ENT+1]; > > > > +static int32_t *ENV_CURVE; > > > > /* multiple table */ > > > > #define ML 2 > > > > @@ -1090,6 +1090,7 @@ FM_OPL *OPLCreate(int clock, int rate) > > > > OPL->clock = clock; > > > > OPL->rate = rate; > > > > OPL->max_ch = max_ch; > > > > + ENV_CURVE = g_new(int32_t, 2 * EG_ENT + 1); > > > > Should we use g_new0() ? > > No because the array is filled before being used. I can add a note about > this. > Thanks for the clarification! Yes, if you need to respin, maybe it's better. Thanks, Stefano
diff --git a/hw/audio/fmopl.c b/hw/audio/fmopl.c index 173a7521f2..356d4dfbca 100644 --- a/hw/audio/fmopl.c +++ b/hw/audio/fmopl.c @@ -186,7 +186,7 @@ static int32_t *VIB_TABLE; /* envelope output curve table */ /* attack + decay + OFF */ -static int32_t ENV_CURVE[2*EG_ENT+1]; +static int32_t *ENV_CURVE; /* multiple table */ #define ML 2 @@ -1090,6 +1090,7 @@ FM_OPL *OPLCreate(int clock, int rate) OPL->clock = clock; OPL->rate = rate; OPL->max_ch = max_ch; + ENV_CURVE = g_new(int32_t, 2 * EG_ENT + 1); /* init grobal tables */ OPL_initialize(OPL); /* reset chip */ @@ -1127,6 +1128,7 @@ void OPLDestroy(FM_OPL *OPL) #endif OPL_UnLockTable(); free(OPL); + g_free(ENV_CURVE); } /* ---------- Option handlers ---------- */
This buffer is only used by the adlib audio device. Move it to the .heap to release 32KiB of .bss (size reported on x86_64 host). Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- hw/audio/fmopl.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)