From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jindal, Sonika" Subject: Re: [PATCH 1/2] drm/i915: Updating plane parameters for primary plane in setplane Date: Wed, 20 Aug 2014 11:23:45 +0530 Message-ID: <53F437E9.5040806@intel.com> References: <53E98946.8010109@intel.com> <1408429603-15058-1-git-send-email-sonika.jindal@intel.com> <1408429603-15058-2-git-send-email-sonika.jindal@intel.com> <20140819205004.GV2245@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id A1AD66E1DA for ; Tue, 19 Aug 2014 22:53:48 -0700 (PDT) In-Reply-To: <20140819205004.GV2245@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Matt Roper Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org 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 >> >> Signed-off-by: Sonika Jindal >> --- >> 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 >