On Mon, Jul 13, 2015 at 10:59:32AM +0200, Maarten Lankhorst wrote:
> Op 08-07-15 om 22:12 schreef Daniel Vetter:
> > On Wed, Jul 08, 2015 at 08:25:07PM +0200, Maarten Lankhorst wrote:
> >> Op 08-07-15 om 19:52 schreef Daniel Vetter:
> >>> On Wed, Jul 08, 2015 at 06:35:47PM +0200, Maarten Lankhorst wrote:
> >>>> Op 08-07-15 om 10:55 schreef Daniel Vetter:
> >>>>> On Wed, Jul 08, 2015 at 10:00:22AM +0200, Maarten Lankhorst wrote:
> >>>>>> Op 07-07-15 om 18:43 schreef Daniel Vetter:
> >>>>>>> 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.
> >>>>>> Something like this?
> >>>>>> ---
> >>>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> >>>>>> b/drivers/gpu/drm/drm_atomic_helper.c
> >>>>>> index a1d4e13f3908..2989232f4996 100644
> >>>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
> >>>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> >>>>>> @@ -30,6 +30,7 @@
> >>>>>> #include <drm/drm_plane_helper.h>
> >>>>>> #include <drm/drm_crtc_helper.h>
> >>>>>> #include <drm/drm_atomic_helper.h>
> >>>>>> +#include "drm_crtc_internal.h"
> >>>>>> #include <linux/fence.h>
> >>>>>>
> >>>>>> /**
> >>>>>> @@ -1716,7 +1717,12 @@ drm_atomic_helper_crtc_set_property(struct
> >>>>>> drm_crtc *crtc,
> >>>>>> {
> >>>>>> struct drm_atomic_state *state;
> >>>>>> struct drm_crtc_state *crtc_state;
> >>>>>> - int ret = 0;
> >>>>>> + uint64_t retval;
> >>>>>> + int ret;
> >>>>>> +
> >>>>>> + ret = drm_atomic_get_property(&crtc->base, property, &retval);
> >>>>>> + if (!ret && val == retval)
> >>>>>> + return 0;
> >>>>>>
> >>>>>> state = drm_atomic_state_alloc(crtc->dev);
> >>>>>> if (!state)
> >>>>>> @@ -1776,7 +1782,12 @@ drm_atomic_helper_plane_set_property(struct
> >>>>>> drm_plane *plane,
> >>>>>> {
> >>>>>> struct drm_atomic_state *state;
> >>>>>> struct drm_plane_state *plane_state;
> >>>>>> - int ret = 0;
> >>>>>> + uint64_t retval;
> >>>>>> + int ret;
> >>>>>> +
> >>>>>> + ret = drm_atomic_get_property(&plane->base, property, &retval);
> >>>>>> + if (!ret && val == retval)
> >>>>>> + return 0;
> >>>>>>
> >>>>>> state = drm_atomic_state_alloc(plane->dev);
> >>>>>> if (!state)
> >>>>>> @@ -1836,7 +1847,12 @@ drm_atomic_helper_connector_set_property(struct
> >>>>>> drm_connector *connector,
> >>>>>> {
> >>>>>> struct drm_atomic_state *state;
> >>>>>> struct drm_connector_state *connector_state;
> >>>>>> - int ret = 0;
> >>>>>> + uint64_t retval;
> >>>>>> + int ret;
> >>>>>> +
> >>>>>> + ret = drm_atomic_get_property(&connector->base, property,
> >>>>>> &retval);
> >>>>>> + if (!ret && val == retval)
> >>>>>> + return 0;
> >>>>>>
> >>>>>> state = drm_atomic_state_alloc(connector->dev);
> >>>>>> if (!state)
> >>>>> The reason I didn't do this is that a prop change might still result in
> >>>>> no
> >>>>> hw state change (e.g. if you go automitic->explicit setting matching
> >>>>> automatic one). Hence I think we need to solve this in lower levels
> >>>>> anyway, i.e. in when computing the config. But it shouldn't cause
> >>>>> trouble
> >>>>> yet.
> >>>> Is that a ack or nack?
> >>> I think we shouldn't need this really for i915, and it might cover up
> >>> bugs. I prefer we just do the evade modeset logic you've implemented once
> >>> we switch over to atomic props. Since atm we only have atomic props which
> >>> get updated in pageflips we shouldn't have serious problems here yet (for
> >>> setting the rotation prop to 0° again when fbdev starts up).
> >>>
> >>> Or do I miss something still here?
> >> Yes, if the hardware mode is incompatible with its calculated sw mode,
> >> and we set a different mode from fbdev you get 2 modesets instead of 1.
> > How does that happen? For setting the rotation property we should just
> > duplicate the current crtc state. Since there's no mode changing (they
> > should match perfectly no matter how botched the reconstruction is) there
> > shouldn't be any need to recompute the config completely and discover that
> > there's a mismatch. Which means we'll just do the plane update (which
> > might do a few silly mmios but shouldn't block) and that's it.
> >
> > At least that's what I'd expect - where does this fall apart?
> If crtc is active and primary fb visible, and converted to atomic:
>
> restore_fbdev_mode() ->
> drm_mode_plane_set_obj_prop() ->
> drm_atomic_helper_plane_set_property() ->
> drm_atomic_get_plane_state() ->
> drm_atomic_get_crtc_state()
> crtc state is part of the state, intel_modeset_pipe_config performs
> the initial check if modeset's needed. Lets assume yes:
"Let's assume yes" -> that's imo a bug, so where does this happen so that
we can fix it? Disabling a plane or setting a plane prop really shouldn't
result in a modeset. Well at least if it's not a plane prop that does
required a modeset (but I don't think we have any of those).
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
