From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752707AbdECWAz (ORCPT ); Wed, 3 May 2017 18:00:55 -0400 Received: from mga01.intel.com ([192.55.52.88]:49963 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751216AbdECWAr (ORCPT ); Wed, 3 May 2017 18:00:47 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.38,284,1491289200"; d="scan'208";a="83385527" Date: Wed, 3 May 2017 15:05:23 -0700 From: Manasi Navare To: Jose Abreu Cc: dri-devel@lists.freedesktop.org, Daniel Vetter , Alexey Brodkin , linux-kernel@vger.kernel.org, Carlos Palminha Subject: Re: [PATCH v2 1/2] drm: Introduce crtc->mode_valid() callback Message-ID: <20170503220523.GE3176@intel.com> References: <5316810123fc13dd9f5caac4188b9a9b8ab07240.1493386261.git.joabreu@synopsys.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <5316810123fc13dd9f5caac4188b9a9b8ab07240.1493386261.git.joabreu@synopsys.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Apr 28, 2017 at 02:47:11PM +0100, Jose Abreu wrote: > Some crtc's may have restrictions in the mode they can display. In > this patch a new callback (crtc->mode_valid()) is introduced that > is called at the same stage of connector->mode_valid() callback. > > This shall be implemented if the crtc has some sort of restriction > so that we don't probe modes that will fail in the commit() stage. > For example: A given crtc may be responsible to set a clock value. > If the clock can not produce all the values for the available > modes then this callback can be used to restrict the number of > probbed modes to only the ones that can be displayed. > > If the crtc does not implement the callback then the behaviour will > remain the same. Also, for a given set of crtcs that can be bound to > the connector, if at least one can display the mode then the mode > will be probbed. > > Signed-off-by: Jose Abreu > Cc: Carlos Palminha > Cc: Alexey Brodkin > Cc: Ville Syrjälä > Cc: Daniel Vetter > Cc: Dave Airlie > Cc: Andrzej Hajda > --- > > Changes v1->v2: > - Correct indentation > - Change function name to drm_validate_connector > - Move connnector->mode_valid() to new function > - Change crtc->mode_valid() "mode" field to const > - Code reogarnization > - Return earlier if crtc accepts mode > > drivers/gpu/drm/drm_probe_helper.c | 50 ++++++++++++++++++++++++++++++-- > include/drm/drm_modeset_helper_vtables.h | 26 +++++++++++++++++ > 2 files changed, 74 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c > index 1b0c14a..0741f36 100644 > --- a/drivers/gpu/drm/drm_probe_helper.c > +++ b/drivers/gpu/drm/drm_probe_helper.c > @@ -80,6 +80,52 @@ > return MODE_OK; > } > > +static enum drm_mode_status > +drm_mode_validate_connector(struct drm_connector *connector, > + struct drm_display_mode *mode) > +{ > + const struct drm_connector_helper_funcs *connector_funcs = > + connector->helper_private; > + struct drm_device *dev = connector->dev; > + uint32_t *ids = connector->encoder_ids; > + enum drm_mode_status ret = MODE_OK; > + unsigned int i; > + > + /* Step 1: Validate against connector */ > + if (connector_funcs->mode_valid) > + ret = connector_funcs->mode_valid(connector, mode); > + > + if (ret != MODE_OK) > + return ret; > + > + /* Step 2: Validate against crtc's */ > + for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) { > + struct drm_encoder *encoder = drm_encoder_find(dev, ids[i]); > + struct drm_crtc *crtc; > + > + if (!encoder) > + continue; > + > + drm_for_each_crtc(crtc, dev) { > + const struct drm_crtc_helper_funcs *crtc_funcs; > + > + if (!drm_encoder_crtc_ok(encoder, crtc)) > + continue; > + > + crtc_funcs = crtc->helper_private; > + if (!crtc_funcs || !crtc_funcs->mode_valid) > + return MODE_OK; /* crtc accepts everything */ > + > + ret = crtc_funcs->mode_valid(crtc, mode); > + if (ret == MODE_OK) > + return ret; > + } > + } > + > + /* NOTE: If no crtc or encoder is found then we return MODE_OK */ I think this comment is misleading because here ret is not alwyas MODE_OK. It will be the return value from crtc_func->mode_valid which could also be MODE_NOCLOCK Manasi > + return ret; > +} > + > static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector) > { > struct drm_cmdline_mode *cmdline_mode; > @@ -428,8 +474,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector, > if (mode->status == MODE_OK) > mode->status = drm_mode_validate_flag(mode, mode_flags); > > - if (mode->status == MODE_OK && connector_funcs->mode_valid) > - mode->status = connector_funcs->mode_valid(connector, > + if (mode->status == MODE_OK) > + mode->status = drm_mode_validate_connector(connector, > mode); > } > > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h > index c01c328..ecb055c 100644 > --- a/include/drm/drm_modeset_helper_vtables.h > +++ b/include/drm/drm_modeset_helper_vtables.h > @@ -106,6 +106,32 @@ struct drm_crtc_helper_funcs { > void (*commit)(struct drm_crtc *crtc); > > /** > + * @mode_valid: > + * > + * This callback should be implemented if the crtc has some sort of > + * restriction in the modes it can display. For example, a given crtc > + * may be responsible to set a clock value. If the clock can not > + * produce all the values for the available modes then this callback > + * can be used to restrict the number of probbed modes to only the ones > + * that can be displayed. > + * > + * This is directly called at the same stage of connector->mode_valid > + * callback. > + * > + * NOTE: > + * > + * For a given set of crtc's in a drm_device, if at least one does not > + * have the mode_valid callback, or, at least one returns MODE_OK then > + * the mode will be probbed. > + * > + * RETURNS: > + * > + * drm_mode_status Enum > + */ > + enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc, > + const struct drm_display_mode *mode); > + > + /** > * @mode_fixup: > * > * This callback is used to validate a mode. The parameter mode is the > -- > 1.9.1 > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel