Re: [Intel-gfx] [PATCH 02/18] drm/atomic-helper: Unexport drm_atomic_helper_best_encoder

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Oct 04, 2018 at 02:33:24PM -0400, Sean Paul wrote:
> On Tue, Oct 02, 2018 at 03:35:10PM +0200, Daniel Vetter wrote:
> > It's the default. The exported version was kinda a transition state,
> > before we made this the default.
> > 
> > To stop new atomic drivers from using it (instead of just relying on
> > the default) let's unexport it.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Gustavo Padovan <gustavo@padovan.org>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Sean Paul <seanpaul@chromium.org>
> > Cc: David Airlie <airlied@linux.ie>
> > Cc: VMware Graphics <linux-graphics-maintainer@vmware.com>
> > Cc: Sinclair Yeh <syeh@vmware.com>
> > Cc: Thomas Hellstrom <thellstrom@vmware.com>
> > Cc: Archit Taneja <architt@codeaurora.org>
> > Cc: Neil Armstrong <narmstrong@baylibre.com>
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Hans Verkuil <hverkuil@xs4all.nl>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Russell King <rmk+kernel@armlinux.org.uk>
> > Cc: Jernej Skrabec <jernej.skrabec@siol.net>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Cc: Pierre-Hugues Husson <phh@phh.me>
> > Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > ---
> >  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c |  1 -
> >  drivers/gpu/drm/drm_atomic_helper.c       | 24 +++++++----------------
> >  drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c       |  1 -
> >  drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c      |  1 -
> >  drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c      |  1 -
> >  include/drm/drm_atomic_helper.h           |  2 --
> >  6 files changed, 7 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > index ac37c50d6c4b..5ac979d3450b 100644
> > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > @@ -1957,7 +1957,6 @@ static const struct drm_connector_funcs dw_hdmi_connector_funcs = {
> >  
> >  static const struct drm_connector_helper_funcs dw_hdmi_connector_helper_funcs = {
> >  	.get_modes = dw_hdmi_connector_get_modes,
> > -	.best_encoder = drm_atomic_helper_best_encoder,
> >  };
> >  
> >  static int dw_hdmi_bridge_attach(struct drm_bridge *bridge)
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index f92b7cf4cbd7..8c93f33fe92f 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -92,6 +92,13 @@ drm_atomic_helper_plane_changed(struct drm_atomic_state *state,
> >  	}
> >  }
> >  
> > +static struct drm_encoder *
> > +drm_atomic_helper_best_encoder(struct drm_connector *connector)
> > +{
> > +	WARN_ON(connector->encoder_ids[1]);
> > +	return drm_encoder_find(connector->dev, NULL, connector->encoder_ids[0]);
> > +}
> > +
> >  static int handle_conflicting_encoders(struct drm_atomic_state *state,
> >  				       bool disable_conflicting_encoders)
> >  {
> > @@ -3376,23 +3383,6 @@ int drm_atomic_helper_page_flip_target(struct drm_crtc *crtc,
> >  }
> >  EXPORT_SYMBOL(drm_atomic_helper_page_flip_target);
> >  
> > -/**
> > - * drm_atomic_helper_best_encoder - Helper for
> > - * 	&drm_connector_helper_funcs.best_encoder callback
> > - * @connector: Connector control structure
> > - *
> > - * This is a &drm_connector_helper_funcs.best_encoder callback helper for
> > - * connectors that support exactly 1 encoder, statically determined at driver
> > - * init time.
> > - */
> > -struct drm_encoder *
> > -drm_atomic_helper_best_encoder(struct drm_connector *connector)
> > -{
> > -	WARN_ON(connector->encoder_ids[1]);
> > -	return drm_encoder_find(connector->dev, NULL, connector->encoder_ids[0]);
> > -}
> > -EXPORT_SYMBOL(drm_atomic_helper_best_encoder);
> > -
> >  /**
> >   * DOC: atomic state reset and initialization
> >   *
> > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
> > index 723578117191..4b5378495eea 100644
> > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
> > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
> > @@ -274,7 +274,6 @@ static const struct drm_connector_funcs vmw_legacy_connector_funcs = {
> >  
> >  static const struct
> >  drm_connector_helper_funcs vmw_ldu_connector_helper_funcs = {
> > -	.best_encoder = drm_atomic_helper_best_encoder,
> >  };
> 
> Seems like you can remove this entirely, as well as the helper funcs
> registration call? Same goes for a few other drivers.

Needs a huge audit, at least in the past we've had cases where everything
started oopsing because helpers didn't check carefully whether the vtable
pointer was NULL or not.

Heck even this patch here blew up on amdgpu at first.

So good idea, but maybe not in this patch here :-)
-Daniel

> >  /*
> > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> > index ad0de7f0cd60..4c68ad6f3605 100644
> > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> > @@ -389,7 +389,6 @@ static const struct drm_connector_funcs vmw_sou_connector_funcs = {
> >  
> >  static const struct
> >  drm_connector_helper_funcs vmw_sou_connector_helper_funcs = {
> > -	.best_encoder = drm_atomic_helper_best_encoder,
> >  };
> >  
> >  
> > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> > index f30e839f7bfd..e28bb08114a5 100644
> > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> > @@ -1037,7 +1037,6 @@ static const struct drm_connector_funcs vmw_stdu_connector_funcs = {
> >  
> >  static const struct
> >  drm_connector_helper_funcs vmw_stdu_connector_helper_funcs = {
> > -	.best_encoder = drm_atomic_helper_best_encoder,
> >  };
> >  
> >  
> > diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> > index 657af7b39379..e60c4f0f8827 100644
> > --- a/include/drm/drm_atomic_helper.h
> > +++ b/include/drm/drm_atomic_helper.h
> > @@ -144,8 +144,6 @@ int drm_atomic_helper_page_flip_target(
> >  				uint32_t flags,
> >  				uint32_t target,
> >  				struct drm_modeset_acquire_ctx *ctx);
> > -struct drm_encoder *
> > -drm_atomic_helper_best_encoder(struct drm_connector *connector);
> >  
> >  /* default implementations for state handling */
> >  void drm_atomic_helper_crtc_reset(struct drm_crtc *crtc);
> > -- 
> > 2.19.0.rc2
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel