[24/28] drm: Document drm_connector_helper_funcs
diff mbox

Message ID 1449218769-16577-25-git-send-email-daniel.vetter@ffwll.ch
State New
Headers show

Commit Message

Daniel Vetter Dec. 4, 2015, 8:46 a.m. UTC
Nothing special, except the somewhat awkard split in probe helper
callbacks between here and drm_crtc_funcs.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 include/drm/drm_modeset_helper_vtables.h | 106 +++++++++++++++++++++++++++++--
 1 file changed, 101 insertions(+), 5 deletions(-)

Comments

Thierry Reding Dec. 7, 2015, 2:42 p.m. UTC | #1
On Fri, Dec 04, 2015 at 09:46:05AM +0100, Daniel Vetter wrote:
> Nothing special, except the somewhat awkard split in probe helper

"awkward"

> callbacks between here and drm_crtc_funcs.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  include/drm/drm_modeset_helper_vtables.h | 106 +++++++++++++++++++++++++++++--
>  1 file changed, 101 insertions(+), 5 deletions(-)
> 
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index 66b78c14154e..22cc51b278fb 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -264,18 +264,114 @@ static inline void drm_encoder_helper_add(struct drm_encoder *encoder,
>  
>  /**
>   * struct drm_connector_helper_funcs - helper operations for connectors
> - * @get_modes: get mode list for this connector
> - * @mode_valid: is this mode valid on the given connector? (optional)
> - * @best_encoder: return the preferred encoder for this connector
> - * @atomic_best_encoder: atomic version of @best_encoder
>   *
> - * The helper operations are called by the mid-layer CRTC helper.
> + * These functions are used by the atomic and legacy modeset helpers and by the
> + * probe helpers.
>   */
>  struct drm_connector_helper_funcs {
> +	/**
> +	 * @get_modes:
> +	 *
> +	 * This function should fill in all modes currently valid for the sink
> +	 * into the connector->probe_modes function. It should also update the

What's probe_modes? I've never heard of it. Did you mean ->fill_modes()?
Also it's strange to say "fill into the ... function". Perhaps "pass
into the ... function" instead?

> +	 * EDID property by calling drm_mode_connector_update_edid_property().
> +	 *
> +	 * The usual way to implement this is to cache the EDID retrieved in the
> +	 * probe callback somewhere in the driver-private connector structure.
> +	 * In this function drivers then parse the modes in the EDID and add it

"add them"?

> +	 * by calling drm_add_edid_modes(). But connectors that driver a fixed

"drive"

> +	 * panel can also manually add specific modes using
> +	 * drm_mode_probed_add(). Finally drivers that support audio propably

"probably"

> +	/**
> +	 * @mode_valid:
> +	 *
> +	 * Callback to validate a mode for a connector, irrespective of the
> +	 * specific display configuration.
> +	 *
> +	 * This callback is used by the probe helpers to filter the mode list
> +	 * (which is usually derived from the EDID data block from the sink).
> +	 * See e.g. drm_helper_probe_single_connector_modes().
> +	 *
> +	 * NOTE:
> +	 *
> +	 * This only filters the mode list supplied to userspace in the
> +	 * GETCONNECOTR ioctl. Userspace is free to create modes of its own and

"GETCONNECTOR IOCTL"

> +	 * ask the kernel to use it. It this case the atomic helpers or legacy

"to use them"

> +	 * CRTC heleprs will not call this function. Drivers therefore must

"helpers"

> +	 * still fully validate any mode passed in in a modeset request.
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * Either DRM_MODE_OK or one of the failure reasons in enum

The enum value is MODE_OK, without the DRM_ prefix.

Thierry
Daniel Vetter Dec. 7, 2015, 2:48 p.m. UTC | #2
On Mon, Dec 07, 2015 at 03:42:22PM +0100, Thierry Reding wrote:
> On Fri, Dec 04, 2015 at 09:46:05AM +0100, Daniel Vetter wrote:
> > Nothing special, except the somewhat awkard split in probe helper
> 
> "awkward"
> 
> > callbacks between here and drm_crtc_funcs.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  include/drm/drm_modeset_helper_vtables.h | 106 +++++++++++++++++++++++++++++--
> >  1 file changed, 101 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> > index 66b78c14154e..22cc51b278fb 100644
> > --- a/include/drm/drm_modeset_helper_vtables.h
> > +++ b/include/drm/drm_modeset_helper_vtables.h
> > @@ -264,18 +264,114 @@ static inline void drm_encoder_helper_add(struct drm_encoder *encoder,
> >  
> >  /**
> >   * struct drm_connector_helper_funcs - helper operations for connectors
> > - * @get_modes: get mode list for this connector
> > - * @mode_valid: is this mode valid on the given connector? (optional)
> > - * @best_encoder: return the preferred encoder for this connector
> > - * @atomic_best_encoder: atomic version of @best_encoder
> >   *
> > - * The helper operations are called by the mid-layer CRTC helper.
> > + * These functions are used by the atomic and legacy modeset helpers and by the
> > + * probe helpers.
> >   */
> >  struct drm_connector_helper_funcs {
> > +	/**
> > +	 * @get_modes:
> > +	 *
> > +	 * This function should fill in all modes currently valid for the sink
> > +	 * into the connector->probe_modes function. It should also update the
> 
> What's probe_modes? I've never heard of it. Did you mean ->fill_modes()?
> Also it's strange to say "fill into the ... function". Perhaps "pass
> into the ... function" instead?

connector->probe*d*_modes *list* is what it should read. Fixed.
-Daniel
Thierry Reding Dec. 7, 2015, 3:27 p.m. UTC | #3
On Mon, Dec 07, 2015 at 03:48:37PM +0100, Daniel Vetter wrote:
> On Mon, Dec 07, 2015 at 03:42:22PM +0100, Thierry Reding wrote:
> > On Fri, Dec 04, 2015 at 09:46:05AM +0100, Daniel Vetter wrote:
> > > Nothing special, except the somewhat awkard split in probe helper
> > 
> > "awkward"
> > 
> > > callbacks between here and drm_crtc_funcs.
> > > 
> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > ---
> > >  include/drm/drm_modeset_helper_vtables.h | 106 +++++++++++++++++++++++++++++--
> > >  1 file changed, 101 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> > > index 66b78c14154e..22cc51b278fb 100644
> > > --- a/include/drm/drm_modeset_helper_vtables.h
> > > +++ b/include/drm/drm_modeset_helper_vtables.h
> > > @@ -264,18 +264,114 @@ static inline void drm_encoder_helper_add(struct drm_encoder *encoder,
> > >  
> > >  /**
> > >   * struct drm_connector_helper_funcs - helper operations for connectors
> > > - * @get_modes: get mode list for this connector
> > > - * @mode_valid: is this mode valid on the given connector? (optional)
> > > - * @best_encoder: return the preferred encoder for this connector
> > > - * @atomic_best_encoder: atomic version of @best_encoder
> > >   *
> > > - * The helper operations are called by the mid-layer CRTC helper.
> > > + * These functions are used by the atomic and legacy modeset helpers and by the
> > > + * probe helpers.
> > >   */
> > >  struct drm_connector_helper_funcs {
> > > +	/**
> > > +	 * @get_modes:
> > > +	 *
> > > +	 * This function should fill in all modes currently valid for the sink
> > > +	 * into the connector->probe_modes function. It should also update the
> > 
> > What's probe_modes? I've never heard of it. Did you mean ->fill_modes()?
> > Also it's strange to say "fill into the ... function". Perhaps "pass
> > into the ... function" instead?
> 
> connector->probe*d*_modes *list* is what it should read. Fixed.

As with all the other patches, with this and the nits fixed:

Reviewed-by: Thierry Reding <treding@nvidia.com>

Patch
diff mbox

diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index 66b78c14154e..22cc51b278fb 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -264,18 +264,114 @@  static inline void drm_encoder_helper_add(struct drm_encoder *encoder,
 
 /**
  * struct drm_connector_helper_funcs - helper operations for connectors
- * @get_modes: get mode list for this connector
- * @mode_valid: is this mode valid on the given connector? (optional)
- * @best_encoder: return the preferred encoder for this connector
- * @atomic_best_encoder: atomic version of @best_encoder
  *
- * The helper operations are called by the mid-layer CRTC helper.
+ * These functions are used by the atomic and legacy modeset helpers and by the
+ * probe helpers.
  */
 struct drm_connector_helper_funcs {
+	/**
+	 * @get_modes:
+	 *
+	 * This function should fill in all modes currently valid for the sink
+	 * into the connector->probe_modes function. It should also update the
+	 * EDID property by calling drm_mode_connector_update_edid_property().
+	 *
+	 * The usual way to implement this is to cache the EDID retrieved in the
+	 * probe callback somewhere in the driver-private connector structure.
+	 * In this function drivers then parse the modes in the EDID and add it
+	 * by calling drm_add_edid_modes(). But connectors that driver a fixed
+	 * panel can also manually add specific modes using
+	 * drm_mode_probed_add(). Finally drivers that support audio propably
+	 * want to update the ELD data, too, using drm_edid_to_eld().
+	 *
+	 * This function is only called after the ->detect() hook has indicated
+	 * that a sink is connected and when the EDID isn't overriden through
+	 * sysfs or the kernel commandline.
+	 *
+	 * This callback is used by the probe helpers in e.g.
+	 * drm_helper_probe_single_connector_modes().
+	 *
+	 * RETURNS:
+	 *
+	 * The number of modes added by calling drm_mode_probed_add().
+	 */
 	int (*get_modes)(struct drm_connector *connector);
+
+	/**
+	 * @mode_valid:
+	 *
+	 * Callback to validate a mode for a connector, irrespective of the
+	 * specific display configuration.
+	 *
+	 * This callback is used by the probe helpers to filter the mode list
+	 * (which is usually derived from the EDID data block from the sink).
+	 * See e.g. drm_helper_probe_single_connector_modes().
+	 *
+	 * NOTE:
+	 *
+	 * This only filters the mode list supplied to userspace in the
+	 * GETCONNECOTR ioctl. Userspace is free to create modes of its own and
+	 * ask the kernel to use it. It this case the atomic helpers or legacy
+	 * CRTC heleprs will not call this function. Drivers therefore must
+	 * still fully validate any mode passed in in a modeset request.
+	 *
+	 * RETURNS:
+	 *
+	 * Either DRM_MODE_OK or one of the failure reasons in enum
+	 * &drm_mode_status.
+	 */
 	enum drm_mode_status (*mode_valid)(struct drm_connector *connector,
 					   struct drm_display_mode *mode);
+	/**
+	 * @best_encoder:
+	 *
+	 * This function should select the best encoder for the given connector.
+	 *
+	 * This function is used by both the atomic helpers (in the
+	 * drm_atomic_helper_check_modeset() function) and in the legacy CRTC
+	 * helpers.
+	 *
+	 * NOTE:
+	 *
+	 * In atomic drivers this function is called in the check phase of an
+	 * atomic update. The driver is not allowed to change or inspect
+	 * anything outside of arguments passed-in. Atomic drivers which need to
+	 * inspect dynamic configuration state should instead use
+	 * @atomic_best_encoder.
+	 *
+	 * RETURNS:
+	 *
+	 * Encoder that should be used for the given connector and connector
+	 * state, or NULL if no suitable encoder exists. Note that the helpers
+	 * will ensure that encoders aren't used twice, drivers should not check
+	 * for this.
+	 */
 	struct drm_encoder *(*best_encoder)(struct drm_connector *connector);
+
+	/**
+	 * @atomic_best_encoder:
+	 *
+	 * This is the atomic version of @best_encoder for atomic drivers which
+	 * need to select the best encoder depending upon the desired
+	 * configuration and can't select it statically.
+	 *
+	 * This function is used by drm_atomic_helper_check_modeset() and either
+	 * this or @best_encoder is required.
+	 *
+	 * NOTE:
+	 *
+	 * This function is called in the check phase of an atomic update. The
+	 * driver is not allowed to change anything outside of the free-standing
+	 * state objects passed-in or assembled in the overall &drm_atomic_state
+	 * update tracking structure.
+	 *
+	 * RETURNS:
+	 *
+	 * Encoder that should be used for the given connector and connector
+	 * state, or NULL if no suitable encoder exists. Note that the helpers
+	 * will ensure that encoders aren't used twice, drivers should not check
+	 * for this.
+	 */
 	struct drm_encoder *(*atomic_best_encoder)(struct drm_connector *connector,
 						   struct drm_connector_state *connector_state);
 };