On Wed, Jul 05, 2017 at 11:39:30AM +0530, Sharma, Shashank wrote:
> Regards
>
> Shashank
>
>
> On 7/4/2017 9:06 PM, Daniel Vetter wrote:
> > On Tue, Jul 04, 2017 at 07:41:56PM +0530, Shashank Sharma wrote:
> > > HDMI displays can support various output types, based on
> > > the color space and subsampling type. The possible
> > > outputs from a HDMI 2.0 monitor could be:
> > > - RGB
> > > - YCBCR 444
> > > - YCBCR 422
> > > - YCBCR 420
> > >
> > > This patch adds a drm property "hdmi_output_format", using which,
> > > a user can specify its preference, for the HDMI output type. The
> > > output type enums are similar to the mentioned outputs above. To
> > > handle various subsampling of YCBCR output types, this property
> > > allows two special cases:
> > > - DRM_HDMI_OUTPUT_YCBCR_HQ
> > > This indicates preferred output should be YCBCR output, with highest
> > > subsampling rate by the source/sink, which can be typically:
> > > - ycbcr444
> > > - ycbcr422
> > > - ycbcr420
> > > - DRM_HDMI_OUTPUT_YCBCR_LQ
> > > This indicates preferred output should be YCBCR output, with lowest
> > > subsampling rate supported by source/sink, which can be:
> > > - ycbcr420
> > > - ycbcr422
> > > - ycbcr444
> > >
> > > Default value of the property is set to 0 = RGB, so no changes if you
> > > dont set the property.
> > >
> > > PS: While doing modeset for YCBCR 420 only modes, this property is
> > > ignored, as those timings can be supported only in YCBCR 420
> > > output mode.
> > >
> > > V2: Added description for the new variable to address build warning
> > > V3: Rebase
> > > V4: Rebase
> > > V5: Added get_property counterpart to fix IGT BAT failures (BAT/CI)
> > > Danvet:
> > > - Add documentation for the new property
> > > - Create a sub-section for HDMI properties, and add documentation for
> > > few more HDMI propeties. Added documentation for:
> > > - Broadcast RGB
> > > - aspect ratio
> > >
> > > Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx>
> > > Cc: Jose Abreu <joabreu@xxxxxxxxxxxx>
> > > Cc: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> > > Signed-off-by: Shashank Sharma <shashank.sharma@xxxxxxxxx>
> > Bunch more documentation nitpicks below. I'd personally also split up the
> > patch into documenting the existing props and adding the new format one.
> In fact thats what I would do now. I dont want HDMI 2.0 patch series to get
> blocked due to documentation, as with all the comments
> it seems like some work. Lets do it in this way:
> - This patch series will add the documentation for hdmi_output_property
> - I will send a separate patch which will collectively add documentation for
> all standard HDMI properties.
> > Thanks, Daniel
> >
> > > ---
> > > Documentation/gpu/drm-kms.rst | 12 +++++++
> > > drivers/gpu/drm/drm_atomic.c | 4 +++
> > > drivers/gpu/drm/drm_atomic_helper.c | 4 +++
> > > drivers/gpu/drm/drm_connector.c | 69 ++++++++++++++++++++++++++++++++++++-
> > > drivers/gpu/drm/drm_crtc_internal.h | 1 +
> > > drivers/gpu/drm/drm_mode_config.c | 4 +++
> > > drivers/gpu/drm/i915/intel_modes.c | 13 +++++++
> > > include/drm/drm_connector.h | 18 ++++++++++
> > > include/drm/drm_mode_config.h | 5 +++
> > > 9 files changed, 129 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> > > index 3072841..dcdd6ff 100644
> > > --- a/Documentation/gpu/drm-kms.rst
> > > +++ b/Documentation/gpu/drm-kms.rst
> > > @@ -508,6 +508,18 @@ Standard Connector Properties
> > > .. kernel-doc:: drivers/gpu/drm/drm_connector.c
> > > :doc: standard connector properties
> > > +Standard HDMI Properties
> > > +-----------------------------
> > > +
> > > +.. kernel-doc:: drivers/gpu/drm/drm_connector.c
> > > + :doc: hdmi_output_format
> > > +
> > > +.. kernel-doc:: drivers/gpu/drm/drm_connector.c
> > > + :doc: aspect ratio property
> > I'd have just created 1 DOC: section titled "standard HDMI properties" and
> > listed all of them in 1 comment block. People often forget to add the
> > include stanza in the .rst files, having bigger comments helps with that.
> >
> > But feel free to go either way.
> Yes, this would be easier for me too, but this means I will have to add all
> the documentation in one place, in one file, whether the respective
> property is created at that place or not. I am not sure if everyone would be
> ok with that during review. But if you think we should go ahead,
> thanks for easing this up for me :-).
It might result in a minor merge conflict, but nothing bad should happen
if we put all the docs into one section.
> > > +.. kernel-doc:: drivers/gpu/drm/i915/intel_modes.c
> > > + :doc: Broadcast RGB property
> > Please no generic properties documented in driver code.
> Broadcast RGB property is created here, and also, while I was adding
> documentation for it, I realized its kept in dev_priv, which is specific to
> I915 driver, So its not generic too. I was not sure if that will be OK, if I
> add an Intel specific property's description in DRM layer ?
It should be generic, but oh well it isn't. Either document it in the
core, or we'll leave this to the future when the create helper gets
extracted to the core.
> > > +
> > > Plane Composition Properties
> > > ----------------------------
> > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > index 09ca662..adcb89d 100644
> > > --- a/drivers/gpu/drm/drm_atomic.c
> > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > @@ -1192,6 +1192,8 @@ int drm_atomic_connector_set_property(struct drm_connector *connector,
> > > state->picture_aspect_ratio = val;
> > > } else if (property == connector->scaling_mode_property) {
> > > state->scaling_mode = val;
> > > + } else if (property == config->hdmi_output_property) {
> > > + state->hdmi_output = val;
> > > } else if (connector->funcs->atomic_set_property) {
> > > return connector->funcs->atomic_set_property(connector,
> > > state, property, val);
> > > @@ -1272,6 +1274,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
> > > *val = state->picture_aspect_ratio;
> > > } else if (property == connector->scaling_mode_property) {
> > > *val = state->scaling_mode;
> > > + } else if (property == config->hdmi_output_property) {
> > > + *val = state->hdmi_output;
> > > } else if (connector->funcs->atomic_get_property) {
> > > return connector->funcs->atomic_get_property(connector,
> > > state, property, val);
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > index 23e4661..2e7459f 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -637,6 +637,10 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> > > if (old_connector_state->link_status !=
> > > new_connector_state->link_status)
> > > new_crtc_state->connectors_changed = true;
> > > +
> > > + if (old_connector_state->hdmi_output !=
> > > + new_connector_state->hdmi_output)
> > > + new_crtc_state->connectors_changed = true;
> > > }
> > > if (funcs->atomic_check)
> > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > > index 8072e6e..8357918 100644
> > > --- a/drivers/gpu/drm/drm_connector.c
> > > +++ b/drivers/gpu/drm/drm_connector.c
> > > @@ -227,6 +227,11 @@ int drm_connector_init(struct drm_device *dev,
> > > config->edid_property,
> > > 0);
> > > + if (connector_type != DRM_MODE_CONNECTOR_VIRTUAL)
> > > + drm_object_attach_property(&connector->base,
> > > + config->hdmi_output_property,
> > > + 0);
> > > +
> > > drm_object_attach_property(&connector->base,
> > > config->dpms_property, 0);
> > > @@ -617,6 +622,26 @@ static const struct drm_prop_enum_list drm_link_status_enum_list[] = {
> > > };
> > > DRM_ENUM_NAME_FN(drm_get_link_status_name, drm_link_status_enum_list)
> > > +static const struct drm_prop_enum_list drm_hdmi_output_enum_list[] = {
> > > + { DRM_HDMI_OUTPUT_DEFAULT_RGB, "output_rgb" },
> > > + { DRM_HDMI_OUTPUT_YCBCR444, "output_ycbcr444" },
> > > + { DRM_HDMI_OUTPUT_YCBCR422, "output_ycbcr422" },
> > > + { DRM_HDMI_OUTPUT_YCBCR420, "output_ycbcr420" },
> > > + { DRM_HDMI_OUTPUT_YCBCR_HQ, "output_ycbcr_high_subsampling" },
> > > + { DRM_HDMI_OUTPUT_YCBCR_LQ, "output_ycbcr_low_subsampling" },
> > > + { DRM_HDMI_OUTPUT_INVALID, "invalid_output" },
> > > +};
> > > +
> > > +/**
> > > + * drm_get_hdmi_output_name - return a string for a given hdmi output enum
> > > + * @type: enum of output type
> > > + */
> > > +const char *drm_get_hdmi_output_name(enum drm_hdmi_output_type type)
> > > +{
> > > + return drm_hdmi_output_enum_list[type].name;
> > > +}
> > > +EXPORT_SYMBOL(drm_get_hdmi_output_name);
> > > +
> > > /**
> > > * drm_display_info_set_bus_formats - set the supported bus formats
> > > * @info: display info to store bus formats in
> > > @@ -697,7 +722,36 @@ static const struct drm_prop_enum_list drm_tv_subconnector_enum_list[] = {
> > > { DRM_MODE_SUBCONNECTOR_SCART, "SCART" }, /* TV-out */
> > > };
> > > DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name,
> > > - drm_tv_subconnector_enum_list)
> > > + drm_tv_subconnector_enum_list);
> > > +
> > > +/**
> > > + * DOC: hdmi_output_format
> > > + *
> > > + * hdmi_output_format:
> > > + * Enum property which allows a userspace to provide its preference for a
> > > + * HDMI output format. Standard HDMI 1.4b outputs can support RGB/YCBCR444
> > > + * YCBCR422 output formats, whereas HDMI 2.0 outputs can additionally
> > > + * support YCBCR420 output. Default value of the property is HDMI output
> > > + * RGB.
> > > + *
> > > + * A driver can attach to this property, and then handle the HDMI output
> > > + * based on its capabilities and sink support. There are few helper
> > > + * functions available in drm_modes.c which can help in finding the best
> > > + * suitable output considering a sink/source/mode combination. Refer to
> > > + * drm_modes.c:drm_display_info_hdmi_output_type()
> > > + */
> > > +int drm_connector_create_hdmi_properties(struct drm_device *dev)
> > > +{
> > > + struct drm_property *prop;
> > > +
> > > + prop = drm_property_create_enum(dev, 0, "hdmi_output_format",
> > > + drm_hdmi_output_enum_list,
> > > + ARRAY_SIZE(drm_hdmi_output_enum_list));
> > > + if (!prop)
> > > + return -ENOMEM;
> > > + dev->mode_config.hdmi_output_property = prop;
> > > + return 0;
> > > +}
> > > /**
> > > * DOC: standard connector properties
> > > @@ -1024,6 +1078,19 @@ int drm_connector_attach_scaling_mode_property(struct drm_connector *connector,
> > > }
> > > EXPORT_SYMBOL(drm_connector_attach_scaling_mode_property);
> > > +
> > > +/**
> > > + * DOC: aspect ratio property
> > > + *
> > > + * aspect ratio:
> > > + * Enum property to override the HDMI output's aspect ratio.
> > > + * When this property is set, the aspect ratio of the frame in
> > > + * AVI infoframes is set as per the property value. For example
> > > + * if userspace sets the property value to DRM_MODE_PICTURE_ASPECT_4_3
> > > + * the output aspect ratio is set to 4:3 (regardless of the PAR of mode)
> > Would be good to document all possible values. Same for the other
> > properties, plus insert a link to the function that creates the property
> > (where we have that).
> This is the part, which makes me feel that I should create this patch
> separately, as this seems like slightly bigger cleanup than
> I thought, so I dont want this to be a part of HDMI 2.0 series.
Writing good docs is hard work, but it's part of creating a new uapi :-)
But yeah you can split out the overall cleanup if you think that's easier
(it'll probably result in some fun small conflicts).
> >
> > > + *
> > > + */
> > > +
> > > /**
> > > * drm_mode_create_aspect_ratio_property - create aspect ratio property
> > > * @dev: DRM device
> > > diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
> > > index d077c54..e6c4adc 100644
> > > --- a/drivers/gpu/drm/drm_crtc_internal.h
> > > +++ b/drivers/gpu/drm/drm_crtc_internal.h
> > > @@ -140,6 +140,7 @@ int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
> > > struct drm_property *property,
> > > uint64_t value);
> > > int drm_connector_create_standard_properties(struct drm_device *dev);
> > > +int drm_connector_create_hdmi_properties(struct drm_device *dev);
> > > const char *drm_get_connector_force_name(enum drm_connector_force force);
> > > /* IOCTL */
> > > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> > > index d986225..3ba9262 100644
> > > --- a/drivers/gpu/drm/drm_mode_config.c
> > > +++ b/drivers/gpu/drm/drm_mode_config.c
> > > @@ -210,6 +210,10 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
> > > if (ret)
> > > return ret;
> > > + ret = drm_connector_create_hdmi_properties(dev);
> > > + if (ret)
> > > + return ret;
> > We still create all the property values instead of just the ones that the
> > source hw supports. E.g. aspect ratio property is only created if needed,
> > not unconditionally on all connectors.
> >
> > Note that users see properties through xrandr and the gui screen
> > configuration tools, adding properties that never do anything is
> > confusing, more for properties where you can select invalid values.
> If I understood this properly, you want me to create the property in I915
> layer, just before attaching it to object (similar to aspect ratio and
> others)
> something like:
> intel_attach_hdmi_output_property()
> {
> if (!mode_config->hdmi_output_property)
> mode_config->hdmi_output_property = create_prop();
> now_attach_property();
> }
Yes.
> I was assuming that once other drivers move to HDMI 2.0, they might all need
> this for YCBCR420 outputs, so it would be ok to create it generically.
> But at the same time, this also seems like a good idea.
gen8 isn't going to magically support hdmi2.0. We shouldn't advertise it
to users either. Same holds for other drivers and other features. Having a
generic/shared function to create it is good, but automically creating
something that on many drivers/platforms does nothing is not good, that
only confuses users (and userspace programmers).
Cheers, Daniel
>
> - Shashank
> > > +
> > > prop = drm_property_create_enum(dev, DRM_MODE_PROP_IMMUTABLE,
> > > "type", drm_plane_type_enum_list,
> > > ARRAY_SIZE(drm_plane_type_enum_list));
> > > diff --git a/drivers/gpu/drm/i915/intel_modes.c b/drivers/gpu/drm/i915/intel_modes.c
> > > index 951e834..d07de45 100644
> > > --- a/drivers/gpu/drm/i915/intel_modes.c
> > > +++ b/drivers/gpu/drm/i915/intel_modes.c
> > > @@ -104,6 +104,19 @@ static const struct drm_prop_enum_list broadcast_rgb_names[] = {
> > > { INTEL_BROADCAST_RGB_LIMITED, "Limited 16:235" },
> > > };
> > > +/**
> > > + * DOC: Broadcast RGB property
> > > + *
> > > + * Broadcast RGB:
> > > + * Enum property to indicate RGB color range of a HDMI output.
> > > + * For limited range RGB outputs, a remapping of pixel values from 0-255
> > > + * should be remaped to a range of 16-235. When this property is set to
> > > + * Limited 16:235 and CTM is set, the hardware will be programmed with the
> > > + * result of the multiplication of CTM by the limited range matrix, to
> > > + * ensure the pixels normaly in the range 0..1.0 are remapped to the range
> > > + * 16/255..235/255.
> > > + *
> > > + */
> > > void
> > > intel_attach_broadcast_rgb_property(struct drm_connector *connector)
> > > {
> > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > index 4bc0882..3773600 100644
> > > --- a/include/drm/drm_connector.h
> > > +++ b/include/drm/drm_connector.h
> > > @@ -319,6 +319,17 @@ struct drm_tv_connector_state {
> > > unsigned int hue;
> > > };
> > > +/* 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 ? */
> > > +};
> > > +
> > > /**
> > > * struct drm_connector_state - mutable connector state
> > > * @connector: backpointer to the connector
> > > @@ -363,6 +374,12 @@ struct drm_connector_state {
> > > * upscaling, mostly used for built-in panels.
> > > */
> > > unsigned int scaling_mode;
> > > +
> > > + /**
> > > + * @hdmi_output: Connector property to control the
> > > + * HDMI output mode (RGB/YCBCR444/422/420).
> > > + */
> > > + enum drm_hdmi_output_type hdmi_output;
> > > };
> > > /**
> > > @@ -991,6 +1008,7 @@ static inline void drm_connector_unreference(struct drm_connector *connector)
> > > const char *drm_get_connector_status_name(enum drm_connector_status status);
> > > const char *drm_get_subpixel_order_name(enum subpixel_order order);
> > > +const char *drm_get_hdmi_output_name(enum drm_hdmi_output_type type);
> > > const char *drm_get_dpms_name(int val);
> > > const char *drm_get_dvi_i_subconnector_name(int val);
> > > const char *drm_get_dvi_i_select_name(int val);
> > > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> > > index 4298171..1887261 100644
> > > --- a/include/drm/drm_mode_config.h
> > > +++ b/include/drm/drm_mode_config.h
> > > @@ -740,6 +740,11 @@ struct drm_mode_config {
> > > * the position of the output on the host's screen.
> > > */
> > > struct drm_property *suggested_y_property;
> > > + /**
> > > + * @hdmi_output_property: output pixel format from HDMI display
> > > + * Default is set for RGB
> > > + */
> > > + struct drm_property *hdmi_output_property;
> > > /* dumb ioctl parameters */
> > > uint32_t preferred_depth, prefer_shadow;
> > > --
> > > 2.7.4
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
