| Message ID | 1449218769-16577-25-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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>
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); };
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(-)