Hi Rodrigo,
On 5 November 2015 at 18:49, Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> wrote:
> With this we know if IPS is actually enabled.
> It might not be activated on BDW since Hardware take
> the decision and do its transition. However we have
> the visibility of the state on our driver what we didn't
> had until this patch. At least on BDW.
>
> Since ips_ready means that ips will be enabled and ips_disable()
> checks for the state of our enabled/disabled state machine
> we can remove that FIXME that was there for crtc_load_lut
> workaround for Haswell.
>
> With this state machine and ips being disabled from
> different places and many times when testcases with sink_crtc
> for instance it is better to have it protected with its own mutex lock.
> Ohterwise we cannot guarantee consitent ips.enabled state with the
> register bit.
Thinking about this, I have two comments (and second Chris's
similar-ish comment about PSR):
Should this perhaps be a CRTC property rather than a device property?
Is it tied to a particular pipe, or can it move away from pipe A?
Secondly, having a vblank wait to enable IPS is pretty unfortunate, as
it makes modesets take longer. I like the PSR enable being split away
from the modeset sequence, so perhaps we could do something like:
enum ips_state {
IPS_DISABLED = 0, /**< unsupported or explicitly disabled by module param */
IPS_READY, /**< IPS can be enabled if a suitable state is applied to
the CRTC (planes enabled, cdclk not exceeding 95% on BDW) */
IPS_ARMED, /**< suitable configuration applied; IPS pending activation */
IPS_ACTIVE
};
Having this on the CRTC state means that we could walk through the
following process:
- IPS_READY set on suitable platforms when not explictly disabled
- modeset arrives: atomic_check examines conditions and changes
state from IPS_READY to IPS_ARMED
- next pageflip arrives and changes state from IPS_ARMED to
IPS_ACTIVE, activates IPS
Being a member of the CRTC state means that anyone duplicating the
pipe's CRTC state could discover the IPS status like that, and
eliminates the need for a second mutex.
Maybe that's not the best approach, but I think we need to find a way
to take the synchronous vblank wait out of the modeset path. Using a
workqueue is another option, but synchronisation would need to be
quite carefully handled.
Cheers,
Daniel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
