| Message ID | 1499177524-26292-5-git-send-email-shashank.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? Also I don't think this will compile since we don't have y420_vdb_modes[] yet. > + 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
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. 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
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. > 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
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
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@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, 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
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)) { + 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);
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(-)