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@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_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.
> >
> >> + 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@xxxxxxxxxxxxxxxxxxxxx
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
