[PATCH v6 03/12] drm/exynos: atomic phase 1: add .mode_set_nofb() callback

[Joonyoung Shim]

On 05/23/2015 12:33 AM, Gustavo Padovan wrote:
> Hi Joonyoung,
> 
> 2015-05-22 Joonyoung Shim :
> 
>> On 05/22/2015 05:02 AM, Gustavo Padovan wrote:
>>> From: Gustavo Padovan 
>>>
>>> The new atomic infrastructure needs the .mode_set_nofb() callback to
>>> update CRTC timings before setting any plane.
>>>
>>> Signed-off-by: Gustavo Padovan 
>>> ---
>>>  drivers/gpu/drm/exynos/exynos_drm_crtc.c | 60 
>>> +---
>>>  1 file changed, 9 insertions(+), 51 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c 
>>> b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>> index 61b8cfe..54b74e1 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>> @@ -81,59 +81,16 @@ exynos_drm_crtc_mode_fixup(struct drm_crtc *crtc,
>>> return true;
>>>  }
>>>  
>>> -static int
>>> -exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode 
>>> *mode,
>>> - struct drm_display_mode *adjusted_mode, int x, int y,
>>> - struct drm_framebuffer *old_fb)
>>> -{
>>> -   struct drm_framebuffer *fb = crtc->primary->fb;
>>> -   unsigned int crtc_w;
>>> -   unsigned int crtc_h;
>>> -   int ret;
>>> -
>>> -   /*
>>> -* copy the mode data adjusted by mode_fixup() into crtc->mode
>>> -* so that hardware can be seet to proper mode.
>>> -*/
>>> -   memcpy(>mode, adjusted_mode, sizeof(*adjusted_mode));
>>
>> This can cause any problem because exynos drm drivers use crtc->mode
>> directly as adjusted_mode. It's necessary to consider using
>> crtc_state->adjusted_mode in exynos drm drivers.
> 
> Yes, we are moving to use adjusted_mode in exynos, see this patch from
> Tobias. It makes crtc uses the adjusted_mode instead. So I think it is
> okay to remove this memcpy from here.
> 
> http://www.spinics.net/lists/linux-samsung-soc/msg44790.html

Yeah, but that is just one, it can be other missing points. It's better
to care all missing points with removing memcpy of adjusted_mode.

[PATCH v6 03/12] drm/exynos: atomic phase 1: add .mode_set_nofb() callback

[Joonyoung Shim]

On 05/22/2015 05:02 AM, Gustavo Padovan wrote:
> From: Gustavo Padovan 
> 
> The new atomic infrastructure needs the .mode_set_nofb() callback to
> update CRTC timings before setting any plane.
> 
> Signed-off-by: Gustavo Padovan 
> ---
>  drivers/gpu/drm/exynos/exynos_drm_crtc.c | 60 
> +---
>  1 file changed, 9 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c 
> b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> index 61b8cfe..54b74e1 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> @@ -81,59 +81,16 @@ exynos_drm_crtc_mode_fixup(struct drm_crtc *crtc,
>   return true;
>  }
>  
> -static int
> -exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode 
> *mode,
> -   struct drm_display_mode *adjusted_mode, int x, int y,
> -   struct drm_framebuffer *old_fb)
> -{
> - struct drm_framebuffer *fb = crtc->primary->fb;
> - unsigned int crtc_w;
> - unsigned int crtc_h;
> - int ret;
> -
> - /*
> -  * copy the mode data adjusted by mode_fixup() into crtc->mode
> -  * so that hardware can be seet to proper mode.
> -  */
> - memcpy(>mode, adjusted_mode, sizeof(*adjusted_mode));

This can cause any problem because exynos drm drivers use crtc->mode
directly as adjusted_mode. It's necessary to consider using
crtc_state->adjusted_mode in exynos drm drivers.

Please refer a patch of Daniel Stone using hwmode instead of mode but it
cannot use for atomic.

http://lists.freedesktop.org/archives/dri-devel/2015-March/079546.html

> -
> - ret = exynos_check_plane(crtc->primary, fb);
> - if (ret < 0)
> - return ret;
> -
> - crtc_w = fb->width - x;
> - crtc_h = fb->height - y;
> - exynos_plane_mode_set(crtc->primary, crtc, fb, 0, 0,
> -   crtc_w, crtc_h, x, y, crtc_w, crtc_h);
> -
> - return 0;
> -}
> -
> -static int exynos_drm_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
> -   struct drm_framebuffer *old_fb)
> +static void
> +exynos_drm_crtc_mode_set_nofb(struct drm_crtc *crtc)
>  {
>   struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
> - struct drm_framebuffer *fb = crtc->primary->fb;
> - unsigned int crtc_w;
> - unsigned int crtc_h;
> - int ret;
>  
> - /* when framebuffer changing is requested, crtc's dpms should be on */
> - if (exynos_crtc->dpms > DRM_MODE_DPMS_ON) {
> - DRM_ERROR("failed framebuffer changing request.\n");
> - return -EPERM;
> - }
> -
> - ret = exynos_check_plane(crtc->primary, fb);
> - if (ret)
> - return ret;
> -
> - crtc_w = fb->width - x;
> - crtc_h = fb->height - y;
> - exynos_update_plane(crtc->primary, crtc, fb, 0, 0,
> - crtc_w, crtc_h, x, y, crtc_w, crtc_h);
> + if (WARN_ON(!crtc->state))
> + return;
>  
> - return 0;
> + if (exynos_crtc->ops->commit)
> + exynos_crtc->ops->commit(exynos_crtc);

This will be called again by crtc_funcs->commit from
drm_crtc_helper_set_mode. It seems to need to remove the call from
exynos_drm_crtc_commit like below.

diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c 
b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
index 48ccab7..aa981c2 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -63,9 +63,6 @@ static void exynos_drm_crtc_commit(struct drm_crtc *crtc)

if (exynos_crtc->ops->win_commit)
exynos_crtc->ops->win_commit(exynos_crtc, exynos_plane->zpos);
-
-   if (exynos_crtc->ops->commit)
-   exynos_crtc->ops->commit(exynos_crtc);
 }

 static bool

>  }
>  
>  static void exynos_drm_crtc_disable(struct drm_crtc *crtc)
> @@ -158,8 +115,9 @@ static struct drm_crtc_helper_funcs 
> exynos_crtc_helper_funcs = {
>   .prepare= exynos_drm_crtc_prepare,
>   .commit = exynos_drm_crtc_commit,
>   .mode_fixup = exynos_drm_crtc_mode_fixup,
> - .mode_set   = exynos_drm_crtc_mode_set,
> - .mode_set_base  = exynos_drm_crtc_mode_set_base,
> + .mode_set   = drm_helper_crtc_mode_set,
> + .mode_set_nofb  = exynos_drm_crtc_mode_set_nofb,
> + .mode_set_base  = drm_helper_crtc_mode_set_base,
>   .disable= exynos_drm_crtc_disable,
>  };
>  
> 

Thanks.

[PATCH v6 03/12] drm/exynos: atomic phase 1: add .mode_set_nofb() callback

[Gustavo Padovan]

Hi Joonyoung,

2015-05-22 Joonyoung Shim :

> On 05/22/2015 05:02 AM, Gustavo Padovan wrote:
> > From: Gustavo Padovan 
> > 
> > The new atomic infrastructure needs the .mode_set_nofb() callback to
> > update CRTC timings before setting any plane.
> > 
> > Signed-off-by: Gustavo Padovan 
> > ---
> >  drivers/gpu/drm/exynos/exynos_drm_crtc.c | 60 
> > +---
> >  1 file changed, 9 insertions(+), 51 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c 
> > b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> > index 61b8cfe..54b74e1 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> > @@ -81,59 +81,16 @@ exynos_drm_crtc_mode_fixup(struct drm_crtc *crtc,
> > return true;
> >  }
> >  
> > -static int
> > -exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode 
> > *mode,
> > - struct drm_display_mode *adjusted_mode, int x, int y,
> > - struct drm_framebuffer *old_fb)
> > -{
> > -   struct drm_framebuffer *fb = crtc->primary->fb;
> > -   unsigned int crtc_w;
> > -   unsigned int crtc_h;
> > -   int ret;
> > -
> > -   /*
> > -* copy the mode data adjusted by mode_fixup() into crtc->mode
> > -* so that hardware can be seet to proper mode.
> > -*/
> > -   memcpy(>mode, adjusted_mode, sizeof(*adjusted_mode));
> 
> This can cause any problem because exynos drm drivers use crtc->mode
> directly as adjusted_mode. It's necessary to consider using
> crtc_state->adjusted_mode in exynos drm drivers.

Yes, we are moving to use adjusted_mode in exynos, see this patch from
Tobias. It makes crtc uses the adjusted_mode instead. So I think it is
okay to remove this memcpy from here.

http://www.spinics.net/lists/linux-samsung-soc/msg44790.html

> 
> Please refer a patch of Daniel Stone using hwmode instead of mode but it
> cannot use for atomic.
> 
> http://lists.freedesktop.org/archives/dri-devel/2015-March/079546.html

> 
> > -
> > -   ret = exynos_check_plane(crtc->primary, fb);
> > -   if (ret < 0)
> > -   return ret;
> > -
> > -   crtc_w = fb->width - x;
> > -   crtc_h = fb->height - y;
> > -   exynos_plane_mode_set(crtc->primary, crtc, fb, 0, 0,
> > - crtc_w, crtc_h, x, y, crtc_w, crtc_h);
> > -
> > -   return 0;
> > -}
> > -
> > -static int exynos_drm_crtc_mode_set_base(struct drm_crtc *crtc, int x, int 
> > y,
> > - struct drm_framebuffer *old_fb)
> > +static void
> > +exynos_drm_crtc_mode_set_nofb(struct drm_crtc *crtc)
> >  {
> > struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
> > -   struct drm_framebuffer *fb = crtc->primary->fb;
> > -   unsigned int crtc_w;
> > -   unsigned int crtc_h;
> > -   int ret;
> >  
> > -   /* when framebuffer changing is requested, crtc's dpms should be on */
> > -   if (exynos_crtc->dpms > DRM_MODE_DPMS_ON) {
> > -   DRM_ERROR("failed framebuffer changing request.\n");
> > -   return -EPERM;
> > -   }
> > -
> > -   ret = exynos_check_plane(crtc->primary, fb);
> > -   if (ret)
> > -   return ret;
> > -
> > -   crtc_w = fb->width - x;
> > -   crtc_h = fb->height - y;
> > -   exynos_update_plane(crtc->primary, crtc, fb, 0, 0,
> > -   crtc_w, crtc_h, x, y, crtc_w, crtc_h);
> > +   if (WARN_ON(!crtc->state))
> > +   return;
> >  
> > -   return 0;
> > +   if (exynos_crtc->ops->commit)
> > +   exynos_crtc->ops->commit(exynos_crtc);
> 
> This will be called again by crtc_funcs->commit from
> drm_crtc_helper_set_mode. It seems to need to remove the call from
> exynos_drm_crtc_commit like below.
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c 
> b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> index 48ccab7..aa981c2 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> @@ -63,9 +63,6 @@ static void exynos_drm_crtc_commit(struct drm_crtc *crtc)
>  
>   if (exynos_crtc->ops->win_commit)
>   exynos_crtc->ops->win_commit(exynos_crtc, exynos_plane->zpos);
> -
> - if (exynos_crtc->ops->commit)
> - exynos_crtc->ops->commit(exynos_crtc);
>  }

Okay. Removed.

Gustavo

[PATCH v6 03/12] drm/exynos: atomic phase 1: add .mode_set_nofb() callback

[Gustavo Padovan]

From: Gustavo Padovan 

The new atomic infrastructure needs the .mode_set_nofb() callback to
update CRTC timings before setting any plane.

Signed-off-by: Gustavo Padovan 
---
 drivers/gpu/drm/exynos/exynos_drm_crtc.c | 60 +---
 1 file changed, 9 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c 
b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
index 61b8cfe..54b74e1 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -81,59 +81,16 @@ exynos_drm_crtc_mode_fixup(struct drm_crtc *crtc,
return true;
 }

-static int
-exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode,
- struct drm_display_mode *adjusted_mode, int x, int y,
- struct drm_framebuffer *old_fb)
-{
-   struct drm_framebuffer *fb = crtc->primary->fb;
-   unsigned int crtc_w;
-   unsigned int crtc_h;
-   int ret;
-
-   /*
-* copy the mode data adjusted by mode_fixup() into crtc->mode
-* so that hardware can be seet to proper mode.
-*/
-   memcpy(>mode, adjusted_mode, sizeof(*adjusted_mode));
-
-   ret = exynos_check_plane(crtc->primary, fb);
-   if (ret < 0)
-   return ret;
-
-   crtc_w = fb->width - x;
-   crtc_h = fb->height - y;
-   exynos_plane_mode_set(crtc->primary, crtc, fb, 0, 0,
- crtc_w, crtc_h, x, y, crtc_w, crtc_h);
-
-   return 0;
-}
-
-static int exynos_drm_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
- struct drm_framebuffer *old_fb)
+static void
+exynos_drm_crtc_mode_set_nofb(struct drm_crtc *crtc)
 {
struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
-   struct drm_framebuffer *fb = crtc->primary->fb;
-   unsigned int crtc_w;
-   unsigned int crtc_h;
-   int ret;

-   /* when framebuffer changing is requested, crtc's dpms should be on */
-   if (exynos_crtc->dpms > DRM_MODE_DPMS_ON) {
-   DRM_ERROR("failed framebuffer changing request.\n");
-   return -EPERM;
-   }
-
-   ret = exynos_check_plane(crtc->primary, fb);
-   if (ret)
-   return ret;
-
-   crtc_w = fb->width - x;
-   crtc_h = fb->height - y;
-   exynos_update_plane(crtc->primary, crtc, fb, 0, 0,
-   crtc_w, crtc_h, x, y, crtc_w, crtc_h);
+   if (WARN_ON(!crtc->state))
+   return;

-   return 0;
+   if (exynos_crtc->ops->commit)
+   exynos_crtc->ops->commit(exynos_crtc);
 }

 static void exynos_drm_crtc_disable(struct drm_crtc *crtc)
@@ -158,8 +115,9 @@ static struct drm_crtc_helper_funcs 
exynos_crtc_helper_funcs = {
.prepare= exynos_drm_crtc_prepare,
.commit = exynos_drm_crtc_commit,
.mode_fixup = exynos_drm_crtc_mode_fixup,
-   .mode_set   = exynos_drm_crtc_mode_set,
-   .mode_set_base  = exynos_drm_crtc_mode_set_base,
+   .mode_set   = drm_helper_crtc_mode_set,
+   .mode_set_nofb  = exynos_drm_crtc_mode_set_nofb,
+   .mode_set_base  = drm_helper_crtc_mode_set_base,
.disable= exynos_drm_crtc_disable,
 };

-- 
2.1.0