| Message ID | 1421152420-20375-2-git-send-email-sonika.jindal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jan 13, 2015 at 06:03:39PM +0530, Sonika Jindal wrote: > Taking rotation into account while checking the plane > and adjusting the sizes accordingly. > > Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > --- > drivers/gpu/drm/drm_plane_helper.c | 79 ++++++++++++++++++++++++++++++++++-- > include/drm/drm_plane_helper.h | 3 +- > 2 files changed, 77 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c > index f24c4cf..4badd69 100644 > --- a/drivers/gpu/drm/drm_plane_helper.c > +++ b/drivers/gpu/drm/drm_plane_helper.c > @@ -138,9 +138,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane, > int max_scale, > bool can_position, > bool can_update_disabled, > - bool *visible) > + bool *visible, > + unsigned int rotation) > { > int hscale, vscale; > + int crtc_x, crtc_y; > + unsigned int crtc_w, crtc_h; > + uint32_t src_x, src_y, src_w, src_h; > > if (!fb) { > *visible = false; > @@ -158,9 +162,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane, > return -EINVAL; > } > > + if (fb) > + drm_rect_rotate(src, fb->width << 16, fb->height << 16, > + rotation); > + > /* Check scaling */ > - hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale); > - vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale); > + hscale = drm_rect_calc_hscale_relaxed(src, dest, min_scale, max_scale); > + vscale = drm_rect_calc_vscale_relaxed(src, dest, min_scale, max_scale); This is an unrelated change. Relaxed scaling allows the the src/dest rectangles to be reduced in size in order to keep the scaling ration within the min/max range. I suppose we should switch to using it to make the behaviour uniform across drivers, but definitely should be done with a separate patch. > if (hscale < 0 || vscale < 0) { > DRM_DEBUG_KMS("Invalid scaling of plane\n"); > return -ERANGE; > @@ -182,6 +190,68 @@ int drm_plane_helper_check_update(struct drm_plane *plane, > return -EINVAL; > } > > + crtc_x = dest->x1; > + crtc_y = dest->y1; > + crtc_w = drm_rect_width(dest); > + crtc_h = drm_rect_height(dest); You don't adjust these in any way so they are not needed. > + > + if (*visible) { > + /* check again in case clipping clamped the results */ > + hscale = drm_rect_calc_hscale(src, dest, > + DRM_PLANE_HELPER_NO_SCALING, > + DRM_PLANE_HELPER_NO_SCALING); First you allowed scaling and now you don't. What's up with that? > + if (hscale < 0) { > + DRM_DEBUG_KMS("Horizontal scaling factor out of limits\n"); > + drm_rect_debug_print(src, true); > + drm_rect_debug_print(dest, false); > + > + return hscale; > + } > + > + vscale = drm_rect_calc_vscale(src, dest, > + DRM_PLANE_HELPER_NO_SCALING, > + DRM_PLANE_HELPER_NO_SCALING); > + if (vscale < 0) { > + DRM_DEBUG_KMS("Vertical scaling factor out of limits\n"); > + drm_rect_debug_print(src, true); > + drm_rect_debug_print(dest, false); > + > + return vscale; > + } > + > + /* Make the source viewport size an exact multiple of the scaling factors. */ > + drm_rect_adjust_size(src, > + drm_rect_width(dest) * hscale - drm_rect_width(src), > + drm_rect_height(dest) * vscale - drm_rect_height(src)); > + > + drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16, > + rotation); > + > + /* > + * Hardware doesn't handle subpixel coordinates. That's a purely hardware specific detail. It should not be part of the helpers. And in any case the returned src coordinates must remain in fixed point format. > + * Adjust to (macro)pixel boundary, but be careful not to > + * increase the source viewport size, because that could > + * push the downscaling factor out of bounds. > + */ > + src_x = src->x1 >> 16; > + src_w = drm_rect_width(src) >> 16; > + src_y = src->y1 >> 16; > + src_h = drm_rect_height(src) >> 16; > + } > + > + if (*visible) { > + src->x1 = src_x; > + src->x2 = src_x + src_w; > + src->y1 = src_y; > + src->y2 = src_y + src_h; > + } > + > + dest->x1 = crtc_x; > + dest->x2 = crtc_x + crtc_w; > + dest->y1 = crtc_y; > + dest->y2 = crtc_y + crtc_h; > + > + > return 0; > } > EXPORT_SYMBOL(drm_plane_helper_check_update); > @@ -258,7 +328,8 @@ int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc, > &src, &dest, &clip, > DRM_PLANE_HELPER_NO_SCALING, > DRM_PLANE_HELPER_NO_SCALING, > - false, false, &visible); > + false, false, &visible, > + DRM_ROTATE_0); > if (ret) > return ret; > > diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h > index a185392..ef9eb04 100644 > --- a/include/drm/drm_plane_helper.h > +++ b/include/drm/drm_plane_helper.h > @@ -84,7 +84,8 @@ extern int drm_plane_helper_check_update(struct drm_plane *plane, > int max_scale, > bool can_position, > bool can_update_disabled, > - bool *visible); > + bool *visible, > + unsigned int rotation); > extern int drm_primary_helper_update(struct drm_plane *plane, > struct drm_crtc *crtc, > struct drm_framebuffer *fb, > -- > 1.7.10.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tuesday 13 January 2015 07:02 PM, Ville Syrjälä wrote: > On Tue, Jan 13, 2015 at 06:03:39PM +0530, Sonika Jindal wrote: >> Taking rotation into account while checking the plane >> and adjusting the sizes accordingly. >> >> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> >> --- >> drivers/gpu/drm/drm_plane_helper.c | 79 ++++++++++++++++++++++++++++++++++-- >> include/drm/drm_plane_helper.h | 3 +- >> 2 files changed, 77 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c >> index f24c4cf..4badd69 100644 >> --- a/drivers/gpu/drm/drm_plane_helper.c >> +++ b/drivers/gpu/drm/drm_plane_helper.c >> @@ -138,9 +138,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane, >> int max_scale, >> bool can_position, >> bool can_update_disabled, >> - bool *visible) >> + bool *visible, >> + unsigned int rotation) >> { >> int hscale, vscale; >> + int crtc_x, crtc_y; >> + unsigned int crtc_w, crtc_h; >> + uint32_t src_x, src_y, src_w, src_h; >> >> if (!fb) { >> *visible = false; >> @@ -158,9 +162,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane, >> return -EINVAL; >> } >> >> + if (fb) >> + drm_rect_rotate(src, fb->width << 16, fb->height << 16, >> + rotation); >> + >> /* Check scaling */ >> - hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale); >> - vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale); >> + hscale = drm_rect_calc_hscale_relaxed(src, dest, min_scale, max_scale); >> + vscale = drm_rect_calc_vscale_relaxed(src, dest, min_scale, max_scale); > This is an unrelated change. Relaxed scaling allows the the src/dest > rectangles to be reduced in size in order to keep the scaling ration > within the min/max range. I suppose we should switch to using it to > make the behaviour uniform across drivers, but definitely should be > done with a separate patch. Since, I added drm_rect_rotate before this, it changes the src sizes and it was giving me Invalid scaling if we don't let the sizes to be changed using _relaxed functions. I am trying this for 90/270 rotation. I can move it to a separate patch if required. >> if (hscale < 0 || vscale < 0) { >> DRM_DEBUG_KMS("Invalid scaling of plane\n"); >> return -ERANGE; >> @@ -182,6 +190,68 @@ int drm_plane_helper_check_update(struct drm_plane *plane, >> return -EINVAL; >> } >> >> + crtc_x = dest->x1; >> + crtc_y = dest->y1; >> + crtc_w = drm_rect_width(dest); >> + crtc_h = drm_rect_height(dest); > You don't adjust these in any way so they are not needed. > >> + >> + if (*visible) { >> + /* check again in case clipping clamped the results */ >> + hscale = drm_rect_calc_hscale(src, dest, >> + DRM_PLANE_HELPER_NO_SCALING, >> + DRM_PLANE_HELPER_NO_SCALING); > First you allowed scaling and now you don't. What's up with that? Hmm, so I can simply use min_scale and max_scale here. >> + if (hscale < 0) { >> + DRM_DEBUG_KMS("Horizontal scaling factor out of limits\n"); >> + drm_rect_debug_print(src, true); >> + drm_rect_debug_print(dest, false); >> + >> + return hscale; >> + } >> + >> + vscale = drm_rect_calc_vscale(src, dest, >> + DRM_PLANE_HELPER_NO_SCALING, >> + DRM_PLANE_HELPER_NO_SCALING); >> + if (vscale < 0) { >> + DRM_DEBUG_KMS("Vertical scaling factor out of limits\n"); >> + drm_rect_debug_print(src, true); >> + drm_rect_debug_print(dest, false); >> + >> + return vscale; >> + } >> + >> + /* Make the source viewport size an exact multiple of the scaling factors. */ >> + drm_rect_adjust_size(src, >> + drm_rect_width(dest) * hscale - drm_rect_width(src), >> + drm_rect_height(dest) * vscale - drm_rect_height(src)); >> + >> + drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16, >> + rotation); >> + >> + /* >> + * Hardware doesn't handle subpixel coordinates. > That's a purely hardware specific detail. It should not be part of the > helpers. And in any case the returned src coordinates must remain in > fixed point format. > I was just trying to make it same as what we do in intel_check_sprite_plane. So, looks like I can remove this one and the other part which you pointed above. >> + * Adjust to (macro)pixel boundary, but be careful not to >> + * increase the source viewport size, because that could >> + * push the downscaling factor out of bounds. >> + */ >> + src_x = src->x1 >> 16; >> + src_w = drm_rect_width(src) >> 16; >> + src_y = src->y1 >> 16; >> + src_h = drm_rect_height(src) >> 16; >> + } >> + >> + if (*visible) { >> + src->x1 = src_x; >> + src->x2 = src_x + src_w; >> + src->y1 = src_y; >> + src->y2 = src_y + src_h; >> + } >> + >> + dest->x1 = crtc_x; >> + dest->x2 = crtc_x + crtc_w; >> + dest->y1 = crtc_y; >> + dest->y2 = crtc_y + crtc_h; >> + >> + >> return 0; >> } >> EXPORT_SYMBOL(drm_plane_helper_check_update); >> @@ -258,7 +328,8 @@ int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc, >> &src, &dest, &clip, >> DRM_PLANE_HELPER_NO_SCALING, >> DRM_PLANE_HELPER_NO_SCALING, >> - false, false, &visible); >> + false, false, &visible, >> + DRM_ROTATE_0); >> if (ret) >> return ret; >> >> diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h >> index a185392..ef9eb04 100644 >> --- a/include/drm/drm_plane_helper.h >> +++ b/include/drm/drm_plane_helper.h >> @@ -84,7 +84,8 @@ extern int drm_plane_helper_check_update(struct drm_plane *plane, >> int max_scale, >> bool can_position, >> bool can_update_disabled, >> - bool *visible); >> + bool *visible, >> + unsigned int rotation); >> extern int drm_primary_helper_update(struct drm_plane *plane, >> struct drm_crtc *crtc, >> struct drm_framebuffer *fb, >> -- >> 1.7.10.4 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Jan 14, 2015 at 10:05:53AM +0530, sonika wrote: > > On Tuesday 13 January 2015 07:02 PM, Ville Syrjälä wrote: > > On Tue, Jan 13, 2015 at 06:03:39PM +0530, Sonika Jindal wrote: > >> Taking rotation into account while checking the plane > >> and adjusting the sizes accordingly. > >> > >> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > >> --- > >> drivers/gpu/drm/drm_plane_helper.c | 79 ++++++++++++++++++++++++++++++++++-- > >> include/drm/drm_plane_helper.h | 3 +- > >> 2 files changed, 77 insertions(+), 5 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c > >> index f24c4cf..4badd69 100644 > >> --- a/drivers/gpu/drm/drm_plane_helper.c > >> +++ b/drivers/gpu/drm/drm_plane_helper.c > >> @@ -138,9 +138,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane, > >> int max_scale, > >> bool can_position, > >> bool can_update_disabled, > >> - bool *visible) > >> + bool *visible, > >> + unsigned int rotation) > >> { > >> int hscale, vscale; > >> + int crtc_x, crtc_y; > >> + unsigned int crtc_w, crtc_h; > >> + uint32_t src_x, src_y, src_w, src_h; > >> > >> if (!fb) { > >> *visible = false; > >> @@ -158,9 +162,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane, > >> return -EINVAL; > >> } > >> > >> + if (fb) > >> + drm_rect_rotate(src, fb->width << 16, fb->height << 16, > >> + rotation); > >> + > >> /* Check scaling */ > >> - hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale); > >> - vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale); > >> + hscale = drm_rect_calc_hscale_relaxed(src, dest, min_scale, max_scale); > >> + vscale = drm_rect_calc_vscale_relaxed(src, dest, min_scale, max_scale); > > This is an unrelated change. Relaxed scaling allows the the src/dest > > rectangles to be reduced in size in order to keep the scaling ration > > within the min/max range. I suppose we should switch to using it to > > make the behaviour uniform across drivers, but definitely should be > > done with a separate patch. > Since, I added drm_rect_rotate before this, it changes the src sizes and > it was giving me > Invalid scaling if we don't let the sizes to be changed using _relaxed > functions. I am trying this > for 90/270 rotation. That would indicate a bug somewhere. Pontetially the bug could be in whatever test you're using as well. > I can move it to a separate patch if required. We nee to figure out why you got the error first.
We do exactly like this for sprite plane (ie, rotate the rect, then check scaling and adjust the size accordingly from drm_rect_calc_hscale_relaxed) That's why I saw the need of this for primary plane as well. For sprite plane 90 rotation, intel_check_sprite_plane does the adjustments and the rotated sizes are fine. But since we don't do any of those stuff for primary the destination size doesn't come right, and I get a little corrupted output after rotation. With this change, the rotated plane is properly adjusted in the viewport. So, I don't think it is a bug in test. -----Original Message----- From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] Sent: Wednesday, January 14, 2015 2:58 PM To: Jindal, Sonika Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to drm_plane_helper_check_update On Wed, Jan 14, 2015 at 10:05:53AM +0530, sonika wrote: > > On Tuesday 13 January 2015 07:02 PM, Ville Syrjälä wrote: > > On Tue, Jan 13, 2015 at 06:03:39PM +0530, Sonika Jindal wrote: > >> Taking rotation into account while checking the plane and adjusting > >> the sizes accordingly. > >> > >> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > >> --- > >> drivers/gpu/drm/drm_plane_helper.c | 79 ++++++++++++++++++++++++++++++++++-- > >> include/drm/drm_plane_helper.h | 3 +- > >> 2 files changed, 77 insertions(+), 5 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/drm_plane_helper.c > >> b/drivers/gpu/drm/drm_plane_helper.c > >> index f24c4cf..4badd69 100644 > >> --- a/drivers/gpu/drm/drm_plane_helper.c > >> +++ b/drivers/gpu/drm/drm_plane_helper.c > >> @@ -138,9 +138,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane, > >> int max_scale, > >> bool can_position, > >> bool can_update_disabled, > >> - bool *visible) > >> + bool *visible, > >> + unsigned int rotation) > >> { > >> int hscale, vscale; > >> + int crtc_x, crtc_y; > >> + unsigned int crtc_w, crtc_h; > >> + uint32_t src_x, src_y, src_w, src_h; > >> > >> if (!fb) { > >> *visible = false; > >> @@ -158,9 +162,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane, > >> return -EINVAL; > >> } > >> > >> + if (fb) > >> + drm_rect_rotate(src, fb->width << 16, fb->height << 16, > >> + rotation); > >> + > >> /* Check scaling */ > >> - hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale); > >> - vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale); > >> + hscale = drm_rect_calc_hscale_relaxed(src, dest, min_scale, max_scale); > >> + vscale = drm_rect_calc_vscale_relaxed(src, dest, min_scale, > >> +max_scale); > > This is an unrelated change. Relaxed scaling allows the the src/dest > > rectangles to be reduced in size in order to keep the scaling ration > > within the min/max range. I suppose we should switch to using it to > > make the behaviour uniform across drivers, but definitely should be > > done with a separate patch. > Since, I added drm_rect_rotate before this, it changes the src sizes > and it was giving me Invalid scaling if we don't let the sizes to be > changed using _relaxed functions. I am trying this for 90/270 > rotation. That would indicate a bug somewhere. Pontetially the bug could be in whatever test you're using as well. > I can move it to a separate patch if required. We nee to figure out why you got the error first. -- Ville Syrjälä Intel OTC
Since we do drm_rect_rotate with 90 rotation, the src->w changes to src->h. Now, when we call drm_rect_calc_hscale, the hscale calculated is lesser than the min_hscale (which is no_scaling by default), so it returns -ERANGE. So, I think we _relaxed is the function which should be called to update the destination size appropriately. Please correct me if I am wrong. -----Original Message----- From: Jindal, Sonika Sent: Wednesday, January 14, 2015 3:06 PM To: 'Ville Syrjälä' Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Subject: RE: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to drm_plane_helper_check_update We do exactly like this for sprite plane (ie, rotate the rect, then check scaling and adjust the size accordingly from drm_rect_calc_hscale_relaxed) That's why I saw the need of this for primary plane as well. For sprite plane 90 rotation, intel_check_sprite_plane does the adjustments and the rotated sizes are fine. But since we don't do any of those stuff for primary the destination size doesn't come right, and I get a little corrupted output after rotation. With this change, the rotated plane is properly adjusted in the viewport. So, I don't think it is a bug in test. -----Original Message----- From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] Sent: Wednesday, January 14, 2015 2:58 PM To: Jindal, Sonika Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to drm_plane_helper_check_update On Wed, Jan 14, 2015 at 10:05:53AM +0530, sonika wrote: > > On Tuesday 13 January 2015 07:02 PM, Ville Syrjälä wrote: > > On Tue, Jan 13, 2015 at 06:03:39PM +0530, Sonika Jindal wrote: > >> Taking rotation into account while checking the plane and adjusting > >> the sizes accordingly. > >> > >> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > >> --- > >> drivers/gpu/drm/drm_plane_helper.c | 79 ++++++++++++++++++++++++++++++++++-- > >> include/drm/drm_plane_helper.h | 3 +- > >> 2 files changed, 77 insertions(+), 5 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/drm_plane_helper.c > >> b/drivers/gpu/drm/drm_plane_helper.c > >> index f24c4cf..4badd69 100644 > >> --- a/drivers/gpu/drm/drm_plane_helper.c > >> +++ b/drivers/gpu/drm/drm_plane_helper.c > >> @@ -138,9 +138,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane, > >> int max_scale, > >> bool can_position, > >> bool can_update_disabled, > >> - bool *visible) > >> + bool *visible, > >> + unsigned int rotation) > >> { > >> int hscale, vscale; > >> + int crtc_x, crtc_y; > >> + unsigned int crtc_w, crtc_h; > >> + uint32_t src_x, src_y, src_w, src_h; > >> > >> if (!fb) { > >> *visible = false; > >> @@ -158,9 +162,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane, > >> return -EINVAL; > >> } > >> > >> + if (fb) > >> + drm_rect_rotate(src, fb->width << 16, fb->height << 16, > >> + rotation); > >> + > >> /* Check scaling */ > >> - hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale); > >> - vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale); > >> + hscale = drm_rect_calc_hscale_relaxed(src, dest, min_scale, max_scale); > >> + vscale = drm_rect_calc_vscale_relaxed(src, dest, min_scale, > >> +max_scale); > > This is an unrelated change. Relaxed scaling allows the the src/dest > > rectangles to be reduced in size in order to keep the scaling ration > > within the min/max range. I suppose we should switch to using it to > > make the behaviour uniform across drivers, but definitely should be > > done with a separate patch. > Since, I added drm_rect_rotate before this, it changes the src sizes > and it was giving me Invalid scaling if we don't let the sizes to be > changed using _relaxed functions. I am trying this for 90/270 > rotation. That would indicate a bug somewhere. Pontetially the bug could be in whatever test you're using as well. > I can move it to a separate patch if required. We nee to figure out why you got the error first. -- Ville Syrjälä Intel OTC
On Wed, Jan 14, 2015 at 09:49:36AM +0000, Jindal, Sonika wrote: > Since we do drm_rect_rotate with 90 rotation, the src->w changes to src->h. > Now, when we call drm_rect_calc_hscale, the hscale calculated is lesser than the min_hscale (which is no_scaling by default), so it returns -ERANGE. If you want no scaling then with 90/270 rotation then your src->w should be equal to dst->h. Then the calculated vscale will be 1.0. If it's not, then your test is passing in bad coordinates. > So, I think we _relaxed is the function which should be called to update the destination size appropriately. > Please correct me if I am wrong. > > -----Original Message----- > From: Jindal, Sonika > Sent: Wednesday, January 14, 2015 3:06 PM > To: 'Ville Syrjälä' > Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org > Subject: RE: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to drm_plane_helper_check_update > > We do exactly like this for sprite plane (ie, rotate the rect, then check scaling and adjust the size accordingly from drm_rect_calc_hscale_relaxed) That's why I saw the need of this for primary plane as well. > For sprite plane 90 rotation, intel_check_sprite_plane does the adjustments and the rotated sizes are fine. But since we don't do any of those stuff for primary the destination size doesn't come right, and I get a little corrupted output after rotation. > With this change, the rotated plane is properly adjusted in the viewport. > So, I don't think it is a bug in test. > > -----Original Message----- > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > Sent: Wednesday, January 14, 2015 2:58 PM > To: Jindal, Sonika > Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to drm_plane_helper_check_update > > On Wed, Jan 14, 2015 at 10:05:53AM +0530, sonika wrote: > > > > On Tuesday 13 January 2015 07:02 PM, Ville Syrjälä wrote: > > > On Tue, Jan 13, 2015 at 06:03:39PM +0530, Sonika Jindal wrote: > > >> Taking rotation into account while checking the plane and adjusting > > >> the sizes accordingly. > > >> > > >> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > > >> --- > > >> drivers/gpu/drm/drm_plane_helper.c | 79 ++++++++++++++++++++++++++++++++++-- > > >> include/drm/drm_plane_helper.h | 3 +- > > >> 2 files changed, 77 insertions(+), 5 deletions(-) > > >> > > >> diff --git a/drivers/gpu/drm/drm_plane_helper.c > > >> b/drivers/gpu/drm/drm_plane_helper.c > > >> index f24c4cf..4badd69 100644 > > >> --- a/drivers/gpu/drm/drm_plane_helper.c > > >> +++ b/drivers/gpu/drm/drm_plane_helper.c > > >> @@ -138,9 +138,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane, > > >> int max_scale, > > >> bool can_position, > > >> bool can_update_disabled, > > >> - bool *visible) > > >> + bool *visible, > > >> + unsigned int rotation) > > >> { > > >> int hscale, vscale; > > >> + int crtc_x, crtc_y; > > >> + unsigned int crtc_w, crtc_h; > > >> + uint32_t src_x, src_y, src_w, src_h; > > >> > > >> if (!fb) { > > >> *visible = false; > > >> @@ -158,9 +162,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane, > > >> return -EINVAL; > > >> } > > >> > > >> + if (fb) > > >> + drm_rect_rotate(src, fb->width << 16, fb->height << 16, > > >> + rotation); > > >> + > > >> /* Check scaling */ > > >> - hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale); > > >> - vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale); > > >> + hscale = drm_rect_calc_hscale_relaxed(src, dest, min_scale, max_scale); > > >> + vscale = drm_rect_calc_vscale_relaxed(src, dest, min_scale, > > >> +max_scale); > > > This is an unrelated change. Relaxed scaling allows the the src/dest > > > rectangles to be reduced in size in order to keep the scaling ration > > > within the min/max range. I suppose we should switch to using it to > > > make the behaviour uniform across drivers, but definitely should be > > > done with a separate patch. > > Since, I added drm_rect_rotate before this, it changes the src sizes > > and it was giving me Invalid scaling if we don't let the sizes to be > > changed using _relaxed functions. I am trying this for 90/270 > > rotation. > > That would indicate a bug somewhere. Pontetially the bug could be in whatever test you're using as well. > > > I can move it to a separate patch if required. > > We nee to figure out why you got the error first. > > -- > Ville Syrjälä > Intel OTC
I think I am confused here.. Since sprite plane handles this scaling and rotation in kernel, I thought it should be taken care in driver for primary as well. From the test, I don't really change the src sizes for primary as well as sprite to take care of 90/270 rotation. -----Original Message----- From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] Sent: Wednesday, January 14, 2015 5:14 PM To: Jindal, Sonika Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to drm_plane_helper_check_update On Wed, Jan 14, 2015 at 09:49:36AM +0000, Jindal, Sonika wrote: > Since we do drm_rect_rotate with 90 rotation, the src->w changes to src->h. > Now, when we call drm_rect_calc_hscale, the hscale calculated is lesser than the min_hscale (which is no_scaling by default), so it returns -ERANGE. If you want no scaling then with 90/270 rotation then your src->w should be equal to dst->h. Then the calculated vscale will be 1.0. If it's not, then your test is passing in bad coordinates. > So, I think we _relaxed is the function which should be called to update the destination size appropriately. > Please correct me if I am wrong. > > -----Original Message----- > From: Jindal, Sonika > Sent: Wednesday, January 14, 2015 3:06 PM > To: 'Ville Syrjälä' > Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org > Subject: RE: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to > drm_plane_helper_check_update > > We do exactly like this for sprite plane (ie, rotate the rect, then check scaling and adjust the size accordingly from drm_rect_calc_hscale_relaxed) That's why I saw the need of this for primary plane as well. > For sprite plane 90 rotation, intel_check_sprite_plane does the adjustments and the rotated sizes are fine. But since we don't do any of those stuff for primary the destination size doesn't come right, and I get a little corrupted output after rotation. > With this change, the rotated plane is properly adjusted in the viewport. > So, I don't think it is a bug in test. > > -----Original Message----- > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > Sent: Wednesday, January 14, 2015 2:58 PM > To: Jindal, Sonika > Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to > drm_plane_helper_check_update > > On Wed, Jan 14, 2015 at 10:05:53AM +0530, sonika wrote: > > > > On Tuesday 13 January 2015 07:02 PM, Ville Syrjälä wrote: > > > On Tue, Jan 13, 2015 at 06:03:39PM +0530, Sonika Jindal wrote: > > >> Taking rotation into account while checking the plane and > > >> adjusting the sizes accordingly. > > >> > > >> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > > >> --- > > >> drivers/gpu/drm/drm_plane_helper.c | 79 ++++++++++++++++++++++++++++++++++-- > > >> include/drm/drm_plane_helper.h | 3 +- > > >> 2 files changed, 77 insertions(+), 5 deletions(-) > > >> > > >> diff --git a/drivers/gpu/drm/drm_plane_helper.c > > >> b/drivers/gpu/drm/drm_plane_helper.c > > >> index f24c4cf..4badd69 100644 > > >> --- a/drivers/gpu/drm/drm_plane_helper.c > > >> +++ b/drivers/gpu/drm/drm_plane_helper.c > > >> @@ -138,9 +138,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane, > > >> int max_scale, > > >> bool can_position, > > >> bool can_update_disabled, > > >> - bool *visible) > > >> + bool *visible, > > >> + unsigned int rotation) > > >> { > > >> int hscale, vscale; > > >> + int crtc_x, crtc_y; > > >> + unsigned int crtc_w, crtc_h; > > >> + uint32_t src_x, src_y, src_w, src_h; > > >> > > >> if (!fb) { > > >> *visible = false; > > >> @@ -158,9 +162,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane, > > >> return -EINVAL; > > >> } > > >> > > >> + if (fb) > > >> + drm_rect_rotate(src, fb->width << 16, fb->height << 16, > > >> + rotation); > > >> + > > >> /* Check scaling */ > > >> - hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale); > > >> - vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale); > > >> + hscale = drm_rect_calc_hscale_relaxed(src, dest, min_scale, max_scale); > > >> + vscale = drm_rect_calc_vscale_relaxed(src, dest, min_scale, > > >> +max_scale); > > > This is an unrelated change. Relaxed scaling allows the the > > > src/dest rectangles to be reduced in size in order to keep the > > > scaling ration within the min/max range. I suppose we should > > > switch to using it to make the behaviour uniform across drivers, > > > but definitely should be done with a separate patch. > > Since, I added drm_rect_rotate before this, it changes the src sizes > > and it was giving me Invalid scaling if we don't let the sizes to be > > changed using _relaxed functions. I am trying this for 90/270 > > rotation. > > That would indicate a bug somewhere. Pontetially the bug could be in whatever test you're using as well. > > > I can move it to a separate patch if required. > > We nee to figure out why you got the error first. > > -- > Ville Syrjälä > Intel OTC -- Ville Syrjälä Intel OTC
On Wed, Jan 14, 2015 at 01:54:06PM +0000, Jindal, Sonika wrote: > I think I am confused here.. > Since sprite plane handles this scaling and rotation in kernel, I thought it should be taken care in driver for primary as well. Well, userspace still has to know how the src and dst coordinates relate to each other. Src coordinates are based on the memory layout. So src x=0,y=0 is the first pixel in memory. x=1,y=0 is the second pixel in memory etc. Dst coordinates are based on the display pipe. So dst x=0,y=0 is the first pixel that gets pushed out by the pipe for each frame. x=1,y=0 is the second pixel getting pushed out. Let's say we have a 4x2 FB and a display mode of 8x4. Then we want to use a sprite plane to present the FB with 0 and 90 degree rotation, and we want to position the sprite plane at coordinates 2,0 on the CRTC. It should look something like this: rotation=0 src: x1=0,y1=0,x2=4,y2=2 (w=4, h=2) dst: x1=2,y1=0,x2=6,y2=2 (w=4, h=2) FB: CRTC: 0,0 ---- 0,0 -------- |abcd| | abcd | |efgh| | efgh | ---- 4,2 | | | | -------- 8,4 rotation=90 src=0,0 -> 4,2 (w=4, h=2) dst=2,0 -> 4,4 (w=2, h=4) FB: CRTC: 0,0 ---- 0,0 -------- |abcd| | dh | |efgh| | cg | ---- 4,2 | bf | | ae | -------- 8,4 > >From the test, I don't really change the src sizes for primary as well as sprite to take care of 90/270 rotation. Sounds a bit buggy then. > > -----Original Message----- > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > Sent: Wednesday, January 14, 2015 5:14 PM > To: Jindal, Sonika > Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to drm_plane_helper_check_update > > On Wed, Jan 14, 2015 at 09:49:36AM +0000, Jindal, Sonika wrote: > > Since we do drm_rect_rotate with 90 rotation, the src->w changes to src->h. > > Now, when we call drm_rect_calc_hscale, the hscale calculated is lesser than the min_hscale (which is no_scaling by default), so it returns -ERANGE. > > If you want no scaling then with 90/270 rotation then your src->w should be equal to dst->h. Then the calculated vscale will be 1.0. If it's not, then your test is passing in bad coordinates. > > > So, I think we _relaxed is the function which should be called to update the destination size appropriately. > > Please correct me if I am wrong. > > > > -----Original Message----- > > From: Jindal, Sonika > > Sent: Wednesday, January 14, 2015 3:06 PM > > To: 'Ville Syrjälä' > > Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org > > Subject: RE: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to > > drm_plane_helper_check_update > > > > We do exactly like this for sprite plane (ie, rotate the rect, then check scaling and adjust the size accordingly from drm_rect_calc_hscale_relaxed) That's why I saw the need of this for primary plane as well. > > For sprite plane 90 rotation, intel_check_sprite_plane does the adjustments and the rotated sizes are fine. But since we don't do any of those stuff for primary the destination size doesn't come right, and I get a little corrupted output after rotation. > > With this change, the rotated plane is properly adjusted in the viewport. > > So, I don't think it is a bug in test. > > > > -----Original Message----- > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > > Sent: Wednesday, January 14, 2015 2:58 PM > > To: Jindal, Sonika > > Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org > > Subject: Re: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to > > drm_plane_helper_check_update > > > > On Wed, Jan 14, 2015 at 10:05:53AM +0530, sonika wrote: > > > > > > On Tuesday 13 January 2015 07:02 PM, Ville Syrjälä wrote: > > > > On Tue, Jan 13, 2015 at 06:03:39PM +0530, Sonika Jindal wrote: > > > >> Taking rotation into account while checking the plane and > > > >> adjusting the sizes accordingly. > > > >> > > > >> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > > > >> --- > > > >> drivers/gpu/drm/drm_plane_helper.c | 79 ++++++++++++++++++++++++++++++++++-- > > > >> include/drm/drm_plane_helper.h | 3 +- > > > >> 2 files changed, 77 insertions(+), 5 deletions(-) > > > >> > > > >> diff --git a/drivers/gpu/drm/drm_plane_helper.c > > > >> b/drivers/gpu/drm/drm_plane_helper.c > > > >> index f24c4cf..4badd69 100644 > > > >> --- a/drivers/gpu/drm/drm_plane_helper.c > > > >> +++ b/drivers/gpu/drm/drm_plane_helper.c > > > >> @@ -138,9 +138,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane, > > > >> int max_scale, > > > >> bool can_position, > > > >> bool can_update_disabled, > > > >> - bool *visible) > > > >> + bool *visible, > > > >> + unsigned int rotation) > > > >> { > > > >> int hscale, vscale; > > > >> + int crtc_x, crtc_y; > > > >> + unsigned int crtc_w, crtc_h; > > > >> + uint32_t src_x, src_y, src_w, src_h; > > > >> > > > >> if (!fb) { > > > >> *visible = false; > > > >> @@ -158,9 +162,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane, > > > >> return -EINVAL; > > > >> } > > > >> > > > >> + if (fb) > > > >> + drm_rect_rotate(src, fb->width << 16, fb->height << 16, > > > >> + rotation); > > > >> + > > > >> /* Check scaling */ > > > >> - hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale); > > > >> - vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale); > > > >> + hscale = drm_rect_calc_hscale_relaxed(src, dest, min_scale, max_scale); > > > >> + vscale = drm_rect_calc_vscale_relaxed(src, dest, min_scale, > > > >> +max_scale); > > > > This is an unrelated change. Relaxed scaling allows the the > > > > src/dest rectangles to be reduced in size in order to keep the > > > > scaling ration within the min/max range. I suppose we should > > > > switch to using it to make the behaviour uniform across drivers, > > > > but definitely should be done with a separate patch. > > > Since, I added drm_rect_rotate before this, it changes the src sizes > > > and it was giving me Invalid scaling if we don't let the sizes to be > > > changed using _relaxed functions. I am trying this for 90/270 > > > rotation. > > > > That would indicate a bug somewhere. Pontetially the bug could be in whatever test you're using as well. > > > > > I can move it to a separate patch if required. > > > > We nee to figure out why you got the error first. > > > > -- > > Ville Syrjälä > > Intel OTC > > -- > Ville Syrjälä > Intel OTC
So, if we do the source and dst adjustments in userspace, the logic in intel_check_sprite_plane will not hold good there. That's how it gets right coordinates and w,h for sprite for 90 rotation. I thought drm_rect_rotate and other functions are there to handle such scenarios. -----Original Message----- From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] Sent: Wednesday, January 14, 2015 11:38 PM To: Jindal, Sonika Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to drm_plane_helper_check_update On Wed, Jan 14, 2015 at 01:54:06PM +0000, Jindal, Sonika wrote: > I think I am confused here.. > Since sprite plane handles this scaling and rotation in kernel, I thought it should be taken care in driver for primary as well. Well, userspace still has to know how the src and dst coordinates relate to each other. Src coordinates are based on the memory layout. So src x=0,y=0 is the first pixel in memory. x=1,y=0 is the second pixel in memory etc. Dst coordinates are based on the display pipe. So dst x=0,y=0 is the first pixel that gets pushed out by the pipe for each frame. x=1,y=0 is the second pixel getting pushed out. Let's say we have a 4x2 FB and a display mode of 8x4. Then we want to use a sprite plane to present the FB with 0 and 90 degree rotation, and we want to position the sprite plane at coordinates 2,0 on the CRTC. It should look something like this: rotation=0 src: x1=0,y1=0,x2=4,y2=2 (w=4, h=2) dst: x1=2,y1=0,x2=6,y2=2 (w=4, h=2) FB: CRTC: 0,0 ---- 0,0 -------- |abcd| | abcd | |efgh| | efgh | ---- 4,2 | | | | -------- 8,4 rotation=90 src=0,0 -> 4,2 (w=4, h=2) dst=2,0 -> 4,4 (w=2, h=4) FB: CRTC: 0,0 ---- 0,0 -------- |abcd| | dh | |efgh| | cg | ---- 4,2 | bf | | ae | -------- 8,4 > >From the test, I don't really change the src sizes for primary as well as sprite to take care of 90/270 rotation. Sounds a bit buggy then. > > -----Original Message----- > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > Sent: Wednesday, January 14, 2015 5:14 PM > To: Jindal, Sonika > Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to > drm_plane_helper_check_update > > On Wed, Jan 14, 2015 at 09:49:36AM +0000, Jindal, Sonika wrote: > > Since we do drm_rect_rotate with 90 rotation, the src->w changes to src->h. > > Now, when we call drm_rect_calc_hscale, the hscale calculated is lesser than the min_hscale (which is no_scaling by default), so it returns -ERANGE. > > If you want no scaling then with 90/270 rotation then your src->w should be equal to dst->h. Then the calculated vscale will be 1.0. If it's not, then your test is passing in bad coordinates. > > > So, I think we _relaxed is the function which should be called to update the destination size appropriately. > > Please correct me if I am wrong. > > > > -----Original Message----- > > From: Jindal, Sonika > > Sent: Wednesday, January 14, 2015 3:06 PM > > To: 'Ville Syrjälä' > > Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org > > Subject: RE: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to > > drm_plane_helper_check_update > > > > We do exactly like this for sprite plane (ie, rotate the rect, then check scaling and adjust the size accordingly from drm_rect_calc_hscale_relaxed) That's why I saw the need of this for primary plane as well. > > For sprite plane 90 rotation, intel_check_sprite_plane does the adjustments and the rotated sizes are fine. But since we don't do any of those stuff for primary the destination size doesn't come right, and I get a little corrupted output after rotation. > > With this change, the rotated plane is properly adjusted in the viewport. > > So, I don't think it is a bug in test. > > > > -----Original Message----- > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > > Sent: Wednesday, January 14, 2015 2:58 PM > > To: Jindal, Sonika > > Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org > > Subject: Re: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to > > drm_plane_helper_check_update > > > > On Wed, Jan 14, 2015 at 10:05:53AM +0530, sonika wrote: > > > > > > On Tuesday 13 January 2015 07:02 PM, Ville Syrjälä wrote: > > > > On Tue, Jan 13, 2015 at 06:03:39PM +0530, Sonika Jindal wrote: > > > >> Taking rotation into account while checking the plane and > > > >> adjusting the sizes accordingly. > > > >> > > > >> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > > > >> --- > > > >> drivers/gpu/drm/drm_plane_helper.c | 79 ++++++++++++++++++++++++++++++++++-- > > > >> include/drm/drm_plane_helper.h | 3 +- > > > >> 2 files changed, 77 insertions(+), 5 deletions(-) > > > >> > > > >> diff --git a/drivers/gpu/drm/drm_plane_helper.c > > > >> b/drivers/gpu/drm/drm_plane_helper.c > > > >> index f24c4cf..4badd69 100644 > > > >> --- a/drivers/gpu/drm/drm_plane_helper.c > > > >> +++ b/drivers/gpu/drm/drm_plane_helper.c > > > >> @@ -138,9 +138,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane, > > > >> int max_scale, > > > >> bool can_position, > > > >> bool can_update_disabled, > > > >> - bool *visible) > > > >> + bool *visible, > > > >> + unsigned int rotation) > > > >> { > > > >> int hscale, vscale; > > > >> + int crtc_x, crtc_y; > > > >> + unsigned int crtc_w, crtc_h; > > > >> + uint32_t src_x, src_y, src_w, src_h; > > > >> > > > >> if (!fb) { > > > >> *visible = false; > > > >> @@ -158,9 +162,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane, > > > >> return -EINVAL; > > > >> } > > > >> > > > >> + if (fb) > > > >> + drm_rect_rotate(src, fb->width << 16, fb->height << 16, > > > >> + rotation); > > > >> + > > > >> /* Check scaling */ > > > >> - hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale); > > > >> - vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale); > > > >> + hscale = drm_rect_calc_hscale_relaxed(src, dest, min_scale, max_scale); > > > >> + vscale = drm_rect_calc_vscale_relaxed(src, dest, min_scale, > > > >> +max_scale); > > > > This is an unrelated change. Relaxed scaling allows the the > > > > src/dest rectangles to be reduced in size in order to keep the > > > > scaling ration within the min/max range. I suppose we should > > > > switch to using it to make the behaviour uniform across drivers, > > > > but definitely should be done with a separate patch. > > > Since, I added drm_rect_rotate before this, it changes the src > > > sizes and it was giving me Invalid scaling if we don't let the > > > sizes to be changed using _relaxed functions. I am trying this for > > > 90/270 rotation. > > > > That would indicate a bug somewhere. Pontetially the bug could be in whatever test you're using as well. > > > > > I can move it to a separate patch if required. > > > > We nee to figure out why you got the error first. > > > > -- > > Ville Syrjälä > > Intel OTC > > -- > Ville Syrjälä > Intel OTC -- Ville Syrjälä Intel OTC
On Thu, Jan 15, 2015 at 02:04:02AM +0000, Jindal, Sonika wrote: > So, if we do the source and dst adjustments in userspace, the logic in intel_check_sprite_plane will not hold good there. I'm not sure what adjustments you mean. Src says exactly which part of the FB you want to present to the user, rotation doesn't change that fact. Dst says exactly where on the screen you want the image to appear, again rotation doesn't change that fact. > That's how it gets right coordinates and w,h for sprite for 90 rotation. > I thought drm_rect_rotate and other functions are there to handle such scenarios. They are there mostly to make sure clipping happens correctly. They aren't there to magically fix userspace bugs. The only magicy part is the relaxed scaling and the rounding of coordinates based on hardware limits, and that stuff only exists so that it's easier to write userspace code without having to know the limitations of the hardware. > > -----Original Message----- > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > Sent: Wednesday, January 14, 2015 11:38 PM > To: Jindal, Sonika > Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to drm_plane_helper_check_update > > On Wed, Jan 14, 2015 at 01:54:06PM +0000, Jindal, Sonika wrote: > > I think I am confused here.. > > Since sprite plane handles this scaling and rotation in kernel, I thought it should be taken care in driver for primary as well. > > Well, userspace still has to know how the src and dst coordinates relate to each other. > > Src coordinates are based on the memory layout. So src x=0,y=0 is the first pixel in memory. x=1,y=0 is the second pixel in memory etc. > > Dst coordinates are based on the display pipe. So dst x=0,y=0 is the first pixel that gets pushed out by the pipe for each frame. x=1,y=0 is the second pixel getting pushed out. > > Let's say we have a 4x2 FB and a display mode of 8x4. Then we want to use a sprite plane to present the FB with 0 and 90 degree rotation, and we want to position the sprite plane at coordinates 2,0 on the CRTC. > It should look something like this: > > rotation=0 > src: x1=0,y1=0,x2=4,y2=2 (w=4, h=2) > dst: x1=2,y1=0,x2=6,y2=2 (w=4, h=2) > FB: CRTC: > 0,0 ---- 0,0 -------- > |abcd| | abcd | > |efgh| | efgh | > ---- 4,2 | | > | | > -------- 8,4 > > rotation=90 > src=0,0 -> 4,2 (w=4, h=2) > dst=2,0 -> 4,4 (w=2, h=4) > FB: CRTC: > 0,0 ---- 0,0 -------- > |abcd| | dh | > |efgh| | cg | > ---- 4,2 | bf | > | ae | > -------- 8,4 > > > >From the test, I don't really change the src sizes for primary as well as sprite to take care of 90/270 rotation. > > Sounds a bit buggy then. > > > > > -----Original Message----- > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > > Sent: Wednesday, January 14, 2015 5:14 PM > > To: Jindal, Sonika > > Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org > > Subject: Re: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to > > drm_plane_helper_check_update > > > > On Wed, Jan 14, 2015 at 09:49:36AM +0000, Jindal, Sonika wrote: > > > Since we do drm_rect_rotate with 90 rotation, the src->w changes to src->h. > > > Now, when we call drm_rect_calc_hscale, the hscale calculated is lesser than the min_hscale (which is no_scaling by default), so it returns -ERANGE. > > > > If you want no scaling then with 90/270 rotation then your src->w should be equal to dst->h. Then the calculated vscale will be 1.0. If it's not, then your test is passing in bad coordinates. > > > > > So, I think we _relaxed is the function which should be called to update the destination size appropriately. > > > Please correct me if I am wrong. > > > > > > -----Original Message----- > > > From: Jindal, Sonika > > > Sent: Wednesday, January 14, 2015 3:06 PM > > > To: 'Ville Syrjälä' > > > Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org > > > Subject: RE: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to > > > drm_plane_helper_check_update > > > > > > We do exactly like this for sprite plane (ie, rotate the rect, then check scaling and adjust the size accordingly from drm_rect_calc_hscale_relaxed) That's why I saw the need of this for primary plane as well. > > > For sprite plane 90 rotation, intel_check_sprite_plane does the adjustments and the rotated sizes are fine. But since we don't do any of those stuff for primary the destination size doesn't come right, and I get a little corrupted output after rotation. > > > With this change, the rotated plane is properly adjusted in the viewport. > > > So, I don't think it is a bug in test. > > > > > > -----Original Message----- > > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > > > Sent: Wednesday, January 14, 2015 2:58 PM > > > To: Jindal, Sonika > > > Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org > > > Subject: Re: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to > > > drm_plane_helper_check_update > > > > > > On Wed, Jan 14, 2015 at 10:05:53AM +0530, sonika wrote: > > > > > > > > On Tuesday 13 January 2015 07:02 PM, Ville Syrjälä wrote: > > > > > On Tue, Jan 13, 2015 at 06:03:39PM +0530, Sonika Jindal wrote: > > > > >> Taking rotation into account while checking the plane and > > > > >> adjusting the sizes accordingly. > > > > >> > > > > >> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > > > > >> --- > > > > >> drivers/gpu/drm/drm_plane_helper.c | 79 ++++++++++++++++++++++++++++++++++-- > > > > >> include/drm/drm_plane_helper.h | 3 +- > > > > >> 2 files changed, 77 insertions(+), 5 deletions(-) > > > > >> > > > > >> diff --git a/drivers/gpu/drm/drm_plane_helper.c > > > > >> b/drivers/gpu/drm/drm_plane_helper.c > > > > >> index f24c4cf..4badd69 100644 > > > > >> --- a/drivers/gpu/drm/drm_plane_helper.c > > > > >> +++ b/drivers/gpu/drm/drm_plane_helper.c > > > > >> @@ -138,9 +138,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane, > > > > >> int max_scale, > > > > >> bool can_position, > > > > >> bool can_update_disabled, > > > > >> - bool *visible) > > > > >> + bool *visible, > > > > >> + unsigned int rotation) > > > > >> { > > > > >> int hscale, vscale; > > > > >> + int crtc_x, crtc_y; > > > > >> + unsigned int crtc_w, crtc_h; > > > > >> + uint32_t src_x, src_y, src_w, src_h; > > > > >> > > > > >> if (!fb) { > > > > >> *visible = false; > > > > >> @@ -158,9 +162,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane, > > > > >> return -EINVAL; > > > > >> } > > > > >> > > > > >> + if (fb) > > > > >> + drm_rect_rotate(src, fb->width << 16, fb->height << 16, > > > > >> + rotation); > > > > >> + > > > > >> /* Check scaling */ > > > > >> - hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale); > > > > >> - vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale); > > > > >> + hscale = drm_rect_calc_hscale_relaxed(src, dest, min_scale, max_scale); > > > > >> + vscale = drm_rect_calc_vscale_relaxed(src, dest, min_scale, > > > > >> +max_scale); > > > > > This is an unrelated change. Relaxed scaling allows the the > > > > > src/dest rectangles to be reduced in size in order to keep the > > > > > scaling ration within the min/max range. I suppose we should > > > > > switch to using it to make the behaviour uniform across drivers, > > > > > but definitely should be done with a separate patch. > > > > Since, I added drm_rect_rotate before this, it changes the src > > > > sizes and it was giving me Invalid scaling if we don't let the > > > > sizes to be changed using _relaxed functions. I am trying this for > > > > 90/270 rotation. > > > > > > That would indicate a bug somewhere. Pontetially the bug could be in whatever test you're using as well. > > > > > > > I can move it to a separate patch if required. > > > > > > We nee to figure out why you got the error first. > > > > > > -- > > > Ville Syrjälä > > > Intel OTC > > > > -- > > Ville Syrjälä > > Intel OTC > > -- > Ville Syrjälä > Intel OTC
diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c index f24c4cf..4badd69 100644 --- a/drivers/gpu/drm/drm_plane_helper.c +++ b/drivers/gpu/drm/drm_plane_helper.c @@ -138,9 +138,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane, int max_scale, bool can_position, bool can_update_disabled, - bool *visible) + bool *visible, + unsigned int rotation) { int hscale, vscale; + int crtc_x, crtc_y; + unsigned int crtc_w, crtc_h; + uint32_t src_x, src_y, src_w, src_h; if (!fb) { *visible = false; @@ -158,9 +162,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane, return -EINVAL; } + if (fb) + drm_rect_rotate(src, fb->width << 16, fb->height << 16, + rotation); + /* Check scaling */ - hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale); - vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale); + hscale = drm_rect_calc_hscale_relaxed(src, dest, min_scale, max_scale); + vscale = drm_rect_calc_vscale_relaxed(src, dest, min_scale, max_scale); if (hscale < 0 || vscale < 0) { DRM_DEBUG_KMS("Invalid scaling of plane\n"); return -ERANGE; @@ -182,6 +190,68 @@ int drm_plane_helper_check_update(struct drm_plane *plane, return -EINVAL; } + crtc_x = dest->x1; + crtc_y = dest->y1; + crtc_w = drm_rect_width(dest); + crtc_h = drm_rect_height(dest); + + if (*visible) { + /* check again in case clipping clamped the results */ + hscale = drm_rect_calc_hscale(src, dest, + DRM_PLANE_HELPER_NO_SCALING, + DRM_PLANE_HELPER_NO_SCALING); + if (hscale < 0) { + DRM_DEBUG_KMS("Horizontal scaling factor out of limits\n"); + drm_rect_debug_print(src, true); + drm_rect_debug_print(dest, false); + + return hscale; + } + + vscale = drm_rect_calc_vscale(src, dest, + DRM_PLANE_HELPER_NO_SCALING, + DRM_PLANE_HELPER_NO_SCALING); + if (vscale < 0) { + DRM_DEBUG_KMS("Vertical scaling factor out of limits\n"); + drm_rect_debug_print(src, true); + drm_rect_debug_print(dest, false); + + return vscale; + } + + /* Make the source viewport size an exact multiple of the scaling factors. */ + drm_rect_adjust_size(src, + drm_rect_width(dest) * hscale - drm_rect_width(src), + drm_rect_height(dest) * vscale - drm_rect_height(src)); + + drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16, + rotation); + + /* + * Hardware doesn't handle subpixel coordinates. + * Adjust to (macro)pixel boundary, but be careful not to + * increase the source viewport size, because that could + * push the downscaling factor out of bounds. + */ + src_x = src->x1 >> 16; + src_w = drm_rect_width(src) >> 16; + src_y = src->y1 >> 16; + src_h = drm_rect_height(src) >> 16; + } + + if (*visible) { + src->x1 = src_x; + src->x2 = src_x + src_w; + src->y1 = src_y; + src->y2 = src_y + src_h; + } + + dest->x1 = crtc_x; + dest->x2 = crtc_x + crtc_w; + dest->y1 = crtc_y; + dest->y2 = crtc_y + crtc_h; + + return 0; } EXPORT_SYMBOL(drm_plane_helper_check_update); @@ -258,7 +328,8 @@ int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc, &src, &dest, &clip, DRM_PLANE_HELPER_NO_SCALING, DRM_PLANE_HELPER_NO_SCALING, - false, false, &visible); + false, false, &visible, + DRM_ROTATE_0); if (ret) return ret; diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h index a185392..ef9eb04 100644 --- a/include/drm/drm_plane_helper.h +++ b/include/drm/drm_plane_helper.h @@ -84,7 +84,8 @@ extern int drm_plane_helper_check_update(struct drm_plane *plane, int max_scale, bool can_position, bool can_update_disabled, - bool *visible); + bool *visible, + unsigned int rotation); extern int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc, struct drm_framebuffer *fb,
Taking rotation into account while checking the plane and adjusting the sizes accordingly. Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> --- drivers/gpu/drm/drm_plane_helper.c | 79 ++++++++++++++++++++++++++++++++++-- include/drm/drm_plane_helper.h | 3 +- 2 files changed, 77 insertions(+), 5 deletions(-)