diff mbox

[RFC,05/12] drm/i915/dsi: remove unnecessary dsi device callbacks

Message ID beed6026438e327e2ea03ad2756000145518a89c.1421410274.git.jani.nikula@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jani Nikula Jan. 16, 2015, 12:27 p.m. UTC
Remove all the trivial and/or dummy callbacks from intel dsi device
ops. Merge send_otp_cmds into panel_reset as they're called back to
back.

This will be helpful for switching to use drm_panel for the
callbacks. If we ever need the additional callbacks, we should add them
to drm_panel funcs.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dsi.c           | 32 ++-------------------
 drivers/gpu/drm/i915/intel_dsi.h           | 20 -------------
 drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 45 ++----------------------------
 3 files changed, 5 insertions(+), 92 deletions(-)

Comments

Shobhit Kumar Jan. 22, 2015, 11:23 a.m. UTC | #1
On 01/16/2015 05:57 PM, Jani Nikula wrote:
> Remove all the trivial and/or dummy callbacks from intel dsi device
> ops. Merge send_otp_cmds into panel_reset as they're called back to
> back.
>
> This will be helpful for switching to use drm_panel for the
> callbacks. If we ever need the additional callbacks, we should add them
> to drm_panel funcs.
>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_dsi.c           | 32 ++-------------------
>   drivers/gpu/drm/i915/intel_dsi.h           | 20 -------------
>   drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 45 ++----------------------------
>   3 files changed, 5 insertions(+), 92 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 9b0eaa9db845..fc218b7754b3 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -70,12 +70,6 @@ static void band_gap_reset(struct drm_i915_private *dev_priv)
>   	mutex_unlock(&dev_priv->dpio_lock);
>   }
>
> -static struct intel_dsi *intel_attached_dsi(struct drm_connector *connector)
> -{
> -	return container_of(intel_attached_encoder(connector),
> -			    struct intel_dsi, base);
> -}
> -
>   static inline bool is_vid_mode(struct intel_dsi *intel_dsi)
>   {
>   	return intel_dsi->operation_mode == INTEL_DSI_VIDEO_MODE;
> @@ -99,7 +93,6 @@ static bool intel_dsi_compute_config(struct intel_encoder *encoder,
>   	struct intel_connector *intel_connector = intel_dsi->attached_connector;
>   	struct drm_display_mode *fixed_mode = intel_connector->panel.fixed_mode;
>   	struct drm_display_mode *adjusted_mode = &config->adjusted_mode;
> -	struct drm_display_mode *mode = &config->requested_mode;
>
>   	DRM_DEBUG_KMS("\n");
>
> @@ -109,10 +102,6 @@ static bool intel_dsi_compute_config(struct intel_encoder *encoder,
>   	/* DSI uses short packets for sync events, so clear mode flags for DSI */
>   	adjusted_mode->flags = 0;
>
> -	if (intel_dsi->dev.dev_ops->mode_fixup)
> -		return intel_dsi->dev.dev_ops->mode_fixup(&intel_dsi->dev,
> -							  mode, adjusted_mode);
> -

There had been a instance where we had to drive different resolution 
(lower) than the native one. Also in VBT there is a field to make this 
generic at least from driver perspective to give the needed target 
resolution. In case target resolution is same as native, nothing gets 
changed, else mode_fixup function adjusts the mode accordingly keeping 
timing as same and enabling scalar. Might not be useful in general, but 
did find a use internally.

Either way

Reviewed-By: Shobhit Kumar <shobhit.kumar@intel.com>

>   	return true;
>   }
>
> @@ -269,9 +258,6 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder)
>   	if (intel_dsi->dev.dev_ops->panel_reset)
>   		intel_dsi->dev.dev_ops->panel_reset(&intel_dsi->dev);
>
> -	if (intel_dsi->dev.dev_ops->send_otp_cmds)
> -		intel_dsi->dev.dev_ops->send_otp_cmds(&intel_dsi->dev);
> -
>   	for_each_dsi_port(port, intel_dsi->ports)
>   		wait_for_dsi_fifo_empty(intel_dsi, port);
>
> @@ -484,7 +470,6 @@ intel_dsi_mode_valid(struct drm_connector *connector,
>   {
>   	struct intel_connector *intel_connector = to_intel_connector(connector);
>   	struct drm_display_mode *fixed_mode = intel_connector->panel.fixed_mode;
> -	struct intel_dsi *intel_dsi = intel_attached_dsi(connector);
>
>   	DRM_DEBUG_KMS("\n");
>
> @@ -500,7 +485,7 @@ intel_dsi_mode_valid(struct drm_connector *connector,
>   			return MODE_PANEL;
>   	}
>
> -	return intel_dsi->dev.dev_ops->mode_valid(&intel_dsi->dev, mode);
> +	return MODE_OK;
>   }
>
>   /* return txclkesc cycles in terms of divider and duration in us */
> @@ -749,20 +734,7 @@ static void intel_dsi_pre_pll_enable(struct intel_encoder *encoder)
>   static enum drm_connector_status
>   intel_dsi_detect(struct drm_connector *connector, bool force)
>   {
> -	struct intel_dsi *intel_dsi = intel_attached_dsi(connector);
> -	struct intel_encoder *intel_encoder = &intel_dsi->base;
> -	enum intel_display_power_domain power_domain;
> -	enum drm_connector_status connector_status;
> -	struct drm_i915_private *dev_priv = intel_encoder->base.dev->dev_private;
> -
> -	DRM_DEBUG_KMS("\n");
> -	power_domain = intel_display_port_power_domain(intel_encoder);
> -
> -	intel_display_power_get(dev_priv, power_domain);
> -	connector_status = intel_dsi->dev.dev_ops->detect(&intel_dsi->dev);
> -	intel_display_power_put(dev_priv, power_domain);
> -
> -	return connector_status;
> +	return connector_status_connected;
>   }
>
>   static int intel_dsi_get_modes(struct drm_connector *connector)
> diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
> index 2bb8c46c7889..22f87036a256 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.h
> +++ b/drivers/gpu/drm/i915/intel_dsi.h
> @@ -47,33 +47,13 @@ struct intel_dsi_dev_ops {
>
>   	void (*disable_panel_power)(struct intel_dsi_device *dsi);
>
> -	/* one time programmable commands if needed */
> -	void (*send_otp_cmds)(struct intel_dsi_device *dsi);
> -
>   	/* This callback must be able to assume DSI commands can be sent */
>   	void (*enable)(struct intel_dsi_device *dsi);
>
>   	/* This callback must be able to assume DSI commands can be sent */
>   	void (*disable)(struct intel_dsi_device *dsi);
>
> -	int (*mode_valid)(struct intel_dsi_device *dsi,
> -			  struct drm_display_mode *mode);
> -
> -	bool (*mode_fixup)(struct intel_dsi_device *dsi,
> -			   const struct drm_display_mode *mode,
> -			   struct drm_display_mode *adjusted_mode);
> -
> -	void (*mode_set)(struct intel_dsi_device *dsi,
> -			 struct drm_display_mode *mode,
> -			 struct drm_display_mode *adjusted_mode);
> -
> -	enum drm_connector_status (*detect)(struct intel_dsi_device *dsi);
> -
> -	bool (*get_hw_state)(struct intel_dsi_device *dev);
> -
>   	struct drm_display_mode *(*get_modes)(struct intel_dsi_device *dsi);
> -
> -	void (*destroy) (struct intel_dsi_device *dsi);
>   };
>
>   struct intel_dsi {
> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> index 5493aef5a6a3..b0e7327a485f 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> @@ -559,18 +559,6 @@ static bool generic_init(struct intel_dsi_device *dsi)
>   	return true;
>   }
>
> -static int generic_mode_valid(struct intel_dsi_device *dsi,
> -		   struct drm_display_mode *mode)
> -{
> -	return MODE_OK;
> -}
> -
> -static bool generic_mode_fixup(struct intel_dsi_device *dsi,
> -		    const struct drm_display_mode *mode,
> -		    struct drm_display_mode *adjusted_mode) {
> -	return true;
> -}
> -
>   static void generic_panel_reset(struct intel_dsi_device *dsi)
>   {
>   	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
> @@ -580,26 +568,18 @@ static void generic_panel_reset(struct intel_dsi_device *dsi)
>   	char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_ASSERT_RESET];
>
>   	generic_exec_sequence(intel_dsi, sequence);
> -}
> -
> -static void generic_disable_panel_power(struct intel_dsi_device *dsi)
> -{
> -	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
> -	struct drm_device *dev = intel_dsi->base.base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -
> -	char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_DEASSERT_RESET];
>
> +	sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_INIT_OTP];
>   	generic_exec_sequence(intel_dsi, sequence);
>   }
>
> -static void generic_send_otp_cmds(struct intel_dsi_device *dsi)
> +static void generic_disable_panel_power(struct intel_dsi_device *dsi)
>   {
>   	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
>   	struct drm_device *dev = intel_dsi->base.base.dev;
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>
> -	char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_INIT_OTP];
> +	char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_DEASSERT_RESET];
>
>   	generic_exec_sequence(intel_dsi, sequence);
>   }
> @@ -626,16 +606,6 @@ static void generic_disable(struct intel_dsi_device *dsi)
>   	generic_exec_sequence(intel_dsi, sequence);
>   }
>
> -static enum drm_connector_status generic_detect(struct intel_dsi_device *dsi)
> -{
> -	return connector_status_connected;
> -}
> -
> -static bool generic_get_hw_state(struct intel_dsi_device *dev)
> -{
> -	return true;
> -}
> -
>   static struct drm_display_mode *generic_get_modes(struct intel_dsi_device *dsi)
>   {
>   	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
> @@ -646,20 +616,11 @@ static struct drm_display_mode *generic_get_modes(struct intel_dsi_device *dsi)
>   	return dev_priv->vbt.lfp_lvds_vbt_mode;
>   }
>
> -static void generic_destroy(struct intel_dsi_device *dsi) { }
> -
> -/* Callbacks. We might not need them all. */
>   struct intel_dsi_dev_ops vbt_generic_dsi_display_ops = {
>   	.init = generic_init,
> -	.mode_valid = generic_mode_valid,
> -	.mode_fixup = generic_mode_fixup,
>   	.panel_reset = generic_panel_reset,
>   	.disable_panel_power = generic_disable_panel_power,
> -	.send_otp_cmds = generic_send_otp_cmds,
>   	.enable = generic_enable,
>   	.disable = generic_disable,
> -	.detect = generic_detect,
> -	.get_hw_state = generic_get_hw_state,
>   	.get_modes = generic_get_modes,
> -	.destroy = generic_destroy,
>   };
>

Regards
Shobhit
Jani Nikula Jan. 22, 2015, 1:23 p.m. UTC | #2
On Thu, 22 Jan 2015, Shobhit Kumar <shobhit.kumar@linux.intel.com> wrote:
> On 01/16/2015 05:57 PM, Jani Nikula wrote:
>> Remove all the trivial and/or dummy callbacks from intel dsi device
>> ops. Merge send_otp_cmds into panel_reset as they're called back to
>> back.
>>
>> This will be helpful for switching to use drm_panel for the
>> callbacks. If we ever need the additional callbacks, we should add them
>> to drm_panel funcs.
>>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dsi.c           | 32 ++-------------------
>>   drivers/gpu/drm/i915/intel_dsi.h           | 20 -------------
>>   drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 45 ++----------------------------
>>   3 files changed, 5 insertions(+), 92 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
>> index 9b0eaa9db845..fc218b7754b3 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>> @@ -70,12 +70,6 @@ static void band_gap_reset(struct drm_i915_private *dev_priv)
>>   	mutex_unlock(&dev_priv->dpio_lock);
>>   }
>>
>> -static struct intel_dsi *intel_attached_dsi(struct drm_connector *connector)
>> -{
>> -	return container_of(intel_attached_encoder(connector),
>> -			    struct intel_dsi, base);
>> -}
>> -
>>   static inline bool is_vid_mode(struct intel_dsi *intel_dsi)
>>   {
>>   	return intel_dsi->operation_mode == INTEL_DSI_VIDEO_MODE;
>> @@ -99,7 +93,6 @@ static bool intel_dsi_compute_config(struct intel_encoder *encoder,
>>   	struct intel_connector *intel_connector = intel_dsi->attached_connector;
>>   	struct drm_display_mode *fixed_mode = intel_connector->panel.fixed_mode;
>>   	struct drm_display_mode *adjusted_mode = &config->adjusted_mode;
>> -	struct drm_display_mode *mode = &config->requested_mode;
>>
>>   	DRM_DEBUG_KMS("\n");
>>
>> @@ -109,10 +102,6 @@ static bool intel_dsi_compute_config(struct intel_encoder *encoder,
>>   	/* DSI uses short packets for sync events, so clear mode flags for DSI */
>>   	adjusted_mode->flags = 0;
>>
>> -	if (intel_dsi->dev.dev_ops->mode_fixup)
>> -		return intel_dsi->dev.dev_ops->mode_fixup(&intel_dsi->dev,
>> -							  mode, adjusted_mode);
>> -
>
> There had been a instance where we had to drive different resolution 
> (lower) than the native one. Also in VBT there is a field to make this 
> generic at least from driver perspective to give the needed target 
> resolution. In case target resolution is same as native, nothing gets 
> changed, else mode_fixup function adjusts the mode accordingly keeping 
> timing as same and enabling scalar. Might not be useful in general, but 
> did find a use internally.

Can we just have the driver return the desired mode from .get_modes in
that case?

BR,
Jani.


>
> Either way
>
> Reviewed-By: Shobhit Kumar <shobhit.kumar@intel.com>
>
>>   	return true;
>>   }
>>
>> @@ -269,9 +258,6 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder)
>>   	if (intel_dsi->dev.dev_ops->panel_reset)
>>   		intel_dsi->dev.dev_ops->panel_reset(&intel_dsi->dev);
>>
>> -	if (intel_dsi->dev.dev_ops->send_otp_cmds)
>> -		intel_dsi->dev.dev_ops->send_otp_cmds(&intel_dsi->dev);
>> -
>>   	for_each_dsi_port(port, intel_dsi->ports)
>>   		wait_for_dsi_fifo_empty(intel_dsi, port);
>>
>> @@ -484,7 +470,6 @@ intel_dsi_mode_valid(struct drm_connector *connector,
>>   {
>>   	struct intel_connector *intel_connector = to_intel_connector(connector);
>>   	struct drm_display_mode *fixed_mode = intel_connector->panel.fixed_mode;
>> -	struct intel_dsi *intel_dsi = intel_attached_dsi(connector);
>>
>>   	DRM_DEBUG_KMS("\n");
>>
>> @@ -500,7 +485,7 @@ intel_dsi_mode_valid(struct drm_connector *connector,
>>   			return MODE_PANEL;
>>   	}
>>
>> -	return intel_dsi->dev.dev_ops->mode_valid(&intel_dsi->dev, mode);
>> +	return MODE_OK;
>>   }
>>
>>   /* return txclkesc cycles in terms of divider and duration in us */
>> @@ -749,20 +734,7 @@ static void intel_dsi_pre_pll_enable(struct intel_encoder *encoder)
>>   static enum drm_connector_status
>>   intel_dsi_detect(struct drm_connector *connector, bool force)
>>   {
>> -	struct intel_dsi *intel_dsi = intel_attached_dsi(connector);
>> -	struct intel_encoder *intel_encoder = &intel_dsi->base;
>> -	enum intel_display_power_domain power_domain;
>> -	enum drm_connector_status connector_status;
>> -	struct drm_i915_private *dev_priv = intel_encoder->base.dev->dev_private;
>> -
>> -	DRM_DEBUG_KMS("\n");
>> -	power_domain = intel_display_port_power_domain(intel_encoder);
>> -
>> -	intel_display_power_get(dev_priv, power_domain);
>> -	connector_status = intel_dsi->dev.dev_ops->detect(&intel_dsi->dev);
>> -	intel_display_power_put(dev_priv, power_domain);
>> -
>> -	return connector_status;
>> +	return connector_status_connected;
>>   }
>>
>>   static int intel_dsi_get_modes(struct drm_connector *connector)
>> diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
>> index 2bb8c46c7889..22f87036a256 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi.h
>> +++ b/drivers/gpu/drm/i915/intel_dsi.h
>> @@ -47,33 +47,13 @@ struct intel_dsi_dev_ops {
>>
>>   	void (*disable_panel_power)(struct intel_dsi_device *dsi);
>>
>> -	/* one time programmable commands if needed */
>> -	void (*send_otp_cmds)(struct intel_dsi_device *dsi);
>> -
>>   	/* This callback must be able to assume DSI commands can be sent */
>>   	void (*enable)(struct intel_dsi_device *dsi);
>>
>>   	/* This callback must be able to assume DSI commands can be sent */
>>   	void (*disable)(struct intel_dsi_device *dsi);
>>
>> -	int (*mode_valid)(struct intel_dsi_device *dsi,
>> -			  struct drm_display_mode *mode);
>> -
>> -	bool (*mode_fixup)(struct intel_dsi_device *dsi,
>> -			   const struct drm_display_mode *mode,
>> -			   struct drm_display_mode *adjusted_mode);
>> -
>> -	void (*mode_set)(struct intel_dsi_device *dsi,
>> -			 struct drm_display_mode *mode,
>> -			 struct drm_display_mode *adjusted_mode);
>> -
>> -	enum drm_connector_status (*detect)(struct intel_dsi_device *dsi);
>> -
>> -	bool (*get_hw_state)(struct intel_dsi_device *dev);
>> -
>>   	struct drm_display_mode *(*get_modes)(struct intel_dsi_device *dsi);
>> -
>> -	void (*destroy) (struct intel_dsi_device *dsi);
>>   };
>>
>>   struct intel_dsi {
>> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> index 5493aef5a6a3..b0e7327a485f 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> @@ -559,18 +559,6 @@ static bool generic_init(struct intel_dsi_device *dsi)
>>   	return true;
>>   }
>>
>> -static int generic_mode_valid(struct intel_dsi_device *dsi,
>> -		   struct drm_display_mode *mode)
>> -{
>> -	return MODE_OK;
>> -}
>> -
>> -static bool generic_mode_fixup(struct intel_dsi_device *dsi,
>> -		    const struct drm_display_mode *mode,
>> -		    struct drm_display_mode *adjusted_mode) {
>> -	return true;
>> -}
>> -
>>   static void generic_panel_reset(struct intel_dsi_device *dsi)
>>   {
>>   	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
>> @@ -580,26 +568,18 @@ static void generic_panel_reset(struct intel_dsi_device *dsi)
>>   	char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_ASSERT_RESET];
>>
>>   	generic_exec_sequence(intel_dsi, sequence);
>> -}
>> -
>> -static void generic_disable_panel_power(struct intel_dsi_device *dsi)
>> -{
>> -	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
>> -	struct drm_device *dev = intel_dsi->base.base.dev;
>> -	struct drm_i915_private *dev_priv = dev->dev_private;
>> -
>> -	char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_DEASSERT_RESET];
>>
>> +	sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_INIT_OTP];
>>   	generic_exec_sequence(intel_dsi, sequence);
>>   }
>>
>> -static void generic_send_otp_cmds(struct intel_dsi_device *dsi)
>> +static void generic_disable_panel_power(struct intel_dsi_device *dsi)
>>   {
>>   	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
>>   	struct drm_device *dev = intel_dsi->base.base.dev;
>>   	struct drm_i915_private *dev_priv = dev->dev_private;
>>
>> -	char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_INIT_OTP];
>> +	char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_DEASSERT_RESET];
>>
>>   	generic_exec_sequence(intel_dsi, sequence);
>>   }
>> @@ -626,16 +606,6 @@ static void generic_disable(struct intel_dsi_device *dsi)
>>   	generic_exec_sequence(intel_dsi, sequence);
>>   }
>>
>> -static enum drm_connector_status generic_detect(struct intel_dsi_device *dsi)
>> -{
>> -	return connector_status_connected;
>> -}
>> -
>> -static bool generic_get_hw_state(struct intel_dsi_device *dev)
>> -{
>> -	return true;
>> -}
>> -
>>   static struct drm_display_mode *generic_get_modes(struct intel_dsi_device *dsi)
>>   {
>>   	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
>> @@ -646,20 +616,11 @@ static struct drm_display_mode *generic_get_modes(struct intel_dsi_device *dsi)
>>   	return dev_priv->vbt.lfp_lvds_vbt_mode;
>>   }
>>
>> -static void generic_destroy(struct intel_dsi_device *dsi) { }
>> -
>> -/* Callbacks. We might not need them all. */
>>   struct intel_dsi_dev_ops vbt_generic_dsi_display_ops = {
>>   	.init = generic_init,
>> -	.mode_valid = generic_mode_valid,
>> -	.mode_fixup = generic_mode_fixup,
>>   	.panel_reset = generic_panel_reset,
>>   	.disable_panel_power = generic_disable_panel_power,
>> -	.send_otp_cmds = generic_send_otp_cmds,
>>   	.enable = generic_enable,
>>   	.disable = generic_disable,
>> -	.detect = generic_detect,
>> -	.get_hw_state = generic_get_hw_state,
>>   	.get_modes = generic_get_modes,
>> -	.destroy = generic_destroy,
>>   };
>>
>
> Regards
> Shobhit
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Shobhit Kumar Jan. 23, 2015, 9:44 a.m. UTC | #3
On 01/22/2015 06:53 PM, Jani Nikula wrote:
> On Thu, 22 Jan 2015, Shobhit Kumar <shobhit.kumar@linux.intel.com> wrote:
>> On 01/16/2015 05:57 PM, Jani Nikula wrote:
>>> Remove all the trivial and/or dummy callbacks from intel dsi device
>>> ops. Merge send_otp_cmds into panel_reset as they're called back to
>>> back.
>>>
>>> This will be helpful for switching to use drm_panel for the
>>> callbacks. If we ever need the additional callbacks, we should add them
>>> to drm_panel funcs.
>>>
>>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/intel_dsi.c           | 32 ++-------------------
>>>    drivers/gpu/drm/i915/intel_dsi.h           | 20 -------------
>>>    drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 45 ++----------------------------
>>>    3 files changed, 5 insertions(+), 92 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
>>> index 9b0eaa9db845..fc218b7754b3 100644
>>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>>> @@ -70,12 +70,6 @@ static void band_gap_reset(struct drm_i915_private *dev_priv)
>>>    	mutex_unlock(&dev_priv->dpio_lock);
>>>    }
>>>
>>> -static struct intel_dsi *intel_attached_dsi(struct drm_connector *connector)
>>> -{
>>> -	return container_of(intel_attached_encoder(connector),
>>> -			    struct intel_dsi, base);
>>> -}
>>> -
>>>    static inline bool is_vid_mode(struct intel_dsi *intel_dsi)
>>>    {
>>>    	return intel_dsi->operation_mode == INTEL_DSI_VIDEO_MODE;
>>> @@ -99,7 +93,6 @@ static bool intel_dsi_compute_config(struct intel_encoder *encoder,
>>>    	struct intel_connector *intel_connector = intel_dsi->attached_connector;
>>>    	struct drm_display_mode *fixed_mode = intel_connector->panel.fixed_mode;
>>>    	struct drm_display_mode *adjusted_mode = &config->adjusted_mode;
>>> -	struct drm_display_mode *mode = &config->requested_mode;
>>>
>>>    	DRM_DEBUG_KMS("\n");
>>>
>>> @@ -109,10 +102,6 @@ static bool intel_dsi_compute_config(struct intel_encoder *encoder,
>>>    	/* DSI uses short packets for sync events, so clear mode flags for DSI */
>>>    	adjusted_mode->flags = 0;
>>>
>>> -	if (intel_dsi->dev.dev_ops->mode_fixup)
>>> -		return intel_dsi->dev.dev_ops->mode_fixup(&intel_dsi->dev,
>>> -							  mode, adjusted_mode);
>>> -
>>
>> There had been a instance where we had to drive different resolution
>> (lower) than the native one. Also in VBT there is a field to make this
>> generic at least from driver perspective to give the needed target
>> resolution. In case target resolution is same as native, nothing gets
>> changed, else mode_fixup function adjusts the mode accordingly keeping
>> timing as same and enabling scalar. Might not be useful in general, but
>> did find a use internally.
>
> Can we just have the driver return the desired mode from .get_modes in
> that case?

Okay, I think I did not explain correctly. Get modes is modified to give 
the needed target mode only so that userspace creates buffer of the 
needed resolution, but in fixup which is called at modeset, we correct 
the adjusted_mode back to have native resolutions so that modeset is 
correctly done. if we do not do like this, during modeset resolutions 
will be wrong as per the timings.

Regards
Shobhit

>
> BR,
> Jani.
>
>
>>
>> Either way
>>
>> Reviewed-By: Shobhit Kumar <shobhit.kumar@intel.com>
>>
>>>    	return true;
>>>    }
>>>
>>> @@ -269,9 +258,6 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder)
>>>    	if (intel_dsi->dev.dev_ops->panel_reset)
>>>    		intel_dsi->dev.dev_ops->panel_reset(&intel_dsi->dev);
>>>
>>> -	if (intel_dsi->dev.dev_ops->send_otp_cmds)
>>> -		intel_dsi->dev.dev_ops->send_otp_cmds(&intel_dsi->dev);
>>> -
>>>    	for_each_dsi_port(port, intel_dsi->ports)
>>>    		wait_for_dsi_fifo_empty(intel_dsi, port);
>>>
>>> @@ -484,7 +470,6 @@ intel_dsi_mode_valid(struct drm_connector *connector,
>>>    {
>>>    	struct intel_connector *intel_connector = to_intel_connector(connector);
>>>    	struct drm_display_mode *fixed_mode = intel_connector->panel.fixed_mode;
>>> -	struct intel_dsi *intel_dsi = intel_attached_dsi(connector);
>>>
>>>    	DRM_DEBUG_KMS("\n");
>>>
>>> @@ -500,7 +485,7 @@ intel_dsi_mode_valid(struct drm_connector *connector,
>>>    			return MODE_PANEL;
>>>    	}
>>>
>>> -	return intel_dsi->dev.dev_ops->mode_valid(&intel_dsi->dev, mode);
>>> +	return MODE_OK;
>>>    }
>>>
>>>    /* return txclkesc cycles in terms of divider and duration in us */
>>> @@ -749,20 +734,7 @@ static void intel_dsi_pre_pll_enable(struct intel_encoder *encoder)
>>>    static enum drm_connector_status
>>>    intel_dsi_detect(struct drm_connector *connector, bool force)
>>>    {
>>> -	struct intel_dsi *intel_dsi = intel_attached_dsi(connector);
>>> -	struct intel_encoder *intel_encoder = &intel_dsi->base;
>>> -	enum intel_display_power_domain power_domain;
>>> -	enum drm_connector_status connector_status;
>>> -	struct drm_i915_private *dev_priv = intel_encoder->base.dev->dev_private;
>>> -
>>> -	DRM_DEBUG_KMS("\n");
>>> -	power_domain = intel_display_port_power_domain(intel_encoder);
>>> -
>>> -	intel_display_power_get(dev_priv, power_domain);
>>> -	connector_status = intel_dsi->dev.dev_ops->detect(&intel_dsi->dev);
>>> -	intel_display_power_put(dev_priv, power_domain);
>>> -
>>> -	return connector_status;
>>> +	return connector_status_connected;
>>>    }
>>>
>>>    static int intel_dsi_get_modes(struct drm_connector *connector)
>>> diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
>>> index 2bb8c46c7889..22f87036a256 100644
>>> --- a/drivers/gpu/drm/i915/intel_dsi.h
>>> +++ b/drivers/gpu/drm/i915/intel_dsi.h
>>> @@ -47,33 +47,13 @@ struct intel_dsi_dev_ops {
>>>
>>>    	void (*disable_panel_power)(struct intel_dsi_device *dsi);
>>>
>>> -	/* one time programmable commands if needed */
>>> -	void (*send_otp_cmds)(struct intel_dsi_device *dsi);
>>> -
>>>    	/* This callback must be able to assume DSI commands can be sent */
>>>    	void (*enable)(struct intel_dsi_device *dsi);
>>>
>>>    	/* This callback must be able to assume DSI commands can be sent */
>>>    	void (*disable)(struct intel_dsi_device *dsi);
>>>
>>> -	int (*mode_valid)(struct intel_dsi_device *dsi,
>>> -			  struct drm_display_mode *mode);
>>> -
>>> -	bool (*mode_fixup)(struct intel_dsi_device *dsi,
>>> -			   const struct drm_display_mode *mode,
>>> -			   struct drm_display_mode *adjusted_mode);
>>> -
>>> -	void (*mode_set)(struct intel_dsi_device *dsi,
>>> -			 struct drm_display_mode *mode,
>>> -			 struct drm_display_mode *adjusted_mode);
>>> -
>>> -	enum drm_connector_status (*detect)(struct intel_dsi_device *dsi);
>>> -
>>> -	bool (*get_hw_state)(struct intel_dsi_device *dev);
>>> -
>>>    	struct drm_display_mode *(*get_modes)(struct intel_dsi_device *dsi);
>>> -
>>> -	void (*destroy) (struct intel_dsi_device *dsi);
>>>    };
>>>
>>>    struct intel_dsi {
>>> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>>> index 5493aef5a6a3..b0e7327a485f 100644
>>> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>>> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>>> @@ -559,18 +559,6 @@ static bool generic_init(struct intel_dsi_device *dsi)
>>>    	return true;
>>>    }
>>>
>>> -static int generic_mode_valid(struct intel_dsi_device *dsi,
>>> -		   struct drm_display_mode *mode)
>>> -{
>>> -	return MODE_OK;
>>> -}
>>> -
>>> -static bool generic_mode_fixup(struct intel_dsi_device *dsi,
>>> -		    const struct drm_display_mode *mode,
>>> -		    struct drm_display_mode *adjusted_mode) {
>>> -	return true;
>>> -}
>>> -
>>>    static void generic_panel_reset(struct intel_dsi_device *dsi)
>>>    {
>>>    	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
>>> @@ -580,26 +568,18 @@ static void generic_panel_reset(struct intel_dsi_device *dsi)
>>>    	char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_ASSERT_RESET];
>>>
>>>    	generic_exec_sequence(intel_dsi, sequence);
>>> -}
>>> -
>>> -static void generic_disable_panel_power(struct intel_dsi_device *dsi)
>>> -{
>>> -	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
>>> -	struct drm_device *dev = intel_dsi->base.base.dev;
>>> -	struct drm_i915_private *dev_priv = dev->dev_private;
>>> -
>>> -	char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_DEASSERT_RESET];
>>>
>>> +	sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_INIT_OTP];
>>>    	generic_exec_sequence(intel_dsi, sequence);
>>>    }
>>>
>>> -static void generic_send_otp_cmds(struct intel_dsi_device *dsi)
>>> +static void generic_disable_panel_power(struct intel_dsi_device *dsi)
>>>    {
>>>    	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
>>>    	struct drm_device *dev = intel_dsi->base.base.dev;
>>>    	struct drm_i915_private *dev_priv = dev->dev_private;
>>>
>>> -	char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_INIT_OTP];
>>> +	char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_DEASSERT_RESET];
>>>
>>>    	generic_exec_sequence(intel_dsi, sequence);
>>>    }
>>> @@ -626,16 +606,6 @@ static void generic_disable(struct intel_dsi_device *dsi)
>>>    	generic_exec_sequence(intel_dsi, sequence);
>>>    }
>>>
>>> -static enum drm_connector_status generic_detect(struct intel_dsi_device *dsi)
>>> -{
>>> -	return connector_status_connected;
>>> -}
>>> -
>>> -static bool generic_get_hw_state(struct intel_dsi_device *dev)
>>> -{
>>> -	return true;
>>> -}
>>> -
>>>    static struct drm_display_mode *generic_get_modes(struct intel_dsi_device *dsi)
>>>    {
>>>    	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
>>> @@ -646,20 +616,11 @@ static struct drm_display_mode *generic_get_modes(struct intel_dsi_device *dsi)
>>>    	return dev_priv->vbt.lfp_lvds_vbt_mode;
>>>    }
>>>
>>> -static void generic_destroy(struct intel_dsi_device *dsi) { }
>>> -
>>> -/* Callbacks. We might not need them all. */
>>>    struct intel_dsi_dev_ops vbt_generic_dsi_display_ops = {
>>>    	.init = generic_init,
>>> -	.mode_valid = generic_mode_valid,
>>> -	.mode_fixup = generic_mode_fixup,
>>>    	.panel_reset = generic_panel_reset,
>>>    	.disable_panel_power = generic_disable_panel_power,
>>> -	.send_otp_cmds = generic_send_otp_cmds,
>>>    	.enable = generic_enable,
>>>    	.disable = generic_disable,
>>> -	.detect = generic_detect,
>>> -	.get_hw_state = generic_get_hw_state,
>>>    	.get_modes = generic_get_modes,
>>> -	.destroy = generic_destroy,
>>>    };
>>>
>>
>> Regards
>> Shobhit
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
Daniel Vetter Jan. 23, 2015, 3:22 p.m. UTC | #4
On Fri, Jan 23, 2015 at 03:14:41PM +0530, Shobhit Kumar wrote:
> On 01/22/2015 06:53 PM, Jani Nikula wrote:
> >On Thu, 22 Jan 2015, Shobhit Kumar <shobhit.kumar@linux.intel.com> wrote:
> >>There had been a instance where we had to drive different resolution
> >>(lower) than the native one. Also in VBT there is a field to make this
> >>generic at least from driver perspective to give the needed target
> >>resolution. In case target resolution is same as native, nothing gets
> >>changed, else mode_fixup function adjusts the mode accordingly keeping
> >>timing as same and enabling scalar. Might not be useful in general, but
> >>did find a use internally.
> >
> >Can we just have the driver return the desired mode from .get_modes in
> >that case?
> 
> Okay, I think I did not explain correctly. Get modes is modified to give the
> needed target mode only so that userspace creates buffer of the needed
> resolution, but in fixup which is called at modeset, we correct the
> adjusted_mode back to have native resolutions so that modeset is correctly
> done. if we do not do like this, during modeset resolutions will be wrong as
> per the timings.

I'm confused. Can you please give an example in real numbers about the
different resolution and how it's all fixed up in hw?

E.g. 800x600 framebuffer -> pfit -> 1024x756 panel,

get_modes gives 800x600, adjusted mode corrects to 1024x756. And please
mention what vbt has to do in all this too.

I think I need an example since I can't figure out what exactly your
describing ...

Thanks, Daniel
Shobhit Kumar Jan. 27, 2015, 8:41 a.m. UTC | #5
On 01/23/2015 08:52 PM, Daniel Vetter wrote:
> On Fri, Jan 23, 2015 at 03:14:41PM +0530, Shobhit Kumar wrote:
>> On 01/22/2015 06:53 PM, Jani Nikula wrote:
>>> On Thu, 22 Jan 2015, Shobhit Kumar <shobhit.kumar@linux.intel.com> wrote:
>>>> There had been a instance where we had to drive different resolution
>>>> (lower) than the native one. Also in VBT there is a field to make this
>>>> generic at least from driver perspective to give the needed target
>>>> resolution. In case target resolution is same as native, nothing gets
>>>> changed, else mode_fixup function adjusts the mode accordingly keeping
>>>> timing as same and enabling scalar. Might not be useful in general, but
>>>> did find a use internally.
>>>
>>> Can we just have the driver return the desired mode from .get_modes in
>>> that case?
>>
>> Okay, I think I did not explain correctly. Get modes is modified to give the
>> needed target mode only so that userspace creates buffer of the needed
>> resolution, but in fixup which is called at modeset, we correct the
>> adjusted_mode back to have native resolutions so that modeset is correctly
>> done. if we do not do like this, during modeset resolutions will be wrong as
>> per the timings.
>
> I'm confused. Can you please give an example in real numbers about the
> different resolution and how it's all fixed up in hw?
>
> E.g. 800x600 framebuffer -> pfit -> 1024x756 panel,
>
> get_modes gives 800x600, adjusted mode corrects to 1024x756. And please

We had a 19x12 DSI panel which we needed to drive at 12x8 due to lack of 
12x8 panels for testing purposes. So get_modes returned 12x8 so that 
user space gave 12x8 FBs, and internally in mode_fixup we adjusted 
correctly for the 19x12 panel timings and enabled pfit

> mention what vbt has to do in all this too.

VBT allows to avoid panel specific and build specific hard coding in the 
code, which should work as is for normal 19x12 usage as well. Block #42 
panel xres and yres are used to indicate this lower needed resolution 
when different from panel_fixed_mode.

Regards
Shobhit

>
> I think I need an example since I can't figure out what exactly your
> describing ...
>
> Thanks, Daniel
>
Daniel Vetter Jan. 27, 2015, 1:09 p.m. UTC | #6
On Tue, Jan 27, 2015 at 02:11:18PM +0530, Shobhit Kumar wrote:
> On 01/23/2015 08:52 PM, Daniel Vetter wrote:
> >On Fri, Jan 23, 2015 at 03:14:41PM +0530, Shobhit Kumar wrote:
> >>On 01/22/2015 06:53 PM, Jani Nikula wrote:
> >>>On Thu, 22 Jan 2015, Shobhit Kumar <shobhit.kumar@linux.intel.com> wrote:
> >>>>There had been a instance where we had to drive different resolution
> >>>>(lower) than the native one. Also in VBT there is a field to make this
> >>>>generic at least from driver perspective to give the needed target
> >>>>resolution. In case target resolution is same as native, nothing gets
> >>>>changed, else mode_fixup function adjusts the mode accordingly keeping
> >>>>timing as same and enabling scalar. Might not be useful in general, but
> >>>>did find a use internally.
> >>>
> >>>Can we just have the driver return the desired mode from .get_modes in
> >>>that case?
> >>
> >>Okay, I think I did not explain correctly. Get modes is modified to give the
> >>needed target mode only so that userspace creates buffer of the needed
> >>resolution, but in fixup which is called at modeset, we correct the
> >>adjusted_mode back to have native resolutions so that modeset is correctly
> >>done. if we do not do like this, during modeset resolutions will be wrong as
> >>per the timings.
> >
> >I'm confused. Can you please give an example in real numbers about the
> >different resolution and how it's all fixed up in hw?
> >
> >E.g. 800x600 framebuffer -> pfit -> 1024x756 panel,
> >
> >get_modes gives 800x600, adjusted mode corrects to 1024x756. And please
> 
> We had a 19x12 DSI panel which we needed to drive at 12x8 due to lack of
> 12x8 panels for testing purposes. So get_modes returned 12x8 so that user
> space gave 12x8 FBs, and internally in mode_fixup we adjusted correctly for
> the 19x12 panel timings and enabled pfit

Hm, is that a real use-case shipping to customers or just a hack for
development? In the later case I think we can just hardcode the edid for
edp ...
-Daniel
Chris Wilson Jan. 27, 2015, 1:13 p.m. UTC | #7
On Tue, Jan 27, 2015 at 02:09:21PM +0100, Daniel Vetter wrote:
> On Tue, Jan 27, 2015 at 02:11:18PM +0530, Shobhit Kumar wrote:
> > On 01/23/2015 08:52 PM, Daniel Vetter wrote:
> > >On Fri, Jan 23, 2015 at 03:14:41PM +0530, Shobhit Kumar wrote:
> > >>On 01/22/2015 06:53 PM, Jani Nikula wrote:
> > >>>On Thu, 22 Jan 2015, Shobhit Kumar <shobhit.kumar@linux.intel.com> wrote:
> > >>>>There had been a instance where we had to drive different resolution
> > >>>>(lower) than the native one. Also in VBT there is a field to make this
> > >>>>generic at least from driver perspective to give the needed target
> > >>>>resolution. In case target resolution is same as native, nothing gets
> > >>>>changed, else mode_fixup function adjusts the mode accordingly keeping
> > >>>>timing as same and enabling scalar. Might not be useful in general, but
> > >>>>did find a use internally.
> > >>>
> > >>>Can we just have the driver return the desired mode from .get_modes in
> > >>>that case?
> > >>
> > >>Okay, I think I did not explain correctly. Get modes is modified to give the
> > >>needed target mode only so that userspace creates buffer of the needed
> > >>resolution, but in fixup which is called at modeset, we correct the
> > >>adjusted_mode back to have native resolutions so that modeset is correctly
> > >>done. if we do not do like this, during modeset resolutions will be wrong as
> > >>per the timings.
> > >
> > >I'm confused. Can you please give an example in real numbers about the
> > >different resolution and how it's all fixed up in hw?
> > >
> > >E.g. 800x600 framebuffer -> pfit -> 1024x756 panel,
> > >
> > >get_modes gives 800x600, adjusted mode corrects to 1024x756. And please
> > 
> > We had a 19x12 DSI panel which we needed to drive at 12x8 due to lack of
> > 12x8 panels for testing purposes. So get_modes returned 12x8 so that user
> > space gave 12x8 FBs, and internally in mode_fixup we adjusted correctly for
> > the 19x12 panel timings and enabled pfit
> 
> Hm, is that a real use-case shipping to customers or just a hack for
> development? In the later case I think we can just hardcode the edid for
> edp ...

Also how is this different from userspace creating a 800x600 mode and
giving it to the kernel which then uses the pfitter to display it at
native resolution. That is how it works today. This should also be
possible with a video= parameter...
-Chris
Shobhit Kumar Jan. 28, 2015, 5:08 a.m. UTC | #8
On 01/27/2015 06:43 PM, Chris Wilson wrote:
> On Tue, Jan 27, 2015 at 02:09:21PM +0100, Daniel Vetter wrote:
>> On Tue, Jan 27, 2015 at 02:11:18PM +0530, Shobhit Kumar wrote:
>>> On 01/23/2015 08:52 PM, Daniel Vetter wrote:
>>>> On Fri, Jan 23, 2015 at 03:14:41PM +0530, Shobhit Kumar wrote:
>>>>> On 01/22/2015 06:53 PM, Jani Nikula wrote:
>>>>>> On Thu, 22 Jan 2015, Shobhit Kumar <shobhit.kumar@linux.intel.com> wrote:
>>>>>>> There had been a instance where we had to drive different resolution
>>>>>>> (lower) than the native one. Also in VBT there is a field to make this
>>>>>>> generic at least from driver perspective to give the needed target
>>>>>>> resolution. In case target resolution is same as native, nothing gets
>>>>>>> changed, else mode_fixup function adjusts the mode accordingly keeping
>>>>>>> timing as same and enabling scalar. Might not be useful in general, but
>>>>>>> did find a use internally.
>>>>>>
>>>>>> Can we just have the driver return the desired mode from .get_modes in
>>>>>> that case?
>>>>>
>>>>> Okay, I think I did not explain correctly. Get modes is modified to give the
>>>>> needed target mode only so that userspace creates buffer of the needed
>>>>> resolution, but in fixup which is called at modeset, we correct the
>>>>> adjusted_mode back to have native resolutions so that modeset is correctly
>>>>> done. if we do not do like this, during modeset resolutions will be wrong as
>>>>> per the timings.
>>>>
>>>> I'm confused. Can you please give an example in real numbers about the
>>>> different resolution and how it's all fixed up in hw?
>>>>
>>>> E.g. 800x600 framebuffer -> pfit -> 1024x756 panel,
>>>>
>>>> get_modes gives 800x600, adjusted mode corrects to 1024x756. And please
>>>
>>> We had a 19x12 DSI panel which we needed to drive at 12x8 due to lack of
>>> 12x8 panels for testing purposes. So get_modes returned 12x8 so that user
>>> space gave 12x8 FBs, and internally in mode_fixup we adjusted correctly for
>>> the 19x12 panel timings and enabled pfit
>>
>> Hm, is that a real use-case shipping to customers or just a hack for
>> development? In the later case I think we can just hardcode the edid for
>> edp ...
>
> Also how is this different from userspace creating a 800x600 mode and
> giving it to the kernel which then uses the pfitter to display it at
> native resolution. That is how it works today. This should also be
> possible with a video= parameter...

Its different in a way, that user space changes will need a new system 
build which is not allowed as per the requirements that we had and hence 
no hard coding in code anywhere as well.

As I said earlier also that this case might not be useful in general and 
I am okay to remove this callback.

Regards
Shobhit
Daniel Vetter Jan. 28, 2015, 9:17 a.m. UTC | #9
On Wed, Jan 28, 2015 at 10:38:45AM +0530, Shobhit Kumar wrote:
> On 01/27/2015 06:43 PM, Chris Wilson wrote:
> >On Tue, Jan 27, 2015 at 02:09:21PM +0100, Daniel Vetter wrote:
> >>On Tue, Jan 27, 2015 at 02:11:18PM +0530, Shobhit Kumar wrote:
> >>>On 01/23/2015 08:52 PM, Daniel Vetter wrote:
> >>>>On Fri, Jan 23, 2015 at 03:14:41PM +0530, Shobhit Kumar wrote:
> >>>>>On 01/22/2015 06:53 PM, Jani Nikula wrote:
> >>>>>>On Thu, 22 Jan 2015, Shobhit Kumar <shobhit.kumar@linux.intel.com> wrote:
> >>>>>>>There had been a instance where we had to drive different resolution
> >>>>>>>(lower) than the native one. Also in VBT there is a field to make this
> >>>>>>>generic at least from driver perspective to give the needed target
> >>>>>>>resolution. In case target resolution is same as native, nothing gets
> >>>>>>>changed, else mode_fixup function adjusts the mode accordingly keeping
> >>>>>>>timing as same and enabling scalar. Might not be useful in general, but
> >>>>>>>did find a use internally.
> >>>>>>
> >>>>>>Can we just have the driver return the desired mode from .get_modes in
> >>>>>>that case?
> >>>>>
> >>>>>Okay, I think I did not explain correctly. Get modes is modified to give the
> >>>>>needed target mode only so that userspace creates buffer of the needed
> >>>>>resolution, but in fixup which is called at modeset, we correct the
> >>>>>adjusted_mode back to have native resolutions so that modeset is correctly
> >>>>>done. if we do not do like this, during modeset resolutions will be wrong as
> >>>>>per the timings.
> >>>>
> >>>>I'm confused. Can you please give an example in real numbers about the
> >>>>different resolution and how it's all fixed up in hw?
> >>>>
> >>>>E.g. 800x600 framebuffer -> pfit -> 1024x756 panel,
> >>>>
> >>>>get_modes gives 800x600, adjusted mode corrects to 1024x756. And please
> >>>
> >>>We had a 19x12 DSI panel which we needed to drive at 12x8 due to lack of
> >>>12x8 panels for testing purposes. So get_modes returned 12x8 so that user
> >>>space gave 12x8 FBs, and internally in mode_fixup we adjusted correctly for
> >>>the 19x12 panel timings and enabled pfit
> >>
> >>Hm, is that a real use-case shipping to customers or just a hack for
> >>development? In the later case I think we can just hardcode the edid for
> >>edp ...
> >
> >Also how is this different from userspace creating a 800x600 mode and
> >giving it to the kernel which then uses the pfitter to display it at
> >native resolution. That is how it works today. This should also be
> >possible with a video= parameter...
> 
> Its different in a way, that user space changes will need a new system build
> which is not allowed as per the requirements that we had and hence no hard
> coding in code anywhere as well.
> 
> As I said earlier also that this case might not be useful in general and I
> am okay to remove this callback.

Yeah I think if rebuilding vbt for testing is ok, but rebuilding the
kernel isn't then that just smells like needless red tape. I'll reconsider
as soon as we need this for shipping systems of course.
-Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 9b0eaa9db845..fc218b7754b3 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -70,12 +70,6 @@  static void band_gap_reset(struct drm_i915_private *dev_priv)
 	mutex_unlock(&dev_priv->dpio_lock);
 }
 
-static struct intel_dsi *intel_attached_dsi(struct drm_connector *connector)
-{
-	return container_of(intel_attached_encoder(connector),
-			    struct intel_dsi, base);
-}
-
 static inline bool is_vid_mode(struct intel_dsi *intel_dsi)
 {
 	return intel_dsi->operation_mode == INTEL_DSI_VIDEO_MODE;
@@ -99,7 +93,6 @@  static bool intel_dsi_compute_config(struct intel_encoder *encoder,
 	struct intel_connector *intel_connector = intel_dsi->attached_connector;
 	struct drm_display_mode *fixed_mode = intel_connector->panel.fixed_mode;
 	struct drm_display_mode *adjusted_mode = &config->adjusted_mode;
-	struct drm_display_mode *mode = &config->requested_mode;
 
 	DRM_DEBUG_KMS("\n");
 
@@ -109,10 +102,6 @@  static bool intel_dsi_compute_config(struct intel_encoder *encoder,
 	/* DSI uses short packets for sync events, so clear mode flags for DSI */
 	adjusted_mode->flags = 0;
 
-	if (intel_dsi->dev.dev_ops->mode_fixup)
-		return intel_dsi->dev.dev_ops->mode_fixup(&intel_dsi->dev,
-							  mode, adjusted_mode);
-
 	return true;
 }
 
@@ -269,9 +258,6 @@  static void intel_dsi_pre_enable(struct intel_encoder *encoder)
 	if (intel_dsi->dev.dev_ops->panel_reset)
 		intel_dsi->dev.dev_ops->panel_reset(&intel_dsi->dev);
 
-	if (intel_dsi->dev.dev_ops->send_otp_cmds)
-		intel_dsi->dev.dev_ops->send_otp_cmds(&intel_dsi->dev);
-
 	for_each_dsi_port(port, intel_dsi->ports)
 		wait_for_dsi_fifo_empty(intel_dsi, port);
 
@@ -484,7 +470,6 @@  intel_dsi_mode_valid(struct drm_connector *connector,
 {
 	struct intel_connector *intel_connector = to_intel_connector(connector);
 	struct drm_display_mode *fixed_mode = intel_connector->panel.fixed_mode;
-	struct intel_dsi *intel_dsi = intel_attached_dsi(connector);
 
 	DRM_DEBUG_KMS("\n");
 
@@ -500,7 +485,7 @@  intel_dsi_mode_valid(struct drm_connector *connector,
 			return MODE_PANEL;
 	}
 
-	return intel_dsi->dev.dev_ops->mode_valid(&intel_dsi->dev, mode);
+	return MODE_OK;
 }
 
 /* return txclkesc cycles in terms of divider and duration in us */
@@ -749,20 +734,7 @@  static void intel_dsi_pre_pll_enable(struct intel_encoder *encoder)
 static enum drm_connector_status
 intel_dsi_detect(struct drm_connector *connector, bool force)
 {
-	struct intel_dsi *intel_dsi = intel_attached_dsi(connector);
-	struct intel_encoder *intel_encoder = &intel_dsi->base;
-	enum intel_display_power_domain power_domain;
-	enum drm_connector_status connector_status;
-	struct drm_i915_private *dev_priv = intel_encoder->base.dev->dev_private;
-
-	DRM_DEBUG_KMS("\n");
-	power_domain = intel_display_port_power_domain(intel_encoder);
-
-	intel_display_power_get(dev_priv, power_domain);
-	connector_status = intel_dsi->dev.dev_ops->detect(&intel_dsi->dev);
-	intel_display_power_put(dev_priv, power_domain);
-
-	return connector_status;
+	return connector_status_connected;
 }
 
 static int intel_dsi_get_modes(struct drm_connector *connector)
diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
index 2bb8c46c7889..22f87036a256 100644
--- a/drivers/gpu/drm/i915/intel_dsi.h
+++ b/drivers/gpu/drm/i915/intel_dsi.h
@@ -47,33 +47,13 @@  struct intel_dsi_dev_ops {
 
 	void (*disable_panel_power)(struct intel_dsi_device *dsi);
 
-	/* one time programmable commands if needed */
-	void (*send_otp_cmds)(struct intel_dsi_device *dsi);
-
 	/* This callback must be able to assume DSI commands can be sent */
 	void (*enable)(struct intel_dsi_device *dsi);
 
 	/* This callback must be able to assume DSI commands can be sent */
 	void (*disable)(struct intel_dsi_device *dsi);
 
-	int (*mode_valid)(struct intel_dsi_device *dsi,
-			  struct drm_display_mode *mode);
-
-	bool (*mode_fixup)(struct intel_dsi_device *dsi,
-			   const struct drm_display_mode *mode,
-			   struct drm_display_mode *adjusted_mode);
-
-	void (*mode_set)(struct intel_dsi_device *dsi,
-			 struct drm_display_mode *mode,
-			 struct drm_display_mode *adjusted_mode);
-
-	enum drm_connector_status (*detect)(struct intel_dsi_device *dsi);
-
-	bool (*get_hw_state)(struct intel_dsi_device *dev);
-
 	struct drm_display_mode *(*get_modes)(struct intel_dsi_device *dsi);
-
-	void (*destroy) (struct intel_dsi_device *dsi);
 };
 
 struct intel_dsi {
diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
index 5493aef5a6a3..b0e7327a485f 100644
--- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
+++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
@@ -559,18 +559,6 @@  static bool generic_init(struct intel_dsi_device *dsi)
 	return true;
 }
 
-static int generic_mode_valid(struct intel_dsi_device *dsi,
-		   struct drm_display_mode *mode)
-{
-	return MODE_OK;
-}
-
-static bool generic_mode_fixup(struct intel_dsi_device *dsi,
-		    const struct drm_display_mode *mode,
-		    struct drm_display_mode *adjusted_mode) {
-	return true;
-}
-
 static void generic_panel_reset(struct intel_dsi_device *dsi)
 {
 	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
@@ -580,26 +568,18 @@  static void generic_panel_reset(struct intel_dsi_device *dsi)
 	char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_ASSERT_RESET];
 
 	generic_exec_sequence(intel_dsi, sequence);
-}
-
-static void generic_disable_panel_power(struct intel_dsi_device *dsi)
-{
-	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
-	struct drm_device *dev = intel_dsi->base.base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
-	char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_DEASSERT_RESET];
 
+	sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_INIT_OTP];
 	generic_exec_sequence(intel_dsi, sequence);
 }
 
-static void generic_send_otp_cmds(struct intel_dsi_device *dsi)
+static void generic_disable_panel_power(struct intel_dsi_device *dsi)
 {
 	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
 	struct drm_device *dev = intel_dsi->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_INIT_OTP];
+	char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_DEASSERT_RESET];
 
 	generic_exec_sequence(intel_dsi, sequence);
 }
@@ -626,16 +606,6 @@  static void generic_disable(struct intel_dsi_device *dsi)
 	generic_exec_sequence(intel_dsi, sequence);
 }
 
-static enum drm_connector_status generic_detect(struct intel_dsi_device *dsi)
-{
-	return connector_status_connected;
-}
-
-static bool generic_get_hw_state(struct intel_dsi_device *dev)
-{
-	return true;
-}
-
 static struct drm_display_mode *generic_get_modes(struct intel_dsi_device *dsi)
 {
 	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
@@ -646,20 +616,11 @@  static struct drm_display_mode *generic_get_modes(struct intel_dsi_device *dsi)
 	return dev_priv->vbt.lfp_lvds_vbt_mode;
 }
 
-static void generic_destroy(struct intel_dsi_device *dsi) { }
-
-/* Callbacks. We might not need them all. */
 struct intel_dsi_dev_ops vbt_generic_dsi_display_ops = {
 	.init = generic_init,
-	.mode_valid = generic_mode_valid,
-	.mode_fixup = generic_mode_fixup,
 	.panel_reset = generic_panel_reset,
 	.disable_panel_power = generic_disable_panel_power,
-	.send_otp_cmds = generic_send_otp_cmds,
 	.enable = generic_enable,
 	.disable = generic_disable,
-	.detect = generic_detect,
-	.get_hw_state = generic_get_hw_state,
 	.get_modes = generic_get_modes,
-	.destroy = generic_destroy,
 };