Hi Mahesh,
Thank you for the patch.
On Monday, 23 July 2018 13:44:51 EEST Mahesh Kumar wrote:
> This patch implements get_crc_sources callback, which returns list of
> all the crc sources supported by driver in current platform.
>
> Changes Since V1:
> - move sources list per-crtc
> - init sources-list only for gen3
> Changes Since V2:
> - Adopt to driver style
> - Address other review comments from Laurent Pinchart
I'm pretty sure this has changed since v4 as well.
> Signed-off-by: Mahesh Kumar <mahesh1.ku...@intel.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> ---
> drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 85 ++++++++++++++++++++++++++++++-
> drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 3 ++
> 2 files changed, 87 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 43e67cffdee0..39981ce422e1
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -691,6 +691,68 @@ static const struct drm_crtc_helper_funcs
> crtc_helper_funcs = {
> .atomic_disable = rcar_du_crtc_atomic_disable,
> };
>
> +static void rcar_du_crtc_crc_sources_list_init(struct rcar_du_crtc *rcrtc)
Let's shorten the function name to rcar_du_crtc_crc_init().
> +{
> + struct rcar_du_device *rcdu = rcrtc->group->dev;
> + const char **sources;
> + unsigned int count;
> + int i = -1;
> +
> + /* CRC available only on Gen3 HW. */
> + if (rcdu->info->gen < 3)
> + return;
> +
> + /* Reserve 1 for "auto" source. */
> + count = rcrtc->vsp->num_planes + 1;
> +
> + sources = kmalloc_array(count, sizeof(*sources), GFP_KERNEL);
> + if (!sources)
> + return;
> +
> + sources[0] = kstrdup("auto", GFP_KERNEL);
> + if (!sources[0])
> + goto error;
> +
> + for (i = 0; i < rcrtc->vsp->num_planes; ++i) {
> + struct drm_plane *plane = &rcrtc->vsp->planes[i].plane;
> + char name[16];
> +
> + sprintf(name, "plane%u", plane->base.id);
> + sources[i + 1] = kstrdup(name, GFP_KERNEL);
> + if (!sources[i + 1])
> + goto error;
> + }
> +
> + rcrtc->sources = sources;
> + rcrtc->sources_count = count;
> + return;
> +
> +error:
> + while (i >= 0) {
> + kfree(sources[i]);
> + i--;
> + }
> + kfree(sources);
> +
> + rcrtc->sources = NULL;
> + rcrtc->sources_count = 0;
The two last lines are not needed as ->sources and ->sources_count are only
initialized at the very end of the function if no error occurred.
> +}
> +
> +static void rcar_du_crtc_crc_sources_list_uninit(struct rcar_du_crtc
> *rcrtc)
Similarly, and for consistency as the driver uses the _cleanup suffix
throughout the code, I would name this rcar_du_crtc_crc_cleanup().
With those three small changes,
Reviewed-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> +{
> + unsigned int i;
> +
> + if (!rcrtc->sources)
> + return;
> +
> + for (i = 0; i < rcrtc->sources_count; i++)
> + kfree(rcrtc->sources[i]);
> + kfree(rcrtc->sources);
> +
> + rcrtc->sources = NULL;
> + rcrtc->sources_count = 0;
> +}
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
