On Wed, 21 Oct 2015 16:03:07 +0200,
Russell King - ARM Linux wrote:
>
> On Wed, Oct 21, 2015 at 03:41:44PM +0200, Takashi Iwai wrote:
> > On Wed, 21 Oct 2015 11:27:44 +0200,
> > Russell King - ARM Linux wrote:
> > >
> > > On Tue, Oct 20, 2015 at 07:31:48PM +0530, Vinod Koul wrote:
> > > > On Tue, Oct 20, 2015 at 09:08:00AM +0100, Russell King - ARM Linux wrote:
> > > > > > > Currently i915/audio component works as you described. The audio is
> > > > > > > optional and HDMI graphics works without audio, while HDMI HD-audio
> > > > > > > mandates i915 graphics.
> > > > > >
> > > > > > Right, but we also add additional interface on top of this to allow
> > > > > > things like ensuring display is on when audio wants to run and now
> > > > > > notification for events.
> > > > > >
> > > > > > I don't see a reason why this can be used as single interface to bind as
> > > > > > well as talk to display from various components, unless I missed obvious
> > > > > > which prevent us from doing this in non i915 cases...
> > > > >
> > > > > Maybe I can comment more specifically if I saw the code. Right now all
> > > > > I'm aware of is this idea without any code, and I don't like it.
> > > >
> > > > Ok, i will be post my patches tomorrow. FWIW uses interface in
> > > > sound/hda/hdac_i915.c for display power up/down
> > >
> > > This:
> > >
> > > component_match_add(dev, &match, hdac_component_master_match, bus);
> > > ret = component_master_add_with_match(dev, &hdac_component_master_ops,
> > > match);
> > > if (ret < 0)
> > > goto out_err;
> > >
> > > /*
> > > * Atm, we don't support deferring the component binding, so make sure
> > > * i915 is loaded and that the binding successfully completes.
> > > */
> > > request_module("i915");
> > >
> > > if (!acomp->ops) {
> > > ret = -ENODEV;
> > > goto out_master_del;
> > > }
> > >
> > > is really not nice.
> >
> > Yeah, admittedly it's a really ugly workaround. It's coded in that
> > way just to make our lives easier: it works well in most cases for
> > i915 / hd-audio.
> >
> > Actually, it's easy to modify the hda code in the right way, i.e. to
> > defer via component bind ops. However, one drawback is that it's not
> > trivial to handle the fallback case; namely, HD-audio might not need
> > i915 binding, depending on the chip model and the configuration.
> >
> > Braswell and Skylake have a (virtually) single audio controller as
> > default, and this manages both the analog and HDMI/DP codecs. The
> > i915 interaction is required only for the latter codecs, and thus for
> > the former, it's optional. If we defer binding, the instantiation
> > won't happen unless it's bound with i915. So, if i915 isn't
> > initialized (e.g. by booting with nomodeset option), the whole audio
> > will be gone, too. OTOH, Haswell and Braswell have a dedicated HDA
> > controller for HDMI/DP, and they must be disabled (or fail) unless
> > bound with graphics.
> >
> > It's a corner case, so we might better ignore this. But it'd be
> > certainly a "regression" -- at least, I used to test this in that way
> > sometimes in the past. So it remains in the current way as is.
> >
> > It's one of the reasons I mentioned it being a stop gap. But, I think
> > the concept, passing ops via component, is actually working, and
> > should be applicable to other drm/audio drivers. That's the point.
>
> It's only the point if you can code it up properly, which from what I
> read in that file, it isn't.
An idea can fly without coding, too :)
> Build the i915 DRM drivers as a module, load up the system, and then
> try removing the i915 DRM module and see what happens to the audio part.
> For starters, you have no protection what so ever against acomp->ops or
> acomp->dev becoming NULL - it's hellishly racy.
Yes, very likely.
> Secondly, you reject the initialisation if acomp->ops isn't set, but you
> allow acomp->ops to later become unset by the i915 DRM module being
> removed. If you can cope with acomp->ops being unset at a random point
> during the audio driver's use, why can't you cope with it being set at
> some random point later?
Setting/unsetting on the fly would be picky because the code does
refcounting. Maybe an easier option is to inc/dec module usage
count appropriately in use.
> I don't think this code has been thought through at all.
True, more hardening needed.
thanks,
Takashi
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
