[02/18] drm/atomic-helper: Unexport drm_atomic_helper_best_encoder
diff mbox series

Message ID 20181002133526.13685-3-daniel.vetter@ffwll.ch
State New
Headers show
Series
  • atomic helper cleanups
Related show

Commit Message

Daniel Vetter Oct. 2, 2018, 1:35 p.m. UTC
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(-)

Comments

Laurent Pinchart Oct. 2, 2018, 1:53 p.m. UTC | #1
Hi Daniel,

Thank you for the patch.

On Tuesday, 2 October 2018 16:35:10 EEST 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

[snip]

> 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]);

As you're removing the documentation, I would add a comment here to explain 
the WARN_ON. Something along the lines of "For connectors that support 
multiple encoders, the .atomic_best_encoder() or .atomic_encoder() operation 
must be implemented".

You could also rename the function to make it more explicit that it's a 
default for the single encoder case.

> +	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
>   *

[snip]
Daniel Vetter Oct. 3, 2018, 9:08 a.m. UTC | #2
On Tue, Oct 02, 2018 at 04:53:12PM +0300, Laurent Pinchart wrote:
> Hi Daniel,
> 
> Thank you for the patch.
> 
> On Tuesday, 2 October 2018 16:35:10 EEST 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
> 
> [snip]
> 
> > 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]);
> 
> As you're removing the documentation, I would add a comment here to explain 
> the WARN_ON. Something along the lines of "For connectors that support 
> multiple encoders, the .atomic_best_encoder() or .atomic_encoder() operation 
> must be implemented".
> 
> You could also rename the function to make it more explicit that it's a 
> default for the single encoder case.

So pick_single_encoder_for_connector()? I think that would avoid the need
for a comment. r-b: you with that fixed up?

Thanks, Daniel

> 
> > +	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
> >   *
> 
> [snip]
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 
>
Laurent Pinchart Oct. 4, 2018, 5:13 p.m. UTC | #3
Hi Daniel,

On Wednesday, 3 October 2018 12:08:38 EEST Daniel Vetter wrote:
> On Tue, Oct 02, 2018 at 04:53:12PM +0300, Laurent Pinchart wrote:
> > On Tuesday, 2 October 2018 16:35:10 EEST 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
> > 
> > [snip]
> > 
> > > 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]);
> > 
> > As you're removing the documentation, I would add a comment here to
> > explain the WARN_ON. Something along the lines of "For connectors that
> > support multiple encoders, the .atomic_best_encoder() or .atomic_encoder()
> > operation must be implemented".
> > 
> > You could also rename the function to make it more explicit that it's a
> > default for the single encoder case.
> 
> So pick_single_encoder_for_connector()?

Works for me.

> I think that would avoid the need for a comment.

I'd still keep the comment, it won't hurt, and you have it already :-)

> r-b: you with that fixed up?

With the comment and function renamed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > > +	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
> > >   *
> > 
> > [snip]
Sean Paul Oct. 4, 2018, 6:33 p.m. UTC | #4
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.

Sean

>  
>  /*
> 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
Daniel Vetter Oct. 4, 2018, 7:33 p.m. UTC | #5
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

Patch
diff mbox series

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,
 };
 
 /*
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);