[Intel-gfx] [PATCH 06/20] drm: add helper to validate YCBCR420 modes
Sharma, Shashank
shashank.sharma at intel.com
Thu Jul 13 12:42:50 UTC 2017
Regards
Shashank
On 7/13/2017 6:01 PM, Ville Syrjälä wrote:
> On Thu, Jul 13, 2017 at 11:02:18AM +0530, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 7/12/2017 10:48 PM, Ville Syrjälä wrote:
>>> On Mon, Jul 10, 2017 at 04:48:34PM +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.
>>>> - A bool variable (ycbcr_420_allowed) in the drm connector structure.
>>>> While handling the EDID from HDMI 2.0 sinks, its important to know
>>>> if the source is capable of handling YCBCR420 output, so that no
>>>> YCBCR 420 modes will be listed for sources which can't handle it.
>>>> A driver should set this variable if it wants to see YCBCR420 modes
>>>> in the modedb.
>>>>
>>>> V5: Introduced the patch in series.
>>>> V5-resend: Squashed two patches (validate YCBCR420 and add YCBCR420
>>>> identifier)
>>>>
>>>> Signed-off-by: Shashank Sharma <shashank.sharma at 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_connector.h | 9 +++++++++
>>>> include/drm/drm_edid.h | 1 +
>>>> include/drm/drm_modes.h | 5 +++++
>>>> 6 files changed, 49 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>>>> index 10dab62..44be128 100644
>>>> --- a/drivers/gpu/drm/drm_edid.c
>>>> +++ b/drivers/gpu/drm/drm_edid.c
>>>> @@ -2950,10 +2950,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)) {
>>> Still think the valid check is pointless.
>> I still think that we should only pass valid CEA VIC to VDB check, but
>> Ok, I will remove that.
>>> Skipping that would also avoid
>>> having to make the function non-static
>> Ok
>>> (the EXPORT_SYMBOL there seems
>>> unnecessary either way).
>> Why ?
> It's all in the same module AFAICS.
>
> Oh, and the patch ordering here still looks potentially problematic. We
> should add the 420 mode validation before we start adding the 420 modes.
> Otherwise anyone bisecting through this may get a black/corrupted screen
> if they have a monitor with "420 only" modes.
Actually, the 420 mode validation patch uses 420_only bitmap, which is
added in add_420_modes patch.
So I can't put this patch before other one (you reviewed that sequence
once, and found that).
- 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[] = {
>>> ^
>>> This thing needs to be updated as well.
>> Oh sorry, good catch.
>> - Shashank
>>>> 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_connector.h b/include/drm/drm_connector.h
>>>> index b3af8e3..225e092 100644
>>>> --- a/include/drm/drm_connector.h
>>>> +++ b/include/drm/drm_connector.h
>>>> @@ -746,6 +746,15 @@ struct drm_connector {
>>>> bool interlace_allowed;
>>>> bool doublescan_allowed;
>>>> bool stereo_allowed;
>>>> +
>>>> + /**
>>>> + * @ycbcr_420_allowed : This bool indicates if this connector is
>>>> + * capable of handling YCBCR 420 output. While parsing the EDID
>>>> + * blocks, its very helpful to know, if the source is capable of
>>>> + * handling YCBCR 420 outputs.
>>>> + */
>>>> + bool ycbcr_420_allowed;
>>>> +
>>>> /**
>>>> * @registered: Is this connector exposed (registered) with userspace?
>>>> * Protected by @mutex.
>>>> 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
>>>>
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
More information about the Intel-gfx
mailing list