From: Ville Syrjälä ville.syrjala@linux.intel.com
Using a flexible array for the blob data was a mistake by me. It forces all users of the blob data to cast blob->data to something else. void* is clearly superior so let's go back to the original scheme.
Not a clean revert as the code has moved.
This reverts commit d63f5e6bf6f2a1573ea39c9937cdf5ab0b3a4b77.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_property.c | 1 + include/drm/drm_property.h | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c index bae50e6b819d..0f6620fea3de 100644 --- a/drivers/gpu/drm/drm_property.c +++ b/drivers/gpu/drm/drm_property.c @@ -550,6 +550,7 @@ drm_property_create_blob(struct drm_device *dev, size_t length, /* This must be explicitly initialised, so we can safely call list_del * on it in the removal handler, even if it isn't in a file list. */ INIT_LIST_HEAD(&blob->head_file); + blob->data = (void *)blob + sizeof(*blob); blob->length = length; blob->dev = dev;
diff --git a/include/drm/drm_property.h b/include/drm/drm_property.h index 8a522b4bed40..265fd1f2e112 100644 --- a/include/drm/drm_property.h +++ b/include/drm/drm_property.h @@ -209,7 +209,7 @@ struct drm_property_blob { struct list_head head_global; struct list_head head_file; size_t length; - unsigned char data[]; + void *data; };
struct drm_prop_enum_list {
From: Ville Syrjälä ville.syrjala@linux.intel.com
Now that blob->data is void* again we don't need the casts anymore.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_atomic.c | 3 +-- drivers/gpu/drm/drm_atomic_helper.c | 2 +- drivers/gpu/drm/drm_edid.c | 3 +-- drivers/gpu/drm/drm_fb_helper.c | 2 +- drivers/gpu/drm/drm_plane.c | 2 +- 5 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 46733d534587..8945357212ba 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -391,8 +391,7 @@ int drm_atomic_set_mode_prop_for_crtc(struct drm_crtc_state *state, if (blob) { if (blob->length != sizeof(struct drm_mode_modeinfo) || drm_mode_convert_umode(state->crtc->dev, &state->mode, - (const struct drm_mode_modeinfo *) - blob->data)) + blob->data)) return -EINVAL;
state->mode_blob = drm_property_blob_get(blob); diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index ae3cbfe9e01c..6211a1b20405 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3816,7 +3816,7 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc, }
/* Prepare GAMMA_LUT with the legacy values. */ - blob_data = (struct drm_color_lut *) blob->data; + blob_data = blob->data; for (i = 0; i < size; i++) { blob_data[i].red = red[i]; blob_data[i].green = green[i]; diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 788fee4b4bf9..134069f36482 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -1575,8 +1575,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector, struct edid *override = NULL;
if (connector->override_edid) - override = drm_edid_duplicate((const struct edid *) - connector->edid_blob_ptr->data); + override = drm_edid_duplicate(connector->edid_blob_ptr->data);
if (!override) override = drm_load_edid_firmware(connector); diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 035784ddd133..0646b108030b 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1351,7 +1351,7 @@ static struct drm_property_blob *setcmap_new_gamma_lut(struct drm_crtc *crtc, if (IS_ERR(gamma_lut)) return gamma_lut;
- lut = (struct drm_color_lut *)gamma_lut->data; + lut = gamma_lut->data; if (cmap->start || cmap->len != size) { u16 *r = crtc->gamma_store; u16 *g = r + crtc->gamma_size; diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 09de6ecb3968..9851616cf0f3 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -104,7 +104,7 @@ static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane if (IS_ERR(blob)) return -1;
- blob_data = (struct drm_format_modifier_blob *)blob->data; + blob_data = blob->data; blob_data->version = FORMAT_BLOB_CURRENT; blob_data->count_formats = plane->format_count; blob_data->formats_offset = sizeof(struct drm_format_modifier_blob);
Regards
Shashank
On 2/24/2018 12:55 AM, Ville Syrjala wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Now that blob->data is void* again we don't need the casts anymore.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/drm_atomic.c | 3 +-- drivers/gpu/drm/drm_atomic_helper.c | 2 +- drivers/gpu/drm/drm_edid.c | 3 +-- drivers/gpu/drm/drm_fb_helper.c | 2 +- drivers/gpu/drm/drm_plane.c | 2 +- 5 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 46733d534587..8945357212ba 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -391,8 +391,7 @@ int drm_atomic_set_mode_prop_for_crtc(struct drm_crtc_state *state, if (blob) { if (blob->length != sizeof(struct drm_mode_modeinfo) || drm_mode_convert_umode(state->crtc->dev, &state->mode,
(const struct drm_mode_modeinfo *)blob->data))
blob->data)) return -EINVAL;state->mode_blob = drm_property_blob_get(blob);
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index ae3cbfe9e01c..6211a1b20405 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3816,7 +3816,7 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc, }
/* Prepare GAMMA_LUT with the legacy values. */
- blob_data = (struct drm_color_lut *) blob->data;
- blob_data = blob->data; for (i = 0; i < size; i++) { blob_data[i].red = red[i]; blob_data[i].green = green[i];
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 788fee4b4bf9..134069f36482 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -1575,8 +1575,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector, struct edid *override = NULL;
if (connector->override_edid)
override = drm_edid_duplicate((const struct edid *)connector->edid_blob_ptr->data);
override = drm_edid_duplicate(connector->edid_blob_ptr->data);if (!override) override = drm_load_edid_firmware(connector);
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 035784ddd133..0646b108030b 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1351,7 +1351,7 @@ static struct drm_property_blob *setcmap_new_gamma_lut(struct drm_crtc *crtc, if (IS_ERR(gamma_lut)) return gamma_lut;
- lut = (struct drm_color_lut *)gamma_lut->data;
- lut = gamma_lut->data; if (cmap->start || cmap->len != size) { u16 *r = crtc->gamma_store; u16 *g = r + crtc->gamma_size;
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 09de6ecb3968..9851616cf0f3 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -104,7 +104,7 @@ static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane if (IS_ERR(blob)) return -1;
- blob_data = (struct drm_format_modifier_blob *)blob->data;
- blob_data = blob->data;
Reviewed-by: Shashank Sharma shashank.sharma@intel.com
blob_data->version = FORMAT_BLOB_CURRENT; blob_data->count_formats = plane->format_count; blob_data->formats_offset = sizeof(struct drm_format_modifier_blob);
From: Ville Syrjälä ville.syrjala@linux.intel.com
While we want to potentially support multiple different gamma/degamma LUT sizes we can (and should) at least check that the blob length is a multiple of the LUT entry size.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_atomic.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 8945357212ba..933edec0299d 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -413,6 +413,7 @@ drm_atomic_replace_property_blob_from_id(struct drm_device *dev, struct drm_property_blob **blob, uint64_t blob_id, ssize_t expected_size, + ssize_t expected_size_mod, bool *replaced) { struct drm_property_blob *new_blob = NULL; @@ -422,7 +423,13 @@ drm_atomic_replace_property_blob_from_id(struct drm_device *dev, if (new_blob == NULL) return -EINVAL;
- if (expected_size > 0 && expected_size != new_blob->length) { + if (expected_size > 0 && + new_blob->length != expected_size) { + drm_property_blob_put(new_blob); + return -EINVAL; + } + if (expected_size_mod > 0 && + new_blob->length % expected_size_mod != 0) { drm_property_blob_put(new_blob); return -EINVAL; } @@ -470,7 +477,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, ret = drm_atomic_replace_property_blob_from_id(dev, &state->degamma_lut, val, - -1, + -1, sizeof(struct drm_color_lut), &replaced); state->color_mgmt_changed |= replaced; return ret; @@ -478,7 +485,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, ret = drm_atomic_replace_property_blob_from_id(dev, &state->ctm, val, - sizeof(struct drm_color_ctm), + sizeof(struct drm_color_ctm), -1, &replaced); state->color_mgmt_changed |= replaced; return ret; @@ -486,7 +493,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, ret = drm_atomic_replace_property_blob_from_id(dev, &state->gamma_lut, val, - -1, + -1, sizeof(struct drm_color_lut), &replaced); state->color_mgmt_changed |= replaced; return ret;
Regards
Shashank
On 2/24/2018 12:55 AM, Ville Syrjala wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
While we want to potentially support multiple different gamma/degamma LUT sizes we can (and should) at least check that the blob length is a multiple of the LUT entry size.
I dint understand the exact idea behind doing this, how is this going to benefit ? May be a bit more description ?
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/drm_atomic.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 8945357212ba..933edec0299d 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -413,6 +413,7 @@ drm_atomic_replace_property_blob_from_id(struct drm_device *dev, struct drm_property_blob **blob, uint64_t blob_id, ssize_t expected_size,
ssize_t expected_size_mod, bool *replaced)@@ -422,7 +423,13 @@ drm_atomic_replace_property_blob_from_id(struct drm_device *dev, if (new_blob == NULL) return -EINVAL;
if (expected_size > 0 && expected_size != new_blob->length) {
if (expected_size > 0 &&new_blob->length != expected_size) {drm_property_blob_put(new_blob);return -EINVAL;}
One line needed here, matching the previous if() pattern
if (expected_size_mod > 0 &&new_blob->length % expected_size_mod != 0) { drm_property_blob_put(new_blob); return -EINVAL;@@ -470,7 +477,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, ret = drm_atomic_replace_property_blob_from_id(dev, &state->degamma_lut, val,
-1,
-1, sizeof(struct drm_color_lut), &replaced);@@ -478,7 +485,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, ret = drm_atomic_replace_property_blob_from_id(dev, &state->ctm, val,
sizeof(struct drm_color_ctm),
sizeof(struct drm_color_ctm), -1, &replaced);@@ -486,7 +493,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, ret = drm_atomic_replace_property_blob_from_id(dev, &state->gamma_lut, val,
-1,
-1, sizeof(struct drm_color_lut), &replaced);
On Thu, Mar 01, 2018 at 06:43:21PM +0530, Sharma, Shashank wrote:
Regards
Shashank
On 2/24/2018 12:55 AM, Ville Syrjala wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
While we want to potentially support multiple different gamma/degamma LUT sizes we can (and should) at least check that the blob length is a multiple of the LUT entry size.
I dint understand the exact idea behind doing this, how is this going to benefit ? May be a bit more description ?
The benefit is rejecting garbage fed in from userspace.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/drm_atomic.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 8945357212ba..933edec0299d 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -413,6 +413,7 @@ drm_atomic_replace_property_blob_from_id(struct drm_device *dev, struct drm_property_blob **blob, uint64_t blob_id, ssize_t expected_size,
ssize_t expected_size_mod, bool *replaced)@@ -422,7 +423,13 @@ drm_atomic_replace_property_blob_from_id(struct drm_device *dev, if (new_blob == NULL) return -EINVAL;
if (expected_size > 0 && expected_size != new_blob->length) {
if (expected_size > 0 &&new_blob->length != expected_size) {drm_property_blob_put(new_blob);return -EINVAL;}One line needed here, matching the previous if() pattern
What line? Don't understand.
if (expected_size_mod > 0 &&new_blob->length % expected_size_mod != 0) { drm_property_blob_put(new_blob); return -EINVAL;@@ -470,7 +477,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, ret = drm_atomic_replace_property_blob_from_id(dev, &state->degamma_lut, val,
-1,
-1, sizeof(struct drm_color_lut), &replaced);@@ -478,7 +485,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, ret = drm_atomic_replace_property_blob_from_id(dev, &state->ctm, val,
sizeof(struct drm_color_ctm),
sizeof(struct drm_color_ctm), -1, &replaced);@@ -486,7 +493,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, ret = drm_atomic_replace_property_blob_from_id(dev, &state->gamma_lut, val,
-1,
-1, sizeof(struct drm_color_lut), &replaced);
Regards
Shashank
On 3/1/2018 6:54 PM, Ville Syrjälä wrote:
On Thu, Mar 01, 2018 at 06:43:21PM +0530, Sharma, Shashank wrote:
Regards
Shashank
On 2/24/2018 12:55 AM, Ville Syrjala wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
While we want to potentially support multiple different gamma/degamma LUT sizes we can (and should) at least check that the blob length is a multiple of the LUT entry size.
I dint understand the exact idea behind doing this, how is this going to benefit ? May be a bit more description ?
The benefit is rejecting garbage fed in from userspace.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/drm_atomic.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 8945357212ba..933edec0299d 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -413,6 +413,7 @@ drm_atomic_replace_property_blob_from_id(struct drm_device *dev, struct drm_property_blob **blob, uint64_t blob_id, ssize_t expected_size,
ssize_t expected_size_mod, bool *replaced)@@ -422,7 +423,13 @@ drm_atomic_replace_property_blob_from_id(struct drm_device *dev, if (new_blob == NULL) return -EINVAL;
if (expected_size > 0 && expected_size != new_blob->length) {
if (expected_size > 0 &&new_blob->length != expected_size) {drm_property_blob_put(new_blob);return -EINVAL;}One line needed here, matching the previous if() pattern
What line? Don't understand.
I mean, I can see a blank line before previous if() condition, so lets keep the same pattern for this if() too
- Shashank
if (expected_size_mod > 0 &&new_blob->length % expected_size_mod != 0) { drm_property_blob_put(new_blob); return -EINVAL; }@@ -470,7 +477,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, ret = drm_atomic_replace_property_blob_from_id(dev, &state->degamma_lut, val,
-1,
-1, sizeof(struct drm_color_lut), &replaced); state->color_mgmt_changed |= replaced; return ret;@@ -478,7 +485,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, ret = drm_atomic_replace_property_blob_from_id(dev, &state->ctm, val,
sizeof(struct drm_color_ctm),
sizeof(struct drm_color_ctm), -1, &replaced); state->color_mgmt_changed |= replaced; return ret;@@ -486,7 +493,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, ret = drm_atomic_replace_property_blob_from_id(dev, &state->gamma_lut, val,
-1,
-1, sizeof(struct drm_color_lut), &replaced); state->color_mgmt_changed |= replaced; return ret;
On Thu, Mar 01, 2018 at 07:58:07PM +0530, Sharma, Shashank wrote:
Regards
Shashank
On 3/1/2018 6:54 PM, Ville Syrjälä wrote:
On Thu, Mar 01, 2018 at 06:43:21PM +0530, Sharma, Shashank wrote:
Regards
Shashank
On 2/24/2018 12:55 AM, Ville Syrjala wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
While we want to potentially support multiple different gamma/degamma LUT sizes we can (and should) at least check that the blob length is a multiple of the LUT entry size.
I dint understand the exact idea behind doing this, how is this going to benefit ? May be a bit more description ?
The benefit is rejecting garbage fed in from userspace.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/drm_atomic.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 8945357212ba..933edec0299d 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -413,6 +413,7 @@ drm_atomic_replace_property_blob_from_id(struct drm_device *dev, struct drm_property_blob **blob, uint64_t blob_id, ssize_t expected_size,
ssize_t expected_size_mod, bool *replaced)@@ -422,7 +423,13 @@ drm_atomic_replace_property_blob_from_id(struct drm_device *dev, if (new_blob == NULL) return -EINVAL;
if (expected_size > 0 && expected_size != new_blob->length) {
if (expected_size > 0 &&new_blob->length != expected_size) {drm_property_blob_put(new_blob);return -EINVAL;}One line needed here, matching the previous if() pattern
What line? Don't understand.
I mean, I can see a blank line before previous if() condition, so lets keep the same pattern for this if() too
OTOH the two ifs are related so maybe just keep them together? Doesn't actually matter to me though.
- Shashank
if (expected_size_mod > 0 &&new_blob->length % expected_size_mod != 0) { drm_property_blob_put(new_blob); return -EINVAL; }@@ -470,7 +477,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, ret = drm_atomic_replace_property_blob_from_id(dev, &state->degamma_lut, val,
-1,
-1, sizeof(struct drm_color_lut), &replaced); state->color_mgmt_changed |= replaced; return ret;@@ -478,7 +485,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, ret = drm_atomic_replace_property_blob_from_id(dev, &state->ctm, val,
sizeof(struct drm_color_ctm),
sizeof(struct drm_color_ctm), -1, &replaced); state->color_mgmt_changed |= replaced; return ret;@@ -486,7 +493,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, ret = drm_atomic_replace_property_blob_from_id(dev, &state->gamma_lut, val,
-1,
-1, sizeof(struct drm_color_lut), &replaced); state->color_mgmt_changed |= replaced; return ret;
On Fri, Feb 23, 2018 at 09:25:03PM +0200, Ville Syrjala wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
While we want to potentially support multiple different gamma/degamma LUT sizes we can (and should) at least check that the blob length is a multiple of the LUT entry size.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/drm_atomic.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 8945357212ba..933edec0299d 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -413,6 +413,7 @@ drm_atomic_replace_property_blob_from_id(struct drm_device *dev, struct drm_property_blob **blob, uint64_t blob_id, ssize_t expected_size,
ssize_t expected_size_mod,
Needs kerneldoc, and I'm not sure it's the most descriptive name. Maybe expected_array_element_size?
With or without the bikeshed, but with the kerneldoc fixed:
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
Up to this patch in the series. -Daniel
bool *replaced){ struct drm_property_blob *new_blob = NULL; @@ -422,7 +423,13 @@ drm_atomic_replace_property_blob_from_id(struct drm_device *dev, if (new_blob == NULL) return -EINVAL;
if (expected_size > 0 && expected_size != new_blob->length) {
if (expected_size > 0 &&new_blob->length != expected_size) {drm_property_blob_put(new_blob);return -EINVAL;}if (expected_size_mod > 0 &&new_blob->length % expected_size_mod != 0) { drm_property_blob_put(new_blob); return -EINVAL;@@ -470,7 +477,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, ret = drm_atomic_replace_property_blob_from_id(dev, &state->degamma_lut, val,
-1,
-1, sizeof(struct drm_color_lut), &replaced);@@ -478,7 +485,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, ret = drm_atomic_replace_property_blob_from_id(dev, &state->ctm, val,
sizeof(struct drm_color_ctm),
sizeof(struct drm_color_ctm), -1, &replaced);@@ -486,7 +493,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, ret = drm_atomic_replace_property_blob_from_id(dev, &state->gamma_lut, val,
-1,
-1, sizeof(struct drm_color_lut), &replaced);-- 2.13.6
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
From: Ville Syrjälä ville.syrjala@linux.intel.com
While we want to potentially support multiple different gamma/degamma LUT sizes we can (and should) at least check that the blob length is a multiple of the LUT entry size.
v2: s/expected_size_mod/expected_elem_size/ (Daniel) Add kernel doc (Daniel)
Cc: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch --- Documentation/gpu/drm-kms.rst | 3 +++ drivers/gpu/drm/drm_atomic.c | 39 +++++++++++++++++++++++++++++++++++---- 2 files changed, 38 insertions(+), 4 deletions(-)
diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst index 56a3780e39b8..1dffd1ac4cd4 100644 --- a/Documentation/gpu/drm-kms.rst +++ b/Documentation/gpu/drm-kms.rst @@ -286,6 +286,9 @@ Atomic Mode Setting Function Reference .. kernel-doc:: drivers/gpu/drm/drm_atomic.c :export:
+.. kernel-doc:: drivers/gpu/drm/drm_atomic.c + :internal: + CRTC Abstraction ================
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 01060669dd49..a55f611d2a6f 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -408,11 +408,36 @@ int drm_atomic_set_mode_prop_for_crtc(struct drm_crtc_state *state, } EXPORT_SYMBOL(drm_atomic_set_mode_prop_for_crtc);
+/** + * drm_atomic_replace_property_blob_from_id - lookup the new blob and replace the old one with it + * @dev: DRM device + * @blob: a pointer to the member blob to be replaced + * @blob_id: ID of the new blob + * @expected_size: total expected size of the blob data (in bytes) + * @expected_elem_size: expected element size of the blob data (in bytes) + * @replaced: did the blob get replaced? + * + * Replace @blob with another blob with the ID @blob_id. If @blob_id is zero + * @blob becomes NULL. + * + * If @expected_size is positive the new blob length is expected to be equal + * to @expected_size bytes. If @expected_elem_size is positive the new blob + * length is expected to be a multiple of @expected_elem_size bytes. Otherwise + * an error is returned. + * + * @replaced will indicate to the caller whether the blob was replaced or not. + * If the old and new blobs we in fact the same blob @replaced will be false + * otherwise it will be true. + * + * RETURNS: + * Zero on success, error code on failure. + */ static int drm_atomic_replace_property_blob_from_id(struct drm_device *dev, struct drm_property_blob **blob, uint64_t blob_id, ssize_t expected_size, + ssize_t expected_elem_size, bool *replaced) { struct drm_property_blob *new_blob = NULL; @@ -422,7 +447,13 @@ drm_atomic_replace_property_blob_from_id(struct drm_device *dev, if (new_blob == NULL) return -EINVAL;
- if (expected_size > 0 && expected_size != new_blob->length) { + if (expected_size > 0 && + new_blob->length != expected_size) { + drm_property_blob_put(new_blob); + return -EINVAL; + } + if (expected_elem_size > 0 && + new_blob->length % expected_elem_size != 0) { drm_property_blob_put(new_blob); return -EINVAL; } @@ -470,7 +501,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, ret = drm_atomic_replace_property_blob_from_id(dev, &state->degamma_lut, val, - -1, + -1, sizeof(struct drm_color_lut), &replaced); state->color_mgmt_changed |= replaced; return ret; @@ -478,7 +509,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, ret = drm_atomic_replace_property_blob_from_id(dev, &state->ctm, val, - sizeof(struct drm_color_ctm), + sizeof(struct drm_color_ctm), -1, &replaced); state->color_mgmt_changed |= replaced; return ret; @@ -486,7 +517,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, ret = drm_atomic_replace_property_blob_from_id(dev, &state->gamma_lut, val, - -1, + -1, sizeof(struct drm_color_lut), &replaced); state->color_mgmt_changed |= replaced; return ret;
From: Ville Syrjälä ville.syrjala@linux.intel.com
Provide a small helper to convert the blob length in bytes to the number of LUT entries.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- include/drm/drm_color_mgmt.h | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h index 03a59cbce621..7ddf4457f3c1 100644 --- a/include/drm/drm_color_mgmt.h +++ b/include/drm/drm_color_mgmt.h @@ -37,4 +37,9 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc, int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc, int gamma_size);
+static inline int drm_color_lut_size(const struct drm_property_blob *blob) +{ + return blob->length / sizeof(struct drm_color_lut); +} + #endif
On Fri, Feb 23, 2018 at 09:25:04PM +0200, Ville Syrjala wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Provide a small helper to convert the blob length in bytes to the number of LUT entries.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
include/drm/drm_color_mgmt.h | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h index 03a59cbce621..7ddf4457f3c1 100644 --- a/include/drm/drm_color_mgmt.h +++ b/include/drm/drm_color_mgmt.h @@ -37,4 +37,9 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc, int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc, int gamma_size);
Add a bit of kerneldoc and you get my
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
for this and all remaining patches. -Daniel
+static inline int drm_color_lut_size(const struct drm_property_blob *blob) +{
- return blob->length / sizeof(struct drm_color_lut);
+}
#endif
2.13.6
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Mar 06, 2018 at 08:54:21AM +0100, Daniel Vetter wrote:
On Fri, Feb 23, 2018 at 09:25:04PM +0200, Ville Syrjala wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Provide a small helper to convert the blob length in bytes to the number of LUT entries.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
include/drm/drm_color_mgmt.h | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h index 03a59cbce621..7ddf4457f3c1 100644 --- a/include/drm/drm_color_mgmt.h +++ b/include/drm/drm_color_mgmt.h @@ -37,4 +37,9 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc, int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc, int gamma_size);
Add a bit of kerneldoc and you get my
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
for this and all remaining patches.
Entire series pushed to drm-misc-next. Thanks for the reviews.
-Daniel
+static inline int drm_color_lut_size(const struct drm_property_blob *blob) +{
- return blob->length / sizeof(struct drm_color_lut);
+}
#endif
2.13.6
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
From: Ville Syrjälä ville.syrjala@linux.intel.com
Provide a small helper to convert the blob length in bytes to the number of LUT entries.
v2: Add kerneldoc (Daniel)
Cc: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch --- include/drm/drm_color_mgmt.h | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h index b3b6d302ca8c..44f04233e3db 100644 --- a/include/drm/drm_color_mgmt.h +++ b/include/drm/drm_color_mgmt.h @@ -38,6 +38,18 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc, int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc, int gamma_size);
+/** + * drm_color_lut_size - calculate the number of entries in the LUT + * @blob: blob containing the LUT + * + * Returns: + * The number of entries in the color LUT stored in @blob. + */ +static inline int drm_color_lut_size(const struct drm_property_blob *blob) +{ + return blob->length / sizeof(struct drm_color_lut); +} + enum drm_color_encoding { DRM_COLOR_YCBCR_BT601, DRM_COLOR_YCBCR_BT709,
From: Ville Syrjälä ville.syrjala@linux.intel.com
Now that blob->data is void* again we don't need to cast it.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/i915/intel_color.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c index a383d993b844..e8ede69754a9 100644 --- a/drivers/gpu/drm/i915/intel_color.c +++ b/drivers/gpu/drm/i915/intel_color.c @@ -144,8 +144,7 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state) i9xx_load_ycbcr_conversion_matrix(intel_crtc); return; } else if (crtc_state->ctm) { - struct drm_color_ctm *ctm = - (struct drm_color_ctm *)crtc_state->ctm->data; + struct drm_color_ctm *ctm = crtc_state->ctm->data; uint64_t input[9] = { 0, };
if (intel_crtc_state->limited_color_range) { @@ -254,8 +253,7 @@ static void cherryview_load_csc_matrix(struct drm_crtc_state *state) uint32_t mode;
if (state->ctm) { - struct drm_color_ctm *ctm = - (struct drm_color_ctm *) state->ctm->data; + struct drm_color_ctm *ctm = state->ctm->data; uint16_t coeffs[9] = { 0, }; int i;
@@ -322,7 +320,7 @@ static void i9xx_load_luts_internal(struct drm_crtc *crtc, }
if (blob) { - struct drm_color_lut *lut = (struct drm_color_lut *) blob->data; + struct drm_color_lut *lut = blob->data; for (i = 0; i < 256; i++) { uint32_t word = (drm_color_lut_extract(lut[i].red, 8) << 16) | @@ -392,8 +390,7 @@ static void bdw_load_degamma_lut(struct drm_crtc_state *state) PAL_PREC_SPLIT_MODE | PAL_PREC_AUTO_INCREMENT);
if (state->degamma_lut) { - struct drm_color_lut *lut = - (struct drm_color_lut *) state->degamma_lut->data; + struct drm_color_lut *lut = state->degamma_lut->data;
for (i = 0; i < lut_size; i++) { uint32_t word = @@ -427,8 +424,7 @@ static void bdw_load_gamma_lut(struct drm_crtc_state *state, u32 offset) offset);
if (state->gamma_lut) { - struct drm_color_lut *lut = - (struct drm_color_lut *) state->gamma_lut->data; + struct drm_color_lut *lut = state->gamma_lut->data;
for (i = 0; i < lut_size; i++) { uint32_t word = @@ -560,7 +556,7 @@ static void cherryview_load_luts(struct drm_crtc_state *state) }
if (state->degamma_lut) { - lut = (struct drm_color_lut *) state->degamma_lut->data; + lut = state->degamma_lut->data; lut_size = INTEL_INFO(dev_priv)->color.degamma_lut_size; for (i = 0; i < lut_size; i++) { /* Write LUT in U0.14 format. */ @@ -575,7 +571,7 @@ static void cherryview_load_luts(struct drm_crtc_state *state) }
if (state->gamma_lut) { - lut = (struct drm_color_lut *) state->gamma_lut->data; + lut = state->gamma_lut->data; lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size; for (i = 0; i < lut_size; i++) { /* Write LUT in U0.10 format. */
From: Ville Syrjälä ville.syrjala@linux.intel.com
Now that blob->data is void* again we don't need to cast it.
v2: Rebase
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/i915/intel_color.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c index 89ab0f70aa22..92f148832a89 100644 --- a/drivers/gpu/drm/i915/intel_color.c +++ b/drivers/gpu/drm/i915/intel_color.c @@ -153,8 +153,7 @@ static void ilk_load_csc_matrix(struct drm_crtc_state *crtc_state) ilk_load_ycbcr_conversion_matrix(intel_crtc); return; } else if (crtc_state->ctm) { - struct drm_color_ctm *ctm = - (struct drm_color_ctm *)crtc_state->ctm->data; + struct drm_color_ctm *ctm = crtc_state->ctm->data; const u64 *input; u64 temp[9];
@@ -262,8 +261,7 @@ static void cherryview_load_csc_matrix(struct drm_crtc_state *state) uint32_t mode;
if (state->ctm) { - struct drm_color_ctm *ctm = - (struct drm_color_ctm *) state->ctm->data; + struct drm_color_ctm *ctm = state->ctm->data; uint16_t coeffs[9] = { 0, }; int i;
@@ -330,7 +328,7 @@ static void i9xx_load_luts_internal(struct drm_crtc *crtc, }
if (blob) { - struct drm_color_lut *lut = (struct drm_color_lut *) blob->data; + struct drm_color_lut *lut = blob->data; for (i = 0; i < 256; i++) { uint32_t word = (drm_color_lut_extract(lut[i].red, 8) << 16) | @@ -400,8 +398,7 @@ static void bdw_load_degamma_lut(struct drm_crtc_state *state) PAL_PREC_SPLIT_MODE | PAL_PREC_AUTO_INCREMENT);
if (state->degamma_lut) { - struct drm_color_lut *lut = - (struct drm_color_lut *) state->degamma_lut->data; + struct drm_color_lut *lut = state->degamma_lut->data;
for (i = 0; i < lut_size; i++) { uint32_t word = @@ -435,8 +432,7 @@ static void bdw_load_gamma_lut(struct drm_crtc_state *state, u32 offset) offset);
if (state->gamma_lut) { - struct drm_color_lut *lut = - (struct drm_color_lut *) state->gamma_lut->data; + struct drm_color_lut *lut = state->gamma_lut->data;
for (i = 0; i < lut_size; i++) { uint32_t word = @@ -568,7 +564,7 @@ static void cherryview_load_luts(struct drm_crtc_state *state) }
if (state->degamma_lut) { - lut = (struct drm_color_lut *) state->degamma_lut->data; + lut = state->degamma_lut->data; lut_size = INTEL_INFO(dev_priv)->color.degamma_lut_size; for (i = 0; i < lut_size; i++) { /* Write LUT in U0.14 format. */ @@ -583,7 +579,7 @@ static void cherryview_load_luts(struct drm_crtc_state *state) }
if (state->gamma_lut) { - lut = (struct drm_color_lut *) state->gamma_lut->data; + lut = state->gamma_lut->data; lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size; for (i = 0; i < lut_size; i++) { /* Write LUT in U0.10 format. */
From: Ville Syrjälä ville.syrjala@linux.intel.com
Avoid all the sizeof(drm_color_lut) business by using drm_color_lut_size() to convert the blob length into number of LUT entries.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/i915/intel_color.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c index e8ede69754a9..029c2c931fab 100644 --- a/drivers/gpu/drm/i915/intel_color.c +++ b/drivers/gpu/drm/i915/intel_color.c @@ -39,7 +39,7 @@ #define CTM_COEFF_NEGATIVE(coeff) (((coeff) & CTM_COEFF_SIGN) != 0) #define CTM_COEFF_ABS(coeff) ((coeff) & (CTM_COEFF_SIGN - 1))
-#define LEGACY_LUT_LENGTH (sizeof(struct drm_color_lut) * 256) +#define LEGACY_LUT_LENGTH 256
/* Post offset values for RGB->YCBCR conversion */ #define POSTOFF_RGB_TO_YUV_HI 0x800 @@ -79,7 +79,7 @@ static bool crtc_state_is_legacy_gamma(struct drm_crtc_state *state) return !state->degamma_lut && !state->ctm && state->gamma_lut && - state->gamma_lut->length == LEGACY_LUT_LENGTH; + drm_color_lut_size(state->gamma_lut) == LEGACY_LUT_LENGTH; }
/* @@ -611,19 +611,17 @@ int intel_color_check(struct drm_crtc *crtc, struct drm_i915_private *dev_priv = to_i915(crtc->dev); size_t gamma_length, degamma_length;
- degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size * - sizeof(struct drm_color_lut); - gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size * - sizeof(struct drm_color_lut); + degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size; + gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
/* * We allow both degamma & gamma luts at the right size or * NULL. */ if ((!crtc_state->degamma_lut || - crtc_state->degamma_lut->length == degamma_length) && + drm_color_lut_size(crtc_state->degamma_lut) == degamma_length) && (!crtc_state->gamma_lut || - crtc_state->gamma_lut->length == gamma_length)) + drm_color_lut_size(crtc_state->gamma_lut) == gamma_length)) return 0;
/*
Regards
Shashank
On 2/24/2018 12:55 AM, Ville Syrjala wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Using a flexible array for the blob data was a mistake by me. It forces all users of the blob data to cast blob->data to something else. void* is clearly superior so let's go back to the original scheme.
Not a clean revert as the code has moved.
This reverts commit d63f5e6bf6f2a1573ea39c9937cdf5ab0b3a4b77.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/drm_property.c | 1 + include/drm/drm_property.h | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c index bae50e6b819d..0f6620fea3de 100644 --- a/drivers/gpu/drm/drm_property.c +++ b/drivers/gpu/drm/drm_property.c @@ -550,6 +550,7 @@ drm_property_create_blob(struct drm_device *dev, size_t length, /* This must be explicitly initialised, so we can safely call list_del * on it in the removal handler, even if it isn't in a file list. */ INIT_LIST_HEAD(&blob->head_file);
- blob->data = (void *)blob + sizeof(*blob); blob->length = length; blob->dev = dev;
diff --git a/include/drm/drm_property.h b/include/drm/drm_property.h index 8a522b4bed40..265fd1f2e112 100644 --- a/include/drm/drm_property.h +++ b/include/drm/drm_property.h @@ -209,7 +209,7 @@ struct drm_property_blob { struct list_head head_global; struct list_head head_file; size_t length;
- unsigned char data[];
void *data; };
struct drm_prop_enum_list {
Reviewed-by: Shashank Sharma shashank.sharma@intel.com
Regards Shashank
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Sharma, Shashank -
Ville Syrjala -
Ville Syrjälä