| Message ID | 1436252911-5703-3-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote: > This allows the first atomic call during hw init to be a real modeset, > which is useful for forcing a recalculation. fbcon is optional, you can't rely on anything being done in any specific way. What exactly do you need this for, what's the implications? -Daniel > > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/drm_fb_helper.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index cac422916c7a..33b5e4ecaf46 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -322,10 +322,12 @@ static bool restore_fbdev_mode(struct drm_fb_helper *fb_helper) > drm_warn_on_modeset_not_all_locked(dev); > > list_for_each_entry(plane, &dev->mode_config.plane_list, head) { > - if (plane->type != DRM_PLANE_TYPE_PRIMARY) > + if (plane->type != DRM_PLANE_TYPE_PRIMARY && > + (!plane->state || plane->state->fb)) > drm_plane_force_disable(plane); > > - if (dev->mode_config.rotation_property) { > + if (dev->mode_config.rotation_property && > + (!plane->state || plane->state->rotation != BIT(DRM_ROTATE_0))) { > drm_mode_plane_set_obj_prop(plane, > dev->mode_config.rotation_property, > BIT(DRM_ROTATE_0)); > -- > 2.1.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 07-07-15 om 11:18 schreef Daniel Vetter: > On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote: >> This allows the first atomic call during hw init to be a real modeset, >> which is useful for forcing a recalculation. > fbcon is optional, you can't rely on anything being done in any specific > way. What exactly do you need this for, what's the implications? In the hw readout I noticed some warnings when I wasn't setting any mode property in the readout. I want the first function to be the modeset, so we have a sane base to commit changes on. Ideally this whole function would have a atomic counterpart which does it in one go. :)
On Tue, Jul 07, 2015 at 12:20:10PM +0200, Maarten Lankhorst wrote: > Op 07-07-15 om 11:18 schreef Daniel Vetter: > > On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote: > >> This allows the first atomic call during hw init to be a real modeset, > >> which is useful for forcing a recalculation. > > fbcon is optional, you can't rely on anything being done in any specific > > way. What exactly do you need this for, what's the implications? > In the hw readout I noticed some warnings when I wasn't setting any mode property in the readout. > I want the first function to be the modeset, so we have a sane base to commit changes on. > Ideally this whole function would have a atomic counterpart which does it in one go. :) Yeah. Otoh as soon as we have atomic modeset working we can replace all the legacy entry points with atomic helpers, and then even plane_disable will be a full atomic modeset. What did fall apart with just touching properties/planes now? -Daniel
Op 07-07-15 om 14:10 schreef Daniel Vetter: > On Tue, Jul 07, 2015 at 12:20:10PM +0200, Maarten Lankhorst wrote: >> Op 07-07-15 om 11:18 schreef Daniel Vetter: >>> On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote: >>>> This allows the first atomic call during hw init to be a real modeset, >>>> which is useful for forcing a recalculation. >>> fbcon is optional, you can't rely on anything being done in any specific >>> way. What exactly do you need this for, what's the implications? >> In the hw readout I noticed some warnings when I wasn't setting any mode property in the readout. >> I want the first function to be the modeset, so we have a sane base to commit changes on. >> Ideally this whole function would have a atomic counterpart which does it in one go. :) > Yeah. Otoh as soon as we have atomic modeset working we can replace all > the legacy entry points with atomic helpers, and then even plane_disable > will be a full atomic modeset. > > What did fall apart with just touching properties/planes now? Setting rotation on the primary plane caused it to be disabled because the src and dst rectangle are not set up yet until the modeset. So the check function saw that the plane should be invisible and performs the update. It's also an extra vblank wait when the primary plane is visible.
Op 07-07-15 om 14:10 schreef Daniel Vetter: > On Tue, Jul 07, 2015 at 12:20:10PM +0200, Maarten Lankhorst wrote: >> Op 07-07-15 om 11:18 schreef Daniel Vetter: >>> On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote: >>>> This allows the first atomic call during hw init to be a real modeset, >>>> which is useful for forcing a recalculation. >>> fbcon is optional, you can't rely on anything being done in any specific >>> way. What exactly do you need this for, what's the implications? >> In the hw readout I noticed some warnings when I wasn't setting any mode property in the readout. >> I want the first function to be the modeset, so we have a sane base to commit changes on. >> Ideally this whole function would have a atomic counterpart which does it in one go. :) > Yeah. Otoh as soon as we have atomic modeset working we can replace all > the legacy entry points with atomic helpers, and then even plane_disable > will be a full atomic modeset. > > What did fall apart with just touching properties/planes now? Also when i915 is fully atomic it calculates in intel_modeset_compute_config if a modeset is needed after the first atomic call. Right now because intel_modeset_compute_config is only called in set_config so this works as expected. Otherwise drm_plane_force_disable or rotate_0 will force a modeset, and if the final mode is different this will introduce a double modeset.
On Tue, Jul 07, 2015 at 04:32:44PM +0200, Maarten Lankhorst wrote: > Op 07-07-15 om 14:10 schreef Daniel Vetter: > > On Tue, Jul 07, 2015 at 12:20:10PM +0200, Maarten Lankhorst wrote: > >> Op 07-07-15 om 11:18 schreef Daniel Vetter: > >>> On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote: > >>>> This allows the first atomic call during hw init to be a real modeset, > >>>> which is useful for forcing a recalculation. > >>> fbcon is optional, you can't rely on anything being done in any specific > >>> way. What exactly do you need this for, what's the implications? > >> In the hw readout I noticed some warnings when I wasn't setting any mode property in the readout. > >> I want the first function to be the modeset, so we have a sane base to commit changes on. > >> Ideally this whole function would have a atomic counterpart which does it in one go. :) > > Yeah. Otoh as soon as we have atomic modeset working we can replace all > > the legacy entry points with atomic helpers, and then even plane_disable > > will be a full atomic modeset. > > > > What did fall apart with just touching properties/planes now? > Setting rotation on the primary plane caused it to be disabled because > the src and dst rectangle are not set up yet until the modeset. So the > check function saw that the plane should be invisible and performs the > update. Sounds like a bug - we need to recreate more of the primary plane state. Or maybe we need to call the atomic_check function for the primary plane to compute all that derived state. But we really can't rely upon userspace to do a modeset first, e.g. X at start (if there's no fbcon) loves to read and then write back all the properties (or at least did). We really need to handle this in the backend properly. > It's also an extra vblank wait when the primary plane is visible. Another bug, no-op changes with the same fb should not result in a vblank wait. At least not when using the helpers (or if there is a case I need to copypaste the fix again). -Daniel
On Tue, Jul 07, 2015 at 05:08:34PM +0200, Maarten Lankhorst wrote: > Op 07-07-15 om 14:10 schreef Daniel Vetter: > > On Tue, Jul 07, 2015 at 12:20:10PM +0200, Maarten Lankhorst wrote: > >> Op 07-07-15 om 11:18 schreef Daniel Vetter: > >>> On Tue, Jul 07, 2015 at 09:08:13AM +0200, Maarten Lankhorst wrote: > >>>> This allows the first atomic call during hw init to be a real modeset, > >>>> which is useful for forcing a recalculation. > >>> fbcon is optional, you can't rely on anything being done in any specific > >>> way. What exactly do you need this for, what's the implications? > >> In the hw readout I noticed some warnings when I wasn't setting any mode property in the readout. > >> I want the first function to be the modeset, so we have a sane base to commit changes on. > >> Ideally this whole function would have a atomic counterpart which does it in one go. :) > > Yeah. Otoh as soon as we have atomic modeset working we can replace all > > the legacy entry points with atomic helpers, and then even plane_disable > > will be a full atomic modeset. > > > > What did fall apart with just touching properties/planes now? > Also when i915 is fully atomic it calculates in intel_modeset_compute_config > if a modeset is needed after the first atomic call. Right now because > intel_modeset_compute_config is only called in set_config so this works as expected. > Otherwise drm_plane_force_disable or rotate_0 will force a modeset, > and if the final mode is different this will introduce a double modeset. For expensive properties (i.e. a no-op changes causes something that takes time like modeset or vblank wait) we need to make sure we filter them out in atomic_check. Yeah not quite there yet with pure atomic, but meanwhile the existing legacy set_prop functions should all filter out no-op changes themselves. If we don't do that for rotation then that's a bug. Same for disabling planes harder, that shouldn't take time. Especially since fbcon only force-disable non-primary plane, and for driver load that's the exact thing we already do in the driver anyway. -Daniel
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index cac422916c7a..33b5e4ecaf46 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -322,10 +322,12 @@ static bool restore_fbdev_mode(struct drm_fb_helper *fb_helper) drm_warn_on_modeset_not_all_locked(dev); list_for_each_entry(plane, &dev->mode_config.plane_list, head) { - if (plane->type != DRM_PLANE_TYPE_PRIMARY) + if (plane->type != DRM_PLANE_TYPE_PRIMARY && + (!plane->state || plane->state->fb)) drm_plane_force_disable(plane); - if (dev->mode_config.rotation_property) { + if (dev->mode_config.rotation_property && + (!plane->state || plane->state->rotation != BIT(DRM_ROTATE_0))) { drm_mode_plane_set_obj_prop(plane, dev->mode_config.rotation_property, BIT(DRM_ROTATE_0));
This allows the first atomic call during hw init to be a real modeset, which is useful for forcing a recalculation. Cc: dri-devel@lists.freedesktop.org Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/drm_fb_helper.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)