[PATCH v2 0/8] Introduce new mode validation callbacks

[PATCH v2 0/8] Introduce new mode validation callbacks

[Jose Abreu]

This series is a follow up from the discussion at [1]. We start by
introducing crtc->mode_valid(), encoder->mode_valid() and
bridge->mode_valid() callbacks which will be used in followup
patches.

We proceed by introducing new helpers to call this new callbacks
at 2/8, 3/8 and 4/8.

Next, at 5/8 we modify the connector probe helper so that only modes
which are supported by a given encoder+crtc combination are probbed.

At 6/8 a helper function is introduced that calls all mode_valid()
from a set of bridges.

At 7/8 we call all the mode_valid() callbacks for a given pipeline,
except the connector->mode_valid one, so that the mode is validated.
This is done before calling mode_fixup().

Finally, at 8/8 we use the new crtc->mode_valid() callback in arcpgu
and remove the atomic_check() callback.

[1] https://patchwork.kernel.org/patch/9702233/

Jose Abreu (8):
  drm: Add crtc/encoder/bridge->mode_valid() callbacks
  drm: Add drm_crtc_mode_valid()
  drm: Add drm_encoder_mode_valid()
  drm: Add drm_connector_mode_valid()
  drm: Use new mode_valid() helpers in connector probe helper
  drm: Introduce drm_bridge_mode_valid()
  drm: Use mode_valid() in atomic modeset
  drm: arc: Use crtc->mode_valid() callback

Cc: Carlos Palminha <palminha@synopsys.com>
Cc: Alexey Brodkin <abrodkin@synopsys.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Dave Airlie <airlied@linux.ie>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Archit Taneja <architt@codeaurora.org>

 drivers/gpu/drm/arc/arcpgu_crtc.c        | 39 ++++++++++-------
 drivers/gpu/drm/drm_atomic_helper.c      | 75 ++++++++++++++++++++++++++++++--
 drivers/gpu/drm/drm_bridge.c             | 33 ++++++++++++++
 drivers/gpu/drm/drm_connector.c          | 23 ++++++++++
 drivers/gpu/drm/drm_crtc.c               | 22 ++++++++++
 drivers/gpu/drm/drm_crtc_internal.h      |  7 +++
 drivers/gpu/drm/drm_encoder.c            | 23 ++++++++++
 drivers/gpu/drm/drm_probe_helper.c       | 60 +++++++++++++++++++++++--
 include/drm/drm_bridge.h                 | 22 ++++++++++
 include/drm/drm_modeset_helper_vtables.h | 45 +++++++++++++++++++
 10 files changed, 328 insertions(+), 21 deletions(-)

-- 
1.9.1

[PATCH v2 1/8] drm: Add crtc/encoder/bridge->mode_valid() callbacks

[Jose Abreu]

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 <joabreu@synopsys.com>
Cc: Carlos Palminha <palminha@synopsys.com>
Cc: Alexey Brodkin <abrodkin@synopsys.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Dave Airlie <airlied@linux.ie>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Archit Taneja <architt@codeaurora.org>
---

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
-- 
1.9.1

Re: [PATCH v2 1/8] drm: Add crtc/encoder/bridge->mode_valid() callbacks

[Daniel Vetter]

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 <joabreu@synopsys.com>
> Cc: Carlos Palminha <palminha@synopsys.com>
> Cc: Alexey Brodkin <abrodkin@synopsys.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Dave Airlie <airlied@linux.ie>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Archit Taneja <architt@codeaurora.org>
> ---
> 
> 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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2 1/8] drm: Add crtc/encoder/bridge->mode_valid() callbacks

[Jose Abreu]

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 <joabreu@synopsys.com>
>> Cc: Carlos Palminha <palminha@synopsys.com>
>> Cc: Alexey Brodkin <abrodkin@synopsys.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Dave Airlie <airlied@linux.ie>
>> Cc: Andrzej Hajda <a.hajda@samsung.com>
>> Cc: Archit Taneja <architt@codeaurora.org>
>> ---
>>
>> 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

Re: [PATCH v2 1/8] drm: Add crtc/encoder/bridge->mode_valid() callbacks

[Laurent Pinchart]

Hi Daniel,

On Wednesday 10 May 2017 10:03:37 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 <joabreu@synopsys.com>
> > Cc: Carlos Palminha <palminha@synopsys.com>
> > Cc: Alexey Brodkin <abrodkin@synopsys.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Dave Airlie <airlied@linux.ie>
> > Cc: Andrzej Hajda <a.hajda@samsung.com>
> > Cc: Archit Taneja <architt@codeaurora.org>
> > ---
> > 
> > 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.

I was going to mention that. Interactions between mode_valid and mode_fixup 
are not defined clearly. Additionally, even though it might be a bit out of 
scope for this patch series, I think we should also define what mode_fixup is 
allowed to fix and what it should reject straight away.

Thinking about it, do we really need two separate operations ? As I understand 
it, mode_fixup is mostly (only ? - this is where we need documentation) used 
to fixup the pixel clock frequency as clock generators usually have 
limitations in their dividers. We assume that the sync won't care too much, 
and happily feed it with a mode that is slightly different from what userspace 
requested. Given that mode_valid should accepts mode for which the exact pixel 
clock frequency can't bee achieved, and that the atomic commit will fixup that 
frequency anyway, can't we apply the same processing to modes enumerated by 
the connector, and merge the mode_valid and mode_fixup operations ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/8] drm: Add crtc/encoder/bridge->mode_valid() callbacks

[Daniel Vetter]

On Fri, May 12, 2017 at 11:24:12AM +0300, Laurent Pinchart wrote:
> Hi Daniel,
> 
> On Wednesday 10 May 2017 10:03:37 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 <joabreu@synopsys.com>
> > > Cc: Carlos Palminha <palminha@synopsys.com>
> > > Cc: Alexey Brodkin <abrodkin@synopsys.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: Dave Airlie <airlied@linux.ie>
> > > Cc: Andrzej Hajda <a.hajda@samsung.com>
> > > Cc: Archit Taneja <architt@codeaurora.org>
> > > ---
> > > 
> > > 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.
> 
> I was going to mention that. Interactions between mode_valid and mode_fixup 
> are not defined clearly. Additionally, even though it might be a bit out of 
> scope for this patch series, I think we should also define what mode_fixup is 
> allowed to fix and what it should reject straight away.
> 
> Thinking about it, do we really need two separate operations ? As I understand 
> it, mode_fixup is mostly (only ? - this is where we need documentation) used 
> to fixup the pixel clock frequency as clock generators usually have 
> limitations in their dividers. We assume that the sync won't care too much, 
> and happily feed it with a mode that is slightly different from what userspace 
> requested. Given that mode_valid should accepts mode for which the exact pixel 
> clock frequency can't bee achieved, and that the atomic commit will fixup that 
> frequency anyway, can't we apply the same processing to modes enumerated by 
> the connector, and merge the mode_valid and mode_fixup operations ?

They are fairly similar, except mode_fixup also has the adjusted mode, and
needs to fill that one out. With atomic there's also the complication
that drivers with not-so-simple checks use atomic_check, and we really
can't fake the entire atomic states. So even if we could merge mode_fixup
and mode_valid somehow, we'll be left with atomic_check.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH v2 2/8] drm: Add drm_crtc_mode_valid()

[Jose Abreu]

Add a new helper to call crtc->mode_valid callback.

Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Cc: Carlos Palminha <palminha@synopsys.com>
Cc: Alexey Brodkin <abrodkin@synopsys.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Dave Airlie <airlied@linux.ie>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Archit Taneja <architt@codeaurora.org>
---
 drivers/gpu/drm/drm_crtc.c          | 22 ++++++++++++++++++++++
 drivers/gpu/drm/drm_crtc_internal.h |  3 +++
 2 files changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 5af25ce..07ae705 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -38,6 +38,7 @@
 #include <drm/drm_crtc.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_fourcc.h>
+#include <drm/drm_modeset_helper_vtables.h>
 #include <drm/drm_modeset_lock.h>
 #include <drm/drm_atomic.h>
 #include <drm/drm_auth.h>
@@ -741,3 +742,24 @@ int drm_mode_crtc_set_obj_prop(struct drm_mode_object *obj,
 
 	return ret;
 }
