On Thu, 2018-05-03 at 10:46 +0000, Lisovskiy, Stanislav wrote:
> 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.
>
So settling here a discussion also. Just to have it in writing as we
spoke offline, the problem is that we basically have to manipulate with
two different AVI inforame fields(itc bit and content type bits) using
the same DRM property.
This obviously implies some coding/decoding logic already, my initial
approach was just to have those bits in place( bit 2 controls IT
content bit, bits 1..0 control content-type cn1..cn0 bits) in order
not to have any other decoding logic in set/get property calls(also I
saw there are other non-contigious bit masks and sets in uapi header).
>
> > +/* HDMI content type and itc bit bound together for simplicity */
> > +#define DRM_MODE_CONTENT_TYPE_NO_DATA 0
> > +#define DRM_MODE_CONTENT_TYPE_GRAPHICS 4
> > +#define DRM_MODE_CONTENT_TYPE_PHOTO 5
> > +#define DRM_MODE_CONTENT_TYPE_CINEMA 6
> > +#define DRM_MODE_CONTENT_TYPE_GAME 7
If those "magic" numbers defined here are not OK, then I need to add
some additional decoding layer. Should I just add a macros or helper
functions then, which would transform contigious
DRM_MODE_CONTENT_TYPE_* values into correspondent AVI infoframe itc and
content-type fields?
> > +
> > /* Aspect ratio flag bitmask (4 bits 22:19) */
> > #define DRM_MODE_FLAG_PIC_AR_MASK (0x0F<<19)
> > #define DRM_MODE_FLAG_PIC_AR_NONE \
>
>
--
Best Regards,
Lisovskiy Stanislav
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
