| Message ID | 1383990548-30737-6-git-send-email-shobhit.kumar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, 09 Nov 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote: > Basically ULPS handling during enable/disable has been moved to > pre_enable and post_disable phases. PLL and panel power disable > also has been moved to post_disable phase. The ULPS entry/exit > sequneces as suggested by HW team is as follows - > > During enable time - > set DEVICE_READY --> Clear DEVICE_READY --> set DEVICE_READY > > And during disable time to flush all FIFOs - > set ENTER_SLEEP --> EXIT_SLEEP --> ENTER_SLEEP > > Also during disbale sequnece sub-encoder disable is moved to the end > after port is disabled. > > v2: Based on comments from Ville > - Detailed epxlaination in the commit messgae > - Moved parameter changes out into another patch > - Backlight enabling will be a new patch > > Signed-off-by: Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com> > Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 11 ++++ > drivers/gpu/drm/i915/intel_dsi.c | 111 ++++++++++++++++++++++++++------------ > drivers/gpu/drm/i915/intel_dsi.h | 2 + > 3 files changed, 91 insertions(+), 33 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index a2bbff9..55c16cb 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2433,6 +2433,17 @@ int vlv_freq_opcode(int ddr_freq, int val); > #define POSTING_READ(reg) (void)I915_READ_NOTRACE(reg) > #define POSTING_READ16(reg) (void)I915_READ16_NOTRACE(reg) > > +#define I915_WRITE_BITS(reg, val, mask) \ > +do { \ > + u32 tmp, data; \ > + tmp = I915_READ((reg)); \ > + tmp &= ~(mask); \ > + data = (val) & (mask); \ > + data = data | tmp; \ > + I915_WRITE((reg), data); \ > +} while(0) I would still prefer the explicit read, modify, and write in the code instead of this, but it's a matter of taste I'll leave for Daniel to call the shots on. One reason for my dislike is how easy it will be to accidentally get the val and mask parameters mixed up. I would instinctively put the mask before the value (common convention of context before the rest), which would be wrong here. I am not saying changing the order would make me like this. > + > + > /* "Broadcast RGB" property */ > #define INTEL_BROADCAST_RGB_AUTO 0 > #define INTEL_BROADCAST_RGB_FULL 1 > diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c > index 8dc9a38..9e67f78 100644 > --- a/drivers/gpu/drm/i915/intel_dsi.c > +++ b/drivers/gpu/drm/i915/intel_dsi.c > @@ -101,46 +101,59 @@ static void intel_dsi_pre_pll_enable(struct intel_encoder *encoder) > vlv_enable_dsi_pll(encoder); > } > > -static void intel_dsi_pre_enable(struct intel_encoder *encoder) > -{ > - DRM_DEBUG_KMS("\n"); > -} > - > -static void intel_dsi_enable(struct intel_encoder *encoder) > +void intel_dsi_device_ready(struct intel_encoder *encoder) > { > struct drm_i915_private *dev_priv = encoder->base.dev->dev_private; > struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); > struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base); > int pipe = intel_crtc->pipe; > - u32 temp; > > DRM_DEBUG_KMS("\n"); > > if (intel_dsi->dev.dev_ops->panel_reset) > intel_dsi->dev.dev_ops->panel_reset(&intel_dsi->dev); > > - temp = I915_READ(MIPI_DEVICE_READY(pipe)); > - if ((temp & DEVICE_READY) == 0) { > - temp &= ~ULPS_STATE_MASK; > - I915_WRITE(MIPI_DEVICE_READY(pipe), temp | DEVICE_READY); > - } else if (temp & ULPS_STATE_MASK) { > - temp &= ~ULPS_STATE_MASK; > - I915_WRITE(MIPI_DEVICE_READY(pipe), temp | ULPS_STATE_EXIT); > - /* > - * We need to ensure that there is a minimum of 1 ms time > - * available before clearing the UPLS exit state. > - */ > - msleep(2); > - I915_WRITE(MIPI_DEVICE_READY(pipe), temp); > - } > + I915_WRITE_BITS(MIPI_PORT_CTRL(pipe), LP_OUTPUT_HOLD, LP_OUTPUT_HOLD); > + usleep_range(1000, 1500); > + I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), DEVICE_READY | > + ULPS_STATE_EXIT, DEVICE_READY | ULPS_STATE_MASK); > + usleep_range(2000, 2500); > + I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), DEVICE_READY, > + DEVICE_READY | ULPS_STATE_MASK); > + usleep_range(2000, 2500); > + I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), 0x00, > + DEVICE_READY | ULPS_STATE_MASK); > + usleep_range(2000, 2500); > + I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), DEVICE_READY, > + DEVICE_READY | ULPS_STATE_MASK); It seems like an odd dance, but if that's what the hw folks say, I guess we'll just have to take their word for it... > + usleep_range(2000, 2500); > > if (intel_dsi->dev.dev_ops->send_otp_cmds) > intel_dsi->dev.dev_ops->send_otp_cmds(&intel_dsi->dev); Maybe the hooks would better belong in intel_dsi_pre_enable, not here. The panel_reset hook could be renamed pre_enable while at it. > > +} > +static void intel_dsi_pre_enable(struct intel_encoder *encoder) > +{ > + DRM_DEBUG_KMS("\n"); > + > + /* put device in ready state */ > + intel_dsi_device_ready(encoder); > +} > + > +static void intel_dsi_enable(struct intel_encoder *encoder) > +{ > + struct drm_device *dev = encoder->base.dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); > + struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base); > + int pipe = intel_crtc->pipe; > + u32 temp; > + > + DRM_DEBUG_KMS("\n"); > + > if (is_cmd_mode(intel_dsi)) > I915_WRITE(MIPI_MAX_RETURN_PKT_SIZE(pipe), 8 * 4); > - > - if (is_vid_mode(intel_dsi)) { > + else { > msleep(20); /* XXX */ > dpi_send_cmd(intel_dsi, TURN_ON); > msleep(100); > @@ -157,7 +170,8 @@ static void intel_dsi_enable(struct intel_encoder *encoder) > > static void intel_dsi_disable(struct intel_encoder *encoder) > { > - struct drm_i915_private *dev_priv = encoder->base.dev->dev_private; > + struct drm_device *dev = encoder->base.dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); > struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base); > int pipe = intel_crtc->pipe; > @@ -165,8 +179,6 @@ static void intel_dsi_disable(struct intel_encoder *encoder) > > DRM_DEBUG_KMS("\n"); > > - intel_dsi->dev.dev_ops->disable(&intel_dsi->dev); > - > if (is_vid_mode(intel_dsi)) { > dpi_send_cmd(intel_dsi, SHUTDOWN); > msleep(10); > @@ -179,19 +191,52 @@ static void intel_dsi_disable(struct intel_encoder *encoder) > msleep(2); > } > > - temp = I915_READ(MIPI_DEVICE_READY(pipe)); > - if (temp & DEVICE_READY) { > - temp &= ~DEVICE_READY; > - temp &= ~ULPS_STATE_MASK; > - I915_WRITE(MIPI_DEVICE_READY(pipe), temp); > - } > + /* if disable packets are sent before sending shutdown packet then in > + * some next enable sequence send turn on packet error is observed */ > + if (intel_dsi->dev.dev_ops->disable) > + intel_dsi->dev.dev_ops->disable(&intel_dsi->dev); > } > > -static void intel_dsi_post_disable(struct intel_encoder *encoder) > +void intel_dsi_clear_device_ready(struct intel_encoder *encoder) > { > + struct drm_i915_private *dev_priv = encoder->base.dev->dev_private; > + struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); > + struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base); > + int pipe = intel_crtc->pipe; > + > DRM_DEBUG_KMS("\n"); > > + I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), ULPS_STATE_ENTER, > + ULPS_STATE_MASK); > + usleep_range(2000, 2500); > + > + I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), ULPS_STATE_EXIT, > + ULPS_STATE_MASK); > + usleep_range(2000, 2500); > + > + I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), ULPS_STATE_ENTER, > + ULPS_STATE_MASK); > + usleep_range(2000, 2500); > + > + I915_WRITE_BITS(MIPI_PORT_CTRL(pipe), 0, LP_OUTPUT_HOLD); > + usleep_range(1000, 1500); > + > + if (wait_for(((I915_READ(MIPI_PORT_CTRL(pipe)) & 0x20000) > + == 0x00000), 30)) > + DRM_ERROR("DSI LP not going Low\n"); AFE_LATCHOUT? > + > + I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), 0x00, DEVICE_READY); > + usleep_range(2000, 2500); > + > vlv_disable_dsi_pll(encoder); > + > + if (intel_dsi->dev.dev_ops->disable_panel_power) > + intel_dsi->dev.dev_ops->disable_panel_power(&intel_dsi->dev); Here too, hook better suited at intel_dsi_post_disable. And call it post_disable. > +} > +static void intel_dsi_post_disable(struct intel_encoder *encoder) > +{ > + DRM_DEBUG_KMS("\n"); > + intel_dsi_clear_device_ready(encoder); > } > > static bool intel_dsi_get_hw_state(struct intel_encoder *encoder, > diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h > index 14509d6..387dfe1 100644 > --- a/drivers/gpu/drm/i915/intel_dsi.h > +++ b/drivers/gpu/drm/i915/intel_dsi.h > @@ -41,6 +41,8 @@ struct intel_dsi_dev_ops { > > void (*panel_reset)(struct intel_dsi_device *dsi); > > + void (*disable_panel_power)(struct intel_dsi_device *dsi); > + > /* one time programmable commands if needed */ > void (*send_otp_cmds)(struct intel_dsi_device *dsi); > > -- > 1.7.9.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Nov 15, 2013 at 10:27:25AM +0200, Jani Nikula wrote: > On Sat, 09 Nov 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote: > > Basically ULPS handling during enable/disable has been moved to > > pre_enable and post_disable phases. PLL and panel power disable > > also has been moved to post_disable phase. The ULPS entry/exit > > sequneces as suggested by HW team is as follows - > > > > During enable time - > > set DEVICE_READY --> Clear DEVICE_READY --> set DEVICE_READY > > > > And during disable time to flush all FIFOs - > > set ENTER_SLEEP --> EXIT_SLEEP --> ENTER_SLEEP > > > > Also during disbale sequnece sub-encoder disable is moved to the end > > after port is disabled. > > > > v2: Based on comments from Ville > > - Detailed epxlaination in the commit messgae > > - Moved parameter changes out into another patch > > - Backlight enabling will be a new patch > > > > Signed-off-by: Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com> > > Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.h | 11 ++++ > > drivers/gpu/drm/i915/intel_dsi.c | 111 ++++++++++++++++++++++++++------------ > > drivers/gpu/drm/i915/intel_dsi.h | 2 + > > 3 files changed, 91 insertions(+), 33 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index a2bbff9..55c16cb 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -2433,6 +2433,17 @@ int vlv_freq_opcode(int ddr_freq, int val); > > #define POSTING_READ(reg) (void)I915_READ_NOTRACE(reg) > > #define POSTING_READ16(reg) (void)I915_READ16_NOTRACE(reg) > > > > +#define I915_WRITE_BITS(reg, val, mask) \ > > +do { \ > > + u32 tmp, data; \ > > + tmp = I915_READ((reg)); \ > > + tmp &= ~(mask); \ > > + data = (val) & (mask); \ > > + data = data | tmp; \ > > + I915_WRITE((reg), data); \ > > +} while(0) > > I would still prefer the explicit read, modify, and write in the code > instead of this, but it's a matter of taste I'll leave for Daniel to > call the shots on. Yeah, this looks a bit funny. We could compute the tmp value once (where the mask is mutliple times the same thing) and then just or in the right bits. That should make the I915_WRITE calls fit ont on line, too, which helps readability. Also we put POSTING_READs before any waits to ensure the write has actually landed. It's mostly documentation. And while I'm at it: We generally frown upon readbacks of register value and prefer to just keep track of things in software well enough. The reason for that is that register readbacks allows us too much flexibility in adding subtile state-depencies. Which long-term makes the code a real pain to maintain. -Daniel
On Friday 15 November 2013 02:25 PM, Daniel Vetter wrote: > On Fri, Nov 15, 2013 at 10:27:25AM +0200, Jani Nikula wrote: >> On Sat, 09 Nov 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote: >>> Basically ULPS handling during enable/disable has been moved to >>> pre_enable and post_disable phases. PLL and panel power disable >>> also has been moved to post_disable phase. The ULPS entry/exit >>> sequneces as suggested by HW team is as follows - >>> >>> During enable time - >>> set DEVICE_READY --> Clear DEVICE_READY --> set DEVICE_READY >>> >>> And during disable time to flush all FIFOs - >>> set ENTER_SLEEP --> EXIT_SLEEP --> ENTER_SLEEP >>> >>> Also during disbale sequnece sub-encoder disable is moved to the end >>> after port is disabled. >>> >>> v2: Based on comments from Ville >>> - Detailed epxlaination in the commit messgae >>> - Moved parameter changes out into another patch >>> - Backlight enabling will be a new patch >>> >>> Signed-off-by: Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com> >>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com> >>> --- >>> drivers/gpu/drm/i915/i915_drv.h | 11 ++++ >>> drivers/gpu/drm/i915/intel_dsi.c | 111 ++++++++++++++++++++++++++------------ >>> drivers/gpu/drm/i915/intel_dsi.h | 2 + >>> 3 files changed, 91 insertions(+), 33 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >>> index a2bbff9..55c16cb 100644 >>> --- a/drivers/gpu/drm/i915/i915_drv.h >>> +++ b/drivers/gpu/drm/i915/i915_drv.h >>> @@ -2433,6 +2433,17 @@ int vlv_freq_opcode(int ddr_freq, int val); >>> #define POSTING_READ(reg) (void)I915_READ_NOTRACE(reg) >>> #define POSTING_READ16(reg) (void)I915_READ16_NOTRACE(reg) >>> >>> +#define I915_WRITE_BITS(reg, val, mask) \ >>> +do { \ >>> + u32 tmp, data; \ >>> + tmp = I915_READ((reg)); \ >>> + tmp &= ~(mask); \ >>> + data = (val) & (mask); \ >>> + data = data | tmp; \ >>> + I915_WRITE((reg), data); \ >>> +} while(0) >> >> I would still prefer the explicit read, modify, and write in the code >> instead of this, but it's a matter of taste I'll leave for Daniel to >> call the shots on. > > Yeah, this looks a bit funny. We could compute the tmp value once (where > the mask is mutliple times the same thing) and then just or in the right > bits. That should make the I915_WRITE calls fit ont on line, too, which > helps readability. > > Also we put POSTING_READs before any waits to ensure the write has > actually landed. It's mostly documentation. > > And while I'm at it: We generally frown upon readbacks of register value > and prefer to just keep track of things in software well enough. The > reason for that is that register readbacks allows us too much flexibility > in adding subtile state-depencies. Which long-term makes the code a real > pain to maintain. Ok. Will work on updating the patch accordingly and take care of other comments as well in next patch set update soon. Regards Shobhit
On Wednesday 20 November 2013 07:09 AM, Shobhit Kumar wrote: > On Friday 15 November 2013 02:25 PM, Daniel Vetter wrote: >> On Fri, Nov 15, 2013 at 10:27:25AM +0200, Jani Nikula wrote: >>> On Sat, 09 Nov 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote: >>>> Basically ULPS handling during enable/disable has been moved to >>>> pre_enable and post_disable phases. PLL and panel power disable >>>> also has been moved to post_disable phase. The ULPS entry/exit >>>> sequneces as suggested by HW team is as follows - >>>> >>>> During enable time - >>>> set DEVICE_READY --> Clear DEVICE_READY --> set DEVICE_READY >>>> >>>> And during disable time to flush all FIFOs - >>>> set ENTER_SLEEP --> EXIT_SLEEP --> ENTER_SLEEP >>>> >>>> Also during disbale sequnece sub-encoder disable is moved to the end >>>> after port is disabled. >>>> >>>> v2: Based on comments from Ville >>>> - Detailed epxlaination in the commit messgae >>>> - Moved parameter changes out into another patch >>>> - Backlight enabling will be a new patch >>>> >>>> Signed-off-by: Yogesh Mohan Marimuthu >>>> <yogesh.mohan.marimuthu@intel.com> >>>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/i915_drv.h | 11 ++++ >>>> drivers/gpu/drm/i915/intel_dsi.c | 111 >>>> ++++++++++++++++++++++++++------------ >>>> drivers/gpu/drm/i915/intel_dsi.h | 2 + >>>> 3 files changed, 91 insertions(+), 33 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h >>>> b/drivers/gpu/drm/i915/i915_drv.h >>>> index a2bbff9..55c16cb 100644 >>>> --- a/drivers/gpu/drm/i915/i915_drv.h >>>> +++ b/drivers/gpu/drm/i915/i915_drv.h >>>> @@ -2433,6 +2433,17 @@ int vlv_freq_opcode(int ddr_freq, int val); >>>> #define POSTING_READ(reg) (void)I915_READ_NOTRACE(reg) >>>> #define POSTING_READ16(reg) (void)I915_READ16_NOTRACE(reg) >>>> >>>> +#define I915_WRITE_BITS(reg, val, mask) \ >>>> +do { \ >>>> + u32 tmp, data; \ >>>> + tmp = I915_READ((reg)); \ >>>> + tmp &= ~(mask); \ >>>> + data = (val) & (mask); \ >>>> + data = data | tmp; \ >>>> + I915_WRITE((reg), data); \ >>>> +} while(0) >>> >>> I would still prefer the explicit read, modify, and write in the code >>> instead of this, but it's a matter of taste I'll leave for Daniel to >>> call the shots on. >> >> Yeah, this looks a bit funny. We could compute the tmp value once (where >> the mask is mutliple times the same thing) and then just or in the right >> bits. That should make the I915_WRITE calls fit ont on line, too, which >> helps readability. >> >> Also we put POSTING_READs before any waits to ensure the write has >> actually landed. It's mostly documentation. >> >> And while I'm at it: We generally frown upon readbacks of register value >> and prefer to just keep track of things in software well enough. The >> reason for that is that register readbacks allows us too much flexibility >> in adding subtile state-depencies. Which long-term makes the code a real >> pain to maintain. > > Ok. Will work on updating the patch accordingly and take care of other > comments as well in next patch set update soon. Sorry took me more time than I anticipated to rework on this due to other critical stuff. Looking at the code and doing more testing I now confirmed that there is no READ/Modify/WRITE needed for ULPS and hence I will convert I915_WRITE_BITS to normal I915_WRITE. Will be sending updated patches on Monday Regards Shobhit
On Friday 15 November 2013 01:57 PM, Jani Nikula wrote: > On Sat, 09 Nov 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote: >> Basically ULPS handling during enable/disable has been moved to >> pre_enable and post_disable phases. PLL and panel power disable >> also has been moved to post_disable phase. The ULPS entry/exit >> sequneces as suggested by HW team is as follows - >> >> During enable time - >> set DEVICE_READY --> Clear DEVICE_READY --> set DEVICE_READY >> >> And during disable time to flush all FIFOs - >> set ENTER_SLEEP --> EXIT_SLEEP --> ENTER_SLEEP >> >> Also during disbale sequnece sub-encoder disable is moved to the end >> after port is disabled. >> >> v2: Based on comments from Ville >> - Detailed epxlaination in the commit messgae >> - Moved parameter changes out into another patch >> - Backlight enabling will be a new patch >> >> Signed-off-by: Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com> >> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.h | 11 ++++ >> drivers/gpu/drm/i915/intel_dsi.c | 111 ++++++++++++++++++++++++++------------ >> drivers/gpu/drm/i915/intel_dsi.h | 2 + >> 3 files changed, 91 insertions(+), 33 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index a2bbff9..55c16cb 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -2433,6 +2433,17 @@ int vlv_freq_opcode(int ddr_freq, int val); >> #define POSTING_READ(reg) (void)I915_READ_NOTRACE(reg) >> #define POSTING_READ16(reg) (void)I915_READ16_NOTRACE(reg) >> >> +#define I915_WRITE_BITS(reg, val, mask) \ >> +do { \ >> + u32 tmp, data; \ >> + tmp = I915_READ((reg)); \ >> + tmp &= ~(mask); \ >> + data = (val) & (mask); \ >> + data = data | tmp; \ >> + I915_WRITE((reg), data); \ >> +} while(0) > > I would still prefer the explicit read, modify, and write in the code > instead of this, but it's a matter of taste I'll leave for Daniel to > call the shots on. > > One reason for my dislike is how easy it will be to accidentally get the > val and mask parameters mixed up. I would instinctively put the mask > before the value (common convention of context before the rest), which > would be wrong here. I am not saying changing the order would make me > like this. As mentioned in another mail, this is not required and I will remove it > >> + >> + >> /* "Broadcast RGB" property */ >> #define INTEL_BROADCAST_RGB_AUTO 0 >> #define INTEL_BROADCAST_RGB_FULL 1 >> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c >> index 8dc9a38..9e67f78 100644 >> --- a/drivers/gpu/drm/i915/intel_dsi.c >> +++ b/drivers/gpu/drm/i915/intel_dsi.c >> @@ -101,46 +101,59 @@ static void intel_dsi_pre_pll_enable(struct intel_encoder *encoder) >> vlv_enable_dsi_pll(encoder); >> } >> >> -static void intel_dsi_pre_enable(struct intel_encoder *encoder) >> -{ >> - DRM_DEBUG_KMS("\n"); >> -} >> - >> -static void intel_dsi_enable(struct intel_encoder *encoder) >> +void intel_dsi_device_ready(struct intel_encoder *encoder) >> { >> struct drm_i915_private *dev_priv = encoder->base.dev->dev_private; >> struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); >> struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base); >> int pipe = intel_crtc->pipe; >> - u32 temp; >> >> DRM_DEBUG_KMS("\n"); >> >> if (intel_dsi->dev.dev_ops->panel_reset) >> intel_dsi->dev.dev_ops->panel_reset(&intel_dsi->dev); >> >> - temp = I915_READ(MIPI_DEVICE_READY(pipe)); >> - if ((temp & DEVICE_READY) == 0) { >> - temp &= ~ULPS_STATE_MASK; >> - I915_WRITE(MIPI_DEVICE_READY(pipe), temp | DEVICE_READY); >> - } else if (temp & ULPS_STATE_MASK) { >> - temp &= ~ULPS_STATE_MASK; >> - I915_WRITE(MIPI_DEVICE_READY(pipe), temp | ULPS_STATE_EXIT); >> - /* >> - * We need to ensure that there is a minimum of 1 ms time >> - * available before clearing the UPLS exit state. >> - */ >> - msleep(2); >> - I915_WRITE(MIPI_DEVICE_READY(pipe), temp); >> - } >> + I915_WRITE_BITS(MIPI_PORT_CTRL(pipe), LP_OUTPUT_HOLD, LP_OUTPUT_HOLD); >> + usleep_range(1000, 1500); >> + I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), DEVICE_READY | >> + ULPS_STATE_EXIT, DEVICE_READY | ULPS_STATE_MASK); >> + usleep_range(2000, 2500); >> + I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), DEVICE_READY, >> + DEVICE_READY | ULPS_STATE_MASK); >> + usleep_range(2000, 2500); >> + I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), 0x00, >> + DEVICE_READY | ULPS_STATE_MASK); >> + usleep_range(2000, 2500); >> + I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), DEVICE_READY, >> + DEVICE_READY | ULPS_STATE_MASK); > > It seems like an odd dance, but if that's what the hw folks say, I guess > we'll just have to take their word for it... Yeah, but I reconfirmed from HW team and this is also now updated in documents > >> + usleep_range(2000, 2500); >> >> if (intel_dsi->dev.dev_ops->send_otp_cmds) >> intel_dsi->dev.dev_ops->send_otp_cmds(&intel_dsi->dev); > > Maybe the hooks would better belong in intel_dsi_pre_enable, not > here. The panel_reset hook could be renamed pre_enable while at it. > Ok, moved the hooks into pre_enable, but I am not too sure on changing the names as the current name suggests what exactly we are doing with the panel. If you agree I will leave that bit out but do other changes. >> >> +} >> +static void intel_dsi_pre_enable(struct intel_encoder *encoder) >> +{ >> + DRM_DEBUG_KMS("\n"); >> + >> + /* put device in ready state */ >> + intel_dsi_device_ready(encoder); >> +} >> + >> +static void intel_dsi_enable(struct intel_encoder *encoder) >> +{ >> + struct drm_device *dev = encoder->base.dev; >> + struct drm_i915_private *dev_priv = dev->dev_private; >> + struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); >> + struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base); >> + int pipe = intel_crtc->pipe; >> + u32 temp; >> + >> + DRM_DEBUG_KMS("\n"); >> + >> if (is_cmd_mode(intel_dsi)) >> I915_WRITE(MIPI_MAX_RETURN_PKT_SIZE(pipe), 8 * 4); >> - >> - if (is_vid_mode(intel_dsi)) { >> + else { >> msleep(20); /* XXX */ >> dpi_send_cmd(intel_dsi, TURN_ON); >> msleep(100); >> @@ -157,7 +170,8 @@ static void intel_dsi_enable(struct intel_encoder *encoder) >> >> static void intel_dsi_disable(struct intel_encoder *encoder) >> { >> - struct drm_i915_private *dev_priv = encoder->base.dev->dev_private; >> + struct drm_device *dev = encoder->base.dev; >> + struct drm_i915_private *dev_priv = dev->dev_private; >> struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); >> struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base); >> int pipe = intel_crtc->pipe; >> @@ -165,8 +179,6 @@ static void intel_dsi_disable(struct intel_encoder *encoder) >> >> DRM_DEBUG_KMS("\n"); >> >> - intel_dsi->dev.dev_ops->disable(&intel_dsi->dev); >> - >> if (is_vid_mode(intel_dsi)) { >> dpi_send_cmd(intel_dsi, SHUTDOWN); >> msleep(10); >> @@ -179,19 +191,52 @@ static void intel_dsi_disable(struct intel_encoder *encoder) >> msleep(2); >> } >> >> - temp = I915_READ(MIPI_DEVICE_READY(pipe)); >> - if (temp & DEVICE_READY) { >> - temp &= ~DEVICE_READY; >> - temp &= ~ULPS_STATE_MASK; >> - I915_WRITE(MIPI_DEVICE_READY(pipe), temp); >> - } >> + /* if disable packets are sent before sending shutdown packet then in >> + * some next enable sequence send turn on packet error is observed */ >> + if (intel_dsi->dev.dev_ops->disable) >> + intel_dsi->dev.dev_ops->disable(&intel_dsi->dev); >> } >> >> -static void intel_dsi_post_disable(struct intel_encoder *encoder) >> +void intel_dsi_clear_device_ready(struct intel_encoder *encoder) >> { >> + struct drm_i915_private *dev_priv = encoder->base.dev->dev_private; >> + struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); >> + struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base); >> + int pipe = intel_crtc->pipe; >> + >> DRM_DEBUG_KMS("\n"); >> >> + I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), ULPS_STATE_ENTER, >> + ULPS_STATE_MASK); >> + usleep_range(2000, 2500); >> + >> + I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), ULPS_STATE_EXIT, >> + ULPS_STATE_MASK); >> + usleep_range(2000, 2500); >> + >> + I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), ULPS_STATE_ENTER, >> + ULPS_STATE_MASK); >> + usleep_range(2000, 2500); >> + >> + I915_WRITE_BITS(MIPI_PORT_CTRL(pipe), 0, LP_OUTPUT_HOLD); >> + usleep_range(1000, 1500); >> + >> + if (wait_for(((I915_READ(MIPI_PORT_CTRL(pipe)) & 0x20000) >> + == 0x00000), 30)) >> + DRM_ERROR("DSI LP not going Low\n"); > > AFE_LATCHOUT? Yeah, fixed it. Rap on me hardcodings :) > >> + >> + I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), 0x00, DEVICE_READY); >> + usleep_range(2000, 2500); >> + >> vlv_disable_dsi_pll(encoder); >> + >> + if (intel_dsi->dev.dev_ops->disable_panel_power) >> + intel_dsi->dev.dev_ops->disable_panel_power(&intel_dsi->dev); > > Here too, hook better suited at intel_dsi_post_disable. And call it > post_disable. Will move the hook but not too sure on changing the name as above You can expect updated patches by Monday. Sorry it took me some time to get back to this. Regards Shobiht
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index a2bbff9..55c16cb 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2433,6 +2433,17 @@ int vlv_freq_opcode(int ddr_freq, int val); #define POSTING_READ(reg) (void)I915_READ_NOTRACE(reg) #define POSTING_READ16(reg) (void)I915_READ16_NOTRACE(reg) +#define I915_WRITE_BITS(reg, val, mask) \ +do { \ + u32 tmp, data; \ + tmp = I915_READ((reg)); \ + tmp &= ~(mask); \ + data = (val) & (mask); \ + data = data | tmp; \ + I915_WRITE((reg), data); \ +} while(0) + + /* "Broadcast RGB" property */ #define INTEL_BROADCAST_RGB_AUTO 0 #define INTEL_BROADCAST_RGB_FULL 1 diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c index 8dc9a38..9e67f78 100644 --- a/drivers/gpu/drm/i915/intel_dsi.c +++ b/drivers/gpu/drm/i915/intel_dsi.c @@ -101,46 +101,59 @@ static void intel_dsi_pre_pll_enable(struct intel_encoder *encoder) vlv_enable_dsi_pll(encoder); } -static void intel_dsi_pre_enable(struct intel_encoder *encoder) -{ - DRM_DEBUG_KMS("\n"); -} - -static void intel_dsi_enable(struct intel_encoder *encoder) +void intel_dsi_device_ready(struct intel_encoder *encoder) { struct drm_i915_private *dev_priv = encoder->base.dev->dev_private; struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base); int pipe = intel_crtc->pipe; - u32 temp; DRM_DEBUG_KMS("\n"); if (intel_dsi->dev.dev_ops->panel_reset) intel_dsi->dev.dev_ops->panel_reset(&intel_dsi->dev); - temp = I915_READ(MIPI_DEVICE_READY(pipe)); - if ((temp & DEVICE_READY) == 0) { - temp &= ~ULPS_STATE_MASK; - I915_WRITE(MIPI_DEVICE_READY(pipe), temp | DEVICE_READY); - } else if (temp & ULPS_STATE_MASK) { - temp &= ~ULPS_STATE_MASK; - I915_WRITE(MIPI_DEVICE_READY(pipe), temp | ULPS_STATE_EXIT); - /* - * We need to ensure that there is a minimum of 1 ms time - * available before clearing the UPLS exit state. - */ - msleep(2); - I915_WRITE(MIPI_DEVICE_READY(pipe), temp); - } + I915_WRITE_BITS(MIPI_PORT_CTRL(pipe), LP_OUTPUT_HOLD, LP_OUTPUT_HOLD); + usleep_range(1000, 1500); + I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), DEVICE_READY | + ULPS_STATE_EXIT, DEVICE_READY | ULPS_STATE_MASK); + usleep_range(2000, 2500); + I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), DEVICE_READY, + DEVICE_READY | ULPS_STATE_MASK); + usleep_range(2000, 2500); + I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), 0x00, + DEVICE_READY | ULPS_STATE_MASK); + usleep_range(2000, 2500); + I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), DEVICE_READY, + DEVICE_READY | ULPS_STATE_MASK); + usleep_range(2000, 2500); if (intel_dsi->dev.dev_ops->send_otp_cmds) intel_dsi->dev.dev_ops->send_otp_cmds(&intel_dsi->dev); +} +static void intel_dsi_pre_enable(struct intel_encoder *encoder) +{ + DRM_DEBUG_KMS("\n"); + + /* put device in ready state */ + intel_dsi_device_ready(encoder); +} + +static void intel_dsi_enable(struct intel_encoder *encoder) +{ + struct drm_device *dev = encoder->base.dev; + struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); + struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base); + int pipe = intel_crtc->pipe; + u32 temp; + + DRM_DEBUG_KMS("\n"); + if (is_cmd_mode(intel_dsi)) I915_WRITE(MIPI_MAX_RETURN_PKT_SIZE(pipe), 8 * 4); - - if (is_vid_mode(intel_dsi)) { + else { msleep(20); /* XXX */ dpi_send_cmd(intel_dsi, TURN_ON); msleep(100); @@ -157,7 +170,8 @@ static void intel_dsi_enable(struct intel_encoder *encoder) static void intel_dsi_disable(struct intel_encoder *encoder) { - struct drm_i915_private *dev_priv = encoder->base.dev->dev_private; + struct drm_device *dev = encoder->base.dev; + struct drm_i915_private *dev_priv = dev->dev_private; struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base); int pipe = intel_crtc->pipe; @@ -165,8 +179,6 @@ static void intel_dsi_disable(struct intel_encoder *encoder) DRM_DEBUG_KMS("\n"); - intel_dsi->dev.dev_ops->disable(&intel_dsi->dev); - if (is_vid_mode(intel_dsi)) { dpi_send_cmd(intel_dsi, SHUTDOWN); msleep(10); @@ -179,19 +191,52 @@ static void intel_dsi_disable(struct intel_encoder *encoder) msleep(2); } - temp = I915_READ(MIPI_DEVICE_READY(pipe)); - if (temp & DEVICE_READY) { - temp &= ~DEVICE_READY; - temp &= ~ULPS_STATE_MASK; - I915_WRITE(MIPI_DEVICE_READY(pipe), temp); - } + /* if disable packets are sent before sending shutdown packet then in + * some next enable sequence send turn on packet error is observed */ + if (intel_dsi->dev.dev_ops->disable) + intel_dsi->dev.dev_ops->disable(&intel_dsi->dev); } -static void intel_dsi_post_disable(struct intel_encoder *encoder) +void intel_dsi_clear_device_ready(struct intel_encoder *encoder) { + struct drm_i915_private *dev_priv = encoder->base.dev->dev_private; + struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); + struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base); + int pipe = intel_crtc->pipe; + DRM_DEBUG_KMS("\n"); + I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), ULPS_STATE_ENTER, + ULPS_STATE_MASK); + usleep_range(2000, 2500); + + I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), ULPS_STATE_EXIT, + ULPS_STATE_MASK); + usleep_range(2000, 2500); + + I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), ULPS_STATE_ENTER, + ULPS_STATE_MASK); + usleep_range(2000, 2500); + + I915_WRITE_BITS(MIPI_PORT_CTRL(pipe), 0, LP_OUTPUT_HOLD); + usleep_range(1000, 1500); + + if (wait_for(((I915_READ(MIPI_PORT_CTRL(pipe)) & 0x20000) + == 0x00000), 30)) + DRM_ERROR("DSI LP not going Low\n"); + + I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), 0x00, DEVICE_READY); + usleep_range(2000, 2500); + vlv_disable_dsi_pll(encoder); + + if (intel_dsi->dev.dev_ops->disable_panel_power) + intel_dsi->dev.dev_ops->disable_panel_power(&intel_dsi->dev); +} +static void intel_dsi_post_disable(struct intel_encoder *encoder) +{ + DRM_DEBUG_KMS("\n"); + intel_dsi_clear_device_ready(encoder); } static bool intel_dsi_get_hw_state(struct intel_encoder *encoder, diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h index 14509d6..387dfe1 100644 --- a/drivers/gpu/drm/i915/intel_dsi.h +++ b/drivers/gpu/drm/i915/intel_dsi.h @@ -41,6 +41,8 @@ struct intel_dsi_dev_ops { void (*panel_reset)(struct intel_dsi_device *dsi); + void (*disable_panel_power)(struct intel_dsi_device *dsi); + /* one time programmable commands if needed */ void (*send_otp_cmds)(struct intel_dsi_device *dsi);
Basically ULPS handling during enable/disable has been moved to pre_enable and post_disable phases. PLL and panel power disable also has been moved to post_disable phase. The ULPS entry/exit sequneces as suggested by HW team is as follows - During enable time - set DEVICE_READY --> Clear DEVICE_READY --> set DEVICE_READY And during disable time to flush all FIFOs - set ENTER_SLEEP --> EXIT_SLEEP --> ENTER_SLEEP Also during disbale sequnece sub-encoder disable is moved to the end after port is disabled. v2: Based on comments from Ville - Detailed epxlaination in the commit messgae - Moved parameter changes out into another patch - Backlight enabling will be a new patch Signed-off-by: Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 11 ++++ drivers/gpu/drm/i915/intel_dsi.c | 111 ++++++++++++++++++++++++++------------ drivers/gpu/drm/i915/intel_dsi.h | 2 + 3 files changed, 91 insertions(+), 33 deletions(-)