| Message ID | 1461702945-14185-12-git-send-email-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 26/04/16 23:35, Laurent Pinchart wrote: > The omapdrm DSS manager enable/disable operations check the DSS manager > state to avoid double enabling/disabling. Move that code to the DSS > manager to decrease the dependency of the DRM layer to the DSS layer. Shouldn't omapdrm know if the CRTC is enabled or not, and avoid double-enable/disable by just looking at its internal state? If so, we could remove dispc_mgr_is_enabled() call as you do, and add a WARN_ON() to omapdss if the mgr is already enabled/disabled to catch bugs in omapdrm. Tomi
On Tue, May 10, 2016 at 04:28:22PM +0300, Tomi Valkeinen wrote: > > On 26/04/16 23:35, Laurent Pinchart wrote: > > The omapdrm DSS manager enable/disable operations check the DSS manager > > state to avoid double enabling/disabling. Move that code to the DSS > > manager to decrease the dependency of the DRM layer to the DSS layer. > > Shouldn't omapdrm know if the CRTC is enabled or not, and avoid > double-enable/disable by just looking at its internal state? > > If so, we could remove dispc_mgr_is_enabled() call as you do, and add a > WARN_ON() to omapdss if the mgr is already enabled/disabled to catch > bugs in omapdrm. Yeah, atomic helpers guarantees you that it'll never screw this up and disable/enable twice. The only tricky bit to consider is boot-up, where the firmware might have left something enabled, but by default the reset helpers assume everything is off, and just reset sw state to all off. Either patch up that state to match (you only need to set very few things, no need to recover all the details about the mode), or manually shut things down. But you need to make sure that at driver load sw matches hw state. -Daniel
Hi Daniel, On Wednesday 11 May 2016 09:40:14 Daniel Vetter wrote: > On Tue, May 10, 2016 at 04:28:22PM +0300, Tomi Valkeinen wrote: > > On 26/04/16 23:35, Laurent Pinchart wrote: > > > The omapdrm DSS manager enable/disable operations check the DSS manager > > > state to avoid double enabling/disabling. Move that code to the DSS > > > manager to decrease the dependency of the DRM layer to the DSS layer. > > > > Shouldn't omapdrm know if the CRTC is enabled or not, and avoid > > double-enable/disable by just looking at its internal state? > > > > If so, we could remove dispc_mgr_is_enabled() call as you do, and add a > > WARN_ON() to omapdss if the mgr is already enabled/disabled to catch > > bugs in omapdrm. > > Yeah, atomic helpers guarantees you that it'll never screw this up and > disable/enable twice. It's more complex than that. The omapdrm driver has a complicated call stack, going through omapdss subdrivers that make verification difficult at the moment. The CRTCs are not enabled/disabled directly in response to the DRM core's calls to the CRTC enable and disable operations, but through the omapdss driver from the encoder helper enable and disable operations. That's something I'd like to clean up, but I'd rather not mess with it right now. > The only tricky bit to consider is boot-up, where the firmware might have > left something enabled, but by default the reset helpers assume everything > is off, and just reset sw state to all off. > > Either patch up that state to match (you only need to set very few things, > no need to recover all the details about the mode), or manually shut > things down. But you need to make sure that at driver load sw matches hw > state. > -Daniel
Hi Tomi, On Tuesday 10 May 2016 16:28:22 Tomi Valkeinen wrote: > On 26/04/16 23:35, Laurent Pinchart wrote: > > The omapdrm DSS manager enable/disable operations check the DSS manager > > state to avoid double enabling/disabling. Move that code to the DSS > > manager to decrease the dependency of the DRM layer to the DSS layer. > > Shouldn't omapdrm know if the CRTC is enabled or not, and avoid > double-enable/disable by just looking at its internal state? Ideally yes, and more than that, we shouldn't look at any omapdrm-specific state for the CRTC, but only at the CRTC core state. However, given the driver design that enables/disables CRTCs through a complicated call stack starting from the encoder enable/disable functions instead of in the CRTC enable/disable handlers, this is hard to track at the moment. I'd like to clean this mess up, but that will be a separate (and likely quite large) patch series. > If so, we could remove dispc_mgr_is_enabled() call as you do, and add a > WARN_ON() to omapdss if the mgr is already enabled/disabled to catch > bugs in omapdrm.
diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c index f83608b69e68..f18a6911923a 100644 --- a/drivers/gpu/drm/omapdrm/dss/dispc.c +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c @@ -2887,7 +2887,6 @@ bool dispc_mgr_is_enabled(enum omap_channel channel) { return !!mgr_fld_read(channel, DISPC_MGR_FLD_ENABLE); } -EXPORT_SYMBOL(dispc_mgr_is_enabled); void dispc_wb_enable(bool enable) { diff --git a/drivers/gpu/drm/omapdrm/dss/output.c b/drivers/gpu/drm/omapdrm/dss/output.c index 829232ad8c81..e6f67d76ec95 100644 --- a/drivers/gpu/drm/omapdrm/dss/output.c +++ b/drivers/gpu/drm/omapdrm/dss/output.c @@ -218,12 +218,18 @@ EXPORT_SYMBOL(dss_mgr_set_lcd_config); int dss_mgr_enable(enum omap_channel channel) { + if (dispc_mgr_is_enabled(channel)) + return 0; + return dss_mgr_ops->enable(channel); } EXPORT_SYMBOL(dss_mgr_enable); void dss_mgr_disable(enum omap_channel channel) { + if (!dispc_mgr_is_enabled(channel)) + return; + dss_mgr_ops->disable(channel); } EXPORT_SYMBOL(dss_mgr_disable); diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c index 4c56d6a68390..35d59c385d38 100644 --- a/drivers/gpu/drm/omapdrm/omap_crtc.c +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c @@ -139,9 +139,6 @@ static void omap_crtc_set_enabled(struct drm_crtc *crtc, bool enable) return; } - if (dispc_mgr_is_enabled(channel) == enable) - return; - if (omap_crtc->channel == OMAP_DSS_CHANNEL_DIGIT) { /* * Digit output produces some sync lost interrupts during the
The omapdrm DSS manager enable/disable operations check the DSS manager state to avoid double enabling/disabling. Move that code to the DSS manager to decrease the dependency of the DRM layer to the DSS layer. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- drivers/gpu/drm/omapdrm/dss/dispc.c | 1 - drivers/gpu/drm/omapdrm/dss/output.c | 6 ++++++ drivers/gpu/drm/omapdrm/omap_crtc.c | 3 --- 3 files changed, 6 insertions(+), 4 deletions(-)