| Message ID | 20161129184202.10640-3-eric@anholt.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Nov 29, 2016 at 10:41:58AM -0800, Eric Anholt wrote: > From: Boris Brezillon <boris.brezillon@free-electrons.com> > > Some generic TV connector properties are exposed in drm_mode_config, but > they are currently handled independently in each DRM encoder driver. > > Extend the drm_connector_state to store TV related states, and modify the > drm_atomic_connector_{set,get}_property() helpers to fill the connector > state accordingly. > > Each driver is then responsible for checking and applying the new config > in its ->atomic_mode_{check,set}() operations. > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > Signed-off-by: Eric Anholt <eric@anholt.net> > --- > drivers/gpu/drm/drm_atomic.c | 50 ++++++++++++++++++++++++++++++++++++++++++++ > include/drm/drm_connector.h | 32 ++++++++++++++++++++++++++++ > 2 files changed, 82 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 23739609427d..02b0668f51e1 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -986,12 +986,38 @@ int drm_atomic_connector_set_property(struct drm_connector *connector, > * now?) atomic writes to DPMS property: > */ > return -EINVAL; > + } else if (property == config->tv_select_subconnector_property) { > + state->tv.subconnector = val; > + } else if (property == config->tv_left_margin_property) { > + state->tv.margins.left = val; > + } else if (property == config->tv_right_margin_property) { > + state->tv.margins.right = val; > + } else if (property == config->tv_top_margin_property) { > + state->tv.margins.bottom = val; s/bottom/top/ > + } else if (property == config->tv_bottom_margin_property) { > + state->tv.margins.right = val; s/right/bottom/ > + } else if (property == config->tv_mode_property) { > + state->tv.mode = val; > + } else if (property == config->tv_brightness_property) { > + state->tv.brightness = val; > + } else if (property == config->tv_contrast_property) { > + state->tv.contrast = val; > + } else if (property == config->tv_flicker_reduction_property) { > + state->tv.flicker_reduction = val; > + } else if (property == config->tv_overscan_property) { > + state->tv.overscan = val; > + } else if (property == config->tv_saturation_property) { > + state->tv.saturation = val; > + } else if (property == config->tv_hue_property) { > + state->tv.hue = val; > } else if (connector->funcs->atomic_set_property) { > return connector->funcs->atomic_set_property(connector, > state, property, val); > } else { > return -EINVAL; > } > + > + return 0; > } > EXPORT_SYMBOL(drm_atomic_connector_set_property); > > @@ -1022,6 +1048,30 @@ drm_atomic_connector_get_property(struct drm_connector *connector, > *val = (state->crtc) ? state->crtc->base.id : 0; > } else if (property == config->dpms_property) { > *val = connector->dpms; > + } else if (property == config->tv_select_subconnector_property) { > + *val = state->tv.subconnector; > + } else if (property == config->tv_left_margin_property) { > + *val = state->tv.margins.left; > + } else if (property == config->tv_right_margin_property) { > + *val = state->tv.margins.right; > + } else if (property == config->tv_top_margin_property) { > + *val = state->tv.margins.bottom; s/bottom/top/ > + } else if (property == config->tv_bottom_margin_property) { > + *val = state->tv.margins.right; s/right/bottom/ > + } else if (property == config->tv_mode_property) { > + *val = state->tv.mode; > + } else if (property == config->tv_brightness_property) { > + *val = state->tv.brightness; > + } else if (property == config->tv_contrast_property) { > + *val = state->tv.contrast; > + } else if (property == config->tv_flicker_reduction_property) { > + *val = state->tv.flicker_reduction; > + } else if (property == config->tv_overscan_property) { > + *val = state->tv.overscan; > + } else if (property == config->tv_saturation_property) { > + *val = state->tv.saturation; > + } else if (property == config->tv_hue_property) { > + *val = state->tv.hue; > } else if (connector->funcs->atomic_get_property) { > return connector->funcs->atomic_get_property(connector, > state, property, val); > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > index ac9d7d8e0e43..2382d44e5fff 100644 > --- a/include/drm/drm_connector.h > +++ b/include/drm/drm_connector.h > @@ -194,10 +194,40 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info, > unsigned int num_formats); > > /** > + * struct drm_tv_connector_state - TV connector related states > + * @subconnector: selected subconnector > + * @margins: left/right/top/bottom margins > + * @mode: TV mode > + * @brightness: brightness in percent > + * @contrast: contrast in percent > + * @flicker_reduction: flicker reduction in percent > + * @overscan: overscan in percent > + * @saturation: saturation in percent > + * @hue: hue in percent > + */ > +struct drm_tv_connector_state { > + int subconnector; I think an explicit enum for the possible values would be nice. Slightly annoying/fragile since it needs to match the uabi ones, but we I think there's a slight preference to explicit enums internally. > + struct { > + int left; > + int right; > + int top; > + int bottom; > + } margins; > + int mode; > + int brightness; > + int contrast; > + int flicker_reduction; > + int overscan; > + int saturation; > + int hue; All the above properties are unsigned when exposed to userspace. I think for consistency would be good to make them match on the kernel side too. With the above issues addressed: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> And since no one else seems to work on an atomic TV encoder feel free to just stash into the next vc4 pull request. -Daniel > +}; > + > +/** > * struct drm_connector_state - mutable connector state > * @connector: backpointer to the connector > * @best_encoder: can be used by helpers and drivers to select the encoder > * @state: backpointer to global drm_atomic_state > + * @tv: TV connector state > */ > struct drm_connector_state { > struct drm_connector *connector; > @@ -213,6 +243,8 @@ struct drm_connector_state { > struct drm_encoder *best_encoder; > > struct drm_atomic_state *state; > + > + struct drm_tv_connector_state tv; > }; > > /** > -- > 2.10.2 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Daniel, On Tue, 29 Nov 2016 21:14:10 +0100 Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Nov 29, 2016 at 10:41:58AM -0800, Eric Anholt wrote: > > From: Boris Brezillon <boris.brezillon@free-electrons.com> > > > > Some generic TV connector properties are exposed in drm_mode_config, but > > they are currently handled independently in each DRM encoder driver. > > > > Extend the drm_connector_state to store TV related states, and modify the > > drm_atomic_connector_{set,get}_property() helpers to fill the connector > > state accordingly. > > > > Each driver is then responsible for checking and applying the new config > > in its ->atomic_mode_{check,set}() operations. > > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > > Signed-off-by: Eric Anholt <eric@anholt.net> > > --- > > drivers/gpu/drm/drm_atomic.c | 50 ++++++++++++++++++++++++++++++++++++++++++++ > > include/drm/drm_connector.h | 32 ++++++++++++++++++++++++++++ > > 2 files changed, 82 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > index 23739609427d..02b0668f51e1 100644 > > --- a/drivers/gpu/drm/drm_atomic.c > > +++ b/drivers/gpu/drm/drm_atomic.c > > @@ -986,12 +986,38 @@ int drm_atomic_connector_set_property(struct drm_connector *connector, > > * now?) atomic writes to DPMS property: > > */ > > return -EINVAL; > > + } else if (property == config->tv_select_subconnector_property) { > > + state->tv.subconnector = val; > > + } else if (property == config->tv_left_margin_property) { > > + state->tv.margins.left = val; > > + } else if (property == config->tv_right_margin_property) { > > + state->tv.margins.right = val; > > + } else if (property == config->tv_top_margin_property) { > > + state->tv.margins.bottom = val; > > s/bottom/top/ > > > + } else if (property == config->tv_bottom_margin_property) { > > + state->tv.margins.right = val; > > s/right/bottom/ > > > + } else if (property == config->tv_mode_property) { > > + state->tv.mode = val; > > + } else if (property == config->tv_brightness_property) { > > + state->tv.brightness = val; > > + } else if (property == config->tv_contrast_property) { > > + state->tv.contrast = val; > > + } else if (property == config->tv_flicker_reduction_property) { > > + state->tv.flicker_reduction = val; > > + } else if (property == config->tv_overscan_property) { > > + state->tv.overscan = val; > > + } else if (property == config->tv_saturation_property) { > > + state->tv.saturation = val; > > + } else if (property == config->tv_hue_property) { > > + state->tv.hue = val; > > } else if (connector->funcs->atomic_set_property) { > > return connector->funcs->atomic_set_property(connector, > > state, property, val); > > } else { > > return -EINVAL; > > } > > + > > + return 0; > > } > > EXPORT_SYMBOL(drm_atomic_connector_set_property); > > > > @@ -1022,6 +1048,30 @@ drm_atomic_connector_get_property(struct drm_connector *connector, > > *val = (state->crtc) ? state->crtc->base.id : 0; > > } else if (property == config->dpms_property) { > > *val = connector->dpms; > > + } else if (property == config->tv_select_subconnector_property) { > > + *val = state->tv.subconnector; > > + } else if (property == config->tv_left_margin_property) { > > + *val = state->tv.margins.left; > > + } else if (property == config->tv_right_margin_property) { > > + *val = state->tv.margins.right; > > + } else if (property == config->tv_top_margin_property) { > > + *val = state->tv.margins.bottom; > > s/bottom/top/ > > + } else if (property == config->tv_bottom_margin_property) { > > + *val = state->tv.margins.right; > > s/right/bottom/ Oops. I'll fix those typos. > > > + } else if (property == config->tv_mode_property) { > > + *val = state->tv.mode; > > + } else if (property == config->tv_brightness_property) { > > + *val = state->tv.brightness; > > + } else if (property == config->tv_contrast_property) { > > + *val = state->tv.contrast; > > + } else if (property == config->tv_flicker_reduction_property) { > > + *val = state->tv.flicker_reduction; > > + } else if (property == config->tv_overscan_property) { > > + *val = state->tv.overscan; > > + } else if (property == config->tv_saturation_property) { > > + *val = state->tv.saturation; > > + } else if (property == config->tv_hue_property) { > > + *val = state->tv.hue; > > } else if (connector->funcs->atomic_get_property) { > > return connector->funcs->atomic_get_property(connector, > > state, property, val); > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > > index ac9d7d8e0e43..2382d44e5fff 100644 > > --- a/include/drm/drm_connector.h > > +++ b/include/drm/drm_connector.h > > @@ -194,10 +194,40 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info, > > unsigned int num_formats); > > > > /** > > + * struct drm_tv_connector_state - TV connector related states > > + * @subconnector: selected subconnector > > + * @margins: left/right/top/bottom margins > > + * @mode: TV mode > > + * @brightness: brightness in percent > > + * @contrast: contrast in percent > > + * @flicker_reduction: flicker reduction in percent > > + * @overscan: overscan in percent > > + * @saturation: saturation in percent > > + * @hue: hue in percent > > + */ > > +struct drm_tv_connector_state { > > + int subconnector; > > I think an explicit enum for the possible values would be nice. Slightly > annoying/fragile since it needs to match the uabi ones, but we I think > there's a slight preference to explicit enums internally. Okay. > > > + struct { > > + int left; > > + int right; > > + int top; > > + int bottom; > > + } margins; > > + int mode; > > + int brightness; > > + int contrast; > > + int flicker_reduction; > > + int overscan; > > + int saturation; > > + int hue; > > All the above properties are unsigned when exposed to userspace. I think > for consistency would be good to make them match on the kernel side too. Sure. > > With the above issues addressed: > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > And since no one else seems to work on an atomic TV encoder feel free to > just stash into the next vc4 pull request. Thanks for the review. Boris
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 23739609427d..02b0668f51e1 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -986,12 +986,38 @@ int drm_atomic_connector_set_property(struct drm_connector *connector, * now?) atomic writes to DPMS property: */ return -EINVAL; + } else if (property == config->tv_select_subconnector_property) { + state->tv.subconnector = val; + } else if (property == config->tv_left_margin_property) { + state->tv.margins.left = val; + } else if (property == config->tv_right_margin_property) { + state->tv.margins.right = val; + } else if (property == config->tv_top_margin_property) { + state->tv.margins.bottom = val; + } else if (property == config->tv_bottom_margin_property) { + state->tv.margins.right = val; + } else if (property == config->tv_mode_property) { + state->tv.mode = val; + } else if (property == config->tv_brightness_property) { + state->tv.brightness = val; + } else if (property == config->tv_contrast_property) { + state->tv.contrast = val; + } else if (property == config->tv_flicker_reduction_property) { + state->tv.flicker_reduction = val; + } else if (property == config->tv_overscan_property) { + state->tv.overscan = val; + } else if (property == config->tv_saturation_property) { + state->tv.saturation = val; + } else if (property == config->tv_hue_property) { + state->tv.hue = val; } else if (connector->funcs->atomic_set_property) { return connector->funcs->atomic_set_property(connector, state, property, val); } else { return -EINVAL; } + + return 0; } EXPORT_SYMBOL(drm_atomic_connector_set_property); @@ -1022,6 +1048,30 @@ drm_atomic_connector_get_property(struct drm_connector *connector, *val = (state->crtc) ? state->crtc->base.id : 0; } else if (property == config->dpms_property) { *val = connector->dpms; + } else if (property == config->tv_select_subconnector_property) { + *val = state->tv.subconnector; + } else if (property == config->tv_left_margin_property) { + *val = state->tv.margins.left; + } else if (property == config->tv_right_margin_property) { + *val = state->tv.margins.right; + } else if (property == config->tv_top_margin_property) { + *val = state->tv.margins.bottom; + } else if (property == config->tv_bottom_margin_property) { + *val = state->tv.margins.right; + } else if (property == config->tv_mode_property) { + *val = state->tv.mode; + } else if (property == config->tv_brightness_property) { + *val = state->tv.brightness; + } else if (property == config->tv_contrast_property) { + *val = state->tv.contrast; + } else if (property == config->tv_flicker_reduction_property) { + *val = state->tv.flicker_reduction; + } else if (property == config->tv_overscan_property) { + *val = state->tv.overscan; + } else if (property == config->tv_saturation_property) { + *val = state->tv.saturation; + } else if (property == config->tv_hue_property) { + *val = state->tv.hue; } else if (connector->funcs->atomic_get_property) { return connector->funcs->atomic_get_property(connector, state, property, val); diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index ac9d7d8e0e43..2382d44e5fff 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -194,10 +194,40 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info, unsigned int num_formats); /** + * struct drm_tv_connector_state - TV connector related states + * @subconnector: selected subconnector + * @margins: left/right/top/bottom margins + * @mode: TV mode + * @brightness: brightness in percent + * @contrast: contrast in percent + * @flicker_reduction: flicker reduction in percent + * @overscan: overscan in percent + * @saturation: saturation in percent + * @hue: hue in percent + */ +struct drm_tv_connector_state { + int subconnector; + struct { + int left; + int right; + int top; + int bottom; + } margins; + int mode; + int brightness; + int contrast; + int flicker_reduction; + int overscan; + int saturation; + int hue; +}; + +/** * struct drm_connector_state - mutable connector state * @connector: backpointer to the connector * @best_encoder: can be used by helpers and drivers to select the encoder * @state: backpointer to global drm_atomic_state + * @tv: TV connector state */ struct drm_connector_state { struct drm_connector *connector; @@ -213,6 +243,8 @@ struct drm_connector_state { struct drm_encoder *best_encoder; struct drm_atomic_state *state; + + struct drm_tv_connector_state tv; }; /**