Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 5 Jun 2019 10:22:21 +0100
From:   Liviu Dudau <Liviu.Dudau@arm.com>
To:     "Lowry Li (Arm Technology China)" <Lowry.Li@arm.com>
Cc:     "james qian wang (Arm Technology China)" <james.qian.wang@arm.com>,
        "maarten.lankhorst@linux.intel.com" 
        <maarten.lankhorst@linux.intel.com>,
        "seanpaul@chromium.org" <seanpaul@chromium.org>,
        "airlied@linux.ie" <airlied@linux.ie>,
        Brian Starkey <Brian.Starkey@arm.com>,
        "Julien Yin (Arm Technology China)" <Julien.Yin@arm.com>,
        "Jonathan Chai (Arm Technology China)" <Jonathan.Chai@arm.com>,
        Ayan Halder <Ayan.Halder@arm.com>,
        "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        nd <nd@arm.com>
Subject: Re: [PATCH v1 1/2] drm/komeda: Add rotation support on Komeda driver
Message-ID: <20190605092221.GS15316@e110455-lin.cambridge.arm.com>
References: <1555902945-2877-1-git-send-email-lowry.li@arm.com>
 <1555902945-2877-2-git-send-email-lowry.li@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <1555902945-2877-2-git-send-email-lowry.li@arm.com>
User-Agent: Mutt/1.11.4 (2019-03-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Lowry,

On Mon, Apr 22, 2019 at 04:16:26AM +0100, Lowry Li (Arm Technology China) wrote:
> - Adds rotation property to plane.
> - Komeda display rotation support diverges from the specific formats,
> so need to check the user required rotation type with the format caps
> and reject the commit if it can not be supported.
> - In the layer validate flow, sets the rotation value to the layer
> state. If r90 or r270, swap the width and height of the data flow
> for next stage.
> 
> Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com>
> ---
>  drivers/gpu/drm/arm/display/komeda/komeda_format_caps.h  | 11 +++++++++++
>  .../gpu/drm/arm/display/komeda/komeda_pipeline_state.c   |  7 +++++++
>  drivers/gpu/drm/arm/display/komeda/komeda_plane.c        | 16 ++++++++++++++++
>  3 files changed, 34 insertions(+)
> 
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_format_caps.h b/drivers/gpu/drm/arm/display/komeda/komeda_format_caps.h
> index bc3b2df36..96de22e 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_format_caps.h
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_format_caps.h
> @@ -79,6 +79,17 @@ struct komeda_format_caps_table {
>  
>  extern u64 komeda_supported_modifiers[];
>  
> +static inline const char *komeda_get_format_name(u32 fourcc, u64 modifier)
> +{
> +	struct drm_format_name_buf buf;
> +	static char name[64];
> +
> +	snprintf(name, sizeof(name), "%s with modifier: 0x%llx.",
> +		 drm_get_format_name(fourcc, &buf), modifier);
> +
> +	return name;
> +}

Can you roll the content of this function inside the if () {.....} part? We
only have one use for it, I don't see the need to split it into a separate
function.

With that: Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>

Best regards,
Liviu

> +
>  const struct komeda_format_caps *
>  komeda_get_format_caps(struct komeda_format_caps_table *table,
>  		       u32 fourcc, u64 modifier);
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> index 9b29e9a..8c133e4 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> @@ -317,6 +317,13 @@ struct komeda_pipeline_state *
>  	/* update the data flow for the next stage */
>  	komeda_component_set_output(&dflow->input, &layer->base, 0);
>  
> +	/*
> +	 * The rotation has been handled by layer, so adjusted the data flow for
> +	 * the next stage.
> +	 */
> +	if (drm_rotation_90_or_270(st->rot))
> +		swap(dflow->in_h, dflow->in_w);
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
> index 14d6861..5e5bfdb 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
> @@ -9,12 +9,14 @@
>  #include <drm/drm_plane_helper.h>
>  #include "komeda_dev.h"
>  #include "komeda_kms.h"
> +#include "komeda_framebuffer.h"
>  
>  static int
>  komeda_plane_init_data_flow(struct drm_plane_state *st,
>  			    struct komeda_data_flow_cfg *dflow)
>  {
>  	struct drm_framebuffer *fb = st->fb;
> +	const struct komeda_format_caps *caps = to_kfb(fb)->format_caps;
>  
>  	memset(dflow, 0, sizeof(*dflow));
>  
> @@ -35,6 +37,15 @@
>  	dflow->in_w = st->src_w >> 16;
>  	dflow->in_h = st->src_h >> 16;
>  
> +	dflow->rot = drm_rotation_simplify(st->rotation, caps->supported_rots);
> +	if (!has_bits(dflow->rot, caps->supported_rots)) {
> +		DRM_DEBUG_ATOMIC("rotation(0x%x) isn't supported by %s.\n",
> +				 dflow->rot,
> +				 komeda_get_format_name(caps->fourcc,
> +							fb->modifier));
> +		return -EINVAL;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -233,6 +244,11 @@ static int komeda_plane_add(struct komeda_kms_dev *kms,
>  
>  	drm_plane_helper_add(plane, &komeda_plane_helper_funcs);
>  
> +	err = drm_plane_create_rotation_property(plane, DRM_MODE_ROTATE_0,
> +						 layer->supported_rots);
> +	if (err)
> +		goto cleanup;
> +
>  	err = drm_plane_create_alpha_property(plane);
>  	if (err)
>  		goto cleanup;
> -- 
> 1.9.1
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