| Message ID | 1398272641-4368-2-git-send-email-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Apr 23, 2014 at 10:04:00AM -0700, Matt Roper wrote: > Pull the parameter checking from drm_primary_helper_update() out into > its own function; drivers that provide their own setplane() > implementations rather than using the helper may still want to share > this parameter checking logic. > > A few of the checks here were also updated based on suggestions by > Ville Syrjälä. > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > --- > drivers/gpu/drm/drm_plane_helper.c | 148 +++++++++++++++++++++++++++++-------- > include/drm/drm_plane_helper.h | 9 +++ > 2 files changed, 128 insertions(+), 29 deletions(-) > > diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c > index 65c4a00..9bbbdf2 100644 > --- a/drivers/gpu/drm/drm_plane_helper.c > +++ b/drivers/gpu/drm/drm_plane_helper.c > @@ -66,6 +66,102 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc, > } > > /** > + * drm_primary_helper_check_update() - Check primary plane update for validity > + * @plane: plane object to update > + * @crtc: owning CRTC of owning plane > + * @fb: framebuffer to flip onto plane > + * @crtc_x: x offset of primary plane on crtc > + * @crtc_y: y offset of primary plane on crtc > + * @crtc_w: width of primary plane rectangle on crtc > + * @crtc_h: height of primary plane rectangle on crtc > + * @src_x: x offset of @fb for panning > + * @src_y: y offset of @fb for panning > + * @src_w: width of source rectangle in @fb > + * @src_h: height of source rectangle in @fb > + * @can_scale: is primary plane scaling legal? > + * @can_position: is it legal to position the primary plane such that it > + * doesn't cover the entire crtc? > + * > + * Checks that a desired primary plane update is valid. Drivers that provide > + * their own primary plane handling may still wish to call this function to > + * avoid duplication of error checking code. > + * > + * RETURNS: > + * Zero if update appears valid, error code on failure > + */ > +int drm_primary_helper_check_update(struct drm_plane *plane, > + struct drm_crtc *crtc, > + struct drm_framebuffer *fb, > + int crtc_x, int crtc_y, > + unsigned int crtc_w, unsigned int crtc_h, > + uint32_t src_x, uint32_t src_y, > + uint32_t src_w, uint32_t src_h, > + bool can_scale, > + bool can_position) > +{ > + struct drm_rect dest = { > + .x1 = crtc_x, > + .y1 = crtc_y, > + .x2 = crtc_x + crtc_w, > + .y2 = crtc_y + crtc_h, > + }; > + struct drm_rect src = { > + .x1 = src_x >> 16, > + .y1 = src_y >> 16, > + .x2 = (src_x + src_w) >> 16, > + .y2 = (src_y + src_h) >> 16, I think you want '(x>>16) + (y>>16)' instead. Otherwise you may end up increasing the source width/height. > + }; > + const struct drm_rect clip = { > + .x2 = crtc->mode.hdisplay, > + .y2 = crtc->mode.vdisplay, > + }; > + int hscale, vscale; > + int visible; > + > + if (!crtc->enabled) { > + DRM_DEBUG_KMS("Cannot update primary plane of a disabled CRTC.\n"); > + return -EINVAL; > + } We allow this for sprites, so I'd allow it for everything. I'd be fine with leaving this restriction in drm_primary_helper_update() simply because I have no interst in auditing every other driver. Although I think we still overwrite the primary plane configure during setcrtc. We should really change that so that the user can pre-configure all the planes and then watch them pop into view during a modeset as previously configured. > + > + /* setplane API takes shifted source rectangle values; unshift them */ > + src_x >>= 16; > + src_y >>= 16; > + src_w >>= 16; > + src_h >>= 16; > + > + /* check scaling */ > + if (!can_scale && (crtc_w != src_w || crtc_h != src_h)) { > + DRM_DEBUG_KMS("Can't scale primary plane\n"); > + return -EINVAL; > + } > + > + /* > + * Drivers that can scale should perform their own min/max scale > + * factor checks. > + */ > + hscale = drm_rect_calc_hscale(&src, &dest, 0, INT_MAX); > + vscale = drm_rect_calc_vscale(&src, &dest, 0, INT_MAX); > + visible = drm_rect_clip_scaled(&src, &dest, &clip, hscale, vscale); w/o sub-pixel coordinates the scaling factors will be truncated to integers, which makes the clipping rather bogus if the plane can actually scale. I think I'd just make this code assume that scaling isn't supported, and if the driver supports scaling it can just implent the appropriate code itself. We can try to refactor some of the scaling aware code from intel_sprite later if warranted. But I'm starting to question the usefulness of this function. We anyway have to reclip in i915 code due to stereo/doublewide/etc, and actually you also reclip in the plane helper since the clipped coordinates aren't passed back to the caller. In order to avoid the extra work, I'd just have the caller pass in all the drm_rects. That would actually make this function useful even for i915 since you could then pass the correct clip rect rather than assume that it comes from crtc->mode. > + if (!visible) > + /* > + * Primary plane isn't visible; some drivers can handle this > + * so we just return success here. Drivers that can't > + * (including those that use the primary plane helper's > + * update function) will return an error from their > + * update_plane handler. > + */ > + return 0; > + > + if (!can_position && !drm_rect_equals(&dest, &clip)) { > + DRM_DEBUG_KMS("Primary plane must cover entire CRTC\n"); > + return -EINVAL; > + } > + > + return 0; > +} > +EXPORT_SYMBOL(drm_primary_helper_check_update); > + > +/** > * drm_primary_helper_update() - Helper for primary plane update > * @plane: plane object to update > * @crtc: owning CRTC of owning plane > @@ -113,6 +209,12 @@ int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc, > .x = src_x >> 16, > .y = src_y >> 16, > }; > + struct drm_rect src = { > + .x1 = src_x >> 16, > + .y1 = src_y >> 16, > + .x2 = (src_x + src_w) >> 16, > + .y2 = (src_y + src_h) >> 16, > + }; > struct drm_rect dest = { > .x1 = crtc_x, > .y1 = crtc_y, > @@ -124,40 +226,28 @@ int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc, > .y2 = crtc->mode.vdisplay, > }; > struct drm_connector **connector_list; > + int visible; > + int hscale, vscale; > int num_connectors, ret; > > - if (!crtc->enabled) { > - DRM_DEBUG_KMS("Cannot update primary plane of a disabled CRTC.\n"); > - return -EINVAL; > - } > - > - /* Disallow subpixel positioning */ > - if ((src_x | src_y | src_w | src_h) & SUBPIXEL_MASK) { > - DRM_DEBUG_KMS("Primary plane does not support subpixel positioning\n"); > - return -EINVAL; > - } > - > - /* Disallow scaling */ > - src_w >>= 16; > - src_h >>= 16; > - if (crtc_w != src_w || crtc_h != src_h) { > - DRM_DEBUG_KMS("Can't scale primary plane\n"); > - return -EINVAL; > - } > - > - /* Make sure primary plane covers entire CRTC */ > - drm_rect_intersect(&dest, &clip); > - if (dest.x1 != 0 || dest.y1 != 0 || > - dest.x2 != crtc->mode.hdisplay || dest.y2 != crtc->mode.vdisplay) { > - DRM_DEBUG_KMS("Primary plane must cover entire CRTC\n"); > - return -EINVAL; > - } > - > - /* Framebuffer must be big enough to cover entire plane */ > - ret = drm_crtc_check_viewport(crtc, crtc_x, crtc_y, &crtc->mode, fb); > + ret = drm_primary_helper_check_update(plane, crtc, fb, > + crtc_x, crtc_y, crtc_w, crtc_h, > + src_x, src_y, src_w, src_h, > + false, false); > if (ret) > return ret; > > + hscale = drm_rect_calc_hscale(&src, &dest, 0, INT_MAX); > + vscale = drm_rect_calc_vscale(&src, &dest, 0, INT_MAX); > + visible = drm_rect_clip_scaled(&src, &dest, &clip, hscale, vscale); > + if (!visible) > + /* > + * Primary plane isn't visible. Note that unless a driver > + * provides their own disable function, this will just > + * wind up returning -EINVAL to userspace. > + */ > + return plane->funcs->disable_plane(plane); > + > /* Find current connectors for CRTC */ > num_connectors = get_connectors_for_crtc(crtc, NULL, 0); > BUG_ON(num_connectors == 0); > diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h > index 09824be..373201a 100644 > --- a/include/drm/drm_plane_helper.h > +++ b/include/drm/drm_plane_helper.h > @@ -31,6 +31,15 @@ > * planes. > */ > > +extern int drm_primary_helper_check_update(struct drm_plane *plane, > + struct drm_crtc *crtc, > + struct drm_framebuffer *fb, > + int crtc_x, int crtc_y, > + unsigned int crtc_w, unsigned int crtc_h, > + uint32_t src_x, uint32_t src_y, > + uint32_t src_w, uint32_t src_h, > + bool can_scale, > + bool can_position); > extern int drm_primary_helper_update(struct drm_plane *plane, > struct drm_crtc *crtc, > struct drm_framebuffer *fb, > -- > 1.8.5.1
diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c index 65c4a00..9bbbdf2 100644 --- a/drivers/gpu/drm/drm_plane_helper.c +++ b/drivers/gpu/drm/drm_plane_helper.c @@ -66,6 +66,102 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc, } /** + * drm_primary_helper_check_update() - Check primary plane update for validity + * @plane: plane object to update + * @crtc: owning CRTC of owning plane + * @fb: framebuffer to flip onto plane + * @crtc_x: x offset of primary plane on crtc + * @crtc_y: y offset of primary plane on crtc + * @crtc_w: width of primary plane rectangle on crtc + * @crtc_h: height of primary plane rectangle on crtc + * @src_x: x offset of @fb for panning + * @src_y: y offset of @fb for panning + * @src_w: width of source rectangle in @fb + * @src_h: height of source rectangle in @fb + * @can_scale: is primary plane scaling legal? + * @can_position: is it legal to position the primary plane such that it + * doesn't cover the entire crtc? + * + * Checks that a desired primary plane update is valid. Drivers that provide + * their own primary plane handling may still wish to call this function to + * avoid duplication of error checking code. + * + * RETURNS: + * Zero if update appears valid, error code on failure + */ +int drm_primary_helper_check_update(struct drm_plane *plane, + struct drm_crtc *crtc, + struct drm_framebuffer *fb, + int crtc_x, int crtc_y, + unsigned int crtc_w, unsigned int crtc_h, + uint32_t src_x, uint32_t src_y, + uint32_t src_w, uint32_t src_h, + bool can_scale, + bool can_position) +{ + struct drm_rect dest = { + .x1 = crtc_x, + .y1 = crtc_y, + .x2 = crtc_x + crtc_w, + .y2 = crtc_y + crtc_h, + }; + struct drm_rect src = { + .x1 = src_x >> 16, + .y1 = src_y >> 16, + .x2 = (src_x + src_w) >> 16, + .y2 = (src_y + src_h) >> 16, + }; + const struct drm_rect clip = { + .x2 = crtc->mode.hdisplay, + .y2 = crtc->mode.vdisplay, + }; + int hscale, vscale; + int visible; + + if (!crtc->enabled) { + DRM_DEBUG_KMS("Cannot update primary plane of a disabled CRTC.\n"); + return -EINVAL; + } + + /* setplane API takes shifted source rectangle values; unshift them */ + src_x >>= 16; + src_y >>= 16; + src_w >>= 16; + src_h >>= 16; + + /* check scaling */ + if (!can_scale && (crtc_w != src_w || crtc_h != src_h)) { + DRM_DEBUG_KMS("Can't scale primary plane\n"); + return -EINVAL; + } + + /* + * Drivers that can scale should perform their own min/max scale + * factor checks. + */ + hscale = drm_rect_calc_hscale(&src, &dest, 0, INT_MAX); + vscale = drm_rect_calc_vscale(&src, &dest, 0, INT_MAX); + visible = drm_rect_clip_scaled(&src, &dest, &clip, hscale, vscale); + if (!visible) + /* + * Primary plane isn't visible; some drivers can handle this + * so we just return success here. Drivers that can't + * (including those that use the primary plane helper's + * update function) will return an error from their + * update_plane handler. + */ + return 0; + + if (!can_position && !drm_rect_equals(&dest, &clip)) { + DRM_DEBUG_KMS("Primary plane must cover entire CRTC\n"); + return -EINVAL; + } + + return 0; +} +EXPORT_SYMBOL(drm_primary_helper_check_update); + +/** * drm_primary_helper_update() - Helper for primary plane update * @plane: plane object to update * @crtc: owning CRTC of owning plane @@ -113,6 +209,12 @@ int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc, .x = src_x >> 16, .y = src_y >> 16, }; + struct drm_rect src = { + .x1 = src_x >> 16, + .y1 = src_y >> 16, + .x2 = (src_x + src_w) >> 16, + .y2 = (src_y + src_h) >> 16, + }; struct drm_rect dest = { .x1 = crtc_x, .y1 = crtc_y, @@ -124,40 +226,28 @@ int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc, .y2 = crtc->mode.vdisplay, }; struct drm_connector **connector_list; + int visible; + int hscale, vscale; int num_connectors, ret; - if (!crtc->enabled) { - DRM_DEBUG_KMS("Cannot update primary plane of a disabled CRTC.\n"); - return -EINVAL; - } - - /* Disallow subpixel positioning */ - if ((src_x | src_y | src_w | src_h) & SUBPIXEL_MASK) { - DRM_DEBUG_KMS("Primary plane does not support subpixel positioning\n"); - return -EINVAL; - } - - /* Disallow scaling */ - src_w >>= 16; - src_h >>= 16; - if (crtc_w != src_w || crtc_h != src_h) { - DRM_DEBUG_KMS("Can't scale primary plane\n"); - return -EINVAL; - } - - /* Make sure primary plane covers entire CRTC */ - drm_rect_intersect(&dest, &clip); - if (dest.x1 != 0 || dest.y1 != 0 || - dest.x2 != crtc->mode.hdisplay || dest.y2 != crtc->mode.vdisplay) { - DRM_DEBUG_KMS("Primary plane must cover entire CRTC\n"); - return -EINVAL; - } - - /* Framebuffer must be big enough to cover entire plane */ - ret = drm_crtc_check_viewport(crtc, crtc_x, crtc_y, &crtc->mode, fb); + ret = drm_primary_helper_check_update(plane, crtc, fb, + crtc_x, crtc_y, crtc_w, crtc_h, + src_x, src_y, src_w, src_h, + false, false); if (ret) return ret; + hscale = drm_rect_calc_hscale(&src, &dest, 0, INT_MAX); + vscale = drm_rect_calc_vscale(&src, &dest, 0, INT_MAX); + visible = drm_rect_clip_scaled(&src, &dest, &clip, hscale, vscale); + if (!visible) + /* + * Primary plane isn't visible. Note that unless a driver + * provides their own disable function, this will just + * wind up returning -EINVAL to userspace. + */ + return plane->funcs->disable_plane(plane); + /* Find current connectors for CRTC */ num_connectors = get_connectors_for_crtc(crtc, NULL, 0); BUG_ON(num_connectors == 0); diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h index 09824be..373201a 100644 --- a/include/drm/drm_plane_helper.h +++ b/include/drm/drm_plane_helper.h @@ -31,6 +31,15 @@ * planes. */ +extern int drm_primary_helper_check_update(struct drm_plane *plane, + struct drm_crtc *crtc, + struct drm_framebuffer *fb, + int crtc_x, int crtc_y, + unsigned int crtc_w, unsigned int crtc_h, + uint32_t src_x, uint32_t src_y, + uint32_t src_w, uint32_t src_h, + bool can_scale, + bool can_position); extern int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc, struct drm_framebuffer *fb,
Pull the parameter checking from drm_primary_helper_update() out into its own function; drivers that provide their own setplane() implementations rather than using the helper may still want to share this parameter checking logic. A few of the checks here were also updated based on suggestions by Ville Syrjälä. Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: dri-devel@lists.freedesktop.org Signed-off-by: Matt Roper <matthew.d.roper@intel.com> --- drivers/gpu/drm/drm_plane_helper.c | 148 +++++++++++++++++++++++++++++-------- include/drm/drm_plane_helper.h | 9 +++ 2 files changed, 128 insertions(+), 29 deletions(-)