Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FKMS "Broadcast RGB" property, and KMS command line parsing update #3103

Merged
merged 12 commits into from
Aug 5, 2019
Merged

FKMS "Broadcast RGB" property, and KMS command line parsing update #3103

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
b9a4d08
drm/vc4: Add "Broadcast RGB" connector property
6by9 Jun 14, 2019
c7988c6
drm/connector: Add documentation for drm_cmdline_mode
mripard Jun 19, 2019
1411df1
drm/modes: Rewrite the command line parser
mripard Jun 19, 2019
52806bb
drm/modes: Support modes names on the command line
mripard Jun 19, 2019
c71d2f7
drm/modes: Allow to specify rotation and reflection on the commandline
mripard Jun 19, 2019
39c3383
drm/connector: Introduce a TV margins structure
mripard Jun 19, 2019
b71baa2
drm/modes: Parse overscan properties
mripard Jun 19, 2019
a226748
drm/atomic: Add a function to reset connector TV properties
mripard Jun 19, 2019
00aed4b
drm/vc4: hdmi: Set default state margin at reset
mripard Jun 19, 2019
a95567c
drm/vc4: fkms: Set default state margin at reset
6by9 Jul 23, 2019
c9a3a9e
drm/modes: Don't apply cmdline's rotation if it wasn't specified
6by9 Jul 23, 2019
f9b2cbb
configs: Add CONFIG_FRAMEBUFFER_CONSOLE_ROTATION to Pi configs
6by9 Jul 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
190 changes: 177 additions & 13 deletions drivers/gpu/drm/vc4/vc4_firmware_kms.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,13 @@ to_vc4_fkms_encoder(struct drm_encoder *encoder)
return container_of(encoder, struct vc4_fkms_encoder, base);
}

/* "Broadcast RGB" property.
* Allows overriding of HDMI full or limited range RGB
*/
#define VC4_BROADCAST_RGB_AUTO 0
#define VC4_BROADCAST_RGB_FULL 1
#define VC4_BROADCAST_RGB_LIMITED 2

/* VC4 FKMS connector KMS struct */
struct vc4_fkms_connector {
struct drm_connector base;
Expand All @@ -297,6 +304,8 @@ struct vc4_fkms_connector {
struct vc4_dev *vc4_dev;
u32 display_number;
u32 display_type;

struct drm_property *broadcast_rgb_property;
};

static inline struct vc4_fkms_connector *
Expand All @@ -305,6 +314,16 @@ to_vc4_fkms_connector(struct drm_connector *connector)
return container_of(connector, struct vc4_fkms_connector, base);
}

/* VC4 FKMS connector state */
struct vc4_fkms_connector_state {
struct drm_connector_state base;

int broadcast_rgb;
};

#define to_vc4_fkms_connector_state(x) \
container_of(x, struct vc4_fkms_connector_state, base)

static u32 vc4_get_display_type(u32 display_number)
{
const u32 display_types[] = {
Expand Down Expand Up @@ -832,8 +851,6 @@ static void vc4_crtc_mode_set_nofb(struct drm_crtc *crtc)
mode->picture_aspect_ratio, mode->flags);
mb.timings.display = vc4_crtc->display_number;

mb.timings.video_id_code = frame.avi.video_code;

mb.timings.clock = mode->clock;
mb.timings.hdisplay = mode->hdisplay;
mb.timings.hsync_start = mode->hsync_start;
Expand Down Expand Up @@ -871,11 +888,30 @@ static void vc4_crtc_mode_set_nofb(struct drm_crtc *crtc)
break;
}

if (!vc4_encoder->hdmi_monitor)
if (!vc4_encoder->hdmi_monitor) {
mb.timings.flags |= TIMINGS_FLAGS_DVI;
else if (drm_default_rgb_quant_range(mode) ==
mb.timings.video_id_code = frame.avi.video_code;
} else {
struct vc4_fkms_connector_state *conn_state =
to_vc4_fkms_connector_state(vc4_crtc->connector->state);

/* Do not provide a VIC as the HDMI spec requires that we do not
* signal the opposite of the defined range in the AVI
* infoframe.
*/
mb.timings.video_id_code = 0;

if (conn_state->broadcast_rgb == VC4_BROADCAST_RGB_AUTO) {
/* See CEA-861-E - 5.1 Default Encoding Parameters */
if (drm_default_rgb_quant_range(mode) ==
HDMI_QUANTIZATION_RANGE_LIMITED)
mb.timings.flags |= TIMINGS_FLAGS_RGB_LIMITED;
mb.timings.flags |= TIMINGS_FLAGS_RGB_LIMITED;
} else {
if (conn_state->broadcast_rgb ==
VC4_BROADCAST_RGB_LIMITED)
mb.timings.flags |= TIMINGS_FLAGS_RGB_LIMITED;
}
}

/*
FIXME: To implement
Expand Down Expand Up @@ -1337,13 +1373,95 @@ static void vc4_fkms_connector_destroy(struct drm_connector *connector)
drm_connector_cleanup(connector);
}

/**
* vc4_connector_duplicate_state - duplicate connector state
* @connector: digital connector
*
* Allocates and returns a copy of the connector state (both common and
* digital connector specific) for the specified connector.
*
* Returns: The newly allocated connector state, or NULL on failure.
*/
struct drm_connector_state *
vc4_connector_duplicate_state(struct drm_connector *connector)
{
struct vc4_fkms_connector_state *state;

state = kmemdup(connector->state, sizeof(*state), GFP_KERNEL);
if (!state)
return NULL;

__drm_atomic_helper_connector_duplicate_state(connector, &state->base);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either I've missed something or the state gets copied twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kmemdup allocates and copies sizeof(struct vc4_fkms_connector_state).
__drm_atomic_helper_connector_duplicate_state copies sizeof(drm_connector_state), which is smaller.

This was reproducing the same functionality as in the Intel i915 driver, and they also copy the base state twice.
https://elixir.bootlin.com/linux/v5.2.6/source/drivers/gpu/drm/i915/intel_atomic.c#L150

To clean it up we'll end up with a funky offsetof(broadcast_rgb) for the generic solution. At the moment it'll only be copying one int. I might just assign the int across with a comment should extra members get added.

return &state->base;
}

/**
* vc4_connector_atomic_get_property - hook for connector->atomic_get_property.
* @connector: Connector to get the property for.
* @state: Connector state to retrieve the property from.
* @property: Property to retrieve.
* @val: Return value for the property.
*
* Returns the atomic property value for a digital connector.
*/
int vc4_connector_atomic_get_property(struct drm_connector *connector,
const struct drm_connector_state *state,
struct drm_property *property,
uint64_t *val)
{
struct vc4_fkms_connector *fkms_connector =
to_vc4_fkms_connector(connector);
struct vc4_fkms_connector_state *vc4_conn_state =
to_vc4_fkms_connector_state(state);

if (property == fkms_connector->broadcast_rgb_property) {
*val = vc4_conn_state->broadcast_rgb;
} else {
DRM_DEBUG_ATOMIC("Unknown property [PROP:%d:%s]\n",
property->base.id, property->name);
return -EINVAL;
}

return 0;
}

/**
* vc4_connector_atomic_set_property - hook for connector->atomic_set_property.
* @connector: Connector to set the property for.
* @state: Connector state to set the property on.
* @property: Property to set.
* @val: New value for the property.
*
* Sets the atomic property value for a digital connector.
*/
int vc4_connector_atomic_set_property(struct drm_connector *connector,
struct drm_connector_state *state,
struct drm_property *property,
uint64_t val)
{
struct vc4_fkms_connector *fkms_connector =
to_vc4_fkms_connector(connector);
struct vc4_fkms_connector_state *vc4_conn_state =
to_vc4_fkms_connector_state(state);

if (property == fkms_connector->broadcast_rgb_property) {
vc4_conn_state->broadcast_rgb = val;
return 0;
}

DRM_DEBUG_ATOMIC("Unknown property [PROP:%d:%s]\n",
property->base.id, property->name);
return -EINVAL;
}

static const struct drm_connector_funcs vc4_fkms_connector_funcs = {
.detect = vc4_fkms_connector_detect,
.fill_modes = drm_helper_probe_single_connector_modes,
.destroy = vc4_fkms_connector_destroy,
.reset = drm_atomic_helper_connector_reset,
.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
.atomic_duplicate_state = vc4_connector_duplicate_state,
.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
.atomic_get_property = vc4_connector_atomic_get_property,
.atomic_set_property = vc4_connector_atomic_set_property,
};

static const struct drm_connector_helper_funcs vc4_fkms_connector_helper_funcs = {
Expand All @@ -1356,12 +1474,40 @@ static const struct drm_connector_helper_funcs vc4_fkms_lcd_conn_helper_funcs =
.best_encoder = vc4_fkms_connector_best_encoder,
};

static const struct drm_prop_enum_list broadcast_rgb_names[] = {
{ VC4_BROADCAST_RGB_AUTO, "Automatic" },
{ VC4_BROADCAST_RGB_FULL, "Full" },
{ VC4_BROADCAST_RGB_LIMITED, "Limited 16:235" },
};

static void
vc4_attach_broadcast_rgb_property(struct vc4_fkms_connector *fkms_connector)
{
struct drm_device *dev = fkms_connector->base.dev;
struct drm_property *prop;

prop = fkms_connector->broadcast_rgb_property;
if (!prop) {
prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM,
"Broadcast RGB",
broadcast_rgb_names,
ARRAY_SIZE(broadcast_rgb_names));
if (!prop)
return;

fkms_connector->broadcast_rgb_property = prop;
}

drm_object_attach_property(&fkms_connector->base.base, prop, 0);
}

