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

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

[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: 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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Deepak M]

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: 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_panel_vbt.c | 48 ++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
index c6e18fe..db8e210 100644
--- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
+++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
@@ -1035,12 +1035,60 @@ 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 int vbt_panel_get_info(struct drm_panel *panel,
+					struct drm_connector *connector)
+{
+	struct intel_connector *intel_connector =
+				to_intel_connector(connector);
+
+	if (intel_connector) {
+		connector->display_info.width_mm =
+				intel_connector->panel.fixed_mode->width_mm;
+		connector->display_info.height_mm =
+				intel_connector->panel.fixed_mode->height_mm;
+	}
+	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,
+	.get_info = vbt_panel_get_info,
 };
 
 struct drm_panel *vbt_panel_init(struct intel_dsi *intel_dsi, u16 panel_id)
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for series starting with [1/2] drm: Add few more wrapper functions for drm panel

[Patchwork]

== Summary ==

Series 3624v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/3624/revisions/1/mbox/

Test gem_cs_prefetch:
        Subgroup basic-default:
                incomplete -> PASS       (ilk-hp8440p)
Test gem_ctx_param_basic:
        Subgroup basic-default:
                incomplete -> PASS       (snb-x220t)
        Subgroup invalid-ctx-get:
                incomplete -> PASS       (snb-x220t)
        Subgroup non-root-set-no-zeromap:
                incomplete -> PASS       (snb-x220t)
        Subgroup root-set:
                incomplete -> PASS       (snb-x220t)
        Subgroup root-set-no-zeromap-disabled:
                incomplete -> PASS       (snb-x220t)
        Subgroup root-set-no-zeromap-enabled:
                incomplete -> PASS       (snb-x220t)
Test gem_exec_basic:
        Subgroup basic-blt:
                incomplete -> PASS       (snb-x220t)
Test gem_flink_basic:
        Subgroup bad-flink:
                incomplete -> PASS       (snb-x220t)
Test gem_linear_blits:
        Subgroup basic:
                incomplete -> PASS       (snb-x220t)
Test gem_mmap:
        Subgroup basic:
                incomplete -> PASS       (snb-x220t)
        Subgroup basic-small-bo:
                incomplete -> PASS       (snb-x220t)
Test gem_mmap_gtt:
        Subgroup basic:
                incomplete -> PASS       (snb-x220t)
        Subgroup basic-copy:
                incomplete -> PASS       (snb-x220t)
        Subgroup basic-read-no-prefault:
                incomplete -> PASS       (snb-x220t)
        Subgroup basic-read-write-distinct:
                incomplete -> PASS       (snb-x220t)
        Subgroup basic-write-cpu-read-gtt:
                incomplete -> PASS       (snb-x220t)
        Subgroup basic-write-read-distinct:
                incomplete -> PASS       (snb-x220t)
Test gem_pread:
        Subgroup basic:
                incomplete -> PASS       (snb-x220t)
Test gem_render_linear_blits:
        Subgroup basic:
                incomplete -> PASS       (snb-x220t)
Test gem_render_tiled_blits:
        Subgroup basic:
                incomplete -> PASS       (snb-x220t)
Test gem_ringfill:
        Subgroup basic-default:
                incomplete -> PASS       (snb-x220t)
        Subgroup basic-default-bomb:
                incomplete -> PASS       (ivb-t430s)
        Subgroup basic-default-forked:
                incomplete -> PASS       (snb-x220t)
        Subgroup basic-default-hang:
                incomplete -> PASS       (snb-x220t)
Test gem_storedw_loop:
        Subgroup basic-bsd1:
                incomplete -> SKIP       (snb-x220t)
        Subgroup basic-render:
                incomplete -> PASS       (snb-x220t)
        Subgroup basic-vebox:
                incomplete -> SKIP       (snb-x220t)
Test gem_sync:
        Subgroup basic-blt:
                incomplete -> PASS       (snb-x220t)
Test gem_tiled_pread_basic:
                incomplete -> PASS       (snb-x220t)
Test kms_addfb_basic:
        Subgroup addfb25-modifier-no-flag:
                incomplete -> PASS       (snb-x220t)
        Subgroup bad-pitch-0:
                incomplete -> PASS       (snb-x220t)
        Subgroup bad-pitch-1024:
                incomplete -> PASS       (snb-x220t)
        Subgroup bad-pitch-128:
                incomplete -> PASS       (snb-x220t)
        Subgroup bad-pitch-256:
                incomplete -> PASS       (snb-x220t)
        Subgroup bad-pitch-999:
                incomplete -> PASS       (snb-x220t)
        Subgroup basic-x-tiled:
                incomplete -> PASS       (snb-x220t)
        Subgroup framebuffer-vs-set-tiling:
                incomplete -> PASS       (snb-x220t)
        Subgroup no-handle:
                incomplete -> PASS       (snb-x220t)
        Subgroup size-max:
                incomplete -> PASS       (snb-x220t)
        Subgroup too-wide:
                incomplete -> PASS       (snb-x220t)
        Subgroup unused-handle:
                incomplete -> PASS       (snb-x220t)
        Subgroup unused-modifier:
                incomplete -> PASS       (snb-x220t)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                incomplete -> PASS       (snb-x220t)
Test kms_force_connector_basic:
        Subgroup force-load-detect:
                dmesg-fail -> FAIL       (snb-dellxps)
                fail       -> SKIP       (ilk-hp8440p)
        Subgroup prune-stale-modes:
                skip       -> PASS       (snb-x220t)
                pass       -> SKIP       (ilk-hp8440p)
Test kms_pipe_crc_basic:
        Subgroup nonblocking-crc-pipe-c-frame-sequence:
                incomplete -> SKIP       (snb-x220t)
        Subgroup read-crc-pipe-a-frame-sequence:
                incomplete -> PASS       (snb-x220t)
        Subgroup read-crc-pipe-b:
                incomplete -> PASS       (snb-x220t)
        Subgroup read-crc-pipe-c-frame-sequence:
                incomplete -> SKIP       (snb-x220t)
        Subgroup suspend-read-crc-pipe-a:
                incomplete -> PASS       (snb-x220t)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                incomplete -> FAIL       (snb-x220t)
                pass       -> DMESG-WARN (bsw-nuc-2)
        Subgroup basic-rte:
                dmesg-warn -> PASS       (bsw-nuc-2)
Test prime_self_import:
        Subgroup basic-with_two_bos:
                incomplete -> PASS       (snb-x220t)

bdw-nuci7        total:164  pass:153  dwarn:0   dfail:0   fail:0   skip:11 
bdw-ultra        total:167  pass:153  dwarn:0   dfail:0   fail:0   skip:14 
bsw-nuc-2        total:167  pass:136  dwarn:1   dfail:0   fail:0   skip:30 
byt-nuc          total:167  pass:141  dwarn:1   dfail:0   fail:0   skip:25 
hsw-gt2          total:167  pass:156  dwarn:0   dfail:1   fail:0   skip:10 
ilk-hp8440p      total:167  pass:117  dwarn:0   dfail:0   fail:0   skip:50 
ivb-t430s        total:167  pass:152  dwarn:0   dfail:0   fail:1   skip:14 
skl-i5k-2        total:167  pass:150  dwarn:1   dfail:0   fail:0   skip:16 
snb-dellxps      total:167  pass:144  dwarn:0   dfail:0   fail:1   skip:22 
snb-x220t        total:167  pass:144  dwarn:0   dfail:0   fail:2   skip:21 

Results at /archive/results/CI_IGT_test/Patchwork_1444/

e4599905334de9349501a383afb8503a1dde5728 drm-intel-nightly: 2016y-02m-18d-17h-13m-22s UTC integration manifest
5db201fbca7602784c9016292768fd03aced8d98 drm/i915: Add functions to execute the new sequences from VBT
1943085a8fd4900aac695f9c3bda502c19f3a639 drm: Add few more wrapper functions for drm panel

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

On Fri, 19 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.

I want to see where all these new drm_panel_* functions would be called
in intel_dsi.c.

Also, this patch touches drm core, not i915. You need to send this kind
of stuff to dri-devel@lists.freedesktop.org to get them merged,
preferably Cc'ing the relevant maintainers (try
scripts/get_maintainer.pl). But for that, I think you need to have the
use case and the in-kernel user as well, so please send the intel_dsi.c
patch too.

BR,
Jani.

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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

On Fri, 19 Feb 2016, Deepak M <m.deepak@intel.com> wrote:
> 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: 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_panel_vbt.c | 48 ++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> index c6e18fe..db8e210 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> @@ -1035,12 +1035,60 @@ 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 int vbt_panel_get_info(struct drm_panel *panel,
> +					struct drm_connector *connector)
> +{
> +	struct intel_connector *intel_connector =
> +				to_intel_connector(connector);
> +
> +	if (intel_connector) {
> +		connector->display_info.width_mm =
> +				intel_connector->panel.fixed_mode->width_mm;
> +		connector->display_info.height_mm =
> +				intel_connector->panel.fixed_mode->height_mm;
> +	}
> +	return 0;

I think we could do this part in the ->get_modes hook. For all the other
displays, it's the ->get_modes hook that reads the EDID, and ultimately
sets the display_info from EDID. We wouldn't need a new hook at all.

Also, this should be a separate change. If we can get the size
information from the fixed mode from VBT, we should do that in
->get_modes, and backport this fix for stable kernels. It's sorely
needed for BYT/CHV too.


BR,
Jani.


> +}
> +
>  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,
> +	.get_info = vbt_panel_get_info,
>  };
>  
>  struct drm_panel *vbt_panel_init(struct intel_dsi *intel_dsi, u16 panel_id)

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Jani Nikula]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[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 <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;
 }
 
+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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2632 bytes --]

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel