| Message ID | 20181004202446.22905-18-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | atomic helper cleanup, take 2 | expand |
On Thu, Oct 04, 2018 at 10:24:42PM +0200, Daniel Vetter wrote: > It's for legacy drivers only (atomic ones should use > drm_atomic_helper_check_plane_state() instead), and there's no users > left except the one in the primary plane helpers. > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/drm_plane_helper.c | 49 +++++++----------------------- > include/drm/drm_plane_helper.h | 11 ------- > 2 files changed, 11 insertions(+), 49 deletions(-) > > diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c > index 965286231600..33b3e6892787 100644 > --- a/drivers/gpu/drm/drm_plane_helper.c > +++ b/drivers/gpu/drm/drm_plane_helper.c > @@ -100,43 +100,17 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc, > return count; > } > > -/** > - * drm_plane_helper_check_update() - Check 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 > - * @dst: integer destination coordinates > - * @rotation: plane rotation > - * @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 plane such that it > - * doesn't cover the entire crtc? This will generally > - * only be false for primary planes. > - * @can_update_disabled: can the plane be updated while the crtc > - * is disabled? > - * @visible: output parameter indicating whether plane is still visible after > - * clipping > - * > - * Checks that a desired plane update is valid. Drivers that provide > - * their own plane handling rather than helper-provided implementations 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_plane_helper_check_update(struct drm_plane *plane, > - struct drm_crtc *crtc, > - struct drm_framebuffer *fb, > - struct drm_rect *src, > - struct drm_rect *dst, > - unsigned int rotation, > - int min_scale, > - int max_scale, > - bool can_position, > - bool can_update_disabled, > - bool *visible) > +static int drm_plane_helper_check_update(struct drm_plane *plane, This patch makes the function static, which does not render the comment useless. It would make sense to keep the comment around, but maybe in a more dense format without all paramters explained. Maybe just a simple: /* * Checks that a desired plane update is valid. * Returns 0 if update appears valid, error code on failure */ Sam
On Thu, Oct 4, 2018 at 11:03 PM Sam Ravnborg <sam@ravnborg.org> wrote: > On Thu, Oct 04, 2018 at 10:24:42PM +0200, Daniel Vetter wrote: > > It's for legacy drivers only (atomic ones should use > > drm_atomic_helper_check_plane_state() instead), and there's no users > > left except the one in the primary plane helpers. > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > --- > > drivers/gpu/drm/drm_plane_helper.c | 49 +++++++----------------------- > > include/drm/drm_plane_helper.h | 11 ------- > > 2 files changed, 11 insertions(+), 49 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c > > index 965286231600..33b3e6892787 100644 > > --- a/drivers/gpu/drm/drm_plane_helper.c > > +++ b/drivers/gpu/drm/drm_plane_helper.c > > @@ -100,43 +100,17 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc, > > return count; > > } > > > > -/** > > - * drm_plane_helper_check_update() - Check 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 > > - * @dst: integer destination coordinates > > - * @rotation: plane rotation > > - * @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 plane such that it > > - * doesn't cover the entire crtc? This will generally > > - * only be false for primary planes. > > - * @can_update_disabled: can the plane be updated while the crtc > > - * is disabled? > > - * @visible: output parameter indicating whether plane is still visible after > > - * clipping > > - * > > - * Checks that a desired plane update is valid. Drivers that provide > > - * their own plane handling rather than helper-provided implementations 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_plane_helper_check_update(struct drm_plane *plane, > > - struct drm_crtc *crtc, > > - struct drm_framebuffer *fb, > > - struct drm_rect *src, > > - struct drm_rect *dst, > > - unsigned int rotation, > > - int min_scale, > > - int max_scale, > > - bool can_position, > > - bool can_update_disabled, > > - bool *visible) > > +static int drm_plane_helper_check_update(struct drm_plane *plane, > > This patch makes the function static, which does not render the comment useless. > It would make sense to keep the comment around, but maybe in a more dense > format without all paramters explained. > > Maybe just a simple: > /* > * Checks that a desired plane update is valid. > * Returns 0 if update appears valid, error code on failure > */ I felt like the function name sufficiently explains what it does. I do occasionally keep the kerneldoc when making a function static, but only when it explains something tricky. "People who work on drm core code" is a different audience from "people who write drm drivers". If we'd want comments like the above on all static functions in the drm core, someone would need to do a _lot_ of typing :-) -Daniel
diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c index 965286231600..33b3e6892787 100644 --- a/drivers/gpu/drm/drm_plane_helper.c +++ b/drivers/gpu/drm/drm_plane_helper.c @@ -100,43 +100,17 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc, return count; } -/** - * drm_plane_helper_check_update() - Check 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 - * @dst: integer destination coordinates - * @rotation: plane rotation - * @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 plane such that it - * doesn't cover the entire crtc? This will generally - * only be false for primary planes. - * @can_update_disabled: can the plane be updated while the crtc - * is disabled? - * @visible: output parameter indicating whether plane is still visible after - * clipping - * - * Checks that a desired plane update is valid. Drivers that provide - * their own plane handling rather than helper-provided implementations 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_plane_helper_check_update(struct drm_plane *plane, - struct drm_crtc *crtc, - struct drm_framebuffer *fb, - struct drm_rect *src, - struct drm_rect *dst, - unsigned int rotation, - int min_scale, - int max_scale, - bool can_position, - bool can_update_disabled, - bool *visible) +static int drm_plane_helper_check_update(struct drm_plane *plane, + struct drm_crtc *crtc, + struct drm_framebuffer *fb, + struct drm_rect *src, + struct drm_rect *dst, + unsigned int rotation, + int min_scale, + int max_scale, + bool can_position, + bool can_update_disabled, + bool *visible) { struct drm_plane_state plane_state = { .plane = plane, @@ -173,7 +147,6 @@ int drm_plane_helper_check_update(struct drm_plane *plane, return 0; } -EXPORT_SYMBOL(drm_plane_helper_check_update); /** * drm_primary_helper_update() - Helper for primary plane update diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h index 1999781781c7..7bff930b8b10 100644 --- a/include/drm/drm_plane_helper.h +++ b/include/drm/drm_plane_helper.h @@ -38,17 +38,6 @@ */ #define DRM_PLANE_HELPER_NO_SCALING (1<<16) -int drm_plane_helper_check_update(struct drm_plane *plane, - struct drm_crtc *crtc, - struct drm_framebuffer *fb, - struct drm_rect *src, - struct drm_rect *dest, - unsigned int rotation, - int min_scale, - int max_scale, - bool can_position, - bool can_update_disabled, - bool *visible); int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc, struct drm_framebuffer *fb,
It's for legacy drivers only (atomic ones should use drm_atomic_helper_check_plane_state() instead), and there's no users left except the one in the primary plane helpers. Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/drm_plane_helper.c | 49 +++++++----------------------- include/drm/drm_plane_helper.h | 11 ------- 2 files changed, 11 insertions(+), 49 deletions(-)