Hi Daniel,
Thank you for your inputs. Please read our response below and let us
know your opinion.
- 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.
- 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.
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.
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
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.
- 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.
[Vandana/Pradeep]We will look into this.
Thanks,
Vandana
-----Original Message-----
From: Daniel Vetter [mailto:[email protected]] On Behalf Of Daniel Vetter
Sent: Tuesday, November 26, 2013 11:57 PM
To: Kannan, Vandana
Cc: [email protected]
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
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
