| Message ID | 1375877860-25203-1-git-send-email-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Aug 7, 2013 at 2:17 PM, Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> wrote: > The drm_agp_clear() function is only defined on platforms with AGP > support. Move the drm_core_has_AGP() check from drm_agp_clear() to the > caller to let the compiler optimize the drm_agp_clear() call away. > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> Can't we use the usual approach of an empty static inline helper for the !CONFIG_AGP case here? -Daniel
Hi Daniel, On Wednesday 07 August 2013 14:19:34 Daniel Vetter wrote: > On Wed, Aug 7, 2013 at 2:17 PM, Laurent Pinchart wrote: > > The drm_agp_clear() function is only defined on platforms with AGP > > support. Move the drm_core_has_AGP() check from drm_agp_clear() to the > > caller to let the compiler optimize the drm_agp_clear() call away. > > > > Signed-off-by: Laurent Pinchart > > <laurent.pinchart+renesas@ideasonboard.com> > > Can't we use the usual approach of an empty static inline helper for > the !CONFIG_AGP case here? Sure, that's possible as well. I find my solution slightly more explicit as it shows that drm_agp_clear() will only be called for platforms that have AGP, but I would be fine with a static inline as well.
Hi On Wed, Aug 7, 2013 at 2:39 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Daniel, > > On Wednesday 07 August 2013 14:19:34 Daniel Vetter wrote: >> On Wed, Aug 7, 2013 at 2:17 PM, Laurent Pinchart wrote: >> > The drm_agp_clear() function is only defined on platforms with AGP >> > support. Move the drm_core_has_AGP() check from drm_agp_clear() to the >> > caller to let the compiler optimize the drm_agp_clear() call away. >> > >> > Signed-off-by: Laurent Pinchart >> > <laurent.pinchart+renesas@ideasonboard.com> >> >> Can't we use the usual approach of an empty static inline helper for >> the !CONFIG_AGP case here? > > Sure, that's possible as well. I find my solution slightly more explicit as it > shows that drm_agp_clear() will only be called for platforms that have AGP, > but I would be fine with a static inline as well. But the compile fails with -O0.. we don't depend on optimizations to avoid compiler errors. Besides, "static inline" is standard kernel coding-style so I think we should stick to it. I've attached a patch, feel free to pick it up. Otherwise, I will give it some test-compiles once I get home and send it to dri-devel. Thanks David
diff --git a/drivers/gpu/drm/drm_agpsupport.c b/drivers/gpu/drm/drm_agpsupport.c index e301d65..084a674 100644 --- a/drivers/gpu/drm/drm_agpsupport.c +++ b/drivers/gpu/drm/drm_agpsupport.c @@ -439,7 +439,7 @@ void drm_agp_clear(struct drm_device *dev) { struct drm_agp_mem *entry, *tempe; - if (!drm_core_has_AGP(dev) || !dev->agp) + if (!dev->agp) return; if (drm_core_check_feature(dev, DRIVER_MODESET)) return; diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index dddd799..1e2ad35 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -195,7 +195,8 @@ int drm_lastclose(struct drm_device * dev) mutex_lock(&dev->struct_mutex); - drm_agp_clear(dev); + if (drm_core_has_AGP(dev)) + drm_agp_clear(dev); if (drm_core_check_feature(dev, DRIVER_SG) && dev->sg && !drm_core_check_feature(dev, DRIVER_MODESET)) {
The drm_agp_clear() function is only defined on platforms with AGP support. Move the drm_core_has_AGP() check from drm_agp_clear() to the caller to let the compiler optimize the drm_agp_clear() call away. Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> --- drivers/gpu/drm/drm_agpsupport.c | 2 +- drivers/gpu/drm/drm_drv.c | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) This fixes a link-time build error in drm-next. An alternative approach would be to guard the drm_agp_clear() call in drm_drv.c with an #if __OS_HAS_AGP. I can resubmit the patch to do so if preferred.