[PATCH 02/15] drm: Add drm_device->drm_fb_helper_private pointer

[Daniel Vetter]

On Fri, Oct 20, 2017 at 07:11:18PM +0200, Noralf Trønnes wrote:
> 
> Den 20.10.2017 18.00, skrev Ville Syrjälä:
> > On Fri, Oct 20, 2017 at 05:44:15PM +0200, Noralf Trønnes wrote:
> > > Den 20.10.2017 15.52, skrev Ville Syrjälä:
> > > > On Fri, Oct 20, 2017 at 01:02:00AM +0200, Noralf Trønnes wrote:
> > > > > drm_fb_helper is *the* way of doing fbdev emulation so add a pointer to
> > > > > struct drm_device. This makes it possible to add callback helpers for
> > > > > .last_close and .output_poll_changed further reducing fbdev emulation
> > > > > footprint in drivers. The pointer is set by drm_fb_helper_init() and
> > > > > cleared by drm_fb_helper_fini().
> > > > > 
> > > > > Signed-off-by: Noralf Trønnes <noralf at tronnes.org>
> > > > > ---
> > > > > 
> > > > > This patch was initially part of the patchset: drm/tinydrm: Use vmalloc BO
> > > > > Changes since previous version:
> > > > > - Change member name: fbdev -> drm_fb_helper_private
> > > > > - Expand docs
> > > > > - Set and clear pointer in drm_fb_helper_init/fini()
> > > > > 
> > > > >    drivers/gpu/drm/drm_fb_helper.c | 13 +++++++++++--
> > > > >    include/drm/drm_device.h        |  9 +++++++++
> > > > >    2 files changed, 20 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > > > > index 954cdd48de92..c07b7af8de4c 100644
> > > > > --- a/drivers/gpu/drm/drm_fb_helper.c
> > > > > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > > > > @@ -799,8 +799,10 @@ int drm_fb_helper_init(struct drm_device *dev,
> > > > >    	struct drm_mode_config *config = &dev->mode_config;
> > > > >    	int i;
> > > > > -	if (!drm_fbdev_emulation)
> > > > > +	if (!drm_fbdev_emulation) {
> > > > > +		dev->drm_fb_helper_private = fb_helper;
> > > > >    		return 0;
> > > > > +	}
> > > > >    	if (!max_conn_count)
> > > > >    		return -EINVAL;
> > > > > @@ -835,6 +837,8 @@ int drm_fb_helper_init(struct drm_device *dev,
> > > > >    		i++;
> > > > >    	}
> > > > > +	dev->drm_fb_helper_private = fb_helper;
> > > > > +
> > > > >    	return 0;
> > > > >    out_free:
> > > > >    	drm_fb_helper_crtc_free(fb_helper);
> > > > > @@ -913,7 +917,12 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
> > > > >    {
> > > > >    	struct fb_info *info;
> > > > > -	if (!drm_fbdev_emulation || !fb_helper)
> > > > > +	if (!fb_helper)
> > > > > +		return;
> > > > > +
> > > > > +	fb_helper->dev->drm_fb_helper_private = NULL;
> > > > > +
> > > > > +	if (!drm_fbdev_emulation)
> > > > >    		return;
> > > > >    	cancel_work_sync(&fb_helper->resume_work);
> > > > > diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
> > > > > index e21af87a2f3c..6b26262658ae 100644
> > > > > --- a/include/drm/drm_device.h
> > > > > +++ b/include/drm/drm_device.h
> > > > > @@ -17,6 +17,7 @@ struct drm_vblank_crtc;
> > > > >    struct drm_sg_mem;
> > > > >    struct drm_local_map;
> > > > >    struct drm_vma_offset_manager;
> > > > > +struct drm_fb_helper;
> > > > >    struct inode;
> > > > > @@ -185,6 +186,14 @@ struct drm_device {
> > > > >    	struct drm_vma_offset_manager *vma_offset_manager;
> > > > >    	/*@} */
> > > > >    	int switch_power_state;
> > > > > +
> > > > > +	/**
> > > > > +	 * @drm_fb_helper_private:
> > > > > +	 *
> > > > > +	 * Pointer to the fbdev emulation structure.
> > > > > +	 * Set by drm_fb_helper_init() and cleared by drm_fb_helper_fini().
> > > > > +	 */
> > > > > +	struct drm_fb_helper *drm_fb_helper_private;
> > > > Just 'fb_helper' maybe? Not sure what the _private here is supposed to
> > > > mean, and drm_ seems very much redundant.
> > > I first called it fbdev, Daniel suggested fbdev_helper_private, then I
> > > took it all the way and called it drm_fb_helper_private. I believe the
> > > _private part is an indication that this is not part of the core, but a
> > > helper, like drm_plane->helper_private.
> > IMO those we should rename to helper_funcs since that's what they always
> > are. IIRC they used to be void* and then the _private made more sense to
> > me since you couldn't directly know what they were pointing at.
> > 
> > To me _private means that it's totally up to driver to specify what to
> > put there (eg. like we have private_flags in the mode structure).
> > 
> > And I believe the "helper" part in "fb_helper" should be enough of a hint
> > to anyone that this has something to do with a helper ;)
> 
> Daniel, do you have any objections to calling it: fb_helper?

I retract my bikeshed, looks like you&Ville are much more informed :-)
-Daniel

> 
> Noralf.
> 
> > > fb_helper is the common variable name used in drm_fb_helper (which
> > > really should have been called drm_fbdev_helper imo).
> > Yeah. On a first glance one might think drm_fb_helper has something
> > to do with drm_framebuffers.
> > 
> > > These are the names that drivers use:
> > > 
> > >       struct amdgpu_fbdev *rfbdev;
> > >       struct drm_fb_helper    *fbdev;
> > >       struct ast_fbdev *fbdev;
> > >       struct drm_fb_helper fb.helper;
> > >       struct cirrus_fbdev        *gfbdev;
> > >       struct drm_fb_helper    fb_helper;
> > >       struct drm_fb_helper *fb_helper;
> > >       void *fbdev;
> > >       struct hibmc_fbdev *fbdev;
> > >       struct mga_fbdev *mfbdev;
> > >       struct drm_fb_helper *fbdev;
> > >       struct nouveau_fbdev *fbcon;
> > >       struct drm_fb_helper *fbdev;
> > >       struct qxl_fbdev *qfbdev;
> > >       struct radeon_fbdev *rfbdev;
> > >       struct drm_fb_helper fbdev_helper;
> > >       struct tegra_fbdev *fbdev;
> > >       struct udl_fbdev *fbdev;
> > >       struct virtio_gpu_fbdev *vgfbdev;
> > >       struct vbox_fbdev *fbdev;
> > > 
> > > Noralf.
> > > 
> > > > >    };
> > > > >    #endif
> > > > > -- 
> > > > > 2.14.2
> > > > > 
> > > > > _______________________________________________
> > > > > dri-devel mailing list
> > > > > dri-devel at lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch