On Thu, 2018-05-03 at 13:18 +0300, Jani Nikula wrote:
> On Wed, 02 May 2018, StanLis <stanislav.lisovskiy@xxxxxxxxx> wrote:
> > From: Stanislav Lisovskiy <stanislav.lisovskiy@xxxxxxxxx>
> >
> >
> > diff --git a/Documentation/gpu/kms-properties.csv
> > b/Documentation/gpu/kms-properties.csv
> > index 07ed22ea3bd6..bfde04eddd14 100644
> > --- a/Documentation/gpu/kms-properties.csv
> > +++ b/Documentation/gpu/kms-properties.csv
> > @@ -17,6 +17,7 @@ Owner Module/Drivers,Group,Property
> > Name,Type,Property Values,Object attached,De
> > ,Virtual GPU,“suggested X”,RANGE,"Min=0,
> > Max=0xffffffff",Connector,property to suggest an X offset for a
> > connector
> > ,,“suggested Y”,RANGE,"Min=0, Max=0xffffffff",Connector,property
> > to suggest an Y offset for a connector
> > ,Optional,"""aspect ratio""",ENUM,"{ ""None"", ""4:3"", ""16:9""
> > }",Connector,TDB
> > +,Optional,"""content type""",ENUM,"{ ""No Data"", ""Graphics"",
> > ""Photo"", ""Cinema"", ""Game"" }",Connector,TBD
> > i915,Generic,"""Broadcast RGB""",ENUM,"{ ""Automatic"", ""Full"",
> > ""Limited 16:235"" }",Connector,"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."
> > ,,“audio”,ENUM,"{ ""force-dvi"", ""off"", ""auto"", ""on""
> > }",Connector,TBD
> > ,SDVO-TV,“mode”,ENUM,"{ ""NTSC_M"", ""NTSC_J"", ""NTSC_443"",
> > ""PAL_B"" } etc.",Connector,TBD
> > diff --git a/drivers/gpu/drm/drm_atomic.c
> > b/drivers/gpu/drm/drm_atomic.c
> > index 3d9ae057a6cd..046fc5249859 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -1270,6 +1270,15 @@ static int
> > drm_atomic_connector_set_property(struct drm_connector *connector,
> > state->link_status = val;
> > } else if (property == config->aspect_ratio_property) {
> > state->picture_aspect_ratio = val;
> > + } else if (property == config->content_type_property) {
> > + /*
> > + * Lowest two bits of content_type property
> > control
> > + * content_type, bit 2 controls itc bit.
> > + * It was decided to have a single property called
> > + * content_type, instead of content_type and itc.
>
> Who decided? I was trying to look through the history, but couldn't
> find
> the review discussion on this part.
>
> This approach encodes meaning to bits in the property value, while no
> such promise is actually made in the property interface. Do you
> expect
> to support graphics, photo, cinema, game with and without itc?
>
> Is the intention to be able to extend this later on? If yes, why is
> itc
> in bit 2 instead of e.g. bit 0, so the content-type could actually
> grow?
There was a discussion here with Hans Verkuil, somewhat 2-3 weeks ago,
he indicated that HDMI 1.4 spec was wrong in assuming that itc and
content-type bits can be used separately. Then it was decided offline
with Ville Sirjala that we're not going to create a separate property
for itc bit for a sake of simplicity.
In short, the content type bits CN1..CN0 are not valid without itc bit.
>
> > + */
> > + state->content_type = val & 3;
> > + state->it_content = val >> 2;
>
> I'm mostly asking because this is really confusing considering the
> property value definitions, and should IMO be written in terms of the
> macros, respecting SPOT, instead of adding magic numbers here.
>
> Also, later on you set frame->content_type =
> HDMI_CONTENT_TYPE_GRAPHICS,
> which is inconsistent with the masking above.
If you mean those lines, where it initializes the avi infoframe
structure, it sets itc bit at the same time with content_type and from
the frame
encoding point of view those are still separate, so there is no
violation:
+ frame->content_type = HDMI_CONTENT_TYPE_GRAPHICS;
+ frame->itc = 0;
So we have a single property setting both at the same time, as
there is no point to have content-type without itc bit set.
Everytime property is set, those state bits are changed and then
encoded into frame.
+ frame.avi.content_type = connector->state->content_type;
+ frame.avi.itc = connector->state->it_content;
> BR,
> Jani.
>
>
> PS. Please wait for the discussion to settle on this before sending
> another version. Accumulating tons of revisions of the patch does not
> make it move any faster.
Well, there was plenty of discussion already, Ville, Hans and Daniel
gave recommendations and acknowleged those changes earlier, so I
thought that everything is clear here at least. Any comments are still
welcome of course, however I think I need to fix the stuff otherwise
that simple patch will hang here forever imho.
Version 8 just fixes the latest comment from Ville, that docs should
use string values in property description. Everytime I get any
comments, I fix them.
>
--
Best Regards,
Lisovskiy Stanislav
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
