On Wed, Jul 05, 2017 at 03:49:51PM +0530, Sharma, Shashank wrote:
> 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@xxxxxxxxxxxxxxx>
> >>>> Signed-off-by: Shashank Sharma <shashank.sharma@xxxxxxxxx>
> >>>> ---
> >>>> 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,
All non-cea modes will return 0.
> why should we bother checking them in map at the
> first place ?
Checking the vic against some range is pointless work. Just
checking the bit blindly is the most optimal way.
>
> - 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
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