+
+/**
+ * drm_crtc_mode_valid - call crtc->mode_valid callback, if any.
+ * @crtc: crtc
+ * @mode: mode to be validated
+ *
+ * If no mode_valid callback is available this will return MODE_OK.
+ *
+ * Returns: drm_mode_status Enum
+ */
+enum drm_mode_status drm_crtc_mode_valid(struct drm_crtc *crtc,
+					 const struct drm_display_mode *mode)
+{
+	const struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
+
+	if (!crtc_funcs || !crtc_funcs->mode_valid)
+		return MODE_OK;
+
+	return crtc_funcs->mode_valid(crtc, mode);
+}
+EXPORT_SYMBOL(drm_crtc_mode_valid);
diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index d077c54..3800abd 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -45,6 +45,9 @@ int drm_crtc_check_viewport(const struct drm_crtc *crtc,
 
 struct dma_fence *drm_crtc_create_fence(struct drm_crtc *crtc);
 
+enum drm_mode_status drm_crtc_mode_valid(struct drm_crtc *crtc,
+					 const struct drm_display_mode *mode);
+
 /* IOCTLs */
 int drm_mode_getcrtc(struct drm_device *dev,
 		     void *data, struct drm_file *file_priv);
-- 
1.9.1

Re: [PATCH v2 2/8] drm: Add drm_crtc_mode_valid()

[Daniel Vetter]

On Tue, May 09, 2017 at 06:00:09PM +0100, Jose Abreu wrote:
> Add a new helper to call crtc->mode_valid callback.
> 
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> Cc: Carlos Palminha <palminha@synopsys.com>
> Cc: Alexey Brodkin <abrodkin@synopsys.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Dave Airlie <airlied@linux.ie>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Archit Taneja <architt@codeaurora.org>
> ---
>  drivers/gpu/drm/drm_crtc.c          | 22 ++++++++++++++++++++++
>  drivers/gpu/drm/drm_crtc_internal.h |  3 +++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 5af25ce..07ae705 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -38,6 +38,7 @@
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_edid.h>
>  #include <drm/drm_fourcc.h>
> +#include <drm/drm_modeset_helper_vtables.h>
>  #include <drm/drm_modeset_lock.h>
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_auth.h>
> @@ -741,3 +742,24 @@ int drm_mode_crtc_set_obj_prop(struct drm_mode_object *obj,
>  
>  	return ret;
>  }
> +
> +/**
> + * drm_crtc_mode_valid - call crtc->mode_valid callback, if any.
> + * @crtc: crtc
> + * @mode: mode to be validated
> + *
> + * If no mode_valid callback is available this will return MODE_OK.
> + *
> + * Returns: drm_mode_status Enum
> + */
> +enum drm_mode_status drm_crtc_mode_valid(struct drm_crtc *crtc,
> +					 const struct drm_display_mode *mode)
> +{
> +	const struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;

This is clearly a helper func, but you place it into the core and
EXPORT_SYMBOL it. Imo this should be entirely internal to the helpers,
perhaps just stuff them all into drm_probe_helpers.c? Header file would be
drm_crtc_helper_internal.h.

That also means no need for kernel-doc (only the driver api is formally
documented) and then these 3 patches are so tiny it's better to squash
them into the patch that adds their users.

Thanks, Daniel
> +
> +	if (!crtc_funcs || !crtc_funcs->mode_valid)
> +		return MODE_OK;
> +
> +	return crtc_funcs->mode_valid(crtc, mode);
> +}
> +EXPORT_SYMBOL(drm_crtc_mode_valid);
> diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
> index d077c54..3800abd 100644
> --- a/drivers/gpu/drm/drm_crtc_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_internal.h
> @@ -45,6 +45,9 @@ int drm_crtc_check_viewport(const struct drm_crtc *crtc,
>  
>  struct dma_fence *drm_crtc_create_fence(struct drm_crtc *crtc);
>  
> +enum drm_mode_status drm_crtc_mode_valid(struct drm_crtc *crtc,
> +					 const struct drm_display_mode *mode);
> +
>  /* IOCTLs */
>  int drm_mode_getcrtc(struct drm_device *dev,
>  		     void *data, struct drm_file *file_priv);
> -- 
> 1.9.1
> 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2 2/8] drm: Add drm_crtc_mode_valid()

[Jose Abreu]

Hi Daniel,


On 10-05-2017 08:59, Daniel Vetter wrote:
> On Tue, May 09, 2017 at 06:00:09PM +0100, Jose Abreu wrote:
>> Add a new helper to call crtc->mode_valid callback.
>>
>> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>> Cc: Carlos Palminha <palminha@synopsys.com>
>> Cc: Alexey Brodkin <abrodkin@synopsys.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Dave Airlie <airlied@linux.ie>
>> Cc: Andrzej Hajda <a.hajda@samsung.com>
>> Cc: Archit Taneja <architt@codeaurora.org>
>> ---
>>  drivers/gpu/drm/drm_crtc.c          | 22 ++++++++++++++++++++++
>>  drivers/gpu/drm/drm_crtc_internal.h |  3 +++
>>  2 files changed, 25 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index 5af25ce..07ae705 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -38,6 +38,7 @@
>>  #include <drm/drm_crtc.h>
>>  #include <drm/drm_edid.h>
>>  #include <drm/drm_fourcc.h>
>> +#include <drm/drm_modeset_helper_vtables.h>
>>  #include <drm/drm_modeset_lock.h>
>>  #include <drm/drm_atomic.h>
>>  #include <drm/drm_auth.h>
>> @@ -741,3 +742,24 @@ int drm_mode_crtc_set_obj_prop(struct drm_mode_object *obj,
>>  
>>  	return ret;
>>  }
>> +
>> +/**
>> + * drm_crtc_mode_valid - call crtc->mode_valid callback, if any.
>> + * @crtc: crtc
>> + * @mode: mode to be validated
>> + *
>> + * If no mode_valid callback is available this will return MODE_OK.
>> + *
>> + * Returns: drm_mode_status Enum
>> + */
>> +enum drm_mode_status drm_crtc_mode_valid(struct drm_crtc *crtc,
>> +					 const struct drm_display_mode *mode)
>> +{
>> +	const struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
> This is clearly a helper func, but you place it into the core and
> EXPORT_SYMBOL it. Imo this should be entirely internal to the helpers,
> perhaps just stuff them all into drm_probe_helpers.c? Header file would be
> drm_crtc_helper_internal.h.

Yeah, at first I was not planning to export it but then I saw
that drm_bridge_mode_fixup() is exported (and is in drm_bridge.c)
so it kind of felt right to place this in drm_crtc.c. Anyway, I
will move them to drm_probe_helpers.c, indeed there is no point
in exporting this.

>
> That also means no need for kernel-doc (only the driver api is formally
> documented) and then these 3 patches are so tiny it's better to squash
> them into the patch that adds their users.

Ok, will remove the docs but I think its better to have a single
patch which adds all the helpers so that I can use the
suggested-by tag. Thanks!

Best regards,
Jose Miguel Abreu

>
> Thanks, Daniel
>> +
>> +	if (!crtc_funcs || !crtc_funcs->mode_valid)
>> +		return MODE_OK;
>> +
>> +	return crtc_funcs->mode_valid(crtc, mode);
>> +}
>> +EXPORT_SYMBOL(drm_crtc_mode_valid);
>> diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
>> index d077c54..3800abd 100644
>> --- a/drivers/gpu/drm/drm_crtc_internal.h
>> +++ b/drivers/gpu/drm/drm_crtc_internal.h
>> @@ -45,6 +45,9 @@ int drm_crtc_check_viewport(const struct drm_crtc *crtc,
>>  
>>  struct dma_fence *drm_crtc_create_fence(struct drm_crtc *crtc);
>>  
>> +enum drm_mode_status drm_crtc_mode_valid(struct drm_crtc *crtc,
>> +					 const struct drm_display_mode *mode);
>> +
>>  /* IOCTLs */
>>  int drm_mode_getcrtc(struct drm_device *dev,
>>  		     void *data, struct drm_file *file_priv);
>> -- 
>> 1.9.1
>>
>>

Re: [PATCH v2 2/8] drm: Add drm_crtc_mode_valid()

[Daniel Vetter]

On Wed, May 10, 2017 at 09:57:30AM +0100, Jose Abreu wrote:
> Hi Daniel,
> 
> 
> On 10-05-2017 08:59, Daniel Vetter wrote:
> > On Tue, May 09, 2017 at 06:00:09PM +0100, Jose Abreu wrote:
> >> Add a new helper to call crtc->mode_valid callback.
> >>
> >> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> >> Cc: Carlos Palminha <palminha@synopsys.com>
> >> Cc: Alexey Brodkin <abrodkin@synopsys.com>
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> Cc: Dave Airlie <airlied@linux.ie>
> >> Cc: Andrzej Hajda <a.hajda@samsung.com>
> >> Cc: Archit Taneja <architt@codeaurora.org>
> >> ---
> >>  drivers/gpu/drm/drm_crtc.c          | 22 ++++++++++++++++++++++
> >>  drivers/gpu/drm/drm_crtc_internal.h |  3 +++
> >>  2 files changed, 25 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> >> index 5af25ce..07ae705 100644
> >> --- a/drivers/gpu/drm/drm_crtc.c
> >> +++ b/drivers/gpu/drm/drm_crtc.c
> >> @@ -38,6 +38,7 @@
> >>  #include <drm/drm_crtc.h>
> >>  #include <drm/drm_edid.h>
> >>  #include <drm/drm_fourcc.h>
> >> +#include <drm/drm_modeset_helper_vtables.h>
> >>  #include <drm/drm_modeset_lock.h>
> >>  #include <drm/drm_atomic.h>
> >>  #include <drm/drm_auth.h>
> >> @@ -741,3 +742,24 @@ int drm_mode_crtc_set_obj_prop(struct drm_mode_object *obj,
> >>  
> >>  	return ret;
> >>  }
> >> +
> >> +/**
> >> + * drm_crtc_mode_valid - call crtc->mode_valid callback, if any.
> >> + * @crtc: crtc
> >> + * @mode: mode to be validated
> >> + *
> >> + * If no mode_valid callback is available this will return MODE_OK.
> >> + *
> >> + * Returns: drm_mode_status Enum
> >> + */
> >> +enum drm_mode_status drm_crtc_mode_valid(struct drm_crtc *crtc,
> >> +					 const struct drm_display_mode *mode)
> >> +{
> >> +	const struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
> > This is clearly a helper func, but you place it into the core and
> > EXPORT_SYMBOL it. Imo this should be entirely internal to the helpers,
> > perhaps just stuff them all into drm_probe_helpers.c? Header file would be
> > drm_crtc_helper_internal.h.
> 
> Yeah, at first I was not planning to export it but then I saw
> that drm_bridge_mode_fixup() is exported (and is in drm_bridge.c)
> so it kind of felt right to place this in drm_crtc.c. Anyway, I
> will move them to drm_probe_helpers.c, indeed there is no point
> in exporting this.

Bridge is a bit special, since there's the bridge integration through
atomic/legacy helpers, but drivers could also wire up a bridge on their
own (i.e. without using the drm_encoder->bridge pointer). Not sure anyone
is doing that right now, but that was the idea behind having the helpers
all exported.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH v2 3/8] drm: Add drm_encoder_mode_valid()

[Jose Abreu]

Add a new helper to call encoder->mode_valid callback.

Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Cc: Carlos Palminha <palminha@synopsys.com>
Cc: Alexey Brodkin <abrodkin@synopsys.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Dave Airlie <airlied@linux.ie>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Archit Taneja <architt@codeaurora.org>
---
 drivers/gpu/drm/drm_crtc_internal.h |  2 ++
 drivers/gpu/drm/drm_encoder.c       | 23 +++++++++++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index 3800abd..6165bc9 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -129,6 +129,8 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
 /* drm_encoder.c */
 int drm_encoder_register_all(struct drm_device *dev);
 void drm_encoder_unregister_all(struct drm_device *dev);
+enum drm_mode_status drm_encoder_mode_valid(struct drm_encoder *encoder,
+					    const struct drm_display_mode *mode);
 
 /* IOCTL */
 int drm_mode_getencoder(struct drm_device *dev,
diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
index 0708779..af75f42 100644
--- a/drivers/gpu/drm/drm_encoder.c
+++ b/drivers/gpu/drm/drm_encoder.c
@@ -23,6 +23,7 @@
 #include <linux/export.h>
 #include <drm/drmP.h>
 #include <drm/drm_encoder.h>
+#include <drm/drm_modeset_helper_vtables.h>
 
 #include "drm_crtc_internal.h"
 
@@ -239,3 +240,25 @@ int drm_mode_getencoder(struct drm_device *dev, void *data,
 
 	return 0;
 }
+
+/**
+ * drm_encoder_mode_valid - call encoder->mode_valid callback, if any.
+ * @encoder: encoder
+ * @mode: mode to be validated
+ *
+ * If no mode_valid callback is available this will return MODE_OK.
+ *
+ * Returns: drm_mode_status Enum
+ */
+enum drm_mode_status drm_encoder_mode_valid(struct drm_encoder *encoder,
+					    const struct drm_display_mode *mode)
+{
+	const struct drm_encoder_helper_funcs *encoder_funcs =
+		encoder->helper_private;
+
+	if (!encoder_funcs || !encoder_funcs->mode_valid)
+		return MODE_OK;
+
+	return encoder_funcs->mode_valid(encoder, mode);
+}
+EXPORT_SYMBOL(drm_encoder_mode_valid);
-- 
1.9.1

[PATCH v2 4/8] drm: Add drm_connector_mode_valid()

[Jose Abreu]

Add a new helper to call connector->mode_valid callback.

Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Cc: Carlos Palminha <palminha@synopsys.com>
Cc: Alexey Brodkin <abrodkin@synopsys.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Dave Airlie <airlied@linux.ie>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Archit Taneja <architt@codeaurora.org>
---

TODO: function prototype should receive a const, but currently
mode_valid declaration is not const. In order to change this
I needed to touch *every* driver who uses this callback.
Postponed to when I have the time.

 drivers/gpu/drm/drm_connector.c     | 23 +++++++++++++++++++++++
 drivers/gpu/drm/drm_crtc_internal.h |  2 ++
 2 files changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 9f84761..f2634a2 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -24,6 +24,7 @@
 #include <drm/drm_connector.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_encoder.h>
+#include <drm/drm_modeset_helper_vtables.h>
 
 #include "drm_crtc_internal.h"
 #include "drm_internal.h"
@@ -1418,3 +1419,25 @@ struct drm_tile_group *drm_mode_create_tile_group(struct drm_device *dev,
 	return tg;
 }
 EXPORT_SYMBOL(drm_mode_create_tile_group);
+
+/**
+ * drm_connector_mode_valid - call connector->mode_valid callback, if any.
+ * @connector: connector
+ * @mode: mode to be validated
+ *
+ * If no mode_valid callback is available this will return MODE_OK.
+ *
+ * Returns: drm_mode_status Enum
+ */
+enum drm_mode_status drm_connector_mode_valid(struct drm_connector *connector,
+					      struct drm_display_mode *mode)
+{
+	const struct drm_connector_helper_funcs *connector_funcs =
+		connector->helper_private;
+
+	if (!connector_funcs || !connector_funcs->mode_valid)
+		return MODE_OK;
+
+	return connector_funcs->mode_valid(connector, mode);
+}
+EXPORT_SYMBOL(drm_connector_mode_valid);
diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index 6165bc9..018b154 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -146,6 +146,8 @@ int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
 				    uint64_t value);
 int drm_connector_create_standard_properties(struct drm_device *dev);
 const char *drm_get_connector_force_name(enum drm_connector_force force);
+enum drm_mode_status drm_connector_mode_valid(struct drm_connector *connector,
+					      struct drm_display_mode *mode);
 
 /* IOCTL */
 int drm_mode_connector_property_set_ioctl(struct drm_device *dev,
-- 
1.9.1

[PATCH v2 5/8] drm: Use new mode_valid() helpers in connector probe helper

[Jose Abreu]

This changes the connector probe helper function to use the new
encoder->mode_valid() and crtc->mode_valid() helper callbacks to
validate the modes.

The new callbacks are optional so the behaviour remains the same
if they are not implemented. If they are, then the code loops
through all the connector's encodersXcrtcs and calls the
callback.

If at least a valid encoderXcrtc combination is found which
accepts the mode then the function returns MODE_OK.

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Cc: Carlos Palminha <palminha@synopsys.com>
Cc: Alexey Brodkin <abrodkin@synopsys.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Dave Airlie <airlied@linux.ie>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Archit Taneja <architt@codeaurora.org>
---

Changes v1->v2:
	- Use new helpers suggested by Ville
	- Change documentation (Daniel)

 drivers/gpu/drm/drm_probe_helper.c | 60 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 57 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 1b0c14a..de47413 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -39,6 +39,8 @@
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_edid.h>
 
+#include "drm_crtc_internal.h"
+
 /**
  * DOC: output probing helper overview
  *
@@ -80,6 +82,54 @@
 	return MODE_OK;
 }
 
+static enum drm_mode_status
+drm_mode_validate_connector(struct drm_connector *connector,
+			    struct drm_display_mode *mode)
+{
+	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 */
+	ret = drm_connector_mode_valid(connector, mode);
+	if (ret != MODE_OK)
+		return ret;
+
+	/* Step 2: Validate against encoders and crtcs */
+	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;
+
+		ret = drm_encoder_mode_valid(encoder, mode);
+		if (ret != MODE_OK) {
+			/* No point in continuing for crtc check as this encoder
+			 * will not accept the mode anyway. If all encoders
+			 * reject the mode then, at exit, ret will not be
+			 * MODE_OK. */
+			continue;
+		}
+
+		drm_for_each_crtc(crtc, dev) {
+			if (!drm_encoder_crtc_ok(encoder, crtc))
+				continue;
+
+			ret = drm_crtc_mode_valid(crtc, mode);
+			if (ret == MODE_OK) {
+				/* If we get to this point there is at least
+				 * one combination of encoder+crtc that works
+				 * for this mode. Lets return now. */
+				return ret;
+			}
+		}
+	}
+
+	return ret;
+}
+
 static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
 {
 	struct drm_cmdline_mode *cmdline_mode;
@@ -284,7 +334,11 @@ void drm_kms_helper_poll_enable(struct drm_device *dev)
  *    - drm_mode_validate_flag() checks the modes against basic connector
  *      capabilities (interlace_allowed,doublescan_allowed,stereo_allowed)
  *    - the optional &drm_connector_helper_funcs.mode_valid helper can perform
- *      driver and/or hardware specific checks
+ *      driver and/or sink specific checks
+ *    - the optional &drm_crtc_helper_funcs.mode_valid and
+ *      &drm_encoder_helper_funcs.mode_valid helpers can perform driver and/or
+ *      source specific checks which are also enforced by the modeset/atomic
+ *      helpers
  *
  * 5. Any mode whose status is not OK is pruned from the connector's modes list,
  *    accompanied by a debug message indicating the reason for the mode's
@@ -428,8 +482,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);
 	}
 
-- 
1.9.1

Re: [PATCH v2 5/8] drm: Use new mode_valid() helpers in connector probe helper

[Daniel Vetter]

On Tue, May 09, 2017 at 06:00:12PM +0100, Jose Abreu wrote:
> This changes the connector probe helper function to use the new
> encoder->mode_valid() and crtc->mode_valid() helper callbacks to
> validate the modes.
> 
> The new callbacks are optional so the behaviour remains the same
> if they are not implemented. If they are, then the code loops
> through all the connector's encodersXcrtcs and calls the
> callback.
> 
> If at least a valid encoderXcrtc combination is found which
> accepts the mode then the function returns MODE_OK.
> 
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> Cc: Carlos Palminha <palminha@synopsys.com>
> Cc: Alexey Brodkin <abrodkin@synopsys.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Dave Airlie <airlied@linux.ie>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Archit Taneja <architt@codeaurora.org>
> ---
> 
> Changes v1->v2:
> 	- Use new helpers suggested by Ville
> 	- Change documentation (Daniel)
> 
>  drivers/gpu/drm/drm_probe_helper.c | 60 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 57 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 1b0c14a..de47413 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -39,6 +39,8 @@
>  #include <drm/drm_fb_helper.h>
>  #include <drm/drm_edid.h>
>  
> +#include "drm_crtc_internal.h"
> +
>  /**
>   * DOC: output probing helper overview
>   *
> @@ -80,6 +82,54 @@
>  	return MODE_OK;
>  }
>  
> +static enum drm_mode_status
> +drm_mode_validate_connector(struct drm_connector *connector,
> +			    struct drm_display_mode *mode)
> +{
> +	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 */
> +	ret = drm_connector_mode_valid(connector, mode);
> +	if (ret != MODE_OK)
> +		return ret;
> +
> +	/* Step 2: Validate against encoders and crtcs */
> +	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;
> +
> +		ret = drm_encoder_mode_valid(encoder, mode);
> +		if (ret != MODE_OK) {
> +			/* No point in continuing for crtc check as this encoder
> +			 * will not accept the mode anyway. If all encoders
> +			 * reject the mode then, at exit, ret will not be
> +			 * MODE_OK. */
> +			continue;
> +		}

One thing I've forgotten the last time around: Please also check
bridge->mode_valid here. The encoder->bridge mapping is fixed.

Otherwise I think this looks good.
-Daniel

> +
> +		drm_for_each_crtc(crtc, dev) {
> +			if (!drm_encoder_crtc_ok(encoder, crtc))
> +				continue;
> +
> +			ret = drm_crtc_mode_valid(crtc, mode);
> +			if (ret == MODE_OK) {
> +				/* If we get to this point there is at least
> +				 * one combination of encoder+crtc that works
> +				 * for this mode. Lets return now. */
> +				return ret;
> +			}
> +		}
> +	}
> +
> +	return ret;
> +}
> +
>  static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
>  {
>  	struct drm_cmdline_mode *cmdline_mode;
> @@ -284,7 +334,11 @@ void drm_kms_helper_poll_enable(struct drm_device *dev)
>   *    - drm_mode_validate_flag() checks the modes against basic connector
>   *      capabilities (interlace_allowed,doublescan_allowed,stereo_allowed)
>   *    - the optional &drm_connector_helper_funcs.mode_valid helper can perform
> - *      driver and/or hardware specific checks
> + *      driver and/or sink specific checks
> + *    - the optional &drm_crtc_helper_funcs.mode_valid and
> + *      &drm_encoder_helper_funcs.mode_valid helpers can perform driver and/or
> + *      source specific checks which are also enforced by the modeset/atomic
> + *      helpers
>   *
>   * 5. Any mode whose status is not OK is pruned from the connector's modes list,
>   *    accompanied by a debug message indicating the reason for the mode's
> @@ -428,8 +482,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);
>  	}
>  
> -- 
> 1.9.1
> 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2 5/8] drm: Use new mode_valid() helpers in connector probe helper

[Jose Abreu]

Hi Daniel,


On 10-05-2017 09:01, Daniel Vetter wrote:
> On Tue, May 09, 2017 at 06:00:12PM +0100, Jose Abreu wrote:
>> This changes the connector probe helper function to use the new
>> encoder->mode_valid() and crtc->mode_valid() helper callbacks to
>> validate the modes.
>>
>> The new callbacks are optional so the behaviour remains the same
>> if they are not implemented. If they are, then the code loops
>> through all the connector's encodersXcrtcs and calls the
>> callback.
>>
>> If at least a valid encoderXcrtc combination is found which
>> accepts the mode then the function returns MODE_OK.
>>
>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>> Cc: Carlos Palminha <palminha@synopsys.com>
>> Cc: Alexey Brodkin <abrodkin@synopsys.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Dave Airlie <airlied@linux.ie>
>> Cc: Andrzej Hajda <a.hajda@samsung.com>
>> Cc: Archit Taneja <architt@codeaurora.org>
>> ---
>>
>> Changes v1->v2:
>> 	- Use new helpers suggested by Ville
>> 	- Change documentation (Daniel)
>>
>>  drivers/gpu/drm/drm_probe_helper.c | 60 ++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 57 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
>> index 1b0c14a..de47413 100644
>> --- a/drivers/gpu/drm/drm_probe_helper.c
>> +++ b/drivers/gpu/drm/drm_probe_helper.c
>> @@ -39,6 +39,8 @@
>>  #include <drm/drm_fb_helper.h>
>>  #include <drm/drm_edid.h>
>>  
>> +#include "drm_crtc_internal.h"
>> +
>>  /**
>>   * DOC: output probing helper overview
>>   *
>> @@ -80,6 +82,54 @@
>>  	return MODE_OK;
>>  }
>>  
>> +static enum drm_mode_status
>> +drm_mode_validate_connector(struct drm_connector *connector,
>> +			    struct drm_display_mode *mode)
>> +{
>> +	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 */
>> +	ret = drm_connector_mode_valid(connector, mode);
>> +	if (ret != MODE_OK)
>> +		return ret;
>> +
>> +	/* Step 2: Validate against encoders and crtcs */
>> +	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;
>> +
>> +		ret = drm_encoder_mode_valid(encoder, mode);
>> +		if (ret != MODE_OK) {
>> +			/* No point in continuing for crtc check as this encoder
>> +			 * will not accept the mode anyway. If all encoders
>> +			 * reject the mode then, at exit, ret will not be
>> +			 * MODE_OK. */
>> +			continue;
>> +		}
> One thing I've forgotten the last time around: Please also check
> bridge->mode_valid here. The encoder->bridge mapping is fixed.

Ok, will add in next version.

Best regards,
Jose Miguel Abreu

>
> Otherwise I think this looks good.
> -Daniel
>
>> +
>> +		drm_for_each_crtc(crtc, dev) {
>> +			if (!drm_encoder_crtc_ok(encoder, crtc))
>> +				continue;
>> +
>> +			ret = drm_crtc_mode_valid(crtc, mode);
>> +			if (ret == MODE_OK) {
>> +				/* If we get to this point there is at least
>> +				 * one combination of encoder+crtc that works
>> +				 * for this mode. Lets return now. */
>> +				return ret;
>> +			}
>> +		}
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>>  static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
>>  {
>>  	struct drm_cmdline_mode *cmdline_mode;
>> @@ -284,7 +334,11 @@ void drm_kms_helper_poll_enable(struct drm_device *dev)
>>   *    - drm_mode_validate_flag() checks the modes against basic connector
>>   *      capabilities (interlace_allowed,doublescan_allowed,stereo_allowed)
>>   *    - the optional &drm_connector_helper_funcs.mode_valid helper can perform
>> - *      driver and/or hardware specific checks
>> + *      driver and/or sink specific checks
>> + *    - the optional &drm_crtc_helper_funcs.mode_valid and
>> + *      &drm_encoder_helper_funcs.mode_valid helpers can perform driver and/or
>> + *      source specific checks which are also enforced by the modeset/atomic
>> + *      helpers
>>   *
>>   * 5. Any mode whose status is not OK is pruned from the connector's modes list,
>>   *    accompanied by a debug message indicating the reason for the mode's
>> @@ -428,8 +482,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);
>>  	}
>>  
>> -- 
>> 1.9.1
>>
>>

Re: [PATCH v2 5/8] drm: Use new mode_valid() helpers in connector probe helper

[Laurent Pinchart]

Hi Jose,

Thank you for the patch.

On Tuesday 09 May 2017 18:00:12 Jose Abreu wrote:
> This changes the connector probe helper function to use the new
> encoder->mode_valid() and crtc->mode_valid() helper callbacks to
> validate the modes.
> 
> The new callbacks are optional so the behaviour remains the same
> if they are not implemented. If they are, then the code loops
> through all the connector's encodersXcrtcs and calls the
> callback.
> 
> If at least a valid encoderXcrtc combination is found which
> accepts the mode then the function returns MODE_OK.
> 
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> Cc: Carlos Palminha <palminha@synopsys.com>
> Cc: Alexey Brodkin <abrodkin@synopsys.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Dave Airlie <airlied@linux.ie>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Archit Taneja <architt@codeaurora.org>
> ---
> 
> Changes v1->v2:
> 	- Use new helpers suggested by Ville
> 	- Change documentation (Daniel)
> 
>  drivers/gpu/drm/drm_probe_helper.c | 60 +++++++++++++++++++++++++++++++++--
>  1 file changed, 57 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_probe_helper.c
> b/drivers/gpu/drm/drm_probe_helper.c index 1b0c14a..de47413 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -39,6 +39,8 @@
>  #include <drm/drm_fb_helper.h>
>  #include <drm/drm_edid.h>
> 
> +#include "drm_crtc_internal.h"
> +
>  /**
>   * DOC: output probing helper overview
>   *
> @@ -80,6 +82,54 @@
>  	return MODE_OK;
>  }
> 
> +static enum drm_mode_status
> +drm_mode_validate_connector(struct drm_connector *connector,
> +			    struct drm_display_mode *mode)

This does more than validating the mode against the connector, it validates it 
against the whole pipeline. I would call the function 
drm_mode_validate_pipeline() (or any other similar name).

> +{
> +	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 */
> +	ret = drm_connector_mode_valid(connector, mode);
> +	if (ret != MODE_OK)
> +		return ret;
> +
> +	/* Step 2: Validate against encoders and crtcs */
> +	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;
> +
> +		ret = drm_encoder_mode_valid(encoder, mode);
> +		if (ret != MODE_OK) {
> +			/* No point in continuing for crtc check as this 
encoder
> +			 * will not accept the mode anyway. If all encoders
> +			 * reject the mode then, at exit, ret will not be
> +			 * MODE_OK. */
> +			continue;
> +		}
> +
> +		drm_for_each_crtc(crtc, dev) {
> +			if (!drm_encoder_crtc_ok(encoder, crtc))
> +				continue;
> +
> +			ret = drm_crtc_mode_valid(crtc, mode);
> +			if (ret == MODE_OK) {
> +				/* If we get to this point there is at least
> +				 * one combination of encoder+crtc that works
> +				 * for this mode. Lets return now. */
> +				return ret;
> +			}
> +		}
> +	}
> +
> +	return ret;
> +}
> +
>  static int drm_helper_probe_add_cmdline_mode(struct drm_connector
> *connector)
>  {
>  	struct drm_cmdline_mode *cmdline_mode;
> @@ -284,7 +334,11 @@ void drm_kms_helper_poll_enable(struct drm_device *dev)
>   *    - drm_mode_validate_flag() checks the modes against basic connector
>   *      capabilities (interlace_allowed,doublescan_allowed,stereo_allowed)
>   *    - the optional &drm_connector_helper_funcs.mode_valid helper can
> perform
> - *      driver and/or hardware specific checks
> + *      driver and/or sink specific checks
> + *    - the optional &drm_crtc_helper_funcs.mode_valid and
> + *      &drm_encoder_helper_funcs.mode_valid helpers can perform driver
> and/or
> + *      source specific checks which are also enforced by the
> modeset/atomic
> + *      helpers
>   *
>   * 5. Any mode whose status is not OK is pruned from the connector's modes
> list,
>   *    accompanied by a debug message indicating the reason for the mode's
> @@ -428,8 +482,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);

I would reverse the arguments order to match the style of the other validation 
functions.

>  	}

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 5/8] drm: Use new mode_valid() helpers in connector probe helper

[Jose Abreu]

Hi Laurent,


On 12-05-2017 10:35, Laurent Pinchart wrote:
> Hi Jose,
>
> Thank you for the patch.
>
> On Tuesday 09 May 2017 18:00:12 Jose Abreu wrote:
>> This changes the connector probe helper function to use the new
>> encoder->mode_valid() and crtc->mode_valid() helper callbacks to
>> validate the modes.
>>
>> The new callbacks are optional so the behaviour remains the same
>> if they are not implemented. If they are, then the code loops
>> through all the connector's encodersXcrtcs and calls the
>> callback.
>>
>> If at least a valid encoderXcrtc combination is found which
>> accepts the mode then the function returns MODE_OK.
>>
>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>> Cc: Carlos Palminha <palminha@synopsys.com>
>> Cc: Alexey Brodkin <abrodkin@synopsys.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Dave Airlie <airlied@linux.ie>
>> Cc: Andrzej Hajda <a.hajda@samsung.com>
>> Cc: Archit Taneja <architt@codeaurora.org>
>> ---
>>
>> Changes v1->v2:
>> 	- Use new helpers suggested by Ville
>> 	- Change documentation (Daniel)
>>
>>  drivers/gpu/drm/drm_probe_helper.c | 60 +++++++++++++++++++++++++++++++++--
>>  1 file changed, 57 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_probe_helper.c
>> b/drivers/gpu/drm/drm_probe_helper.c index 1b0c14a..de47413 100644
>> --- a/drivers/gpu/drm/drm_probe_helper.c
>> +++ b/drivers/gpu/drm/drm_probe_helper.c
>> @@ -39,6 +39,8 @@
>>  #include <drm/drm_fb_helper.h>
>>  #include <drm/drm_edid.h>
>>
>> +#include "drm_crtc_internal.h"
>> +
>>  /**
>>   * DOC: output probing helper overview
>>   *
>> @@ -80,6 +82,54 @@
>>  	return MODE_OK;
>>  }
>>
>> +static enum drm_mode_status
>> +drm_mode_validate_connector(struct drm_connector *connector,
>> +			    struct drm_display_mode *mode)
> This does more than validating the mode against the connector, it validates it 
> against the whole pipeline. I would call the function 
> drm_mode_validate_pipeline() (or any other similar name).

Yeah, in previous version I had something similar but I changed
in order to address review comments. I can change again though...

>
>> +{
>> +	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 */
>> +	ret = drm_connector_mode_valid(connector, mode);
>> +	if (ret != MODE_OK)
>> +		return ret;
>> +
>> +	/* Step 2: Validate against encoders and crtcs */
>> +	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;
>> +
>> +		ret = drm_encoder_mode_valid(encoder, mode);
>> +		if (ret != MODE_OK) {
>> +			/* No point in continuing for crtc check as this 
> encoder
>> +			 * will not accept the mode anyway. If all encoders
>> +			 * reject the mode then, at exit, ret will not be
>> +			 * MODE_OK. */
>> +			continue;
>> +		}
>> +
>> +		drm_for_each_crtc(crtc, dev) {
>> +			if (!drm_encoder_crtc_ok(encoder, crtc))
>> +				continue;
>> +
>> +			ret = drm_crtc_mode_valid(crtc, mode);
>> +			if (ret == MODE_OK) {
>> +				/* If we get to this point there is at least
>> +				 * one combination of encoder+crtc that works
>> +				 * for this mode. Lets return now. */
>> +				return ret;
>> +			}
>> +		}
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>>  static int drm_helper_probe_add_cmdline_mode(struct drm_connector
>> *connector)
>>  {
>>  	struct drm_cmdline_mode *cmdline_mode;
>> @@ -284,7 +334,11 @@ void drm_kms_helper_poll_enable(struct drm_device *dev)
>>   *    - drm_mode_validate_flag() checks the modes against basic connector
>>   *      capabilities (interlace_allowed,doublescan_allowed,stereo_allowed)
>>   *    - the optional &drm_connector_helper_funcs.mode_valid helper can
>> perform
>> - *      driver and/or hardware specific checks
>> + *      driver and/or sink specific checks
>> + *    - the optional &drm_crtc_helper_funcs.mode_valid and
>> + *      &drm_encoder_helper_funcs.mode_valid helpers can perform driver
>> and/or
>> + *      source specific checks which are also enforced by the
>> modeset/atomic
>> + *      helpers
>>   *
>>   * 5. Any mode whose status is not OK is pruned from the connector's modes
>> list,
>>   *    accompanied by a debug message indicating the reason for the mode's
>> @@ -428,8 +482,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);
> I would reverse the arguments order to match the style of the other validation 
> functions.

Hmm, I think it makes more sense to pass connector first and then
mode ...

Best regards,
Jose Miguel Abreu

>
>>  	}

Re: [PATCH v2 5/8] drm: Use new mode_valid() helpers in connector probe helper

[Laurent Pinchart]

Hi Jose,

On Friday 12 May 2017 17:06:14 Jose Abreu wrote:
> On 12-05-2017 10:35, Laurent Pinchart wrote:
> > On Tuesday 09 May 2017 18:00:12 Jose Abreu wrote:
> >> This changes the connector probe helper function to use the new
> >> encoder->mode_valid() and crtc->mode_valid() helper callbacks to
> >> validate the modes.
> >> 
> >> The new callbacks are optional so the behaviour remains the same
> >> if they are not implemented. If they are, then the code loops
> >> through all the connector's encodersXcrtcs and calls the
> >> callback.
> >> 
> >> If at least a valid encoderXcrtc combination is found which
> >> accepts the mode then the function returns MODE_OK.
> >> 
> >> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> >> Cc: Carlos Palminha <palminha@synopsys.com>
> >> Cc: Alexey Brodkin <abrodkin@synopsys.com>
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> Cc: Dave Airlie <airlied@linux.ie>
> >> Cc: Andrzej Hajda <a.hajda@samsung.com>
> >> Cc: Archit Taneja <architt@codeaurora.org>
> >> ---
> >> 
> >> Changes v1->v2:
> >> 	- Use new helpers suggested by Ville
> >> 	- Change documentation (Daniel)
> >> 	
> >>  drivers/gpu/drm/drm_probe_helper.c | 60 ++++++++++++++++++++++++++++++--
> >>  1 file changed, 57 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/drm_probe_helper.c
> >> b/drivers/gpu/drm/drm_probe_helper.c index 1b0c14a..de47413 100644
> >> --- a/drivers/gpu/drm/drm_probe_helper.c
> >> +++ b/drivers/gpu/drm/drm_probe_helper.c

[snip]

> >> +static enum drm_mode_status
> >> +drm_mode_validate_connector(struct drm_connector *connector,
> >> +			    struct drm_display_mode *mode)
> > 
> > This does more than validating the mode against the connector, it
> > validates it against the whole pipeline. I would call the function
> > drm_mode_validate_pipeline() (or any other similar name).
> 
> Yeah, in previous version I had something similar but I changed
> in order to address review comments. I can change again though...

Sorry, I haven't seen v1. I think it makes more sense to reflect in its name 
the fact that the function validates the mode against the whole pipeline, but 
I'll let others disagree.

> >> +{
> >> +	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 */
> >> +	ret = drm_connector_mode_valid(connector, mode);
> >> +	if (ret != MODE_OK)
> >> +		return ret;
> >> +
> >> +	/* Step 2: Validate against encoders and crtcs */
> >> +	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;
> >> +
> >> +		ret = drm_encoder_mode_valid(encoder, mode);
> >> +		if (ret != MODE_OK) {
> >> +			/* No point in continuing for crtc check as this
> > 
> > encoder
> > 
> >> +			 * will not accept the mode anyway. If all encoders
> >> +			 * reject the mode then, at exit, ret will not be
> >> +			 * MODE_OK. */
> >> +			continue;
> >> +		}
> >> +
> >> +		drm_for_each_crtc(crtc, dev) {
> >> +			if (!drm_encoder_crtc_ok(encoder, crtc))
> >> +				continue;
> >> +
> >> +			ret = drm_crtc_mode_valid(crtc, mode);
> >> +			if (ret == MODE_OK) {
> >> +				/* If we get to this point there is at least
> >> +				 * one combination of encoder+crtc that works
> >> +				 * for this mode. Lets return now. */
> >> +				return ret;
> >> +			}
> >> +		}
> >> +	}
> >> +
> >> +	return ret;
> >> +}

[snip]

> >> @@ -428,8 +482,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);
> > 
> > I would reverse the arguments order to match the style of the other
> > validation functions.
> 
> Hmm, I think it makes more sense to pass connector first and then
> mode ...

I disagree, as this function validates a mode against a pipeline, the same way 
the other validation functions validate a mode against other parameters, but 
it's your patch :-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 5/8] drm: Use new mode_valid() helpers in connector probe helper

[Daniel Vetter]

On Sun, May 14, 2017 at 02:04:24PM +0300, Laurent Pinchart wrote:
> On Friday 12 May 2017 17:06:14 Jose Abreu wrote:
> > On 12-05-2017 10:35, Laurent Pinchart wrote:
> > > On Tuesday 09 May 2017 18:00:12 Jose Abreu wrote:
> > >> +		if (mode->status == MODE_OK)
> > >> +			mode->status = drm_mode_validate_connector(connector,
> > >> 
> > >>  								   mode);
> > > 
> > > I would reverse the arguments order to match the style of the other
> > > validation functions.
> > 
> > Hmm, I think it makes more sense to pass connector first and then
> > mode ...
> 
> I disagree, as this function validates a mode against a pipeline, the same way 
> the other validation functions validate a mode against other parameters, but 
> it's your patch :-)

Call it drm_connector_validate_mode, because the first argument is
generally the object we operate on :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2 5/8] drm: Use new mode_valid() helpers in connector probe helper

[Laurent Pinchart]

On Monday 15 May 2017 08:47:49 Daniel Vetter wrote:
> On Sun, May 14, 2017 at 02:04:24PM +0300, Laurent Pinchart wrote:
> > On Friday 12 May 2017 17:06:14 Jose Abreu wrote:
> >> On 12-05-2017 10:35, Laurent Pinchart wrote:
> >>> On Tuesday 09 May 2017 18:00:12 Jose Abreu wrote:
> >>>> +		if (mode->status == MODE_OK)
> >>>> +			mode->status = 
drm_mode_validate_connector(connector,
> >>>> 
> >>>>  								   
mode);
> >>> 
> >>> I would reverse the arguments order to match the style of the other
> >>> validation functions.
> >> 
> >> Hmm, I think it makes more sense to pass connector first and then
> >> mode ...
> > 
> > I disagree, as this function validates a mode against a pipeline, the same
> > way the other validation functions validate a mode against other
> > parameters, but it's your patch :-)
> 
> Call it drm_connector_validate_mode, because the first argument is
> generally the object we operate on :-)

