| Message ID | 1398877623-16930-3-git-send-email-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.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 | 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. > + > + 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
On Thu, May 15, 2014 at 10:51 PM, Lee, Chon Ming <chon.ming.lee@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.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 | 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.. 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
> -----Original Message----- > From: Rob Clark [mailto:robdclark@gmail.com] > Sent: Friday, May 16, 2014 11:05 AM > To: Lee, Chon Ming > Cc: Roper, Matthew D; Intel Graphics Development; dri- > devel@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.lee@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.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 | 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
On Fri, May 16, 2014 at 7:25 AM, Lee, Chon Ming <chon.ming.lee@intel.com> wrote: >> > 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? I think what Rob means is that for now we should just use the primary plane helpers everywhere for easier conversion. Then later on we can enable the primary plane position feature on chv. So doing things step-by-step. -Daniel
On Fri, May 16, 2014 at 10:51:44AM +0800, Lee, Chon Ming wrote: ... > > +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. Right, but this is a general helper function in the DRM core that is meant to be usable on all hardware and on all vendors' drivers (including the simple primary planes that are automatically created by helper functions for drivers that don't provide their own primary plane support). The goal here is to centralize the common parameter checking in one place so that all drivers don't have to duplicate the same set of checks, but give a little bit of flexibility so that drivers for more feature-rich hardware can relax some of the restrictions that their hardware can actually handle (such as Cherryview being able to do primary plane windowing as you pointed out). It's true that the i915-specific implementation could be further extended to pass true for the 'can_position' parameter when running on Cherrytrail and then program the hardware accordingly, but that's really an extra feature beyond what I'm adding here; we'd want to add that as a follow-on patch later and come up with a whole extra set of tests to exercise it. I'd rather focus on getting this general i915 support here merged first, then go back and start enabling new hardware functionalities like that on the newer platforms that can handle it. I'll add Cherryview primary plane windowing to my TODO list for future work if nobody beats me to it (I think some of the guys in VPG may already be looking into this). Matt
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; + } + + 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);
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.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 | 123 ++++++++++++++++++++++++++++--------- include/drm/drm_plane_helper.h | 24 +++++++- 2 files changed, 116 insertions(+), 31 deletions(-)