Re: [PATCH v2 08/12] drm: bridge/dw_hdmi: add audio config interfaces

[Yang Kuankuan]

On 02/02/2015 06:53 AM, Russell King - ARM Linux wrote:
> On Mon, Feb 02, 2015 at 12:02:50PM +0800, Daniel Kurtz wrote:
> > Hi ykk,
> >
> > On Sat, Jan 31, 2015 at 10:34 PM, Yang Kuankuan <ykk@xxxxxxxxxxxxxx> wrote:
> > > On 01/31/2015 06:48 AM, Russell King - ARM Linux wrote:
> > > > > +void hdmi_audio_clk_enable(struct dw_hdmi *hdmi)
> > > > > +{
> > > > > +       if (hdmi->audio_enable)
> > > > > +               return;
> > > > > +
> > > > > +       mutex_lock(&hdmi->audio_mutex);
> > > > > +       hdmi->audio_enable = true;
> > > > > +       hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE,
> > > > > HDMI_MC_CLKDIS);
> > > > > +       mutex_unlock(&hdmi->audio_mutex);
> > > > This is racy.  The test needs to be within the mutex-protected region.
> > > This function will be called by other driver (dw-hdmi-audio), both modify
> > > the variable "hdmi->audio_enable", so i add the mutex-protected.
> > >From your comment it isn't clear whether you understand what Russell meant.
> > He is say you should do the following:
> >
> > {
> >         mutex_lock(&hdmi->audio_mutex);
> >
> >         if (hdmi->audio_enable) {
> >                mutex_unlock(&hdmi->audio_mutex);
> >                return;
> >         }
> >         hdmi->audio_enable = true;
> >         hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS);
> >
> >         mutex_unlock(&hdmi->audio_mutex);
> > }
> Yes, however, I prefer this kind of layout:
>
> 	mutex_lock(&hdmi->audio_mutex);
>   
> 	if (!hdmi->audio_enable) {
> 		hdmi->audio_enable = true;
> 		hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE,
> 			  HDMI_MC_CLKDIS);
>   	}
>
> 	mutex_unlock(&hdmi->audio_mutex);
>
> but that's a matter of personal opinion.  The important thing is that the
> testing and setting of the flag are both within the protected region.
>
> However, there are other bugs here: what if the audio driver is calling
> the sample rate setting function at the same time that a mode switch is
> occuring.  We actually need a mutex to protect more than just the
> audio_enable flag.

Okay, got it.

Thanks.  : )
> > By the way, it doesn't matter that the function is called from another driver.
> > What matters is that this function can be called concurrently on
> > multiple different threads of execution to change the hdmi audio
> > enable state.
> > >From DRM land, it is called with DRM lock held when enabling/disabling
> > hdmi audio (mode_set / DPMS).
> > It is also called from audio land, when enabling/disabling audio in
> > response to some audio events (userspace ioctls?).  I'm not sure
> > exactly how the audio side works, or what locks are involved, but this
> > mutex synchronizes calls from these two worlds to ensure that
> > "hdmi->audio_enable" field always matches the current (intended)
> > status of the hdmi audio clock.  This would be useful, for example, if
> > you needed to temporarily disable all HDMI clocks during a mode set,
> > and then restore the audio clock to its pre-mode_set state:
> There's some rather quirky comments in the driver right now which make
> me uneasy about changing things too much.
>
> My initial idea would be that audio should remain disabled until the
> audio driver wants it enabled, and the CTS/N values should either be
> left alone, or set to a value which disables them (there is an iMX6
> errata which says to set N=0 initially, but as seems common with iMX6
> errata, I see no code implementing the method specified in the
> documentation - I have found code implementing something similar
> though.)

I am confused with the way that audio driver realize the display 
resolution-changing.
If audio driver cannot knowing that, then audio clock may be closed 
permanently ?

> However, there is this in the binding function:
>
>          /*
>           * To prevent overflows in HDMI_IH_FC_STAT2, set the clk regenerator
>           * N and cts values before enabling phy
>           */
>          hdmi_init_clk_regenerator(hdmi);
>
> which sets the N/CTS values assuming a 74.25MHz video clock and a 48kHz
> sample rate.  I've always wondered why this is necessary (I haven't
> experimented with that yet.)
>
> Then there's this in the mode set function:
>
>                  /* HDMI Initialization Step E - Configure audio */
>                  hdmi_clk_regenerator_update_pixel_clock(hdmi);
>                  hdmi_enable_audio_clk(hdmi);
>
> Where these "steps" come from, I've no idea (I can't find any documentation
> which specifies them - maybe its from the Synopsis documentation?) but
> this has always raised the question "what if audio is not enabled?"
static void hdmi_keep_audio_clk_status(struct dw_hdmi *hdmi)
{
    mutex_lock(&hdmi->audio_mutex);

    if (hdmi->audio_enable)
        hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE,
                            HDMI_MC_CLKDIS);
    else
        hdmi_modb(hdmi, HDMI_MC_CLKDIS_AUDCLK_DISABLE,
                            HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS);

    mutex_unlock(&hdmi->audio_mutex);
}

     /* HDMI Initialization Step E - Configure audio */
    hdmi_clk_regenerator_update_pixel_clock(hdmi);
    hdmi_keep_audio_clk_status(hdmi);

keep audio status maybe suitable here.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/