Re: [PATCH v3 1/3] drm/connector: Add generic underscan properties
On Mon, Dec 03, 2018 at 07:50:53AM -0800, Eric Anholt wrote:
> Boris Brezillon writes:
>
> > On Mon, 3 Dec 2018 16:40:11 +0200
> > Ville Syrjälä wrote:
> >
> >> On Thu, Nov 22, 2018 at 12:23:29PM +0100, Boris Brezillon wrote:
> >> > @@ -924,6 +978,29 @@ struct drm_connector {
> >> > */
> >> > struct drm_property_blob *path_blob_ptr;
> >> >
> >> > +/**
> >> > + * @underscan_mode_property: Optional connector underscan mode.
> >> > Used by
> >> > + * the driver to scale the output image and compensate an
> >> > overscan done
> >> > + * on the display side.
> >> > + */
> >> > +struct drm_property *underscan_mode_property;
> >> > +
> >> > +/**
> >> > + * @underscan_hborder_property: Optional connector underscan
> >> > horizontal
> >> > + * border (expressed in pixels). Used by the driver to adjust
> >> > the
> >> > + * output image position and compensate an overscan done on the
> >> > display
> >> > + * side.
> >> > + */
> >> > +struct drm_property *underscan_hborder_property;
> >> > +
> >> > +/**
> >> > + * @underscan_hborder_property: Optional connector underscan
> >> > vertical
> >> > + * border (expressed in pixels). Used by the driver to adjust
> >> > the
> >> > + * output image position and compensate an overscan done on the
> >> > display
> >> > + * side.
> >> > + */
> >> > +struct drm_property *underscan_vborder_property;
> >>
> >> I'm wondering why we're adding these new props when we already have the
> >> (slightly more flexible) margin properties for TV out. We could just
> >> reuse those AFAICS.
> >
> > I'm not against the idea, but I can't use
> > drm_mode_create_tv_properties() directly, as most props created by this
> > function are not applicable to an HDMI displays. Should I move the
> > margins props out of the tv_connector_state and provide new helpers to
> > create those props?
>
> TV margin props look good to me, FWIW.
Yeah extracting the margin props from the tv props sounds like a good
idea. If we go full ocd we'd also split out margin_connector_state or
something like that (should be doable with some cocci), but not sure
that's fully worth it. Tuning margins is largely an analog TV issue I
think, so could just leave them there.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v3 1/3] drm/connector: Add generic underscan properties
Boris Brezillon writes:
> On Mon, 3 Dec 2018 16:40:11 +0200
> Ville Syrjälä wrote:
>
>> On Thu, Nov 22, 2018 at 12:23:29PM +0100, Boris Brezillon wrote:
>> > @@ -924,6 +978,29 @@ struct drm_connector {
>> > */
>> >struct drm_property_blob *path_blob_ptr;
>> >
>> > + /**
>> > + * @underscan_mode_property: Optional connector underscan mode. Used by
>> > + * the driver to scale the output image and compensate an overscan done
>> > + * on the display side.
>> > + */
>> > + struct drm_property *underscan_mode_property;
>> > +
>> > + /**
>> > + * @underscan_hborder_property: Optional connector underscan horizontal
>> > + * border (expressed in pixels). Used by the driver to adjust the
>> > + * output image position and compensate an overscan done on the display
>> > + * side.
>> > + */
>> > + struct drm_property *underscan_hborder_property;
>> > +
>> > + /**
>> > + * @underscan_hborder_property: Optional connector underscan vertical
>> > + * border (expressed in pixels). Used by the driver to adjust the
>> > + * output image position and compensate an overscan done on the display
>> > + * side.
>> > + */
>> > + struct drm_property *underscan_vborder_property;
>>
>> I'm wondering why we're adding these new props when we already have the
>> (slightly more flexible) margin properties for TV out. We could just
>> reuse those AFAICS.
>
> I'm not against the idea, but I can't use
> drm_mode_create_tv_properties() directly, as most props created by this
> function are not applicable to an HDMI displays. Should I move the
> margins props out of the tv_connector_state and provide new helpers to
> create those props?
TV margin props look good to me, FWIW.
signature.asc
Description: PGP signature
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v3 1/3] drm/connector: Add generic underscan properties
On Mon, 3 Dec 2018 16:40:11 +0200
Ville Syrjälä wrote:
> On Thu, Nov 22, 2018 at 12:23:29PM +0100, Boris Brezillon wrote:
> > @@ -924,6 +978,29 @@ struct drm_connector {
> > */
> > struct drm_property_blob *path_blob_ptr;
> >
> > + /**
> > +* @underscan_mode_property: Optional connector underscan mode. Used by
> > +* the driver to scale the output image and compensate an overscan done
> > +* on the display side.
> > +*/
> > + struct drm_property *underscan_mode_property;
> > +
> > + /**
> > +* @underscan_hborder_property: Optional connector underscan horizontal
> > +* border (expressed in pixels). Used by the driver to adjust the
> > +* output image position and compensate an overscan done on the display
> > +* side.
> > +*/
> > + struct drm_property *underscan_hborder_property;
> > +
> > + /**
> > +* @underscan_hborder_property: Optional connector underscan vertical
> > +* border (expressed in pixels). Used by the driver to adjust the
> > +* output image position and compensate an overscan done on the display
> > +* side.
> > +*/
> > + struct drm_property *underscan_vborder_property;
>
> I'm wondering why we're adding these new props when we already have the
> (slightly more flexible) margin properties for TV out. We could just
> reuse those AFAICS.
I'm not against the idea, but I can't use
drm_mode_create_tv_properties() directly, as most props created by this
function are not applicable to an HDMI displays. Should I move the
margins props out of the tv_connector_state and provide new helpers to
create those props?
>
> > +
> > #define DRM_CONNECTOR_POLL_HPD (1 << 0)
> > #define DRM_CONNECTOR_POLL_CONNECT (1 << 1)
> > #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2)
> > @@ -1180,6 +1257,9 @@ int drm_mode_create_dvi_i_properties(struct
> > drm_device *dev);
> > int drm_mode_create_tv_properties(struct drm_device *dev,
> > unsigned int num_modes,
> > const char * const modes[]);
> > +int drm_connector_attach_underscan_properties(struct drm_connector
> > *connector,
> > + u32 mode_mask, u64 max_hborder,
> > + u64 max_vborder);
> > int drm_mode_create_scaling_mode_property(struct drm_device *dev);
> > int drm_connector_attach_content_type_property(struct drm_connector *dev);
> > int drm_connector_attach_scaling_mode_property(struct drm_connector
> > *connector,
> > --
> > 2.17.1
>
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v3 1/3] drm/connector: Add generic underscan properties
On Thu, Nov 22, 2018 at 12:23:29PM +0100, Boris Brezillon wrote:
> @@ -924,6 +978,29 @@ struct drm_connector {
>*/
> struct drm_property_blob *path_blob_ptr;
>
> + /**
> + * @underscan_mode_property: Optional connector underscan mode. Used by
> + * the driver to scale the output image and compensate an overscan done
> + * on the display side.
> + */
> + struct drm_property *underscan_mode_property;
> +
> + /**
> + * @underscan_hborder_property: Optional connector underscan horizontal
> + * border (expressed in pixels). Used by the driver to adjust the
> + * output image position and compensate an overscan done on the display
> + * side.
> + */
> + struct drm_property *underscan_hborder_property;
> +
> + /**
> + * @underscan_hborder_property: Optional connector underscan vertical
> + * border (expressed in pixels). Used by the driver to adjust the
> + * output image position and compensate an overscan done on the display
> + * side.
> + */
> + struct drm_property *underscan_vborder_property;
I'm wondering why we're adding these new props when we already have the
(slightly more flexible) margin properties for TV out. We could just
reuse those AFAICS.
> +
> #define DRM_CONNECTOR_POLL_HPD (1 << 0)
> #define DRM_CONNECTOR_POLL_CONNECT (1 << 1)
> #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2)
> @@ -1180,6 +1257,9 @@ int drm_mode_create_dvi_i_properties(struct drm_device
> *dev);
> int drm_mode_create_tv_properties(struct drm_device *dev,
> unsigned int num_modes,
> const char * const modes[]);
> +int drm_connector_attach_underscan_properties(struct drm_connector
> *connector,
> + u32 mode_mask, u64 max_hborder,
> + u64 max_vborder);
> int drm_mode_create_scaling_mode_property(struct drm_device *dev);
> int drm_connector_attach_content_type_property(struct drm_connector *dev);
> int drm_connector_attach_scaling_mode_property(struct drm_connector
> *connector,
> --
> 2.17.1
--
Ville Syrjälä
Intel
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v3 1/3] drm/connector: Add generic underscan properties
Brian Starkey writes: > Hi Boris, > > Just because I happened to read the docs in here, one typo below: > > On Thu, Nov 22, 2018 at 12:23:29PM +0100, Boris Brezillon wrote: >>diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c >>index c555e17ab8d7..170317248da6 100644 >>--- a/drivers/gpu/drm/drm_connector.c >>+++ b/drivers/gpu/drm/drm_connector.c >>@@ -971,6 +971,38 @@ DRM_ENUM_NAME_FN(drm_get_content_protection_name, >>drm_cp_enum_list) >> * can also expose this property to external outputs, in which case they >> * must support "None", which should be the default (since external screens >> * have a built-in scaler). >>+ * >>+ * Connectors for non-analog outputs may also have standardized underscan >>+ * properties (drivers can set this up by calling >>+ * drm_connector_attach_content_protection_property() on initialization): > > Should be drm_connector_attach_underscan_properties() Other than this typo, this series is: Reviewed-by: Eric Anholt signature.asc Description: PGP signature ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v3 1/3] drm/connector: Add generic underscan properties
Hi Boris,
Just because I happened to read the docs in here, one typo below:
On Thu, Nov 22, 2018 at 12:23:29PM +0100, Boris Brezillon wrote:
>We have 3 drivers defining the "underscan", "underscan hborder" and
>"underscan vborder" properties (radeon, amd and nouveau) and we are
>about to add the same kind of thing in VC4.
>
>Define generic underscan props and add new fields to the drm_connector
>state so that the property parsing logic can be shared by all DRM
>drivers.
>
>A driver can now attach underscan properties to its connector through
>the drm_connector_attach_underscan_properties() helper, and can
>check/apply the underscan setup based on the
>drm_connector_state->underscan fields.
>
>Signed-off-by: Boris Brezillon
>---
>Changes in v3:
>- None
>
>Changes in v2:
>- Add a new section in the connector props doc to describe underscan
> props
>- Fix description of auto mode (auto means apply underscan for HDMI
> monitors only)
>- Fix description of vborder/hborder:
>right_border = left_border = hborder
>top_border = bottom_border = vborder
> not
>right_border = left_border = hborder / 2
>top_border = bottom_border = vborder / 2
>- Rename drm_underscan into drm_underscan_state
>---
> drivers/gpu/drm/drm_atomic_uapi.c | 12 +++
> drivers/gpu/drm/drm_connector.c | 127 ++
> include/drm/drm_connector.h | 80 +++
> 3 files changed, 219 insertions(+)
>
>diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
>b/drivers/gpu/drm/drm_atomic_uapi.c
>index d5b7f315098c..39db6e31c565 100644
>--- a/drivers/gpu/drm/drm_atomic_uapi.c
>+++ b/drivers/gpu/drm/drm_atomic_uapi.c
>@@ -740,6 +740,12 @@ static int drm_atomic_connector_set_property(struct
>drm_connector *connector,
>
> return set_out_fence_for_connector(state->state, connector,
> fence_ptr);
>+ } else if (property == connector->underscan_mode_property) {
>+ state->underscan.mode = val;
>+ } else if (property == connector->underscan_hborder_property) {
>+ state->underscan.hborder = val;
>+ } else if (property == connector->underscan_vborder_property) {
>+ state->underscan.vborder = val;
> } else if (connector->funcs->atomic_set_property) {
> return connector->funcs->atomic_set_property(connector,
> state, property, val);
>@@ -799,6 +805,12 @@ drm_atomic_connector_get_property(struct drm_connector
>*connector,
> *val = state->scaling_mode;
> } else if (property == connector->content_protection_property) {
> *val = state->content_protection;
>+ } else if (property == connector->underscan_mode_property) {
>+ *val = state->underscan.mode;
>+ } else if (property == connector->underscan_hborder_property) {
>+ *val = state->underscan.hborder;
>+ } else if (property == connector->underscan_vborder_property) {
>+ *val = state->underscan.vborder;
> } else if (property == config->writeback_fb_id_property) {
> /* Writeback framebuffer is one-shot, write and forget */
> *val = 0;
>diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>index c555e17ab8d7..170317248da6 100644
>--- a/drivers/gpu/drm/drm_connector.c
>+++ b/drivers/gpu/drm/drm_connector.c
>@@ -971,6 +971,38 @@ DRM_ENUM_NAME_FN(drm_get_content_protection_name,
>drm_cp_enum_list)
> *can also expose this property to external outputs, in which case they
> *must support "None", which should be the default (since external screens
> *have a built-in scaler).
>+ *
>+ * Connectors for non-analog outputs may also have standardized underscan
>+ * properties (drivers can set this up by calling
>+ * drm_connector_attach_content_protection_property() on initialization):
Should be drm_connector_attach_underscan_properties()
Cheers,
-Brian
>+ *
>+ * underscan:
>+ *This properties defines whether underscan is activated or not, and when
>+ *it is activated, how the horizontal and vertical borders are calculated:
>+ *
>+ *off:
>+ *Underscan is disabled. The output image shouldn't be scaled to
>+ *take screen borders into account.
>+ *on:
>+ *Underscan is activated and horizontal and vertical borders are
>+ *specified through the "underscan hborder" and
>+ *"underscan vborder" properties.
>+ *auto:
>+ *Underscan is activated only for HDMI monitors.
>+ *
>+ * underscan hborder:
>+ *Horizontal border expressed in pixels. The border is symmetric, which
>+ *means you'll have a border of 'hborder' pixels on the left and on the
>+ *same border on the right.
>+ *When this value is 0 and underscan is "on" or "auto", the driver will
>+ *pick a default value (the default value is driver dependent).
>+ *
>+ * underscan vborder:
>+ *
[PATCH v3 1/3] drm/connector: Add generic underscan properties
We have 3 drivers defining the "underscan", "underscan hborder" and
"underscan vborder" properties (radeon, amd and nouveau) and we are
about to add the same kind of thing in VC4.
Define generic underscan props and add new fields to the drm_connector
state so that the property parsing logic can be shared by all DRM
drivers.
A driver can now attach underscan properties to its connector through
the drm_connector_attach_underscan_properties() helper, and can
check/apply the underscan setup based on the
drm_connector_state->underscan fields.
Signed-off-by: Boris Brezillon
---
Changes in v3:
- None
Changes in v2:
- Add a new section in the connector props doc to describe underscan
props
- Fix description of auto mode (auto means apply underscan for HDMI
monitors only)
- Fix description of vborder/hborder:
right_border = left_border = hborder
top_border = bottom_border = vborder
not
right_border = left_border = hborder / 2
top_border = bottom_border = vborder / 2
- Rename drm_underscan into drm_underscan_state
---
drivers/gpu/drm/drm_atomic_uapi.c | 12 +++
drivers/gpu/drm/drm_connector.c | 127 ++
include/drm/drm_connector.h | 80 +++
3 files changed, 219 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
b/drivers/gpu/drm/drm_atomic_uapi.c
index d5b7f315098c..39db6e31c565 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -740,6 +740,12 @@ static int drm_atomic_connector_set_property(struct
drm_connector *connector,
return set_out_fence_for_connector(state->state, connector,
fence_ptr);
+ } else if (property == connector->underscan_mode_property) {
+ state->underscan.mode = val;
+ } else if (property == connector->underscan_hborder_property) {
+ state->underscan.hborder = val;
+ } else if (property == connector->underscan_vborder_property) {
+ state->underscan.vborder = val;
} else if (connector->funcs->atomic_set_property) {
return connector->funcs->atomic_set_property(connector,
state, property, val);
@@ -799,6 +805,12 @@ drm_atomic_connector_get_property(struct drm_connector
*connector,
*val = state->scaling_mode;
} else if (property == connector->content_protection_property) {
*val = state->content_protection;
+ } else if (property == connector->underscan_mode_property) {
+ *val = state->underscan.mode;
+ } else if (property == connector->underscan_hborder_property) {
+ *val = state->underscan.hborder;
+ } else if (property == connector->underscan_vborder_property) {
+ *val = state->underscan.vborder;
} else if (property == config->writeback_fb_id_property) {
/* Writeback framebuffer is one-shot, write and forget */
*val = 0;
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index c555e17ab8d7..170317248da6 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -971,6 +971,38 @@ DRM_ENUM_NAME_FN(drm_get_content_protection_name,
drm_cp_enum_list)
* can also expose this property to external outputs, in which case they
* must support "None", which should be the default (since external screens
* have a built-in scaler).
+ *
+ * Connectors for non-analog outputs may also have standardized underscan
+ * properties (drivers can set this up by calling
+ * drm_connector_attach_content_protection_property() on initialization):
+ *
+ * underscan:
+ * This properties defines whether underscan is activated or not, and when
+ * it is activated, how the horizontal and vertical borders are calculated:
+ *
+ * off:
+ * Underscan is disabled. The output image shouldn't be scaled to
+ * take screen borders into account.
+ * on:
+ * Underscan is activated and horizontal and vertical borders are
+ * specified through the "underscan hborder" and
+ * "underscan vborder" properties.
+ * auto:
+ * Underscan is activated only for HDMI monitors.
+ *
+ * underscan hborder:
+ * Horizontal border expressed in pixels. The border is symmetric, which
+ * means you'll have a border of 'hborder' pixels on the left and on the
+ * same border on the right.
+ * When this value is 0 and underscan is "on" or "auto", the driver will
+ * pick a default value (the default value is driver dependent).
+ *
+ * underscan vborder:
+ * Vertical border expressed in pixels. The border is symmetric, which
+ * means you'll have a border of 'vborder' pixels on the top and the same
+ * border on the bottom.
+ * When this value is 0 and underscan is "on" or "auto", the driver will
+ * pick a default