But the function doesn't validate a mode for a connector, it validates a mode 
for a complete pipeline...

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 5/8] drm: Use new mode_valid() helpers in connector probe helper

[Jose Abreu]

Hi Laurent,


On 15-05-2017 08:05, Laurent Pinchart wrote:
> On Monday 15 May 2017 08:47:49 Daniel Vetter wrote:
>> On Sun, May 14, 2017 at 02:04:24PM +0300, Laurent Pinchart wrote:
>>> On Friday 12 May 2017 17:06:14 Jose Abreu wrote:
>>>> On 12-05-2017 10:35, Laurent Pinchart wrote:
>>>>> On Tuesday 09 May 2017 18:00:12 Jose Abreu wrote:
>>>>>> +		if (mode->status == MODE_OK)
>>>>>> +			mode->status = 
> drm_mode_validate_connector(connector,
>>>>>>  								   
> mode);
>>>>> I would reverse the arguments order to match the style of the other
>>>>> validation functions.
>>>> Hmm, I think it makes more sense to pass connector first and then
>>>> mode ...
>>> I disagree, as this function validates a mode against a pipeline, the same
>>> way the other validation functions validate a mode against other
>>> parameters, but it's your patch :-)
>> Call it drm_connector_validate_mode, because the first argument is
>> generally the object we operate on :-)
> But the function doesn't validate a mode for a connector, it validates a mode 
> for a complete pipeline...
>

Hmm, but note that in the same function there is
drm_mode_validate_size() and drm_mode_validate_flag() calls,
which take as first argument the mode and then the object to
validate (I hadn't seen this). So, maybe leave it as
drm_mode_validate_connector() as it takes a connector as argument
or change to drm_mode_validate_pipeline() as you said, or even
drm_mode_validate_datapath(), drm_mode_validate_videopath(),
drm_mode_validate_components() ?

Best regards,
Jose Miguel Abreu

[PATCH v2 6/8] drm: Introduce drm_bridge_mode_valid()

[Jose Abreu]

Introduce a new helper function which calls mode_valid() callback
for all bridges in an encoder chain.

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Cc: Carlos Palminha <palminha@synopsys.com>
Cc: Alexey Brodkin <abrodkin@synopsys.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Dave Airlie <airlied@linux.ie>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Archit Taneja <architt@codeaurora.org>
---
 drivers/gpu/drm/drm_bridge.c | 33 +++++++++++++++++++++++++++++++++
 include/drm/drm_bridge.h     |  2 ++
 2 files changed, 35 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 86a7637..dc8cdfe 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -206,6 +206,39 @@ bool drm_bridge_mode_fixup(struct drm_bridge *bridge,
 EXPORT_SYMBOL(drm_bridge_mode_fixup);
 
 /**
+ * drm_bridge_mode_valid - validate the mode against all bridges in the
+ * 			   encoder chain.
+ * @bridge: bridge control structure
+ * @mode: desired mode to be validated
+ *
+ * Calls &drm_bridge_funcs.mode_valid for all the bridges in the encoder
+ * chain, starting from the first bridge to the last. If at least one bridge
+ * does not accept the mode the function returns the error code.
+ *
+ * Note: the bridge passed should be the one closest to the encoder.
+ *
+ * RETURNS:
+ * MODE_OK on success, drm_mode_status Enum error code on failure
+ */
+enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge,
+					   const struct drm_display_mode *mode)
+{
+	enum drm_mode_status ret = MODE_OK;
+
+	if (!bridge)
+		return ret;
+
+	if (bridge->funcs->mode_valid)
+		ret = bridge->funcs->mode_valid(bridge, mode);
+
+	if (ret != MODE_OK)
+		return ret;
+
+	return drm_bridge_mode_valid(bridge->next, mode);
+}
+EXPORT_SYMBOL(drm_bridge_mode_valid);
+
+/**
  * drm_bridge_disable - disables all bridges in the encoder chain
  * @bridge: bridge control structure
  *
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 00c6c36..8358eb3 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -233,6 +233,8 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
 bool drm_bridge_mode_fixup(struct drm_bridge *bridge,
 			const struct drm_display_mode *mode,
 			struct drm_display_mode *adjusted_mode);
+enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge,
+					   const struct drm_display_mode *mode);
 void drm_bridge_disable(struct drm_bridge *bridge);
 void drm_bridge_post_disable(struct drm_bridge *bridge);
 void drm_bridge_mode_set(struct drm_bridge *bridge,
-- 
1.9.1

Re: [PATCH v2 6/8] drm: Introduce drm_bridge_mode_valid()

[Ville Syrjälä]

On Tue, May 09, 2017 at 06:00:13PM +0100, Jose Abreu wrote:
> Introduce a new helper function which calls mode_valid() callback
> for all bridges in an encoder chain.
> 
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> Cc: Carlos Palminha <palminha@synopsys.com>
> Cc: Alexey Brodkin <abrodkin@synopsys.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Dave Airlie <airlied@linux.ie>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Archit Taneja <architt@codeaurora.org>
> ---
>  drivers/gpu/drm/drm_bridge.c | 33 +++++++++++++++++++++++++++++++++
>  include/drm/drm_bridge.h     |  2 ++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 86a7637..dc8cdfe 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -206,6 +206,39 @@ bool drm_bridge_mode_fixup(struct drm_bridge *bridge,
>  EXPORT_SYMBOL(drm_bridge_mode_fixup);
>  
>  /**
> + * drm_bridge_mode_valid - validate the mode against all bridges in the
> + * 			   encoder chain.
> + * @bridge: bridge control structure
> + * @mode: desired mode to be validated
> + *
> + * Calls &drm_bridge_funcs.mode_valid for all the bridges in the encoder
> + * chain, starting from the first bridge to the last. If at least one bridge
> + * does not accept the mode the function returns the error code.
> + *
> + * Note: the bridge passed should be the one closest to the encoder.
> + *
> + * RETURNS:
> + * MODE_OK on success, drm_mode_status Enum error code on failure
> + */
> +enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge,
> +					   const struct drm_display_mode *mode)
> +{
> +	enum drm_mode_status ret = MODE_OK;
> +
> +	if (!bridge)
> +		return ret;
> +
> +	if (bridge->funcs->mode_valid)
> +		ret = bridge->funcs->mode_valid(bridge, mode);
> +
> +	if (ret != MODE_OK)
> +		return ret;
> +
> +	return drm_bridge_mode_valid(bridge->next, mode);

Looks like it should be pretty trivial to avoid the recursion.

Am I correct in interpreting this that bridges have some kind of
a hand rolled linked list implementation? Reusing the standard
linked lists would allow you to use list_for_each() etc.

> +}
> +EXPORT_SYMBOL(drm_bridge_mode_valid);
> +
> +/**
>   * drm_bridge_disable - disables all bridges in the encoder chain
>   * @bridge: bridge control structure
>   *
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 00c6c36..8358eb3 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -233,6 +233,8 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>  bool drm_bridge_mode_fixup(struct drm_bridge *bridge,
>  			const struct drm_display_mode *mode,
>  			struct drm_display_mode *adjusted_mode);
> +enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge,
> +					   const struct drm_display_mode *mode);
>  void drm_bridge_disable(struct drm_bridge *bridge);
>  void drm_bridge_post_disable(struct drm_bridge *bridge);
>  void drm_bridge_mode_set(struct drm_bridge *bridge,
> -- 
> 1.9.1
> 

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v2 6/8] drm: Introduce drm_bridge_mode_valid()

[Jose Abreu]

Hi Ville,


On 10-05-2017 14:41, Ville Syrjälä wrote:
> On Tue, May 09, 2017 at 06:00:13PM +0100, Jose Abreu wrote:
>> Introduce a new helper function which calls mode_valid() callback
>> for all bridges in an encoder chain.
>>
>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>> Cc: Carlos Palminha <palminha@synopsys.com>
>> Cc: Alexey Brodkin <abrodkin@synopsys.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Dave Airlie <airlied@linux.ie>
>> Cc: Andrzej Hajda <a.hajda@samsung.com>
>> Cc: Archit Taneja <architt@codeaurora.org>
>> ---
>>  drivers/gpu/drm/drm_bridge.c | 33 +++++++++++++++++++++++++++++++++
>>  include/drm/drm_bridge.h     |  2 ++
>>  2 files changed, 35 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>> index 86a7637..dc8cdfe 100644
>> --- a/drivers/gpu/drm/drm_bridge.c
>> +++ b/drivers/gpu/drm/drm_bridge.c
>> @@ -206,6 +206,39 @@ bool drm_bridge_mode_fixup(struct drm_bridge *bridge,
>>  EXPORT_SYMBOL(drm_bridge_mode_fixup);
>>  
>>  /**
>> + * drm_bridge_mode_valid - validate the mode against all bridges in the
>> + * 			   encoder chain.
>> + * @bridge: bridge control structure
>> + * @mode: desired mode to be validated
>> + *
>> + * Calls &drm_bridge_funcs.mode_valid for all the bridges in the encoder
>> + * chain, starting from the first bridge to the last. If at least one bridge
>> + * does not accept the mode the function returns the error code.
>> + *
>> + * Note: the bridge passed should be the one closest to the encoder.
>> + *
>> + * RETURNS:
>> + * MODE_OK on success, drm_mode_status Enum error code on failure
>> + */
>> +enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge,
>> +					   const struct drm_display_mode *mode)
>> +{
>> +	enum drm_mode_status ret = MODE_OK;
>> +
>> +	if (!bridge)
>> +		return ret;
>> +
>> +	if (bridge->funcs->mode_valid)
>> +		ret = bridge->funcs->mode_valid(bridge, mode);
>> +
>> +	if (ret != MODE_OK)
>> +		return ret;
>> +
>> +	return drm_bridge_mode_valid(bridge->next, mode);
> Looks like it should be pretty trivial to avoid the recursion.
>
> Am I correct in interpreting this that bridges have some kind of
> a hand rolled linked list implementation? Reusing the standard
> linked lists would allow you to use list_for_each() etc.

I reused the drm_bridge_mode_fixup but now I see how its done
like that: so that the fixup is propagated in the correct order.
As for mode_valid we just need to check if ret != MODE_OK then I
think we can use the list_for_each_entry(bridge->list).

Best regards,
Jose Miguel Abreu

>
>> +}
>> +EXPORT_SYMBOL(drm_bridge_mode_valid);
>> +
>> +/**
>>   * drm_bridge_disable - disables all bridges in the encoder chain
>>   * @bridge: bridge control structure
>>   *
>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>> index 00c6c36..8358eb3 100644
>> --- a/include/drm/drm_bridge.h
>> +++ b/include/drm/drm_bridge.h
>> @@ -233,6 +233,8 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>>  bool drm_bridge_mode_fixup(struct drm_bridge *bridge,
>>  			const struct drm_display_mode *mode,
>>  			struct drm_display_mode *adjusted_mode);
>> +enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge,
>> +					   const struct drm_display_mode *mode);
>>  void drm_bridge_disable(struct drm_bridge *bridge);
>>  void drm_bridge_post_disable(struct drm_bridge *bridge);
>>  void drm_bridge_mode_set(struct drm_bridge *bridge,
>> -- 
>> 1.9.1
>>

Re: [PATCH v2 6/8] drm: Introduce drm_bridge_mode_valid()

[Jose Abreu]

Hi Ville,


On 10-05-2017 15:01, Jose Abreu wrote:
> Hi Ville,
>
>
> On 10-05-2017 14:41, Ville Syrjälä wrote:
>> On Tue, May 09, 2017 at 06:00:13PM +0100, Jose Abreu wrote:
>>> Introduce a new helper function which calls mode_valid() callback
>>> for all bridges in an encoder chain.
>>>
>>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>>> Cc: Carlos Palminha <palminha@synopsys.com>
>>> Cc: Alexey Brodkin <abrodkin@synopsys.com>
>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Cc: Dave Airlie <airlied@linux.ie>
>>> Cc: Andrzej Hajda <a.hajda@samsung.com>
>>> Cc: Archit Taneja <architt@codeaurora.org>
>>> ---
>>>  drivers/gpu/drm/drm_bridge.c | 33 +++++++++++++++++++++++++++++++++
>>>  include/drm/drm_bridge.h     |  2 ++
>>>  2 files changed, 35 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>>> index 86a7637..dc8cdfe 100644
>>> --- a/drivers/gpu/drm/drm_bridge.c
>>> +++ b/drivers/gpu/drm/drm_bridge.c
>>> @@ -206,6 +206,39 @@ bool drm_bridge_mode_fixup(struct drm_bridge *bridge,
>>>  EXPORT_SYMBOL(drm_bridge_mode_fixup);
>>>  
>>>  /**
>>> + * drm_bridge_mode_valid - validate the mode against all bridges in the
>>> + * 			   encoder chain.
>>> + * @bridge: bridge control structure
>>> + * @mode: desired mode to be validated
>>> + *
>>> + * Calls &drm_bridge_funcs.mode_valid for all the bridges in the encoder
>>> + * chain, starting from the first bridge to the last. If at least one bridge
>>> + * does not accept the mode the function returns the error code.
>>> + *
>>> + * Note: the bridge passed should be the one closest to the encoder.
>>> + *
>>> + * RETURNS:
>>> + * MODE_OK on success, drm_mode_status Enum error code on failure
>>> + */
>>> +enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge,
>>> +					   const struct drm_display_mode *mode)
>>> +{
>>> +	enum drm_mode_status ret = MODE_OK;
>>> +
>>> +	if (!bridge)
>>> +		return ret;
>>> +
>>> +	if (bridge->funcs->mode_valid)
>>> +		ret = bridge->funcs->mode_valid(bridge, mode);
>>> +
>>> +	if (ret != MODE_OK)
>>> +		return ret;
>>> +
>>> +	return drm_bridge_mode_valid(bridge->next, mode);
>> Looks like it should be pretty trivial to avoid the recursion.
>>
>> Am I correct in interpreting this that bridges have some kind of
>> a hand rolled linked list implementation? Reusing the standard
>> linked lists would allow you to use list_for_each() etc.
> I reused the drm_bridge_mode_fixup but now I see how its done
> like that: so that the fixup is propagated in the correct order.
> As for mode_valid we just need to check if ret != MODE_OK then I
> think we can use the list_for_each_entry(bridge->list).

Oops, I got this wrong sorry. I meant there is a list but its for
all the system bridges. This is a "custom" linked list yeah.

Best regards,
Jose Miguel Abreu

>
> Best regards,
> Jose Miguel Abreu
>
>>> +}
>>> +EXPORT_SYMBOL(drm_bridge_mode_valid);
>>> +
>>> +/**
>>>   * drm_bridge_disable - disables all bridges in the encoder chain
>>>   * @bridge: bridge control structure
>>>   *
>>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>>> index 00c6c36..8358eb3 100644
>>> --- a/include/drm/drm_bridge.h
>>> +++ b/include/drm/drm_bridge.h
>>> @@ -233,6 +233,8 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>>>  bool drm_bridge_mode_fixup(struct drm_bridge *bridge,
>>>  			const struct drm_display_mode *mode,
>>>  			struct drm_display_mode *adjusted_mode);
>>> +enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge,
>>> +					   const struct drm_display_mode *mode);
>>>  void drm_bridge_disable(struct drm_bridge *bridge);
>>>  void drm_bridge_post_disable(struct drm_bridge *bridge);
>>>  void drm_bridge_mode_set(struct drm_bridge *bridge,
>>> -- 
>>> 1.9.1
>>>

Re: [PATCH v2 6/8] drm: Introduce drm_bridge_mode_valid()

[Daniel Vetter]

On Wed, May 10, 2017 at 04:41:09PM +0300, Ville Syrjälä wrote:
> On Tue, May 09, 2017 at 06:00:13PM +0100, Jose Abreu wrote:
> > Introduce a new helper function which calls mode_valid() callback
> > for all bridges in an encoder chain.
> > 
> > Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> > Cc: Carlos Palminha <palminha@synopsys.com>
> > Cc: Alexey Brodkin <abrodkin@synopsys.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Dave Airlie <airlied@linux.ie>
> > Cc: Andrzej Hajda <a.hajda@samsung.com>
> > Cc: Archit Taneja <architt@codeaurora.org>
> > ---
> >  drivers/gpu/drm/drm_bridge.c | 33 +++++++++++++++++++++++++++++++++
> >  include/drm/drm_bridge.h     |  2 ++
> >  2 files changed, 35 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index 86a7637..dc8cdfe 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -206,6 +206,39 @@ bool drm_bridge_mode_fixup(struct drm_bridge *bridge,
> >  EXPORT_SYMBOL(drm_bridge_mode_fixup);
> >  
> >  /**
> > + * drm_bridge_mode_valid - validate the mode against all bridges in the
> > + * 			   encoder chain.
> > + * @bridge: bridge control structure
> > + * @mode: desired mode to be validated
> > + *
> > + * Calls &drm_bridge_funcs.mode_valid for all the bridges in the encoder
> > + * chain, starting from the first bridge to the last. If at least one bridge
> > + * does not accept the mode the function returns the error code.
> > + *
> > + * Note: the bridge passed should be the one closest to the encoder.
> > + *
> > + * RETURNS:
> > + * MODE_OK on success, drm_mode_status Enum error code on failure
> > + */
> > +enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge,
> > +					   const struct drm_display_mode *mode)
> > +{
> > +	enum drm_mode_status ret = MODE_OK;
> > +
> > +	if (!bridge)
> > +		return ret;
> > +
> > +	if (bridge->funcs->mode_valid)
> > +		ret = bridge->funcs->mode_valid(bridge, mode);
> > +
> > +	if (ret != MODE_OK)
> > +		return ret;
> > +
> > +	return drm_bridge_mode_valid(bridge->next, mode);
> 
> Looks like it should be pretty trivial to avoid the recursion.
> 
> Am I correct in interpreting this that bridges have some kind of
> a hand rolled linked list implementation? Reusing the standard
> linked lists would allow you to use list_for_each() etc.

Yeah it's a hand-rolled list, but current hw also has a bridge nesting
depth of 2, so it really doesn't matter. I guess once we have real long
chains of bridges we can fix this (and just using list_head sounds like a
great idea).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2 6/8] drm: Introduce drm_bridge_mode_valid()

