On Tue, May 17, 2016 at 04:18:16PM +0300, Ander Conselvan de Oliveira wrote:
> Signed-off-by: Ander Conselvan de Oliveira
> <ander.conselvan.de.olive...@intel.com>
> ---
> drivers/gpu/drm/i915/intel_dpll_mgr.c | 68
> ++++++++++++++++++++++++++++++++---
You also need to add a stanza to Documentation/DocBook/gpu.tmpl to
actually include this.
Would also be good to document struct intel_dpll and the vfunc stuff and
pull that into the same section in gpu.tmpl too.
> 1 file changed, 63 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index c283ba4..36566f8 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -23,6 +23,13 @@
A few sentences in a DOC: section about what this is supposed to do (also
pulled into gpu.tmpl) would be nice. Especially a short overview over the
expected calling sequence (in speudo-code, if you have asciidoc installed
that should work on top of drm-intel-nightly with topic/kerneldoc).
>
> #include "intel_drv.h"
>
> +/** intel_get_shared_dpll_by_id - get a DPLL given its id
I think kernel-doc wants a newline after the /**
> + * @dev_priv: i915 device instance
> + * @id: pll id
> + *
> + * Returns:
> + * A pointer 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)
> @@ -30,6 +37,13 @@ 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)
> @@ -41,6 +55,15 @@ intel_get_shared_dpll_id(struct drm_i915_private *dev_priv,
> return (enum intel_dpll_id) (pll - dev_priv->shared_dplls);
> }
>
> +/** intel_shared_dpll_config_get - get a reference to a DPLL for a crtc
> + * @config: a intel_shared_dpll_config struct
> + * @pll: pll to get a reference to
> + * @crtc: crtc that references the pll
> + *
> + * This gets a reference to @pll for @crtc. Internally is sets the pll's
> + * crtc_mask. @config should be the pll configuration in an atomic state.
> + * A crtc can reference a pll only once.
Please add "The acquired reference should be dropped again with
intel_shared_dpll_config_put()." With that kerneldoc will add a
cross-reference.
Also I thought with atomic we don't need get/put anymore, and state
handling is tracked entirely through the atomic check/commit stuff? Also,
grep tells me that we could make this static, which avoids the need to add
kernel-doc and also resolves some of the confusion we have here. Maybe
even just inline _get()/_put() to avoid confusing name - these aren't
refcounted anymore, but tracking in atomic states.
> + */
> void
> intel_shared_dpll_config_get(struct intel_shared_dpll_config *config,
> struct intel_shared_dpll *pll,
> @@ -52,6 +75,14 @@ intel_shared_dpll_config_get(struct
> intel_shared_dpll_config *config,
> config[id].crtc_mask |= 1 << crtc->pipe;
> }
>
> +/** intel_shared_dpll_config_put - release a reference to a DPLL for a crtc
> + * @config: a intel_shared_dpll_config struct
> + * @pll: pll that is referenced by @crtc
> + * @crtc: crtc that references the pll
> + *
> + * This releases the reference to @pll for @crtc. Internally is sets the
> pll's
> + * crtc_mask. @config should be the pll configuration in an atomic state.
> + */
> void
> intel_shared_dpll_config_put(struct intel_shared_dpll_config *config,
> struct intel_shared_dpll *pll,
> @@ -80,6 +111,9 @@ 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 mode_set hook
> + * @crtc: crtc which has a shared dpll
> + */
> void intel_prepare_shared_dpll(struct intel_crtc *crtc)
> {
> struct drm_device *dev = crtc->base.dev;
> @@ -102,12 +136,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)
> {
> @@ -148,6 +180,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;
> @@ -250,6 +288,11 @@ intel_reference_shared_dpll(struct intel_shared_dpll
> *pll,
> intel_shared_dpll_config_get(shared_dpll, pll, crtc);
> }
>
> +/** intel_shared_dpll_commit - make atomic state DPLL configuration effective
> + * @state: atomic state
> + *
> + * Makes the DPLL configuration in @state effective.
> + */
> void intel_shared_dpll_commit(struct drm_atomic_state *state)
In atomic we use commit to push out (mostly) the hw state. Just pushing
out the software state is done using drm_atomic_helper_swap_state(). And
indeed we call this function here right next to the helper. I think better
to first rename this to _swap_state() for consistency, and then maybe
cross-reference to drm_atomic_helper_swap_state(), e.g.
"This is the dpll version of drm_atomic_helper_swap_state() since the
helper does not handle driver-specific global state."
> {
> struct drm_i915_private *dev_priv = to_i915(state->dev);
> @@ -1725,6 +1768,11 @@ 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 = dev->dev_private;
> @@ -1768,6 +1816,16 @@ 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.
> + *
> + * Returns:
> + * A shared DPLL to be used by @crtc and @encoder with the given @crtc_state.
> + */
Major confusion here between this get and the other get. Maybe rename this
to _find_?
> struct intel_shared_dpll *
> intel_get_shared_dpll(struct intel_crtc *crtc,
> struct intel_crtc_state *crtc_state,
> --
> 2.5.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Higher level comment: Imo typing kerneldoc only makes sense if you take
the opportunity to review the exposed API with fresh eyes, and clean up
stuff before typing the documentation. Aka the stuff I've mentioned above.
Otherwise it's just busywork and extracting new files shuffling stuff
around for more noise and still not less confusion.
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
