Re: [PATCH v5 10/10] drm/rcar-du/crc: Implement get_crc_sources callback

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.kumar@intel.com Cc: dri-devel@lists.freedesktop.org Cc: Laurent Pinchart laurent.pinchart@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.pinchart@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;

+}