Re: [PATCH v2 3/4] drm/mgag200: Use simple encoder

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1.1: Type: text/plain, Size: 4987 bytes --]

Hi Sam

thanks for reviewing the patch set.

Am 20.02.20 um 19:56 schrieb Sam Ravnborg:
> Hi Thomas.
> 
> On Tue, Feb 18, 2020 at 09:48:14AM +0100, Thomas Zimmermann wrote:
>> The mgag200 driver uses an empty implementation for its encoder. Replace
>> the code with the generic simple encoder.
>>
>> v2:
>> 	* rebase onto new simple-encoder interface
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>  drivers/gpu/drm/mgag200/mgag200_drv.h  |  7 ---
>>  drivers/gpu/drm/mgag200/mgag200_mode.c | 61 ++------------------------
>>  2 files changed, 3 insertions(+), 65 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
>> index aa32aad222c2..9bb9e8e14539 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
>> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
>> @@ -95,7 +95,6 @@
>>  #define MATROX_DPMS_CLEARED (-1)
>>  
>>  #define to_mga_crtc(x) container_of(x, struct mga_crtc, base)
>> -#define to_mga_encoder(x) container_of(x, struct mga_encoder, base)
>>  #define to_mga_connector(x) container_of(x, struct mga_connector, base)
>>  
>>  struct mga_crtc {
>> @@ -110,12 +109,6 @@ struct mga_mode_info {
>>  	struct mga_crtc *crtc;
>>  };
>>  
>> -struct mga_encoder {
>> -	struct drm_encoder base;
>> -	int last_dpms;
>> -};
>> -
>> -
>>  struct mga_i2c_chan {
>>  	struct i2c_adapter adapter;
>>  	struct drm_device *dev;
> 
> Any particular reason why the drm_encoder is not embedded in struct
> mga_device?
> 
> I found it more elegant - like you did it for ast in the previous patch.

I think I wanted something that uses drm_simple_encoder_create(). But I
can change that. The embedded variant is indeed better.

Best regards
Thomas

> 
> I also noted there is one more unused "last_dpms" - but it is outside
> the scope of this patch to remove it.
> 
> 	Sam
> 
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> index 62a8e9ccb16d..957ea1057b6c 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> @@ -15,6 +15,7 @@
>>  #include <drm/drm_fourcc.h>
>>  #include <drm/drm_plane_helper.h>
>>  #include <drm/drm_probe_helper.h>
>> +#include <drm/drm_simple_kms_helper.h>
>>  
>>  #include "mgag200_drv.h"
>>  
>> @@ -1449,72 +1450,16 @@ static void mga_crtc_init(struct mga_device *mdev)
>>  	drm_crtc_helper_add(&mga_crtc->base, &mga_helper_funcs);
>>  }
>>  
>> -/*
>> - * The encoder comes after the CRTC in the output pipeline, but before
>> - * the connector. It's responsible for ensuring that the digital
>> - * stream is appropriately converted into the output format. Setup is
>> - * very simple in this case - all we have to do is inform qemu of the
>> - * colour depth in order to ensure that it displays appropriately
>> - */
>> -
>> -/*
>> - * These functions are analagous to those in the CRTC code, but are intended
>> - * to handle any encoder-specific limitations
>> - */
>> -static void mga_encoder_mode_set(struct drm_encoder *encoder,
>> -				struct drm_display_mode *mode,
>> -				struct drm_display_mode *adjusted_mode)
>> -{
>> -
>> -}
>> -
>> -static void mga_encoder_dpms(struct drm_encoder *encoder, int state)
>> -{
>> -	return;
>> -}
>> -
>> -static void mga_encoder_prepare(struct drm_encoder *encoder)
>> -{
>> -}
>> -
>> -static void mga_encoder_commit(struct drm_encoder *encoder)
>> -{
>> -}
>> -
>> -static void mga_encoder_destroy(struct drm_encoder *encoder)
>> -{
>> -	struct mga_encoder *mga_encoder = to_mga_encoder(encoder);
>> -	drm_encoder_cleanup(encoder);
>> -	kfree(mga_encoder);
>> -}
>> -
>> -static const struct drm_encoder_helper_funcs mga_encoder_helper_funcs = {
>> -	.dpms = mga_encoder_dpms,
>> -	.mode_set = mga_encoder_mode_set,
>> -	.prepare = mga_encoder_prepare,
>> -	.commit = mga_encoder_commit,
>> -};
>> -
>> -static const struct drm_encoder_funcs mga_encoder_encoder_funcs = {
>> -	.destroy = mga_encoder_destroy,
>> -};
>> -
>>  static struct drm_encoder *mga_encoder_init(struct drm_device *dev)
>>  {
>>  	struct drm_encoder *encoder;
>> -	struct mga_encoder *mga_encoder;
>>  
>> -	mga_encoder = kzalloc(sizeof(struct mga_encoder), GFP_KERNEL);
>> -	if (!mga_encoder)
>> +	encoder = drm_simple_encoder_create(dev, DRM_MODE_ENCODER_DAC);
>> +	if (IS_ERR(encoder))
>>  		return NULL;
>>  
>> -	encoder = &mga_encoder->base;
>>  	encoder->possible_crtcs = 0x1;
>>  
>> -	drm_encoder_init(dev, encoder, &mga_encoder_encoder_funcs,
>> -			 DRM_MODE_ENCODER_DAC, NULL);
>> -	drm_encoder_helper_add(encoder, &mga_encoder_helper_funcs);
>> -
>>  	return encoder;
>>  }
>>  
>> -- 
>> 2.25.0

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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