static struct drm_connector *
vc4_fkms_connector_init(struct drm_device *dev, struct drm_encoder *encoder,
u32 display_num)
{
struct drm_connector *connector = NULL;
struct vc4_fkms_connector *fkms_connector;
struct vc4_fkms_connector_state *conn_state = NULL;
struct vc4_dev *vc4_dev = to_vc4_dev(dev);
int ret = 0;

Expand All @@ -1370,16 +1516,28 @@ vc4_fkms_connector_init(struct drm_device *dev, struct drm_encoder *encoder,
fkms_connector = devm_kzalloc(dev->dev, sizeof(*fkms_connector),
GFP_KERNEL);
if (!fkms_connector) {
ret = -ENOMEM;
goto fail;
return ERR_PTR(-ENOMEM);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary braces, although checkpatch seems unconcerned.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checkpatch not being fussy for once. I'm glad I was sitting down!


/*
* Allocate enough memory to hold vc4_fkms_connector_state,
*/
conn_state = kzalloc(sizeof(*conn_state), GFP_KERNEL);
if (!conn_state) {
kfree(fkms_connector);
return ERR_PTR(-ENOMEM);
}

connector = &fkms_connector->base;

fkms_connector->encoder = encoder;
fkms_connector->display_number = display_num;
fkms_connector->display_type = vc4_get_display_type(display_num);
fkms_connector->vc4_dev = vc4_dev;

__drm_atomic_helper_connector_reset(connector,
&conn_state->base);

if (fkms_connector->display_type == DRM_MODE_ENCODER_DSI) {
drm_connector_init(dev, connector, &vc4_fkms_connector_funcs,
DRM_MODE_CONNECTOR_DSI);
Expand All @@ -1400,10 +1558,14 @@ vc4_fkms_connector_init(struct drm_device *dev, struct drm_encoder *encoder,
connector->interlace_allowed = 0;
}

/* Create and attach TV margin props to this connector. */
ret = drm_mode_create_tv_margin_properties(dev);
if (ret)
return ERR_PTR(ret);
/* Create and attach TV margin props to this connector.
* Already done for SDTV outputs.
*/
if (fkms_connector->display_type != DRM_MODE_ENCODER_TVDAC) {
ret = drm_mode_create_tv_margin_properties(dev);
if (ret)
goto fail;
}

drm_connector_attach_tv_margin_properties(connector);

Expand All @@ -1412,6 +1574,8 @@ vc4_fkms_connector_init(struct drm_device *dev, struct drm_encoder *encoder,

connector->doublescan_allowed = 0;

vc4_attach_broadcast_rgb_property(fkms_connector);

drm_connector_attach_encoder(connector, encoder);

return connector;
Expand Down