Re: [PATCH v5 04/17] drm: add helper to validate ycbcr420 modes

["Sharma, Shashank" <shashank.sharma@intel.com>]

Regards

Shashank


On 7/5/2017 3:46 PM, Ville Syrjälä wrote:
> On Wed, Jul 05, 2017 at 08:48:40AM +0530, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 7/4/2017 9:26 PM, Ville Syrjälä wrote:
>>> On Tue, Jul 04, 2017 at 07:41:51PM +0530, Shashank Sharma wrote:
>>>> YCBCR420 modes are supported only on HDMI 2.0 capable sources.
>>>> This patch adds a drm helper to validate YCBCR420-only mode
>>>> on a particular connector. This function will help pruning
>>>> the YCBCR420-only modes from the connector's modelist.
>>>>
>>>> V5: Introduced the patch in series.
>>>>
>>>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/drm_edid.c         |  3 ++-
>>>>    drivers/gpu/drm/drm_modes.c        | 28 ++++++++++++++++++++++++++++
>>>>    drivers/gpu/drm/drm_probe_helper.c |  4 ++++
>>>>    include/drm/drm_edid.h             |  1 +
>>>>    include/drm/drm_modes.h            |  5 +++++
>>>>    5 files changed, 40 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>>>> index b879662..ad26c5e 100644
>>>> --- a/drivers/gpu/drm/drm_edid.c
>>>> +++ b/drivers/gpu/drm/drm_edid.c
>>>> @@ -2947,10 +2947,11 @@ u8 drm_match_cea_mode(const struct drm_display_mode *to_match)
>>>>    }
>>>>    EXPORT_SYMBOL(drm_match_cea_mode);
>>>>    
>>>> -static bool drm_valid_cea_vic(u8 vic)
>>>> +bool drm_valid_cea_vic(u8 vic)
>>>>    {
>>>>    	return vic > 0 && vic < ARRAY_SIZE(edid_cea_modes);
>>>>    }
>>>> +EXPORT_SYMBOL(drm_valid_cea_vic);
>>>>    
>>>>    /**
>>>>     * drm_get_cea_aspect_ratio - get the picture aspect ratio corresponding to
>>>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
>>>> index f2493b9..3b53c8e3 100644
>>>> --- a/drivers/gpu/drm/drm_modes.c
>>>> +++ b/drivers/gpu/drm/drm_modes.c
>>>> @@ -1083,6 +1083,34 @@ drm_mode_validate_size(const struct drm_display_mode *mode,
>>>>    }
>>>>    EXPORT_SYMBOL(drm_mode_validate_size);
>>>>    
>>>> +/**
>>>> + * drm_mode_ycbcr420_only - add 'ycbcr420-only' modes only when allowed
>>>> + * @mode: mode to check
>>>> + * @connector: drm connector under action
>>>> + *
>>>> + * This function is a helper which can be used to filter out any YCBCR420
>>>> + * only mode, when the source doesn't support it.
>>>> + *
>>>> + * Returns:
>>>> + * The mode status
>>>> + */
>>>> +enum drm_mode_status
>>>> +drm_mode_validate_ycbcr420(const struct drm_display_mode *mode,
>>>> +			   struct drm_connector *connector)
>>>> +{
>>>> +	u8 vic = drm_match_cea_mode(mode);
>>>> +	enum drm_mode_status status = MODE_OK;
>>>> +	struct drm_hdmi_info *hdmi = &connector->display_info.hdmi;
>>>> +
>>>> +	if (drm_valid_cea_vic(vic) && test_bit(vic, hdmi->y420_vdb_modes)) {
>>> The drm_valid_cea_vic() check seems redundant to me.
>>> Why do you think we need it?
>> drm_match_cea_mode() returns 0 in case if a vic is not found. if we take
>> that to test_bit() it will check bit 0
>> and give wrong results.
> Why would bit 0 be set? That would sound like a bug in the mode parsing.
I know, and it wont be, but do we want to take the wrong VIC as input in 
first place ? Many detailed modes, and non-cea modes
will return 0 for VIC, why should we bother checking them in map at the 
first place ?

- Shashank
>> So first we have to check valid vic, this is
>> also an additional check for YCBCR420 mode
>> as they must have a valid vic
>>> Also I don't think this will compile since we don't have
>>> y420_vdb_modes[] yet.
>> Ah, now I recall the reason I wanted to give you, when you asked me to
>> move this patch, before we add ycbcr420 modes.
>> So now this has to go after patch 5 right ? I would re-sequence
>> accordingly.
>>
>> - Shashank
>>>> +		if (!connector->ycbcr_420_allowed)
>>>> +			status = MODE_NO_420;
>>>> +	}
>>>> +
>>>> +	return status;
>>>> +}
>>>> +EXPORT_SYMBOL(drm_mode_validate_ycbcr420);
>>>> +
>>>>    #define MODE_STATUS(status) [MODE_ ## status + 3] = #status
>>>>    
>>>>    static const char * const drm_mode_status_names[] = {
>>>> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
>>>> index 00e6832..904966c 100644
>>>> --- a/drivers/gpu/drm/drm_probe_helper.c
>>>> +++ b/drivers/gpu/drm/drm_probe_helper.c
>>>> @@ -528,6 +528,10 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>>>>    		if (mode->status == MODE_OK)
>>>>    			mode->status = drm_mode_validate_pipeline(mode,
>>>>    								  connector);
>>>> +
>>>> +		if (mode->status == MODE_OK)
>>>> +			mode->status = drm_mode_validate_ycbcr420(mode,
>>>> +								  connector);
>>>>    	}
>>>>    
>>>>    prune:
>>>> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>>>> index 89c0062..b55b2a7 100644
>>>> --- a/include/drm/drm_edid.h
>>>> +++ b/include/drm/drm_edid.h
>>>> @@ -477,4 +477,5 @@ void drm_edid_get_monitor_name(struct edid *edid, char *name,
>>>>    struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
>>>>    					   int hsize, int vsize, int fresh,
>>>>    					   bool rb);
>>>> +bool drm_valid_cea_vic(u8 vic);
>>>>    #endif /* __DRM_EDID_H__ */
>>>> diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
>>>> index 94ac771..f8a1268 100644
>>>> --- a/include/drm/drm_modes.h
>>>> +++ b/include/drm/drm_modes.h
>>>> @@ -80,6 +80,7 @@ struct videomode;
>>>>     * @MODE_ONE_SIZE: only one resolution is supported
>>>>     * @MODE_NO_REDUCED: monitor doesn't accept reduced blanking
>>>>     * @MODE_NO_STEREO: stereo modes not supported
>>>> + * @MODE_NO_420: ycbcr 420 modes not supported
>>>>     * @MODE_STALE: mode has become stale
>>>>     * @MODE_BAD: unspecified reason
>>>>     * @MODE_ERROR: error condition
>>>> @@ -124,6 +125,7 @@ enum drm_mode_status {
>>>>    	MODE_ONE_SIZE,
>>>>    	MODE_NO_REDUCED,
>>>>    	MODE_NO_STEREO,
>>>> +	MODE_NO_420,
>>>>    	MODE_STALE = -3,
>>>>    	MODE_BAD = -2,
>>>>    	MODE_ERROR = -1
>>>> @@ -496,6 +498,9 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
>>>>    enum drm_mode_status drm_mode_validate_basic(const struct drm_display_mode *mode);
>>>>    enum drm_mode_status drm_mode_validate_size(const struct drm_display_mode *mode,
>>>>    					    int maxX, int maxY);
>>>> +enum drm_mode_status
>>>> +drm_mode_validate_ycbcr420(const struct drm_display_mode *mode,
>>>> +			   struct drm_connector *connector);
>>>>    void drm_mode_prune_invalid(struct drm_device *dev,
>>>>    			    struct list_head *mode_list, bool verbose);
>>>>    void drm_mode_sort(struct list_head *mode_list);
>>>> -- 
>>>> 2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx