| Message ID | 1352326005-7215-1-git-send-email-thierry.reding@avionic-design.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Nov 7, 2012 at 5:06 PM, Thierry Reding <thierry.reding@avionic-design.de> wrote: > When running the xf86-video-modesetting driver on top of a KMS driver, > leaving X causes the active encoder's DPMS mode to be set to off by the > drm_crtc_helper_disable() function. This doesn't set the connector's > DPMS mode to off, however, which results in subsequent calls to the > drm_helper_connector_dpms() function to not enable the encoder because > the connector's DPMS mode hasn't changed. > > This patch works around this by forcing the connector's DPMS mode to off > if the encoder has changed, which always happens when a new mode is set > after drm_crtc_helper_disable(). The code that sets the connector DPMS > mode to on will then also enable the CRTC and encoder because the mode > actually changed. > > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de> Reviewed-by: Alex Deucher <alexander.deucher@amd.com> > --- > drivers/gpu/drm/drm_crtc_helper.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c > index 1227adf..1b5ebf1 100644 > --- a/drivers/gpu/drm/drm_crtc_helper.c > +++ b/drivers/gpu/drm/drm_crtc_helper.c > @@ -649,6 +649,12 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set) > */ > if (connector->encoder) > connector->encoder->crtc = NULL; > + /* > + * HACK: Force DPMS mode to off so that the encoder > + * has a chance of being reenabled along with the > + * connector later on. > + */ > + connector->dpms = DRM_MODE_DPMS_OFF; > connector->encoder = new_encoder; > } > } > -- > 1.8.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Nov 08, 2012 at 12:48:18PM -0500, Alex Deucher wrote: > On Wed, Nov 7, 2012 at 5:06 PM, Thierry Reding > <thierry.reding@avionic-design.de> wrote: > > When running the xf86-video-modesetting driver on top of a KMS driver, > > leaving X causes the active encoder's DPMS mode to be set to off by the > > drm_crtc_helper_disable() function. This doesn't set the connector's > > DPMS mode to off, however, which results in subsequent calls to the > > drm_helper_connector_dpms() function to not enable the encoder because > > the connector's DPMS mode hasn't changed. > > > > This patch works around this by forcing the connector's DPMS mode to off > > if the encoder has changed, which always happens when a new mode is set > > after drm_crtc_helper_disable(). The code that sets the connector DPMS > > mode to on will then also enable the CRTC and encoder because the mode > > actually changed. > > > > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de> > > Reviewed-by: Alex Deucher <alexander.deucher@amd.com> This is really much uglier than I would have liked, but I honestly can't think of any better way that would be this easy to implement. An alternative solution would be to add dpms fields to both drm_crtc and drm_encoder structures and modify drm_helper_connector_dpms() to run the CRTC's and encoder's .dpms() functions if the new mode is different from that stored in the respective dpms fields. Coding that, however, will probably be more involved and more likely to introduce errors somewhere else because of the extra accounting required. In case this workaround isn't acceptable I can take a look at coding up a cleaner fix. Thierry > > --- > > drivers/gpu/drm/drm_crtc_helper.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c > > index 1227adf..1b5ebf1 100644 > > --- a/drivers/gpu/drm/drm_crtc_helper.c > > +++ b/drivers/gpu/drm/drm_crtc_helper.c > > @@ -649,6 +649,12 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set) > > */ > > if (connector->encoder) > > connector->encoder->crtc = NULL; > > + /* > > + * HACK: Force DPMS mode to off so that the encoder > > + * has a chance of being reenabled along with the > > + * connector later on. > > + */ > > + connector->dpms = DRM_MODE_DPMS_OFF; > > connector->encoder = new_encoder; > > } > > } > > -- > > 1.8.0 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/dri-devel > >
On Thu, Nov 08, 2012 at 08:01:57PM +0100, Thierry Reding wrote: > On Thu, Nov 08, 2012 at 12:48:18PM -0500, Alex Deucher wrote: > > On Wed, Nov 7, 2012 at 5:06 PM, Thierry Reding > > <thierry.reding@avionic-design.de> wrote: > > > When running the xf86-video-modesetting driver on top of a KMS driver, > > > leaving X causes the active encoder's DPMS mode to be set to off by the > > > drm_crtc_helper_disable() function. This doesn't set the connector's > > > DPMS mode to off, however, which results in subsequent calls to the > > > drm_helper_connector_dpms() function to not enable the encoder because > > > the connector's DPMS mode hasn't changed. > > > > > > This patch works around this by forcing the connector's DPMS mode to off > > > if the encoder has changed, which always happens when a new mode is set > > > after drm_crtc_helper_disable(). The code that sets the connector DPMS > > > mode to on will then also enable the CRTC and encoder because the mode > > > actually changed. > > > > > > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de> > > > > Reviewed-by: Alex Deucher <alexander.deucher@amd.com> > > This is really much uglier than I would have liked, but I honestly can't > think of any better way that would be this easy to implement. An > alternative solution would be to add dpms fields to both drm_crtc and > drm_encoder structures and modify drm_helper_connector_dpms() to run the > CRTC's and encoder's .dpms() functions if the new mode is different from > that stored in the respective dpms fields. Coding that, however, will > probably be more involved and more likely to introduce errors somewhere > else because of the extra accounting required. > > In case this workaround isn't acceptable I can take a look at coding up > a cleaner fix. Imo the confusion around dpms in the crtc helper all stem from the totally unclear semantics of dmps vs. modeset calls. Besides your case, where we essentially disable a encoder/crtc without updating the dpms properties, theres the symmetric issue that after a modeset call all newly active encoders/crtcs are fully enabled, but the connector dpms property is still the same it was before the modeset call. Which is even more funny since userspace is allowed to change the dpms property on an unused connector. Additional, the i915 modeset code now also updates the userspace visible dpms property after a modeset, to avoid potential confusion in userspace - that way the dpms property always reflects reality when the connector is used, and is ignored when it is unused. Dunno whether forcing dpms to on after a modeset is the semantics we want though. -Daniel
On Fri, Nov 09, 2012 at 10:26:52PM +0100, Daniel Vetter wrote: > On Thu, Nov 08, 2012 at 08:01:57PM +0100, Thierry Reding wrote: > > On Thu, Nov 08, 2012 at 12:48:18PM -0500, Alex Deucher wrote: > > > On Wed, Nov 7, 2012 at 5:06 PM, Thierry Reding > > > <thierry.reding@avionic-design.de> wrote: > > > > When running the xf86-video-modesetting driver on top of a KMS driver, > > > > leaving X causes the active encoder's DPMS mode to be set to off by the > > > > drm_crtc_helper_disable() function. This doesn't set the connector's > > > > DPMS mode to off, however, which results in subsequent calls to the > > > > drm_helper_connector_dpms() function to not enable the encoder because > > > > the connector's DPMS mode hasn't changed. > > > > > > > > This patch works around this by forcing the connector's DPMS mode to off > > > > if the encoder has changed, which always happens when a new mode is set > > > > after drm_crtc_helper_disable(). The code that sets the connector DPMS > > > > mode to on will then also enable the CRTC and encoder because the mode > > > > actually changed. > > > > > > > > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de> > > > > > > Reviewed-by: Alex Deucher <alexander.deucher@amd.com> > > > > This is really much uglier than I would have liked, but I honestly can't > > think of any better way that would be this easy to implement. An > > alternative solution would be to add dpms fields to both drm_crtc and > > drm_encoder structures and modify drm_helper_connector_dpms() to run the > > CRTC's and encoder's .dpms() functions if the new mode is different from > > that stored in the respective dpms fields. Coding that, however, will > > probably be more involved and more likely to introduce errors somewhere > > else because of the extra accounting required. > > > > In case this workaround isn't acceptable I can take a look at coding up > > a cleaner fix. > > Imo the confusion around dpms in the crtc helper all stem from the totally > unclear semantics of dmps vs. modeset calls. Besides your case, where we > essentially disable a encoder/crtc without updating the dpms properties, > theres the symmetric issue that after a modeset call all newly active > encoders/crtcs are fully enabled, but the connector dpms property is still > the same it was before the modeset call. Which is even more funny since > userspace is allowed to change the dpms property on an unused connector. > > Additional, the i915 modeset code now also updates the userspace visible > dpms property after a modeset, to avoid potential confusion in userspace - > that way the dpms property always reflects reality when the connector is > used, and is ignored when it is unused. Dunno whether forcing dpms to on > after a modeset is the semantics we want though. For the atomic modeset at least, I would prefer not to magically flip some random properties. That is, if the user doesn't specify a property its value should remain the same. Currently, I'm treating DPMS as a special case, and changing its value just like the non-atomic modeset does. The rule of not changing anything the user didn't specify does make the whole thing a bit more difficult to use though. What if we have a property for something relatively innocuous, let's say chroma siting for example, and the user changes the pixel format from packed YUV to planar YUV, but the hardware only supports the MPEG2 chroma siting convetion for packed YUV, and MPEG1 chroma siting for planar YUV. Now, if the user doesn't specify the chroma siting, we would need to reject the request. But I guess this case is not that much different from the user specifying the wrong value for chroma siting. We could solve it by having some kind of fuzzy vs. strict control. I suppose we might even want more fine grained control over this behaviour, so just two levels of fuzzyness may not be enough. Then we'd need to assign each property to a different class, depending on how extreme the result of changing it would be.
On Mon, Nov 12, 2012 at 3:04 AM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Fri, Nov 09, 2012 at 10:26:52PM +0100, Daniel Vetter wrote: >> On Thu, Nov 08, 2012 at 08:01:57PM +0100, Thierry Reding wrote: >> > On Thu, Nov 08, 2012 at 12:48:18PM -0500, Alex Deucher wrote: >> > > On Wed, Nov 7, 2012 at 5:06 PM, Thierry Reding >> > > <thierry.reding@avionic-design.de> wrote: >> > > > When running the xf86-video-modesetting driver on top of a KMS driver, >> > > > leaving X causes the active encoder's DPMS mode to be set to off by the >> > > > drm_crtc_helper_disable() function. This doesn't set the connector's >> > > > DPMS mode to off, however, which results in subsequent calls to the >> > > > drm_helper_connector_dpms() function to not enable the encoder because >> > > > the connector's DPMS mode hasn't changed. >> > > > >> > > > This patch works around this by forcing the connector's DPMS mode to off >> > > > if the encoder has changed, which always happens when a new mode is set >> > > > after drm_crtc_helper_disable(). The code that sets the connector DPMS >> > > > mode to on will then also enable the CRTC and encoder because the mode >> > > > actually changed. >> > > > >> > > > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de> >> > > >> > > Reviewed-by: Alex Deucher <alexander.deucher@amd.com> >> > >> > This is really much uglier than I would have liked, but I honestly can't >> > think of any better way that would be this easy to implement. An >> > alternative solution would be to add dpms fields to both drm_crtc and >> > drm_encoder structures and modify drm_helper_connector_dpms() to run the >> > CRTC's and encoder's .dpms() functions if the new mode is different from >> > that stored in the respective dpms fields. Coding that, however, will >> > probably be more involved and more likely to introduce errors somewhere >> > else because of the extra accounting required. >> > >> > In case this workaround isn't acceptable I can take a look at coding up >> > a cleaner fix. >> >> Imo the confusion around dpms in the crtc helper all stem from the totally >> unclear semantics of dmps vs. modeset calls. Besides your case, where we >> essentially disable a encoder/crtc without updating the dpms properties, >> theres the symmetric issue that after a modeset call all newly active >> encoders/crtcs are fully enabled, but the connector dpms property is still >> the same it was before the modeset call. Which is even more funny since >> userspace is allowed to change the dpms property on an unused connector. >> >> Additional, the i915 modeset code now also updates the userspace visible >> dpms property after a modeset, to avoid potential confusion in userspace - >> that way the dpms property always reflects reality when the connector is >> used, and is ignored when it is unused. Dunno whether forcing dpms to on >> after a modeset is the semantics we want though. > > For the atomic modeset at least, I would prefer not to magically flip > some random properties. That is, if the user doesn't specify a property > its value should remain the same. Currently, I'm treating DPMS as > a special case, and changing its value just like the non-atomic modeset > does. I do prefer that, at least on lastclose, that we revert properties back to default/initial state, so that userspace unaware of some new or driver specific properties doesn't get confused. But dpms is a bit of an odd-duck property. Probably we need to split out the property value "what userspace wants it to be" from the current state which could change temporarily as the modeset is applied. At the end of the modeset the driver should somehow try to put things back to the state that userspace wants according to the dpms property. BR, -R > The rule of not changing anything the user didn't specify does make > the whole thing a bit more difficult to use though. What if we have a > property for something relatively innocuous, let's say chroma siting for > example, and the user changes the pixel format from packed YUV to planar > YUV, but the hardware only supports the MPEG2 chroma siting convetion > for packed YUV, and MPEG1 chroma siting for planar YUV. Now, if the user > doesn't specify the chroma siting, we would need to reject the request. > But I guess this case is not that much different from the user specifying > the wrong value for chroma siting. > > We could solve it by having some kind of fuzzy vs. strict control. I > suppose we might even want more fine grained control over this > behaviour, so just two levels of fuzzyness may not be enough. Then > we'd need to assign each property to a different class, depending on > how extreme the result of changing it would be. > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Mon, Nov 12, 2012 at 3:11 PM, Rob Clark <robdclark@gmail.com> wrote: > I do prefer that, at least on lastclose, that we revert properties > back to default/initial state, so that userspace unaware of some new > or driver specific properties doesn't get confused. > > But dpms is a bit of an odd-duck property. Probably we need to split > out the property value "what userspace wants it to be" from the > current state which could change temporarily as the modeset is > applied. At the end of the modeset the driver should somehow try to > put things back to the state that userspace wants according to the > dpms property. That could result into some serious flicker if e.g. we enable things but dpms is off, so we switch things off. Then userspace does a dpms on again after all modesets have completed. iirc some desktops do this to fake less flickery modeset, but it seems to only result in more (since userspace doesn't know which outputs to disable and which are unaffected by a given modeset changes). Imo simply updating the dpms property (both the internal tracking in the helper, but also the userspace visible part) is the best option. At least it's what intel now does with it's own modeset code ... -Daniel
On Mon, Nov 12, 2012 at 8:17 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Mon, Nov 12, 2012 at 3:11 PM, Rob Clark <robdclark@gmail.com> wrote: >> I do prefer that, at least on lastclose, that we revert properties >> back to default/initial state, so that userspace unaware of some new >> or driver specific properties doesn't get confused. >> >> But dpms is a bit of an odd-duck property. Probably we need to split >> out the property value "what userspace wants it to be" from the >> current state which could change temporarily as the modeset is >> applied. At the end of the modeset the driver should somehow try to >> put things back to the state that userspace wants according to the >> dpms property. > > That could result into some serious flicker if e.g. we enable things > but dpms is off, so we switch things off. Then userspace does a dpms > on again after all modesets have completed. iirc some desktops do this > to fake less flickery modeset, but it seems to only result in more > (since userspace doesn't know which outputs to disable and which are > unaffected by a given modeset changes). Imo simply updating the dpms > property (both the internal tracking in the helper, but also the > userspace visible part) is the best option. At least it's what intel > now does with it's own modeset code ... no, I mean at the end of modeset we should replace dpms(ON) w/ dpms(what_userspace_set_connector_property_to) well, that works unless userspace assumes that the modeset implicitly turns dpms(ON).. then we have problems. BR, -R > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Mon, Nov 12, 2012 at 3:25 PM, Rob Clark <robdclark@gmail.com> wrote: > On Mon, Nov 12, 2012 at 8:17 AM, Daniel Vetter <daniel@ffwll.ch> wrote: >> On Mon, Nov 12, 2012 at 3:11 PM, Rob Clark <robdclark@gmail.com> wrote: >>> I do prefer that, at least on lastclose, that we revert properties >>> back to default/initial state, so that userspace unaware of some new >>> or driver specific properties doesn't get confused. >>> >>> But dpms is a bit of an odd-duck property. Probably we need to split >>> out the property value "what userspace wants it to be" from the >>> current state which could change temporarily as the modeset is >>> applied. At the end of the modeset the driver should somehow try to >>> put things back to the state that userspace wants according to the >>> dpms property. >> >> That could result into some serious flicker if e.g. we enable things >> but dpms is off, so we switch things off. Then userspace does a dpms >> on again after all modesets have completed. iirc some desktops do this >> to fake less flickery modeset, but it seems to only result in more >> (since userspace doesn't know which outputs to disable and which are >> unaffected by a given modeset changes). Imo simply updating the dpms >> property (both the internal tracking in the helper, but also the >> userspace visible part) is the best option. At least it's what intel >> now does with it's own modeset code ... > > no, I mean at the end of modeset we should replace dpms(ON) w/ > dpms(what_userspace_set_connector_property_to) Oh, for atomic modeset that's definitely the better option (we simply need code to be clever enought to not enable stuff if it's not required). But there's the pesky problem of what should happen with the legacy modeset stuff, which I've thought is the topic here ... > well, that works unless userspace assumes that the modeset implicitly > turns dpms(ON).. then we have problems. Presume it does assume that, since that's how the code currently works. Save that the userspace visible dpms property doesn't reflect reality at all. -Daniel
On Mon, Nov 12, 2012 at 8:29 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Mon, Nov 12, 2012 at 3:25 PM, Rob Clark <robdclark@gmail.com> wrote: >> On Mon, Nov 12, 2012 at 8:17 AM, Daniel Vetter <daniel@ffwll.ch> wrote: >>> On Mon, Nov 12, 2012 at 3:11 PM, Rob Clark <robdclark@gmail.com> wrote: >>>> I do prefer that, at least on lastclose, that we revert properties >>>> back to default/initial state, so that userspace unaware of some new >>>> or driver specific properties doesn't get confused. >>>> >>>> But dpms is a bit of an odd-duck property. Probably we need to split >>>> out the property value "what userspace wants it to be" from the >>>> current state which could change temporarily as the modeset is >>>> applied. At the end of the modeset the driver should somehow try to >>>> put things back to the state that userspace wants according to the >>>> dpms property. >>> >>> That could result into some serious flicker if e.g. we enable things >>> but dpms is off, so we switch things off. Then userspace does a dpms >>> on again after all modesets have completed. iirc some desktops do this >>> to fake less flickery modeset, but it seems to only result in more >>> (since userspace doesn't know which outputs to disable and which are >>> unaffected by a given modeset changes). Imo simply updating the dpms >>> property (both the internal tracking in the helper, but also the >>> userspace visible part) is the best option. At least it's what intel >>> now does with it's own modeset code ... >> >> no, I mean at the end of modeset we should replace dpms(ON) w/ >> dpms(what_userspace_set_connector_property_to) > > Oh, for atomic modeset that's definitely the better option (we simply > need code to be clever enought to not enable stuff if it's not > required). But there's the pesky problem of what should happen with > the legacy modeset stuff, which I've thought is the topic here ... well, I was hoping to avoid too many special cases for legacy stuff.. it would be nice if we could do this for atomic and regular modeset. If userspace does assume modeset lights up the displays that it had previously disabled, then maybe we should just declare it to be so and make sure the user visible properties are updated to reflect this. BR, -R >> well, that works unless userspace assumes that the modeset implicitly >> turns dpms(ON).. then we have problems. > > Presume it does assume that, since that's how the code currently > works. Save that the userspace visible dpms property doesn't reflect > reality at all. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index 1227adf..1b5ebf1 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -649,6 +649,12 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set) */ if (connector->encoder) connector->encoder->crtc = NULL; + /* + * HACK: Force DPMS mode to off so that the encoder + * has a chance of being reenabled along with the + * connector later on. + */ + connector->dpms = DRM_MODE_DPMS_OFF; connector->encoder = new_encoder; } }
When running the xf86-video-modesetting driver on top of a KMS driver, leaving X causes the active encoder's DPMS mode to be set to off by the drm_crtc_helper_disable() function. This doesn't set the connector's DPMS mode to off, however, which results in subsequent calls to the drm_helper_connector_dpms() function to not enable the encoder because the connector's DPMS mode hasn't changed. This patch works around this by forcing the connector's DPMS mode to off if the encoder has changed, which always happens when a new mode is set after drm_crtc_helper_disable(). The code that sets the connector DPMS mode to on will then also enable the CRTC and encoder because the mode actually changed. Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de> --- drivers/gpu/drm/drm_crtc_helper.c | 6 ++++++ 1 file changed, 6 insertions(+)