[Intel-gfx] [PATCH 08/24] drm/i915: Do not add planes from intel_atomic_setup_scalers.

[Konduru, Chandra]

> -----Original Message-----
> From: Maarten Lankhorst [mailto:maarten.lankhorst at linux.intel.com]
> Sent: Friday, June 05, 2015 11:39 PM
> To: Konduru, Chandra; Roper, Matthew D
> Cc: intel-gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 08/24] drm/i915: Do not add planes from
> intel_atomic_setup_scalers.
> 
> Op 05-06-15 om 21:05 schreef Konduru, Chandra:
> >>>>>>>> @@ -396,6 +377,24 @@ int intel_atomic_setup_scalers(struct
> >> drm_device
> >>>>>>> *dev,
> >>>>>>>>  		}
> >>>>>>>>  	}
> >>>>>>>>
> >>>>>>>> +	/* plane not part of mask must leave hq mode? */
> >>>>>>>> +	if (num_scalers_need > 1 && scaler_state->scalers[0].in_use &&
> >>>>>>>> +	    scaler_state->scalers[0].mode == PS_SCALER_MODE_HQ) {
> >>>>>>>> +		scaler_state->scalers[0].mode =
> >> PS_SCALER_MODE_DYN;
> >>>>>>>> +
> >>>>>>>> +		intel_crtc->atomic.skl_update_scaler0 =
> >>>>>>>> +			PS_SCALER_EN | PS_SCALER_MODE_DYN;
> >>>>>>>> +	}
> >>>>>>>> +
> >>>>>>>> +	/* plane not part of mask can enter hq mode? */
> >>>>>>>> +	if (num_scalers_need == 1 && scaler_state->scalers[0].in_use
> >> &&
> >>>>>>>> +	    intel_crtc->pipe != PIPE_C && scaler_state->scalers[0].mode
> >> !=
> >>>>>>> PS_SCALER_MODE_HQ) {
> >>>>>>>> +		scaler_state->scalers[0].mode =
> >> PS_SCALER_MODE_HQ;
> >>>>>>>> +
> >>>>>>>> +		intel_crtc->atomic.skl_update_scaler0 =
> >>>>>>>> +			PS_SCALER_EN | PS_SCALER_MODE_HQ;
> >>>>>>>> +	}
> >>>>>>>> +
> >>>>>>> I don't have access to the hw spec at the moment; is scaler #0 the only
> >>>>>>> one that can ever go into HQ mode?
> >>>>>> Yes
> >>>>>>
> >>>>>>> If there isn't a hardware
> >>>>>>> requirement about this, then it seems like we're missing the case where
> >>>>>>> planes A and B get scalers 0 and 1.  Then plane A (and thus scaler 0) is
> >>>>>>> disabled, which should allow scaler 1 to go into HQ mode.
> >>>>>> In this case, scaler 0 to be allocated to plane B to operate in HQ mode.
> >>>>> Is it really bad to keep it on scaler 1 for a while until the next time the
> plane
> >> is
> >>>>> added?
> >>>>>
> >>>>>>> I guess it's not immediately clear to me why we need to not pull the
> >>>>>>> other planes into the transaction.  Is this just to avoid doing some
> >>>>>>> extra work for a plane that hasn't changed, or does it cause a problem
> >>>>>>> somehow?
> >>>>>> Per atomic design, unchanged planes can be added to transaction.
> >>>>>> And scaler implementation is using this design feature.
> >>>>>> Not sure what the issue here, but we need this feature continue
> >>>>>> to available.
> >>>>>>
> >>>>> Unchanged planes can be added, but this could pull in a primary plane,
> >> which
> >>>>> would need
> >>>>> to set atomic.wait_for_flips then. I can do that as special case when
> adding
> >> a
> >>>>> plane if
> >>>>> that's preferred.
> >>>> Here primary plane can get added if that is the only plane using scaler
> which
> >>>> isn't part of the transaction. But here addition of primary plane isn't adding
> >>>> or changing its FB. So why it needs to set atomic.wait_for_flips?
> >>> In the atomic case, intel_check_primary_plane() sets
> >>> atomic.wait_for_flips if the primary plane is part of the original
> >>> transaction (no matter what about the primary plane is changing).
> >>> However if we pull in the primary plane via
> >>> intel_atomic_setup_scalers(), that gets called after we've finished
> >>> checking all of the planes (that were originally part of the
> >>> transaction), so I don't think wait_for_flips will get set in that case.
> >>> So I think pulling in the plane as Chandra was will still avoid an
> >>> unnecessary wait.  Even though it's in the transaction, I believe the
> >>> 'check' step is bypassed (which means we need to be careful about doing
> >>> stuff like this if it could affect derived state...but I think in this
> >>> case it's safe).
> >>>
> >>> N.B.  The wait we're talking about here will only be triggered when a
> >>> SKL or BXT platform disables a "sprite" plane (thus triggering the
> >>> primary's scaler to switch to HQ mode) while there's a pending legacy
> >>> pageflip on the primary plane.  I don't think we have a userspace today
> >>> that can trigger this...Weston won't use sprites at all in a non-atomic
> >>> manner and although SNA has an Xv sprite adapter, it won't ever be
> >>> performing scaling of the primary plane afaik.
> >> Correct. The wait will be a nop if there are no legacy page flips. But for
> >> completeness sake I included it. :-)
> > So you will be keeping current code to add plane as-is and let respective
> > update_plane function take care of updating scaler quality instead
> > of the above update_scaler0?
> >
> Yes, see "[PATCH v2 08/27] drm/i915: Move scaler setup to check crtc function,
> v2."
OK. Please run kms_plane_scaling and kms_panel_fitting 
visual tests to make sure they are passing.