diff mbox series

[v2,4/9] drm/udl: Inline DPMS code into CRTC enable and disable functions

Message ID 20191206124713.5748-5-tzimmermann@suse.de (mailing list archive)
State New, archived
Headers show
Series drm/udl: Convert to simple-pipe helpers and clean up | expand

Commit Message

Thomas Zimmermann Dec. 6, 2019, 12:47 p.m. UTC 
DPMS functionality is only used by the CRTC's enable and disable
functions. Inline the code.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/udl/udl_modeset.c | 84 +++++++++++--------------------
 1 file changed, 30 insertions(+), 54 deletions(-)

Comments

Emil Velikov Dec. 9, 2019, 2:35 p.m. UTC | #1 
On Fri, 6 Dec 2019 at 12:47, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> DPMS functionality is only used by the CRTC's enable and disable
> functions. Inline the code.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/udl/udl_modeset.c | 84 +++++++++++--------------------
>  1 file changed, 30 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> index 72884cbda131..4681446c2323 100644
> --- a/drivers/gpu/drm/udl/udl_modeset.c
> +++ b/drivers/gpu/drm/udl/udl_modeset.c
> @@ -48,25 +48,9 @@ static char *udl_vidreg_unlock(char *buf)
>   *  0x01 H and V sync off (screen blank but powered)
>   *  0x07 DPMS powerdown (requires modeset to come back)
>   */
> -static char *udl_set_blank(char *buf, int dpms_mode)
> +static char *udl_set_blank_mode(char *buf, u8 mode)
>  {
> -       u8 reg;
> -       switch (dpms_mode) {
> -       case DRM_MODE_DPMS_OFF:
> -               reg = 0x07;
> -               break;
> -       case DRM_MODE_DPMS_STANDBY:
> -               reg = 0x05;
> -               break;
> -       case DRM_MODE_DPMS_SUSPEND:
> -               reg = 0x01;
> -               break;
> -       case DRM_MODE_DPMS_ON:
> -               reg = 0x00;
> -               break;
> -       }
> -
As a follow-up, please add/use symbolic names for the the four states.
Apart from the cosmetic aspect, this allows us to trivially change
from DPMS_OFF to DPMS_SUSPEND or DPMS_STANDBY at any random point.

As-is the series is:
Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>

Thanks
Emil
Thomas Zimmermann Dec. 10, 2019, 8:16 a.m. UTC | #2 
Hi

Am 09.12.19 um 15:35 schrieb Emil Velikov:
> On Fri, 6 Dec 2019 at 12:47, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>
>> DPMS functionality is only used by the CRTC's enable and disable
>> functions. Inline the code.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>  drivers/gpu/drm/udl/udl_modeset.c | 84 +++++++++++--------------------
>>  1 file changed, 30 insertions(+), 54 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
>> index 72884cbda131..4681446c2323 100644
>> --- a/drivers/gpu/drm/udl/udl_modeset.c
>> +++ b/drivers/gpu/drm/udl/udl_modeset.c
>> @@ -48,25 +48,9 @@ static char *udl_vidreg_unlock(char *buf)
>>   *  0x01 H and V sync off (screen blank but powered)
>>   *  0x07 DPMS powerdown (requires modeset to come back)
>>   */
>> -static char *udl_set_blank(char *buf, int dpms_mode)
>> +static char *udl_set_blank_mode(char *buf, u8 mode)
>>  {
>> -       u8 reg;
>> -       switch (dpms_mode) {
>> -       case DRM_MODE_DPMS_OFF:
>> -               reg = 0x07;
>> -               break;
>> -       case DRM_MODE_DPMS_STANDBY:
>> -               reg = 0x05;
>> -               break;
>> -       case DRM_MODE_DPMS_SUSPEND:
>> -               reg = 0x01;
>> -               break;
>> -       case DRM_MODE_DPMS_ON:
>> -               reg = 0x00;
>> -               break;
>> -       }
>> -
> As a follow-up, please add/use symbolic names for the the four states.
> Apart from the cosmetic aspect, this allows us to trivially change
> from DPMS_OFF to DPMS_SUSPEND or DPMS_STANDBY at any random point.
> 
> As-is the series is:
> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>

Thanks. The symbolic names are trivial to add here. I'll put them into
v3 of this patchset.

Best regards
Thomas

> 
> Thanks
> Emil
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index 72884cbda131..4681446c2323 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -48,25 +48,9 @@  static char *udl_vidreg_unlock(char *buf)
  *  0x01 H and V sync off (screen blank but powered)
  *  0x07 DPMS powerdown (requires modeset to come back)
  */
-static char *udl_set_blank(char *buf, int dpms_mode)
+static char *udl_set_blank_mode(char *buf, u8 mode)
 {
-	u8 reg;
-	switch (dpms_mode) {
-	case DRM_MODE_DPMS_OFF:
-		reg = 0x07;
-		break;
-	case DRM_MODE_DPMS_STANDBY:
-		reg = 0x05;
-		break;
-	case DRM_MODE_DPMS_SUSPEND:
-		reg = 0x01;
-		break;
-	case DRM_MODE_DPMS_ON:
-		reg = 0x00;
-		break;
-	}
-
-	return udl_set_register(buf, 0x1f, reg);
+	return udl_set_register(buf, 0x1f, mode);
 }
 
 static char *udl_set_color_depth(char *buf, u8 selection)
@@ -237,6 +221,11 @@  static int udl_crtc_write_mode_to_hw(struct drm_crtc *crtc)
 	char *buf;
 	int retval;
 
+	if (udl->mode_buf_len == 0) {
+		DRM_ERROR("No mode set\n");
+		return -EINVAL;
+	}
+
 	urb = udl_get_urb(dev);
 	if (!urb)
 		return -ENOMEM;
@@ -249,38 +238,6 @@  static int udl_crtc_write_mode_to_hw(struct drm_crtc *crtc)
 	return retval;
 }
 
-
-static void udl_crtc_dpms(struct drm_crtc *crtc, int mode)
-{
-	struct drm_device *dev = crtc->dev;
-	struct udl_device *udl = dev->dev_private;
-	int retval;
-
-	if (mode == DRM_MODE_DPMS_OFF) {
-		char *buf;
-		struct urb *urb;
-		urb = udl_get_urb(dev);
-		if (!urb)
-			return;
-
-		buf = (char *)urb->transfer_buffer;
-		buf = udl_vidreg_lock(buf);
-		buf = udl_set_blank(buf, mode);
-		buf = udl_vidreg_unlock(buf);
-
-		buf = udl_dummy_render(buf);
-		retval = udl_submit_urb(dev, urb, buf - (char *)
-					urb->transfer_buffer);
-	} else {
-		if (udl->mode_buf_len == 0) {
-			DRM_ERROR("Trying to enable DPMS with no mode\n");
-			return;
-		}
-		udl_crtc_write_mode_to_hw(crtc);
-	}
-
-}
-
 /*
  * Simple display pipeline
  */
@@ -311,6 +268,8 @@  udl_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
 	char *wrptr;
 	int color_depth = UDL_COLOR_DEPTH_16BPP;
 
+	crtc_state->no_vblank = true;
+
 	udl->crtc = crtc;
 
 	buf = (char *)udl->mode_buf;
@@ -327,7 +286,7 @@  udl_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
 	wrptr = udl_set_base8bpp(wrptr, 2 * mode->vdisplay * mode->hdisplay);
 
 	wrptr = udl_set_vid_cmds(wrptr, mode);
-	wrptr = udl_set_blank(wrptr, DRM_MODE_DPMS_ON);
+	wrptr = udl_set_blank_mode(wrptr, 0x00);
 	wrptr = udl_vidreg_unlock(wrptr);
 
 	wrptr = udl_dummy_render(wrptr);
@@ -339,15 +298,32 @@  udl_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
 
 	udl_handle_damage(fb, 0, 0, fb->width, fb->height);
 
-	udl_crtc_dpms(&pipe->crtc, DRM_MODE_DPMS_ON);
+	if (!crtc_state->mode_changed)
+		return;
 
-	crtc_state->no_vblank = true;
+	/* enable display */
+	udl_crtc_write_mode_to_hw(crtc);
 }
 
 static void
 udl_simple_display_pipe_disable(struct drm_simple_display_pipe *pipe)
 {
-	udl_crtc_dpms(&pipe->crtc, DRM_MODE_DPMS_OFF);
+	struct drm_crtc *crtc= &pipe->crtc;
+	struct drm_device *dev = crtc->dev;
+	struct urb *urb;
+	char *buf;
+
+	urb = udl_get_urb(dev);
+	if (!urb)
+		return;
+
+	buf = (char *)urb->transfer_buffer;
+	buf = udl_vidreg_lock(buf);
+	buf = udl_set_blank_mode(buf, 0x07);
+	buf = udl_vidreg_unlock(buf);
+	buf = udl_dummy_render(buf);
+
+	udl_submit_urb(dev, urb, buf - (char *)urb->transfer_buffer);
 }
 
 static int