Re: [PATCHv3 02/30] drm/omap: refactor CRTC HW property setup

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Tomi,

Thank you for the patch.

On Tuesday 28 Mar 2017 16:07:48 Tomi Valkeinen wrote:
> The current driver doesn't expose any of the CRTC HW properties like
> background color or transparency key, and sets them at CRTC enable time.
> 
> Refactor this into a separate function and call that function from
> omap_crtc_atomic_flush(). This is the behavior we want when the
> properties can be configured, so this patch makes it easier to add
> patches later which implement those properties.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_crtc.c | 25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c
> b/drivers/gpu/drm/omapdrm/omap_crtc.c index 2fe735c269fc..49fc61963af4
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -198,15 +198,7 @@ static void omap_crtc_set_enabled(struct drm_crtc
> *crtc, bool enable) static int omap_crtc_dss_enable(enum omap_channel
> channel)
>  {
>  	struct omap_crtc *omap_crtc = omap_crtcs[channel];
> -	struct omap_overlay_manager_info info;
> 
> -	memset(&info, 0, sizeof(info));
> -	info.default_color = 0x00000000;
> -	info.trans_key = 0x00000000;
> -	info.trans_key_type = OMAP_DSS_COLOR_KEY_GFX_DST;
> -	info.trans_enabled = false;
> -
> -	dispc_mgr_setup(omap_crtc->channel, &info);
>  	dispc_mgr_set_timings(omap_crtc->channel,
>  			&omap_crtc->vm);
>  	omap_crtc_set_enabled(&omap_crtc->base, true);
> @@ -313,6 +305,21 @@ void omap_crtc_vblank_irq(struct drm_crtc *crtc)
>  	DBG("%s: apply done", omap_crtc->name);
>  }
> 
> +static void omap_crtc_write_crtc_properties(struct drm_crtc *crtc)
> +{
> +	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
> +	struct omap_overlay_manager_info info;
> +
> +	memset(&info, 0, sizeof(info));
> +
> +	info.default_color = 0x000000;
> +	info.trans_enabled = false;
> +	info.partial_alpha_enabled = false;
> +	info.cpr_enable = false;

Nitpicking, you could initialize info when declaring it, I think it would be 
slightly more efficient.

> +
> +	dispc_mgr_setup(omap_crtc->channel, &info);

dispc_mgr_setup() was previously called from omap_crtc_dss_enable() with the 
DSS disabled. Now it can be called with the DSS enabled. Have you double-
checked that this isn't an issue ?

> +}
> +
>  /* ------------------------------------------------------------------------
>   * CRTC Functions
>   */
> @@ -410,6 +417,8 @@ static void omap_crtc_atomic_flush(struct drm_crtc
> *crtc, dispc_mgr_set_gamma(omap_crtc->channel, lut, length);
>  	}
> 
> +	omap_crtc_write_crtc_properties(crtc);
> +
>  	/* Only flush the CRTC if it is currently enabled. */
>  	if (!omap_crtc->enabled)
>  		return;

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel