On Thu, Jul 24, 2014 at 5:31 AM, David Herrmann <dh.herrmann@xxxxxxxxx> wrote: > Hi > > On Wed, Jul 23, 2014 at 9:25 PM, Daniel Vetter <daniel@xxxxxxxx> wrote: >> On Wed, Jul 23, 2014 at 05:26:38PM +0200, David Herrmann wrote: >>> Lets order things correctly: >>> ->load() >>> ->fistopen() >>> ->open() >>> ->close() >>> ->lastclose() >>> ->unload() >>> >>> This doesn't change much as only savage and radeon use ->firstopen() and >>> they just do map-initialization. Therefore, the global drm mutex makes >>> sure there cannot be any other f_op between ->open() and ->firstopen(). >>> However, once we get rid of that lock, we really want ->firstopen() to >>> initialize the device before ->open() is called. >>> >>> Furthermore, this fixes the clean-up path in drm_open(). We currently >>> don't cleanup the drm_file object if ->firstopen() fails. >>> >>> Signed-off-by: David Herrmann <dh.herrmann@xxxxxxxxx> >>> --- >>> drivers/gpu/drm/drm_fops.c | 139 +++++++++++++++++++++------------------------ >>> include/drm/drmP.h | 2 +- >>> 2 files changed, 66 insertions(+), 75 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c >>> index 021fe5d..8e73519 100644 >>> --- a/drivers/gpu/drm/drm_fops.c >>> +++ b/drivers/gpu/drm/drm_fops.c >>> @@ -45,7 +45,7 @@ EXPORT_SYMBOL(drm_global_mutex); >>> >>> static int drm_open_helper(struct file *filp, struct drm_minor *minor); >>> >>> -static int drm_setup(struct drm_device * dev) >>> +static int drm_firstopen(struct drm_device * dev) >> >> All the stuff in here is only for legacy drivers. Imo we should rename >> this to drm_legacy_setup and hide it harder. >> >> Also touching the init ordering for ums drivers is a bit risky, I'd advise >> against it. firstopen is officially dead for kms driver and really there's >> nothing legit you can do in there. imx abused until they've switched over >> to the component framework. >> >> I'd just drop this one tbh. > > I did a driver audit when writing this patch, there're only 2 drivers > using firstopen: > > * radeon_cp: sets two fields of dev_private and calls drm_addmap() on > those. In lastclose(), it calls drm_rmmap() > * savage: sets several fields in dev_private, calls > arch_phys_wc_add() for some regions and requests a drm-map for those > via drm_addmap(). On lastclose(), it reverts exactly those steps. > > Taking into account that firstopen() just runs with drm_device > context, not with drm_file context, I really think we should be > calling it _before_ doing ->open(). I even think the drivers would > fail horribly if we don't do this patch and some other user-context > sneaks in between both calls (currently impossible thanks to > drm_global_mutex). > > Both ->firstopen() callbacks are fairly trivial. In favor of making > the error-paths work in drm_open() I'd really like to get this in. > This patch is preparing for my drm_file rework that can make > drm_dev_unregister() stop all open files immediately. > > But I honestly don't care much for UMS drivers, so if you NACK this, > I'd just ignore the error code and add a comment that we don't do > error-handling for UMS. Well, radeon UMS support is in the kernel deprecated and hasn't been built by default for a while and I doubt anyone has tested it in years. I'm not sure what the current state of savage is. I'm fine either way. Alex > > Cheers > David > _______________________________________________ > dri-devel mailing list > dri-devel@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/dri-devel _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