[Laurent Pinchart]

Hi Daniel,

On Wednesday 10 May 2017 17:14:33 Daniel Vetter wrote:
> On Wed, May 10, 2017 at 04:41:09PM +0300, Ville Syrjälä wrote:
> > On Tue, May 09, 2017 at 06:00:13PM +0100, Jose Abreu wrote:
> > > Introduce a new helper function which calls mode_valid() callback
> > > for all bridges in an encoder chain.
> > > 
> > > Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> > > Cc: Carlos Palminha <palminha@synopsys.com>
> > > Cc: Alexey Brodkin <abrodkin@synopsys.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: Dave Airlie <airlied@linux.ie>
> > > Cc: Andrzej Hajda <a.hajda@samsung.com>
> > > Cc: Archit Taneja <architt@codeaurora.org>
> > > ---
> > > 
> > >  drivers/gpu/drm/drm_bridge.c | 33 +++++++++++++++++++++++++++++++++
> > >  include/drm/drm_bridge.h     |  2 ++
> > >  2 files changed, 35 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > > index 86a7637..dc8cdfe 100644
> > > --- a/drivers/gpu/drm/drm_bridge.c
> > > +++ b/drivers/gpu/drm/drm_bridge.c
> > > @@ -206,6 +206,39 @@ bool drm_bridge_mode_fixup(struct drm_bridge
> > > *bridge,
> > > 
> > >  EXPORT_SYMBOL(drm_bridge_mode_fixup);
> > >  
> > >  /**
> > > 
> > > + * drm_bridge_mode_valid - validate the mode against all bridges in the
> > > + * 			   encoder chain.
> > > + * @bridge: bridge control structure
> > > + * @mode: desired mode to be validated
> > > + *
> > > + * Calls &drm_bridge_funcs.mode_valid for all the bridges in the
> > > encoder
> > > + * chain, starting from the first bridge to the last. If at least one
> > > bridge + * does not accept the mode the function returns the error
> > > code.
> > > + *
> > > + * Note: the bridge passed should be the one closest to the encoder.
> > > + *
> > > + * RETURNS:
> > > + * MODE_OK on success, drm_mode_status Enum error code on failure
> > > + */
> > > +enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge,
> > > +					   const struct drm_display_mode 
*mode)
> > > +{
> > > +	enum drm_mode_status ret = MODE_OK;
> > > +
> > > +	if (!bridge)
> > > +		return ret;
> > > +
> > > +	if (bridge->funcs->mode_valid)
> > > +		ret = bridge->funcs->mode_valid(bridge, mode);
> > > +
> > > +	if (ret != MODE_OK)
> > > +		return ret;
> > > +
> > > +	return drm_bridge_mode_valid(bridge->next, mode);
> > 
> > Looks like it should be pretty trivial to avoid the recursion.
> > 
> > Am I correct in interpreting this that bridges have some kind of
> > a hand rolled linked list implementation? Reusing the standard
> > linked lists would allow you to use list_for_each() etc.
> 
> Yeah it's a hand-rolled list, but current hw also has a bridge nesting
> depth of 2, so it really doesn't matter. I guess once we have real long
> chains of bridges we can fix this (and just using list_head sounds like a
> great idea).

Even if not really needed right now, it's a pretty easy cleanup, if Jose has 
time to handle it in v3 of this series let's not postpone it ;-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 6/8] drm: Introduce drm_bridge_mode_valid()

[Archit Taneja]

On 05/12/2017 03:08 PM, Laurent Pinchart wrote:
> Hi Daniel,
>
> On Wednesday 10 May 2017 17:14:33 Daniel Vetter wrote:
>> On Wed, May 10, 2017 at 04:41:09PM +0300, Ville Syrjälä wrote:
>>> On Tue, May 09, 2017 at 06:00:13PM +0100, Jose Abreu wrote:
>>>> Introduce a new helper function which calls mode_valid() callback
>>>> for all bridges in an encoder chain.
>>>>
>>>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>>>> Cc: Carlos Palminha <palminha@synopsys.com>
>>>> Cc: Alexey Brodkin <abrodkin@synopsys.com>
>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>> Cc: Dave Airlie <airlied@linux.ie>
>>>> Cc: Andrzej Hajda <a.hajda@samsung.com>
>>>> Cc: Archit Taneja <architt@codeaurora.org>
>>>> ---
>>>>
>>>>  drivers/gpu/drm/drm_bridge.c | 33 +++++++++++++++++++++++++++++++++
>>>>  include/drm/drm_bridge.h     |  2 ++
>>>>  2 files changed, 35 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>>>> index 86a7637..dc8cdfe 100644
>>>> --- a/drivers/gpu/drm/drm_bridge.c
>>>> +++ b/drivers/gpu/drm/drm_bridge.c
>>>> @@ -206,6 +206,39 @@ bool drm_bridge_mode_fixup(struct drm_bridge
>>>> *bridge,
>>>>
>>>>  EXPORT_SYMBOL(drm_bridge_mode_fixup);
>>>>
>>>>  /**
>>>>
>>>> + * drm_bridge_mode_valid - validate the mode against all bridges in the
>>>> + * 			   encoder chain.
>>>> + * @bridge: bridge control structure
>>>> + * @mode: desired mode to be validated
>>>> + *
>>>> + * Calls &drm_bridge_funcs.mode_valid for all the bridges in the
>>>> encoder
>>>> + * chain, starting from the first bridge to the last. If at least one
>>>> bridge + * does not accept the mode the function returns the error
>>>> code.
>>>> + *
>>>> + * Note: the bridge passed should be the one closest to the encoder.
>>>> + *
>>>> + * RETURNS:
>>>> + * MODE_OK on success, drm_mode_status Enum error code on failure
>>>> + */
>>>> +enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge,
>>>> +					   const struct drm_display_mode
> *mode)
>>>> +{
>>>> +	enum drm_mode_status ret = MODE_OK;
>>>> +
>>>> +	if (!bridge)
>>>> +		return ret;
>>>> +
>>>> +	if (bridge->funcs->mode_valid)
>>>> +		ret = bridge->funcs->mode_valid(bridge, mode);
>>>> +
>>>> +	if (ret != MODE_OK)
>>>> +		return ret;
>>>> +
>>>> +	return drm_bridge_mode_valid(bridge->next, mode);
>>>
>>> Looks like it should be pretty trivial to avoid the recursion.
>>>
>>> Am I correct in interpreting this that bridges have some kind of
>>> a hand rolled linked list implementation? Reusing the standard
>>> linked lists would allow you to use list_for_each() etc.
>>
>> Yeah it's a hand-rolled list, but current hw also has a bridge nesting
>> depth of 2, so it really doesn't matter. I guess once we have real long
>> chains of bridges we can fix this (and just using list_head sounds like a
>> great idea).
>
> Even if not really needed right now, it's a pretty easy cleanup, if Jose has
> time to handle it in v3 of this series let's not postpone it ;-)

jfyi, some of the bridge functions call the ops from the last bridge in the
chain to first, so we'd need to use list_for_each_entry_prev() (or something
like that) for them.

Archit

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 6/8] drm: Introduce drm_bridge_mode_valid()

[Laurent Pinchart]

Hi Archit,

On Friday 12 May 2017 16:20:07 Archit Taneja wrote:
> On 05/12/2017 03:08 PM, Laurent Pinchart wrote:
> > On Wednesday 10 May 2017 17:14:33 Daniel Vetter wrote:
> >> On Wed, May 10, 2017 at 04:41:09PM +0300, Ville Syrjälä wrote:
> >>> On Tue, May 09, 2017 at 06:00:13PM +0100, Jose Abreu wrote:
> >>>> Introduce a new helper function which calls mode_valid() callback
> >>>> for all bridges in an encoder chain.
> >>>> 
> >>>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> >>>> Cc: Carlos Palminha <palminha@synopsys.com>
> >>>> Cc: Alexey Brodkin <abrodkin@synopsys.com>
> >>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>>> Cc: Dave Airlie <airlied@linux.ie>
> >>>> Cc: Andrzej Hajda <a.hajda@samsung.com>
> >>>> Cc: Archit Taneja <architt@codeaurora.org>
> >>>> ---
> >>>> 
> >>>>  drivers/gpu/drm/drm_bridge.c | 33 +++++++++++++++++++++++++++++++++
> >>>>  include/drm/drm_bridge.h     |  2 ++
> >>>>  2 files changed, 35 insertions(+)
> >>>> 
> >>>> diff --git a/drivers/gpu/drm/drm_bridge.c
> >>>> b/drivers/gpu/drm/drm_bridge.c
> >>>> index 86a7637..dc8cdfe 100644
> >>>> --- a/drivers/gpu/drm/drm_bridge.c
> >>>> +++ b/drivers/gpu/drm/drm_bridge.c
> >>>> @@ -206,6 +206,39 @@ bool drm_bridge_mode_fixup(struct drm_bridge
> >>>> *bridge,
> >>>> 
> >>>>  EXPORT_SYMBOL(drm_bridge_mode_fixup);
> >>>>  
> >>>>  /**
> >>>> 
> >>>> + * drm_bridge_mode_valid - validate the mode against all bridges in
> >>>> the
> >>>> + * 			   encoder chain.
> >>>> + * @bridge: bridge control structure
> >>>> + * @mode: desired mode to be validated
> >>>> + *
> >>>> + * Calls &drm_bridge_funcs.mode_valid for all the bridges in the
> >>>> encoder
> >>>> + * chain, starting from the first bridge to the last. If at least one
> >>>> bridge + * does not accept the mode the function returns the error
> >>>> code.
> >>>> + *
> >>>> + * Note: the bridge passed should be the one closest to the encoder.
> >>>> + *
> >>>> + * RETURNS:
> >>>> + * MODE_OK on success, drm_mode_status Enum error code on failure
> >>>> + */
> >>>> +enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge,
> >>>> +					   const struct 
drm_display_mode
> > 
> > *mode)
> > 
> >>>> +{
> >>>> +	enum drm_mode_status ret = MODE_OK;
> >>>> +
> >>>> +	if (!bridge)
> >>>> +		return ret;
> >>>> +
> >>>> +	if (bridge->funcs->mode_valid)
> >>>> +		ret = bridge->funcs->mode_valid(bridge, mode);
> >>>> +
> >>>> +	if (ret != MODE_OK)
> >>>> +		return ret;
> >>>> +
> >>>> +	return drm_bridge_mode_valid(bridge->next, mode);
> >>> 
> >>> Looks like it should be pretty trivial to avoid the recursion.
> >>> 
> >>> Am I correct in interpreting this that bridges have some kind of
> >>> a hand rolled linked list implementation? Reusing the standard
> >>> linked lists would allow you to use list_for_each() etc.
> >> 
> >> Yeah it's a hand-rolled list, but current hw also has a bridge nesting
> >> depth of 2, so it really doesn't matter. I guess once we have real long
> >> chains of bridges we can fix this (and just using list_head sounds like a
> >> great idea).
> > 
> > Even if not really needed right now, it's a pretty easy cleanup, if Jose
> > has time to handle it in v3 of this series let's not postpone it ;-)
> 
> jfyi, some of the bridge functions call the ops from the last bridge in the
> chain to first, so we'd need to use list_for_each_entry_prev() (or something
> like that) for them.

And now that I think about it, for some of the operations (especially 
enable/disable) I believe that the bridge should be able to decide whether to 
call the next/previous bridge first or to configure its hardware first. I can 
image bridges that need the previous bridge in the chain to provide a valid 
clock before they get started, as well as bridges that need to be started with 
the incoming video signal stopped.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 6/8] drm: Introduce drm_bridge_mode_valid()

[Archit Taneja]

On 05/12/2017 04:31 PM, Laurent Pinchart wrote:
> Hi Archit,
>
> On Friday 12 May 2017 16:20:07 Archit Taneja wrote:
>> On 05/12/2017 03:08 PM, Laurent Pinchart wrote:
>>> On Wednesday 10 May 2017 17:14:33 Daniel Vetter wrote:
>>>> On Wed, May 10, 2017 at 04:41:09PM +0300, Ville Syrjälä wrote:
>>>>> On Tue, May 09, 2017 at 06:00:13PM +0100, Jose Abreu wrote:
>>>>>> Introduce a new helper function which calls mode_valid() callback
>>>>>> for all bridges in an encoder chain.
>>>>>>
>>>>>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>>>>>> Cc: Carlos Palminha <palminha@synopsys.com>
>>>>>> Cc: Alexey Brodkin <abrodkin@synopsys.com>
>>>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>>>> Cc: Dave Airlie <airlied@linux.ie>
>>>>>> Cc: Andrzej Hajda <a.hajda@samsung.com>
>>>>>> Cc: Archit Taneja <architt@codeaurora.org>
>>>>>> ---
>>>>>>
>>>>>>  drivers/gpu/drm/drm_bridge.c | 33 +++++++++++++++++++++++++++++++++
>>>>>>  include/drm/drm_bridge.h     |  2 ++
>>>>>>  2 files changed, 35 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/drm_bridge.c
>>>>>> b/drivers/gpu/drm/drm_bridge.c
>>>>>> index 86a7637..dc8cdfe 100644
>>>>>> --- a/drivers/gpu/drm/drm_bridge.c
>>>>>> +++ b/drivers/gpu/drm/drm_bridge.c
>>>>>> @@ -206,6 +206,39 @@ bool drm_bridge_mode_fixup(struct drm_bridge
>>>>>> *bridge,
>>>>>>
>>>>>>  EXPORT_SYMBOL(drm_bridge_mode_fixup);
>>>>>>
>>>>>>  /**
>>>>>>
>>>>>> + * drm_bridge_mode_valid - validate the mode against all bridges in
>>>>>> the
>>>>>> + * 			   encoder chain.
>>>>>> + * @bridge: bridge control structure
>>>>>> + * @mode: desired mode to be validated
>>>>>> + *
>>>>>> + * Calls &drm_bridge_funcs.mode_valid for all the bridges in the
>>>>>> encoder
>>>>>> + * chain, starting from the first bridge to the last. If at least one
>>>>>> bridge + * does not accept the mode the function returns the error
>>>>>> code.
>>>>>> + *
>>>>>> + * Note: the bridge passed should be the one closest to the encoder.
>>>>>> + *
>>>>>> + * RETURNS:
>>>>>> + * MODE_OK on success, drm_mode_status Enum error code on failure
>>>>>> + */
>>>>>> +enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge,
>>>>>> +					   const struct
> drm_display_mode
>>>
>>> *mode)
>>>
>>>>>> +{
>>>>>> +	enum drm_mode_status ret = MODE_OK;
>>>>>> +
>>>>>> +	if (!bridge)
>>>>>> +		return ret;
>>>>>> +
>>>>>> +	if (bridge->funcs->mode_valid)
>>>>>> +		ret = bridge->funcs->mode_valid(bridge, mode);
>>>>>> +
>>>>>> +	if (ret != MODE_OK)
>>>>>> +		return ret;
>>>>>> +
>>>>>> +	return drm_bridge_mode_valid(bridge->next, mode);
>>>>>
>>>>> Looks like it should be pretty trivial to avoid the recursion.
>>>>>
>>>>> Am I correct in interpreting this that bridges have some kind of
>>>>> a hand rolled linked list implementation? Reusing the standard
>>>>> linked lists would allow you to use list_for_each() etc.
>>>>
>>>> Yeah it's a hand-rolled list, but current hw also has a bridge nesting
>>>> depth of 2, so it really doesn't matter. I guess once we have real long
>>>> chains of bridges we can fix this (and just using list_head sounds like a
>>>> great idea).
>>>
>>> Even if not really needed right now, it's a pretty easy cleanup, if Jose
>>> has time to handle it in v3 of this series let's not postpone it ;-)
>>
>> jfyi, some of the bridge functions call the ops from the last bridge in the
>> chain to first, so we'd need to use list_for_each_entry_prev() (or something
>> like that) for them.
>
> And now that I think about it, for some of the operations (especially
> enable/disable) I believe that the bridge should be able to decide whether to
> call the next/previous bridge first or to configure its hardware first. I can
> image bridges that need the previous bridge in the chain to provide a valid
> clock before they get started, as well as bridges that need to be started with
> the incoming video signal stopped.

