Re: [PATCH v5 79/80] drm/omapdrm: Remove struct drm_fb_helper from struct omap_fbdev.

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 26.09.24 um 15:07 schrieb Tomi Valkeinen:
> Hi,
>
> On 24/09/2024 10:13, Thomas Zimmermann wrote:
>> Store instances of drm_fb_helper and struct omap_fbdev separately.
>> This will allow omapdrm to use the common fbdev client, which allocates
>> its own instance of struct drm_fb_helper.
>>
>> There is at most one instance of each per DRM device, so both can be
>> referenced directly from the omap and DRM device structures. A later
>> patchset might rework the common fbdev client to allow for storing
>> both, drm_fb_helper and omap_fbdev, together in the same place.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> Acked-by: Javier Martinez Canillas <javierm@redhat.com>
>> ---
>>   drivers/gpu/drm/omapdrm/omap_drv.h   |  3 ++
>>   drivers/gpu/drm/omapdrm/omap_fbdev.c | 42 +++++++++++++++-------------
>>   2 files changed, 26 insertions(+), 19 deletions(-)
>
> The ywrap seems to be broken, but it's broken without this series too. 
> With some quick hacks I managed to get the omapdrm's panning function 
> called, and it works the same way with or without this series.
>
> Tested on DRA76 EVM.
>
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>
> For reference, in case someone is interested fixing omapdrm's ywrap code:
>
> - get_fb() doesn't work. I don't understand the point of the function. 
> It compares fbi->fix.id and MODULE_NAME, and the "fbi->fix.id" has 
> changed along the years so the check no longer works. Afaik, the whole 
> function is not needed, and we can just use fbi->par.
>
> - omap_fbdev_pan_display() doesn't seem to get called when the console 
> scrolls. I can get it called with an userspace app that does some y 
> panning. Maybe FBINFO_HWACCEL_YWRAP is not supported anymore?

If ywrap doesn't work and no one complained, can we remove it? The rest 
of the code is equivalent to drm_fbdev_dma.c, so we could remove omap's 
implementation entirely.

Best regards
Thomas

>
>  Tomi
>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h 
>> b/drivers/gpu/drm/omapdrm/omap_drv.h
>> index 4c7217b35f6b..d903568fd8cc 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_drv.h
>> +++ b/drivers/gpu/drm/omapdrm/omap_drv.h
>> @@ -32,6 +32,7 @@
>>   #define MODULE_NAME     "omapdrm"
>>     struct omap_drm_usergart;
>> +struct omap_fbdev;
>>     struct omap_drm_pipeline {
>>       struct drm_crtc *crtc;
>> @@ -97,6 +98,8 @@ struct omap_drm_private {
>>         /* memory bandwidth limit if it is needed on the platform */
>>       unsigned int max_bandwidth;
>> +
>> +    struct omap_fbdev *fbdev;
>>   };
>>     diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c 
>> b/drivers/gpu/drm/omapdrm/omap_fbdev.c
>> index 523be34682ca..044e80403c3b 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
>> @@ -13,6 +13,7 @@
>>   #include <drm/drm_fourcc.h>
>>   #include <drm/drm_framebuffer.h>
>>   #include <drm/drm_gem_framebuffer_helper.h>
>> +#include <drm/drm_managed.h>
>>   #include <drm/drm_util.h>
>>     #include "omap_drv.h"
>> @@ -26,10 +27,8 @@ module_param_named(ywrap, ywrap_enabled, bool, 0644);
>>    * fbdev funcs, to implement legacy fbdev interface on top of drm 
>> driver
>>    */
>>   -#define to_omap_fbdev(x) container_of(x, struct omap_fbdev, base)
>> -
>>   struct omap_fbdev {
>> -    struct drm_fb_helper base;
>> +    struct drm_device *dev;
>>       bool ywrap_enabled;
>>         /* for deferred dmm roll when getting called in atomic ctx */
>> @@ -41,7 +40,7 @@ static struct drm_fb_helper *get_fb(struct fb_info 
>> *fbi);
>>   static void pan_worker(struct work_struct *work)
>>   {
>>       struct omap_fbdev *fbdev = container_of(work, struct 
>> omap_fbdev, work);
>> -    struct drm_fb_helper *helper = &fbdev->base;
>> +    struct drm_fb_helper *helper = fbdev->dev->fb_helper;
>>       struct fb_info *fbi = helper->info;
>>       struct drm_gem_object *bo = drm_gem_fb_get_obj(helper->fb, 0);
>>       int npages;
>> @@ -55,24 +54,25 @@ FB_GEN_DEFAULT_DEFERRED_DMAMEM_OPS(omap_fbdev,
>>                      drm_fb_helper_damage_range,
>>                      drm_fb_helper_damage_area)
>>   -static int omap_fbdev_pan_display(struct fb_var_screeninfo *var,
>> -        struct fb_info *fbi)
>> +static int omap_fbdev_pan_display(struct fb_var_screeninfo *var, 
>> struct fb_info *fbi)
>>   {
>>       struct drm_fb_helper *helper = get_fb(fbi);
>> -    struct omap_fbdev *fbdev = to_omap_fbdev(helper);
>> +    struct omap_drm_private *priv;
>> +    struct omap_fbdev *fbdev;
>>         if (!helper)
>>           goto fallback;
>>   +    priv = helper->dev->dev_private;
>> +    fbdev = priv->fbdev;
>> +
>>       if (!fbdev->ywrap_enabled)
>>           goto fallback;
>>   -    if (drm_can_sleep()) {
>> +    if (drm_can_sleep())
>>           pan_worker(&fbdev->work);
>> -    } else {
>> -        struct omap_drm_private *priv = helper->dev->dev_private;
>> +    else
>>           queue_work(priv->wq, &fbdev->work);
>> -    }
>>         return 0;
>>   @@ -92,7 +92,6 @@ static void omap_fbdev_fb_destroy(struct fb_info 
>> *info)
>>       struct drm_fb_helper *helper = info->par;
>>       struct drm_framebuffer *fb = helper->fb;
>>       struct drm_gem_object *bo = drm_gem_fb_get_obj(fb, 0);
>> -    struct omap_fbdev *fbdev = to_omap_fbdev(helper);
>>         DBG();
>>   @@ -104,7 +103,7 @@ static void omap_fbdev_fb_destroy(struct 
>> fb_info *info)
>>         drm_client_release(&helper->client);
>>       drm_fb_helper_unprepare(helper);
>> -    kfree(fbdev);
>> +    kfree(helper);
>>   }
>>     /*
>> @@ -128,9 +127,9 @@ static const struct fb_ops omap_fb_ops = {
>>   static int omap_fbdev_create(struct drm_fb_helper *helper,
>>           struct drm_fb_helper_surface_size *sizes)
>>   {
>> -    struct omap_fbdev *fbdev = to_omap_fbdev(helper);
>>       struct drm_device *dev = helper->dev;
>>       struct omap_drm_private *priv = dev->dev_private;
>> +    struct omap_fbdev *fbdev = priv->fbdev;
>>       struct drm_framebuffer *fb = NULL;
>>       union omap_gem_size gsize;
>>       struct fb_info *fbi = NULL;
>> @@ -338,6 +337,7 @@ static const struct drm_client_funcs 
>> omap_fbdev_client_funcs = {
>>     void omap_fbdev_setup(struct drm_device *dev)
>>   {
>> +    struct omap_drm_private *priv = dev->dev_private;
>>       struct omap_fbdev *fbdev;
>>       struct drm_fb_helper *helper;
>>       int ret;
>> @@ -345,24 +345,28 @@ void omap_fbdev_setup(struct drm_device *dev)
>>       drm_WARN(dev, !dev->registered, "Device has not been 
>> registered.\n");
>>       drm_WARN(dev, dev->fb_helper, "fb_helper is already set!\n");
>>   -    fbdev = kzalloc(sizeof(*fbdev), GFP_KERNEL);
>> +    fbdev = drmm_kzalloc(dev, sizeof(*fbdev), GFP_KERNEL);
>>       if (!fbdev)
>>           return;
>> -    helper = &fbdev->base;
>> +    fbdev->dev = dev;
>> +    INIT_WORK(&fbdev->work, pan_worker);
>> +
>> +    priv->fbdev = fbdev;
>>   +    helper = kzalloc(sizeof(*helper), GFP_KERNEL);
>> +    if (!helper)
>> +        return;
>>       drm_fb_helper_prepare(dev, helper, 32, &omap_fb_helper_funcs);
>>         ret = drm_client_init(dev, &helper->client, "fbdev", 
>> &omap_fbdev_client_funcs);
>>       if (ret)
>>           goto err_drm_client_init;
>>   -    INIT_WORK(&fbdev->work, pan_worker);
>> -
>>       drm_client_register(&helper->client);
>>         return;
>>     err_drm_client_init:
>>       drm_fb_helper_unprepare(helper);
>> -    kfree(fbdev);
>> +    kfree(helper);
>>   }
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)