drm: Add few more wrapper functions for drm panel
Subsystems (4)
PATCH 1/2 drm: Add few more wrapper functions for drm panel
Currently there are few pair of functions which are called during the panel enable/disable sequence. To improve the granularity, adding few more wrapper functions so that the functions are more specific on what they are doing.
Cc David Airlie <[email protected]>
Cc Ville Syrjälä <[email protected]>
Cc Daniel Vetter <[email protected]>
Cc Jani Nikula <[email protected]>
Signed-off-by Deepak M <[email protected]>
Signed-off-by Gaurav K Singh <[email protected]>
--- include/drm/drm_panel.h | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)
include/drm/drm_panel.h +47 -0
@@ -73,6 +73,12 @@ struct drm_panel_funcs {int (*get_modes)(struct drm_panel *panel);int (*get_timings)(struct drm_panel *panel,unsigned int num_timings,struct display_timing *timings); +int (*power_on)(struct drm_panel *panel); +int (*power_off)(struct drm_panel *panel); +int (*backlight_on)(struct drm_panel *panel); +int (*backlight_off)(struct drm_panel *panel); +int (*get_info)(struct drm_panel *panel, +struct drm_connector *connector); };struct drm_panel {
@@ -117,6 +123,47 @@ static inline int drm_panel_enable(struct drm_panel *panel)return panel ? -ENOSYS : -EINVAL ; } +static inline int drm_panel_power_on (struct drm_panel *panel) +{ +if (panel && panel->funcs && panel->funcs->power_on) +return panel->funcs->power_on (panel); + +return panel ? -ENOSYS : -EINVAL ; +} + +static inline int drm_panel_power_off (struct drm_panel *panel) +{ +if (panel && panel->funcs && panel->funcs->power_off) +return panel->funcs->power_off (panel); + +return panel ? -ENOSYS : -EINVAL ; +} + +static inline int drm_panel_backlight_on (struct drm_panel *panel) +{ +if (panel && panel->funcs && panel->funcs->backlight_on) +return panel->funcs->backlight_on (panel); + +return panel ? -ENOSYS : -EINVAL ; +} + +static inline int drm_panel_backlight_off (struct drm_panel *panel) +{ +if (panel && panel->funcs && panel->funcs->backlight_off) +return panel->funcs->backlight_off (panel); + +return panel ? -ENOSYS : -EINVAL ; +} + +static inline int drm_panel_get_info (struct drm_panel *panel, +struct drm_connector *connector) +{ +if (connector && panel && panel->funcs && panel->funcs->get_info) +return panel->funcs->get_info (panel, connector); + +return panel ? -ENOSYS : -EINVAL ; +} +static inline int drm_panel_get_modes (struct drm_panel *panel) {if (panel && panel->funcs && panel->funcs->get_modes)
Signature · 6 lines
1.9.1 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH 2/2 drm/i915: Add functions to execute the new sequences from VBT
From: Gaurav K Singh <[email protected]>
New sequences are added in the mipi sequence block of the VBT from version 3 onwards. The sequences are added to make the code more generic as the panel related info are placed in the VBT.
Cc David Airlie <[email protected]>
Cc Ville Syrjälä <[email protected]>
Cc Daniel Vetter <[email protected]>
Cc Jani Nikula <[email protected]>
Signed-off-by Gaurav K Singh <[email protected]>
Signed-off-by Shobhit Kumar <[email protected]>
Signed-off-by Deepak M <[email protected]>
--- drivers/gpu/drm/i915/intel_dsi.c | 8 ++++++++ drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 32 ++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+)
drivers/gpu/drm/i915/intel_dsi.c +8 -0
@@ -461,6 +461,8 @@ static void intel_dsi_enable(struct intel_encoder *encoder)intel_dsi_port_enable (encoder); } +drm_panel_backlight_on (intel_dsi->panel); +intel_panel_enable_backlight (intel_dsi->attached_connector); }
@@ -485,6 +487,8 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder)if (intel_dsi->gpio_panel)gpiod_set_value_cansleep (intel_dsi->gpio_panel,1 ); +/* Panel Enable */ +drm_panel_power_on (intel_dsi->panel);msleep (intel_dsi->panel_on_delay);if (IS_VALLEYVIEW (dev) ||IS_CHERRYVIEW (dev)) {
@@ -537,6 +541,7 @@ static void intel_dsi_pre_disable(struct intel_encoder *encoder)DRM_DEBUG_KMS ("\n" );intel_panel_disable_backlight (intel_dsi->attached_connector); +drm_panel_backlight_off (intel_dsi->panel);if (is_vid_mode (intel_dsi)) {/* Send Shutdown command to the panel in LP mode */
@@ -651,6 +656,9 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder)drm_panel_unprepare (intel_dsi->panel); +/* Disable Panel */ +drm_panel_power_off (intel_dsi->panel); +msleep (intel_dsi->panel_off_delay);msleep (intel_dsi->panel_pwr_cycle_delay);
drivers/gpu/drm/i915/intel_dsi_panel_vbt.c +32 -0
@@ -404,12 +404,44 @@ static int vbt_panel_get_modes(struct drm_panel *panel)return 1 ; } +static int vbt_panel_power_on (struct drm_panel *panel) +{ +generic_exec_sequence (panel,MIPI_SEQ_POWER_ON ); + +return 0 ; +} + +static int vbt_panel_power_off (struct drm_panel *panel) +{ +generic_exec_sequence (panel,MIPI_SEQ_POWER_OFF ); + +return 0 ; +} + +static int vbt_panel_backlight_on (struct drm_panel *panel) +{ +generic_exec_sequence (panel,MIPI_SEQ_BACKLIGHT_ON ); + +return 0 ; +} + +static int vbt_panel_backlight_off (struct drm_panel *panel) +{ +generic_exec_sequence (panel,MIPI_SEQ_BACKLIGHT_OFF ); + +return 0 ; +} +static const struct drm_panel_funcs vbt_panel_funcs = { .disable = vbt_panel_disable, .unprepare = vbt_panel_unprepare, .prepare = vbt_panel_prepare, .enable = vbt_panel_enable, .get_modes = vbt_panel_get_modes, + .power_on = vbt_panel_power_on, + .power_off = vbt_panel_power_off, + .backlight_on = vbt_panel_backlight_on, + .backlight_off = vbt_panel_backlight_off, };struct drm_panel *vbt_panel_init (struct intel_dsi *intel_dsi,u16 panel_id)
Signature · 6 lines
1.9.1 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Cc Thierry the drm panel maintainer.
Quoted 21 lines · Deepak M
Currently there are few pair of functions which are called during the panel enable/disable sequence. To improve the granularity, adding few more wrapper functions so that the functions are more specific on what they are doing.
Cc David Airlie <[email protected]>Cc Ville Syrjälä <[email protected]>Cc Daniel Vetter <[email protected]>Cc Jani Nikula <[email protected]>Signed-off-by Deepak M <[email protected]>Signed-off-by Gaurav K Singh <[email protected]>--- include/drm/drm_panel.h | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)include/drm/drm_panel.h +0
-0
include/drm/drm_panel.h struct drm_panel_funcs { · line 79
Quoted 7 lines
include/drm/drm_panel.h +4
-0@@ -73,6 +73,12 @@ struct drm_panel_funcs {int (*get_modes)(struct drm_panel *panel);int (*get_timings)(struct drm_panel *panel,unsigned int num_timings,struct display_timing *timings); +int (*power_on)(struct drm_panel *panel); +int (*power_off)(struct drm_panel *panel); +int (*backlight_on)(struct drm_panel *panel); +int (*backlight_off)(struct drm_panel *panel);
Please read the kernel-doc comment above the struct, and justify the need to add new ones. Please update the kernel-doc comment to reflect the changes.
For example, the documentation for .prepare() says, "drivers can use this to turn the panel on". What's the order of power_on and prepare (and their counterparts)? What's the division of responsibilities?
Similarly, the documentation for .enable() says, "This will typically enable the backlight". What's the order of backlight_on and prepare (and their counterparts)? What's the division of responsibilities?
include/drm/drm_panel.h struct drm_panel_funcs { · line 81
Quoted 2 lines
include/drm/drm_panel.h +2
-0+int (*get_info)(struct drm_panel *panel, +struct drm_connector *connector);
As I said in my earlier review, I don't think this is needed.
BR,
Jani.
Quoted 51 lines
include/drm/drm_panel.h +41
-0};struct drm_panel {@@ -117,6 +123,47 @@ static inline int drm_panel_enable(struct drm_panel *panel)return panel ? -ENOSYS : -EINVAL ; } +static inline int drm_panel_power_on (struct drm_panel *panel) +{ +if (panel && panel->funcs && panel->funcs->power_on) +return panel->funcs->power_on (panel); + +return panel ? -ENOSYS : -EINVAL ; +} + +static inline int drm_panel_power_off (struct drm_panel *panel) +{ +if (panel && panel->funcs && panel->funcs->power_off) +return panel->funcs->power_off (panel); + +return panel ? -ENOSYS : -EINVAL ; +} + +static inline int drm_panel_backlight_on (struct drm_panel *panel) +{ +if (panel && panel->funcs && panel->funcs->backlight_on) +return panel->funcs->backlight_on (panel); + +return panel ? -ENOSYS : -EINVAL ; +} + +static inline int drm_panel_backlight_off (struct drm_panel *panel) +{ +if (panel && panel->funcs && panel->funcs->backlight_off) +return panel->funcs->backlight_off (panel); + +return panel ? -ENOSYS : -EINVAL ; +} + +static inline int drm_panel_get_info (struct drm_panel *panel, +struct drm_connector *connector) +{ +if (connector && panel && panel->funcs && panel->funcs->get_info) +return panel->funcs->get_info (panel, connector); + +return panel ? -ENOSYS : -EINVAL ; +} +static inline int drm_panel_get_modes (struct drm_panel *panel) {if (panel && panel->funcs && panel->funcs->get_modes)
Signature · 5 lines
Jani Nikula, Intel Open Source Technology Center _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH 1/2 drm: Add few more wrapper functions for drm panel
Currently there are few pair of functions which are called during the panel enable/disable sequence. To improve the granularity, adding few more wrapper functions so that the functions are more specific on what they are doing.
Cc Thierry Reding <[email protected]>
Cc David Airlie <[email protected]>
Cc Ville Syrjälä <[email protected]>
Cc Daniel Vetter <[email protected]>
Cc Jani Nikula <[email protected]>
Signed-off-by Deepak M <[email protected]>
Signed-off-by Gaurav K Singh <[email protected]>
--- include/drm/drm_panel.h | 92 +++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 77 insertions(+), 15 deletions(-)
include/drm/drm_panel.h +77 -15
@@ -33,23 +33,33 @@ struct display_timing;/** * struct drm_panel_funcs - perform operations on a given panel -* @disable: disable panel (turn off back light, etc.) -* @unprepare: turn off panel -* @prepare: turn on panel and perform set up -* @enable: enable panel (turn on back light, etc.) +* @disable: disable panel +* @unprepare: uninitialize the panel +* @prepare: initialze the panel +* @enable: enable panel * @get_modes: add modes to the connector that the panel is attached to and * return the number of modes added * @get_timings: copy display timings into the provided array and return * the number of display timings available +* @power_on: powering on the panel +* @power_off: powering off the panel +* @backlight_on: turning on backlight +* @backlight_off: turning off backlight +* +* The .power_on function is called to turn on the panel and wait for it to +* become ready. * * The .prepare() function is typically called before the display controller -* starts to transmit video data. Panel drivers can use this to turn the panel -* on and wait for it to become ready. If additional configuration is required -* (via a control bus such as I2C, SPI or DSI for example) this is a good time -* to do that. +* starts to transmit video data. Panels will be iniatilzed during this call. +* If additional configuration is required (via a control bus such as I2C, +* SPI or DSI for example) this is a good time to do that. Some panels expects +* to wait and also to send some cmds before enabling the panel. +* +* The .enable() will typically enable the panel, some DSI panels need +* additional operations to opearte in differnt modes other than initalizing. * -* After the display controller has started transmitting video data, it's safe -* to call the .enable() function. This will typically enable the backlight to +* The .backlight_on() After the display controller has started transmitting +* video data, it's safe to turn on the panel backlight, which will * make the image on screen visible. Some panels require a certain amount of * time or frames before the image is displayed. This function is responsible * for taking this into account before enabling the backlight to avoid visual
@@ -57,13 +67,20 @@ struct display_timing;* * Before stopping video transmission from the display controller it can be * necessary to turn off the panel to avoid visual glitches. This is done in -* the .disable() function. Analogously to .enable() this typically involves -* turning off the backlight and waiting for some time to make sure no image -* is visible on the panel. It is then safe for the display controller to -* cease transmission of video data. +* the .backlight_off() function, this typically involves turning off the +* backlight and waiting for some time to make sure no image is visible +* on the panel. +* +* .disable() function will cease transmission of video data. +* +* .unprepare() function will typically uninitalize the panel. * * To save power when no video data is transmitted, a driver can power down -* the panel. This is the job of the .unprepare() function. +* the panel. This is the job of the .power_off() function. +* +* The below sequence can be followed while calling these ops +* Enable : power_on(), prepare(), enable(), backlight_on() +* Disable : backlight_off(), disable(), unprepare(), power_off() */ struct drm_panel_funcs {int (*disable)(struct drm_panel *panel);
@@ -73,6 +90,10 @@ struct drm_panel_funcs {int (*get_modes)(struct drm_panel *panel);int (*get_timings)(struct drm_panel *panel,unsigned int num_timings,struct display_timing *timings); +int (*power_on)(struct drm_panel *panel); +int (*power_off)(struct drm_panel *panel); +int (*backlight_on)(struct drm_panel *panel); +int (*backlight_off)(struct drm_panel *panel); };struct drm_panel {
@@ -117,6 +138,47 @@ static inline int drm_panel_enable(struct drm_panel *panel)return panel ? -ENOSYS : -EINVAL ; } +static inline int drm_panel_power_on (struct drm_panel *panel) +{ +if (panel && panel->funcs && panel->funcs->power_on) +return panel->funcs->power_on (panel); + +return panel ? -ENOSYS : -EINVAL ; +} + +static inline int drm_panel_power_off (struct drm_panel *panel) +{ +if (panel && panel->funcs && panel->funcs->power_off) +return panel->funcs->power_off (panel); + +return panel ? -ENOSYS : -EINVAL ; +} + +static inline int drm_panel_backlight_on (struct drm_panel *panel) +{ +if (panel && panel->funcs && panel->funcs->backlight_on) +return panel->funcs->backlight_on (panel); + +return panel ? -ENOSYS : -EINVAL ; +} + +static inline int drm_panel_backlight_off (struct drm_panel *panel) +{ +if (panel && panel->funcs && panel->funcs->backlight_off) +return panel->funcs->backlight_off (panel); + +return panel ? -ENOSYS : -EINVAL ; +} + +static inline int drm_panel_get_info (struct drm_panel *panel, +struct drm_connector *connector) +{ +if (connector && panel && panel->funcs && panel->funcs->get_info) +return panel->funcs->get_info (panel, connector); + +return panel ? -ENOSYS : -EINVAL ; +} +static inline int drm_panel_get_modes (struct drm_panel *panel) {if (panel && panel->funcs && panel->funcs->get_modes)
Signature · 6 lines
1.9.1 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 13 lines · Deepak M
Currently there are few pair of functions which are called during the panel enable/disable sequence. To improve the granularity, adding few more wrapper functions so that the functions are more specific on what they are doing.
Cc Thierry Reding <[email protected]>Cc David Airlie <[email protected]>Cc Ville Syrjälä <[email protected]>Cc Daniel Vetter <[email protected]>Cc Jani Nikula <[email protected]>Signed-off-by Deepak M <[email protected]>Signed-off-by Gaurav K Singh <[email protected]>
Quoted 3 lines
--- include/drm/drm_panel.h | 92 +++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 77 insertions(+), 15 deletions(-)
Sorry, but no. You're not giving any rationale for why you think the granularity needs to be increased. The documentation already states that panel drivers can use ->enable() and ->disable() to turn on and off the backlight, why do you need the extra callbacks? The same is true for the ->prepare() and ->unprepare() callbacks, which are described to perform what your new ->power_on() and ->power_off() callbacks would do.
Increasing the granularity isn't always a good thing. It means that drivers can now call the functions in many more variations.
If you think you really need finer granularity, you need to do a better job of explaining why. Also, Cc'ing me on the second patch, which I do not have in any of my inboxes, and which I assume contains a usage example of these new callbacks, might help me understand the need for this.
One more comment below...
Quoted 1 line
include/drm/drm_panel.h +0
-0
[...]
Quoted 11 lines
struct
drm_panel_funcs{include/drm/drm_panel.h +4
-0int (*disable)(struct drm_panel *panel);@@ -73,6 +90,10 @@ struct drm_panel_funcs {int (*get_modes)(struct drm_panel *panel);int (*get_timings)(struct drm_panel *panel,unsigned int num_timings,struct display_timing *timings); +int (*power_on)(struct drm_panel *panel); +int (*power_off)(struct drm_panel *panel); +int (*backlight_on)(struct drm_panel *panel); +int (*backlight_off)(struct drm_panel *panel); };
[...]
include/drm/drm_panel.h static inline int drm_panel_enable(struct drm_panel *panel) · line 180
Quoted 8 lines
include/drm/drm_panel.h +8
-0+static inline int drm_panel_get_info (struct drm_panel *panel, +struct drm_connector *connector) +{ +if (connector && panel && panel->funcs && panel->funcs->get_info) +return panel->funcs->get_info (panel, connector); + +return panel ? -ENOSYS : -EINVAL ; +}
This callback no longer exists, so this patch is going to break compilation.
Thierry
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel