Re: [Intel-gfx] [PATCH 2/4] drm/plane-helper: Add drm_primary_helper_check_update() (v2)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, May 15, 2014 at 10:51 PM, Lee, Chon Ming
<chon.ming.lee@xxxxxxxxx> wrote:
> On 04/30 10:07, Matt Roper wrote:
>> Pull the parameter checking from drm_primary_helper_update() out into
>> its own function; drivers that provide their own setplane()
>> implementations rather than using the helper may still want to share
>> this parameter checking logic.
>>
>> A few of the checks here were also updated based on suggestions by
>> Ville Syrjälä.
>>
>> v2:
>>   - Pass src/dest/clip rects and min/max scaling down to helper to avoid
>>     duplication of effort between helper and drivers (suggested by
>>     Ville).
>>   - Allow caller to specify whether the primary plane should be
>>     updatable while the crtc is disabled.
>>
>> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
>> Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
>> ---
>>  drivers/gpu/drm/drm_plane_helper.c | 123 ++++++++++++++++++++++++++++---------
>>  include/drm/drm_plane_helper.h     |  24 +++++++-
>>  2 files changed, 116 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
>> index b601233..11e8b82 100644
>> --- a/drivers/gpu/drm/drm_plane_helper.c
>> +++ b/drivers/gpu/drm/drm_plane_helper.c
>> @@ -26,6 +26,7 @@
>>  #include <linux/list.h>
>>  #include <drm/drmP.h>
>>  #include <drm/drm_rect.h>
>> +#include <drm/drm_plane_helper.h>
>>
>>  #define SUBPIXEL_MASK 0xffff
>>
>> @@ -66,6 +67,77 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc,
>>  }
>>
>>  /**
>> + * drm_primary_helper_check_update() - Check primary plane update for validity
>> + * @plane: plane object to update
>> + * @crtc: owning CRTC of owning plane
>> + * @fb: framebuffer to flip onto plane
>> + * @src: source coordinates in 16.16 fixed point
>> + * @dest: integer destination coordinates
>> + * @clip: integer clipping coordinates
>> + * @min_scale: minimum @src:@dest scaling factor in 16.16 fixed point
>> + * @max_scale: maximum @src:@dest scaling factor in 16.16 fixed point
>> + * @can_position: is it legal to position the primary plane such that it
>> + *                doesn't cover the entire crtc?
>> + * @can_update_disabled: can the primary plane be updated while the crtc
>> + *                       is disabled?
>> + * @visible: output parameter indicating whether plane is still visible after
>> + *           clipping
>> + *
>> + * Checks that a desired primary plane update is valid.  Drivers that provide
>> + * their own primary plane handling may still wish to call this function to
>> + * avoid duplication of error checking code.
>> + *
>> + * RETURNS:
>> + * Zero if update appears valid, error code on failure
>> + */
>> +int drm_primary_helper_check_update(struct drm_plane *plane,
>> +                                 struct drm_crtc *crtc,
>> +                                 struct drm_framebuffer *fb,
>> +                                 struct drm_rect *src,
>> +                                 struct drm_rect *dest,
>> +                                 const struct drm_rect *clip,
>> +                                 int min_scale,
>> +                                 int max_scale,
>> +                                 bool can_position,
>> +                                 bool can_update_disabled,
>> +                                 bool *visible)
>> +{
>> +     int hscale, vscale;
>> +
>> +     if (!crtc->enabled && !can_update_disabled) {
>> +             DRM_DEBUG_KMS("Cannot update primary plane of a disabled CRTC.\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     /* Check scaling */
>> +     hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale);
>> +     vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale);
>> +     if (hscale < 0 || vscale < 0) {
>> +             DRM_DEBUG_KMS("Invalid scaling of primary plane\n");
>> +             return -ERANGE;
>> +     }
>> +
>> +     *visible = drm_rect_clip_scaled(src, dest, clip, hscale, vscale);
>> +     if (!visible)
>> +             /*
>> +              * Primary plane isn't visible; some drivers can handle this
>> +              * so we just return success here.  Drivers that can't
>> +              * (including those that use the primary plane helper's
>> +              * update function) will return an error from their
>> +              * update_plane handler.
>> +              */
>> +             return 0;
>> +
>> +     if (!can_position && !drm_rect_equals(dest, clip)) {
>> +             DRM_DEBUG_KMS("Primary plane must cover entire CRTC\n");
>> +             return -EINVAL;
>> +     }
>
> Cherryview display allow the primary plane to be position at any location
> similiar to sprite plane for certain port. So, this shouldn't need to check here.
>
> And the width/height doesn't need to cover the whole screen.

In that case, wouldn't it make sense (at least when you want to expose
those features) to *not* use primary plane helpers for that hw?

IMHO, the primary plane helpers should be for "traditional" crtcs
which do not have these features..

BR,
-R

