Re: [PATCH 4/6] drm/i915: check for audio and infoframe changes across mode sets v2

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 01 Dec 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Dec 01, 2014 at 04:37:41PM +0000, Chris Wilson wrote:
>> On Mon, Dec 01, 2014 at 05:30:41PM +0100, Daniel Vetter wrote:
>> > On Mon, Dec 01, 2014 at 04:09:07PM +0000, Chris Wilson wrote:
>> > > On Mon, Dec 01, 2014 at 08:04:30AM -0800, Jesse Barnes wrote:
>> > > > On Mon, 01 Dec 2014 12:25:45 +0200
>> > > > Jani Nikula <jani.nikula@linux.intel.com> wrote:
>> > > > 
>> > > > > On Thu, 06 Nov 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>> > > > > > If these change (e.g. after a modeset following a fastboot), we need to
>> > > > > > do a full mode set.
>> > > > > >
>> > > > > > v2:
>> > > > > >   - put under pipe_config check so we don't deref a null state (Jesse)
>> > > > > >
>> > > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>> > > > > > ---
>> > > > > >  drivers/gpu/drm/i915/intel_display.c | 12 +++++++++++-
>> > > > > >  1 file changed, 11 insertions(+), 1 deletion(-)
>> > > > > >
>> > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> > > > > > index cb96f11..c86eee6 100644
>> > > > > > --- a/drivers/gpu/drm/i915/intel_display.c
>> > > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
>> > > > > > @@ -11230,8 +11230,18 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>> > > > > >  						   &modeset_pipes,
>> > > > > >  						   &prepare_pipes,
>> > > > > >  						   &disable_pipes);
>> > > > > > -	if (IS_ERR(pipe_config))
>> > > > > > +	if (IS_ERR(pipe_config)) {
>> > > > > >  		goto fail;
>> > > > > > +	} else if (pipe_config) {
>> > > > > > +		if (to_intel_crtc(set->crtc)->new_config->has_audio !=
>> > > > > > +		    to_intel_crtc(set->crtc)->config.has_audio)
>> > > > > > +			config->mode_changed = true;
>> > > > > > +
>> > > > > > +		/* Force mode sets for any infoframe stuff */
>> > > > > > +		if (to_intel_crtc(set->crtc)->new_config->has_infoframe ||
>> > > > > > +		    to_intel_crtc(set->crtc)->config.has_infoframe)
>> > > > > 
>> > > > > Jesse, is "||" correct here? This is bound to force a lot of mode sets.
>> > > > > 
>> > > > > See https://bugs.freedesktop.org/show_bug.cgi?id=86683
>> > > > 
>> > > > Yeah, we talked about it on IRC.  I was being extra conservative here,
>> > > > since we don't yet have code to analyze whether we need a full mode set
>> > > > due to something that will require an infoframe change.  Currently, the
>> > > > only way for us to write a new infoframe is through a mode set, and the
>> > > > old code would just happen to do a mode set most of the time we wanted
>> > > > it to (i.e. when the mode timings changed), but I don't think we'd ever
>> > > > thought of the implications of infoframe changes for stuff that doesn't
>> > > > require a mode set.
>> > > > 
>> > > > We could drop this hunk and continue to play it fast & loose, but what
>> > > > we really need here is a fuller check about whether we need a mode set
>> > > > due to requiring an infoframe change, which means pulling them apart a
>> > > > bit.
>> > > 
>> > > We need to be conservative. If we spot extraneous modesets in the wild,
>> > > we will be motivated to fix it. As a compromise, if you include a
>> > > 
>> > > DRM_DEBUG_KMS("infoframes detected - forcing full modeset\n")
>> > > 
>> > > that should be sufficient clue for us to pick up the thread again later.
>> > 
>> > The problem is that with the current code we'll do a modeset for all hdmi
>> > outputs, unconditionally. Even without fastboot=1 set. Which is too much
>> > and a regression. E.g. just having virtual > screen and scrolling with the
>> > mouse old-style would now result in a modeset each time we move one pixel.
>> > Or at least that's my analysis here (didnt test).
>> 
>> Would it be just as easy to construct a scenario that had an infroframe
>> change that didn't get applied with the revert? Besides which a full
>> modeset on every pan should be good motiviation to make them nonblocking
>> (or at least use vblank workers to do the clenaup) ;-)
>
> Without fastboot we still force a full modeset at boot-up (that part
> hasn't been cleanup up yet and brought out from under the fastboot
> switch). So for users that don't set special options reverting this won't
> cause any more modesets.

The bug doesn't mention any special options.

BR,
Jani.


> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx