Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756390AbcLAI1p (ORCPT ); Thu, 1 Dec 2016 03:27:45 -0500 Received: from mail.free-electrons.com ([62.4.15.54]:51019 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750864AbcLAI1o (ORCPT ); Thu, 1 Dec 2016 03:27:44 -0500 Date: Thu, 1 Dec 2016 09:27:36 +0100 From: Boris Brezillon To: Daniel Vetter Cc: Eric Anholt , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org Subject: Re: [PATCH 2/6] drm: Add TV connector states to drm_connector_state Message-ID: <20161201092736.6f03ca54@bbrezillon> In-Reply-To: <20161129201410.vpnap53xiqfpxb3u@phenom.ffwll.local> References: <20161129184202.10640-1-eric@anholt.net> <20161129184202.10640-3-eric@anholt.net> <20161129201410.vpnap53xiqfpxb3u@phenom.ffwll.local> X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.30; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6308 Lines: 172 Hi Daniel, On Tue, 29 Nov 2016 21:14:10 +0100 Daniel Vetter wrote: > 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/ 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 > > 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