[Intel-gfx] [PATCH 08/24] drm/i915: Do not add planes from intel_atomic_setup_scalers.
Daniel Vetter
daniel at ffwll.ch
Mon Jun 15 04:48:04 PDT 2015
On Thu, Jun 04, 2015 at 05:39:52AM +0200, Maarten Lankhorst wrote:
> Hey,
>
> Op 04-06-15 om 01:33 schreef Matt Roper:
> > On Wed, Jun 03, 2015 at 12:32:43PM -0700, Konduru, Chandra wrote:
> >>
> >>> -----Original Message-----
> >>> From: Maarten Lankhorst [mailto:maarten.lankhorst at linux.intel.com]
> >>> Sent: Wednesday, June 03, 2015 12:02 AM
> >>> 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 03-06-15 om 03:52 schreef Konduru, Chandra:
> >>>>> -----Original Message-----
> >>>>> From: Roper, Matthew D
> >>>>> Sent: Tuesday, June 02, 2015 6:30 PM
> >>>>> To: Maarten Lankhorst
> >>>>> Cc: intel-gfx at lists.freedesktop.org; Konduru, Chandra
> >>>>> Subject: Re: [Intel-gfx] [PATCH 08/24] drm/i915: Do not add planes from
> >>>>> intel_atomic_setup_scalers.
> >>>>>
> >>>>> On Mon, Jun 01, 2015 at 03:27:11PM +0200, Maarten Lankhorst wrote:
> >>>>>> This may postpone going to HQ mode until the plane is in the
> >>>>>> drm_atomic_state if it's not using scaler 0, but it does allow moving
> >>>>>> intel_atomic_setup_scalers to the crtc check function.
> >>>>>>
> >>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> >>>>>> ---
> >>>>>> drivers/gpu/drm/i915/intel_atomic.c | 41 ++++++++++++++++++-------------
> >>> ---
> >>>>> --
> >>>>>> drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++++++---------
> >>>>>> drivers/gpu/drm/i915/intel_drv.h | 2 ++
> >>>>>> 3 files changed, 39 insertions(+), 31 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> >>>>> b/drivers/gpu/drm/i915/intel_atomic.c
> >>>>>> index 1edd1651c045..a8202fa0daa8 100644
> >>>>>> --- a/drivers/gpu/drm/i915/intel_atomic.c
> >>>>>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> >>>>>> @@ -100,14 +100,6 @@ int intel_atomic_check(struct drm_device *dev,
> >>>>>> if (ret)
> >>>>>> return ret;
> >>>>>>
> >>>>>> - /*
> >>>>>> - * FIXME: move to crtc atomic check function once this is
> >>>>>> - * more atomic friendly.
> >>>>>> - */
> >>>>>> - ret = intel_atomic_setup_scalers(dev, nuclear_crtc, crtc_state);
> >>>>>> - if (ret)
> >>>>>> - return ret;
> >>>>>> -
> >>>>>> return ret;
> >>>>>> }
> >>>>>>
> >>>>>> @@ -336,21 +328,10 @@ int intel_atomic_setup_scalers(struct drm_device
> >>>>> *dev,
> >>>>>> /* find the plane that set the bit as scaler_user */
> >>>>>> plane = drm_state->planes[i];
> >>>>>>
> >>>>>> - /*
> >>>>>> - * to enable/disable hq mode, add planes that are using
> >>>>> scaler
> >>>>>> - * into this transaction
> >>>>>> - */
> >>>>>> if (!plane) {
> >>>>>> - struct drm_plane_state *state;
> >>>>>> - plane = drm_plane_from_index(dev, i);
> >>>>>> - state =
> >>>>> drm_atomic_get_plane_state(drm_state, plane);
> >>>>>> - if (IS_ERR(state)) {
> >>>>>> - DRM_DEBUG_KMS("Failed to add
> >>>>> [PLANE:%d] to drm_state\n",
> >>>>>> - plane->base.id);
> >>>>>> - return PTR_ERR(state);
> >>>>>> - }
> >>>>>> + DRM_DEBUG_KMS("Failed to find [PLANE:%d]
> >>>>> in drm_state\n", plane->base.id);
> >>>>>> + continue;
> >>>>>> }
> >>>>>> -
> >>>>>> intel_plane = to_intel_plane(plane);
> >>>>>>
> >>>>>> /* plane on different crtc cannot be a scaler user of this
> >>>>> crtc */
> >>>>>> @@ -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. :-)
Just an aside: Bending over backwards for wait_for_flips is imo pointless
since we'll ditch all that code anyway. Well as soon as we've moved
pageflips over to atomic at least. So a bit of temporary hacks should be
fine here.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the Intel-gfx
mailing list