[PATCH 1/4] DRM: in-kernel cursor update on the top of mode setting.

[Jesse Barnes]

On Monday, January 5, 2009 12:58 pm Tiago Vignatti wrote:

> +
> +int drmCursorSetDev(int fd, char *name)
> +{
> +    struct drm_mode_cursor_setdev arg;
> +
> +    strncpy(arg.name, name, strlen(name));
> +    arg.name[strlen(name)] = '\0';

arg.name is on 32 bytes wide, right?  So shouldn't the lat arg of strncpy be 
the size of arg.name rather than the passed in value?  (I think this applies 
to the ioctl arg parsing as well.)


> +/* XXX: define a sane max */
> +#define MAX_INPUT_DEVICES 32
> +
> +char *cursor_devices[MAX_INPUT_DEVICES];

You could probably just make this a list instead, included in the drm 
mode_config struct perhaps.

> +int cursorReady = 0;
> +
> +struct drm_cursor {
> +    struct drm_crtc *crtc;
> +    int dx, dy;
> +    int hotx, hoty;
> +};
> +struct drm_cursor *cursor = NULL;
> +
> +
> +int drm_cursor_init(struct drm_crtc *crtc)
> +{
> +    int i, ret = 0;
> +
> +    DRM_DEBUG("\n");
> +
> +    cursor = kzalloc(sizeof(struct drm_cursor), GFP_KERNEL);
> +    if (!cursor)
> +        return -ENOMEM;
> +
> +    for (i = 0; i < MAX_INPUT_DEVICES; i++)
> +        cursor_devices[i] = NULL;
> +
> +    cursor->crtc = crtc;
> +    cursor->dx = crtc->fb->width / 2;
> +    cursor->dy = crtc->fb->height / 2;
> +    cursor->hotx = 0;
> +    cursor->hoty = 0;
> +
> +    cursorReady = 1;

Seems like the cursor should be part of the mode config structure instead?  
And maybe you could just use mode_config->cursor to see whether one was 
active instead of having a separate cursorReady flag...

> +    /* Fake a movement to update the cursor's position */
> +    if (crtc->funcs->cursor_move) {
> +        ret = crtc->funcs->cursor_move(crtc, cursor->dx - cursor->hotx,
> +                                       cursor->dy - cursor->hoty);
> +    } else {
> +        DRM_ERROR("crtc does not support cursor shorcut\n");
> +        ret = -EFAULT;
> +        goto out;
> +    }

This should probably just be -EINVAL or -ENOTTY to indicate that the low level 
driver doesn't support this feature.  Then you wouldn't need the error 
message either.

> +struct drm_mode_cursor_setdev {
> +    char name[32];
> +};
> +

I wonder if this should be just a dev_t instead...

> +struct drm_mode_cursor_hotspot {
> +    int hotx;
> +    int hoty;
> +};
> +
>  /**
>   * \name Ioctls Definitions
>   */
> @@ -1356,6 +1365,8 @@ struct drm_mode_crtc_lut {
>  #define DRM_IOCTL_MODE_GETENCODER      DRM_IOWR(0xB0, struct
> drm_mode_get_encoder) #define DRM_IOCTL_MODE_GETGAMMA        DRM_IOWR(0xB1,
> struct drm_mode_crtc_lut) #define DRM_IOCTL_MODE_SETGAMMA       
> DRM_IOWR(0xB2, struct drm_mode_crtc_lut) +#define
> DRM_IOCTL_MODE_CURSORSETDEV    DRM_IOWR(0xB3, struct
> drm_mode_cursor_setdev) +#define DRM_IOCTL_MODE_CURSORHOTSPOT  
> DRM_IOWR(0xB4, struct drm_mode_cursor_hotspot)

Combined with the other cursor ioctl this may be sufficient to support even sw 
rendered cursors (if we ever decided to do that), though we'd probably want 
to add another flag to the existing cursor ioctl to allow for control of 
accel methods (which would be another thing to add).

It would be cool if you could update this patchset against the upstream kernel 
and post it to lkml; the input guys may have some feedback too.
-- 
Jesse Barnes, Intel Open Source Technology Center