| Message ID | 1475584337-8900-6-git-send-email-ander.conselvan.de.oliveira@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@intel.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 ;-) > + */ > 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). 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
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
On Wed, 19 Oct 2016, Ander Conselvan De Oliveira <conselvan2@gmail.com> wrote: > On Thu, 2016-10-13 at 15:46 +0200, Daniel Vetter wrote: >> > + /** >> > + * @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. Agreed. BR, Jani.
On Wed, Oct 19, 2016 at 06:29:13PM +0300, Jani Nikula wrote: > On Wed, 19 Oct 2016, Ander Conselvan De Oliveira <conselvan2@gmail.com> wrote: > > On Thu, 2016-10-13 at 15:46 +0200, Daniel Vetter wrote: > >> > + /** > >> > + * @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. > > Agreed. Someone volunteering? I'd hope it would be at most a bit of shuffling with the generator, we should have the type and all that handy already. Except maybe lots of corner-cases ... And ack on the struct layout bikeshed I had. -Daniel
On Thu, 20 Oct 2016, Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, Oct 19, 2016 at 06:29:13PM +0300, Jani Nikula wrote: >> On Wed, 19 Oct 2016, Ander Conselvan De Oliveira <conselvan2@gmail.com> wrote: >> > On Thu, 2016-10-13 at 15:46 +0200, Daniel Vetter wrote: >> >> > + /** >> >> > + * @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. >> >> Agreed. > > Someone volunteering? I'd hope it would be at most a bit of shuffling with > the generator, we should have the type and all that handy already. Except > maybe lots of corner-cases ... I think the problem was that I couldn't figure out how to make Sphinx do cross references within inline preformatted text. And I thought that was less important than fixing up the struct presentation that I'm not all too happy about currently. See [1] first. I don't think we need to have both the definition and members. It's just wasted vertical space. I'd suggest we drop the definition altogether, and have the members list contain the member types, ideally with cross-references. If that means having to use normal font instead of monospace, I'd go with it anyway. Thoughts? BR, Jani. [1] https://01.org/linuxgraphics/gfx-docs/drm/driver-api/infrastructure.html#c.device_driver > > And ack on the struct layout bikeshed I had. > -Daniel
On Thu, 2016-10-20 at 11:19 +0300, Jani Nikula wrote: > On Thu, 20 Oct 2016, Daniel Vetter <daniel@ffwll.ch> wrote: > > > > On Wed, Oct 19, 2016 at 06:29:13PM +0300, Jani Nikula wrote: > > > > > > On Wed, 19 Oct 2016, Ander Conselvan De Oliveira <conselvan2@gmail.com> > > > wrote: > > > > > > > > On Thu, 2016-10-13 at 15:46 +0200, Daniel Vetter wrote: > > > > > > > > > > > > > > > > > + /** > > > > > > + * @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. > > > Agreed. > > Someone volunteering? I'd hope it would be at most a bit of shuffling with > > the generator, we should have the type and all that handy already. Except > > maybe lots of corner-cases ... > I think the problem was that I couldn't figure out how to make Sphinx do > cross references within inline preformatted text. And I thought that was > less important than fixing up the struct presentation that I'm not all > too happy about currently. See [1] first. I don't think we need to have > both the definition and members. It's just wasted vertical space. > > I'd suggest we drop the definition altogether, and have the members list > contain the member types, ideally with cross-references. If that means > having to use normal font instead of monospace, I'd go with it anyway. > > Thoughts? I think that makes sense. There's no extra information there (and if you forget to add a kerneldoc tag to a field, it isn't even listed). The definition is in the code anyway. I was actually a bit surprised to see the definition in the doc the first time. Ander
On Thu, Oct 20, 2016 at 11:56:07AM +0300, Ander Conselvan De Oliveira wrote: > On Thu, 2016-10-20 at 11:19 +0300, Jani Nikula wrote: > > On Thu, 20 Oct 2016, Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > > On Wed, Oct 19, 2016 at 06:29:13PM +0300, Jani Nikula wrote: > > > > > > > > On Wed, 19 Oct 2016, Ander Conselvan De Oliveira <conselvan2@gmail.com> > > > > wrote: > > > > > > > > > > On Thu, 2016-10-13 at 15:46 +0200, Daniel Vetter wrote: > > > > > > > > > > > > > > > > > > > > + /** > > > > > > > + * @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. > > > > Agreed. > > > Someone volunteering? I'd hope it would be at most a bit of shuffling with > > > the generator, we should have the type and all that handy already. Except > > > maybe lots of corner-cases ... > > I think the problem was that I couldn't figure out how to make Sphinx do > > cross references within inline preformatted text. And I thought that was > > less important than fixing up the struct presentation that I'm not all > > too happy about currently. See [1] first. I don't think we need to have > > both the definition and members. It's just wasted vertical space. > > > > I'd suggest we drop the definition altogether, and have the members list > > contain the member types, ideally with cross-references. If that means > > having to use normal font instead of monospace, I'd go with it anyway. > > > > Thoughts? > > I think that makes sense. There's no extra information there (and if you forget > to add a kerneldoc tag to a field, it isn't even listed). The definition is in > the code anyway. > > I was actually a bit surprised to see the definition in the doc the first time. Assuming we're talking just about structs here: +1. For functions we already have the signature in the heading, and that also already hyperlinks. There's also the difference that the detailed list has the full type+name, whereas structs only have the name. I like the style used in functions much more (and then we could just nuke the definition I think), and then hyperlink them properly. -Daniel
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 + * 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. + */ 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. + */ 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 + */ + unsigned crtc_mask; + + /** + * @hw_state: hardware configuration for the DPLL. + */ 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. + */ 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 + */ 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
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@intel.com> --- 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(-)