Re: [Intel-gfx] [PATCH v5 1/2] drm: content-type property for HDMI connector
On Mon, Apr 23, 2018 at 06:44:53AM +, Lisovskiy, Stanislav wrote: > > > This table is seriously deprecated, because it's unreadable and > > unmaintainable. Quoting from the docs: > > > "Because this table is very unwieldy, do not add any new properties here. > > Instead document them in a section above." > > > We haven't yet moved all the existing properties over to the new layout > > (patches very much welcome, especially if you spot that an entire area > > you're touching like HDMI properties isn't moved yet). But the old table > > is very much not cool to add stuff to. > > > Sorry that I didn't notice this earlier, missed it in the diffstat. > > >> as optional. > >> I think it might be a bit misleading, if I add that one to standard > >> connector properties, > >> there are probably should be created some HDMI specific property chapter > >> then, as there > >> are already plenty of HDMI specific ones(force audio, broadcast rgb, > >> aspect ratio). > >> > >> Shouldn't it then go in a separate patch may be? Because this work is > >> surely needed, however > >> goes a bit out of scope of this patch. > > > Don't make it worse by adding more unmaintainable and unreadable entries > > to this table. That's the minimum. Moving all the standard hdmi properties > > first would obviously be even better. That also makes reviewing easier, > > since it's clearer what's there already, and how your new thing fits in. > > Ok, then I think I will add a new section called HDMI Specific Connector > Properties > under Standard Connector Properties and move content type description there. > I can add another entries there too, however would prefer to do it in a > separate patch > as those are not subject of that commit. Sounds all good. -Daniel > > > > > Best Regards, > > > > Lisovskiy Stanislav > > > > > --- > > > Documentation/gpu/kms-properties.csv | 1 + > > > drivers/gpu/drm/drm_atomic.c | 17 ++ > > > drivers/gpu/drm/drm_connector.c | 51 > > > drivers/gpu/drm/drm_edid.c | 2 ++ > > > include/drm/drm_connector.h | 18 ++ > > > include/drm/drm_mode_config.h| 5 +++ > > > include/uapi/drm/drm_mode.h | 7 > > > 7 files changed, 101 insertions(+) > > > > > > diff --git a/Documentation/gpu/kms-properties.csv > > > b/Documentation/gpu/kms-properties.csv > > > index 6b28b014cb7d..a91c9211b8d6 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=0x",Connector,property to suggest an X offset for a connector > > > ,,“suggested Y”,RANGE,"Min=0, Max=0x",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 7d25c42f22db..479499f5848e 100644 > > > --- a/drivers/gpu/drm/drm_atomic.c > > > +++ b/drivers/gpu/drm/drm_atomic.c > > > @@ -1266,6 +1266,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. > > > + */ > > > + state->content_type = val & 3; > > > + state->it_content = val >> 2; > > > } else if (property == connector->scaling_mode_property) { > > > state->scaling_mode = val; > > > } else if (property == connector->content_protection_property) { > > > @@ -1351,6 +1360,14 @@ drm_atomic_connector_get_property(struct > > > drm_connector
RE: [Intel-gfx] [PATCH v5 1/2] drm: content-type property for HDMI connector
> This table is seriously deprecated, because it's unreadable and > unmaintainable. Quoting from the docs: > "Because this table is very unwieldy, do not add any new properties here. > Instead document them in a section above." > We haven't yet moved all the existing properties over to the new layout > (patches very much welcome, especially if you spot that an entire area > you're touching like HDMI properties isn't moved yet). But the old table > is very much not cool to add stuff to. > Sorry that I didn't notice this earlier, missed it in the diffstat. >> as optional. >> I think it might be a bit misleading, if I add that one to standard >> connector properties, >> there are probably should be created some HDMI specific property chapter >> then, as there >> are already plenty of HDMI specific ones(force audio, broadcast rgb, aspect >> ratio). >> >> Shouldn't it then go in a separate patch may be? Because this work is surely >> needed, however >> goes a bit out of scope of this patch. > Don't make it worse by adding more unmaintainable and unreadable entries > to this table. That's the minimum. Moving all the standard hdmi properties > first would obviously be even better. That also makes reviewing easier, > since it's clearer what's there already, and how your new thing fits in. Ok, then I think I will add a new section called HDMI Specific Connector Properties under Standard Connector Properties and move content type description there. I can add another entries there too, however would prefer to do it in a separate patch as those are not subject of that commit. > > Best Regards, > > Lisovskiy Stanislav > > > --- > > Documentation/gpu/kms-properties.csv | 1 + > > drivers/gpu/drm/drm_atomic.c | 17 ++ > > drivers/gpu/drm/drm_connector.c | 51 > > drivers/gpu/drm/drm_edid.c | 2 ++ > > include/drm/drm_connector.h | 18 ++ > > include/drm/drm_mode_config.h| 5 +++ > > include/uapi/drm/drm_mode.h | 7 > > 7 files changed, 101 insertions(+) > > > > diff --git a/Documentation/gpu/kms-properties.csv > > b/Documentation/gpu/kms-properties.csv > > index 6b28b014cb7d..a91c9211b8d6 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=0x",Connector,property to suggest an X offset for a connector > > ,,“suggested Y”,RANGE,"Min=0, Max=0x",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 7d25c42f22db..479499f5848e 100644 > > --- a/drivers/gpu/drm/drm_atomic.c > > +++ b/drivers/gpu/drm/drm_atomic.c > > @@ -1266,6 +1266,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. > > + */ > > + state->content_type = val & 3; > > + state->it_content = val >> 2; > > } else if (property == connector->scaling_mode_property) { > > state->scaling_mode = val; > > } else if (property == connector->content_protection_property) { > > @@ -1351,6 +1360,14 @@ drm_atomic_connector_get_property(struct > > drm_connector *connector, > > *val = state->link_status; > > } else if (property == config->aspect_ratio_property) { > > *val = state->picture_aspect_ratio; > > + } else if (property == config->content_type_property) { > > + /* > > + * Lowest two bits of
Re: [Intel-gfx] [PATCH v5 1/2] drm: content-type property for HDMI connector
On Fri, Apr 20, 2018 at 08:31:56AM +, Lisovskiy, Stanislav wrote: > > > From: Daniel Vetter [daniel.vet...@ffwll.ch] on behalf of Daniel Vetter > [dan...@ffwll.ch] > > > The property documentation to tie all the bits together (property, helper, > > internals) is missing. It should be in > > > https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#standard-connector-properties > > > Probably want to start with an intro-paragraph for HDMI properties > > (someone should document the broadcast prop too eventually). > > > Pls also make sure the resulting docs look pretty and that it's all > > nicely hyperlink: > > >$ make htmldocs > > Thank you for your feedback. The thing is that I actually looked into docs, > but I think there should have been already some HDMI specific property > documentation, > because this one is actually not a standard connector property, so I've added > it only to > > https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#existing-kms-properties > table This table is seriously deprecated, because it's unreadable and unmaintainable. Quoting from the docs: "Because this table is very unwieldy, do not add any new properties here. Instead document them in a section above." We haven't yet moved all the existing properties over to the new layout (patches very much welcome, especially if you spot that an entire area you're touching like HDMI properties isn't moved yet). But the old table is very much not cool to add stuff to. Sorry that I didn't notice this earlier, missed it in the diffstat. > as optional. > I think it might be a bit misleading, if I add that one to standard connector > properties, > there are probably should be created some HDMI specific property chapter > then, as there > are already plenty of HDMI specific ones(force audio, broadcast rgb, aspect > ratio). > > Shouldn't it then go in a separate patch may be? Because this work is surely > needed, however > goes a bit out of scope of this patch. Don't make it worse by adding more unmaintainable and unreadable entries to this table. That's the minimum. Moving all the standard hdmi properties first would obviously be even better. That also makes reviewing easier, since it's clearer what's there already, and how your new thing fits in. -Daniel > > Best Regards, > > Lisovskiy Stanislav > > > --- > > Documentation/gpu/kms-properties.csv | 1 + > > drivers/gpu/drm/drm_atomic.c | 17 ++ > > drivers/gpu/drm/drm_connector.c | 51 > > drivers/gpu/drm/drm_edid.c | 2 ++ > > include/drm/drm_connector.h | 18 ++ > > include/drm/drm_mode_config.h| 5 +++ > > include/uapi/drm/drm_mode.h | 7 > > 7 files changed, 101 insertions(+) > > > > diff --git a/Documentation/gpu/kms-properties.csv > > b/Documentation/gpu/kms-properties.csv > > index 6b28b014cb7d..a91c9211b8d6 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=0x",Connector,property to suggest an X offset for a connector > > ,,“suggested Y”,RANGE,"Min=0, Max=0x",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 7d25c42f22db..479499f5848e 100644 > > --- a/drivers/gpu/drm/drm_atomic.c > > +++ b/drivers/gpu/drm/drm_atomic.c > > @@ -1266,6 +1266,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
RE: [Intel-gfx] [PATCH v5 1/2] drm: content-type property for HDMI connector
From: Daniel Vetter [daniel.vet...@ffwll.ch] on behalf of Daniel Vetter [dan...@ffwll.ch] > The property documentation to tie all the bits together (property, helper, > internals) is missing. It should be in > https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#standard-connector-properties > Probably want to start with an intro-paragraph for HDMI properties > (someone should document the broadcast prop too eventually). > Pls also make sure the resulting docs look pretty and that it's all > nicely hyperlink: >$ make htmldocs Thank you for your feedback. The thing is that I actually looked into docs, but I think there should have been already some HDMI specific property documentation, because this one is actually not a standard connector property, so I've added it only to https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#existing-kms-properties table as optional. I think it might be a bit misleading, if I add that one to standard connector properties, there are probably should be created some HDMI specific property chapter then, as there are already plenty of HDMI specific ones(force audio, broadcast rgb, aspect ratio). Shouldn't it then go in a separate patch may be? Because this work is surely needed, however goes a bit out of scope of this patch. Best Regards, Lisovskiy Stanislav > --- > Documentation/gpu/kms-properties.csv | 1 + > drivers/gpu/drm/drm_atomic.c | 17 ++ > drivers/gpu/drm/drm_connector.c | 51 > drivers/gpu/drm/drm_edid.c | 2 ++ > include/drm/drm_connector.h | 18 ++ > include/drm/drm_mode_config.h| 5 +++ > include/uapi/drm/drm_mode.h | 7 > 7 files changed, 101 insertions(+) > > diff --git a/Documentation/gpu/kms-properties.csv > b/Documentation/gpu/kms-properties.csv > index 6b28b014cb7d..a91c9211b8d6 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=0x",Connector,property > to suggest an X offset for a connector > ,,“suggested Y”,RANGE,"Min=0, Max=0x",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 7d25c42f22db..479499f5848e 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -1266,6 +1266,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. > + */ > + state->content_type = val & 3; > + state->it_content = val >> 2; > } else if (property == connector->scaling_mode_property) { > state->scaling_mode = val; > } else if (property == connector->content_protection_property) { > @@ -1351,6 +1360,14 @@ drm_atomic_connector_get_property(struct drm_connector > *connector, > *val = state->link_status; > } else if (property == config->aspect_ratio_property) { > *val = state->picture_aspect_ratio; > + } 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. > + */ > + *val = state->content_type | (state->it_content << 2); > } else if (property == connector->scaling_mode_property) { > *val =
Re: [Intel-gfx] [PATCH v5 1/2] drm: content-type property for HDMI connector
On Thu, Apr 19, 2018 at 03:38:53PM +0300, StanLis wrote: > From: Stanislav Lisovskiy> > Added content_type property to drm_connector_state > in order to properly handle external HDMI TV content-type setting. > > v2: > * Moved helper function which attaches content type property >to the drm core, as was suggested. >Removed redundant connector state initialization. > > v3: > * Removed caps in drm_content_type_enum_list. >After some discussion it turned out that HDMI Spec 1.4 >was wrongly assuming that IT Content(itc) bit doesn't affect >Content type states, however itc bit needs to be manupulated >as well. In order to not expose additional property for itc, >for sake of simplicity it was decided to bind those together >in same "content type" property. > > v4: > * Added it_content checking in intel_digital_connector_atomic_check. >Fixed documentation for new content type enum. > > v5: > * Moved patch revision's description to commit messages. > > Signed-off-by: Stanislav Lisovskiy The property documentation to tie all the bits together (property, helper, internals) is missing. It should be in https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#standard-connector-properties Probably want to start with an intro-paragraph for HDMI properties (someone should document the broadcast prop too eventually). Pls also make sure the resulting docs look pretty and that it's all nicely hyperlink: $ make htmldocs Thanks, Daniel > --- > Documentation/gpu/kms-properties.csv | 1 + > drivers/gpu/drm/drm_atomic.c | 17 ++ > drivers/gpu/drm/drm_connector.c | 51 > drivers/gpu/drm/drm_edid.c | 2 ++ > include/drm/drm_connector.h | 18 ++ > include/drm/drm_mode_config.h| 5 +++ > include/uapi/drm/drm_mode.h | 7 > 7 files changed, 101 insertions(+) > > diff --git a/Documentation/gpu/kms-properties.csv > b/Documentation/gpu/kms-properties.csv > index 6b28b014cb7d..a91c9211b8d6 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=0x",Connector,property > to suggest an X offset for a connector > ,,“suggested Y”,RANGE,"Min=0, Max=0x",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 7d25c42f22db..479499f5848e 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -1266,6 +1266,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. > + */ > + state->content_type = val & 3; > + state->it_content = val >> 2; > } else if (property == connector->scaling_mode_property) { > state->scaling_mode = val; > } else if (property == connector->content_protection_property) { > @@ -1351,6 +1360,14 @@ drm_atomic_connector_get_property(struct drm_connector > *connector, > *val = state->link_status; > } else if (property == config->aspect_ratio_property) { > *val = state->picture_aspect_ratio; > + } 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. > +