I guess converting into list would be a good start to achieve this. We'd probably
need to extend/redo the drm_bridge_attach() API to tweak the order in the which
the ops are called.

Thanks,
Archit

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 6/8] drm: Introduce drm_bridge_mode_valid()

[Daniel Vetter]

On Fri, May 12, 2017 at 02:01:49PM +0300, Laurent Pinchart wrote:
> Hi Archit,
> 
> On Friday 12 May 2017 16:20:07 Archit Taneja wrote:
> > On 05/12/2017 03:08 PM, Laurent Pinchart wrote:
> > > On Wednesday 10 May 2017 17:14:33 Daniel Vetter wrote:
> > >> On Wed, May 10, 2017 at 04:41:09PM +0300, Ville Syrjälä wrote:
> > >>> On Tue, May 09, 2017 at 06:00:13PM +0100, Jose Abreu wrote:
> > >>>> Introduce a new helper function which calls mode_valid() callback
> > >>>> for all bridges in an encoder chain.
> > >>>> 
> > >>>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> > >>>> Cc: Carlos Palminha <palminha@synopsys.com>
> > >>>> Cc: Alexey Brodkin <abrodkin@synopsys.com>
> > >>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > >>>> Cc: Dave Airlie <airlied@linux.ie>
> > >>>> Cc: Andrzej Hajda <a.hajda@samsung.com>
> > >>>> Cc: Archit Taneja <architt@codeaurora.org>
> > >>>> ---
> > >>>> 
> > >>>>  drivers/gpu/drm/drm_bridge.c | 33 +++++++++++++++++++++++++++++++++
> > >>>>  include/drm/drm_bridge.h     |  2 ++
> > >>>>  2 files changed, 35 insertions(+)
> > >>>> 
> > >>>> diff --git a/drivers/gpu/drm/drm_bridge.c
> > >>>> b/drivers/gpu/drm/drm_bridge.c
> > >>>> index 86a7637..dc8cdfe 100644
> > >>>> --- a/drivers/gpu/drm/drm_bridge.c
> > >>>> +++ b/drivers/gpu/drm/drm_bridge.c
> > >>>> @@ -206,6 +206,39 @@ bool drm_bridge_mode_fixup(struct drm_bridge
> > >>>> *bridge,
> > >>>> 
> > >>>>  EXPORT_SYMBOL(drm_bridge_mode_fixup);
> > >>>>  
> > >>>>  /**
> > >>>> 
> > >>>> + * drm_bridge_mode_valid - validate the mode against all bridges in
> > >>>> the
> > >>>> + * 			   encoder chain.
> > >>>> + * @bridge: bridge control structure
> > >>>> + * @mode: desired mode to be validated
> > >>>> + *
> > >>>> + * Calls &drm_bridge_funcs.mode_valid for all the bridges in the
> > >>>> encoder
> > >>>> + * chain, starting from the first bridge to the last. If at least one
> > >>>> bridge + * does not accept the mode the function returns the error
> > >>>> code.
> > >>>> + *
> > >>>> + * Note: the bridge passed should be the one closest to the encoder.
> > >>>> + *
> > >>>> + * RETURNS:
> > >>>> + * MODE_OK on success, drm_mode_status Enum error code on failure
> > >>>> + */
> > >>>> +enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge,
> > >>>> +					   const struct 
> drm_display_mode
> > > 
> > > *mode)
> > > 
> > >>>> +{
> > >>>> +	enum drm_mode_status ret = MODE_OK;
> > >>>> +
> > >>>> +	if (!bridge)
> > >>>> +		return ret;
> > >>>> +
> > >>>> +	if (bridge->funcs->mode_valid)
> > >>>> +		ret = bridge->funcs->mode_valid(bridge, mode);
> > >>>> +
> > >>>> +	if (ret != MODE_OK)
> > >>>> +		return ret;
> > >>>> +
> > >>>> +	return drm_bridge_mode_valid(bridge->next, mode);
> > >>> 
> > >>> Looks like it should be pretty trivial to avoid the recursion.
> > >>> 
> > >>> Am I correct in interpreting this that bridges have some kind of
> > >>> a hand rolled linked list implementation? Reusing the standard
> > >>> linked lists would allow you to use list_for_each() etc.
> > >> 
> > >> Yeah it's a hand-rolled list, but current hw also has a bridge nesting
> > >> depth of 2, so it really doesn't matter. I guess once we have real long
> > >> chains of bridges we can fix this (and just using list_head sounds like a
> > >> great idea).
> > > 
> > > Even if not really needed right now, it's a pretty easy cleanup, if Jose
> > > has time to handle it in v3 of this series let's not postpone it ;-)
> > 
> > jfyi, some of the bridge functions call the ops from the last bridge in the
> > chain to first, so we'd need to use list_for_each_entry_prev() (or something
> > like that) for them.
> 
> And now that I think about it, for some of the operations (especially 
> enable/disable) I believe that the bridge should be able to decide whether to 
> call the next/previous bridge first or to configure its hardware first. I can 
> image bridges that need the previous bridge in the chain to provide a valid 
> clock before they get started, as well as bridges that need to be started with 
> the incoming video signal stopped.

That's why we have pre_/post_ hooks ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH v2 7/8] drm: Use mode_valid() in atomic modeset

[Jose Abreu]

This patches makes use of the new mode_valid() callbacks introduced
previously to validate the full video pipeline when modesetting.

This calls the connector->mode_valid(), encoder->mode_valid(),
bridge->mode_valid() and crtc->mode_valid() so that we can
make sure that the mode will be accepted in every components.

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Cc: Carlos Palminha <palminha@synopsys.com>
Cc: Alexey Brodkin <abrodkin@synopsys.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Dave Airlie <airlied@linux.ie>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Archit Taneja <architt@codeaurora.org>
---

Changes v1->v2:
	- Removed call to connector->mode_valid (Ville, Daniel)
	- Change function name (Ville)
	- Use for_each_new_connector_in_state (Ville)
	- Do not validate if connector and mode didn't change (Ville)
	- Use new helpers to call mode_valid

 drivers/gpu/drm/drm_atomic_helper.c | 75 +++++++++++++++++++++++++++++++++++--
 1 file changed, 72 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 8be9719..19d0ea1 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -452,6 +452,69 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
 	return 0;
 }
 
+static enum drm_mode_status mode_valid_path(struct drm_connector *connector,
+					    struct drm_encoder *encoder,
+					    struct drm_crtc *crtc,
+					    struct drm_display_mode *mode)
+{
+	enum drm_mode_status ret;
+
+	ret = drm_encoder_mode_valid(encoder, mode);
+	if (ret != MODE_OK) {
+		DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] mode_valid() failed\n",
+				encoder->base.id, encoder->name);
+		return ret;
+	}
+
+	ret = drm_bridge_mode_valid(encoder->bridge, mode);
+	if (ret != MODE_OK) {
+		DRM_DEBUG_ATOMIC("[BRIDGE] mode_valid() failed\n");
+		return ret;
+	}
+
+	ret = drm_crtc_mode_valid(crtc, mode);
+	if (ret != MODE_OK) {
+		DRM_DEBUG_ATOMIC("[CRTC:%d:%s] mode_valid() failed\n",
+				crtc->base.id, crtc->name);
+		return ret;
+	}
+
+	return ret;
+}
+
+static int
+mode_valid(struct drm_atomic_state *state)
+{
+	struct drm_connector_state *conn_state;
+	struct drm_connector *connector;
+	int i;
+
+	for_each_new_connector_in_state(state, connector, conn_state, i) {
+		struct drm_encoder *encoder = conn_state->best_encoder;
+		struct drm_crtc *crtc = conn_state->crtc;
+		struct drm_crtc_state *crtc_state;
+		enum drm_mode_status mode_status;
+		struct drm_display_mode *mode;
+
+		if (!crtc || !encoder)
+			continue;
+
+		crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+		if (!crtc_state)
+			continue;
+		if (!crtc_state->mode_changed && !crtc_state->connectors_changed)
+			continue;
+
+		mode = &crtc_state->mode;
+
+		mode_status = mode_valid_path(connector, encoder, crtc, mode);
+		if (mode_status != MODE_OK)
+			return -EINVAL;
+	}
+
+	return 0;
+}
+
 /**
  * drm_atomic_helper_check_modeset - validate state object for modeset changes
  * @dev: DRM device
@@ -466,13 +529,15 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
  * 2. &drm_connector_helper_funcs.atomic_check to validate the connector state.
  * 3. If it's determined a modeset is needed then all connectors on the affected crtc
  *    crtc are added and &drm_connector_helper_funcs.atomic_check is run on them.
- * 4. &drm_bridge_funcs.mode_fixup is called on all encoder bridges.
- * 5. &drm_encoder_helper_funcs.atomic_check is called to validate any encoder state.
+ * 4. &drm_encoder_helper_funcs.mode_valid, &drm_bridge_funcs.mode_valid and
+ *    &drm_crtc_helper_funcs.mode_valid are called on the affected components.
+ * 5. &drm_bridge_funcs.mode_fixup is called on all encoder bridges.
+ * 6. &drm_encoder_helper_funcs.atomic_check is called to validate any encoder state.
  *    This function is only called when the encoder will be part of a configured crtc,
  *    it must not be used for implementing connector property validation.
  *    If this function is NULL, &drm_atomic_encoder_helper_funcs.mode_fixup is called
  *    instead.
- * 6. &drm_crtc_helper_funcs.mode_fixup is called last, to fix up the mode with crtc constraints.
+ * 7. &drm_crtc_helper_funcs.mode_fixup is called last, to fix up the mode with crtc constraints.
  *
  * &drm_crtc_state.mode_changed is set when the input mode is changed.
  * &drm_crtc_state.connectors_changed is set when a connector is added or
@@ -617,6 +682,10 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
 			return ret;
 	}
 
+	ret = mode_valid(state);
+	if (ret)
+		return ret;
+
 	return mode_fixup(state);
 }
 EXPORT_SYMBOL(drm_atomic_helper_check_modeset);
-- 
1.9.1

Re: [PATCH v2 7/8] drm: Use mode_valid() in atomic modeset

[Archit Taneja]

On 5/9/2017 10:30 PM, Jose Abreu wrote:
> This patches makes use of the new mode_valid() callbacks introduced
> previously to validate the full video pipeline when modesetting.
> 
> This calls the connector->mode_valid(), encoder->mode_valid(),
> bridge->mode_valid() and crtc->mode_valid() so that we can
> make sure that the mode will be accepted in every components.
> 
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> Cc: Carlos Palminha <palminha@synopsys.com>
> Cc: Alexey Brodkin <abrodkin@synopsys.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Dave Airlie <airlied@linux.ie>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Archit Taneja <architt@codeaurora.org>
> ---
> 
> Changes v1->v2:
> 	- Removed call to connector->mode_valid (Ville, Daniel)
> 	- Change function name (Ville)
> 	- Use for_each_new_connector_in_state (Ville)
> 	- Do not validate if connector and mode didn't change (Ville)
> 	- Use new helpers to call mode_valid
> 
>   drivers/gpu/drm/drm_atomic_helper.c | 75 +++++++++++++++++++++++++++++++++++--
>   1 file changed, 72 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 8be9719..19d0ea1 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -452,6 +452,69 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
>   	return 0;
>   }
>   
> +static enum drm_mode_status mode_valid_path(struct drm_connector *connector,
> +					    struct drm_encoder *encoder,
> +					    struct drm_crtc *crtc,
> +					    struct drm_display_mode *mode)
> +{
> +	enum drm_mode_status ret;
> +
> +	ret = drm_encoder_mode_valid(encoder, mode);
> +	if (ret != MODE_OK) {
> +		DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] mode_valid() failed\n",
> +				encoder->base.id, encoder->name);
> +		return ret;
> +	}
> +
> +	ret = drm_bridge_mode_valid(encoder->bridge, mode);
> +	if (ret != MODE_OK) {
> +		DRM_DEBUG_ATOMIC("[BRIDGE] mode_valid() failed\n");
> +		return ret;
> +	}
> +
> +	ret = drm_crtc_mode_valid(crtc, mode);
> +	if (ret != MODE_OK) {
> +		DRM_DEBUG_ATOMIC("[CRTC:%d:%s] mode_valid() failed\n",
> +				crtc->base.id, crtc->name);
> +		return ret;
> +	}
> +
> +	return ret;
> +}
> +
> +static int
> +mode_valid(struct drm_atomic_state *state)
> +{
> +	struct drm_connector_state *conn_state;
> +	struct drm_connector *connector;
> +	int i;
> +
> +	for_each_new_connector_in_state(state, connector, conn_state, i) {
> +		struct drm_encoder *encoder = conn_state->best_encoder;
> +		struct drm_crtc *crtc = conn_state->crtc;
> +		struct drm_crtc_state *crtc_state;
> +		enum drm_mode_status mode_status;
> +		struct drm_display_mode *mode;
> +
> +		if (!crtc || !encoder)
> +			continue;
> +
> +		crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> +		if (!crtc_state)
> +			continue;
> +		if (!crtc_state->mode_changed && !crtc_state->connectors_changed)
> +			continue;
> +
> +		mode = &crtc_state->mode;
> +
> +		mode_status = mode_valid_path(connector, encoder, crtc, mode);
> +		if (mode_status != MODE_OK)
> +			return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>   /**
>    * drm_atomic_helper_check_modeset - validate state object for modeset changes
>    * @dev: DRM device
> @@ -466,13 +529,15 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
>    * 2. &drm_connector_helper_funcs.atomic_check to validate the connector state.
>    * 3. If it's determined a modeset is needed then all connectors on the affected crtc
>    *    crtc are added and &drm_connector_helper_funcs.atomic_check is run on them.
> - * 4. &drm_bridge_funcs.mode_fixup is called on all encoder bridges.
> - * 5. &drm_encoder_helper_funcs.atomic_check is called to validate any encoder state.
> + * 4. &drm_encoder_helper_funcs.mode_valid, &drm_bridge_funcs.mode_valid and
> + *    &drm_crtc_helper_funcs.mode_valid are called on the affected components.
> + * 5. &drm_bridge_funcs.mode_fixup is called on all encoder bridges.
> + * 6. &drm_encoder_helper_funcs.atomic_check is called to validate any encoder state.
>    *    This function is only called when the encoder will be part of a configured crtc,
>    *    it must not be used for implementing connector property validation.
>    *    If this function is NULL, &drm_atomic_encoder_helper_funcs.mode_fixup is called
>    *    instead.
> - * 6. &drm_crtc_helper_funcs.mode_fixup is called last, to fix up the mode with crtc constraints.
> + * 7. &drm_crtc_helper_funcs.mode_fixup is called last, to fix up the mode with crtc constraints.
>    *
>    * &drm_crtc_state.mode_changed is set when the input mode is changed.
>    * &drm_crtc_state.connectors_changed is set when a connector is added or
> @@ -617,6 +682,10 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
>   			return ret;
>   	}
>   
> +	ret = mode_valid(state);
> +	if (ret)
> +		return ret;
> +

Since we've ensured that the modes won't fail, can mode_fixup() and
the mode_fixup() ops for crtc/bridge/encoders be assured to not fail
too? This is assuming that all drivers have moved to using the new
mode_valid() ops correctly.

Thanks,
Archit

>   	return mode_fixup(state);
>   }
>   EXPORT_SYMBOL(drm_atomic_helper_check_modeset);
> 

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v2 7/8] drm: Use mode_valid() in atomic modeset

[Daniel Vetter]

On Wed, May 10, 2017 at 09:38:00PM +0530, Archit Taneja wrote:
> 
> 
> On 5/9/2017 10:30 PM, Jose Abreu wrote:
> > This patches makes use of the new mode_valid() callbacks introduced
> > previously to validate the full video pipeline when modesetting.
> > 
> > This calls the connector->mode_valid(), encoder->mode_valid(),
> > bridge->mode_valid() and crtc->mode_valid() so that we can
> > make sure that the mode will be accepted in every components.
> > 
> > Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> > Cc: Carlos Palminha <palminha@synopsys.com>
> > Cc: Alexey Brodkin <abrodkin@synopsys.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Dave Airlie <airlied@linux.ie>
> > Cc: Andrzej Hajda <a.hajda@samsung.com>
> > Cc: Archit Taneja <architt@codeaurora.org>
> > ---
> > 
> > Changes v1->v2:
> > 	- Removed call to connector->mode_valid (Ville, Daniel)
> > 	- Change function name (Ville)
> > 	- Use for_each_new_connector_in_state (Ville)
> > 	- Do not validate if connector and mode didn't change (Ville)
> > 	- Use new helpers to call mode_valid
> > 
> >   drivers/gpu/drm/drm_atomic_helper.c | 75 +++++++++++++++++++++++++++++++++++--
> >   1 file changed, 72 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index 8be9719..19d0ea1 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -452,6 +452,69 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
> >   	return 0;
> >   }
> > +static enum drm_mode_status mode_valid_path(struct drm_connector *connector,
> > +					    struct drm_encoder *encoder,
> > +					    struct drm_crtc *crtc,
> > +					    struct drm_display_mode *mode)
> > +{
> > +	enum drm_mode_status ret;
> > +
> > +	ret = drm_encoder_mode_valid(encoder, mode);
> > +	if (ret != MODE_OK) {
> > +		DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] mode_valid() failed\n",
> > +				encoder->base.id, encoder->name);
> > +		return ret;
> > +	}
> > +
> > +	ret = drm_bridge_mode_valid(encoder->bridge, mode);
> > +	if (ret != MODE_OK) {
> > +		DRM_DEBUG_ATOMIC("[BRIDGE] mode_valid() failed\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = drm_crtc_mode_valid(crtc, mode);
> > +	if (ret != MODE_OK) {
> > +		DRM_DEBUG_ATOMIC("[CRTC:%d:%s] mode_valid() failed\n",
> > +				crtc->base.id, crtc->name);
> > +		return ret;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static int
> > +mode_valid(struct drm_atomic_state *state)
> > +{
> > +	struct drm_connector_state *conn_state;
> > +	struct drm_connector *connector;
> > +	int i;
> > +
> > +	for_each_new_connector_in_state(state, connector, conn_state, i) {
> > +		struct drm_encoder *encoder = conn_state->best_encoder;
> > +		struct drm_crtc *crtc = conn_state->crtc;
> > +		struct drm_crtc_state *crtc_state;
> > +		enum drm_mode_status mode_status;
> > +		struct drm_display_mode *mode;
> > +
> > +		if (!crtc || !encoder)
> > +			continue;
> > +
> > +		crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> > +		if (!crtc_state)
> > +			continue;
> > +		if (!crtc_state->mode_changed && !crtc_state->connectors_changed)
> > +			continue;
> > +
> > +		mode = &crtc_state->mode;
> > +
> > +		mode_status = mode_valid_path(connector, encoder, crtc, mode);
> > +		if (mode_status != MODE_OK)
> > +			return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >   /**
> >    * drm_atomic_helper_check_modeset - validate state object for modeset changes
> >    * @dev: DRM device
> > @@ -466,13 +529,15 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
> >    * 2. &drm_connector_helper_funcs.atomic_check to validate the connector state.
> >    * 3. If it's determined a modeset is needed then all connectors on the affected crtc
> >    *    crtc are added and &drm_connector_helper_funcs.atomic_check is run on them.
> > - * 4. &drm_bridge_funcs.mode_fixup is called on all encoder bridges.
> > - * 5. &drm_encoder_helper_funcs.atomic_check is called to validate any encoder state.
> > + * 4. &drm_encoder_helper_funcs.mode_valid, &drm_bridge_funcs.mode_valid and
> > + *    &drm_crtc_helper_funcs.mode_valid are called on the affected components.
> > + * 5. &drm_bridge_funcs.mode_fixup is called on all encoder bridges.
> > + * 6. &drm_encoder_helper_funcs.atomic_check is called to validate any encoder state.
> >    *    This function is only called when the encoder will be part of a configured crtc,
> >    *    it must not be used for implementing connector property validation.
> >    *    If this function is NULL, &drm_atomic_encoder_helper_funcs.mode_fixup is called
> >    *    instead.
> > - * 6. &drm_crtc_helper_funcs.mode_fixup is called last, to fix up the mode with crtc constraints.
> > + * 7. &drm_crtc_helper_funcs.mode_fixup is called last, to fix up the mode with crtc constraints.
> >    *
> >    * &drm_crtc_state.mode_changed is set when the input mode is changed.
> >    * &drm_crtc_state.connectors_changed is set when a connector is added or
> > @@ -617,6 +682,10 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
> >   			return ret;
> >   	}
> > +	ret = mode_valid(state);
> > +	if (ret)
> > +		return ret;
> > +
> 
> Since we've ensured that the modes won't fail, can mode_fixup() and
> the mode_fixup() ops for crtc/bridge/encoders be assured to not fail
> too? This is assuming that all drivers have moved to using the new
> mode_valid() ops correctly.

The entire point of re-checking is that userspace can create its own modes
and submit them to the kernel, bypassing the current
connector->mode_valid() check. I think almost all drivers get this wrong
unfortunately :(
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2 7/8] drm: Use mode_valid() in atomic modeset

[Laurent Pinchart]

Hi Daniel,

On Wednesday 10 May 2017 19:55:56 Daniel Vetter wrote:
> On Wed, May 10, 2017 at 09:38:00PM +0530, Archit Taneja wrote:
> > On 5/9/2017 10:30 PM, Jose Abreu wrote:
> > > This patches makes use of the new mode_valid() callbacks introduced
> > > previously to validate the full video pipeline when modesetting.
> > > 
> > > This calls the connector->mode_valid(), encoder->mode_valid(),
> > > bridge->mode_valid() and crtc->mode_valid() so that we can
> > > make sure that the mode will be accepted in every components.
> > > 
> > > Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> > > Cc: Carlos Palminha <palminha@synopsys.com>
> > > Cc: Alexey Brodkin <abrodkin@synopsys.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: Dave Airlie <airlied@linux.ie>
> > > Cc: Andrzej Hajda <a.hajda@samsung.com>
> > > Cc: Archit Taneja <architt@codeaurora.org>
> > > ---
> > > 
> > > Changes v1->v2:
> > > 	- Removed call to connector->mode_valid (Ville, Daniel)
> > > 	- Change function name (Ville)
> > > 	- Use for_each_new_connector_in_state (Ville)
> > > 	- Do not validate if connector and mode didn't change (Ville)
> > > 	- Use new helpers to call mode_valid
> > > 	
> > >   drivers/gpu/drm/drm_atomic_helper.c | 75 +++++++++++++++++++++++++++--
> > >   1 file changed, 72 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > > b/drivers/gpu/drm/drm_atomic_helper.c index 8be9719..19d0ea1 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -452,6 +452,69 @@ static int handle_conflicting_encoders(struct
> > > drm_atomic_state *state,
> > >   	return 0;
> > >   }
> > > 
> > > +static enum drm_mode_status mode_valid_path(struct drm_connector
> > > *connector,
> > > +					    struct drm_encoder *encoder,
> > > +					    struct drm_crtc *crtc,
> > > +					    struct drm_display_mode *mode)
> > > +{
> > > +	enum drm_mode_status ret;
> > > +
> > > +	ret = drm_encoder_mode_valid(encoder, mode);
> > > +	if (ret != MODE_OK) {
> > > +		DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] mode_valid() failed\n",
> > > +				encoder->base.id, encoder->name);
> > > +		return ret;
> > > +	}
> > > +
> > > +	ret = drm_bridge_mode_valid(encoder->bridge, mode);
> > > +	if (ret != MODE_OK) {
> > > +		DRM_DEBUG_ATOMIC("[BRIDGE] mode_valid() failed\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	ret = drm_crtc_mode_valid(crtc, mode);
> > > +	if (ret != MODE_OK) {
> > > +		DRM_DEBUG_ATOMIC("[CRTC:%d:%s] mode_valid() failed\n",
> > > +				crtc->base.id, crtc->name);
> > > +		return ret;
> > > +	}
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int
> > > +mode_valid(struct drm_atomic_state *state)
> > > +{
> > > +	struct drm_connector_state *conn_state;
> > > +	struct drm_connector *connector;
> > > +	int i;
> > > +
> > > +	for_each_new_connector_in_state(state, connector, conn_state, i) {
> > > +		struct drm_encoder *encoder = conn_state->best_encoder;
> > > +		struct drm_crtc *crtc = conn_state->crtc;
> > > +		struct drm_crtc_state *crtc_state;
> > > +		enum drm_mode_status mode_status;
> > > +		struct drm_display_mode *mode;
> > > +
> > > +		if (!crtc || !encoder)
> > > +			continue;
> > > +
> > > +		crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> > > +		if (!crtc_state)
> > > +			continue;
> > > +		if (!crtc_state->mode_changed && !crtc_state
> > > ->connectors_changed)
> > > +			continue;
> > > +
> > > +		mode = &crtc_state->mode;
> > > +
> > > +		mode_status = mode_valid_path(connector, encoder, crtc, mode);
> > > +		if (mode_status != MODE_OK)
> > > +			return -EINVAL;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >   /**
> > >    * drm_atomic_helper_check_modeset - validate state object for modeset
> > >    changes * @dev: DRM device
> > > @@ -466,13 +529,15 @@ static int handle_conflicting_encoders(struct
> > > drm_atomic_state *state,
> > >   * 2. &drm_connector_helper_funcs.atomic_check to validate the
> > >   connector state.
> > >   * 3. If it's determined a modeset is needed then all connectors on the
> > >   affected crtc
> > >   *    crtc are added and &drm_connector_helper_funcs.atomic_check is
> > >   run on them.
> > > - * 4. &drm_bridge_funcs.mode_fixup is called on all encoder bridges.
> > > - * 5. &drm_encoder_helper_funcs.atomic_check is called to validate any
> > > encoder state.
> > > + * 4. &drm_encoder_helper_funcs.mode_valid,
> > > &drm_bridge_funcs.mode_valid and
> > > + *    &drm_crtc_helper_funcs.mode_valid are called on the affected
> > > components.
> > > + * 5. &drm_bridge_funcs.mode_fixup is called on all encoder bridges.
> > > + * 6. &drm_encoder_helper_funcs.atomic_check is called to validate any
> > > encoder state.
> > >   *    This function is only called when the encoder will be part of a
> > >   configured crtc,
> > >   *    it must not be used for implementing connector property
> > >   validation.
> > >   *    If this function is NULL,
> > >   &drm_atomic_encoder_helper_funcs.mode_fixup is called
> > >   *    instead.
> > > - * 6. &drm_crtc_helper_funcs.mode_fixup is called last, to fix up the
> > > mode with crtc constraints.
> > > + * 7. &drm_crtc_helper_funcs.mode_fixup is called last, to fix up the
> > > mode with crtc constraints.
> > >    *
> > >    * &drm_crtc_state.mode_changed is set when the input mode is changed.
> > >    * &drm_crtc_state.connectors_changed is set when a connector is added
> > >    or
> > > 
> > > @@ -617,6 +682,10 @@ static int handle_conflicting_encoders(struct
> > > drm_atomic_state *state,
> > >   			return ret;
> > >   	}
> > > 
> > > +	ret = mode_valid(state);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > 
> > Since we've ensured that the modes won't fail, can mode_fixup() and
> > the mode_fixup() ops for crtc/bridge/encoders be assured to not fail
> > too? This is assuming that all drivers have moved to using the new
> > mode_valid() ops correctly.
> 
> The entire point of re-checking is that userspace can create its own modes
> and submit them to the kernel, bypassing the current
> connector->mode_valid() check. I think almost all drivers get this wrong
> unfortunately :(

How about documenting the expected driver behaviour in details ? I won't 
volunteer as I have to confess it's not clear to me what we expect from 
drivers.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 7/8] drm: Use mode_valid() in atomic modeset

[Daniel Vetter]

On Fri, May 12, 2017 at 12:53:56PM +0300, Laurent Pinchart wrote:
> Hi Daniel,
> 
> On Wednesday 10 May 2017 19:55:56 Daniel Vetter wrote:
> > On Wed, May 10, 2017 at 09:38:00PM +0530, Archit Taneja wrote:
> > > On 5/9/2017 10:30 PM, Jose Abreu wrote:
> > > > This patches makes use of the new mode_valid() callbacks introduced
> > > > previously to validate the full video pipeline when modesetting.
> > > > 
> > > > This calls the connector->mode_valid(), encoder->mode_valid(),
> > > > bridge->mode_valid() and crtc->mode_valid() so that we can
> > > > make sure that the mode will be accepted in every components.
> > > > 
> > > > Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> > > > Cc: Carlos Palminha <palminha@synopsys.com>
> > > > Cc: Alexey Brodkin <abrodkin@synopsys.com>
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > Cc: Dave Airlie <airlied@linux.ie>
> > > > Cc: Andrzej Hajda <a.hajda@samsung.com>
> > > > Cc: Archit Taneja <architt@codeaurora.org>
> > > > ---
> > > > 
> > > > Changes v1->v2:
> > > > 	- Removed call to connector->mode_valid (Ville, Daniel)
> > > > 	- Change function name (Ville)
> > > > 	- Use for_each_new_connector_in_state (Ville)
> > > > 	- Do not validate if connector and mode didn't change (Ville)
> > > > 	- Use new helpers to call mode_valid
> > > > 	
> > > >   drivers/gpu/drm/drm_atomic_helper.c | 75 +++++++++++++++++++++++++++--
> > > >   1 file changed, 72 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > > > b/drivers/gpu/drm/drm_atomic_helper.c index 8be9719..19d0ea1 100644
> > > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > @@ -452,6 +452,69 @@ static int handle_conflicting_encoders(struct
> > > > drm_atomic_state *state,
> > > >   	return 0;
> > > >   }
> > > > 
> > > > +static enum drm_mode_status mode_valid_path(struct drm_connector
> > > > *connector,
> > > > +					    struct drm_encoder *encoder,
> > > > +					    struct drm_crtc *crtc,
> > > > +					    struct drm_display_mode *mode)
> > > > +{
> > > > +	enum drm_mode_status ret;
> > > > +
> > > > +	ret = drm_encoder_mode_valid(encoder, mode);
> > > > +	if (ret != MODE_OK) {
> > > > +		DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] mode_valid() failed\n",
> > > > +				encoder->base.id, encoder->name);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	ret = drm_bridge_mode_valid(encoder->bridge, mode);
> > > > +	if (ret != MODE_OK) {
> > > > +		DRM_DEBUG_ATOMIC("[BRIDGE] mode_valid() failed\n");
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	ret = drm_crtc_mode_valid(crtc, mode);
> > > > +	if (ret != MODE_OK) {
> > > > +		DRM_DEBUG_ATOMIC("[CRTC:%d:%s] mode_valid() failed\n",
> > > > +				crtc->base.id, crtc->name);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static int
> > > > +mode_valid(struct drm_atomic_state *state)
> > > > +{
> > > > +	struct drm_connector_state *conn_state;
> > > > +	struct drm_connector *connector;
> > > > +	int i;
> > > > +
> > > > +	for_each_new_connector_in_state(state, connector, conn_state, i) {
> > > > +		struct drm_encoder *encoder = conn_state->best_encoder;
> > > > +		struct drm_crtc *crtc = conn_state->crtc;
> > > > +		struct drm_crtc_state *crtc_state;
> > > > +		enum drm_mode_status mode_status;
> > > > +		struct drm_display_mode *mode;
> > > > +
> > > > +		if (!crtc || !encoder)
> > > > +			continue;
> > > > +
> > > > +		crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> > > > +		if (!crtc_state)
> > > > +			continue;
> > > > +		if (!crtc_state->mode_changed && !crtc_state
> > > > ->connectors_changed)
> > > > +			continue;
> > > > +
> > > > +		mode = &crtc_state->mode;
> > > > +
> > > > +		mode_status = mode_valid_path(connector, encoder, crtc, mode);
> > > > +		if (mode_status != MODE_OK)
> > > > +			return -EINVAL;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > >   /**
> > > >    * drm_atomic_helper_check_modeset - validate state object for modeset
> > > >    changes * @dev: DRM device
> > > > @@ -466,13 +529,15 @@ static int handle_conflicting_encoders(struct
> > > > drm_atomic_state *state,
> > > >   * 2. &drm_connector_helper_funcs.atomic_check to validate the
> > > >   connector state.
> > > >   * 3. If it's determined a modeset is needed then all connectors on the
> > > >   affected crtc
> > > >   *    crtc are added and &drm_connector_helper_funcs.atomic_check is
> > > >   run on them.
> > > > - * 4. &drm_bridge_funcs.mode_fixup is called on all encoder bridges.
> > > > - * 5. &drm_encoder_helper_funcs.atomic_check is called to validate any
> > > > encoder state.
> > > > + * 4. &drm_encoder_helper_funcs.mode_valid,
> > > > &drm_bridge_funcs.mode_valid and
> > > > + *    &drm_crtc_helper_funcs.mode_valid are called on the affected
> > > > components.
> > > > + * 5. &drm_bridge_funcs.mode_fixup is called on all encoder bridges.
> > > > + * 6. &drm_encoder_helper_funcs.atomic_check is called to validate any
> > > > encoder state.
> > > >   *    This function is only called when the encoder will be part of a
> > > >   configured crtc,
> > > >   *    it must not be used for implementing connector property
> > > >   validation.
> > > >   *    If this function is NULL,
> > > >   &drm_atomic_encoder_helper_funcs.mode_fixup is called
> > > >   *    instead.
> > > > - * 6. &drm_crtc_helper_funcs.mode_fixup is called last, to fix up the
> > > > mode with crtc constraints.
> > > > + * 7. &drm_crtc_helper_funcs.mode_fixup is called last, to fix up the
> > > > mode with crtc constraints.
> > > >    *
> > > >    * &drm_crtc_state.mode_changed is set when the input mode is changed.
> > > >    * &drm_crtc_state.connectors_changed is set when a connector is added
> > > >    or
> > > > 
> > > > @@ -617,6 +682,10 @@ static int handle_conflicting_encoders(struct
> > > > drm_atomic_state *state,
> > > >   			return ret;
> > > >   	}
> > > > 
> > > > +	ret = mode_valid(state);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > 
> > > Since we've ensured that the modes won't fail, can mode_fixup() and
> > > the mode_fixup() ops for crtc/bridge/encoders be assured to not fail
> > > too? This is assuming that all drivers have moved to using the new
> > > mode_valid() ops correctly.
> > 
> > The entire point of re-checking is that userspace can create its own modes
> > and submit them to the kernel, bypassing the current
> > connector->mode_valid() check. I think almost all drivers get this wrong
> > unfortunately :(
> 
> How about documenting the expected driver behaviour in details ? I won't 
> volunteer as I have to confess it's not clear to me what we expect from 
> drivers.

It already is? Or where exactly is it lacking in the docs?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH v2 8/8] drm: arc: Use crtc->mode_valid() callback

[Jose Abreu]

Now that we have a callback to check if crtc supports a given mode
we can use it in arcpgu so that we restrict the number of probbed
modes to the ones we can actually display.

This is specially useful because arcpgu crtc is responsible to set
a clock value in the commit() stage but unfortunatelly this clock
does not support all the needed ranges.

Also, remove the atomic_check() callback as mode_valid() callback
will be called before.

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Cc: Carlos Palminha <palminha@synopsys.com>
Cc: Alexey Brodkin <abrodkin@synopsys.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Dave Airlie <airlied@linux.ie>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Archit Taneja <architt@codeaurora.org>
---
 drivers/gpu/drm/arc/arcpgu_crtc.c | 39 ++++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c b/drivers/gpu/drm/arc/arcpgu_crtc.c
index ad9a959..01cae0a 100644
--- a/drivers/gpu/drm/arc/arcpgu_crtc.c
+++ b/drivers/gpu/drm/arc/arcpgu_crtc.c
@@ -32,6 +32,18 @@
 	{ "r8g8b8", 24, {16, 8}, {8, 8}, {0, 8}, {0, 0}, DRM_FORMAT_RGB888 },
 };
 
+static bool arc_pgu_is_mode_valid(struct arcpgu_drm_private *arcpgu,
+				  const struct drm_display_mode *mode)
+{
+	long rate, clk_rate = mode->clock * 1000;
+
+	rate = clk_round_rate(arcpgu->clk, clk_rate);
+	if (rate != clk_rate)
+		return false;
+
+	return true;
+}
+
 static void arc_pgu_set_pxl_fmt(struct drm_crtc *crtc)
 {
 	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
@@ -64,6 +76,17 @@ static void arc_pgu_set_pxl_fmt(struct drm_crtc *crtc)
 	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
 };
 
+enum drm_mode_status arc_pgu_crtc_mode_valid(struct drm_crtc *crtc,
+					     const struct drm_display_mode *mode)
+{
+	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
+
+	if (!arc_pgu_is_mode_valid(arcpgu, mode))
+		return MODE_NOCLOCK;
+
+	return MODE_OK;
+}
+
 static void arc_pgu_crtc_mode_set_nofb(struct drm_crtc *crtc)
 {
 	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
@@ -129,20 +152,6 @@ static void arc_pgu_crtc_disable(struct drm_crtc *crtc)
 			      ~ARCPGU_CTRL_ENABLE_MASK);
 }
 
-static int arc_pgu_crtc_atomic_check(struct drm_crtc *crtc,
-				     struct drm_crtc_state *state)
-{
-	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
-	struct drm_display_mode *mode = &state->adjusted_mode;
-	long rate, clk_rate = mode->clock * 1000;
-
-	rate = clk_round_rate(arcpgu->clk, clk_rate);
-	if (rate != clk_rate)
-		return -EINVAL;
-
-	return 0;
-}
-
 static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc,
 				      struct drm_crtc_state *state)
 {
@@ -158,6 +167,7 @@ static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc,
 }
 
 static const struct drm_crtc_helper_funcs arc_pgu_crtc_helper_funcs = {
+	.mode_valid	= arc_pgu_crtc_mode_valid,
 	.mode_set	= drm_helper_crtc_mode_set,
 	.mode_set_base	= drm_helper_crtc_mode_set_base,
 	.mode_set_nofb	= arc_pgu_crtc_mode_set_nofb,
@@ -165,7 +175,6 @@ static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc,
 	.disable	= arc_pgu_crtc_disable,
 	.prepare	= arc_pgu_crtc_disable,
 	.commit		= arc_pgu_crtc_enable,
-	.atomic_check	= arc_pgu_crtc_atomic_check,
 	.atomic_begin	= arc_pgu_crtc_atomic_begin,
 };
 
-- 
1.9.1

Re: [PATCH v2 8/8] drm: arc: Use crtc->mode_valid() callback

[Laurent Pinchart]

Hi Jose,

Thank you for the patch.

On Tuesday 09 May 2017 18:00:15 Jose Abreu wrote:
> Now that we have a callback to check if crtc supports a given mode
> we can use it in arcpgu so that we restrict the number of probbed
> modes to the ones we can actually display.
> 
> This is specially useful because arcpgu crtc is responsible to set
> a clock value in the commit() stage but unfortunatelly this clock
> does not support all the needed ranges.
> 
> Also, remove the atomic_check() callback as mode_valid() callback
> will be called before.
> 
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> Cc: Carlos Palminha <palminha@synopsys.com>
> Cc: Alexey Brodkin <abrodkin@synopsys.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Dave Airlie <airlied@linux.ie>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Archit Taneja <architt@codeaurora.org>
> ---
>  drivers/gpu/drm/arc/arcpgu_crtc.c | 39 ++++++++++++++++++++---------------
>  1 file changed, 24 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c
> b/drivers/gpu/drm/arc/arcpgu_crtc.c index ad9a959..01cae0a 100644
> --- a/drivers/gpu/drm/arc/arcpgu_crtc.c
> +++ b/drivers/gpu/drm/arc/arcpgu_crtc.c
> @@ -32,6 +32,18 @@
>  	{ "r8g8b8", 24, {16, 8}, {8, 8}, {0, 8}, {0, 0}, DRM_FORMAT_RGB888 },
>  };
> 
> +static bool arc_pgu_is_mode_valid(struct arcpgu_drm_private *arcpgu,
> +				  const struct drm_display_mode *mode)
> +{
> +	long rate, clk_rate = mode->clock * 1000;
> +
> +	rate = clk_round_rate(arcpgu->clk, clk_rate);
> +	if (rate != clk_rate)
> +		return false;

This isn't anything new introduced by this patch, but shouldn't drivers allow 
for some margin in clock frequencies ? Surely if the mode requires a 
60.000.000 Hz frequency and the hardware can only generate 59.999.999 Hz or 
60.000.001 Hz we shouldn't fail. As far as I understand, this is something the 
mode_fixup() operation is supposed to handle, but the arc driver doesn't 
implement it.

> +	return true;
> +}

Can't you inline this in arc_pgu_crtc_mode_valid() as there's a single caller 
?

>  static void arc_pgu_set_pxl_fmt(struct drm_crtc *crtc)
>  {
>  	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
> @@ -64,6 +76,17 @@ static void arc_pgu_set_pxl_fmt(struct drm_crtc *crtc)
>  	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
>  };
> 
> +enum drm_mode_status arc_pgu_crtc_mode_valid(struct drm_crtc *crtc,
> +					     const struct drm_display_mode 
*mode)
> +{
> +	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
> +
> +	if (!arc_pgu_is_mode_valid(arcpgu, mode))
> +		return MODE_NOCLOCK;
> +
> +	return MODE_OK;
> +}
> +
>  static void arc_pgu_crtc_mode_set_nofb(struct drm_crtc *crtc)
>  {
>  	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
> @@ -129,20 +152,6 @@ static void arc_pgu_crtc_disable(struct drm_crtc *crtc)
> ~ARCPGU_CTRL_ENABLE_MASK);
>  }
> 
> -static int arc_pgu_crtc_atomic_check(struct drm_crtc *crtc,
> -				     struct drm_crtc_state *state)
> -{
> -	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
> -	struct drm_display_mode *mode = &state->adjusted_mode;
> -	long rate, clk_rate = mode->clock * 1000;
> -
> -	rate = clk_round_rate(arcpgu->clk, clk_rate);
> -	if (rate != clk_rate)
> -		return -EINVAL;
> -
> -	return 0;
> -}
> -
>  static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc,
>  				      struct drm_crtc_state *state)
>  {
> @@ -158,6 +167,7 @@ static void arc_pgu_crtc_atomic_begin(struct drm_crtc
> *crtc, }
> 
>  static const struct drm_crtc_helper_funcs arc_pgu_crtc_helper_funcs = {
> +	.mode_valid	= arc_pgu_crtc_mode_valid,
>  	.mode_set	= drm_helper_crtc_mode_set,
>  	.mode_set_base	= drm_helper_crtc_mode_set_base,
>  	.mode_set_nofb	= arc_pgu_crtc_mode_set_nofb,
> @@ -165,7 +175,6 @@ static void arc_pgu_crtc_atomic_begin(struct drm_crtc
> *crtc, .disable	= arc_pgu_crtc_disable,
>  	.prepare	= arc_pgu_crtc_disable,
>  	.commit		= arc_pgu_crtc_enable,
> -	.atomic_check	= arc_pgu_crtc_atomic_check,
>  	.atomic_begin	= arc_pgu_crtc_atomic_begin,
>  };

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 8/8] drm: arc: Use crtc->mode_valid() callback

[Jose Abreu]

Hi Laurent,


Sorry for the late reply.


On 12-05-2017 10:57, Laurent Pinchart wrote:
> Hi Jose,
>
> Thank you for the patch.
>
> On Tuesday 09 May 2017 18:00:15 Jose Abreu wrote:
>> Now that we have a callback to check if crtc supports a given mode
>> we can use it in arcpgu so that we restrict the number of probbed
>> modes to the ones we can actually display.
>>
>> This is specially useful because arcpgu crtc is responsible to set
>> a clock value in the commit() stage but unfortunatelly this clock
>> does not support all the needed ranges.
>>
>> Also, remove the atomic_check() callback as mode_valid() callback
>> will be called before.
>>
>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>> Cc: Carlos Palminha <palminha@synopsys.com>
>> Cc: Alexey Brodkin <abrodkin@synopsys.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Dave Airlie <airlied@linux.ie>
>> Cc: Andrzej Hajda <a.hajda@samsung.com>
>> Cc: Archit Taneja <architt@codeaurora.org>
>> ---
>>  drivers/gpu/drm/arc/arcpgu_crtc.c | 39 ++++++++++++++++++++---------------
>>  1 file changed, 24 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c
>> b/drivers/gpu/drm/arc/arcpgu_crtc.c index ad9a959..01cae0a 100644
>> --- a/drivers/gpu/drm/arc/arcpgu_crtc.c
>> +++ b/drivers/gpu/drm/arc/arcpgu_crtc.c
>> @@ -32,6 +32,18 @@
>>  	{ "r8g8b8", 24, {16, 8}, {8, 8}, {0, 8}, {0, 0}, DRM_FORMAT_RGB888 },
>>  };
>>
>> +static bool arc_pgu_is_mode_valid(struct arcpgu_drm_private *arcpgu,
>> +				  const struct drm_display_mode *mode)
>> +{
>> +	long rate, clk_rate = mode->clock * 1000;
>> +
>> +	rate = clk_round_rate(arcpgu->clk, clk_rate);
>> +	if (rate != clk_rate)
>> +		return false;
> This isn't anything new introduced by this patch, but shouldn't drivers allow 
> for some margin in clock frequencies ? Surely if the mode requires a 
> 60.000.000 Hz frequency and the hardware can only generate 59.999.999 Hz or 
> 60.000.001 Hz we shouldn't fail. As far as I understand, this is something the 
> mode_fixup() operation is supposed to handle, but the arc driver doesn't 
> implement it.

Its funny you mentioned this because I had exactly the same
discussion with Alexey (arcpgu maintainer) last Friday. Perhaps
we could think about a better way for this.

The main problem is that clock driver does not have all the
available clock values, so some modes will not have a compliant
clock value. Right now we are using clk_round_rate() and checking
if the return value matches the one we supplied, but as you
mentioned for 60vs59.94, for example, the mode will still fail to
commit because the clock will not be the same. We reached to the
conclusion that we could in this function have a max deviation
(which would be determined by max allowed tmds variation in HDMI
spec). What do you think?

Initially my idea was that clock driver should support this
variation and just return the same rate if the clock supported it
(i.e. if the deviation was not that much), but, as mentioned by
Alexey, the clock driver is agnostic of TMDS deviation, its just
a clock driver.

>
>> +	return true;
>> +}
> Can't you inline this in arc_pgu_crtc_mode_valid() as there's a single caller 
> ?

Yeah, I will in next version. This was a leftover from previous
version where atomic_check() and mode_valid() did the same
validation.

Thanks!

Best regards,
Jose Miguel Abreu

>>  static void arc_pgu_set_pxl_fmt(struct drm_crtc *crtc)
>>  {
>>  	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
>> @@ -64,6 +76,17 @@ static void arc_pgu_set_pxl_fmt(struct drm_crtc *crtc)
>>  	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
>>  };
>>
>> +enum drm_mode_status arc_pgu_crtc_mode_valid(struct drm_crtc *crtc,
>> +					     const struct drm_display_mode 
> *mode)
>> +{
>> +	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
>> +
>> +	if (!arc_pgu_is_mode_valid(arcpgu, mode))
>> +		return MODE_NOCLOCK;
>> +
>> +	return MODE_OK;
>> +}
>> +
>>  static void arc_pgu_crtc_mode_set_nofb(struct drm_crtc *crtc)
>>  {
>>  	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
>> @@ -129,20 +152,6 @@ static void arc_pgu_crtc_disable(struct drm_crtc *crtc)
>> ~ARCPGU_CTRL_ENABLE_MASK);
>>  }
>>
>> -static int arc_pgu_crtc_atomic_check(struct drm_crtc *crtc,
>> -				     struct drm_crtc_state *state)
>> -{
>> -	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
>> -	struct drm_display_mode *mode = &state->adjusted_mode;
>> -	long rate, clk_rate = mode->clock * 1000;
>> -
>> -	rate = clk_round_rate(arcpgu->clk, clk_rate);
>> -	if (rate != clk_rate)
>> -		return -EINVAL;
>> -
>> -	return 0;
>> -}
>> -
>>  static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc,
>>  				      struct drm_crtc_state *state)
>>  {
>> @@ -158,6 +167,7 @@ static void arc_pgu_crtc_atomic_begin(struct drm_crtc
>> *crtc, }
>>
>>  static const struct drm_crtc_helper_funcs arc_pgu_crtc_helper_funcs = {
>> +	.mode_valid	= arc_pgu_crtc_mode_valid,
>>  	.mode_set	= drm_helper_crtc_mode_set,
>>  	.mode_set_base	= drm_helper_crtc_mode_set_base,
>>  	.mode_set_nofb	= arc_pgu_crtc_mode_set_nofb,
>> @@ -165,7 +175,6 @@ static void arc_pgu_crtc_atomic_begin(struct drm_crtc
>> *crtc, .disable	= arc_pgu_crtc_disable,
>>  	.prepare	= arc_pgu_crtc_disable,
>>  	.commit		= arc_pgu_crtc_enable,
>> -	.atomic_check	= arc_pgu_crtc_atomic_check,
>>  	.atomic_begin	= arc_pgu_crtc_atomic_begin,
>>  };