[v4,09/16] drm: rockchip: add bpc and color mode setting
diff mbox

Message ID 1441087308-25455-1-git-send-email-ykk@rock-chips.com
State New
Headers show

Commit Message

Yakir Yang Sept. 1, 2015, 6:01 a.m. UTC
From: Mark Yao <yzq@rock-chips.com>

Add bpc and color mode setting in rockchip_drm_vop driver, so
connector could try to use the edid drm_display_info to config
vop output mode.

Signed-off-by: Mark Yao <yzq@rock-chips.com>
Signed-off-by: Yakir Yang <ykk@rock-chips.com>
---
Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 46 +++++++++++++++++++------
 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c     |  2 +-
 drivers/gpu/drm/rockchip/rockchip_drm_drv.h     |  2 +-
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c     | 33 ++++++++++++++++--
 4 files changed, 68 insertions(+), 15 deletions(-)

Comments

Heiko Stübner Sept. 1, 2015, 9 p.m. UTC | #1
Hi Yakir,

Am Dienstag, 1. September 2015, 14:01:48 schrieb Yakir Yang:
> From: Mark Yao <yzq@rock-chips.com>
> 
> Add bpc and color mode setting in rockchip_drm_vop driver, so
> connector could try to use the edid drm_display_info to config
> vop output mode.
> 
> Signed-off-by: Mark Yao <yzq@rock-chips.com>
> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
> ---
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
> 
>  drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 46
> +++++++++++++++++++------ drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c     |
>  2 +-
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.h     |  2 +-
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c     | 33 ++++++++++++++++--
>  4 files changed, 68 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c index cebff9e..efea045
> 100644
> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> @@ -11,11 +11,6 @@
>   * Free Software Foundation; either version 2 of the License, or (at your
>   * option) any later version.
>   */
> -#include <drm/drmP.h>
> -#include <drm/drm_crtc_helper.h>
> -#include <drm/drm_panel.h>
> -#include <drm/drm_of.h>
> -#include <drm/drm_dp_helper.h>
> 
>  #include <linux/component.h>
>  #include <linux/mfd/syscon.h>
> @@ -27,6 +22,13 @@
>  #include <video/of_videomode.h>
>  #include <video/videomode.h>
> 
> +#include <drm/drmP.h>
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_crtc_helper.h>
> +#include <drm/drm_panel.h>
> +#include <drm/drm_of.h>
> +#include <drm/drm_dp_helper.h>
> +
>  #include <drm/bridge/analogix_dp.h>
> 
>  #include "rockchip_drm_drv.h"
> @@ -125,20 +127,44 @@ static void rockchip_dp_drm_encoder_mode_set(struct
> drm_encoder *encoder, /* do nothing */
>  }
> 
> +static drm_connector *rockchip_dp_get_connector(struct rockchip_dp_device

missing a "struct" -> static struct drm_connector


> *dp) +{
> +	struct drm_connector *connector;
> +	struct drm_device *drm_dev = dp->drm_dev;
> +
> +	drm_for_each_connector(connector, drm_dev) {
> +		if (connector->encoder != &dp->encoder)
> +			return connector;
> +	}
> +
> +	return NULL;
> +}
> +
>  static void rockchip_dp_drm_encoder_prepare(struct drm_encoder *encoder)
>  {
>  	struct rockchip_dp_device *dp = encoder_to_dp(encoder);
> +	struct drm_connector *connector;
> +	int ret = 0;
>  	u32 val;
> -	int ret;
> 
> -	ret = rockchip_drm_crtc_mode_config(encoder->crtc,
> -					    DRM_MODE_CONNECTOR_eDP,
> -					    ROCKCHIP_OUT_MODE_AAAA);
> -	if (ret < 0) {
> +	connector = rockchip_dp_get_connector(dp);
> +	if (!connector) {
> +		DRM_ERROR("Failed to get connector by encoder[%p]\n", encoder);
> +		return;
> +	}
> +
> +	if (connector->display_info.color_formats | DRM_COLOR_FORMAT_RGB444)
> +		ret = rockchip_drm_crtc_mode_config(
> +			encoder->crtc, DRM_MODE_CONNECTOR_eDP,
> +			connector->display_info.bpc, DRM_COLOR_FORMAT_RGB444);
> +	if (!ret) {
>  		dev_err(dp->dev, "Could not set crtc mode config: %d.\n", ret);
>  		return;
>  	}
> 
> +	connector->display_info.bpc = ret;
> +	connector->display_info.color_formats = DRM_COLOR_FORMAT_RGB444;
> +
>  	ret = rockchip_drm_encoder_get_mux_id(dp->dev->of_node, encoder);
>  	if (ret < 0)
>  		return;
> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c index 80d6fc8..428a3c1 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> @@ -215,7 +215,7 @@ static void dw_hdmi_rockchip_encoder_commit(struct
> drm_encoder *encoder) static void dw_hdmi_rockchip_encoder_prepare(struct
> drm_encoder *encoder) {
>  	rockchip_drm_crtc_mode_config(encoder->crtc, DRM_MODE_CONNECTOR_HDMIA,
> -				      ROCKCHIP_OUT_MODE_AAAA);
> +				      10, DRM_COLOR_FORMAT_RGB444);
>  }
> 
>  static struct drm_encoder_helper_funcs
> dw_hdmi_rockchip_encoder_helper_funcs = { diff --git
> a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h index dc4e5f0..ef1d7fb 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> @@ -59,7 +59,7 @@ void rockchip_unregister_crtc_funcs(struct drm_device
> *dev, int pipe); int rockchip_drm_encoder_get_mux_id(struct device_node
> *node,
>  				    struct drm_encoder *encoder);
>  int rockchip_drm_crtc_mode_config(struct drm_crtc *crtc, int
> connector_type, -				  int out_mode);
> +				  int bpc, int color);
>  int rockchip_drm_dma_attach_device(struct drm_device *drm_dev,
>  				   struct device *dev);
>  void rockchip_drm_dma_detach_device(struct drm_device *drm_dev,
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index 34b78e7..5d7f9b6 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -811,14 +811,41 @@ static const struct drm_plane_funcs vop_plane_funcs =
> {
> 
>  int rockchip_drm_crtc_mode_config(struct drm_crtc *crtc,
>  				  int connector_type,
> -				  int out_mode)
> +				  int bpc, int color)
>  {
>  	struct vop *vop = to_vop(crtc);
> -
>  	vop->connector_type = connector_type;
>  	vop->connector_out_mode = out_mode;

this line should probably go away, as the source var "out_mode" does not exist 
in the function params any more, making the compilation break, and is set 
below anyway.


Heiko

> 
> -	return 0;
> +	/*
> +	 * RK3288 vop only support RGB Color output.
> +	 */
> +	if (color != DRM_COLOR_FORMAT_RGB444) {
> +		DRM_ERROR("Only support output RGB444, not support%d\n",
> +			  color);
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Fixme: I don't know how to describe the ROCKCHIP_OUT_MODE_P565's
> +	 * bpc, 5 or 6?
> +	 *
> +	 */
> +	if (bpc >= 10) {
> +		bpc = 10;
> +		vop->connector_out_mode = ROCKCHIP_OUT_MODE_AAAA;
> +	} else if (bpc >= 8) {
> +		bpc = 8;
> +		vop->connector_out_mode = ROCKCHIP_OUT_MODE_P888;
> +	} else if (bpc >= 6) {
> +		bpc = 6;
> +		vop->connector_out_mode = ROCKCHIP_OUT_MODE_P888;
> +	} else {
> +		DRM_ERROR("unsupport bpc %d\n", bpc);
> +		return -EINVAL;
> +	}
> +
> +	return bpc;
>  }
>  EXPORT_SYMBOL_GPL(rockchip_drm_crtc_mode_config);
Yakir Yang Sept. 2, 2015, 2:06 a.m. UTC | #2
Hi Heiko,

? 09/02/2015 05:00 AM, Heiko Stuebner ??:
> Hi Yakir,
>
> Am Dienstag, 1. September 2015, 14:01:48 schrieb Yakir Yang:
>> From: Mark Yao <yzq@rock-chips.com>
>>
>> Add bpc and color mode setting in rockchip_drm_vop driver, so
>> connector could try to use the edid drm_display_info to config
>> vop output mode.
>>
>> Signed-off-by: Mark Yao <yzq@rock-chips.com>
>> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
>> ---
>> Changes in v4: None
>> Changes in v3: None
>> Changes in v2: None
>>
>>   drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 46
>> +++++++++++++++++++------ drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c     |
>>   2 +-
>>   drivers/gpu/drm/rockchip/rockchip_drm_drv.h     |  2 +-
>>   drivers/gpu/drm/rockchip/rockchip_drm_vop.c     | 33 ++++++++++++++++--
>>   4 files changed, 68 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
>> b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c index cebff9e..efea045
>> 100644
>> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
>> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
>> @@ -11,11 +11,6 @@
>>    * Free Software Foundation; either version 2 of the License, or (at your
>>    * option) any later version.
>>    */
>> -#include <drm/drmP.h>
>> -#include <drm/drm_crtc_helper.h>
>> -#include <drm/drm_panel.h>
>> -#include <drm/drm_of.h>
>> -#include <drm/drm_dp_helper.h>
>>
>>   #include <linux/component.h>
>>   #include <linux/mfd/syscon.h>
>> @@ -27,6 +22,13 @@
>>   #include <video/of_videomode.h>
>>   #include <video/videomode.h>
>>
>> +#include <drm/drmP.h>
>> +#include <drm/drm_crtc.h>
>> +#include <drm/drm_crtc_helper.h>
>> +#include <drm/drm_panel.h>
>> +#include <drm/drm_of.h>
>> +#include <drm/drm_dp_helper.h>
>> +
>>   #include <drm/bridge/analogix_dp.h>
>>
>>   #include "rockchip_drm_drv.h"
>> @@ -125,20 +127,44 @@ static void rockchip_dp_drm_encoder_mode_set(struct
>> drm_encoder *encoder, /* do nothing */
>>   }
>>
>> +static drm_connector *rockchip_dp_get_connector(struct rockchip_dp_device
> missing a "struct" -> static struct drm_connector

What the hell mistake I make in v5  :-(

Thanks a lot, I would fix this as soon as possible

>
>> *dp) +{
>> +	struct drm_connector *connector;
>> +	struct drm_device *drm_dev = dp->drm_dev;
>> +
>> +	drm_for_each_connector(connector, drm_dev) {
>> +		if (connector->encoder != &dp->encoder)
>> +			return connector;
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>>   static void rockchip_dp_drm_encoder_prepare(struct drm_encoder *encoder)
>>   {
>>   	struct rockchip_dp_device *dp = encoder_to_dp(encoder);
>> +	struct drm_connector *connector;
>> +	int ret = 0;
>>   	u32 val;
>> -	int ret;
>>
>> -	ret = rockchip_drm_crtc_mode_config(encoder->crtc,
>> -					    DRM_MODE_CONNECTOR_eDP,
>> -					    ROCKCHIP_OUT_MODE_AAAA);
>> -	if (ret < 0) {
>> +	connector = rockchip_dp_get_connector(dp);
>> +	if (!connector) {
>> +		DRM_ERROR("Failed to get connector by encoder[%p]\n", encoder);
>> +		return;
>> +	}
>> +
>> +	if (connector->display_info.color_formats | DRM_COLOR_FORMAT_RGB444)
>> +		ret = rockchip_drm_crtc_mode_config(
>> +			encoder->crtc, DRM_MODE_CONNECTOR_eDP,
>> +			connector->display_info.bpc, DRM_COLOR_FORMAT_RGB444);
>> +	if (!ret) {
>>   		dev_err(dp->dev, "Could not set crtc mode config: %d.\n", ret);
>>   		return;
>>   	}
>>
>> +	connector->display_info.bpc = ret;
>> +	connector->display_info.color_formats = DRM_COLOR_FORMAT_RGB444;
>> +
>>   	ret = rockchip_drm_encoder_get_mux_id(dp->dev->of_node, encoder);
>>   	if (ret < 0)
>>   		return;
>> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>> b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c index 80d6fc8..428a3c1 100644
>> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>> @@ -215,7 +215,7 @@ static void dw_hdmi_rockchip_encoder_commit(struct
>> drm_encoder *encoder) static void dw_hdmi_rockchip_encoder_prepare(struct
>> drm_encoder *encoder) {
>>   	rockchip_drm_crtc_mode_config(encoder->crtc, DRM_MODE_CONNECTOR_HDMIA,
>> -				      ROCKCHIP_OUT_MODE_AAAA);
>> +				      10, DRM_COLOR_FORMAT_RGB444);
>>   }
>>
>>   static struct drm_encoder_helper_funcs
>> dw_hdmi_rockchip_encoder_helper_funcs = { diff --git
>> a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
>> b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h index dc4e5f0..ef1d7fb 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
>> @@ -59,7 +59,7 @@ void rockchip_unregister_crtc_funcs(struct drm_device
>> *dev, int pipe); int rockchip_drm_encoder_get_mux_id(struct device_node
>> *node,
>>   				    struct drm_encoder *encoder);
>>   int rockchip_drm_crtc_mode_config(struct drm_crtc *crtc, int
>> connector_type, -				  int out_mode);
>> +				  int bpc, int color);
>>   int rockchip_drm_dma_attach_device(struct drm_device *drm_dev,
>>   				   struct device *dev);
>>   void rockchip_drm_dma_detach_device(struct drm_device *drm_dev,
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index 34b78e7..5d7f9b6 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> @@ -811,14 +811,41 @@ static const struct drm_plane_funcs vop_plane_funcs =
>> {
>>
>>   int rockchip_drm_crtc_mode_config(struct drm_crtc *crtc,
>>   				  int connector_type,
>> -				  int out_mode)
>> +				  int bpc, int color)
>>   {
>>   	struct vop *vop = to_vop(crtc);
>> -
>>   	vop->connector_type = connector_type;
>>   	vop->connector_out_mode = out_mode;
> this line should probably go away, as the source var "out_mode" does not exist
> in the function params any more, making the compilation break, and is set
> below anyway.

Sorry for the failed, there are must be a problem when I backport those code
to chrome-3.14 to verify.

Thanks a lot, I would update next version soon.
- Yakir

>
> Heiko
>
>> -	return 0;
>> +	/*
>> +	 * RK3288 vop only support RGB Color output.
>> +	 */
>> +	if (color != DRM_COLOR_FORMAT_RGB444) {
>> +		DRM_ERROR("Only support output RGB444, not support%d\n",
>> +			  color);
>> +		return -EINVAL;
>> +	}
>> +
>> +	/*
>> +	 * Fixme: I don't know how to describe the ROCKCHIP_OUT_MODE_P565's
>> +	 * bpc, 5 or 6?
>> +	 *
>> +	 */
>> +	if (bpc >= 10) {
>> +		bpc = 10;
>> +		vop->connector_out_mode = ROCKCHIP_OUT_MODE_AAAA;
>> +	} else if (bpc >= 8) {
>> +		bpc = 8;
>> +		vop->connector_out_mode = ROCKCHIP_OUT_MODE_P888;
>> +	} else if (bpc >= 6) {
>> +		bpc = 6;
>> +		vop->connector_out_mode = ROCKCHIP_OUT_MODE_P888;
>> +	} else {
>> +		DRM_ERROR("unsupport bpc %d\n", bpc);
>> +		return -EINVAL;
>> +	}
>> +
>> +	return bpc;
>>   }
>>   EXPORT_SYMBOL_GPL(rockchip_drm_crtc_mode_config);
>
>
>
Thierry Reding Sept. 2, 2015, 8:34 a.m. UTC | #3
On Wed, Sep 02, 2015 at 10:06:36AM +0800, Yakir Yang wrote:
> ? 09/02/2015 05:00 AM, Heiko Stuebner ??:
> >Am Dienstag, 1. September 2015, 14:01:48 schrieb Yakir Yang:
[...]
> >>diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> >>b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index 34b78e7..5d7f9b6 100644
> >>--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> >>+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> >>@@ -811,14 +811,41 @@ static const struct drm_plane_funcs vop_plane_funcs =
> >>{
> >>
> >>  int rockchip_drm_crtc_mode_config(struct drm_crtc *crtc,
> >>  				  int connector_type,
> >>-				  int out_mode)
> >>+				  int bpc, int color)
> >>  {
> >>  	struct vop *vop = to_vop(crtc);
> >>-
> >>  	vop->connector_type = connector_type;
> >>  	vop->connector_out_mode = out_mode;
> >this line should probably go away, as the source var "out_mode" does not exist
> >in the function params any more, making the compilation break, and is set
> >below anyway.
> 
> Sorry for the failed, there are must be a problem when I backport those code
> to chrome-3.14 to verify.
> 
> Thanks a lot, I would update next version soon.

*sigh*

I get the feeling that you're going about upstreaming the wrong way. If
you post patches to upstream mailing lists and you expect people to
apply those patches, then your target is the upstream kernel. So you
should be basing all of your work on top of a recent release candidate
directly from Linus or some recent version of linux-next.

Your current approach is already making people waste time trying to
build the patches and fail because you've been testing on something
other than mainline or linux-next.

At the very least your code must compile when applied against a recent
upstream tree. I would also expect you to make sure the code works at
runtime, though, contrary to build testing, not everybody will be able
to verify that you've actually done so. It is ultimately your platform
maintainer's (i.e. Heiko's) responsibility to ensure that because they
will get to deal with user complaints if people can't run an upstream
kernel on the devices.

I realize that the upstream kernel isn't what's going to end up in
products later on, but that doesn't change the fact that you're
submitting code for inclusion in the mainline tree. If you need to
backport code to some ChromeOS tree, then that should be done after
you've verified that things build and run upstream. Doing so makes life
a lot easier for your upstream maintainers, and that in turn makes it
more likely to get your code merged.

Thierry
Yakir Yang Sept. 2, 2015, 10:02 a.m. UTC | #4
Thierry,

? 2015/9/2 16:34, Thierry Reding ??:
> On Wed, Sep 02, 2015 at 10:06:36AM +0800, Yakir Yang wrote:
>> ? 09/02/2015 05:00 AM, Heiko Stuebner ??:
>>> Am Dienstag, 1. September 2015, 14:01:48 schrieb Yakir Yang:
> [...]
>>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>> b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index 34b78e7..5d7f9b6 100644
>>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>> @@ -811,14 +811,41 @@ static const struct drm_plane_funcs vop_plane_funcs =
>>>> {
>>>>
>>>>   int rockchip_drm_crtc_mode_config(struct drm_crtc *crtc,
>>>>   				  int connector_type,
>>>> -				  int out_mode)
>>>> +				  int bpc, int color)
>>>>   {
>>>>   	struct vop *vop = to_vop(crtc);
>>>> -
>>>>   	vop->connector_type = connector_type;
>>>>   	vop->connector_out_mode = out_mode;
>>> this line should probably go away, as the source var "out_mode" does not exist
>>> in the function params any more, making the compilation break, and is set
>>> below anyway.
>> Sorry for the failed, there are must be a problem when I backport those code
>> to chrome-3.14 to verify.
>>
>> Thanks a lot, I would update next version soon.
> *sigh*
>
> I get the feeling that you're going about upstreaming the wrong way. If
> you post patches to upstream mailing lists and you expect people to
> apply those patches, then your target is the upstream kernel. So you
> should be basing all of your work on top of a recent release candidate
> directly from Linus or some recent version of linux-next.

Yeah, do feel I am in the wrong way now. I used to write my patch on
linux-next branch, then backport some head file to productor kernel,
so I can check compiled failed. After that, I backport the dp driver code
to normal productor kernel, start to debug the function.

So I used to ensure no compiled failed on upstrean kernel, actually it's
also hard to ensure, cause I just backport head file. Not even debug the
function on upstream kernel.

> Your current approach is already making people waste time trying to
> build the patches and fail because you've been testing on something
> other than mainline or linux-next.
>
> At the very least your code must compile when applied against a recent
> upstream tree. I would also expect you to make sure the code works at
> runtime, though, contrary to build testing, not everybody will be able
> to verify that you've actually done so. It is ultimately your platform
> maintainer's (i.e. Heiko's) responsibility to ensure that because they
> will get to deal with user complaints if people can't run an upstream
> kernel on the devices.

Oh, first time to know this rule. So I should work on Heiko's github
kernel branch at the first time to start send upstream.

> I realize that the upstream kernel isn't what's going to end up in
> products later on, but that doesn't change the fact that you're
> submitting code for inclusion in the mainline tree. If you need to
> backport code to some ChromeOS tree, then that should be done after
> you've verified that things build and run upstream. Doing so makes life
> a lot easier for your upstream maintainers, and that in turn makes it
> more likely to get your code merged.

Feel free now, I would correct those in bellow version. Thanks
for your remind (or maybe complain :-D ).

- Yakir
> Thierry
Thierry Reding Sept. 3, 2015, 8:38 a.m. UTC | #5
On Wed, Sep 02, 2015 at 06:02:25PM +0800, Yakir Yang wrote:
> ? 2015/9/2 16:34, Thierry Reding ??:
[...]
> >At the very least your code must compile when applied against a recent
> >upstream tree. I would also expect you to make sure the code works at
> >runtime, though, contrary to build testing, not everybody will be able
> >to verify that you've actually done so. It is ultimately your platform
> >maintainer's (i.e. Heiko's) responsibility to ensure that because they
> >will get to deal with user complaints if people can't run an upstream
> >kernel on the devices.
> 
> Oh, first time to know this rule. So I should work on Heiko's github
> kernel branch at the first time to start send upstream.

It's usually not necessary to rebase on a specific platform tree. Most
platform trees should feed into linux-next anyway, so linux-next would
be the appropriate base in almost all cases.

Note, though, that that's only true if you expect somebody else to merge
your code. The reason is that whoever will end up applying your patches
will likely apply to a tree that feeds into linux-next, and that way you
both end up having roughly the same base.

On the other hand if you are a maintainer yourself you should be keeping
a branch based on the latest -rc1. That's especially important if your
tree feeds into linux-next, because basing on linux-next will break very
horribly that way.

So for this particular case I would expect either Mark or Inki to apply
these patches when they're ready. Their trees should be based on the
latest -rc1. At least the Exynos DRM tree feeds into linux-next, so you
should be fine if you use linux-next as a base.

Mark, have you ever considered having your tree added to linux-next?

I'm beginning to think that we need to make that a requirement for all
DRM drivers so that we can resolve integration issues early on rather
than Dave having to deal with them when he pulls code in.

Thierry
Yakir Yang Sept. 6, 2015, 2:06 a.m. UTC | #6
Hi Thierry,

? 09/03/2015 04:38 PM, Thierry Reding ??:
> On Wed, Sep 02, 2015 at 06:02:25PM +0800, Yakir Yang wrote:
>> ? 2015/9/2 16:34, Thierry Reding ??:
> [...]
>>> At the very least your code must compile when applied against a recent
>>> upstream tree. I would also expect you to make sure the code works at
>>> runtime, though, contrary to build testing, not everybody will be able
>>> to verify that you've actually done so. It is ultimately your platform
>>> maintainer's (i.e. Heiko's) responsibility to ensure that because they
>>> will get to deal with user complaints if people can't run an upstream
>>> kernel on the devices.
>> Oh, first time to know this rule. So I should work on Heiko's github
>> kernel branch at the first time to start send upstream.
> It's usually not necessary to rebase on a specific platform tree. Most
> platform trees should feed into linux-next anyway, so linux-next would
> be the appropriate base in almost all cases.
>
> Note, though, that that's only true if you expect somebody else to merge
> your code. The reason is that whoever will end up applying your patches
> will likely apply to a tree that feeds into linux-next, and that way you
> both end up having roughly the same base.
>
> On the other hand if you are a maintainer yourself you should be keeping
> a branch based on the latest -rc1. That's especially important if your
> tree feeds into linux-next, because basing on linux-next will break very
> horribly that way.
>
> So for this particular case I would expect either Mark or Inki to apply
> these patches when they're ready. Their trees should be based on the
> latest -rc1. At least the Exynos DRM tree feeds into linux-next, so you
> should be fine if you use linux-next as a base.

Glad to know this, thanks,
- Yakir

>
> Mark, have you ever considered having your tree added to linux-next?
>
> I'm beginning to think that we need to make that a requirement for all
> DRM drivers so that we can resolve integration issues early on rather
> than Dave having to deal with them when he pulls code in.
>
> Thierry

Patch
diff mbox

diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index cebff9e..efea045 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -11,11 +11,6 @@ 
  * Free Software Foundation; either version 2 of the License, or (at your
  * option) any later version.
  */
-#include <drm/drmP.h>
-#include <drm/drm_crtc_helper.h>
-#include <drm/drm_panel.h>
-#include <drm/drm_of.h>
-#include <drm/drm_dp_helper.h>
 
 #include <linux/component.h>
 #include <linux/mfd/syscon.h>
@@ -27,6 +22,13 @@ 
 #include <video/of_videomode.h>
 #include <video/videomode.h>
 
+#include <drm/drmP.h>
+#include <drm/drm_crtc.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_panel.h>
+#include <drm/drm_of.h>
+#include <drm/drm_dp_helper.h>
+
 #include <drm/bridge/analogix_dp.h>
 
 #include "rockchip_drm_drv.h"
@@ -125,20 +127,44 @@  static void rockchip_dp_drm_encoder_mode_set(struct drm_encoder *encoder,
 	/* do nothing */
 }
 
+static drm_connector *rockchip_dp_get_connector(struct rockchip_dp_device *dp)
+{
+	struct drm_connector *connector;
+	struct drm_device *drm_dev = dp->drm_dev;
+
+	drm_for_each_connector(connector, drm_dev) {
+		if (connector->encoder != &dp->encoder)
+			return connector;
+	}
+
+	return NULL;
+}
+
 static void rockchip_dp_drm_encoder_prepare(struct drm_encoder *encoder)
 {
 	struct rockchip_dp_device *dp = encoder_to_dp(encoder);
+	struct drm_connector *connector;
+	int ret = 0;
 	u32 val;
-	int ret;
 
-	ret = rockchip_drm_crtc_mode_config(encoder->crtc,
-					    DRM_MODE_CONNECTOR_eDP,
-					    ROCKCHIP_OUT_MODE_AAAA);
-	if (ret < 0) {
+	connector = rockchip_dp_get_connector(dp);
+	if (!connector) {
+		DRM_ERROR("Failed to get connector by encoder[%p]\n", encoder);
+		return;
+	}
+
+	if (connector->display_info.color_formats | DRM_COLOR_FORMAT_RGB444)
+		ret = rockchip_drm_crtc_mode_config(
+			encoder->crtc, DRM_MODE_CONNECTOR_eDP,
+			connector->display_info.bpc, DRM_COLOR_FORMAT_RGB444);
+	if (!ret) {
 		dev_err(dp->dev, "Could not set crtc mode config: %d.\n", ret);
 		return;
 	}
 
+	connector->display_info.bpc = ret;
+	connector->display_info.color_formats = DRM_COLOR_FORMAT_RGB444;
+
 	ret = rockchip_drm_encoder_get_mux_id(dp->dev->of_node, encoder);
 	if (ret < 0)
 		return;
diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
index 80d6fc8..428a3c1 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
@@ -215,7 +215,7 @@  static void dw_hdmi_rockchip_encoder_commit(struct drm_encoder *encoder)
 static void dw_hdmi_rockchip_encoder_prepare(struct drm_encoder *encoder)
 {
 	rockchip_drm_crtc_mode_config(encoder->crtc, DRM_MODE_CONNECTOR_HDMIA,
-				      ROCKCHIP_OUT_MODE_AAAA);
+				      10, DRM_COLOR_FORMAT_RGB444);
 }
 
 static struct drm_encoder_helper_funcs dw_hdmi_rockchip_encoder_helper_funcs = {
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
index dc4e5f0..ef1d7fb 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
@@ -59,7 +59,7 @@  void rockchip_unregister_crtc_funcs(struct drm_device *dev, int pipe);
 int rockchip_drm_encoder_get_mux_id(struct device_node *node,
 				    struct drm_encoder *encoder);
 int rockchip_drm_crtc_mode_config(struct drm_crtc *crtc, int connector_type,
-				  int out_mode);
+				  int bpc, int color);
 int rockchip_drm_dma_attach_device(struct drm_device *drm_dev,
 				   struct device *dev);
 void rockchip_drm_dma_detach_device(struct drm_device *drm_dev,
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 34b78e7..5d7f9b6 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -811,14 +811,41 @@  static const struct drm_plane_funcs vop_plane_funcs = {
 
 int rockchip_drm_crtc_mode_config(struct drm_crtc *crtc,
 				  int connector_type,
-				  int out_mode)
+				  int bpc, int color)
 {
 	struct vop *vop = to_vop(crtc);
-
 	vop->connector_type = connector_type;
 	vop->connector_out_mode = out_mode;
 
-	return 0;
+	/*
+	 * RK3288 vop only support RGB Color output.
+	 */
+	if (color != DRM_COLOR_FORMAT_RGB444) {
+		DRM_ERROR("Only support output RGB444, not support%d\n",
+			  color);
+		return -EINVAL;
+	}
+
+	/*
+	 * Fixme: I don't know how to describe the ROCKCHIP_OUT_MODE_P565's
+	 * bpc, 5 or 6?
+	 *
+	 */
+	if (bpc >= 10) {
+		bpc = 10;
+		vop->connector_out_mode = ROCKCHIP_OUT_MODE_AAAA;
+	} else if (bpc >= 8) {
+		bpc = 8;
+		vop->connector_out_mode = ROCKCHIP_OUT_MODE_P888;
+	} else if (bpc >= 6) {
+		bpc = 6;
+		vop->connector_out_mode = ROCKCHIP_OUT_MODE_P888;
+	} else {
+		DRM_ERROR("unsupport bpc %d\n", bpc);
+		return -EINVAL;
+	}
+
+	return bpc;
 }
 EXPORT_SYMBOL_GPL(rockchip_drm_crtc_mode_config);