Re: [PATCH 5/7] drm/i915: Update kerneldoc for intel_dpll_mgr.c

[Ander Conselvan De Oliveira <conselvan2@gmail.com>]

On Thu, 2016-10-13 at 15:46 +0200, Daniel Vetter wrote:
> On Tue, Oct 04, 2016 at 03:32:15PM +0300, Ander Conselvan de Oliveira wrote:
> > 
> > The documentation for most of the non-static members and structs were
> > missing. Fix that.
> > 
> > v2: Fix typos (Durga)
> > 
> > v3: Rebase.
> >     Fix make docs warnings.
> >     Document more.
> > 
> > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@inte
> > l.com>
> As mentioned in an earlier patch, I think we should also move
> intel_atomic_get_shared_dpll_state from intel_atomic.c into
> intel_dpll_mgr.c. Grouping functions by the data structures they touch
> makes imo much more sense, than grouping them by topic. That way all the
> dpll stuff is in one place.
> 
> > 
> > ---
> >  Documentation/gpu/i915.rst            |  12 +++
> >  drivers/gpu/drm/i915/intel_dpll_mgr.c |  90 ++++++++++++++++++--
> >  drivers/gpu/drm/i915/intel_dpll_mgr.h | 155 ++++++++++++++++++++++++++++++-
> > ---
> >  3 files changed, 237 insertions(+), 20 deletions(-)
> > 
> > diff --git a/Documentation/gpu/i915.rst b/Documentation/gpu/i915.rst
> > index 87aaffc..c19e437 100644
> > --- a/Documentation/gpu/i915.rst
> > +++ b/Documentation/gpu/i915.rst
> > @@ -204,6 +204,18 @@ Video BIOS Table (VBT)
> >  .. kernel-doc:: drivers/gpu/drm/i915/intel_vbt_defs.h
> >     :internal:
> >  
> > +Display PLLs
> > +------------
> > +
> > +.. kernel-doc:: drivers/gpu/drm/i915/intel_dpll_mgr.c
> > +   :doc: Display PLLs
> > +
> > +.. kernel-doc:: drivers/gpu/drm/i915/intel_dpll_mgr.c
> > +   :internal:
> > +
> > +.. kernel-doc:: drivers/gpu/drm/i915/intel_dpll_mgr.h
> > +   :internal:
> > +
> >  Memory Management and Command Submission
> >  ========================================
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > index 9446446..8c4efa9 100644
> > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > @@ -23,6 +23,25 @@
> >  
> >  #include "intel_drv.h"
> >  
> > +/**
> > + * DOC: Display PLLs
> > + *
> > + * Display PLLs used for driving outputs vary by platform. While some have
> > + * per-pipe or per-encoder dedicated PLLs, others allow the use of any PLL
> > + * from a pool. In the latter scenario, it is possible that multiple pipes
> > + * share a PLL if their configurations match.
> > + *
> > + * This file provides an abstraction over display PLLs. The function
> > + * intel_shared_dpll_init() initializes the PLLs for the given
> > platform.  The
> > + * users of a PLL are tracked and that tracking is integrated with the
> > atomic
> > + * modest interface. During an atomic operation, a PLL can be requested for
> > a
> > + * given crtc and encoder configuration by calling intel_get_shared_dpll()
> > and
> s/crtc/CRTC/ for consistency (abbreviations are all upercase), pls do that
> on the entire doc.
> 
> > 
> > + * a previously used PLL can be released with intel_release_shared_dpll().
> > + * Changes to the users are first staged in the atomic state, and then made
> > + * effective by calling intel_shared_dpll_swap_state() during the atomic
> > + * commit phase.
> > + */
> > +
> >  struct intel_shared_dpll *
> >  skl_find_link_pll(struct drm_i915_private *dev_priv, int clock)
> >  {
> > @@ -61,6 +80,14 @@ skl_find_link_pll(struct drm_i915_private *dev_priv, int
> > clock)
> >  	return pll;
> >  }
> >  
> > +/**
> > + * intel_get_shared_dpll_by_id - get a DPLL given its id
> > + * @dev_priv: i915 device instance
> > + * @id: pll id
> > + *
> > + * Returns:
> > + * A pointer to the DPLL with @id
> > + */
> >  struct intel_shared_dpll *
> >  intel_get_shared_dpll_by_id(struct drm_i915_private *dev_priv,
> >  			    enum intel_dpll_id id)
> > @@ -68,6 +95,14 @@ intel_get_shared_dpll_by_id(struct drm_i915_private
> > *dev_priv,
> >  	return &dev_priv->shared_dplls[id];
> >  }
> >  
> > +/**
> > + * intel_get_shared_dpll_id - get the id of a DPLL
> > + * @dev_priv: i915 device instance
> > + * @pll: the DPLL
> > + *
> > + * Returns:
> > + * The id of @pll
> > + */
> >  enum intel_dpll_id
> >  intel_get_shared_dpll_id(struct drm_i915_private *dev_priv,
> >  			 struct intel_shared_dpll *pll)
> > @@ -96,6 +131,13 @@ void assert_shared_dpll(struct drm_i915_private
> > *dev_priv,
> >  			pll->name, onoff(state), onoff(cur_state));
> >  }
> >  
> > +/**
> > + * intel_prepare_shared_dpll - call a dpll's prepare hook
> > + * @crtc: crtc which has a shared dpll
> > + *
> > + * This calls the PLL's prepare hook if it has one and if the PLL is not
> > + * already enabled. The prepare hook is platform specific.
> > + */
> >  void intel_prepare_shared_dpll(struct intel_crtc *crtc)
> >  {
> >  	struct drm_device *dev = crtc->base.dev;
> > @@ -118,12 +160,10 @@ void intel_prepare_shared_dpll(struct intel_crtc
> > *crtc)
> >  }
> >  
> >  /**
> > - * intel_enable_shared_dpll - enable PCH PLL
> > - * @dev_priv: i915 private structure
> > - * @pipe: pipe PLL to enable
> > + * intel_enable_shared_dpll - enable a crtc's shared DPLL
> > + * @crtc: crtc which has a shared DPLL
> >   *
> > - * The PCH PLL needs to be enabled before the PCH transcoder, since it
> > - * drives the transcoder clock.
> > + * Enable the shared DPLL used by @crtc.
> >   */
> >  void intel_enable_shared_dpll(struct intel_crtc *crtc)
> >  {
> > @@ -164,6 +204,12 @@ out:
> >  	mutex_unlock(&dev_priv->dpll_lock);
> >  }
> >  
> > +/**
> > + * intel_disable_shared_dpll - disable a crtc's shared DPLL
> > + * @crtc: crtc which has a shared DPLL
> > + *
> > + * Disable the shared DPLL used by @crtc.
> > + */
> >  void intel_disable_shared_dpll(struct intel_crtc *crtc)
> >  {
> >  	struct drm_device *dev = crtc->base.dev;
> > @@ -266,6 +312,16 @@ intel_reference_shared_dpll(struct intel_shared_dpll
> > *pll,
> >  	shared_dpll[pll->id].crtc_mask |= 1 << crtc->pipe;
> >  }
> >  
> > +/**
> > + * intel_shared_dpll_swap_state - make atomic DPLL configuration effective
> > + * @state: atomic state
> > + *
> > + * This is the dpll version of drm_atomic_helper_swap_state() since the
> > + * helper does not handle driver-specific global state.
> > + *
> > + * Note that this doesn't actually swap states, the dpll configutation in
> > + * @state is left unchanged.
> Hm, what do you mean with that? I guess you mean that it only puts the
> state from drm_atomic_state into dev_priv, and not the other way round.
> Tbh that's a bit unexpected (yes atm we don't have a reason for that), so
> instead of documenting this oddity I'd just fix it. And then maybe mention
> that "For consistency with atomic helpers this also puts the current state
> into @state, i.e. it does a complete swap, even though right now that's
> not needed."
> 
> > 
> > + */
> >  void intel_shared_dpll_swap_state(struct drm_atomic_state *state)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(state->dev);
> > @@ -1822,6 +1878,12 @@ static const struct intel_dpll_mgr bxt_pll_mgr = {
> >  	.get_dpll = bxt_get_dpll,
> >  };
> >  
> > +/**
> > + * intel_shared_dpll_init - Initialize shared DPLLs
> > + * @dev: drm device
> > + *
> > + * Initialize shared DPLLs for @dev.
> > + */
> >  void intel_shared_dpll_init(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > @@ -1865,6 +1927,21 @@ void intel_shared_dpll_init(struct drm_device *dev)
> >  		intel_ddi_pll_init(dev);
> >  }
> >  
> > +/**
> > + * intel_get_shared_dpll - get a shared DPLL for crtc and encoder
> > combination
> > + * @crtc: crtc
> > + * @crtc_state: atomic state for @crtc
> > + * @encoder: encoder
> > + *
> > + * Find an appropriate DPLL for the given crtc and encoder combination. A
> > + * reference from the @crtc to the returned pll is registered in the atomic
> > + * state. That configuration is made effective by calling
> > + * intel_shared_dpll_swap_state(). The reference should be released by
> > calling
> > + * intel_release_shared_dpll().
> > + *
> > + * Returns:
> > + * A shared DPLL to be used by @crtc and @encoder with the given
> > @crtc_state.
> > + */
> >  struct intel_shared_dpll *
> >  intel_get_shared_dpll(struct intel_crtc *crtc,
> >  		      struct intel_crtc_state *crtc_state,
> > @@ -1885,6 +1962,9 @@ intel_get_shared_dpll(struct intel_crtc *crtc,
> >   * @crtc: crtc
> >   * @state: atomic state
> >   *
> > + * This function releases the reference from @crtc to @dpll from the
> > + * atomic @state. The new configuration is made effective by calling
> > + * intel_shared_dpll_swap_state().
> >   */
> >  void intel_release_shared_dpll(struct intel_shared_dpll *dpll,
> >  			       struct intel_crtc *crtc,
> > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > index 9a7db65..40f1a6f 100644
> > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > @@ -40,32 +40,72 @@ struct intel_encoder;
> >  struct intel_shared_dpll;
> >  struct intel_dpll_mgr;
> >  
> > +/**
> > + * enum intel_dpll_id - possible DPLL ids
> > + *
> > + * Enumeration of possible IDs for a DPLL. Real shared dpll ids must be >=
> > 0.
> > + */
> >  enum intel_dpll_id {
> > -	DPLL_ID_PRIVATE = -1, /* non-shared dpll in use */
> > -	/* real shared dpll ids must be >= 0 */
> > +	/**
> > +	 * @DPLL_ID_PRIVATE: non-shared dpll in use
> > +	 */
> > +	DPLL_ID_PRIVATE = -1,
> > +
> > +	/**
> > +	 * @DPLL_ID_PCH_PLL_A: DPLL A in ILK, SNB and IVB
> > +	 */
> >  	DPLL_ID_PCH_PLL_A = 0,
> > +	/**
> > +	 * @DPLL_ID_PCH_PLL_B: DPLL B in ILK, SNB and IVB
> > +	 */
> >  	DPLL_ID_PCH_PLL_B = 1,
> > -	/* hsw/bdw */
> > +
> > +
> > +	/**
> > +	 * @DPLL_ID_WRPLL1: HSW and BDW WRPLL1
> > +	 */
> >  	DPLL_ID_WRPLL1 = 0,
> > +	/**
> > +	 * @DPLL_ID_WRPLL2: HSW and BDW WRPLL2
> > +	 */
> >  	DPLL_ID_WRPLL2 = 1,
> > +	/**
> > +	 * @DPLL_ID_SPLL: HSW and BDW SPLL
> > +	 */
> >  	DPLL_ID_SPLL = 2,
> > +	/**
> > +	 * @DPLL_ID_LCPLL_810: HSW and BDW 0.81 GHz LCPLL
> > +	 */
> >  	DPLL_ID_LCPLL_810 = 3,
> > +	/**
> > +	 * @DPLL_ID_LCPLL_1350: HSW and BDW 1.35 GHz LCPLL
> > +	 */
> >  	DPLL_ID_LCPLL_1350 = 4,
> > +	/**
> > +	 * @DPLL_ID_LCPLL_2700: HSW and BDW 2.7 GHz LCPLL
> > +	 */
> >  	DPLL_ID_LCPLL_2700 = 5,
> >  
> > -	/* skl */
> > +
> > +	/**
> > +	 * @DPLL_ID_SKL_DPLL0: SKL and later DPLL0
> > +	 */
> >  	DPLL_ID_SKL_DPLL0 = 0,
> > +	/**
> > +	 * @DPLL_ID_SKL_DPLL1: SKL and later DPLL1
> > +	 */
> >  	DPLL_ID_SKL_DPLL1 = 1,
> > +	/**
> > +	 * @DPLL_ID_SKL_DPLL2: SKL and later DPLL2
> > +	 */
> >  	DPLL_ID_SKL_DPLL2 = 2,
> > +	/**
> > +	 * @DPLL_ID_SKL_DPLL3: SKL and later DPLL3
> > +	 */
> >  	DPLL_ID_SKL_DPLL3 = 3,
> >  };
> >  #define I915_NUM_PLLS 6
> >  
> > -/** Inform the state checker that the DPLL is kept enabled even if not
> > - * in use by any crtc.
> > - */
> > -#define INTEL_DPLL_ALWAYS_ON	(1 << 0)
> > -
> >  struct intel_dpll_hw_state {
> >  	/* i9xx, pch plls */
> >  	uint32_t dpll;
> > @@ -93,39 +133,124 @@ struct intel_dpll_hw_state {
> >  		 pcsdw12;
> >  };
> >  
> > +/**
> > + * struct intel_shared_dpll_state - hold the DPLL atomic state
> > + *
> > + * This structure holds an atomic state for the DPLL, that can represent
> > + * either its current state or a desired furture state which would be
> > + * applied by an atomic mode set.
> I think it'd be good to reference where we store pointers to that, i.e.
> where in intel_atomic_state and intel_shared_dpll it sits. Best if you do that
> using the struct &intel_atomic_state reference style (so that it becomes a
> hyperlink once that's documented).
> 
> Also links to the main functions used to manage this would be good I
> think, i.e. release and get.
> 
> > 
> > + */
> >  struct intel_shared_dpll_state {
> > -	unsigned crtc_mask; /* mask of CRTCs sharing this PLL */
> > +	/**
> > +	 * @crtc_mask: mask of crtc using this DPLL, active or not
> s/crtc/CRTCs/
> 
> > 
> > +	 */
> > +	unsigned crtc_mask;
> > +
> > +	/**
> > +	 * @hw_state: hardware configuration for the DPLL.
> "... stored in struct &intel_dpll_hw_state." - I love my hyperlinks ;-)

I'll add that, but I think it's silly. The type of the field is struct
intel_dpll_hw_state, so I think it would be more natural if the documentation
tool would add that link automatically.


> > 
> > +	 */
> >  	struct intel_dpll_hw_state hw_state;
> >  };
> >  
> > +/**
> > + * struct intel_shared_dpll_funcs - platform specific hooks for managing
> > DPLLs
> > + */
> >  struct intel_shared_dpll_funcs {
> > -	/* The mode_set hook is optional and should be used together with
> > the
> > -	 * intel_prepare_shared_dpll function. */
> > +	/**
> > +	 * @prepare:
> > +	 *
> > +	 * Optional hook to perform operations prior to enabling the PLL.
> > +	 * Called from intel_prepare_shared_dpll() function.
> Missing the language about "if the pll is not already enabled."
> 
> > 
> > +	 */
> >  	void (*prepare)(struct drm_i915_private *dev_priv,
> >  			 struct intel_shared_dpll *pll);
> > +
> > +	/**
> > +	 * @enable:
> > +	 *
> > +	 * Hook for enabling the pll, called from
> > intel_enable_shared_dpll()
> > +	 * if the pll is not already enabled.
> > +	 */
> >  	void (*enable)(struct drm_i915_private *dev_priv,
> >  		       struct intel_shared_dpll *pll);
> > +
> > +	/**
> > +	 * @disable:
> > +	 *
> > +	 * Hook for disabling the pll, called from
> > intel_disable_shared_dpll()
> > +	 * only when it is safe to disable the pll, i.e., there are no more
> > +	 * tracked users for it.
> > +	 */
> >  	void (*disable)(struct drm_i915_private *dev_priv,
> >  			struct intel_shared_dpll *pll);
> > +
> > +	/**
> > +	 * @get_hw_state:
> > +	 *
> > +	 * Hook for reading the values currently programmed to the DPLL
> > +	 * registers. This is used for initial hw state readout and state
> > +	 * verification after a mode set.
> > +	 */
> >  	bool (*get_hw_state)(struct drm_i915_private *dev_priv,
> >  			     struct intel_shared_dpll *pll,
> >  			     struct intel_dpll_hw_state *hw_state);
> >  };
> >  
> > +/**
> > + * struct intel_shared_dpll - display PLL with tracked state and users
> > + */
> >  struct intel_shared_dpll {
> > +	/**
> > +	 * @state:
> > +	 *
> > +	 * Store the state for the pll, including the its hw state
> > +	 * and crtcs using it.
> > +	 */
> >  	struct intel_shared_dpll_state state;
> >  
> > -	unsigned active_mask; /* mask of active CRTCs (i.e. DPMS on) */
> > -	bool on; /* is the PLL actually active? Disabled during modeset */
> > +	/**
> > +	 * @active_mask: mask of active CRTCs (i.e. DPMS on) using this
> > DPLL
> > +	 */
> > +	unsigned active_mask;
> > +
> > +	/**
> > +	 * @on: is the PLL actually active? Disabled during modeset
> > +	 */
> > +	bool on;
> > +
> > +	/**
> > +	 * @name: DPLL name; used for logging
> > +	 */
> >  	const char *name;
> > -	/* should match the index in the dev_priv->shared_dplls array */
> > +
> > +	/**
> > +	 * @id: unique indentifier for this DPLL; should match the index in
> > the
> > +	 * dev_priv->shared_dplls array
> > +	 */
> >  	enum intel_dpll_id id;
> >  
> > +	/**
> > +	 * @funcs: platform specific hooks
> > +	 */
> >  	struct intel_shared_dpll_funcs funcs;
> >  
> > +	/**
> > +	 * @flags:
> > +	 *
> > +	 * accepted flags: INTEL_DPLL_ALWAYS_ON
> Hm, I've started to just document flags in-line as an enumeration, to keep
> things tightly grouped. And then also place the #define right in front of
> the kernel-doc for @flags. See e.g. @flags in drm_property.h for a really
> long example of that (but there the flags are in an uabi header, so a bit
> special).

Like this?

struct intel_shared_dpll {

...

#define INTEL_DPLL_ALWAYS_ON    (1 << 0)
        /**
         * @flags:
         *
         * INTEL_DPLL_ALWAYS_ON
         *     Inform the state checker that the DPLL is kept enabled even if
         *     not in use by any CRTC.
         */
        uint32_t flags;
};


Ander

> 
> With the nitpicks addressed somehow:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> > 
> > +	 */
> >  	uint32_t flags;
> >  };
> >  
> > +/**
> > + * INTEL_DPLL_ALWAYS_ON
> > + *
> > + * Inform the state checker that the DPLL is kept enabled even if not
> > + * in use by any crtc.
> > + */
> > +#define INTEL_DPLL_ALWAYS_ON	(1 << 0)
> > +
> > +
> >  #define SKL_DPLL0 0
> >  #define SKL_DPLL1 1
> >  #define SKL_DPLL2 2
> > -- 
> > 2.5.5
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx