Re: [PATCH 4/6] drm/rockchip: dw_hdmi: Add vendor hdmi properties

[Pekka Paalanen]

On Wed, 12 Aug 2020 16:30:17 +0300
Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:

> Hi Algea,
> 
> On Wed, Aug 12, 2020 at 07:08:10PM +0800, crj wrote:
> > 在 2020/8/12 17:33, Laurent Pinchart 写道:  
> > > On Wed, Aug 12, 2020 at 04:35:43PM +0800, Algea Cao wrote:  
> > >> Introduce struct dw_hdmi_property_ops in plat_data to support
> > >> vendor hdmi property.
> > >>
> > >> Implement hdmi vendor properties color_depth_property and
> > >> hdmi_output_property to config hdmi output color depth and
> > >> color format.
> > >>
> > >> The property "hdmi_output_format", the possible value
> > >> could be:
> > >>           - RGB
> > >>           - YCBCR 444
> > >>           - YCBCR 422
> > >>           - YCBCR 420
> > >>
> > >> Default value of the property is set to 0 = RGB, so no changes if you
> > >> don't set the property.
> > >>
> > >> The property "hdmi_output_depth" possible value could be
> > >>           - Automatic
> > >>             This indicates prefer highest color depth, it is
> > >>             30bit on rockcip platform.
> > >>           - 24bit
> > >>           - 30bit
> > >> The default value of property is 24bit.
> > >>
> > >> Signed-off-by: Algea Cao <algea.cao@xxxxxxxxxxxxxx>
> > >> ---
> > >>
> > >>   drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 174 ++++++++++++++++++++
> > >>   include/drm/bridge/dw_hdmi.h                |  22 +++
> > >>   2 files changed, 196 insertions(+)
> > >>
> > >> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > >> index 23de359a1dec..8f22d9a566db 100644
> > >> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > >> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > >> @@ -52,6 +52,27 @@
> > >>   
> > >>   #define HIWORD_UPDATE(val, mask)	(val | (mask) << 16)
> > >>   
> > >> +/* HDMI output pixel format */
> > >> +enum drm_hdmi_output_type {
> > >> +	DRM_HDMI_OUTPUT_DEFAULT_RGB, /* default RGB */
> > >> +	DRM_HDMI_OUTPUT_YCBCR444, /* YCBCR 444 */
> > >> +	DRM_HDMI_OUTPUT_YCBCR422, /* YCBCR 422 */
> > >> +	DRM_HDMI_OUTPUT_YCBCR420, /* YCBCR 420 */
> > >> +	DRM_HDMI_OUTPUT_YCBCR_HQ, /* Highest subsampled YUV */
> > >> +	DRM_HDMI_OUTPUT_YCBCR_LQ, /* Lowest subsampled YUV */
> > >> +	DRM_HDMI_OUTPUT_INVALID, /* Guess what ? */
> > >> +};  
> > >
> > > Vendor-specific properties shouldn't use names starting with drm_ or
> > > DRM_, that's for the DRM core. But this doesn't seem specific to
> > > Rockchip at all, it should be a standard property. Additionally, new
> > > properties need to come with a userspace implementation showing their
> > > usage, in X.org, Weston, the Android DRM/KMS HW composer, or another
> > > relevant upstream project (a test tool is usually not enough).  
> > 
> > We use these properties only in Android HW composer, But we can't upstream
> > 
> > our HW composer code right now.  Can we use this properties as private 
> > property
> > 
> > and do not upstream HW composer for the time being?  
> 
> It's not my decision, it's a policy of the DRM subsystem to require an
> open implementation in userspace to validate all API additions.

Also read
https://www.kernel.org/doc/html/latest/gpu/drm-uapi.html#open-source-userspace-requirements
very carefully: it calls for a FOSS userspace project's proper upstream
to have reviewed and accepted the patches to use the new UAPI, but
those patches must NOT be MERGED at that time yet.


Thanks,
pq

Attachment: pgptvsDNHpEX1.pgp
Description: OpenPGP digital signature