On Wed, Nov 18, 2015 at 06:39:45PM +0000, Vivi, Rodrigo wrote:
> On Wed, 2015-11-18 at 11:12 +0100, Daniel Vetter wrote:
> > On Thu, Nov 05, 2015 at 10:50:04AM -0800, Rodrigo Vivi wrote:
> > > PSR is still disabled by default, but even passing
> > > i915.enable_psr=1
> > > at this point we weren't able to get PSR working because with
> > > fastboot by default in place we weren't executing the path that
> > > enables
> > > encoder and consequently PSR.
> > >
> > > Now with psr_ready in place and PSR using crtc signature we can
> > > move
> > > its enable/disable sequences from the encoder enable to the post
> > > atomic modeset functions.
> > >
> > > i915.enable_psr parameter is still used to enable/disable psr
> > > feature
> > > on the next primary plane update. So current test cases that relies
> > > on this flow still works.
> > >
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/intel_ddi.c | 2 --
> > > drivers/gpu/drm/i915/intel_display.c | 15 +++++++++++++++
> > > drivers/gpu/drm/i915/intel_dp.c | 5 -----
> > > drivers/gpu/drm/i915/intel_drv.h | 2 ++
> > > 4 files changed, 17 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > > b/drivers/gpu/drm/i915/intel_ddi.c
> > > index b8f8dee..36db970 100644
> > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > @@ -2404,7 +2404,6 @@ static void intel_enable_ddi(struct
> > > intel_encoder *intel_encoder)
> > > intel_dp_stop_link_train(intel_dp);
> > >
> > > intel_edp_backlight_on(intel_dp);
> > > - intel_psr_enable(intel_crtc);
> > > intel_edp_drrs_enable(intel_dp);
> > > }
> > >
> > > @@ -2432,7 +2431,6 @@ static void intel_disable_ddi(struct
> > > intel_encoder *intel_encoder)
> > > struct intel_dp *intel_dp =
> > > enc_to_intel_dp(encoder);
> > >
> > > intel_edp_drrs_disable(intel_dp);
> > > - intel_psr_disable(intel_crtc);
> > > intel_edp_backlight_off(intel_dp);
> > > }
> > > }
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index 869929d..f67e2ee 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -4687,6 +4687,9 @@ static void intel_post_plane_update(struct
> > > intel_crtc *crtc)
> > > if (atomic->enable_ips)
> > > intel_ips_enable(crtc);
> > >
> > > + if (atomic->enable_psr)
> > > + intel_psr_enable(crtc);
> >
> > What we need here is a post-modeset fixup hook for
> > encoders/connectors.
> > That would avoid the layering violation of going through a crtc and
> > then
> > doing the crtc->encoder lookup you do in the previous patch. And we
> > have
> > other uses for this, e.g. mipi self refresh, fixing up infoframes and
> > all
> > those things.
>
> I see your point but I'm not sure if I totally got your idea.
>
> So would it be a generic hook when we detect encoder/connector being
> enabled/disabled?
>
> and how to determine that? could you please elaborate your idea a bit
> more?
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c3bc6ab77508..427daac480f5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13273,6 +13273,11 @@ static int intel_atomic_commit(struct drm_device *dev,
modeset_put_power_domains(dev_priv, put_domains);
intel_post_plane_update(intel_crtc);
+
+ if (needs_modeset || update_pipe) {
+ for_each_encoder_on_crtc(dev, crtc, encoder)
+ encoder->enable_fixup(encoder);
+ }
}
/* FIXME: add subpixel order */
With this enable_fixup (maybe we should call it post_plane_enable) will
will get called both when we do a full modeset, but also when we just do a
fast modeset/pipe update. Then we can move everything that we're currently
doing in ->enable which also must be done for fastboot into
->post_plane_enable, like infoframes, DRRS, psr or whatever.
The only tricky part is that the core won't do the book-keeping for us
anymore, i.e. we need to check ourselves in this new hook whether we need
to do the setup or not, since if userspace only changes the scaling a lot
we might get a lot of ->post_plane_enable calls without any ->disable
calls in between. So that's also where you have to put your checks:
intel_dp_post_plane_enable()
{
if (!psr_enabled)
enable_psr();
if (!DRRS_on)
enable_drrs();
/* maybe in the future */
if (!audio_enabled)
enable_audio();
}
intel_hdmi_post_plane_enable()
{
/* ifxup infoframes if they're crap from the bios */
}
Cheers, 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
