Re: [Intel-gfx] [PATCH 04/31] drm/i915: Handle actual IPS enabled state.

[Ville Syrjälä]

On Fri, Nov 13, 2015 at 06:20:00PM +, Daniel Stone wrote:
> Hi Rodrigo,
> 
> On 5 November 2015 at 18:49, Rodrigo Vivi  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.

Long time ago I posted a patch to make ips enable asynchronously from
a vblank work.

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 04/31] drm/i915: Handle actual IPS enabled state.

[Daniel Stone]

Hi Rodrigo,

On 5 November 2015 at 18:49, Rodrigo Vivi  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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 04/31] drm/i915: Handle actual IPS enabled state.

[Daniel Stone]

Hi,

On 13 November 2015 at 18:38, Ville Syrjälä
 wrote:
> On Fri, Nov 13, 2015 at 06:20:00PM +, Daniel Stone wrote:
>> 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.
>
> Long time ago I posted a patch to make ips enable asynchronously from
> a vblank work.

Too long ago for my mail client, it seems ... got a link handy?

Cheers,
Daniel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 04/31] drm/i915: Handle actual IPS enabled state.

[Daniel Stone]

On 13 November 2015 at 20:28, Ville Syrjälä
 wrote:
> On Fri, Nov 13, 2015 at 06:55:18PM +, Daniel Stone wrote:
>> On 13 November 2015 at 18:38, Ville Syrjälä
>>  wrote:
>> > On Fri, Nov 13, 2015 at 06:20:00PM +, Daniel Stone wrote:
>> >> 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.
>> >
>> > Long time ago I posted a patch to make ips enable asynchronously from
>> > a vblank work.
>>
>> Too long ago for my mail client, it seems ... got a link handy?
>
> http://lists.freedesktop.org/archives/intel-gfx/2014-June/047626.html

+struct intel_vblank_notify {

Oh dear ...

Cheers,
Daniel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 04/31] drm/i915: Handle actual IPS enabled state.

[Ville Syrjälä]

On Fri, Nov 13, 2015 at 06:55:18PM +, Daniel Stone wrote:
> Hi,
> 
> On 13 November 2015 at 18:38, Ville Syrjälä
>  wrote:
> > On Fri, Nov 13, 2015 at 06:20:00PM +, Daniel Stone wrote:
> >> 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.
> >
> > Long time ago I posted a patch to make ips enable asynchronously from
> > a vblank work.
> 
> Too long ago for my mail client, it seems ... got a link handy?

http://lists.freedesktop.org/archives/intel-gfx/2014-June/047626.html

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 04/31] drm/i915: Handle actual IPS enabled state.

[Daniel Stone]

Hi,

On 5 November 2015 at 18:49, Rodrigo Vivi  wrote:
> +void intel_ips_init(struct drm_i915_private dev_priv)

That would be *dev_priv.

Cheers,
Daniel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx