Re: [PATCH v3 04/11] drm/i915: Handle adjust better in intel_pipe_config_compare

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Nov 20, 2017 at 11:38:49AM +0100, Daniel Vetter wrote:
> On Mon, Nov 13, 2017 at 07:24:22PM +0200, Ville Syrjälä wrote:
> > On Fri, Nov 10, 2017 at 02:02:45PM +0100, Daniel Vetter wrote:
> > > On Fri, Nov 10, 2017 at 12:34:56PM +0100, Maarten Lankhorst wrote:
> > > > Some parameters use CHECK_BOOLL, but should really use
> > > > CHECK_BOOL_INCOMPLETE. We cannot currently check whether
> > > > the inherited infoframes and audio are set up correctly.
> > > > 
> > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_display.c | 19 +++++++++++++++++--
> > > >  1 file changed, 17 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > index 425167da560b..f5933b0719c9 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -11076,6 +11076,9 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
> > > >  			  bool adjust)
> > > >  {
> > > >  	bool ret = true;
> > > > +	bool fixup_inherited = adjust &&
> > > > +		(current_config->base.mode.private_flags & I915_MODE_FLAG_INHERITED) &&
> > > > +		!(pipe_config->base.mode.private_flags & I915_MODE_FLAG_INHERITED);
> > > >  
> > > >  #define PIPE_CONF_CHECK_X(name)	\
> > > >  	if (current_config->name != pipe_config->name) { \
> > > > @@ -11104,6 +11107,17 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
> > > >  		ret = false; \
> > > >  	}
> > > >  
> > > 
> > > Maybe add a comment here like
> > > 
> > > /*
> > >  * Checks state where we only read out the enabling, but not the entire
> > >  * state itself (like full infoframes or ELD for audio). These states
> > >  * require a full modeset on bootup to fix up.
> > 
> > has_audio shouldn't need the ELD. If audio is enabled then it's enabled,
> > otherwise it's not.
> > 
> > So I guess if the BIOS doesn't enable audio, then what we really need is
> > a way to enable it without a full modeset. And I can't immediately
> > recall if we can do that or not.
> 
> Afaiui the audio enable bit fires off the irq on the snd driver, which
> then reads the eld to figure out how to set up the audio side.

You're thinking about the ELD valid bit, which is totally different. And
I don't think we use that (on modern platforms at least) ever since we
added these eld notify hooks and whatnot. Well, we still frob the bit but
the audio driver no longer uses the corresponding interrupt, I think.

> 
> So we indeed need the eld to be correct. We might also need more stuff
> (maybe audio only reads eld when the irq fires, I didn't check).
>  
> > I'm worried that ignoring the issue will just lead to piles of bugs
> > where people complain that their audio doesn't work.
> 
> I'm not blocking your patches, the r-b was included ... what do you mean?
> I'm all for fixing this ...

I have no idea what you're saying here. What patches?

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx