Message ID | 20200625120011.16168-10-tzimmermann@suse.de (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | drm: Support simple-framebuffer devices and firmware fbs | expand |
On Thu, Jun 25, 2020 at 02:00:11PM +0200, Thomas Zimmermann wrote: > We register the simplekms device with the DRM platform helpers. A > native driver for the graphics hardware will kickout the simplekms > driver before taking over the device. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > drivers/gpu/drm/tiny/Kconfig | 1 + > drivers/gpu/drm/tiny/simplekms.c | 94 +++++++++++++++++++++++++++++++- > 2 files changed, 92 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig > index 50dbde8bdcb2..a47ed337a7fe 100644 > --- a/drivers/gpu/drm/tiny/Kconfig > +++ b/drivers/gpu/drm/tiny/Kconfig > @@ -33,6 +33,7 @@ config DRM_SIMPLEKMS > depends on DRM > select DRM_GEM_SHMEM_HELPER > select DRM_KMS_HELPER > + select DRM_PLATFORM_HELPER > help > DRM driver for simple platform-provided framebuffers. > > diff --git a/drivers/gpu/drm/tiny/simplekms.c b/drivers/gpu/drm/tiny/simplekms.c > index ae5d3cbadbe8..a903a4e0100a 100644 > --- a/drivers/gpu/drm/tiny/simplekms.c > +++ b/drivers/gpu/drm/tiny/simplekms.c > @@ -5,6 +5,7 @@ > #include <linux/platform_data/simplefb.h> > #include <linux/platform_device.h> > #include <linux/regulator/consumer.h> > +#include <linux/spinlock.h> > > #include <drm/drm_atomic_state_helper.h> > #include <drm/drm_connector.h> > @@ -17,6 +18,7 @@ > #include <drm/drm_gem_shmem_helper.h> > #include <drm/drm_managed.h> > #include <drm/drm_modeset_helper_vtables.h> > +#include <drm/drm_platform.h> > #include <drm/drm_probe_helper.h> > #include <drm/drm_simple_kms_helper.h> > > @@ -36,6 +38,12 @@ > #define SIMPLEKMS_MODE(hd, vd) \ > DRM_SIMPLE_MODE(hd, vd, RES_MM(hd), RES_MM(vd)) > > +/* > + * Protects the platform device's drvdata against > + * concurrent manipulation. > + */ > +static DEFINE_SPINLOCK(simplekms_drvdata_lock); > + > /* > * Helpers for simplefb > */ > @@ -211,6 +219,7 @@ struct simplekms_device { > unsigned int pitch; > > /* memory management */ > + struct drm_aperture *aperture; > struct resource *mem; > void __iomem *screen_base; > > @@ -224,6 +233,8 @@ static struct simplekms_device *simplekms_device_of_dev(struct drm_device *dev) > return container_of(dev, struct simplekms_device, dev); > } > > +static void simplekms_device_cleanup(struct simplekms_device *sdev); > + > /* > * Hardware > */ > @@ -514,22 +525,72 @@ static int simplekms_device_init_fb(struct simplekms_device *sdev) > * Memory management > */ > > +static void simplekms_aperture_kickout(struct drm_aperture *ap) > +{ > + struct drm_device *dev = ap->dev; > + struct simplekms_device *sdev = simplekms_device_of_dev(dev); > + struct platform_device *pdev = sdev->pdev; > + > + if (WARN_ON(!sdev->aperture)) > + return; /* BUG: driver already got kicked out */ > + > + drm_dev_unregister(dev); From a semantic pov I think the platform driver getting kicked out is more like a hotunplug, so drm_dev_unplug(dev); here is imo better. That then also gives you a nice drm_dev_enter/exit to sprinkle over the various driver callbacks, instead of the racy ->aperture check reinvented wheel here. I also wonder whether we couldn't go full driver model for these platform devices, and instead of this here call a core driver model function to force the unbding of the driver. Only change we'd need it that our ->remove hook uses drm_dev_unplug(). Also I guess this nice plan doesn't work if efifb or vesafb don't have a platform_device of their own that the drm_platform.c code could use to nuke platform drivers. But if they have, we could just use device_driver_detach() from drm_platform.c and wouldn't need any of this. Worst case efi and vesa drm drivers could instantiate the platform device they bind against themselves ... I think that would be a lot cleaner than hand-rolling our own hotunplug infrastructure here. Adding Greg in case we're missing anything here. > + > + sdev->aperture = NULL; /* memory is released by platform helpers */ > + > + spin_lock(&simplekms_drvdata_lock); > + sdev = platform_get_drvdata(pdev); > + platform_set_drvdata(pdev, NULL); /* required; see simplekms_remove() */ > + spin_unlock(&simplekms_drvdata_lock); > + > + /* > + * Return if a concurrent simplekms_remove() cleans up the > + * device. See simplekms_remove(). > + */ > + if (!sdev) > + return; > + > + /* > + * After the aperture has been released, there's no reason > + * to keep the DRM device around. > + */ > + simplekms_device_cleanup(sdev); Uh, you already unregistered, this unregisters again. Maybe a bit too much :-) > +} > + > +static const struct drm_aperture_funcs simplekms_aperture_funcs = { > + .kickout = simplekms_aperture_kickout, > +}; > + > static int simplekms_device_init_mm(struct simplekms_device *sdev) > { > + struct drm_device *dev = &sdev->dev; > struct platform_device *pdev = sdev->pdev; > struct resource *mem; > + struct drm_aperture *ap; > void __iomem *screen_base; > + int ret; > > mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > if (!mem) > return -EINVAL; > > + ap = drmm_aperture_acquire(dev, mem->start, resource_size(mem), > + &simplekms_aperture_funcs); > + if (IS_ERR(ap)) { > + ret = PTR_ERR(ap); > + drm_err(dev, > + "could not acquire memory range [0x%llx:0x%llx]: " > + "error %d\n", mem->start, mem->end, ret); > + return ret; > + } > + > screen_base = devm_ioremap_wc(&pdev->dev, mem->start, > resource_size(mem)); > if (!screen_base) > return -ENOMEM; > > sdev->mem = mem; > + sdev->aperture = ap; > sdev->screen_base = screen_base; > > return 0; > @@ -625,6 +686,9 @@ simplekms_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe, > struct drm_framebuffer *fb = state->fb; > void *vmap; > > + if (!sdev->aperture) > + return; > + > vmap = drm_gem_shmem_vmap(fb->obj[0]); > if (!vmap) > return; > @@ -645,6 +709,9 @@ simplekms_simple_display_pipe_update(struct drm_simple_display_pipe *pipe, > struct drm_rect clip; > void *vmap; > > + if (!sdev->aperture) > + return; > + > if (!drm_atomic_helper_damage_merged(old_plane_state, state, &clip)) > return; > > @@ -716,11 +783,12 @@ static int simplekms_device_init_modeset(struct simplekms_device *sdev) > * Init / Cleanup > */ > > -static void simplekms_device_cleanup(struct simplekms_device* sdev) > +static void simplekms_device_cleanup(struct simplekms_device *sdev) > { > struct drm_device *dev = &sdev->dev; > > - drm_dev_unregister(dev); > + if (dev->registered) > + drm_dev_unregister(dev); > } > > static struct simplekms_device * > @@ -797,7 +865,27 @@ static int simplekms_probe(struct platform_device *pdev) > > static int simplekms_remove(struct platform_device *pdev) > { > - struct simplekms_device *sdev = platform_get_drvdata(pdev); > + struct simplekms_device *sdev; > + > + spin_lock(&simplekms_drvdata_lock); > + sdev = platform_get_drvdata(pdev); > + platform_set_drvdata(pdev, NULL); > + spin_unlock(&simplekms_drvdata_lock); > + > + /* > + * The platform driver shares its reference to dev with the > + * platform helpers for apertures. That reference is either > + * released here when unloading the driver; or it's released > + * when the driver gets kicked out by another driver. In the > + * latter case, the aperture release routine clears the data > + * field of the platform device. > + * > + * Therefore, sdev being NULL is a valid state if the driver > + * has been kicked out by another DRM driver. In this case, > + * it's all been cleaned up and we can return immediately. > + */ > + if (!sdev) > + return 0; > > simplekms_device_cleanup(sdev); > > -- > 2.27.0 >
On Mon, Jun 29, 2020 at 11:22:30AM +0200, Daniel Vetter wrote: > On Thu, Jun 25, 2020 at 02:00:11PM +0200, Thomas Zimmermann wrote: > > We register the simplekms device with the DRM platform helpers. A > > native driver for the graphics hardware will kickout the simplekms > > driver before taking over the device. > > > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > > --- > > drivers/gpu/drm/tiny/Kconfig | 1 + > > drivers/gpu/drm/tiny/simplekms.c | 94 +++++++++++++++++++++++++++++++- > > 2 files changed, 92 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig > > index 50dbde8bdcb2..a47ed337a7fe 100644 > > --- a/drivers/gpu/drm/tiny/Kconfig > > +++ b/drivers/gpu/drm/tiny/Kconfig > > @@ -33,6 +33,7 @@ config DRM_SIMPLEKMS > > depends on DRM > > select DRM_GEM_SHMEM_HELPER > > select DRM_KMS_HELPER > > + select DRM_PLATFORM_HELPER > > help > > DRM driver for simple platform-provided framebuffers. > > > > diff --git a/drivers/gpu/drm/tiny/simplekms.c b/drivers/gpu/drm/tiny/simplekms.c > > index ae5d3cbadbe8..a903a4e0100a 100644 > > --- a/drivers/gpu/drm/tiny/simplekms.c > > +++ b/drivers/gpu/drm/tiny/simplekms.c > > @@ -5,6 +5,7 @@ > > #include <linux/platform_data/simplefb.h> > > #include <linux/platform_device.h> > > #include <linux/regulator/consumer.h> > > +#include <linux/spinlock.h> > > > > #include <drm/drm_atomic_state_helper.h> > > #include <drm/drm_connector.h> > > @@ -17,6 +18,7 @@ > > #include <drm/drm_gem_shmem_helper.h> > > #include <drm/drm_managed.h> > > #include <drm/drm_modeset_helper_vtables.h> > > +#include <drm/drm_platform.h> > > #include <drm/drm_probe_helper.h> > > #include <drm/drm_simple_kms_helper.h> > > > > @@ -36,6 +38,12 @@ > > #define SIMPLEKMS_MODE(hd, vd) \ > > DRM_SIMPLE_MODE(hd, vd, RES_MM(hd), RES_MM(vd)) > > > > +/* > > + * Protects the platform device's drvdata against > > + * concurrent manipulation. > > + */ > > +static DEFINE_SPINLOCK(simplekms_drvdata_lock); > > + > > /* > > * Helpers for simplefb > > */ > > @@ -211,6 +219,7 @@ struct simplekms_device { > > unsigned int pitch; > > > > /* memory management */ > > + struct drm_aperture *aperture; > > struct resource *mem; > > void __iomem *screen_base; > > > > @@ -224,6 +233,8 @@ static struct simplekms_device *simplekms_device_of_dev(struct drm_device *dev) > > return container_of(dev, struct simplekms_device, dev); > > } > > > > +static void simplekms_device_cleanup(struct simplekms_device *sdev); > > + > > /* > > * Hardware > > */ > > @@ -514,22 +525,72 @@ static int simplekms_device_init_fb(struct simplekms_device *sdev) > > * Memory management > > */ > > > > +static void simplekms_aperture_kickout(struct drm_aperture *ap) > > +{ > > + struct drm_device *dev = ap->dev; > > + struct simplekms_device *sdev = simplekms_device_of_dev(dev); > > + struct platform_device *pdev = sdev->pdev; > > + > > + if (WARN_ON(!sdev->aperture)) > > + return; /* BUG: driver already got kicked out */ > > + > > + drm_dev_unregister(dev); > > >From a semantic pov I think the platform driver getting kicked out is more > like a hotunplug, so drm_dev_unplug(dev); here is imo better. > > That then also gives you a nice drm_dev_enter/exit to sprinkle over the > various driver callbacks, instead of the racy ->aperture check reinvented > wheel here. > > I also wonder whether we couldn't go full driver model for these platform > devices, and instead of this here call a core driver model function to > force the unbding of the driver. Only change we'd need it that our > ->remove hook uses drm_dev_unplug(). Yes, please do that. Or, use the "virtual bus/device" code that some people at Intel are still trying to get into mergable shape. See the posts on the netdev list for those details. Don't use platform devices for anything that is not actually a platform device (i.e. something described by hardware resources). thanks, greg k-h
On Mon, Jun 29, 2020 at 06:04:21PM +0200, Greg KH wrote: > Yes, please do that. Or, use the "virtual bus/device" code that some > people at Intel are still trying to get into mergable shape. See the > posts on the netdev list for those details. Any pointers on that? There's also some ongoing discussion with MFD and that's not been mentioned at all.
On Mon, Jun 29, 2020 at 05:23:16PM +0100, Mark Brown wrote: > On Mon, Jun 29, 2020 at 06:04:21PM +0200, Greg KH wrote: > > > Yes, please do that. Or, use the "virtual bus/device" code that some > > people at Intel are still trying to get into mergable shape. See the > > posts on the netdev list for those details. > > Any pointers on that? There's also some ongoing discussion with MFD and > that's not been mentioned at all. https://lore.kernel.org/r/20200520070227.3392100-2-jeffrey.t.kirsher@intel.com
On Mon, Jun 29, 2020 at 10:04 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Mon, Jun 29, 2020 at 11:22:30AM +0200, Daniel Vetter wrote: > > On Thu, Jun 25, 2020 at 02:00:11PM +0200, Thomas Zimmermann wrote: > > > We register the simplekms device with the DRM platform helpers. A > > > native driver for the graphics hardware will kickout the simplekms > > > driver before taking over the device. > > > > > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > > > --- > > > drivers/gpu/drm/tiny/Kconfig | 1 + > > > drivers/gpu/drm/tiny/simplekms.c | 94 +++++++++++++++++++++++++++++++- > > > 2 files changed, 92 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig > > > index 50dbde8bdcb2..a47ed337a7fe 100644 > > > --- a/drivers/gpu/drm/tiny/Kconfig > > > +++ b/drivers/gpu/drm/tiny/Kconfig > > > @@ -33,6 +33,7 @@ config DRM_SIMPLEKMS > > > depends on DRM > > > select DRM_GEM_SHMEM_HELPER > > > select DRM_KMS_HELPER > > > + select DRM_PLATFORM_HELPER > > > help > > > DRM driver for simple platform-provided framebuffers. > > > > > > diff --git a/drivers/gpu/drm/tiny/simplekms.c b/drivers/gpu/drm/tiny/simplekms.c > > > index ae5d3cbadbe8..a903a4e0100a 100644 > > > --- a/drivers/gpu/drm/tiny/simplekms.c > > > +++ b/drivers/gpu/drm/tiny/simplekms.c > > > @@ -5,6 +5,7 @@ > > > #include <linux/platform_data/simplefb.h> > > > #include <linux/platform_device.h> > > > #include <linux/regulator/consumer.h> > > > +#include <linux/spinlock.h> > > > > > > #include <drm/drm_atomic_state_helper.h> > > > #include <drm/drm_connector.h> > > > @@ -17,6 +18,7 @@ > > > #include <drm/drm_gem_shmem_helper.h> > > > #include <drm/drm_managed.h> > > > #include <drm/drm_modeset_helper_vtables.h> > > > +#include <drm/drm_platform.h> > > > #include <drm/drm_probe_helper.h> > > > #include <drm/drm_simple_kms_helper.h> > > > > > > @@ -36,6 +38,12 @@ > > > #define SIMPLEKMS_MODE(hd, vd) \ > > > DRM_SIMPLE_MODE(hd, vd, RES_MM(hd), RES_MM(vd)) > > > > > > +/* > > > + * Protects the platform device's drvdata against > > > + * concurrent manipulation. > > > + */ > > > +static DEFINE_SPINLOCK(simplekms_drvdata_lock); > > > + > > > /* > > > * Helpers for simplefb > > > */ > > > @@ -211,6 +219,7 @@ struct simplekms_device { > > > unsigned int pitch; > > > > > > /* memory management */ > > > + struct drm_aperture *aperture; > > > struct resource *mem; > > > void __iomem *screen_base; > > > > > > @@ -224,6 +233,8 @@ static struct simplekms_device *simplekms_device_of_dev(struct drm_device *dev) > > > return container_of(dev, struct simplekms_device, dev); > > > } > > > > > > +static void simplekms_device_cleanup(struct simplekms_device *sdev); > > > + > > > /* > > > * Hardware > > > */ > > > @@ -514,22 +525,72 @@ static int simplekms_device_init_fb(struct simplekms_device *sdev) > > > * Memory management > > > */ > > > > > > +static void simplekms_aperture_kickout(struct drm_aperture *ap) > > > +{ > > > + struct drm_device *dev = ap->dev; > > > + struct simplekms_device *sdev = simplekms_device_of_dev(dev); > > > + struct platform_device *pdev = sdev->pdev; > > > + > > > + if (WARN_ON(!sdev->aperture)) > > > + return; /* BUG: driver already got kicked out */ > > > + > > > + drm_dev_unregister(dev); > > > > >From a semantic pov I think the platform driver getting kicked out is more > > like a hotunplug, so drm_dev_unplug(dev); here is imo better. > > > > That then also gives you a nice drm_dev_enter/exit to sprinkle over the > > various driver callbacks, instead of the racy ->aperture check reinvented > > wheel here. > > > > I also wonder whether we couldn't go full driver model for these platform > > devices, and instead of this here call a core driver model function to > > force the unbding of the driver. Only change we'd need it that our > > ->remove hook uses drm_dev_unplug(). > > Yes, please do that. Or, use the "virtual bus/device" code that some > people at Intel are still trying to get into mergable shape. See the > posts on the netdev list for those details. > > Don't use platform devices for anything that is not actually a platform > device (i.e. something described by hardware resources). Well, 'simple-framebuffer' is described by DT and includes h/w resources such as clocks. So this is a gray area. I'm not saying we couldn't use virtual bus for DT nodes, but we'll need some clear guidelines of when to use virtual vs. platform devices. No doubt I'll get a 'virtual bus' binding if folks are directed to make things a virtual device. Rob
On Mon, Jun 29, 2020 at 08:13:51PM -0600, Rob Herring wrote: > On Mon, Jun 29, 2020 at 10:04 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > On Mon, Jun 29, 2020 at 11:22:30AM +0200, Daniel Vetter wrote: > > > On Thu, Jun 25, 2020 at 02:00:11PM +0200, Thomas Zimmermann wrote: > > > > We register the simplekms device with the DRM platform helpers. A > > > > native driver for the graphics hardware will kickout the simplekms > > > > driver before taking over the device. > > > > > > > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > > > > --- > > > > drivers/gpu/drm/tiny/Kconfig | 1 + > > > > drivers/gpu/drm/tiny/simplekms.c | 94 +++++++++++++++++++++++++++++++- > > > > 2 files changed, 92 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig > > > > index 50dbde8bdcb2..a47ed337a7fe 100644 > > > > --- a/drivers/gpu/drm/tiny/Kconfig > > > > +++ b/drivers/gpu/drm/tiny/Kconfig > > > > @@ -33,6 +33,7 @@ config DRM_SIMPLEKMS > > > > depends on DRM > > > > select DRM_GEM_SHMEM_HELPER > > > > select DRM_KMS_HELPER > > > > + select DRM_PLATFORM_HELPER > > > > help > > > > DRM driver for simple platform-provided framebuffers. > > > > > > > > diff --git a/drivers/gpu/drm/tiny/simplekms.c b/drivers/gpu/drm/tiny/simplekms.c > > > > index ae5d3cbadbe8..a903a4e0100a 100644 > > > > --- a/drivers/gpu/drm/tiny/simplekms.c > > > > +++ b/drivers/gpu/drm/tiny/simplekms.c > > > > @@ -5,6 +5,7 @@ > > > > #include <linux/platform_data/simplefb.h> > > > > #include <linux/platform_device.h> > > > > #include <linux/regulator/consumer.h> > > > > +#include <linux/spinlock.h> > > > > > > > > #include <drm/drm_atomic_state_helper.h> > > > > #include <drm/drm_connector.h> > > > > @@ -17,6 +18,7 @@ > > > > #include <drm/drm_gem_shmem_helper.h> > > > > #include <drm/drm_managed.h> > > > > #include <drm/drm_modeset_helper_vtables.h> > > > > +#include <drm/drm_platform.h> > > > > #include <drm/drm_probe_helper.h> > > > > #include <drm/drm_simple_kms_helper.h> > > > > > > > > @@ -36,6 +38,12 @@ > > > > #define SIMPLEKMS_MODE(hd, vd) \ > > > > DRM_SIMPLE_MODE(hd, vd, RES_MM(hd), RES_MM(vd)) > > > > > > > > +/* > > > > + * Protects the platform device's drvdata against > > > > + * concurrent manipulation. > > > > + */ > > > > +static DEFINE_SPINLOCK(simplekms_drvdata_lock); > > > > + > > > > /* > > > > * Helpers for simplefb > > > > */ > > > > @@ -211,6 +219,7 @@ struct simplekms_device { > > > > unsigned int pitch; > > > > > > > > /* memory management */ > > > > + struct drm_aperture *aperture; > > > > struct resource *mem; > > > > void __iomem *screen_base; > > > > > > > > @@ -224,6 +233,8 @@ static struct simplekms_device *simplekms_device_of_dev(struct drm_device *dev) > > > > return container_of(dev, struct simplekms_device, dev); > > > > } > > > > > > > > +static void simplekms_device_cleanup(struct simplekms_device *sdev); > > > > + > > > > /* > > > > * Hardware > > > > */ > > > > @@ -514,22 +525,72 @@ static int simplekms_device_init_fb(struct simplekms_device *sdev) > > > > * Memory management > > > > */ > > > > > > > > +static void simplekms_aperture_kickout(struct drm_aperture *ap) > > > > +{ > > > > + struct drm_device *dev = ap->dev; > > > > + struct simplekms_device *sdev = simplekms_device_of_dev(dev); > > > > + struct platform_device *pdev = sdev->pdev; > > > > + > > > > + if (WARN_ON(!sdev->aperture)) > > > > + return; /* BUG: driver already got kicked out */ > > > > + > > > > + drm_dev_unregister(dev); > > > > > > >From a semantic pov I think the platform driver getting kicked out is more > > > like a hotunplug, so drm_dev_unplug(dev); here is imo better. > > > > > > That then also gives you a nice drm_dev_enter/exit to sprinkle over the > > > various driver callbacks, instead of the racy ->aperture check reinvented > > > wheel here. > > > > > > I also wonder whether we couldn't go full driver model for these platform > > > devices, and instead of this here call a core driver model function to > > > force the unbding of the driver. Only change we'd need it that our > > > ->remove hook uses drm_dev_unplug(). > > > > Yes, please do that. Or, use the "virtual bus/device" code that some > > people at Intel are still trying to get into mergable shape. See the > > posts on the netdev list for those details. > > > > Don't use platform devices for anything that is not actually a platform > > device (i.e. something described by hardware resources). > > Well, 'simple-framebuffer' is described by DT and includes h/w > resources such as clocks. So this is a gray area. I'm not saying we > couldn't use virtual bus for DT nodes, but we'll need some clear > guidelines of when to use virtual vs. platform devices. No doubt I'll > get a 'virtual bus' binding if folks are directed to make things a > virtual device. If it is described by DT, then I have no objection for it to be a platform device. thanks, greg k-h
diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig index 50dbde8bdcb2..a47ed337a7fe 100644 --- a/drivers/gpu/drm/tiny/Kconfig +++ b/drivers/gpu/drm/tiny/Kconfig @@ -33,6 +33,7 @@ config DRM_SIMPLEKMS depends on DRM select DRM_GEM_SHMEM_HELPER select DRM_KMS_HELPER + select DRM_PLATFORM_HELPER help DRM driver for simple platform-provided framebuffers. diff --git a/drivers/gpu/drm/tiny/simplekms.c b/drivers/gpu/drm/tiny/simplekms.c index ae5d3cbadbe8..a903a4e0100a 100644 --- a/drivers/gpu/drm/tiny/simplekms.c +++ b/drivers/gpu/drm/tiny/simplekms.c @@ -5,6 +5,7 @@ #include <linux/platform_data/simplefb.h> #include <linux/platform_device.h> #include <linux/regulator/consumer.h> +#include <linux/spinlock.h> #include <drm/drm_atomic_state_helper.h> #include <drm/drm_connector.h> @@ -17,6 +18,7 @@ #include <drm/drm_gem_shmem_helper.h> #include <drm/drm_managed.h> #include <drm/drm_modeset_helper_vtables.h> +#include <drm/drm_platform.h> #include <drm/drm_probe_helper.h> #include <drm/drm_simple_kms_helper.h> @@ -36,6 +38,12 @@ #define SIMPLEKMS_MODE(hd, vd) \ DRM_SIMPLE_MODE(hd, vd, RES_MM(hd), RES_MM(vd)) +/* + * Protects the platform device's drvdata against + * concurrent manipulation. + */ +static DEFINE_SPINLOCK(simplekms_drvdata_lock); + /* * Helpers for simplefb */ @@ -211,6 +219,7 @@ struct simplekms_device { unsigned int pitch; /* memory management */ + struct drm_aperture *aperture; struct resource *mem; void __iomem *screen_base; @@ -224,6 +233,8 @@ static struct simplekms_device *simplekms_device_of_dev(struct drm_device *dev) return container_of(dev, struct simplekms_device, dev); } +static void simplekms_device_cleanup(struct simplekms_device *sdev); + /* * Hardware */ @@ -514,22 +525,72 @@ static int simplekms_device_init_fb(struct simplekms_device *sdev) * Memory management */ +static void simplekms_aperture_kickout(struct drm_aperture *ap) +{ + struct drm_device *dev = ap->dev; + struct simplekms_device *sdev = simplekms_device_of_dev(dev); + struct platform_device *pdev = sdev->pdev; + + if (WARN_ON(!sdev->aperture)) + return; /* BUG: driver already got kicked out */ + + drm_dev_unregister(dev); + + sdev->aperture = NULL; /* memory is released by platform helpers */ + + spin_lock(&simplekms_drvdata_lock); + sdev = platform_get_drvdata(pdev); + platform_set_drvdata(pdev, NULL); /* required; see simplekms_remove() */ + spin_unlock(&simplekms_drvdata_lock); + + /* + * Return if a concurrent simplekms_remove() cleans up the + * device. See simplekms_remove(). + */ + if (!sdev) + return; + + /* + * After the aperture has been released, there's no reason + * to keep the DRM device around. + */ + simplekms_device_cleanup(sdev); +} + +static const struct drm_aperture_funcs simplekms_aperture_funcs = { + .kickout = simplekms_aperture_kickout, +}; + static int simplekms_device_init_mm(struct simplekms_device *sdev) { + struct drm_device *dev = &sdev->dev; struct platform_device *pdev = sdev->pdev; struct resource *mem; + struct drm_aperture *ap; void __iomem *screen_base; + int ret; mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); if (!mem) return -EINVAL; + ap = drmm_aperture_acquire(dev, mem->start, resource_size(mem), + &simplekms_aperture_funcs); + if (IS_ERR(ap)) { + ret = PTR_ERR(ap); + drm_err(dev, + "could not acquire memory range [0x%llx:0x%llx]: " + "error %d\n", mem->start, mem->end, ret); + return ret; + } + screen_base = devm_ioremap_wc(&pdev->dev, mem->start, resource_size(mem)); if (!screen_base) return -ENOMEM; sdev->mem = mem; + sdev->aperture = ap; sdev->screen_base = screen_base; return 0; @@ -625,6 +686,9 @@ simplekms_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe, struct drm_framebuffer *fb = state->fb; void *vmap; + if (!sdev->aperture) + return; + vmap = drm_gem_shmem_vmap(fb->obj[0]); if (!vmap) return; @@ -645,6 +709,9 @@ simplekms_simple_display_pipe_update(struct drm_simple_display_pipe *pipe, struct drm_rect clip; void *vmap; + if (!sdev->aperture) + return; + if (!drm_atomic_helper_damage_merged(old_plane_state, state, &clip)) return; @@ -716,11 +783,12 @@ static int simplekms_device_init_modeset(struct simplekms_device *sdev) * Init / Cleanup */ -static void simplekms_device_cleanup(struct simplekms_device* sdev) +static void simplekms_device_cleanup(struct simplekms_device *sdev) { struct drm_device *dev = &sdev->dev; - drm_dev_unregister(dev); + if (dev->registered) + drm_dev_unregister(dev); } static struct simplekms_device * @@ -797,7 +865,27 @@ static int simplekms_probe(struct platform_device *pdev) static int simplekms_remove(struct platform_device *pdev) { - struct simplekms_device *sdev = platform_get_drvdata(pdev); + struct simplekms_device *sdev; + + spin_lock(&simplekms_drvdata_lock); + sdev = platform_get_drvdata(pdev); + platform_set_drvdata(pdev, NULL); + spin_unlock(&simplekms_drvdata_lock); + + /* + * The platform driver shares its reference to dev with the + * platform helpers for apertures. That reference is either + * released here when unloading the driver; or it's released + * when the driver gets kicked out by another driver. In the + * latter case, the aperture release routine clears the data + * field of the platform device. + * + * Therefore, sdev being NULL is a valid state if the driver + * has been kicked out by another DRM driver. In this case, + * it's all been cleaned up and we can return immediately. + */ + if (!sdev) + return 0; simplekms_device_cleanup(sdev);
We register the simplekms device with the DRM platform helpers. A native driver for the graphics hardware will kickout the simplekms driver before taking over the device. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/gpu/drm/tiny/Kconfig | 1 + drivers/gpu/drm/tiny/simplekms.c | 94 +++++++++++++++++++++++++++++++- 2 files changed, 92 insertions(+), 3 deletions(-)