>
>> +
>> +     return 0;
>> +}
>> +EXPORT_SYMBOL(drm_primary_helper_check_update);
>> +
>> +/**
>>   * drm_primary_helper_update() - Helper for primary plane update
>>   * @plane: plane object to update
>>   * @crtc: owning CRTC of owning plane
>> @@ -113,51 +185,42 @@ int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,
>>               .x = src_x >> 16,
>>               .y = src_y >> 16,
>>       };
>> +     struct drm_rect src = {
>> +             .x1 = src_x,
>> +             .y1 = src_y,
>> +             .x2 = src_x + src_w,
>> +             .y2 = src_y + src_h,
>> +     };
>>       struct drm_rect dest = {
>>               .x1 = crtc_x,
>>               .y1 = crtc_y,
>>               .x2 = crtc_x + crtc_w,
>>               .y2 = crtc_y + crtc_h,
>>       };
>> -     struct drm_rect clip = {
>> +     const struct drm_rect clip = {
>>               .x2 = crtc->mode.hdisplay,
>>               .y2 = crtc->mode.vdisplay,
>>       };
>>       struct drm_connector **connector_list;
>>       int num_connectors, ret;
>> +     bool visible;
>>
>> -     if (!crtc->enabled) {
>> -             DRM_DEBUG_KMS("Cannot update primary plane of a disabled CRTC.\n");
>> -             return -EINVAL;
>> -     }
>> -
>> -     /* Disallow subpixel positioning */
>> -     if ((src_x | src_y | src_w | src_h) & SUBPIXEL_MASK) {
>> -             DRM_DEBUG_KMS("Primary plane does not support subpixel positioning\n");
>> -             return -EINVAL;
>> -     }
>> -
>> -     /* Disallow scaling */
>> -     src_w >>= 16;
>> -     src_h >>= 16;
>> -     if (crtc_w != src_w || crtc_h != src_h) {
>> -             DRM_DEBUG_KMS("Can't scale primary plane\n");
>> -             return -EINVAL;
>> -     }
>> -
>> -     /* Make sure primary plane covers entire CRTC */
>> -     drm_rect_intersect(&dest, &clip);
>> -     if (dest.x1 != 0 || dest.y1 != 0 ||
>> -         dest.x2 != crtc->mode.hdisplay || dest.y2 != crtc->mode.vdisplay) {
>> -             DRM_DEBUG_KMS("Primary plane must cover entire CRTC\n");
>> -             return -EINVAL;
>> -     }
>> -
>> -     /* Framebuffer must be big enough to cover entire plane */
>> -     ret = drm_crtc_check_viewport(crtc, crtc_x, crtc_y, &crtc->mode, fb);
>> +     ret = drm_primary_helper_check_update(plane, crtc, fb,
>> +                                           &src, &dest, &clip,
>> +                                           DRM_PLANE_HELPER_NO_SCALING,
>> +                                           DRM_PLANE_HELPER_NO_SCALING,
>> +                                           false, false, &visible);
>>       if (ret)
>>               return ret;
>>
>> +     if (!visible)
>> +             /*
>> +              * Primary plane isn't visible.  Note that unless a driver
>> +              * provides their own disable function, this will just
>> +              * wind up returning -EINVAL to userspace.
>> +              */
>> +             return plane->funcs->disable_plane(plane);
>> +
>>       /* Find current connectors for CRTC */
>>       num_connectors = get_connectors_for_crtc(crtc, NULL, 0);
>>       BUG_ON(num_connectors == 0);
>> diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
>> index 09824be..05e1357 100644
>> --- a/include/drm/drm_plane_helper.h
>> +++ b/include/drm/drm_plane_helper.h
>> @@ -24,6 +24,17 @@
>>  #ifndef DRM_PLANE_HELPER_H
>>  #define DRM_PLANE_HELPER_H
>>
>> +#include <drm/drm_rect.h>
>> +
>> +/*
>> + * Drivers that don't allow primary plane scaling may pass this macro in place
>> + * of the min/max scale parameters of the update checker function.
>> + *
>> + * Due to src being in 16.16 fixed point and dest being in integer pixels,
>> + * 1<<16 represents no scaling.
>> + */
>> +#define DRM_PLANE_HELPER_NO_SCALING (1<<16)
>> +
>>  /**
>>   * DOC: plane helpers
>>   *
>> @@ -31,6 +42,17 @@
>>   * planes.
>>   */
>>
>> +extern int drm_primary_helper_check_update(struct drm_plane *plane,
>> +                                        struct drm_crtc *crtc,
>> +                                        struct drm_framebuffer *fb,
>> +                                        struct drm_rect *src,
>> +                                        struct drm_rect *dest,
>> +                                        const struct drm_rect *clip,
>> +                                        int min_scale,
>> +                                        int max_scale,
>> +                                        bool can_position,
>> +                                        bool can_update_disabled,
>> +                                        bool *visible);
>>  extern int drm_primary_helper_update(struct drm_plane *plane,
>>                                    struct drm_crtc *crtc,
>>                                    struct drm_framebuffer *fb,
>> @@ -42,7 +64,7 @@ extern int drm_primary_helper_disable(struct drm_plane *plane);
>>  extern void drm_primary_helper_destroy(struct drm_plane *plane);
>>  extern const struct drm_plane_funcs drm_primary_helper_funcs;
>>  extern struct drm_plane *drm_primary_helper_create_plane(struct drm_device *dev,
>> -                                                      uint32_t *formats,
>> +                                                      const uint32_t *formats,
>>                                                        int num_formats);
>>
>>
>> --
>> 1.8.5.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel





[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux