Tomi Valkeinen
2024-Sep-26 13:07 UTC
[PATCH v5 79/80] drm/omapdrm: Remove struct drm_fb_helper from struct omap_fbdev.
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 at suse.de> > Cc: Tomi Valkeinen <tomi.valkeinen at ideasonboard.com> > Acked-by: Javier Martinez Canillas <javierm at 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 at 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? 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
2024-Sep-27 14:46 UTC
[PATCH v5 79/80] drm/omapdrm: Remove struct drm_fb_helper from struct omap_fbdev.
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 at suse.de> >> Cc: Tomi Valkeinen <tomi.valkeinen at ideasonboard.com> >> Acked-by: Javier Martinez Canillas <javierm at 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 at 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)