On Wed, Apr 17, 2019 at 03:24:00PM +0200, Daniel Vetter wrote:
> On Tue, Apr 16, 2019 at 08:46:24PM +0200, Noralf Trønnes wrote:
> >
> >
> > Den 16.04.2019 09.59, skrev Daniel Vetter:
> > > On Sun, Apr 07, 2019 at 06:52:33PM +0200, Noralf Trønnes wrote:
> > >> drm_fb_helper_is_bound() is used to check if DRM userspace is in control.
> > >> This is done by looking at the fb on the primary plane. By the time
> > >> fb-helper gets around to committing, it's possible that the facts have
> > >> changed.
> > >>
> > >> Avoid this race by holding the drm_device->master_mutex lock while
> > >> committing. When DRM userspace does its first open, it will now wait
> > >> until fb-helper is done. The helper will stay away if there's a master.
> > >>
> > >> Locking rule: Always take the fb-helper lock first.
> > >>
> > >> v2:
> > >> - Remove drm_fb_helper_is_bound() (Daniel Vetter)
> > >> - No need to check fb_helper->dev->master in
> > >> drm_fb_helper_single_fb_probe(), restore_fbdev_mode() has the check.
> > >>
> > >> Suggested-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > >> Signed-off-by: Noralf Trønnes <noralf@xxxxxxxxxxx>
> > >> ---
> > >> drivers/gpu/drm/drm_auth.c | 20 ++++++++
> > >> drivers/gpu/drm/drm_fb_helper.c | 90 ++++++++++++++++-----------------
> > >> drivers/gpu/drm/drm_internal.h | 2 +
> > >> 3 files changed, 67 insertions(+), 45 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
> > >> index 1669c42c40ed..db199807b7dc 100644
> > >> --- a/drivers/gpu/drm/drm_auth.c
> > >> +++ b/drivers/gpu/drm/drm_auth.c
> > >> @@ -368,3 +368,23 @@ void drm_master_put(struct drm_master **master)
> > >> *master = NULL;
> > >> }
> > >> EXPORT_SYMBOL(drm_master_put);
> > >> +
> > >> +/* Used by drm_client and drm_fb_helper */
> > >> +bool drm_master_internal_acquire(struct drm_device *dev)
> > >> +{
> > >> + mutex_lock(&dev->master_mutex);
> > >> + if (dev->master) {
> > >> + mutex_unlock(&dev->master_mutex);
> > >> + return false;
> > >> + }
> > >> +
> > >> + return true;
> > >> +}
> > >> +EXPORT_SYMBOL(drm_master_internal_acquire);
> > >> +
> > >> +/* Used by drm_client and drm_fb_helper */
> > >> +void drm_master_internal_release(struct drm_device *dev)
> > >> +{
> > >> + mutex_unlock(&dev->master_mutex);
> > >> +}
> > >> +EXPORT_SYMBOL(drm_master_internal_release);
> > >> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > >> index 84791dd4a90d..a6be09ae899b 100644
> > >> --- a/drivers/gpu/drm/drm_fb_helper.c
> > >> +++ b/drivers/gpu/drm/drm_fb_helper.c
> > >> @@ -44,6 +44,7 @@
> > >>
> > >> #include "drm_crtc_internal.h"
> > >> #include "drm_crtc_helper_internal.h"
> > >> +#include "drm_internal.h"
> > >>
> > >> static bool drm_fbdev_emulation = true;
> > >> module_param_named(fbdev_emulation, drm_fbdev_emulation, bool, 0600);
> > >> @@ -509,7 +510,7 @@ static int restore_fbdev_mode_legacy(struct drm_fb_helper *fb_helper)
> > >> return ret;
> > >> }
> > >>
> > >> -static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)
> > >> +static int restore_fbdev_mode_force(struct drm_fb_helper *fb_helper)
> > >
> > > Bikeshed: usually the function variant that's run with locks already taken
> > > is called _locked or has a __ prefix. _force feels a bit misplaced.
> >
> > This isn't a _locked function in the usual sense, it is: apply modeset
> > even if there is a DRM master. So we are _forcing a modeset on a
> > possibly unexpecting DRM userspace. To me a _locked function would imply
> > that the caller _must_ take a lock in order to use it.
>
> Hm, good point. See my comments later on, I'm not sure the "forcing" is
> really what we want. I think lastclose would be much better served if it
> also checks for master status.
>
> > But no big deal, I can rename it _locked if that reads better. After a
> > few years of reading kernel code I've come to appreciate the consistency
> > in how things are done and named. Every time things are different it
> > slows down my internal logic/pattern parser.
>
> If you agree with the entire "we should _force" then I think the __ prefix
> would fit. It's the general "beware, this is special/internal" annotation.
> My _locked suggestion was under the assumption that there's never a real
> case where we want to do the unprotected modeset for an internal
> drm_client.
>
> > >> {
> > >> struct drm_device *dev = fb_helper->dev;
> > >>
> > >> @@ -519,6 +520,21 @@ static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)
> > >> return restore_fbdev_mode_legacy(fb_helper);
> > >> }
> > >>
> > >> +static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)
> > >> +{
> > >> + struct drm_device *dev = fb_helper->dev;
> > >> + int ret;
> > >> +
> > >> + if (!drm_master_internal_acquire(dev))
> > >> + return -EBUSY;
> > >> +
> > >> + ret = restore_fbdev_mode_force(fb_helper);
> > >> +
> > >> + drm_master_internal_release(dev);
> > >> +
> > >> + return ret;
> > >> +}
> > >> +
> > >> /**
> > >> * drm_fb_helper_restore_fbdev_mode_unlocked - restore fbdev configuration
> > >> * @fb_helper: driver-allocated fbdev helper, can be NULL
> > >> @@ -556,34 +572,6 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
> > >> }
> > >> EXPORT_SYMBOL(drm_fb_helper_restore_fbdev_mode_unlocked);
> > >>
> > >> -static bool drm_fb_helper_is_bound(struct drm_fb_helper *fb_helper)
> > >> -{
> > >> - struct drm_device *dev = fb_helper->dev;
> > >> - struct drm_crtc *crtc;
> > >> - int bound = 0, crtcs_bound = 0;
> > >> -
> > >> - /*
> > >> - * Sometimes user space wants everything disabled, so don't steal the
> > >> - * display if there's a master.
> > >> - */
> > >> - if (READ_ONCE(dev->master))
> > >> - return false;
> > >> -
> > >> - drm_for_each_crtc(crtc, dev) {
> > >> - drm_modeset_lock(&crtc->mutex, NULL);
> > >> - if (crtc->primary->fb)
> > >> - crtcs_bound++;
> > >> - if (crtc->primary->fb == fb_helper->fb)
> > >> - bound++;
> > >> - drm_modeset_unlock(&crtc->mutex);
> > >> - }
> > >> -
> > >> - if (bound < crtcs_bound)
> > >> - return false;
> > >> -
> > >> - return true;
> > >> -}
> > >> -
> > >> #ifdef CONFIG_MAGIC_SYSRQ
> > >> /*
> > >> * restore fbcon display for all kms driver's using this helper, used for sysrq
> > >> @@ -604,7 +592,7 @@ static bool drm_fb_helper_force_kernel_mode(void)
> > >> continue;
> > >>
> > >> mutex_lock(&helper->lock);
> > >> - ret = restore_fbdev_mode(helper);
> > >> + ret = restore_fbdev_mode_force(helper);
> > >
> > > I'd leave this as-is, because:
> > > a) I'm too lazy to review the locking of our open/close calls to convince
> > > myself that lastclose can't race with the next open
> > > b) it won't hurt
> > > c) leaves the door open to easily make our open/close more concurrent in
> > > the future
> > >
> >
> > I'm not actually changing anything here, it's restore_fbdev_mode() that
> > has changed, it now bails out if there's a DRM master. If we don't
> > change this to _force/_locked, then sysrq won't work if there's a DRM
> > master. Which kind of defeats the whole idea of this 'give me fbcon
> > right now' functionality doesn't it?
>
> Uh, I was blind, I thought this was for lastclose. With this one here I
> think _force makes sense. I still think that the main drm_client_modeset
> interface should be the normal one which checks for master for you.
Ok, I read the later patches and you already have a commit() and a
commit_force(). Count me convinced on all points.
Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
as-is on this patch.
Cheers, Daniel
>
> > >> if (ret)
> > >> error = true;
> > >> mutex_unlock(&helper->lock);
> > >> @@ -663,20 +651,22 @@ static void dpms_legacy(struct drm_fb_helper *fb_helper, int dpms_mode)
> > >> static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
> > >> {
> > >> struct drm_fb_helper *fb_helper = info->par;
> > >> + struct drm_device *dev = fb_helper->dev;
> > >>
> > >> /*
> > >> * For each CRTC in this fb, turn the connectors on/off.
> > >> */
> > >> mutex_lock(&fb_helper->lock);
> > >> - if (!drm_fb_helper_is_bound(fb_helper)) {
> > >> - mutex_unlock(&fb_helper->lock);
> > >> - return;
> > >> - }
> > >> + if (!drm_master_internal_acquire(dev))
> > >> + goto unlock;
> > >>
> > >> - if (drm_drv_uses_atomic_modeset(fb_helper->dev))
> > >> + if (drm_drv_uses_atomic_modeset(dev))
> > >> restore_fbdev_mode_atomic(fb_helper, dpms_mode == DRM_MODE_DPMS_ON);
> > >> else
> > >> dpms_legacy(fb_helper, dpms_mode);
> > >> +
> > >> + drm_master_internal_release(dev);
> > >> +unlock:
> > >> mutex_unlock(&fb_helper->lock);
> > >> }
> > >>
> > >> @@ -1509,6 +1499,7 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info)
> > >> int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
> > >> {
> > >> struct drm_fb_helper *fb_helper = info->par;
> > >> + struct drm_device *dev = fb_helper->dev;
> > >> int ret;
> > >>
> > >> if (oops_in_progress)
> > >> @@ -1516,9 +1507,9 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
> > >>
> > >> mutex_lock(&fb_helper->lock);
> > >>
> > >> - if (!drm_fb_helper_is_bound(fb_helper)) {
> > >> + if (!drm_master_internal_acquire(dev)) {
> > >> ret = -EBUSY;
> > >> - goto out;
> > >> + goto unlock;
> > >> }
> > >>
> > >> if (info->fix.visual == FB_VISUAL_TRUECOLOR)
> > >> @@ -1528,7 +1519,8 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
> > >> else
> > >> ret = setcmap_legacy(cmap, info);
> > >>
> > >> -out:
> > >> + drm_master_internal_release(dev);
> > >> +unlock:
> > >> mutex_unlock(&fb_helper->lock);
> > >>
> > >> return ret;
> > >> @@ -1548,12 +1540,13 @@ int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd,
> > >> unsigned long arg)
> > >> {
> > >> struct drm_fb_helper *fb_helper = info->par;
> > >> + struct drm_device *dev = fb_helper->dev;
> > >> struct drm_mode_set *mode_set;
> > >> struct drm_crtc *crtc;
> > >> int ret = 0;
> > >>
> > >> mutex_lock(&fb_helper->lock);
> > >> - if (!drm_fb_helper_is_bound(fb_helper)) {
> > >> + if (!drm_master_internal_acquire(dev)) {
> > >> ret = -EBUSY;
> > >> goto unlock;
> > >> }
> > >> @@ -1591,11 +1584,12 @@ int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd,
> > >> }
> > >>
> > >> ret = 0;
> > >> - goto unlock;
> > >> + break;
> > >> default:
> > >> ret = -ENOTTY;
> > >> }
> > >>
> > >> + drm_master_internal_release(dev);
> > >> unlock:
> > >> mutex_unlock(&fb_helper->lock);
> > >> return ret;
> > >> @@ -1847,15 +1841,18 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var,
> > >> return -EBUSY;
> > >>
> > >> mutex_lock(&fb_helper->lock);
> > >> - if (!drm_fb_helper_is_bound(fb_helper)) {
> > >> - mutex_unlock(&fb_helper->lock);
> > >> - return -EBUSY;
> > >> + if (!drm_master_internal_acquire(dev)) {
> > >> + ret = -EBUSY;
> > >> + goto unlock;
> > >> }
> > >>
> > >> if (drm_drv_uses_atomic_modeset(dev))
> > >> ret = pan_display_atomic(var, info);
> > >> else
> > >> ret = pan_display_legacy(var, info);
> > >> +
> > >> + drm_master_internal_release(dev);
> > >> +unlock:
> > >> mutex_unlock(&fb_helper->lock);
> > >>
> > >> return ret;
> > >> @@ -2014,7 +2011,7 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
> > >> DRM_INFO("Cannot find any crtc or sizes\n");
> > >>
> > >> /* First time: disable all crtc's.. */
> > >> - if (!fb_helper->deferred_setup && !READ_ONCE(fb_helper->dev->master))
> > >> + if (!fb_helper->deferred_setup)
> > >> restore_fbdev_mode(fb_helper);
> > >
> > > I think we need to return the errno here, since without that the higher
> > > levels won't reprobe correctly. Plus we need to remap -EBUSY to -EAGAIN
> > > (or change the check in __drm_fb_helper_initial_config_and_unlock to also
> > > retry on -EBUSY).
> > >
> >
> > I don't think so, because -EAGAIN is returned unconditionally on the
> > line below. The restore_fbdev_mode() call is just to disable the outputs
> > _if_ there's no DRM master, which that function now checks for.
>
> Hm right, my logic parser went *boink* I think. Looks good on 2nd reading.
> -Daniel
> >
> > Noralf.
> >
> > >> return -EAGAIN;
> > >> }
> > >> @@ -2842,6 +2839,7 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config);
> > >> */
> > >> int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
> > >> {
> > >> + struct drm_device *dev = fb_helper->dev;
> > >> int err = 0;
> > >>
> > >> if (!drm_fbdev_emulation || !fb_helper)
> > >> @@ -2854,12 +2852,14 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
> > >> return err;
> > >> }
> > >>
> > >> - if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
> > >> + if (!fb_helper->fb || !drm_master_internal_acquire(dev)) {
> > >> fb_helper->delayed_hotplug = true;
> > >> mutex_unlock(&fb_helper->lock);
> > >> return err;
> > >> }
> > >>
> > >> + drm_master_internal_release(dev);
> > >> +
> > >> DRM_DEBUG_KMS("\n");
> > >>
> > >> drm_setup_crtcs(fb_helper, fb_helper->fb->width, fb_helper->fb->height);
> > >> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> > >> index d9a483a5fce0..3ee97c9998a2 100644
> > >> --- a/drivers/gpu/drm/drm_internal.h
> > >> +++ b/drivers/gpu/drm/drm_internal.h
> > >> @@ -91,6 +91,8 @@ int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
> > >> struct drm_file *file_priv);
> > >> int drm_master_open(struct drm_file *file_priv);
> > >> void drm_master_release(struct drm_file *file_priv);
> > >> +bool drm_master_internal_acquire(struct drm_device *dev);
> > >> +void drm_master_internal_release(struct drm_device *dev);
> > >>
> > >> /* drm_sysfs.c */
> > >> extern struct class *drm_class;
> > >
> > > With the nits addressed:
> > >
> > > Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > >
> > >> --
> > >> 2.20.1
> > >>
> > >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
