[PATCH v2 2/2] drm/omap: Major omap_modeset_init() cleanup

[Tomi Valkeinen]

On 20/03/17 18:06, Jyri Sarha wrote:
> Cleanup overly complex omap_modeset_init(). The function is trying to
> support many unusual configuration, that have never been tested and
> are not supported by other parts of the dirver.
> 
> After cleanup the init function creates exactly one connector,
> encoder, crtc, and primary plane per each connected dss-device. Each
> connector->encoder->crtc chain is expected to be separate and each
> crtc is connect to a single dss-channel. If the configuration does not
> match the expectations or exceeds the available resources, the
> configuration is rejected.
> 
> Signed-off-by: Jyri Sarha <jsarha at ti.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_crtc.c  |  23 ++++-
>  drivers/gpu/drm/omapdrm/omap_drv.c   | 173 +++++++++--------------------------
>  drivers/gpu/drm/omapdrm/omap_drv.h   |   2 +-
>  drivers/gpu/drm/omapdrm/omap_plane.c |   3 +
>  4 files changed, 68 insertions(+), 133 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
> index 2fe735c..4bafc7b 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -521,6 +521,11 @@ static int omap_crtc_atomic_get_property(struct drm_crtc *crtc,
>  
>  void omap_crtc_pre_init(void)
>  {
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(omap_crtcs); i++)
> +		omap_crtcs[i] = NULL;
> +

Use memset to clear.

> @@ -318,9 +279,9 @@ static int omap_modeset_init(struct drm_device *dev)
>  	int num_ovls = dss_feat_get_num_ovls();
>  	int num_mgrs = dss_feat_get_num_mgrs();
>  	int num_crtcs = 0;
> -	int i, id = 0;
> +	int crtc_idx = 0, plane_idx = 0;

In a bit longer function it's usually better to initialize the variables
near to the place where they are first used, so that you will see the
initial value when looking at the code in question.

> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
> index 386d90a..8fc1f4a 100644
> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
> @@ -371,6 +371,9 @@ struct drm_plane *omap_plane_init(struct drm_device *dev,
>  	return plane;
>  
>  error:
> +	dev_err(dev->dev, "%s(): could not create plane: %s\n",
> +		__func__, plane_names[id]);
> +
>  	kfree(omap_plane);
>  	return NULL;
>  }

omap_plane_init() still uses "id" instead of "idx", and it's stored in
struct omap_plane as 'id', and there's no explicit mapping of the DRM
idx to the DSS plane id. This is confusing as the DRM plane idx is not
strictly tied to the DSS plane id.

So, call it "idx" when it's about the DRM plane index and call it
something else when it's about the dss plane id (it's "enum omap_plane"
in dss, but that clashes with drm's struct omap_plane. So...
'dss_plane'?). And create a function that maps the DRM plane idx to DSS
plane id (for now it can be just a direct map).

This change should probably be an additional patch.

Otherwise I think this looks good, much cleaner than the current confusion.

 Tomi

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20170321/6ad9638d/attachment.sig>