Re: [PATCH 06/20] drm: add helper to validate YCBCR420 modes

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Jul 13, 2017 at 06:12:50PM +0530, Sharma, Shashank wrote:
> 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@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).

I guess then you just need to add the bitmap already in the validation
patch.

An alternative would be just squash the patches, but that seems a bit
drastic, and probably would mix up too many things in one patch.

> - 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@lists.freedesktop.org
> >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel