> -----Original Message----- > From: Roper, Matthew D > Sent: Thursday, April 09, 2015 3:23 PM > To: Konduru, Chandra > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Vetter, Daniel; Conselvan De Oliveira, Ander > Subject: Re: [PATCH 06/14] drm/i915: Keep sprite plane src rect in 16.16 format > > On Thu, Apr 09, 2015 at 03:08:55PM -0700, Konduru, Chandra wrote: > > > > > > > -----Original Message----- > > > From: Roper, Matthew D > > > Sent: Thursday, April 09, 2015 2:51 PM > > > To: Konduru, Chandra > > > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Vetter, Daniel; Conselvan De > > > Oliveira, Ander > > > Subject: Re: [PATCH 06/14] drm/i915: Keep sprite plane src rect in > > > 16.16 format > > > > > > On Tue, Apr 07, 2015 at 03:28:39PM -0700, Chandra Konduru wrote: > > > > This patch keeps intel_plane_state->src rect back into 16.16 format. > > > > > > > > v2: > > > > -sprite src rect to match primary format (Matt, Daniel) > > > > > > > > Signed-off-by: Chandra Konduru <chandra.konduru@xxxxxxxxx> > > > > > > This looks good, and matches what we had discussed, but don't you > > > also need to add the corresponding unshift in > > > intel_commit_sprite_plane() when we actually pull the values out and make > use of them? > > The unshift is in patch #14 which should have been in this patch. > > > > From #14 relevant hunk is: > > @@ -1081,10 +1122,10 @@ intel_commit_sprite_plane(struct drm_plane > *plane, > > crtc_y = state->dst.y1; > > crtc_w = drm_rect_width(&state->dst); > > crtc_h = drm_rect_height(&state->dst); > > - src_x = state->src.x1; > > - src_y = state->src.y1; > > - src_w = drm_rect_width(&state->src); > > - src_h = drm_rect_height(&state->src); > > + src_x = state->src.x1 >> 16; > > + src_y = state->src.y1 >> 16; > > + src_w = drm_rect_width(&state->src) >> 16; > > + src_h = drm_rect_height(&state->src) >> 16; > > intel_plane->update_plane(plane, crtc, fb, > > crtc_x, crtc_y, crtc_w, crtc_h, > > src_x, src_y, src_w, src_h); > > Yep, you're right; I got the patch numbers mixed up while flipping back and forth > between patches, and then confused myself further by looking at the wrong > patch while writing my follow up reply. > > As long as we pull this hunk from #14 back into this patch, that should be the > proper fix. You can ignore my other comments below where I was just > confusing myself by looking at the wrong patch numbers. OK, then I will send #6, #14 updated (i.e., moving hunk from #14 to #6) and #9 with updated commit message to address scaler quality. > > > Matt > > > > > > The goal is to keep the meaning of > > > the structure fields consistent at all times > > > (16.16 fixed pt), but once we pull the values out of the structure, > > > we wind up passing them to functions that doing use fixed point, so > > > we do need to unshift at that point. > > > > > > It looks like in patch #13 you do switch the low-level > > > skl_update_plane() to make use of 16.16 input parameters. However > > > any commit that we bisect through between #6 and #13 is going to > > > wind up treating > > > 16.16 values as integer values, which I assume will blow up. > > Patch #13 doesn't touches skl_update_plane(). Perhaps you may be referring > to #14. > > In #14, it does unshift as I mentioned above. I think I need to move > > above hunk to #6 to fix any potential issue due to bisect. > > > > Let me know if you see any potential issue after moving the above hunk to #6. > > > > > Also, you haven't touched any of the other platforms (ilk, vlv, ivb, > > > etc.) so they're all still going to have problems. > > Shifting and unshifting is happening in intel_check_plane and > > intel_commit_plane which is common for all platforms. I don't see what > > the problem you are referring to? > > > > > > I think the easiest short-term solution is to do the unshifting in > > > commit_plane and leave the hardware-specific programming functions as-is. > > This is what I'm doing now keeping platform_update_plane(parameters) > > use unshifted values (i.e., regular ints). I don't see any value to > > pass function parameters as 16.16 values. If there is a need arise we > > can change the semantics of parameters at a later time. > > > > > Longer term, > > > maybe it makes sense for a future patchset to change the actual > > > register- programming functions (foo_update_plane) so that they take > > > a plane_state directly and do their own unshifting? In that case > > > we'd need to update them to do their own unshifting, but at least we > > > wouldn't have to pull all the values out in commit_plane, just to pass them to > these functions. > > > > > > > > > Matt > > > > > > > --- > > > > drivers/gpu/drm/i915/intel_sprite.c | 8 ++++---- > > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > > > > b/drivers/gpu/drm/i915/intel_sprite.c > > > > index ac4aa68..c05fb36 100644 > > > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > > > @@ -1006,10 +1006,10 @@ intel_check_sprite_plane(struct drm_plane > > > *plane, > > > > } > > > > > > > > if (state->visible) { > > > > - src->x1 = src_x; > > > > - src->x2 = src_x + src_w; > > > > - src->y1 = src_y; > > > > - src->y2 = src_y + src_h; > > > > + src->x1 = src_x << 16; > > > > + src->x2 = (src_x + src_w) << 16; > > > > + src->y1 = src_y << 16; > > > > + src->y2 = (src_y + src_h) << 16; > > > > } > > > > > > > > dst->x1 = crtc_x; > > > > -- > > > > 1.7.9.5 > > > > > > > > > > -- > > > Matt Roper > > > Graphics Software Engineer > > > IoTG Platform Enabling & Development Intel Corporation > > > (916) 356-2795 > > -- > 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