Re: [PATCH v6 7/9] drm/hisilicon/hibmc: Add connector for VDAC

[Rongrong Zou <zourongrong@huawei.com>]

在 2016/11/11 9:45, Sean Paul 写道:
> On Fri, Oct 28, 2016 at 3:28 AM, Rongrong Zou <zourongrong@gmail.com> wrote:
>> Add connector funcs and helper funcs for VDAC.
>>
>> Signed-off-by: Rongrong Zou <zourongrong@gmail.com>
>> ---
>>   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c  |  8 +++
>>   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h  |  2 +
>>   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c | 76 ++++++++++++++++++++++++
>>   3 files changed, 86 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> index ba191e1..4253603 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> @@ -131,6 +131,14 @@ static int hibmc_kms_init(struct hibmc_drm_device *hidev)
>>                  return ret;
>>          }
>>
>> +       ret = hibmc_connector_init(hidev);
>> +       if (ret) {
>> +               DRM_ERROR("failed to init connector\n");
>> +               return ret;
>> +       }
>> +
>> +       drm_mode_connector_attach_encoder(&hidev->connector,
>> +                                         &hidev->encoder);
>
> The connector should be initialized in the vdac driver with the
> encoder, not in the drv file (same as plane/crtc)

applied, thanks.

>
>>          return 0;
>>   }
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> index 401cea4..450247d 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> @@ -48,6 +48,7 @@ struct hibmc_drm_device {
>>          struct drm_plane plane;
>>          struct drm_crtc crtc;
>>          struct drm_encoder encoder;
>> +       struct drm_connector connector;
>
> No need to keep track here

will allocate with devm_kzalloc(), thanks.

>
>>          bool mode_config_initialized;
>>
>>          /* ttm */
>> @@ -89,6 +90,7 @@ static inline struct hibmc_bo *gem_to_hibmc_bo(struct drm_gem_object *gem)
>>   int hibmc_plane_init(struct hibmc_drm_device *hidev);
>>   int hibmc_crtc_init(struct hibmc_drm_device *hidev);
>>   int hibmc_encoder_init(struct hibmc_drm_device *hidev);
>> +int hibmc_connector_init(struct hibmc_drm_device *hidev);
>>   int hibmc_fbdev_init(struct hibmc_drm_device *hidev);
>>   void hibmc_fbdev_fini(struct hibmc_drm_device *hidev);
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
>> index 953f659..ebefcd1 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
>> @@ -87,3 +87,79 @@ int hibmc_encoder_init(struct hibmc_drm_device *hidev)
>>          drm_encoder_helper_add(encoder, &hibmc_encoder_helper_funcs);
>>          return 0;
>>   }
>> +
>> +static int hibmc_connector_get_modes(struct drm_connector *connector)
>> +{
>> +       int count;
>> +
>> +       count = drm_add_modes_noedid(connector, 800, 600);
>> +       drm_set_preferred_mode(connector, defx, defy);
>
> So you have defx/defy as module parameters, but then hardcode the
> 800x600 mode. If defx/defy is anything other than 800/600, this won't
> work. I think you should just remove the defx/defy module params and
> rely on userspace adding modes as appropriate.

will remove them, thanks.

>
>> +       return count;
>> +}
>> +
>> +static int hibmc_connector_mode_valid(struct drm_connector *connector,
>> +                                     struct drm_display_mode *mode)
>> +{
>> +       struct hibmc_drm_device *hiprivate =
>> +        container_of(connector, struct hibmc_drm_device, connector);
>> +       unsigned long size = mode->hdisplay * mode->vdisplay * 4;
>
> Why * 4 here and why * 2 below? You support formats less than 32 bpp,
> so the * 4 isn't necessarily correct for all formats. Is the * 2 to
> account for front & back buffer?
>

it is from bochs, the original comments below:
	/*
	 * Make sure we can fit two framebuffers into video memory.
	 * This allows up to 1600x1200 with 16 MB (default size).
	 * If you want more try this:
	 *     'qemu -vga std -global VGA.vgamem_mb=32 $otherargs'
	 */
i think it is not necessary for hibmc, so will remove it and return
MODE_OK, thanks.

>
>> +
>> +       if (size * 2 > hiprivate->fb_size)
>> +               return MODE_BAD;
>> +
>> +       return MODE_OK;
>> +}
>> +
>> +static struct drm_encoder *
>> +hibmc_connector_best_encoder(struct drm_connector *connector)
>> +{
>> +       int enc_id = connector->encoder_ids[0];
>> +
>> +       /* pick the encoder ids */
>> +       if (enc_id)
>> +               return drm_encoder_find(connector->dev, enc_id);
>
> Can't you just do return drm_encoder_find(connector->dev,
> connector->encoder_ids[0]); ?
>
> ie: won't drm_encoder_find do the right thing if you pass in id == 0?

hibmc_connector_best_encoder(struct drm_connector *connector)
{
	return drm_encoder_find(connector->dev, connector->encoder_ids[0]);
}
looks more simple, i like it, thanks:)

>
>> +
>> +       return NULL;
>> +}
>> +
>> +static enum drm_connector_status hibmc_connector_detect(struct drm_connector
>> +                                                *connector, bool force)
>> +{
>> +       return connector_status_connected;
>
> Perhaps this should be connector_status_unknown, since you don't
> necessarily know it's connected.

it is always connected in hibmc's scenario, usually it is used as a
management chip on server, we use KVM(keyboard, mouse, video) rather than
a directly connected monitor. I will modify if a phisical monitor
is connected later.

>
>> +}
>> +
>> +static const struct drm_connector_helper_funcs
>> +       hibmc_connector_connector_helper_funcs = {
>> +       .get_modes = hibmc_connector_get_modes,
>> +       .mode_valid = hibmc_connector_mode_valid,
>> +       .best_encoder = hibmc_connector_best_encoder,
>> +};
>> +
>> +static const struct drm_connector_funcs hibmc_connector_connector_funcs = {
>> +       .dpms = drm_atomic_helper_connector_dpms,
>> +       .detect = hibmc_connector_detect,
>> +       .fill_modes = drm_helper_probe_single_connector_modes,
>> +       .destroy = drm_connector_cleanup,
>> +       .reset = drm_atomic_helper_connector_reset,
>> +       .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
>> +       .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>> +};
>> +
>> +int hibmc_connector_init(struct hibmc_drm_device *hidev)
>> +{
>> +       struct drm_device *dev = hidev->dev;
>> +       struct drm_connector *connector = &hidev->connector;
>> +       int ret;
>> +
>> +       ret = drm_connector_init(dev, connector,
>> +                                &hibmc_connector_connector_funcs,
>> +                                DRM_MODE_CONNECTOR_VGA);
>> +       if (ret) {
>> +               DRM_ERROR("failed to init connector\n");
>> +               return ret;
>> +       }
>> +       drm_connector_helper_add(connector,
>> +                                &hibmc_connector_connector_helper_funcs);
>> +
>> +       return 0;
>> +}
>> --
>> 1.9.1
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> _______________________________________________
> linuxarm mailing list
> linuxarm@huawei.com
> http://rnd-openeuler.huawei.com/mailman/listinfo/linuxarm
>
> .
>


-- 
Regards, Rongrong

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel