| Message ID | 1440516028-11127-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Aug 25, 2015 at 11:20 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > Using bool and returning true upon error is very uncommon. Also an int > return value is actually what all the callers which did check it seem > to have expected. > > v2: Restore hunk misplaced in a rebase, spotted by Rob. > > Cc: Rob Clark <robdclark@gmail.com> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> Reviewed-by: Rob Clark <robdclark@gmail.com> > --- > drivers/gpu/drm/drm_fb_helper.c | 19 +++++++++++-------- > include/drm/drm_fb_helper.h | 6 +++--- > 2 files changed, 14 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index 418d299f3b12..859134e0d72d 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -320,11 +320,10 @@ int drm_fb_helper_debug_leave(struct fb_info *info) > } > EXPORT_SYMBOL(drm_fb_helper_debug_leave); > > -static bool restore_fbdev_mode(struct drm_fb_helper *fb_helper) > +static int restore_fbdev_mode(struct drm_fb_helper *fb_helper) > { > struct drm_device *dev = fb_helper->dev; > struct drm_plane *plane; > - bool error = false; > int i; > > drm_warn_on_modeset_not_all_locked(dev); > @@ -348,14 +347,15 @@ static bool restore_fbdev_mode(struct drm_fb_helper *fb_helper) > if (crtc->funcs->cursor_set) { > ret = crtc->funcs->cursor_set(crtc, NULL, 0, 0, 0); > if (ret) > - error = true; > + return ret; > } > > ret = drm_mode_set_config_internal(mode_set); > if (ret) > - error = true; > + return ret; > } > - return error; > + > + return 0; > } > > /** > @@ -365,12 +365,15 @@ static bool restore_fbdev_mode(struct drm_fb_helper *fb_helper) > * This should be called from driver's drm ->lastclose callback > * when implementing an fbcon on top of kms using this helper. This ensures that > * the user isn't greeted with a black screen when e.g. X dies. > + * > + * RETURNS: > + * Zero if everything went ok, negative error code otherwise. > */ > -bool drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper) > +int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper) > { > struct drm_device *dev = fb_helper->dev; > - bool ret; > - bool do_delayed = false; > + bool do_delayed; > + int ret; > > drm_modeset_lock_all(dev); > ret = restore_fbdev_mode(fb_helper); > diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h > index dbab4622b58f..67de1f10008e 100644 > --- a/include/drm/drm_fb_helper.h > +++ b/include/drm/drm_fb_helper.h > @@ -136,7 +136,7 @@ int drm_fb_helper_set_par(struct fb_info *info); > int drm_fb_helper_check_var(struct fb_var_screeninfo *var, > struct fb_info *info); > > -bool drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper); > +int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper); > > struct fb_info *drm_fb_helper_alloc_fbi(struct drm_fb_helper *fb_helper); > void drm_fb_helper_unregister_fbi(struct drm_fb_helper *fb_helper); > @@ -226,10 +226,10 @@ static inline int drm_fb_helper_check_var(struct fb_var_screeninfo *var, > return 0; > } > > -static inline bool > +static inline int > drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper) > { > - return true; > + return 0; > } > > static inline struct fb_info * > -- > 1.8.3.1 >
On Tue, Aug 25, 2015 at 03:20:02PM -0400, Rob Clark wrote: > On Tue, Aug 25, 2015 at 11:20 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > Using bool and returning true upon error is very uncommon. Also an int > > return value is actually what all the callers which did check it seem > > to have expected. > > > > v2: Restore hunk misplaced in a rebase, spotted by Rob. > > > > Cc: Rob Clark <robdclark@gmail.com> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Reviewed-by: Rob Clark <robdclark@gmail.com> Merged the first 2 patches from this series to drm-misc, thanks for your review. -Daniel > > > > --- > > drivers/gpu/drm/drm_fb_helper.c | 19 +++++++++++-------- > > include/drm/drm_fb_helper.h | 6 +++--- > > 2 files changed, 14 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > > index 418d299f3b12..859134e0d72d 100644 > > --- a/drivers/gpu/drm/drm_fb_helper.c > > +++ b/drivers/gpu/drm/drm_fb_helper.c > > @@ -320,11 +320,10 @@ int drm_fb_helper_debug_leave(struct fb_info *info) > > } > > EXPORT_SYMBOL(drm_fb_helper_debug_leave); > > > > -static bool restore_fbdev_mode(struct drm_fb_helper *fb_helper) > > +static int restore_fbdev_mode(struct drm_fb_helper *fb_helper) > > { > > struct drm_device *dev = fb_helper->dev; > > struct drm_plane *plane; > > - bool error = false; > > int i; > > > > drm_warn_on_modeset_not_all_locked(dev); > > @@ -348,14 +347,15 @@ static bool restore_fbdev_mode(struct drm_fb_helper *fb_helper) > > if (crtc->funcs->cursor_set) { > > ret = crtc->funcs->cursor_set(crtc, NULL, 0, 0, 0); > > if (ret) > > - error = true; > > + return ret; > > } > > > > ret = drm_mode_set_config_internal(mode_set); > > if (ret) > > - error = true; > > + return ret; > > } > > - return error; > > + > > + return 0; > > } > > > > /** > > @@ -365,12 +365,15 @@ static bool restore_fbdev_mode(struct drm_fb_helper *fb_helper) > > * This should be called from driver's drm ->lastclose callback > > * when implementing an fbcon on top of kms using this helper. This ensures that > > * the user isn't greeted with a black screen when e.g. X dies. > > + * > > + * RETURNS: > > + * Zero if everything went ok, negative error code otherwise. > > */ > > -bool drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper) > > +int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper) > > { > > struct drm_device *dev = fb_helper->dev; > > - bool ret; > > - bool do_delayed = false; > > + bool do_delayed; > > + int ret; > > > > drm_modeset_lock_all(dev); > > ret = restore_fbdev_mode(fb_helper); > > diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h > > index dbab4622b58f..67de1f10008e 100644 > > --- a/include/drm/drm_fb_helper.h > > +++ b/include/drm/drm_fb_helper.h > > @@ -136,7 +136,7 @@ int drm_fb_helper_set_par(struct fb_info *info); > > int drm_fb_helper_check_var(struct fb_var_screeninfo *var, > > struct fb_info *info); > > > > -bool drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper); > > +int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper); > > > > struct fb_info *drm_fb_helper_alloc_fbi(struct drm_fb_helper *fb_helper); > > void drm_fb_helper_unregister_fbi(struct drm_fb_helper *fb_helper); > > @@ -226,10 +226,10 @@ static inline int drm_fb_helper_check_var(struct fb_var_screeninfo *var, > > return 0; > > } > > > > -static inline bool > > +static inline int > > drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper) > > { > > - return true; > > + return 0; > > } > > > > static inline struct fb_info * > > -- > > 1.8.3.1 > >
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 7254
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
ILK -2 302/302 300/302
SNB 315/315 315/315
IVB 336/336 336/336
BYT -1 283/283 282/283
HSW 378/378 378/378
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
*ILK igt@kms_flip@flip-vs-dpms-interruptible PASS(1) DMESG_WARN(1)
*ILK igt@kms_flip@wf_vblank-vs-modeset-interruptible PASS(1) DMESG_WARN(1)
*BYT igt@gem_tiled_partial_pwrite_pread@reads PASS(1) FAIL(1)
Note: You need to pay more attention to line start with '*'
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 418d299f3b12..859134e0d72d 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -320,11 +320,10 @@ int drm_fb_helper_debug_leave(struct fb_info *info) } EXPORT_SYMBOL(drm_fb_helper_debug_leave); -static bool restore_fbdev_mode(struct drm_fb_helper *fb_helper) +static int restore_fbdev_mode(struct drm_fb_helper *fb_helper) { struct drm_device *dev = fb_helper->dev; struct drm_plane *plane; - bool error = false; int i; drm_warn_on_modeset_not_all_locked(dev); @@ -348,14 +347,15 @@ static bool restore_fbdev_mode(struct drm_fb_helper *fb_helper) if (crtc->funcs->cursor_set) { ret = crtc->funcs->cursor_set(crtc, NULL, 0, 0, 0); if (ret) - error = true; + return ret; } ret = drm_mode_set_config_internal(mode_set); if (ret) - error = true; + return ret; } - return error; + + return 0; } /** @@ -365,12 +365,15 @@ static bool restore_fbdev_mode(struct drm_fb_helper *fb_helper) * This should be called from driver's drm ->lastclose callback * when implementing an fbcon on top of kms using this helper. This ensures that * the user isn't greeted with a black screen when e.g. X dies. + * + * RETURNS: + * Zero if everything went ok, negative error code otherwise. */ -bool drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper) +int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper) { struct drm_device *dev = fb_helper->dev; - bool ret; - bool do_delayed = false; + bool do_delayed; + int ret; drm_modeset_lock_all(dev); ret = restore_fbdev_mode(fb_helper); diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h index dbab4622b58f..67de1f10008e 100644 --- a/include/drm/drm_fb_helper.h +++ b/include/drm/drm_fb_helper.h @@ -136,7 +136,7 @@ int drm_fb_helper_set_par(struct fb_info *info); int drm_fb_helper_check_var(struct fb_var_screeninfo *var, struct fb_info *info); -bool drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper); +int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper); struct fb_info *drm_fb_helper_alloc_fbi(struct drm_fb_helper *fb_helper); void drm_fb_helper_unregister_fbi(struct drm_fb_helper *fb_helper); @@ -226,10 +226,10 @@ static inline int drm_fb_helper_check_var(struct fb_var_screeninfo *var, return 0; } -static inline bool +static inline int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper) { - return true; + return 0; } static inline struct fb_info *
Using bool and returning true upon error is very uncommon. Also an int return value is actually what all the callers which did check it seem to have expected. v2: Restore hunk misplaced in a rebase, spotted by Rob. Cc: Rob Clark <robdclark@gmail.com> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/drm_fb_helper.c | 19 +++++++++++-------- include/drm/drm_fb_helper.h | 6 +++--- 2 files changed, 14 insertions(+), 11 deletions(-)