[PATCH 02/17] drm/tegra: Do not enable output on .mode_set()

[Daniel Vetter]

On Tue, Nov 04, 2014 at 04:26:12PM +0100, Thierry Reding wrote:
> On Tue, Nov 04, 2014 at 10:11:22AM -0500, Sean Paul wrote:
> > On Tue, Nov 4, 2014 at 9:50 AM, Thierry Reding <thierry.reding at gmail.com> wrote:
> > > On Mon, Nov 03, 2014 at 12:18:30PM -0500, Sean Paul wrote:
> > >> On Mon, Nov 3, 2014 at 4:27 AM, Thierry Reding <thierry.reding at gmail.com> wrote:
> > >> > From: Thierry Reding <treding at nvidia.com>
> > >> >
> > >> > The output is already enabled in .dpms(), doing it in .mode_set() too
> > >> > can cause noticeable flicker.
> > >> >
> > >>
> > >> I think this should be coupled with "drm/tegra: DPMS off/on in encoder
> > >> prepare/commit" that I sent earlier this week. Without it, the driver
> > >> can get into a state where connector status is on, but the output is
> > >> disabled.
> > >
> > > I'm not sure I exactly understand which problem that patch fixes, but
> > > I'll give it some testing to see if it doesn't break anything.
> > >
> > 
> > I'll try to explain it more clearly.
> > 
> > The problem occurs when userspace does set_property(dpms_on), then modeset.
> > 
> > When the set_property ioctl to set dpms = on is called, the tegra
> > driver goes through drm_helper_connector_dpms(). At this point,
> > because the connector has not participated in a modeset,
> > connector->encoder == NULL. In drm_helper_connector_dpms(), we set
> > connector->dpms = DRM_MODE_DPMS_ON, and skip everything else because
> > !encoder && !crtc.
> > 
> > When modeset is called, we go through drm_crtc_helper_set_config().
> > This function will populate connector->encoder with the appropriate
> > encoder and call drm_crtc_helper_set_mode(), which goes through the
> > prepare/mode_set/commit calls. Finally, drm_crtc_helper_set_config()
> > iterates through the connectors involved in the modeset and calls
> > their dpms() func. In our case, as above, we go through
> > drm_helper_connector_dpms(), but it just exits early because mode ==
> > connector->dpms.
> > 
> > So we end up in a state where connector->dpms == DRM_MODE_DPMS_ON, and
> > the output, as well as any connected panel, has never been enabled.
> > The reason is that they're only enabled in encoder_funcs->dpms(),
> > which is never called in the above scenario.
> > 
> > I hope that makes more sense :-)
> 
> Yes, perfect sense. It's somewhat unfortunate that the helpers even
> allow this to happen. It doesn't seem like it's only the Tegra driver
> using it wrongly. Anyway, I've applied this to my tree.

The helper support for dpms is completely tacked onto the side and yeah
you get to do all the hard work of keeping track of state in the driver.

For modesets atomic should improve on the situation, except that I didn't
write any helpers to implement DPMS. Oops, totally missed that entry
point.

So maybe we should write new helpers to implement connector->dpms on top
of atomic which are actually sane?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch