Hi Daniel, On Tue, 29 Nov 2016 21:14:10 +0100 Daniel Vetter <dan...@ffwll.ch> wrote:
> On Tue, Nov 29, 2016 at 10:41:58AM -0800, Eric Anholt wrote: > > From: Boris Brezillon <boris.brezil...@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.brezil...@free-electrons.com> > > Signed-off-by: Eric Anholt <e...@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.vet...@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