diff mbox series

[v2,4/5] drm/modes: Parse overscan properties

Message ID af8bf60b421190a7c43d90d68cea92c02ebeebce.1554988934.git-series.maxime.ripard@bootlin.com (mailing list archive)
State New, archived
Headers show
Series drm/vc4: Allow for more boot-time configuration | expand

Commit Message

Maxime Ripard April 11, 2019, 1:22 p.m. UTC
Properly configuring the overscan properties might be needed for the
initial setup of the framebuffer for display that still have overscan.
Let's allow for more properties on the kernel command line to setup each
margin.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 47 ++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/drm_modes.c     | 44 ++++++++++++++++++++++++++++++++-
 include/drm/drm_connector.h     |  1 +-
 3 files changed, 92 insertions(+)

Comments

Daniel Vetter April 15, 2019, 4:45 p.m. UTC | #1
On Thu, Apr 11, 2019 at 03:22:42PM +0200, Maxime Ripard wrote:
> Properly configuring the overscan properties might be needed for the
> initial setup of the framebuffer for display that still have overscan.
> Let's allow for more properties on the kernel command line to setup each
> margin.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 47 ++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/drm_modes.c     | 44 ++++++++++++++++++++++++++++++++-
>  include/drm/drm_connector.h     |  1 +-
>  3 files changed, 92 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 8781897559b2..4e403fe1f451 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -2567,6 +2567,51 @@ static void drm_setup_crtc_rotation(struct drm_fb_helper *fb_helper,
>  	fb_helper->sw_rotations |= DRM_MODE_ROTATE_0;
>  }
>  
> +static void drm_setup_connector_margins(struct drm_connector *connector)
> +{
> +	struct drm_cmdline_mode *cmdline = &connector->cmdline_mode;
> +	struct drm_modeset_acquire_ctx ctx;
> +	struct drm_atomic_state *state;
> +	struct drm_device *dev = connector->dev;
> +	int ret;
> +
> +	if (!drm_drv_uses_atomic_modeset(dev))
> +		return;
> +
> +	drm_modeset_acquire_init(&ctx, 0);

You can't do more than one atomic commit, and you need to restore
everything (since userspace might have changed stuff) every time. That's
why all the atomic code goes through restore_fbdev_mode_atomic eventually.
See also how rotation is handled already.

I guess you can omit handling this for non-atomic, since no one cares.

> +
> +	state = drm_atomic_state_alloc(dev);
> +	state->acquire_ctx = &ctx;
> +
> +retry:
> +	drm_atomic_set_property(state, &connector->base,
> +				dev->mode_config.tv_left_margin_property,
> +				cmdline->overscan_left);
> +
> +	drm_atomic_set_property(state, &connector->base,
> +				dev->mode_config.tv_right_margin_property,
> +				cmdline->overscan_right);
> +
> +	drm_atomic_set_property(state, &connector->base,
> +				dev->mode_config.tv_top_margin_property,
> +				cmdline->overscan_top);
> +
> +	drm_atomic_set_property(state, &connector->base,
> +				dev->mode_config.tv_bottom_margin_property,
> +				cmdline->overscan_bottom);
> +
> +	ret = drm_atomic_commit(state);
> +	if (ret == -EDEADLK) {
> +		drm_atomic_state_clear(state);
> +		drm_modeset_backoff(&ctx);
> +		goto retry;
> +	}
> +
> +	drm_atomic_state_put(state);
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +}
> +
>  static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
>  			    u32 width, u32 height)
>  {
> @@ -2680,6 +2725,8 @@ static void drm_setup_crtcs_fb(struct drm_fb_helper *fb_helper)
>  		struct drm_connector *connector =
>  					fb_helper->connector_info[i]->connector;
>  
> +		drm_setup_connector_margins(connector);
> +
>  		/* use first connected connector for the physical dimensions */
>  		if (connector->status == connector_status_connected) {
>  			info->var.width = connector->display_info.width_mm;
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index ac8d70b92b62..493ba3ccde70 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -1586,6 +1586,50 @@ static int drm_mode_parse_cmdline_options(char *str, size_t len,
>  		} else if (!strncmp(option, "reflect_y", delim - option)) {
>  			rotation |= DRM_MODE_REFLECT_Y;
>  			sep = delim;
> +		} else if (!strncmp(option, "overscan_right", delim - option)) {
> +			const char *value = delim + 1;
> +			unsigned int margin;
> +
> +			margin = simple_strtol(value, &sep, 10);
> +
> +			/* Make sure we have parsed something */
> +			if (sep == value)
> +				return -EINVAL;
> +
> +			mode->overscan_right = margin;
> +		} else if (!strncmp(option, "overscan_left", delim - option)) {
> +			const char *value = delim + 1;
> +			unsigned int margin;
> +
> +			margin = simple_strtol(value, &sep, 10);
> +
> +			/* Make sure we have parsed something */
> +			if (sep == value)
> +				return -EINVAL;
> +
> +			mode->overscan_left = margin;
> +		} else if (!strncmp(option, "overscan_top", delim - option)) {
> +			const char *value = delim + 1;
> +			unsigned int margin;
> +
> +			margin = simple_strtol(value, &sep, 10);
> +
> +			/* Make sure we have parsed something */
> +			if (sep == value)
> +				return -EINVAL;
> +
> +			mode->overscan_top = margin;
> +		} else if (!strncmp(option, "overscan_bottom", delim - option)) {
> +			const char *value = delim + 1;
> +			unsigned int margin;
> +
> +			margin = simple_strtol(value, &sep, 10);
> +
> +			/* Make sure we have parsed something */
> +			if (sep == value)
> +				return -EINVAL;
> +
> +			mode->overscan_bottom = margin;
>  		} else {
>  			return -EINVAL;
>  		}

This will conflict quite a bit with Noralf's series to extract the modeset
code from fb helpers. Cross review might be a good idea.
-Daniel

> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index dfe7f2304b35..44d9885dd401 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -903,6 +903,7 @@ struct drm_cmdline_mode {
>  	int xres, yres;
>  	int bpp;
>  	int refresh;
> +	int overscan_right, overscan_top, overscan_left, overscan_bottom;
>  	bool rb;
>  	bool interlace;
>  	bool cvt;
> -- 
> git-series 0.9.1
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Noralf Trønnes April 16, 2019, 2:52 p.m. UTC | #2
Den 11.04.2019 15.22, skrev Maxime Ripard:
> Properly configuring the overscan properties might be needed for the
> initial setup of the framebuffer for display that still have overscan.
> Let's allow for more properties on the kernel command line to setup each
> margin.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 47 ++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/drm_modes.c     | 44 ++++++++++++++++++++++++++++++++-
>  include/drm/drm_connector.h     |  1 +-
>  3 files changed, 92 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 8781897559b2..4e403fe1f451 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -2567,6 +2567,51 @@ static void drm_setup_crtc_rotation(struct drm_fb_helper *fb_helper,
>  	fb_helper->sw_rotations |= DRM_MODE_ROTATE_0;
>  }
>  
> +static void drm_setup_connector_margins(struct drm_connector *connector)
> +{
> +	struct drm_cmdline_mode *cmdline = &connector->cmdline_mode;
> +	struct drm_modeset_acquire_ctx ctx;
> +	struct drm_atomic_state *state;
> +	struct drm_device *dev = connector->dev;
> +	int ret;
> +
> +	if (!drm_drv_uses_atomic_modeset(dev))
> +		return;
> +
> +	drm_modeset_acquire_init(&ctx, 0);
> +
> +	state = drm_atomic_state_alloc(dev);
> +	state->acquire_ctx = &ctx;
> +
> +retry:
> +	drm_atomic_set_property(state, &connector->base,
> +				dev->mode_config.tv_left_margin_property,
> +				cmdline->overscan_left);
> +
> +	drm_atomic_set_property(state, &connector->base,
> +				dev->mode_config.tv_right_margin_property,
> +				cmdline->overscan_right);
> +
> +	drm_atomic_set_property(state, &connector->base,
> +				dev->mode_config.tv_top_margin_property,
> +				cmdline->overscan_top);
> +
> +	drm_atomic_set_property(state, &connector->base,
> +				dev->mode_config.tv_bottom_margin_property,
> +				cmdline->overscan_bottom);
> +
> +	ret = drm_atomic_commit(state);
> +	if (ret == -EDEADLK) {
> +		drm_atomic_state_clear(state);
> +		drm_modeset_backoff(&ctx);
> +		goto retry;
> +	}
> +
> +	drm_atomic_state_put(state);
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +}
> +

Should we set these property values in drm_connector_init()?
I assume that DRM userspace would want to use these values as well.

Noralf.

>  static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
>  			    u32 width, u32 height)
>  {
> @@ -2680,6 +2725,8 @@ static void drm_setup_crtcs_fb(struct drm_fb_helper *fb_helper)
>  		struct drm_connector *connector =
>  					fb_helper->connector_info[i]->connector;
>  
> +		drm_setup_connector_margins(connector);
> +
>  		/* use first connected connector for the physical dimensions */
>  		if (connector->status == connector_status_connected) {
>  			info->var.width = connector->display_info.width_mm;
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index ac8d70b92b62..493ba3ccde70 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -1586,6 +1586,50 @@ static int drm_mode_parse_cmdline_options(char *str, size_t len,
>  		} else if (!strncmp(option, "reflect_y", delim - option)) {
>  			rotation |= DRM_MODE_REFLECT_Y;
>  			sep = delim;
> +		} else if (!strncmp(option, "overscan_right", delim - option)) {
> +			const char *value = delim + 1;
> +			unsigned int margin;
> +
> +			margin = simple_strtol(value, &sep, 10);
> +
> +			/* Make sure we have parsed something */
> +			if (sep == value)
> +				return -EINVAL;
> +
> +			mode->overscan_right = margin;
> +		} else if (!strncmp(option, "overscan_left", delim - option)) {
> +			const char *value = delim + 1;
> +			unsigned int margin;
> +
> +			margin = simple_strtol(value, &sep, 10);
> +
> +			/* Make sure we have parsed something */
> +			if (sep == value)
> +				return -EINVAL;
> +
> +			mode->overscan_left = margin;
> +		} else if (!strncmp(option, "overscan_top", delim - option)) {
> +			const char *value = delim + 1;
> +			unsigned int margin;
> +
> +			margin = simple_strtol(value, &sep, 10);
> +
> +			/* Make sure we have parsed something */
> +			if (sep == value)
> +				return -EINVAL;
> +
> +			mode->overscan_top = margin;
> +		} else if (!strncmp(option, "overscan_bottom", delim - option)) {
> +			const char *value = delim + 1;
> +			unsigned int margin;
> +
> +			margin = simple_strtol(value, &sep, 10);
> +
> +			/* Make sure we have parsed something */
> +			if (sep == value)
> +				return -EINVAL;
> +
> +			mode->overscan_bottom = margin;
>  		} else {
>  			return -EINVAL;
>  		}
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index dfe7f2304b35..44d9885dd401 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -903,6 +903,7 @@ struct drm_cmdline_mode {
>  	int xres, yres;
>  	int bpp;
>  	int refresh;
> +	int overscan_right, overscan_top, overscan_left, overscan_bottom;
>  	bool rb;
>  	bool interlace;
>  	bool cvt;
>
Maxime Ripard April 17, 2019, 2:07 p.m. UTC | #3
Hi Noralf,

On Tue, Apr 16, 2019 at 04:52:20PM +0200, Noralf Trønnes wrote:
> Den 11.04.2019 15.22, skrev Maxime Ripard:
> > Properly configuring the overscan properties might be needed for the
> > initial setup of the framebuffer for display that still have overscan.
> > Let's allow for more properties on the kernel command line to setup each
> > margin.
> >
> > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> > ---
> >  drivers/gpu/drm/drm_fb_helper.c | 47 ++++++++++++++++++++++++++++++++++-
> >  drivers/gpu/drm/drm_modes.c     | 44 ++++++++++++++++++++++++++++++++-
> >  include/drm/drm_connector.h     |  1 +-
> >  3 files changed, 92 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > index 8781897559b2..4e403fe1f451 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -2567,6 +2567,51 @@ static void drm_setup_crtc_rotation(struct drm_fb_helper *fb_helper,
> >  	fb_helper->sw_rotations |= DRM_MODE_ROTATE_0;
> >  }
> >
> > +static void drm_setup_connector_margins(struct drm_connector *connector)
> > +{
> > +	struct drm_cmdline_mode *cmdline = &connector->cmdline_mode;
> > +	struct drm_modeset_acquire_ctx ctx;
> > +	struct drm_atomic_state *state;
> > +	struct drm_device *dev = connector->dev;
> > +	int ret;
> > +
> > +	if (!drm_drv_uses_atomic_modeset(dev))
> > +		return;
> > +
> > +	drm_modeset_acquire_init(&ctx, 0);
> > +
> > +	state = drm_atomic_state_alloc(dev);
> > +	state->acquire_ctx = &ctx;
> > +
> > +retry:
> > +	drm_atomic_set_property(state, &connector->base,
> > +				dev->mode_config.tv_left_margin_property,
> > +				cmdline->overscan_left);
> > +
> > +	drm_atomic_set_property(state, &connector->base,
> > +				dev->mode_config.tv_right_margin_property,
> > +				cmdline->overscan_right);
> > +
> > +	drm_atomic_set_property(state, &connector->base,
> > +				dev->mode_config.tv_top_margin_property,
> > +				cmdline->overscan_top);
> > +
> > +	drm_atomic_set_property(state, &connector->base,
> > +				dev->mode_config.tv_bottom_margin_property,
> > +				cmdline->overscan_bottom);
> > +
> > +	ret = drm_atomic_commit(state);
> > +	if (ret == -EDEADLK) {
> > +		drm_atomic_state_clear(state);
> > +		drm_modeset_backoff(&ctx);
> > +		goto retry;
> > +	}
> > +
> > +	drm_atomic_state_put(state);
> > +	drm_modeset_drop_locks(&ctx);
> > +	drm_modeset_acquire_fini(&ctx);
> > +}
> > +
>
> Should we set these property values in drm_connector_init()?
> I assume that DRM userspace would want to use these values as well.

I'm not sure, I've looked into it, and most drivers will create those
properties and attached *after* running drm_connector_init.

If you're using drm_mode_create_tv_margin_properties, then the first
thing it does is to check whether that property has been created
already, so we're covered.

For drm_connector_attach_tv_margins_properties though, this will force
overwrite the value.

One thing I'm not really fond of either is that you would do this even
if the driver cannot support the margins at all.

Maybe we can put this into drm_connector_attach_tv_margins_properties?

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Noralf Trønnes April 17, 2019, 3:30 p.m. UTC | #4
Den 17.04.2019 16.07, skrev Maxime Ripard:
> Hi Noralf,
> 
> On Tue, Apr 16, 2019 at 04:52:20PM +0200, Noralf Trønnes wrote:
>> Den 11.04.2019 15.22, skrev Maxime Ripard:
>>> Properly configuring the overscan properties might be needed for the
>>> initial setup of the framebuffer for display that still have overscan.
>>> Let's allow for more properties on the kernel command line to setup each
>>> margin.
>>>
>>> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
>>> ---
>>>  drivers/gpu/drm/drm_fb_helper.c | 47 ++++++++++++++++++++++++++++++++++-
>>>  drivers/gpu/drm/drm_modes.c     | 44 ++++++++++++++++++++++++++++++++-
>>>  include/drm/drm_connector.h     |  1 +-
>>>  3 files changed, 92 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>>> index 8781897559b2..4e403fe1f451 100644
>>> --- a/drivers/gpu/drm/drm_fb_helper.c
>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>>> @@ -2567,6 +2567,51 @@ static void drm_setup_crtc_rotation(struct drm_fb_helper *fb_helper,
>>>  	fb_helper->sw_rotations |= DRM_MODE_ROTATE_0;
>>>  }
>>>
>>> +static void drm_setup_connector_margins(struct drm_connector *connector)
>>> +{
>>> +	struct drm_cmdline_mode *cmdline = &connector->cmdline_mode;
>>> +	struct drm_modeset_acquire_ctx ctx;
>>> +	struct drm_atomic_state *state;
>>> +	struct drm_device *dev = connector->dev;
>>> +	int ret;
>>> +
>>> +	if (!drm_drv_uses_atomic_modeset(dev))
>>> +		return;
>>> +
>>> +	drm_modeset_acquire_init(&ctx, 0);
>>> +
>>> +	state = drm_atomic_state_alloc(dev);
>>> +	state->acquire_ctx = &ctx;
>>> +
>>> +retry:
>>> +	drm_atomic_set_property(state, &connector->base,
>>> +				dev->mode_config.tv_left_margin_property,
>>> +				cmdline->overscan_left);
>>> +
>>> +	drm_atomic_set_property(state, &connector->base,
>>> +				dev->mode_config.tv_right_margin_property,
>>> +				cmdline->overscan_right);
>>> +
>>> +	drm_atomic_set_property(state, &connector->base,
>>> +				dev->mode_config.tv_top_margin_property,
>>> +				cmdline->overscan_top);
>>> +
>>> +	drm_atomic_set_property(state, &connector->base,
>>> +				dev->mode_config.tv_bottom_margin_property,
>>> +				cmdline->overscan_bottom);
>>> +
>>> +	ret = drm_atomic_commit(state);
>>> +	if (ret == -EDEADLK) {
>>> +		drm_atomic_state_clear(state);
>>> +		drm_modeset_backoff(&ctx);
>>> +		goto retry;
>>> +	}
>>> +
>>> +	drm_atomic_state_put(state);
>>> +	drm_modeset_drop_locks(&ctx);
>>> +	drm_modeset_acquire_fini(&ctx);
>>> +}
>>> +
>>
>> Should we set these property values in drm_connector_init()?
>> I assume that DRM userspace would want to use these values as well.
> 
> I'm not sure, I've looked into it, and most drivers will create those
> properties and attached *after* running drm_connector_init.
> 
> If you're using drm_mode_create_tv_margin_properties, then the first
> thing it does is to check whether that property has been created
> already, so we're covered.
> 
> For drm_connector_attach_tv_margins_properties though, this will force
> overwrite the value.
> 
> One thing I'm not really fond of either is that you would do this even
> if the driver cannot support the margins at all.
> 
> Maybe we can put this into drm_connector_attach_tv_margins_properties?
> 

That sounds like a good idea.
It would leave out these which doesn't use that _attach function though:
- i2c/ch7006_drv.c
- i915/intel_tv.c

Not sure how to handle those. i915 uses the connector state to set the
initial margins. Can we set the margins on the connector state in
drm_connector_init() and use those as default values in
drm_connector_attach_tv_margin_properties()?

Noralf.

> Maxime
> 
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>
Noralf Trønnes April 17, 2019, 3:38 p.m. UTC | #5
Den 17.04.2019 17.30, skrev Noralf Trønnes:
> 
> 
> Den 17.04.2019 16.07, skrev Maxime Ripard:
>> Hi Noralf,
>>
>> On Tue, Apr 16, 2019 at 04:52:20PM +0200, Noralf Trønnes wrote:
>>> Den 11.04.2019 15.22, skrev Maxime Ripard:
>>>> Properly configuring the overscan properties might be needed for the
>>>> initial setup of the framebuffer for display that still have overscan.
>>>> Let's allow for more properties on the kernel command line to setup each
>>>> margin.
>>>>
>>>> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
>>>> ---
>>>>  drivers/gpu/drm/drm_fb_helper.c | 47 ++++++++++++++++++++++++++++++++++-
>>>>  drivers/gpu/drm/drm_modes.c     | 44 ++++++++++++++++++++++++++++++++-
>>>>  include/drm/drm_connector.h     |  1 +-
>>>>  3 files changed, 92 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>>>> index 8781897559b2..4e403fe1f451 100644
>>>> --- a/drivers/gpu/drm/drm_fb_helper.c
>>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>>>> @@ -2567,6 +2567,51 @@ static void drm_setup_crtc_rotation(struct drm_fb_helper *fb_helper,
>>>>  	fb_helper->sw_rotations |= DRM_MODE_ROTATE_0;
>>>>  }
>>>>
>>>> +static void drm_setup_connector_margins(struct drm_connector *connector)
>>>> +{
>>>> +	struct drm_cmdline_mode *cmdline = &connector->cmdline_mode;
>>>> +	struct drm_modeset_acquire_ctx ctx;
>>>> +	struct drm_atomic_state *state;
>>>> +	struct drm_device *dev = connector->dev;
>>>> +	int ret;
>>>> +
>>>> +	if (!drm_drv_uses_atomic_modeset(dev))
>>>> +		return;
>>>> +
>>>> +	drm_modeset_acquire_init(&ctx, 0);
>>>> +
>>>> +	state = drm_atomic_state_alloc(dev);
>>>> +	state->acquire_ctx = &ctx;
>>>> +
>>>> +retry:
>>>> +	drm_atomic_set_property(state, &connector->base,
>>>> +				dev->mode_config.tv_left_margin_property,
>>>> +				cmdline->overscan_left);
>>>> +
>>>> +	drm_atomic_set_property(state, &connector->base,
>>>> +				dev->mode_config.tv_right_margin_property,
>>>> +				cmdline->overscan_right);
>>>> +
>>>> +	drm_atomic_set_property(state, &connector->base,
>>>> +				dev->mode_config.tv_top_margin_property,
>>>> +				cmdline->overscan_top);
>>>> +
>>>> +	drm_atomic_set_property(state, &connector->base,
>>>> +				dev->mode_config.tv_bottom_margin_property,
>>>> +				cmdline->overscan_bottom);
>>>> +
>>>> +	ret = drm_atomic_commit(state);
>>>> +	if (ret == -EDEADLK) {
>>>> +		drm_atomic_state_clear(state);
>>>> +		drm_modeset_backoff(&ctx);
>>>> +		goto retry;
>>>> +	}
>>>> +
>>>> +	drm_atomic_state_put(state);
>>>> +	drm_modeset_drop_locks(&ctx);
>>>> +	drm_modeset_acquire_fini(&ctx);
>>>> +}
>>>> +
>>>
>>> Should we set these property values in drm_connector_init()?
>>> I assume that DRM userspace would want to use these values as well.
>>
>> I'm not sure, I've looked into it, and most drivers will create those
>> properties and attached *after* running drm_connector_init.
>>
>> If you're using drm_mode_create_tv_margin_properties, then the first
>> thing it does is to check whether that property has been created
>> already, so we're covered.
>>
>> For drm_connector_attach_tv_margins_properties though, this will force
>> overwrite the value.
>>
>> One thing I'm not really fond of either is that you would do this even
>> if the driver cannot support the margins at all.
>>
>> Maybe we can put this into drm_connector_attach_tv_margins_properties?
>>
> 
> That sounds like a good idea.
> It would leave out these which doesn't use that _attach function though:
> - i2c/ch7006_drv.c
> - i915/intel_tv.c
> 
> Not sure how to handle those. i915 uses the connector state to set the
> initial margins. Can we set the margins on the connector state in
> drm_connector_init() and use those as default values in
> drm_connector_attach_tv_margin_properties()?

Nevermind, that won't work as there's no state yet at that point if I'm
not mistaken.

> 
> Noralf.
> 
>> Maxime
>>
>> --
>> Maxime Ripard, Bootlin
>> Embedded Linux and Kernel engineering
>> https://bootlin.com
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 8781897559b2..4e403fe1f451 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -2567,6 +2567,51 @@  static void drm_setup_crtc_rotation(struct drm_fb_helper *fb_helper,
 	fb_helper->sw_rotations |= DRM_MODE_ROTATE_0;
 }
 
+static void drm_setup_connector_margins(struct drm_connector *connector)
+{
+	struct drm_cmdline_mode *cmdline = &connector->cmdline_mode;
+	struct drm_modeset_acquire_ctx ctx;
+	struct drm_atomic_state *state;
+	struct drm_device *dev = connector->dev;
+	int ret;
+
+	if (!drm_drv_uses_atomic_modeset(dev))
+		return;
+
+	drm_modeset_acquire_init(&ctx, 0);
+
+	state = drm_atomic_state_alloc(dev);
+	state->acquire_ctx = &ctx;
+
+retry:
+	drm_atomic_set_property(state, &connector->base,
+				dev->mode_config.tv_left_margin_property,
+				cmdline->overscan_left);
+
+	drm_atomic_set_property(state, &connector->base,
+				dev->mode_config.tv_right_margin_property,
+				cmdline->overscan_right);
+
+	drm_atomic_set_property(state, &connector->base,
+				dev->mode_config.tv_top_margin_property,
+				cmdline->overscan_top);
+
+	drm_atomic_set_property(state, &connector->base,
+				dev->mode_config.tv_bottom_margin_property,
+				cmdline->overscan_bottom);
+
+	ret = drm_atomic_commit(state);
+	if (ret == -EDEADLK) {
+		drm_atomic_state_clear(state);
+		drm_modeset_backoff(&ctx);
+		goto retry;
+	}
+
+	drm_atomic_state_put(state);
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+}
+
 static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
 			    u32 width, u32 height)
 {
@@ -2680,6 +2725,8 @@  static void drm_setup_crtcs_fb(struct drm_fb_helper *fb_helper)
 		struct drm_connector *connector =
 					fb_helper->connector_info[i]->connector;
 
+		drm_setup_connector_margins(connector);
+
 		/* use first connected connector for the physical dimensions */
 		if (connector->status == connector_status_connected) {
 			info->var.width = connector->display_info.width_mm;
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index ac8d70b92b62..493ba3ccde70 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1586,6 +1586,50 @@  static int drm_mode_parse_cmdline_options(char *str, size_t len,
 		} else if (!strncmp(option, "reflect_y", delim - option)) {
 			rotation |= DRM_MODE_REFLECT_Y;
 			sep = delim;
+		} else if (!strncmp(option, "overscan_right", delim - option)) {
+			const char *value = delim + 1;
+			unsigned int margin;
+
+			margin = simple_strtol(value, &sep, 10);
+
+			/* Make sure we have parsed something */
+			if (sep == value)
+				return -EINVAL;
+
+			mode->overscan_right = margin;
+		} else if (!strncmp(option, "overscan_left", delim - option)) {
+			const char *value = delim + 1;
+			unsigned int margin;
+
+			margin = simple_strtol(value, &sep, 10);
+
+			/* Make sure we have parsed something */
+			if (sep == value)
+				return -EINVAL;
+
+			mode->overscan_left = margin;
+		} else if (!strncmp(option, "overscan_top", delim - option)) {
+			const char *value = delim + 1;
+			unsigned int margin;
+
+			margin = simple_strtol(value, &sep, 10);
+
+			/* Make sure we have parsed something */
+			if (sep == value)
+				return -EINVAL;
+
+			mode->overscan_top = margin;
+		} else if (!strncmp(option, "overscan_bottom", delim - option)) {
+			const char *value = delim + 1;
+			unsigned int margin;
+
+			margin = simple_strtol(value, &sep, 10);
+
+			/* Make sure we have parsed something */
+			if (sep == value)
+				return -EINVAL;
+
+			mode->overscan_bottom = margin;
 		} else {
 			return -EINVAL;
 		}
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index dfe7f2304b35..44d9885dd401 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -903,6 +903,7 @@  struct drm_cmdline_mode {
 	int xres, yres;
 	int bpp;
 	int refresh;
+	int overscan_right, overscan_top, overscan_left, overscan_bottom;
 	bool rb;
 	bool interlace;
 	bool cvt;