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

Noralf Trønnes
2019-04-17T15:31:03Z

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