Re: [RFC v2 4/4] drm/i915: Enable DSI panel enable/disable based on PMIC

["Kumar, Shobhit" <shobhit.kumar@linux.intel.com>]

On 1/9/2015 6:47 PM, Jani Nikula wrote:
> On Fri, 02 Jan 2015, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
>> This allows for proper PPS during enable/disable of BYT-T platforms
>> where these signals are routed through PMIC. Needs DRM_PANEL to be
>> selected by default as well
>>
>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>> ---
>>   drivers/gpu/drm/i915/Kconfig               |  1 +
>>   drivers/gpu/drm/i915/intel_dsi.c           | 16 ++++++++++++++++
>>   drivers/gpu/drm/i915/intel_dsi.h           |  6 ++++++
>>   drivers/gpu/drm/i915/intel_dsi_panel_vbt.c |  1 +
>>   4 files changed, 24 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
>> index 4e39ab3..3210dbb 100644
>> --- a/drivers/gpu/drm/i915/Kconfig
>> +++ b/drivers/gpu/drm/i915/Kconfig
>> @@ -18,6 +18,7 @@ config DRM_I915
>>   	select INPUT if ACPI
>>   	select ACPI_VIDEO if ACPI
>>   	select ACPI_BUTTON if ACPI
>> +	select DRM_PANEL
>>   	help
>>   	  Choose this option if you have a system that has "Intel Graphics
>>   	  Media Accelerator" or "HD Graphics" integrated graphics,
>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
>> index 42b6d6f..431e7cb 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>> @@ -26,6 +26,7 @@
>>   #include <drm/drmP.h>
>>   #include <drm/drm_crtc.h>
>>   #include <drm/drm_edid.h>
>> +#include <drm/drm_panel.h>
>>   #include <drm/i915_drm.h>
>>   #include <linux/slab.h>
>>   #include "i915_drv.h"
>> @@ -230,6 +231,8 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder)
>>
>>   	DRM_DEBUG_KMS("\n");
>>
>> +	drm_panel_enable(intel_dsi->panel);
>> +
>>   	/* Disable DPOunit clock gating, can stall pipe
>>   	 * and we need DPLL REFA always enabled */
>>   	tmp = I915_READ(DPLL(pipe));
>> @@ -392,6 +395,8 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder)
>>
>>   	msleep(intel_dsi->panel_off_delay);
>>   	msleep(intel_dsi->panel_pwr_cycle_delay);
>> +
>> +	drm_panel_disable(intel_dsi->panel);
>>   }
>
> As I explained in a private mail, I intend to convert all of our panel
> driver callbacks to the drm_panel model. So we'd have two sets of
> drm_panel_* hooks sprinkled here, one for handling the panel power and
> the other for the generic vbt panel driver. An alternative would be to
> have the vbt panel driver handle this internally, but I don't really
> know which one is better. In any case this has a fairly small footprint
> so it's easy to change one way or another.

PMIC based control is something unique for current platforms (BYT/CHT) 
and all future platforms will go with SoC based control. Given that, I 
think we should not put this is VBT panel driver and keep separate.

>
>>
>>   static bool intel_dsi_get_hw_state(struct intel_encoder *encoder,
>> @@ -896,6 +901,17 @@ void intel_dsi_init(struct drm_device *dev)
>>   	fixed_mode->type |= DRM_MODE_TYPE_PREFERRED;
>>   	intel_panel_init(&intel_connector->panel, fixed_mode, NULL);
>>
>> +	/* Initialize the PMIC based drm_panel if available on the platform */
>> +	if (intel_dsi->pps_blc == PPS_BLC_PMIC) {
>> +		intel_dsi->panel = name_drm_find_panel("crystal_cove_panel");
>> +		if (!intel_dsi->panel) {
>> +			DRM_ERROR("PMIC Panel control will not work !!\n");
>> +			return;
>> +		}
>> +
>> +		drm_panel_attach(intel_dsi->panel, connector);
>> +	}
>> +
>
> I think there's an init order problem here. If the panel driver hasn't
> been registered yet this will fail. I don't know what the answer to that
> should be.

Agree and for my testing I made both PMIC and This panel driver as 
in-built and i915 as module. I was hoping for some answer !!

>
>>   	return;
>>
>>   err:
>> diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
>> index 8fe2064..4a9242d 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi.h
>> +++ b/drivers/gpu/drm/i915/intel_dsi.h
>> @@ -33,6 +33,9 @@
>>   #define DSI_DUAL_LINK_FRONT_BACK	1
>>   #define DSI_DUAL_LINK_PIXEL_ALT		2
>>
>> +#define PPS_BLC_PMIC	0
>> +#define PPS_BLC_SOC	1
>> +
>>   struct intel_dsi_device {
>>   	unsigned int panel_id;
>>   	const char *name;
>> @@ -83,6 +86,8 @@ struct intel_dsi {
>>
>>   	struct intel_connector *attached_connector;
>>
>> +	struct drm_panel *panel;
>> +
>>   	/* bit mask of ports being driven */
>>   	u16 ports;
>>
>> @@ -116,6 +121,7 @@ struct intel_dsi {
>>   	u32 dphy_reg;
>>   	u32 video_frmt_cfg_bits;
>>   	u16 lp_byte_clk;
>> +	u8 pps_blc;
>>
>>   	/* timeouts in byte clocks */
>>   	u16 lp_rx_timeout;
>> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> index 5493aef..0612d33 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> @@ -297,6 +297,7 @@ static bool generic_init(struct intel_dsi_device *dsi)
>>   	intel_dsi->pixel_format = mipi_config->videomode_color_format << 7;
>>   	intel_dsi->dual_link = mipi_config->dual_link;
>>   	intel_dsi->pixel_overlap = mipi_config->pixel_overlap;
>> +	intel_dsi->pps_blc = mipi_config->pwm_blc;
>
> If the drm_panel for crystal cove is going to be like in this patch, I
> think this part belongs in intel_dsi.c.

If you are suggesting check mipi_config->pwm_blc directly in intel_dsi, 
that can be done. Was just abstracting all VBT fields in generic VBT 
based panel driver

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