Re: [PATCH v2 02/12] drm/fb-helper: Avoid race with DRM userspace
Den 17.04.2019 15.26, skrev Daniel Vetter: > 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 > Signed-off-by: Noralf Trønnes > --- > 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 > > as-is on this patch. > Thanks. I want to apply this after the for-5.2 cutoff so we can get some testing on this before it hits the world. I know from the timeline chart that the cutoff is some time after -rc5, but I don't know _excatly_ when that is. I looked at dim, but all I could glean from it was that it had something to do with the state of a -fixes branch. Noralf. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 02/12] drm/fb-helper: Avoid race with DRM userspace
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
> > >> Signed-off-by: Noralf Trønnes
> > >> ---
> > >> 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(>master_mutex);
> > >> +if (dev->master) {
> > >> +mutex_unlock(>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(>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 =
Re: [PATCH v2 02/12] drm/fb-helper: Avoid race with DRM userspace
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
> >> Signed-off-by: Noralf Trønnes
> >> ---
> >> 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(>master_mutex);
> >> + if (dev->master) {
> >> + mutex_unlock(>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(>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)
> >> }
> >>
Re: [PATCH v2 02/12] drm/fb-helper: Avoid race with DRM userspace
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
>> Signed-off-by: Noralf Trønnes
>> ---
>> 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(>master_mutex);
>> +if (dev->master) {
>> +mutex_unlock(>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(>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.
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.
>> {
>> 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(>mutex, NULL);
>> -if (crtc->primary->fb)
>> -crtcs_bound++;
>> -if (crtc->primary->fb == fb_helper->fb)
>> -bound++;
>> -
Re: [PATCH v2 02/12] drm/fb-helper: Avoid race with DRM userspace
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 > Signed-off-by: Noralf Trønnes With the changes asked by Daniel, Reviewed-by: Maxime Ripard Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 02/12] drm/fb-helper: Avoid race with DRM userspace
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
> Signed-off-by: Noralf Trønnes
> ---
> 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(>master_mutex);
> + if (dev->master) {
> + mutex_unlock(>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(>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.
> {
> 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(>mutex, NULL);
> - if (crtc->primary->fb)
> - crtcs_bound++;
> - if (crtc->primary->fb == fb_helper->fb)
> - bound++;
> - drm_modeset_unlock(>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(>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
[PATCH v2 02/12] drm/fb-helper: Avoid race with DRM userspace
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
Signed-off-by: Noralf Trønnes
---
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(>master_mutex);
+ if (dev->master) {
+ mutex_unlock(>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(>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)
{
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(>mutex, NULL);
- if (crtc->primary->fb)
- crtcs_bound++;
- if (crtc->primary->fb == fb_helper->fb)
- bound++;
- drm_modeset_unlock(>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(>lock);
- ret = restore_fbdev_mode(helper);
+ ret = restore_fbdev_mode_force(helper);
if (ret)
error = true;
mutex_unlock(>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(_helper->lock);
- if (!drm_fb_helper_is_bound(fb_helper)) {
-
