| Message ID | 20180105165600.45568-6-noralf@tronnes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/05/2018 10:56 AM, Noralf Trønnes wrote: > Embed the mode in tinydrm_connector instead of doing an devm_ allocation. > Remove unnecessary use of ret variable at the end of > tinydrm_display_pipe_init(). > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > --- > drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c | 34 +++++++++++------------------ > 1 file changed, 13 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c > index f41fc506ff87..dadd26d53216 100644 > --- a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c > +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c > @@ -15,7 +15,7 @@ > > struct tinydrm_connector { > struct drm_connector base; > - const struct drm_display_mode *mode; > + struct drm_display_mode mode; > }; > > static inline struct tinydrm_connector * > @@ -29,7 +29,7 @@ static int tinydrm_connector_get_modes(struct drm_connector *connector) > struct tinydrm_connector *tconn = to_tinydrm_connector(connector); > struct drm_display_mode *mode; > > - mode = drm_mode_duplicate(connector->dev, tconn->mode); > + mode = drm_mode_duplicate(connector->dev, &tconn->mode); > if (!mode) { > DRM_ERROR("Failed to duplicate mode\n"); > return 0; > @@ -92,7 +92,7 @@ tinydrm_connector_create(struct drm_device *drm, > if (!tconn) > return ERR_PTR(-ENOMEM); > > - tconn->mode = mode; > + tconn->mode = *mode; I see there is a special drm_mode_copy() function, so I am wondering if this is safe. > connector = &tconn->base; > > drm_connector_helper_add(connector, &tinydrm_connector_hfuncs); > @@ -199,35 +199,27 @@ tinydrm_display_pipe_init(struct tinydrm_device *tdev, > unsigned int rotation) > { > struct drm_device *drm = tdev->drm; > - struct drm_display_mode *mode_copy; > + struct drm_display_mode mode_copy; > struct drm_connector *connector; > int ret; > > - mode_copy = devm_kmalloc(drm->dev, sizeof(*mode_copy), GFP_KERNEL); > - if (!mode_copy) > - return -ENOMEM; > - > - *mode_copy = *mode; > - ret = tinydrm_rotate_mode(mode_copy, rotation); > + mode_copy = *mode; Same here. > + ret = tinydrm_rotate_mode(&mode_copy, rotation); > if (ret) { > DRM_ERROR("Illegal rotation value %u\n", rotation); > return -EINVAL; > } > > - drm->mode_config.min_width = mode_copy->hdisplay; > - drm->mode_config.max_width = mode_copy->hdisplay; > - drm->mode_config.min_height = mode_copy->vdisplay; > - drm->mode_config.max_height = mode_copy->vdisplay; > + drm->mode_config.min_width = mode_copy.hdisplay; > + drm->mode_config.max_width = mode_copy.hdisplay; > + drm->mode_config.min_height = mode_copy.vdisplay; > + drm->mode_config.max_height = mode_copy.vdisplay; > > - connector = tinydrm_connector_create(drm, mode_copy, connector_type); > + connector = tinydrm_connector_create(drm, &mode_copy, connector_type); > if (IS_ERR(connector)) > return PTR_ERR(connector); > > - ret = drm_simple_display_pipe_init(drm, &tdev->pipe, funcs, formats, > - format_count, NULL, connector); > - if (ret) > - return ret; > - > - return 0; > + return drm_simple_display_pipe_init(drm, &tdev->pipe, funcs, formats, > + format_count, NULL, connector); > } > EXPORT_SYMBOL(tinydrm_display_pipe_init); >
Den 05.01.2018 20.14, skrev David Lechner: > On 01/05/2018 10:56 AM, Noralf Trønnes wrote: >> Embed the mode in tinydrm_connector instead of doing an devm_ >> allocation. >> Remove unnecessary use of ret variable at the end of >> tinydrm_display_pipe_init(). >> >> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> >> --- >> drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c | 34 >> +++++++++++------------------ >> 1 file changed, 13 insertions(+), 21 deletions(-) >> >> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c >> b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c >> index f41fc506ff87..dadd26d53216 100644 >> --- a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c >> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c >> @@ -15,7 +15,7 @@ >> struct tinydrm_connector { >> struct drm_connector base; >> - const struct drm_display_mode *mode; >> + struct drm_display_mode mode; >> }; >> static inline struct tinydrm_connector * >> @@ -29,7 +29,7 @@ static int tinydrm_connector_get_modes(struct >> drm_connector *connector) >> struct tinydrm_connector *tconn = to_tinydrm_connector(connector); >> struct drm_display_mode *mode; >> - mode = drm_mode_duplicate(connector->dev, tconn->mode); >> + mode = drm_mode_duplicate(connector->dev, &tconn->mode); >> if (!mode) { >> DRM_ERROR("Failed to duplicate mode\n"); >> return 0; >> @@ -92,7 +92,7 @@ tinydrm_connector_create(struct drm_device *drm, >> if (!tconn) >> return ERR_PTR(-ENOMEM); >> - tconn->mode = mode; >> + tconn->mode = *mode; > > I see there is a special drm_mode_copy() function, so I am wondering > if this > is safe. > It is safe, the underlying drm_mode_object isn't initialized/registered. So to me an assignment like this makes it clear that it is so, but Laurent also asked about this, so maybe I'm the odd one here. I'll switch to drm_mode_copy(). Noralf. >> connector = &tconn->base; >> drm_connector_helper_add(connector, &tinydrm_connector_hfuncs); >> @@ -199,35 +199,27 @@ tinydrm_display_pipe_init(struct tinydrm_device >> *tdev, >> unsigned int rotation) >> { >> struct drm_device *drm = tdev->drm; >> - struct drm_display_mode *mode_copy; >> + struct drm_display_mode mode_copy; >> struct drm_connector *connector; >> int ret; >> - mode_copy = devm_kmalloc(drm->dev, sizeof(*mode_copy), >> GFP_KERNEL); >> - if (!mode_copy) >> - return -ENOMEM; >> - >> - *mode_copy = *mode; >> - ret = tinydrm_rotate_mode(mode_copy, rotation); >> + mode_copy = *mode; > > Same here. > >> + ret = tinydrm_rotate_mode(&mode_copy, rotation); >> if (ret) { >> DRM_ERROR("Illegal rotation value %u\n", rotation); >> return -EINVAL; >> } >> - drm->mode_config.min_width = mode_copy->hdisplay; >> - drm->mode_config.max_width = mode_copy->hdisplay; >> - drm->mode_config.min_height = mode_copy->vdisplay; >> - drm->mode_config.max_height = mode_copy->vdisplay; >> + drm->mode_config.min_width = mode_copy.hdisplay; >> + drm->mode_config.max_width = mode_copy.hdisplay; >> + drm->mode_config.min_height = mode_copy.vdisplay; >> + drm->mode_config.max_height = mode_copy.vdisplay; >> - connector = tinydrm_connector_create(drm, mode_copy, >> connector_type); >> + connector = tinydrm_connector_create(drm, &mode_copy, >> connector_type); >> if (IS_ERR(connector)) >> return PTR_ERR(connector); >> - ret = drm_simple_display_pipe_init(drm, &tdev->pipe, funcs, >> formats, >> - format_count, NULL, connector); >> - if (ret) >> - return ret; >> - >> - return 0; >> + return drm_simple_display_pipe_init(drm, &tdev->pipe, funcs, >> formats, >> + format_count, NULL, connector); >> } >> EXPORT_SYMBOL(tinydrm_display_pipe_init); >> >
On 01/06/2018 06:49 AM, Noralf Trønnes wrote: > > Den 05.01.2018 20.14, skrev David Lechner: >> On 01/05/2018 10:56 AM, Noralf Trønnes wrote: >>> Embed the mode in tinydrm_connector instead of doing an devm_ allocation. >>> Remove unnecessary use of ret variable at the end of >>> tinydrm_display_pipe_init(). >>> >>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> >>> --- >>> drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c | 34 +++++++++++------------------ >>> 1 file changed, 13 insertions(+), 21 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c >>> index f41fc506ff87..dadd26d53216 100644 >>> --- a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c >>> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c >>> @@ -15,7 +15,7 @@ >>> struct tinydrm_connector { >>> struct drm_connector base; >>> - const struct drm_display_mode *mode; >>> + struct drm_display_mode mode; >>> }; >>> static inline struct tinydrm_connector * >>> @@ -29,7 +29,7 @@ static int tinydrm_connector_get_modes(struct drm_connector *connector) >>> struct tinydrm_connector *tconn = to_tinydrm_connector(connector); >>> struct drm_display_mode *mode; >>> - mode = drm_mode_duplicate(connector->dev, tconn->mode); >>> + mode = drm_mode_duplicate(connector->dev, &tconn->mode); >>> if (!mode) { >>> DRM_ERROR("Failed to duplicate mode\n"); >>> return 0; >>> @@ -92,7 +92,7 @@ tinydrm_connector_create(struct drm_device *drm, >>> if (!tconn) >>> return ERR_PTR(-ENOMEM); >>> - tconn->mode = mode; >>> + tconn->mode = *mode; >> >> I see there is a special drm_mode_copy() function, so I am wondering if this >> is safe. >> > > It is safe, the underlying drm_mode_object isn't initialized/registered. > So to me an assignment like this makes it clear that it is so, but Laurent > also asked about this, so maybe I'm the odd one here. > I'll switch to drm_mode_copy(). > I think a comment to this effect would be enough.
diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c index f41fc506ff87..dadd26d53216 100644 --- a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c @@ -15,7 +15,7 @@ struct tinydrm_connector { struct drm_connector base; - const struct drm_display_mode *mode; + struct drm_display_mode mode; }; static inline struct tinydrm_connector * @@ -29,7 +29,7 @@ static int tinydrm_connector_get_modes(struct drm_connector *connector) struct tinydrm_connector *tconn = to_tinydrm_connector(connector); struct drm_display_mode *mode; - mode = drm_mode_duplicate(connector->dev, tconn->mode); + mode = drm_mode_duplicate(connector->dev, &tconn->mode); if (!mode) { DRM_ERROR("Failed to duplicate mode\n"); return 0; @@ -92,7 +92,7 @@ tinydrm_connector_create(struct drm_device *drm, if (!tconn) return ERR_PTR(-ENOMEM); - tconn->mode = mode; + tconn->mode = *mode; connector = &tconn->base; drm_connector_helper_add(connector, &tinydrm_connector_hfuncs); @@ -199,35 +199,27 @@ tinydrm_display_pipe_init(struct tinydrm_device *tdev, unsigned int rotation) { struct drm_device *drm = tdev->drm; - struct drm_display_mode *mode_copy; + struct drm_display_mode mode_copy; struct drm_connector *connector; int ret; - mode_copy = devm_kmalloc(drm->dev, sizeof(*mode_copy), GFP_KERNEL); - if (!mode_copy) - return -ENOMEM; - - *mode_copy = *mode; - ret = tinydrm_rotate_mode(mode_copy, rotation); + mode_copy = *mode; + ret = tinydrm_rotate_mode(&mode_copy, rotation); if (ret) { DRM_ERROR("Illegal rotation value %u\n", rotation); return -EINVAL; } - drm->mode_config.min_width = mode_copy->hdisplay; - drm->mode_config.max_width = mode_copy->hdisplay; - drm->mode_config.min_height = mode_copy->vdisplay; - drm->mode_config.max_height = mode_copy->vdisplay; + drm->mode_config.min_width = mode_copy.hdisplay; + drm->mode_config.max_width = mode_copy.hdisplay; + drm->mode_config.min_height = mode_copy.vdisplay; + drm->mode_config.max_height = mode_copy.vdisplay; - connector = tinydrm_connector_create(drm, mode_copy, connector_type); + connector = tinydrm_connector_create(drm, &mode_copy, connector_type); if (IS_ERR(connector)) return PTR_ERR(connector); - ret = drm_simple_display_pipe_init(drm, &tdev->pipe, funcs, formats, - format_count, NULL, connector); - if (ret) - return ret; - - return 0; + return drm_simple_display_pipe_init(drm, &tdev->pipe, funcs, formats, + format_count, NULL, connector); } EXPORT_SYMBOL(tinydrm_display_pipe_init);
Embed the mode in tinydrm_connector instead of doing an devm_ allocation. Remove unnecessary use of ret variable at the end of tinydrm_display_pipe_init(). Signed-off-by: Noralf Trønnes <noralf@tronnes.org> --- drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c | 34 +++++++++++------------------ 1 file changed, 13 insertions(+), 21 deletions(-)