Re: [PATCH 2/7] drm/i915: Add get_eld audio component

From: Takashi Iwai <tiwai@suse.de>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: alsa-devel@alsa-project.org,
	Mengdong Lin <mengdong.lin@linux.intel.com>,
	Vinod Koul <vinod.koul@intel.com>,
	intel-gfx@lists.freedesktop.org,
	Daniel Vetter <daniel.vetter@intel.com>,
	David Henningsson <david.henningsson@canonical.com>
Subject: Re: [PATCH 2/7] drm/i915: Add get_eld audio component
Date: Mon, 30 Nov 2015 17:53:24 +0100	[thread overview]
Message-ID: <s5ha8pvqvi3.wl-tiwai@suse.de> (raw)
In-Reply-To: <20151130160933.GT4437@intel.com>

On Mon, 30 Nov 2015 17:09:33 +0100,
Ville Syrjälä wrote:
> 
> On Mon, Nov 30, 2015 at 05:24:41PM +0200, Ville Syrjälä wrote:
> > On Mon, Nov 30, 2015 at 02:37:46PM +0100, Takashi Iwai wrote:
> > > Implement a new i915_audio_component_ops, get_eld().  It's called by
> > > the audio driver to fetch the current ELD of the given HDMI/DP port.
> > > It returns the size of ELD bytes if it's valid, or zero if the audio
> > > is disabled or unplugged, or a negative error code.
> > 
> > Why do we need this? Isn't it something the eld notify hook should
> > pass from i915 to the audio driver?
> > 
> > At least with the locking you have for this, the audio driver can not
> > call this from the eld notify hook since it would deadlock.
> 
> Hmm. Actually the locking isn't perhaps quite like that atm. But I guess
> the mode_config.mutex will make it so.
> 
> Apart from that it seesm to me that you should pull the av_mutex
> lock/unlock from the .audio_code_eanble/disable hooks into
> intel_audio_codec_enable/disable, so that it protects the audio_enabled
> flag as well. Not sure if the eld_notify should be called while holding
> that lock or not. If we need to avoid calling it from the eld_notify
> anywya due to other locks then maybe it can be under av_mutex as well.

Currently I'm thinking of:

- not allow to call get_eld directly from eld_notify;
  I found that the existing eld_repoll work in the HDA can be reused
  easily, so let's follow Daniel's advice.

- drm_select_eld() seems requring mode_config.mutex and
  connection_mutex modeset lock in anyway; so let get_eld taking
  both.

  Is it OK to call drm_modeset_lock_all() for simplicity?

- Get rid of av_mutex from get_eld instead;
  get_eld doesn't conflict with other ops

In that way, audio_enabled flag is protected in both places via
modeset lock, I suppose.

thanks,

Takashi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx