Re: [PATCH 1/4] drm/i915: Add more dev ops for MIPI sub encoder

Shobhit Kumar
2013-10-22T09:39:05Z

On 10/21/2013 6:57 PM, Jani Nikula wrote:
>
> Hi Shobhit -
>
> On Mon, 21 Oct 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
>> Also add new fields in intel_dsi to have all dphy related parameters.
>> These will be useful even when we go for pure generic MIPI design
>
> I feel like we have a different idea of what the ideal generic design
> is. For me, the goal is that we change the struct intel_dsi_device to
> struct drm_bridge, and those drm_bridge drivers are all about the panel,
> and have as few details about i915 or our hardware as possible. Having
> the bridge drivers fill in register values to be written by the core DSI
> code does not fit that. Yes, I had some of those in my bridge conversion
> patches too, but I did not intend we'd keep adding more.
>
> I'd rather we provide generic helpers the bridge driver can call.

Yeah, look like our ideas are different. In your goal with drm_bridge
architecture, we will still end up having multiple bridge drivers for
each different panel. But my goal is to have a single driver which can
work for multiple panels. Since we already have enabled some panels with
sub-encoder architecture for completeness I was planning to maintain
generic driver as one sub-encoder. But actually we can do away with all
sub-encoder and do not need even drm_bridge and all implementation will
be in intel_dsi.c. Panel specific configurations or sequences will come
from VBT which I have tried to convert as parameters.

All the parameters DSI/DPHY spec specific and none of them particularly
relate to our hardware.

>
>> Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com>
>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_dsi.c | 9 ++++++++-
>> drivers/gpu/drm/i915/intel_dsi.h | 29 +++++++++++++++++++++++++++++
>> 2 files changed, 37 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
>> index 9a2fdd2..34e19b7 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>> @@ -147,6 +147,9 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
>>
>> DRM_DEBUG_KMS("\n");
>>
>> + if (intel_dsi->dev.dev_ops->panel_reset)
>> + intel_dsi->dev.dev_ops->panel_reset(&intel_dsi->dev);
>
> Would this map to ->pre_enable in drm_bridge?

I have not yet migrated to drm_bridge and need to check thes call flows
for drm_bridge

>
>> +
>> temp = I915_READ(MIPI_DEVICE_READY(pipe));
>> if ((temp & DEVICE_READY) == 0) {
>> temp &= ~ULPS_STATE_MASK;
>> @@ -162,6 +165,9 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
>> I915_WRITE(MIPI_DEVICE_READY(pipe), temp);
>> }
>>
>> + if (intel_dsi->dev.dev_ops->send_otp_cmds)
>> + intel_dsi->dev.dev_ops->send_otp_cmds(&intel_dsi->dev);
>
> What is otp? one time programming? Why not in ->enable?

Yes. OTP is done before sending pixel stream and enable is done after we
start pixel stream

>
>> +
>> if (is_cmd_mode(intel_dsi))
>> I915_WRITE(MIPI_MAX_RETURN_PKT_SIZE(pipe), 8 * 4);
>>
>> @@ -176,7 +182,8 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
>> POSTING_READ(MIPI_PORT_CTRL(pipe));
>> }
>>
>> - intel_dsi->dev.dev_ops->enable(&intel_dsi->dev);
>> + if (intel_dsi->dev.dev_ops->enable)
>> + intel_dsi->dev.dev_ops->enable(&intel_dsi->dev);
>> }
>>
>> static void intel_dsi_disable(struct intel_encoder *encoder)
>> diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
>> index c7765f3..b71c9b3 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi.h
>> +++ b/drivers/gpu/drm/i915/intel_dsi.h
>> @@ -39,6 +39,13 @@ struct intel_dsi_device {
>> struct intel_dsi_dev_ops {
>> bool (*init)(struct intel_dsi_device *dsi);
>>
>> + void (*panel_reset)(struct intel_dsi_device *dsi);
>> +
>> + void (*disable_panel_power)(struct intel_dsi_device *dsi);
>
> What is the enabling counterpart to disable_panel_power? panel_reset?

Yes.

>
>> +
>> + /* send one time programmable commands */
>> + 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);
>>
>> @@ -89,6 +96,28 @@ struct intel_dsi {
>>
>> /* eot for MIPI_EOT_DISABLE register */
>> u32 eot_disable;
>> +
>> + u16 dsi_clock_freq;
>> + u8 video_mode_type;
>> + u32 data_width;
>> + u8 dither;
>> + u32 port_bits;
>> + u8 escape_clk_div;
>> + u32 lp_rx_timeout;
>> + u8 turn_arnd_val;
>> + u16 init_count;
>> + u16 rst_timer_val;
>> + u16 hs_to_lp_count;
>> + u16 lp_byte_clk;
>> + u32 bw_timer;
>> + u16 clk_lp_to_hs_count;
>> + u16 clk_hs_to_lp_count;
>> + u32 video_frmt_cfg_bits;
>> + u32 dphy_reg;
>> +
>> + u8 backlight_off_delay; /*in ms*/
>> + bool send_shutdown;
>> + u8 shutdown_pkt_delay; /*in ms*/
>> };
>>
>> static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder)
>> --
>> 1.7.9.5
>>
>

Regards
Shobhit