On Thu, Oct 31, 2013 at 8:19 PM, Tomasz Figa <tomasz.figa@xxxxxxxxx> wrote:
> Hi Sean,
>
> On Tuesday 29 of October 2013 12:12:52 Sean Paul wrote:
>> This patch changes the manager ops callbacks from accepting the subdrv
>> device pointer to taking a pointer to the manager. This will allow us
>> to move closer to decoupling manager/display from subdrv, and
>> subsequently decoupling the crtc/plane from the encoder.
>
> The idea of changing callbacks argument itself is fine for me, but I
> wonder if by the way we couldn't refactor the code in a way that would
> allow type checking of context structures. This would make the code a bit
> less error-prone (or maybe I'm just a bit too paranoid...).
>
> Anyway, please see my remaining comments inline.
>
>> Signed-off-by: Sean Paul <seanpaul@xxxxxxxxxxxx>
>> ---
>>
>> Changes in v2:
>> - Instead of passing context, pass manager
>> - Properly assign ctx->dev in fimd driver
>> Changes in v3:
>> - Added vidi implementation
>>
>> drivers/gpu/drm/exynos/exynos_drm_connector.c | 2 +-
>> drivers/gpu/drm/exynos/exynos_drm_drv.h | 35 ++++----
>> drivers/gpu/drm/exynos/exynos_drm_encoder.c | 27 +++---
>> drivers/gpu/drm/exynos/exynos_drm_fimd.c | 114
>> ++++++++++++++------------ drivers/gpu/drm/exynos/exynos_drm_hdmi.c
>> | 72 ++++++++-------- drivers/gpu/drm/exynos/exynos_drm_vidi.c |
>> 83 ++++++++++--------- 6 files changed, 180 insertions(+), 153
>> deletions(-)
> [snip]
>> @@ -182,16 +184,16 @@ static struct exynos_drm_display_ops
>> fimd_display_ops = { .power_on = fimd_display_power_on,
>> };
>>
>> -static void fimd_win_mode_set(struct device *dev,
>> - struct exynos_drm_overlay *overlay)
>> +static void fimd_win_mode_set(struct exynos_drm_manager *mgr,
>> + struct exynos_drm_overlay *overlay)
>> {
>> - struct fimd_context *ctx = get_fimd_context(dev);
>> + struct fimd_context *ctx = mgr->ctx;
>> struct fimd_win_data *win_data;
>> int win;
>> unsigned long offset;
>>
>> if (!overlay) {
>> - dev_err(dev, "overlay is NULL\n");
>> + DRM_ERROR("overlay is NULL\n");
>
> This change does not seem to be related to $subject.
>
It is. fimd_win_mode_set does not take dev as an argument any longer,
as such it's undefined.
>> return;
>> }
>>
>> @@ -231,9 +233,8 @@ static void fimd_win_mode_set(struct device *dev,
>> overlay->fb_width, overlay->crtc_width);
>> }
>>
>> -static void fimd_win_set_pixfmt(struct device *dev, unsigned int win)
>> +static void fimd_win_set_pixfmt(struct fimd_context *ctx, unsigned int
>> win) {
>
> Again not really related to $subject. Maybe this should be done in a
> preparatory patch preceeding this one? (+ same comment for several
> identical changes below)
>
I think it's directly related to the subject. We no longer pass dev as
an argument, so that has indirect effects on other functions.
>> - struct fimd_context *ctx = get_fimd_context(dev);
>> struct fimd_win_data *win_data = &ctx->win_data[win];
>> unsigned long val;
>>
> [snip]
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
>> b/drivers/gpu/drm/exynos/exynos_drm_hdmi.c index aebcc0e..ca0a87f
>> 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
>> @@ -129,11 +129,9 @@ static struct edid *drm_hdmi_get_edid(struct device
>> *dev,
>>
>> return NULL;
>> }
>> -
>> -static int drm_hdmi_check_mode(struct device *dev,
>> +static int drm_hdmi_check_mode_ctx(struct drm_hdmi_context *ctx,
>> struct drm_display_mode *mode)
>> {
>> - struct drm_hdmi_context *ctx = to_context(dev);
>> int ret = 0;
>>
>> /*
>> @@ -153,6 +151,14 @@ static int drm_hdmi_check_mode(struct device *dev,
>> return 0;
>> }
>>
>> +static int drm_hdmi_check_mode(struct device *dev,
>> + struct drm_display_mode *mode)
>> +{
>> + struct drm_hdmi_context *ctx = to_context(dev);
>> +
>> + return drm_hdmi_check_mode_ctx(ctx, mode);
>> +}
>> +
>
> nit: I don't think such wrapper is necessary.
>
> It seems to be easy enough to get from dev to ctx, so depending on the
> amount of user of drm_hdmi_check_mode() it might be better to simply
> change them to pass ctx instead of dev.
>
This is a display_op that is defined to accept dev. It's changed later
in the patchset to accept display, at which point the wrapper goes
away.
> [snip]
>> @@ -403,19 +404,23 @@ static void vidi_subdrv_remove(struct drm_device
>> *drm_dev, struct device *dev) /* TODO. */
>> }
>>
>> -static int vidi_power_on(struct vidi_context *ctx, bool enable)
>> +static int vidi_power_on(struct exynos_drm_manager *mgr, bool enable)
>> {
>> - struct exynos_drm_subdrv *subdrv = &ctx->subdrv;
>> - struct device *dev = subdrv->dev;
>> + struct vidi_context *ctx = mgr->ctx;
>> +
>> + DRM_DEBUG_KMS("%s\n", __FILE__);
>> +
>> + if (enable != false && enable != true)
>> + return -EINVAL;
>
> Huh? What value would you expect a bool to have if not false or true?
>
> Anyway, this shouldn't really matter, as the check bellow assumes that
> anything non-zero is true.
>
This is pre-existing vidi code, I just moved it.
Sean
>>
>> if (enable) {
>> ctx->suspended = false;
>>
>
> Just for clarity, this is the check I mentioned above.
>
> Best regards,
> Tomasz
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
