Re: [Intel-gfx] [PATCH v8 1/2] drm: content-type property for HDMI connector

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux