| Message ID | 20240924071734.98201-80-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Provide client setup helper and convert drivers | expand |
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? 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); > }
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); >> } >
Hi, On 27/09/2024 17:46, Thomas Zimmermann wrote: > 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. Personally I'm fine with dropping it, but I have to say the console scrolling looked reaaaally slow without it. I'll try to find the time to at least figure out when it broke. Maybe it was a recent thing, and thus no one has complained. 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); }