On Tue, Aug 19, 2014 at 11:56:42AM +0530, sonika.jindal@xxxxxxxxx wrote: > From: Sonika Jindal <sonika.jindal@xxxxxxxxx> > > Signed-off-by: Sonika Jindal <sonika.jindal@xxxxxxxxx> > --- > drivers/gpu/drm/i915/intel_display.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index e9b578e..1b0e403 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -11528,6 +11528,21 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc, > .x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0, > .y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0, > }; > + const struct { > + int crtc_x, crtc_y; > + unsigned int crtc_w, crtc_h; > + uint32_t src_x, src_y, src_w, src_h; > + } orig = { > + .crtc_x = crtc_x, > + .crtc_y = crtc_y, > + .crtc_w = crtc_w, > + .crtc_h = crtc_h, > + .src_x = src_x, > + .src_y = src_y, > + .src_w = src_w, > + .src_h = src_h, > + }; > + struct intel_plane *intel_plane = to_intel_plane(plane); > bool visible; > int ret; > > @@ -11604,6 +11619,15 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc, > > return 0; > } > + intel_plane->crtc_x = orig.crtc_x; > + intel_plane->crtc_y = orig.crtc_y; > + intel_plane->crtc_w = orig.crtc_w; > + intel_plane->crtc_h = orig.crtc_h; > + intel_plane->src_x = orig.src_x; > + intel_plane->src_y = orig.src_y; > + intel_plane->src_w = orig.src_w; > + intel_plane->src_h = orig.src_h; > + intel_plane->obj = obj; > > ret = intel_pipe_set_base(crtc, src.x1, src.y1, fb); > if (ret) I haven't been following all of this thread carefully, but wouldn't you want to update the intel_plane fields after the point where the function can no longer fail? E.g., if I try to setplane() to a new location while I have a pageflip pending, my request will fail and the plane position won't update, yet you're still going to be updating the internal state tracking variables here. Then if you do an intel_plane_restore() later you're going to see the plane jump unexpectedly. On the other hand, what about the case where the setplane succeeds, but the plane isn't visible (either because it's fully clipped or because your crtc isn't active right now). Those are other paths through the intel_primary_plane_setplane() function that don't seem to be updating the intel_plane fields, even though it seems like you'd want to so that your plane doesn't snap back to an old position on a later intel_plane_restore(). Sorry if I'm overlooking something obvious here; I haven't been keeping up with this whole email thread. Matt -- Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx