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

Noralf Trønnes
2017-10-20T17:11:18Z

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@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?

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@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel