Re: [PATCH 1/3] drm/connector: Add generic underscan properties

[Boris Brezillon]

On Mon, 7 May 2018 17:25:30 +0200
Daniel Vetter  wrote:

> On Mon, May 07, 2018 at 05:15:33PM +0200, Daniel Vetter wrote:
> > On Mon, May 07, 2018 at 04:44:32PM +0200, 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 
> > > ---
> > >  drivers/gpu/drm/drm_atomic.c|  12 
> > >  drivers/gpu/drm/drm_connector.c | 120 
> > > 
> > >  include/drm/drm_connector.h |  78 ++
> > >  3 files changed, 210 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > index dc850b4b6e21..b7312bd172c9 100644
> > > --- a/drivers/gpu/drm/drm_atomic.c
> > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > @@ -1278,6 +1278,12 @@ static int 
> > > drm_atomic_connector_set_property(struct drm_connector *connector,
> > >   return -EINVAL;
> > >   }
> > >   state->content_protection = val;
> > > + } 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);
> > > @@ -1359,6 +1365,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 (connector->funcs->atomic_get_property) {
> > >   return connector->funcs->atomic_get_property(connector,
> > >   state, property, val);
> > > diff --git a/drivers/gpu/drm/drm_connector.c 
> > > b/drivers/gpu/drm/drm_connector.c
> > > index dfc8ca1e9413..9937390b8a25 100644
> > > --- a/drivers/gpu/drm/drm_connector.c
> > > +++ b/drivers/gpu/drm/drm_connector.c
> > > @@ -914,6 +914,31 @@ 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).  
> > 
> > I think a new section here would be good, to make it more obvious this is
> > a group of properties. Plus a reference to
> > drm_connector_attach_underscan_properties().
> >   
> > > + *
> > > + * 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 and horizontal and vertical 
> > > borders are
> > > + *   automatically chosen by the driver.
> > > + *
> > > + * underscan hborder:
> > > + *   Horizontal border expressed in pixels. The border is symmetric, 
> > > which
> > > + *   means you'll have half of this value placed on the left and the 
> > > other
> > > + *   half on the right.
> > > + *
> > > + * underscan vborder:
> > > + *   Vertical border expressed in pixels. The border is symmetric, 
> > > which
> > > + *   means you'll have half of this value placed on the top and the 
> 

Re: [PATCH 1/3] drm/connector: Add generic underscan properties

[Boris Brezillon]

On Tue, 8 May 2018 10:18:10 +1000
Ben Skeggs  wrote:

> On 8 May 2018 at 04:26, Harry Wentland  wrote:
> >
> >
> > On 2018-05-07 12:19 PM, Boris Brezillon wrote:  
> >> On Mon, 7 May 2018 18:01:44 +0300
> >> Ville Syrjälä  wrote:
> >>  
> >>> On Mon, May 07, 2018 at 04:44:32PM +0200, 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 
>  ---
>   drivers/gpu/drm/drm_atomic.c|  12 
>   drivers/gpu/drm/drm_connector.c | 120 
>  
>   include/drm/drm_connector.h |  78 ++
>   3 files changed, 210 insertions(+)
> 
>  diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>  index dc850b4b6e21..b7312bd172c9 100644
>  --- a/drivers/gpu/drm/drm_atomic.c
>  +++ b/drivers/gpu/drm/drm_atomic.c
>  @@ -1278,6 +1278,12 @@ static int 
>  drm_atomic_connector_set_property(struct drm_connector *connector,
>  return -EINVAL;
>  }
>  state->content_protection = val;
>  +   } 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);
>  @@ -1359,6 +1365,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 (connector->funcs->atomic_get_property) {
>  return connector->funcs->atomic_get_property(connector,
>  state, property, val);
>  diff --git a/drivers/gpu/drm/drm_connector.c 
>  b/drivers/gpu/drm/drm_connector.c
>  index dfc8ca1e9413..9937390b8a25 100644
>  --- a/drivers/gpu/drm/drm_connector.c
>  +++ b/drivers/gpu/drm/drm_connector.c
>  @@ -914,6 +914,31 @@ 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).
>  + *
>  + * 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.  
> >>>
> >>> How is the output scaled?  
> >>
> >> In HW. The formula is
> >>
> >>   hfactor = (hdisplay - hborder) / hdisplay
> >>   vfactor = (vdisplay - vborder) / vdisplay
> >>  
> >>> What does the user mode hdisplay/vdisplay mean
> >>> in this case?  
> >>
> >> The same as before this patch: the output resolution. You just add
> >> black margins.
> >>  
> >>> What if I want underscan without scaling?  
> >>
> >> Then don't involve the DRM driver and do that from userspace: just
> >> fill the visible portion of the framebuffer and leave the rest black.
> >> There nothing the DRM driver 

Re: [PATCH 1/3] drm/connector: Add generic underscan properties

[Ben Skeggs]

On 8 May 2018 at 04:26, Harry Wentland  wrote:
>
>
> On 2018-05-07 12:19 PM, Boris Brezillon wrote:
>> On Mon, 7 May 2018 18:01:44 +0300
>> Ville Syrjälä  wrote:
>>
>>> On Mon, May 07, 2018 at 04:44:32PM +0200, 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 
 ---
  drivers/gpu/drm/drm_atomic.c|  12 
  drivers/gpu/drm/drm_connector.c | 120 
 
  include/drm/drm_connector.h |  78 ++
  3 files changed, 210 insertions(+)

 diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
 index dc850b4b6e21..b7312bd172c9 100644
 --- a/drivers/gpu/drm/drm_atomic.c
 +++ b/drivers/gpu/drm/drm_atomic.c
 @@ -1278,6 +1278,12 @@ static int drm_atomic_connector_set_property(struct 
 drm_connector *connector,
 return -EINVAL;
 }
 state->content_protection = val;
 +   } 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);
 @@ -1359,6 +1365,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 (connector->funcs->atomic_get_property) {
 return connector->funcs->atomic_get_property(connector,
 state, property, val);
 diff --git a/drivers/gpu/drm/drm_connector.c 
 b/drivers/gpu/drm/drm_connector.c
 index dfc8ca1e9413..9937390b8a25 100644
 --- a/drivers/gpu/drm/drm_connector.c
 +++ b/drivers/gpu/drm/drm_connector.c
 @@ -914,6 +914,31 @@ 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).
 + *
 + * 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.
>>>
>>> How is the output scaled?
>>
>> In HW. The formula is
>>
>>   hfactor = (hdisplay - hborder) / hdisplay
>>   vfactor = (vdisplay - vborder) / vdisplay
>>
>>> What does the user mode hdisplay/vdisplay mean
>>> in this case?
>>
>> The same as before this patch: the output resolution. You just add
>> black margins.
>>
>>> What if I want underscan without scaling?
>>
>> Then don't involve the DRM driver and do that from userspace: just
>> fill the visible portion of the framebuffer and leave the rest black.
>> There nothing the DRM driver can do to help with that, except maybe
>> exposing the information about the active area of the screen. It would
>> be nice to do that, but that means patching all userspace libs to take
>> this into account.
>>
>>>
 + * auto:
 + * Underscan is activated and horizontal and vertical borders are
 + * 

Re: [PATCH 1/3] drm/connector: Add generic underscan properties

[kbuild test robot]

Hi Boris,

I love your patch! Yet something to improve:

[auto build test ERROR on anholt/for-next]
[also build test ERROR on v4.17-rc4 next-20180507]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Boris-Brezillon/drm-connector-Provide-generic-support-for-underscan/20180508-022336
base:   https://github.com/anholt/linux for-next
config: x86_64-randconfig-x010-201818 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64 

All error/warnings (new ones prefixed by >>):

   drivers/gpu//drm/drm_connector.c: In function 
'drm_connector_attach_underscan_properties':
>> drivers/gpu//drm/drm_connector.c:1188:50: warning: passing argument 3 of 
>> 'drm_property_add_enum' makes integer from pointer without a cast 
>> [-Wint-conversion]
  ret = drm_property_add_enum(prop, entry->type, entry->name);
 ^
   In file included from include/drm/drm_crtc.h:42:0,
from include/drm/drmP.h:69,
from drivers/gpu//drm/drm_connector.c:23:
   include/drm/drm_property.h:263:5: note: expected 'uint64_t {aka long long 
unsigned int}' but argument is of type 'const char * const'
int drm_property_add_enum(struct drm_property *property, int index,
^
>> drivers/gpu//drm/drm_connector.c:1188:9: error: too few arguments to 
>> function 'drm_property_add_enum'
  ret = drm_property_add_enum(prop, entry->type, entry->name);
^
   In file included from include/drm/drm_crtc.h:42:0,
from include/drm/drmP.h:69,
from drivers/gpu//drm/drm_connector.c:23:
   include/drm/drm_property.h:263:5: note: declared here
int drm_property_add_enum(struct drm_property *property, int index,
^

vim +/drm_property_add_enum +1188 drivers/gpu//drm/drm_connector.c

  1141  
  1142  /**
  1143   * drm_connector_attach_underscan_properties - attach atomic underscan
  1144   * properties
  1145   * @connector: connector to attach underscan mode properties on.
  1146   * @mode_mask: bitmask of %DRM_UNDERSCAN_XX modes encoding the supported
  1147   * underscan modes.
  1148   * @max_hborder: maximum size of the horizontal border expressed in 
pixels.
  1149   *   Should be > 0.
  1150   * @max_vborder: maximum size of the vertical border expressed in 
pixels.
  1151   *   Should be > 0.
  1152   *
  1153   * This is used to add support for underscan to atomic drivers.
  1154   * The underscan config will be set to _connector_state.underscan
  1155   * and can be used from _connector_helper_funcs->atomic_check for
  1156   * validation.
  1157   *
  1158   * Returns:
  1159   * Zero on success, negative errno on failure.
  1160   */
  1161  int drm_connector_attach_underscan_properties(struct drm_connector 
*connector,
  1162u32 mode_mask, u64 
max_hborder,
  1163u64 max_vborder)
  1164  {
  1165  unsigned int i, nmodes = 
ARRAY_SIZE(drm_underscan_mode_enum_list);
  1166  struct drm_device *dev = connector->dev;
  1167  struct drm_property *prop;
  1168  
  1169  if (!max_hborder || !max_vborder)
  1170  return -EINVAL;
  1171  
  1172  if (!hweight32(mode_mask) || (mode_mask & ~GENMASK(nmodes - 1, 
0)))
  1173  return -EINVAL;
  1174  
  1175  prop = drm_property_create(dev, DRM_MODE_PROP_ENUM, "underscan",
  1176 hweight32(mode_mask));
  1177  if (!prop)
  1178  return -ENOMEM;
  1179  
  1180  for (i = 0; i < ARRAY_SIZE(drm_underscan_mode_enum_list); i++) {
  1181  const struct drm_prop_enum_list *entry;
  1182  int ret;
  1183  
  1184  if (!(BIT(i) & mode_mask))
  1185  continue;
  1186  
  1187  entry = _underscan_mode_enum_list[i];
> 1188  ret = drm_property_add_enum(prop, entry->type, 
> entry->name);
  1189  if (ret)
  1190  goto err_free_mode_prop;
  1191  }
  1192  
  1193  connector->underscan_mode_property = prop;
  1194  
  1195  prop = drm_property_create_range(dev, 0, "underscan hborder", 0,
  1196   max_hborder);
  1197  if (!prop)
  1198  goto err_free_mode_prop;
  1199  
  1200  connector->underscan_hborder_property = prop;
  1201  
  1202  prop = drm_property_create_range(dev, 0, "underscan vborder", 0,
  1203   

Re: [PATCH 1/3] drm/connector: Add generic underscan properties

[Harry Wentland]

On 2018-05-07 12:19 PM, Boris Brezillon wrote:
> On Mon, 7 May 2018 18:01:44 +0300
> Ville Syrjälä  wrote:
> 
>> On Mon, May 07, 2018 at 04:44:32PM +0200, 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 
>>> ---
>>>  drivers/gpu/drm/drm_atomic.c|  12 
>>>  drivers/gpu/drm/drm_connector.c | 120 
>>> 
>>>  include/drm/drm_connector.h |  78 ++
>>>  3 files changed, 210 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>>> index dc850b4b6e21..b7312bd172c9 100644
>>> --- a/drivers/gpu/drm/drm_atomic.c
>>> +++ b/drivers/gpu/drm/drm_atomic.c
>>> @@ -1278,6 +1278,12 @@ static int drm_atomic_connector_set_property(struct 
>>> drm_connector *connector,
>>> return -EINVAL;
>>> }
>>> state->content_protection = val;
>>> +   } 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);
>>> @@ -1359,6 +1365,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 (connector->funcs->atomic_get_property) {
>>> return connector->funcs->atomic_get_property(connector,
>>> state, property, val);
>>> diff --git a/drivers/gpu/drm/drm_connector.c 
>>> b/drivers/gpu/drm/drm_connector.c
>>> index dfc8ca1e9413..9937390b8a25 100644
>>> --- a/drivers/gpu/drm/drm_connector.c
>>> +++ b/drivers/gpu/drm/drm_connector.c
>>> @@ -914,6 +914,31 @@ 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).
>>> + *
>>> + * 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.  
>>
>> How is the output scaled?
> 
> In HW. The formula is
> 
>   hfactor = (hdisplay - hborder) / hdisplay
>   vfactor = (vdisplay - vborder) / vdisplay
> 
>> What does the user mode hdisplay/vdisplay mean
>> in this case?
> 
> The same as before this patch: the output resolution. You just add
> black margins.
> 
>> What if I want underscan without scaling?
> 
> Then don't involve the DRM driver and do that from userspace: just
> fill the visible portion of the framebuffer and leave the rest black.
> There nothing the DRM driver can do to help with that, except maybe
> exposing the information about the active area of the screen. It would
> be nice to do that, but that means patching all userspace libs to take
> this into account.
> 
>>
>>> + * auto:
>>> + * Underscan is activated and horizontal and vertical borders are
>>> + * automatically chosen by the driver.  
>>
>> Seems overly vague to be useful. You didn't even seem to implement it
>> for vc4.
> 

FWIW, amdgpu treats UNDERSCAN_AUTO like UNDERSCAN_ON. 

Re: [PATCH 1/3] drm/connector: Add generic underscan properties

[Boris Brezillon]

On Mon, 7 May 2018 18:01:44 +0300
Ville Syrjälä  wrote:

> On Mon, May 07, 2018 at 04:44:32PM +0200, 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 
> > ---
> >  drivers/gpu/drm/drm_atomic.c|  12 
> >  drivers/gpu/drm/drm_connector.c | 120 
> > 
> >  include/drm/drm_connector.h |  78 ++
> >  3 files changed, 210 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index dc850b4b6e21..b7312bd172c9 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -1278,6 +1278,12 @@ static int drm_atomic_connector_set_property(struct 
> > drm_connector *connector,
> > return -EINVAL;
> > }
> > state->content_protection = val;
> > +   } 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);
> > @@ -1359,6 +1365,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 (connector->funcs->atomic_get_property) {
> > return connector->funcs->atomic_get_property(connector,
> > state, property, val);
> > diff --git a/drivers/gpu/drm/drm_connector.c 
> > b/drivers/gpu/drm/drm_connector.c
> > index dfc8ca1e9413..9937390b8a25 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -914,6 +914,31 @@ 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).
> > + *
> > + * 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.  
> 
> How is the output scaled?

In HW. The formula is

hfactor = (hdisplay - hborder) / hdisplay
vfactor = (vdisplay - vborder) / vdisplay

> What does the user mode hdisplay/vdisplay mean
> in this case?

The same as before this patch: the output resolution. You just add
black margins.

> What if I want underscan without scaling?

Then don't involve the DRM driver and do that from userspace: just
fill the visible portion of the framebuffer and leave the rest black.
There nothing the DRM driver can do to help with that, except maybe
exposing the information about the active area of the screen. It would
be nice to do that, but that means patching all userspace libs to take
this into account.

> 
> > + * auto:
> > + * Underscan is activated and horizontal and vertical borders are
> > + * automatically chosen by the driver.  
> 
> Seems overly vague to be useful. You didn't even seem to implement it
> for vc4.

Because I don't understand it either. I was just trying to keep things
working for drivers already exposing these properties.

> 
> > + *
> > + * 

Re: [PATCH 1/3] drm/connector: Add generic underscan properties

[Daniel Vetter]

On Mon, May 07, 2018 at 05:15:33PM +0200, Daniel Vetter wrote:
> On Mon, May 07, 2018 at 04:44:32PM +0200, 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 
> > ---
> >  drivers/gpu/drm/drm_atomic.c|  12 
> >  drivers/gpu/drm/drm_connector.c | 120 
> > 
> >  include/drm/drm_connector.h |  78 ++
> >  3 files changed, 210 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index dc850b4b6e21..b7312bd172c9 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -1278,6 +1278,12 @@ static int drm_atomic_connector_set_property(struct 
> > drm_connector *connector,
> > return -EINVAL;
> > }
> > state->content_protection = val;
> > +   } 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);
> > @@ -1359,6 +1365,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 (connector->funcs->atomic_get_property) {
> > return connector->funcs->atomic_get_property(connector,
> > state, property, val);
> > diff --git a/drivers/gpu/drm/drm_connector.c 
> > b/drivers/gpu/drm/drm_connector.c
> > index dfc8ca1e9413..9937390b8a25 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -914,6 +914,31 @@ 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).
> 
> I think a new section here would be good, to make it more obvious this is
> a group of properties. Plus a reference to
> drm_connector_attach_underscan_properties().
> 
> > + *
> > + * 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 and horizontal and vertical borders are
> > + * automatically chosen by the driver.
> > + *
> > + * underscan hborder:
> > + * Horizontal border expressed in pixels. The border is symmetric, which
> > + * means you'll have half of this value placed on the left and the other
> > + * half on the right.
> > + *
> > + * underscan vborder:
> > + * Vertical border expressed in pixels. The border is symmetric, which
> > + * means you'll have half of this value placed on the top and the other
> > + * half on the bottom.
> >   */
> >  
> >  int drm_connector_create_standard_properties(struct drm_device *dev)
> > @@ -1108,6 +1133,101 @@ int drm_mode_create_tv_properties(struct drm_device 
> > *dev,
> >  }
> >  EXPORT_SYMBOL(drm_mode_create_tv_properties);
> >  
> > +static const struct drm_prop_enum_list drm_underscan_mode_enum_list[] = {
> > +   { DRM_UNDERSCAN_OFF, "off" },
> > +   { 

Re: [PATCH 1/3] drm/connector: Add generic underscan properties

[Daniel Vetter]

On Mon, May 07, 2018 at 04:44:32PM +0200, 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 
> ---
>  drivers/gpu/drm/drm_atomic.c|  12 
>  drivers/gpu/drm/drm_connector.c | 120 
> 
>  include/drm/drm_connector.h |  78 ++
>  3 files changed, 210 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index dc850b4b6e21..b7312bd172c9 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1278,6 +1278,12 @@ static int drm_atomic_connector_set_property(struct 
> drm_connector *connector,
>   return -EINVAL;
>   }
>   state->content_protection = val;
> + } 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);
> @@ -1359,6 +1365,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 (connector->funcs->atomic_get_property) {
>   return connector->funcs->atomic_get_property(connector,
>   state, property, val);
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index dfc8ca1e9413..9937390b8a25 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -914,6 +914,31 @@ 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).

I think a new section here would be good, to make it more obvious this is
a group of properties. Plus a reference to
drm_connector_attach_underscan_properties().

> + *
> + * 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 and horizontal and vertical borders are
> + *   automatically chosen by the driver.
> + *
> + * underscan hborder:
> + *   Horizontal border expressed in pixels. The border is symmetric, which
> + *   means you'll have half of this value placed on the left and the other
> + *   half on the right.
> + *
> + * underscan vborder:
> + *   Vertical border expressed in pixels. The border is symmetric, which
> + *   means you'll have half of this value placed on the top and the other
> + *   half on the bottom.
>   */
>  
>  int drm_connector_create_standard_properties(struct drm_device *dev)
> @@ -1108,6 +1133,101 @@ int drm_mode_create_tv_properties(struct drm_device 
> *dev,
>  }
>  EXPORT_SYMBOL(drm_mode_create_tv_properties);
>  
> +static const struct drm_prop_enum_list drm_underscan_mode_enum_list[] = {
> + { DRM_UNDERSCAN_OFF, "off" },
> + { DRM_UNDERSCAN_ON, "on" },
> + { DRM_UNDERSCAN_AUTO, "auto" },
> +};
> +
> +/**
> + * drm_connector_attach_underscan_properties - attach atomic underscan
> + * 

Re: [PATCH 1/3] drm/connector: Add generic underscan properties

[Ville Syrjälä]

On Mon, May 07, 2018 at 04:44:32PM +0200, 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 
> ---
>  drivers/gpu/drm/drm_atomic.c|  12 
>  drivers/gpu/drm/drm_connector.c | 120 
> 
>  include/drm/drm_connector.h |  78 ++
>  3 files changed, 210 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index dc850b4b6e21..b7312bd172c9 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1278,6 +1278,12 @@ static int drm_atomic_connector_set_property(struct 
> drm_connector *connector,
>   return -EINVAL;
>   }
>   state->content_protection = val;
> + } 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);
> @@ -1359,6 +1365,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 (connector->funcs->atomic_get_property) {
>   return connector->funcs->atomic_get_property(connector,
>   state, property, val);
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index dfc8ca1e9413..9937390b8a25 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -914,6 +914,31 @@ 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).
> + *
> + * 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.

How is the output scaled? What does the user mode hdisplay/vdisplay mean
in this case? What if I want underscan without scaling?

> + *   auto:
> + *   Underscan is activated and horizontal and vertical borders are
> + *   automatically chosen by the driver.

Seems overly vague to be useful. You didn't even seem to implement it
for vc4.

> + *
> + * underscan hborder:
> + *   Horizontal border expressed in pixels. The border is symmetric, which
> + *   means you'll have half of this value placed on the left and the other
> + *   half on the right.

Seems like a slightly odd way to specify this. I think for the TV margins
we have one value for each edge.

> + *
> + * underscan vborder:
> + *   Vertical border expressed in pixels. The border is symmetric, which
> + *   means you'll have half of this value placed on the top and the other
> + *   half on the bottom.
>   */
>  
>  int drm_connector_create_standard_properties(struct drm_device *dev)
> @@ -1108,6 +1133,101 @@ int drm_mode_create_tv_properties(struct drm_device 
> *dev,
>  }
>  EXPORT_SYMBOL(drm_mode_create_tv_properties);
>  
> +static const struct drm_prop_enum_list drm_underscan_mode_enum_list[] = {
> + { DRM_UNDERSCAN_OFF, "off" },
> + { DRM_UNDERSCAN_ON, "on" },
> +