| Message ID | 1408429603-15058-2-git-send-email-sonika.jindal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Aug 19, 2014 at 11:56:42AM +0530, sonika.jindal@intel.com wrote: > From: Sonika Jindal <sonika.jindal@intel.com> > > Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > --- > 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
On 8/20/2014 2:20 AM, Matt Roper wrote: > On Tue, Aug 19, 2014 at 11:56:42AM +0530, sonika.jindal@intel.com wrote: >> From: Sonika Jindal <sonika.jindal@intel.com> >> >> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> >> --- >> 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(). > I understand your points, but I am not sure why the updation of these intel_plane member variables was left in the first place. Do you a reason why this was not present till now? Am I missing something? > Sorry if I'm overlooking something obvious here; I haven't been keeping > up with this whole email thread. > > > Matt >
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)