On Thu, Apr 16, 2015 at 12:46:53AM +0300, Laurent Pinchart wrote: > Hi Daniel, > > Thank you for the patch. > > On Friday 10 April 2015 16:22:39 Daniel Vetter wrote: > > It's a silly thing to do and surprises driver writers. Most likely > > this did already blow up for exynos. > > > > It's also a silly thing to change plane state when it's off, but fbdev > > is silly (it does an unconditional modeset over all planes). And > > userspace can be evil. So I think we need this. > > > > With this check in the helpers we can remove the one in i915 code for > > the same conditions (becuase ->crtc iff ->fb). > > > > Cc: Gustavo Padovan <gustavo@xxxxxxxxxxx> > > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx > > Cc: Inki Dae <inki.dae@xxxxxxxxxxx> > > Cc: Matt Roper <matthew.d.roper@xxxxxxxxx> > > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx> > > Acked-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > Tested-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > > (with the ongoing omapdrm atomic update conversion work) > > > --- > > drivers/gpu/drm/drm_atomic_helper.c | 3 ++- > > drivers/gpu/drm/i915/intel_atomic_plane.c | 4 ---- > > 2 files changed, 2 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > > b/drivers/gpu/drm/drm_atomic_helper.c index 41c38edade74..e1556143d811 > > 100644 > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > @@ -1216,7 +1216,8 @@ void drm_atomic_helper_commit_planes(struct drm_device > > *dev, if (drm_atomic_plane_disabling(plane, old_plane_state) && > > funcs->atomic_disable) > > funcs->atomic_disable(plane, old_plane_state); > > - else > > + else if (plane->state->crtc || > > + drm_atomic_plane_disabling(plane, old_plane_state)) > > funcs->atomic_update(plane, old_plane_state); > > The test is so trivial that I wonder whether it makes sense to make > atomic_disable() optional. Wouldn't it be easier to either make > atomic_disable() mandatory, or to remove it completely ? Yeah it's fairly simple. I think it's more to give drivers some guidance and to more closely resemble the legacy plane entry points which also had the split between update and disable. Imo doesn't really hurt to keep this around. Merged to topic/drm-misc, thanks for the feedback. -Daniel > > > } > > > > diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c > > b/drivers/gpu/drm/i915/intel_atomic_plane.c index > > 976b89156570..cb383a0fc392 100644 > > --- a/drivers/gpu/drm/i915/intel_atomic_plane.c > > +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c > > @@ -172,10 +172,6 @@ static void intel_plane_atomic_update(struct drm_plane > > *plane, struct intel_plane_state *intel_state = > > to_intel_plane_state(plane->state); > > > > - /* Don't disable an already disabled plane */ > > - if (!plane->state->fb && !old_state->fb) > > - return; > > - > > intel_plane->commit_plane(plane, intel_state); > > } > > -- > Regards, > > Laurent Pinchart > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
