> -----Original Message-----
> From: Rob Clark [mailto:robdcl...@gmail.com]
> Sent: Friday, May 16, 2014 11:05 AM
> To: Lee, Chon Ming
> Cc: Roper, Matthew D; Intel Graphics Development; dri-
> de...@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 2/4] drm/plane-helper: Add
> drm_primary_helper_check_update() (v2)
>
> On Thu, May 15, 2014 at 10:51 PM, Lee, Chon Ming
> <chon.ming....@intel.com> wrote:
> > On 04/30 10:07, 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ä.
> >>
> >> v2:
> >> - Pass src/dest/clip rects and min/max scaling down to helper to avoid
> >> duplication of effort between helper and drivers (suggested by
> >> Ville).
> >> - Allow caller to specify whether the primary plane should be
> >> updatable while the crtc is disabled.
> >>
> >> Cc: Ville Syrjälä <ville.syrj...@linux.intel.com>
> >> Cc: dri-de...@lists.freedesktop.org
> >> Signed-off-by: Matt Roper <matthew.d.ro...@intel.com>
> >> ---
> >> drivers/gpu/drm/drm_plane_helper.c | 123
> ++++++++++++++++++++++++++++---------
> >> include/drm/drm_plane_helper.h | 24 +++++++-
> >> 2 files changed, 116 insertions(+), 31 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_plane_helper.c
> >> b/drivers/gpu/drm/drm_plane_helper.c
> >> index b601233..11e8b82 100644
> >> --- a/drivers/gpu/drm/drm_plane_helper.c
> >> +++ b/drivers/gpu/drm/drm_plane_helper.c
> >> @@ -26,6 +26,7 @@
> >> #include <linux/list.h>
> >> #include <drm/drmP.h>
> >> #include <drm/drm_rect.h>
> >> +#include <drm/drm_plane_helper.h>
> >>
> >> #define SUBPIXEL_MASK 0xffff
> >>
> >> @@ -66,6 +67,77 @@ 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
> >> + * @src: source coordinates in 16.16 fixed point
> >> + * @dest: integer destination coordinates
> >> + * @clip: integer clipping coordinates
> >> + * @min_scale: minimum @src:@dest scaling factor in 16.16 fixed
> >> +point
> >> + * @max_scale: maximum @src:@dest scaling factor in 16.16 fixed
> >> +point
> >> + * @can_position: is it legal to position the primary plane such that it
> >> + * doesn't cover the entire crtc?
> >> + * @can_update_disabled: can the primary plane be updated while the
> crtc
> >> + * is disabled?
> >> + * @visible: output parameter indicating whether plane is still visible
> after
> >> + * clipping
> >> + *
> >> + * 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,
> >> + struct drm_rect *src,
> >> + struct drm_rect *dest,
> >> + const struct drm_rect *clip,
> >> + int min_scale,
> >> + int max_scale,
> >> + bool can_position,
> >> + bool can_update_disabled,
> >> + bool *visible) {
> >> + int hscale, vscale;
> >> +
> >> + if (!crtc->enabled && !can_update_disabled) {
> >> + DRM_DEBUG_KMS("Cannot update primary plane of a disabled
> CRTC.\n");
> >> + return -EINVAL;
> >> + }
> >> +
> >> + /* Check scaling */
> >> + hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale);
> >> + vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale);
> >> + if (hscale < 0 || vscale < 0) {
> >> + DRM_DEBUG_KMS("Invalid scaling of primary plane\n");
> >> + return -ERANGE;
> >> + }
> >> +
> >> + *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;
> >> + }
> >
> > Cherryview display allow the primary plane to be position at any
> > location similiar to sprite plane for certain port. So, this shouldn't need
> > to
> check here.
> >
> > And the width/height doesn't need to cover the whole screen.
>
> In that case, wouldn't it make sense (at least when you want to expose those
> features) to *not* use primary plane helpers for that hw?
>
> IMHO, the primary plane helpers should be for "traditional" crtcs which do
> not have these features..
>
I don't have the usage model for that windowing the primary plane now. Let
say a primary plane at the beginning is using in traditional way, but if it
want to switch to window mode, it might not make sense to create another plane
to handle this right?
Regards,
Chon Ming
> BR,
> -R
>
> >
> >> +
> >> + 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,51 +185,42 @@ 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,
> >> + .y1 = src_y,
> >> + .x2 = src_x + src_w,
> >> + .y2 = src_y + src_h,
> >> + };
> >> struct drm_rect dest = {
> >> .x1 = crtc_x,
> >> .y1 = crtc_y,
> >> .x2 = crtc_x + crtc_w,
> >> .y2 = crtc_y + crtc_h,
> >> };
> >> - struct drm_rect clip = {
> >> + const struct drm_rect clip = {
> >> .x2 = crtc->mode.hdisplay,
> >> .y2 = crtc->mode.vdisplay,
> >> };
> >> struct drm_connector **connector_list;
> >> int num_connectors, ret;
> >> + bool visible;
> >>
> >> - 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,
> >> + &src, &dest, &clip,
> >> + DRM_PLANE_HELPER_NO_SCALING,
> >> + DRM_PLANE_HELPER_NO_SCALING,
> >> + false, false, &visible);
> >> if (ret)
> >> return ret;
> >>
> >> + 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..05e1357 100644
> >> --- a/include/drm/drm_plane_helper.h
> >> +++ b/include/drm/drm_plane_helper.h
> >> @@ -24,6 +24,17 @@
> >> #ifndef DRM_PLANE_HELPER_H
> >> #define DRM_PLANE_HELPER_H
> >>
> >> +#include <drm/drm_rect.h>
> >> +
> >> +/*
> >> + * Drivers that don't allow primary plane scaling may pass this
> >> +macro in place
> >> + * of the min/max scale parameters of the update checker function.
> >> + *
> >> + * Due to src being in 16.16 fixed point and dest being in integer
> >> +pixels,
> >> + * 1<<16 represents no scaling.
> >> + */
> >> +#define DRM_PLANE_HELPER_NO_SCALING (1<<16)
> >> +
> >> /**
> >> * DOC: plane helpers
> >> *
> >> @@ -31,6 +42,17 @@
> >> * planes.
> >> */
> >>
> >> +extern int drm_primary_helper_check_update(struct drm_plane *plane,
> >> + struct drm_crtc *crtc,
> >> + struct drm_framebuffer *fb,
> >> + struct drm_rect *src,
> >> + struct drm_rect *dest,
> >> + const struct drm_rect *clip,
> >> + int min_scale,
> >> + int max_scale,
> >> + bool can_position,
> >> + bool can_update_disabled,
> >> + bool *visible);
> >> extern int drm_primary_helper_update(struct drm_plane *plane,
> >> struct drm_crtc *crtc,
> >> struct drm_framebuffer *fb, @@
> >> -42,7 +64,7 @@ extern int drm_primary_helper_disable(struct drm_plane
> >> *plane); extern void drm_primary_helper_destroy(struct drm_plane
> >> *plane); extern const struct drm_plane_funcs
> >> drm_primary_helper_funcs; extern struct drm_plane
> *drm_primary_helper_create_plane(struct drm_device *dev,
> >> - uint32_t *formats,
> >> + const uint32_t
> >> + *formats,
> >> int
> >> num_formats);
> >>
> >>
> >> --
> >> 1.8.5.1
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
