Hi,
On Friday, April 5, 2013, Takashi Iwai <tiwai@xxxxxxx> wrote:
> At Fri, 5 Apr 2013 19:02:08 +0530,
> mateen wrote:
>>
>> Hi,
>> I checked with Takshi's patch and would like to suggest couple of changes.
>>
>> 1. We keep dmix_down_sem(dmix) and dmix_up_sem(dmix) as macros instead of
>> defining them as functions.
>
> No, no. That's a very bad practice. Don't do it. If any, use static
> inline.
>
Agreed.
> But in this case, there is no merit to do inline. Let compiler
> optimize.
>> 2. We free up the semaphore in snd_pcm_dmix_close() if it is recognized
>> that the semaphore is held up by the same process, which will be indicated
>> by dmix->semlocked flag.
>
> Why do you need reacquire the very same lock at all...?
> I see no point for it.
>
Agree,
That lock is still with the same process.we need not reacquire it.
> Takashi
>
>>
>> Mateen.
>>
>> diff -Nuar dir2/pcm_direct.h dir1/pcm_direct.h
>> --- dir2/pcm_direct.h 2009-12-16 20:48:51.000000000 +0530
>> +++ dir1/pcm_direct.h 2013-04-05 17:06:48.331497000 +0530
>> @@ -123,6 +123,7 @@
>> int ipc_gid; /* IPC socket gid */
>> int semid; /* IPC global semaphore identification */
>> int shmid; /* IPC global shared memory identification */
>> + int semlocked;
>> snd_pcm_direct_share_t *shmptr; /* pointer to shared memory area */
>> snd_pcm_t *spcm; /* slave PCM handle */
>> snd_pcm_uframes_t appl_ptr;
>> diff -Nuar dir2/pcm_dmix.c dir1/pcm_dmix.c
>> --- dir2/pcm_dmix.c 2009-12-16 20:48:51.000000000 +0530
>> +++ dir1/pcm_dmix.c 2013-04-05 17:04:02.781109000 +0530
>> @@ -285,8 +285,17 @@
>> */
>> #ifndef DOC_HIDDEN
>> #ifdef NO_CONCURRENT_ACCESS
>> -#define dmix_down_sem(dmix) snd_pcm_direct_semaphore_down(dmix,
>> DIRECT_IPC_SEM_CLIENT)
>> -#define dmix_up_sem(dmix) snd_pcm_direct_semaphore_up(dmix,
>> DIRECT_IPC_SEM_CLIENT)
>> +#define dmix_down_sem(dmix) \
>> +{\
>> +if (!dmix->semlocked++)\
>> + snd_pcm_direct_semaphore_down(dmix, DIRECT_IPC_SEM_CLIENT);\
>> +}
>> +#define dmix_up_sem(dmix) \
>> +{\
>> + if (!--dmix->semlocked)\
>> + snd_pcm_direct_semaphore_up(dmix, DIRECT_IPC_SEM_CLIENT);\
>> +}
>> +
>> #else
>> #define dmix_down_sem(dmix)
>> #define dmix_up_sem(dmix)
>> @@ -764,7 +773,13 @@
>>
>> if (dmix->timer)
>> snd_timer_close(dmix->timer);
>> - snd_pcm_direct_semaphore_down(dmix, DIRECT_IPC_SEM_CLIENT);
>> + if(!dmix->semlocked)
>> + snd_pcm_direct_semaphore_down(dmix, DIRECT_IPC_SEM_CLIENT);
>> + else{
>> + snd_pcm_direct_semaphore_up(dmix, DIRECT_IPC_SEM_CLIENT);
>> + snd_pcm_direct_semaphore_down(dmix, DIRECT_IPC_SEM_CLIENT);
>> + }
>> +
>> snd_pcm_close(dmix->spcm);
>> if (dmix->server)
>> snd_pcm_direct_server_discard(dmix);
>>
>>
>>
>>
>> ------------------------------
>> ----------------------------------------
>>
>> Message: 1
>> Date: Thu, 04 Apr 2013 18:30:01 +0200
>> From: Takashi Iwai <tiwai@xxxxxxx>
>> To: Jaroslav Kysela <perex@xxxxxxxx>
>> Cc: alsa-devel@xxxxxxxxxxxxxxxx
>> Subject: Re: Deadlock over semaphore issue with aplay
>> while using dmix
>> Message-ID: <s5h38v645ti.wl%tiwai@xxxxxxx>
>> Content-Type: text/plain; charset=US-ASCII
>>
>> At Thu, 04 Apr 2013 13:33:08 +0200,
>> Jaroslav Kysela wrote:
>> >
>> > Date 4.4.2013 11:27, mateen wrote:
>> > > Hi,
>> > >
>> > > I seeing sometimes deadlock issue with dmix when I press CTRL+C.
>> > >
>> > > Aplay's signal handler calls snd_pcm_close() if an interrupt occurs.
>> > > snd_pcm_close() will internally call pcm->ops->close() which will fall
>> to
>> > > snd_pcm_dmix_close() in case you are using dmix.
>> > >
>> > > snd_pcm_dmix_close() will try to acquire the semaphore with
>> > > snd_pcm_direct_semaphore_down(dmix, DIRECT_IPC_SEM_CLIENT).
>> > > The same semaphore is acquired in snd_pcm_dmix_sync_area() with
>> > > dmix_down_sem() in case of non-concurrent access.
>> > >
>> > > If semaphore is acquired in sn> [2 <text/html; ISO-8859-1 (quoted-printable)>]
>>
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
