Re: [PATCH 2/5] drm/i915: Parse EDID probed modes for DRRS support

[Vandana Kannan <vandana.kannan@intel.com>]

On Dec-18-2013 2:36 PM, Chris Wilson wrote:
> On Wed, Dec 18, 2013 at 01:41:21PM +0530, Vandana Kannan wrote:
>> On Dec-17-2013 5:58 PM, Chris Wilson wrote:
>>> On Tue, Dec 17, 2013 at 10:58:24AM +0530, Vandana Kannan wrote:
>>>> From: Pradeep Bhat <pradeep.bhat@intel.com>
>>>>
>>>> This patch and finds out the lowest refresh rate supported for the resolution
>>>> same as the fixed_mode, based on the implementaion find_panel_downclock.
>>>> It also checks the VBT fields to see if panel supports seamless DRRS or not.
>>>> Based on above data it marks whether eDP panel supports seamless DRRS or not.
>>>> This information is needed for supporting seamless DRRS switch for
>>>> certain power saving usecases. This patch is tested by enabling the DRM logs
>>>> and user should see whether Seamless DRRS is supported or not.
>>>>
>>>> Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
>>>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/i915_drv.h  |    2 ++
>>>>  drivers/gpu/drm/i915/intel_dp.c  |   47 ++++++++++++++++++++++++++++++++++++++
>>>>  drivers/gpu/drm/i915/intel_drv.h |   29 +++++++++++++++++++++++
>>>>  3 files changed, 78 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>>> index 02e11dc..c9bca16 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>> @@ -1462,8 +1462,10 @@ typedef struct drm_i915_private {
>>>>  	/* Reclocking support */
>>>>  	bool render_reclock_avail;
>>>>  	bool lvds_downclock_avail;
>>>> +	bool edp_downclock_avail;
>>>>  	/* indicates the reduced downclock for LVDS*/
>>>>  	int lvds_downclock;
>>>> +	int edp_downclock;
>>>>  	u16 orig_clock;
>>>
>>> Do any machines have both edp and lvds? Shouldn't this be a part of the
>>> panel state?
>>>
>> If there is a machine having both edp and lvds, then edp takes higher
>> priority. edp_downclock_avail and edp_downclock were added here
>> following the existing code having lvds_downclock_avail and
>> lvds_downclock here. If required, edp_downclock_avail and edp_downclock
>> can be moved to intel_panel structure. Kindly let us know.
> 
> And we can consolidate both into intel_panel.
>  
For this patch series, I will move edp_downclock_avail and edp_downclock
to intel_panel, and following that, work on a separate patch to move
lvds_downclock_avail and lvds_downclock to intel_panel.
>>>>  
>>>> +/**
>>>> + * This enum is used to indicate the DRRS support type.
>>>> + * The values of the enum map 1-to-1 with the values from VBT.
>>>> + */
>>>> +enum edp_panel_type {
>>>> +	DRRS_NOT_SUPPORTED = -1,
>>>> +	STATIC_DRRS_SUPPORT = 0,
>>>> +	SEAMLESS_DRRS_SUPPORT = 2
>>>> +};
>>>> +/**
>>>> + * HIGH_RR is the highest eDP panel refresh rate read from EDID
>>>> + * LOW_RR is the lowest eDP panel refresh rate found from EDID
>>>> + * parsing for same resolution.
>>>> + */
>>>> +enum edp_drrs_refresh_rate_type {
>>>> +	DRRS_HIGH_RR,
>>>> +	DRRS_LOW_RR,
>>>> +	DRRS_MAX_RR, /* RR count */
>>>> +};
>>>> +/**
>>>> + * The drrs_info struct will represent the DRRS feature for eDP
>>>> + * panel.
>>>> + */
>>>> +struct drrs_info {
>>>> +	int is_drrs_supported;
>>>> +	int drrs_refresh_rate_type;
>>>
>>> So what was the point of the enums again? Are you purposely trying to
>>> disable gcc and sparse's type-safety?
>>>
>> The enum edp_panel_type is required to check DRRS capability of the
>> panel before performing any enabling. We will look into an
>> implementation which can do without edp_drrs_refresh_rate_type.
> 
> All I am saying is have enum, use enum. It improves type safety.
> -Chris
> 
Ok. I will not be making any change related to this.