Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933433AbcK2UOR (ORCPT ); Tue, 29 Nov 2016 15:14:17 -0500 Received: from mail-wm0-f68.google.com ([74.125.82.68]:35793 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932359AbcK2UOI (ORCPT ); Tue, 29 Nov 2016 15:14:08 -0500 Date: Tue, 29 Nov 2016 21:14:10 +0100 From: Daniel Vetter To: Eric Anholt Cc: dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/6] drm: Add TV connector states to drm_connector_state Message-ID: <20161129201410.vpnap53xiqfpxb3u@phenom.ffwll.local> Mail-Followup-To: Eric Anholt , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org References: <20161129184202.10640-1-eric@anholt.net> <20161129184202.10640-3-eric@anholt.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161129184202.10640-3-eric@anholt.net> X-Operating-System: Linux phenom 4.8.0-1-amd64 User-Agent: NeoMutt/20161104 (1.7.1) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6644 Lines: 188 On Tue, Nov 29, 2016 at 10:41:58AM -0800, Eric Anholt wrote: > From: Boris Brezillon > > 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 > Signed-off-by: Eric Anholt > --- > 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 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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch