Re: [Intel-gfx] [PATCH 0/6] Enabling DRRS support in the kernel
On Thu, 28 Nov 2013 14:14:09 +0100 Daniel Vetter dan...@ffwll.ch wrote: [Vandana/Pradeep] This patch series is for seamless DRRS support and has to be a separate path from mode set as we just toggle the PIPE_CONF_RR_SWTICH for seamless RR transition without flicker. Complete modeset using setCrtc will result in partial blanking or flicker. We are not implementing fast boot as part of this series. Most eDP panels today which support multiple refresh rates, support seamless DRRS. This means that hardware capability is made use of by toggling a bit in pipe config to switch between refresh rates on the fly. The patch series is aimed to take advantage of this capability to switch refresh rates based on idleness or video playback requirement. Hence, we chose the connector attribute/ set property path. Also in order to support the Media refresh rates of 50/48 we thought it incorrect to populate those modes in probed_mode list as those values are not from the EDID. So in order for User space to know the media refresh rates supported we have exposed a set_property interface which makes User space aware of these media refresh rates. Also we do not intend to do a complete mode set on media use cases in which case we think using setCrtc is not needed. Using setCrtc will invariably result in intermittant blanking or flicker which cannot be avoided, and gives undesirable effect during playback. Seamless DRRS doesn't need a complete modeset and hence no flicker or temporary blanking. Changing the toggle RR bit in PIPE_CONF is faster. Also since BDW has only single M/N/TU register the current code takes care of it easily. Kindly let us know your opinion. Since Seamless DRRS for eDP is separate path from modeset we can keep the connector property attribute as it is. First the technical stuff: - Adding the additional modes to the mode list is imo totally ok and as the design intents. We are already adding other modes from the vbt here if those are available. The only restriction is that the kernel may not add modes which don't work. But as long as the vbt tells us that 48Hz will work we can (and imo should) use it. - Second changing the frequency changes the refresh rate. Atm both our userspace and the kernel support code assume that this can only be done through a SetCrtc call. So adjusting the dotclock is actually the right interface and we don't need any additional property. Now the more political stuff: - We have the long-term goal of solid fastboot support. We've the designed we've picked that actually means 95% effort to get flicker-free SetCrtc going and 5% actual code for fastboot. We're not there yet, there's only a preview available which is disabled by default and uses a few hacks. But generally all features that are relevant need to use the new infrastructure and help move things forward. - Upstream has a time horizon of 5-10 years for the kernel/userspace interface. Which means quick hacks are not allowed. And in my opinion your property is a quick hack, which nicely simplified the implementation. So in short I know that my request extends the scope of your patches. But upstream also imposes differing constraints than a product tree. It should be possible to still merge the automatic, in-kernel only DRRS bits though, right? Then work on making dotclock/refresh changes from mode sets flicker free (shouldn't be a massive amount of work). -- Jesse Barnes, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/6] Enabling DRRS support in the kernel
into this. Thanks, Vandana -Original Message- From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel Vetter Sent: Tuesday, November 26, 2013 11:57 PM To: Kannan, Vandana Cc: intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH 0/6] Enabling DRRS support in the kernel On Tue, Nov 19, 2013 at 11:36:58AM +0530, Vandana Kannan wrote: Dynamic Refresh Rate Switching (DRRS) is a power conservation feature which enables switching between low and high refresh rates based on the usage scenario. This feature is applicable for internal eDP panel. Indication that the panel can support DRRS is given by the panel EDID, which would list multiple refresh rates for one resolution. DRRS is of 2 types - Static DRRS involves execution of the entire mode set sequence to switch between refresh rate. seamless DRRS involves refresh rate switching during runtime without any blanking effect/mode set. The vendor fills in a VBT field indicating static/seamless DRRS based on the panel spec. This information is needed to enable seamless DRRS in kernel. The patch series supports idleness detection in display i915 driver and switch to low refresh rate. It also provides set_property API for user space to request for different refresh rates for active use cases like video playback at 48/50 Hz. Pradeep Bhat (5): drm/i915: Adding VBT fields to support eDP DRRS feature drm/i915: Parse EDID probed modes for DRRS support drm/i915: Add support for DRRS set property to switch RR drm/i915: Support to read DMRRS field from VBT structure drm/i915: Adding support for DMRRS for media playback Vandana Kannan (1): drm/i915: Idleness detection for DRRS My apologies for delaying my high-level maintainer review for so long - there's been a bit a firedrill around here so it took a while to write it all down. Overall a nice patch series, but I think we need to shuffle a few things around to better align with some of the longer-term driver architecture reworks and goals: - You add another copypaste of the code to deduce the reduced clock from the edid to intel_dp.c. Imo it's better to add that to intel_panel.c to struct intel_panel - we already track the panel fixed mode in there, so this would naturally fit. - Imo we should track the reduced clock in the pipe config and also cross-check it in the state checker. That will lead to a bit of fun on bdw, so we need to special case the checker there so that it only checks that the clock matches either of the possible clocks. For this we need a bool downclock_available in struct intel_crtc_config and a 2nd set of m_n values, both set in the compute_config function for DP outputs. For consistency it'd be nice to at least move the downclock_available logic for lvds also over to that. But since we first need to clean up the dpll handling to make lvds downclocking sane that's imo not really a priority at all. The entire point behind all this pipe state tracking is to make sure we don't miss anything when fastbooting. - The connector attribute is imo the wrong interface - userspace already supplies a mode attribute with dotclock to SetCrtc. Imo we simply need to fix up our fixed_mode logic (preferrably as a neat helper in intel_panel.c) to select the right one of the availabel downclocks, even when upscaling. The downside for now is that this will result in flickering. But we need a real flicker-free fast-update-path in our modeset code anyway to make fastboot happen for real. And a few other cool things. I'll increase the pain a bit in the hope that this moves forward again, so no quick hack please (even if it's very simple to do in the case of drrs). - Finally a quick testcase which iterates through all the downclock modes in kms_flip - together with the cross-checking and all the vblank timing tests we already have that should give us solid test coverage. To keep test runtimes reasonable I think just a pageflipping test with time checking is more than enough. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/6] Enabling DRRS support in the kernel
On Thu, Nov 28, 2013 at 10:22:59AM +, Kannan, Vandana wrote: Hi Daniel, Thank you for your inputs. Please read our response below and let us know your opinion. Can you please switch to a mail client that does proper quoting? It makes reading your response a bit hard - I've fixed it up here quickly. - You add another copypaste of the code to deduce the reduced clock from the edid to intel_dp.c. Imo it's better to add that to intel_panel.c to struct intel_panel - we already track the panel fixed mode in there, so this would naturally fit. [Pradeep/Vandana] We will work to move the piece of code to deduce reduced clock from EDID to intel_panel.c. However, we will be making this change only for eDP at this point in time, and will work to rearrange the same for LVDS a little later. On my quick readthrough of your code it looked like it's identical code, so two patches (first to extract the code from lvds, 2nd to use in in the dp code) look like the sane choice. Just moving the place around where you add the new copy doesn't help that much. - Imo we should track the reduced clock in the pipe config and also cross-check it in the state checker. That will lead to a bit of fun on bdw, so we need to special case the checker there so that it only checks that the clock matches either of the possible clocks. For this we need a bool downclock_available in struct intel_crtc_config and a 2nd set of m_n values, both set in the compute_config function for DP outputs. [Vandana/Pradeep] Computing the 2nd set of M_N values in compute_config() for the lowest refresh rate available can done. But in case of the use case of media playback at 50/48 Hz, which we have kept in mind during design/implementation, would mean that M_N values have to be computed initially and kept for all different refresh rates possible. The current implementation gives the flexibility of computing M_N as and when required by user space. If it is ok to keep an array of pre computed M_N values for different Refresh rates computed at intel_dp_compute_config then we could do this. Kindly let us know your opinion. Imo the pipe config should only contain the low/normal frequencies for automatic switching. After all the switching should be transparent to userspace since we're never in the lowfreq mode when there's activity. For consistency it'd be nice to at least move the downclock_available logic for lvds also over to that. But since we first need to clean up the dpll handling to make lvds downclocking sane that's imo not really a priority at all. [Vandana/Pradeep]As mentioned above we will consider this on lower priority and not as part of this patch series. Yeah, like I've said this is out of scope. Switching the downclock_avail bool though should simplify things a bit, so I'd include that. The entire point behind all this pipe state tracking is to make sure we don't miss anything when fastbooting. - The connector attribute is imo the wrong interface - userspace already supplies a mode attribute with dotclock to SetCrtc. Imo we simply need to fix up our fixed_mode logic (preferrably as a neat helper in intel_panel.c) to select the right one of the availabel downclocks, even when upscaling. The downside for now is that this will result in flickering. But we need a real flicker-free fast-update-path in our modeset code anyway to make fastboot happen for real. And a few other cool things. I'll increase the pain a bit in the hope that this moves forward again, so no quick hack please (even if it's very simple to do in the case of drrs). [Vandana/Pradeep] This patch series is for seamless DRRS support and has to be a separate path from mode set as we just toggle the PIPE_CONF_RR_SWTICH for seamless RR transition without flicker. Complete modeset using setCrtc will result in partial blanking or flicker. We are not implementing fast boot as part of this series. Most eDP panels today which support multiple refresh rates, support seamless DRRS. This means that hardware capability is made use of by toggling a bit in pipe config to switch between refresh rates on the fly. The patch series is aimed to take advantage of this capability to switch refresh rates based on idleness or video playback requirement. Hence, we chose the connector attribute/ set property path. Also in order to support the Media refresh rates of 50/48 we thought it incorrect to populate those modes in probed_mode list as those values are not from the EDID. So in order for User space to know the media refresh rates supported we have exposed a set_property interface which makes User space aware of these media refresh rates. Also we do not intend to do a complete mode set
Re: [Intel-gfx] [PATCH 0/6] Enabling DRRS support in the kernel
On Tue, Nov 19, 2013 at 11:36:58AM +0530, Vandana Kannan wrote: Dynamic Refresh Rate Switching (DRRS) is a power conservation feature which enables switching between low and high refresh rates based on the usage scenario. This feature is applicable for internal eDP panel. Indication that the panel can support DRRS is given by the panel EDID, which would list multiple refresh rates for one resolution. DRRS is of 2 types - Static DRRS involves execution of the entire mode set sequence to switch between refresh rate. seamless DRRS involves refresh rate switching during runtime without any blanking effect/mode set. The vendor fills in a VBT field indicating static/seamless DRRS based on the panel spec. This information is needed to enable seamless DRRS in kernel. The patch series supports idleness detection in display i915 driver and switch to low refresh rate. It also provides set_property API for user space to request for different refresh rates for active use cases like video playback at 48/50 Hz. Pradeep Bhat (5): drm/i915: Adding VBT fields to support eDP DRRS feature drm/i915: Parse EDID probed modes for DRRS support drm/i915: Add support for DRRS set property to switch RR drm/i915: Support to read DMRRS field from VBT structure drm/i915: Adding support for DMRRS for media playback Vandana Kannan (1): drm/i915: Idleness detection for DRRS My apologies for delaying my high-level maintainer review for so long - there's been a bit a firedrill around here so it took a while to write it all down. Overall a nice patch series, but I think we need to shuffle a few things around to better align with some of the longer-term driver architecture reworks and goals: - You add another copypaste of the code to deduce the reduced clock from the edid to intel_dp.c. Imo it's better to add that to intel_panel.c to struct intel_panel - we already track the panel fixed mode in there, so this would naturally fit. - Imo we should track the reduced clock in the pipe config and also cross-check it in the state checker. That will lead to a bit of fun on bdw, so we need to special case the checker there so that it only checks that the clock matches either of the possible clocks. For this we need a bool downclock_available in struct intel_crtc_config and a 2nd set of m_n values, both set in the compute_config function for DP outputs. For consistency it'd be nice to at least move the downclock_available logic for lvds also over to that. But since we first need to clean up the dpll handling to make lvds downclocking sane that's imo not really a priority at all. The entire point behind all this pipe state tracking is to make sure we don't miss anything when fastbooting. - The connector attribute is imo the wrong interface - userspace already supplies a mode attribute with dotclock to SetCrtc. Imo we simply need to fix up our fixed_mode logic (preferrably as a neat helper in intel_panel.c) to select the right one of the availabel downclocks, even when upscaling. The downside for now is that this will result in flickering. But we need a real flicker-free fast-update-path in our modeset code anyway to make fastboot happen for real. And a few other cool things. I'll increase the pain a bit in the hope that this moves forward again, so no quick hack please (even if it's very simple to do in the case of drrs). - Finally a quick testcase which iterates through all the downclock modes in kms_flip - together with the cross-checking and all the vblank timing tests we already have that should give us solid test coverage. To keep test runtimes reasonable I think just a pageflipping test with time checking is more than enough. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 0/6] Enabling DRRS support in the kernel
Dynamic Refresh Rate Switching (DRRS) is a power conservation feature which enables switching between low and high refresh rates based on the usage scenario. This feature is applicable for internal eDP panel. Indication that the panel can support DRRS is given by the panel EDID, which would list multiple refresh rates for one resolution. DRRS is of 2 types - Static DRRS involves execution of the entire mode set sequence to switch between refresh rate. seamless DRRS involves refresh rate switching during runtime without any blanking effect/mode set. The vendor fills in a VBT field indicating static/seamless DRRS based on the panel spec. This information is needed to enable seamless DRRS in kernel. The patch series supports idleness detection in display i915 driver and switch to low refresh rate. It also provides set_property API for user space to request for different refresh rates for active use cases like video playback at 48/50 Hz. Pradeep Bhat (5): drm/i915: Adding VBT fields to support eDP DRRS feature drm/i915: Parse EDID probed modes for DRRS support drm/i915: Add support for DRRS set property to switch RR drm/i915: Support to read DMRRS field from VBT structure drm/i915: Adding support for DMRRS for media playback Vandana Kannan (1): drm/i915: Idleness detection for DRRS drivers/gpu/drm/i915/i915_drv.h | 30 drivers/gpu/drm/i915/i915_reg.h |1 + drivers/gpu/drm/i915/intel_bios.c| 25 +++ drivers/gpu/drm/i915/intel_bios.h| 31 drivers/gpu/drm/i915/intel_display.c | 13 ++ drivers/gpu/drm/i915/intel_dp.c | 296 ++ drivers/gpu/drm/i915/intel_drv.h | 48 ++ drivers/gpu/drm/i915/intel_pm.c | 113 + drivers/gpu/drm/i915/intel_sprite.c |3 + 9 files changed, 560 insertions(+) -- 1.7.9.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
