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@xxxxxxxxxxxxxxx] > >>> Sent: Wednesday, June 03, 2015 12:02 AM > >>> To: Konduru, Chandra; Roper, Matthew D > >>> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx > >>> Subject: Re: [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@xxxxxxxxxxxxxxxxxxxxx; Konduru, Chandra > >>>>> Subject: Re: [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@xxxxxxxxxxxxxxx> > >>>>>> --- > >>>>>> 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
