> -----Original Message----- > From: Maarten Lankhorst [mailto:maarten.lankhorst@xxxxxxxxxxxxxxx] > Sent: Friday, June 05, 2015 11:39 PM > 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 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. _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
