| Message ID | 1428445727-18103-7-git-send-email-chandra.konduru@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@intel.com> 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 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. Also, you haven't touched any of the other platforms (ilk, vlv, ivb, etc.) so they're all still going to have problems. I think the easiest short-term solution is to do the unshifting in commit_plane and leave the hardware-specific programming functions as-is. 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 >
> -----Original Message----- > From: Roper, Matthew D > Sent: Thursday, April 09, 2015 2:51 PM > To: Konduru, Chandra > Cc: intel-gfx@lists.freedesktop.org; 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@intel.com> > > 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); > 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
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@lists.freedesktop.org; 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@intel.com> > > > > 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. 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
> -----Original Message----- > From: Roper, Matthew D > Sent: Thursday, April 09, 2015 3:23 PM > To: Konduru, Chandra > Cc: intel-gfx@lists.freedesktop.org; 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@lists.freedesktop.org; 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@intel.com> > > > > > > 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
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;
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@intel.com> --- drivers/gpu/drm/i915/intel_sprite.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)