Introduce crtc->mode_valid() callback
Subsystems (2)
v2 cover Introduce crtc->mode_valid() callback
This patchset introduces a new callback for crtc, called mode_valid() that is responsible to limit the number of probbed modes. Just like connector->mode_valid(), this new callback is called at mode probbing stage so that we can validate the mode.
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. This way we can restrict the number of modes that are handed to userspace, so that we dont hand a mode that will fail the commit() stage.
I guess this can also happen for other drivers so we introduce the callback in the core.
The behaviour remains the same for crtcs that don't have the callback. Also, for a given set of crtcs that can be bound to the connector, if at least one can display the mode then the mode will be probbed.
For more info about why this is needed in arcpgu, please refer here [1].
[1] https://patchwork.kernel.org/patch/9694177/
Jose Abreu (2):
drm: Introduce crtc->mode_valid() callback
drm: arcpgu: Use crtc->mode_valid() callback
drivers/gpu/drm/arc/arcpgu_crtc.c | 28 ++++++++++++++++-- drivers/gpu/drm/drm_probe_helper.c | 50 ++++++++++++++++++++++++++++++-- include/drm/drm_modeset_helper_vtables.h | 26 +++++++++++++++++ 3 files changed, 99 insertions(+), 5 deletions(-)
Signature · 1 line
1.9.1
PATCH v2 1/2 drm: Introduce crtc->mode_valid() callback
Some crtc's may have restrictions in the mode they can display. In this patch a new callback (crtc->mode_valid()) is introduced that is called at the same stage of connector->mode_valid() callback.
This shall be implemented if the crtc has some sort of restriction so that we don't probe modes that will fail in the commit() stage. For example: A given crtc may be responsible to set a clock value. If the clock can not produce all the values for the available modes then this callback can be used to restrict the number of probbed modes to only the ones that can be displayed.
If the crtc does not implement the callback then the behaviour will remain the same. Also, for a given set of crtcs that can be bound to the connector, if at least one can display the mode then the mode will be probbed.
---
Changes v1->v2:
- Correct indentation
- Change function name to drm_validate_connector
- Move connnector->mode_valid() to new function
- Change crtc->mode_valid() "mode" field to const
- Code reogarnization
- Return earlier if crtc accepts mode
drivers/gpu/drm/drm_probe_helper.c | 50 ++++++++++++++++++++++++++++++-- include/drm/drm_modeset_helper_vtables.h | 26 +++++++++++++++++ 2 files changed, 74 insertions(+), 2 deletions(-)
drivers/gpu/drm/drm_probe_helper.c +48 -2
@@ -80,6 +80,52 @@return MODE_OK ; } +static enum drm_mode_status +drm_mode_validate_connector (struct drm_connector *connector, +struct drm_display_mode *mode) +{ +const struct drm_connector_helper_funcs *connector_funcs = + connector->helper_private; +struct drm_device *dev = connector->dev; +uint32_t *ids = connector->encoder_ids; +enum drm_mode_status ret =MODE_OK ; +unsigned int i; + +/* Step 1: Validate against connector */ +if (connector_funcs->mode_valid) + ret = connector_funcs->mode_valid (connector, mode); + +if (ret !=MODE_OK ) +return ret; + +/* Step 2: Validate against crtc's */ +for (i =0 ; i <DRM_CONNECTOR_MAX_ENCODER ; i++) { +struct drm_encoder *encoder =drm_encoder_find (dev, ids[i]); +struct drm_crtc *crtc; + +if (!encoder) +continue ; + +drm_for_each_crtc (crtc, dev) { +const struct drm_crtc_helper_funcs *crtc_funcs; + +if (!drm_encoder_crtc_ok (encoder, crtc)) +continue ; + + crtc_funcs = crtc->helper_private; +if (!crtc_funcs || !crtc_funcs->mode_valid) +return MODE_OK ;/* crtc accepts everything */ + + ret = crtc_funcs->mode_valid (crtc, mode); +if (ret ==MODE_OK ) +return ret; + } + } + +/* NOTE: If no crtc or encoder is found then we return MODE_OK */ +return ret; +} +static int drm_helper_probe_add_cmdline_mode (struct drm_connector *connector) {struct drm_cmdline_mode *cmdline_mode;
@@ -428,8 +474,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,if (mode->status ==MODE_OK ) mode->status =drm_mode_validate_flag (mode, mode_flags); -if (mode->status ==MODE_OK && connector_funcs->mode_valid) - mode->status = connector_funcs->mode_valid (connector, +if (mode->status ==MODE_OK ) + mode->status =drm_mode_validate_connector (connector, mode); }
include/drm/drm_modeset_helper_vtables.h +26 -0
@@ -106,6 +106,32 @@ struct drm_crtc_helper_funcs {void (*commit)(struct drm_crtc *crtc);/** +* @mode_valid: +* +* This callback should be implemented if the crtc has some sort of +* restriction in the modes it can display. For example, a given crtc +* may be responsible to set a clock value. If the clock can not +* produce all the values for the available modes then this callback +* can be used to restrict the number of probbed modes to only the ones +* that can be displayed. +* +* This is directly called at the same stage of connector->mode_valid +* callback. +* +* NOTE: +* +* For a given set of crtc's in a drm_device, if at least one does not +* have the mode_valid callback, or, at least one returns MODE_OK then +* the mode will be probbed. +* +* RETURNS: +* +* drm_mode_status Enum +*/ +enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc, +const struct drm_display_mode *mode); + +/** * @mode_fixup: * * This callback is used to validate a mode. The parameter mode is the
Signature · 1 line
1.9.1
Re: [PATCH v2 1/2] drm: Introduce crtc->mode_valid() callback
Quoted 79 lines · Jose Abreu
Some crtc's may have restrictions in the mode they can display. In this patch a new callback (
crtc->mode_valid()) is introduced that is called at the same stage ofconnector->mode_valid()callback.This shall be implemented if the crtc has some sort of restriction so that we don't probe modes that will fail in the
commit()stage. For example: A given crtc may be responsible to set a clock value. If the clock can not produce all the values for the available modes then this callback can be used to restrict the number of probbed modes to only the ones that can be displayed.If the crtc does not implement the callback then the behaviour will remain the same. Also, for a given set of crtcs that can be bound to the connector, if at least one can display the mode then the mode will be probbed.
Signed-off-by Jose Abreu <[email protected]>Cc Carlos Palminha <[email protected]>Cc Alexey Brodkin <[email protected]>Cc Ville Syrjälä <[email protected]>Cc Daniel Vetter <[email protected]>Cc Dave Airlie <[email protected]>Cc Andrzej Hajda <[email protected]>---
Changes
v1->v2:
- Correct indentation
- Change function name todrm_validate_connector
- Moveconnnector->mode_valid()to new function
- Changecrtc->mode_valid()"mode" field to const
- Code reogarnization
- Return earlier if crtc accepts modedrivers/gpu/drm/drm_probe_helper.c | 50 ++++++++++++++++++++++++++++++-- include/drm/drm_modeset_helper_vtables.h | 26 +++++++++++++++++ 2 files changed, 74 insertions(+), 2 deletions(-)drivers/gpu/drm/drm_probe_helper.c +34
-0@@ -80,6 +80,52 @@return MODE_OK ; } +static enum drm_mode_status +drm_mode_validate_connector (struct drm_connector *connector, +struct drm_display_mode *mode) +{ +const struct drm_connector_helper_funcs *connector_funcs = + connector->helper_private; +struct drm_device *dev = connector->dev; +uint32_t *ids = connector->encoder_ids; +enum drm_mode_status ret =MODE_OK ; +unsigned int i; + +/* Step 1: Validate against connector */ +if (connector_funcs->mode_valid) + ret = connector_funcs->mode_valid (connector, mode); + +if (ret !=MODE_OK ) +return ret; + +/* Step 2: Validate against crtc's */ +for (i =0 ; i <DRM_CONNECTOR_MAX_ENCODER ; i++) { +struct drm_encoder *encoder =drm_encoder_find (dev, ids[i]); +struct drm_crtc *crtc; + +if (!encoder) +continue ; + +drm_for_each_crtc (crtc, dev) { +const struct drm_crtc_helper_funcs *crtc_funcs; + +if (!drm_encoder_crtc_ok (encoder, crtc)) +continue ; + + crtc_funcs = crtc->helper_private; +if (!crtc_funcs || !crtc_funcs->mode_valid)
drivers/gpu/drm/drm_probe_helper.c line 126
Quoted 10 lines
drivers/gpu/drm/drm_probe_helper.c +9
-0+return MODE_OK ;/* crtc accepts everything */ + + ret = crtc_funcs->mode_valid (crtc, mode); +if (ret ==MODE_OK ) +return ret; + } + } + +/* NOTE: If no crtc or encoder is found then we return MODE_OK */
I think this comment is misleading because here ret is not alwyas MODE_OK. It will be the return value from crtc_func->mode_valid which could also be MODE_NOCLOCK
Manasi
Quoted 62 lines
drivers/gpu/drm/drm_probe_helper.c +3
-0+return ret; +} +static int drm_helper_probe_add_cmdline_mode (struct drm_connector *connector) {drivers/gpu/drm/drm_probe_helper.c +2
-2struct drm_cmdline_mode *cmdline_mode;@@ -428,8 +474,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,if (mode->status ==MODE_OK ) mode->status =drm_mode_validate_flag (mode, mode_flags); -if (mode->status ==MODE_OK && connector_funcs->mode_valid) - mode->status = connector_funcs->mode_valid (connector, +if (mode->status ==MODE_OK ) + mode->status =drm_mode_validate_connector (connector, mode); }include/drm/drm_modeset_helper_vtables.h +26
-0@@ -106,6 +106,32 @@ struct drm_crtc_helper_funcs {void (*commit)(struct drm_crtc *crtc);/** +* @mode_valid: +* +* This callback should be implemented if the crtc has some sort of +* restriction in the modes it can display. For example, a given crtc +* may be responsible to set a clock value. If the clock can not +* produce all the values for the available modes then this callback +* can be used to restrict the number of probbed modes to only the ones +* that can be displayed. +* +* This is directly called at the same stage of connector->mode_valid +* callback. +* +* NOTE: +* +* For a given set of crtc's in a drm_device, if at least one does not +* have the mode_valid callback, or, at least one returns MODE_OK then +* the mode will be probbed. +* +* RETURNS: +* +* drm_mode_status Enum +*/ +enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc, +const struct drm_display_mode *mode); + +/** * @mode_fixup: * * This callback is used to validate a mode. The parameter mode is the Signature · 7 lines
1.9.1 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH v2 2/2 drm: arcpgu: Use crtc->mode_valid() callback
In reply to Jose AbreuNow 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.
---
Changes v1->v2:
- Reduce code duplication by using helper function
- crtc->mode_valid() "mode" field is now const
drivers/gpu/drm/arc/arcpgu_crtc.c | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-)
drivers/gpu/drm/arc/arcpgu_crtc.c +25 -3
@@ -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);
@@ -134,10 +157,8 @@ static int arc_pgu_crtc_atomic_check(struct drm_crtc *crtc, {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) +if (!arc_pgu_is_mode_valid (arcpgu, mode))return -EINVAL ;return 0 ;
@@ -158,6 +179,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,
Signature · 1 line
1.9.1