[PATCH] [inline assembly] fix pcm_dmix_i386.h assembly chunk interfaces

[FRÉDÉRIC RECOULES]

Hi Takashi, 

Thanks for your comments. 
I addressed the style issues and I resubmitted the patches (V3). 

Note about the MMX patch: 
The mm1 register is read by the chunk, but its value is not used 
(the instruction packssdw put the value of mm1 in the high 32 bits of mm0 
but then, only the low 32 bits are used. My tool made an over-approximation 
but it is now fixed). 

By the way, the first 3 patches produce the same binary output as master. 
However, I looked in the 'test' folder, but I do not know how to run a test for 
the pcm. For now, I have no time to investigate how alsa should be run till the 
next week. 

> But there were some issues with the patch format. IIRC, the 
> patch 2 couldn't be applied to the latest git tree cleanly (some space 
> etter issues?), so I had to manually modify it. 

Sorry for that, it looks like my text editor remove space at the end of the line 
automatically. It should not be the case with the new patches. 

Regards, 
Frédéric 


De: "tiwai" <tiwai at suse.de> 
À: "FRÉDÉRIC RECOULES" <frederic.recoules at univ-grenoble-alpes.fr> 
Cc: "alsa-devel" <alsa-devel at alsa-project.org>, "frederic recoules" <frederic.recoules at orange.fr> 
Envoyé: Mardi 5 Mai 2020 15:52:19 
Objet: Re: [PATCH] [inline assembly] fix pcm_dmix_i386.h assembly chunk interfaces 

On Mon, 04 May 2020 21:25:16 +0200, 
FRÉDÉRIC RECOULES wrote: 
> 
> Hi Takashi, 
> 
> I would like an update on the review process ([PATCH */6 V2] [pcm_dmix 
> assembly]) 
> 
> As a reminder, I split the changes in 6 distinct patches. 
> The 3 first patches produce exactly the same binary output, so they do not 
> need testing. 
> The 4th one has just some minor change due to the fact that I added an 
> instruction -- I am highly confident it breaks nothing. 

The compile tests passed with a few different compiler set, so that's 
good. But there were some issues with the patch format. IIRC, the 
patch 2 couldn't be applied to the latest git tree cleanly (some space 
letter issues?), so I had to manually modify it. 

Also, some style issues: 

- Please avoid a prefix like "[configure]" in the subject. 
The prefix with "[PATCH xxx]" is good, and this should remain, but 
the next prefix should be "configure:" instead. Otherwise the 
prefix with the brackets are pruned at applying a patch via git-am. 

- Please give more texts about why the change is done. 
In all your patches, there are no explanations why you change it. 
It's often more important than describing what you're changing. 
For example, the patch 2 "change the token by symbolic names". Why 
is this needed to be symbolic names? Write some more information in 
each patch description. 

- We usually use #ifdef without space between "#" and "ifdef". 
Let's keep that style consistently. 

> If you need I test the 2 last ones (that reduce the size of the produced 
> binary), could you point me out what test I should run? 

We need at least some build tests with different compiler versions and 
check whether dmix actually works (not necessarily on all of them but 
some of those compiled results). 

> Meanwhile, my deadline comes and I would really appreciate to see the patches 
> applied by wednesday night. 

If you can work on the above and resubmit v3 patchset, I'll happily 
apply them. 


Thanks! 

Takashi