[PATCH 1/2] drm: Add few more wrapper functions for drm panel

List overview All Threads
Download

newer

older

[PATCH] drm/i915: Resume DP MST...

[PATCH 3/3] drm: panel-simple:...

Deepak M

29 Feb 2016 29 Feb '16

11:45 a.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 airlied@linux.ie Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Daniel Vetter daniel.vetter@intel.com Cc: Jani Nikula jani.nikula@intel.com Signed-off-by: Deepak M m.deepak@intel.com Signed-off-by: Gaurav K Singh gaurav.k.singh@intel.com --- include/drm/drm_panel.h | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)

diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h index 13ff44b..c729f6d 100644 --- a/include/drm/drm_panel.h +++ b/include/drm/drm_panel.h @@ -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)

-- 1.9.1

Show replies by date

Deepak M

29 Feb 29 Feb

11:45 a.m.

New subject: [PATCH 2/2] drm/i915: Add functions to execute the new sequences from VBT

From: Gaurav K Singh gaurav.k.singh@intel.com

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 airlied@linux.ie Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Daniel Vetter daniel.vetter@intel.com Cc: Jani Nikula jani.nikula@intel.com Signed-off-by: Gaurav K Singh gaurav.k.singh@intel.com Signed-off-by: Shobhit Kumar shobhit.kumar@intel.com Signed-off-by: Deepak M m.deepak@intel.com --- drivers/gpu/drm/i915/intel_dsi.c | 8 ++++++++ drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 32 ++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c index b928c50..82f6822 100644 --- a/drivers/gpu/drm/i915/intel_dsi.c +++ b/drivers/gpu/drm/i915/intel_dsi.c @@ -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);

diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c index 787f01c..01a2743 100644 --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c @@ -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)

-- 1.9.1

Jani Nikula

12:12 p.m.

Cc: Thierry the drm panel maintainer.

On Mon, 29 Feb 2016, Deepak M m.deepak@intel.com wrote:

...

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 airlied@linux.ie Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Daniel Vetter daniel.vetter@intel.com Cc: Jani Nikula jani.nikula@intel.com Signed-off-by: Deepak M m.deepak@intel.com Signed-off-by: Gaurav K Singh gaurav.k.singh@intel.com

include/drm/drm_panel.h | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)

diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h index 13ff44b..c729f6d 100644 --- a/include/drm/drm_panel.h +++ b/include/drm/drm_panel.h @@ -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?

...

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.

...

};

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)

-- Jani Nikula, Intel Open Source Technology Center

Deepak M

2 Mar 2 Mar

12:58 p.m.

Cc: Thierry Reding thierry.reding@gmail.com Cc: David Airlie airlied@linux.ie Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Daniel Vetter daniel.vetter@intel.com Cc: Jani Nikula jani.nikula@intel.com Signed-off-by: Deepak M m.deepak@intel.com Signed-off-by: Gaurav K Singh gaurav.k.singh@intel.com --- include/drm/drm_panel.h | 92 +++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 77 insertions(+), 15 deletions(-)

diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h index 13ff44b..dadbcea 100644 --- a/include/drm/drm_panel.h +++ b/include/drm/drm_panel.h @@ -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; }

-- 1.9.1

Thierry Reding

3:25 p.m.

On Wed, Mar 02, 2016 at 06:28:31PM +0530, Deepak M wrote:

...

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 thierry.reding@gmail.com Cc: David Airlie airlied@linux.ie Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Daniel Vetter daniel.vetter@intel.com Cc: Jani Nikula jani.nikula@intel.com Signed-off-by: Deepak M m.deepak@intel.com Signed-off-by: Gaurav K Singh gaurav.k.singh@intel.com

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

...

diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h

[...]

...

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);

};

[...]

...

+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

3338

Age (days ago)

3340

Last active (days ago)

dri-devel@lists.freedesktop.org

4 comments

3 participants

tags (0)

participants (3)

Deepak M
Jani Nikula
Thierry Reding