From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752431AbdEJJFP (ORCPT ); Wed, 10 May 2017 05:05:15 -0400 Received: from smtprelay4.synopsys.com ([198.182.47.9]:51345 "EHLO smtprelay.synopsys.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751735AbdEJJFN (ORCPT ); Wed, 10 May 2017 05:05:13 -0400 Subject: Re: [PATCH v2 1/8] drm: Add crtc/encoder/bridge->mode_valid() callbacks To: Daniel Vetter References: <0187832f758eb489258212b0ec917d5013366fe5.1494347165.git.joabreu@synopsys.com> <20170510080337.iqesj4zqvaewpqk2@phenom.ffwll.local> CC: Jose Abreu , , , Carlos Palminha , Alexey Brodkin , =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= , Dave Airlie , Andrzej Hajda , Archit Taneja From: Jose Abreu Message-ID: Date: Wed, 10 May 2017 10:05:08 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20170510080337.iqesj4zqvaewpqk2@phenom.ffwll.local> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 8bit X-Originating-IP: [10.107.19.62] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Daniel, On 10-05-2017 09:03, Daniel Vetter wrote: > On Tue, May 09, 2017 at 06:00:08PM +0100, Jose Abreu wrote: >> This adds a new callback to crtc, encoder and bridge helper functions >> called mode_valid(). This callback shall be implemented if the >> corresponding component has some sort of restriction in the modes >> that can be displayed. A NULL callback implicates that the component >> can display all the modes. >> >> We also change the description of connector->mode_valid() callback >> so that it matches the existing behaviour: It is never called in >> atomic check phase. >> >> Only the callbacks were implemented to simplify review process, >> following patches will make use of them. >> >> Signed-off-by: Jose Abreu >> Cc: Carlos Palminha >> Cc: Alexey Brodkin >> Cc: Ville Syrjälä >> Cc: Daniel Vetter >> Cc: Dave Airlie >> Cc: Andrzej Hajda >> Cc: Archit Taneja >> --- >> >> Changes v1->v2: >> - Change description of connector->mode_valid() (Daniel) >> >> include/drm/drm_bridge.h | 20 ++++++++++++++ >> include/drm/drm_modeset_helper_vtables.h | 45 ++++++++++++++++++++++++++++++++ >> 2 files changed, 65 insertions(+) >> >> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h >> index fdd82fc..00c6c36 100644 >> --- a/include/drm/drm_bridge.h >> +++ b/include/drm/drm_bridge.h >> @@ -59,6 +59,26 @@ struct drm_bridge_funcs { >> void (*detach)(struct drm_bridge *bridge); >> >> /** >> + * @mode_valid: >> + * >> + * This callback is used to check if a specific mode is valid in this >> + * bridge. This should be implemented if the bridge has some sort of >> + * restriction in the modes it can display. For example, a given bridge >> + * 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 modes to only the ones that >> + * can be displayed. >> + * >> + * This is called at mode probe and at atomic check phase. >> + * >> + * RETURNS: >> + * >> + * drm_mode_status Enum >> + */ >> + enum drm_mode_status (*mode_valid)(struct drm_bridge *crtc, >> + const struct drm_display_mode *mode); >> + >> + /** >> * @mode_fixup: >> * >> * This callback is used to validate and adjust a mode. The paramater >> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h >> index c01c328..eec2c70 100644 >> --- a/include/drm/drm_modeset_helper_vtables.h >> +++ b/include/drm/drm_modeset_helper_vtables.h >> @@ -106,6 +106,26 @@ struct drm_crtc_helper_funcs { >> void (*commit)(struct drm_crtc *crtc); >> >> /** >> + * @mode_valid: >> + * >> + * This callback is used to check if a specific mode is valid in this >> + * crtc. This 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 modes to only the ones that >> + * can be displayed. >> + * >> + * This is called at mode probe and at atomic check phase. >> + * >> + * 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 >> @@ -457,6 +477,26 @@ struct drm_encoder_helper_funcs { >> void (*dpms)(struct drm_encoder *encoder, int mode); >> >> /** >> + * @mode_valid: >> + * >> + * This callback is used to check if a specific mode is valid in this >> + * encoder. This should be implemented if the encoder has some sort >> + * of restriction in the modes it can display. For example, a given >> + * encoder 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 modes to only the ones that >> + * can be displayed. >> + * >> + * This is called at mode probe and at atomic check phase. >> + * >> + * RETURNS: >> + * >> + * drm_mode_status Enum >> + */ >> + enum drm_mode_status (*mode_valid)(struct drm_encoder *crtc, >> + const struct drm_display_mode *mode); >> + >> + /** >> * @mode_fixup: >> * >> * This callback is used to validate and adjust a mode. The parameter >> @@ -795,6 +835,11 @@ struct drm_connector_helper_funcs { >> * (which is usually derived from the EDID data block from the sink). >> * See e.g. drm_helper_probe_single_connector_modes(). >> * >> + * This callback is never called in atomic check phase so that userspace >> + * can override kernel sink checks in case of broken EDID with wrong >> + * limits from the sink. You can use the remaining mode_valid() >> + * callbacks to validate the mode against your video path. >> + * >> * NOTE: >> * >> * This only filters the mode list supplied to userspace in the > Kerneldoc review seems to still be missing. One case that needs to be > updated is this note here. But there's a pile of other places where we > reference one of the mode_valid or mode_fixup functions, and they should > all be updated. > > Also, it'd be good to explain what to put into mode_valid and what to put > into mode_fixup, for objects which have both. I can help with this, but I > think it'd be good if you make a first round, since that might catch some > interactions we've missed. > > Thanks, Daniel Ok, I will need some time to review and update this. I think until the end of this week I will have another version to send. Thanks! Best regards, Jose Miguel Abreu