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

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Jernej Skrabec <jernej.skrabec@siol.net>,
	Thomas Hellstrom <thellstrom@vmware.com>,
	VMware Graphics <linux-graphics-maintainer@vmware.com>,
	Sinclair Yeh <syeh@vmware.com>, David Airlie <airlied@linux.ie>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	Neil Armstrong <narmstrong@baylibre.com>,
	Archit Taneja <architt@codeaurora.org>,
	DRI Development <dri-devel@lists.freedesktop.org>,
	Hans Verkuil <hverkuil@xs4all.nl>,
	Jani Nikula <jani.nikula@intel.com>,
	Russell King <rmk+kernel@armlinux.org.uk>,
	Sean Paul <seanpaul@chromium.org>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	Fabio Estevam <fabio.estevam@nxp.com>,
	Daniel Vetter <daniel.vetter@intel.com>,
	Pierre-Hugues Husson <phh@phh.me>
Subject: Re: [PATCH 02/18] drm/atomic-helper: Unexport drm_atomic_helper_best_encoder
Date: Thu, 04 Oct 2018 20:13:17 +0300	[thread overview]
Message-ID: <19985496.8oYBDqUf3h@avalon> (raw)
In-Reply-To: <20181003090838.GB23685@phenom.ffwll.local>

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]

-- 
Regards,

Laurent Pinchart

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx